From 59cfbbba6af1be24d77c4e2a4397f6f67ba03457 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 20 Apr 2026 10:08:14 -0400 Subject: [PATCH] fix(sdk): extractCurrentMilestone Backlog leak + state.begin-phase flag parsing (#2455) * fix(sdk): extractCurrentMilestone Backlog leak + state.begin-phase flag parsing Closes #2422 Closes #2420 Co-Authored-By: Claude Sonnet 4.6 * fix: patch-version semver in milestone boundary regex + flag-parser validation Two follow-on correctness issues identified in code review: 1. roadmap.ts: currentVersionMatch and nextMilestoneRegex only captured major.minor (v(\d+\.\d+)), collapsing v2.0.1 to "2.0". A sub-heading "## v2.0.2 Phase Details" would match the same prefix and be incorrectly skipped. Both patterns updated to v(\d+(?:\.\d+)+) to capture full semver. 2. state-mutation.ts: pair-wise flag parsing loop advanced i by 2 unconditionally, so a missing flag value caused the next flag token to be assigned as the value (e.g. flags['phase'] = '--name'). Fix: iterate with i++ and validate that the candidate value exists and does not start with '--' before assigning; throw GSDError('missing value for --') on invalid input. Added regression test. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- sdk/src/query/roadmap.test.ts | 51 ++++++++++++++++++++++++++++ sdk/src/query/roadmap.ts | 43 ++++++++++++----------- sdk/src/query/state-mutation.test.ts | 44 ++++++++++++++++++++++++ sdk/src/query/state-mutation.ts | 29 ++++++++++++++-- 4 files changed, 145 insertions(+), 22 deletions(-) diff --git a/sdk/src/query/roadmap.test.ts b/sdk/src/query/roadmap.test.ts index e98fed2cc..efa00bd6e 100644 --- a/sdk/src/query/roadmap.test.ts +++ b/sdk/src/query/roadmap.test.ts @@ -144,6 +144,57 @@ describe('extractCurrentMilestone', () => { const result = await extractCurrentMilestone(content, tmpDir); expect(result).toBe('current content'); }); + + // ─── Bug #2422: preamble Backlog leak ───────────────────────────────── + it('bug-2422: does not include ## Backlog section before the current milestone', async () => { + const roadmapWithBacklog = `# ROADMAP + +## Backlog +### Phase 999.1: Parking lot item A +### Phase 999.2: Parking lot item B + +### 🚧 v2.0 My Milestone (In Progress) +- [ ] **Phase 100: Real work** + +## v2.0 Phase Details +### Phase 100: Real work +**Goal**: Do stuff. +`; + const state = `---\nmilestone: v2.0\n---\n# State\n`; + await writeFile(join(tmpDir, '.planning', 'STATE.md'), state); + await writeFile(join(tmpDir, '.planning', 'ROADMAP.md'), roadmapWithBacklog); + + const result = await extractCurrentMilestone(roadmapWithBacklog, tmpDir); + + // Must NOT include backlog phases + expect(result).not.toContain('Phase 999.1'); + expect(result).not.toContain('Phase 999.2'); + expect(result).not.toContain('Parking lot'); + // Must include the actual v2.0 content + expect(result).toContain('Phase 100'); + }); + + // ─── Bug #2422: same-version sub-heading truncation ─────────────────── + it('bug-2422: does not truncate at same-version sub-heading (## v2.0 Phase Details)', async () => { + const roadmapWithDetails = `# ROADMAP + +### 🚧 v2.0 My Milestone (In Progress) +- [ ] **Phase 100: Real work** + +## v2.0 Phase Details +### Phase 100: Real work +**Goal**: Do stuff. +`; + const state = `---\nmilestone: v2.0\n---\n# State\n`; + await writeFile(join(tmpDir, '.planning', 'STATE.md'), state); + await writeFile(join(tmpDir, '.planning', 'ROADMAP.md'), roadmapWithDetails); + + const result = await extractCurrentMilestone(roadmapWithDetails, tmpDir); + + // The detail section must survive — not be cut off + expect(result).toContain('Phase 100'); + expect(result).toContain('Phase Details'); + }); }); // ─── roadmapGetPhase ────────────────────────────────────────────────────── diff --git a/sdk/src/query/roadmap.ts b/sdk/src/query/roadmap.ts index 15bd059c6..f0bf55fa3 100644 --- a/sdk/src/query/roadmap.ts +++ b/sdk/src/query/roadmap.ts @@ -110,7 +110,7 @@ export async function extractCurrentMilestone(content: string, projectDir: strin // Fallback: derive from ROADMAP in-progress marker if (!version) { - const inProgressMatch = content.match(/🚧\s*\*\*v(\d+\.\d+)\s/); + const inProgressMatch = content.match(/🚧\s*\*\*v(\d+(?:\.\d+)+)\s/); if (inProgressMatch) { version = 'v' + inProgressMatch[1]; } @@ -130,30 +130,35 @@ export async function extractCurrentMilestone(content: string, projectDir: strin const sectionStart = sectionMatch.index; - // Find end: next milestone heading at same or higher level, or EOF + // Find end: next milestone heading at same or higher level, or EOF. + // Skip headings that belong to the SAME version (e.g. "## v2.0 Phase Details"). const headingLevelMatch = sectionMatch[1].match(/^(#{1,3})\s/); const headingLevel = headingLevelMatch ? headingLevelMatch[1].length : 2; const restContent = content.slice(sectionStart + sectionMatch[0].length); - const nextMilestonePattern = new RegExp( - `^#{1,${headingLevel}}\\s+(?:.*v\\d+\\.\\d+|✅|📋|🚧)`, - 'mi' - ); - const nextMatch = restContent.match(nextMilestonePattern); - let sectionEnd: number; - if (nextMatch && nextMatch.index !== undefined) { - sectionEnd = sectionStart + sectionMatch[0].length + nextMatch.index; - } else { - sectionEnd = content.length; + // Extract current version so same-version sub-headings are not treated as boundaries. + // Capture full semver (major.minor.patch) so v2.0.1 is not collapsed to "2.0". + const currentVersionMatch = version ? version.match(/v(\d+(?:\.\d+)+)/i) : null; + const currentVersionStr = currentVersionMatch ? currentVersionMatch[1] : ''; + + const nextMilestoneRegex = new RegExp( + `^#{1,${headingLevel}}\\s+(?:.*v(\\d+(?:\\.\\d+)+)[^\\n]*|.*(?:✅|📋|🚧))`, + 'gm' + ); + + let sectionEnd = content.length; + let m: RegExpExecArray | null; + while ((m = nextMilestoneRegex.exec(restContent)) !== null) { + const matchedVersion = m[1]; + // Skip headings that reference the same version (e.g. "## v2.0 Phase Details"). + if (matchedVersion && currentVersionStr && matchedVersion === currentVersionStr) continue; + sectionEnd = sectionStart + sectionMatch[0].length + m.index; + break; } - const beforeMilestones = content.slice(0, sectionStart); - const currentSection = content.slice(sectionStart, sectionEnd); - - // Strip
from preamble - const preamble = beforeMilestones.replace(/
[\s\S]*?<\/details>/gi, ''); - - return preamble + currentSection; + // Return only the current milestone section — never include the preamble, which + // may contain ## Backlog and other non-current-milestone phases. + return content.slice(sectionStart, sectionEnd); } // ─── Internal helpers ───────────────────────────────────────────────────── diff --git a/sdk/src/query/state-mutation.test.ts b/sdk/src/query/state-mutation.test.ts index d0f17d237..ab37031aa 100644 --- a/sdk/src/query/state-mutation.test.ts +++ b/sdk/src/query/state-mutation.test.ts @@ -304,6 +304,50 @@ describe('stateBeginPhase', () => { expect(content).toContain('Executing Phase 11'); expect(content).toContain('State Mutations'); }); + + // ─── Bug #2420: flag-form args not parsed ──────────────────────────── + it('bug-2420: parses --phase/--name/--plans flag-form args correctly', async () => { + const { stateBeginPhase } = await import('./state-mutation.js'); + + // This is how execute-phase.md calls it: flag form + const result = await stateBeginPhase( + ['--phase', '99', '--name', 'probe-test', '--plans', '1'], + tmpDir + ); + const data = result.data as Record; + + // Must return the actual values, not the flag names + expect(data.phase).toBe('99'); + expect(data.name).toBe('probe-test'); + expect(data.plan_count).toBe('1'); + + // STATE.md must contain clean output, not literal "--phase" + const content = await readFile(join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + expect(content).not.toContain('--phase'); + expect(content).not.toContain('--name'); + expect(content).not.toContain('--plans'); + expect(content).toContain('Executing Phase 99'); + expect(content).toContain('probe-test'); + }); + + it('bug-2420: positional args still work after flag-parsing fix', async () => { + const { stateBeginPhase } = await import('./state-mutation.js'); + + const result = await stateBeginPhase(['42', 'Positional Test', '5'], tmpDir); + const data = result.data as Record; + expect(data.phase).toBe('42'); + expect(data.name).toBe('Positional Test'); + expect(data.plan_count).toBe('5'); + }); + + it('bug-2420: flag parser throws when a flag value is missing (next token is a flag)', async () => { + const { stateBeginPhase } = await import('./state-mutation.js'); + + // --phase has no value — next token is --name, which is itself a flag. + await expect( + stateBeginPhase(['--phase', '--name', 'Title', '--plans', '1'], tmpDir) + ).rejects.toThrow('missing value for --phase'); + }); }); // ─── stateAdvancePlan ─────────────────────────────────────────────────────── diff --git a/sdk/src/query/state-mutation.ts b/sdk/src/query/state-mutation.ts index 564c3a69c..c2647f2dd 100644 --- a/sdk/src/query/state-mutation.ts +++ b/sdk/src/query/state-mutation.ts @@ -331,9 +331,32 @@ export const statePatch: QueryHandler = async (args, projectDir) => { * @returns QueryResult with { phase, name, plan_count } */ export const stateBeginPhase: QueryHandler = async (args, projectDir) => { - const phaseNumber = args[0]; - const phaseName = args[1] || ''; - const planCount = args[2] || '?'; + // Accept both flag form (--phase 04 --name "Title" --plans 3) and positional form (04 "Title" 3). + let phaseNumber: string; + let phaseName: string; + let planCount: string; + + if (args.length > 0 && typeof args[0] === 'string' && args[0].startsWith('--')) { + const flags: Record = {}; + for (let i = 0; i < args.length; i++) { + const token = args[i]; + if (typeof token !== 'string' || !token.startsWith('--')) continue; + const key = token.slice(2); + const value = args[i + 1]; + if (value === undefined || (typeof value === 'string' && value.startsWith('--'))) { + throw new GSDError(`missing value for --${key}`, ErrorClassification.Validation); + } + flags[key] = value as string; + i += 1; + } + phaseNumber = flags['phase'] ?? ''; + phaseName = flags['name'] ?? ''; + planCount = flags['plans'] ?? flags['plan-count'] ?? '?'; + } else { + phaseNumber = args[0] ?? ''; + phaseName = args[1] || ''; + planCount = args[2] || '?'; + } if (!phaseNumber) { throw new GSDError('phase number required', ErrorClassification.Validation);