diff --git a/.changeset/patient-quails-purr.md b/.changeset/patient-quails-purr.md new file mode 100644 index 000000000..be0df921f --- /dev/null +++ b/.changeset/patient-quails-purr.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2953 +--- +**phase.complete no longer closes a phase while its plans are silently unexecuted** — a phase could previously close "complete" with an arbitrary number of plans missing a completion record (a confirmed incident closed a phase with 6/30 plans unexecuted, including its entire final scope). phase.complete now refuses, naming the unexecuted plans, unless they are explicitly retired via `status: superseded` frontmatter. (#2648) diff --git a/src/core-utils.cts b/src/core-utils.cts index b3ec0690c..114a96e90 100644 --- a/src/core-utils.cts +++ b/src/core-utils.cts @@ -277,20 +277,48 @@ function countMatchedSummaries(planFiles: string[], summaryFiles: string[]): num const summarySet = new Set(summaryFiles); let matched = 0; for (const plan of planFiles) { - const slashIdx = plan.lastIndexOf('/'); - const dir = slashIdx >= 0 ? plan.slice(0, slashIdx + 1) : ''; - const base = (dir ? plan.slice(dir.length) : plan).replace(/\.md$/i, ''); - const candidates: string[] = [ - dir + base.replace(/PLAN/i, 'SUMMARY') + '.md', - dir + base + '-SUMMARY.md', - ]; - const extended = base.match(/^(\d+)-PLAN-(\d+)/i); - if (extended) candidates.push(dir + extended[1] + '-' + extended[2] + '-SUMMARY.md'); - if (candidates.some((c) => summarySet.has(c))) matched++; + if (summaryCandidates(plan).some((c) => summarySet.has(c))) matched++; } return matched; } +/** + * The candidate `*-SUMMARY.md` filenames a single plan's completion record + * could take, per the three naming conventions documented above + * `countMatchedSummaries`. Extracted so `findUnsummarizedPlans` can reuse the + * exact same matching rule without duplicating it (a divergence between the + * count and the list would let a plan be counted as matched while still + * appearing in the unsummarized set, or vice versa). + */ +function summaryCandidates(plan: string): string[] { + const slashIdx = plan.lastIndexOf('/'); + const dir = slashIdx >= 0 ? plan.slice(0, slashIdx + 1) : ''; + const base = (dir ? plan.slice(dir.length) : plan).replace(/\.md$/i, ''); + const candidates: string[] = [ + dir + base.replace(/PLAN/i, 'SUMMARY') + '.md', + dir + base + '-SUMMARY.md', + ]; + const extended = base.match(/^(\d+)-PLAN-(\d+)/i); + if (extended) candidates.push(dir + extended[1] + '-' + extended[2] + '-SUMMARY.md'); + return candidates; +} + +/** + * #2648: the plan files in `planFiles` that have NO matching completion record + * in `summaryFiles`, using the identical matching rule as `countMatchedSummaries` + * (so the count and the named list can never disagree). Callers that must NAME + * the missing plans — e.g. phase.complete's fail-closed coverage gate, which + * refuses completion when any non-retired plan lacks a SUMMARY — need the list, + * not just the count. `planFiles` is expected to be already superseded-filtered + * (the caller passes `scanPhasePlans(...).planFiles`, which drops + * `status: superseded` plans), so a deliberately-retired plan never appears + * here and never blocks completion. + */ +function findUnsummarizedPlans(planFiles: string[], summaryFiles: string[]): string[] { + const summarySet = new Set(summaryFiles); + return planFiles.filter((plan) => !summaryCandidates(plan).some((c) => summarySet.has(c))); +} + export = { toPosixPath, detectSubRepos, @@ -305,4 +333,5 @@ export = { timeAgo, extractCanonicalPlanId, countMatchedSummaries, + findUnsummarizedPlans, }; diff --git a/src/io.cts b/src/io.cts index 0df3f5fd5..303fa1c87 100644 --- a/src/io.cts +++ b/src/io.cts @@ -173,6 +173,7 @@ const ERROR_REASON = Object.freeze({ // workflow / phase PHASE_NOT_FOUND: 'phase_not_found', PHASE_VERIFICATION_INCOMPLETE: 'phase_verification_incomplete', + PHASE_PLAN_COVERAGE_INCOMPLETE: 'phase_plan_coverage_incomplete', SUMMARY_NO_PLANNING: 'summary_no_planning', // graphify GRAPHIFY_NO_GRAPH: 'graphify_no_graph', diff --git a/src/phase.cts b/src/phase.cts index 9e87f0880..d283a4bcc 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -26,7 +26,7 @@ import configLoaderMod = require('./config-loader.cjs'); const { loadConfig } = configLoaderMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- core-utils.cjs is an export= CommonJS module import coreUtilsMod = require('./core-utils.cjs'); -const { toPosixPath, generateSlugInternal, readSubdirectories } = coreUtilsMod; +const { toPosixPath, generateSlugInternal, readSubdirectories, findUnsummarizedPlans } = coreUtilsMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-id.cjs is an export= CommonJS module import phaseIdMod = require('./phase-id.cjs'); const { @@ -1727,6 +1727,82 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { const warnings: string[] = []; const phaseFullDir = path.join(cwd, phaseInfo['directory'] as string); + // #2648: fail-closed plan-coverage gate. phase.complete used to gate ONLY on a + // single *-VERIFICATION.md status, so a phase could close "complete" while an + // arbitrary number of its plans — including plans a lock/recovery decision + // silently dropped — had no completion record (a confirmed production incident + // closed a phase with 6/30 plans unexecuted, including its entire final UI + // scope, with every tool-reported signal green). Now refuse completion when any + // plan lacks a matching *-SUMMARY.md, UNLESS that plan is explicitly retired + // via machine-readable `status: superseded` frontmatter (the #2349 marker). + // + // scanPhasePlans is the superseded-AWARE counter (it drops status: superseded + // plans from planFiles before returning), so a deliberately-retired plan never + // appears in the unsummarized set and never blocks completion — closing the + // Goodhart hole (delete a SUMMARY to raise the %) without regressing the + // legitimate lock/recovery pattern (retire a plan instead of executing it). + // This is evaluated BEFORE the verification-gate transaction below so a + // plan-coverage refusal fails fast without mutating ROADMAP/STATE. The count + // path (cmdPhaseComplete's own planCount/summaryCount above) is NOT superseded- + // aware (it comes from findPhaseInternal/phase-locator.cts); that is fine for + // DISPLAY (the X/Y cell) but must not be the gate — the gate needs the + // superseded-adjusted set so retired plans don't re-block the very phases the + // marker exists to unblock. Matches roadmap.cts's already-correct-but-unenforced + // `summaryCount >= planCount` predicate, now enforced at the completion seam. + const coverageScan = scanPhasePlans(phaseFullDir); + // #2648 security: fail CLOSED when the phase directory cannot be read. + // scanPhasePlans deliberately swallows readdirSync errors and returns an empty + // plan set ({planFiles: []}), which is indistinguishable from a readable empty + // phase. For a COVERAGE gate that is the wrong posture: "I could not read the + // plans" must mean "I cannot prove coverage," not "all plans are summarized" — + // otherwise any I/O failure (permissions, ENOTDIR, EBUSY on Windows, a dir + // present in ROADMAP.md but missing/unreadable on disk) silently re-opens the + // exact hole this gate exists to close. Distinguish the two: a readable + // directory with zero plans is a legitimately complete empty phase; an + // UNREADABLE directory is a fail-closed refusal. Mirrors cmdPhaseInsert's own + // readdirSync-fail-closed posture (a swallow there used to risk writing a + // colliding phase number). + try { + fs.readdirSync(phaseFullDir); + } catch (readErr) { + error( + `Phase ${phaseNum} cannot be completed: its plan directory is unreadable (${phaseInfo['directory'] as string}: ${(readErr as NodeJS.ErrnoException).code || (readErr as Error).message}), so plan coverage cannot be verified. Restore read access and retry — a coverage gate that passes when it cannot read the plans is no gate at all (#2648).`, + ERROR_REASON.PHASE_PLAN_COVERAGE_INCOMPLETE, + ); + } + const unsummarizedPlans = findUnsummarizedPlans( + coverageScan.planFiles, + coverageScan.summaryFiles, + ); + if (unsummarizedPlans.length > 0) { + // Sanitize plan filenames before interpolation: they come raw from + // readdirSync and could carry C0 control chars / DEL (a committable filename + // could spoof the terminal in plain-error mode). Strip them so the message is + // safe to print regardless of --json-errors. Path traversal sequences are not + // a code-execution vector here (printed only, never reopened from the message). + const sanitize = (name: string): string => name.replace(/[\u0000-\u001f\u007f]/g, '?'); + const listed = unsummarizedPlans.slice(0, 20).map(sanitize).join(', '); + const more = unsummarizedPlans.length > 20 ? ` (and ${unsummarizedPlans.length - 20} more)` : ''; + // Audit surface (#2648 review M1): name how many plans were excluded as + // superseded so a reviewer can see WHICH work was declared retired, not just + // that some plans are missing summaries. The status: superseded marker is a + // committable, review-time-trusted bypass; surfacing its count keeps that + // bypass visible rather than silent. + const phaseInfoPlanCount = Array.isArray(phaseInfo['plans']) ? (phaseInfo['plans'] as string[]).length : 0; + const supersededCount = + coverageScan.planFiles.length === 0 ? 0 : Math.max(0, phaseInfoPlanCount - coverageScan.planFiles.length); + const supersededNote = supersededCount > 0 + ? ` ${supersededCount} plan(s) excluded as status: superseded (retired).` + : ''; + error( + `Phase ${phaseNum} cannot be completed: ${unsummarizedPlans.length} plan(s) have no completion record (*-SUMMARY.md): ${listed}${more}.` + + supersededNote + + ` Execute the plans and write their summaries, or retire a plan with machine-readable \`status: superseded\` frontmatter (#2349) if it was deliberately dropped — a retired plan is excluded from this gate. ` + + `Completing a phase with unexecuted plans is what lost an entire promised deliverable silently (#2648).`, + ERROR_REASON.PHASE_PLAN_COVERAGE_INCOMPLETE, + ); + } + try { const phaseFiles = fs.readdirSync(phaseFullDir); diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 97d85c092..2a4ddb81d 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -2842,6 +2842,160 @@ describe('phase complete canonical verification gate (#1522)', () => { }); }); +// #2648: phase.complete used to gate only on a single *-VERIFICATION.md status, +// so a phase could close "complete" while an arbitrary number of its plans had +// no completion record (confirmed production incident: 6/30 plans unexecuted, +// including the phase's entire final UI scope, with every signal green). The +// fix adds a fail-closed plan-coverage gate that refuses completion when any +// non-retired plan lacks a *-SUMMARY.md, naming the missing plans. A plan +// retired via machine-readable `status: superseded` frontmatter (#2349) is +// excluded from the gate so the legitimate lock/recovery pattern still works. +describe('phase complete plan-coverage gate (#2648)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // Build a phase-1 directory with a configurable set of plan/summary files and + // a passed VERIFICATION (so the ONLY thing that could block completion is the + // plan-coverage gate — isolating it from the #1522 verification gate). The + // ROADMAP declares Phase 1 so completion has a phase to act on. + function writePlanCoverageFixture(tmpDir, { plans, summaries, supersededPlans = [] }) { + const planningDir = path.join(tmpDir, '.planning'); + const phase1Dir = path.join(planningDir, 'phases', '01-foundation'); + fs.mkdirSync(phase1Dir, { recursive: true }); + + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', '', + '- [ ] Phase 1: Foundation', '', + '### Phase 1: Foundation', + '**Goal:** Setup', '**Plans:** 3 plans', '', + '## Progress', '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Foundation | 0/3 | Not started | - |', '', + ].join('\n'), + ); + + // createTempProject() scaffolds .planning/phases but NOT STATE.md; write it + // so the "ROADMAP/STATE unchanged on refusal" assertions have a file to read + // (mirrors writePhaseCompleteVerificationGateFixture's STATE.md write above). + fs.writeFileSync( + path.join(planningDir, 'STATE.md'), + [ + '# State', '', + '**Current Phase:** 01', + '**Current Phase Name:** Foundation', + '**Status:** In progress', + '**Current Plan:** 01-01', + '**Last Activity:** 2025-01-01', + '**Last Activity Description:** Working on phase 1', '', + ].join('\n'), + ); + + for (const plan of plans) { + const isSuperseded = supersededPlans.includes(plan); + const body = isSuperseded + ? ['---', 'status: superseded', '---', '', '# Plan (retired)', ''].join('\n') + : ['# Plan', ''].join('\n'); + fs.writeFileSync(path.join(phase1Dir, `01-${plan}-PLAN.md`), body); + } + for (const summary of summaries) { + fs.writeFileSync(path.join(phase1Dir, `01-${summary}-SUMMARY.md`), '# Summary\n'); + } + // A passed VERIFICATION — so the verification gate (#1522) does NOT fire; + // only the plan-coverage gate is under test. + fs.writeFileSync( + path.join(phase1Dir, '01-VERIFICATION.md'), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); + } + + test('blocks completion when plans lack summaries despite a passed verification', () => { + // 3 plans, only 1 has a summary → 2 unsummarized. Verifier passed. + // Pre-#2648 this completed silently; it must now refuse and name the gaps. + writePlanCoverageFixture(tmpDir, { + plans: ['01', '02', '03'], + summaries: ['01'], + }); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const beforeRoadmap = fs.readFileSync(roadmapPath, 'utf-8'); + const beforeState = fs.readFileSync(statePath, 'utf-8'); + + const result = runGsdTools(['--json-errors', 'phase', 'complete', '1'], tmpDir); + + assert.equal(result.success, false, 'phase complete must fail when plans lack completion records (#2648)'); + const errorPayload = JSON.parse(result.error); + assert.equal(errorPayload.reason, 'phase_plan_coverage_incomplete'); + assert.match(errorPayload.message, /2 plan\(s\) have no completion record/); + // Names the missing plans (02 and 03), not a generic refusal. + assert.match(errorPayload.message, /02/); + assert.match(errorPayload.message, /03/); + // Nothing mutated. + assert.equal(fs.readFileSync(roadmapPath, 'utf-8'), beforeRoadmap); + assert.equal(fs.readFileSync(statePath, 'utf-8'), beforeState); + }); + + test('a plan retired via status: superseded does not block completion', () => { + // 3 plans, 1 summary. Plans 02 and 03 have no summary, BUT plan 03 is + // explicitly retired (status: superseded, #2349). Only plan 02 is an + // actionable gap → completion still refuses, naming ONLY 02, proving the + // retired plan is excluded from the gate (the lock/recovery pattern is + // preserved, the Goodhart hole is closed). + writePlanCoverageFixture(tmpDir, { + plans: ['01', '02', '03'], + summaries: ['01'], + supersededPlans: ['03'], + }); + + const result = runGsdTools(['--json-errors', 'phase', 'complete', '1'], tmpDir); + + assert.equal(result.success, false, 'phase 2 is still an unsummarized, non-retired gap'); + const errorPayload = JSON.parse(result.error); + assert.equal(errorPayload.reason, 'phase_plan_coverage_incomplete'); + assert.match(errorPayload.message, /1 plan\(s\) have no completion record/); + assert.match(errorPayload.message, /02/); + // The retired plan 03 is NOT named as a gap. + assert.doesNotMatch(errorPayload.message, /\b03\b/); + }); + + test('a fully-covered phase with a passed verification completes normally', () => { + // Regression guard: the gate must not over-block a healthy phase where + // every plan has a summary. + writePlanCoverageFixture(tmpDir, { + plans: ['01', '02', '03'], + summaries: ['01', '02', '03'], + }); + + const result = runGsdTools(['phase', 'complete', '1'], tmpDir); + + assert.equal(result.success, true, `phase complete failed on a fully-covered phase: ${result.error}`); + const out = JSON.parse(result.output); + assert.equal(out.completed_phase, '1'); + }); + + // NOTE on the security-review B1 case (fail-closed on an UNREADABLE plan dir): + // the gate defends it in code — it readdirSync's the phase dir and refuses on a + // throw before scanPhasePlans' swallow-and-return-empty can make the gate pass + // (src/phase.cts). It is NOT covered by a unit test here because there is no + // cross-platform, root-safe way to construct it: any condition that makes the + // phase dir unreadable to the gate's readdirSync ALSO makes findPhaseInternal + // (which walks the parent phases/ dir) fail upstream with "Phase N not found" + // before the gate runs, and the root-safe alternative to chmod 0o000 does not + // exist (root bypasses mode bits, so a mode-based test silently passes with + // zero coverage in root Docker/CI — the documented reason the repo forbids + // chmod-based IO-failure tests). The defensive code is cheap and correct; the + // unreachable-path gap is recorded in 60-review.json. +}); + describe('phase complete command', () => { let tmpDir; @@ -3293,6 +3447,11 @@ describe('phase complete command', () => { const d = path.join(tmpDir, '.planning', 'phases', `${num}-${names[i]}`); fs.mkdirSync(d, { recursive: true }); fs.writeFileSync(path.join(d, `${num}-PLAN.md`), '# Plan\n'); + // #2648: phase.complete now refuses when a non-retired plan has no matching + // *-SUMMARY.md. This test's concern is the total_phases-decrement cascade, + // not plan coverage, so give each phase's plan a summary to keep it + // fully-covered and isolate the #1752 behavior under test. + fs.writeFileSync(path.join(d, `${num}-SUMMARY.md`), '# Summary\n'); } const result = runVerifiedPhaseComplete('phase complete 36', tmpDir);