golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#2648): phase.complete refuses when non-retired plans lack summaries (fail-closed coverage gate) (#2953)

Browse Source 
* 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 <sim@users.noreply.github.com>
This commit is contained in:
Tom Boucher
2026-07-31 18:11:40 -04:00
committed by GitHub
parent 5a0a9f0972
commit 388837219d
5 changed files with 281 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

49
src/core-utils.cts
View File

@@ -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,
};

1
src/io.cts
View File

@@ -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',

78
src/phase.cts
View File

@@ -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);