golem15/msd-core
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#3849): phase allocation counts numbers held by sibling git worktrees (#4042)

Browse Source 
* test(#3849): phase allocation must skip numbers held by sibling worktrees (failing first)

* fix(#3849): phase allocation counts numbers held by sibling git worktrees

Both allocators (cmdPhaseAdd, cmdPhaseAddBatch) chose max+1 over numbers
gathered from ONE checkout — headers, bullets (add only), on-disk dirs.
Every sibling git worktree carries its own .planning/ on its own branch,
so a phase minted there was invisible and the same number was allocated
twice (the reported incident: two Phase 441s, one with six written
plans, surfaced a day late by human memory).

New shared horizon collectSiblingWorktreePhaseNums: one
git worktree list --porcelain, then per sibling — phase-dir names (the
cheap scan that would have caught the incident) and the WHOLE sibling
ROADMAP.md headers (a row can predate its dir; milestone-scoping would
be wrong — a number used under any milestone on another branch is
taken). Widen, never refuse: unreadable sibling / no .planning / not a
git repo / git unavailable each contribute nothing and allocation is
unchanged. Reuses isSentinelPhaseId and the allocators' own patterns.

Secondary (#1229 never reached batch): cmdPhaseAddBatch now also scans
roadmap bullets — a bullet-only 'Phase N' row was invisible to batch
allocation, exactly the condition #1229 was filed for.

Also fixes the two new tests' result-key access (output.phases, not
output.results).

* fix(#3849): review fold-ins — subprocess band, bounded test git, linked-worktree fixture

- execFileSync options now match the repo's git band (10s window,
  windowsHide, 4MiB maxBuffer) — a spurious 4s timeout silently reverted
  to the pre-fix collision.
- test git helper bounded (15s) per local/no-unbounded-spawn.
- new fixture: allocation FROM a linked worktree counts the main
  checkout — the incident's actual topology direction.
- fixture-setup rmSync carries the sanctioned lint-disable (setup, not
  teardown; cleanup() still owns directory removal).

* fix(#3849): exempt the sibling-worktree scan from the enumeration-drift guard

collectSiblingWorktreePhaseNums reads a SIBLING checkout's phases dir —
a different question from the cwd-scoped listMilestonePhaseDirs the
guard routes everything to (which cannot see another worktree's
.planning at all). Function-scoped, per ADR-3180 Decision 4(a): any
other re-derivation in phase.cts is still caught. The GREEN bench
caught the omission.

* chore(#3849): changeset fragment (pr number backfilled after PR creation)

* chore(#3849): backfill changeset PR number (4042)

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-29 11:35:19 -04:00
committed by GitHub
parent 519ac23ebb
commit 0bf778c352
4 changed files with 255 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/lively-geese-run.md Normal file
View File

@@ -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)

5
scripts/lint-phase-enumeration-drift.cjs
View File

@@ -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'])],

81
src/phase.cts
View File

@@ -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<number>): 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<number>();
collectSiblingWorktreePhaseNums(cwd, siblingNums);
for (const num of siblingNums) {
if (num > maxPhase) maxPhase = num;
}
}
const added: Record<string, unknown>[] = [];
for (const description of descriptions) {

165
tests/phase.test.cjs
View File

@@ -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)
// ─────────────────────────────────────────────────────────────────────────────