fix(3601): preserve peer-depth decimal phases on integer phase removal
`phase remove N` for an integer phase silently deleted the adjacent
`### Phase N.1:` decimal section when the decimal was a peer-depth
heading. The bug was in the section-removal regex inside
get-shit-done/bin/lib/phase.cjs:updateRoadmapAfterPhaseRemoval:
(?=\n#{2,4}\s+Phase\s+\d+\s*:|$)
The lookahead required the next header's digits to be followed by
`\s*:` — true for `### Phase 3:` but false for `### Phase 2.1:` because
the `.1` breaks the match. The non-greedy `[\s\S]*?` body then consumed
`Phase 2.1` along with `Phase 2` until it found the next integer
header. The on-disk phase directory `.planning/phases/02.1-*` survived
but its ROADMAP entry was gone — disk/ROADMAP inconsistency.
The fix uses a depth-aware lookahead: capture the hash count of the
header being removed with a named group `(?<h>#{2,4})` and require the
end-of-section lookahead to match the SAME depth via `\k<h>(?!#)`. The
`(?!#)` guards against `###` accidentally matching a deeper `####`
header by anchoring on the captured hash count.
This preserves two contracts simultaneously:
- #3601: removing `### Phase 2:` (depth 3) stops at the next depth-3
header, including `### Phase 2.1:` — the peer-level decimal is
preserved.
- #3355: removing `### Phase 27:` (depth 3) continues past
`#### Phase 27.1:` (depth 4, a child of the integer phase) until it
reaches the next depth-3 header. The child decimal is part of the
integer phase being removed.
The regression test exercises the public CLI via runGsdTools and
asserts on typed JSON output from `roadmap get-phase --json` — no raw
text matching on ROADMAP.md content (per CONTRIBUTING.md
"Prohibited: Raw Text Matching on Test Outputs").
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
5
.changeset/3601-phase-remove-decimal-section-loss.md
Normal file
5
.changeset/3601-phase-remove-decimal-section-loss.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
issue: 3601
|
||||
---
|
||||
**`phase remove N` no longer collapses adjacent peer-depth decimal phases** — `updateRoadmapAfterPhaseRemoval` in `get-shit-done/bin/lib/phase.cjs` now uses a depth-aware end-of-section lookahead that captures the hash count of the header being removed and stops only at a subsequent header of the SAME depth. Previously the lookahead required the next header's digits to be followed by `\s*:`, which failed on `### Phase 2.1:` (the `.1` blocks the match) and silently consumed the decimal sibling along with the integer phase. The fix preserves `### Phase 2.1:` (peer-depth decimal) while still removing `#### Phase 27.1:` (child-depth decimal under `### Phase 27:`).
|
||||
@@ -886,7 +886,24 @@ function updateRoadmapAfterPhaseRemoval(roadmapPath, targetPhase, isDecimal, rem
|
||||
let content = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
const escaped = escapeRegex(targetPhase);
|
||||
|
||||
content = content.replace(new RegExp(`\\n?#{2,4}\\s*Phase\\s+${escaped}\\s*:[\\s\\S]*?(?=\\n#{2,4}\\s+Phase\\s+\\d+\\s*:|$)`, 'i'), '');
|
||||
// #3601: the end-of-section lookahead is depth-aware. It captures the
|
||||
// hash count of the header being removed and stops only at a subsequent
|
||||
// header of the SAME depth, whether integer or decimal. This preserves
|
||||
// two existing contracts:
|
||||
//
|
||||
// (#3601 case) Remove `### Phase 2:` and stop at `### Phase 2.1:` —
|
||||
// `Phase 2.1` is a peer-level decimal phase (depth 3) and must be
|
||||
// preserved.
|
||||
//
|
||||
// (#3355 case) Remove `### Phase 27:` and continue past
|
||||
// `#### Phase 27.1:` (depth 4 — a child of Phase 27) until the next
|
||||
// depth-3 header. The child decimal is part of the integer phase
|
||||
// being removed.
|
||||
//
|
||||
// The `(?!#)` negative lookahead after the backreference prevents the
|
||||
// depth-3 match from being satisfied by a depth-4+ header that starts
|
||||
// with the same three hashes.
|
||||
content = content.replace(new RegExp(`\\n?(?<h>#{2,4})\\s*Phase\\s+${escaped}\\s*:[\\s\\S]*?(?=\\n\\k<h>(?!#)\\s+Phase\\s+\\d+(?:\\.\\d+)*\\s*:|$)`, 'i'), '');
|
||||
content = content.replace(new RegExp(`\\n?-\\s*\\[[ x]\\]\\s*.*Phase\\s+${escaped}[:\\s][^\\n]*`, 'gi'), '');
|
||||
content = content.replace(new RegExp(`\\n?\\|\\s*${escaped}\\.?\\s[^|]*\\|[^\\n]*`, 'gi'), '');
|
||||
|
||||
|
||||
159
tests/bug-3601-phase-remove-preserves-decimal-sections.test.cjs
Normal file
159
tests/bug-3601-phase-remove-preserves-decimal-sections.test.cjs
Normal file
@@ -0,0 +1,159 @@
|
||||
/**
|
||||
* Bug #3601: `phase remove N` for an integer phase can also delete the
|
||||
* adjacent decimal phase section (`### Phase N.1:`) when the decimal is a
|
||||
* peer-level header at the same depth as the integer being removed.
|
||||
*
|
||||
* Root cause: the section-removal regex in
|
||||
* `get-shit-done/bin/lib/phase.cjs:updateRoadmapAfterPhaseRemoval` used a
|
||||
* depth-blind lookahead (`(?=\n#{2,4}\s+Phase\s+\d+\s*:|$)`) that required
|
||||
* the next header's digits to be followed by `\s*:`. `### Phase 2.1:`
|
||||
* (depth 3, decimal) did not satisfy `\d+\s*:` because of the `.1`, so
|
||||
* the non-greedy match consumed `Phase 2.1` along with `Phase 2` until it
|
||||
* reached the next integer header.
|
||||
*
|
||||
* The fix makes the lookahead depth-aware: it captures the hash count of
|
||||
* the header being removed and stops only at a subsequent header of the
|
||||
* SAME depth, integer or decimal. That preserves the #3355 contract
|
||||
* (`#### Phase 27.1:` at depth 4 is a CHILD of `### Phase 27:` at depth 3
|
||||
* and must be removed alongside it) while fixing the #3601 contract
|
||||
* (`### Phase 2.1:` at depth 3 is a PEER of `### Phase 2:` and must be
|
||||
* preserved).
|
||||
*
|
||||
* Assertions go through the typed `roadmap get-phase --json` query so no
|
||||
* test asserts on raw ROADMAP.md text content.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
process.env.GSD_TEST_MODE = '1';
|
||||
|
||||
const { describe, test, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
function writeRoadmap(tmpDir, body) {
|
||||
fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), body);
|
||||
}
|
||||
function writeState(tmpDir, version) {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'STATE.md'),
|
||||
`---\nmilestone: ${version}\n---\n`,
|
||||
);
|
||||
}
|
||||
function ensurePhaseDir(tmpDir, name) {
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', name), { recursive: true });
|
||||
}
|
||||
function getPhase(tmpDir, phaseNum) {
|
||||
const r = runGsdTools(['roadmap', 'get-phase', phaseNum, '--json'], tmpDir);
|
||||
if (!r.success) return { found: false, error: r.error };
|
||||
return JSON.parse(r.output);
|
||||
}
|
||||
|
||||
describe('bug #3601: phase remove preserves peer-depth decimal sections', () => {
|
||||
let tmpDir;
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject('bug-3601-');
|
||||
});
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('removing Phase 2 preserves peer-depth Phase 2.1 and renumbers Phase 3 → 2', () => {
|
||||
writeState(tmpDir, 'v1.0.0');
|
||||
writeRoadmap(
|
||||
tmpDir,
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'## Current Milestone: v1.0.0 - Test',
|
||||
'',
|
||||
'### Phase 2: Parent',
|
||||
'**Goal:** RemoveMeGoal',
|
||||
'',
|
||||
'### Phase 2.1: Follow-up',
|
||||
'**Goal:** PreserveDecimalGoal',
|
||||
'',
|
||||
'### Phase 3: Trailing',
|
||||
'**Goal:** PreserveTrailingGoal',
|
||||
'',
|
||||
].join('\n'),
|
||||
);
|
||||
ensurePhaseDir(tmpDir, '02-parent');
|
||||
ensurePhaseDir(tmpDir, '02.1-follow-up');
|
||||
ensurePhaseDir(tmpDir, '03-trailing');
|
||||
|
||||
const r = runGsdTools(['phase', 'remove', '2'], tmpDir);
|
||||
assert.ok(r.success, `phase remove failed: ${r.error || r.output}`);
|
||||
|
||||
// The peer-depth decimal (Phase 2.1) must still be queryable — its
|
||||
// unique goal proves the section body survived.
|
||||
const decimal = getPhase(tmpDir, '2.1');
|
||||
assert.strictEqual(decimal.found, true, 'Phase 2.1 deleted alongside Phase 2');
|
||||
assert.strictEqual(decimal.phase_name, 'Follow-up');
|
||||
assert.strictEqual(decimal.goal, 'PreserveDecimalGoal');
|
||||
|
||||
// Phase 3 must have been renumbered to Phase 2.
|
||||
const renumbered = getPhase(tmpDir, '2');
|
||||
assert.strictEqual(renumbered.found, true);
|
||||
assert.strictEqual(renumbered.phase_name, 'Trailing');
|
||||
assert.strictEqual(
|
||||
renumbered.goal,
|
||||
'PreserveTrailingGoal',
|
||||
'Phase 3 → Phase 2 renumber did not carry the right section content',
|
||||
);
|
||||
|
||||
// The removed Parent goal must not be retrievable from any current phase.
|
||||
const parentLookup = getPhase(tmpDir, '3');
|
||||
assert.notStrictEqual(
|
||||
parentLookup.goal,
|
||||
'RemoveMeGoal',
|
||||
'removed parent goal reappeared under a phase header',
|
||||
);
|
||||
});
|
||||
|
||||
test('removing Phase 5 preserves Phase 5.1 and Phase 5.2 (multiple peer decimals)', () => {
|
||||
writeState(tmpDir, 'v1.0.0');
|
||||
writeRoadmap(
|
||||
tmpDir,
|
||||
[
|
||||
'# Roadmap',
|
||||
'',
|
||||
'## Current Milestone: v1.0.0 - Test',
|
||||
'',
|
||||
'### Phase 5: Parent',
|
||||
'**Goal:** RemoveParent',
|
||||
'',
|
||||
'### Phase 5.1: First child',
|
||||
'**Goal:** ChildAGoal',
|
||||
'',
|
||||
'### Phase 5.2: Second child',
|
||||
'**Goal:** ChildBGoal',
|
||||
'',
|
||||
'### Phase 6: Tail',
|
||||
'**Goal:** TailGoal',
|
||||
'',
|
||||
].join('\n'),
|
||||
);
|
||||
ensurePhaseDir(tmpDir, '05-parent');
|
||||
ensurePhaseDir(tmpDir, '05.1-first-child');
|
||||
ensurePhaseDir(tmpDir, '05.2-second-child');
|
||||
ensurePhaseDir(tmpDir, '06-tail');
|
||||
|
||||
const r = runGsdTools(['phase', 'remove', '5'], tmpDir);
|
||||
assert.ok(r.success);
|
||||
|
||||
const decimalA = getPhase(tmpDir, '5.1');
|
||||
assert.strictEqual(decimalA.found, true, 'Phase 5.1 deleted');
|
||||
assert.strictEqual(decimalA.goal, 'ChildAGoal');
|
||||
|
||||
const decimalB = getPhase(tmpDir, '5.2');
|
||||
assert.strictEqual(decimalB.found, true, 'Phase 5.2 deleted');
|
||||
assert.strictEqual(decimalB.goal, 'ChildBGoal');
|
||||
|
||||
const tail = getPhase(tmpDir, '5');
|
||||
assert.strictEqual(tail.found, true);
|
||||
assert.strictEqual(tail.goal, 'TailGoal', 'Phase 6 → Phase 5 renumber misfired');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user