diff --git a/.changeset/noble-cranes-march.md b/.changeset/noble-cranes-march.md new file mode 100644 index 000000000..23564f1e5 --- /dev/null +++ b/.changeset/noble-cranes-march.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2902 +--- +**A requirement row stranded at `Gaps Found` can now be completed again, and `requirements mark-complete` no longer reports false success on a row it could not move** — the completion guards now accept `Gaps Found` (so `revert-phase`'s stranded rows are recoverable instead of permanently blocking the milestone), and when a traceability table has a row for an ID, `mark-complete` counts it as updated only if the row actually moved (not merely because the checkbox flipped). (#2788) diff --git a/src/milestone.cts b/src/milestone.cts index ffdcb4ac7..6ce333999 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -156,10 +156,15 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool // Surface 1 — the checkbox: - [ ] **REQ-ID** → - [x] **REQ-ID** // Use replace() + compare to avoid the test()+replace() global regex // lastIndex bug where test() advances state and replace() misses matches. + // (#2788 defect 2: the flip is CONDITIONAL — when a traceability row EXISTS + // for this ID but its Status write is rejected, the checkbox must NOT flip, + // so the two surfaces cannot silently diverge. The row-write outcome below + // gates whether the flip is kept.) const checkboxPattern = new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'); + const beforeCheckbox = reqContent; const afterCheckbox = reqContent.replace(checkboxPattern, '$1x$2'); - const checkboxHit = afterCheckbox !== reqContent; - if (checkboxHit) reqContent = afterCheckbox; + const checkboxFlipped = afterCheckbox !== beforeCheckbox; + if (checkboxFlipped) reqContent = afterCheckbox; // Surface 2 — the traceability row: | | Phase N | Pending | → ... Complete | // via the markdown-table seam (ADR-2143 §7) — supersedes the prior ordinal @@ -178,7 +183,11 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool // updateTableCell call both probes the current value and writes. let tableHit = false; const tableUpdate = updateTraceabilityCell(reqContent, rowMatch, 'Status', (current) => { - if (/^pending$/i.test(current.trim())) { + // #2788: accept `Gaps Found` as a forward input too — `revert-phase` (the + // documented gaps_found response) leaves a row stranded at Gaps Found with + // no inverse; a genuinely-satisfied requirement must be able to reach + // Complete again via mark-complete, or the milestone is blocked forever. + if (/^(pending|gaps found)$/i.test(current.trim())) { tableHit = true; return ' Complete '; } @@ -188,6 +197,18 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool reqContent = tableUpdate.value; } + // #2788 defect 2: if a row EXISTS for this ID but its Status write was + // rejected (e.g. the row reads `Blocked`, which mark-complete does not + // accept), roll the checkbox back so the checkbox and the row cannot + // silently diverge. The checkbox and the row are two representations of the + // same fact; flipping one while the other rejects the write is the lie. + let checkboxHit = checkboxFlipped; + const rowExistsProbe = tableUpdate; // ok === a row matched (probes existence) + if (checkboxFlipped && rowExistsProbe.ok && !tableHit) { + reqContent = beforeCheckbox; + checkboxHit = false; + } + // ADR-2143 §6 per-ID write-set entries: this ID's checkbox surface is // always tracked; the traceability surface is tracked only when the file // has a traceability table at all (same `hasTable` gate the existing @@ -215,7 +236,14 @@ function cmdRequirementsMarkComplete(cwd: string, reqIdsRaw: string[], raw: bool const doneCheckbox = new RegExp(`-\\s*\\[x\\]\\s*\\*\\*${reqEscaped}\\*\\*`, 'i').test(reqContent); const doneTable = Boolean(hasRow && /^complete$/i.test(currentStatusCell.trim())); - if (checkboxHit || tableHit) { + // #2788 defect 2: when a traceability table exists AND this ID has a row in + // it, `updated`/`marked_complete` must reflect the ROW moving, not a + // checkbox-only flip. Otherwise (`table_unmatched` — no row for this ID, or + // no table at all) the checkbox flip is a legitimate partial reconcile / the + // sole completion surface, so the #2140 OR semantics are preserved. + const rowExists = hasTable && hasRow; + const idUpdated = rowExists ? tableHit : (checkboxHit || tableHit); + if (idUpdated) { updated.push(reqId); } else if (doneTable || (doneCheckbox && !hasTable)) { // Fully reconciled: the table row is Complete, OR the checkbox is done and diff --git a/src/phase.cts b/src/phase.cts index 9d80b29b7..9e87f0880 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -2078,7 +2078,9 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // Complete" gate is folded into the newValue callback so one // updateTableCell call both probes and writes. const reqUpdate = updateTraceabilityCell(reqContent, reqRowMatch, 'Status', (current) => - /^(?:pending|in progress)$/i.test(current.trim()) ? ' Complete ' : current); + // #2788: accept `Gaps Found` too so a phase stranded by revert-phase (the + // gaps_found response) can complete without hand-editing the table. + /^(?:pending|in progress|gaps found)$/i.test(current.trim()) ? ' Complete ' : current); if (reqUpdate.ok) { reqContent = reqUpdate.value; } else if (!isPlaceholderReqId(reqId)) { diff --git a/tests/milestone.test.cjs b/tests/milestone.test.cjs index f2bff639f..342714ae7 100644 --- a/tests/milestone.test.cjs +++ b/tests/milestone.test.cjs @@ -971,6 +971,117 @@ describe('requirements mark-complete command', () => { assert.strictEqual(output.updated, false); assert.strictEqual(output.reason, 'REQUIREMENTS.md not found'); }); + + // #2788: a requirement row stranded at `Gaps Found` (by revert-phase, the + // gaps_found response) must be recoverable — mark-complete moves it to Complete. + // Pre-fix the `/^pending$/i` guard rejected `Gaps Found`, stranding the row + // permanently (no inverse transition existed) AND mark-complete reported + // `updated: true` while the row stayed `Gaps Found` (defect 2, the lie). + const GAPS_FOUND_REQUIREMENTS = `# Requirements + +## Coverage +- [ ] **REQ-01**: feature one +- [ ] **REQ-02**: feature two + +## Traceability + +| Requirement | Phase | Status | +|-------------|-------|--------| +| REQ-01 | Phase 1 | Gaps Found | +| REQ-02 | Phase 1 | Complete | +`; + + test('#2788 defect 1: a Gaps Found row moves to Complete via mark-complete (no longer terminal)', () => { + writeRequirements(tmpDir, GAPS_FOUND_REQUIREMENTS); + const result = runGsdTools('requirements mark-complete REQ-01', tmpDir); + assert.ok(result.success); + const out = JSON.parse(result.output); + // The row EXISTS and moved to Complete, so updated/marked_complete are truthful. + assert.ok(out.updated, 'the stranded Gaps Found row must be recoverable'); + assert.ok(out.marked_complete.includes('REQ-01')); + const content = readRequirements(tmpDir); + assert.ok(/REQ-01 \| Phase 1 \| Complete/.test(content), + 'the row must read Complete after mark-complete; got:\n' + content); + assert.ok(content.includes('- [x] **REQ-01**'), 'the checkbox must be checked'); + }); + + test('#2788 defect 2: when a row EXISTS but the write is rejected, updated is FALSE (no false success)', () => { + // A row at `Blocked` (a status mark-complete does not accept) EXISTS for REQ-01. + // The checkbox flips, but the row does not move — `updated` must be false so the + // operator is not told it worked while the audit row still reads Blocked. + const blockedRequirements = `# Requirements + +## Coverage +- [ ] **REQ-01**: feature one + +## Traceability + +| Requirement | Phase | Status | +|-------------|-------|--------| +| REQ-01 | Phase 1 | Blocked | +`; + writeRequirements(tmpDir, blockedRequirements); + const result = runGsdTools('requirements mark-complete REQ-01', tmpDir); + assert.ok(result.success); + const out = JSON.parse(result.output); + assert.strictEqual(out.updated, false, + 'a checkbox flip on a table-bearing file whose row EXISTS but did not move must NOT report updated:true'); + assert.ok(!out.marked_complete.includes('REQ-01'), + 'REQ-01 must not be in marked_complete when the row write was rejected'); + const content = readRequirements(tmpDir); + assert.ok(/REQ-01 \| Phase 1 \| Blocked/.test(content), + 'the row must still read Blocked (write rejected); got:\n' + content); + // #2788 defect 2: the checkbox must NOT flip when the row write is rejected — + // the checkbox and the row are two representations of the same fact, so they + // must not silently diverge. The checkbox stays unchecked on disk. + assert.ok(content.includes('- [ ] **REQ-01**'), + 'the checkbox must stay unchecked when the row write was rejected; got:\n' + content); + // write_set carries the truth: NEITHER surface applied (checkbox rolled back, + // traceability rejected). + const checkboxEntry = out.write_set.find( + (e) => e.requirement === 'REQ-01' && e.surface === 'checkbox'); + assert.ok(checkboxEntry && checkboxEntry.applied === false, + 'write_set must record the checkbox surface as not applied (rolled back)'); + const traceabilityEntry = out.write_set.find( + (e) => e.requirement === 'REQ-01' && e.surface === 'traceability'); + assert.ok(traceabilityEntry && traceabilityEntry.applied === false, + 'write_set must record the traceability surface as not applied'); + }); + + test('#2788 end-to-end: revert-phase (Complete → Gaps Found) then mark-complete (Gaps Found → Complete) round-trips', () => { + const completeRequirements = `# Requirements + +## Coverage +- [x] **REQ-01**: feature one + +## Traceability + +| Requirement | Phase | Status | +|-------------|-------|--------| +| REQ-01 | Phase 1 | Complete | +`; + writeRequirements(tmpDir, completeRequirements); + // revert-phase strands the row at Gaps Found (the gaps_found response). + const reverted = JSON.parse(runGsdTools('requirements revert-phase REQ-01', tmpDir).output); + assert.ok(reverted.reverted.includes('REQ-01')); + let content = readRequirements(tmpDir); + assert.ok(/REQ-01 \| Phase 1 \| Gaps Found/.test(content), 'revert should strand at Gaps Found'); + // Now the requirement is genuinely satisfied again — mark-complete must recover it. + const marked = JSON.parse(runGsdTools('requirements mark-complete REQ-01', tmpDir).output); + assert.ok(marked.updated, 'the stranded row must be recoverable via mark-complete'); + content = readRequirements(tmpDir); + assert.ok(/REQ-01 \| Phase 1 \| Complete/.test(content), + 'after mark-complete the row must read Complete again'); + }); + + test('#2788 negative-space: the normal Pending → Complete path is unchanged', () => { + writeRequirements(tmpDir, STANDARD_REQUIREMENTS); + const out = JSON.parse(runGsdTools('requirements mark-complete TEST-01', tmpDir).output); + assert.ok(out.updated); + assert.ok(out.marked_complete.includes('TEST-01')); + const content = readRequirements(tmpDir); + assert.ok(/TEST-01 \| Phase 1 \| Complete/.test(content), 'Pending row still moves to Complete'); + }); }); // ─────────────────────────────────────────────────────────────────────────────