diff --git a/.changeset/eager-wasps-travel.md b/.changeset/eager-wasps-travel.md new file mode 100644 index 000000000..f339902ea --- /dev/null +++ b/.changeset/eager-wasps-travel.md @@ -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) diff --git a/src/phase.cts b/src/phase.cts index 0fd5a9d67..60ec38a02 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -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): 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; diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 93d2dd85c..7738bc936 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -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)); + }); +});