diff --git a/.changeset/sturdy-pumas-snooze.md b/.changeset/sturdy-pumas-snooze.md new file mode 100644 index 000000000..9ee20d115 --- /dev/null +++ b/.changeset/sturdy-pumas-snooze.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2339 +--- +**`phase complete` no longer silently drops requirement IDs the roadmap cites but REQUIREMENTS.md never defined** — completing a phase whose `**Requirements**:` line named an unregistered REQ-ID reported `requirements_updated: true` with zero warnings while the file was left byte-for-byte unchanged, indistinguishable from a run that wrote everything. Ghost IDs now raise a warning, `requirements_updated` reflects whether a write actually landed, an active heading like `## v1 Requirements` is no longer mistaken for a deferred section, and a phase whose every cited ID is unregistered still reports its missing-requirement rows instead of "No requirements or decisions to check." diff --git a/src/gap-checker.cts b/src/gap-checker.cts index 4994e88fa..c68a48083 100644 --- a/src/gap-checker.cts +++ b/src/gap-checker.cts @@ -334,7 +334,16 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp const mismatchMsg = '## Post-Planning Gap Analysis\n\nextracted 0 of N — possible format mismatch in CONTEXT.md decisions block.\n'; // If there are also requirement items, include them in the return with the // mismatch summary appended, so the caller still sees requirement coverage. - if (items.length > 0) { + // #2334 HIGH 1: gate on `ghostReqIds.length > 0` too — identical defect to + // the one fixed at #2316-6b (~34 lines below, at the `items.length === 0` + // early return): a phase whose EVERY cited REQ-ID is unregistered has + // `items.length === 0` (all its requirement items were filtered out at + // ~line 297) but still has real ghost rows to report. Without this guard, + // a single malformed `` line in CONTEXT.md made an all-ghost + // phase's ghost rows silently vanish (this could-not-parse branch fell + // through to the bare `mismatchMsg`-only return below, dropping ghost + // rows that the general path further down correctly surfaces). + if (items.length > 0 || ghostReqIds.length > 0) { const rows = sortRows([ ...detectCoverage(items, planText), ...ghostReqIds.map(id => ({ source: 'REQUIREMENTS.md', item: id, status: 'Missing from REQUIREMENTS.md' })), @@ -362,7 +371,13 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp } // #1365: if no items at all, surface a clean no-check message. - if (items.length === 0) { + // #2316-6b: this must NOT fire when `ghostReqIds` is non-empty — a phase + // whose EVERY cited REQ-ID is unregistered has `items.length === 0` (all + // its requirement items were filtered out at ~line 297) but still has real + // ghost rows to report below. Without this guard, an all-orphan phase + // reported LESS than a partially-orphan one (which falls through to the + // general path further down and correctly surfaces its ghost rows). + if (items.length === 0 && ghostReqIds.length === 0) { return { enabled: true, rows: [], diff --git a/src/phase.cts b/src/phase.cts index e22334ec7..e7e60d6dc 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -131,6 +131,20 @@ function updateTraceabilityCell( return { ok: true, value: before + result.value + after }; } +/** + * Extract the MAJOR version segment from a version-ish string: "v1", "v1.3", + * "V1.0", and "1.0" all yield "1"; "v2" yields "2". Used (#2334 BLOCKER fix) + * to compare a `## v ...` REQUIREMENTS.md heading against the current + * milestone's version at MAJOR-version granularity only — "v1" heading vs + * milestone "v1.3" is the SAME major version and must not be treated as a + * version mismatch. Returns null when `raw` has no leading digit run (not a + * version-shaped string), which the caller treats as "cannot resolve". + */ +function extractMajorVersion(raw: string): string | null { + const m = raw.trim().match(/^v?(\d+)/i); + return m ? m[1] : null; +} + function describeNonCanonicalPlans(dirFiles: string[], matchedFiles: string[]): string | null { const matched = new Set(matchedFiles); const offenders = dirFiles.filter((f) => looksLikePlanFile(f) && !matched.has(f)); @@ -1919,14 +1933,45 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { const originalReqContent = fs.readFileSync(reqPath, 'utf-8'); let reqContent = originalReqContent; + // #2316: `citedReqIds` — the REQ-IDs ROADMAP's own **Requirements:** + // line for this phase actually cites — is hoisted out of the + // `if (reqMatch)` block (previously scoped only inside it) so the + // ghost-ID cross-check below (~#2316-1) can consult it. `TBD` is the + // literal placeholder `phase.add`/`-batch`/`-insert` seed + // (`**Requirements**: TBD`, src/phase.cts:833,920,1078) — never a + // real REQ-ID, so it is filtered out wherever a cited-ID list feeds + // a warning (#2316-7 boundary). + const isPlaceholderReqId = (id: string): boolean => id.toUpperCase() === 'TBD'; + let citedReqIds: string[] = []; + // #2316-1: Traceability-row writes that matched NO row (ghost or + // otherwise) — the `if (reqUpdate.ok)` below previously had no + // `else`, discarding this fact silently instead of surfacing it. + const traceabilityWriteMisses: string[] = []; + if (reqMatch) { - const reqIds = reqMatch[1] + // #2334 HIGH 3: filter the tokenized capture to the REQ-ID SHAPE — + // the SAME shape bodyReqIds (`\*\*([A-Z][A-Z0-9]*-\d+)\*\*`, below) + // and tableReqIds (`([A-Z][A-Z0-9]*-\d+)`, below) already require — + // so the ghost-ID / unregistered comparisons stay shape-symmetric. + // Without this, `[^\n]+` split on `[,\s]+` turned EVERY word after + // the ID list into a "cited REQ-ID": the shipped + // `templates/roadmap.md:32` line + // `**Requirements**: [REQ-01, REQ-02] ` + // warned to register ``, etc., and + // `**Requirements:** None` warned to register the literal word + // `None`. This subsumes the `TBD` placeholder special-case (`TBD` + // does not match the REQ-ID shape either); `isPlaceholderReqId` is + // kept below as a defensive no-op for any caller that still hands + // it a raw token. + const REQ_ID_SHAPE_RE = /^[A-Z][A-Z0-9]*-\d+$/i; + citedReqIds = reqMatch[1] .replace(/[\[\]]/g, '') .split(/[,\s]+/) .map((r) => r.trim()) - .filter(Boolean); + .filter(Boolean) + .filter((r) => REQ_ID_SHAPE_RE.test(r)); - for (const reqId of reqIds) { + for (const reqId of citedReqIds) { const reqEscaped = escapeRegex(reqId); reqContent = reqContent.replace( new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'), @@ -1951,14 +1996,19 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // updateTableCell call both probes and writes. const reqUpdate = updateTraceabilityCell(reqContent, reqRowMatch, 'Status', (current) => /^(?:pending|in progress)$/i.test(current.trim()) ? ' Complete ' : current); - if (reqUpdate.ok) reqContent = reqUpdate.value; + if (reqUpdate.ok) { + reqContent = reqUpdate.value; + } else if (!isPlaceholderReqId(reqId)) { + traceabilityWriteMisses.push(reqId); + } } } // #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. + // "future", or an OFF-milestone `v` (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 @@ -1966,7 +2016,47 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // 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; + // + // #2334 BLOCKER fix (regresses closed bug #1159 against GSD's OWN + // shipped template): #2316-4a dropped the bare `v\d+` alternative + // entirely to stop it over-matching an ACTIVE heading like "## v1 + // Requirements" — but the shipped `templates/requirements.md:35` + // scaffold ships `## v2 Requirements` / "Deferred to future release" + // as its ONLY deferred marker, and `v\d+` was the ONLY alternative + // that ever matched a bare version heading (the deferred-ness lives + // in body prose, not the heading text). Dropping it regressed #1159 + // for every project scaffolded from the shipped template. + // + // Fix: make the `v` alternative MILESTONE-AWARE instead of + // deleting it. A `## v ...` heading is deferred ONLY when `` + // (MAJOR version only — "v1" vs milestone "v1.3" is the SAME major + // version) does not match the CURRENT milestone's major version, + // resolved via `stateExtractField` against STATE.md's `milestone:` + // frontmatter field (the same seam `getMilestoneInfo`/state.cts's + // frontmatter builder already use — no bespoke frontmatter parsing). + // "## v1 Requirements" while the milestone is v1.x is the ACTIVE + // milestone's own section (#2316's original ask) and must NOT be + // swallowed; "## v2 Requirements" while the milestone is v1.x is a + // genuinely future milestone (#1159's ask, and the literal shipped- + // template shape) and MUST stay suppressed. `deferred`/`backlog`/ + // `future` are unaffected by milestone resolution — a genuinely + // deferred heading always spells one of those words too (see + // #2316-5 regression guard: "## Deferred v2 Requirements", "## + // Future Backlog", "## Deferred", "## Backlog", "## Future"). + // + // Fail-safe: when the milestone version cannot be resolved at all + // (no STATE.md, or no `milestone:` field), fall back to the OLD + // pre-#2316-4a behavior and treat every `v\d+` heading as deferred. + // A false "deferred" here only ever SUPPRESSES a warning — strictly + // safer than spamming a warning on every v\d+-headed scaffold when + // we cannot tell whether it names the active milestone. + const DEFERRED_KEYWORD_RE = /\b(?:deferred|backlog|future)\b/i; + const HEADING_VERSION_RE = /\bv(\d+)(?:\.\d+)*\b/i; + const stateRawForMilestone = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : null; + const currentMilestoneRaw = stateRawForMilestone + ? stateExtractField(stateRawForMilestone, 'milestone') + : null; + const currentMilestoneMajor = currentMilestoneRaw ? extractMajorVersion(currentMilestoneRaw) : null; const bodyReqIds: string[] = []; // deferredDepth: the heading level that opened the current deferred block, // or 0 when we are in an active section. @@ -1990,10 +2080,19 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { } // Heading at same level or shallower than current deferred opener, // or no active deferred block yet. - if (DEFERRED_HEADING_RE.test(text)) { + if (DEFERRED_KEYWORD_RE.test(text)) { deferredDepth = depth; // enter a deferred block } else { - deferredDepth = 0; // back in an active section + const versionMatch = text.match(HEADING_VERSION_RE); + if (versionMatch) { + const headingMajor = versionMatch[1]; + deferredDepth = + currentMilestoneMajor === null || headingMajor !== currentMilestoneMajor + ? depth // unresolved milestone (fail-safe) or off-milestone version -> deferred + : 0; // same major version as the current milestone -> active + } else { + deferredDepth = 0; // back in an active section + } } continue; } @@ -2031,8 +2130,79 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { ); } + // #2316-1: ghost REQ-IDs — cited by ROADMAP's own **Requirements:** + // line for this phase, but registered NOWHERE in REQUIREMENTS.md + // (neither its body nor its Traceability table). The `unregistered` + // check above only ever compares REQUIREMENTS.md's own body against + // its own Traceability table; it never consults `citedReqIds`, so an + // ID that ROADMAP cites but REQUIREMENTS.md never defines at all was + // previously invisible to every guard. `TBD` (the phase.add/-batch/ + // -insert placeholder) is excluded — see #2316-7 boundary. + // + // #2334 HIGH 2: classify "ghost" by PROBING THE ACTUAL WRITE + // SURFACES this same function just wrote to (:1947 checkbox, + // :1967 Traceability row) — case-insensitively — mirroring + // milestone.cts's `notFound`/`hasRow`/`doneCheckbox` classification + // (src/milestone.cts:117-141,209-215), instead of set-differencing + // `bodyReqIds` (deferred-filtered, case-sensitive, bold-only) and + // `tableReqIds` (case-sensitive) against `citedReqIds`. Those two + // indexes can disagree with the writes: an ID under a `## + // Deferred` heading gets its checkbox ticked by the write loop + // above but is deliberately EXCLUDED from `bodyReqIds` by the + // deferred-heading filter (#1159), so the old set-diff reported it + // as an unregistered ghost in the SAME response that just ticked + // its checkbox; a case-mismatched citation (`known-01` vs + // `**KNOWN-01**`) lands its write via the writes' case-insensitive + // regexes but failed the old set-diff's case-SENSITIVE + // `Array.includes`/`Set.has`. An ID whose checkbox OR Traceability + // row actually matched is registered — not a ghost — regardless of + // which section (deferred or not) it lives under. + const reqIsRegisteredAnywhere = (id: string): boolean => { + const reqEscaped = escapeRegex(id); + // Surface 1 — checkbox, EITHER state (`[ ]` or `[x]`), case- + // insensitive: existence check, not the write's space-only match. + if (new RegExp(`-\\s*\\[[ xX]\\]\\s*\\*\\*${reqEscaped}\\*\\*`, 'i').test(reqContent)) { + return true; + } + // Surface 2 — Traceability row exists at all (any Status value), + // via the SAME no-op-probe-through-updateTraceabilityCell + // technique milestone.cts's `hasRow` uses (:210-214): a case- + // insensitive first-cell match, regardless of current Status. + const rowProbeMatch = (row: Record): boolean => + (Object.values(row)[0] ?? '').trim().toLowerCase() === id.toLowerCase(); + return updateTraceabilityCell(reqContent, rowProbeMatch, 'Status', (current) => current).ok; + }; + const ghostReqIds = citedReqIds.filter( + (id) => !isPlaceholderReqId(id) && !reqIsRegisteredAnywhere(id), + ); + if (ghostReqIds.length > 0) { + warnings.push( + `ROADMAP Phase ${phaseNum} cites REQ-ID(s) not registered anywhere in REQUIREMENTS.md (neither body nor Traceability table): ${ghostReqIds.join(', ')} — add them to REQUIREMENTS.md or correct the ROADMAP citation`, + ); + } + + // #2316-1 cont.: a cited ID whose Traceability-row write matched no + // row for a reason OTHER than being a ghost (e.g. a malformed table) + // still deserves a warning instead of a silent discard — but skip + // IDs already reported above as ghosts to avoid a duplicate message + // for the same root cause. + const traceabilityWriteFailures = traceabilityWriteMisses.filter( + (id) => !ghostReqIds.includes(id), + ); + if (traceabilityWriteFailures.length > 0) { + warnings.push( + `REQUIREMENTS.md: Traceability row write skipped for REQ-ID(s) cited by ROADMAP (no matching row found): ${traceabilityWriteFailures.join(', ')}`, + ); + } + writes.push({ filePath: reqPath, before: originalReqContent, after: reqContent }); - requirementsUpdated = true; + // #2316-3: `requirements_updated` must reflect whether REQUIREMENTS.md + // content actually CHANGED, not merely that the file existed in the + // transaction — mirrors the `writes.push({filePath,before,after})` + // diff-tracking pattern used for the ROADMAP write above. A phase + // whose citations match nothing (ghost REQ-IDs only) must report + // `false`, not a bare "the file was present" `true`. + requirementsUpdated = reqContent !== originalReqContent; } } diff --git a/tests/check-gap-analysis-plan-post-e2e.test.cjs b/tests/check-gap-analysis-plan-post-e2e.test.cjs index 62b4bf87a..d234db1a2 100644 --- a/tests/check-gap-analysis-plan-post-e2e.test.cjs +++ b/tests/check-gap-analysis-plan-post-e2e.test.cjs @@ -434,6 +434,108 @@ describe('Full pipeline: render-hooks plan:post discovers gate, then check dispa }); }); +// ─── Section 3b: issue #2316 (Secondary B) — all-unregistered ghost REQ-IDs ── +// +// runGapAnalysis's `items.length === 0` short-circuit fires BEFORE ghostReqIds +// (phaseReqIds cited by ROADMAP but absent from REQUIREMENTS.md) is folded +// into `rows`. When EVERY cited REQ-ID is a ghost, reqItems is filtered down +// to [] and there is no CONTEXT.md, so items.length === 0 and the ghost rows +// never appear — the phase reports "No requirements or decisions to check." +// instead of the ⚠ Missing from REQUIREMENTS.md rows. A phase with ONE +// unregistered ID mixed with a registered one already surfaces correctly +// (see the mapped-REQ-ID-absent test in Section 2 above) — only the +// ALL-unregistered case is broken. + +describe('issue #2316 (Secondary B): all-unregistered phaseReqIds must still emit ghost rows, not the empty-state message', () => { + let tmpDir; + let phaseDir; + + beforeEach(() => { + tmpDir = createTempProject(); + phaseDir = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(phaseDir, { recursive: true }); + const init = runGsdTools('config-ensure-section', tmpDir); + assert.ok(init.success, `config-ensure-section failed: ${init.error}`); + writeRequirements(path.join(tmpDir, '.planning'), ['KNOWN-01']); + writePlan(phaseDir, '01', '# Plan\n\nImplements KNOWN-01.\n'); + }); + + afterEach(() => cleanup(tmpDir)); + + test( + '#2316-6a (control, mixed known+ghost): a phase mapping one registered and one ghost REQ-ID already surfaces the ghost row', + () => { + const r = runGapCheck([phaseDir, 'KNOWN-01,ORPHAN-01'], tmpDir); + assert.ok(r.success, `check failed: ${r.error}`); + const out = JSON.parse(r.output); + assert.ok( + out.table.includes('ORPHAN-01') && out.table.includes('Missing from REQUIREMENTS.md'), + `#2316-6a FAILED: mixed control must still list ORPHAN-01 as Missing, got table: ${out.table}`, + ); + assert.strictEqual(out.counts.total, 2, '#2316-6a FAILED: total must count both REQ-IDs'); + }, + ); + + test( + '#2316-6b: a phase whose EVERY cited REQ-ID is unregistered must still return the ghost rows, not "No requirements or decisions to check."', + () => { + const r = runGapCheck([phaseDir, 'ORPHAN-01,ORPHAN-02'], tmpDir); + assert.ok(r.success, `check failed: ${r.error}`); + const out = JSON.parse(r.output); + assert.ok( + !/No requirements or decisions to check/.test(out.table), + `#2316-6b FAILED: all-unregistered REQ-IDs must not collapse to the empty-state message.\nFull table: ${out.table}`, + ); + assert.ok( + out.table.includes('ORPHAN-01') && out.table.includes('ORPHAN-02'), + `#2316-6b FAILED: table must list both ghost REQ-IDs, got: ${out.table}`, + ); + assert.ok( + out.table.includes('Missing from REQUIREMENTS.md'), + `#2316-6b FAILED: table must show "Missing from REQUIREMENTS.md" status for the ghost IDs, got: ${out.table}`, + ); + assert.strictEqual(out.counts.total, 2, '#2316-6b FAILED: total must count both ghost REQ-IDs, not 0'); + assert.strictEqual(out.counts.uncovered, 2, '#2316-6b FAILED: uncovered must count both ghost REQ-IDs, not 0'); + assert.strictEqual(out.enabled, true, '#2316-6b FAILED: enabled must still be true'); + }, + ); + + test( + '#2334 HIGH 1: all-ghost REQ-IDs + a malformed CONTEXT.md line (could-not-parse) must still return the ghost rows, not silently drop to the mismatch-only message', + () => { + // Same "items.length === 0" defect #2316-6b fixed ~34 lines below in + // runGapAnalysis, but in the sibling `ctxExtraction.outcome === + // 'could-not-parse'` branch a few lines ABOVE it: that branch gated its + // own ghost-row inclusion on the unguarded `items.length > 0`, so a + // malformed line made both ghost rows vanish and `total` + // drop 2 -> 0 for the exact same all-ghost phase #2316-6b already + // covers on the happy (non-malformed) CONTEXT.md path. + fs.writeFileSync( + path.join(phaseDir, 'CONTEXT.md'), + '# Phase Context\n\n\n## Implementation Decisions\n\n' + + '- **D-01 this line is malformed and has no closing bold/colon\n\n', + ); + const r = runGapCheck([phaseDir, 'ORPHAN-01,ORPHAN-02'], tmpDir); + assert.ok(r.success, `check failed: ${r.error}`); + const out = JSON.parse(r.output); + assert.ok( + !/No requirements or decisions to check/.test(out.table), + `#2334 HIGH 1 FAILED: all-ghost + could-not-parse must not collapse to the empty-state message.\nFull table: ${out.table}`, + ); + assert.ok( + out.table.includes('ORPHAN-01') && out.table.includes('ORPHAN-02'), + `#2334 HIGH 1 FAILED: table must still list both ghost REQ-IDs despite the malformed decisions line, got: ${out.table}`, + ); + assert.ok( + /format mismatch/i.test(out.table), + `#2334 HIGH 1 FAILED: the could-not-parse signal must still be present in the table, got: ${out.table}`, + ); + assert.strictEqual(out.counts.total, 2, '#2334 HIGH 1 FAILED: total must count both ghost REQ-IDs, not 0'); + assert.strictEqual(out.counts.uncovered, 2, '#2334 HIGH 1 FAILED: uncovered must count both ghost REQ-IDs, not 0'); + }, + ); +}); + // ─── Section 4: Pure resolver tests against real registry ──────────────────── describe('resolveLoopHooks plan:post — pure function against real registry', () => { diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 376338f61..6bdf34b2e 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -8743,3 +8743,607 @@ describe('bug-2502: insert-phase must update STATE.md next-phase recommendation' }); }); } + +// ───────────────────────────────────────────────────────────────────────────── +// Regressions: issue #2316 — phase complete ghost REQ-ID / silent-discard defects +// +// ROADMAP.md may cite a REQ-ID in a phase's `**Requirements:**` line that was +// never registered anywhere in REQUIREMENTS.md (neither its body nor its +// Traceability table). The only existing cross-check compares REQUIREMENTS.md's +// own body against its own Traceability table — it never consults the REQ-IDs +// ROADMAP actually cites — so such a "ghost" REQ-ID is invisible: the checkbox/ +// Traceability write for it silently matches nothing, and the command reports +// `requirements_updated: true, warnings: []` identically to a run that wrote +// something real. +// +// Contract asserted by #2316-3: `requirements_updated` reflects whether +// REQUIREMENTS.md's content actually changed (before !== after write), not +// merely whether the file existed in the transaction. +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Build a 3-phase fixture: + * Phase 01 "known" — cites KNOWN-01, which IS registered in REQUIREMENTS.md + * (body checkbox + Traceability row). CONTROL phase. + * Phase 02 "orphan" — cites ORPHAN-01, ORPHAN-02, which are registered + * NOWHERE in REQUIREMENTS.md. BUG phase. + * Phase 03 "tbd" — carries the literal `**Requirements**: TBD` placeholder + * seeded by phase.add/-batch/-insert. BOUNDARY phase: + * "TBD" itself must never be treated as a ghost REQ-ID. + */ +function build2316GhostReqFixture() { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2316-ghost-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + const phase1Dir = path.join(phasesDir, '01-known'); + const phase2Dir = path.join(phasesDir, '02-orphan'); + const phase3Dir = path.join(phasesDir, '03-tbd'); + fs.mkdirSync(phase1Dir, { recursive: true }); + fs.mkdirSync(phase2Dir, { recursive: true }); + fs.mkdirSync(phase3Dir, { recursive: true }); + + fs.writeFileSync(path.join(planDir, 'REQUIREMENTS.md'), [ + '# Requirements', + '', + '## Functional Requirements', + '', + '- [ ] **KNOWN-01** Known and registered requirement.', + '', + '## Traceability', + '', + '| Requirement | Phase | Status |', + '|-------------|-------|--------|', + '| KNOWN-01 | Phase 01 | Pending |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), [ + '# Roadmap', + '', + '- [ ] Phase 01: Known', + '- [ ] Phase 02: Orphan', + '- [ ] Phase 03: Tbd', + '', + '### Phase 01: Known', + '**Goal:** Build the known thing', + '**Requirements:** KNOWN-01', + '**Plans:** 1 plans', + '', + '### Phase 02: Orphan', + '**Goal:** Build the orphan thing', + '**Requirements:** ORPHAN-01, ORPHAN-02', + '**Plans:** 1 plans', + '', + '### Phase 03: Tbd', + '**Goal:** Not yet mapped', + '**Requirements**: TBD', + '**Plans:** 1 plans', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Known | 0/1 | Not started | - |', + '| 02. Orphan | 0/1 | Not started | - |', + '| 03. Tbd | 0/1 | Not started | - |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'STATE.md'), [ + '# State', + '', + '**Current Phase:** 01', + '**Current Phase Name:** Known', + '**Status:** In progress', + '**Completed Phases:** 0', + '**Total Phases:** 3', + '**Progress:** 0%', + '', + ].join('\n')); + + for (const [dir, n] of [[phase1Dir, '01'], [phase2Dir, '02'], [phase3Dir, '03']]) { + fs.writeFileSync(path.join(dir, `${n}-01-PLAN.md`), '# Plan\nDo the work.\n'); + fs.writeFileSync(path.join(dir, `${n}-01-SUMMARY.md`), '# Summary\nDone.\n'); + } + + return { tmpDir, planDir }; +} + +describe('issue #2316: phase complete ghost REQ-ID / silent-discard defects', () => { + test( + '#2316-1: ROADMAP-cited REQ-ID absent from REQUIREMENTS.md entirely → phase complete must report non-empty warnings naming the ghost ID(s)', + () => { + const { tmpDir } = build2316GhostReqFixture(); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '2'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + assert.ok( + warnings.length > 0, + `#2316-1 FAILED: expected non-empty warnings for ghost REQ-IDs ORPHAN-01/ORPHAN-02 ` + + `(cited by ROADMAP Phase 02 but registered nowhere in REQUIREMENTS.md), got warnings:[].\n` + + `Full output: ${output}`, + ); + assert.ok( + warnings.some((w) => w.includes('ORPHAN-01')), + `#2316-1 FAILED: warnings must name ORPHAN-01, got: ${JSON.stringify(warnings)}`, + ); + assert.ok( + warnings.some((w) => w.includes('ORPHAN-02')), + `#2316-1 FAILED: warnings must name ORPHAN-02, got: ${JSON.stringify(warnings)}`, + ); + assert.strictEqual( + parsed.has_warnings, true, + `#2316-1 FAILED: has_warnings must be true when ghost REQ-IDs are cited, got: ${JSON.stringify(parsed)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2316-2 (control): a phase citing only a registered REQ-ID still ticks the checkbox + flips the Traceability row, and does not warn', + () => { + const { tmpDir, planDir } = build2316GhostReqFixture(); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + assert.deepStrictEqual( + parsed.warnings || [], [], + `#2316-2 FAILED: control phase (KNOWN-01 is registered) must not warn, got: ${JSON.stringify(parsed.warnings)}`, + ); + assert.strictEqual(parsed.has_warnings, false, '#2316-2 FAILED: has_warnings must be false for the control phase'); + + const reqContent = fs.readFileSync(path.join(planDir, 'REQUIREMENTS.md'), 'utf-8'); + assert.ok( + /-\s*\[x\]\s*\*\*KNOWN-01\*\*/.test(reqContent), + `#2316-2 FAILED: checkbox for KNOWN-01 must be ticked.\nREQUIREMENTS.md:\n${reqContent}`, + ); + assert.ok( + /\|\s*KNOWN-01\s*\|\s*Phase 01\s*\|\s*Complete\s*\|/.test(reqContent), + `#2316-2 FAILED: Traceability row for KNOWN-01 must flip to Complete.\nREQUIREMENTS.md:\n${reqContent}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2316-3: requirements_updated must reflect whether REQUIREMENTS.md content actually changed, not merely that the file existed in the transaction', + () => { + // Case A (BUG): nothing written (ghost REQ-IDs only) → requirements_updated must be false. + const ghost = build2316GhostReqFixture(); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '2'], ghost.tmpDir); + const parsed = JSON.parse(output); + assert.strictEqual( + parsed.requirements_updated, false, + `#2316-3a FAILED: requirements_updated must be false when REQUIREMENTS.md content did not change ` + + `(ghost REQ-IDs ORPHAN-01/ORPHAN-02 match nothing to tick or flip).\nFull output: ${output}`, + ); + } finally { + cleanup(ghost.tmpDir); + } + + // Case B (CONTROL): a real write landed → requirements_updated must remain true. + // Guards against a fix that over-broadens to "always false". + const known = build2316GhostReqFixture(); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], known.tmpDir); + const parsed = JSON.parse(output); + assert.strictEqual( + parsed.requirements_updated, true, + `#2316-3b FAILED: requirements_updated must remain true when a real checkbox/Traceability write landed.\n` + + `Full output: ${output}`, + ); + } finally { + cleanup(known.tmpDir); + } + }, + ); + + test( + '#2316-7 (boundary): the literal "**Requirements**: TBD" placeholder seeded by phase.add/-batch/-insert must not produce a ghost-ID warning for the token "TBD"', + () => { + const { tmpDir } = build2316GhostReqFixture(); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '3'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + assert.ok( + !warnings.some((w) => /\bTBD\b/.test(w)), + `#2316-7 FAILED: the TBD placeholder must not be treated as a ghost REQ-ID, got warnings: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); +}); + +/** + * Build a 2-phase fixture where REQUIREMENTS.md has ONE body section (name + * controlled by `heading`) containing a REQ-ID (PRESET-01) that is absent from + * the Traceability table. `heading` is the only variable across callers, so + * this isolates the DEFERRED_HEADING_RE / milestone-aware-version-heading + * behavior precisely. + * + * `milestone` (default `v1.3`, #2334 BLOCKER regression guard) is written + * into STATE.md's `milestone:` frontmatter so a `## v ...` heading resolves + * deterministically against the current milestone's MAJOR version instead of + * silently exercising the "milestone unresolved" fail-safe path. Pass `null` + * to omit the field entirely (fail-safe probe). + * + * `bodyProse`, when given, is inserted as its own paragraph directly under + * `heading` and above the PRESET-01 line — used to pin the shipped + * `templates/requirements.md` shape, where the deferred-ness lives in body + * prose ("Deferred to future release...") rather than in the heading text. + */ +function build2316HeadingFixture(heading, milestone = 'v1.3', bodyProse = null) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2316-heading-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + const phase1Dir = path.join(phasesDir, '01-preset'); + const phase2Dir = path.join(phasesDir, '02-next'); + fs.mkdirSync(phase1Dir, { recursive: true }); + fs.mkdirSync(phase2Dir, { recursive: true }); + + fs.writeFileSync(path.join(planDir, 'REQUIREMENTS.md'), [ + '# Requirements', + '', + heading, + '', + ...(bodyProse ? [bodyProse, ''] : []), + '- **PRESET-01** Body requirement missing from traceability table.', + '', + '## Traceability', + '', + '| Requirement | Phase | Status |', + '|-------------|-------|--------|', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), [ + '# Roadmap', + '', + '- [ ] Phase 01: Preset', + '- [ ] Phase 02: Next', + '', + '### Phase 01: Preset', + '**Goal:** Build preset', + '**Requirements:** TBD', + '**Plans:** 1 plans', + '', + '### Phase 02: Next', + '**Goal:** whatever', + '**Requirements:** TBD', + '**Plans:** 1 plans', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Preset | 0/1 | Not started | - |', + '| 02. Next | 0/1 | Not started | - |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'STATE.md'), [ + ...(milestone !== null ? ['---', `milestone: ${milestone}`, '---'] : []), + '# State', + '', + '**Current Phase:** 01', + '**Completed Phases:** 0', + '**Total Phases:** 2', + '**Progress:** 0%', + '', + ].join('\n')); + + for (const [dir, n] of [[phase1Dir, '01'], [phase2Dir, '02']]) { + fs.writeFileSync(path.join(dir, `${n}-01-PLAN.md`), '# Plan\nDo the work.\n'); + fs.writeFileSync(path.join(dir, `${n}-01-SUMMARY.md`), '# Summary\nDone.\n'); + } + + return tmpDir; +} + +describe('issue #2316 (Secondary A): DEFERRED_HEADING_RE\'s bare `v\\d+` alternative over-matches an active heading', () => { + test( + '#2316-4a: "## v1 Requirements" heading with a body REQ-ID missing from the Traceability table must still surface the body-vs-table warning (an active "v1" heading is not deferred)', + () => { + const tmpDir = build2316HeadingFixture('## v1 Requirements'); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + assert.ok( + warnings.some((w) => /PRESET-01/.test(w) && /Traceability/i.test(w)), + `#2316-4a FAILED: expected a body-vs-table Traceability warning naming PRESET-01 under the ` + + `"## v1 Requirements" heading (the bare v\\d+ regex alternative wrongly treats it as deferred), ` + + `got warnings: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2316-4b (control): "## Functional" heading (identical fixture, non-versioned heading name) must surface the same body-vs-table warning', + () => { + const tmpDir = build2316HeadingFixture('## Functional'); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + assert.ok( + warnings.some((w) => /PRESET-01/.test(w) && /Traceability/i.test(w)), + `#2316-4b FAILED: control heading "## Functional" must also warn for PRESET-01, got warnings: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2334 BLOCKER: "## v2 Requirements" heading with the shipped templates/requirements.md body prose ("Deferred to future release. Tracked but not in current roadmap."), milestone v1.3 -> the bare v\\d+ heading must be treated as deferred (SAME major-version mismatch: v2 != v1) and NOT surface the body-vs-table warning', + () => { + // Fixture copied VERBATIM from gsd-core/templates/requirements.md:35-37. + const tmpDir = build2316HeadingFixture( + '## v2 Requirements', + 'v1.3', + 'Deferred to future release. Tracked but not in current roadmap.', + ); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const traceWarnings = warnings.filter((w) => /Traceability/i.test(w)); + assert.strictEqual( + traceWarnings.length, 0, + `#2334 BLOCKER FAILED: "## v2 Requirements" under milestone v1.3 (the shipped template's OWN deferred ` + + `section — #1159's original fix target) must stay suppressed, got: ${JSON.stringify(traceWarnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2334 BLOCKER (milestone-unresolved fail-safe): "## v1 Requirements" heading with NO milestone field in STATE.md must fall back to the OLD pre-#2316-4a behavior and suppress the warning (a false "deferred" only ever suppresses, never spams)', + () => { + const tmpDir = build2316HeadingFixture('## v1 Requirements', /* milestone */ null); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const traceWarnings = warnings.filter((w) => /Traceability/i.test(w)); + assert.strictEqual( + traceWarnings.length, 0, + `#2334 fail-safe FAILED: with no resolvable milestone, "## v1 Requirements" must fall back to treating ` + + `v\\d+ as deferred (suppress), got: ${JSON.stringify(traceWarnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + for (const heading of ['## Deferred', '## Backlog', '## Future']) { + test( + `#2316-5: genuinely deferred heading "${heading}" must still suppress the body-vs-table warning (regression guard — #1159 introduced this filter deliberately; the v\\d+ fix must not remove deferred/backlog/future detection)`, + () => { + const tmpDir = build2316HeadingFixture(heading); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const traceWarnings = warnings.filter((w) => /Traceability/i.test(w)); + assert.strictEqual( + traceWarnings.length, 0, + `#2316-5 FAILED: heading "${heading}" must still be treated as deferred (no warning), ` + + `got: ${JSON.stringify(traceWarnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + } +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #2334 HIGH 2 / HIGH 3: ghost-REQ-ID classification must probe the ACTUAL +// write surfaces (checkbox + Traceability row, case-insensitive — mirroring +// milestone.cts's notFound/hasRow/doneCheckbox classification), not set-diff +// the deferred-filtered/case-sensitive bodyReqIds/tableReqIds indexes; and the +// cited-REQ-ID tokenizer must be filtered to the REQ-ID shape before it ever +// feeds a warning. +// ───────────────────────────────────────────────────────────────────────────── + +function build2334GhostSurfaceFixture({ reqBody, roadmapRequirementsLine, traceabilityRows = [] }) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2334-ghost-surface-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + const phase1Dir = path.join(phasesDir, '01-preset'); + const phase2Dir = path.join(phasesDir, '02-next'); + fs.mkdirSync(phase1Dir, { recursive: true }); + fs.mkdirSync(phase2Dir, { recursive: true }); + + fs.writeFileSync(path.join(planDir, 'REQUIREMENTS.md'), [ + '# Requirements', + '', + ...reqBody, + '', + '## Traceability', + '', + '| Requirement | Phase | Status |', + '|-------------|-------|--------|', + ...traceabilityRows, + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), [ + '# Roadmap', + '', + '- [ ] Phase 01: Preset', + '- [ ] Phase 02: Next', + '', + '### Phase 01: Preset', + '**Goal:** Build preset', + `**Requirements**: ${roadmapRequirementsLine}`, + '**Plans:** 1 plans', + '', + '### Phase 02: Next', + '**Goal:** whatever', + '**Requirements:** TBD', + '**Plans:** 1 plans', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + '| 01. Preset | 0/1 | Not started | - |', + '| 02. Next | 0/1 | Not started | - |', + '', + ].join('\n')); + + fs.writeFileSync(path.join(planDir, 'STATE.md'), [ + '---', 'milestone: v1.3', '---', + '# State', + '', + '**Current Phase:** 01', + '**Completed Phases:** 0', + '**Total Phases:** 2', + '**Progress:** 0%', + '', + ].join('\n')); + + for (const [dir, n] of [[phase1Dir, '01'], [phase2Dir, '02']]) { + fs.writeFileSync(path.join(dir, `${n}-01-PLAN.md`), '# Plan\nDo the work.\n'); + fs.writeFileSync(path.join(dir, `${n}-01-SUMMARY.md`), '# Summary\nDone.\n'); + } + + return tmpDir; +} + +describe('issue #2334: ghost-REQ-ID classification must probe write surfaces, not lossy indexes', () => { + test( + '#2334 HIGH 2a: an ID under "## Deferred" whose checkbox this run TICKS must NOT be reported as a ghost', + () => { + const tmpDir = build2334GhostSurfaceFixture({ + reqBody: ['## Deferred', '', '- [ ] **KNOWN-01**: some deferred requirement'], + roadmapRequirementsLine: '[KNOWN-01]', + }); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const reqContent = fs.readFileSync(path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), 'utf-8'); + assert.ok( + /-\s*\[x\]\s*\*\*KNOWN-01\*\*/i.test(reqContent), + `#2334 HIGH 2a FAILED (fixture invariant): checkbox for KNOWN-01 must have been ticked.\n${reqContent}`, + ); + assert.ok( + !warnings.some((w) => /not registered anywhere/i.test(w) && /KNOWN-01/i.test(w)), + `#2334 HIGH 2a FAILED: KNOWN-01 (checkbox ticked this run, under a Deferred heading) must not be ` + + `reported as a ghost, got: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2334 HIGH 2b: a case-mismatched citation ("known-01" vs "**KNOWN-01**") whose write lands must NOT be reported as a ghost', + () => { + const tmpDir = build2334GhostSurfaceFixture({ + reqBody: ['## Active', '', '- [ ] **KNOWN-01**: some active requirement'], + roadmapRequirementsLine: '[known-01]', + traceabilityRows: ['| KNOWN-01 | Phase 1 | Pending |'], + }); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const reqContent = fs.readFileSync(path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), 'utf-8'); + assert.ok( + /-\s*\[x\]\s*\*\*KNOWN-01\*\*/i.test(reqContent), + `#2334 HIGH 2b FAILED (fixture invariant): checkbox must have been ticked.\n${reqContent}`, + ); + assert.ok( + /\|\s*KNOWN-01\s*\|[^|]*\|\s*Complete\s*\|/i.test(reqContent), + `#2334 HIGH 2b FAILED (fixture invariant): Traceability row must have flipped to Complete.\n${reqContent}`, + ); + assert.strictEqual(parsed.requirements_updated, true, '#2334 HIGH 2b FAILED: requirements_updated must be true'); + assert.ok( + !warnings.some((w) => /not registered anywhere/i.test(w)), + `#2334 HIGH 2b FAILED: a case-mismatched citation whose write landed must not warn as a ghost, ` + + `got: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2334 HIGH 3: the shipped templates/roadmap.md inline-HTML-comment Requirements line must not warn to register ""', + () => { + const tmpDir = build2334GhostSurfaceFixture({ + reqBody: ['## Active', '', '- [ ] **REQ-01**: something'], + roadmapRequirementsLine: '[REQ-01, REQ-02] ', + traceabilityRows: ['| REQ-01 | Phase 1 | Pending |'], + }); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + const junkTokens = ['']; + for (const token of junkTokens) { + assert.ok( + !warnings.some((w) => w.includes(token)), + `#2334 HIGH 3 FAILED: warnings must not name the non-REQ-ID token "${token}" from the inline HTML ` + + `comment, got: ${JSON.stringify(warnings)}`, + ); + } + // REQ-02 is a genuine, shape-valid, unregistered REQ-ID — it MUST + // still be flagged (the shape filter must not over-broaden to + // silence real ghosts). + assert.ok( + warnings.some((w) => /not registered anywhere/i.test(w) && /REQ-02/.test(w)), + `#2334 HIGH 3 FAILED: REQ-02 (real, shape-valid, unregistered REQ-ID) must still be flagged as a ` + + `ghost, got: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); + + test( + '#2334 HIGH 3 (boundary): "**Requirements:** None" must not warn to register the literal word "None"', + () => { + const tmpDir = build2334GhostSurfaceFixture({ + reqBody: ['## Active'], + roadmapRequirementsLine: 'None', + }); + try { + const { output } = runVerifiedPhaseComplete(['phase', 'complete', '1'], tmpDir); + const parsed = JSON.parse(output); + const warnings = parsed.warnings || []; + assert.ok( + !warnings.some((w) => /\bNone\b/.test(w)), + `#2334 HIGH 3 boundary FAILED: "None" must not be treated as a cited REQ-ID, got: ${JSON.stringify(warnings)}`, + ); + } finally { + cleanup(tmpDir); + } + }, + ); +});