* test(#3572): pin single-frontmatter contract for phase remove STATE.md writes Failing-first regression for #3572: when the removed phase has a directory and the body lacks Total Phases/of-N, cmdPhaseRemove's no-op-guard bypass prepended the count field to the WHOLE file — before the opening fence — corrupting STATE.md into two frontmatter blocks. Rows also strengthen the #2640 coverage (whose first-match assertions pass even on a corrupted file) and pin the issue's ROADMAP-only control. * fix(#3572): keep phase remove's STATE.md write single-block and drop the removed heading from ROADMAP Two defects in the decimal-phase removal path: (1) the #2640 no-op-guard bypass prepended 'Total Phases: N' to the WHOLE file content, landing it before the opening fence and corrupting STATE.md into two frontmatter blocks; the field now inserts at the top of the BODY, after the closing fence (EOL-aware, frontmatter-less files unchanged in behavior). (2) updateRoadmapAfterPhaseRemoval matched the raw query token ('1.1') against the normalized zero-padded heading ('Phase 01.1:'), so the removed phase stayed in ROADMAP and the resync counted it; the heading, checklist, and progress-row matchers are now zero-pad tolerant, which also covers unpadded integer headings. * fix(#3572): clamp phase-count decrements at zero; harden EOL detection; pin controls Review findings: a stale 'Total Phases: 0' could decrement to -1 on the next removal (both the field and the 'of N' phrase now clamp at 0); insertStateBodyFieldAtTop detects EOL from the first line ending so an LF-dominant file with a stray CRLF cannot fall through to the raw prepend; the issue's insert-alone control is pinned; row 1 pins the body-field value (dir-count provenance) alongside the roadmap-derived frontmatter count. * fix(#3572): keep CRLF endings intact in the body-field insertion Green-run failure root cause: splitting on '\n' but re-joining on a detected '\r\n' doubled every carriage return in CRLF files. Split and join uniformly on '\n' so each '\r' stays attached to the line it terminated. * chore(#3572): add changeset fragment * chore(#3572): backfill changeset pr number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/eager-wasps-travel.md
Normal file
5
.changeset/eager-wasps-travel.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3594
|
||||
---
|
||||
**`phase remove` no longer corrupts STATE.md after removing an inserted (decimal) phase** — the removed-phase write prepended a second, partially-wrong frontmatter block (and left the phase's ROADMAP heading behind, so total_phases kept counting it); removal now updates STATE.md in place as a single block, drops the heading, and clamps phase counts at zero. (#3572)
|
||||
@@ -1724,6 +1724,14 @@ function updateRoadmapAfterPhaseRemoval(
|
||||
withPlanningLock(cwd, () => {
|
||||
let content = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
const escaped = escapeRegex(targetPhase);
|
||||
// #3572: ROADMAP headings and rows carry the normalized (zero-padded) form
|
||||
// of a decimal id — `phase insert 1` writes `### Phase 01.1:` while the
|
||||
// user's remove query is usually unpadded (`1.1`) — and integer headings
|
||||
// legitimately appear both padded (`02`) and unpadded (`2`). A `0*` prefix
|
||||
// makes the token padding-insensitive in both directions without widening
|
||||
// to other ids: the token stays anchored between `Phase\s+`/line-start and
|
||||
// `:`/whitespace/end, so `0*2` still never matches `Phase 12:`.
|
||||
const padTolerant = `0*${escaped}`;
|
||||
|
||||
// SECTION-DELETION (not a section-body edit) — removes the phase's ENTIRE
|
||||
// detail section INCLUDING its own heading line. Migrated onto deleteSection
|
||||
@@ -1737,7 +1745,7 @@ function updateRoadmapAfterPhaseRemoval(
|
||||
// away everything after it — including a trailing `## Progress` heading and
|
||||
// its tracking table.
|
||||
const phaseHeadingRe = new RegExp(
|
||||
`^Phase\\s+${escaped}${OPTIONAL_PHASE_TAG_SOURCE}\\s*:`,
|
||||
`^Phase\\s+${padTolerant}${OPTIONAL_PHASE_TAG_SOURCE}\\s*:`,
|
||||
'i',
|
||||
);
|
||||
content = deleteSection(
|
||||
@@ -1745,7 +1753,7 @@ function updateRoadmapAfterPhaseRemoval(
|
||||
(h) => h.level >= 2 && h.level <= 4 && phaseHeadingRe.test(h.text),
|
||||
);
|
||||
content = content.replace(
|
||||
new RegExp(`\\n?-\\s*\\[[ x]\\]\\s*.*Phase\\s+${escaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*`, 'gi'),
|
||||
new RegExp(`\\n?-\\s*\\[[ x]\\]\\s*.*Phase\\s+${padTolerant}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*`, 'gi'),
|
||||
'',
|
||||
);
|
||||
// ROW-DELETION (not a cell update) — removes the WHOLE Progress-table row
|
||||
@@ -1776,7 +1784,7 @@ function updateRoadmapAfterPhaseRemoval(
|
||||
const matchRemovedProgressRow = (row: Record<string, string>): boolean => {
|
||||
const firstCellRaw = (Object.values(row)[0] ?? '').trim();
|
||||
if (isDecimal) {
|
||||
return new RegExp(`^${escaped}\\.?(?:\\s|$)`, 'i').test(firstCellRaw);
|
||||
return new RegExp(`^${padTolerant}\\.?(?:\\s|$)`, 'i').test(firstCellRaw);
|
||||
}
|
||||
const leadingMatch = firstCellRaw.match(/^0*(\d+)(\.\d+)?/);
|
||||
if (!leadingMatch || leadingMatch[2]) return false;
|
||||
@@ -1919,6 +1927,34 @@ interface PhaseRemoveOptions {
|
||||
force?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* #3572: insert `fieldLine` at the start of STATE.md's BODY — immediately after
|
||||
* the leading frontmatter block's closing `---` fence — so a body field never
|
||||
* lands before the opening fence. The former whole-content prepend
|
||||
* (`field + content`) put the line ABOVE the opening `---`, and
|
||||
* syncStateFrontmatter then treated the scrambled fence structure as TWO
|
||||
* frontmatter blocks, rebuilding a derived one on top of the original
|
||||
* (milestone_name from a ROADMAP heading, total_phases counting the removed
|
||||
* phase, a stray 'Total Phases: 0' between fences). A file with no leading
|
||||
* frontmatter is all body: the field goes to content start, preserving the
|
||||
* former behavior for that shape.
|
||||
*/
|
||||
function insertStateBodyFieldAtTop(content: string, fieldLine: string): string {
|
||||
// Split AND join on bare '\n' so CRLF line endings stay attached to their
|
||||
// own lines — each '\r' remains the tail of the line it terminated, where
|
||||
// the trimmed fence compare still matches it. (#3572 review: splitting on
|
||||
// '\n' but re-joining on a detected '\r\n' doubled every carriage return.)
|
||||
const lines = content.split('\n');
|
||||
if ((lines[0] ?? '').trim() === '---') {
|
||||
const closeIdx = lines.findIndex((l: string, i: number) => i > 0 && l.trim() === '---');
|
||||
if (closeIdx !== -1) {
|
||||
lines.splice(closeIdx + 1, 0, '', fieldLine);
|
||||
return lines.join('\n');
|
||||
}
|
||||
}
|
||||
return fieldLine + '\n' + content;
|
||||
}
|
||||
|
||||
function cmdPhaseRemove(
|
||||
cwd: string,
|
||||
targetPhase: string,
|
||||
@@ -2034,15 +2070,21 @@ function cmdPhaseRemove(
|
||||
let modified = stateContent;
|
||||
const totalRaw = stateExtractField(modified, 'Total Phases');
|
||||
if (totalRaw) {
|
||||
// #3572 review: clamp at 0 — a stale 'Total Phases: 0' (e.g. written by
|
||||
// an earlier remove whose dir-count was 0) must not decrement to -1 on
|
||||
// the next removal.
|
||||
modified =
|
||||
stateReplaceField(modified, 'Total Phases', String(parseInt(totalRaw, 10) - 1)) ||
|
||||
modified;
|
||||
stateReplaceField(
|
||||
modified,
|
||||
'Total Phases',
|
||||
String(Math.max(0, parseInt(totalRaw, 10) - 1)),
|
||||
) || modified;
|
||||
}
|
||||
const ofMatch = modified.match(/(\bof\s+)(\d+)(\s*(?:\(|phases?))/i);
|
||||
if (ofMatch) {
|
||||
modified = modified.replace(
|
||||
/(\bof\s+)(\d+)(\s*(?:\(|phases?))/i,
|
||||
`$1${parseInt(ofMatch[2], 10) - 1}$3`,
|
||||
`$1${Math.max(0, parseInt(ofMatch[2], 10) - 1)}$3`,
|
||||
);
|
||||
}
|
||||
// #2640: if neither body field was found, the transform is a no-op.
|
||||
@@ -2066,15 +2108,22 @@ function cmdPhaseRemove(
|
||||
// just-deleted directory as still present and write a `Total Phases`
|
||||
// one too high. Identity is also what the comment above already
|
||||
// claims this filter does, and the block is gated on targetDir.
|
||||
const remainingPhases = subdirs.filter((d) => d !== targetDir).length;
|
||||
// (#3572 note: this body field counts DIRECTORIES on disk; the
|
||||
// frontmatter progress.* block is rebuilt by syncStateFrontmatter
|
||||
// from the post-removal ROADMAP — the two counts legitimately differ
|
||||
// when phases exist in ROADMAP without directories.)
|
||||
const remainingPhases = Math.max(0, subdirs.filter((d) => d !== targetDir).length);
|
||||
if (totalRaw) {
|
||||
modified =
|
||||
stateReplaceField(modified, 'Total Phases', String(remainingPhases)) || modified;
|
||||
} else {
|
||||
// No 'Total Phases:' field in the body — append one so the no-op
|
||||
// guard sees a diff. syncStateFrontmatter will then rebuild the
|
||||
// frontmatter progress.* block from the real disk/ROADMAP count.
|
||||
modified = `Total Phases: ${remainingPhases}\n` + modified;
|
||||
// No 'Total Phases:' field in the body — insert one at the start of
|
||||
// the BODY so the no-op guard sees a diff. #3572: the former
|
||||
// whole-content prepend landed the line BEFORE the opening '---'
|
||||
// fence and corrupted STATE.md into two frontmatter blocks.
|
||||
// syncStateFrontmatter will still rebuild the frontmatter
|
||||
// progress.* block from the real disk/ROADMAP count.
|
||||
modified = insertStateBodyFieldAtTop(modified, `Total Phases: ${remainingPhases}`);
|
||||
}
|
||||
}
|
||||
return modified;
|
||||
|
||||
@@ -11618,3 +11618,172 @@ still to be determined by the roadmap.
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3572: phase remove must not prepend a second frontmatter block ──────────
|
||||
|
||||
describe('bug #3572: phase remove must not corrupt STATE.md into two frontmatter blocks', () => {
|
||||
const ISSUE_STATE = [
|
||||
'---',
|
||||
'gsd_state_version: 1.0',
|
||||
'milestone: v1.0',
|
||||
'milestone_name: First',
|
||||
'current_phase: 2',
|
||||
'current_phase_name: Feature',
|
||||
'status: executing',
|
||||
'stopped_at: Phase 1 complete',
|
||||
'last_updated: "2026-08-16T10:00:00.000Z"',
|
||||
'last_activity: 2026-08-16',
|
||||
'last_activity_desc: "Phase 1 complete."',
|
||||
'progress:',
|
||||
' total_phases: 2',
|
||||
' completed_phases: 1',
|
||||
' total_plans: 2',
|
||||
' completed_plans: 1',
|
||||
' percent: 50',
|
||||
'---',
|
||||
'',
|
||||
'# Project State',
|
||||
'',
|
||||
'Some prose here that must survive.',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
const TWO_PHASE_ROADMAP = '# Roadmap\n\n## Milestone v1.0\n\n### Phase 1: Setup\n**Goal:** Bootstrap the project.\n\n### Phase 2: Feature\n**Goal:** Ship the feature.\n';
|
||||
|
||||
function setupProject(t, stateMd = ISSUE_STATE, eol = '\n') {
|
||||
const tmpDir = createTempProject('gsd-3572-');
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), TWO_PHASE_ROADMAP);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
stateMd.split('\n').join(eol),
|
||||
);
|
||||
t.after(() => cleanup(tmpDir));
|
||||
return tmpDir;
|
||||
}
|
||||
|
||||
function fenceLineCount(content) {
|
||||
return content.split(/\r?\n/).filter((l) => l.trim() === '---').length;
|
||||
}
|
||||
|
||||
test('#3572: phase remove of an inserted decimal phase keeps STATE.md a single frontmatter block', (t) => {
|
||||
const tmpDir = setupProject(t);
|
||||
// The issue's exact sequence: insert creates the directory; remove then has a
|
||||
// targetDir !== null, and the body lacks Total Phases/of-N — the trigger.
|
||||
let r = runGsdTools('phase insert 1 "Inserted probe"', tmpDir);
|
||||
assert.ok(r.success, `phase insert failed: ${r.error}`);
|
||||
r = runGsdTools('phase remove 1.1', tmpDir);
|
||||
assert.ok(r.success, `phase remove failed: ${r.error}`);
|
||||
assert.strictEqual(JSON.parse(r.output).state_updated, true, 'the #2640 resync must still happen');
|
||||
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.ok(after.startsWith('---\n') || after.startsWith('---\r\n'), 'file must still OPEN with the frontmatter fence');
|
||||
assert.strictEqual(fenceLineCount(after), 2, `exactly one frontmatter block (2 fence lines); got ${fenceLineCount(after)}:\n${after.slice(0, 400)}`);
|
||||
assert.strictEqual((after.match(/gsd_state_version/g) || []).length, 1, 'exactly one gsd_state_version — no second derived block');
|
||||
assert.ok(after.includes('Some prose here that must survive.'), 'body prose must survive verbatim');
|
||||
assert.match(after, /^Total Phases:\s*\d+$/m, 'the inserted count field must live in the BODY (line-start), not before the first fence');
|
||||
// Pinned value: the body field counts DIRECTORIES on disk (0 after removing
|
||||
// the only directory); the frontmatter progress block derives from ROADMAP
|
||||
// (2 below) — the two counters have different provenance by design (#2640/#2528).
|
||||
assert.match(after, /^Total Phases:\s*0$/m, 'body field = remaining on-disk phase directories');
|
||||
const fm = after.match(/total_phases:\s*(\d+)/);
|
||||
assert.ok(fm, 'frontmatter progress.total_phases present');
|
||||
assert.strictEqual(fm[1], '2', `total_phases must resync to the 2 remaining roadmap phases; got ${fm[1]}`);
|
||||
});
|
||||
|
||||
test('#3572: integer-phase remove with directory also stays single-block (strengthens #2640)', (t) => {
|
||||
const tmpDir = setupProject(t);
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-feature'), { recursive: true });
|
||||
const r = runGsdTools('phase remove 2', tmpDir);
|
||||
assert.ok(r.success, `phase remove failed: ${r.error}`);
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.strictEqual(fenceLineCount(after), 2, `single frontmatter block; got ${fenceLineCount(after)}`);
|
||||
assert.ok(after.startsWith('---'), 'opens with the fence');
|
||||
assert.ok(after.includes('Some prose here that must survive.'), 'body prose preserved');
|
||||
});
|
||||
|
||||
test('#3572: existing body Total Phases decremented in place, single block', (t) => {
|
||||
const stateWithField = ISSUE_STATE.replace(
|
||||
'Some prose here that must survive.',
|
||||
'Total Phases: 2\n\nSome prose here that must survive.',
|
||||
);
|
||||
const tmpDir = setupProject(t, stateWithField);
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-feature'), { recursive: true });
|
||||
const r = runGsdTools('phase remove 2', tmpDir);
|
||||
assert.ok(r.success, `phase remove failed: ${r.error}`);
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.strictEqual(fenceLineCount(after), 2, 'single frontmatter block');
|
||||
const bodyCounts = after.match(/^Total Phases:\s*(\d+)$/gm) || [];
|
||||
assert.strictEqual(bodyCounts.length, 1, `exactly one Total Phases field; got ${bodyCounts.length}`);
|
||||
assert.match(bodyCounts[0], /^Total Phases:\s*1$/, `field decremented to 1; got ${bodyCounts[0]}`);
|
||||
});
|
||||
|
||||
test('#3572: frontmatter-less STATE.md gets the field at content start', (t) => {
|
||||
const tmpDir = setupProject(t, '# Bare state\n\nNo fences at all here.\n');
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-feature'), { recursive: true });
|
||||
const r = runGsdTools('phase remove 2', tmpDir);
|
||||
assert.ok(r.success, `phase remove failed: ${r.error}`);
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.match(after, /^Total Phases:\s*\d+$/m, 'field lands at content start when the whole file is body');
|
||||
assert.ok(after.includes('No fences at all here.'), 'original body preserved');
|
||||
});
|
||||
|
||||
test('#3572: CRLF STATE.md stays single-block with CRLF preserved', (t) => {
|
||||
const tmpDir = setupProject(t, ISSUE_STATE, '\r\n');
|
||||
let r = runGsdTools('phase insert 1 "Inserted probe"', tmpDir);
|
||||
assert.ok(r.success, `phase insert failed: ${r.error}`);
|
||||
r = runGsdTools('phase remove 1.1', tmpDir);
|
||||
assert.ok(r.success, `phase remove failed: ${r.error}`);
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.strictEqual(fenceLineCount(after), 2, `single frontmatter block under CRLF; got ${fenceLineCount(after)}`);
|
||||
assert.ok(after.includes('Some prose here that must survive.'), 'body prose preserved');
|
||||
assert.match(after, /^Total Phases:\s*\d+\r?$/m, 'count field present in body');
|
||||
});
|
||||
|
||||
test('#3572: ROADMAP-only phase removal leaves STATE.md untouched (issue control)', (t) => {
|
||||
const tmpDir = setupProject(t);
|
||||
const before = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
const r = runGsdTools('phase remove 2', tmpDir); // phase 2 has NO directory
|
||||
assert.ok(r.success, `phase remove failed: ${r.error}`);
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.strictEqual(after, before, 'issue control: removal without a directory must not touch STATE.md');
|
||||
});
|
||||
});
|
||||
|
||||
describe('bug #3572 controls and clamps', () => {
|
||||
test('#3572 control: phase insert alone leaves STATE.md untouched (issue control #2)', (t) => {
|
||||
const tmpDir = createTempProject('gsd-3572-ctl-');
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
'# Roadmap\n\n### Phase 1: A\n**Goal:** x\n\n### Phase 2: B\n**Goal:** y\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
'---\ngsd_state_version: 1.0\nprogress:\n total_phases: 2\n---\n\n# Project State\n\nBody.\n',
|
||||
);
|
||||
const before = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
const r = runGsdTools('phase insert 1 "Probe"', tmpDir);
|
||||
assert.ok(r.success, `phase insert failed: ${r.error}`);
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.strictEqual(after, before, 'issue control: insert alone must not touch STATE.md');
|
||||
t.after(() => cleanup(tmpDir));
|
||||
});
|
||||
|
||||
test('#3572 clamp: a stale Total Phases: 0 never decrements to -1 on the next removal', (t) => {
|
||||
const tmpDir = createTempProject('gsd-3572-clamp-');
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
'# Roadmap\n\n### Phase 1: A\n**Goal:** x\n\n### Phase 2: B\n**Goal:** y\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
'---\ngsd_state_version: 1.0\nprogress:\n total_phases: 2\n---\n\n# Project State\n\nTotal Phases: 0\n',
|
||||
);
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-b'), { recursive: true });
|
||||
const r = runGsdTools('phase remove 2', tmpDir);
|
||||
assert.ok(r.success, `phase remove failed: ${r.error}`);
|
||||
const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.doesNotMatch(after, /Total Phases:\s*-\d+/, 'count must never go negative');
|
||||
assert.match(after, /^Total Phases:\s*0$/m, 'stale zero stays clamped at 0');
|
||||
t.after(() => cleanup(tmpDir));
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user