diff --git a/.changeset/merry-otters-wave.md b/.changeset/merry-otters-wave.md new file mode 100644 index 000000000..e230aa421 --- /dev/null +++ b/.changeset/merry-otters-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4450 +--- +**`phase.add --ws` now numbers the next phase from the workstream's own roadmap** — in a project with sibling git worktrees, `phase.add`/`phase.add-batch` with `--ws` minted a phase number pulled from the root roadmap's maximum (e.g. Phase 40 in a workstream whose own roadmap stopped at Phase 2), creating a `40-` directory and a `Depends on: Phase 39` entry pointing at a phase that does not exist in the workstream. The sibling-worktree widening horizon is now scoped like every other number source: a workstream-scoped allocation counts numbers held by the same workstream in sibling worktrees only. (#4225) diff --git a/src/phase.cts b/src/phase.cts index 832f345bf..df1679537 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -1235,12 +1235,24 @@ function assertDescriptionPreservesMilestoneScope(cwd: string, description: stri * before any directory does; milestone-scoping is wrong here because a number * used under any milestone on another branch is still taken). * + * #4225 — the horizon must track the ALLOCATION scope. When the allocation is + * workstream-scoped (`--ws`/`GSD_WORKSTREAM`, resolved into the env before + * dispatch), the sibling's copy of the SAME workstream is what carries that + * scope's independent numbering; the sibling's ROOT roadmap and phases/ + * belong to a different numbering universe (docs/FEATURES.md §51 REQ-WS-01 — + * workstream state is isolated in `.planning/workstreams/{name}/`) and must + * not contribute. `planningDir(wt, ws)` reuses the canonical resolver, so the + * sibling scope matches the local scope's own resolution (env workstream plus + * env project segment) by construction; `ws === null` (no workstream active) + * keeps the #3849 root-scope horizon byte-for-byte. + * * 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. + * A sibling that simply lacks the active workstream's directory is the same + * fail-open case: it contributes nothing. 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; @@ -1257,6 +1269,13 @@ function collectSiblingWorktreePhaseNums(cwd: string, used: Set): void { } catch { return; // not a git repo / git unavailable — unchanged behavior } + // #4225: the env workstream, read once with planningDir's own discriminator + // (`?? null` = deliberately no workstream — never re-derived per sibling). + // A poisoned value would already have thrown at the local `planningDir(cwd)` + // call every allocator makes before reaching this horizon; the per-sibling + // try/catch below still keeps any resolution failure fail-open. + const ws = process.env['GSD_WORKSTREAM'] ?? null; + const siblingPlanningDir = (wt: string): string => planningDir(wt, ws); 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; @@ -1265,17 +1284,17 @@ function collectSiblingWorktreePhaseNums(cwd: string, used: Set): void { 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'))) { + for (const entry of fs.readdirSync(path.join(siblingPlanningDir(wt), '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 */ + /* worktree has no .planning (or no copy of this scope) — normal, contributes nothing */ } try { - const content = fs.readFileSync(path.join(wt, '.planning', 'ROADMAP.md'), 'utf-8'); + const content = fs.readFileSync(path.join(siblingPlanningDir(wt), 'ROADMAP.md'), 'utf-8'); let m: RegExpExecArray | null; headerPattern.lastIndex = 0; while ((m = headerPattern.exec(content)) !== null) { @@ -1283,7 +1302,7 @@ function collectSiblingWorktreePhaseNums(cwd: string, used: Set): void { if (!isSentinelPhaseId(num)) used.add(num); } } catch { - /* no roadmap in that worktree — normal, contributes nothing */ + /* no roadmap in that worktree (or scope) — normal, contributes nothing */ } } } diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 83c36d646..5eb1fa273 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -2660,6 +2660,279 @@ describe('phase add allocation vs sibling git worktrees (#3849)', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// workstream-scoped phase allocation vs sibling git worktrees (#4225) +// +// The #3849 widening horizon (collectSiblingWorktreePhaseNums) scans each +// sibling's ROOT .planning/. A --ws allocation must instead scan the sibling's +// copy of the SAME workstream: workstream numbering is independent of the root +// roadmap (Workstream Namespacing REQ-WS-01 — workstream state is isolated in +// .planning/workstreams/{name}/), so a sibling's root-roadmap numbers must not +// leak into a workstream allocation. +// ───────────────────────────────────────────────────────────────────────────── + +describe('phase add --ws workstream-scoped allocation vs sibling git worktrees (#4225)', () => { + const activeWorktrees = []; + const activeDirs = []; + + function git(args, cwd) { + return execFileSync('git', args, { cwd, encoding: 'utf-8', timeout: 15_000 }); + } + + /** + * The #4225 topology: a committed ROOT roadmap with phases 1..rootMax (every + * linked worktree carries it), plus workstream `wsName` whose own roadmap and + * phases/ hold 1..wsMax. + */ + function initWsRepo(repoDir, rootMax, wsName, wsMax) { + const rootLines = ['# Roadmap', '', '## Milestone v1.0', '']; + for (let i = 1; i <= rootMax; i++) { + rootLines.push(`### Phase ${i}: root phase ${i}`, '', '**Goal:** root goal', ''); + } + fs.mkdirSync(path.join(repoDir, '.planning', 'phases'), { recursive: true }); + fs.writeFileSync(path.join(repoDir, '.planning', 'ROADMAP.md'), rootLines.join('\n') + '\n'); + + const wsBase = path.join(repoDir, '.planning', 'workstreams', wsName); + const wsLines = ['# Workstream Roadmap', '', '## Milestone v1.0', '']; + for (let i = 1; i <= wsMax; i++) { + wsLines.push(`### Phase ${i}: ws phase ${i}`, '', '**Goal:** ws goal', ''); + } + fs.mkdirSync(path.join(wsBase, 'phases'), { recursive: true }); + if (wsMax > 0) { + fs.mkdirSync(path.join(wsBase, 'phases', String(wsMax).padStart(2, '0') + '-ws-phase-' + wsMax)); + } + fs.writeFileSync(path.join(wsBase, 'ROADMAP.md'), wsLines.join('\n') + '\n'); + + 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); + return wsBase; + } + + /** A sibling linked worktree checked out at HEAD (carries the committed root roadmap). */ + function addSiblingAtHead(repoDir) { + const sha = git(['rev-parse', 'HEAD'], repoDir).trim(); + const worktreeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-sib-')); + git(['worktree', 'add', '--detach', worktreeDir, sha], repoDir); + activeWorktrees.push({ repoDir, worktreeDir }); + 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 --ws numbers from the workstream, not the sibling root roadmaps (issue verbatim)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-main-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 39, 'ws-alpha', 2); + addSiblingAtHead(repoDir); + + const result = runGsdTools(['query', 'phase.add', 'New workstream feature', '--ws', 'ws-alpha'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 3, + '#4225: the workstream\'s own next number (max 2 + 1), not the root roadmap\'s 39 + 1' + ); + assert.strictEqual(output.padded, '03'); + assert.strictEqual( + output.directory, + '.planning/workstreams/ws-alpha/phases/03-new-workstream-feature', + 'directory must land inside the workstream with the workstream-scoped number' + ); + + const wsRoadmap = fs.readFileSync( + path.join(repoDir, '.planning', 'workstreams', 'ws-alpha', 'ROADMAP.md'), + 'utf-8' + ); + assert.ok(wsRoadmap.includes('### Phase 3: New workstream feature'), 'ws roadmap entry must be Phase 3'); + assert.ok(wsRoadmap.includes('**Depends on:** Phase 2'), 'dependency must reference the in-workstream predecessor'); + + // The root roadmap and root phases/ are a different numbering universe — untouched. + const rootRoadmap = fs.readFileSync(path.join(repoDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok(!rootRoadmap.includes('New workstream feature'), 'root roadmap must not gain the workstream phase'); + assert.ok( + !fs.readdirSync(path.join(repoDir, '.planning', 'phases')).some((e) => e.startsWith('40-')), + 'root phases/ must not gain a 40- directory' + ); + }); + + test('phase add --ws still skips a number held by the SAME workstream in a sibling', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-same-ws-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 39, 'ws-alpha', 2); + const sibling = addSiblingAtHead(repoDir); + + // The sibling's ws-alpha copy holds Phase 3 (a branch that already minted it). + const sibWsPhases = path.join(sibling, '.planning', 'workstreams', 'ws-alpha', 'phases'); + fs.mkdirSync(path.join(sibWsPhases, '03-sibling-only'), { recursive: true }); + const sibWsRoadmap = path.join(sibling, '.planning', 'workstreams', 'ws-alpha', 'ROADMAP.md'); + fs.appendFileSync(sibWsRoadmap, '\n### Phase 3: sibling-only phase\n\n**Goal:** taken\n'); + + const result = runGsdTools(['query', 'phase.add', 'Contended', '--ws', 'ws-alpha'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 4, + '#4225 keeps the #3849 widening: a number taken by the same workstream on another branch is still taken — but the sibling root roadmap\'s 39 must not decide it' + ); + }); + + test('phase add --ws numbers the FIRST phase of an empty workstream as 1', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-empty-ws-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 39, 'ws-empty', 0); + addSiblingAtHead(repoDir); + + const result = runGsdTools(['query', 'phase.add', 'First steps', '--ws', 'ws-empty'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.phase_number, 1, 'an empty workstream starts at 1, not root-max+1 (40)'); + assert.strictEqual(output.padded, '01'); + assert.ok( + fs.existsSync(path.join(repoDir, '.planning', 'workstreams', 'ws-empty', 'phases', '01-first-steps')), + 'directory should be 01-first-steps inside the workstream' + ); + }); + + test('phase add --ws at the root maximum is coincidentally equal, with an in-workstream dependency', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-ws39-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 39, 'ws-alpha', 39); + addSiblingAtHead(repoDir); + + const result = runGsdTools(['query', 'phase.add', 'Fortieth in workstream', '--ws', 'ws-alpha'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.phase_number, 40, 'the workstream\'s own 39+1 — correct for its own reason'); + const wsRoadmap = fs.readFileSync( + path.join(repoDir, '.planning', 'workstreams', 'ws-alpha', 'ROADMAP.md'), + 'utf-8' + ); + assert.ok( + wsRoadmap.includes('**Depends on:** Phase 39'), + 'Phase 39 dependency is legitimate here: it exists in THIS workstream' + ); + }); + + test('a sibling holding only ANOTHER workstream\'s numbers does not affect --ws allocation', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-beta-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 39, 'ws-alpha', 2); + const sibling = addSiblingAtHead(repoDir); + + // The sibling's ws-beta (a different, independent numbering universe) is at 50. + const betaPhases = path.join(sibling, '.planning', 'workstreams', 'ws-beta', 'phases'); + fs.mkdirSync(path.join(betaPhases, '50-beta-heavy'), { recursive: true }); + const betaRoadmap = path.join(sibling, '.planning', 'workstreams', 'ws-beta', 'ROADMAP.md'); + fs.mkdirSync(path.dirname(betaRoadmap), { recursive: true }); + fs.writeFileSync(betaRoadmap, ['# Workstream Roadmap', '', '### Phase 50: beta heavy', '', '**Goal:** beta', ''].join('\n') + '\n'); + + const result = runGsdTools(['query', 'phase.add', 'Alpha next', '--ws', 'ws-alpha'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 3, + 'cross-workstream numbers (root 39 in the sibling root roadmap, ws-beta 50) are different universes — ws-alpha numbers from its own 2+1' + ); + }); + + test('a sibling without the workstream directory contributes nothing (fail open)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-no-ws-sib-')); + activeDirs.push(repoDir); + const wsBase = initWsRepo(repoDir, 39, 'ws-alpha', 2); + const sibling = addSiblingAtHead(repoDir); + + // The sibling predates the workstream: its checkout has no ws-alpha at all. + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- fixture SETUP, not teardown: strips the sibling's workstreams/ so it lacks the active scope; teardown() owns the dir's removal + fs.rmSync(path.join(sibling, '.planning', 'workstreams'), { recursive: true, force: true }); + + const result = runGsdTools(['query', 'phase.add', 'Local only', '--ws', 'ws-alpha'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 3, + 'a missing workstream scope in a sibling contributes nothing — local ws sources decide' + ); + assert.ok(fs.existsSync(path.join(wsBase, 'phases', '03-local-only'))); + }); + + test('phase add-batch --ws numbers sequentially from the workstream, not the root roadmap', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-batch-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 39, 'ws-alpha', 2); + addSiblingAtHead(repoDir); + + const result = runGsdTools( + ['query', 'phase.add-batch', '--descriptions', '["Batch A","Batch B"]', '--ws', 'ws-alpha'], + repoDir + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.phases[0].phase_number, 3, '#4225: the batch allocator shares the scoped horizon'); + assert.strictEqual(output.phases[1].phase_number, 4); + }); + + test('without --ws, allocation keeps the #3849 global sibling horizon byte-for-byte', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-flat-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 440, 'ws-alpha', 2); + const sibling = addSiblingAtHead(repoDir); + + // The sibling's ROOT roadmap holds Phase 441 (#3849's shape). + fs.appendFileSync( + path.join(sibling, '.planning', 'ROADMAP.md'), + '\n### Phase 441: sibling root phase\n\n**Goal:** taken\n' + ); + + const result = runGsdTools(['query', 'phase.add', 'Flat allocation'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 442, + 'no --ws: the sibling ROOT roadmap still widens the horizon exactly as #3849 shipped' + ); + }); + + test('phase next-decimal --ws is unaffected (planningDir-scoped already)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4225-dec-')); + activeDirs.push(repoDir); + initWsRepo(repoDir, 39, 'ws-alpha', 2); + addSiblingAtHead(repoDir); + + const result = runGsdTools(['query', 'phase.next-decimal', '2', '--ws', 'ws-alpha'], repoDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.next, '02.1', 'next-decimal was never sibling-widened; the fix must not change it'); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // phase add-batch command (#2165) // ─────────────────────────────────────────────────────────────────────────────