fix(#2372): widen decision-coverage-plan to all planner-canonical tags, drop misleading "(or body)" (#2443)
* fix(#2372): widen decision-coverage scan to planner-canonical tags, fix message Bug: check.decision-coverage-plan's remediation message told the user to cite decisions "(or body)" but extractPlanDesignatedSections only scanned <objective>/<tasks>/<task>/<action>. A decision cited in <read_first>, <behavior>, <verify>, <acceptance_criteria>, or <done> was invisible to the gate — false BLOCKING coverage gap, plus the message's own fix-hint sent the user to "the body" where re-citing still failed. Two-part fix (must change together — that drift was the bug): 1. Widen XML_DECISION_TAGS_RE in src/check-command-router.cts to also match <read_first>, <behavior>, <verify>, <acceptance_criteria>, <done>. These are all planner-canonical tags the planner is told to use (plan-phase.md:830-862, plan-phase.md:772). The body negative- lookahead mirrors the opening-tag set so each tag's body is captured independently. 2. Correct buildPlanMessage to name ONLY the surfaces the extractor actually scans (front-matter must_haves/truths/objective, designated markdown headings, and the nine planner-canonical tag bodies). The misleading "(or body)" clause is gone. Also updates the planner's documented contract (agents/gsd-planner.md:69) and user-facing docs (docs/CONFIGURATION.md, docs/USER-GUIDE.md) to reflect the wider scan. Regression tests in tests/decisions.test.cjs cover each newly-scanned tag body, a control (no citation still uncovered), and a message/extractor parity assertion that names every scanned surface — so the two cannot drift apart again. Out of scope (per triage): cmdDecisionCoverageVerify/buildVerifyMessage is a separate command (decision-coverage-verify) checking shipped artifacts, not plan citations — untouched. * chore(#2372): regenerate agent-size-baseline + golden-install-parity fixtures gsd-planner.md grew 49172 → 49294 (+122 chars) from the widened decision- coverage contract (5 new scanned tag names + heading clarification). Growth is justified: the contract surface is itself the fix — the prior text under-described what the gate scans, which was the bug. Updates: - tests/agent-size-baseline.json (gsd-planner.md: 49172 → 49294) - 17 tests/fixtures/golden-install-parity/*.json (one hash per runtime) - tests/fixtures/install-tree/*.json (regenerated by gen:golden) * fix(#2372): per-tag matching — outer-tag citations survive inner-tag nesting Code review (subagent) flagged a Medium edge-case regression from the single-alternation regex: when a newly-scanned tag nests inside another scanned tag, the alternation's negative lookahead halts the outer tag's body at the inner tag — losing any D-NN citation in the outer tag's prefix prose. Concretely: <action>per D-05 <verify>npm test</verify></action> → 3-tag alternation (old): captured 'per D-05 <verify>npm test</verify>' as <action> body → D-05 caught → 9-tag alternation (bug): captured 'npm test' only (from <verify>); D-05 in <action> prefix LOST Switches extractXmlTagBodies to per-tag matching: each tag gets its own regex whose negative-lookahead tempers only against the SAME tag's reopening. So <verify> inside <action> is absorbed into <action>'s body (D-05 caught) AND <verify> is matched separately on its own pass. Per-tag preserves both: - the reporter's case (sibling tags inside <read_first>) - nested-tag citations in outer-tag prefix prose - ReDoS safety (each per-tag regex keeps the #2128 body tempering) Also adds the reviewer's other requested edge-case tests: - non-scanned tag (<name>) bearing D-NN must NOT count - self-closing form <read_first /> safely ignored - attribute form <verify type="...">D-NN</verify> (canonical planner shape) - CRLF newlines in tag body do not break capture * chore(changeset): backfill pr:2443 in .changeset/noble-elks-chatter.md
This commit is contained in:
Tom Boucher
@@ -922,3 +922,205 @@ describe('parseDecisions — titled-colon bullet form (#1639)', () => {
|
||||
assert.equal(out.length, 13, 'all 13 titled-colon decisions must parse (not vacuously 0)');
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #2372: decision-coverage-plan must scan planner-canonical tag bodies ─────
|
||||
//
|
||||
// Bug: buildPlanMessage() told the user to cite decisions "(or body)" but
|
||||
// extractPlanDesignatedSections() only scanned <objective>/<tasks>/<task>/<action>.
|
||||
// A decision cited in <read_first>, <behavior>, <verify>, <acceptance_criteria>,
|
||||
// or <done> was invisible — false BLOCKING coverage gap, and the message sent the
|
||||
// fixer to "the body", where a re-citation still failed. Fix widens the scan to
|
||||
// the planner-canonical tag set AND corrects the message to name scanned surfaces.
|
||||
|
||||
describe('check.decision-coverage-plan — planner-canonical tag scanning (#2372)', () => {
|
||||
let tmpDir;
|
||||
let planningDir;
|
||||
let phaseDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject('gsd-2372-');
|
||||
planningDir = path.join(tmpDir, '.planning');
|
||||
phaseDir = path.join(planningDir, 'phases', '01-init');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
});
|
||||
|
||||
afterEach(() => cleanup(tmpDir));
|
||||
|
||||
// Common CONTEXT.md with a single trackable decision D-01.
|
||||
const CONTEXT_WITH_D01 = [
|
||||
'# Context',
|
||||
'',
|
||||
'<decisions>',
|
||||
'- **D-01:** Use OAuth 2.0 for authentication',
|
||||
'</decisions>',
|
||||
].join('\n');
|
||||
|
||||
// Helper: write CONTEXT + a PLAN whose body wraps `innerTagBody` in `tagName`.
|
||||
function writeContextAndPlan(tagName, innerTagBody) {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
writePlanFile(phaseDir, '01', `# Plan\n\n<tasks>\n<task>\n <${tagName}>\n${innerTagBody}\n </${tagName}>\n</task>\n</tasks>\n`);
|
||||
}
|
||||
|
||||
const cases = [
|
||||
{ tag: 'read_first', citation: '- path/to/CONTEXT.md (D-01 — auth decision)' },
|
||||
{ tag: 'behavior', citation: 'Honor D-01: redirect unauthenticated users to OAuth flow.' },
|
||||
{ tag: 'verify', citation: 'Verify D-01: token exchange returns 200 with access_token.' },
|
||||
{ tag: 'acceptance_criteria', citation: 'D-01 honored: every protected route requires a valid OAuth token.' },
|
||||
{ tag: 'done', citation: 'D-01 implemented — OAuth 2.0 flow live.' },
|
||||
];
|
||||
|
||||
for (const { tag, citation } of cases) {
|
||||
test(`D-NN cited in <${tag}> body → covered (no false gap)`, () => {
|
||||
writeContextAndPlan(tag, citation);
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.total, 1, `expected total=1, got ${JSON.stringify(parsed)}`);
|
||||
assert.strictEqual(parsed.covered, 1, `D-01 cited in <${tag}> must count as covered. Got: ${JSON.stringify(parsed)}`);
|
||||
assert.strictEqual(parsed.passed, true, `gate must pass when D-01 is cited in <${tag}>. Got: ${JSON.stringify(parsed)}`);
|
||||
assert.strictEqual(parsed.uncovered.length, 0, `uncovered must be empty. Got: ${JSON.stringify(parsed.uncovered)}`);
|
||||
});
|
||||
}
|
||||
|
||||
test('control: D-NN cited nowhere → still uncovered (no false green from widening)', () => {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
writePlanFile(phaseDir, '01', '# Plan\n\n<tasks>\n<task>\n <action>\n Implement the feature.\n </action>\n</task>\n</tasks>\n');
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.total, 1);
|
||||
assert.strictEqual(parsed.covered, 0);
|
||||
assert.strictEqual(parsed.passed, false);
|
||||
assert.strictEqual(parsed.uncovered.length, 1);
|
||||
assert.strictEqual(parsed.uncovered[0].id, 'D-01');
|
||||
});
|
||||
|
||||
// Message/extractor parity: the remediation text must name ONLY the surfaces the
|
||||
// extractor actually scans. Asserts no "(or body)" claim it doesn't back, AND
|
||||
// that every newly-scanned tag is named in the message — so the two cannot drift
|
||||
// apart again. The reporter's bug was exactly this drift.
|
||||
test('buildPlanMessage names every scanned surface (no "(or body)" drift, #2372)', () => {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
writePlanFile(phaseDir, '01', '# Plan\nNo decision citation in any scanned surface.\n');
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.passed, false, 'fixture intentionally leaves D-01 uncovered');
|
||||
const msg = parsed.message || '';
|
||||
|
||||
// The misleading "(or body)" clause that promised a scope the extractor didn't implement is gone.
|
||||
assert.ok(!/\(or body\)/i.test(msg), `message must not claim "(or body)" — that was the bug. Got: "${msg}"`);
|
||||
|
||||
// Every surface the extractor now scans MUST appear by name in the message — if any is
|
||||
// missing, the message has drifted from the scan again.
|
||||
const requiredSurfaceNames = [
|
||||
'must_haves', 'truths', 'objective',
|
||||
'<objective>', '<tasks>', '<task>', '<action>',
|
||||
'<read_first>', '<behavior>', '<verify>', '<acceptance_criteria>', '<done>',
|
||||
];
|
||||
for (const surface of requiredSurfaceNames) {
|
||||
assert.ok(
|
||||
msg.includes(surface),
|
||||
`message must name scanned surface "${surface}" — otherwise the message/extractor drift apart. Got: "${msg}"`
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
// Reviewer-driven edge cases (code review on the initial widening flagged these):
|
||||
//
|
||||
// 1. Nested scanned tag inside another scanned tag: a citation in the OUTER tag's
|
||||
// prefix prose must still count. Initial widening used a single alternation whose
|
||||
// negative lookahead halted the outer tag's body at any inner scanned tag — losing
|
||||
// the prefix citation. Switched to per-tag matching so each tag's body terminates
|
||||
// only at its own closing tag (other scanned tags pass through as text into this body).
|
||||
// 2. Non-scanned tag bearing a D-NN citation must NOT count toward coverage — guards
|
||||
// against future over-widening.
|
||||
// 3. Self-closing form `<read_first />` has no body and must not error or match.
|
||||
// 4. Attribute form `<verify type="automated">D-01</verify>` is the canonical planner
|
||||
// shape for <verify> and must match.
|
||||
// 5. CRLF newlines inside tag bodies must not break capture.
|
||||
|
||||
test('D-NN in outer scanned tag prefix is not lost when inner scanned tag follows (per-tag capture)', () => {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
// The bug shape: <action>per D-01 <verify>...</verify></action> — D-01 lives in <action>'s prefix.
|
||||
writePlanFile(phaseDir, '01', [
|
||||
'# Plan',
|
||||
'',
|
||||
'<tasks>',
|
||||
'<task>',
|
||||
' <action>',
|
||||
' Implement per D-01 — OAuth 2.0 flow.',
|
||||
' <verify>token exchange returns 200</verify>',
|
||||
' </action>',
|
||||
'</task>',
|
||||
'</tasks>',
|
||||
].join('\n'));
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.covered, 1, `D-01 in <action> prefix must be caught (per-tag capture). Got: ${JSON.stringify(parsed)}`);
|
||||
assert.strictEqual(parsed.passed, true);
|
||||
});
|
||||
|
||||
test('D-NN inside a non-scanned tag body does NOT count toward coverage', () => {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
// <name> is not in the scanned set — citation here must not be picked up.
|
||||
writePlanFile(phaseDir, '01', '# Plan\n\n<name>per D-01</name>\n');
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.total, 1);
|
||||
assert.strictEqual(parsed.covered, 0, 'non-scanned tag body must not count. Got: ' + JSON.stringify(parsed));
|
||||
assert.strictEqual(parsed.passed, false);
|
||||
assert.strictEqual(parsed.uncovered.length, 1);
|
||||
});
|
||||
|
||||
test('self-closing scanned tag form is safely ignored (no body to scan)', () => {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
// Self-closing form has no body. The gate must not crash and must not match the (absent) body.
|
||||
writePlanFile(phaseDir, '01', '# Plan\n\n<tasks>\n<task>\n<read_first />\n<action>Implement feature.</action>\n</task>\n</tasks>\n');
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.total, 1);
|
||||
assert.strictEqual(parsed.covered, 0);
|
||||
assert.strictEqual(parsed.passed, false);
|
||||
});
|
||||
|
||||
test('attribute form `<verify type="...">D-NN</verify>` is scanned (canonical planner shape)', () => {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
writePlanFile(phaseDir, '01', '# Plan\n\n<tasks>\n<task>\n<verify type="automated">Run npm test per D-01</verify>\n</task>\n</tasks>\n');
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.covered, 1, 'attribute form on scanned tag must match. Got: ' + JSON.stringify(parsed));
|
||||
assert.strictEqual(parsed.passed, true);
|
||||
});
|
||||
|
||||
test('CRLF newlines inside scanned tag body do not break capture', () => {
|
||||
writeContextFile(phaseDir, CONTEXT_WITH_D01);
|
||||
const planBody = ['# Plan', '', '<tasks>', '<task>', ' <action>', ' Implement per D-01.', ' </action>', '</task>', '</tasks>', ''].join('\r\n');
|
||||
fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), planBody);
|
||||
|
||||
const contextPath = path.join(phaseDir, 'CONTEXT.md');
|
||||
const result = runDecisionCoveragePlan(phaseDir, contextPath, tmpDir);
|
||||
const parsed = JSON.parse(result.output || '{}');
|
||||
|
||||
assert.strictEqual(parsed.covered, 1, 'CRLF body must not break capture. Got: ' + JSON.stringify(parsed));
|
||||
assert.strictEqual(parsed.passed, true);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user