From 669d6a1f32e29cbe9436437b5a61550b5f817125 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 5 May 2026 15:02:30 -0400 Subject: [PATCH] fix(#3127): make state.begin-phase idempotent on mid-flight phases (#3145) * 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 --- .../fix-3127-state-begin-phase-idempotent.md | 5 + get-shit-done/bin/lib/state.cjs | 152 +++++++++------- ...3127-state-begin-phase-idempotent.test.cjs | 166 ++++++++++++++++++ 3 files changed, 260 insertions(+), 63 deletions(-) create mode 100644 .changeset/fix-3127-state-begin-phase-idempotent.md create mode 100644 tests/bug-3127-state-begin-phase-idempotent.test.cjs diff --git a/.changeset/fix-3127-state-begin-phase-idempotent.md b/.changeset/fix-3127-state-begin-phase-idempotent.md new file mode 100644 index 000000000..9658488ff --- /dev/null +++ b/.changeset/fix-3127-state-begin-phase-idempotent.md @@ -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. diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 7bfbd35fc..6f570eed6 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -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; diff --git a/tests/bug-3127-state-begin-phase-idempotent.test.cjs b/tests/bug-3127-state-begin-phase-idempotent.test.cjs new file mode 100644 index 000000000..1b376205b --- /dev/null +++ b/tests/bug-3127-state-begin-phase-idempotent.test.cjs @@ -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 }); + } + }); +});