fix(#2125): route complete-phase resolver through canonical parser (review)
Orthogonal review surfaced that resolvePhaseIdForCompletePhase (state.cts) and cmdStateCompletePhase's idempotency check still used an unanchored /(\d+[A-Z]?(?:\.\d+)*)/i — even more permissive than the parseProsePhaseField regex this phase fixes. Reachable corruption: after `milestone complete v0.5`, `state complete-phase` (no --phase) mined "0.5" from the body line "Phase: Milestone v0.5 complete" and rewrote STATE.md as "Phase 0.5 complete". Both sites now delegate to phase-id.cts:parsePhaseFromProse (the same anchored parser), so a milestone-closure line yields no token and the existing "unable to resolve" guard fires instead of corrupting. Canonical tokens (3, 03, 3A, 3.3, "3 of 5", "1 — Setup") are preserved unchanged. Regression (tests/state.test.cjs, complete-phase suite): `state complete-phase` on a "Milestone v0.5 complete" STATE.md now rejects and does not mine "0.5". Demonstrated fail-first. Refs #2125, #2121 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -2713,9 +2713,13 @@ function resolvePhaseIdForCompletePhase(content: string, overridePhase: string |
|
|||||||
stateExtractField(content, 'Phase') ||
|
stateExtractField(content, 'Phase') ||
|
||||||
'';
|
'';
|
||||||
|
|
||||||
// Accept canonical phase token only (e.g. 3, 03, 3A, 3.3, 10.2)
|
// #2125: parse via the canonical anchored parser so a narrative `Phase:`
|
||||||
const phaseMatch = String(candidate).match(/(\d+[A-Z]?(?:\.\d+)*)/i);
|
// body line (e.g. "Milestone v0.5 complete") does not mine a bogus token —
|
||||||
return phaseMatch ? phaseMatch[1] : null;
|
// the old unanchored regex yielded "0.5" and rewrote STATE.md as
|
||||||
|
// "Phase 0.5 complete". A canonical token at the start of the value
|
||||||
|
// (3, 03, 3A, 3.3, 10.2, "3 of 5", "1 — Setup") is preserved; a milestone
|
||||||
|
// closure line yields null, so the caller's "unable to resolve" guard fires.
|
||||||
|
return parsePhaseFromProse(candidate).phase;
|
||||||
}
|
}
|
||||||
|
|
||||||
function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string): void {
|
function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string): void {
|
||||||
@@ -2742,8 +2746,9 @@ function cmdStateCompletePhase(cwd: string, raw: boolean, overridePhase?: string
|
|||||||
// The handler is now a no-op in that case so re-invocation from downstream
|
// The handler is now a no-op in that case so re-invocation from downstream
|
||||||
// workflows cannot regress the project state.
|
// workflows cannot regress the project state.
|
||||||
const existingCurrentPhaseRaw = stateExtractField(content, 'Current Phase') || '';
|
const existingCurrentPhaseRaw = stateExtractField(content, 'Current Phase') || '';
|
||||||
const existingCurrentPhaseMatch = String(existingCurrentPhaseRaw).match(/(\d+[A-Z]?(?:\.\d+)*)/i);
|
// #2125: same canonical parser as resolvePhaseIdForCompletePhase so the two
|
||||||
const existingCurrentPhase = existingCurrentPhaseMatch ? existingCurrentPhaseMatch[1] : null;
|
// sites cannot diverge on the token they extract.
|
||||||
|
const existingCurrentPhase = parsePhaseFromProse(existingCurrentPhaseRaw).phase;
|
||||||
if (existingCurrentPhase && existingCurrentPhase !== resolvedPhase) {
|
if (existingCurrentPhase && existingCurrentPhase !== resolvedPhase) {
|
||||||
output(
|
output(
|
||||||
{ updated: [], phase: resolvedPhase, idempotent: true, note: 'phase already superseded; no-op' },
|
{ updated: [], phase: resolvedPhase, idempotent: true, note: 'phase already superseded; no-op' },
|
||||||
|
|||||||
@@ -3055,6 +3055,39 @@ describe('state complete-phase: decorated Phase fallback (#2761 nitpick)', () =>
|
|||||||
assert.ok(!after.includes('Status: Phase Phase complete'));
|
assert.ok(!after.includes('Status: Phase Phase complete'));
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('rejects a milestone-closure Phase line, never mines the version token (#2111 / #2125)', () => {
|
||||||
|
// After `milestone complete v0.5`, the only phase signal is the narrative
|
||||||
|
// `Phase: Milestone v0.5 complete`. The old unanchored resolver mined "0.5"
|
||||||
|
// and rewrote Status as "Phase 0.5 complete"; the anchored parser yields no
|
||||||
|
// token, so complete-phase must reject rather than corrupt STATE.md.
|
||||||
|
const stateMd = [
|
||||||
|
'---',
|
||||||
|
'milestone: v0.5',
|
||||||
|
'---',
|
||||||
|
'',
|
||||||
|
'# State',
|
||||||
|
'',
|
||||||
|
'**Status:** Awaiting next milestone',
|
||||||
|
'**Last Activity:** 2024-01-15',
|
||||||
|
'',
|
||||||
|
'## Current Position',
|
||||||
|
'',
|
||||||
|
'Phase: Milestone v0.5 complete',
|
||||||
|
'',
|
||||||
|
].join('\n');
|
||||||
|
const statePath = path.join(tmpDir, '.planning', 'STATE.md');
|
||||||
|
fs.writeFileSync(statePath, stateMd);
|
||||||
|
|
||||||
|
const result = runGsdTools('state complete-phase', tmpDir);
|
||||||
|
assert.ok(result.success, 'command should return JSON error payload, not crash');
|
||||||
|
const output = JSON.parse(result.output);
|
||||||
|
assert.ok(output.error, 'expected a resolution error, not a phase mined from the version string');
|
||||||
|
|
||||||
|
const after = fs.readFileSync(statePath, 'utf-8');
|
||||||
|
assert.ok(!after.includes('Phase 0.5 complete'), `must not mine "0.5" from the version: ${after}`);
|
||||||
|
assert.ok(!after.includes('Phase: 0.5'), `must not rewrite Current Position to Phase 0.5: ${after}`);
|
||||||
|
});
|
||||||
|
|
||||||
test('supports explicit phase override for complete-phase disambiguation (#3063)', () => {
|
test('supports explicit phase override for complete-phase disambiguation (#3063)', () => {
|
||||||
const stateMd = [
|
const stateMd = [
|
||||||
'---',
|
'---',
|
||||||
|
|||||||
Reference in New Issue
Block a user