* fix(#3350): prefer lowest outstanding phase over positional next in phase complete * chore(#3350): add changeset fragment * chore(#3350): backfill changeset pr field --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/sturdy-hawks-munch.md
Normal file
5
.changeset/sturdy-hawks-munch.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3482
|
||||
---
|
||||
phase complete now selects the lowest genuinely-outstanding lower-numbered phase as next_phase instead of a merely-positionally-next higher phase heading, and keeps STATE.md frontmatter current_phase and current_phase_name paired (both describe the same phase) even for narrative-prose STATE.md files
|
||||
@@ -2899,7 +2899,17 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
// pattern mirrors the sibling phasePattern's anchoring (only whitespace/bold
|
||||
// between the box and "Phase", a required `:`) so unrelated checklist lines
|
||||
// that merely mention "Phase N" don't match.
|
||||
if (isLastPhase && roadmapContent !== null) {
|
||||
// #3350: this stage answers a DIFFERENT question than stages 1-2 ("what is
|
||||
// the next actionable phase?" vs "is this the last phase?"), so it must not
|
||||
// be gated on their answer. Gating on isLastPhase let a merely-positionally
|
||||
// next higher heading (stage 2) permanently mask a genuinely-outstanding
|
||||
// lower phase — stage 2 cleared isLastPhase and this scan never ran. The
|
||||
// scan already refuses anything not strictly lower than the completed phase
|
||||
// (plus sentinels, #2949), so running it unconditionally cannot manufacture
|
||||
// a wrong answer: when no lower phase is outstanding it finds nothing and
|
||||
// stages 1-2's pick stands unchanged; in the masking case isLastPhase is
|
||||
// already false, so the last-phase signal has no reachable regression.
|
||||
if (roadmapContent !== null) {
|
||||
try {
|
||||
const milestoneScope = extractCurrentMilestone(roadmapContent, cwd);
|
||||
const cbPattern = new RegExp(
|
||||
@@ -2988,11 +2998,27 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
|
||||
// the intent; pass it as authoritative so the sync's prose
|
||||
// re-derivation cannot rewrite current_phase_name to the name's own
|
||||
// parenthetical (`Closer-ruling measurement (D1a)` → `D1a`).
|
||||
stateContent = syncStateFrontmatter(
|
||||
stateContent,
|
||||
cwd,
|
||||
nextPhaseDisplayName ? { current_phase_name: nextPhaseDisplayName } : undefined,
|
||||
);
|
||||
// #3350: PAIR the override. When STATE.md's body carries no Current
|
||||
// Phase / Phase field to re-derive from (narrative prose), the #905
|
||||
// preserve guard in syncStateFrontmatter keeps the OLD frontmatter
|
||||
// current_phase while the authoritative current_phase_name advances —
|
||||
// leaving the two fields describing different phases. Pin BOTH to the
|
||||
// resolved next phase in that case. When the body DOES carry the field
|
||||
// (completePhaseCore just rewrote it), stay name-only so the body's
|
||||
// richer `N of T (name)` derived shape survives the sync.
|
||||
const fmBody = frontmatterMod.stripFrontmatter(stateContent);
|
||||
const bodyHasPhaseField =
|
||||
stateExtractField(fmBody, 'Current Phase') != null ||
|
||||
stateExtractField(fmBody, 'Phase') != null;
|
||||
const authoritativeFm: Record<string, string> | undefined = nextPhaseDisplayName
|
||||
? bodyHasPhaseField || !nextPhaseNum
|
||||
? { current_phase_name: nextPhaseDisplayName }
|
||||
: {
|
||||
current_phase: String(nextPhaseNum),
|
||||
current_phase_name: nextPhaseDisplayName,
|
||||
}
|
||||
: undefined;
|
||||
stateContent = syncStateFrontmatter(stateContent, cwd, authoritativeFm);
|
||||
|
||||
writes.push({ filePath: statePath, before: originalStateContent, after: stateContent });
|
||||
}
|
||||
|
||||
@@ -5232,8 +5232,14 @@ describe('phase complete excludes 999.x backlog from next-phase (#2129)', () =>
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
|
||||
const output = JSON.parse(result.output);
|
||||
// Should find phase 3 from roadmap, NOT 999.1 from filesystem
|
||||
assert.strictEqual(output.next_phase, '3', 'next_phase should be 3, not 999.1');
|
||||
// #3350: with stage 3 (#2028 lowest-outstanding) no longer gated behind
|
||||
// stages 1-2 missing, the unchecked Phase 1 row outranks the positionally-
|
||||
// next Phase 3 heading — a phase is complete iff its roadmap checkbox is
|
||||
// `[x]` (#2028), and Phase 1's row is unchecked despite its dir on disk
|
||||
// (same drift shape #2949's realLowerOutstandingPhaseStillSelected pins).
|
||||
// The #2129 contract itself is unchanged: 999.x is NEVER selected.
|
||||
assert.notEqual(output.next_phase, '999.1', '999.x backlog must never be next_phase');
|
||||
assert.strictEqual(output.next_phase, '1', 'lowest unchecked phase (1) wins over positional 3 (#3350); never 999.1');
|
||||
assert.strictEqual(output.is_last_phase, false, 'should not be last phase');
|
||||
});
|
||||
});
|
||||
@@ -10967,3 +10973,289 @@ describe('phase complete stage-3 sentinel filter (#2949)', () => {
|
||||
});
|
||||
}
|
||||
})();
|
||||
|
||||
// #3350 — phase complete: a merely-positionally-next higher phase heading
|
||||
// (stage 2) must not mask a genuinely-outstanding lower phase (stage 3), and
|
||||
// STATE.md frontmatter's current_phase/current_phase_name must stay paired.
|
||||
// Matrix: .gsd/bug/fix-3350-phase-complete-name-desync/50-test-matrix.md
|
||||
describe('phase complete lowest-outstanding vs positional-next (#3350)', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject('gsd-3350-');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
/** Scaffold a phase dir with one executed plan (PLAN + SUMMARY). */
|
||||
function scaffoldPhase(slug, planNum) {
|
||||
const dir = path.join(tmpDir, '.planning', 'phases', slug);
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
const padded = String(planNum).padStart(2, '0');
|
||||
fs.writeFileSync(path.join(dir, `${padded}-01-PLAN.md`), '# Plan');
|
||||
fs.writeFileSync(path.join(dir, `${padded}-01-SUMMARY.md`), '# Summary');
|
||||
}
|
||||
|
||||
const statePath = () => path.join(tmpDir, '.planning', 'STATE.md');
|
||||
|
||||
/**
|
||||
* Row-4 roadmap: phase 5 is completed (dir on disk); phases 3 and 4 are
|
||||
* outstanding (unchecked, never executed, no dirs — optional); higher
|
||||
* headings 6 and 7 are pre-declared with no dirs (stage-1 miss, stage-2 hit).
|
||||
*/
|
||||
function writeRow4Roadmap({ withLowerDirs = false, withHigherDir = false } = {}) {
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
`# Roadmap
|
||||
|
||||
- [ ] **Phase 3: Attestation Freeze**
|
||||
- [ ] **Phase 4: Corpus Maturation**
|
||||
- [ ] **Phase 5: Harness Mechanization**
|
||||
- [ ] **Phase 6: Oracle Re-Hardening**
|
||||
- [ ] **Phase 7: Recall Monitoring**
|
||||
|
||||
### Phase 3: Attestation Freeze
|
||||
**Goal:** three
|
||||
**Plans:** 1 plans
|
||||
|
||||
### Phase 4: Corpus Maturation
|
||||
**Goal:** four
|
||||
**Plans:** 1 plans
|
||||
|
||||
### Phase 5: Harness Mechanization
|
||||
**Goal:** five
|
||||
**Plans:** 1 plans
|
||||
|
||||
### Phase 6: Oracle Re-Hardening
|
||||
**Goal:** six
|
||||
**Plans:** 1 plans
|
||||
|
||||
### Phase 7: Recall Monitoring
|
||||
**Goal:** seven
|
||||
**Plans:** 1 plans
|
||||
`,
|
||||
);
|
||||
scaffoldPhase('05-harness-mechanization', 5);
|
||||
if (withLowerDirs) {
|
||||
scaffoldPhase('03-attestation-freeze', 3);
|
||||
scaffoldPhase('04-corpus-maturation', 4);
|
||||
}
|
||||
if (withHigherDir) {
|
||||
scaffoldPhase('06-oracle-re-hardening', 6);
|
||||
}
|
||||
}
|
||||
|
||||
/** Explicit-field STATE.md (body fields present — the 0xdhx fixture shape). */
|
||||
function writeExplicitState() {
|
||||
fs.writeFileSync(
|
||||
statePath(),
|
||||
`# State
|
||||
|
||||
**Current Phase:** 5
|
||||
**Current Phase Name:** Harness Mechanization
|
||||
**Status:** In progress
|
||||
**Current Plan:** 05-01
|
||||
**Last Activity:** 2025-01-01
|
||||
**Last Activity Description:** Working on phase 5
|
||||
`,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Narrative-prose STATE.md (NO body fields) with frontmatter parked on the
|
||||
* just-completed phase — the filed #3350 shape: without the pairing override
|
||||
* current_phase stays at 5 while current_phase_name advances.
|
||||
*/
|
||||
function writeNarrativeState() {
|
||||
fs.writeFileSync(
|
||||
statePath(),
|
||||
`---
|
||||
current_phase: 5
|
||||
current_phase_name: Harness Mechanization
|
||||
---
|
||||
|
||||
# State
|
||||
|
||||
We are wrapping up phase 5 of the milestone; the next actionable phase is
|
||||
still to be determined by the roadmap.
|
||||
`,
|
||||
);
|
||||
}
|
||||
|
||||
function parseFrontmatterField(content, key) {
|
||||
const m = content.match(new RegExp(`^${key}:\\s*(.*)$`, 'm'));
|
||||
return m ? m[1].trim().replace(/^["']|["']$/g, '') : null;
|
||||
}
|
||||
|
||||
test('lowerOutstandingBeatsHigherHeading', () => {
|
||||
// Row 4 (failing-first): completing 5 with outstanding 3/4 AND pre-declared
|
||||
// higher headings 6/7 must pick the lowest outstanding phase (3), not the
|
||||
// positionally-next heading (6).
|
||||
writeRow4Roadmap();
|
||||
writeExplicitState();
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
|
||||
assert.strictEqual(output.completed_phase, '5');
|
||||
assert.strictEqual(output.is_last_phase, false, 'a higher phase exists — not last');
|
||||
assert.strictEqual(
|
||||
parseInt(String(output.next_phase), 10),
|
||||
3,
|
||||
`lowest outstanding phase 3 must be next_phase (got ${output.next_phase})`,
|
||||
);
|
||||
assert.strictEqual(output.next_phase_name, 'attestation-freeze');
|
||||
});
|
||||
|
||||
test('narrativeStateFrontmatterPairing', () => {
|
||||
// Row 4 + acceptance #2/#5 (failing-first): with a narrative STATE.md and
|
||||
// frontmatter parked on the completed phase, BOTH frontmatter fields must
|
||||
// describe the resolved next phase (3) after the write — never a split.
|
||||
writeRow4Roadmap();
|
||||
writeNarrativeState();
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(parseInt(String(output.next_phase), 10), 3);
|
||||
|
||||
const state = fs.readFileSync(statePath(), 'utf-8');
|
||||
const fmPhase = parseFrontmatterField(state, 'current_phase');
|
||||
const fmName = parseFrontmatterField(state, 'current_phase_name');
|
||||
assert.ok(fmPhase !== null, 'frontmatter current_phase must exist');
|
||||
assert.ok(fmName !== null, 'frontmatter current_phase_name must exist');
|
||||
assert.strictEqual(
|
||||
parseInt(fmPhase, 10),
|
||||
3,
|
||||
`current_phase must advance to the resolved next phase 3 (got ${fmPhase})`,
|
||||
);
|
||||
assert.match(fmName, /attestation freeze/i, `current_phase_name must name phase 3 (got ${fmName})`);
|
||||
});
|
||||
|
||||
test('higherStillWinsWhenNoLowerOutstanding', () => {
|
||||
// Row 2 non-regression (acceptance #4): N+k really is the correct next phase
|
||||
// when no lower phase is genuinely outstanding.
|
||||
writeRow4Roadmap();
|
||||
writeExplicitState();
|
||||
// Check phases 3 and 4 off — the higher heading is then the right answer.
|
||||
const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md');
|
||||
fs.writeFileSync(
|
||||
roadmapPath,
|
||||
fs.readFileSync(roadmapPath, 'utf-8')
|
||||
.replace('- [ ] **Phase 3: Attestation Freeze**', '- [x] **Phase 3: Attestation Freeze**')
|
||||
.replace('- [ ] **Phase 4: Corpus Maturation**', '- [x] **Phase 4: Corpus Maturation**'),
|
||||
);
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(parseInt(String(output.next_phase), 10), 6, 'positionally-next 6 wins when no lower phase is outstanding');
|
||||
assert.strictEqual(output.is_last_phase, false);
|
||||
});
|
||||
|
||||
test('lowerOnlyStillSelected', () => {
|
||||
// Row 3 non-regression (#2028 original shape): lower outstanding, no higher.
|
||||
writeRow4Roadmap();
|
||||
writeExplicitState();
|
||||
const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md');
|
||||
let roadmap = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
roadmap = roadmap
|
||||
.replace('- [ ] **Phase 6: Oracle Re-Hardening**\n', '')
|
||||
.replace('- [ ] **Phase 7: Recall Monitoring**\n', '')
|
||||
.replace('### Phase 6: Oracle Re-Hardening\n**Goal:** six\n**Plans:** 1 plans\n\n', '')
|
||||
.replace('### Phase 7: Recall Monitoring\n**Goal:** seven\n**Plans:** 1 plans\n', '');
|
||||
|
||||
fs.writeFileSync(roadmapPath, roadmap);
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(parseInt(String(output.next_phase), 10), 3, '#2028 behavior preserved');
|
||||
assert.strictEqual(output.is_last_phase, false);
|
||||
});
|
||||
|
||||
test('nothingOutstandingStillCompletesMilestone', () => {
|
||||
// Row 1 non-regression: no higher phase (heading or dir) and no unchecked
|
||||
// lower phase → is_last_phase stays true. Higher HEADINGS alone keep
|
||||
// is_last_phase false (stage 2 matches headings regardless of checkbox
|
||||
// state), so this fixture drops them entirely.
|
||||
writeRow4Roadmap();
|
||||
writeExplicitState();
|
||||
const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md');
|
||||
let roadmap = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
roadmap = roadmap
|
||||
.replace('- [ ] **Phase 3: Attestation Freeze**', '- [x] **Phase 3: Attestation Freeze**')
|
||||
.replace('- [ ] **Phase 4: Corpus Maturation**', '- [x] **Phase 4: Corpus Maturation**')
|
||||
.replace('- [ ] **Phase 6: Oracle Re-Hardening**\n', '')
|
||||
.replace('- [ ] **Phase 7: Recall Monitoring**\n', '')
|
||||
.replace('### Phase 6: Oracle Re-Hardening\n**Goal:** six\n**Plans:** 1 plans\n\n', '')
|
||||
.replace('### Phase 7: Recall Monitoring\n**Goal:** seven\n**Plans:** 1 plans\n', '');
|
||||
fs.writeFileSync(roadmapPath, roadmap);
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.is_last_phase, true, 'nothing outstanding — milestone completes');
|
||||
assert.strictEqual(output.next_phase, null);
|
||||
});
|
||||
|
||||
test('sentinelStillExcludedWithHigherPresent', () => {
|
||||
// Acceptance #3: the #2949 sentinel filter keeps working alongside the
|
||||
// ungate — an unchecked 0.x backlog row never becomes next_phase.
|
||||
writeRow4Roadmap();
|
||||
writeExplicitState();
|
||||
const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md');
|
||||
fs.writeFileSync(
|
||||
roadmapPath,
|
||||
'- [ ] **Phase 0.1: Backlog sentinel item**\n' + fs.readFileSync(roadmapPath, 'utf-8'),
|
||||
);
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(
|
||||
parseInt(String(output.next_phase), 10),
|
||||
3,
|
||||
`real phase 3 (not the 0.1 sentinel) is next_phase (got ${output.next_phase})`,
|
||||
);
|
||||
});
|
||||
|
||||
test('explicitBodyFieldsMoveTogether', () => {
|
||||
// Row 4 explicit-field variant (0xdhx fixture A): the body fields AND the
|
||||
// frontmatter must move to phase 3 together — never both-wrong on 6.
|
||||
writeRow4Roadmap();
|
||||
writeExplicitState();
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(parseInt(String(output.next_phase), 10), 3);
|
||||
|
||||
const state = fs.readFileSync(statePath(), 'utf-8');
|
||||
assert.ok(/\*\*Current Phase:\*\*\s*3\b/.test(state), `body Current Phase must be 3, got: ${state.match(/\*\*Current Phase:\*\*.*/)?.[0]}`);
|
||||
assert.match(state, /\*\*Current Phase Name:\*\*\s*Attestation Freeze/);
|
||||
const fmPhase = parseFrontmatterField(state, 'current_phase');
|
||||
const fmName = parseFrontmatterField(state, 'current_phase_name');
|
||||
assert.strictEqual(parseInt(fmPhase, 10), 3, `frontmatter current_phase must derive to 3 (got ${fmPhase})`);
|
||||
assert.match(fmName, /attestation freeze/i, `frontmatter current_phase_name must name phase 3 (got ${fmName})`);
|
||||
});
|
||||
|
||||
test('diskPresentHigherStillLosesToLowerOutstanding', () => {
|
||||
// Row 4 stage-1 variant (failing-first): a higher phase directory ON DISK
|
||||
// (stage-1 hit) also must not mask an outstanding lower phase.
|
||||
writeRow4Roadmap({ withHigherDir: true });
|
||||
writeExplicitState();
|
||||
|
||||
const result = runVerifiedPhaseComplete('phase complete 5', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error || result.output}`);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(
|
||||
parseInt(String(output.next_phase), 10),
|
||||
3,
|
||||
`lowest outstanding phase 3 beats the on-disk higher phase 6 (got ${output.next_phase})`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user