From 4a19d4db2ba20653e654eaa808233831719a0a0c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 22 May 2026 11:39:45 -0400 Subject: [PATCH] fix(3815): phase.insert handles checked-bullet ROADMAP format (#79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(3815): phase.insert parser handles checked-bullet ROADMAP format phaseInsert (TS) and cmdPhaseInsert (CJS) previously used a heading-only regex (#{2,4}\s*Phase\s+N:) to locate the target phase. On projects whose ROADMAP uses the checked-bullet format (- [ ] **Phase N: name** or - [ ] Phase N: name), the lookup always failed with "Phase N not found". Extend the locator to also accept the bullet form — mirroring the patterns already used by phaseRemove and phaseComplete. When bullet-style is detected, insert a new bullet entry after the matched line (preserving bold/plain style to match surrounding entries). The heading-style code path is unchanged. Also fix a pre-existing test timeout: the first registry-integration test in phase-lifecycle.test.ts was failing with STACK_TRACE_ERROR (masked timeout) because the cold import of index.js takes >5 s. Added { timeout: 30_000 }. Co-Authored-By: Claude Sonnet 4.6 * fix(3815): refine hybrid-ROADMAP detection, preserve #3098 parity Tighten the bullet-style branch guard: only treat a ROADMAP as bullet-style (and apply the bullet-insert path) when it contains ZERO heading-style phase entries (anyHeadingPattern test). A mixed (hybrid) ROADMAP — headings for some phases, bullet summaries for others — is the #3098 case where the detail section is absent; that path must still error with "missing a detail section". Adds a regression test (#3098 preserved) in both TS and CJS to confirm that a heading-style ROADMAP with a bullet-only entry for the target phase still fires the "missing a detail section" error, not the bullet-insert path. Co-Authored-By: Claude Sonnet 4.6 * chore: add changeset for #3815 phase.insert bullet-roadmap fix Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .../3815-phase-insert-bullet-roadmap.md | 5 + get-shit-done/bin/lib/phase.cjs | 102 +++++++++++--- sdk/src/query/phase-lifecycle.test.ts | 124 +++++++++++++++++- sdk/src/query/phase-lifecycle.ts | 72 +++++++++- tests/phase.test.cjs | 11 +- 5 files changed, 288 insertions(+), 26 deletions(-) create mode 100644 .changeset/3815-phase-insert-bullet-roadmap.md diff --git a/.changeset/3815-phase-insert-bullet-roadmap.md b/.changeset/3815-phase-insert-bullet-roadmap.md new file mode 100644 index 000000000..cb04be7ed --- /dev/null +++ b/.changeset/3815-phase-insert-bullet-roadmap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3815 +--- +**`phase.insert` now handles checked-bullet ROADMAP format** — `gsd-sdk query phase.insert` (and `gsd-tools phase insert`) previously threw "Phase N not found" on ROADMAPs that use the `- [ ] **Phase N: name**` checklist format instead of `### Phase N: name` headings. The parser now detects purely bullet-style ROADMAPs and inserts the new decimal phase entry between the target bullet and the next phase bullet (preserving bold vs plain formatting). Hybrid ROADMAPs that mix heading-style phases with bullet summaries continue to produce the "missing a detail section" error from #3098, since a bullet-only entry in that context means the `### Phase N:` detail section is absent. (#3815) diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index 0afd100c6..dbea48464 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -759,8 +759,32 @@ function cmdPhaseInsert(cwd, afterPhase, description, raw) { const normalizedAfter = normalizePhaseName(afterPhase); const afterPhaseEscaped = phaseMarkdownRegexSource(normalizedAfter); const targetPattern = new RegExp(`#{2,4}\\s*Phase\\s+${afterPhaseEscaped}:`, 'i'); - if (!targetPattern.test(content)) { - const checklistPattern = new RegExp(`-\\s*\\[[ x]\\]\\s*\\*\\*Phase\\s+${afterPhaseEscaped}:`, 'i'); + const headingMatch = targetPattern.test(content); + + // #3815: also recognise the checked-bullet phase format used by projects + // that list phases as `- [ ] **Phase N: name**` or `- [ ] Phase N: name` + // (both bold and plain variants). Mirrors phaseRemove / phaseComplete. + // + // Bullet-style only activates when there are NO heading-style phases in the + // milestone content. A bullet entry in a hybrid (headings + bullets) ROADMAP + // means the detail section is missing — that is the #3098 case and must keep + // producing the "missing a detail section" error. + const bulletPattern = new RegExp( + `-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+${afterPhaseEscaped}[:\\s]`, + 'i', + ); + const anyHeadingPattern = /#{2,4}\s*Phase\s+\d/i; + const roadmapHasHeadingPhases = anyHeadingPattern.test(content); + const isBulletStyle = !headingMatch && bulletPattern.test(content) && !roadmapHasHeadingPhases; + + if (!headingMatch && !isBulletStyle) { + // Bug #3098 parity: when the ROADMAP uses heading-style phases and only + // the summary checklist exists for this phase (no `### Phase N:` detail + // section), point the user at the missing detail section. + const checklistPattern = new RegExp( + `-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+${afterPhaseEscaped}[:\\s]`, + 'i', + ); if (checklistPattern.test(content)) { error(`Phase ${afterPhase} exists in roadmap summary but is missing a detail section (### Phase ${afterPhase}: ...).`); } @@ -806,28 +830,70 @@ function cmdPhaseInsert(cwd, afterPhase, description, raw) { platformEnsureDir(dirPath); platformWriteSync(path.join(dirPath, '.gitkeep'), ''); - // Build phase entry - const phaseEntry = `\n### Phase ${_decimalPhase}: ${description} (INSERTED)\n\n**Goal:** [Urgent work - to be planned]\n**Requirements**: TBD\n**Depends on:** Phase ${afterPhase}\n**Plans:** 0 plans\n\nPlans:\n- [ ] TBD (run ${formatGsdSlash('plan-phase', resolveRuntime(cwd))} ${_decimalPhase} to break down)\n`; + let updatedContent; - // Insert after the target phase section - const headerPattern = new RegExp(`(#{2,4}\\s*Phase\\s+${afterPhaseEscaped}:[^\\n]*\\n)`, 'i'); - const headerMatch = rawContent.match(headerPattern); - if (!headerMatch) { - error(`Could not find Phase ${afterPhase} header`); - } + if (isBulletStyle) { + // #3815: Insert in checked-bullet format, mirroring the style of the + // surrounding entries. Detect whether the matched bullet uses bold + // (`**Phase N: …**`) to preserve file-internal format consistency. + const boldBulletPattern = new RegExp( + `-\\s*\\[[ x]\\]\\s*\\*\\*Phase\\s+${afterPhaseEscaped}:`, + 'i', + ); + const useBold = boldBulletPattern.test(content); + const phaseLabel = useBold + ? `**Phase ${_decimalPhase}: ${description}**` + : `Phase ${_decimalPhase}: ${description}`; + const bulletEntry = `\n- [ ] ${phaseLabel}`; - const headerIdx = rawContent.indexOf(headerMatch[0]); - const afterHeader = rawContent.slice(headerIdx + headerMatch[0].length); - const nextPhaseMatch = afterHeader.match(/\n#{2,4}\s+Phase\s+\d/i); + // Locate the target bullet line in the raw content + const targetBulletPattern = new RegExp( + `(-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+${afterPhaseEscaped}[:\\s][^\\n]*)`, + 'i', + ); + const bulletMatchResult = rawContent.match(targetBulletPattern); + if (!bulletMatchResult) { + error(`Could not find Phase ${afterPhase} bullet line`); + } - let insertIdx; - if (nextPhaseMatch) { - insertIdx = headerIdx + headerMatch[0].length + nextPhaseMatch.index; + const bulletLineEnd = rawContent.indexOf(bulletMatchResult[0]) + bulletMatchResult[0].length; + const afterBullet = rawContent.slice(bulletLineEnd); + const nextBulletMatch = afterBullet.match(/\n-\s*\[[ x]\]\s*(?:\*\*)?Phase\s+\d/i); + + let insertIdx; + if (nextBulletMatch) { + insertIdx = bulletLineEnd + nextBulletMatch.index; + } else { + insertIdx = bulletLineEnd; + } + + updatedContent = rawContent.slice(0, insertIdx) + bulletEntry + rawContent.slice(insertIdx); } else { - insertIdx = rawContent.length; + // Heading-style insert (original path) + // Build phase entry + const phaseEntry = `\n### Phase ${_decimalPhase}: ${description} (INSERTED)\n\n**Goal:** [Urgent work - to be planned]\n**Requirements**: TBD\n**Depends on:** Phase ${afterPhase}\n**Plans:** 0 plans\n\nPlans:\n- [ ] TBD (run ${formatGsdSlash('plan-phase', resolveRuntime(cwd))} ${_decimalPhase} to break down)\n`; + + // Insert after the target phase section + const headerPattern = new RegExp(`(#{2,4}\\s*Phase\\s+${afterPhaseEscaped}:[^\\n]*\\n)`, 'i'); + const headerMatch = rawContent.match(headerPattern); + if (!headerMatch) { + error(`Could not find Phase ${afterPhase} header`); + } + + const headerIdx = rawContent.indexOf(headerMatch[0]); + const afterHeader = rawContent.slice(headerIdx + headerMatch[0].length); + const nextPhaseMatch = afterHeader.match(/\n#{2,4}\s+Phase\s+\d/i); + + let insertIdx; + if (nextPhaseMatch) { + insertIdx = headerIdx + headerMatch[0].length + nextPhaseMatch.index; + } else { + insertIdx = rawContent.length; + } + + updatedContent = rawContent.slice(0, insertIdx) + phaseEntry + rawContent.slice(insertIdx); } - const updatedContent = rawContent.slice(0, insertIdx) + phaseEntry + rawContent.slice(insertIdx); platformWriteSync(roadmapPath, updatedContent); return { decimalPhase: _decimalPhase, dirName: _dirName }; }); diff --git a/sdk/src/query/phase-lifecycle.test.ts b/sdk/src/query/phase-lifecycle.test.ts index 9f7def534..a79faee3e 100644 --- a/sdk/src/query/phase-lifecycle.test.ts +++ b/sdk/src/query/phase-lifecycle.test.ts @@ -705,6 +705,123 @@ describe('phaseInsert', () => { await expect(phaseInsert([], tmpDir)).rejects.toThrow('after-phase and description required'); }); + + it('#3098 preserved: throws when bullet-only entry in a heading-style ROADMAP (hybrid = missing detail section)', async () => { + // A hybrid ROADMAP: phase 9 has a heading but phase 10 only has a bullet + // summary entry. Insert must still reject with "missing a detail section" + // because the surrounding ROADMAP uses heading-style phases. + const { phaseInsert } = await import('./phase-lifecycle.js'); + const hybridRoadmap = `# Roadmap\n\n## Current Milestone\n\n### Phase 9: Foundation\n\n- [ ] **Phase 10: Queries**\n`; + await setupTestProject(tmpDir, { + roadmap: hybridRoadmap, + phases: ['09-foundation'], + }); + + await expect(phaseInsert(['10', 'Hotfix'], tmpDir)).rejects.toThrow('missing a detail section'); + }); + + // ─── #3815: checked-bullet ROADMAP format ───────────────────────────── + + const BULLET_ONLY_ROADMAP = `# Roadmap + +## Current Milestone: v2.0 Agent Skills + +- [x] **Phase 01: bootstrap** — completed 2026-04-01 +- [ ] **Phase 02: core-loop** +- [ ] **Phase 03: persistence** + +--- +*Last updated: 2026-05-01* +`; + + it('#3815: inserts decimal phase between bullets in a checked-bullet ROADMAP', async () => { + const { phaseInsert } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir, { + roadmap: BULLET_ONLY_ROADMAP, + phases: ['01-bootstrap', '02-core-loop', '03-persistence'], + }); + + const result = await phaseInsert(['02', 'hot patch'], tmpDir); + const data = result.data as Record; + + expect(data.phase_number).toBe('02.1'); + expect(data.after_phase).toBe('02'); + expect(data.name).toBe('hot patch'); + + const roadmap = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + + // New bullet must be present + expect(roadmap).toMatch(/- \[ \].*Phase 02\.1:.*hot patch/i); + + // New bullet must appear AFTER the Phase 02 bullet + const phase02Idx = roadmap.indexOf('Phase 02: core-loop'); + const insertedIdx = roadmap.search(/Phase 02\.1:/i); + expect(phase02Idx).toBeGreaterThanOrEqual(0); + expect(insertedIdx).toBeGreaterThan(phase02Idx); + + // New bullet must appear BEFORE the Phase 03 bullet (positional assertion) + const phase03Idx = roadmap.indexOf('Phase 03: persistence'); + expect(insertedIdx).toBeLessThan(phase03Idx); + + // Must NOT corrupt the surrounding bullet structure + expect(roadmap).toContain('- [x] **Phase 01: bootstrap**'); + expect(roadmap).toContain('- [ ] **Phase 02: core-loop**'); + expect(roadmap).toContain('- [ ] **Phase 03: persistence**'); + }); + + it('#3815: inserts at end of bullet list when target is the last phase', async () => { + const { phaseInsert } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir, { + roadmap: BULLET_ONLY_ROADMAP, + phases: ['01-bootstrap', '02-core-loop', '03-persistence'], + }); + + const result = await phaseInsert(['03', 'final extra'], tmpDir); + const data = result.data as Record; + + expect(data.phase_number).toBe('03.1'); + + const roadmap = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + expect(roadmap).toMatch(/- \[ \].*Phase 03\.1:.*final extra/i); + + // Must appear after Phase 03 bullet + const phase03Idx = roadmap.indexOf('Phase 03: persistence'); + const insertedIdx = roadmap.search(/Phase 03\.1:/i); + expect(insertedIdx).toBeGreaterThan(phase03Idx); + }); + + it('#3815: plain bullet format (no bold) also works', async () => { + const { phaseInsert } = await import('./phase-lifecycle.js'); + const PLAIN_BULLET_ROADMAP = `# Roadmap + +## Current Milestone: v2.0 + +- [ ] Phase 01: foo +- [ ] Phase 02: bar +- [ ] Phase 03: baz + +--- +*Last updated: 2026-05-01* +`; + await setupTestProject(tmpDir, { + roadmap: PLAIN_BULLET_ROADMAP, + phases: ['01-foo', '02-bar', '03-baz'], + }); + + const result = await phaseInsert(['02', 'inserted'], tmpDir); + const data = result.data as Record; + + expect(data.phase_number).toBe('02.1'); + + const roadmap = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + expect(roadmap).toMatch(/- \[ \].*Phase 02\.1:.*inserted/i); + + const phase02Idx = roadmap.indexOf('Phase 02: bar'); + const insertedIdx = roadmap.search(/Phase 02\.1:/i); + const phase03Idx = roadmap.indexOf('Phase 03: baz'); + expect(insertedIdx).toBeGreaterThan(phase02Idx); + expect(insertedIdx).toBeLessThan(phase03Idx); + }); }); // ─── phaseScaffold ────────────────────────────────────────────────────── @@ -1599,6 +1716,11 @@ describe('milestoneComplete help-flag defense', () => { // ─── Registry integration ────────────────────────────────────────────────── describe('lifecycle handlers in registry', () => { + // Registry assembly (invariant checks + large handler map) routinely takes + // 5–10 s on a cold ESM module cache. The second test below reuses the + // cached import and completes instantly, but the first import is the cold + // path. Raise the per-test timeout so the warm-up doesn't surface as a + // spurious "Test timed out" failure (pre-existing: STACK_TRACE_ERROR mask). it('registers all 7 lifecycle handlers with dot notation', async () => { const { createRegistry } = await import('./index.js'); const registry = createRegistry(); @@ -1612,7 +1734,7 @@ describe('lifecycle handlers in registry', () => { const handler = registry.getHandler(cmd); expect(handler, `${cmd} should be registered`).toBeDefined(); } - }); + }, 30_000); it('registers space-delimited aliases', async () => { const { createRegistry } = await import('./index.js'); diff --git a/sdk/src/query/phase-lifecycle.ts b/sdk/src/query/phase-lifecycle.ts index d2a4176d2..b76548cef 100644 --- a/sdk/src/query/phase-lifecycle.ts +++ b/sdk/src/query/phase-lifecycle.ts @@ -409,12 +409,34 @@ export const phaseInsert: QueryHandler = async (args, projectDir, workstream) => const unpadded = normalizedAfter.replace(/^0+/, ''); const afterPhaseEscaped = unpadded.replace(/\./g, '\\.'); const targetPattern = new RegExp(`#{2,4}\\s*Phase\\s+0*${afterPhaseEscaped}:`, 'i'); - if (!targetPattern.test(content)) { - // Bug #3098 parity: when only the summary checklist exists for this - // phase (no `### Phase N:` detail section), point the user at the - // missing detail section rather than implying the phase is absent. + const headingMatch = targetPattern.test(content); + + // #3815: also recognise the checked-bullet phase format used by projects + // that list phases as `- [ ] **Phase N: name**` or `- [ ] Phase N: name` + // (both bold and plain variants). This mirrors the patterns already used + // by phaseRemove / phaseComplete so insert is consistent with its siblings. + // + // Bullet-style is only used when there are NO heading-style phases at all in + // the milestone content. If the ROADMAP mixes headings + bullets (hybrid + // format), a bullet-only match means the detail section is missing — that + // is the #3098 case and must continue to produce the "missing a detail + // section" error. Only a purely bullet-style ROADMAP (zero heading-style + // phase entries in the milestone) goes through the bullet insert path. + const bulletPattern = new RegExp( + `-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+0*${afterPhaseEscaped}[:\\s]`, + 'i', + ); + const anyHeadingPattern = /#{2,4}\s*Phase\s+\d/i; + const roadmapHasHeadingPhases = anyHeadingPattern.test(content); + const isBulletStyle = !headingMatch && bulletPattern.test(content) && !roadmapHasHeadingPhases; + + if (!headingMatch && !isBulletStyle) { + // Bug #3098 parity: when the ROADMAP uses heading-style phases and only + // the summary checklist exists for this phase (no `### Phase N:` detail + // section), point the user at the missing detail section rather than + // implying the phase is absent. const checklistPattern = new RegExp( - `-\\s*\\[[ x]\\]\\s*\\*\\*Phase\\s+0*${afterPhaseEscaped}:`, + `-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+0*${afterPhaseEscaped}[:\\s]`, 'i', ); if (checklistPattern.test(content)) { @@ -459,6 +481,46 @@ export const phaseInsert: QueryHandler = async (args, projectDir, workstream) => // Create directory with .gitkeep await ensureDirectoryWithGitkeep(dirPath); + if (isBulletStyle) { + // #3815: Insert in checked-bullet format, mirroring the style of the + // surrounding entries. Detect whether the matched bullet uses bold + // (`**Phase N: …**`) to preserve file-internal format consistency. + const boldBulletPattern = new RegExp( + `-\\s*\\[[ x]\\]\\s*\\*\\*Phase\\s+0*${afterPhaseEscaped}:`, + 'i', + ); + const useBold = boldBulletPattern.test(content); + const phaseLabel = useBold + ? `**Phase ${decimalPhase}: ${description}**` + : `Phase ${decimalPhase}: ${description}`; + const bulletEntry = `\n- [ ] ${phaseLabel}`; + + // Locate the target bullet line in the raw content + const targetBulletPattern = new RegExp( + `(-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+0*${afterPhaseEscaped}[:\\s][^\\n]*)`, + 'i', + ); + const bulletMatchResult = rawContent.match(targetBulletPattern); + if (!bulletMatchResult) { + throw new GSDError(`Could not find Phase ${afterPhase} bullet line`, ErrorClassification.Execution); + } + + const bulletLineEnd = rawContent.indexOf(bulletMatchResult[0]) + bulletMatchResult[0].length; + // Find where the next phase bullet starts (or use end of content) + const afterBullet = rawContent.slice(bulletLineEnd); + const nextBulletMatch = afterBullet.match(/\n-\s*\[[ x]\]\s*(?:\*\*)?Phase\s+\d/i); + + let insertIdx: number; + if (nextBulletMatch && nextBulletMatch.index !== undefined) { + insertIdx = bulletLineEnd + nextBulletMatch.index; + } else { + insertIdx = bulletLineEnd; + } + + return rawContent.slice(0, insertIdx) + bulletEntry + rawContent.slice(insertIdx); + } + + // Heading-style insert (original path) // Build phase entry const phaseEntry = `\n### Phase ${decimalPhase}: ${description} (INSERTED)\n\n**Goal:** [Urgent work - to be planned]\n**Requirements**: TBD\n**Depends on:** Phase ${afterPhase}\n**Plans:** 0 plans\n\nPlans:\n- [ ] TBD (run /gsd-plan-phase ${decimalPhase} to break down)\n`; diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 7fe68898a..d2780b05e 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -1357,13 +1357,20 @@ describe('phase insert command', () => { }); test('reports actionable error for summary-only placeholder phase without detail section (#3098)', () => { + // #3098: a hybrid ROADMAP that has heading-style phases for some phases + // but only a bullet summary entry for phase 5 (the detail section is + // missing). Insert must fail with "missing a detail section" rather than + // silently inserting in bullet-style — because the surrounding ROADMAP + // uses headings, so the absent `### Phase 5:` is a genuine omission. + // (Compare with the #3815 case below: a purely bullet-style ROADMAP that + // has NO heading-style phases at all is valid and insert should succeed.) fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'), - `# Roadmap\n\n- [ ] **Phase 5: Placeholder**\n` + `# Roadmap\n\n### Phase 4: Foundation\n**Goal:** Setup\n\n- [ ] **Phase 5: Placeholder**\n` ); const result = runGsdTools('phase insert 5 Hotfix', tmpDir); - assert.ok(!result.success, 'should fail when phase is summary-only placeholder'); + assert.ok(!result.success, 'should fail when phase is summary-only placeholder in a heading-style ROADMAP'); assert.ok(result.error.includes('missing a detail section')); });