fix(#2619): prevent extractCurrentMilestone from truncating on phase-vX.Y headings (#2624)

* fix(#2619): prevent extractCurrentMilestone from truncating on phase-vX.Y headings

extractCurrentMilestone sliced ROADMAP.md to the current milestone by
looking for the next milestone heading with a greedy regex:

    ^#{1,N}\s+(?:.*v\d+\.\d+||📋|🚧)

Any heading that mentioned a version literal matched — including phase
headings like "### Phase 12: v1.0 Tech-Debt Closure". When the current
milestone was at the same heading level as the phases (### 🚧 v1.1 …),
the slice terminated at the first such phase, hiding every phase that
followed from phase.insert, validate.health W007, and other SDK commands.

Fix: add a `(?!Phase\s+\S)` negative lookahead so phase headings can
never be treated as milestone boundaries. Phase headings always start
with the literal `Phase `, so this is a clean exclusion.

Applied to:
- get-shit-done/bin/lib/core.cjs (extractCurrentMilestone)
- sdk/src/query/roadmap.ts (extractCurrentMilestone + extractNextMilestoneSection)

Regression tests:
- tests/roadmap-phase-fallback.test.cjs: extractCurrentMilestone does not
  truncate on phase heading containing vX.Y (#2619)
- sdk/src/query/roadmap.test.ts: extractCurrentMilestone bug-2619: does
  not truncate at a phase heading containing vX.Y

Closes #2619

* fix(#2619): make milestone-boundary Phase lookahead case-insensitive

CodeRabbit follow-up on #2619: the negative lookahead `(?!Phase\s+\S)`
in the SDK milestone-boundary regex was case-sensitive, so headings like
`### PHASE 12: v1.0 Tech-Debt` or `### phase 12: …` still truncated the
milestone slice. Add the `i` flag (now `gmi`).

The sibling CJS regex in get-shit-done/bin/lib/core.cjs already uses the
`mi` flag, so it is already case-insensitive; added a regression test to
lock that in.

- sdk/src/query/roadmap.ts: change flags from `gm` → `gmi`
- sdk/src/query/roadmap.test.ts: add PHASE/phase regression test
- tests/roadmap-phase-fallback.test.cjs: add PHASE/phase regression test

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-04-23 11:53:20 -04:00
committed by GitHub
parent 0a049149e1
commit 74da61fb4a
4 changed files with 143 additions and 4 deletions

View File

@@ -1309,8 +1309,11 @@ function extractCurrentMilestone(content, cwd) {
// Milestone headings look like: ## v2.0, ## Roadmap v2.0, ## ✅ v1.0, etc.
const headingLevel = sectionMatch[1].match(/^(#{1,3})\s/)[1].length;
const restContent = content.slice(sectionStart + sectionMatch[0].length);
// Exclude phase headings (e.g. "### Phase 12: v1.0 Tech-Debt Closure") from
// being treated as milestone boundaries just because they mention vX.Y in
// the title. Phase headings always start with the literal `Phase `. See #2619.
const nextMilestonePattern = new RegExp(
`^#{1,${headingLevel}}\\s+(?:.*v\\d+\\.\\d+|✅|📋|🚧)`,
`^#{1,${headingLevel}}\\s+(?!Phase\\s+\\S)(?:.*v\\d+\\.\\d+|✅|📋|🚧)`,
'mi'
);
const nextMatch = restContent.match(nextMilestonePattern);

View File

@@ -326,6 +326,69 @@ describe('extractCurrentMilestone', () => {
expect(result).toContain('Phase 100');
});
// ─── Bug #2619: phase heading containing vX.Y triggers truncation ─────
it('bug-2619: does not truncate at a phase heading containing vX.Y', async () => {
// A phase title like "Phase 12: v1.0 Tech-Debt Closure" was being treated
// as a milestone boundary because the greedy `.*v(\d+(?:\.\d+)+)` branch
// in nextMilestoneRegex matched any heading with a version literal.
const roadmapWithPhaseVersion = `# ROADMAP
## Phases
### 🚧 v1.1 Launch-Ready (In Progress)
### Phase 11: Structured Logging
**Goal**: Add structured logging
### Phase 12: v1.0 Tech-Debt Closure
**Goal**: Close out v1.0 debt
### Phase 19: Security Audit
**Goal**: Full security audit
`;
const state = `---\nmilestone: v1.1\n---\n# State\n`;
await writeFile(join(tmpDir, '.planning', 'STATE.md'), state);
await writeFile(join(tmpDir, '.planning', 'ROADMAP.md'), roadmapWithPhaseVersion);
const result = await extractCurrentMilestone(roadmapWithPhaseVersion, tmpDir);
// Phase 12 and Phase 19 must both survive — the slice cannot be truncated
// at "### Phase 12: v1.0 Tech-Debt Closure".
expect(result).toContain('### Phase 12: v1.0 Tech-Debt Closure');
expect(result).toContain('### Phase 19: Security Audit');
});
// ─── Bug #2619 (CodeRabbit follow-up): case-insensitive Phase lookahead ───
it('bug-2619: does not truncate at PHASE/phase heading containing vX.Y (case-insensitive)', async () => {
// The negative lookahead `(?!Phase\s+\S)` must be case-insensitive so that
// headings like "### PHASE 12: v1.0 Tech-Debt" or "### phase 12: v1.0 …"
// are also excluded from milestone-boundary matching.
const roadmapMixedCase = `# ROADMAP
## Phases
### 🚧 v1.1 Launch-Ready (In Progress)
### PHASE 11: Structured Logging
**Goal**: Add structured logging
### phase 12: v1.0 Tech-Debt Closure
**Goal**: Close out v1.0 debt
### Phase 19: Security Audit
**Goal**: Full security audit
`;
const state = `---\nmilestone: v1.1\n---\n# State\n`;
await writeFile(join(tmpDir, '.planning', 'STATE.md'), state);
await writeFile(join(tmpDir, '.planning', 'ROADMAP.md'), roadmapMixedCase);
const result = await extractCurrentMilestone(roadmapMixedCase, tmpDir);
expect(result).toContain('### PHASE 11: Structured Logging');
expect(result).toContain('### phase 12: v1.0 Tech-Debt Closure');
expect(result).toContain('### Phase 19: Security Audit');
});
// ─── 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

View File

@@ -195,9 +195,14 @@ export async function extractCurrentMilestone(content: string, projectDir: strin
const currentVersionMatch = version ? version.match(/v(\d+(?:\.\d+)+)/i) : null;
const currentVersionStr = currentVersionMatch ? currentVersionMatch[1] : '';
// Exclude phase headings (e.g. "### Phase 12: v1.0 Tech-Debt Closure") from
// being treated as milestone boundaries just because they mention vX.Y in
// the title. Phase headings always start with the literal `Phase `. See #2619.
const nextMilestoneRegex = new RegExp(
`^#{1,${headingLevel}}\\s+(?:.*v(\\d+(?:\\.\\d+)+)[^\\n]*|.*(?:✅|📋|🚧|🟡))`,
'gm'
`^#{1,${headingLevel}}\\s+(?!Phase\\s+\\S)(?:.*v(\\d+(?:\\.\\d+)+)[^\\n]*|.*(?:✅|📋|🚧|🟡))`,
// `i` flag ensures the `(?!Phase\s+\S)` lookahead matches PHASE/phase too
// (CodeRabbit follow-up on #2619).
'gmi'
);
let sectionEnd = content.length;
@@ -303,7 +308,8 @@ export async function extractNextMilestoneSection(
// Look for the next ## milestone heading after the current one.
const tail = cleaned.slice(currentMatch.index + currentMatch[0].length);
const nextMilestonePattern = /^##\s+([^\n]*(?:v(\d+(?:\.\d+)+)|✅|🚧|🟡|📋)[^\n]*)$/gim;
// Exclude phase headings — see #2619.
const nextMilestonePattern = /^##\s+(?!Phase\s+\S)([^\n]*(?:v(\d+(?:\.\d+)+)|✅|🚧|🟡|📋)[^\n]*)$/gim;
let nextMatch: RegExpExecArray | null;
while ((nextMatch = nextMilestonePattern.exec(tail)) !== null) {
const heading = nextMatch[1];

View File

@@ -207,6 +207,73 @@ describe('roadmap get-phase fallback to full ROADMAP.md (#1634)', () => {
assert.equal(output.goal, 'Remove deprecated modules');
});
test('extractCurrentMilestone does not truncate on phase heading containing vX.Y (#2619)', () => {
// Regression: phase heading like "### Phase 12: v1.0 Tech-Debt Closure"
// was incorrectly treated as a milestone boundary because the greedy
// `.*v\d+\.\d+` subpattern in nextMilestonePattern matched it.
const core = require('../get-shit-done/bin/lib/core.cjs');
writeState(tmpDir, 'v1.1');
const roadmap = `# Roadmap
## Phases
### 🚧 v1.1 Launch-Ready (In Progress)
### Phase 11: Structured Logging
**Goal:** Add structured logging
### Phase 12: v1.0 Tech-Debt Closure
**Goal:** Close out v1.0 debt
### Phase 19: Security Audit
**Goal:** Full security audit
`;
const slice = core.extractCurrentMilestone(roadmap, tmpDir);
assert.ok(
slice.includes('### Phase 12: v1.0 Tech-Debt Closure'),
'slice must include Phase 12 (it lives inside the active milestone)'
);
assert.ok(
slice.includes('### Phase 19: Security Audit'),
'slice must include Phase 19 (truncation at Phase 12 would hide it)'
);
});
test('extractCurrentMilestone handles PHASE/phase (case-insensitive) containing vX.Y (#2619 follow-up)', () => {
// CodeRabbit follow-up: the negative lookahead `(?!Phase\s+\S)` must be
// case-insensitive so PHASE/phase variants are also excluded.
const core = require('../get-shit-done/bin/lib/core.cjs');
writeState(tmpDir, 'v1.1');
const roadmap = `# Roadmap
## Phases
### 🚧 v1.1 Launch-Ready (In Progress)
### PHASE 11: Structured Logging
**Goal:** Add structured logging
### phase 12: v1.0 Tech-Debt Closure
**Goal:** Close out v1.0 debt
### Phase 19: Security Audit
**Goal:** Full security audit
`;
const slice = core.extractCurrentMilestone(roadmap, tmpDir);
assert.ok(
slice.includes('### PHASE 11: Structured Logging'),
'slice must include PHASE 11 (uppercase)'
);
assert.ok(
slice.includes('### phase 12: v1.0 Tech-Debt Closure'),
'slice must include phase 12 (lowercase with vX.Y)'
);
assert.ok(
slice.includes('### Phase 19: Security Audit'),
'slice must include Phase 19 (truncation at phase 12 would hide it)'
);
});
test('section extraction from fallback includes correct content boundaries', () => {
writeState(tmpDir, 'v1.0');
fs.writeFileSync(