diff --git a/.changeset/daring-yaks-zip.md b/.changeset/daring-yaks-zip.md new file mode 100644 index 000000000..b290408a1 --- /dev/null +++ b/.changeset/daring-yaks-zip.md @@ -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. diff --git a/get-shit-done/bin/lib/gap-checker.cjs b/get-shit-done/bin/lib/gap-checker.cjs index cd8e35054..16239ba4e 100644 --- a/get-shit-done/bin/lib/gap-checker.cjs +++ b/get-shit-done/bin/lib/gap-checker.cjs @@ -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 '); } 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, }; diff --git a/get-shit-done/workflows/plan-phase.md b/get-shit-done/workflows/plan-phase.md index 557e94ef7..eb6be4b0d 100644 --- a/get-shit-done/workflows/plan-phase.md +++ b/get-shit-done/workflows/plan-phase.md @@ -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 ``` diff --git a/tests/bug-447-gap-analysis-phase-req-ids.test.cjs b/tests/bug-447-gap-analysis-phase-req-ids.test.cjs new file mode 100644 index 000000000..be5a59481 --- /dev/null +++ b/tests/bug-447-gap-analysis-phase-req-ids.test.cjs @@ -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\n## Implementation Decisions\n\n${dLines}\n\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)'); + }); +});