From be5113abfe75da7f6b6b8349d5743d7f2261a579 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 20 Jul 2026 15:49:57 -0400 Subject: [PATCH] fix(#2408): fold colliding phase statuses + add W023 collision warning (#2461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2408): fold colliding phase statuses + add W023 collision warning Two coupled bugs from #2408: 1. cmdStats last-write-wins status (src/commands.cts:1610-1618): when two on-disk phase directories normalize to the same phase key (e.g. `05-real/` and `05-real-stray/`), the directory-scan merge overwrote `status` with whatever the *current* directory in scan order computed, discarding `existing?.status` entirely. fs.readdirSync order is not stable across platforms, so /gsd-stats could silently report `Not Started` for a phase that is actually `Complete`. Plan/summary counts were already additively merged; only `status` was wrong. Fix: introduced a `foldPhaseStatus(a, b)` helper that returns whichever status is further along the precedence ladder `Complete > Needs Review > Executed > In Progress > Planned > Not Started` (with `Pending` and unrecognized statuses ranked after). The merge site now calls `existing ? foldPhaseStatus(existing.status, status) : status`. The fold is commutative, so the result is identical regardless of read order. 2. cmdValidateHealth had no collision-detection pass (src/verify.cts): codes W001-W022 cover every condition except normalized-key collisions, so an operator got zero signal that anything was wrong. Fix: added W023 — groups phaseDirEntries by their normalized phase key (via the existing normalizePhaseName + extractPhaseToken helpers from phase-id.cjs — same normalization cmdStats uses) and emits a warning for any group with ≥2 dirs. The warning names the normalized key, both directory names (sorted by comparePhaseNum for stable output), and each directory's independently-computed status (via determinePhaseStatus imported from commands.cjs). Wording is deliberately neutral — never guesses which directory is the real one. The optional --repair path from the issue is intentionally NOT implemented in this PR (the issue marked it lower priority and acceptance criterion 4 is vacuously satisfied by omission). Triage correction applied: the issue proposed W022, but that code is already in use for config.json model-tier validation (src/verify.cts :1372-1391). The next free code is W023, used here. Tests: - tests/commands.test.cjs: integration test that 05-real/ (Complete) + 05-real-stray/ (empty/Not Started) collide and stats reports the merged phase as Complete regardless of read order; plus a direct unit test of foldPhaseStatus asserting commutativity + correct precedence for every status pair + correct handling of unrecognized statuses. - tests/health-validation.test.cjs: integration test that W023 fires on the collision naming both dirs + their statuses (and uses neutral wording), plus a negative test that no W023 fires when only one dir exists per key. References: #2408; reporter's three-layer triage + acceptance criteria; triage correction that W022 is already in use (model-tier validation). * chore(#2408): backfill pr:2461 in .changeset/graceful-koalas-forage.md --- .changeset/graceful-koalas-forage.md | 5 ++ src/commands.cts | 48 +++++++++++- src/verify.cts | 51 ++++++++++++- tests/commands.test.cjs | 69 +++++++++++++++++ tests/health-validation.test.cjs | 108 +++++++++++++++++++++++++++ 5 files changed, 279 insertions(+), 2 deletions(-) create mode 100644 .changeset/graceful-koalas-forage.md diff --git a/.changeset/graceful-koalas-forage.md b/.changeset/graceful-koalas-forage.md new file mode 100644 index 000000000..20936f8ea --- /dev/null +++ b/.changeset/graceful-koalas-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2461 +--- +**`/gsd-stats` no longer misreports a phase as Not Started when two directories collide on the same phase key** — `cmdStats` now folds colliding statuses by precedence (Complete > Needs Review > Executed > In Progress > Planned > Not Started) instead of overwriting last-write-wins, so the furthest-along status wins regardless of `fs.readdirSync` order. Separately, `/gsd-health` now emits a new W023 warning whenever two or more real phase directories collide on the same normalized phase key, naming both directories and their independently-computed statuses (neutral wording — never guesses which is the real one). diff --git a/src/commands.cts b/src/commands.cts index 4b5f48e50..fa2377ca2 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -94,6 +94,45 @@ interface EffortSyncChange { // ─── Phase Status ───────────────────────────────────────────────────────────── +/** + * Phase-status precedence ladder — furthest-along wins (#2408). + * + * `cmdStats` builds `phasesByNumber` by scanning on-disk phase directories. + * When two directories normalize to the same phase key (e.g. `05-real/` and + * `05-real-stray/`), the status field must be folded by precedence rather + * than overwritten last-write-wins — otherwise `/gsd-stats` reports whatever + * directory `fs.readdirSync` happened to yield last, which is non-deterministic + * across platforms and can silently call a `Complete` phase `Not Started`. + */ +const PHASE_STATUS_PRECEDENCE: ReadonlyArray = [ + 'Complete', + 'Needs Review', + 'Executed', + 'In Progress', + 'Planned', + 'Not Started', + 'Pending', +]; +const PHASE_STATUS_RANK = new Map( + PHASE_STATUS_PRECEDENCE.map((s, i) => [s, i]), +); + +/** + * Fold two phase statuses by precedence — returns whichever is further along + * the {@link PHASE_STATUS_PRECEDENCE} ladder. Unrecognized statuses fall behind + * every recognized one (so a recognized status always wins over an unknown one; + * two unrecognized statuses favor `a` for determinism). + */ +function foldPhaseStatus(a: string, b: string): string { + const ra = PHASE_STATUS_RANK.get(a); + const rb = PHASE_STATUS_RANK.get(b); + if (ra === undefined && rb === undefined) return a; + if (ra === undefined) return b; + if (rb === undefined) return a; + // Lower rank = higher precedence (Complete=0 wins over Not Started=5). + return ra <= rb ? a : b; +} + /** * Determine phase status by checking plan/summary counts AND verification state. * Introduces "Executed" for phases with all summaries but no passing verification. @@ -1632,7 +1671,12 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { name: existing?.name || phaseName, plans: (existing?.plans || 0) + plans, summaries: (existing?.summaries || 0) + summaries, - status, + // #2408: fold colliding statuses by precedence rather than overwriting + // last-write-wins. fs.readdirSync order is non-deterministic across + // platforms, so a naive overwrite can report a Complete phase as Not + // Started (or vice versa) depending on read order. The fold picks the + // furthest-along status, matching what an operator expects. + status: existing ? foldPhaseStatus(existing.status, status) : status, }); } } catch { /* intentionally empty */ } @@ -1763,6 +1807,8 @@ function cmdCheckCommit(cwd: string, raw: boolean): void { export = { groupFilesBySubrepo, determinePhaseStatus, + foldPhaseStatus, + PHASE_STATUS_PRECEDENCE, cmdGenerateSlug, cmdCurrentTimestamp, cmdListTodos, diff --git a/src/verify.cts b/src/verify.cts index 544497192..f346f385d 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -40,7 +40,7 @@ import configLoaderMod = require('./config-loader.cjs'); const { loadConfig, CONFIG_DEFAULTS } = configLoaderMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); -const { normalizePhaseName, phaseTokenMatches, escapeRegex, getMilestoneFromPhaseId, OPTIONAL_PHASE_TAG_SOURCE, PHASE_NUMBER_TOKEN_SOURCE } = phaseIdMod; +const { normalizePhaseName, phaseTokenMatches, escapeRegex, getMilestoneFromPhaseId, OPTIONAL_PHASE_TAG_SOURCE, PHASE_NUMBER_TOKEN_SOURCE, extractPhaseToken, comparePhaseNum } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocatorMod = require('./phase-locator.cjs'); const { findPhaseInternal } = phaseLocatorMod; @@ -50,6 +50,9 @@ const { getMilestoneInfo, stripShippedMilestones, extractCurrentMilestone } = ro // eslint-disable-next-line @typescript-eslint/no-require-imports import worktreeSafetyMod = require('./worktree-safety.cjs'); const { inspectWorktreeHealth } = worktreeSafetyMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- commands.cjs is an export= CommonJS module +import commandsMod = require('./commands.cjs'); +const { determinePhaseStatus } = commandsMod; const { planningDir, planningRoot } = planningWorkspace; const { extractFrontmatter, parseMustHavesBlock } = frontmatterMod; @@ -1460,6 +1463,52 @@ function cmdValidateHealth( } } + // W023 (#2408): detect two or more real on-disk phase directories that + // normalize to the same phase key (e.g. `05-real/` + `05-real-stray/`). + // The collision silently breaks /gsd-stats status accuracy (now folded by + // precedence — see commands.cts foldPhaseStatus) and forces an operator + // decision. Wording is neutral — never guesses which directory is "real". + { + const groups = new Map(); + for (const e of phaseDirEntries) { + // extractPhaseToken never returns empty — for unparseable dir names it + // falls back to the dir name itself. Two distinct unparseable names + // therefore normalize to distinct keys and cannot false-positive here; + // only dirs whose tokens collapse to the same key (e.g. `05-real` and + // `05-real-stray` → token `05`) produce a collision group. + const token = extractPhaseToken(e.name); + const key = normalizePhaseName(token); + const list = groups.get(key); + if (list) list.push(e.name); + else groups.set(key, [e.name]); + } + for (const [key, dirs] of groups) { + if (dirs.length < 2) continue; + // Compute each dir's status independently so the warning is informative. + // Sort by phase id for stable output regardless of readdir order; tie- + // break on the dir name itself so two dirs sharing the same phase token + // (the collision case itself) still sort deterministically (V8's stable + // sort would otherwise fall back to non-portable fs.readdirSync order). + const described = dirs + .slice() + .sort((a, b) => comparePhaseNum(a, b) || String(a).localeCompare(String(b))) + .map((d) => { + const files = phaseDirFiles.get(d) || []; + const plans = files.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md').length; + const summaries = files.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md').length; + const status = determinePhaseStatus(plans, summaries, path.join(phasesDir, d), 'Not Started'); + return `${d} (${status})`; + }) + .join(', '); + addIssue( + 'warning', + 'W023', + `Phase directories collide on normalized key "${key}": ${described}`, + 'Inspect each directory; rename or remove the duplicate so only one directory maps to this phase key', + ); + } + } + for (const e of phaseDirEntries) { const phaseFiles = phaseDirFiles.get(e.name) || []; const plans = phaseFiles.filter((f) => f.endsWith('-PLAN.md') || f === 'PLAN.md'); diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index e59b5230b..dfaccd332 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -2154,6 +2154,75 @@ describe('stats command', () => { assert.strictEqual(stats.phases_completed, 1); assert.strictEqual(stats.phases.length, 1); }); + + // ─── #2408: cmdStats last-write-wins fix — colliding dirs fold by precedence ── + // + // Two on-disk phase directories that normalize to the same phase key + // (e.g. `05-real/` + `05-real-stray/`) used to silently overwrite `status` + // at the directory-scan merge site (last-write-wins), so /gsd-stats could + // report `Not Started` for a phase that is actually `Complete` depending on + // fs.readdirSync order. The fix folds colliding statuses by precedence + // (Complete > Needs Review > Executed > In Progress > Planned > Not Started), + // so the furthest-along status wins regardless of read order. + + test('#2408: colliding phase directories fold to the furthest-along status (Complete wins over Not Started)', () => { + // Two dirs that both normalize to phase key "05": `05-real/` (Complete) + // and `05-real-stray/` (empty → Not Started). The merged status MUST be + // Complete regardless of which directory the fs yields first. + const realDir = path.join(tmpDir, '.planning', 'phases', '05-real'); + fs.mkdirSync(realDir, { recursive: true }); + fs.writeFileSync(path.join(realDir, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(realDir, '01-01-SUMMARY.md'), '# Summary'); + fs.writeFileSync(path.join(realDir, 'VERIFICATION.md'), '---\nstatus: passed\n---\n# Verified'); + + const strayDir = path.join(tmpDir, '.planning', 'phases', '05-real-stray'); + fs.mkdirSync(strayDir, { recursive: true }); + + // ROADMAP declares Phase 5 so the dir-scan finds an explicit phase to populate. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# Roadmap', + '', + '## Milestone v1', + '', + '### Phase 5: Real', + '**Goal:** The real phase', + ].join('\n') + ); + + const result = runGsdTools('stats', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const stats = JSON.parse(result.output); + assert.strictEqual(stats.phases_total, 1, 'two colliding dirs must merge into one phase'); + assert.strictEqual(stats.phases_completed, 1, 'Complete status must win over Not Started after the fold'); + const phase05 = stats.phases.find((p) => p.number === '05'); + assert.ok(phase05, 'phase 05 must appear in stats output'); + assert.strictEqual(phase05.status, 'Complete', 'folded status must be Complete, not Not Started'); + }); + + test('#2408: foldPhaseStatus is commutative and order-independent (property)', () => { + // Direct unit test of the fold: a Complete colliding with a Not Started + // must yield Complete regardless of argument order. This is the property + // that makes the merge-site fix correct independent of fs read order. + const { foldPhaseStatus, PHASE_STATUS_PRECEDENCE } = require('../gsd-core/bin/lib/commands.cjs'); + assert.strictEqual(foldPhaseStatus('Complete', 'Not Started'), 'Complete'); + assert.strictEqual(foldPhaseStatus('Not Started', 'Complete'), 'Complete'); + assert.strictEqual(foldPhaseStatus('Complete', 'Complete'), 'Complete'); + // Every recognized status folded with a lower-precedence one wins. + for (let i = 0; i < PHASE_STATUS_PRECEDENCE.length - 1; i++) { + const higher = PHASE_STATUS_PRECEDENCE[i]; + const lower = PHASE_STATUS_PRECEDENCE[i + 1]; + assert.strictEqual(foldPhaseStatus(higher, lower), higher, `${higher} should beat ${lower}`); + assert.strictEqual(foldPhaseStatus(lower, higher), higher, `${higher} should beat ${lower} (commutative)`); + } + // Unrecognized status never beats a recognized one. + assert.strictEqual(foldPhaseStatus('Complete', '???'), 'Complete'); + assert.strictEqual(foldPhaseStatus('???', 'Complete'), 'Complete'); + // Two unrecognized → returns first arg (deterministic). + assert.strictEqual(foldPhaseStatus('foo', 'bar'), 'foo'); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/health-validation.test.cjs b/tests/health-validation.test.cjs index 6a5d0d9bb..d2c10ab78 100644 --- a/tests/health-validation.test.cjs +++ b/tests/health-validation.test.cjs @@ -1043,6 +1043,114 @@ describe('Drift item W005 — phaseDirNameRe: 999.X-name dirs must not trigger W }); }); +// ── W023 (#2408): collision warning for two on-disk phase dirs normalizing to +// the same phase key. The check groups phaseDirEntries by their normalized +// key (normalizePhaseName) and emits a warning for any group with ≥2 dirs, +// listing both directory names and each one's independently-computed status. +// Wording is neutral — never guesses which directory is "real". +describe('W023 — colliding phase directories (issue #2408)', () => { + let tmpDir; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2408-')); + const { planningDir, phasesDir } = mkplanning(tmpDir); + writeProjectMd(planningDir); + writeStateMd(planningDir, '1.0'); + writeConfigJson(planningDir); + + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '## Milestone v1.0', + '', + '### Phase 5: Real', + '**Goal:** The real phase', + ].join('\n'), + ); + + // 05-real/ — Complete (PLAN + SUMMARY + passing VERIFICATION) + const realDir = path.join(phasesDir, '05-real'); + fs.mkdirSync(realDir, { recursive: true }); + fs.writeFileSync(path.join(realDir, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(realDir, '01-01-SUMMARY.md'), '# Summary'); + fs.writeFileSync(path.join(realDir, 'VERIFICATION.md'), '---\nstatus: passed\n---\n# Verified'); + + // 05-real-stray/ — empty → Not Started + fs.mkdirSync(path.join(phasesDir, '05-real-stray'), { recursive: true }); + }); + + after(() => { cleanup(tmpDir); }); + + test('emits W023 naming both colliding directories and their statuses', () => { + const result = runGsdTools(['validate', 'health', '--json'], tmpDir); + assert.strictEqual(result.success, true, `unexpected failure: ${result.error}`); + const data = JSON.parse(result.output); + const w023 = (data.warnings ?? []).filter((w) => w.code === 'W023'); + assert.strictEqual(w023.length, 1, `Expected exactly one W023, got: ${JSON.stringify(w023)}`); + const msg = w023[0].message; + assert.ok(msg.includes('"05"'), `W023 message must name the normalized phase key, got: ${msg}`); + assert.ok(msg.includes('05-real'), `W023 must name the real dir, got: ${msg}`); + assert.ok(msg.includes('05-real-stray'), `W023 must name the stray dir, got: ${msg}`); + // Independently-computed statuses appear alongside each dir. + assert.ok(msg.includes('Complete'), `W023 must include 05-real's Complete status, got: ${msg}`); + assert.ok(msg.includes('Not Started'), `W023 must include 05-real-stray's Not Started status, got: ${msg}`); + // Neutral wording across every rendered field (message AND fix/action) — + // never guesses which dir is "the real one" or names a specific dir as the + // one to remove. The check scans the union of message+fix so a future drift + // toward "remove 05-real-stray" in either field fails this test. + const combined = `${w023[0].message} ${w023[0].fix || ''}`; + assert.ok(!/real directory is/i.test(combined) && !/remove\s+05-real-stray/i.test(combined), + `W023 wording must stay neutral across all rendered fields, got: ${combined}`); + }); + + test('W023 listing order is deterministic across readdirSync orders (tie-break on dir name)', () => { + // Re-running validate health should produce the same W023 message byte-for- + // byte. The collision case itself (two dirs sharing the same phase token) + // is exactly where comparePhaseNum returns 0 — the dir-name tie-break is + // what keeps the comma-separated order stable. fs.readdirSync order is + // non-deterministic across platforms; without the tie-break, this test + // could flake on some hosts. Run twice and assert identical output. + const r1 = runGsdTools(['validate', 'health', '--json'], tmpDir); + const r2 = runGsdTools(['validate', 'health', '--json'], tmpDir); + assert.strictEqual(r1.success && r2.success, true); + const m1 = JSON.parse(r1.output).warnings.find((w) => w.code === 'W023').message; + const m2 = JSON.parse(r2.output).warnings.find((w) => w.code === 'W023').message; + assert.strictEqual(m1, m2, `W023 must be deterministic across runs, got:\n ${m1}\n ${m2}`); + // And specifically: the lower-name dir comes first under localeCompare. + assert.ok(m1.indexOf('05-real (') < m1.indexOf('05-real-stray ('), + `tie-break must place '05-real' before '05-real-stray' (localeCompare), got: ${m1}`); + }); + + test('does NOT emit W023 when no collision exists (single dir per key)', () => { + // Fresh tree with a single non-colliding dir. + const clean = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2408-clean-')); + const { planningDir, phasesDir } = mkplanning(clean); + writeProjectMd(planningDir); + writeStateMd(planningDir, '1.0'); + writeConfigJson(planningDir); + fs.writeFileSync( + path.join(planningDir, 'ROADMAP.md'), + '# Roadmap\n\n## Milestone v1.0\n\n### Phase 5: Real\n**Goal:** The real phase\n', + ); + const realDir = path.join(phasesDir, '05-real'); + fs.mkdirSync(realDir, { recursive: true }); + fs.writeFileSync(path.join(realDir, '01-01-PLAN.md'), '# Plan'); + fs.writeFileSync(path.join(realDir, '01-01-SUMMARY.md'), '# Summary'); + + try { + const result = runGsdTools(['validate', 'health', '--json'], clean); + assert.strictEqual(result.success, true, `unexpected failure: ${result.error}`); + const data = JSON.parse(result.output); + const w023 = (data.warnings ?? []).filter((w) => w.code === 'W023'); + assert.strictEqual(w023.length, 0, `Expected zero W023 without collision, got: ${JSON.stringify(w023)}`); + } finally { + cleanup(clean); + } + }); +}); + // ── Drift Item W006-archived: PHASE_TOKEN_FROM_DIR_RE / MILESTONE_ARCHIVE_DIR_RE ─ // // forEachArchivedPhaseToken() in verify.cjs uses two inline regex constants.