diff --git a/.changeset/tidy-bears-jump.md b/.changeset/tidy-bears-jump.md new file mode 100644 index 000000000..5f9ef2f5a --- /dev/null +++ b/.changeset/tidy-bears-jump.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4504 +--- +**`config-set --dry-run` now actually previews instead of writing** — the flag was silently accepted and ignored, so a probing call still mutated `.planning/config.json` for real; a second dry-run's `previousValue` proved the first had persisted. Both mutating branches (a real set, and the `config-set null` unset path) now honor `--dry-run`, reporting a `dry_run: true` / `would_update` or `would_unset` preview with the current value and writing nothing. Validation and secret masking run identically whether or not `--dry-run` is passed. (#4444) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index d8977c376..0263cd9f4 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -784,7 +784,7 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load error, output: output, }); - if (!handled) config.cmdConfigSet(cwd, args[1], args[2], raw); + if (!handled) config.cmdConfigSet(cwd, args[1], args[2], raw, { dryRun: args.includes('--dry-run') }); } function routeConfigSetModelProfile({ args, cwd, raw }) { diff --git a/src/config.cts b/src/config.cts index 1a746074a..a1ae98b5e 100644 --- a/src/config.cts +++ b/src/config.cts @@ -627,19 +627,36 @@ function _unsetNestedValue( * Does not call `output()`, so can be used as one step in a command without triggering `exit(0)` in * the happy path. But note that `error()` will still `exit(1)` out of the process. */ +/** + * Loads `.planning/config.json` as a plain object, or `{}` if the file does + * not exist. A parse failure calls `error()` (process-exiting) rather than + * throwing, matching every caller's existing behavior. + * + * Single source for this load+parse step — `setConfigValue`, + * `unsetConfigValue`, `setConfigValues`, `previewConfigValue`, and + * `previewUnsetConfigValue` all delegate here instead of each repeating the + * same try/catch (CLAUDE.md's "Generative Fix Divergence" known-defect + * pattern: independently-guessed copies of the same logic can silently + * drift apart). + */ +function loadConfigJson(cwd: string): Record { + const configPath = path.join(planningDir(cwd), 'config.json'); + let config: Record = {}; + try { + if (fs.existsSync(configPath)) { + config = JSON.parse(fs.readFileSync(configPath, 'utf-8')) as Record; + } + } catch (err) { + error('Failed to read config.json: ' + (err as Error).message, ERROR_REASON.CONFIG_PARSE_FAILED); + } + return config; +} + function unsetConfigValue(cwd: string, keyPath: string): UnsetConfigValueResult { const configPath = path.join(planningDir(cwd), 'config.json'); return withPlanningLock(cwd, () => { - // Load existing config or start with empty object - let config: Record = {}; - try { - if (fs.existsSync(configPath)) { - config = JSON.parse(fs.readFileSync(configPath, 'utf-8')) as Record; - } - } catch (err) { - error('Failed to read config.json: ' + (err as Error).message, ERROR_REASON.CONFIG_PARSE_FAILED); - } + const config = loadConfigJson(cwd); const { previousValue, existed } = _unsetNestedValue(config, keyPath); @@ -664,15 +681,7 @@ function setConfigValue(cwd: string, keyPath: string, parsedValue: unknown): Set const configPath = path.join(planningDir(cwd), 'config.json'); return withPlanningLock(cwd, () => { - // Load existing config or start with empty object - let config: Record = {}; - try { - if (fs.existsSync(configPath)) { - config = JSON.parse(fs.readFileSync(configPath, 'utf-8')) as Record; - } - } catch (err) { - error('Failed to read config.json: ' + (err as Error).message, ERROR_REASON.CONFIG_PARSE_FAILED); - } + const config = loadConfigJson(cwd); const previousValue = _setNestedValue(config, keyPath, parsedValue); @@ -686,6 +695,31 @@ function setConfigValue(cwd: string, keyPath: string, parsedValue: unknown): Set }) as SetConfigValueResult; } +/** + * #4444: read-only preview counterpart to `setConfigValue` — loads config + * exactly like the real setter and reuses `_setNestedValue` (the SAME + * traversal/creation logic, including its prototype-pollution guards) on a + * throwaway in-memory copy that is NEVER written back to disk. This is what + * makes the dry-run preview provably identical to what the real write would + * compute, rather than a second, hand-maintained traversal that could drift + * from the real one. + */ +function previewConfigValue(cwd: string, keyPath: string, parsedValue: unknown): { key: string; value: unknown; previousValue: unknown } { + const config = loadConfigJson(cwd); + const previousValue = _setNestedValue(config, keyPath, parsedValue); + return { key: keyPath, value: parsedValue, previousValue }; +} + +/** + * #4444: read-only preview counterpart to `unsetConfigValue` — same pattern + * as `previewConfigValue`, reusing `_unsetNestedValue` on a throwaway copy. + */ +function previewUnsetConfigValue(cwd: string, keyPath: string): { key: string; value: null; previousValue: unknown; existed: boolean } { + const config = loadConfigJson(cwd); + const { previousValue, existed } = _unsetNestedValue(config, keyPath); + return { key: keyPath, value: null, previousValue, existed }; +} + /** * Batched sibling of setConfigValue: apply multiple key-path writes in a * single load → set-all → write cycle inside ONE withPlanningLock call. @@ -707,15 +741,7 @@ function setConfigValues( const configPath = path.join(planningDir(cwd), 'config.json'); return withPlanningLock(cwd, () => { - // Load existing config or start with empty object - let config: Record = {}; - try { - if (fs.existsSync(configPath)) { - config = JSON.parse(fs.readFileSync(configPath, 'utf-8')) as Record; - } - } catch (err) { - error('Failed to read config.json: ' + (err as Error).message, ERROR_REASON.CONFIG_PARSE_FAILED); - } + const config = loadConfigJson(cwd); const results: SetConfigValueResult[] = []; for (const entry of entries) { @@ -757,7 +783,12 @@ function assertEnumValue(parsedValue: unknown, rawVal: string, allowed: readonly * Note that this exits the process (via `output()`) even in the happy path; use `setConfigValue()` * directly if you need to avoid this. */ -function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | undefined, raw: boolean): void { +interface ConfigSetOptions { + dryRun?: boolean; +} + +function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | undefined, raw: boolean, options: ConfigSetOptions = {}): void { + const dryRun = options.dryRun === true; if (!keyPath) { error('Usage: config-set ', ERROR_REASON.USAGE); } @@ -805,6 +836,18 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | // present, truthy-adjacent value that consumers must special-case — worst for // secret keys where a leftover value can be passed as a real credential. if (parsedValue === null) { + if (dryRun) { + const preview = previewUnsetConfigValue(cwd, kp); + if (isSecretKey(kp)) { + const maskedPrev = preview.previousValue === undefined + ? undefined + : maskSecret(preview.previousValue as Parameters[0]); + output({ dry_run: true, would_unset: true, key: kp, value: null, previousValue: maskedPrev, masked: true }, raw, `${kp} unset (dry run)`); + return; + } + output({ dry_run: true, would_unset: true, key: kp, value: null, previousValue: preview.previousValue }, raw, `${kp} unset (dry run)`); + return; + } const unsetResult = unsetConfigValue(cwd, kp); if (isSecretKey(kp)) { const maskedPrev = unsetResult.previousValue === undefined @@ -1014,6 +1057,20 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | } } + if (dryRun) { + const preview = previewConfigValue(cwd, kp, parsedValue); + if (isSecretKey(kp)) { + const masked = maskSecret(parsedValue as Parameters[0]); + const maskedPrev = preview.previousValue === undefined + ? undefined + : maskSecret(preview.previousValue as Parameters[0]); + output({ dry_run: true, would_update: true, key: kp, value: masked, previousValue: maskedPrev, masked: true }, raw, `${kp}=${masked} (dry run)`); + return; + } + output({ dry_run: true, would_update: true, key: kp, value: parsedValue, previousValue: preview.previousValue }, raw, `${kp}=${String(parsedValue)} (dry run)`); + return; + } + const setConfigValueResult = setConfigValue(cwd, kp, parsedValue); // Mask secrets in both JSON and text output. The plaintext is written diff --git a/tests/config.test.cjs b/tests/config.test.cjs index 2f7318695..a89856d6f 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -964,6 +964,134 @@ describe('config-set null — unset/clear (#2046)', () => { }); }); +// ─── config-set --dry-run (#4444) ──────────────────────────────────────────── + +describe('config-set --dry-run (#4444)', () => { + let tmpDir; + + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + test('config-set --dry-run does not write to config.json (first call, key absent)', () => { + const result = runGsdTools('config-set model_profile quality --dry-run', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.dry_run, true); + + const configPath = path.join(tmpDir, '.planning', 'config.json'); + if (fs.existsSync(configPath)) { + const config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + assert.strictEqual( + Object.prototype.hasOwnProperty.call(config, 'model_profile'), + false, + 'model_profile must not be written to config.json by a --dry-run call' + ); + } + }); + + test('config-set --dry-run does not persist across repeated dry-run calls (repro: previousValue must not reflect a prior dry run)', () => { + const first = runGsdTools('config-set review.timeouts.antigravity 1 --dry-run', tmpDir); + assert.ok(first.success, `Command failed: ${first.error}`); + const firstOutput = JSON.parse(first.output); + assert.strictEqual(firstOutput.dry_run, true); + assert.strictEqual(firstOutput.previousValue, undefined); + + const second = runGsdTools('config-set review.timeouts.antigravity 3600 --dry-run', tmpDir); + assert.ok(second.success, `Command failed: ${second.error}`); + const secondOutput = JSON.parse(second.output); + assert.strictEqual(secondOutput.dry_run, true); + // The reported bug: this used to be 1 (the first "dry run"'s value), + // proving it was actually persisted to disk. + assert.strictEqual( + secondOutput.previousValue, + undefined, + 'a second --dry-run call must not see a value the first --dry-run "wrote"' + ); + }); + + test('config-set: a real write after dry-run calls sees the pre-dry-run value, not a dry-run leak', () => { + const seed = runGsdTools('config-set review.timeouts.antigravity 10', tmpDir); + assert.ok(seed.success, `seed failed: ${seed.error}`); + + const dryRun = runGsdTools('config-set review.timeouts.antigravity 9999 --dry-run', tmpDir); + assert.ok(dryRun.success, `Command failed: ${dryRun.error}`); + assert.strictEqual(JSON.parse(dryRun.output).dry_run, true); + + // The dry-run must not have changed the on-disk value. + assert.strictEqual(readConfig(tmpDir).review.timeouts.antigravity, 10); + + const real = runGsdTools('config-set review.timeouts.antigravity 20', tmpDir); + assert.ok(real.success, `Command failed: ${real.error}`); + assert.strictEqual( + JSON.parse(real.output).previousValue, + 10, + 'the real write must see the seeded value (10), not the dry-run value (9999)' + ); + assert.strictEqual(readConfig(tmpDir).review.timeouts.antigravity, 20); + }); + + test('config-set --dry-run correctly previews the current value without mutating it', () => { + const seed = runGsdTools('config-set model_profile quality', tmpDir); + assert.ok(seed.success, `seed failed: ${seed.error}`); + + const dryRun = runGsdTools('config-set model_profile speed --dry-run', tmpDir); + assert.ok(dryRun.success, `Command failed: ${dryRun.error}`); + const output = JSON.parse(dryRun.output); + assert.strictEqual(output.dry_run, true); + assert.strictEqual(output.previousValue, 'quality'); + assert.strictEqual(output.value, 'speed'); + + // Unchanged on disk. + assert.strictEqual(readConfig(tmpDir).model_profile, 'quality'); + }); + + test('config-set --dry-run still validates: an invalid value is still rejected, not silently previewed', () => { + const result = runGsdTools('config-set context badvalue --dry-run', tmpDir); + assert.strictEqual(result.success, false, 'an invalid enum value must still be rejected under --dry-run'); + assert.match(result.error, /Invalid context value/i); + + // Nothing written. + const configPath = path.join(tmpDir, '.planning', 'config.json'); + if (fs.existsSync(configPath)) { + assert.strictEqual( + Object.prototype.hasOwnProperty.call(JSON.parse(fs.readFileSync(configPath, 'utf-8')), 'context'), + false + ); + } + }); + + test('config-set --dry-run masks secret values in the preview exactly like the real write does', () => { + const seed = runGsdTools('config-set brave_search sk-test-original', tmpDir); + assert.ok(seed.success, `seed failed: ${seed.error}`); + + const dryRun = runGsdTools('config-set brave_search sk-test-newvalue --dry-run', tmpDir); + assert.ok(dryRun.success, `Command failed: ${dryRun.error}`); + const output = JSON.parse(dryRun.output); + assert.strictEqual(output.dry_run, true); + assert.strictEqual(output.masked, true); + assert.doesNotMatch(JSON.stringify(output), /sk-test-newvalue/, 'the new secret value must not appear in plaintext'); + assert.doesNotMatch(JSON.stringify(output), /sk-test-original/, 'the previous secret value must not appear in plaintext'); + + // Unchanged on disk. + assert.strictEqual(readConfig(tmpDir).brave_search, 'sk-test-original'); + }); + + test('config-set null --dry-run does not unset (dry-run covers the unset branch too)', () => { + const seed = runGsdTools('config-set review.models.gemini foo', tmpDir); + assert.ok(seed.success, `seed failed: ${seed.error}`); + + const dryRun = runGsdTools('config-set review.models.gemini null --dry-run', tmpDir); + assert.ok(dryRun.success, `Command failed: ${dryRun.error}`); + const output = JSON.parse(dryRun.output); + assert.strictEqual(output.dry_run, true); + assert.strictEqual(output.would_unset, true); + + // Still present on disk — the dry-run must not have unset it. + assert.strictEqual(readConfig(tmpDir).review.models.gemini, 'foo'); + }); +}); + // ─── config-set (research_before_questions and discuss_mode) ────────────────── describe('config-set research_before_questions and discuss_mode', () => { diff --git a/tests/install-minimal-hooks.test.cjs b/tests/install-minimal-hooks.test.cjs index e02515023..080c6a323 100644 --- a/tests/install-minimal-hooks.test.cjs +++ b/tests/install-minimal-hooks.test.cjs @@ -20,7 +20,7 @@ process.env.GSD_TEST_MODE = '1'; -const { test, describe, beforeEach, afterEach, before } = require('node:test'); +const { test, describe, beforeEach, afterEach, before, after } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); @@ -120,36 +120,31 @@ describe('install: --help profile counts match PROFILES (#834)', () => { return r.stdout; } - test('core line advertises PROFILES.core.length main-loop skills', () => { + test('core/standard/full lines advertise correct, drift-tracked skill counts', () => { const out = helpText(); - const m = out.match(/core\s+—\s+~?(\d+)\s+main-loop skills/); - assert.ok(m, `--help must advertise a core profile skill count; got:\n${out}`); + + const mCore = out.match(/core\s+—\s+~?(\d+)\s+main-loop skills/); + assert.ok(mCore, `--help must advertise a core profile skill count; got:\n${out}`); assert.strictEqual( - Number(m[1]), + Number(mCore[1]), PROFILES.core.length, - `--help core count (${m[1]}) must equal PROFILES.core.length (${PROFILES.core.length})`, + `--help core count (${mCore[1]}) must equal PROFILES.core.length (${PROFILES.core.length})`, ); - }); - test('standard line advertises PROFILES.standard.length skills', () => { - const out = helpText(); - const m = out.match(/standard\s+—\s+~?(\d+)\s+skills/); - assert.ok(m, `--help must advertise a standard profile skill count; got:\n${out}`); + const mStandard = out.match(/standard\s+—\s+~?(\d+)\s+skills/); + assert.ok(mStandard, `--help must advertise a standard profile skill count; got:\n${out}`); assert.strictEqual( - Number(m[1]), + Number(mStandard[1]), PROFILES.standard.length, - `--help standard count (${m[1]}) must equal PROFILES.standard.length (${PROFILES.standard.length})`, + `--help standard count (${mStandard[1]}) must equal PROFILES.standard.length (${PROFILES.standard.length})`, ); - }); - test('full line does not hardcode a drift-prone skill count', () => { - const out = helpText(); - const m = out.match(/full\s+—\s+([^\n]*?)\s+\(default\)/); - assert.ok(m, `--help must advertise a full profile line; got:\n${out}`); + const mFull = out.match(/full\s+—\s+([^\n]*?)\s+\(default\)/); + assert.ok(mFull, `--help must advertise a full profile line; got:\n${out}`); assert.doesNotMatch( - m[1], + mFull[1], /\d/, - `--help full line must not hardcode a numeric skill count (drifts); got: "${m[1]}"`, + `--help full line must not hardcode a numeric skill count (drifts); got: "${mFull[1]}"`, ); }); }); @@ -351,41 +346,36 @@ describe('install-profiles: allowlist scope guards', () => { // ─── Section 10: --minimal install — per-runtime E2E (spawned) ─────────────── -describe('install: --minimal honoured for every runtime in --global mode', () => { +describe('install: --minimal honoured for every runtime, on-disk matches manifest', () => { for (const runtime of SKILL_RUNTIMES) { - test(`${runtime} --global --minimal: mode=minimal, correct skills, zero agents`, () => { - const { manifest, root } = runMinimalInstall({ runtime, scope: 'global', extraArgs: ['--minimal'] }); - try { - assert.ok(manifest, `${runtime} global must produce manifest`); - assert.strictEqual(manifest.mode, 'minimal'); - assert.deepStrictEqual( - [...manifestSkillSet(manifest)].sort(), - [...MINIMAL_SKILL_ALLOWLIST].sort(), - ); - assert.strictEqual(manifestAgentCount(manifest), 0); - } finally { - cleanup(root); - } - }); - } -}); + for (const scope of ['global', 'local']) { + test(`${runtime} --${scope} --minimal: mode, skills, zero agents, on-disk matches manifest`, () => { + const { manifest, configDir, root } = runMinimalInstall({ runtime, scope, extraArgs: ['--minimal'] }); + try { + assert.ok(manifest, `${runtime} ${scope} must produce manifest`); + assert.strictEqual(manifest.mode, 'minimal'); + assert.deepStrictEqual( + [...manifestSkillSet(manifest)].sort(), + [...MINIMAL_SKILL_ALLOWLIST].sort(), + ); + assert.strictEqual(manifestAgentCount(manifest), 0); -describe('install: --minimal honoured for every runtime in --local mode', () => { - for (const runtime of SKILL_RUNTIMES) { - test(`${runtime} --local --minimal: mode=minimal, correct skills, zero agents`, () => { - const { manifest, root } = runMinimalInstall({ runtime, scope: 'local', extraArgs: ['--minimal'] }); - try { - assert.ok(manifest, `${runtime} local must produce manifest`); - assert.strictEqual(manifest.mode, 'minimal'); - assert.deepStrictEqual( - [...manifestSkillSet(manifest)].sort(), - [...MINIMAL_SKILL_ALLOWLIST].sort(), - ); - assert.strictEqual(manifestAgentCount(manifest), 0); - } finally { - cleanup(root); - } - }); + const onDisk = collectSkillBasenamesOnDiskSandboxed(configDir, runtime, scope, root); + const inManifest = manifestSkillSet(manifest); + assert.deepStrictEqual([...onDisk].sort(), [...inManifest].sort()); + // Not the shared listAgentFiles() helper: asserts on the INSTALLED + // dest dir (must be empty in --minimal mode), not the source roster. + const agentsDir = path.join(configDir, 'agents'); + if (fs.existsSync(agentsDir)) { + const gsdAgents = fs.readdirSync(agentsDir) + .filter(f => f.startsWith('gsd-') && f.endsWith('.md')); + assert.deepStrictEqual(gsdAgents, []); + } + } finally { + cleanup(root); + } + }); + } } }); @@ -407,36 +397,36 @@ describe('install: Cline --minimal (rules-based, no skills/ dir)', () => { } }); -describe('install: on-disk skill files match manifest for --minimal', () => { - for (const runtime of SKILL_RUNTIMES) { - for (const scope of ['global', 'local']) { - test(`${runtime} --${scope} --minimal: on-disk matches manifest`, () => { - const { manifest, configDir, root } = runMinimalInstall({ - runtime, scope, extraArgs: ['--minimal'], - }); - try { - assert.ok(manifest); - const onDisk = collectSkillBasenamesOnDiskSandboxed(configDir, runtime, scope, root); - const inManifest = manifestSkillSet(manifest); - assert.deepStrictEqual([...onDisk].sort(), [...inManifest].sort()); - // Not the shared listAgentFiles() helper: asserts on the INSTALLED - // dest dir (must be empty in --minimal mode), not the source roster. - const agentsDir = path.join(configDir, 'agents'); - if (fs.existsSync(agentsDir)) { - const gsdAgents = fs.readdirSync(agentsDir) - .filter(f => f.startsWith('gsd-') && f.endsWith('.md')); - assert.deepStrictEqual(gsdAgents, []); - } - } finally { - cleanup(root); - } - }); - } - } -}); - // ─── Section 11: --minimal manifest mode + downgrade ───────────────────────── +// Shared across "manifest records mode" and "install-minimal-backcompat": both +// describe blocks below independently re-installed the IDENTICAL +// `--claude --global --minimal` configuration just to check different fields +// of the same manifest/profile-marker output. Install it once and derive +// everything both sets of tests need. +let _sharedMinimalManifestInstall; +function sharedMinimalManifestInstall() { + if (_sharedMinimalManifestInstall) return _sharedMinimalManifestInstall; + const targetDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-minimal-shared-')); + runNode( + [INSTALL_SCRIPT, '--claude', '--global', '--config-dir', targetDir, '--minimal'], + { env: installerEnv(), timeoutMs: 120000 }, + ); + const manifestPath = path.join(targetDir, MANIFEST_NAME); + const m = fs.existsSync(manifestPath) ? JSON.parse(fs.readFileSync(manifestPath, 'utf8')) : {}; + const skillCount = Object.keys(m.files || {}).filter( + k => k.startsWith('skills/') && k.endsWith('/SKILL.md'), + ).length; + const markerPath = path.join(targetDir, '.gsd-profile'); + const profileMarker = fs.existsSync(markerPath) ? fs.readFileSync(markerPath, 'utf8').trim() : null; + const agentCount = Object.keys(m.files || {}).filter(k => k.startsWith('agents/')).length; + _sharedMinimalManifestInstall = { targetDir, mode: m.mode, skillCount, agentCount, profileMarker }; + return _sharedMinimalManifestInstall; +} +after(() => { + if (_sharedMinimalManifestInstall) cleanup(_sharedMinimalManifestInstall.targetDir); +}); + describe('install: manifest records mode for both profiles', () => { function manifestModeAfterInstall(extraArgs) { const targetDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-manifest-mode-')); @@ -467,7 +457,7 @@ describe('install: manifest records mode for both profiles', () => { }); test('--minimal records mode: "minimal" with exactly 8 skills and 0 agents', () => { - const r = manifestModeAfterInstall(['--minimal']); + const r = sharedMinimalManifestInstall(); assert.strictEqual(r.mode, 'minimal'); assert.strictEqual(r.skillCount, 8); assert.strictEqual(r.agentCount, 0); @@ -514,13 +504,13 @@ describe('install-minimal-backcompat: --minimal and --profile=core produce same } test('--minimal produces mode "minimal" with exactly 8 skills', () => { - const r = installAndGetManifest(['--minimal']); + const r = sharedMinimalManifestInstall(); assert.strictEqual(r.mode, 'minimal'); assert.strictEqual(r.skillCount, 8); }); test('--minimal writes .gsd-profile marker "core"', () => { - const r = installAndGetManifest(['--minimal']); + const r = sharedMinimalManifestInstall(); assert.strictEqual(r.profileMarker, 'core'); }); @@ -2887,35 +2877,27 @@ describe('#1834: installer deploys .sh hooks alongside .js hooks', () => { cleanup(tmpDir); }); - test('gsd-session-state.sh is present after install', () => { + test('gsd-session-state.sh, gsd-validate-commit.sh, gsd-phase-boundary.sh, and all SH_HOOKS are present after install', () => { const hooksDir = runInstaller(tmpDir); - const target = path.join(hooksDir, 'gsd-session-state.sh'); + + const sessionStateTarget = path.join(hooksDir, 'gsd-session-state.sh'); assert.ok( - fs.existsSync(target), + fs.existsSync(sessionStateTarget), 'gsd-session-state.sh must be installed to hooks/ — missing file causes SessionStart hook errors' ); - }); - test('gsd-validate-commit.sh is present after install', () => { - const hooksDir = runInstaller(tmpDir); - const target = path.join(hooksDir, 'gsd-validate-commit.sh'); + const validateCommitTarget = path.join(hooksDir, 'gsd-validate-commit.sh'); assert.ok( - fs.existsSync(target), + fs.existsSync(validateCommitTarget), 'gsd-validate-commit.sh must be installed to hooks/ — missing file causes PreToolUse hook errors' ); - }); - test('gsd-phase-boundary.sh is present after install', () => { - const hooksDir = runInstaller(tmpDir); - const target = path.join(hooksDir, 'gsd-phase-boundary.sh'); + const phaseBoundaryTarget = path.join(hooksDir, 'gsd-phase-boundary.sh'); assert.ok( - fs.existsSync(target), + fs.existsSync(phaseBoundaryTarget), 'gsd-phase-boundary.sh must be installed to hooks/ — missing file causes PostToolUse hook errors' ); - }); - test('all three .sh hooks are present after a single install', () => { - const hooksDir = runInstaller(tmpDir); for (const hook of SH_HOOKS) { assert.ok( fs.existsSync(path.join(hooksDir, hook)), @@ -2996,7 +2978,7 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ return path.join(configDir, 'hooks'); } - test('#2586: gsd-context-monitor.js is no longer staged for Codex at all', () => { + test('#2586: no longer staged, dependency closure empty, staged set equals closure', () => { // Was: "the installed context-monitor hook LOADS AND RUNS, not merely // exists" — that row's premise (Codex ships this hook) is exactly what // #2586 removes: its only documented metrics source is Claude's own @@ -3006,21 +2988,18 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ // remains covered live via Windsurf's own guards — see the // "#4087 review: Windsurf install..." describe block below, unaffected // by this change. - const hooksDir = installCodex(tmpDir); - assert.strictEqual(fs.existsSync(path.join(hooksDir, 'gsd-context-monitor.js')), false, - 'gsd-context-monitor.js must not be staged for Codex post-#2586'); - assert.strictEqual(fs.existsSync(path.join(hooksDir, 'lib')), false, - 'hooks/lib/ must not exist at all — nothing else Codex stages requires a lib/ helper'); - }); - - // AC4: this is the row that stops the bug recurring. It derives the - // requirement graph from the SHIPPED files rather than restating today's three - // helpers, so a Codex-bundled hook that grows a new lib dependency fails here - // instead of in a user's session. - test('every helper required by a staged Codex hook — transitively — is staged', () => { const hooksDir = installCodex(tmpDir); const libDir = path.join(hooksDir, 'lib'); + assert.strictEqual(fs.existsSync(path.join(hooksDir, 'gsd-context-monitor.js')), false, + 'gsd-context-monitor.js must not be staged for Codex post-#2586'); + assert.strictEqual(fs.existsSync(libDir), false, + 'hooks/lib/ must not exist at all — nothing else Codex stages requires a lib/ helper'); + + // AC4: this proves the requirement graph derived from the SHIPPED files + // rather than restating today's helpers, so a Codex-bundled hook that + // grows a new lib dependency fails here instead of in a user's session. + // // Seed from hook scripts: only the explicit './lib/X' spelling is a lib // requirement. A bare './X' from a hook script is a sibling in hooks/ // (gsd-check-update-worker.js requires './managed-hooks-registry.cjs'), @@ -3069,6 +3048,65 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ scan(fs.readFileSync(staged, 'utf8'), libRe); next = [...required].find((f) => !checked.has(f)); } + + // Was: "fewer staged than available, and graphify absent". That passes while + // over-staging (an extra git-cmd.js keeps the count below the total and + // leaves graphify absent), so it did not prove its own title — the #3579 + // boundary is that helpers nothing requires must NOT ship (review of #4087). + // Now compared as SETS, with the difference asserted in both directions. + const stagedLibs = fs.existsSync(libDir) ? fs.readdirSync(libDir).sort() : []; + // #2586: Codex's closure is now legitimately empty (gsd-context-monitor.js, + // the only staged Codex hook that ever required a helper, is no longer + // staged) — the deepStrictEqual below is still the real assertion and + // holds for the empty case too; the non-empty case remains covered live + // by the "#4087 review: Windsurf install..." describe block below. + assert.strictEqual(stagedLibs.length, 0, 'no helpers should be staged for Codex post-#2586'); + + // Derive the closure independently of the installer. + const seedRe3 = /require\(\s*['"]\.\/lib\/([A-Za-z0-9._-]+)['"]\s*\)/g; + const libRe3 = /require\(\s*['"]\.\/(?:lib\/)?([A-Za-z0-9._-]+)['"]\s*\)/g; + const srcLibDir = path.join(__dirname, '..', 'hooks', 'lib'); + const required3 = new Set(); + const scan3 = (source, re) => { + re.lastIndex = 0; + let m; + while ((m = re.exec(source)) !== null) { + if (/[A-Za-z0-9]/.test(m[1])) required3.add(m[1]); + } + }; + const resolveName = (name) => [name, `${name}.js`, `${name}.cjs`] + .find((c) => fs.existsSync(path.join(srcLibDir, c))); + + for (const entry of fs.readdirSync(hooksDir)) { + const full = path.join(hooksDir, entry); + if (!fs.statSync(full).isFile() || !/\.(js|cjs)$/.test(entry)) continue; + scan3(fs.readFileSync(full, 'utf8'), seedRe3); + } + const closure3 = new Set(); + let next3 = [...required3].find((f) => !closure3.has(resolveName(f) || f)); + while (next3 !== undefined) { + const resolved = resolveName(next3); + assert.ok(resolved, `hooks/lib/${next3} is required but absent from source — packaging bug`); + closure3.add(resolved); + scan3(fs.readFileSync(path.join(srcLibDir, resolved), 'utf8'), libRe3); + next3 = [...required3].find((f) => !closure3.has(resolveName(f) || f)); + } + + const expected3 = [...closure3].sort(); + assert.deepStrictEqual( + stagedLibs, expected3, + 'the staged helper set must equal the dependency closure exactly. Extra files violate the ' + + '#3579 boundary (helpers no Codex hook requires must not ship); missing files mean a hook ' + + `throws MODULE_NOT_FOUND at load. staged=${JSON.stringify(stagedLibs)} ` + + `expected=${JSON.stringify(expected3)}`, + ); + // Non-vacuity: the source dir must hold MORE than the closure, or an + // over-staging bug would be undetectable by this comparison. + const available3 = fs.readdirSync(srcLibDir); + assert.ok( + available3.length > expected3.length, + `precondition: source must offer more helpers than the closure needs (available=${available3.length}, closure=${expected3.length})`, + ); }); // The #3579 boundary this fix must preserve: derive what is needed, do not @@ -3147,69 +3185,6 @@ describe('#4087 regression: Codex install stages the hook helpers its hooks requ ); }); }); - - test('the staged helper set EQUALS the dependency closure — no more, no less', () => { - // Was: "fewer staged than available, and graphify absent". That passes while - // over-staging (an extra git-cmd.js keeps the count below the total and - // leaves graphify absent), so it did not prove its own title — the #3579 - // boundary is that helpers nothing requires must NOT ship (review of #4087). - // Now compared as SETS, with the difference asserted in both directions. - const hooksDir = installCodex(tmpDir); - const libDir = path.join(hooksDir, 'lib'); - const stagedLibs = fs.existsSync(libDir) ? fs.readdirSync(libDir).sort() : []; - // #2586: Codex's closure is now legitimately empty (gsd-context-monitor.js, - // the only staged Codex hook that ever required a helper, is no longer - // staged) — the deepStrictEqual below is still the real assertion and - // holds for the empty case too; the non-empty case remains covered live - // by the "#4087 review: Windsurf install..." describe block below. - assert.strictEqual(stagedLibs.length, 0, 'no helpers should be staged for Codex post-#2586'); - - // Derive the closure independently of the installer. - const seedRe = /require\(\s*['"]\.\/lib\/([A-Za-z0-9._-]+)['"]\s*\)/g; - const libRe = /require\(\s*['"]\.\/(?:lib\/)?([A-Za-z0-9._-]+)['"]\s*\)/g; - const srcLibDir = path.join(__dirname, '..', 'hooks', 'lib'); - const required = new Set(); - const scan = (source, re) => { - re.lastIndex = 0; - let m; - while ((m = re.exec(source)) !== null) { - if (/[A-Za-z0-9]/.test(m[1])) required.add(m[1]); - } - }; - const resolveName = (name) => [name, `${name}.js`, `${name}.cjs`] - .find((c) => fs.existsSync(path.join(srcLibDir, c))); - - for (const entry of fs.readdirSync(hooksDir)) { - const full = path.join(hooksDir, entry); - if (!fs.statSync(full).isFile() || !/\.(js|cjs)$/.test(entry)) continue; - scan(fs.readFileSync(full, 'utf8'), seedRe); - } - const closure = new Set(); - let next = [...required].find((f) => !closure.has(resolveName(f) || f)); - while (next !== undefined) { - const resolved = resolveName(next); - assert.ok(resolved, `hooks/lib/${next} is required but absent from source — packaging bug`); - closure.add(resolved); - scan(fs.readFileSync(path.join(srcLibDir, resolved), 'utf8'), libRe); - next = [...required].find((f) => !closure.has(resolveName(f) || f)); - } - - const expected = [...closure].sort(); - assert.deepStrictEqual( - stagedLibs, expected, - 'the staged helper set must equal the dependency closure exactly. Extra files violate the ' - + '#3579 boundary (helpers no Codex hook requires must not ship); missing files mean a hook ' - + `throws MODULE_NOT_FOUND at load. staged=${JSON.stringify(stagedLibs)} ` - + `expected=${JSON.stringify(expected)}`, - ); - // Non-vacuity: the source dir must hold MORE than the closure, or an - // over-staging bug would be undetectable by this comparison. - const available = fs.readdirSync(srcLibDir); - assert.ok( - available.length > expected.length, - `precondition: source must offer more helpers than the closure needs (available=${available.length}, closure=${expected.length})`, - ); - }); }); // ─── #3023: pi must not stage its shared-hooks bundle in pi's reserved hooks/ ── @@ -3257,8 +3232,9 @@ describe('#4087 review: Windsurf install stages the hook helpers its hooks requi return path.join(configDir, 'hooks'); } - test('both installed Windsurf guards LOAD AND RUN, not merely exist', () => { + test('both installed Windsurf guards LOAD AND RUN, and their transitive helpers are staged', () => { const hooksDir = installWindsurf(tmpDir); + for (const script of ['gsd-windsurf-pre-write.js', 'gsd-windsurf-pre-command.js']) { const hook = path.join(hooksDir, script); assert.ok(fs.existsSync(hook), `precondition: ${script} must be staged`); @@ -3271,10 +3247,7 @@ describe('#4087 review: Windsurf install stages the hook helpers its hooks requi assert.doesNotMatch(String(result.stderr || ''), /MODULE_NOT_FOUND|Cannot find module/, `no missing-module error may reach stderr for ${script}`); } - }); - test('the helpers the Windsurf guards require are staged, transitively', () => { - const hooksDir = installWindsurf(tmpDir); const libDir = path.join(hooksDir, 'lib'); assert.ok(fs.existsSync(libDir), 'hooks/lib/ must be staged for Windsurf'); // Direct requires of the two guards, plus what hook-exit.js itself requires @@ -3293,8 +3266,8 @@ describe('#3023 pi shared-hooks bundle avoids the host-reserved hooks/ directory const PI_BUNDLE_DIR = 'gsd-hooks'; for (const scope of ['local', 'global']) { - test(`pi ${scope} install does not create the host-reserved hooks/ directory`, (t) => { - const { configDir, root } = runMinimalInstall({ runtime: 'pi', scope }); + test(`pi ${scope} install: no host-reserved hooks/ dir, bundle staged under ${PI_BUNDLE_DIR}/, manifested`, (t) => { + const { manifest, configDir, root } = runMinimalInstall({ runtime: 'pi', scope }); t.after(() => cleanup(root)); const reserved = path.join(configDir, PI_RESERVED_DIR); @@ -3304,11 +3277,6 @@ describe('#3023 pi shared-hooks bundle avoids the host-reserved hooks/ directory `pi reserves /${PI_RESERVED_DIR} as its deprecated extension location; ` + `GSD must not create it (found ${reserved})` ); - }); - - test(`pi ${scope} install stages the shared hooks bundle under ${PI_BUNDLE_DIR}/`, (t) => { - const { configDir, root } = runMinimalInstall({ runtime: 'pi', scope }); - t.after(() => cleanup(root)); const bundle = path.join(configDir, PI_BUNDLE_DIR); assert.equal( @@ -3332,11 +3300,6 @@ describe('#3023 pi shared-hooks bundle avoids the host-reserved hooks/ directory true, 'the CommonJS marker must live inside the bundle directory' ); - }); - - test(`pi ${scope} install manifests the bundle under ${PI_BUNDLE_DIR}/`, (t) => { - const { manifest, root } = runMinimalInstall({ runtime: 'pi', scope }); - t.after(() => cleanup(root)); assert.ok(manifest && manifest.files, 'pi install must write a file manifest'); const keys = Object.keys(manifest.files);