* fix(#2200): scope phase-complete roadmap writes to the current milestone runPhaseCompleteTransaction's roadmap mutators ran unanchored / un-milestone- scoped / first-match over the whole ROADMAP: the phase-checkbox flip could check a bullet inside a backticked prose literal or an earlier Backlog entry instead of the closing phase's bullet (a transposition), and the Plans-count writer could bind to a same-numbered phase in a shipped milestone. - Add currentMilestoneRawRanges(content, cwd) to roadmap-parser.cts: raw [start,end) offsets of the active milestone's region(s) (primary section + the optional Phase Details section), mirroring extractCurrentMilestone's selection. - Line-anchor the phase checkbox pattern (^ + m flag) so an inline / backticked prose literal cannot match. - Apply the phase-checkbox flip, the Plans-count write, and the per-plan checkbox flips ONLY within the current milestone window(s) via a mutateMilestonePhase helper (splice later windows first so offsets stay stable). Fall back to whole- content mutation when there is no versioned active milestone (prior behaviour). The Progress-table writer stays as-is (already scoped to ## Progress, #2012). Closes #2200 * docs(#2200): add changeset fragment * docs(#2200): backfill PR number in changeset fragment
This commit is contained in:
Tom Boucher
5
.changeset/proud-ravens-jump.md
Normal file
5
.changeset/proud-ravens-jump.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 2229
|
||||
---
|
||||
**`phase complete` no longer checks the wrong ROADMAP checkbox or writes the plan count into a shipped milestone** — the roadmap mutators ran unanchored and un-milestone-scoped, so they could flip a bullet inside a backticked prose literal or a Backlog entry instead of the closing phase's, and write the plan count into a same-numbered phase in a shipped milestone. The checkbox flip is now line-anchored and both writers are scoped to the current milestone. (#2200)
|
||||
@@ -44,7 +44,7 @@ import phaseLocatorMod = require('./phase-locator.cjs');
|
||||
const { findPhaseInternal, getArchivedPhaseDirs } = phaseLocatorMod;
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports -- roadmap-parser.cjs is an export= CommonJS module
|
||||
import roadmapParserMod = require('./roadmap-parser.cjs');
|
||||
const { stripShippedMilestones, extractCurrentMilestone, getMilestonePhaseFilter } = roadmapParserMod;
|
||||
const { stripShippedMilestones, extractCurrentMilestone, getMilestonePhaseFilter, currentMilestoneRawRanges } = roadmapParserMod;
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module
|
||||
import planningWorkspace = require('./planning-workspace.cjs');
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module
|
||||
@@ -1475,13 +1475,13 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
// so completing an already-checked phase (idempotent re-run) checked the
|
||||
// wrong phase's box. Mirrors the tight pattern used by phase-insert
|
||||
// (`]\\s*(?:\\*\\*)?Phase`).
|
||||
// #2067/#2200: line-anchored (^ + m flag, optional leading indent) so an
|
||||
// inline / backticked prose literal cannot match. Milestone-scoped below
|
||||
// (mutateMilestonePhase) so a Backlog entry or a same-numbered shipped-
|
||||
// milestone phase cannot be flipped either.
|
||||
const checkboxPattern = new RegExp(
|
||||
`(-\\s*\\[)[ ](\\]\\s*(?:\\*\\*)?\\s*Phase\\s+${phaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*)`,
|
||||
'i',
|
||||
);
|
||||
roadmapContent = roadmapContent.replace(
|
||||
checkboxPattern,
|
||||
`$1x$2 (completed ${today})`,
|
||||
`^[ \\t]*(-\\s*\\[)[ ](\\]\\s*(?:\\*\\*)?\\s*Phase\\s+${phaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*)`,
|
||||
'im',
|
||||
);
|
||||
|
||||
const tableRowPattern = new RegExp(
|
||||
@@ -1522,21 +1522,47 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
`(#{2,4}\\s*Phase\\s+${phaseEscaped}(?:(?!\\n#{1,4}\\s)[\\s\\S])*?\\*\\*Plans:\\*\\*\\s*)[^\\n]+`,
|
||||
'i',
|
||||
);
|
||||
roadmapContent = roadmapContent.replace(
|
||||
planCountPattern,
|
||||
`$1${summaryCount}/${planCount} plans complete`,
|
||||
);
|
||||
|
||||
const phaseInfoSummaries = phaseInfo['summaries'] as string[];
|
||||
for (const summaryFile of phaseInfoSummaries) {
|
||||
const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', '');
|
||||
if (!planId) continue;
|
||||
const planEscaped = escapeRegex(planId);
|
||||
const planCheckboxPattern = new RegExp(
|
||||
`(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`,
|
||||
'i',
|
||||
);
|
||||
roadmapContent = (roadmapContent).replace(planCheckboxPattern, '$1x$2');
|
||||
|
||||
// #2200: apply the phase-checkbox flip, the plan-count write, and the
|
||||
// per-plan checkbox flips ONLY within the current milestone's region(s)
|
||||
// (primary section + optional Phase Details section). A bullet/heading in
|
||||
// a shipped milestone, a Backlog section, or a backticked prose literal is
|
||||
// outside the window and stays untouched. With no versioned active
|
||||
// milestone, fall back to whole-content mutation (prior behaviour).
|
||||
const mutateMilestonePhase = (slice: string): string => {
|
||||
let s = slice;
|
||||
s = s.replace(checkboxPattern, `$1x$2 (completed ${today})`);
|
||||
s = s.replace(planCountPattern, `$1${summaryCount}/${planCount} plans complete`);
|
||||
for (const summaryFile of phaseInfoSummaries) {
|
||||
const planId = summaryFile.replace('-SUMMARY.md', '').replace('SUMMARY.md', '');
|
||||
if (!planId) continue;
|
||||
const planEscaped = escapeRegex(planId);
|
||||
const planCheckboxPattern = new RegExp(
|
||||
`(-\\s*\\[) (\\]\\s*(?:\\*\\*)?${planEscaped}(?:\\*\\*)?)`,
|
||||
'i',
|
||||
);
|
||||
s = s.replace(planCheckboxPattern, '$1x$2');
|
||||
}
|
||||
return s;
|
||||
};
|
||||
|
||||
const milestoneRanges = currentMilestoneRawRanges(roadmapContent, cwd);
|
||||
if (milestoneRanges) {
|
||||
// Splice later windows first so an earlier window's offsets are not
|
||||
// shifted by a length-changing mutation in a later window.
|
||||
const windows = [milestoneRanges.details, milestoneRanges.primary]
|
||||
.filter((w): w is { start: number; end: number } => w !== null)
|
||||
.sort((a, b) => b.start - a.start);
|
||||
for (const w of windows) {
|
||||
roadmapContent =
|
||||
roadmapContent.slice(0, w.start)
|
||||
+ mutateMilestonePhase(roadmapContent.slice(w.start, w.end))
|
||||
+ roadmapContent.slice(w.end);
|
||||
}
|
||||
} else {
|
||||
roadmapContent = mutateMilestonePhase(roadmapContent);
|
||||
}
|
||||
|
||||
writes.push({
|
||||
|
||||
@@ -556,6 +556,92 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p
|
||||
return isDirInMilestone as MilestonePhaseFilter;
|
||||
}
|
||||
|
||||
/**
|
||||
* #2200: raw [start,end) offsets of the current milestone's region(s) in ROADMAP
|
||||
* content, for scoping write-path mutations (phase-checkbox flip, Plans-count
|
||||
* writer) so they cannot touch a backticked prose literal, a Backlog entry, or a
|
||||
* same-numbered phase in a shipped milestone.
|
||||
*
|
||||
* Mirrors the region selection in `extractCurrentMilestone` (version detection →
|
||||
* active heading → next milestone boundary → optional Phase Details section).
|
||||
* Returns null when there is no versioned active milestone; callers then fall
|
||||
* back to whole-content mutation (the prior behaviour).
|
||||
*
|
||||
* NOTE: keep the region logic here in sync with extractCurrentMilestone.
|
||||
*/
|
||||
function currentMilestoneRawRanges(
|
||||
content: string,
|
||||
cwd?: string,
|
||||
): { primary: { start: number; end: number }; details: { start: number; end: number } | null } | null {
|
||||
if (!cwd) return null;
|
||||
|
||||
let version: string | null = null;
|
||||
try {
|
||||
const statePath = path.join(planningDir(cwd), 'STATE.md');
|
||||
const stateRaw = platformReadSync(statePath);
|
||||
if (stateRaw !== null) {
|
||||
const milestoneMatch = stateRaw.match(/^milestone:\s*(.+)/m);
|
||||
if (milestoneMatch) version = milestoneMatch[1].trim();
|
||||
}
|
||||
} catch { /* ignore */ }
|
||||
if (!version) {
|
||||
const inProgressMatch = content.match(/(?:🚧|🔄)\s*\*\*v(\d+\.\d+)\s/);
|
||||
if (inProgressMatch) version = 'v' + inProgressMatch[1];
|
||||
}
|
||||
if (!version) return null;
|
||||
|
||||
const escapedVersion = escapeRegex(version);
|
||||
const sectionPattern = new RegExp(
|
||||
`(^#{1,3}\\s+(?!Phase\\s+\\S).*${escapedVersion}\\b[^\\n]*)`,
|
||||
'gmi',
|
||||
);
|
||||
const headingMatches = [...content.matchAll(sectionPattern)];
|
||||
if (headingMatches.length === 0) return null;
|
||||
|
||||
const closedMarkerPattern = /\b(?:CLOSED|ARCHIVED|ABANDONED|SHIPPED|FAILED)\b|✅|🗄/i;
|
||||
const activeMarkerPattern = /\b(?:STARTED|ACTIVE|WIP)\b|in\s+progress|🚧|🔄/i;
|
||||
const isClosed = (h: string) => closedMarkerPattern.test(h) && !activeMarkerPattern.test(h);
|
||||
const firstMatch = headingMatches[0];
|
||||
const selected = headingMatches.find((m) => !isClosed(m[1])) || firstMatch;
|
||||
const sectionStart = selected.index ?? 0;
|
||||
|
||||
const computeSectionEnd = (headingText: string, headingStart: number): number => {
|
||||
const level = (headingText.match(/^(#{1,3})\s/) ?? ['', '#'])[1].length;
|
||||
const afterHeading = headingStart + headingText.length;
|
||||
for (const h of tokenizeHeadings(content)) {
|
||||
if (h.offset <= headingStart) continue;
|
||||
if (h.offset < afterHeading) continue;
|
||||
if (h.level > level) continue;
|
||||
if (/^Phase\s+\S/i.test(h.text)) continue;
|
||||
if (!/v\d+\.\d+|✅|📋|🚧/i.test(h.text)) continue;
|
||||
return h.offset;
|
||||
}
|
||||
return content.length;
|
||||
};
|
||||
const sectionEnd = computeSectionEnd(selected[0], sectionStart);
|
||||
|
||||
const selectedVersionToken = selected[1].match(
|
||||
/v\d+(?:\.\d+)+(?:[-.][A-Za-z0-9]+)*/i,
|
||||
)?.[0];
|
||||
const detailsVersionBoundary = selectedVersionToken
|
||||
? new RegExp(`${escapeRegex(selectedVersionToken)}(?![\\w.-])`, 'i')
|
||||
: null;
|
||||
const detailsMatch = headingMatches.find(
|
||||
(m) =>
|
||||
/\(Phase\s+Details\)/i.test(m[1]) &&
|
||||
!isClosed(m[1]) &&
|
||||
(!detailsVersionBoundary || detailsVersionBoundary.test(m[1])) &&
|
||||
(m.index ?? 0) >= sectionEnd,
|
||||
);
|
||||
let details: { start: number; end: number } | null = null;
|
||||
if (detailsMatch) {
|
||||
const detailsStart = detailsMatch.index ?? 0;
|
||||
details = { start: detailsStart, end: computeSectionEnd(detailsMatch[0], detailsStart) };
|
||||
}
|
||||
|
||||
return { primary: { start: sectionStart, end: sectionEnd }, details };
|
||||
}
|
||||
|
||||
export = {
|
||||
stripShippedMilestones,
|
||||
extractCurrentMilestone,
|
||||
@@ -563,4 +649,5 @@ export = {
|
||||
getRoadmapPhaseInternal,
|
||||
getMilestoneInfo,
|
||||
getMilestonePhaseFilter,
|
||||
currentMilestoneRawRanges,
|
||||
};
|
||||
|
||||
@@ -1965,6 +1965,59 @@ describe('feat-3594: roadmap parser does not crash on ANY corpus fixture', () =>
|
||||
});
|
||||
}
|
||||
|
||||
// ─── #2200: currentMilestoneRawRanges scopes phase-complete writes ────────────
|
||||
// The phase-complete roadmap mutators (checkbox-flip + Plans writer) must mutate
|
||||
// only within the active milestone so they cannot touch a backticked prose
|
||||
// literal, a Backlog entry, or a same-numbered phase in a shipped milestone.
|
||||
// This tests the scoping helper the fix rests on (the mutators' command path is
|
||||
// covered by the existing phase-complete suite; gsd-test confirms no regression).
|
||||
{
|
||||
const { describe: d3, test: t3 } = require('node:test');
|
||||
const a3 = require('node:assert/strict');
|
||||
const fs3 = require('node:fs');
|
||||
const path3 = require('node:path');
|
||||
const { createTempProject: ctp3, cleanup: cu3 } = require('./helpers.cjs');
|
||||
const rp3 = require('../gsd-core/bin/lib/roadmap-parser.cjs');
|
||||
|
||||
d3('#2200 currentMilestoneRawRanges — scopes writes to the active milestone', () => {
|
||||
t3('the active window contains the active phase bullet, excludes Backlog + prose + shipped', () => {
|
||||
const tmpDir = ctp3('fix-2200-');
|
||||
try {
|
||||
// Shipped milestone (in a <details> block) + Backlog + a backticked prose
|
||||
// literal all come BEFORE the active milestone — the typical layout.
|
||||
const roadmap = [
|
||||
'# Roadmap', '',
|
||||
'## Backlog', '- [ ] **Phase 1: Some Future Idea**', '',
|
||||
'> See `- [ ] **Phase 1: Alpha**` in the active milestone.', '',
|
||||
'<details><summary>✅ v0.9 Old</summary>',
|
||||
'- [x] **Phase 1: Legacy**',
|
||||
'### Phase 1: Legacy',
|
||||
'**Plans:** 9/9 plans complete',
|
||||
'</details>', '',
|
||||
'## v1.0 — Active', '',
|
||||
'- [ ] **Phase 1: Alpha**', '',
|
||||
'### Phase 1: Alpha',
|
||||
'**Plans:** 0/1 plans complete', '',
|
||||
].join('\n');
|
||||
fs3.writeFileSync(path3.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap);
|
||||
fs3.writeFileSync(path3.join(tmpDir, '.planning', 'STATE.md'), '---\nmilestone: v1.0\ncurrent_phase: 1\n---\n');
|
||||
const ranges = rp3.currentMilestoneRawRanges(roadmap, tmpDir);
|
||||
a3.ok(ranges, 'a versioned active milestone must yield ranges');
|
||||
const primary = roadmap.slice(ranges.primary.start, ranges.primary.end);
|
||||
a3.ok(primary.includes('- [ ] **Phase 1: Alpha**'), 'active phase bullet is inside the window');
|
||||
a3.ok(!primary.includes('Some Future Idea'), 'a Backlog entry is outside the active window');
|
||||
a3.ok(!primary.includes('See `- [ ]'), 'a backticked prose literal is outside the active window');
|
||||
a3.ok(!primary.includes('9/9 plans complete'), 'a shipped milestone plan-count line is outside the active window');
|
||||
} finally {
|
||||
cu3(tmpDir);
|
||||
}
|
||||
});
|
||||
|
||||
t3('returns null without a versioned active milestone (whole-content fallback)', () => {
|
||||
a3.strictEqual(rp3.currentMilestoneRawRanges('# Roadmap\n- [ ] **Phase 1: X**\n', undefined), null);
|
||||
});
|
||||
});
|
||||
}
|
||||
// ─── #2199: bullet/em-dash ROADMAP phase resolution ───────────────────────────
|
||||
// Self-contained block: phase lookup + milestone filter must accept bullet/
|
||||
// checkbox entries with an em-dash/en-dash/hyphen/colon separator, not just the
|
||||
|
||||
Reference in New Issue
Block a user