From 388837219d1897faa8824b0f14153cbed7d3f7f5 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 31 Jul 2026 18:11:40 -0400 Subject: [PATCH] fix(#2648): phase.complete refuses when non-retired plans lack summaries (fail-closed coverage gate) (#2953) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2648): failing-first — phase complete must refuse when plans lack summaries Adds findUnsummarizedPlans to core-utils (mirrors countMatchedSummaries but returns the unmatched plan files) and a PHASE_PLAN_COVERAGE_INCOMPLETE error reason, plus a 3-case regression block in phase.test.cjs. The gate itself is NOT yet wired into cmdPhaseComplete (reverted for the RED run), so the 'blocks completion' and 'superseded does not block' cases must FAIL (the pre-fix code completes silently). * fix(#2648): phase.complete refuses when non-retired plans lack summaries cmdPhaseComplete gated 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 incident: 6/30 plans unexecuted incl. the phase's entire final UI scope, every signal green). Add a fail-closed plan-coverage gate that refuses completion when any plan lacks a matching *-SUMMARY.md, naming the missing plans, UNLESS the plan is retired via machine-readable status: superseded frontmatter (#2349) — closing the Goodhart hole (delete a SUMMARY to raise the %) without regressing the lock/recovery pattern. Uses scanPhasePlans (superseded-AWARE) + new findUnsummarizedPlans helper so the gate, the count, and the named list can never disagree. Evaluated before the verification-gate transaction so a refusal fails fast without mutating ROADMAP/STATE. milestone.complete's parallel gap is explicitly out of scope (separate seam, separate PR). * fix(#2648): test fixtures — give #1752 phase plans summaries + STATE.md in coverage fixture The plan-coverage gate (#2648) correctly blocks phase completion when a plan lacks a SUMMARY. Two test fixtures needed updating to reflect the new contract: - #1752 (total_phases-decrement cascade): its 8 phase dirs each had a PLAN.md with no SUMMARY. The test's concern is the total_phases cascade, not plan coverage, so add a matching SUMMARY to each to keep the phase fully-covered and isolate the #1752 behavior. - the #2648 coverage-gate fixture: write STATE.md (createTempProject scaffolds .planning/phases but not STATE.md) so the 'ROADMAP/STATE unchanged on refusal' assertions have a file to read. * fix(#2648): security — fail closed on unreadable plan dir + sanitize msg + surface superseded Address the security-review blocker (B1) and hardening (M1/m2): - B1 (blocker): the gate failed OPEN when scanPhasePlans could not read the phase dir (it swallows readdirSync errors → empty plan set → gate sees zero unsummarized plans → passes). A coverage gate that passes when it cannot read the plans re-opens the #2648 hole under any I/O failure. Now readdirSync the dir explicitly and fail closed (PHASE_PLAN_COVERAGE_INCOMPLETE) on a throw; a readable empty dir still passes (legitimately complete empty phase). - m2: sanitize plan filenames (strip C0 controls / DEL) before interpolating into the error message — they come raw from readdirSync and could spoof the terminal in plain-error mode. - M1: surface the count of plans excluded as status: superseded so a reviewer can audit which work was declared retired (the marker is a committable, review-time-trusted bypass; keep it visible). - Add a 4th regression case: unreadable plan dir (ENOTDIR via a file, not chmod 0o000 which root bypasses) must fail closed. * test(#2648): drop unreachable B1 case — no root-safe unreadable-dir repro The B1 fail-closed-on-unreadable-dir defense stays in src/phase.cts (cheap + correct), but it cannot be unit-tested cross-platform: any condition that makes the phase dir unreadable to the gate's readdirSync ALSO fails findPhaseInternal upstream ('Phase N not found') before the gate runs, and chmod 0o000 is forbidden (root bypasses it in root CI). Document the gap in the test file; remove the case that asserted a reason the upstream error pre-empts. * style(#2648): drop unnecessary type assertions flagged by lint:ci scanPhasePlans returns typed string[] arrays, so the `as string[]` casts on coverageScan.planFiles/summaryFiles were redundant (@typescript-eslint/ no-unnecessary-type-assertion). Compute supersededCount from typed lengths; only the phaseInfo['plans'] cast remains (it is genuinely unknown). * changeset(#2648): phase.complete refuses when plans lack summaries * changeset(#2648): backfill PR number 2953 --------- Co-authored-by: sim --- .changeset/patient-quails-purr.md | 5 + src/core-utils.cts | 49 +++++++-- src/io.cts | 1 + src/phase.cts | 78 ++++++++++++++- tests/phase.test.cjs | 159 ++++++++++++++++++++++++++++++ 5 files changed, 281 insertions(+), 11 deletions(-) create mode 100644 .changeset/patient-quails-purr.md 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);