refactor(query): deepen phase lifecycle seams (#3267)

* refactor(query): extract phase lifecycle policy module

* refactor(query): extract phase fs and roadmap mutation adapters

* fix(sdk): propagate non-ENOENT readdir errors in phase-filesystem-adapter (CR finding 1)

Swallow only ENOENT in listDirectories; rethrow EACCES, EIO, and other
unexpected errors so callers surface real failures rather than silently
treating a permission-denied phases dir as empty.

Also adds regression test: EACCES from readdir now propagates as thrown
error instead of returning [].

* fix(sdk): propagate non-ENOENT readFile errors in phase-roadmap-mutation (CR finding 4)

readModifyWriteRoadmapMd now falls back to empty content only on ENOENT;
EACCES, EIO, and other errors are rethrown so a subsequent write cannot
clobber real roadmap content that is temporarily unreadable.

Regression tests: EACCES propagates; absent ROADMAP.md still starts empty.

* fix(sdk): omit Depends on: Phase 0 for first sequential phase; align prefix grammar (CR findings 2+3)

Finding 2: buildPhaseRoadmapEntry now omits the "Depends on" line when
phaseId == 1 (prevPhase would be 0, which is not a valid predecessor).
The guard is `prevPhase < 1` so future phase-0 configs are also safe.

Finding 3: collectDecimalSuffixesFromDirNames regex prefix pattern
updated from `[A-Z]{1,6}` to `[A-Z][A-Z0-9]*` (case-insensitive flag
added), matching the grammar used by scanSequentialMaxPhaseFromDirs.
Prevents k014 parity drift for alphanumeric project-code prefixes longer
than six characters or containing digits.

Regression tests for both fixes included.
This commit is contained in:
Tom Boucher
2026-05-09 00:14:59 -04:00
committed by GitHub
parent d8a93ad12d
commit 65abc4fc90
6 changed files with 481 additions and 326 deletions

View File

@@ -1600,3 +1600,122 @@ describe('lifecycle handlers in registry', () => {
}
});
});
// ─── CR-3267 regression: error-propagation in listDirectories ─────────────
describe('listDirectories — CR-3267 finding 1: non-ENOENT errors propagate', () => {
it('propagates EACCES from readdir instead of returning []', async () => {
const { listDirectories } = await import('./phase-filesystem-adapter.js');
// Create a real directory then remove read permission
const dir = await mkdtemp(join(tmpdir(), 'gsd-fs-acl-'));
const inner = join(dir, 'phases');
await mkdir(inner);
try {
await import('node:fs/promises').then(m => m.chmod(inner, 0o000));
await expect(listDirectories(inner)).rejects.toThrow();
} finally {
// Restore so cleanup can delete
await import('node:fs/promises').then(m => m.chmod(inner, 0o755));
await rm(dir, { recursive: true, force: true });
}
});
it('returns [] for ENOENT (directory gone between existsSync and readdir)', async () => {
// existsSync passes, but the directory has been removed before readdir —
// the ENOENT branch must still return [].
const { listDirectories } = await import('./phase-filesystem-adapter.js');
// We can't easily race the real FS, but we can verify the function tolerates
// a path that truly does not exist (existsSync returns false → early []).
const nonExistent = join(tmpdir(), 'gsd-does-not-exist-' + Date.now());
const result = await listDirectories(nonExistent);
expect(result).toEqual([]);
});
});
// ─── CR-3267 regression: error-propagation in readModifyWriteRoadmapMd ─────
describe('readModifyWriteRoadmapMd — CR-3267 finding 4: non-ENOENT errors propagate', () => {
it('propagates EACCES on ROADMAP.md readFile instead of treating as empty', async () => {
const { readModifyWriteRoadmapMd } = await import('./phase-lifecycle.js');
const dir = await mkdtemp(join(tmpdir(), 'gsd-roadmap-acl-'));
const planningDir = join(dir, '.planning');
await mkdir(planningDir, { recursive: true });
const roadmapPath = join(planningDir, 'ROADMAP.md');
await writeFile(roadmapPath, '# Roadmap\n', 'utf-8');
try {
await import('node:fs/promises').then(m => m.chmod(roadmapPath, 0o000));
await expect(
readModifyWriteRoadmapMd(dir, (c) => c)
).rejects.toThrow();
} finally {
await import('node:fs/promises').then(m => m.chmod(roadmapPath, 0o644));
await rm(dir, { recursive: true, force: true });
}
});
it('starts with empty content when ROADMAP.md is absent (ENOENT)', async () => {
const { readModifyWriteRoadmapMd } = await import('./phase-lifecycle.js');
const dir = await mkdtemp(join(tmpdir(), 'gsd-roadmap-noent-'));
const planningDir = join(dir, '.planning');
await mkdir(planningDir, { recursive: true });
// No ROADMAP.md written — must default to '' and create it
try {
const result = await readModifyWriteRoadmapMd(dir, (c) => c + 'NEW');
expect(result).toBe('NEW');
} finally {
await rm(dir, { recursive: true, force: true });
}
});
});
// ─── CR-3267 regression: buildPhaseRoadmapEntry — no "Phase 0" dependency ──
describe('buildPhaseRoadmapEntry — CR-3267 finding 2: first sequential phase has no predecessor', () => {
it('omits Depends on line when phaseId is 1', async () => {
const { buildPhaseRoadmapEntry } = await import('./phase-lifecycle-policy.js');
const entry = buildPhaseRoadmapEntry(1, 'Bootstrap', 'sequential');
expect(entry).not.toContain('Depends on');
expect(entry).not.toContain('Phase 0');
});
it('includes Depends on line when phaseId is 2', async () => {
const { buildPhaseRoadmapEntry } = await import('./phase-lifecycle-policy.js');
const entry = buildPhaseRoadmapEntry(2, 'Second Phase', 'sequential');
expect(entry).toContain('**Depends on:** Phase 1');
});
it('omits Depends on line for custom naming mode regardless of id', async () => {
const { buildPhaseRoadmapEntry } = await import('./phase-lifecycle-policy.js');
const entry = buildPhaseRoadmapEntry('ALPHA', 'Custom', 'custom');
expect(entry).not.toContain('Depends on');
});
});
// ─── CR-3267 regression: collectDecimalSuffixesFromDirNames prefix grammar ─
describe('collectDecimalSuffixesFromDirNames — CR-3267 finding 3: alphanumeric prefixes accepted', () => {
it('matches directories with long alphanumeric project-code prefix', async () => {
const { collectDecimalSuffixesFromDirNames } = await import('./phase-lifecycle-policy.js');
// Prefix "MYAPP01" is longer than 6 chars and contains digits — was rejected before fix
const dirs = ['MYAPP01-3.1-some-work', 'MYAPP01-3.2-other-work', 'unrelated-dir'];
const result = collectDecimalSuffixesFromDirNames('3', dirs);
expect(result.has(1)).toBe(true);
expect(result.has(2)).toBe(true);
});
it('still matches directories with short uppercase-only prefix', async () => {
const { collectDecimalSuffixesFromDirNames } = await import('./phase-lifecycle-policy.js');
const dirs = ['AB-5.1-task', 'AB-5.3-other'];
const result = collectDecimalSuffixesFromDirNames('5', dirs);
expect(result.has(1)).toBe(true);
expect(result.has(3)).toBe(true);
});
it('matches directories with no prefix', async () => {
const { collectDecimalSuffixesFromDirNames } = await import('./phase-lifecycle-policy.js');
const dirs = ['3.1-plain', '3.2-also-plain'];
const result = collectDecimalSuffixesFromDirNames('3', dirs);
expect(result.has(1)).toBe(true);
expect(result.has(2)).toBe(true);
});
});