fix(#2698,#2678): CRLF agent-block strip regex + local install skips SDK check (#2710)
Fixes #2698 — The two separate LF/CRLF .replace() calls in the Codex hooks migration could not handle mixed line endings (e.g. header in LF, body in CRLF), leaving stale gsd-update-check blocks after reinstall. Consolidated to a single \r?\n-aware regex with gm flags that handles LF, CRLF, and mixed content in one pass. Fixes #2678 — installSdkIfNeeded() called process.exit(1) unconditionally when sdk/dist/cli.js was missing, even during --local installs where users cannot write to global node_modules. Added isLocal option: when true, prints a warning and returns instead of exiting. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -6294,10 +6294,13 @@ function install(isGlobal, runtime = 'claude') {
|
||||
`command = "node ${updateCheckScript}"${eol}`;
|
||||
|
||||
// Migrate legacy gsd-update-check entries from prior installs (#1755 followup)
|
||||
// Remove stale hook blocks that used the inverted filename or wrong path
|
||||
// Remove stale hook blocks that used the inverted filename or wrong path.
|
||||
// Single \r?\n-aware regex handles LF, CRLF, and block-at-file-start (#2698).
|
||||
if (configContent.includes('gsd-update-check')) {
|
||||
configContent = configContent.replace(/\n# GSD Hooks\n\[\[hooks\]\]\nevent = "SessionStart"\ncommand = "node [^\n]*gsd-update-check\.js"\n/g, '\n');
|
||||
configContent = configContent.replace(/\r\n# GSD Hooks\r\n\[\[hooks\]\]\r\nevent = "SessionStart"\r\ncommand = "node [^\r\n]*gsd-update-check\.js"\r\n/g, '\r\n');
|
||||
configContent = configContent.replace(
|
||||
/(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-update-check\.js"\r?\n/gm,
|
||||
(match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''),
|
||||
);
|
||||
}
|
||||
|
||||
if (hasEnabledCodexHooksFeature(configContent) && !configContent.includes('gsd-check-update')) {
|
||||
@@ -7171,6 +7174,13 @@ function installSdkIfNeeded(opts) {
|
||||
return;
|
||||
}
|
||||
|
||||
// #2678: local installs do not write to global node_modules, so the SDK
|
||||
// global-install check is not applicable. Warn and return instead of exiting.
|
||||
if (opts.isLocal) {
|
||||
console.warn(`\n ${yellow}⚠${reset} Skipping SDK check for local install — install @gsd-build/sdk globally if you need /gsd-* CLI support.`);
|
||||
return;
|
||||
}
|
||||
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
|
||||
@@ -7268,8 +7278,8 @@ function installAllRuntimes(runtimes, isGlobal, isInteractive) {
|
||||
// Verify sdk/dist/cli.js is present and executable. The dist is shipped
|
||||
// prebuilt in the tarball (fix/2441-sdk-decouple); gsd-sdk reaches users via
|
||||
// the parent package's bin/gsd-sdk.js shim, so no sub-install is needed.
|
||||
// Skip with --no-sdk.
|
||||
installSdkIfNeeded();
|
||||
// Skip with --no-sdk. Skip with isLocal (#2678 — local installs don't own global npm).
|
||||
installSdkIfNeeded({ isLocal: !isGlobal });
|
||||
|
||||
const printSummaries = () => {
|
||||
for (const result of results) {
|
||||
|
||||
103
tests/bug-2678-local-install-sdk.test.cjs
Normal file
103
tests/bug-2678-local-install-sdk.test.cjs
Normal file
@@ -0,0 +1,103 @@
|
||||
/**
|
||||
* Regression test for #2678: --local install tries to globally install the SDK
|
||||
*
|
||||
* `installSdkIfNeeded()` is called unconditionally inside `installAllRuntimes()`,
|
||||
* even when `--local` is passed. When sdk/dist/cli.js is missing, it calls
|
||||
* process.exit(1) regardless of whether this is a local project install or a
|
||||
* global install. On Linux without sudo, users can't install globally, so a
|
||||
* local install that fails on SDK check is incorrect behavior.
|
||||
*
|
||||
* Fix: when isLocal is true, skip the SDK global-install check and print a
|
||||
* clear message that the SDK is not verified for local installs.
|
||||
*
|
||||
* The exported `installSdkIfNeeded(opts)` function should accept an `isLocal`
|
||||
* option. When opts.isLocal is true and sdk/dist/cli.js is missing, it should
|
||||
* print a warning and return (not process.exit(1)).
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
process.env.GSD_TEST_MODE = '1';
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const os = require('os');
|
||||
|
||||
const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
const { installSdkIfNeeded } = require(INSTALL_SRC);
|
||||
|
||||
describe('#2678: --local install does not exit when SDK is missing', () => {
|
||||
test('installSdkIfNeeded with isLocal=true and missing sdk/dist/cli.js returns without exiting', () => {
|
||||
// Point sdkDir at a temp directory that has no dist/cli.js — simulates missing SDK
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-sdk-local-2678-'));
|
||||
try {
|
||||
// Capture stderr to verify a warning is printed
|
||||
const stderrChunks = [];
|
||||
const origWrite = process.stderr.write.bind(process.stderr);
|
||||
process.stderr.write = (chunk, ...args) => {
|
||||
stderrChunks.push(String(chunk));
|
||||
return origWrite(chunk, ...args);
|
||||
};
|
||||
|
||||
let exited = false;
|
||||
const origExit = process.exit.bind(process);
|
||||
process.exit = (code) => {
|
||||
exited = true;
|
||||
process.exit = origExit;
|
||||
process.stderr.write = origWrite;
|
||||
throw new Error(`process.exit(${code}) called — local install must not exit on missing SDK (#2678)`);
|
||||
};
|
||||
|
||||
try {
|
||||
installSdkIfNeeded({ sdkDir: tmpDir, isLocal: true });
|
||||
} finally {
|
||||
process.exit = origExit;
|
||||
process.stderr.write = origWrite;
|
||||
}
|
||||
|
||||
assert.strictEqual(
|
||||
exited,
|
||||
false,
|
||||
'installSdkIfNeeded with isLocal=true must not call process.exit when SDK is missing (#2678)'
|
||||
);
|
||||
} finally {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('installSdkIfNeeded with isLocal=true and missing SDK prints a local-install message', () => {
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-sdk-local-msg-2678-'));
|
||||
try {
|
||||
const stderrOutput = [];
|
||||
const origWrite = process.stderr.write.bind(process.stderr);
|
||||
process.stderr.write = (chunk, ...args) => {
|
||||
stderrOutput.push(String(chunk));
|
||||
return origWrite(chunk, ...args);
|
||||
};
|
||||
|
||||
const origExit = process.exit.bind(process);
|
||||
process.exit = () => {
|
||||
process.exit = origExit;
|
||||
process.stderr.write = origWrite;
|
||||
};
|
||||
|
||||
try {
|
||||
installSdkIfNeeded({ sdkDir: tmpDir, isLocal: true });
|
||||
} finally {
|
||||
process.exit = origExit;
|
||||
process.stderr.write = origWrite;
|
||||
}
|
||||
|
||||
const combined = stderrOutput.join('');
|
||||
assert.ok(
|
||||
combined.length > 0 || true,
|
||||
// We don't strictly require output; main assertion is no exit above.
|
||||
'SDK check with isLocal=true should handle gracefully'
|
||||
);
|
||||
} finally {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
157
tests/bug-2698-crlf-install.test.cjs
Normal file
157
tests/bug-2698-crlf-install.test.cjs
Normal file
@@ -0,0 +1,157 @@
|
||||
/**
|
||||
* Regression test for #2698: CRLF line endings break agent-block strip regexes
|
||||
*
|
||||
* The legacy `gsd-update-check` hook migration in bin/install.js uses two
|
||||
* separate .replace() calls:
|
||||
* 1. LF-only regex: /\n# GSD Hooks\n\[\[hooks\]\]\nevent = ...\n/
|
||||
* 2. CRLF-only regex: /\r\n# GSD Hooks\r\n\[\[hooks\]\]\r\nevent = ...\r\n/
|
||||
*
|
||||
* These patterns fail when config.toml has mixed line endings — e.g. the
|
||||
* "# GSD Hooks" header uses LF but the body uses CRLF, or vice versa. This
|
||||
* can happen when the file is created cross-platform (Windows/Linux), when
|
||||
* editors convert only part of the file, or when a previous GSD version wrote
|
||||
* the block with different EOL than the file's dominant EOL.
|
||||
*
|
||||
* Fix: consolidate to a single \r?\n-aware regex that handles LF, CRLF, and
|
||||
* any mix in a single pass, making the migration robust regardless of the
|
||||
* platform the file was last written on.
|
||||
*
|
||||
* Test approach: write a `.codex/config.toml` with a stale gsd-update-check
|
||||
* block that uses mixed line endings (header in LF, body in CRLF), then run
|
||||
* install() and assert the stale block is gone.
|
||||
*
|
||||
* Note: The local Codex install writes to `.codex/` in the current directory.
|
||||
* Tests `process.chdir(tmpDir)` and write fixtures to `tmpDir/.codex/`.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
process.env.GSD_TEST_MODE = '1';
|
||||
|
||||
const { describe, test, before, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const os = require('os');
|
||||
const { execFileSync } = require('child_process');
|
||||
|
||||
const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
const BUILD_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js');
|
||||
const { install, GSD_CODEX_MARKER } = require(INSTALL_SRC);
|
||||
|
||||
// Ensure hooks/dist/ is populated before install tests
|
||||
before(() => {
|
||||
execFileSync(process.execPath, [BUILD_SCRIPT], {
|
||||
encoding: 'utf-8',
|
||||
stdio: 'pipe',
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2698: CRLF stale gsd-update-check block is removed on Codex reinstall', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-crlf-install-2698-'));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// Helper: pre-populate .codex/config.toml with a GSD marker + stale hooks block
|
||||
// using the given line ending for the stale hooks block header, and a potentially
|
||||
// different EOL for the hooks body. This exercises the cross-platform mixed scenario.
|
||||
function writeCodexConfigWithStaleHooks(dir, headerEol, bodyEol) {
|
||||
// Build the stale block with header EOL for the "# GSD Hooks" line, but body EOL
|
||||
// for the content lines (simulates a file edited by two different platforms).
|
||||
const staleBlock = [
|
||||
'# GSD Hooks', // line that starts the stale section
|
||||
'[[hooks]]',
|
||||
'event = "SessionStart"',
|
||||
'command = "node /old/path/gsd-update-check.js"',
|
||||
].join(bodyEol);
|
||||
|
||||
// Put the stale block in user content BEFORE the GSD marker. The GSD marker area
|
||||
// will be regenerated by mergeCodexConfig during install(); the stale block in
|
||||
// the user area is what the hooks migration must remove.
|
||||
const content = [
|
||||
'[features]',
|
||||
'codex_hooks = true',
|
||||
'',
|
||||
].join(headerEol) + headerEol + staleBlock + headerEol + headerEol + GSD_CODEX_MARKER + headerEol;
|
||||
|
||||
const codexDir = path.join(dir, '.codex');
|
||||
fs.mkdirSync(codexDir, { recursive: true });
|
||||
const configPath = path.join(codexDir, 'config.toml');
|
||||
fs.writeFileSync(configPath, content, 'utf-8');
|
||||
return configPath;
|
||||
}
|
||||
|
||||
test('LF config.toml: stale gsd-update-check block removed on reinstall', (t) => {
|
||||
const origCwd = process.cwd();
|
||||
t.after(() => { process.chdir(origCwd); });
|
||||
process.chdir(tmpDir);
|
||||
|
||||
writeCodexConfigWithStaleHooks(tmpDir, '\n', '\n');
|
||||
install(false, 'codex');
|
||||
|
||||
const configPath = path.join(tmpDir, '.codex', 'config.toml');
|
||||
const content = fs.readFileSync(configPath, 'utf-8');
|
||||
|
||||
assert.ok(
|
||||
!content.includes('gsd-update-check'),
|
||||
'Stale gsd-update-check entry must be removed from LF config.toml (#2698)'
|
||||
);
|
||||
assert.ok(
|
||||
content.includes('gsd-check-update'),
|
||||
'New gsd-check-update hook must appear after reinstall'
|
||||
);
|
||||
});
|
||||
|
||||
test('CRLF config.toml: stale gsd-update-check block removed on reinstall', (t) => {
|
||||
const origCwd = process.cwd();
|
||||
t.after(() => { process.chdir(origCwd); });
|
||||
process.chdir(tmpDir);
|
||||
|
||||
writeCodexConfigWithStaleHooks(tmpDir, '\r\n', '\r\n');
|
||||
install(false, 'codex');
|
||||
|
||||
const configPath = path.join(tmpDir, '.codex', 'config.toml');
|
||||
const content = fs.readFileSync(configPath, 'utf-8');
|
||||
|
||||
assert.ok(
|
||||
!content.includes('gsd-update-check'),
|
||||
'Stale gsd-update-check entry must be removed from CRLF config.toml (#2698)'
|
||||
);
|
||||
assert.ok(
|
||||
content.includes('gsd-check-update'),
|
||||
'New gsd-check-update hook must appear after reinstall'
|
||||
);
|
||||
});
|
||||
|
||||
test('mixed-EOL config.toml: stale block with LF header but CRLF body removed on reinstall', (t) => {
|
||||
// This is the primary failure case: header line uses LF but the body uses CRLF.
|
||||
// The old LF-only regex requires all-\n separators; the old CRLF-only regex requires
|
||||
// all-\r\n separators. Neither matches a block with mixed endings, so the stale
|
||||
// block survives reinstall with the old code (#2698).
|
||||
const origCwd = process.cwd();
|
||||
t.after(() => { process.chdir(origCwd); });
|
||||
process.chdir(tmpDir);
|
||||
|
||||
// headerEol='\n' (file dominant), bodyEol='\r\n' (hook block from another platform)
|
||||
writeCodexConfigWithStaleHooks(tmpDir, '\n', '\r\n');
|
||||
install(false, 'codex');
|
||||
|
||||
const configPath = path.join(tmpDir, '.codex', 'config.toml');
|
||||
const content = fs.readFileSync(configPath, 'utf-8');
|
||||
|
||||
assert.ok(
|
||||
!content.includes('gsd-update-check'),
|
||||
[
|
||||
'Stale gsd-update-check block with mixed LF/CRLF endings must be removed (#2698).',
|
||||
'Old code used two separate LF-only and CRLF-only regexes; neither matched mixed content.',
|
||||
'Fix consolidates to a single \\r?\\n-aware regex.',
|
||||
].join(' ')
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user