* 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).
This commit is contained in:
Tom Boucher
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
Reference in New Issue
Block a user