From b8cb031ce2d9f228c744a4e11600fb4da49e9d7a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 12 Aug 2026 21:01:07 -0400 Subject: [PATCH] fix(#3163): scope phase.add insertion to the current milestone (#3400) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3163): phase add must insert in the active milestone, not the trailing archive Regression for #3163: cmdPhaseAdd/cmdPhaseAddBatch pick the insertion point via rawContent.lastIndexOf('\n---'), the file's last horizontal rule — which on a roadmap with shipped/history material after the active phase list sits deep in archive. Rows 1/2/4 fail RED on next (entry lands after the archive heading); row 3 guards the no-milestone legacy fallback. * fix(#3163): scope phase.add insertion to the current milestone window cmdPhaseAdd and cmdPhaseAddBatch picked the insertion point via rawContent.lastIndexOf('\n---') — the file's last horizontal rule, which on a roadmap with shipped/history material after the active phase list sits deep in archive. Extract phaseEntryInsertOffset(rawContent, cwd): scope the search to currentMilestoneRawRanges' primary window so the entry lands at the end of the active phase list. Fall back to the legacy whole-file heuristic when no current milestone resolves, preserving simple no-milestone roadmaps. Applies to both cmdPhaseAdd and cmdPhaseAddBatch (identical expression); the decimal insert path was already header-anchored and is untouched. * docs(#3163): add changeset * docs(#3163): backfill changeset PR number (3400) --------- Co-authored-by: sim --- .changeset/daring-koalas-zip.md | 5 ++ src/phase.cts | 37 ++++++++---- tests/phase.test.cjs | 103 ++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 12 deletions(-) create mode 100644 .changeset/daring-koalas-zip.md diff --git a/.changeset/daring-koalas-zip.md b/.changeset/daring-koalas-zip.md new file mode 100644 index 000000000..358e53291 --- /dev/null +++ b/.changeset/daring-koalas-zip.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3400 +--- +**`phase add` no longer files new phases inside archived roadmap history** — the insertion point used the file's last horizontal rule, which on a long roadmap sits deep in shipped/archive content, so new phases landed under an unrelated archived phase's heading instead of at the end of the active phase list. Insertion is now scoped to the current milestone. (#3163) diff --git a/src/phase.cts b/src/phase.cts index c3644c522..05b33855a 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -993,6 +993,27 @@ function describeGoalShapedTitle(description: string): string | null { ); } +/** + * #3163: compute the byte offset in `rawContent` where a new `### Phase N:` + * entry should be inserted — at the end of the active phase list, scoped to the + * CURRENT MILESTONE so the entry can never land before a trailing `---` in + * shipped/history/backlog material (the file's last `---` on a long roadmap + * sits deep in archive). When no current milestone can be resolved (no + * STATE.md `milestone:` and no in-progress `🚧`/`🔄` marker), fall back to the + * legacy whole-file lastIndexOf('\n---') so simple no-milestone roadmaps keep + * their existing behavior. + */ +function phaseEntryInsertOffset(rawContent: string, cwd: string): number { + const ranges = currentMilestoneRawRanges(rawContent, cwd); + if (!ranges) { + const legacy = rawContent.lastIndexOf('\n---'); + return legacy > 0 ? legacy : rawContent.length; + } + const window = rawContent.slice(ranges.primary.start, ranges.primary.end); + const lastSeparator = window.lastIndexOf('\n---'); + return lastSeparator > 0 ? ranges.primary.start + lastSeparator : ranges.primary.end; +} + function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: string): void { if (!description) { error('description required for phase add'); @@ -1083,13 +1104,8 @@ function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: const phaseEntry = `\n### Phase ${_newPhaseId}: ${description}\n\n**Goal:** [To be planned]\n**Requirements**: TBD${dependsOn}\n**Plans:** 0 plans\n\nPlans:\n- [ ] TBD (run ${formatGsdSlash('plan-phase', resolveRuntime(cwd)) as string} ${_newPhaseId} to break down)\n`; - let updatedContent: string; - const lastSeparator = rawContent.lastIndexOf('\n---'); - if (lastSeparator > 0) { - updatedContent = rawContent.slice(0, lastSeparator) + phaseEntry + rawContent.slice(lastSeparator); - } else { - updatedContent = rawContent + phaseEntry; - } + const insertAt = phaseEntryInsertOffset(rawContent, cwd); + const updatedContent = rawContent.slice(0, insertAt) + phaseEntry + rawContent.slice(insertAt); platformWriteSync(roadmapPath, updatedContent); return { newPhaseId: _newPhaseId, dirName: _dirName }; @@ -1174,11 +1190,8 @@ function cmdPhaseAddBatch(cwd: string, descriptions: string[], raw: boolean): vo : `\n**Depends on:** Phase ${typeof newPhaseId === 'number' ? newPhaseId - 1 : 'TBD'}`; const phaseEntry = `\n### Phase ${newPhaseId}: ${description}\n\n**Goal:** [To be planned]\n**Requirements**: TBD${dependsOn}\n**Plans:** 0 plans\n\nPlans:\n- [ ] TBD (run ${formatGsdSlash('plan-phase', resolveRuntime(cwd)) as string} ${newPhaseId} to break down)\n`; - const lastSeparator = rawContent.lastIndexOf('\n---'); - rawContent = - lastSeparator > 0 - ? rawContent.slice(0, lastSeparator) + phaseEntry + rawContent.slice(lastSeparator) - : rawContent + phaseEntry; + const insertAt = phaseEntryInsertOffset(rawContent, cwd); + rawContent = rawContent.slice(0, insertAt) + phaseEntry + rawContent.slice(insertAt); added.push({ phase_number: typeof newPhaseId === 'number' ? newPhaseId : String(newPhaseId), padded: diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 9f639b8a5..01f4d62cd 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -1627,6 +1627,109 @@ describe('phase add command', () => { // phase add — orphan directory collision prevention (#2026) // ───────────────────────────────────────────────────────────────────────────── +describe('#3163: phase add inserts in the active milestone phase list, not the trailing archive', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // Fixture: a v1.0 ACTIVE milestone with a phase list, followed by a shipped + // v0.9 archive whose own `---` is the FILE's last `---`. The bug places the + // new phase before that archive `---`; the fix scopes to v1.0's window. + function writeArchiveRoadmap(dir, { singlePhase = false } = {}) { + fs.writeFileSync( + path.join(dir, '.planning', 'STATE.md'), + 'milestone: v1.0\ncurrent_phase: 2\n' + ); + const phases = singlePhase + ? '### Phase 1: Foundation\n**Goal:** setup\n' + : '### Phase 1: Foundation\n**Goal:** setup\n\n### Phase 2: API\n**Goal:** build\n'; + fs.writeFileSync( + path.join(dir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n## v1.0: Active Milestone\n\n' + + phases + + '\n---\n\n## v0.9: Shipped Archive\n\n### Phase 0 (original scope): Bootstrap\n**Goal:** init\n\n#### Operator decisions\n- decided X\n\n---\n' + ); + } + + test('row 1 — phase add lands in the active milestone, before the archive', () => { + writeArchiveRoadmap(tmpDir); + const result = runGsdTools('phase add New Feature', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const phase3 = roadmap.indexOf('### Phase 3: New Feature'); + const phase2 = roadmap.indexOf('### Phase 2: API'); + const archive = roadmap.indexOf('## v0.9: Shipped Archive'); + assert.notStrictEqual(phase3, -1, 'new phase entry should exist in the roadmap'); + assert.ok(phase2 > -1 && phase2 < phase3, `Phase 3 must come after Phase 2; got p2@${phase2} p3@${phase3}`); + assert.ok( + phase3 < archive, + `#3163: Phase 3 must land INSIDE the active v1.0 milestone (before the v0.9 archive), not before the file's last \`---\`; got phase3@${phase3} archive@${archive}` + ); + }); + + test('row 2 — phase add-batch is also scoped to the active milestone', () => { + writeArchiveRoadmap(tmpDir); + const result = runGsdTools( + ['phase', 'add-batch', '--descriptions', '["First Add","Second Add"]'], + tmpDir + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const archive = roadmap.indexOf('## v0.9: Shipped Archive'); + for (const [num, title] of [['3', 'First Add'], ['4', 'Second Add']]) { + const at = roadmap.indexOf(`### Phase ${num}: ${title}`); + assert.notStrictEqual(at, -1, `Phase ${num} (${title}) should exist`); + assert.ok( + at < archive, + `#3163: Phase ${num} must land before the archive; got @${at} archive@${archive}` + ); + } + }); + + test('row 3 — no-milestone fallback keeps legacy insertion before the trailing ---', () => { + // No STATE.md milestone field and no WIP marker → currentMilestoneRawRanges + // returns null → the legacy whole-file lastIndexOf('\n---') path is used. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n### Phase 1: Foundation\n**Goal:** setup\n\n---\n' + ); + const result = runGsdTools('phase add Next Phase', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const phase2 = roadmap.indexOf('### Phase 2: Next Phase'); + const sep = roadmap.indexOf('\n---'); + assert.notStrictEqual(phase2, -1, 'Phase 2 should exist'); + assert.ok( + phase2 < sep, + `no-milestone fallback must preserve legacy placement (before the trailing ---); got phase2@${phase2} sep@${sep}` + ); + }); + + test('row 4 — single-phase milestone still scopes to the active window', () => { + writeArchiveRoadmap(tmpDir, { singlePhase: true }); + const result = runGsdTools('phase add Second Phase', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const phase2 = roadmap.indexOf('### Phase 2: Second Phase'); + const archive = roadmap.indexOf('## v0.9: Shipped Archive'); + assert.notStrictEqual(phase2, -1, 'Phase 2 should exist'); + assert.ok( + phase2 < archive, + `Phase 2 must land inside the single-phase v1.0 window, before the archive; got @${phase2} archive@${archive}` + ); + }); +}); + describe('phase add — orphan directory collision prevention (#2026)', () => { let tmpDir;