From 6140627f5c5d8a994b1625805efa90b47e82003a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 20 Jul 2026 17:26:20 -0400 Subject: [PATCH] fix(#2427): ground smart-entry completion in ROADMAP-derived counts + tighten status regex (#2466) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2427): ground smart-entry completion in ROADMAP-derived counts + tighten status regex Two coupled defects in isComplete (src/smart-entry.cts): 1. Two-scale comparison: isComplete compared global current_phase (from STATE.md body 'Phase: N') against milestone-scoped total_phases (from STATE.md frontmatter progress.total_phases, written once at milestone- switch time and going stale as soon as new phases are appended to the roadmap). When current_phase >= stale total_phases (e.g. 7 >= 4), isComplete tripped true even though later phases were still unchecked in ROADMAP.md. 2. Over-broad status regex: /\bcomplete(d)?|done|shipped\b/i matched any 'shipped' or 'done' substring — including per-phase status like 'Phase X shipped — PR #N' — and falsely satisfied the status side of the completion check. Fix: - Added two new SmartEntrySignals fields: roadmap_total_phases and roadmap_completed_phases, populated by calling the existing deriveProgressFromRoadmap helper (from phase-lifecycle.cts:60) when ROADMAP.md exists. These are global, authoritative counts from the Progress table — never stale. - isComplete now prefers the roadmap-derived counts when available (completed >= total) and falls back to the legacy STATE.md comparison only when the roadmap has no parseable Progress table (backward compat for fresh or non-standard projects). - Tightened the status regex to /\b(milestone\s+complete|all\s+phases\s+complete|complete(d)?)\b/i. Drops 'done' and 'shipped' (per-phase language). Keeps milestone-level signals per ADR-2207 (milestone complete, all phases complete) plus the legacy short form 'complete'/'completed'. Tests (tests/smart-entry.unit.test.cjs gains a #2427 describe block): - Mid-milestone with stale total_phases=4, current_phase=7, per-phase 'shipped' status, and 3 unchecked roadmap phases → NOT complete (the core bug scenario). - All roadmap phases complete classifies as complete even with stale cached total_phases (roadmap wins). - Per-phase 'shipped' or 'done' status alone does NOT satisfy completion when roadmap phases are unchecked. - Legacy fallback: empty roadmap (no Progress table) still classifies via STATE.md comparison (backward compat). The makeProject test helper now accepts a string for the 'roadmap' parameter (written verbatim) in addition to the boolean shorthand, so tests can supply a real Progress table. References: #2427; ADR-2207 (milestone status lifecycle); ADR-2143 (column-name-driven Progress table parsing via deriveProgressFromRoadmap); triage note that this is a read-side fix only (the milestone-switch write path in state.cjs is out of scope). * chore(#2427): backfill pr:2466 in .changeset/curious-rams-run.md --- .changeset/curious-rams-run.md | 5 ++ src/smart-entry.cts | 76 +++++++++++++++++++- tests/smart-entry.unit.test.cjs | 123 +++++++++++++++++++++++++++++++- 3 files changed, 200 insertions(+), 4 deletions(-) create mode 100644 .changeset/curious-rams-run.md diff --git a/.changeset/curious-rams-run.md b/.changeset/curious-rams-run.md new file mode 100644 index 000000000..2835428a4 --- /dev/null +++ b/.changeset/curious-rams-run.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2466 +--- +**`/gsd-next` no longer reports a project as complete while phases are still unchecked** — `smart-entry`'s completion check now grounds in ROADMAP.md's actual Progress table (global, authoritative) instead of STATE.md's stale milestone-scoped total_phases, and its status regex requires milestone-level language (`milestone complete` / `all phases complete` / `complete`) instead of matching any per-phase `shipped` or `done` substring. Together these fix the false-complete misclassification that could route `/gsd-next` toward `/gsd-new-milestone` — which archives still-pending phase directories. diff --git a/src/smart-entry.cts b/src/smart-entry.cts index 91a36b6fa..c8c116b9e 100644 --- a/src/smart-entry.cts +++ b/src/smart-entry.cts @@ -42,6 +42,9 @@ const { planningPaths } = planningWorkspace; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); const { extractFrontmatter } = frontmatter; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-lifecycle.cjs is an export= CommonJS module +import phaseLifecycle = require('./phase-lifecycle.cjs'); +const { deriveProgressFromRoadmap } = phaseLifecycle; // eslint-disable-next-line @typescript-eslint/no-require-imports import stateDocument = require('./state-document.cjs'); const { stateExtractField } = stateDocument; @@ -89,6 +92,15 @@ export interface SmartEntrySignals { verify_failed: boolean; /** last_activity older than IDLE_STALE_MS (computed with the clock seam). */ stale_activity: boolean; + /** + * Global phase counts derived from ROADMAP.md's `## Progress` table (#2427). + * Preferred over the cached milestone-scoped `total_phases` (which goes stale + * when phases are appended after a milestone switch). Null when ROADMAP.md is + * absent or has no parseable Progress table — callers fall back to the legacy + * STATE.md comparison in that case. + */ + roadmap_total_phases: number | null; + roadmap_completed_phases: number | null; } export interface SmartEntryResult { @@ -294,6 +306,8 @@ export function detectSignals(cwd: string, now: () => number = Date.now): SmartE has_git: git.has_git, verify_failed: false, stale_activity: false, + roadmap_total_phases: null, + roadmap_completed_phases: null, }; if (!hasPlanning) return empty; @@ -350,6 +364,24 @@ export function detectSignals(cwd: string, now: () => number = Date.now): SmartE /\bverify-fail(ed)?|verification-fail|uat-fail\b/i.test(statusRaw || '') || detectVerifyFailed(cwd, currentPhaseRaw); + // #2427: derive global phase counts from ROADMAP.md's Progress table. These + // are preferred over STATE.md's cached milestone-scoped `total_phases` (which + // goes stale when phases are appended after a milestone switch) for the + // completion check. Null when ROADMAP.md is absent or has no parseable + // Progress table — isComplete falls back to the legacy comparison in that case. + let roadmapTotalPhases: number | null = null; + let roadmapCompletedPhases: number | null = null; + if (hasRoadmap) { + try { + const roadmapContent = fs.readFileSync(paths.roadmap, 'utf8'); + const derived = deriveProgressFromRoadmap(roadmapContent); + roadmapTotalPhases = derived.totalPhases; + roadmapCompletedPhases = derived.completedPhases; + } catch { + /* ROADMAP.md unreadable — leave null; isComplete falls back to legacy. */ + } + } + return { current_phase: parseIntOrNull(currentPhaseRaw), total_phases: parseIntOrNull(totalPhasesRaw), @@ -364,15 +396,53 @@ export function detectSignals(cwd: string, now: () => number = Date.now): SmartE has_git: git.has_git, verify_failed: verifyFailed, stale_activity: staleActivity, + roadmap_total_phases: roadmapTotalPhases, + roadmap_completed_phases: roadmapCompletedPhases, }; } // ─── Situation classification ───────────────────────────────────────────────── -/** True when the workflow has fully completed all phases. */ +/** + * True when the workflow has fully completed all phases. + * + * #2427: completion is grounded in ROADMAP.md's Progress table (global, + * authoritative, never stale) when available, with a legacy fallback to + * STATE.md's cached `total_phases` when the roadmap has no parseable Progress + * table (e.g. a fresh project or a non-standard roadmap layout). The status + * regex was tightened to require milestone-level completion language + * (`milestone complete` / `all phases complete` / `complete(d)`) and no longer + * matches per-phase messages like "Phase X shipped — PR #N" that falsely + * satisfied the pre-fix alternation (`\bcomplete(d)?|done|shipped\b`). + */ function isComplete(s: SmartEntrySignals): boolean { - if (s.total_phases === null || s.current_phase === null) return false; - return s.current_phase >= s.total_phases && /\bcomplete(d)?|done|shipped\b/i.test(s.status); + // Prefer ROADMAP-derived counts (global, authoritative) over STATE.md's + // cached milestone-scoped total_phases (stale-prone). Fall back to legacy + // when the roadmap has no Progress table. + if (s.roadmap_total_phases !== null && s.roadmap_completed_phases !== null) { + if (s.roadmap_total_phases === 0) return false; + if (s.roadmap_completed_phases < s.roadmap_total_phases) return false; + } else { + // Legacy path: STATE.md comparison. Still subject to the two-scale bug, + // but only fires when ROADMAP.md is absent or has no Progress table. + if (s.total_phases === null || s.current_phase === null) return false; + if (s.current_phase < s.total_phases) return false; + } + // Status regex: require milestone-level completion language. The pre-fix + // regex matched any "shipped" / "done" substring (per-phase language). + // Tightened to match: + // - "milestone complete" (ADR-2207 terminal status — usually written as + // " milestone complete", e.g. "v1.0 milestone complete"; the + // substring match handles both forms) + // - "all phases complete" (ADR-2207 intermediate terminal) + // - "complete" / "completed" (legacy short form — STATE.md milestone + // status is a single value, not a per-phase log) + // Intentionally does NOT match "done" alone even though normalizeStateStatus + // (state-document.cts) treats "done" as "completed" — in the milestone + // status field, "done" is per-phase noise (e.g. "Phase X done"), not a + // milestone-completion signal. Mirrors workstream-inventory-builder.cts's + // terminal pattern \bmilestone\s+complete\b. + return /\b(milestone\s+complete|all\s+phases\s+complete|complete(d)?)\b/i.test(s.status); } /** Idle-stranded: clean tree, committed work not shipped, optionally stale. */ diff --git a/tests/smart-entry.unit.test.cjs b/tests/smart-entry.unit.test.cjs index 0486a4f3a..a76c228ad 100644 --- a/tests/smart-entry.unit.test.cjs +++ b/tests/smart-entry.unit.test.cjs @@ -35,7 +35,12 @@ function makeProject({ state, roadmap = false, git = false, verifyFail = false } fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), state); } if (roadmap) { - fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); + // `roadmap === true` writes a minimal empty roadmap (no Progress table — + // the legacy test default). A string is written verbatim so tests can + // supply a real Progress table for the #2427 roadmap-derived completion + // check. + const content = typeof roadmap === 'string' ? roadmap : '# Roadmap\n'; + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), content); } if (git) { execFileSync('git', ['init'], { cwd: tmpDir, stdio: 'pipe' }); @@ -428,3 +433,119 @@ describe('smart-entry: CLI dispatch (gsd-tools smart-entry)', () => { assert.match(out, /Recommended:/); }); }); + +// ─── #2427: roadmap-grounded completion + tightened status regex ───────────── +// +// Pre-fix bug: isComplete compared global current_phase against stale +// milestone-scoped total_phases (written once at milestone-switch time) and +// matched any "shipped"/"done" substring in the status line — so a project +// mid-milestone with current_phase=7 > stale total_phases=4 AND a per-phase +// "Phase X shipped" status was falsely classified as "complete". The fix +// grounds completion in ROADMAP.md's Progress table (global, authoritative) +// and tightens the regex to require milestone-level language. + +describe('#2427 — roadmap-grounded completion + tightened status regex', () => { + afterEach(removeAll); + + /** + * Build a ROADMAP.md with a `## Progress` table of N rows, M of which are + * `Complete` and the rest `In Progress`. Matches the column-name-driven + * Progress table shape deriveProgressFromRoadmap scans. + */ + function roadmapWithProgress(total, completed) { + const rows = []; + for (let i = 1; i <= total; i++) { + const status = i <= completed ? 'Complete' : 'In Progress'; + const phase = String(i).padStart(2, '0'); + rows.push(`| ${phase} | 0/1 | ${status} | ${status === 'Complete' ? '2026-01-01' : ''} |`); + } + return [ + '# Roadmap', + '', + '## Milestone v1.0', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|---------------|--------|-----------|', + ...rows, + ].join('\n') + '\n'; + } + + test('mid-milestone with stale total_phases + unchecked roadmap phases is NOT complete', () => { + // The core bug: STATE.md says current_phase=7 >= total_phases=4 (stale, + // from a milestone switch when only 4 phases existed). Status contains + // "shipped" from a per-phase message. ROADMAP has 7 phases, only 4 done. + // MUST classify as something OTHER than "complete". + const dir = track(makeProject({ + state: state({ status: 'Phase 7 shipped — PR #42', total_phases: 4, current_phase: 7 }), + roadmap: roadmapWithProgress(7, 4), + })); + const result = classifyProject(dir); + assert.notEqual(result.situation, 'complete', + `mid-milestone (7 phases, 4 done) must NOT be "complete" even with stale total_phases=4 + current_phase=7. Got: ${result.situation}`); + }); + + test('all roadmap phases complete classifies as complete even with stale cached total_phases', () => { + // ROADMAP says all 5 phases are done. STATE.md has stale total_phases=3 + // (from an earlier milestone switch). The roadmap-derived check should + // win and classify as complete. Status uses ADR-2207's actual terminal + // written form " milestone complete" (state-transition.cts:1303). + const dir = track(makeProject({ + state: state({ status: 'v1.0 milestone complete', total_phases: 3, current_phase: 5 }), + roadmap: roadmapWithProgress(5, 5), + })); + const result = classifyProject(dir); + assert.equal(result.situation, 'complete', + `all roadmap phases complete must classify as "complete" regardless of stale cached total_phases. Got: ${result.situation}`); + }); + + test('per-phase "shipped" status alone does NOT satisfy the completion regex', () => { + // The pre-fix regex matched "shipped" as a standalone alternation branch. + // Tightened regex requires milestone-level language. With some phases + // unchecked AND status="shipped", must NOT be complete. + const dir = track(makeProject({ + state: state({ status: 'shipped', total_phases: 5, current_phase: 5 }), + roadmap: roadmapWithProgress(5, 3), + })); + const result = classifyProject(dir); + assert.notEqual(result.situation, 'complete', + `status "shipped" alone (per-phase language) must NOT satisfy completion when roadmap has unchecked phases. Got: ${result.situation}`); + }); + + test('per-phase "done" status alone does NOT satisfy the completion regex', () => { + // Same as above but with "done" — the other over-broad branch the pre-fix + // regex matched. + const dir = track(makeProject({ + state: state({ status: 'done', total_phases: 5, current_phase: 5 }), + roadmap: roadmapWithProgress(5, 3), + })); + const result = classifyProject(dir); + assert.notEqual(result.situation, 'complete', + `status "done" alone must NOT satisfy completion when roadmap has unchecked phases. Got: ${result.situation}`); + }); + + test('legacy fallback: empty roadmap still classifies via STATE.md comparison', () => { + // When ROADMAP.md has no Progress table (fresh project, non-standard + // layout), isComplete falls back to the legacy current_phase >= total_phases + // check. This preserves backward compat for projects that haven't adopted + // the Progress-table convention. + const dir = track(makeProject({ + state: state({ status: 'complete', total_phases: 5, current_phase: 5 }), + roadmap: true, // empty roadmap — no Progress table + })); + const result = classifyProject(dir); + assert.equal(result.situation, 'complete', + `legacy fallback (no roadmap Progress table) must still classify complete via STATE.md. Got: ${result.situation}`); + }); + + test('legacy fallback: empty roadmap with current_phase < total_phases is NOT complete', () => { + const dir = track(makeProject({ + state: state({ status: 'complete', total_phases: 5, current_phase: 3 }), + roadmap: true, + })); + const result = classifyProject(dir); + assert.notEqual(result.situation, 'complete', + `legacy fallback must still reject completion when current_phase < total_phases. Got: ${result.situation}`); + }); +});