fix(#3345): stop counting blocked summaries as completed plans (#3459)

* fix(#3345): stop counting blocked summaries as completed plans

* chore(#3345): set changeset pr reference

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-14 10:24:05 -04:00
committed by GitHub
parent 69e7afd0c7
commit da7d4dac52
6 changed files with 425 additions and 5 deletions

View File

@@ -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.

View File

@@ -1,4 +1,4 @@
{
"maxFiles": 303,
"maxFiles": 304,
"grace": 3
}

View File

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

View File

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

View File

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

View File

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