golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#3701): resolve next_phase from the roadmap, selecting the numerically lowest successor (#3852)

Browse Source 
* test(#3701): failing-first coverage for roadmap-order next_phase resolution

* fix(#3701): resolve next_phase from roadmap order, keeping the disk scan for spelling and fallback

* fix(#3701): select the numerically lowest successor in both scans, not the first row encountered

* chore(#3701): backfill changeset PR number

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-25 08:58:10 -04:00
committed by GitHub
parent 308c17505c
commit aa6c332b5a
4 changed files with 428 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

90
src/phase.cts
View File

@@ -3036,6 +3036,37 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
}
}
// #3701 — the ROADMAP decides WHICH phase is next; the disk decides only HOW it
// is spelled. Both scans select the numerically lowest phase above N.
//
// Both scans below are unchanged in what they match; what changed is that
// the roadmap is no longer gated behind "the disk found nothing". It used
// to be (`if (isLastPhase && roadmapContent !== null)`), which made a wrong
// disk answer uncorrectable: phase directories are created lazily, but
// `phase insert` scaffolds an inserted phase's directory immediately, so an
// inserted decimal is routinely the ONLY directory above N and outranked
// every phase preceding it in the roadmap. Observed: roadmap `1, 2, 02.1,
// 3` with directories for 01 and 02.1 only reported `next_phase: "02.1"`
// after completing 1 — and PERSISTED it to STATE.md — while
// `roadmap.analyze` correctly said `2`.
//
// #3581 fixed exactly this at `init.progress` and named the rule: "the
// frontier is ROADMAP ORDER, not artifact presence". This call site was not
// in that change's scope.
//
// Why the disk scan survives, rather than being replaced:
// 1. It is the only resolver when there is no ROADMAP.md, or when its
// phase rows do not parse.
// 2. When both agree, it carries the SPELLING the output has always used
// — the zero-padded directory token and the on-disk slug (`02`/`beta`),
// where the roadmap would give `2` and a slugified title. Promoting the
// roadmap without this would silently change the reported value on
// every aligned project, which is the majority case.
let diskNextNum: string | null = null;
let diskNextName: string | null = null;
let roadmapNextNum: string | null = null;
let roadmapNextName: string | null = null;
try {
// #3185 (ADR-3180 Decision 1): "which phase directories belong to
// the CURRENT milestone" — routed through the canonical owner
@@ -3051,11 +3082,15 @@ 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;
if (comparePhaseNum(dm[1], phaseNum) > 0) {
nextPhaseNum = dm[1];
nextPhaseName = dm[2] || null;
isLastPhase = false;
break;
// 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
// upstream sort nothing here states. Selecting the minimum explicitly costs
// one comparison and removes the hidden coupling.
if (comparePhaseNum(dm[1], phaseNum) > 0
&& (diskNextNum === null || comparePhaseNum(dm[1], diskNextNum) < 0)) {
diskNextNum = dm[1];
diskNextName = dm[2] || null;
}
}
}
@@ -3069,7 +3104,7 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
* — not a silent data-loss path. */
}
if (isLastPhase && roadmapContent !== null) {
if (roadmapContent !== null) {
try {
const roadmapForPhases = extractCurrentMilestone(roadmapContent, cwd);
// #1591: match BOTH heading-style phases (`### Phase N:`) AND
@@ -3097,15 +3132,28 @@ 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(pm[1])) continue;
if (comparePhaseNum(pm[1], phaseNum) > 0) {
nextPhaseNum = pm[1];
nextPhaseName = pm[2]
// #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`
// headings, so "first match" is a statement about where a line sits in the
// file — not about which phase comes next.
//
// It mattered only once this scan started deciding the answer. Before, it
// ran solely when the disk scan found nothing; now it outranks the disk, so
// a roadmap listing rows out of numeric sequence (`1, 3, 2`) reported
// `next_phase: 3` and PERSISTED it, skipping Phase 2 — on an input the
// pre-#3701 code got right, because the disk scan is numerically sorted.
// Phase NUMBERS define sequence here, exactly as `comparePhaseNum` does for
// the disk scan and for #2028's lowest-outstanding override; the roadmap
// defines which phases EXIST and which milestone they belong to.
if (comparePhaseNum(pm[1], phaseNum) > 0
&& (roadmapNextNum === null || comparePhaseNum(pm[1], roadmapNextNum) < 0)) {
roadmapNextNum = pm[1];
roadmapNextName = pm[2]
.replace(/\(INSERTED\)/i, '')
.trim()
.toLowerCase()
.replace(/\s+/g, '-');
isLastPhase = false;
break;
}
}
} catch {
@@ -3116,6 +3164,26 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
}
}
// Resolve. The roadmap wins on identity; the disk wins on spelling when it
// is talking about the same phase.
if (roadmapNextNum !== null) {
// Same comparator both scans already use to order phases, so "the disk
// and the roadmap mean the same phase" cannot drift from "N is above the
// one just completed". `02` and `2` compare equal, which is the whole
// point — they are the same phase spelled two ways.
const diskAgrees = diskNextNum !== null && comparePhaseNum(diskNextNum, roadmapNextNum) === 0;
nextPhaseNum = diskAgrees ? diskNextNum : roadmapNextNum;
nextPhaseName = diskAgrees ? diskNextName : roadmapNextName;
isLastPhase = false;
} else if (diskNextNum !== null) {
// No usable roadmap (absent, unreadable, or no parseable phase rows) —
// the disk is all there is. Unchanged from the pre-#3701 behaviour.
nextPhaseNum = diskNextNum;
nextPhaseName = diskNextName;
isLastPhase = false;
}
// #2028: don't stamp "All phases complete" when a LOWER-numbered phase is
// still outstanding. The two blocks above only clear isLastPhase when a
// HIGHER-numbered phase exists, so completing the numerically-highest phase