diff --git a/.changeset/eager-sloths-frolic.md b/.changeset/eager-sloths-frolic.md new file mode 100644 index 000000000..7c1cde69a --- /dev/null +++ b/.changeset/eager-sloths-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3073 +--- +**Completing a phase no longer checks the box for a requirement the traceability table records as deferred or blocked** — the phase-completion write flipped the REQUIREMENTS.md checkbox unconditionally and kept the flip when the traceability row existed but rejected the same completion, so a requirement recorded as Deferred or Blocked read as shipped. The checkbox now rolls back when a row exists but rejects the write, matching the existing requirements mark-complete behavior so the two surfaces never silently disagree. diff --git a/src/phase.cts b/src/phase.cts index 251e73e96..fc45d3ce7 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -2241,10 +2241,17 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { for (const reqId of citedReqIds) { const reqEscaped = escapeRegex(reqId); - reqContent = reqContent.replace( - new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'), - '$1x$2', - ); + // Surface 1 — the checkbox: - [ ] **REQ-ID** → - [x] **REQ-ID**. + // #2945: the flip is CONDITIONAL (porting #2788 defect-2's rollback from + // cmdRequirementsMarkComplete). Capture the pre-flip content; if a + // traceability row EXISTS for this ID below but its Status write is rejected + // (Out/Deferred/Blocked), the checkbox is rolled back so the two surfaces + // cannot silently diverge. A requirement recorded as deferred must not read + // as shipped. + const checkboxRe = new RegExp(`(-\\s*\\[)[ ](\\]\\s*\\*\\*${reqEscaped}\\*\\*)`, 'gi'); + const beforeCheckbox = reqContent; + reqContent = reqContent.replace(checkboxRe, '$1x$2'); + const checkboxFlipped = reqContent !== beforeCheckbox; // Traceability row: | | Phase N | Pending|In Progress | -> // ... Complete | via the markdown-table seam (ADR-2143 §7). Match the @@ -2262,15 +2269,33 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // requirement's write. The "only flip Pending/In Progress -> // Complete" gate is folded into the newValue callback so one // updateTableCell call both probes and writes. - const reqUpdate = updateTraceabilityCell(reqContent, reqRowMatch, 'Status', (current) => + // #2945: track tableHit (did the callback actually CHANGE the value?) so the + // checkbox rollback below can distinguish "row existed and accepted" from + // "row existed and rejected". + let tableHit = false; + const reqUpdate = updateTraceabilityCell(reqContent, reqRowMatch, 'Status', (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 (/^(?:pending|in progress|gaps found)$/i.test(current.trim())) { + tableHit = true; + return ' Complete '; + } + return current; + }); if (reqUpdate.ok) { reqContent = reqUpdate.value; } else if (!isPlaceholderReqId(reqId)) { traceabilityWriteMisses.push(reqId); } + + // #2945 defect-2 (port of milestone.cts:200-210): if a row EXISTS for this + // ID but its Status write was rejected (row reads Out/Deferred/Blocked, + // which the callback returned unchanged), roll the checkbox back so the + // checkbox and the row cannot silently diverge. reqUpdate.ok === a row + // matched (existence probe); !tableHit === the callback did not advance it. + if (checkboxFlipped && reqUpdate.ok && !tableHit) { + reqContent = beforeCheckbox; + } } } diff --git a/tests/issue-2945-phase-complete-checkbox-rollback.test.cjs b/tests/issue-2945-phase-complete-checkbox-rollback.test.cjs new file mode 100644 index 000000000..309e1a443 --- /dev/null +++ b/tests/issue-2945-phase-complete-checkbox-rollback.test.cjs @@ -0,0 +1,140 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * Regression test for #2945 — `phase complete`'s inline REQUIREMENTS.md checkbox + * flip is traceability-blind: it flips `- [ ] **REQ-ID**` → `- [x]` unconditionally, + * then attempts the traceability-row write, and KEEPS the flip when the row exists + * but rejects the write (Out/Deferred/Blocked). The sibling `requirements.mark-complete` + * got the #2788 defect-2 rollback; `cmdPhaseComplete`'s inline copy did not. + * + * The fix ports the rollback from `cmdRequirementsMarkComplete` (src/milestone.cts): + * capture beforeCheckbox, track whether the row write actually changed (tableHit), and + * restore beforeCheckbox when the row EXISTS but rejects the write. + * + * Matrix: .gsd/bug/fix/2945-phase-complete-checkbox-rollback/50-test-matrix.md + */ + +const { test, describe, 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'); + +/** + * Write a passed-VERIFICATION marker for the phase, then run `phase complete N`. + * Mirrors phase.test.cjs's writePassedVerificationForPhase: a `-VERIFICATION.md` + * with `status: passed` frontmatter. Requires the phase directory to exist. + */ +function runVerifiedPhaseComplete(args, tmpDir) { + const argv = Array.isArray(args) ? args : args.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g); + const completeIdx = argv.findIndex((t, i) => t === 'complete' && argv[i - 1] === 'phase'); + const phase = argv[completeIdx + 1]; + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const wanted = parseInt(String(phase).replace(/^0+/, ''), 10); + const phaseDirName = fs.readdirSync(phasesDir).find((name) => { + const m = name.match(/^(\d+)/); + return m && parseInt(m[1], 10) === wanted; + }); + if (!phaseDirName) throw new Error(`no phase directory for phase ${phase}`); + fs.writeFileSync( + path.join(phasesDir, phaseDirName, `${phase}-VERIFICATION.md`), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); + return runGsdTools(args, tmpDir); +} + +describe('phase complete checkbox rollback (#2945)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-2945-'); + }); + afterEach(() => { + cleanup(tmpDir); + }); + + /** + * Scaffold a single-phase project whose ROADMAP cites the given REQ-IDs, with a + * REQUIREMENTS.md whose traceability table rows carry the given statuses, then run + * `phase complete 1`. Returns the REQUIREMENTS.md content after completion. + */ + function completeWithRows(reqIds, rowStatuses) { + const reqList = reqIds.join(', '); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n- [ ] Phase 1: The phase\n\n### Phase 1: The phase\n**Goal:** do it\n**Requirements:** ${reqList}\n**Plans:** 1 plans\n`, + ); + const reqLines = reqIds.map((id) => `- [ ] **${id}**: a requirement`).join('\n'); + const tableRows = reqIds.map((id, i) => `| ${id} | Phase 1 | ${rowStatuses[i]} |`).join('\n'); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), + `# Requirements\n\n## v1 Requirements\n\n${reqLines}\n\n## Traceability\n\n| Requirement | Phase | Status |\n|-------------|-------|--------|\n${tableRows}\n`, + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 01\n**Current Phase Name:** The phase\n**Status:** In progress\n**Current Plan:** 01-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n`, + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-the-phase'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary'); + + const result = runVerifiedPhaseComplete('phase complete 1', tmpDir); + assert.ok(result.success, `phase complete failed: ${result.error || result.output}`); + return fs.readFileSync(path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), 'utf-8'); + } + + test('deferredRowRollsBackCheckbox', () => { + // Row 1 (failing-first regression): a Deferred row rejects the Status write, so the + // checkbox must NOT flip (it stays [ ]), and the row must stay Deferred. + const req = completeWithRows(['DEF-01'], ['Deferred']); + assert.ok(req.includes('- [ ] **DEF-01**'), 'checkbox must stay [ ] when the row is Deferred (no silent divergence)'); + assert.ok(/^\| DEF-01 \| Phase 1 \| Deferred \|/m.test(req), 'row must stay Deferred'); + assert.ok(!req.includes('- [x] **DEF-01**'), 'checkbox must NOT have flipped to [x]'); + }); + + test('blockedRowRollsBackCheckbox', () => { + // Row 2: an Out/Blocked row likewise rejects the write → checkbox stays [ ]. + const req = completeWithRows(['BLK-01'], ['Blocked']); + assert.ok(req.includes('- [ ] **BLK-01**'), 'checkbox must stay [ ] when the row is Blocked'); + assert.ok(/^\| BLK-01 \| Phase 1 \| Blocked \|/m.test(req), 'row must stay Blocked'); + assert.ok(!req.includes('- [x] **BLK-01**'), 'checkbox must NOT have flipped to [x]'); + }); + + test('pendingRowStillFlipsAndAdvances', () => { + // Row 3 (negative-space / unchanged forward behavior): a Pending row accepts the write, + // so the checkbox MUST flip to [x] and the row MUST advance to Complete. An over-broad + // rollback would break this. + const req = completeWithRows(['FWD-01'], ['Pending']); + assert.ok(req.includes('- [x] **FWD-01**'), 'checkbox MUST flip to [x] for a Pending (forward) row'); + assert.ok(/^\| FWD-01 \| Phase 1 \| Complete \|/m.test(req), 'row MUST advance to Complete'); + }); + + test('noRowStillFlipsCheckbox', () => { + // Row 4 (acceptance #3): a cited REQ-ID with NO traceability row → checkbox still flips + // (nothing to disagree with). The rollback only fires when a row EXISTS and rejects. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n\n- [ ] Phase 1: The phase\n\n### Phase 1: The phase\n**Goal:** do it\n**Requirements:** NOROW-01\n**Plans:** 1 plans\n`, + ); + // REQUIREMENTS.md with the checkbox but NO traceability table. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), + `# Requirements\n\n## v1 Requirements\n\n- [ ] **NOROW-01**: a requirement with no traceability row\n`, + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 01\n**Current Phase Name:** The phase\n**Status:** In progress\n**Current Plan:** 01-01\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n`, + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-the-phase'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary'); + + const result = runVerifiedPhaseComplete('phase complete 1', tmpDir); + assert.ok(result.success, `phase complete failed: ${result.error || result.output}`); + const req = fs.readFileSync(path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), 'utf-8'); + assert.ok(req.includes('- [x] **NOROW-01**'), 'checkbox MUST flip to [x] when no traceability row exists'); + }); +});