* test(#2945): prove phase complete checkbox ignores traceability row rejection Failing-first regression for #2945. cmdPhaseComplete flips the REQUIREMENTS.md checkbox unconditionally and never rolls back when the traceability row exists but rejects the Status write (Deferred/Blocked), so a deferred requirement reads as shipped. Rows 1-2 assert the checkbox stays [ ] for Deferred/Blocked; Row 3 guards the forward-status flip; Row 4 covers the no-row boundary. * fix(#2945): roll back the REQUIREMENTS checkbox when the traceability row rejects the write cmdPhaseComplete's inline requirement-write loop flipped the REQUIREMENTS.md checkbox unconditionally and kept the flip when the traceability row existed but rejected the Status write (Out/Deferred/Blocked), so a deferred requirement read as shipped — the #2788 defect-2 fix was written into cmdRequirementsMarkComplete (milestone.cts) only, never the phase.cts inline copy. Port the rollback: capture beforeCheckbox, track tableHit in the updateTraceabilityCell callback, and when reqUpdate.ok && !tableHit (row existed but rejected the write), restore beforeCheckbox. The two surfaces can no longer silently diverge. Forward-status rows (Pending/In Progress/Gaps Found) still flip + advance; absent rows still flip (nothing to disagree with). * chore(#2945): add changeset fragment * chore(#2945): backfill changeset PR number 3073 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/eager-sloths-frolic.md
Normal file
5
.changeset/eager-sloths-frolic.md
Normal file
@@ -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.
|
||||
@@ -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: | <REQ-ID> | 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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
140
tests/issue-2945-phase-complete-checkbox-rollback.test.cjs
Normal file
140
tests/issue-2945-phase-complete-checkbox-rollback.test.cjs
Normal file
@@ -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 `<phase>-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');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user