* 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
This commit is contained in:
Tom Boucher
5
.changeset/graceful-koalas-forage.md
Normal file
5
.changeset/graceful-koalas-forage.md
Normal file
@@ -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).
|
||||
@@ -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<string> = [
|
||||
'Complete',
|
||||
'Needs Review',
|
||||
'Executed',
|
||||
'In Progress',
|
||||
'Planned',
|
||||
'Not Started',
|
||||
'Pending',
|
||||
];
|
||||
const PHASE_STATUS_RANK = new Map<string, number>(
|
||||
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,
|
||||
|
||||
@@ -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<string, string[]>();
|
||||
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');
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user