* fix(#447): scope post-planning gap analysis to phase_req_ids §13e gap-analysis diffed the entire REQUIREMENTS.md against a phase's plans even when the phase mapped no REQ-IDs, so a phase mapping nothing reported every unrelated project requirement as "not covered". Teach the gap-analysis CLI a --phase-req-ids option (null/TBD skips the requirements comparison, an ID list scopes to it, absent = unchanged back-compat) and have §13e pass the phase's mapped IDs — mirroring the §13 Requirements Coverage Gate's null/TBD skip. CONTEXT.md decisions stay in scope regardless. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#447): source phase_req_ids from init.plan-phase, not roadmap.get-phase Adversarial-review follow-up. roadmap.get-phase returns the raw phase markdown (not JSON), so `--pick phase_req_ids` yielded nothing and §13e would have skipped the requirements comparison for EVERY phase — a silent regression. phase_req_ids is exposed by init.plan-phase; switch §13e to it. Adds an integration test asserting the query exposes the IDs and that gap-analysis scopes to them (and skips when unmapped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#447): add changeset fragment for PR #538 * fix(#447): surface mapped REQ-IDs absent from REQUIREMENTS.md as explicit missing rows Previously, a phase_req_ids entry not found in REQUIREMENTS.md was silently dropped from the gap report, allowing the analysis to falsely report full coverage when the requirement document had drifted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: update changeset to cover missing-REQ-ID detection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/daring-yaks-zip.md
Normal file
5
.changeset/daring-yaks-zip.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 538
|
||||
---
|
||||
**`/gsd-plan-phase` §13e post-planning gap analysis now scopes to the phase's mapped REQ-IDs** — a phase that maps no requirements (`phase_req_ids` null/TBD) no longer reports every unrelated project requirement as "not covered". CONTEXT.md decisions remain in scope. Mapped REQ-IDs that are listed in the roadmap but absent from `REQUIREMENTS.md` are now surfaced as explicit "⚠ Missing from REQUIREMENTS.md" rows instead of being silently dropped, preventing false "all covered" reports when the requirements document has drifted.
|
||||
@@ -99,7 +99,9 @@ function formatGapTable(rows) {
|
||||
}
|
||||
const header = '| Source | Item | Status |\n|--------|------|--------|';
|
||||
const body = rows.map(r => {
|
||||
const tick = r.status === 'Covered' ? '\u2713 Covered' : '\u2717 Not covered';
|
||||
const tick = r.status === 'Covered' ? '\u2713 Covered'
|
||||
: r.status === 'Missing from REQUIREMENTS.md' ? '\u26a0 Missing from REQUIREMENTS.md'
|
||||
: '\u2717 Not covered';
|
||||
return `| ${r.source} | ${r.item} | ${tick} |`;
|
||||
}).join('\n');
|
||||
return `## Post-Planning Gap Analysis\n\n${header}\n${body}\n`;
|
||||
@@ -116,7 +118,30 @@ function readGate(cwd) {
|
||||
return true;
|
||||
}
|
||||
|
||||
function runGapAnalysis(cwd, phaseDir) {
|
||||
/**
|
||||
* Normalize a raw `--phase-req-ids` argument into the scoping signal used by
|
||||
* runGapAnalysis (#447). Mirrors §13's null/TBD skip semantics.
|
||||
*
|
||||
* undefined → flag absent: compare the whole REQUIREMENTS.md (back-compat)
|
||||
* null | '' | TBD → no requirements mapped to this phase: skip the comparison
|
||||
* "REQ-01,REQ-02" → restrict the comparison to these IDs
|
||||
*
|
||||
* Tolerates JSON-array-ish input (`["REQ-01","REQ-02"]`) since callers may pass
|
||||
* the roadmap value through verbatim.
|
||||
*/
|
||||
function normalizePhaseReqIds(rawVal) {
|
||||
if (rawVal === undefined) return undefined;
|
||||
if (rawVal === null) return null;
|
||||
const v = String(rawVal).replace(/["'[\]()]/g, '').trim();
|
||||
if (v === '' || /^(null|tbd|none)$/i.test(v)) return null;
|
||||
// Tolerate comma-, space-, or newline-separated lists (callers may pass the
|
||||
// roadmap value verbatim, whose serialization is not guaranteed).
|
||||
const ids = v.split(/[\s,]+/).map(s => s.trim()).filter(Boolean);
|
||||
return ids.length === 0 ? null : ids;
|
||||
}
|
||||
|
||||
function runGapAnalysis(cwd, phaseDir, options = {}) {
|
||||
const phaseReqIds = normalizePhaseReqIds(options.phaseReqIds);
|
||||
if (!readGate(cwd)) {
|
||||
return {
|
||||
enabled: false,
|
||||
@@ -131,7 +156,21 @@ function runGapAnalysis(cwd, phaseDir) {
|
||||
|
||||
const reqPath = planningPaths(cwd).requirements;
|
||||
const reqMd = fs.existsSync(reqPath) ? fs.readFileSync(reqPath, 'utf-8') : '';
|
||||
const reqItems = parseRequirements(reqMd).map(r => ({ ...r, source: 'REQUIREMENTS.md' }));
|
||||
let reqItems = parseRequirements(reqMd).map(r => ({ ...r, source: 'REQUIREMENTS.md' }));
|
||||
|
||||
// Scope the requirements comparison to the phase's mapped REQ-IDs (#447).
|
||||
// A phase that maps no requirements (phase_req_ids null/TBD) must not report
|
||||
// every unrelated project REQ-ID as a gap — mirror §13's skip behavior.
|
||||
// CONTEXT.md decisions (below) are always in scope regardless.
|
||||
let ghostReqIds = [];
|
||||
if (phaseReqIds === null) {
|
||||
reqItems = [];
|
||||
} else if (Array.isArray(phaseReqIds)) {
|
||||
const wanted = new Set(phaseReqIds);
|
||||
const foundIds = new Set(reqItems.map(r => r.id));
|
||||
reqItems = reqItems.filter(r => wanted.has(r.id));
|
||||
ghostReqIds = phaseReqIds.filter(id => !foundIds.has(id));
|
||||
}
|
||||
|
||||
// Read the phase directory once; reuse the listing for both context detection
|
||||
// and plan-file enumeration (avoids redundant readdirSync calls).
|
||||
@@ -168,9 +207,12 @@ function runGapAnalysis(cwd, phaseDir) {
|
||||
};
|
||||
}
|
||||
|
||||
const rows = sortRows(detectCoverage(items, planText));
|
||||
const uncovered = rows.filter(r => r.status === 'Not covered').length;
|
||||
const covered = rows.length - uncovered;
|
||||
const rows = sortRows([
|
||||
...detectCoverage(items, planText),
|
||||
...ghostReqIds.map(id => ({ source: 'REQUIREMENTS.md', item: id, status: 'Missing from REQUIREMENTS.md' })),
|
||||
]);
|
||||
const covered = rows.filter(r => r.status === 'Covered').length;
|
||||
const uncovered = rows.length - covered;
|
||||
|
||||
const summary = uncovered === 0
|
||||
? `\u2713 All ${rows.length} items covered by plans`
|
||||
@@ -191,7 +233,13 @@ function cmdGapAnalysis(cwd, args, raw) {
|
||||
error('Usage: gap-analysis --phase-dir <path-to-phase-directory>');
|
||||
}
|
||||
const phaseDir = args[idx + 1];
|
||||
const result = runGapAnalysis(cwd, phaseDir);
|
||||
|
||||
// Optional --phase-req-ids scopes the requirements comparison (#447).
|
||||
// Absent → compare the whole REQUIREMENTS.md (back-compat).
|
||||
const reqIdx = args.indexOf('--phase-req-ids');
|
||||
const phaseReqIds = reqIdx === -1 ? undefined : (args[reqIdx + 1] ?? '');
|
||||
|
||||
const result = runGapAnalysis(cwd, phaseDir, { phaseReqIds });
|
||||
output(result, raw, result.table || result.summary);
|
||||
}
|
||||
|
||||
@@ -200,6 +248,7 @@ module.exports = {
|
||||
detectCoverage,
|
||||
formatGapTable,
|
||||
sortRows,
|
||||
normalizePhaseReqIds,
|
||||
runGapAnalysis,
|
||||
cmdGapAnalysis,
|
||||
};
|
||||
|
||||
@@ -1627,7 +1627,8 @@ one place before execution begins.
|
||||
```bash
|
||||
POST_PLANNING_GAPS=$(gsd_run query config-get workflow.post_planning_gaps --default true 2>/dev/null || echo true)
|
||||
if [ "$POST_PLANNING_GAPS" = "true" ]; then
|
||||
node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" gap-analysis --phase-dir "${PHASE_DIR}"
|
||||
# Scope to this phase's mapped REQ-IDs (#447); null/TBD skips the requirements comparison (CONTEXT.md decisions still reported), mirroring §13.
|
||||
node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" gap-analysis --phase-dir "${PHASE_DIR}" --phase-req-ids "$(node "$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" query init.plan-phase "$PHASE" --pick phase_req_ids 2>/dev/null || echo TBD)"
|
||||
fi
|
||||
```
|
||||
|
||||
|
||||
226
tests/bug-447-gap-analysis-phase-req-ids.test.cjs
Normal file
226
tests/bug-447-gap-analysis-phase-req-ids.test.cjs
Normal file
@@ -0,0 +1,226 @@
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* Bug #447: plan-phase §13e gap-analysis ignores phase_req_ids → false-positive
|
||||
* coverage gaps.
|
||||
*
|
||||
* Root cause: runGapAnalysis() diffs the ENTIRE REQUIREMENTS.md against the
|
||||
* phase's plans, with no awareness of phase_req_ids. §13 (Requirements Coverage
|
||||
* Gate) skips when phase_req_ids is null/TBD, but §13e never inherited that
|
||||
* scoping contract — so a phase that maps no requirements reports every
|
||||
* unrelated project REQ-ID as "Not covered".
|
||||
*
|
||||
* Fix: teach the gap-analysis CLI a --phase-req-ids option (the durable home for
|
||||
* the scoping contract), mirroring §13:
|
||||
* - null / TBD / empty → skip the REQUIREMENTS.md comparison entirely
|
||||
* (CONTEXT.md decisions are still reported).
|
||||
* - explicit ID list → restrict the comparison to those IDs.
|
||||
* - flag absent → backward-compatible (compare the whole file).
|
||||
*/
|
||||
|
||||
const { describe, test, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
describe('gap-analysis --phase-req-ids scoping (#447)', () => {
|
||||
let tmpDir;
|
||||
let phaseDir;
|
||||
|
||||
function writeRequirements(ids) {
|
||||
const lines = ids.map((id, i) => `- [ ] **${id}** Requirement ${i + 1} description`);
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'REQUIREMENTS.md'),
|
||||
`# Requirements\n\n${lines.join('\n')}\n`);
|
||||
}
|
||||
|
||||
function writeContext(decisions) {
|
||||
const dLines = decisions.map(d => `- **${d.id}:** ${d.text}`).join('\n');
|
||||
fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'),
|
||||
`# Phase Context\n\n<decisions>\n## Implementation Decisions\n\n${dLines}\n</decisions>\n`);
|
||||
}
|
||||
|
||||
function writePlan(name, body) {
|
||||
fs.writeFileSync(path.join(phaseDir, `${name}-PLAN.md`), body);
|
||||
}
|
||||
|
||||
function reqRows(out) {
|
||||
return out.rows.filter(r => r.source === 'REQUIREMENTS.md').map(r => r.item);
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
phaseDir = path.join(tmpDir, '.planning', 'phases', '01-test');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
const r = runGsdTools('config-ensure-section', tmpDir);
|
||||
assert.ok(r.success, `config-ensure-section failed: ${r.error}`);
|
||||
});
|
||||
|
||||
afterEach(() => cleanup(tmpDir));
|
||||
|
||||
// ── The core bug ─────────────────────────────────────────────────────────────
|
||||
|
||||
test('phase mapping no REQs (--phase-req-ids TBD) reports zero REQUIREMENTS.md rows', () => {
|
||||
// A REQUIREMENTS.md full of IDs that belong to OTHER phases/milestones.
|
||||
writeRequirements(['BACK-01', 'WEB-03', 'API-07', 'DATA-02']);
|
||||
writePlan('01', '# Plan 1\n\nStandalone phase, maps no project requirements.\n');
|
||||
|
||||
const r = runGsdTools(
|
||||
['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', 'TBD'], tmpDir);
|
||||
assert.ok(r.success, `gap-analysis failed: ${r.error}`);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
assert.deepStrictEqual(reqRows(out), [],
|
||||
'a phase that maps no REQ-IDs must not report unrelated project requirements as gaps');
|
||||
assert.strictEqual(out.counts.uncovered, 0,
|
||||
'no false-positive "not covered" rows for an unmapped phase');
|
||||
});
|
||||
|
||||
test('--phase-req-ids null behaves the same as TBD (skip requirements)', () => {
|
||||
writeRequirements(['BACK-01', 'WEB-03']);
|
||||
writePlan('01', '# Plan\n\nNo mapped reqs.\n');
|
||||
|
||||
const r = runGsdTools(
|
||||
['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', 'null'], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
assert.deepStrictEqual(reqRows(out), []);
|
||||
});
|
||||
|
||||
// ── Scoped to a mapped subset ────────────────────────────────────────────────
|
||||
|
||||
test('explicit ID list restricts the comparison to those REQ-IDs', () => {
|
||||
writeRequirements(['REQ-01', 'REQ-02', 'REQ-03']);
|
||||
// Plan covers REQ-01 only; REQ-02 is mapped to the phase but not yet addressed.
|
||||
writePlan('01', '# Plan\n\nImplements REQ-01 only.\n');
|
||||
|
||||
const r = runGsdTools(
|
||||
['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', 'REQ-01,REQ-02'], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
assert.deepStrictEqual(reqRows(out).sort(), ['REQ-01', 'REQ-02'],
|
||||
'only the phase-mapped REQ-IDs are considered; REQ-03 (another phase) is excluded');
|
||||
const req01 = out.rows.find(x => x.item === 'REQ-01');
|
||||
const req02 = out.rows.find(x => x.item === 'REQ-02');
|
||||
assert.strictEqual(req01.status, 'Covered');
|
||||
assert.strictEqual(req02.status, 'Not covered');
|
||||
});
|
||||
|
||||
test('JSON-array-ish value (["REQ-01"]) is tolerated and scoped', () => {
|
||||
writeRequirements(['REQ-01', 'REQ-02']);
|
||||
writePlan('01', '# Plan\n\nImplements REQ-01.\n');
|
||||
|
||||
const r = runGsdTools(
|
||||
['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', '["REQ-01"]'], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
assert.deepStrictEqual(reqRows(out), ['REQ-01']);
|
||||
});
|
||||
|
||||
// ── CONTEXT.md decisions are unaffected by req scoping ───────────────────────
|
||||
|
||||
test('CONTEXT.md decisions are still reported when requirements are skipped', () => {
|
||||
writeRequirements(['BACK-01', 'WEB-03']);
|
||||
writeContext([{ id: 'D-01', text: 'Use a local notification daemon' }]);
|
||||
writePlan('01', '# Plan\n\nUnrelated work, no decisions addressed.\n');
|
||||
|
||||
const r = runGsdTools(
|
||||
['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', 'TBD'], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
assert.deepStrictEqual(reqRows(out), [], 'requirements skipped');
|
||||
const d01 = out.rows.find(x => x.item === 'D-01');
|
||||
assert.ok(d01, 'CONTEXT.md decision D-01 must still be reported');
|
||||
assert.strictEqual(d01.source, 'CONTEXT.md');
|
||||
assert.strictEqual(d01.status, 'Not covered');
|
||||
});
|
||||
|
||||
// ── Parser robustness (workflow passes the roadmap value verbatim) ───────────
|
||||
|
||||
test('whitespace/newline-separated IDs are tolerated and scoped', () => {
|
||||
writeRequirements(['REQ-01', 'REQ-02', 'REQ-03']);
|
||||
writePlan('01', '# Plan\n\nImplements the first one.\n');
|
||||
const r = runGsdTools(
|
||||
['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', 'REQ-01 REQ-02\nREQ-03'], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
assert.deepStrictEqual(reqRows(out).sort(), ['REQ-01', 'REQ-02', 'REQ-03']);
|
||||
});
|
||||
|
||||
// ── Backward compatibility ───────────────────────────────────────────────────
|
||||
|
||||
test('flag absent → whole REQUIREMENTS.md is compared (unchanged behavior)', () => {
|
||||
writeRequirements(['REQ-01', 'REQ-02']);
|
||||
writePlan('01', '# Plan\n\nImplements REQ-01 only.\n');
|
||||
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
assert.deepStrictEqual(reqRows(out).sort(), ['REQ-01', 'REQ-02'],
|
||||
'with no --phase-req-ids, all requirements are still reported (back-compat)');
|
||||
});
|
||||
|
||||
// ── §13e wiring: init.plan-phase --pick phase_req_ids → gap-analysis ─────────
|
||||
// Guards the exact query the workflow uses. `roadmap.get-phase` returns raw
|
||||
// phase TEXT (not JSON), so --pick yields nothing there; the scoping value
|
||||
// must come from `init.plan-phase`. This test would have caught using the
|
||||
// wrong query (which silently skips requirements for every phase).
|
||||
|
||||
function writeRoadmap(reqLine) {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap\n\n## Phase 1: Test Phase\n**Goal:** Do the thing\n${reqLine}**Success Criteria:**\n- It works\n`);
|
||||
}
|
||||
|
||||
test('init.plan-phase --pick phase_req_ids exposes the mapped IDs, and gap-analysis scopes to them', () => {
|
||||
writeRoadmap('**Requirements:** REQ-01, REQ-02\n');
|
||||
writeRequirements(['REQ-01', 'REQ-02', 'REQ-03']);
|
||||
writePlan('01', '# Plan\n\nImplements the first requirement only.\n');
|
||||
|
||||
const q = runGsdTools(['query', 'init.plan-phase', '1', '--pick', 'phase_req_ids'], tmpDir);
|
||||
assert.ok(q.success, `init.plan-phase query failed: ${q.error}`);
|
||||
const ids = q.output.trim();
|
||||
assert.match(ids, /REQ-01/, 'init.plan-phase must expose phase_req_ids (roadmap.get-phase does NOT)');
|
||||
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', ids], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
assert.deepStrictEqual(reqRows(out).sort(), ['REQ-01', 'REQ-02'],
|
||||
'gap report is scoped to the phase-mapped IDs; REQ-03 (another phase) is excluded');
|
||||
});
|
||||
|
||||
test('mapped REQ-ID absent from REQUIREMENTS.md appears as "Missing" row, not silently dropped', () => {
|
||||
writeRequirements(['REQ-01']);
|
||||
writePlan('01', '# Plan\n\nImplements REQ-01.\n');
|
||||
|
||||
const r = runGsdTools(
|
||||
['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', 'REQ-01,REQ-99'], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
assert.deepStrictEqual(reqRows(out).sort(), ['REQ-01', 'REQ-99'],
|
||||
'REQ-99 (absent from REQUIREMENTS.md) must be present in the report, not silently dropped');
|
||||
const req99 = out.rows.find(x => x.item === 'REQ-99');
|
||||
assert.ok(req99, 'missing mapped ID must have an output row');
|
||||
assert.strictEqual(req99.status, 'Missing from REQUIREMENTS.md');
|
||||
assert.ok(out.counts.uncovered > 0, 'uncovered count must reflect the missing mapped ID');
|
||||
});
|
||||
|
||||
test('phase with no Requirements line → init.plan-phase yields empty → gap-analysis skips requirements', () => {
|
||||
writeRoadmap(''); // no **Requirements:** line
|
||||
writeRequirements(['REQ-01', 'REQ-02']);
|
||||
writePlan('01', '# Plan\n\nStandalone phase.\n');
|
||||
|
||||
const q = runGsdTools(['query', 'init.plan-phase', '1', '--pick', 'phase_req_ids'], tmpDir);
|
||||
assert.ok(q.success, q.error);
|
||||
const ids = q.output.trim(); // expected empty
|
||||
|
||||
// The workflow passes the (possibly empty) value through verbatim.
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir, '--phase-req-ids', ids], tmpDir);
|
||||
assert.ok(r.success, r.error);
|
||||
const out = JSON.parse(r.output);
|
||||
assert.deepStrictEqual(reqRows(out), [],
|
||||
'an unmapped phase reports no requirement gaps (the original #447 bug)');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user