perf(#315): memoize subrepo detection within loadConfig (3 scans → 1) (#398)

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) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-05-27 20:21:50 -04:00
committed by GitHub
parent 4361d83279
commit a393e28b34
3 changed files with 137 additions and 3 deletions

View File

@@ -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.

View File

@@ -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)) {

View File

@@ -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;
}
});
});