* 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
This commit is contained in:
Tom Boucher
5
.changeset/curious-rams-run.md
Normal file
5
.changeset/curious-rams-run.md
Normal file
@@ -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.
|
||||
@@ -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
|
||||
// "<version> 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. */
|
||||
|
||||
@@ -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 "<version> 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}`);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user