* fix(#1343): parse decision bullets with text before the colon parseDecisions() silently dropped any `- **D-NN ...:**` decision bullet whose header had freeform text (a parenthetical, em-dash, or prose) before the `:**`, so the blocking check.decision-coverage-plan gate computed coverage over a narrowed set and reported a false pass. - Broaden bulletRe to tolerate a freeform run before the colon while preserving the optional [bracket] tag capture (drives `trackable`). - Add a parse-miss guard: a line that looks like a D-NN bullet but still fails the regex flushes the current decision and warns instead of vanishing — the gate-integrity floor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#1343): add changeset for decision-coverage false-pass fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#1343): relocate decision-parser regression into owning module test file CI's lint-regression-test-names bans new bug-NNNN-*.test.cjs files. Move the 9 regression cases from tests/bug-1343-parsedecisions-drop.test.cjs into the owning parser test file tests/post-planning-gaps-2493.test.cjs (which already exercises parseDecisions) and delete the banned file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
6
.changeset/1343-decision-coverage-false-pass.md
Normal file
6
.changeset/1343-decision-coverage-false-pass.md
Normal file
@@ -0,0 +1,6 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 1358
|
||||
---
|
||||
|
||||
**`check.decision-coverage-plan` no longer reports a false pass when a `D-NN` decision header has text before the colon** — `parseDecisions` previously dropped any `- **D-NN …:**` bullet whose header contained a `(parenthetical)`, em-dash, or other prose before the `:**`, silently narrowing the trackable set so the blocking coverage gate green-lit a phase whose dropped decisions were never checked. The parser now tolerates a freeform run before the colon (preserving `[bracket]` tags) and warns on any `D-NN` bullet it still cannot parse instead of dropping it. (#1343)
|
||||
@@ -70,7 +70,13 @@ export function parseDecisions(content: unknown): Decision[] {
|
||||
// in addition to numeric-only IDs (D-42). The first character after `D-` must
|
||||
// be alphanumeric, so malformed shapes like `D--foo` or `D-_bar` are rejected.
|
||||
// CJS callers consume {id, text} and ignore the optional extras.
|
||||
const bulletRe = /^\s*-\s+\*\*D-([A-Za-z0-9][A-Za-z0-9_-]*)(?:\s*\[([^\]]+)\])?\s*:\*\*\s*(.*)$/;
|
||||
// #1343: `[^:*]*` replaces the old `\s*` before `:**` so that a freeform run
|
||||
// such as `(parenthetical)`, an em-dash, or other prose between the optional
|
||||
// bracket-tag group and the closing `:**` is tolerated rather than silently
|
||||
// dropping the whole decision. `[^:*]*` subsumes plain whitespace and stops
|
||||
// correctly at `:**`. Capture groups 1 (id), 2 (bracket tags), 3 (text) are
|
||||
// unchanged.
|
||||
const bulletRe = /^\s*-\s+\*\*D-([A-Za-z0-9][A-Za-z0-9_-]*)(?:\s*\[([^\]]+)\])?[^:*]*:\*\*\s*(.*)$/;
|
||||
let current: Decision | null = null;
|
||||
const flush = (): void => {
|
||||
if (current) {
|
||||
@@ -111,6 +117,18 @@ export function parseDecisions(content: unknown): Decision[] {
|
||||
current = { id, text: bulletMatch[3], category, tags, trackable };
|
||||
continue;
|
||||
}
|
||||
// Parse-miss guard (#1343): a line that looks like a `D-NN` decision bullet
|
||||
// but failed `bulletRe` (e.g. a `:` or `*` inside the pre-colon run) must NOT
|
||||
// be silently dropped — a narrowed trackable set lets a blocking coverage gate
|
||||
// report a false pass. Surface it loudly instead.
|
||||
if (/^\s*-\s+\*\*D-/.test(line)) {
|
||||
// A malformed D-bullet still starts a (failed) new decision, so it ends the
|
||||
// previous one — flush before warning so a following continuation line cannot
|
||||
// be mis-appended to the prior valid decision.
|
||||
flush();
|
||||
console.warn(`parseDecisions: ignored unparseable decision bullet: ${trimmed}`);
|
||||
continue;
|
||||
}
|
||||
// Continuation line for current decision (indented with space OR tab,
|
||||
// non-bullet, non-empty) — tab indentation must work too (review F12).
|
||||
if (current && trimmed !== '' && !trimmed.startsWith('-') && /^[ \t]/.test(line)) {
|
||||
|
||||
@@ -30,6 +30,7 @@ const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const REPO_ROOT = path.join(__dirname, '..');
|
||||
const PLAN_PHASE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'plan-phase.md');
|
||||
const { parseDecisions } = require('../gsd-core/bin/lib/decisions.cjs');
|
||||
|
||||
// ─── Workflow file structure ──────────────────────────────────────────────────
|
||||
|
||||
@@ -82,8 +83,6 @@ describe('plan-phase.md Step 13e insertion (#2493)', () => {
|
||||
// ─── Decisions parser ────────────────────────────────────────────────────────
|
||||
|
||||
describe('decisions.cjs parser (shared with #2492)', () => {
|
||||
const { parseDecisions } = require('../gsd-core/bin/lib/decisions.cjs');
|
||||
|
||||
test('extracts D-NN entries from a <decisions> block', () => {
|
||||
const md = `
|
||||
<decisions>
|
||||
@@ -446,3 +445,181 @@ describe('workflow.post_planning_gaps config (#2493)', () => {
|
||||
assert.strictEqual(config.post_planning_gaps, true);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #1343 regression suite ──────────────────────────────────────────────────
|
||||
|
||||
// helper shared across #1343 cases
|
||||
function wrapDecisions(body) {
|
||||
return `<decisions>\n## Decisions\n\n${body}\n</decisions>\n`;
|
||||
}
|
||||
|
||||
describe('#1343 — parseDecisions tolerates freeform text before the colon (regressions)', () => {
|
||||
// ── 1. Parenthetical before colon now parses ─────────────────────────────
|
||||
|
||||
test('all three ids extracted when one bullet has a parenthetical before :**', () => {
|
||||
const md = wrapDecisions(
|
||||
'- **D-01:** a\n' +
|
||||
'- **D-02 (note before colon):** b\n' +
|
||||
'- **D-03 [robust]:** c\n'
|
||||
);
|
||||
const ds = parseDecisions(md);
|
||||
assert.deepStrictEqual(
|
||||
ds.map(d => d.id),
|
||||
['D-01', 'D-02', 'D-03'],
|
||||
'D-02 with parenthetical must not be dropped'
|
||||
);
|
||||
assert.strictEqual(ds.length, 3);
|
||||
});
|
||||
|
||||
test('text is preserved for parenthetical bullet', () => {
|
||||
const md = wrapDecisions('- **D-02 (note before colon):** b\n');
|
||||
const ds = parseDecisions(md);
|
||||
assert.strictEqual(ds[0].id, 'D-02');
|
||||
assert.strictEqual(ds[0].text, 'b');
|
||||
});
|
||||
|
||||
// ── 2. Bracket tags still captured + drive trackable ────────────────────
|
||||
|
||||
test('[informational] tag makes trackable:false', () => {
|
||||
const md = wrapDecisions(
|
||||
'- **D-04 [informational]:** x\n' +
|
||||
'- **D-05:** y\n'
|
||||
);
|
||||
const ds = parseDecisions(md);
|
||||
const d04 = ds.find(d => d.id === 'D-04');
|
||||
const d05 = ds.find(d => d.id === 'D-05');
|
||||
assert.ok(d04, 'D-04 must be present');
|
||||
assert.ok(d04.tags.includes('informational'), 'D-04 tags must include informational');
|
||||
assert.strictEqual(d04.trackable, false, 'D-04 must be non-trackable');
|
||||
assert.ok(d05, 'D-05 must be present');
|
||||
assert.strictEqual(d05.trackable, true, 'D-05 must be trackable');
|
||||
});
|
||||
|
||||
// ── 3. Bracket + parenthetical together ─────────────────────────────────
|
||||
|
||||
test('D-06 [robust] (note) parses correctly', () => {
|
||||
const md = wrapDecisions('- **D-06 [robust] (note):** z\n');
|
||||
const ds = parseDecisions(md);
|
||||
assert.strictEqual(ds.length, 1);
|
||||
assert.strictEqual(ds[0].id, 'D-06');
|
||||
assert.ok(ds[0].tags.includes('robust'), 'tags must include robust');
|
||||
assert.strictEqual(ds[0].text, 'z');
|
||||
});
|
||||
|
||||
// ── 4. Parse-miss WARN floor ─────────────────────────────────────────────
|
||||
|
||||
test('genuinely unparseable bullet (colon inside pre-colon run) is excluded and warns', () => {
|
||||
// `D-07 ratio 3:1` has a colon in the pre-colon run; after [^:*]* matches up
|
||||
// to the first colon, the `:**` anchor fails → bulletRe does not match → falls
|
||||
// through to the parse-miss guard, which must warn and skip.
|
||||
const md = wrapDecisions('- **D-07 ratio 3:1:** w\n');
|
||||
|
||||
const warnMessages = [];
|
||||
const origWarn = console.warn;
|
||||
try {
|
||||
console.warn = (...args) => {
|
||||
warnMessages.push(args.join(' '));
|
||||
};
|
||||
const ds = parseDecisions(md);
|
||||
assert.strictEqual(ds.length, 0, 'unparseable bullet must be excluded from results');
|
||||
} finally {
|
||||
console.warn = origWarn;
|
||||
}
|
||||
|
||||
assert.ok(
|
||||
warnMessages.some(m => m.includes('D-07')),
|
||||
`expected a console.warn mentioning D-07, got: ${JSON.stringify(warnMessages)}`
|
||||
);
|
||||
});
|
||||
|
||||
// ── Test A — malformed D-bullet must not corrupt previous decision's text ─
|
||||
|
||||
test('D-02 malformed flush: D-01 text stays clean, continuation does not attach', () => {
|
||||
// D-02 has a colon inside the pre-colon run ("ratio 3:1"), so bulletRe rejects
|
||||
// it and the parse-miss guard fires. Before this fix the guard skipped WITHOUT
|
||||
// flushing, leaving current=D-01; the following indented continuation line was
|
||||
// then mis-appended to D-01's text.
|
||||
const md = wrapDecisions(
|
||||
'- **D-01:** first decision\n' +
|
||||
'- **D-02 ratio 3:1:** malformed (unparseable, has colon in pre-colon run)\n' +
|
||||
' indented continuation that must NOT attach to D-01\n' +
|
||||
'- **D-03:** third decision\n'
|
||||
);
|
||||
|
||||
const warnMessages = [];
|
||||
const origWarn = console.warn;
|
||||
try {
|
||||
console.warn = (...args) => { warnMessages.push(args.join(' ')); };
|
||||
const ds = parseDecisions(md);
|
||||
|
||||
assert.deepStrictEqual(
|
||||
ds.map(d => d.id),
|
||||
['D-01', 'D-03'],
|
||||
'only D-01 and D-03 should be present (D-02 dropped)'
|
||||
);
|
||||
|
||||
const d01 = ds.find(d => d.id === 'D-01');
|
||||
assert.ok(d01, 'D-01 must be present');
|
||||
assert.strictEqual(
|
||||
d01.text,
|
||||
'first decision',
|
||||
'D-01 text must be exactly "first decision", not polluted by continuation'
|
||||
);
|
||||
assert.ok(!d01.text.includes('indented'), 'D-01 text must NOT contain "indented"');
|
||||
|
||||
const d03 = ds.find(d => d.id === 'D-03');
|
||||
assert.ok(d03, 'D-03 must be present');
|
||||
} finally {
|
||||
console.warn = origWarn;
|
||||
}
|
||||
|
||||
assert.ok(
|
||||
warnMessages.some(m => m.includes('D-02')),
|
||||
`expected a console.warn mentioning D-02, got: ${JSON.stringify(warnMessages)}`
|
||||
);
|
||||
});
|
||||
|
||||
// ── Test B — malformed/unterminated bracket tag: safe tagless trackable ──
|
||||
|
||||
test('D-09 [informational: (missing ]) yields tags=[] and trackable=true', () => {
|
||||
// "- **D-09 [informational:** body" has no closing `]` so the optional bracket
|
||||
// group in bulletRe does not match, leaving tags=[]. The decision is still
|
||||
// captured because the ID and `:**` are intact.
|
||||
//
|
||||
// This is the intentional SAFE DIRECTION for the coverage gate: a tagless
|
||||
// trackable decision can only make the gate STRICTER (it counts toward required
|
||||
// decisions), never produce a false pass. An alternative that silently turned
|
||||
// the decision non-trackable could allow a gate bypass.
|
||||
const md = wrapDecisions('- **D-09 [informational:** body\n');
|
||||
const ds = parseDecisions(md);
|
||||
assert.strictEqual(ds.length, 1, 'D-09 should be present');
|
||||
assert.strictEqual(ds[0].id, 'D-09');
|
||||
assert.deepStrictEqual(ds[0].tags, [], 'tags must be empty (unclosed bracket not parsed)');
|
||||
assert.strictEqual(ds[0].trackable, true, 'trackable must be true (no non-trackable tag matched)');
|
||||
});
|
||||
|
||||
// ── 5. Non-regression: plain bullet + continuation ───────────────────────
|
||||
|
||||
test('D-08 with continuation line parses correctly', () => {
|
||||
const md = wrapDecisions(
|
||||
'- **D-08:** ok\n' +
|
||||
' continuation text here\n'
|
||||
);
|
||||
const ds = parseDecisions(md);
|
||||
assert.strictEqual(ds.length, 1);
|
||||
assert.strictEqual(ds[0].id, 'D-08');
|
||||
assert.ok(ds[0].text.includes('ok'), 'text must include bullet text');
|
||||
assert.ok(ds[0].text.includes('continuation'), 'text must include continuation');
|
||||
});
|
||||
|
||||
test('existing numeric and bracket forms unchanged', () => {
|
||||
const md = wrapDecisions(
|
||||
'- **D-42:** numeric id\n' +
|
||||
'- **D-INFRA-01 [deferred]:** alphanumeric id\n'
|
||||
);
|
||||
const ds = parseDecisions(md);
|
||||
assert.deepStrictEqual(ds.map(d => d.id), ['D-42', 'D-INFRA-01']);
|
||||
assert.ok(ds[1].tags.includes('deferred'));
|
||||
assert.strictEqual(ds[1].trackable, false);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user