From be132445aa8f415138f038b0b3c045151c5d2f64 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 14 Jun 2026 00:00:37 -0400 Subject: [PATCH] fix(#1159): phase complete ignores historical/deferred requirement metadata (#1197) * fix(#1159): phase complete ignores historical/deferred requirement metadata Defect A: `phase complete` emitted a false "has unresolved gaps" warning when a VERIFICATION.md file's frontmatter contained `status: passed` but the body contained `previous_status: gaps_found`. The full-text regex `/status: gaps_found/` matched the substring inside `previous_status: gaps_found`, producing a spurious warning. Fixed by reading only the frontmatter `status` key via `extractFrontmatter` in both `phase.cts` (phase complete warning check) and `commands.cts` (determinePhaseStatus). Defect B: requirement IDs under explicitly deferred/backlog/future/v2 section headings in REQUIREMENTS.md were flagged as missing from the Traceability table. Fixed by splitting the body into markdown sections, detecting deferred-intent headings via a keyword regex, and skipping those sections when collecting IDs to check against the table. Both fixes include boundary tests (false positive suppressed; genuine gap/missing warnings still fire) in 4-phase-complete-cjs-regression.test.cjs. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(#1159): address adversarial-review findings (subheading + case sensitivity) - Defect B: Replace section-split approach with line-by-line depth-tracking to correctly propagate deferred status to sub-headings and ignore headings inside fenced code blocks. Previously `## Future Backlog` / `### Sub` would leak sub-heading IDs (split created a new non-deferred section per heading). - Defect A/commands: restore case-insensitive status matching via toLowerCase() to match the prior /status:\s*passed/i regex semantics. - Add test #1159-B-4 covering the nested-subheading case. Co-Authored-By: Claude Opus 4.8 (1M context) * docs(changeset): backfill PR number for #1159 fix Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .changeset/calm-phase-complete-warnings.md | 6 + src/commands.cts | 14 +- src/phase.cts | 68 +++- .../4-phase-complete-cjs-regression.test.cjs | 372 ++++++++++++++++++ 4 files changed, 450 insertions(+), 10 deletions(-) create mode 100644 .changeset/calm-phase-complete-warnings.md diff --git a/.changeset/calm-phase-complete-warnings.md b/.changeset/calm-phase-complete-warnings.md new file mode 100644 index 000000000..e3dd21c66 --- /dev/null +++ b/.changeset/calm-phase-complete-warnings.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 1197 +--- + +**`phase complete` no longer emits false warnings from historical verification metadata or deferred requirement IDs** — two distinct false-positive warning bugs: (A) the verification-status check used a full-text regex that matched `previous_status: gaps_found` in the file body, triggering an "unresolved gaps" warning even when the current frontmatter `status: passed`; the check now reads only the frontmatter `status` key via `extractFrontmatter`. (B) requirement IDs under explicitly deferred/backlog/future/v2 section headings in `REQUIREMENTS.md` were flagged as missing from the Traceability table; the check now skips any section whose heading matches those terms. (#1197) diff --git a/src/commands.cts b/src/commands.cts index 4a891d14b..6560c0f55 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -110,9 +110,17 @@ function determinePhaseStatus(plans: number, summaries: number, phaseDir: string const verificationFile = files.find(f => f === 'VERIFICATION.md' || f.endsWith('-VERIFICATION.md')); if (verificationFile) { const content = platformReadSync(path.join(phaseDir, verificationFile)) || ''; - if (/status:\s*passed/i.test(content)) return 'Complete'; - if (/status:\s*human_needed/i.test(content)) return 'Needs Review'; - if (/status:\s*gaps_found/i.test(content)) return 'Executed'; + // #1159 (Defect A): read ONLY the frontmatter `status` key to avoid false + // matches from historical body metadata such as `previous_status: gaps_found`. + // Full-text regexes like /status:\s*gaps_found/ match the substring inside + // `previous_status: gaps_found`, producing incorrect phase status labels. + const fm = extractFrontmatter(content) as Record; + // Normalise to lower-case to preserve the prior case-insensitive behaviour + // while reading only the frontmatter `status` key (not the full body text). + const fmStatus = typeof fm['status'] === 'string' ? fm['status'].trim().toLowerCase() : ''; + if (fmStatus === 'passed') return 'Complete'; + if (fmStatus === 'human_needed') return 'Needs Review'; + if (fmStatus === 'gaps_found') return 'Executed'; // Verification exists but unrecognized status — treat as executed return 'Executed'; } diff --git a/src/phase.cts b/src/phase.cts index 216612106..a0cb328f5 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -1373,8 +1373,16 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { (f) => f.includes('-VERIFICATION') && f.endsWith('.md'), )) { const content = fs.readFileSync(path.join(phaseFullDir, file), 'utf-8'); - if (/status: human_needed/.test(content)) warnings.push(`${file}: needs human verification`); - if (/status: gaps_found/.test(content)) warnings.push(`${file}: has unresolved gaps`); + // #1159 (Defect A): read ONLY the frontmatter `status` key to avoid false positives + // from historical metadata in the file body (e.g. `previous_status: gaps_found`). + // A full-text regex like /status: gaps_found/ matches the substring inside + // `previous_status: gaps_found`, producing spurious warnings even when the + // current frontmatter status is `passed`. + const verFm = extractFrontmatter(content) as Record; + // Normalise to lower-case so `status: Passed` (title-case) is not missed. + const verStatus = typeof verFm['status'] === 'string' ? verFm['status'].trim().toLowerCase() : ''; + if (verStatus === 'human_needed') warnings.push(`${file}: needs human verification`); + if (verStatus === 'gaps_found') warnings.push(`${file}: has unresolved gaps`); } } catch { /* intentionally empty */ @@ -1495,12 +1503,58 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { } } + // #1159 (Defect B): collect requirement IDs only from ACTIVE sections. + // Requirements under headings whose text contains "deferred", "backlog", + // "future", or "v2" (case-insensitive) are explicitly out of current scope + // and must not be flagged as missing from the Traceability table. + // + // Strategy: walk lines, track heading depth, and toggle a "deferred" flag + // when a heading matching the pattern is encountered. A sub-heading (higher + // depth) that is ITSELF in a deferred parent remains deferred unless it + // opens a same-or-shallower heading that does NOT match the pattern. + // Lines inside fenced code blocks (``` or ~~~) are treated as content, not + // headings, to avoid false deferred-section detection from code examples. + const DEFERRED_HEADING_RE = /\b(?:deferred|backlog|future|v\d+)\b/i; const bodyReqIds: string[] = []; - const bodyReqPattern = /\*\*([A-Z][A-Z0-9]*-\d+)\*\*/g; - let bodyMatch: RegExpExecArray | null; - while ((bodyMatch = bodyReqPattern.exec(reqContent)) !== null) { - const id = bodyMatch[1]; - if (!bodyReqIds.includes(id)) bodyReqIds.push(id); + // deferredDepth: the heading level that opened the current deferred block, + // or 0 when we are in an active section. + let deferredDepth = 0; + let inFence = false; + for (const line of reqContent.split(/\r?\n/)) { + // Track fenced code blocks (``` or ~~~). + if (/^\s*(?:```|~~~)/.test(line)) { + inFence = !inFence; + continue; + } + if (inFence) continue; // ignore content inside a code fence + + const headingM = line.match(/^(#{1,6})\s+(.*)/); + if (headingM) { + const depth = headingM[1].length; + const text = headingM[2]; + if (deferredDepth > 0 && depth > deferredDepth) { + // Sub-heading inside a deferred block: stays deferred regardless of name. + continue; + } + // Heading at same level or shallower than current deferred opener, + // or no active deferred block yet. + if (DEFERRED_HEADING_RE.test(text)) { + deferredDepth = depth; // enter a deferred block + } else { + deferredDepth = 0; // back in an active section + } + continue; + } + + if (deferredDepth > 0) continue; // skip content in deferred sections + + // Collect bold REQ-ID patterns from active-section lines. + const reqPat = /\*\*([A-Z][A-Z0-9]*-\d+)\*\*/g; + let bodyMatch: RegExpExecArray | null; + while ((bodyMatch = reqPat.exec(line)) !== null) { + const id = bodyMatch[1]; + if (!bodyReqIds.includes(id)) bodyReqIds.push(id); + } } const traceabilityHeadingMatch = reqContent.match(/^#{1,6}\s+Traceability\b/im); diff --git a/tests/4-phase-complete-cjs-regression.test.cjs b/tests/4-phase-complete-cjs-regression.test.cjs index a9d1e2d67..3f6495b7c 100644 --- a/tests/4-phase-complete-cjs-regression.test.cjs +++ b/tests/4-phase-complete-cjs-regression.test.cjs @@ -717,3 +717,375 @@ describe('issue #4 (CJS): cmdPhaseComplete — progress percent clamp', () => { ); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// Regressions: issue #1159 — Defect A +// VERIFICATION.md with `previous_status: gaps_found` in the body but +// `status: passed` in frontmatter must NOT emit a "has unresolved gaps" warning. +// The bug: /status: gaps_found/.test(fullContent) matches the substring inside +// `previous_status: gaps_found`, causing a false positive. +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Build a minimal project fixture with a VERIFICATION.md file whose + * frontmatter status is `verFmStatus` and whose body contains `previous_status: gaps_found`. + * Phase 01 has a plan+summary; Phase 02 exists for next-phase detection. + */ +function createVerificationFixture(verFmStatus) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1159-verif-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + const phase01Dir = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase01Dir, { recursive: true }); + fs.mkdirSync(path.join(phasesDir, '02-api'), { recursive: true }); + + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), [ + '# Roadmap', + '', + '- [ ] Phase 01: Foundation', + '- [ ] Phase 02: API', + '', + '### Phase 01: Foundation', + '**Goal:** Build the foundation', + '**Plans:** 1 plans', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Foundation | 0/1 | Not started | - |', + '| 02. API | 0/1 | Not started | - |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'STATE.md'), [ + '# State', + '', + '**Current Phase:** 01', + '**Current Phase Name:** Foundation', + '**Status:** In progress', + '**Completed Phases:** 0', + '**Total Phases:** 2', + '**Progress:** 0%', + '', + ].join('\n')); + + // No REQUIREMENTS.md intentionally (not needed for this defect check) + + fs.writeFileSync(path.join(phase01Dir, '01-01-PLAN.md'), '# Plan 1\nDo the work.\n'); + fs.writeFileSync(path.join(phase01Dir, '01-01-SUMMARY.md'), '# Summary 1\nDone.\n'); + + // The VERIFICATION.md has the CURRENT status in frontmatter but historical + // `previous_status: gaps_found` in the body — this is the false-positive trigger. + fs.writeFileSync(path.join(phase01Dir, '01-VERIFICATION.md'), [ + '---', + `status: ${verFmStatus}`, + 'phase: "01"', + '---', + '', + '# Verification', + '', + '', + 'previous_status: gaps_found', + '', + '## Summary', + 'All checks passed on re-run.', + '', + ].join('\n')); + + return tmpDir; +} + +describe('issue #1159 (Defect A): VERIFICATION.md historical metadata must not trigger gap warning', () => { + let tmpDir; + + afterEach(() => { + cleanup(tmpDir); + }); + + test( + '#1159-A-1: status:passed + previous_status:gaps_found in body → NO "has unresolved gaps" warning', + () => { + tmpDir = createVerificationFixture('passed'); + const { output } = runGsdTools(['phase', 'complete', '1'], tmpDir); + // The output is JSON; parse and check warnings array + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const gapWarnings = warnings.filter((w) => /unresolved gaps/i.test(w)); + assert.equal( + gapWarnings.length, + 0, + `#1159-A-1 FAILED: got false gap warning(s) when frontmatter status=passed.\n` + + `Warnings: ${JSON.stringify(warnings)}\n` + + `(The regex /status: gaps_found/ matched 'previous_status: gaps_found' in the body.)`, + ); + }, + ); + + test( + '#1159-A-2 (boundary): status:gaps_found in frontmatter → DOES emit "has unresolved gaps" warning', + () => { + tmpDir = createVerificationFixture('gaps_found'); + const { output } = runGsdTools(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const gapWarnings = warnings.filter((w) => /unresolved gaps/i.test(w)); + assert.ok( + gapWarnings.length > 0, + `#1159-A-2 FAILED: expected a gap warning when frontmatter status=gaps_found but got none.\n` + + `Warnings: ${JSON.stringify(warnings)}`, + ); + }, + ); + + test( + '#1159-A-3 (boundary): status:human_needed in frontmatter → DOES emit "needs human verification" warning', + () => { + tmpDir = createVerificationFixture('human_needed'); + const { output } = runGsdTools(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const humanWarnings = warnings.filter((w) => /human verification/i.test(w)); + assert.ok( + humanWarnings.length > 0, + `#1159-A-3 FAILED: expected human-verification warning when frontmatter status=human_needed.\n` + + `Warnings: ${JSON.stringify(warnings)}`, + ); + }, + ); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Regressions: issue #1159 — Defect B +// Requirement IDs (e.g. FILE-001) that appear under explicitly deferred/future/v2 +// sections in REQUIREMENTS.md must NOT be flagged as "missing from Traceability". +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Build a minimal project fixture with a REQUIREMENTS.md that has: + * - An active requirement ACTIVE-001 that IS in the Traceability table + * - A deferred requirement DEFER-001 under a "Deferred v2 Requirements" heading + * that is NOT in the Traceability table (correctly out of scope) + * - Optionally a truly-missing active requirement MISSING-001 (not in table) + */ +function createDeferredReqFixture({ includeMissingActive = false } = {}) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1159-deferred-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + const phase01Dir = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase01Dir, { recursive: true }); + fs.mkdirSync(path.join(phasesDir, '02-api'), { recursive: true }); + + const missingActiveLines = includeMissingActive + ? ['', '- **MISSING-001** Active req not in traceability table.'] + : []; + + fs.writeFileSync(path.join(planDir, 'REQUIREMENTS.md'), [ + '# Requirements', + '', + '## Functional Requirements', + '', + '- **ACTIVE-001** Core feature must work.', + ...missingActiveLines, + '', + '## Deferred v2 Requirements', + '', + '- **DEFER-001** Nice-to-have for v2, explicitly out of scope.', + '', + '## Future Backlog', + '', + '- **FUTURE-001** Consider for next major release.', + '', + '## Traceability', + '', + '| Requirement | Phase | Status |', + '|-------------|-------|--------|', + '| ACTIVE-001 | Phase 01 | Pending |', + '', + ].join('\n')); + + // Roadmap references ACTIVE-001 for phase 01 + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), [ + '# Roadmap', + '', + '- [ ] Phase 01: Foundation', + '- [ ] Phase 02: API', + '', + '### Phase 01: Foundation', + '**Goal:** Build the foundation', + '**Requirements:** ACTIVE-001', + '**Plans:** 1 plans', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Foundation | 0/1 | Not started | - |', + '| 02. API | 0/1 | Not started | - |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'STATE.md'), [ + '# State', + '', + '**Current Phase:** 01', + '**Current Phase Name:** Foundation', + '**Status:** In progress', + '**Completed Phases:** 0', + '**Total Phases:** 2', + '**Progress:** 0%', + '', + ].join('\n')); + + fs.writeFileSync(path.join(phase01Dir, '01-01-PLAN.md'), '# Plan 1\nDo the work.\n'); + fs.writeFileSync(path.join(phase01Dir, '01-01-SUMMARY.md'), '# Summary 1\nDone.\n'); + + return tmpDir; +} + +describe('issue #1159 (Defect B): deferred/future requirement IDs must not trigger traceability warning', () => { + let tmpDir; + + afterEach(() => { + cleanup(tmpDir); + }); + + test( + '#1159-B-1: IDs under "Deferred v2 Requirements" and "Future Backlog" sections → NO traceability warning', + () => { + tmpDir = createDeferredReqFixture({ includeMissingActive: false }); + const { output } = runGsdTools(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const traceWarnings = warnings.filter((w) => /Traceability/i.test(w)); + assert.equal( + traceWarnings.length, + 0, + `#1159-B-1 FAILED: got false traceability warning(s) for deferred/future IDs.\n` + + `Warnings: ${JSON.stringify(warnings)}\n` + + `(DEFER-001 and FUTURE-001 are under deferred/future sections and must be ignored.)`, + ); + }, + ); + + test( + '#1159-B-2 (boundary): truly-missing ACTIVE ID (not in table, not in deferred section) → DOES warn', + () => { + tmpDir = createDeferredReqFixture({ includeMissingActive: true }); + const { output } = runGsdTools(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const traceWarnings = warnings.filter((w) => /Traceability/i.test(w)); + assert.ok( + traceWarnings.length > 0, + `#1159-B-2 FAILED: expected traceability warning for MISSING-001 (active, not in table) but got none.\n` + + `Warnings: ${JSON.stringify(warnings)}`, + ); + // Verify MISSING-001 is specifically mentioned + const mentionsMissing = traceWarnings.some((w) => w.includes('MISSING-001')); + assert.ok( + mentionsMissing, + `#1159-B-2 FAILED: warning exists but MISSING-001 not mentioned.\n` + + `Traceability warnings: ${JSON.stringify(traceWarnings)}`, + ); + }, + ); + + test( + '#1159-B-3 (boundary): deferred IDs must not contaminate warning even when active ID is also missing', + () => { + tmpDir = createDeferredReqFixture({ includeMissingActive: true }); + const { output } = runGsdTools(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const traceWarnings = warnings.filter((w) => /Traceability/i.test(w)); + // DEFER-001 and FUTURE-001 must NOT appear in the traceability warnings + const mentionsDefer = traceWarnings.some((w) => w.includes('DEFER-001') || w.includes('FUTURE-001')); + assert.ok( + !mentionsDefer, + `#1159-B-3 FAILED: deferred IDs (DEFER-001/FUTURE-001) appeared in traceability warning.\n` + + `Traceability warnings: ${JSON.stringify(traceWarnings)}`, + ); + }, + ); + + test( + '#1159-B-4 (subheading): IDs under sub-headings of a deferred section are also suppressed', + () => { + // Codex adversarial finding: splitting on EVERY heading failed to propagate + // deferred status to sub-headings (e.g. "## Future Backlog" → "### Sub"). + // The fix uses heading-depth tracking so sub-headings inherit deferred state. + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1159-subhead-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + const phase01Dir = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase01Dir, { recursive: true }); + fs.mkdirSync(path.join(phasesDir, '02-api'), { recursive: true }); + + fs.writeFileSync(path.join(planDir, 'REQUIREMENTS.md'), [ + '# Requirements', + '', + '## Functional Requirements', + '', + '- **ACTIVE-001** Core feature.', + '', + '## Future Backlog', + '', + '### Sub-category A', + '', + '- **SUB-001** This is under a sub-heading of a deferred section.', + '', + '## Traceability', + '', + '| Requirement | Phase | Status |', + '|-------------|-------|--------|', + '| ACTIVE-001 | Phase 01 | Pending |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), [ + '# Roadmap', + '', + '- [ ] Phase 01: Foundation', + '- [ ] Phase 02: API', + '', + '### Phase 01: Foundation', + '**Goal:** Build the foundation', + '**Requirements:** ACTIVE-001', + '**Plans:** 1 plans', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Foundation | 0/1 | Not started | - |', + '| 02. API | 0/1 | Not started | - |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'STATE.md'), [ + '# State', + '', + '**Current Phase:** 01', + '**Completed Phases:** 0', + '**Total Phases:** 2', + '**Progress:** 0%', + '', + ].join('\n')); + + fs.writeFileSync(path.join(phase01Dir, '01-01-PLAN.md'), '# Plan 1\n'); + fs.writeFileSync(path.join(phase01Dir, '01-01-SUMMARY.md'), '# Summary 1\n'); + + const { output } = runGsdTools(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const traceWarnings = warnings.filter((w) => /Traceability/i.test(w)); + const mentionsSub = traceWarnings.some((w) => w.includes('SUB-001')); + assert.ok( + !mentionsSub, + `#1159-B-4 FAILED: SUB-001 (under sub-heading of deferred section) appeared in warning.\n` + + `Traceability warnings: ${JSON.stringify(traceWarnings)}`, + ); + }, + ); +});