* test(#4699): add failing-first coverage for skipping complete phases in next_phase * fix(#4699): skip already-complete phases in the next_phase cascade Both next-phase scans selected the numerically lowest phase above N without consulting completion state, so completing a reopened phase persisted an already-[x] phase as STATE.md current_phase while roadmap.analyze correctly named the outstanding one (issue repro: completing 2 with phases 1 and 3 already [x] returned next_phase 03). The cascade collects the complete phase numbers from the roadmap checkboxes (milestone-scoped, comparePhaseNum-deduped) and skips them in both the disk scan and the roadmap scan; a [x] checkbox row and its heading sibling both name a phase that is never next. Heading-only and checkbox-less roadmaps behave exactly as before. * test(#4699): align the negative-control expectation with the disk spelling * test(#4699): pin the STATE.md persistence and the all-later-complete tail corner Review findings: the regression never asserted STATE.md current_phase (the issue's actual harm), and the all-later-phases-[x] corner (is_last_phase true, next_phase null) was unpinned. A changeset fragment is included. * docs(#4699): backfill changeset PR number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/daring-finches-zip.md
Normal file
5
.changeset/daring-finches-zip.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4820
|
||||
---
|
||||
**phase complete no longer names an already-complete phase as next** — completing a reopened phase out of order (later phases already [x]) picked the numerically-next phase even when its checkbox was already ticked, persisting it to STATE.md as current_phase. next_phase now skips phases whose roadmap checkbox is already [x], agreeing with roadmap.analyze and init.progress. (#4699)
|
||||
@@ -4237,6 +4237,37 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
let roadmapNextNum: string | null = null;
|
||||
let roadmapNextName: string | null = null;
|
||||
|
||||
// #4699: a phase whose roadmap checkbox is `[x]` is already complete and
|
||||
// must never be selected as next_phase — out-of-order completion (a
|
||||
// reopened phase finished after later phases shipped) otherwise persists
|
||||
// the already-done phase as STATE.md current_phase. Collected from the
|
||||
// same milestone-scoped text the roadmap scan walks; membership is
|
||||
// comparePhaseNum-based so `02` and `2` dedupe. With no ROADMAP.md (or no
|
||||
// parseable rows) the set is empty and the scans behave exactly as
|
||||
// before.
|
||||
const roadmapCompleteNums: string[] = [];
|
||||
if (roadmapContent !== null) {
|
||||
try {
|
||||
const milestoneForComplete = extractCurrentMilestone(roadmapContent, cwd);
|
||||
const completePattern = new RegExp(
|
||||
`-\\s*\\[[xX]\\]\\s*(?:\\*\\*|__)?\\s*Phase\\s+(${PHASE_NUMBER_TOKEN_SOURCE})`,
|
||||
'gi'
|
||||
);
|
||||
let cm: RegExpExecArray | null;
|
||||
while ((cm = completePattern.exec(milestoneForComplete)) !== null) {
|
||||
if (isSentinelPhaseId(cm[1])) continue;
|
||||
if (!roadmapCompleteNums.some((n) => comparePhaseNum(cm![1], n) === 0)) {
|
||||
roadmapCompleteNums.push(cm[1]);
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
/* best-effort: an unreadable milestone section leaves the complete
|
||||
* set empty — the scans then behave exactly as they did pre-#4699. */
|
||||
}
|
||||
}
|
||||
const isCompletePhaseNum = (num: string): boolean =>
|
||||
roadmapCompleteNums.some((n) => comparePhaseNum(num, n) === 0);
|
||||
|
||||
try {
|
||||
// #3185 (ADR-3180 Decision 1): "which phase directories belong to
|
||||
// the CURRENT milestone" — routed through the canonical owner
|
||||
@@ -4252,6 +4283,9 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
if (dm) {
|
||||
// #3185: canonical sentinel predicate (SENTINEL_RANGES [0,999]) — this was a local 999-only literal that admitted Phase 0.
|
||||
if (isSentinelPhaseId(dm[1])) continue;
|
||||
// #4699: an already-complete phase (roadmap checkbox [x]) is never
|
||||
// a next_phase candidate — out-of-order completion must skip it.
|
||||
if (roadmapContent !== null && isCompletePhaseNum(dm[1])) continue;
|
||||
// Numeric MINIMUM above N, not "first encountered". `listMilestonePhaseDirs`
|
||||
// does sort by `comparePhaseNum`, so a `break` on the first hit happens to be
|
||||
// correct today — but that makes this scan's correctness depend on an
|
||||
@@ -4321,6 +4355,10 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
// already skips sentinel dirs on disk via isSentinelPhaseId (#3185);
|
||||
// stage 2's heading scan must not advance into backlog headings either.
|
||||
if (isSentinelPhaseId(pmNum)) continue;
|
||||
// #4699: skip complete phases — a `[x]` checkbox row and the
|
||||
// `## Phase Details` heading of an already-done phase both name a
|
||||
// phase that must never be next_phase.
|
||||
if (roadmapContent !== null && isCompletePhaseNum(pmNum)) continue;
|
||||
// #3701 review: the numeric MINIMUM above N, not the first row above N in
|
||||
// DOCUMENT order. This scan walks raw roadmap text, and one global regex
|
||||
// sweeps both the `## Phases` checklist and the `## Phase Details`
|
||||
|
||||
@@ -15812,3 +15812,140 @@ describe('bug #3982: archived details leak into lowest-outstanding scan', () =>
|
||||
`STATE.md current_phase must not jump backwards into the archived range; got: ${state}`);
|
||||
});
|
||||
});
|
||||
|
||||
// ── #4699 — next_phase must skip phases whose roadmap checkbox is [x] ────────
|
||||
// Out-of-order completion (a reopened phase finished after later phases
|
||||
// shipped) used to persist the already-complete phase as next_phase /
|
||||
// STATE.md current_phase: both next-phase scans select the numerically lowest
|
||||
// phase above N without consulting completion state. Roadmap checkbox state
|
||||
// is the completion rule (#2028) — an [x] phase is never "next".
|
||||
|
||||
describe('phase complete skips already-complete phases as next_phase (#4699)', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject('gsd-4699-');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
function writeRoadmap({ thirdBox = '[x]', fourthBox = '[ ]' } = {}) {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap
|
||||
|
||||
## Phases
|
||||
|
||||
- [x] **Phase 1: One** - Goal one
|
||||
- [ ] **Phase 2: Two** - Goal two
|
||||
- ${thirdBox} **Phase 3: Three** - Goal three
|
||||
- ${fourthBox} **Phase 4: Four** - Goal four
|
||||
|
||||
### Phase 1: One
|
||||
**Goal**: Goal one
|
||||
|
||||
### Phase 2: Two
|
||||
**Goal**: Goal two
|
||||
|
||||
### Phase 3: Three
|
||||
**Goal**: Goal three
|
||||
|
||||
### Phase 4: Four
|
||||
**Goal**: Goal four
|
||||
`,
|
||||
);
|
||||
}
|
||||
|
||||
function scaffoldPhaseDir(n, slug) {
|
||||
const padded = String(n).padStart(2, '0');
|
||||
const dir = path.join(tmpDir, '.planning', 'phases', `${padded}-${slug}`);
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
fs.writeFileSync(path.join(dir, `${padded}-01-PLAN.md`), '# Plan');
|
||||
fs.writeFileSync(path.join(dir, `${padded}-01-SUMMARY.md`), '# Summary');
|
||||
fs.writeFileSync(
|
||||
path.join(dir, `${padded}-VERIFICATION.md`),
|
||||
['---', 'status: passed', '---', '', '# Verification', ''].join('\n'),
|
||||
);
|
||||
}
|
||||
|
||||
function writeMinimalState() {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
'# State\n\n**Current Phase:** 2\n**Status:** In progress\n',
|
||||
);
|
||||
}
|
||||
|
||||
test('completing phase 2 out of order skips the already-complete phase 3 (#4699)', () => {
|
||||
writeRoadmap();
|
||||
writeMinimalState();
|
||||
scaffoldPhaseDir(1, 'one');
|
||||
scaffoldPhaseDir(2, 'two');
|
||||
scaffoldPhaseDir(3, 'three');
|
||||
|
||||
const result = runGsdTools('phase complete 2', tmpDir);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.equal(output.next_phase, '4',
|
||||
'next_phase must skip the already-[x] phase 3 and select the outstanding phase 4');
|
||||
assert.equal(output.is_last_phase, false);
|
||||
// #4699's actual harm was persistence: STATE.md used to carry the
|
||||
// already-complete phase as current_phase.
|
||||
const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.doesNotMatch(state, /current_phase:\s*3(\s|$)/m,
|
||||
'STATE.md must not carry the already-complete phase as current_phase');
|
||||
});
|
||||
|
||||
test('all later phases already [x] completes the milestone tail (#4699 corner)', () => {
|
||||
writeRoadmap({ fourthBox: '[x]' });
|
||||
writeMinimalState();
|
||||
scaffoldPhaseDir(1, 'one');
|
||||
scaffoldPhaseDir(2, 'two');
|
||||
scaffoldPhaseDir(3, 'three');
|
||||
|
||||
const result = runGsdTools('phase complete 2', tmpDir);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.equal(output.is_last_phase, true,
|
||||
'when every phase above N is already [x], completing N is the milestone tail');
|
||||
assert.equal(output.next_phase, null);
|
||||
});
|
||||
|
||||
test('uppercase [X] checkboxes are recognized as complete (#4699)', () => {
|
||||
writeRoadmap({ thirdBox: '[X]' });
|
||||
writeMinimalState();
|
||||
scaffoldPhaseDir(1, 'one');
|
||||
scaffoldPhaseDir(2, 'two');
|
||||
scaffoldPhaseDir(3, 'three');
|
||||
|
||||
const result = runGsdTools('phase complete 2', tmpDir);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.equal(output.next_phase, '4', '[X] is a complete checkbox, case-insensitively');
|
||||
});
|
||||
|
||||
test('checkbox completion matches phase numbers across zero-padding (#4699)', () => {
|
||||
// Roadmap spells the phase without padding; the directory carries the
|
||||
// zero-padded token — comparePhaseNum must dedupe them in the complete set.
|
||||
writeRoadmap({ thirdBox: '[x]' });
|
||||
writeMinimalState();
|
||||
scaffoldPhaseDir(1, 'one');
|
||||
scaffoldPhaseDir(2, 'two');
|
||||
scaffoldPhaseDir(3, 'three');
|
||||
|
||||
const result = runGsdTools('phase complete 2', tmpDir);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.equal(output.next_phase, '4');
|
||||
});
|
||||
|
||||
test('an outstanding phase 3 (unchecked) is still selected — negative control (#4699)', () => {
|
||||
writeRoadmap({ thirdBox: '[ ]' });
|
||||
writeMinimalState();
|
||||
scaffoldPhaseDir(1, 'one');
|
||||
scaffoldPhaseDir(2, 'two');
|
||||
scaffoldPhaseDir(3, 'three');
|
||||
|
||||
const result = runGsdTools('phase complete 2', tmpDir);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.equal(output.next_phase, '03',
|
||||
'without the fix scope change: an unchecked phase 3 stays a valid candidate (disk spelling wins)');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user