From 951d5bf7c005729c67319db1cc7dedd4d09311d2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Apr 2026 10:49:13 -0400 Subject: [PATCH] fix(#2893): surface non-canonical plan filenames instead of silently returning zero plans (#2896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2893): surface non-canonical plan filenames instead of silently returning zero plans Reporter saw `plan_count: 0` from `/gsd:execute-phase` even though five plan files existed on disk. Investigation showed the planner had written files like `01-PLAN-01-foundation.md`, while `phase-plan-index`'s strict filter (`f.endsWith('-PLAN.md') || f === 'PLAN.md'`) rejected them silently — collapsing two distinct states into the same `plans: []` return: - directory truly has no plans (legit empty) - directory has plans but the filter rejected them (user/agent error) The canonical contract is documented in three places: - `agents/gsd-planner.md` write_phase_prompt step (lines 1063-1080) - `commands/gsd/plan-phase.md` - `references/universal-anti-patterns.md` (rule 26) It mandates `{padded_phase}-{NN}-PLAN.md` and explicitly forbids `PLAN-NN.md` / `01-PLAN-01.md` / `plan-NN.md` etc. The strict filter is correct per that contract. The bug is that the executor never tells the user when the contract was violated — they just see `plan_count: 0` with no signal. Fix: add a diagnostic helper `describeNonCanonicalPlans()` that scans the phase directory for files matching `*PLAN*.md` (the diagnostic net) that the canonical filter rejected, excluding legit derivatives like `*-PLAN-OUTLINE.md` and `*-PLAN.pre-bounce.md`. When offenders exist, return a `warning` field naming each one and citing the canonical pattern so the user knows what to rename to. Wired into the three filter sites: - `phase-plan-index` (the executor's main entry point) - `phases list --type plans` - `find-phase` The strict filter itself is unchanged — existing canonical plans behave identically. This is purely a diagnostic that converts silent-empty into loud-with-actionable-error. Tests: - `phase-plan-index returns warning for reporter's exact filename pattern (`01-PLAN-01-foundation.md`)` - `truly empty dir does not emit a warning` - `canonical plans + outline + pre-bounce files do not emit a warning` Closes #2893 * test(#2893): add parity tests for find-phase and phases list --type plans warnings CodeRabbit's only finding on the prior commit: I wired the warning into three filter sites (`phase-plan-index`, `find-phase`, `phases list --type plans`) but only `phase-plan-index` had test coverage for the warning shape. The other two paths could silently diverge during future refactors — exactly the silent-drift class of bug this fix exists to prevent. Add four parity tests mirroring the existing two: - find-phase: non-canonical filenames produce a warning naming each offender + citing the canonical pattern. - find-phase: canonical plan + derivative files (PLAN-OUTLINE, pre-bounce) produce no warning. - phases list --type plans: same non-canonical case, but assert the warning is prefixed with `${dir}: ` (this path aggregates across phase directories so each offender is tagged with its dir). - phases list --type plans: canonical case, no warning. `node --test tests/phase.test.cjs`: 98/98 pass (was 94, +4 new). --- get-shit-done/bin/lib/phase.cjs | 60 +++++++++++++- tests/phase.test.cjs | 133 ++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 3 deletions(-) diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index b656a7bc5..3d6fab9e5 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -8,6 +8,47 @@ const { escapeRegex, loadConfig, normalizePhaseName, comparePhaseNum, findPhaseI const { extractFrontmatter } = require('./frontmatter.cjs'); const { writeStateMd, readModifyWriteStateMd, stateExtractField, stateReplaceField, stateReplaceFieldWithFallback, updatePerformanceMetricsSection } = require('./state.cjs'); +// #2893 — strict canonical filter: `{padded_phase}-{NN}-PLAN.md` or `PLAN.md`. +// Documented in agents/gsd-planner.md (write_phase_prompt step). The wider +// "looks like a plan but isn't canonical" probe below is used to surface a +// loud warning instead of silently returning zero plans. +const isCanonicalPlanFile = (f) => f.endsWith('-PLAN.md') || f === 'PLAN.md'; + +// Any .md file with PLAN anywhere in the basename — the diagnostic net for +// catching agent deviations like `01-PLAN-01-foundation.md` (#2893). +// Excludes derivative files (`-PLAN-OUTLINE.md`, `*.pre-bounce.md`, etc.) that +// the planner legitimately produces alongside canonical plans. +const PLAN_OUTLINE_RE = /-PLAN-OUTLINE\.md$/i; +const PLAN_PRE_BOUNCE_RE = /-PLAN.*\.pre-bounce\.md$/i; +const looksLikePlanFile = (f) => + /\.md$/i.test(f) + && /PLAN/i.test(f) + && !PLAN_OUTLINE_RE.test(f) + && !PLAN_PRE_BOUNCE_RE.test(f); + +/** + * Detect plan-shaped files that the canonical filter would reject. Returns + * a warning string when offenders exist, else null. Centralised so every + * read site (phase-plan-index, phases list --type plans, find-phase) emits + * the same message. + * + * @param {string[]} dirFiles — readdirSync output for one phase directory + * @param {string[]} matchedFiles — what the canonical filter accepted + * @returns {string|null} + */ +function describeNonCanonicalPlans(dirFiles, matchedFiles) { + const matched = new Set(matchedFiles); + const offenders = dirFiles.filter((f) => looksLikePlanFile(f) && !matched.has(f)); + if (offenders.length === 0) return null; + return ( + `Found ${offenders.length} plan-shaped file(s) in this phase that don't match the canonical ` + + `naming convention "{padded_phase}-{NN}-PLAN.md" (or bare "PLAN.md") and were skipped: ` + + offenders.map((f) => `"${f}"`).join(', ') + + `. Rename to the canonical form (e.g. "01-01-PLAN.md") so the executor can detect them. ` + + `See agents/gsd-planner.md write_phase_prompt step for the full contract.` + ); +} + function cmdPhasesList(cwd, options, raw) { const phasesDir = path.join(planningDir(cwd), 'phases'); const { type, phase, includeArchived } = options; @@ -52,13 +93,18 @@ function cmdPhasesList(cwd, options, raw) { // If listing files of a specific type if (type) { const files = []; + const warnings = []; for (const dir of dirs) { const dirPath = path.join(phasesDir, dir); const dirFiles = fs.readdirSync(dirPath); let filtered; if (type === 'plans') { - filtered = dirFiles.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md'); + filtered = dirFiles.filter(isCanonicalPlanFile); + // #2893 — surface plan-shaped files the canonical filter rejected + // so callers (executor init, etc.) don't silently see zero plans. + const w = describeNonCanonicalPlans(dirFiles, filtered); + if (w) warnings.push(`${dir}: ${w}`); } else if (type === 'summaries') { filtered = dirFiles.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); } else { @@ -73,6 +119,7 @@ function cmdPhasesList(cwd, options, raw) { count: files.length, phase_dir: phase ? dirs[0].replace(/^\d+(?:\.\d+)*-?/, '') : null, }; + if (warnings.length) result.warning = warnings.join(' | '); output(result, raw, files.join('\n')); return; } @@ -176,8 +223,10 @@ function cmdFindPhase(cwd, phase, raw) { const phaseDir = path.join(phasesDir, match); const phaseFiles = fs.readdirSync(phaseDir); - const plans = phaseFiles.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md').sort(); + const plans = phaseFiles.filter(isCanonicalPlanFile).sort(); const summaries = phaseFiles.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md').sort(); + // #2893 — same diagnostic as phase-plan-index for consistency. + const planNamingWarning = describeNonCanonicalPlans(phaseFiles, plans); const result = { found: true, @@ -187,6 +236,7 @@ function cmdFindPhase(cwd, phase, raw) { plans, summaries, }; + if (planNamingWarning) result.warning = planNamingWarning; output(result, raw, result.directory); } catch { @@ -229,8 +279,11 @@ function cmdPhasePlanIndex(cwd, phase, raw) { // Get all files in phase directory const phaseFiles = fs.readdirSync(phaseDir); - const planFiles = phaseFiles.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md').sort(); + const planFiles = phaseFiles.filter(isCanonicalPlanFile).sort(); const summaryFiles = phaseFiles.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); + // #2893 — surface plan-shaped files the canonical filter rejected so a + // misnamed plan never silently produces plan_count: 0 at executor init. + const planNamingWarning = describeNonCanonicalPlans(phaseFiles, planFiles); // Build set of plan IDs with summaries const completedPlanIds = new Set( @@ -305,6 +358,7 @@ function cmdPhasePlanIndex(cwd, phase, raw) { incomplete, has_checkpoints: hasCheckpoints, }; + if (planNamingWarning) result.warning = planNamingWarning; output(result, raw); } diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 0b4d500a5..115721b1a 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -222,6 +222,139 @@ describe('phase-plan-index command', () => { assert.deepStrictEqual(output.waves, {}, 'waves should be empty'); assert.deepStrictEqual(output.incomplete, [], 'incomplete should be empty'); assert.strictEqual(output.has_checkpoints, false, 'no checkpoints'); + assert.ok(output.warning === undefined, 'truly empty dir must not emit a warning'); + }); + + // #2893 — when the planner produces filenames that don't match the canonical + // `{padded_phase}-{NN}-PLAN.md` contract, the executor used to silently see + // plan_count: 0 with no signal. Now the response must include a `warning` + // field naming every offender, so the user gets an actionable error instead + // of "execute-phase blocked, no clue why". + test('non-canonical plan filenames surface a warning naming each offender (#2893)', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + + // The reporter's exact symptom: planner wrote `{phase-id}-PLAN-{N}-{slug}.md`. + fs.writeFileSync(path.join(phaseDir, '01-PLAN-01-foundation.md'), '---\n---\n'); + fs.writeFileSync(path.join(phaseDir, '01-PLAN-02-api.md'), '---\n---\n'); + + const result = runGsdTools('phase-plan-index 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.plans.length, 0, 'non-canonical files are not silently accepted'); + assert.ok(typeof output.warning === 'string', 'warning field must be present'); + assert.ok(output.warning.includes('01-PLAN-01-foundation.md'), 'warning names the first offender'); + assert.ok(output.warning.includes('01-PLAN-02-api.md'), 'warning names the second offender'); + assert.ok( + output.warning.includes('{padded_phase}-{NN}-PLAN.md'), + 'warning cites the canonical pattern so user knows what to rename to', + ); + }); + + test('canonical plans suppress the warning even alongside derivative files (#2893)', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + + // Canonical plan + the legitimate derivative artifacts the planner emits. + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '---\nwave: 1\n---\n'); + fs.writeFileSync(path.join(phaseDir, '03-PLAN-OUTLINE.md'), '# outline\n'); + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.pre-bounce.md'), '---\n---\n'); + + const result = runGsdTools('phase-plan-index 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.plans.length, 1, 'canonical plan detected'); + assert.ok( + output.warning === undefined, + `outline and pre-bounce files must not trigger the warning, got: ${output.warning}`, + ); + }); + + // #2893 parity — find-phase reads the same phase directory and applies the + // same canonical filter, so it must emit the same warning shape. Without + // these tests the two code paths could silently diverge. + test('find-phase: non-canonical plan filenames surface the same warning (#2893 parity)', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-PLAN-01-foundation.md'), '---\n---\n'); + fs.writeFileSync(path.join(phaseDir, '01-PLAN-02-api.md'), '---\n---\n'); + + const result = runGsdTools('find-phase 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.found, true, 'phase directory found'); + assert.deepStrictEqual(output.plans, [], 'non-canonical files are not silently accepted'); + assert.ok(typeof output.warning === 'string', 'warning field must be present'); + assert.ok(output.warning.includes('01-PLAN-01-foundation.md'), 'warning names the first offender'); + assert.ok(output.warning.includes('01-PLAN-02-api.md'), 'warning names the second offender'); + assert.ok( + output.warning.includes('{padded_phase}-{NN}-PLAN.md'), + 'warning cites the canonical pattern', + ); + }); + + test('find-phase: canonical plans + derivatives suppress the warning (#2893 parity)', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '---\nwave: 1\n---\n'); + fs.writeFileSync(path.join(phaseDir, '03-PLAN-OUTLINE.md'), '# outline\n'); + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.pre-bounce.md'), '---\n---\n'); + + const result = runGsdTools('find-phase 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.deepStrictEqual(output.plans, ['03-01-PLAN.md'], 'canonical plan detected'); + assert.ok( + output.warning === undefined, + `outline and pre-bounce files must not trigger the warning, got: ${output.warning}`, + ); + }); + + // #2893 parity — `phases list --type plans` aggregates across phase dirs + // and prefixes each warning with `${dir}: ` so the user can locate the + // offending phase. Test mirrors the find-phase pair but accounts for that + // prefix in the assertion. + test('phases list --type plans: non-canonical filenames surface a per-dir warning (#2893 parity)', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '01-PLAN-01-foundation.md'), '---\n---\n'); + fs.writeFileSync(path.join(phaseDir, '01-PLAN-02-api.md'), '---\n---\n'); + + const result = runGsdTools('phases list --type plans --phase 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.deepStrictEqual(output.files, [], 'non-canonical files are not silently accepted'); + assert.ok(typeof output.warning === 'string', 'warning field must be present'); + assert.ok(output.warning.includes('03-api:'), 'warning is prefixed with the offending phase dir'); + assert.ok(output.warning.includes('01-PLAN-01-foundation.md'), 'warning names the first offender'); + assert.ok(output.warning.includes('01-PLAN-02-api.md'), 'warning names the second offender'); + assert.ok( + output.warning.includes('{padded_phase}-{NN}-PLAN.md'), + 'warning cites the canonical pattern', + ); + }); + + test('phases list --type plans: canonical plans suppress the warning (#2893 parity)', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '---\nwave: 1\n---\n'); + fs.writeFileSync(path.join(phaseDir, '03-PLAN-OUTLINE.md'), '# outline\n'); + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.pre-bounce.md'), '---\n---\n'); + + const result = runGsdTools('phases list --type plans --phase 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.deepStrictEqual(output.files, ['03-01-PLAN.md'], 'canonical plan detected'); + assert.ok( + output.warning === undefined, + `outline and pre-bounce files must not trigger the warning, got: ${output.warning}`, + ); }); test('extracts single plan with frontmatter', () => {