From 06463860e46e9a71bb354e49834056c07d241674 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 24 Apr 2026 18:05:33 -0400 Subject: [PATCH] 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 | 24 ++- ...2638-sub-repos-canonical-location.test.cjs | 143 ++++++++++++++++++ tests/core.test.cjs | 5 +- 3 files changed, 165 insertions(+), 7 deletions(-) create mode 100644 tests/bug-2638-sub-repos-canonical-location.test.cjs diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index 66ceb3e6..2334e7b4 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/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; } } diff --git a/tests/bug-2638-sub-repos-canonical-location.test.cjs b/tests/bug-2638-sub-repos-canonical-location.test.cjs new file mode 100644 index 00000000..75d2ca07 --- /dev/null +++ b/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']); + }); +}); diff --git a/tests/core.test.cjs b/tests/core.test.cjs index ef1072b3..3d232de0 100644 --- a/tests/core.test.cjs +++ b/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'); });