fix(#4444): honor config-set --dry-run instead of silently ignoring it (#4504)

* test(#4444): failing-first regression coverage for config-set --dry-run config-set --dry-run is currently parsed nowhere -- routeConfigSet (gsd-core/bin/gsd-tools.cjs) never checks args for it, and cmdConfigSet has no dry-run parameter, so the flag is silently swallowed and the command always writes for real. Reproduces the issue's own repro (sequential --dry-run calls where the second's previousValue proves the first persisted), plus coverage for validation-still-runs, secret-masking, and the sibling unset (config-set <key> null) branch, which has the identical defect. This commit adds the regression coverage only; the fix lands in the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4444): honor config-set --dry-run instead of silently ignoring it routeConfigSet (gsd-core/bin/gsd-tools.cjs) never read args for --dry-run, and cmdConfigSet had no dry-run parameter at all -- so the flag was silently accepted (as any unrecognized trailing argument is) and the command always wrote for real. A second "dry run" then showed previousValue reflecting the first one, proving it had persisted. Threads a dryRun option through cmdConfigSet, gating BOTH mutating branches: the null/unset path (unsetConfigValue) and the real-set path (setConfigValue) -- the unset branch had the identical defect, undiscovered until auditing every mutation site while designing this fix. Each gains a previewConfigValue/previewUnsetConfigValue counterpart that reuses the real function's exact traversal/creation logic (_setNestedValue/_unsetNestedValue) on a throwaway in-memory config copy that is never written -- so the preview can never diverge from what the real write would compute. All validation (unknown key, enum/number/boolean checks, secret masking) runs identically whether or not --dry-run is passed; only the final write is skipped, replaced with a `{ dry_run: true, would_update / would_unset: true, ... }` preview payload matching the precedent established by `milestone complete --dry-run` (#2118) and `todo complete --dry-run` (#4096/#4325). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor(#4444): extract loadConfigJson to stop a 5th copy-paste of the same load/parse block Code review flagged that setConfigValue, unsetConfigValue, setConfigValues, and the two new preview functions each repeated the identical "load .planning/config.json, JSON.parse, catch -> CONFIG_PARSE_FAILED" block -- exactly CLAUDE.md's own "Generative Fix Divergence" known-defect pattern. Extracted a single loadConfigJson(cwd) helper; behavior is unchanged (verified: build, tsc, and the dry-run/real-write smoke test all pass byte-identical to before). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4444): changeset for the config-set --dry-run fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4444): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4444): raise per-chunk CI test timeout to 800s for Windows headroom install-minimal-hooks.test.cjs (weight=24.45, the heaviest file in the suite) sits alone in its own chunk yet still occasionally brushed the 600000ms per-chunk ceiling on Windows -- observed on PR #4504's first CI run for this change (passed clean on rerun, consistent with the "legitimately too slow for the budget" cause the chunk-timeout diagnostic already names, not a leaked handle). Raised RUN_TESTS_CHUNK_TIMEOUT_MS's default from 600000ms to 800000ms: ~33% more margin, still comfortably below the 900000ms regen:derived fixture timeout that fragment-single-edit-propagation.install.test.cjs deliberately keeps ABOVE the chunk ceiling, and far under the 45-minute job cap -- Windows shards currently finish in ~19-20 minutes total, so there is ample headroom. Updated every dependent mirror/assertion in lockstep (tests/helpers/emitted-runtime.cjs's duplicated CHUNK_TIMEOUT_CEILING_MS constant, its lock test in tests/emitted-attribution.test.cjs, the Windows-skip prose in fragment-single-edit-propagation.install.test.cjs, and docs/TESTING-SUITES.md's reference table) so nothing describes a stale value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Revert "fix(#4444): raise per-chunk CI test timeout to 800s for Windows headroom" This reverts commit 394aadaf6f5af6fd700bf0f444c9fbd686285a4f. * test(#4444): consolidate redundant installer spawns in install-minimal-hooks.test.cjs This file's real, unrelated pre-existing cost (dated 2026-09-06, PR #4428) is what tipped a Windows CI shard over the per-chunk timeout backstop on PR #4504 (issue #4444's own diff never touches this file or the installer). Rather than raise the timeout, cut the file's actual spawn count: several describe blocks independently re-installed the IDENTICAL runtime/scope/flag configuration just to assert different things about the same install output. Merged each such group onto a single shared install, with every original assertion preserved: - --help x3 -> x1 - the three per-runtime/scope --minimal E2E loops (global, local, and on-disk-matches-manifest) merged into one loop over SKILL_RUNTIMES x [global, local]: 44 spawns -> 22 - the --minimal manifest-mode/backcompat triple-install -> one shared, memoized install via sharedMinimalManifestInstall() - .sh hooks existence checks (5 tests) -> 1, executable-bit check (its own Windows-conditional skip) left separate - Codex #4087 hook-helper tests (3) -> 1 - Windsurf #4087 hook-helper tests (2) -> 1 - pi shared-hooks-bundle tests (3 per scope) -> 1 per scope Net: ~65 real installer spawns in this file down to ~29, no assertion dropped or weakened. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-07 21:08:06 -04:00
parent 93e141a006
commit 423f38e655
5 changed files with 374 additions and 221 deletions
--- a/tests/config.test.cjs
+++ b/tests/config.test.cjs
@@ -964,6 +964,134 @@ describe('config-set <key> 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 <key> 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', () => {