From a393e28b34b4176a7bc0ae70968a8db6d721e3db Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 27 May 2026 20:21:50 -0400 Subject: [PATCH] =?UTF-8?q?perf(#315):=20memoize=20subrepo=20detection=20w?= =?UTF-8?q?ithin=20loadConfig=20(3=20scans=20=E2=86=92=201)=20(#398)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadConfig called detectSubRepos(cwd) at up to 3 sites per invocation (root-config requiresFilesystem migration, workstream-config requiresFilesystem migration, and the planning.sub_repos filesystem re-sync) — all with the same cwd, yielding identical results. Introduce a per-call lazy memo (getDetectedSubRepos) so the directory scan runs at most once per loadConfig call while preserving all conditional logic. Fixes #315. Co-authored-by: Claude Opus 4.7 (1M context) --- .changeset/315-subrepo-detect-memo.md | 5 + get-shit-done/bin/lib/core.cjs | 17 ++- .../perf-315-loadconfig-subrepo-scan.test.cjs | 118 ++++++++++++++++++ 3 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 .changeset/315-subrepo-detect-memo.md create mode 100644 tests/perf-315-loadconfig-subrepo-scan.test.cjs diff --git a/.changeset/315-subrepo-detect-memo.md b/.changeset/315-subrepo-detect-memo.md new file mode 100644 index 000000000..82df456e3 --- /dev/null +++ b/.changeset/315-subrepo-detect-memo.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 315 +--- +`loadConfig` now memoizes `detectSubRepos(cwd)` per call, collapsing up to 3 redundant directory scans into 1 when migrations and filesystem re-sync both trigger. diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index e71a99115..6bab0a356 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -287,6 +287,17 @@ function loadConfig(cwd, options = {}) { // can inherit from it. This prevents users from duplicating model_overrides, // workflow.*, etc. across every workstream config (#2714). const ws = activeWorkstream; + // #315 — per-call lazy memo: all three detection sites inside this loadConfig + // call operate on the same cwd and the subrepo set cannot change mid-call, so + // a single scan is sufficient. The memo is scoped to THIS call (not module-level) + // so separate loadConfig invocations each get a fresh scan. + let cachedSubRepos; + const getDetectedSubRepos = () => { + if (cachedSubRepos === undefined) cachedSubRepos = detectSubRepos(cwd); + // Return a copy: original detectSubRepos returned a fresh array per call, + // so each site must keep an independent array (avoid cross-site aliasing). + return cachedSubRepos.slice(); + }; let rootParsed = null; if (ws) { const rootConfigPath = path.join(planningRoot(cwd), 'config.json'); @@ -302,7 +313,7 @@ function loadConfig(cwd, options = {}) { // Resolve filesystem-dependent normalizations (multiRepo → planning.sub_repos) for (const norm of rootNorms) { if (norm.requiresFilesystem && !rootNormalized.planning?.sub_repos) { - const detected = detectSubRepos(cwd); + const detected = getDetectedSubRepos(); if (detected.length > 0) { if (!rootNormalized.planning) rootNormalized.planning = {}; rootNormalized.planning.sub_repos = detected; @@ -350,7 +361,7 @@ function loadConfig(cwd, options = {}) { // AND the original file didn't have sub_repos already (preserve existing intent). for (const norm of normalizations) { if (norm.requiresFilesystem && !fileData.planning?.sub_repos) { - const detected = detectSubRepos(cwd); + const detected = getDetectedSubRepos(); if (detected.length > 0) { if (!fileData.planning) fileData.planning = {}; fileData.planning.sub_repos = detected; @@ -364,7 +375,7 @@ function loadConfig(cwd, options = {}) { // Keep planning.sub_repos in sync with actual filesystem const currentSubRepos = fileData.planning?.sub_repos || []; if (Array.isArray(currentSubRepos) && currentSubRepos.length > 0) { - const detected = detectSubRepos(cwd); + const detected = getDetectedSubRepos(); if (detected.length > 0) { const sorted = [...currentSubRepos].sort(); if (JSON.stringify(sorted) !== JSON.stringify(detected)) { diff --git a/tests/perf-315-loadconfig-subrepo-scan.test.cjs b/tests/perf-315-loadconfig-subrepo-scan.test.cjs new file mode 100644 index 000000000..e212e3595 --- /dev/null +++ b/tests/perf-315-loadconfig-subrepo-scan.test.cjs @@ -0,0 +1,118 @@ +'use strict'; + +/** + * Regression test for #315 — perf: repeated subrepo detection in loadConfig. + * + * Before the fix, loadConfig called detectSubRepos(cwd) up to 3 times per + * invocation (all with the same cwd): + * Site 1 (~line 305): root-config requiresFilesystem migration (workstream path) + * Site 2 (~line 353): workstream-config requiresFilesystem migration + * Site 3 (~line 367): planning.sub_repos filesystem re-sync + * + * After the fix, a per-call lazy memo ensures detectSubRepos(cwd) is called + * EXACTLY once regardless of how many sites trigger. + * + * Fixture triggers: + * - options.workstream set → loads root config (site 1 candidate) + * - root config has `multiRepo: true` → requiresFilesystem → site 1 fires + * - workstream config has `planning.sub_repos: [...]` → site 3 fires + * - workstream config also has `multiRepo: true` → requiresFilesystem → site 2 fires + * Combined: 3 distinct call sites attempted; memo should collapse to 1 scan. + */ + +const { describe, test, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); + +// Import loadConfig directly (sync, no CLI subprocess needed) +const { loadConfig } = require('../get-shit-done/bin/lib/core.cjs'); + +// ─── helpers ────────────────────────────────────────────────────────────────── + +function createProjectWithSubRepo(prefix = 'gsd-315-test-') { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + + // Create a child dir that looks like a subrepo + const subRepoDir = path.join(tmpDir, 'sub-service'); + fs.mkdirSync(path.join(subRepoDir, '.git'), { recursive: true }); + + // Create .planning root layout + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases'), { recursive: true }); + + return tmpDir; +} + +function writeRootConfig(tmpDir, obj) { + const configPath = path.join(tmpDir, '.planning', 'config.json'); + fs.writeFileSync(configPath, JSON.stringify(obj, null, 2), 'utf-8'); +} + +function writeWorkstreamConfig(tmpDir, wsName, obj) { + const wsDir = path.join(tmpDir, '.planning', 'workstreams', wsName, 'phases'); + fs.mkdirSync(wsDir, { recursive: true }); + const configPath = path.join(tmpDir, '.planning', 'workstreams', wsName, 'config.json'); + fs.writeFileSync(configPath, JSON.stringify(obj, null, 2), 'utf-8'); +} + +// ─── test ───────────────────────────────────────────────────────────────────── + +describe('perf-315 — loadConfig calls detectSubRepos at most once per invocation', () => { + let tmpDir; + + afterEach(() => { + if (tmpDir) { + fs.rmSync(tmpDir, { recursive: true, force: true }); + tmpDir = null; + } + }); + + test('detectSubRepos scan count is exactly 1 regardless of how many sites trigger', () => { + tmpDir = createProjectWithSubRepo(); + + const wsName = 'test-ws'; + + // Root config: multiRepo: true triggers requiresFilesystem at site 1 + writeRootConfig(tmpDir, { multiRepo: true, model_profile: 'balanced' }); + + // Workstream config: + // - planning.sub_repos set → triggers site 3 (fs-resync) + // - multiRepo: true → triggers site 2 (requiresFilesystem normalization) + writeWorkstreamConfig(tmpDir, wsName, { + multiRepo: true, + planning: { sub_repos: ['sub-service'] }, + model_profile: 'balanced', + }); + + // Spy on fs.readdirSync — count only calls that match detectSubRepos' signature + // (first arg === tmpDir, options object containing withFileTypes: true) + let scanCount = 0; + const originalReaddirSync = fs.readdirSync; + fs.readdirSync = function spyReaddirSync(dirPath, opts) { + if (dirPath === tmpDir && opts && opts.withFileTypes === true) { + scanCount += 1; + } + return originalReaddirSync.call(this, dirPath, opts); + }; + + try { + // Load config with workstream option so all three sites can be reached + const config = loadConfig(tmpDir, { workstream: wsName }); + + // Behavior lock: sub_repos is correctly resolved + assert.ok( + Array.isArray(config.sub_repos) || Array.isArray(config.planning?.sub_repos), + 'sub_repos should be an array in the returned config' + ); + + // Performance assertion: only one scan regardless of how many sites triggered + assert.strictEqual( + scanCount, 1, + `Expected detectSubRepos to scan cwd exactly once, but fs.readdirSync was called ${scanCount} time(s) with the cwd + {withFileTypes:true} signature` + ); + } finally { + fs.readdirSync = originalReaddirSync; + } + }); +});