fix(#4093): give advance-plan's zero-labeled-fields failure a disk-derived recovery decline (#4318)

* test(#4093): regression matrix for advance-plan zero-labeled-fields decline

* fix(#4093): give advance-plan's zero-labeled-fields failure a disk-derived recovery decline

* refactor(#4093): collapse IIFE to a plain block (review finding)

* docs(#4093): document the advance-plan recovery decline + changeset

* chore(#4093): backfill PR number in changeset

* fix(#4093): budget lint-compiled-artifact-sync's tsc compile as a compile, not a probe

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-09-05 10:46:37 -04:00
committed by GitHub
parent d9d16551b7
commit 2e1ede6d99
6 changed files with 385 additions and 9 deletions

View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4318
---
**`state advance-plan` no longer strands you when `## Current Position` has lost its labeled plan-position lines** — the failure now returns reason `plan_position_unreadable` with the phase directory's on-disk plan/summary counts and the exact labeled lines to re-insert, instead of a bare unparseable error with no recovery path. (#4093)

View File

@@ -93,6 +93,10 @@ node gsd-tools.cjs state patch --field1 val1 --field2 val2
# Increment plan counter
node gsd-tools.cjs state advance-plan
# When no labeled plan position can be parsed (e.g. ## Current Position drifted
# to pure narrative prose), declines with reason "plan_position_unreadable" plus
# the disk-derived counts and the exact labeled lines to re-insert; STATE.md is
# left byte-identical.
# Record execution metrics
node gsd-tools.cjs state record-metric --phase N --plan M --duration Xmin [--tasks N] [--files N]

View File

@@ -0,0 +1,133 @@
"use strict";
/**
* TDD RED-evidence classification (#3770).
*
* The `type: tdd` executor gate previously accepted ANY nonzero test command as
* RED: syntax errors, zero-test discovery, fixture crashes, parser errors, and
* unrelated assertions all authorized production edits (GREEN). This module
* defines the compact RED evidence the gate now requires — the TARGET test's
* identity plus a matching assertion failure — and classifies a persisted test
* run into exactly one verdict:
*
* RED_EVIDENCE_OK — nonzero exit AND the target test failed as a REAL test
* (distinctly named, TAP-reported failure). The ONLY
* verdict that may advance to GREEN.
* INVALID_RED — everything else, with a machine-readable reason:
* unexpected_green | zero_tests_discovered |
* nonzero_exit_without_test_failure |
* fixture_or_load_failure | no_target_test_failure |
* invalid_record | unreadable_record (router arm).
*
* Everything here is PURE — no fs, no spawn, no clock — so the executor can
* persist the record (command, exit code, failing test, expected, actual) and
* validate it via `gsd_run check tdd-red-evidence <record.json>`.
*
* TAP parsing reuses the proven primitives from prohibition-enforcement
* (`parseNodeTestSummary`, `tapFailedTestNames`) — the same contract the
* prohibition probe's fail-first prover already relies on (#1259).
*/
Object.defineProperty(exports, "__esModule", { value: true });
exports.classifyRedEvidence = classifyRedEvidence;
exports.buildRedEvidenceRecord = buildRedEvidenceRecord;
const prohibition_enforcement_cjs_1 = require("./prohibition-enforcement.cjs");
/** Basename of a path-like string ('' for non-strings) — separators `/` and `\`. */
function baseOf(p) {
return typeof p === 'string' ? (p.split(/[\\/]/).pop() ?? p) : '';
}
/** Coerce and validate the raw record's scalar fields. Returns null exit_code only when absent/non-numeric. */
function readInput(input) {
const command = typeof input?.command === 'string' ? input.command : '';
const output = typeof input?.output === 'string' ? input.output : '';
const targetTest = typeof input?.targetTest === 'string' ? input.targetTest.trim() : '';
const exitCode = typeof input?.exitCode === 'number' && Number.isFinite(input.exitCode) ? input.exitCode : null;
if (!command || !targetTest || exitCode === null)
return null;
return { command, exitCode, output, targetTest };
}
/**
* Classify a persisted RED-phase test run. Fail-closed: malformed input, an
* unparseable/empty TAP summary, a file-named (load/crash) failure, or a
* failure that is not the target test's are all INVALID_RED — only a nonzero
* exit WITH the distinctly-named target test failing is RED_EVIDENCE_OK.
* Never throws.
*/
function classifyRedEvidence(input) {
const parsed = readInput(input);
if (!parsed) {
return {
verdict: 'INVALID_RED',
reason: 'invalid_record',
evidence: {
command: typeof input?.command === 'string' ? input.command : '',
exit_code: null,
target_test: '',
tests: 0,
pass: 0,
fail: 0,
failing_tests: [],
},
};
}
const { command, exitCode, output, targetTest } = parsed;
const summary = (0, prohibition_enforcement_cjs_1.parseNodeTestSummary)(output);
const failing = (0, prohibition_enforcement_cjs_1.tapFailedTestNames)(output);
const evidence = {
command,
exit_code: exitCode,
target_test: targetTest,
tests: summary.tests,
pass: summary.pass,
fail: summary.fail,
failing_tests: failing,
};
// Existing fail-fast rule, now machine-checked: exit 0 during RED is an
// unexpected GREEN — the feature may already exist or the test is wrong.
if (exitCode === 0) {
return { verdict: 'INVALID_RED', reason: 'unexpected_green', evidence };
}
// Zero-test discovery: the discovery pattern / fixture matched no tests.
// A run that executed nothing cannot prove anything about the behavior.
if (summary.tests === 0) {
return { verdict: 'INVALID_RED', reason: 'zero_tests_discovered', evidence };
}
// Nonzero exit but TAP reports no failing test: harness/setup/parser crash
// whose failure never reached a test assertion (or unparseable output).
if (summary.fail === 0 || failing.length === 0) {
return { verdict: 'INVALID_RED', reason: 'nonzero_exit_without_test_failure', evidence };
}
// Fixture/load failure: every failing entry is named like the target FILE —
// node reports a load-time crash (throw-on-require, syntax error, ENOENT
// fixture) as a file-named `not ok 1 - <file>`, never the target test.
const targetBase = baseOf(input?.targetFile ?? '');
const distinctlyNamed = failing.filter((n) => (targetBase ? baseOf(n) !== targetBase : true));
if (distinctlyNamed.length === 0) {
return { verdict: 'INVALID_RED', reason: 'fixture_or_load_failure', evidence };
}
// Unrelated failure: real tests ran and failed, but none is the target test
// the plan named — an unrelated assertion must not authorize GREEN.
if (!distinctlyNamed.includes(targetTest)) {
return { verdict: 'INVALID_RED', reason: 'no_target_test_failure', evidence };
}
return { verdict: 'RED_EVIDENCE_OK', reason: 'target_test_failed', evidence };
}
/**
* Project a classification into the persisted record shape — command, exit
* code, failing test, expected, actual, verdict, reason — so the evidence
* survives past the terminal and the gate can re-verify it deterministically.
* Pure: JSON-serializable, no timestamps (the record's mtime/commit carries time).
*/
function buildRedEvidenceRecord(input, result) {
const failingTest = result.evidence.failing_tests.find((n) => n === result.evidence.target_test) ??
result.evidence.failing_tests[0] ??
null;
return {
command: result.evidence.command,
exit_code: result.evidence.exit_code,
failing_test: failingTest,
target_test: result.evidence.target_test,
expected: typeof input?.expected === 'string' ? input.expected : null,
actual: typeof input?.actual === 'string' ? input.actual : null,
verdict: result.verdict,
reason: result.reason,
};
}

View File

@@ -927,11 +927,16 @@ function stateReplaceFieldWithFallback(content: string, primary: string, fallbac
* Current Position `Phase:` line, milestone-archived layouts) keep today's
* behavior rather than being newly refused.
*
* Returns `{ dir, outstanding }` where `outstanding` is empty when every plan
* on disk is summarized (vacuously so for a zero-plan phase — #3168's
* zero-plan-phase posture).
* Returns `{ dir, outstanding, planCount, summaryCount }` where `outstanding`
* is empty when every plan on disk is summarized (vacuously so for a zero-plan
* phase — #3168's zero-plan-phase posture). `planCount`/`summaryCount` are the
* countable disk facts behind `outstanding` (live plan files; summaries after
* the #3345 blocked filter) — #4093's recovery decline reports them to a
* caller whose STATE.md has lost its labeled plan position, so the suggested
* repair values are computed from the SAME set `outstanding` was, and can
* never disagree with a count-based decision either.
*/
function scanOutstanding(phasesDir: string, dir: string): { dir: string; outstanding: string[] } | null {
function scanOutstanding(phasesDir: string, dir: string): { dir: string; outstanding: string[]; planCount: number; summaryCount: number } | null {
const phaseDirPath = path.join(phasesDir, dir);
const scan = scanPhasePlans(phaseDirPath);
if (scan.scope !== SCOPE.COMPLETE) return null;
@@ -942,13 +947,13 @@ function scanOutstanding(phasesDir: string, dir: string): { dir: string; outstan
(f) => !planDependencyGraphMod.isSummaryFileBlocked(path.join(phaseDirPath, f)),
);
const outstanding = coreUtilsMod.findUnsummarizedPlans(scan.planFiles, countableSummaries);
return { dir, outstanding };
return { dir, outstanding, planCount: scan.planFiles.length, summaryCount: countableSummaries.length };
}
function unsummarizedPlansForPositionPhase(
cwd: string,
positionPhase: string,
): { dir: string; outstanding: string[] } | null {
): { dir: string; outstanding: string[]; planCount: number; summaryCount: number } | null {
const phasesDir = planningPaths(cwd).phases;
// #3185 (ADR-3180 Decision 1): "which phase directories exist" is owned by
// listMilestonePhaseDirs — no hand-rolled readdirSync here. The owner
@@ -1003,7 +1008,12 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
// named here so the post-lock output path can report it without re-deriving.
// Holder (not a bare let) so TypeScript's closure-unaware narrowing cannot
// collapse the post-lock read to `never` — the callback assigns it.
const outstandingRef: { value: { dir: string; outstanding: string[] } | null } = { value: null };
const outstandingRef: { value: { dir: string; outstanding: string[]; planCount: number; summaryCount: number } | null } = { value: null };
// #4093: the position phase token the callback resolved (Current Position
// `Phase:` line first, frontmatter `current_phase` as fallback), carried out
// so the generic parse-failure decline can derive recovery facts from disk
// without re-reading STATE.md outside the lock. Same holder idiom as above.
const positionPhaseRef: { value: string | null } = { value: null };
const wrote = readModifyWriteStateMd(statePath, (content) => {
// advance-plan has no phase argument of its own — the phase it advances is
// whatever ## Current Position names. Compare that against the milestone
@@ -1013,6 +1023,17 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
const body = stripFrontmatter(content);
const positionScope = matchCurrentPositionSection(body) ?? body;
const positionPhase = parseProsePhaseField(stateExtractField(positionScope, 'Phase')).phase;
// #4093: a Current Position section with ZERO labeled fields has no
// `Phase:` line either; frontmatter `current_phase` is the documented
// survivor of body drift (the reporter's document still carried it, and
// `buildStateFrontmatter` re-derives it from the body only when the body
// HAS the line). It feeds the recovery DECLINE only — never a write.
let fmPhase: string | null = null;
if (positionPhase === null) {
const fmToken = extractFrontmatter(content, statePath)['current_phase'];
fmPhase = typeof fmToken === 'string' && fmToken.trim() !== '' ? fmToken.trim() : null;
}
positionPhaseRef.value = positionPhase ?? fmPhase;
if (positionPhase !== null) {
milestoneConflict = milestoneLockMod.checkMilestonePosition(cwd, positionPhase);
if (milestoneConflict) {
@@ -1100,7 +1121,58 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
}, raw, undefined);
return;
}
output({ error: advancePlanShapeError() }, raw, undefined);
// #4093: the generic terminus — no accepted labeled plan-position shape
// parsed anywhere in the document (the reporter's case: ## Current
// Position drifted to pure narrative prose with zero labeled fields).
// Every OTHER refusal above carries a machine-readable reason and the
// evidence to act on; this one stranded the caller at a bare sentence
// with no recovery path. Give it the same posture: a `reason` the caller
// can branch on, plus — when the position phase can be resolved and its
// directory scanned — the disk-derived facts and the exact labeled lines
// to re-insert. Nothing is WRITTEN: STATE.md is returned byte-identical
// (the callback already returned the original content for this path),
// so the decline is idempotent and no repair is guessed into the file —
// the caller (human or agent) applies the suggested lines and re-runs.
// Disk is the recovery source per #4067's posture; the values below are
// computed from the SAME `scanOutstanding` counts the plans_outstanding
// guard uses, so the two declines can never disagree about a phase.
const positionToken = positionPhaseRef.value;
const diskFacts = positionToken !== null
? unsummarizedPlansForPositionPhase(cwd, positionToken)
: null;
if (diskFacts === null) {
// No resolvable phase (no Phase: line, no current_phase frontmatter, or
// no matching phase directory / incomplete scan): keep today's shape
// error, plus the reason so callers can tell this refusal from the
// ambiguous_* ones without string-matching the sentence.
output({ error: advancePlanShapeError(), reason: 'plan_position_unreadable' }, raw, undefined);
return;
}
const planCount = diskFacts.planCount;
const summarized = diskFacts.summaryCount;
// A summarized count below the plan count means the next plan to execute
// is summarized+1; an equal count means the phase is done on disk and the
// position line should say so (current = total; the next advance-plan run
// takes the #4067-guarded phase-complete branch from it). Zero plan files
// means disk has no opinion — suggest nothing rather than `1 of 0`.
const payload: Record<string, unknown> = {
error: advancePlanShapeError(),
reason: 'plan_position_unreadable',
phase_dir: diskFacts.dir,
disk: { plan_count: planCount, summarized_count: summarized },
};
if (planCount > 0) {
const current = summarized < planCount ? summarized + 1 : planCount;
payload['suggested'] = {
current_plan: current,
total_plans: planCount,
lines: [`Current Plan: ${current}`, `Total Plans in Phase: ${planCount}`],
};
payload['error'] =
`${advancePlanShapeError()} Disk for phase ${diskFacts.dir}: ${summarized} of ${planCount} plan(s) summarized. ` +
`Re-insert a labeled plan position at the top of ## Current Position (e.g. Current Plan: ${current} with Total Plans in Phase: ${planCount}), then re-run.`;
}
output(payload, raw, undefined);
return;
}

View File

@@ -183,8 +183,20 @@ describe('fix-2657: compiled .cjs artifacts are gitignored, not tracked (ADR-457
});
test('lint-compiled-artifact-sync exits 0 with nothing left to check', () => {
// #4093 CI: this spawn is NOT the PROBE class the shared default below
// describes. The script's "nothing left to check" path still runs a FULL
// `tsc -p tsconfig.build.json` compile to a throwaway outDir whenever any
// compiled artifact remains tracked (ten are, deliberately — ADR-457's
// staged end state), and under CI shard load that compile can exceed the
// 15s probe budget, dying to a SIGTERM with empty piped stdout (observed
// twice on ubuntu shard 1/3). Per helpers/timeouts.cjs's own rule, a call
// site that genuinely differs from its class — "a real `tsc` compile" —
// keeps its own local constant with its own justifying comment; see
// tests/ensure-runtime-build.test.cjs's BUILD_TIMEOUT_MS for the other
// instance. 60s is that same class, sized for the cold-cache CI case.
const TSC_COMPILE_TIMEOUT_MS = 60000;
const args = [path.join(REPO_ROOT, 'scripts', 'lint-compiled-artifact-sync.cjs')];
const result = run(process.execPath, args);
const result = run(process.execPath, args, { timeoutMs: TSC_COMPILE_TIMEOUT_MS });
assert.equal(result.status, 0, describeFailure(process.execPath, args, result));
});
});

View File

@@ -2096,6 +2096,156 @@ describe('cmdStateAdvancePlan (state advance-plan)', () => {
assert.ok(updated.includes('Plan: 2 of 3'), 'Plan counter should advance to 2 of 3');
});
});
describe('cmdStateAdvancePlan #4093 zero-labeled-fields recovery decline', () => {
// The reporter's exact shape: ## Current Position has drifted to pure
// narrative prose — zero matches for Phase:/Plan:/Current Plan:/Total
// Plans in Phase: anywhere in the file, bold or plain — while frontmatter
// still carries current_phase and the phase directory still holds the
// plan/summary set that IS the position. advance-plan must not strand the
// caller at a bare "cannot parse" error: the decline carries a
// machine-readable reason plus the disk-derived facts needed to repair.
const narrativeFixture = [
'---',
'status: Executing',
'current_phase: 01',
'current_phase_name: Implementation',
'last_activity: 2026-09-01',
'---',
'',
'# Project State',
'',
'## Current Position',
'',
'**Phase 1 plan 2** (auth flow): EXECUTED — token round-trip verified end to end.',
"Follow-ups captured in plan 3's tasks.",
'',
'## Session',
'',
'Session ID: abc',
'',
].join('\n');
const seedPhaseDir = (dir, planCount, summaryCount) => {
const phaseDir = path.join(tmpDir, '.planning', 'phases', dir);
fs.mkdirSync(phaseDir, { recursive: true });
for (let i = 1; i <= planCount; i++) {
fs.writeFileSync(path.join(phaseDir, `01-0${i}-PLAN.md`), `# plan ${i}\n`);
}
for (let i = 1; i <= summaryCount; i++) {
fs.writeFileSync(path.join(phaseDir, `01-0${i}-SUMMARY.md`), `# summary ${i}\n`);
}
return phaseDir;
};
test('#4093 declines with disk-derived repair guidance when Current Position has zero labeled fields', () => {
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), narrativeFixture);
seedPhaseDir('01-impl', 2, 1);
const result = runGsdTools('state advance-plan', tmpDir);
assert.ok(result.success, `Command should exit 0: ${result.error}`);
const out = JSON.parse(result.output);
assert.ok(typeof out.error === 'string' && /cannot read the plan position/i.test(out.error),
`error should still name the unreadable plan position; got: ${JSON.stringify(out)}`);
assert.strictEqual(out.reason, 'plan_position_unreadable',
`reason should be plan_position_unreadable; got: ${JSON.stringify(out)}`);
assert.strictEqual(out.phase_dir, '01-impl',
`phase_dir should name the disk phase directory; got: ${JSON.stringify(out)}`);
assert.strictEqual(out.disk.plan_count, 2,
`disk.plan_count should count the 2 plan files; got: ${JSON.stringify(out.disk)}`);
assert.strictEqual(out.disk.summarized_count, 1,
`disk.summarized_count should count the 1 summary; got: ${JSON.stringify(out.disk)}`);
assert.strictEqual(out.suggested.current_plan, 2,
`next plan after 1 summarized of 2 is 2; got: ${JSON.stringify(out.suggested)}`);
assert.strictEqual(out.suggested.total_plans, 2,
`suggested total is the on-disk plan count; got: ${JSON.stringify(out.suggested)}`);
assert.ok(out.suggested.lines.includes('Current Plan: 2') && out.suggested.lines.includes('Total Plans in Phase: 2'),
`suggested lines should name the legacy pair to re-insert; got: ${JSON.stringify(out.suggested)}`);
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
assert.strictEqual(after, narrativeFixture,
'a recovery decline must leave STATE.md byte-identical');
});
test('#4093 resolves the position phase from the Phase line when frontmatter has none', () => {
const noFm = [
'# Project State',
'',
'## Current Position',
'',
'Phase: 2 — Build out',
'',
'All narrative from here; the labeled plan lines were displaced by executor notes.',
'',
].join('\n') + '\n';
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), noFm);
const phaseDir = path.join(tmpDir, '.planning', 'phases', '02-second');
fs.mkdirSync(phaseDir, { recursive: true });
for (let i = 1; i <= 3; i++) fs.writeFileSync(path.join(phaseDir, `02-0${i}-PLAN.md`), `# plan ${i}\n`);
fs.writeFileSync(path.join(phaseDir, '02-01-SUMMARY.md'), '# summary 1\n');
const result = runGsdTools('state advance-plan', tmpDir);
assert.ok(result.success, `Command should exit 0: ${result.error}`);
const out = JSON.parse(result.output);
assert.strictEqual(out.reason, 'plan_position_unreadable', `got: ${JSON.stringify(out)}`);
assert.strictEqual(out.phase_dir, '02-second',
`phase should resolve from the Phase: prose line; got: ${JSON.stringify(out)}`);
assert.strictEqual(out.suggested.current_plan, 2, `got: ${JSON.stringify(out.suggested)}`);
assert.strictEqual(out.suggested.total_plans, 3, `got: ${JSON.stringify(out.suggested)}`);
});
test('#4093 names the reason even when no phase can be resolved from disk or frontmatter', () => {
fs.writeFileSync(
path.join(tmpDir, '.planning', 'STATE.md'),
'# Project State\n\n## Current Position\n\nPure narrative, no labeled lines anywhere.\n',
);
const result = runGsdTools('state advance-plan', tmpDir);
assert.ok(result.success, `Command should exit 0: ${result.error}`);
const out = JSON.parse(result.output);
assert.strictEqual(out.reason, 'plan_position_unreadable', `got: ${JSON.stringify(out)}`);
assert.ok(/cannot read the plan position/i.test(out.error),
`the accepted-shape sentence must survive; got: ${out.error}`);
assert.strictEqual(out.phase_dir, undefined,
`no resolvable phase means no phase_dir; got: ${JSON.stringify(out)}`);
assert.strictEqual(out.disk, undefined, `no resolvable phase means no disk block`);
});
test('#4093 omits suggested values when the phase directory has no plan files', () => {
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), narrativeFixture);
seedPhaseDir('01-impl', 0, 0);
const result = runGsdTools('state advance-plan', tmpDir);
assert.ok(result.success, `Command should exit 0: ${result.error}`);
const out = JSON.parse(result.output);
assert.strictEqual(out.reason, 'plan_position_unreadable', `got: ${JSON.stringify(out)}`);
assert.strictEqual(out.phase_dir, '01-impl', `got: ${JSON.stringify(out)}`);
assert.strictEqual(out.disk.plan_count, 0, `got: ${JSON.stringify(out.disk)}`);
assert.strictEqual(out.suggested, undefined,
`zero plans on disk means nothing to suggest; got: ${JSON.stringify(out)}`);
});
test('#4093 gives the same recovery decline for a present-but-unreadable plan field', () => {
fs.writeFileSync(
path.join(tmpDir, '.planning', 'STATE.md'),
narrativeFixture.replace("Follow-ups captured in plan 3's tasks.", 'Plan: TBD'),
);
seedPhaseDir('01-impl', 2, 1);
const result = runGsdTools('state advance-plan', tmpDir);
assert.ok(result.success, `Command should exit 0: ${result.error}`);
const out = JSON.parse(result.output);
assert.strictEqual(out.reason, 'plan_position_unreadable', `got: ${JSON.stringify(out)}`);
assert.strictEqual(out.phase_dir, '01-impl', `got: ${JSON.stringify(out)}`);
assert.strictEqual(out.suggested.current_plan, 2, `got: ${JSON.stringify(out.suggested)}`);
assert.strictEqual(out.advanced, undefined, 'an unreadable position must never be advanced');
});
});
});
describe('cmdStateRecordMetric (state record-metric)', () => {