diff --git a/.changeset/lively-geese-run.md b/.changeset/lively-geese-run.md new file mode 100644 index 000000000..3c0593cde --- /dev/null +++ b/.changeset/lively-geese-run.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4042 +--- +phase add and phase add-batch now count phase numbers held by sibling git worktrees before allocating max+1, instead of colliding with them (the reported incident minted a second Phase 441 while a worktree already held one with six written plans); add-batch also now counts roadmap bullet rows (#1229 finally reaches the batch path) (#3849) diff --git a/scripts/lint-phase-enumeration-drift.cjs b/scripts/lint-phase-enumeration-drift.cjs index 0c14308fe..c3e17995f 100644 --- a/scripts/lint-phase-enumeration-drift.cjs +++ b/scripts/lint-phase-enumeration-drift.cjs @@ -294,7 +294,10 @@ const FUNCTION_SCOPED_EXEMPTIONS = new Map([ [path.join('src', 'verify.cts'), new Set(['cmdValidateHealth', 'cmdVerifySchemaDrift'])], [path.join('src', 'init.cts'), new Set(['detectHasPriorPhases', 'detectUiPhaseActive'])], [path.join('src', 'milestone.cts'), new Set(['archivePhaseDirectories', 'cmdMilestoneComplete', 'cmdPhasesClear'])], - [path.join('src', 'phase.cts'), new Set(['cmdPhasesList', 'cmdPhaseNextDecimal', 'cmdPhasePlanIndex', 'cmdPhaseInsert', 'renameDecimalPhases', 'renameIntegerPhases'])], + // #3849: collectSiblingWorktreePhaseNums reads a SIBLING worktree's phases dir — + // a different checkout listMilestonePhaseDirs (cwd-scoped) cannot serve; the + // allocators' own on-disk scans stay unexempted. + [path.join('src', 'phase.cts'), new Set(['cmdPhasesList', 'cmdPhaseNextDecimal', 'cmdPhasePlanIndex', 'cmdPhaseInsert', 'renameDecimalPhases', 'renameIntegerPhases', 'collectSiblingWorktreePhaseNums'])], [path.join('src', 'audit.cts'), new Set(['listAuditPhaseTargets'])], [path.join('src', 'commands.cts'), new Set(['cmdHistoryDigest'])], [path.join('src', 'state.cts'), new Set(['cmdStateValidate', 'cmdStateSync', 'cmdStateRebuild'])], diff --git a/src/phase.cts b/src/phase.cts index e356d6b7a..e2a0658fd 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -18,6 +18,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { execFileSync } from 'node:child_process'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- io.cjs is an export= CommonJS module import ioMod = require('./io.cjs'); const { output, error, ERROR_REASON, formatDiagnosticToken } = ioMod; @@ -1141,6 +1142,68 @@ function assertDescriptionPreservesMilestoneScope(description: string, command: ); } +/** + * #3849 — widen "used phase numbers" beyond this checkout. Every sibling git + * worktree carries its own `.planning/` on its own branch, so a phase minted + * there is invisible to the cwd-scoped sources (headers, bullets, on-disk + * dirs). Scan each sibling's phase-directory names (cheap — dir names alone + * caught the real incident) and its WHOLE ROADMAP.md headers (a row can exist + * before any directory does; milestone-scoping is wrong here because a number + * used under any milestone on another branch is still taken). + * + * Widen, never refuse: a missing `.planning/`, an unreadable sibling, a + * non-git cwd, or an unavailable git binary each leave `used` untouched — + * allocation then behaves exactly as it did before this horizon existed. + * Sentinels reuse the canonical `isSentinelPhaseId`; the dir pattern is the + * same one the on-disk scan uses, so decimal sub-phases (`411.1-foo`) are + * correctly not integers. + */ +function collectSiblingWorktreePhaseNums(cwd: string, used: Set): void { + let porcelain: string; + try { + porcelain = execFileSync('git', ['worktree', 'list', '--porcelain'], { + cwd, + encoding: 'utf-8', + // Same subprocess band as the other git call sites (smart-entry, check-command-router): + // inside the 5-30s git window, hidden console window on Windows, bounded buffer. + timeout: 10_000, + windowsHide: true, + maxBuffer: 4 * 1024 * 1024, + }); + } catch { + return; // not a git repo / git unavailable — unchanged behavior + } + const dirNumPattern = /^(?:[A-Z][A-Z0-9]*-)?(\d+)-/; + // Same header shape the allocators scan locally (#1729 tag tolerance). + const headerPattern = /#{2,4}\s*Phase\s+(\d+)[A-Z]?(?:\.\d+)*(?:\s*\([^)\n]{0,200}\))?:/gi; + for (const line of porcelain.split('\n')) { + if (!line.startsWith('worktree ')) continue; + const wt = line.slice('worktree '.length).trim(); + if (!wt || path.resolve(wt) === path.resolve(cwd)) continue; + try { + for (const entry of fs.readdirSync(path.join(wt, '.planning', 'phases'))) { + const match = entry.match(dirNumPattern); + if (!match) continue; + const num = parseInt(match[1], 10); + if (!isSentinelPhaseId(num)) used.add(num); + } + } catch { + /* worktree has no .planning — normal, contributes nothing */ + } + try { + const content = fs.readFileSync(path.join(wt, '.planning', 'ROADMAP.md'), 'utf-8'); + let m: RegExpExecArray | null; + headerPattern.lastIndex = 0; + while ((m = headerPattern.exec(content)) !== null) { + const num = parseInt(m[1], 10); + if (!isSentinelPhaseId(num)) used.add(num); + } + } catch { + /* no roadmap in that worktree — normal, contributes nothing */ + } + } +} + function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: string): void { if (!description) { error('description required for phase add'); @@ -1214,6 +1277,9 @@ function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: // section headers, roadmap bullets, AND on-disk dirs above is what prevents the // #1229 collision (a bullet-only Phase N is now counted), so max+1 cannot reuse // an existing number. + // 4) Sibling git worktrees (#3849) — same max+1, wider horizon: a number + // taken on another branch is still taken. + collectSiblingWorktreePhaseNums(cwd, usedPhaseNums); const maxUsed = usedPhaseNums.size > 0 ? Math.max(...usedPhaseNums) : 0; _newPhaseId = maxUsed + 1; const paddedNum = String(_newPhaseId).padStart(2, '0'); @@ -1292,8 +1358,12 @@ function cmdPhaseAddBatch(cwd: string, descriptions: string[], raw: boolean): vo const content = extractCurrentMilestone(rawContent, cwd); let maxPhase = 0; if (config.phase_naming !== 'custom') { + // Same three cwd-scoped sources as cmdPhaseAdd (#1229): headers, roadmap + // bullets, on-disk dirs. The bullet scan was missing here — a bullet-only + // `Phase N` row was invisible to batch allocation (#3849 secondary). // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). const phasePattern = /#{2,4}\s*Phase\s+(\d+)[A-Z]?(?:\.\d+)*(?:\s*\([^)\n]{0,200}\))?:/gi; + const bulletPattern = /^[ \t]*-[ \t]*\[[^\]]{0,200}\][ \t]*\*{0,2}Phase[ \t]+(\d+)(?=[:.\s*]|$)/gim; let m: RegExpExecArray | null; while ((m = phasePattern.exec(content)) !== null) { const num = parseInt(m[1], 10); @@ -1301,6 +1371,11 @@ function cmdPhaseAddBatch(cwd: string, descriptions: string[], raw: boolean): vo if (isSentinelPhaseId(num)) continue; if (num > maxPhase) maxPhase = num; } + while ((m = bulletPattern.exec(content)) !== null) { + const num = parseInt(m[1], 10); + if (isSentinelPhaseId(num)) continue; + if (num > maxPhase) maxPhase = num; + } const phasesOnDisk = path.join(planningDir(cwd), 'phases'); if (fs.existsSync(phasesOnDisk)) { const dirNumPattern = /^(?:[A-Z][A-Z0-9]*-)?(\d+)-/; @@ -1313,6 +1388,12 @@ function cmdPhaseAddBatch(cwd: string, descriptions: string[], raw: boolean): vo if (num > maxPhase) maxPhase = num; } } + // 4) Sibling git worktrees (#3849) — same max+1, wider horizon. + const siblingNums = new Set(); + collectSiblingWorktreePhaseNums(cwd, siblingNums); + for (const num of siblingNums) { + if (num > maxPhase) maxPhase = num; + } } const added: Record[] = []; for (const description of descriptions) { diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index d3611ffcd..149c2b445 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -2495,6 +2495,171 @@ describe('find-phase scalar counts (#3218)', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// phase number allocation across sibling git worktrees (#3849) +// ───────────────────────────────────────────────────────────────────────────── + +describe('phase add allocation vs sibling git worktrees (#3849)', () => { + const activeWorktrees = []; + const activeDirs = []; + + function git(args, cwd) { + return execFileSync('git', args, { cwd, encoding: 'utf-8', timeout: 15_000 }); + } + + function initRepo(repoDir) { + fs.mkdirSync(path.join(repoDir, '.planning', 'phases'), { recursive: true }); + fs.writeFileSync( + path.join(repoDir, '.planning', 'ROADMAP.md'), + ['# Roadmap v1.0', '', '### Phase 440: existing thing', '**Goal:** Setup', '', '---', ''].join('\n') + ); + fs.mkdirSync(path.join(repoDir, '.planning', 'phases', '440-existing-thing')); + git(['init', '-b', 'main'], repoDir); + git(['config', 'user.email', 'test@example.com'], repoDir); + git(['config', 'user.name', 'Test'], repoDir); + git(['add', '-A'], repoDir); + git(['commit', '-m', 'init'], repoDir); + } + + /** Materialize the issue's repro: a sibling worktree branch holding Phase 441. */ + function addSiblingHolding441(repoDir) { + const sha = git(['rev-parse', 'HEAD'], repoDir).trim(); + const worktreeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3849-sib-')); + git(['worktree', 'add', '--detach', worktreeDir, sha], repoDir); + activeWorktrees.push({ repoDir, worktreeDir }); + fs.mkdirSync(path.join(worktreeDir, '.planning', 'phases', '441-conversation-surface'), { recursive: true }); + fs.writeFileSync( + path.join(worktreeDir, '.planning', 'ROADMAP.md'), + ['# Roadmap v1.0', '', '### Phase 440: existing thing', '**Goal:** Setup', '', '### Phase 441: conversation surface', '**Goal:** Talk', '', '---', ''].join('\n') + ); + return worktreeDir; + } + + /** A sibling worktree whose checkout has no .planning at all (fail-open case). */ + function addBareSibling(repoDir) { + const sha = git(['rev-parse', 'HEAD'], repoDir).trim(); + const worktreeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3849-bare-')); + git(['worktree', 'add', '--detach', worktreeDir, sha], repoDir); + activeWorktrees.push({ repoDir, worktreeDir }); + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- fixture SETUP, not teardown: strips the tracked .planning/ so this worktree has none; cleanup() owns the dir's removal with the Windows retry budget + fs.rmSync(path.join(worktreeDir, '.planning'), { recursive: true, force: true }); + return worktreeDir; + } + + function teardown() { + while (activeWorktrees.length) { + const { repoDir, worktreeDir } = activeWorktrees.pop(); + try { + git(['worktree', 'remove', '--force', worktreeDir], repoDir); + } catch (_) { /* best-effort; cleanup() below still removes the directory */ } + cleanup(worktreeDir); + } + while (activeDirs.length) cleanup(activeDirs.pop()); + } + + afterEach(teardown); + + test('phase add skips a number held only by a sibling worktree (dir + roadmap header)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3849-main-')); + activeDirs.push(repoDir); + initRepo(repoDir); + addSiblingHolding441(repoDir); + + const result = runGsdTools('phase add anything', repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 442, + '#3849: sibling worktree holds Phase 441 — allocation must skip to 442, not collide' + ); + assert.ok( + fs.existsSync(path.join(repoDir, '.planning', 'phases', '442-anything')), + 'directory for the non-colliding 442 should be created' + ); + }); + + test('phase add-batch skips a number held only by a sibling worktree', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3849-batch-')); + activeDirs.push(repoDir); + initRepo(repoDir); + addSiblingHolding441(repoDir); + + const result = runGsdTools(['phase', 'add-batch', '--descriptions', '["New Thing"]'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phases[0].phase_number, + 442, + '#3849: the batch allocator must widen its horizon the same way the single-add allocator does' + ); + }); + + test('a sibling worktree without .planning/ changes nothing (fail open)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3849-failopen-')); + activeDirs.push(repoDir); + initRepo(repoDir); + addBareSibling(repoDir); + + const result = runGsdTools('phase add next thing', repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.phase_number, 441, 'no sibling numbers visible — max+1 from the local 440'); + }); + + test('allocation FROM a linked worktree counts the main checkout too (the incident topology)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3849-linked-')); + activeDirs.push(repoDir); + initRepo(repoDir); + // The real incident ran the other direction: cwd is a linked worktree and + // the MAIN checkout (scanned as a sibling here) holds the higher number. + const sha = git(['rev-parse', 'HEAD'], repoDir).trim(); + const worktreeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3849-linked-wt-')); + git(['worktree', 'add', '--detach', worktreeDir, sha], repoDir); + activeWorktrees.push({ repoDir, worktreeDir }); + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- fixture SETUP, not teardown: strips the tracked .planning/ so this worktree has none; cleanup() owns the dir's removal with the Windows retry budget + fs.rmSync(path.join(worktreeDir, '.planning'), { recursive: true, force: true }); + fs.mkdirSync(path.join(worktreeDir, '.planning', 'phases'), { recursive: true }); + fs.writeFileSync( + path.join(worktreeDir, '.planning', 'ROADMAP.md'), + ['# Roadmap v1.0', '', '### Phase 440: base', '**Goal:** Setup', '', '---', ''].join('\n') + ); + + const result = runGsdTools('phase add from linked', worktreeDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 441, + '#3849: the main checkout (440) must be visible when allocating from a linked worktree' + ); + }); + + test('phase add-batch counts bullet-only Phase N rows (#1229 reached the batch path)', () => { + const tmp = createTempProject(); + try { + fs.writeFileSync( + path.join(tmp, '.planning', 'ROADMAP.md'), + ['# Roadmap v1.0', '', '- [ ] **Phase 11: bullet-only phase**', '', '---', ''].join('\n') + ); + const result = runGsdTools(['phase', 'add-batch', '--descriptions', '["After Bullet"]'], tmp); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + assert.strictEqual( + output.phases[0].phase_number, + 12, + '#3849 secondary: a bullet-only Phase 11 must not be re-allocated by add-batch' + ); + } finally { + cleanup(tmp); + } + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // phase add-batch command (#2165) // ─────────────────────────────────────────────────────────────────────────────