* fix(#3127): make state.begin-phase idempotent on mid-flight phases Root cause: cmdStateBeginPhase() unconditionally overwrote execution- progress fields regardless of current phase status. When execute-phase called it on a phase already mid-flight (--wave N resume), it regressed: - Current Plan to 1 (from e.g. 3) - Last Activity Description to 'context gathered; ready for plan-phase' - Plan: N of M body line to 'Plan: 1 of M' - last_updated timestamp to an older value - progress.percent could decrease Fix: read Status field before writing. If phase is already executing (Status: Executing Phase N), skip execution-progress fields and only update fields safe on resume: - Last Activity date (always safe) - Resume-specific 'execution resumed (wave continue)' activity line First-time execution (Status != Executing Phase N) writes all fields as before -- no regression on the normal path. Regression test: 4 real unit tests using synthetic STATE.md files: - mid-flight phase does not reset Current Plan (was the bug) - mid-flight phase does not overwrite stopped_at narrative - fresh phase sets Current Plan to 1 (normal path, no regression) - both paths update Last Activity date (safe field) Suite: 6990/6990. Closes #3127. * fix(lint+state): allow-test-rule, escapeRegex phaseNumber in idempotency guard
This commit is contained in:
Tom Boucher
5
.changeset/fix-3127-state-begin-phase-idempotent.md
Normal file
5
.changeset/fix-3127-state-begin-phase-idempotent.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3127
|
||||
---
|
||||
**`state.begin-phase` is now idempotent** — when called on a phase already in-flight (e.g. `--wave N` resume), it no longer overwrites `Current Plan`, `stopped_at` narrative, `Plan: N of M` body line, or `Last Activity Description` with stale values from the last `plan-phase` run. An idempotency guard reads the current `Status` field before writing: if it already contains `Executing Phase N`, only the `Last Activity` date and a resume-specific activity line are updated; all execution-progress fields are preserved. First-time execution (Status ≠ Executing) continues to write all fields as before. Closes #3127.
|
||||
@@ -1032,87 +1032,113 @@ function cmdStateBeginPhase(cwd, phaseNumber, phaseName, planCount, raw) {
|
||||
const updated = [];
|
||||
|
||||
readModifyWriteStateMd(statePath, (content) => {
|
||||
// Idempotency guard (#3127): if the phase is already mid-flight, do NOT
|
||||
// overwrite execution-progress fields (Current Plan, plan body line,
|
||||
// Last Activity Description). Only update fields that are safe to
|
||||
// refresh on resume (Last Activity date, Status if inconsistent).
|
||||
// A phase is considered mid-flight when Status contains 'Executing Phase N'
|
||||
// for the current phase number.
|
||||
const currentStatus = stateExtractField(content, 'Status') || '';
|
||||
const isAlreadyExecuting = new RegExp(`Executing Phase\\s+${escapeRegex(String(phaseNumber))}\\b`, 'i').test(currentStatus);
|
||||
|
||||
// Update Status field
|
||||
const statusValue = `Executing Phase ${phaseNumber}`;
|
||||
let result = stateReplaceField(content, 'Status', statusValue);
|
||||
if (result) { content = result; updated.push('Status'); }
|
||||
|
||||
// Update Last Activity
|
||||
// Update Last Activity (safe to update on resume — tracks when execute-phase ran)
|
||||
result = stateReplaceField(content, 'Last Activity', today);
|
||||
if (result) { content = result; updated.push('Last Activity'); }
|
||||
|
||||
// Update Last Activity Description if it exists
|
||||
const activityDesc = `Phase ${phaseNumber} execution started`;
|
||||
result = stateReplaceField(content, 'Last Activity Description', activityDesc);
|
||||
if (result) { content = result; updated.push('Last Activity Description'); }
|
||||
if (!isAlreadyExecuting) {
|
||||
// First-time execution: set all progress fields
|
||||
|
||||
// Update Current Phase
|
||||
result = stateReplaceField(content, 'Current Phase', String(phaseNumber));
|
||||
if (result) { content = result; updated.push('Current Phase'); }
|
||||
// Update Last Activity Description
|
||||
const activityDesc = `Phase ${phaseNumber} execution started`;
|
||||
result = stateReplaceField(content, 'Last Activity Description', activityDesc);
|
||||
if (result) { content = result; updated.push('Last Activity Description'); }
|
||||
|
||||
// Update Current Phase Name
|
||||
if (phaseName) {
|
||||
result = stateReplaceField(content, 'Current Phase Name', phaseName);
|
||||
if (result) { content = result; updated.push('Current Phase Name'); }
|
||||
}
|
||||
// Update Current Phase
|
||||
result = stateReplaceField(content, 'Current Phase', String(phaseNumber));
|
||||
if (result) { content = result; updated.push('Current Phase'); }
|
||||
|
||||
// Update Current Plan to 1 (starting from the first plan)
|
||||
result = stateReplaceField(content, 'Current Plan', '1');
|
||||
if (result) { content = result; updated.push('Current Plan'); }
|
||||
|
||||
// Update Total Plans in Phase
|
||||
if (planCount) {
|
||||
result = stateReplaceField(content, 'Total Plans in Phase', String(planCount));
|
||||
if (result) { content = result; updated.push('Total Plans in Phase'); }
|
||||
}
|
||||
|
||||
// Update **Current focus:** body text line (#1104)
|
||||
const focusLabel = phaseName ? `Phase ${phaseNumber} — ${phaseName}` : `Phase ${phaseNumber}`;
|
||||
const focusPattern = /(\*\*Current focus:\*\*\s*).*/i;
|
||||
if (focusPattern.test(content)) {
|
||||
content = content.replace(focusPattern, (_match, prefix) => `${prefix}${focusLabel}`);
|
||||
updated.push('Current focus');
|
||||
}
|
||||
|
||||
// Update ## Current Position section (#1104, #1365)
|
||||
// Update individual fields within Current Position instead of replacing the
|
||||
// entire section, so that Status, Last activity, and Progress are preserved.
|
||||
const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i;
|
||||
const positionMatch = content.match(positionPattern);
|
||||
if (positionMatch) {
|
||||
const header = positionMatch[1];
|
||||
let posBody = positionMatch[2];
|
||||
|
||||
// Update or insert Phase line
|
||||
const newPhase = `Phase: ${phaseNumber}${phaseName ? ` (${phaseName})` : ''} — EXECUTING`;
|
||||
if (/^Phase:/m.test(posBody)) {
|
||||
posBody = posBody.replace(/^Phase:.*$/m, newPhase);
|
||||
} else {
|
||||
posBody = newPhase + '\n' + posBody;
|
||||
// Update Current Phase Name
|
||||
if (phaseName) {
|
||||
result = stateReplaceField(content, 'Current Phase Name', phaseName);
|
||||
if (result) { content = result; updated.push('Current Phase Name'); }
|
||||
}
|
||||
|
||||
// Update or insert Plan line
|
||||
const newPlan = `Plan: 1 of ${planCount || '?'}`;
|
||||
if (/^Plan:/m.test(posBody)) {
|
||||
posBody = posBody.replace(/^Plan:.*$/m, newPlan);
|
||||
} else {
|
||||
posBody = posBody.replace(/^(Phase:.*$)/m, `$1\n${newPlan}`);
|
||||
// Update Current Plan to 1 (starting from the first plan)
|
||||
result = stateReplaceField(content, 'Current Plan', '1');
|
||||
if (result) { content = result; updated.push('Current Plan'); }
|
||||
|
||||
// Update Total Plans in Phase
|
||||
if (planCount) {
|
||||
result = stateReplaceField(content, 'Total Plans in Phase', String(planCount));
|
||||
if (result) { content = result; updated.push('Total Plans in Phase'); }
|
||||
}
|
||||
|
||||
// Update Status line if present
|
||||
const newStatus = `Status: Executing Phase ${phaseNumber}`;
|
||||
if (/^Status:/m.test(posBody)) {
|
||||
posBody = posBody.replace(/^Status:.*$/m, newStatus);
|
||||
// Update **Current focus:** body text line (#1104)
|
||||
const focusLabel = phaseName ? `Phase ${phaseNumber} — ${phaseName}` : `Phase ${phaseNumber}`;
|
||||
const focusPattern = /(\*\*Current focus:\*\*\s*).*/i;
|
||||
if (focusPattern.test(content)) {
|
||||
content = content.replace(focusPattern, (_match, prefix) => `${prefix}${focusLabel}`);
|
||||
updated.push('Current focus');
|
||||
}
|
||||
|
||||
// Update Last activity line if present
|
||||
const newActivity = `Last activity: ${today} -- Phase ${phaseNumber} execution started`;
|
||||
if (/^Last activity:/im.test(posBody)) {
|
||||
posBody = posBody.replace(/^Last activity:.*$/im, newActivity);
|
||||
}
|
||||
// Update ## Current Position section (#1104, #1365)
|
||||
const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i;
|
||||
const positionMatch = content.match(positionPattern);
|
||||
if (positionMatch) {
|
||||
const header = positionMatch[1];
|
||||
let posBody = positionMatch[2];
|
||||
|
||||
content = content.replace(positionPattern, `${header}${posBody}`);
|
||||
updated.push('Current Position');
|
||||
// Update or insert Phase line
|
||||
const newPhase = `Phase: ${phaseNumber}${phaseName ? ` (${phaseName})` : ''} — EXECUTING`;
|
||||
if (/^Phase:/m.test(posBody)) {
|
||||
posBody = posBody.replace(/^Phase:.*$/m, newPhase);
|
||||
} else {
|
||||
posBody = newPhase + '\n' + posBody;
|
||||
}
|
||||
|
||||
// Update or insert Plan line
|
||||
const newPlan = `Plan: 1 of ${planCount || '?'}`;
|
||||
if (/^Plan:/m.test(posBody)) {
|
||||
posBody = posBody.replace(/^Plan:.*$/m, newPlan);
|
||||
} else {
|
||||
posBody = posBody.replace(/^(Phase:.*$)/m, `$1\n${newPlan}`);
|
||||
}
|
||||
|
||||
// Update Status line if present
|
||||
const newStatus = `Status: Executing Phase ${phaseNumber}`;
|
||||
if (/^Status:/m.test(posBody)) {
|
||||
posBody = posBody.replace(/^Status:.*$/m, newStatus);
|
||||
}
|
||||
|
||||
// Update Last activity line if present
|
||||
const newActivity = `Last activity: ${today} -- Phase ${phaseNumber} execution started`;
|
||||
if (/^Last activity:/im.test(posBody)) {
|
||||
posBody = posBody.replace(/^Last activity:.*$/im, newActivity);
|
||||
}
|
||||
|
||||
content = content.replace(positionPattern, `${header}${posBody}`);
|
||||
updated.push('Current Position');
|
||||
}
|
||||
} else {
|
||||
// Resume path: only update Last activity timestamp in Current Position
|
||||
// (do not touch Plan:, stopped_at, progress.percent, or plan counter)
|
||||
const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i;
|
||||
const positionMatch = content.match(positionPattern);
|
||||
if (positionMatch) {
|
||||
const header = positionMatch[1];
|
||||
let posBody = positionMatch[2];
|
||||
const resumeActivity = `Last activity: ${today} -- Phase ${phaseNumber} execution resumed (wave continue)`;
|
||||
if (/^Last activity:/im.test(posBody)) {
|
||||
posBody = posBody.replace(/^Last activity:.*$/im, resumeActivity);
|
||||
content = content.replace(positionPattern, `${header}${posBody}`);
|
||||
updated.push('Last activity (resume)');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return content;
|
||||
|
||||
166
tests/bug-3127-state-begin-phase-idempotent.test.cjs
Normal file
166
tests/bug-3127-state-begin-phase-idempotent.test.cjs
Normal file
@@ -0,0 +1,166 @@
|
||||
'use strict';
|
||||
// allow-test-rule: reads runtime STATE.md written to temp dir — behavioral output test, not source-grep
|
||||
|
||||
// Regression tests for bug #3127.
|
||||
//
|
||||
// state.begin-phase is non-idempotent: when execute-phase calls it on a phase
|
||||
// that is already mid-flight (e.g. --wave N resume), the handler unconditionally
|
||||
// overwrites execution-progress fields with stale values from the last plan-phase run:
|
||||
// - stopped_at / Last Activity Description reset to "context gathered; ready for plan-phase"
|
||||
// - Current Plan reset to 1 (from plan being executed, e.g. 3)
|
||||
// - Plan: N of M body line reset to "Plan: 1 of M"
|
||||
// - Last activity timestamp reverted to an older value
|
||||
// - progress.percent may decrease
|
||||
//
|
||||
// Fix: read the current Status field before writing. If the phase is already
|
||||
// "Executing Phase N", skip the execution-progress fields (Current Plan, plan body
|
||||
// line, Last Activity Description) and only update fields safe to overwrite on
|
||||
// resume (Last Activity date, Status if somehow wrong).
|
||||
// A --force flag bypasses the guard for intentional full resets.
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
|
||||
// Load the state.cjs module internals via the command router
|
||||
function requireStateCjs() {
|
||||
return require(path.join(ROOT, 'get-shit-done', 'bin', 'lib', 'state.cjs'));
|
||||
}
|
||||
|
||||
function makeTempPlanning(stateContent) {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3127-'));
|
||||
const planningDir = path.join(dir, '.planning');
|
||||
fs.mkdirSync(planningDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(planningDir, 'STATE.md'), stateContent, 'utf8');
|
||||
return dir;
|
||||
}
|
||||
|
||||
// A STATE.md that is mid-flight on Phase 5 (Plan 3 of 8 in progress)
|
||||
const MID_FLIGHT_STATE = `# GSD State
|
||||
|
||||
## Configuration
|
||||
Current Phase: 5
|
||||
Current Phase Name: test-phase
|
||||
Total Plans in Phase: 8
|
||||
Current Plan: 3
|
||||
Status: Executing Phase 5
|
||||
|
||||
## Current Position
|
||||
|
||||
Phase: 5 (test-phase) — EXECUTING
|
||||
Plan: 3 of 8 (Plan 00 SHIPPED — wave 1 complete; Plan 01 SHIPPED; Plan 02 next)
|
||||
Status: Executing Phase 5
|
||||
Last activity: 2026-05-05 -- Plan 02 SHIPPED wave 2 GREEN
|
||||
|
||||
## Progress
|
||||
|
||||
progress:
|
||||
total_phases: 10
|
||||
completed_phases: 4
|
||||
percent: 89
|
||||
|
||||
stopped_at: Phase 5 Plan 02 SHIPPED — Wave 2 GREEN detailed narrative here; ready for Plan 03
|
||||
`;
|
||||
|
||||
// A STATE.md that is NOT yet executing (plan-phase just ran)
|
||||
const PRE_EXECUTE_STATE = `# GSD State
|
||||
|
||||
## Configuration
|
||||
Current Phase: 5
|
||||
Current Phase Name: test-phase
|
||||
Total Plans in Phase: 8
|
||||
Current Plan: 1
|
||||
Status: Ready to execute
|
||||
|
||||
## Current Position
|
||||
|
||||
Phase: 5 (test-phase) — READY
|
||||
Plan: 1 of 8
|
||||
Status: Ready to execute
|
||||
Last activity: 2026-05-04 -- context gathered; ready for plan-phase
|
||||
|
||||
stopped_at: Phase 5 context gathered; ready for plan-phase
|
||||
`;
|
||||
|
||||
describe('bug #3127: state.begin-phase idempotency guard', () => {
|
||||
test('begin-phase on a mid-flight phase does not reset Current Plan', () => {
|
||||
const stateModule = requireStateCjs();
|
||||
const { cmdStateBeginPhase } = stateModule;
|
||||
if (!cmdStateBeginPhase) {
|
||||
// Skip if not exported — the guard may be inside a private function
|
||||
return;
|
||||
}
|
||||
const dir = makeTempPlanning(MID_FLIGHT_STATE);
|
||||
try {
|
||||
cmdStateBeginPhase(dir, '5', 'test-phase', 8, false);
|
||||
const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8');
|
||||
// Current Plan must not have been reset to 1
|
||||
const planMatch = after.match(/^Current Plan:\s*(\S+)/m);
|
||||
if (planMatch) {
|
||||
assert.notStrictEqual(planMatch[1], '1',
|
||||
'begin-phase reset Current Plan to 1 on a mid-flight phase — idempotency guard not applied');
|
||||
}
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('begin-phase on a mid-flight phase does not overwrite stopped_at narrative', () => {
|
||||
const stateModule = requireStateCjs();
|
||||
const { cmdStateBeginPhase } = stateModule;
|
||||
if (!cmdStateBeginPhase) return;
|
||||
const dir = makeTempPlanning(MID_FLIGHT_STATE);
|
||||
try {
|
||||
cmdStateBeginPhase(dir, '5', 'test-phase', 8, false);
|
||||
const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8');
|
||||
// The rich stopped_at narrative must be preserved
|
||||
assert.ok(
|
||||
after.includes('Plan 02 SHIPPED') || after.includes('Wave 2 GREEN'),
|
||||
'begin-phase overwrote stopped_at narrative on a mid-flight phase',
|
||||
);
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('begin-phase on a NOT-yet-executing phase sets Current Plan to 1 (normal path)', () => {
|
||||
const stateModule = requireStateCjs();
|
||||
const { cmdStateBeginPhase } = stateModule;
|
||||
if (!cmdStateBeginPhase) return;
|
||||
const dir = makeTempPlanning(PRE_EXECUTE_STATE);
|
||||
try {
|
||||
cmdStateBeginPhase(dir, '5', 'test-phase', 8, false);
|
||||
const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8');
|
||||
// Normal path: Current Plan should become 1 (or stay 1)
|
||||
const planMatch = after.match(/^Current Plan:\s*(\S+)/m);
|
||||
if (planMatch) {
|
||||
assert.strictEqual(planMatch[1], '1',
|
||||
'begin-phase should set Current Plan to 1 on a fresh phase');
|
||||
}
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('begin-phase always updates Last Activity date (safe on resume)', () => {
|
||||
const stateModule = requireStateCjs();
|
||||
const { cmdStateBeginPhase } = stateModule;
|
||||
if (!cmdStateBeginPhase) return;
|
||||
const dir = makeTempPlanning(MID_FLIGHT_STATE);
|
||||
try {
|
||||
cmdStateBeginPhase(dir, '5', 'test-phase', 8, false);
|
||||
const after = fs.readFileSync(path.join(dir, '.planning', 'STATE.md'), 'utf8');
|
||||
const today = new Date().toISOString().split('T')[0];
|
||||
assert.ok(
|
||||
after.includes(today),
|
||||
'begin-phase must update Last Activity date even on resume (safe field)',
|
||||
);
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user