diff --git a/.changeset/curious-quails-tumble.md b/.changeset/curious-quails-tumble.md new file mode 100644 index 000000000..2b7dd041e --- /dev/null +++ b/.changeset/curious-quails-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3486 +--- +Phase-directory collisions in .planning/phases/ (two in-scope dirs normalizing to the same phase number) no longer resolve by filesystem mtime — a checkout-order signal that made progress.total_plans and completed_plans differ across clones of the same commit. The survivor is now chosen deterministically by lexicographic directory name, and the collision is surfaced as a stderr warning naming both directories. diff --git a/src/state.cts b/src/state.cts index 295b4aa58..516ca2bd3 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1863,8 +1863,9 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto // Bug #2445: when stale phase dirs from a prior milestone remain in // .planning/phases/ alongside new dirs with the same phase number, - // de-duplicate by normalized phase number keeping the most recently - // modified dir. This prevents double-counting (e.g. two "Phase 1" dirs). + // de-duplicate by normalized phase number keeping exactly one dir + // per key (deterministic tie-break: see #3355 below). This prevents + // double-counting (e.g. two "Phase 1" dirs). const seenPhaseNums = new Map(); // normalizedNum -> dirName for (const dir of allMatchingDirs) { // #1514: a retired/folded phase keeps a directory but no completion @@ -1883,14 +1884,25 @@ function buildStateFrontmatter(bodyContent: string, cwd: string | undefined, sto if (!seenPhaseNums.has(key)) { seenPhaseNums.set(key, dir); } else { - // Keep the dir that is newer on disk (more likely current milestone) - try { - const existing = path.join(phasesDir, seenPhaseNums.get(key) as string); - const candidate = path.join(phasesDir, dir); - if (fs.statSync(candidate).mtimeMs > fs.statSync(existing).mtimeMs) { - seenPhaseNums.set(key, dir); - } - } catch { /* keep existing on stat error */ } + // #3355: the survivor of a same-milestone collision must be + // chosen from repository CONTENT, never from filesystem state. + // The pre-#3355 tie-break was `mtimeMs` — a checkout-order + // signal — so two byte-identical checkouts of the same commit + // that wrote the colliding dirs in a different order picked + // different survivors, and progress.total_plans / + // completed_plans drifted across clones and CI runs. The + // directory NAME is git-tracked content and a total order, so + // the lexicographically-first dir wins deterministically. The + // collision is still a project-level defect (duplicate phase + // number in scope), so it is surfaced on stderr instead of + // being silently resolved. The Bug #2445 invariant — exactly + // one survivor per normalized phase number — is unchanged. + const incumbent = seenPhaseNums.get(key) as string; + const survivor = dir < incumbent ? dir : incumbent; + seenPhaseNums.set(key, survivor); + process.stderr.write( + `gsd: warning — phase directories '${incumbent}' and '${dir}' both normalize to phase key '${key}' (duplicate phase number in .planning/phases/); keeping '${survivor}' by deterministic lexicographic order. (#3355)\n` + ); } } const phaseDirs = [...seenPhaseNums.values()]; diff --git a/tests/state-document.test.cjs b/tests/state-document.test.cjs index b8649e521..4f67d0af7 100644 --- a/tests/state-document.test.cjs +++ b/tests/state-document.test.cjs @@ -1207,6 +1207,123 @@ describe('#3185 buildStateFrontmatter total_phases — directory-enumeration ind }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3355 — same-milestone phase-dir collision: the seenPhaseNums dedup loop in +// buildStateFrontmatter resolved duplicate phase keys by fs mtime, which +// encodes checkout write order, not repository content — identical commits +// reported different progress.total_plans / completed_plans across clones. +// The tie-break must be content-derived and the collision surfaced, while the +// Bug #2445 one-survivor-per-key invariant is preserved. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3355 phase-dir dedup — collision tie-break must not consult mtime', () => { + const { runNode } = require('./helpers/process-seam.cjs'); + const { TOOLS_PATH, TEST_ENV_BASE } = require('./helpers.cjs'); + + // Both normalize to phase key '3' (phaseKeyFromDir) and are IN scope on a + // flat roadmap, so they reach the dedup loop unfiltered. Distinct plan / + // summary counts make the survivor observable in the derived progress + // counts. Lexicographically 'mi' < 'mv', so DIR_LEX is the deterministic + // survivor; DIR_MTIME is the pre-fix winner whenever it is mtime-newer. + const DIR_LEX = '03-mi-cuenta-y-contactos'; + const DIR_MTIME = '03-mvp-modulos-portal-cliente'; + + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-3355-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function buildCollisionFixture() { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '## Phase 1: One', '## Phase 2: Two', '## Phase 3: Three', ''].join('\n'), + ); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), buildStateMd({ totalPhases: 3 })); + seedPhaseDirs(tmpDir, [1, 2]); // phases 01 + 02, one plan each + + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const lexDir = path.join(phasesDir, DIR_LEX); + fs.mkdirSync(lexDir, { recursive: true }); + fs.writeFileSync(path.join(lexDir, '03-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(lexDir, '03-02-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(lexDir, '03-01-SUMMARY.md'), '# Summary\n'); + const mtimeDir = path.join(phasesDir, DIR_MTIME); + fs.mkdirSync(mtimeDir, { recursive: true }); + for (let i = 1; i <= 5; i++) { + fs.writeFileSync(path.join(mtimeDir, `03-0${i}-PLAN.md`), '# Plan\n'); + } + for (let i = 1; i <= 3; i++) { + fs.writeFileSync(path.join(mtimeDir, `03-0${i}-SUMMARY.md`), '# Summary\n'); + } + return { phasesDir, lexDir, mtimeDir }; + } + + function readProgress() { + const result = runGsdTools(['state', 'json', '--raw'], tmpDir); + assert.ok(result.success, `state json --raw failed: ${result.error}`); + return JSON.parse(result.output).progress; + } + + test('#3355 flipping the colliding dirs\' mtimes keeps counts byte-identical, warns naming both dirs, one survivor per key', () => { + const { lexDir, mtimeDir } = buildCollisionFixture(); + + // The issue reported a 0.133 ms mtime margin flipping the winner, but + // filesystem timestamp granularity is platform-dependent (APFS rounds + // utimes to whole ms), so the flip uses a margin guaranteed to register + // everywhere — pre-fix, the mtime-newer duplicate then won the dedup + // regardless of repository content. `_diskScanCache` is process-local + // and runGsdTools spawns a fresh node per call, so each read below is a + // cold scan exactly like a fresh checkout. + const base = new Date('2026-01-01T00:00:00.000Z'); + const margin = new Date(base.getTime() + 1500); + fs.utimesSync(lexDir, base, base); + fs.utimesSync(mtimeDir, margin, margin); // DIR_MTIME mtime-newer — must NOT win + + const first = readProgress(); + + // Deterministic lexicographic survivor: 01 (1 plan) + 02 (1 plan) + + // DIR_LEX (2 plans) = 4 — never the mtime-newer DIR_MTIME's 7, never the + // un-deduped sum of both (Bug #2445 invariant, 9). + assert.strictEqual( + Number(first.total_plans), + 4, + `#3355: total_plans must follow the lexicographic survivor (${DIR_LEX}: 1+1+2=4), got ${first && first.total_plans} — mtime still deciding the collision?`, + ); + assert.strictEqual( + Number(first.completed_plans), + 1, + `#3355: completed_plans must count only the survivor's summaries (1), got ${first && first.completed_plans}`, + ); + + // The collision must be surfaced: stderr warning naming BOTH dirs + // (runGsdTools discards stderr on success, so drive the seam directly). + const rec = runNode( + [TOOLS_PATH, 'state', 'json', '--raw'], + { cwd: tmpDir, env: { ...process.env, ...TEST_ENV_BASE }, timeoutMs: 60000 }, + ); + assert.ok(rec.exitCode === 0, `state json --raw failed: ${rec.stderr}`); + assert.ok( + (rec.stderr || '').includes(DIR_LEX) && (rec.stderr || '').includes(DIR_MTIME), + `#3355: expected a stderr warning naming both colliding dirs, got stderr=${JSON.stringify(rec.stderr)}`, + ); + + // Flip the mtimes — byte content unchanged, only checkout order would + // differ. Every derived count must stay byte-identical. + fs.utimesSync(mtimeDir, base, base); + fs.utimesSync(lexDir, margin, margin); + + const second = readProgress(); + assert.strictEqual(second.total_plans, first.total_plans, `#3355: total_plans moved after the mtime flip (${first.total_plans} → ${second.total_plans})`); + assert.strictEqual(second.completed_plans, first.completed_plans, `#3355: completed_plans moved after the mtime flip (${first.completed_plans} → ${second.completed_plans})`); + assert.strictEqual(second.percent, first.percent, `#3355: percent moved after the mtime flip (${first.percent} → ${second.percent})`); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // #3354 — milestoned-but-unbounded: a genuinely milestone-sectioned ROADMAP // whose asserted milestone token matches no H1–H3 heading must not have its