* fix(#3262): guard phase writes against milestone-scope headings * fix(#3262): fill changeset pr with 3446 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/graceful-moles-sing.md
Normal file
5
.changeset/graceful-moles-sing.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3446
|
||||
---
|
||||
Phase writes now guard the current milestone's scope. phase add/add-batch/insert reject a description containing a level 1-3 heading with a milestone marker (version token, status marker, or the word Milestone) before anything is written, and the edit-phase workflow captures roadmap milestone-scope (new read-only probe) around its in-place section write and rolls the edit back with an explicit error if the milestone window's scope or phase set changed.
|
||||
@@ -233,11 +233,34 @@ Wait for confirmation. If the user says `n`, exit without writing.
|
||||
</step>
|
||||
|
||||
<step name="write_updated_phase">
|
||||
Before writing, capture the current milestone scope (window scope + phase set) so the write can be verified against it:
|
||||
|
||||
```bash
|
||||
SCOPE_BEFORE=$(gsd_run query roadmap milestone-scope)
|
||||
```
|
||||
|
||||
Write the updated phase back in place in ROADMAP.md.
|
||||
|
||||
Read the full ROADMAP.md content, locate the phase section by its header (`## Phase {N}:` or `### Phase {N}:`), and replace exactly the old section text with the new section text. All content before and after the section (including other phases, milestone headers, and the summary checklist) must be left unchanged.
|
||||
|
||||
After writing ROADMAP.md, update STATE.md Roadmap Evolution:
|
||||
After writing, re-derive the milestone scope and compare it to the capture:
|
||||
|
||||
```bash
|
||||
SCOPE_AFTER=$(gsd_run query roadmap milestone-scope)
|
||||
```
|
||||
|
||||
If `scope` or `phases` differs from SCOPE_BEFORE, the replacement text introduced a heading that terminates the current milestone window (a level 1-3 heading carrying a version token, a ✅/📋/🚧/🔄 marker, or the word "Milestone"). Restore ROADMAP.md by replacing the new section text with the original section text, then report and stop — do NOT update STATE.md and do NOT retry the write:
|
||||
|
||||
```
|
||||
ERROR: edit rejected — milestone scope changed
|
||||
Phase set before: {SCOPE_BEFORE phases} / after: {SCOPE_AFTER phases}
|
||||
ROADMAP.md rolled back to the original section.
|
||||
The updated phase text must not introduce a milestone-scoping heading; rewrite the offending line as plain text or a list item.
|
||||
```
|
||||
|
||||
Exit.
|
||||
|
||||
If the milestone scope is unchanged, update STATE.md Roadmap Evolution:
|
||||
|
||||
```bash
|
||||
gsd_run query state.add-roadmap-evolution \
|
||||
@@ -277,6 +300,7 @@ Fields changed: {changed_field_list}
|
||||
- Don't edit in_progress/completed phases without --force
|
||||
- Don't use raw Write on ROADMAP.md without reading it first; always replace section in place
|
||||
- Don't modify the phase directory structure — only ROADMAP.md changes
|
||||
- Don't introduce a milestone-scoping heading (level 1-3 with a version token, ✅/📋/🚧/🔄 marker, or "Milestone") in field values — it terminates the current milestone window; the post-write scope check rolls the edit back
|
||||
- Don't commit the change — that's the user's decision
|
||||
</anti_patterns>
|
||||
|
||||
@@ -290,6 +314,7 @@ Edit-phase is complete when:
|
||||
- [ ] depends_on references validated; invalid references blocked
|
||||
- [ ] Diff shown and confirmed by user
|
||||
- [ ] Updated phase written back in place; number, position, and status preserved
|
||||
- [ ] Milestone scope verified unchanged after the write (rollback + error on mismatch)
|
||||
- [ ] STATE.md Roadmap Evolution updated
|
||||
- [ ] User informed of next steps
|
||||
</success_criteria>
|
||||
|
||||
@@ -117,8 +117,10 @@
|
||||
* `--include-archived` merge are phase LOCATION and archive
|
||||
* enumeration, not current-milestone enumeration; both legitimately
|
||||
* read the physical set. Its ENUMERATION path routes through the owner.
|
||||
* - `src/roadmap-parser.cts` `getMilestonePhaseFilter`: its two heading/
|
||||
* bullet scans that seed `milestonePhaseNums` deliberately use the local
|
||||
* - `src/roadmap-parser.cts` `getMilestonePhaseFilter` and its #3262-extracted
|
||||
* set-building owner `scanMilestonePhaseIds` (the same two heading/
|
||||
* bullet scans, lifted verbatim so the `roadmap milestone-scope` probe
|
||||
* reads the identical derivation): both deliberately use the local
|
||||
* `999`-only literal, NOT `isSentinelPhaseId`. That canonical predicate
|
||||
* additionally treats a leading `0` as sentinel milestone 0 (via its
|
||||
* `/^0*(\d+)/` backtrack), which would swallow #2554's decimal phase ids
|
||||
@@ -279,7 +281,7 @@ const FUNCTION_SCOPED_EXEMPTIONS = new Map([
|
||||
[path.join('src', 'state.cts'), new Set(['cmdStateValidate', 'cmdStateSync', 'cmdStateRebuild'])],
|
||||
[path.join('src', 'roadmap-upgrade.cts'), new Set(['computeMigrationPlan'])],
|
||||
[path.join('src', 'smart-entry.cts'), new Set(['detectVerifyFailed'])],
|
||||
[path.join('src', 'roadmap-parser.cts'), new Set(['getMilestonePhaseFilter'])],
|
||||
[path.join('src', 'roadmap-parser.cts'), new Set(['getMilestonePhaseFilter', 'scanMilestonePhaseIds'])],
|
||||
[path.join('src', 'planning-snapshot.cts'), new Set(['buildAllPhaseDirNamesField'])],
|
||||
]);
|
||||
|
||||
|
||||
@@ -628,6 +628,14 @@ export const ROADMAP_COMMAND_ALIASES: CommandAlias[] = [
|
||||
"subcommand": "analyze",
|
||||
"mutation": false
|
||||
},
|
||||
{
|
||||
"canonical": "roadmap.milestone-scope",
|
||||
"aliases": [
|
||||
"roadmap milestone-scope"
|
||||
],
|
||||
"subcommand": "milestone-scope",
|
||||
"mutation": false
|
||||
},
|
||||
{
|
||||
"canonical": "roadmap.get-phase",
|
||||
"aliases": [
|
||||
|
||||
@@ -53,7 +53,7 @@ import phaseLocatorMod = require('./phase-locator.cjs');
|
||||
const { findPhaseInternal, getArchivedPhaseDirs, listMilestonePhaseDirs } = 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, currentMilestoneRawRanges, withPhaseSection } = roadmapParserMod;
|
||||
const { stripShippedMilestones, extractCurrentMilestone, currentMilestoneRawRanges, withPhaseSection, findMilestoneScopeHeadingLines } = 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
|
||||
@@ -1030,10 +1030,36 @@ function phaseEntryInsertOffset(rawContent: string, cwd: string): number {
|
||||
return lastSeparator > 0 ? ranges.primary.start + lastSeparator : ranges.primary.end;
|
||||
}
|
||||
|
||||
/**
|
||||
* #3262 (write-time milestone-scope guard): the phase-creation and
|
||||
* phase-insertion entry templates interpolate the caller's `description`
|
||||
* verbatim into `### Phase N: ${description}`. A description embedding a
|
||||
* level 1-3 heading that carries a milestone marker (version token,
|
||||
* ✅/📋/🚧/🔄, or the word "Milestone") would splice a heading that TERMINATES
|
||||
* the current milestone window (`computeMilestoneSectionEnd`) and silently
|
||||
* drops every later phase out of the derived milestone phase set. Reject
|
||||
* before any write or phase-directory creation — the fail-loud sibling of
|
||||
* the edit-phase workflow's depends_on gate. The predicate itself
|
||||
* (`findMilestoneScopeHeadingLines`) is fence-aware and Phase-heading-exempt,
|
||||
* so ordinary descriptions and the phase's own numbered heading never trip it.
|
||||
*/
|
||||
function assertDescriptionPreservesMilestoneScope(description: string, command: string): void {
|
||||
const offending = findMilestoneScopeHeadingLines(description);
|
||||
if (offending.length === 0) return;
|
||||
error(
|
||||
`${command}: description contains a milestone-scoping heading line — writing it to ROADMAP.md would terminate ` +
|
||||
`the current milestone window and silently drop later phases out of the milestone scope. ` +
|
||||
`Offending line(s): ${offending.map((line) => JSON.stringify(line)).join(', ')}. ` +
|
||||
`Rewrite the line so it is not a level 1-3 "#" heading carrying a milestone marker ` +
|
||||
`(a vN.N version token, a ✅/📋/🚧/🔄 marker, or the word "Milestone").`
|
||||
);
|
||||
}
|
||||
|
||||
function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: string): void {
|
||||
if (!description) {
|
||||
error('description required for phase add');
|
||||
}
|
||||
assertDescriptionPreservesMilestoneScope(description, 'phase add');
|
||||
|
||||
const config = loadConfig(cwd);
|
||||
const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
|
||||
@@ -1149,6 +1175,12 @@ function cmdPhaseAddBatch(cwd: string, descriptions: string[], raw: boolean): vo
|
||||
if (!Array.isArray(descriptions) || descriptions.length === 0) {
|
||||
error('descriptions array required for phase add-batch');
|
||||
}
|
||||
// #3262: validate every description BEFORE the lock — the batch is
|
||||
// all-or-nothing, so one offending description must reject the whole batch
|
||||
// with no ROADMAP write and no phase directories created.
|
||||
for (const description of descriptions) {
|
||||
assertDescriptionPreservesMilestoneScope(description, 'phase add-batch');
|
||||
}
|
||||
const config = loadConfig(cwd);
|
||||
const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
|
||||
if (!fs.existsSync(roadmapPath)) {
|
||||
@@ -1230,6 +1262,7 @@ function cmdPhaseInsert(cwd: string, afterPhase: string, description: string, ra
|
||||
if (!afterPhase || !description) {
|
||||
error('after-phase and description required for phase insert');
|
||||
}
|
||||
assertDescriptionPreservesMilestoneScope(description, 'phase insert');
|
||||
|
||||
const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
|
||||
if (!fs.existsSync(roadmapPath)) {
|
||||
|
||||
@@ -36,6 +36,7 @@ const { SCOPE } = planningScopeMod;
|
||||
interface RoadmapModule {
|
||||
cmdRoadmapGetPhase(cwd: string, phase: string | undefined, raw: boolean): void;
|
||||
cmdRoadmapAnalyze(cwd: string, raw: boolean): void;
|
||||
cmdRoadmapMilestoneScope(cwd: string, raw: boolean): void;
|
||||
cmdRoadmapUpdatePlanProgress(cwd: string, phase: string | undefined, raw: boolean): void;
|
||||
cmdRoadmapAnnotateDependencies(cwd: string, phase: string | undefined, raw: boolean): void;
|
||||
}
|
||||
@@ -148,6 +149,9 @@ function routeRoadmapCommand({ roadmap, args, cwd, raw, error }: RouteRoadmapCom
|
||||
handlers: {
|
||||
'get-phase': () => roadmap.cmdRoadmapGetPhase(cwd, args[2], raw),
|
||||
analyze: () => roadmap.cmdRoadmapAnalyze(cwd, raw),
|
||||
// #3262: read-only milestone-window identity probe — the capture/compare
|
||||
// signal for the edit-phase workflow's write-time milestone-scope guard.
|
||||
'milestone-scope': () => roadmap.cmdRoadmapMilestoneScope(cwd, raw),
|
||||
'update-plan-progress': () => roadmap.cmdRoadmapUpdatePlanProgress(cwd, args[2], raw),
|
||||
'annotate-dependencies': () => roadmap.cmdRoadmapAnnotateDependencies(cwd, args[2], raw),
|
||||
'validate': () => {
|
||||
|
||||
@@ -427,6 +427,89 @@ function hasPhaseEntries(markdown: string): boolean {
|
||||
return BULLET_PHASE_LINE_PATTERN.test(stripFencedCode(markdown).text);
|
||||
}
|
||||
|
||||
/**
|
||||
* #3262: the sole owner of "which phase ids does THIS milestone window
|
||||
* declare". Extracted verbatim from `getMilestonePhaseFilter`'s former inline
|
||||
* heading scan + bullet scan so the new `roadmap milestone-scope` probe (the
|
||||
* write-time milestone-scope guard's capture/compare signal) reads the SAME
|
||||
* derivation the phase filter builds its membership set from — never a second
|
||||
* copy of either scan.
|
||||
*
|
||||
* Fence-aware on both scans (tokenizeHeadings + stripFencedCode), matching
|
||||
* `hasPhaseEntries` above: a fenced markdown EXAMPLE of either syntax is not
|
||||
* a declared phase.
|
||||
*
|
||||
* #3185: deliberately NOT isSentinelPhaseId here. That predicate treats a
|
||||
* leading 0 as sentinel milestone 0, which would swallow the #2554 decimal
|
||||
* phase ids ("00.1" is a real phase, not milestone 0). This scan asks a
|
||||
* narrower question — "which phase ids does this window declare" — where only
|
||||
* the 999 icebox range is excluded.
|
||||
*/
|
||||
function scanMilestonePhaseIds(window: string): Set<string> {
|
||||
const ids = new Set<string>();
|
||||
// Use tokenizeHeadings (fence-aware) instead of stripFencedLines + regex.
|
||||
// T4 seam migration: phase headings inside fences are excluded automatically.
|
||||
// #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE).
|
||||
const phaseHeadingPattern = /^(?:\[[^\]]{1,200}\]\s*)?Phase\s+([\w][\w.-]*)(?:\s*\([^)\n]{0,200}\))?\s*:/i;
|
||||
for (const h of tokenizeHeadings(window)) {
|
||||
if (h.level < 2 || h.level > 4) continue;
|
||||
const pm = phaseHeadingPattern.exec(h.text);
|
||||
if (pm && !/^999\b/.test(pm[1])) ids.add(pm[1]);
|
||||
}
|
||||
// #2199: also count bullet/checkbox phase entries (`- [ ] **Phase N — name**`)
|
||||
// so a bullet-house-style ROADMAP populates the milestone phase set instead of
|
||||
// collapsing to a zero-count pass-all filter.
|
||||
let bm: RegExpExecArray | null;
|
||||
const scanner = new RegExp(BULLET_PHASE_LINE_PATTERN.source, 'gim');
|
||||
const unfenced = stripFencedCode(window).text;
|
||||
while ((bm = scanner.exec(unfenced)) !== null) {
|
||||
if (!/^999\b/.test(bm[1])) ids.add(bm[1]);
|
||||
}
|
||||
return ids;
|
||||
}
|
||||
|
||||
/**
|
||||
* #3262 (write-time milestone-scope guard): does this free-text value contain
|
||||
* a heading line that would TERMINATE the current milestone window if spliced
|
||||
* into ROADMAP.md? Returns the offending heading texts (empty array = safe).
|
||||
*
|
||||
* Mirrors the parser's own terminator vocabulary (`computeMilestoneSectionEnd`):
|
||||
* a heading terminates the window when it is level 1-3, is NOT a Phase heading
|
||||
* (`/^Phase\s+\S/i` — the phase's OWN numbered heading is existing, correct,
|
||||
* load-bearing behavior and is never a violation), and carries a milestone
|
||||
* signal. The signal test is the union of `MILESTONE_HEADING_SIGNAL_PATTERN`
|
||||
* (this module's "is this heading a milestone heading" vocabulary) and `🔄`
|
||||
* (which terminates in `extractCurrentMilestoneScoped`'s own preamble pattern)
|
||||
* — deliberately the CONSERVATIVE union: a field value whose line is a level
|
||||
* 1-3 heading naming a version, a status marker, or the word "Milestone" is
|
||||
* exactly the shape that silently narrows the window, so the guard rejects on
|
||||
* any of them rather than re-deriving which specific marker a given roadmap's
|
||||
* terminator would fire on.
|
||||
*
|
||||
* Two deliberate conservatisms, both one-directional (reject more, never less):
|
||||
* - `computeMilestoneSectionEnd` also bounds by the milestone heading's own
|
||||
* level (a `###` marker only terminates a `###`-level milestone heading);
|
||||
* this predicate flags every level 1-3 marker regardless, because which
|
||||
* level the active milestone heading uses is a property of the document at
|
||||
* write time, not of the text being validated.
|
||||
* - level 4+ headings never terminate any window and are not flagged.
|
||||
*
|
||||
* Fence-aware via `tokenizeHeadings`: a FENCED example of a milestone heading
|
||||
* inside a field value does not terminate the real window, so it must not be
|
||||
* a violation either — the parser and this guard must agree on fences.
|
||||
*/
|
||||
function findMilestoneScopeHeadingLines(text: string): string[] {
|
||||
const out: string[] = [];
|
||||
for (const h of tokenizeHeadings(text)) {
|
||||
if (h.level > 3) continue;
|
||||
if (/^Phase\s+\S/i.test(h.text)) continue;
|
||||
if (MILESTONE_HEADING_SIGNAL_PATTERN.test(h.text) || /🔄/.test(h.text)) {
|
||||
out.push(h.text.trim());
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* #3184: pure decision table (no I/O, no regex construction from caller
|
||||
* data) implementing the design's Behavior table rows 1-8 (the remaining
|
||||
@@ -1294,39 +1377,11 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p
|
||||
});
|
||||
}
|
||||
|
||||
// Use tokenizeHeadings (fence-aware) instead of stripFencedLines + regex.
|
||||
// T4 seam migration: phase headings inside fences are excluded automatically.
|
||||
// #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE).
|
||||
const phaseHeadingPattern = /^(?:\[[^\]]{1,200}\]\s*)?Phase\s+([\w][\w.-]*)(?:\s*\([^)\n]{0,200}\))?\s*:/i;
|
||||
for (const h of tokenizeHeadings(roadmap)) {
|
||||
if (h.level < 2 || h.level > 4) continue;
|
||||
const pm = phaseHeadingPattern.exec(h.text);
|
||||
// #3185: deliberately NOT isSentinelPhaseId here. That predicate treats a
|
||||
// leading 0 as sentinel milestone 0, which would swallow the #2554 decimal
|
||||
// phase ids ("00.1" is a real phase, not milestone 0). This scan asks a
|
||||
// narrower question -- "which phase ids does this milestone's window
|
||||
// declare" -- where only the 999 icebox range is excluded.
|
||||
if (pm && !/^999\b/.test(pm[1])) milestonePhaseNums.add(pm[1]);
|
||||
}
|
||||
// #2199: also count bullet/checkbox phase entries (`- [ ] **Phase N — name**`)
|
||||
// so a bullet-house-style ROADMAP populates the milestone phase set instead of
|
||||
// collapsing to a zero-count pass-all filter.
|
||||
// #3184 review finding: this scan must be fence-aware like `hasPhaseEntries`
|
||||
// above — otherwise a fenced markdown EXAMPLE of the bullet syntax inflates
|
||||
// milestonePhaseNums / phaseCount. Strip fences through the canonical seam
|
||||
// first.
|
||||
{
|
||||
let bm: RegExpExecArray | null;
|
||||
const scanner = new RegExp(BULLET_PHASE_LINE_PATTERN.source, 'gim');
|
||||
const roadmapUnfenced = stripFencedCode(roadmap).text;
|
||||
while ((bm = scanner.exec(roadmapUnfenced)) !== null) {
|
||||
// #3185: deliberately NOT isSentinelPhaseId here. That predicate treats a
|
||||
// leading 0 as sentinel milestone 0, which would swallow the #2554 decimal
|
||||
// phase ids ("00.1" is a real phase, not milestone 0). This scan asks a
|
||||
// narrower question -- "which phase ids does this milestone's window
|
||||
// declare" -- where only the 999 icebox range is excluded.
|
||||
if (!/^999\b/.test(bm[1])) milestonePhaseNums.add(bm[1]);
|
||||
}
|
||||
// #3262: the set-building scan now lives in its own named owner
|
||||
// (`scanMilestonePhaseIds`) so the new `roadmap milestone-scope` probe
|
||||
// reads the SAME derivation this filter does — never a second copy.
|
||||
for (const id of scanMilestonePhaseIds(roadmap)) {
|
||||
milestonePhaseNums.add(id);
|
||||
}
|
||||
} catch {
|
||||
/* best-effort (#2245 audit): the real throw source is platformReadSync
|
||||
@@ -1531,4 +1586,10 @@ export = {
|
||||
// #1956: sole owner of the #2012 decoy-avoidance scope for the
|
||||
// `drift-guard phase-status` CLI seam.
|
||||
findRoadmapProgressTable,
|
||||
// #3262 (write-time milestone-scope guard): the window phase-id scan owner
|
||||
// (consumed by getMilestonePhaseFilter above and the roadmap milestone-scope
|
||||
// CLI probe) and the free-text predicate the phase add/add-batch/insert
|
||||
// guards and the edit-phase workflow's pre/post capture are built on.
|
||||
scanMilestonePhaseIds,
|
||||
findMilestoneScopeHeadingLines,
|
||||
};
|
||||
|
||||
@@ -26,7 +26,7 @@ const { SCOPE } = planningScopeMod;
|
||||
type Scope = planningScopeMod.Scope;
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import roadmapParserModule = require('./roadmap-parser.cjs');
|
||||
const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone, listMilestoneHeadings } = roadmapParserModule;
|
||||
const { stripShippedMilestones, extractCurrentMilestone, extractCurrentMilestoneScoped, replaceInCurrentMilestone, listMilestoneHeadings, scanMilestonePhaseIds } = roadmapParserModule;
|
||||
import { tokenizeHeadings } from './markdown-sectionizer.cjs';
|
||||
import { updateTableCell } from './markdown-table.cjs';
|
||||
import { clampPercent } from './phase-lifecycle.cjs';
|
||||
@@ -622,6 +622,40 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void {
|
||||
output(result, raw, undefined);
|
||||
}
|
||||
|
||||
// ─── cmdRoadmapMilestoneScope ────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* #3262 (write-time milestone-scope guard): read-only probe emitting the
|
||||
* current milestone window's IDENTITY — its scope classification and the
|
||||
* phase ids it declares — so the edit-phase workflow can capture it before
|
||||
* its in-place section write, re-derive it after, and roll back on any
|
||||
* change. This is the milestone-scope sibling of the workflow's existing
|
||||
* `depends_on` gate, expressed as a command because the workflow's write is
|
||||
* assistant-driven free-text surgery, not a code path.
|
||||
*
|
||||
* Deliberately NOT `cmdRoadmapAnalyze`: analyze's #3165 recovery re-populates
|
||||
* `phases` from the shipped-milestone-stripped document when the scoped
|
||||
* window is suspect, which is right for a human-facing progress report and
|
||||
* wrong for a before/after equality probe — the refill would mask exactly
|
||||
* the narrowing this guard exists to detect. This probe reports the RAW
|
||||
* window (`extractCurrentMilestoneScoped` + `scanMilestonePhaseIds`), no
|
||||
* fallback, so a narrowed window is always visible as a changed phase set.
|
||||
*/
|
||||
function cmdRoadmapMilestoneScope(cwd: string, raw: boolean): void {
|
||||
const roadmapPath = planningPaths(cwd).roadmap;
|
||||
|
||||
if (!fs.existsSync(roadmapPath)) {
|
||||
output({ error: 'ROADMAP.md not found', scope: SCOPE.UNREADABLE, phases: [], phase_count: 0 }, raw, undefined);
|
||||
return;
|
||||
}
|
||||
|
||||
const rawContent = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
const { value: window, scope } = extractCurrentMilestoneScoped(rawContent, cwd);
|
||||
// Document order (Set insertion order) — deterministic for a given document.
|
||||
const phases = [...scanMilestonePhaseIds(window)];
|
||||
output({ scope, phases, phase_count: phases.length }, raw, undefined);
|
||||
}
|
||||
|
||||
// ─── cmdRoadmapUpdatePlanProgress ─────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
@@ -1153,6 +1187,7 @@ export = {
|
||||
cmdRoadmapGetPhase,
|
||||
getRoadmapPhaseWithFallback,
|
||||
cmdRoadmapAnalyze,
|
||||
cmdRoadmapMilestoneScope,
|
||||
cmdRoadmapUpdatePlanProgress,
|
||||
cmdRoadmapAnnotateDependencies,
|
||||
buildPhaseHeadingRegex,
|
||||
|
||||
287
tests/edit-phase-milestone-scope-guard.test.cjs
Normal file
287
tests/edit-phase-milestone-scope-guard.test.cjs
Normal file
@@ -0,0 +1,287 @@
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* #3262 — milestone-scope guard regression tests.
|
||||
*
|
||||
* The in-place phase editor (edit-phase workflow) and the phase-creation
|
||||
* entry templates (phase add / add-batch / insert) can splice free text into
|
||||
* ROADMAP.md that carries a level 1-3 heading with a milestone marker. Such a
|
||||
* heading terminates the current milestone window (computeMilestoneSectionEnd)
|
||||
* and silently drops phases from the derived milestone phase set. These tests
|
||||
* pin the two mechanical guards this fix adds:
|
||||
*
|
||||
* 1. `roadmap milestone-scope` — read-only probe emitting the current
|
||||
* milestone window identity (scope + declared phase ids) so the edit-phase
|
||||
* workflow can capture/compare around its write.
|
||||
* 2. phase add / add-batch / insert reject a description containing a
|
||||
* milestone-scoping heading line before any write or directory creation.
|
||||
*
|
||||
* Behavioral only — every case drives the gsd-tools CLI seam.
|
||||
*/
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const VERSIONED_ROADMAP = [
|
||||
'# Roadmap',
|
||||
'',
|
||||
'## v1.0 — Foundation',
|
||||
'',
|
||||
'### Phase 1: Setup',
|
||||
'',
|
||||
'**Goal:** bootstrap',
|
||||
'',
|
||||
'### Phase 2: API',
|
||||
'',
|
||||
'**Goal:** api',
|
||||
'',
|
||||
'### Phase 3: UI',
|
||||
'',
|
||||
'**Goal:** ui',
|
||||
'',
|
||||
'## v2.0 — Next',
|
||||
'',
|
||||
'### Phase 4: Extras',
|
||||
'',
|
||||
'**Goal:** extras',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
// The insidious shape: a stray milestone-bearing heading INSIDE the current
|
||||
// milestone terminates the window early. The window still contains phase
|
||||
// entries, so scope reads "complete" — the drop is silent, which is exactly
|
||||
// why the guard must compare the phase SET, not just the scope value.
|
||||
const NARROWED_ROADMAP = [
|
||||
'# Roadmap',
|
||||
'',
|
||||
'## v1.0 — Foundation',
|
||||
'',
|
||||
'### Phase 1: Setup',
|
||||
'',
|
||||
'**Goal:** bootstrap',
|
||||
'',
|
||||
'### Phase 2: API',
|
||||
'',
|
||||
'**Goal:** api',
|
||||
'',
|
||||
'## v2.1 Stretch — 🚧',
|
||||
'',
|
||||
'### Phase 3: UI',
|
||||
'',
|
||||
'**Goal:** ui',
|
||||
'',
|
||||
'## v2.0 — Next',
|
||||
'',
|
||||
'### Phase 4: Extras',
|
||||
'',
|
||||
'**Goal:** extras',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
const FREEFORM_ROADMAP = [
|
||||
'# Roadmap',
|
||||
'',
|
||||
'### Phase 1: Foundation',
|
||||
'',
|
||||
'**Goal:** bootstrap',
|
||||
'',
|
||||
'### Phase 2: Polish',
|
||||
'',
|
||||
'**Goal:** polish',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
function writeRoadmap(tmpDir, content) {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), content);
|
||||
}
|
||||
|
||||
function writeStateMilestone(tmpDir, version) {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), `# State\n\nmilestone: ${version}\n`);
|
||||
}
|
||||
|
||||
function readRoadmap(tmpDir) {
|
||||
return fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
|
||||
}
|
||||
|
||||
function listPhaseDirs(tmpDir) {
|
||||
return fs.readdirSync(path.join(tmpDir, '.planning', 'phases'));
|
||||
}
|
||||
|
||||
function runMilestoneScope(tmpDir) {
|
||||
const res = runGsdTools(['roadmap', 'milestone-scope'], tmpDir);
|
||||
assert.equal(res.success, true, `roadmap milestone-scope should exit 0, got: ${res.error || res.output}`);
|
||||
return JSON.parse(res.output);
|
||||
}
|
||||
|
||||
// ─── roadmap milestone-scope probe ────────────────────────────────────────────
|
||||
|
||||
describe('#3262 roadmap milestone-scope probe', () => {
|
||||
test('reports the current milestone window phase set (later milestones excluded)', () => {
|
||||
const tmp = createTempProject('gsd-3262-scope-');
|
||||
try {
|
||||
writeStateMilestone(tmp, 'v1.0');
|
||||
writeRoadmap(tmp, VERSIONED_ROADMAP);
|
||||
const result = runMilestoneScope(tmp);
|
||||
assert.equal(result.scope, 'complete');
|
||||
assert.deepEqual(result.phases, ['1', '2', '3']);
|
||||
assert.equal(result.phase_count, 3);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('a stray milestone heading inside the window silently narrows the phase set — the signal this probe exists to expose', () => {
|
||||
const tmp = createTempProject('gsd-3262-scope-');
|
||||
try {
|
||||
writeStateMilestone(tmp, 'v1.0');
|
||||
writeRoadmap(tmp, NARROWED_ROADMAP);
|
||||
const result = runMilestoneScope(tmp);
|
||||
// Phase 3 sits after the stray `## v2.1 Stretch — 🚧` terminator: it is
|
||||
// out of the window. Scope stays "complete" (the window still reaches
|
||||
// phase entries), so ONLY the phase set reveals the drop.
|
||||
assert.deepEqual(result.phases, ['1', '2']);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('free-form roadmap (no versioned milestones) reports the whole-document phase set', () => {
|
||||
const tmp = createTempProject('gsd-3262-scope-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const result = runMilestoneScope(tmp);
|
||||
assert.equal(result.scope, 'complete');
|
||||
assert.deepEqual(result.phases, ['1', '2']);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('missing ROADMAP.md is a decodable unreadable result, not a crash', () => {
|
||||
const tmp = createTempProject('gsd-3262-scope-');
|
||||
try {
|
||||
const res = runGsdTools(['roadmap', 'milestone-scope'], tmp);
|
||||
assert.equal(res.success, true);
|
||||
const parsed = JSON.parse(res.output);
|
||||
assert.equal(parsed.scope, 'unreadable');
|
||||
assert.deepEqual(parsed.phases, []);
|
||||
assert.ok(parsed.error, 'should carry an error field naming the missing roadmap');
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─── phase add / add-batch / insert description guard ────────────────────────
|
||||
|
||||
describe('#3262 phase add milestone-scope heading guard', () => {
|
||||
test('rejects a description embedding a versioned milestone heading; ROADMAP and phase dirs untouched', () => {
|
||||
const tmp = createTempProject('gsd-3262-add-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const before = readRoadmap(tmp);
|
||||
const dirsBefore = listPhaseDirs(tmp);
|
||||
const res = runGsdTools(['phase', 'add', 'Extra work\n## v2.1 Evil — 🚧\nmore text'], tmp);
|
||||
assert.equal(res.success, false, 'phase add must reject an embedded milestone heading');
|
||||
assert.match(res.error || res.output, /v2\.1 Evil/, 'error must name the offending line');
|
||||
assert.equal(readRoadmap(tmp), before, 'ROADMAP.md must be byte-unchanged');
|
||||
assert.deepEqual(listPhaseDirs(tmp), dirsBefore, 'no phase directory may be created');
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('rejects a description embedding a "Milestone" word heading', () => {
|
||||
const tmp = createTempProject('gsd-3262-add-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const before = readRoadmap(tmp);
|
||||
const res = runGsdTools(['phase', 'add', 'Retro\n# Milestone Two\nnotes'], tmp);
|
||||
assert.equal(res.success, false);
|
||||
assert.equal(readRoadmap(tmp), before);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('ordinary single-line description still succeeds (no false rejection)', () => {
|
||||
const tmp = createTempProject('gsd-3262-add-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const res = runGsdTools(['phase', 'add', 'Authentication and sessions'], tmp);
|
||||
assert.equal(res.success, true, `ordinary add must keep working: ${res.error || ''}`);
|
||||
assert.match(readRoadmap(tmp), /### Phase 3: Authentication and sessions/);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('a milestone heading inside a fenced code block in the description is not a violation (parser is fence-aware)', () => {
|
||||
const tmp = createTempProject('gsd-3262-add-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const desc = 'Document auth\n\n```\n## v2.0 example heading\n```\n';
|
||||
const res = runGsdTools(['phase', 'add', desc], tmp);
|
||||
assert.equal(res.success, true, `fenced example must not trip the guard: ${res.error || ''}`);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('level-4+ headings and Phase-prefixed headings in a description are not violations', () => {
|
||||
const tmp = createTempProject('gsd-3262-add-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const desc = 'Notes\n#### Sub-notes heading\n### Phase 9: decoy reference';
|
||||
const res = runGsdTools(['phase', 'add', desc], tmp);
|
||||
assert.equal(res.success, true, `level-4/Phase-prefixed headings must not trip the guard: ${res.error || ''}`);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3262 phase insert milestone-scope heading guard', () => {
|
||||
test('rejects a description embedding a closed-marker milestone heading; ROADMAP untouched', () => {
|
||||
const tmp = createTempProject('gsd-3262-insert-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const before = readRoadmap(tmp);
|
||||
const res = runGsdTools(['phase', 'insert', '1', 'Sneaky\n## ✅ v3.0 Done\nnotes'], tmp);
|
||||
assert.equal(res.success, false, 'phase insert must reject an embedded milestone heading');
|
||||
assert.equal(readRoadmap(tmp), before, 'ROADMAP.md must be byte-unchanged');
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('ordinary insert description still succeeds (no false rejection)', () => {
|
||||
const tmp = createTempProject('gsd-3262-insert-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const res = runGsdTools(['phase', 'insert', '1', 'Decimal sub-phase'], tmp);
|
||||
assert.equal(res.success, true, `ordinary insert must keep working: ${res.error || ''}`);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3262 phase add-batch milestone-scope heading guard', () => {
|
||||
test('rejects the batch when any description embeds a milestone heading; ROADMAP untouched', () => {
|
||||
const tmp = createTempProject('gsd-3262-batch-');
|
||||
try {
|
||||
writeRoadmap(tmp, FREEFORM_ROADMAP);
|
||||
const before = readRoadmap(tmp);
|
||||
const descriptions = JSON.stringify(['Good phase', 'Bad\n## v2.1 Evil — 🚧']);
|
||||
const res = runGsdTools(['phase', 'add-batch', '--descriptions', descriptions], tmp);
|
||||
assert.equal(res.success, false, 'add-batch must reject an embedded milestone heading');
|
||||
assert.equal(readRoadmap(tmp), before, 'ROADMAP.md must be byte-unchanged (all-or-nothing)');
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -309,6 +309,51 @@ describe('edit-phase workflow: phase number and position preservation', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Workflow: milestone scope guard (#3262) ─────────────────────────────────
|
||||
|
||||
describe('edit-phase workflow: milestone scope guard (#3262)', () => {
|
||||
test('workflow captures the milestone scope before writing the updated phase', () => {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
const writeStep = content.match(/<step name="write_updated_phase">([\s\S]*?)<\/step>/);
|
||||
assert.ok(writeStep, 'write_updated_phase step must exist');
|
||||
assert.match(
|
||||
writeStep[1],
|
||||
/milestone-scope/,
|
||||
'write_updated_phase must run the roadmap milestone-scope probe before writing'
|
||||
);
|
||||
assert.match(writeStep[1], /SCOPE_BEFORE/i, 'the pre-write capture must be named for the post-write comparison');
|
||||
});
|
||||
|
||||
test('workflow re-derives the milestone scope after the write and rolls back on mismatch', () => {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
const writeStep = content.match(/<step name="write_updated_phase">([\s\S]*?)<\/step>/);
|
||||
assert.ok(writeStep, 'write_updated_phase step must exist');
|
||||
assert.match(writeStep[1], /SCOPE_AFTER/i, 'the post-write re-derivation must be present');
|
||||
assert.match(writeStep[1], /scope|phases/i, 'the comparison must cover the scope and the phase set');
|
||||
assert.match(
|
||||
writeStep[1],
|
||||
/rollback|restore|revert|rolled back/i,
|
||||
'a scope or phase-set mismatch must trigger rollback'
|
||||
);
|
||||
assert.match(
|
||||
writeStep[1],
|
||||
/milestone scope changed|scope changed/i,
|
||||
'the rollback path must surface an explicit error'
|
||||
);
|
||||
});
|
||||
|
||||
test('milestone scope guard success criterion is checked (#3262)', () => {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
const criteria = content.match(/<success_criteria>([\s\S]*?)<\/success_criteria>/);
|
||||
assert.ok(criteria, 'workflow should have a success_criteria section');
|
||||
assert.match(
|
||||
criteria[1],
|
||||
/milestone scope/i,
|
||||
'success criteria must include the milestone-scope verification'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Workflow: STATE.md update ────────────────────────────────────────────────
|
||||
|
||||
describe('edit-phase workflow: STATE.md roadmap evolution', () => {
|
||||
|
||||
@@ -0,0 +1,6 @@
|
||||
{
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"edit-phase.md": "#3262: adds the mechanical milestone-scope guard the issue says this workflow lacks. `write_updated_phase` now captures `gsd_run query roadmap milestone-scope` (the new read-only window-identity probe: scope + declared phase ids) BEFORE the in-place section splice, re-derives it AFTER, and on any `scope`/`phases` difference restores the original section text and exits with an explicit ERROR — the same block-before-write shape as the workflow's existing `validate_depends_on` gate, which is untouched. One success-criteria checkbox and one anti-pattern line accompany it. Net growth is the two small probe invocations plus the rollback/error contract (~1.1 KB over the 12,927-byte base, well inside the DEFAULT 40 KiB hard cap); no prose was extracted or moved. Companion code changes (src/roadmap-parser.cts, src/roadmap.cts, src/phase.cts, src/command-aliases.cts, src/roadmap-command-router.cts) are self-attributing."
|
||||
}
|
||||
}
|
||||
@@ -83,7 +83,9 @@ describe('roadmap-command-router', () => {
|
||||
},
|
||||
});
|
||||
|
||||
assert.equal(message, 'Unknown roadmap subcommand. Available: analyze, get-phase, update-plan-progress, annotate-dependencies, validate, upgrade');
|
||||
// #3262 added the read-only `milestone-scope` probe after `analyze`
|
||||
// (ROADMAP_SUBCOMMANDS order mirrors ROADMAP_COMMAND_ALIASES).
|
||||
assert.equal(message, 'Unknown roadmap subcommand. Available: analyze, milestone-scope, get-phase, update-plan-progress, annotate-dependencies, validate, upgrade');
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user