fix: address branching strategy review findings

.changeset/sunny-pandas-dance.md

@@ -1,5 +1,5 @@
 ---
 type: Fixed
-pr: 3523
+pr: 3527
 ---
 **Top-level `branching_strategy` in `.planning/config.json` no longer triggers a false "unknown config key" warning** — `loadConfig` in `core.cjs` actively read the key via its nested fallback (`git.branching_strategy`) but the `KNOWN_TOP_LEVEL` allowlist was built from dot-notation paths via `.split('.')[0]`, turning `'git.branching_strategy'` into `'git'` instead of `'branching_strategy'`. The fix adds a self-healing on-disk migration (mirroring the existing `multiRepo → planning.sub_repos` precedent): on first `loadConfig`, the top-level key is grafted into `git.branching_strategy` and the stale top-level entry is removed. A module-level deduplication Set also prevents the same unknown-key warning from appearing twice when `loadConfig` is invoked multiple times in a single CLI call. A contract test asserts CJS and SDK agree on legacy-shape fixtures. (#3523)

get-shit-done/bin/lib/core.cjs

@@ -348,6 +348,14 @@ function loadConfig(cwd, options = {}) {
       const raw = platformReadSync(rootConfigPath);
       if (raw === null) throw new Error('missing');
       rootParsed = JSON.parse(raw);
+      if (Object.prototype.hasOwnProperty.call(rootParsed, 'branching_strategy')) {
+        if (!rootParsed.git) rootParsed.git = {};
+        if (rootParsed.git.branching_strategy === undefined) {
+          rootParsed.git.branching_strategy = rootParsed.branching_strategy;
+        }
+        delete rootParsed.branching_strategy;
+        try { platformWriteSync(rootConfigPath, JSON.stringify(rootParsed, null, 2)); } catch {}
+      }
     } catch {
       // Root config missing or unparseable — workstream config stands alone
     }
@@ -407,7 +415,7 @@ function loadConfig(cwd, options = {}) {
     // The nested value wins if already set (matches SDK mergeDefaults precedence, PR #3116).
     if (Object.prototype.hasOwnProperty.call(fileData, 'branching_strategy')) {
       if (!fileData.git) fileData.git = {};
-      if (!fileData.git.branching_strategy) {
+      if (fileData.git.branching_strategy === undefined) {
         fileData.git.branching_strategy = fileData.branching_strategy;
       }
       delete fileData.branching_strategy;

tests/bug-3523-cjs-loadconfig-branching-strategy-warning.test.cjs

@@ -57,11 +57,11 @@ const TEST_ENV_BASE = {
  * Run gsd-tools and return { stdout, stderr, status }.
  * Always captures stderr even when exit code is 0.
  */
-function runWithStderr(args, cwd) {
+function runWithStderr(args, cwd, env = {}) {
   const result = spawnSync(process.execPath, [TOOLS_PATH, ...args], {
     cwd,
     encoding: 'utf-8',
-    env: { ...process.env, ...TEST_ENV_BASE },
+    env: { ...process.env, ...TEST_ENV_BASE, ...env },
   });
   return {
     stdout: result.stdout || '',
@@ -126,6 +126,11 @@ describe('bug-3523 — no warning for legacy top-level branching_strategy', () =
     // After migration write-back, config-get should find git.branching_strategy.
     const result = runWithStderr(['config-get', 'git.branching_strategy'], tmpDir);
 
+    assert.equal(
+      result.status,
+      0,
+      `config-get command must succeed — exit status ${result.status}, stderr: ${result.stderr}`
+    );
     assert.equal(
       result.stderr.trim(),
       '',
@@ -152,7 +157,7 @@ describe('bug-3523 — double-emission reduced to single-emission', () => {
     // Use a key that IS genuinely unknown (not branching_strategy, which is now
     // fixed) to verify the deduplication guard works for other keys too.
     // We verify that the count of warning lines for a single unknown key is
-    // at most 1 — not 2 — even if loadConfig is invoked twice internally.
+    // exactly once — not zero and not two — even if loadConfig is invoked twice internally.
     tmpDir = createTempProject('gsd-3523-dedup-');
     const configPath = path.join(tmpDir, '.planning', 'config.json');
     fs.writeFileSync(
@@ -171,9 +176,10 @@ describe('bug-3523 — double-emission reduced to single-emission', () => {
       .split('\n')
       .filter(l => l.includes('__gsd3523_dedup_sentinel__'));
 
-    assert.ok(
-      warningLines.length <= 1,
-      `Unknown-key warning must appear at most once per process invocation — ` +
+    assert.equal(
+      warningLines.length,
+      1,
+      `Unknown-key warning must appear exactly once per process invocation — ` +
       `appeared ${warningLines.length} times. stderr:\n${result.stderr}`
     );
   });
@@ -251,6 +257,64 @@ describe('bug-3523 — option 3 on-disk migration of branching_strategy', () =>
       'top-level branching_strategy should be removed even when git.branching_strategy already set'
     );
   });
+
+  test('workstream load also self-heals legacy root branching_strategy', () => {
+    tmpDir = createTempProject('gsd-3523-workstream-root-');
+    const rootConfigPath = path.join(tmpDir, '.planning', 'config.json');
+    const workstreamDir = path.join(tmpDir, '.planning', 'workstreams', 'alpha');
+    fs.mkdirSync(workstreamDir, { recursive: true });
+    fs.writeFileSync(
+      rootConfigPath,
+      JSON.stringify({
+        branching_strategy: 'phase',
+        git: { base_branch: 'main' },
+      }, null, 2),
+      'utf-8'
+    );
+    fs.writeFileSync(
+      path.join(workstreamDir, 'config.json'),
+      JSON.stringify({ workflow: { tdd: true } }, null, 2),
+      'utf-8'
+    );
+
+    const triggerResult = runWithStderr(['resolve-model', 'planner'], tmpDir, {
+      GSD_WORKSTREAM: 'alpha',
+    });
+
+    assert.equal(
+      triggerResult.status,
+      0,
+      `workstream load command must succeed — exit status ${triggerResult.status}, stderr: ${triggerResult.stderr}`
+    );
+    assert.equal(
+      triggerResult.stderr.trim(),
+      '',
+      `No warning should fire while migrating root config for a workstream — got: ${triggerResult.stderr}`
+    );
+
+    const onDisk = JSON.parse(fs.readFileSync(rootConfigPath, 'utf-8'));
+    assert.equal(
+      onDisk.git?.branching_strategy,
+      'phase',
+      'Expected root config.json to persist git.branching_strategy after workstream load'
+    );
+    assert.equal(
+      onDisk.branching_strategy,
+      undefined,
+      'Expected root config.json to remove top-level branching_strategy after workstream load'
+    );
+
+    const rootResult = runWithStderr(['config-get', 'git.branching_strategy'], tmpDir);
+    assert.equal(
+      rootResult.status,
+      0,
+      `root config-get command must succeed after workstream migration — exit status ${rootResult.status}, stderr: ${rootResult.stderr}`
+    );
+    assert.ok(
+      rootResult.stdout.includes('phase'),
+      `Expected migrated root git.branching_strategy to be 'phase' but got: ${rootResult.stdout}`
+    );
+  });
 });
 
 // ─── Test 4: CJS↔SDK contract parity ────────────────────────────────────────
@@ -301,6 +365,11 @@ describe('bug-3523 — CJS↔SDK contract: both agree on legacy branching_strate
     // matching what the SDK's mergeDefaults would compute.
     const result = runWithStderr(['config-get', 'git.branching_strategy'], tmpDir);
 
+    assert.equal(
+      result.status,
+      0,
+      `config-get command must succeed — exit status ${result.status}, stderr: ${result.stderr}`
+    );
     assert.equal(
       result.stderr.trim(),
       '',