fix(#2638): write sub_repos to canonical planning.sub_repos (#2668)

loadConfig's multiRepo migration and filesystem-sync writers targeted the
top-level parsed.sub_repos, but KNOWN_TOP_LEVEL (the unknown-key validator's
allowlist) only recognizes planning.sub_repos (canonical per #2561). Each
migration/sync therefore persisted a key the next loadConfig call warned was
unknown.

Redirect both writers to parsed.planning.sub_repos, ensuring parsed.planning
is initialized first. Also self-heal legacy/buggy installs by stripping any
stale top-level sub_repos on load, preserving its value as the
planning.sub_repos seed if that slot is empty.

Tests cover: (a) canonical planning.sub_repos emits no warning, (b) multiRepo
migration writes to planning.sub_repos with no top-level residue,
(c) filesystem sync relocates to planning.sub_repos, (d) stale top-level
sub_repos from older buggy installs is stripped on load.

Closes #2638

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

@@ -288,26 +288,40 @@ function loadConfig(cwd) {
     // Auto-detect and sync sub_repos: scan for child directories with .git
     let configDirty = false;
 
-    // Migrate legacy "multiRepo: true" boolean → sub_repos array
+    // Migrate legacy "multiRepo: true" boolean → planning.sub_repos array.
+    // Canonical location is planning.sub_repos (#2561); writing to top-level
+    // would be flagged as unknown by the validator below (#2638).
     if (parsed.multiRepo === true && !parsed.sub_repos && !parsed.planning?.sub_repos) {
       const detected = detectSubRepos(cwd);
       if (detected.length > 0) {
-        parsed.sub_repos = detected;
         if (!parsed.planning) parsed.planning = {};
+        parsed.planning.sub_repos = detected;
         parsed.planning.commit_docs = false;
         delete parsed.multiRepo;
         configDirty = true;
       }
     }
 
-    // Keep sub_repos in sync with actual filesystem
-    const currentSubRepos = parsed.sub_repos || parsed.planning?.sub_repos || [];
+    // Self-heal legacy/buggy installs: strip any stale top-level sub_repos,
+    // preserving its value as the planning.sub_repos seed if that slot is empty.
+    if (Object.prototype.hasOwnProperty.call(parsed, 'sub_repos')) {
+      if (!parsed.planning) parsed.planning = {};
+      if (!parsed.planning.sub_repos) {
+        parsed.planning.sub_repos = parsed.sub_repos;
+      }
+      delete parsed.sub_repos;
+      configDirty = true;
+    }
+
+    // Keep planning.sub_repos in sync with actual filesystem
+    const currentSubRepos = parsed.planning?.sub_repos || [];
     if (Array.isArray(currentSubRepos) && currentSubRepos.length > 0) {
       const detected = detectSubRepos(cwd);
       if (detected.length > 0) {
         const sorted = [...currentSubRepos].sort();
         if (JSON.stringify(sorted) !== JSON.stringify(detected)) {
-          parsed.sub_repos = detected;
+          if (!parsed.planning) parsed.planning = {};
+          parsed.planning.sub_repos = detected;
           configDirty = true;
         }
       }

tests/bug-2638-sub-repos-canonical-location.test.cjs

@@ -0,0 +1,143 @@
+/**
+ * Regression test for bug #2638.
+ *
+ * loadConfig previously migrated/synced sub_repos to the TOP-LEVEL
+ * `parsed.sub_repos`, but the KNOWN_TOP_LEVEL allowlist only recognizes
+ * `planning.sub_repos` (per #2561 — canonical location). That asymmetry
+ * made loadConfig write a key it then warns is unknown on the next read.
+ *
+ * Fix: writers target `parsed.planning.sub_repos` and strip any stale
+ * top-level copy during the same migration pass.
+ */
+
+const { test, describe, beforeEach, afterEach } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('fs');
+const path = require('path');
+const { execFileSync } = require('child_process');
+const { createTempProject, cleanup } = require('./helpers.cjs');
+
+const { loadConfig } = require('../get-shit-done/bin/lib/core.cjs');
+
+function makeSubRepo(parent, name) {
+  const dir = path.join(parent, name);
+  fs.mkdirSync(dir, { recursive: true });
+  execFileSync('git', ['init'], { cwd: dir, stdio: 'pipe' });
+}
+
+function readConfig(tmpDir) {
+  return JSON.parse(
+    fs.readFileSync(path.join(tmpDir, '.planning', 'config.json'), 'utf-8')
+  );
+}
+
+function writeConfig(tmpDir, obj) {
+  fs.writeFileSync(
+    path.join(tmpDir, '.planning', 'config.json'),
+    JSON.stringify(obj, null, 2)
+  );
+}
+
+describe('bug #2638 — sub_repos canonical location', () => {
+  let tmpDir;
+  let originalCwd;
+  let stderrCapture;
+  let origStderrWrite;
+
+  beforeEach(() => {
+    tmpDir = createTempProject();
+    originalCwd = process.cwd();
+    stderrCapture = '';
+    origStderrWrite = process.stderr.write;
+    process.stderr.write = (chunk) => { stderrCapture += chunk; return true; };
+  });
+
+  afterEach(() => {
+    process.stderr.write = origStderrWrite;
+    process.chdir(originalCwd);
+    cleanup(tmpDir);
+  });
+
+  test('does not warn when planning.sub_repos is set (no top-level sub_repos)', () => {
+    makeSubRepo(tmpDir, 'backend');
+    makeSubRepo(tmpDir, 'frontend');
+    writeConfig(tmpDir, {
+      planning: { sub_repos: ['backend', 'frontend'] },
+    });
+
+    loadConfig(tmpDir);
+
+    assert.ok(
+      !stderrCapture.includes('unknown config key'),
+      `should not warn for planning.sub_repos, got: ${stderrCapture}`
+    );
+    assert.ok(
+      !stderrCapture.includes('sub_repos'),
+      `should not mention sub_repos at all, got: ${stderrCapture}`
+    );
+  });
+
+  test('migrates legacy multiRepo:true into planning.sub_repos (not top-level)', () => {
+    makeSubRepo(tmpDir, 'backend');
+    makeSubRepo(tmpDir, 'frontend');
+    writeConfig(tmpDir, { multiRepo: true });
+
+    loadConfig(tmpDir);
+
+    const after = readConfig(tmpDir);
+    assert.deepStrictEqual(
+      after.planning?.sub_repos,
+      ['backend', 'frontend'],
+      'migration should write to planning.sub_repos'
+    );
+    assert.strictEqual(
+      Object.prototype.hasOwnProperty.call(after, 'sub_repos'),
+      false,
+      'migration must not leave a top-level sub_repos key'
+    );
+    assert.strictEqual(after.multiRepo, undefined, 'legacy multiRepo should be removed');
+
+    assert.ok(
+      !stderrCapture.includes('unknown config key'),
+      `post-migration read should not warn, got: ${stderrCapture}`
+    );
+  });
+
+  test('filesystem sync writes detected list to planning.sub_repos only', () => {
+    makeSubRepo(tmpDir, 'api');
+    makeSubRepo(tmpDir, 'web');
+    writeConfig(tmpDir, { planning: { sub_repos: ['api'] } });
+
+    loadConfig(tmpDir);
+
+    const after = readConfig(tmpDir);
+    assert.deepStrictEqual(after.planning?.sub_repos, ['api', 'web']);
+    assert.strictEqual(
+      Object.prototype.hasOwnProperty.call(after, 'sub_repos'),
+      false,
+      'sync must not create a top-level sub_repos key'
+    );
+    assert.ok(
+      !stderrCapture.includes('unknown config key'),
+      `sync should not produce unknown-key warning, got: ${stderrCapture}`
+    );
+  });
+
+  test('stale top-level sub_repos is stripped on load', () => {
+    makeSubRepo(tmpDir, 'backend');
+    writeConfig(tmpDir, {
+      sub_repos: ['backend'],
+      planning: { sub_repos: ['backend'] },
+    });
+
+    loadConfig(tmpDir);
+
+    const after = readConfig(tmpDir);
+    assert.strictEqual(
+      Object.prototype.hasOwnProperty.call(after, 'sub_repos'),
+      false,
+      'stale top-level sub_repos should be removed to self-heal legacy installs'
+    );
+    assert.deepStrictEqual(after.planning?.sub_repos, ['backend']);
+  });
+});

tests/core.test.cjs

@@ -1519,9 +1519,10 @@ describe('loadConfig sub_repos auto-sync', () => {
     assert.deepStrictEqual(config.sub_repos, ['backend', 'frontend']);
     assert.strictEqual(config.commit_docs, false);
 
-    // Verify config was persisted
+    // Verify config was persisted to the canonical location (planning.sub_repos per #2561/#2638)
     const saved = JSON.parse(fs.readFileSync(path.join(projectRoot, '.planning', 'config.json'), 'utf-8'));
-    assert.deepStrictEqual(saved.sub_repos, ['backend', 'frontend']);
+    assert.deepStrictEqual(saved.planning?.sub_repos, ['backend', 'frontend']);
+    assert.strictEqual(saved.sub_repos, undefined, 'top-level sub_repos should not be written (#2638)');
     assert.strictEqual(saved.multiRepo, undefined, 'multiRepo should be removed');
   });