diff --git a/.changeset/daring-otters-roar.md b/.changeset/daring-otters-roar.md new file mode 100644 index 000000000..6959d835b --- /dev/null +++ b/.changeset/daring-otters-roar.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2404 +--- +**A phase with a deliberately-unexecuted (superseded) plan no longer stays stuck below 100%** — a plan reassigned or dropped mid-phase can never gain a matching SUMMARY, yet plan-scan counted it forever, so the phase read In Progress and the milestone sat below 100% permanently — the plan-level analogue of the retired-phase bug (#1514). Mark such a plan `status: superseded` in its PLAN.md frontmatter and it is now excluded from both the plan and summary counts, so the phase completes honestly (a 13-plan phase with 2 superseded reads 11/11). Plans without the marker are unchanged. (#2349) diff --git a/docs/reference/plan-md.md b/docs/reference/plan-md.md index 3455c8b4d..079e3a6f4 100644 --- a/docs/reference/plan-md.md +++ b/docs/reference/plan-md.md @@ -72,8 +72,24 @@ must_haves: | `autonomous` | Yes | boolean | `true` when all tasks are type `auto`. `false` when the plan contains any `checkpoint:*` task that requires human interaction. | | `requirements` | Yes | array of IDs | Requirement IDs from ROADMAP.md that this plan addresses. Every phase requirement ID must appear in at least one plan's `requirements` field. Empty arrays are a BLOCKER. | | `user_setup` | No | array of objects | External-service setup steps that Claude cannot automate (account creation, secret retrieval, dashboard configuration). When present, execute-phase generates a `USER-SETUP.md` checklist for the developer. | +| `status` | No | `superseded` | Marks a plan that was deliberately reassigned or abandoned mid-phase and will never be executed. A `status: superseded` plan is excluded from the phase's plan and summary counts, so it never holds the phase below 100%. See [Superseded plans](#superseded-plans). Any other value (or the field's absence) has no effect on counting. | | `must_haves` | Yes | object | Goal-backward verification criteria. See below. | +### Superseded plans + +A phase reads complete when every `*-PLAN.md` has a matching `*-SUMMARY.md`. When a plan is reassigned or dropped mid-phase — its work folded into a later plan — it will never gain a summary, and without a marker it would pin the phase below 100% forever (the plan-level analogue of a retired phase). Add `status: superseded` to that plan's frontmatter to exclude it from **both** the plan count (denominator) and the summary count (numerator): + +```yaml +--- +phase: 05-api +plan: "12" +type: execute +status: superseded +--- +``` + +A phase with 13 plans, two of them `superseded`, then reads `11/11 → complete` — no fabricated summary required. The match is case-insensitive. Plans without the marker are counted exactly as before. + --- ## `must_haves` field diff --git a/src/plan-scan.cts b/src/plan-scan.cts index 67731814d..c73290189 100644 --- a/src/plan-scan.cts +++ b/src/plan-scan.cts @@ -7,17 +7,67 @@ * from the prior hand-written .cjs; only types are added. */ -import { existsSync, readdirSync } from 'node:fs'; +import { existsSync, readdirSync, statSync, openSync, readSync, closeSync } from 'node:fs'; import { join } from 'node:path'; // eslint-disable-next-line @typescript-eslint/no-require-imports import coreUtils = require('./core-utils.cjs'); const { countMatchedSummaries } = coreUtils; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import frontmatterMod = require('./frontmatter.cjs'); +const { extractFrontmatter } = frontmatterMod; // Excluded derivative files const PLAN_OUTLINE_RE = /-OUTLINE\.md$/i; const PLAN_PRE_BOUNCE_RE = /\.pre-bounce\.md$/i; const PLAN_REVIEW_RE = /-PLAN-REVIEW\.md$/i; +// #2349: a plan's frontmatter always sits at byte 0 and closes well before the +// body, so only a bounded prefix is ever needed to read the `status` marker. +// Capping the read keeps scanPhasePlans — which loops over every phase directory +// on hot paths (state sync/validate, roadmap progress) — from slurping a +// pathologically large committed plan file into memory just to inspect one key. +const PLAN_FRONTMATTER_READ_CAP = 64 * 1024; + +/** + * #2349: a plan whose frontmatter declares `status: superseded` was deliberately + * reassigned or never executed — its work moved to a later plan, so it can never + * gain a matching `*-SUMMARY.md`. Like a retired phase (#1514, one level up), such + * a plan must be excluded from BOTH the plan and summary counts; otherwise a phase + * with a deliberately-unexecuted plan reads `completed: false` forever, pinning the + * milestone below 100%. Reading only the frontmatter `status` key is the same seam + * verify.cts / phase.cts already use for plan metadata; a plan without the marker is + * counted exactly as before. + * + * This is the only path in scanPhasePlans that opens file *contents* (the rest is + * filename matching), so it is hardened accordingly: `statSync().isFile()` rejects + * anything that is not a regular file — a directory, socket, or a symlink resolving + * to a device such as `/dev/zero` (a git-committable DoS vector; cf. #2378/#2383) — + * BEFORE any open, and the read is bounded to a fixed prefix. Fail-safe throughout: + * a non-regular or unreadable plan is treated as a normal (counted) plan, never + * silently dropped. + */ +function isPlanSuperseded(planFullPath: string): boolean { + let content: string; + try { + const st = statSync(planFullPath); // follows symlinks → resolves to the target's real type + if (!st.isFile()) return false; + const length = Math.min(st.size, PLAN_FRONTMATTER_READ_CAP); + if (length === 0) return false; + const fd = openSync(planFullPath, 'r'); + try { + const buf = Buffer.allocUnsafe(length); + const bytesRead = readSync(fd, buf, 0, length, 0); + content = buf.toString('utf8', 0, bytesRead); + } finally { + closeSync(fd); + } + } catch { + return false; + } + const status = extractFrontmatter(content)['status']; + return typeof status === 'string' && status.trim().toLowerCase() === 'superseded'; +} + function isRootPlanFile(fileName: string): boolean { if (PLAN_OUTLINE_RE.test(fileName)) return false; if (PLAN_PRE_BOUNCE_RE.test(fileName)) return false; @@ -84,7 +134,16 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { } catch { /* ignore unreadable nested layout */ } } - const planFiles = rootPlanFiles.concat(nestedPlanFiles); + const allPlanFiles = rootPlanFiles.concat(nestedPlanFiles); + // #2349: drop plans explicitly marked `status: superseded` from the plan set + // BEFORE counting, so they inflate neither the denominator (planCount) nor, + // via countMatchedSummaries below, the numerator (summaryCount). Plans without + // the marker are untouched, so behaviour is byte-for-behaviour identical for + // every existing phase — only a phase carrying the new marker changes. + const supersededPlanFiles = allPlanFiles.filter((f) => isPlanSuperseded(join(phaseDir, f))); + const planFiles = supersededPlanFiles.length === 0 + ? allPlanFiles + : allPlanFiles.filter((f) => !supersededPlanFiles.includes(f)); const summaryFiles = rootSummaryFiles.concat(nestedSummaryFiles); const planCount = planFiles.length; // Count only summaries that are the PLAN→SUMMARY partner of an existing plan @@ -97,7 +156,14 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { return { planCount, summaryCount, - completed: planCount > 0 && summaryCount >= planCount, + // #2349: gate completion on whether the phase had ANY plans on disk + // (allPlanFiles), NOT on the post-exclusion planCount. A phase whose plans + // were ALL marked superseded has planCount 0, but it is NOT an unplanned + // empty phase — there is simply no remaining work, so it must read complete + // (0 >= 0) rather than being pinned below 100% forever, which is the very + // failure this fix removes. A genuinely empty phase (no plans authored) + // still has allPlanFiles.length 0 and stays not-completed, exactly as before. + completed: allPlanFiles.length > 0 && summaryCount >= planCount, hasNestedPlans, planFiles, summaryFiles, diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index d5be4f408..511d89ad1 100644 --- a/tests/roadmap-parser.test.cjs +++ b/tests/roadmap-parser.test.cjs @@ -1873,6 +1873,164 @@ describe('scanPhasePlans — call-site parity on mixed fixture', () => { assert.strictEqual(result.hasNestedPlans, true, 'plans/ dir exists with plans'); }); }); + +// --------------------------------------------------------------------------- +// #2349: plan-level supersession. A plan whose frontmatter declares +// `status: superseded` was deliberately reassigned / never executed and can +// never gain a matching SUMMARY. Like a retired phase (#1514, one level up) it +// must be excluded from BOTH the plan and summary counts, so the phase can still +// read 100% complete instead of being pinned below it forever. A plan WITHOUT +// the marker must be counted exactly as before (no over-exclusion). +// --------------------------------------------------------------------------- + +describe('scanPhasePlans — superseded plans (#2349)', () => { + const SUPERSEDED_FM = + '---\nphase: "1"\nplan: "6"\ntype: implementation\nstatus: superseded\n---\n\n# Superseded plan\n'; + + function writePlan(dir, name, body) { + fs.writeFileSync(path.join(dir, name), body); + } + + test('a plan marked status: superseded is excluded from planCount and planFiles', () => { + const dir = phaseDir(); + touch(dir, '01-01-PLAN.md', '01-01-SUMMARY.md'); + writePlan(dir, '01-06-PLAN.md', SUPERSEDED_FM); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 1, 'superseded plan not counted in planCount'); + assert.strictEqual(result.summaryCount, 1, 'summaryCount honest at 1'); + assert.strictEqual(result.completed, true, 'phase completes despite the summary-less superseded plan'); + assert.ok(!result.planFiles.includes('01-06-PLAN.md'), 'superseded plan absent from planFiles'); + assert.ok(result.planFiles.includes('01-01-PLAN.md'), 'live plan still present'); + }); + + test('reported case: 13 plans, 2 superseded, 11 summaries → 11/11 completed', () => { + const dir = phaseDir(); + // 11 executed plans, each with its matching summary + for (let i = 1; i <= 11; i++) { + const n = String(i).padStart(2, '0'); + touch(dir, `05-${n}-PLAN.md`, `05-${n}-SUMMARY.md`); + } + // 2 superseded plans, no summaries (work reassigned to later plans) + writePlan(dir, '05-12-PLAN.md', SUPERSEDED_FM); + writePlan(dir, '05-13-PLAN.md', SUPERSEDED_FM); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 11, 'denominator excludes the 2 superseded plans'); + assert.strictEqual(result.summaryCount, 11, 'numerator honest at 11'); + assert.strictEqual(result.completed, true, 'phase reads complete — no longer pinned below 100%'); + }); + + test('boundary: a live unsummarized plan still blocks completion (limit+1)', () => { + const dir = phaseDir(); + touch(dir, '02-01-PLAN.md', '02-01-SUMMARY.md'); + touch(dir, '02-02-PLAN.md'); // live plan, no summary → must still block + writePlan(dir, '02-09-PLAN.md', SUPERSEDED_FM); // superseded, excluded + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 2, 'only the superseded plan is excluded'); + assert.strictEqual(result.summaryCount, 1); + assert.strictEqual(result.completed, false, 'a live unsummarized plan still blocks completion'); + }); + + test('no over-exclusion: a plan without the marker (or with a non-superseded status) is counted', () => { + const dir = phaseDir(); + touch(dir, '03-01-PLAN.md'); // empty file, no frontmatter at all + writePlan(dir, '03-02-PLAN.md', '---\nphase: "3"\nplan: "2"\nstatus: complete\n---\n\n# done\n'); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 2, 'only status: superseded is excluded; other/absent statuses count'); + assert.ok(result.planFiles.includes('03-01-PLAN.md')); + assert.ok(result.planFiles.includes('03-02-PLAN.md')); + }); + + test('superseded marker is case-insensitive and whitespace-tolerant', () => { + const dir = phaseDir(); + touch(dir, '04-01-PLAN.md', '04-01-SUMMARY.md'); + writePlan(dir, '04-07-PLAN.md', '---\nphase: "4"\nplan: "7"\nstatus: Superseded \n---\n\n# x\n'); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 1, 'Superseded (mixed-case, padded) still excluded'); + assert.strictEqual(result.completed, true); + }); + + test('nested layout: a superseded nested plan is excluded too', () => { + const dir = phaseDir(); + const plansDir = path.join(dir, 'plans'); + fs.mkdirSync(plansDir); + touch(plansDir, 'PLAN-01-setup.md', 'SUMMARY-01-setup.md'); + writePlan(plansDir, 'PLAN-02-dropped.md', SUPERSEDED_FM); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 1, 'nested superseded plan excluded'); + assert.strictEqual(result.summaryCount, 1); + assert.strictEqual(result.completed, true); + assert.ok(!result.planFiles.includes('plans/PLAN-02-dropped.md')); + }); + + test('fail-safe: an unreadable plan path (a directory named *-PLAN.md) is counted, not treated as superseded', () => { + const dir = phaseDir(); + touch(dir, '06-01-PLAN.md', '06-01-SUMMARY.md'); + // A *directory* whose name matches a plan file: readdir lists it, but reading + // it as a file throws EISDIR on every platform (root-independent, leak-safe). + // The read-failure path must fail safe to "not superseded" (counted) — never + // silently drop a plan whose frontmatter simply could not be read. + fs.mkdirSync(path.join(dir, '06-02-PLAN.md')); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 2, 'unreadable plan still counted (fail-safe, not excluded)'); + assert.strictEqual(result.completed, false, 'the unreadable plan has no summary → still blocks'); + }); + + test('all plans superseded → phase reads complete, not pinned below 100% (0 >= 0)', () => { + const dir = phaseDir(); + // Every plan in the phase was reassigned elsewhere: nothing left to execute. + writePlan(dir, '08-01-PLAN.md', SUPERSEDED_FM); + writePlan(dir, '08-02-PLAN.md', SUPERSEDED_FM); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 0, 'all plans excluded'); + assert.strictEqual(result.summaryCount, 0); + assert.strictEqual(result.completed, true, 'a fully-superseded phase has no remaining work → complete'); + }); + + test('a genuinely empty phase (no plans authored) still reads NOT complete', () => { + // Regression guard for the all-superseded fix: the completion guard keys off + // whether any plans existed on disk, so a phase with zero plans is unaffected. + const result = scanPhasePlans(phaseDir()); + assert.strictEqual(result.planCount, 0); + assert.strictEqual(result.completed, false, 'no plans authored → not complete (unchanged)'); + }); + + test('a superseded marker is detected even when the plan body is very large', () => { + // Frontmatter sits at byte 0; the scan reads only a bounded prefix, so a large + // body must neither hide the marker nor force reading the whole file. + const dir = phaseDir(); + touch(dir, '09-01-PLAN.md', '09-01-SUMMARY.md'); + const bigBody = 'x'.repeat(200 * 1024); // 200 KB body, well past the read cap + writePlan(dir, '09-09-PLAN.md', `---\nphase: "9"\nplan: "9"\nstatus: superseded\n---\n\n${bigBody}\n`); + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, 1, 'superseded plan with a large body still excluded'); + assert.strictEqual(result.completed, true); + }); + + test('property: K superseded plans never change the completed verdict of N summarized plans', () => { + const fc = require('fast-check'); + fc.assert( + fc.property(fc.integer({ min: 1, max: 8 }), fc.integer({ min: 0, max: 5 }), (n, k) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-plan-scan-prop-')); + try { + for (let i = 1; i <= n; i++) { + const idx = String(i).padStart(2, '0'); + fs.writeFileSync(path.join(dir, `07-${idx}-PLAN.md`), ''); + fs.writeFileSync(path.join(dir, `07-${idx}-SUMMARY.md`), ''); + } + for (let j = 1; j <= k; j++) { + fs.writeFileSync(path.join(dir, `07-${String(50 + j)}-PLAN.md`), SUPERSEDED_FM); + } + const result = scanPhasePlans(dir); + assert.strictEqual(result.planCount, n, 'planCount ignores the K superseded plans'); + assert.strictEqual(result.completed, true, 'N fully-summarized plans stay complete regardless of K'); + } finally { + cleanup(dir); + } + }), + { numRuns: 40 }, + ); + }); +}); }); }