diff --git a/.changeset/lucky-bears-forage.md b/.changeset/lucky-bears-forage.md new file mode 100644 index 000000000..ec9b2d12f --- /dev/null +++ b/.changeset/lucky-bears-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3459 +--- +A plan SUMMARY whose frontmatter declares status: blocked is no longer counted as a completed plan. Previously both the progress counters written to STATE.md (state planned-phase / begin-phase / record-session) and the phase-plan-index read path paired PLAN and SUMMARY files by filename existence alone, so a blocked plan counted as done and was omitted from the incomplete list. Filename existence remains the fallback when a SUMMARY carries no status field, and status: halted summaries still count as completion records (a designed stop), so untouched projects are unaffected. diff --git a/scripts/lint-allow-test-rule-refs.ceiling.json b/scripts/lint-allow-test-rule-refs.ceiling.json index aa077980c..d364ad0f2 100644 --- a/scripts/lint-allow-test-rule-refs.ceiling.json +++ b/scripts/lint-allow-test-rule-refs.ceiling.json @@ -1,4 +1,4 @@ { - "maxFiles": 303, + "maxFiles": 304, "grace": 3 } diff --git a/src/phase.cts b/src/phase.cts index f83e617b4..5e7c9abe6 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -79,7 +79,7 @@ import verifyMod = require('./verify.cjs'); const { readVerificationStatus } = verificationMod; // eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-dependency-graph.cjs is an export= CommonJS module import planDependencyGraphMod = require('./plan-dependency-graph.cjs'); -const { computeHaltPropagation, buildSummaryFileIndex, isSummaryFileHalted } = planDependencyGraphMod; +const { computeHaltPropagation, buildSummaryFileIndex, isSummaryFileHalted, isSummaryFileBlocked } = planDependencyGraphMod; const { planningDir, withPlanningLock, listAvailableWorkstreams, getActiveWorkstream } = planningWorkspace; @@ -769,7 +769,20 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void { // `plans/SUMMARY-01.md` correctly) instead of a bespoke ID-Set built from // extractCanonicalPlanId, which only ever handled the root-canonical // `-PLAN.md`/`-SUMMARY.md` naming form. - const unsummarizedPlanFiles = new Set(findUnsummarizedPlans(planFiles, summaryFiles)); + // + // #3345: the summary list is filtered through the SAME shared predicate + // scanPhasePlans filters its countable set with + // (plan-dependency-graph.cjs's isSummaryFileBlocked), so a SUMMARY declaring + // `status: blocked` reads as NO completion record here — has_summary false, + // the plan lands in `incomplete` — exactly matching the count side. Fail-open + // on a SUMMARY with no status key / unreadable file (filename fallback); + // `status: halted` stays summarized (#2830 designed stop). summaryFileByPlanId + // below still indexes EVERY summary on disk because the halted lookup is a + // file resolution for reading status, not a completion pairing. + const countableSummaryFiles = summaryFiles.filter( + (f) => !isSummaryFileBlocked(path.join(phaseDir, f)), + ); + const unsummarizedPlanFiles = new Set(findUnsummarizedPlans(planFiles, countableSummaryFiles)); // #2830: reverse lookup from a completed plan's id (exact or canonical) to // the actual summary filename, so a plan's own SUMMARY frontmatter can be // read for its `status`. Shared builder (also used by phase-locator.cts's diff --git a/src/plan-dependency-graph.cts b/src/plan-dependency-graph.cts index a209d348f..2c2b46915 100644 --- a/src/plan-dependency-graph.cts +++ b/src/plan-dependency-graph.cts @@ -115,6 +115,66 @@ function buildSummaryFileIndex( return index; } +/** + * #3345: the one place "does this SUMMARY status value mean blocked" is + * decided, sibling to `isHaltedStatus` above. A SUMMARY declaring + * `status: blocked` records a plan that could NOT finish — it is a failure + * record, not a completion record — so it must not count toward + * `completed_plans` (scanPhasePlans's summaryCount) nor read as + * `has_summary: true` in phase-plan-index's `incomplete` construction. + * + * `halted` is deliberately NOT matched here: a designed stop still writes a + * completion record (#2830's model — its dependents get `blocked_by` halt + * propagation), so a `status: halted` SUMMARY keeps counting as summarized. + * Case-insensitive, trims whitespace, and strips an unquoted trailing YAML + * comment exactly like `isHaltedStatus` (same #2830 review defect-2 rule). + */ +function isBlockedStatus(status: unknown): boolean { + if (typeof status !== 'string') return false; + const withoutTrailingComment = status.replace(/\s+#.*$/, ''); + return withoutTrailingComment.trim().toLowerCase() === 'blocked'; +} + +// #3345: SUMMARY frontmatter sits at byte 0 and closes well before the body, +// so only a bounded prefix is ever needed to read the `status` marker — the +// same cap discipline as plan-scan.cts's PLAN_FRONTMATTER_READ_CAP (#2349), +// kept here because this predicate's primary caller (scanPhasePlans) loops +// over every phase directory on hot paths (state sync, roadmap progress). +const SUMMARY_FRONTMATTER_READ_CAP = 64 * 1024; + +/** + * #3345: read a SUMMARY file's frontmatter `status` (bounded-prefix read) and + * report whether it declares `status: blocked`. Returns false — never throws — + * on a missing/unreadable/malformed/non-regular SUMMARY, so an unreadable file + * degrades to the pre-#3345 filename-existence behaviour rather than breaking + * either caller. Fail-open by design, mirroring `isPlanSuperseded`'s posture. + * + * Callers: plan-scan.cts's `scanPhasePlans` (the count side) and phase.cts's + * `cmdPhasePlanIndex` (the read side) BOTH filter their summary lists through + * this one predicate, so the count and the `incomplete` list can never + * re-diverge on the blocked rule. + */ +function isSummaryFileBlocked(summaryPath: string): boolean { + let content: string; + try { + const st = fs.statSync(summaryPath); // follows symlinks → resolves to the target's real type + if (!st.isFile()) return false; + const length = Math.min(st.size, SUMMARY_FRONTMATTER_READ_CAP); + if (length === 0) return false; + const fd = fs.openSync(summaryPath, 'r'); + try { + const buf = Buffer.allocUnsafe(length); + const bytesRead = fs.readSync(fd, buf, 0, length, 0); + content = buf.toString('utf8', 0, bytesRead); + } finally { + fs.closeSync(fd); + } + } catch { + return false; + } + return isBlockedStatus(extractFrontmatter(content, summaryPath)['status']); +} + interface PlanHaltNode { /** Canonical plan id, already resolved — matches another node's `id` for a dependency edge to count. */ id: string; @@ -253,4 +313,13 @@ function computeHaltPropagation(nodes: PlanHaltNode[], precomputedOrder?: string return { order, visited, blockedBy }; } -export = { computeHaltPropagation, isHaltedStatus, buildSummaryFileIndex, isSummaryFileHalted }; +export = { + computeHaltPropagation, + isHaltedStatus, + buildSummaryFileIndex, + isSummaryFileHalted, + // #3345: blocked-SUMMARY detection, shared by the count side (scanPhasePlans) + // and the read side (phase-plan-index) so the two cannot diverge. + isBlockedStatus, + isSummaryFileBlocked, +}; diff --git a/src/plan-scan.cts b/src/plan-scan.cts index 08cedac23..a97259b4e 100644 --- a/src/plan-scan.cts +++ b/src/plan-scan.cts @@ -18,6 +18,9 @@ const { extractFrontmatter } = frontmatterMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planDependencyGraphMod = require('./plan-dependency-graph.cjs'); +const { isSummaryFileBlocked } = planDependencyGraphMod; // Excluded derivative files const PLAN_OUTLINE_RE = /-OUTLINE\.md$/i; @@ -207,7 +210,20 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { // 30-GAPCLOSURE-SUMMARY.md) must not inflate summary_count or flip a phase to // Complete when plans are still missing summaries. summaryFiles (the array) // still holds every summary on disk for callers that read/list them. - const summaryCount = countMatchedSummaries(planFiles, summaryFiles); + // + // #3345: a SUMMARY whose frontmatter declares `status: blocked` is a failure + // record, not a completion record — it is dropped from the COUNTABLE pairing + // set before matching. The bounded-prefix status read is the SHARED predicate + // (plan-dependency-graph.cjs's isSummaryFileBlocked) that phase.cts's read + // path also filters through, so the count and the `incomplete` list cannot + // diverge. Fail-open: a SUMMARY with no `status` key, or one that cannot be read, + // keeps its pre-#3345 filename-existence meaning — untouched projects are + // byte-for-behaviour identical. `status: halted` stays counted (#2830: a + // designed stop still writes a completion record). + const countableSummaryFiles = summaryFiles.filter( + (f) => !isSummaryFileBlocked(join(phaseDir, f)), + ); + const summaryCount = countMatchedSummaries(planFiles, countableSummaryFiles); return { planCount, diff --git a/tests/summary-status-blocked-3345.test.cjs b/tests/summary-status-blocked-3345.test.cjs new file mode 100644 index 000000000..539afa951 --- /dev/null +++ b/tests/summary-status-blocked-3345.test.cjs @@ -0,0 +1,317 @@ +/** + * Tests for SUMMARY-frontmatter-status-aware plan completion (#3345). + * + * Before #3345, both completion readers paired PLAN↔SUMMARY by FILENAME + * EXISTENCE only: `scanPhasePlans`'s `summaryCount`/`completed` + * (src/plan-scan.cts, via core-utils `countMatchedSummaries`) and + * `phase-plan-index`'s `has_summary`/`incomplete` (src/phase.cts, via + * core-utils `findUnsummarizedPlans`) never opened a SUMMARY file, so a + * SUMMARY declaring `status: blocked` counted as a completed plan in STATE.md + * progress counters AND was omitted from phase-plan-index's `incomplete` list. + * + * This suite pins the #3345 contract: + * - a SUMMARY whose frontmatter `status:` is `blocked` is NOT a completion + * record (count side: not counted; read side: has_summary false, lands in + * `incomplete`); + * - filename existence stays the fallback when the SUMMARY carries no + * `status` key (untouched projects are byte-for-behaviour unchanged); + * - `status: halted` STAYS counted — #2830's designed-stop model (a halt + * still writes a completion record; dependents get `blocked_by` + * propagation) is deliberately preserved; + * - PARITY GUARD (Generative Fix Divergence convention): the count side and + * the read side must filter their summary lists through the ONE shared + * predicate (`plan-dependency-graph.cjs`'s `isSummaryFileBlocked`), so the + * two can never re-diverge on this rule; + * - fail-open: an unreadable SUMMARY degrades to the pre-#3345 + * filename-existence behaviour, never throws. + * + * Uses helpers.cjs createTempProject/cleanup per CONTRIBUTING.md and the + * process-seam-backed runGsdTools for the CLI-level assertions. + */ + +'use strict'; + +const { test, describe, mock } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); +const { join } = path; + +function tempDir(prefix) { + return fs.mkdtempSync(join(os.tmpdir(), prefix)); +} + +const planScan = require('../gsd-core/bin/lib/plan-scan.cjs'); +const planDependencyGraph = require('../gsd-core/bin/lib/plan-dependency-graph.cjs'); +const coreUtils = require('../gsd-core/bin/lib/core-utils.cjs'); +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); + +function planBody() { + return '# Plan\n'; +} + +function summaryWithStatus(status) { + return status === undefined + ? '# Summary\n' + : `---\nstatus: ${status}\n---\n\n# Summary\n`; +} + +function writePhase(dir, files) { + fs.mkdirSync(dir, { recursive: true }); + for (const [name, content] of Object.entries(files)) { + fs.writeFileSync(join(dir, name), content); + } +} + +// ─── isBlockedStatus unit (the one place "blocked" is decided) ───────────── + +describe('#3345 isBlockedStatus — truthtable', () => { + test('matches exactly the blocked spelling, case/whitespace/comment tolerant', () => { + const { isBlockedStatus } = planDependencyGraph; + assert.equal(typeof isBlockedStatus, 'function', 'isBlockedStatus must be exported'); + assert.equal(isBlockedStatus('blocked'), true); + assert.equal(isBlockedStatus('Blocked'), true); + assert.equal(isBlockedStatus(' BLOCKED '), true); + // Unquoted trailing YAML comment (same #2830 review defect-2 rule as isHaltedStatus). + assert.equal(isBlockedStatus('blocked # waiting on upstream'), true); + assert.equal(isBlockedStatus('halted'), false, 'halted is NOT blocked (designed stop)'); + assert.equal(isBlockedStatus('complete'), false); + assert.equal(isBlockedStatus('superseded'), false); + assert.equal(isBlockedStatus('blocked#nospace'), false, 'no-whitespace # is not a YAML comment'); + assert.equal(isBlockedStatus(undefined), false); + assert.equal(isBlockedStatus(42), false); + assert.equal(isBlockedStatus(null), false); + }); +}); + +// ─── scanPhasePlans: the count side ──────────────────────────────────────── + +describe('#3345 scanPhasePlans — blocked SUMMARY is not a completion record', () => { + test('status: blocked SUMMARY -> summaryCount 0, completed false', () => { + const dir = tempDir('gsd-3345-blocked-'); + try { + writePhase(dir, { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus('blocked'), + }); + const scan = planScan(dir); + assert.equal(scan.planCount, 1); + assert.equal(scan.summaryCount, 0, 'blocked SUMMARY must not count as a completion record'); + assert.equal(scan.completed, false, 'a blocked plan must not read as phase-complete'); + // The on-disk list is untouched — callers that list summaries still see the file. + assert.deepEqual(scan.summaryFiles, ['01-01-SUMMARY.md']); + } finally { + cleanup(dir); + } + }); + + test('no status key -> filename-existence fallback, counted exactly as before', () => { + const dir = tempDir('gsd-3345-nostatus-'); + try { + writePhase(dir, { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus(undefined), + }); + const scan = planScan(dir); + assert.equal(scan.summaryCount, 1); + assert.equal(scan.completed, true); + } finally { + cleanup(dir); + } + }); + + test('status: complete SUMMARY -> counted', () => { + const dir = tempDir('gsd-3345-complete-'); + try { + writePhase(dir, { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus('complete'), + }); + const scan = planScan(dir); + assert.equal(scan.summaryCount, 1); + assert.equal(scan.completed, true); + } finally { + cleanup(dir); + } + }); + + test('status: halted SUMMARY stays counted (#2830 designed-stop pin)', () => { + const dir = tempDir('gsd-3345-halted-'); + try { + writePhase(dir, { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus('halted'), + }); + const scan = planScan(dir); + assert.equal(scan.summaryCount, 1, 'a designed stop still writes a completion record'); + assert.equal(scan.completed, true); + } finally { + cleanup(dir); + } + }); + + test('mixed phase: 1 blocked + 1 complete of 2 plans -> summaryCount 1, completed false', () => { + const dir = tempDir('gsd-3345-mixed-'); + try { + writePhase(dir, { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus('blocked'), + '01-02-PLAN.md': planBody(), + '01-02-SUMMARY.md': summaryWithStatus(undefined), + }); + const scan = planScan(dir); + assert.equal(scan.planCount, 2); + assert.equal(scan.summaryCount, 1); + assert.equal(scan.completed, false); + } finally { + cleanup(dir); + } + }); + + test('case/whitespace/comment variants of blocked are recognized', () => { + const dir = tempDir('gsd-3345-casing-'); + try { + writePhase(dir, { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus('Blocked # waiting on upstream'), + }); + const scan = planScan(dir); + assert.equal(scan.summaryCount, 0); + assert.equal(scan.completed, false); + } finally { + cleanup(dir); + } + }); + + test('unreadable SUMMARY degrades fail-open to the filename fallback', (t) => { + const dir = tempDir('gsd-3345-unreadable-'); + try { + writePhase(dir, { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus('blocked'), + }); + const mocked = mock.method(fs, 'openSync', () => { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + }); + t.after(() => mocked.mock.restore()); + const scan = planScan(dir); + assert.equal(scan.summaryCount, 1, 'unreadable SUMMARY counts by filename, never throws'); + assert.equal(scan.completed, true); + } finally { + cleanup(dir); + } + }); +}); + +// ─── PARITY GUARD: count side and read side share ONE predicate ──────────── + +describe('#3345 parity — scanPhasePlans counts and findUnsummarizedPlans agree through the shared predicate', () => { + const PARITY_SCENARIOS = [ + { id: 'blocked', files: { '01-01-PLAN.md': planBody(), '01-01-SUMMARY.md': summaryWithStatus('blocked') } }, + { id: 'no-status', files: { '01-01-PLAN.md': planBody(), '01-01-SUMMARY.md': summaryWithStatus(undefined) } }, + { id: 'halted', files: { '01-01-PLAN.md': planBody(), '01-01-SUMMARY.md': summaryWithStatus('halted') } }, + { + id: 'mixed', + files: { + '01-01-PLAN.md': planBody(), + '01-01-SUMMARY.md': summaryWithStatus('blocked'), + '01-02-PLAN.md': planBody(), + '01-02-SUMMARY.md': summaryWithStatus(undefined), + }, + }, + ]; + + for (const scenario of PARITY_SCENARIOS) { + test(`scenario ${scenario.id}: unsummarized-through-predicate mirrors summaryCount`, () => { + const dir = tempDir(`gsd-3345-parity-${scenario.id}-`); + try { + writePhase(dir, scenario.files); + const scan = planScan(dir); + const { isSummaryFileBlocked } = planDependencyGraph; + assert.equal(typeof isSummaryFileBlocked, 'function', 'isSummaryFileBlocked must be exported'); + const countable = scan.summaryFiles.filter((f) => !isSummaryFileBlocked(join(dir, f))); + const unsummarized = coreUtils.findUnsummarizedPlans(scan.planFiles, countable); + assert.equal( + scan.planCount - scan.summaryCount, + unsummarized.length, + 'the count side (scanPhasePlans) and the read side (findUnsummarizedPlans over the SAME predicate-filtered list) must agree', + ); + } finally { + cleanup(dir); + } + }); + } +}); + +// ─── CLI: the read path (phase-plan-index) ───────────────────────────────── + +describe('#3345 phase-plan-index — blocked plan lands in incomplete', () => { + function writeCliPhase(projectDir, summaryContent) { + const phaseDir = join(projectDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(phaseDir, { recursive: true }); + const planFm = [ + '---', + 'phase: 01-test', + 'plan: "01"', + 'type: execute', + 'wave: 1', + 'depends_on: []', + 'files_modified: []', + 'autonomous: true', + '---', + '# plan', + '', + ].join('\n'); + fs.writeFileSync(join(phaseDir, '01-01-PLAN.md'), planFm); + fs.writeFileSync(join(phaseDir, '01-01-SUMMARY.md'), summaryContent); + return phaseDir; + } + + test('blocked SUMMARY -> has_summary false and the plan appears in incomplete', () => { + const project = createTempProject('gsd-3345-idx-blocked-'); + try { + writeCliPhase(project, summaryWithStatus('blocked')); + const r = runGsdTools(['phase-plan-index', '01-test', '--json'], project); + assert.equal(r.exitCode, 0, r.output); + const idx = JSON.parse(r.output); + const plan = idx.plans.find((p) => p.id === '01-01'); + assert.ok(plan, 'plan 01-01 must be indexed'); + assert.equal(plan.has_summary, false, 'a blocked SUMMARY is not a completion record'); + assert.ok(idx.incomplete.includes('01-01'), 'the blocked plan must land in incomplete'); + } finally { + cleanup(project); + } + }); + + test('plain SUMMARY (no status) -> has_summary true, incomplete empty (unchanged)', () => { + const project = createTempProject('gsd-3345-idx-plain-'); + try { + writeCliPhase(project, summaryWithStatus(undefined)); + const r = runGsdTools(['phase-plan-index', '01-test', '--json'], project); + assert.equal(r.exitCode, 0, r.output); + const idx = JSON.parse(r.output); + const plan = idx.plans.find((p) => p.id === '01-01'); + assert.equal(plan.has_summary, true); + assert.deepEqual(idx.incomplete, []); + } finally { + cleanup(project); + } + }); + + test('halted SUMMARY stays has_summary true with halted true (#2830 pin, unchanged)', () => { + const project = createTempProject('gsd-3345-idx-halted-'); + try { + writeCliPhase(project, summaryWithStatus('halted')); + const r = runGsdTools(['phase-plan-index', '01-test', '--json'], project); + assert.equal(r.exitCode, 0, r.output); + const idx = JSON.parse(r.output); + const plan = idx.plans.find((p) => p.id === '01-01'); + assert.equal(plan.has_summary, true, 'a designed stop keeps its completion record'); + assert.equal(plan.halted, true); + assert.deepEqual(idx.incomplete, []); + } finally { + cleanup(project); + } + }); +});