diff --git a/.changeset/3489-complete-phase-idempotent.md b/.changeset/3489-complete-phase-idempotent.md new file mode 100644 index 000000000..915b3f836 --- /dev/null +++ b/.changeset/3489-complete-phase-idempotent.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3489 +--- +**`state complete-phase` is now idempotent — re-invocation no longer rolls STATE.md back.** Previously, running `gsd-sdk query state.complete-phase --phase ` (or `gsd state complete-phase --phase `) a second time on a phase that was already marked complete silently rewound STATE.md to that phase's moment-of-completion, clobbering `Status`, `Last Activity`, `Last Activity Description`, and the `## Current Position` body. Any downstream consumer trusting STATE.md (`/gsd-progress`, planner, the next phase's discuss-phase context loader) was routed back to the rolled-back phase. The handler now reads STATE.md before writing: if the canonical `Current Phase` field already names a phase distinct from the one being completed, the project has clearly advanced past it and the handler returns a no-op (`{ updated: [], phase: "", idempotent: true, note: "phase already superseded; no-op" }`) without touching STATE.md. (#3489) diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 618dd56e5..e0b2c284e 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -1805,6 +1805,27 @@ function cmdStateCompletePhase(cwd, raw, overridePhase) { return; } + // Idempotency guard (#3489). If STATE.md's canonical `Current Phase` field + // already names a phase distinct from the one we are being asked to mark + // complete, the project has advanced past the requested phase (e.g. a + // follow-up phase was inserted, or the next phase began). Re-running + // `state complete-phase --phase ` in that situation previously rolled + // STATE.md back to 's moment-of-completion — silently clobbering Status, + // Last Activity, Last Activity Description, and the Current Position body. + // The handler is now a no-op in that case so re-invocation from downstream + // workflows cannot regress the project state. + const existingCurrentPhaseRaw = stateExtractField(content, 'Current Phase') || ''; + const existingCurrentPhaseMatch = String(existingCurrentPhaseRaw).match(/(\d+[A-Z]?(?:\.\d+)*)/i); + const existingCurrentPhase = existingCurrentPhaseMatch ? existingCurrentPhaseMatch[1] : null; + if (existingCurrentPhase && existingCurrentPhase !== resolvedPhase) { + output( + { updated: [], phase: resolvedPhase, idempotent: true, note: 'phase already superseded; no-op' }, + raw, + 'false', + ); + return; + } + const today = new Date().toISOString().split('T')[0]; const updated = []; diff --git a/tests/bug-3489-complete-phase-idempotent.test.cjs b/tests/bug-3489-complete-phase-idempotent.test.cjs new file mode 100644 index 000000000..d92fa7c8c --- /dev/null +++ b/tests/bug-3489-complete-phase-idempotent.test.cjs @@ -0,0 +1,124 @@ +'use strict'; + +// allow-test-rule: source-text-is-the-product +// State.md is the deployed artifact; asserting on its literal text content +// tests the deployed contract. + +/** + * Regression test for #3489 + * + * `gsd state complete-phase --phase ` was non-idempotent. Re-invoking it + * on a phase already marked complete in STATE.md silently rolled STATE.md + * back to that phase's moment-of-completion — overwriting Status, Last + * Activity, Current Position and the body Status/Phase with stale values + * derived from the just-completed phase. + * + * Expected: when the target phase is already marked complete (and STATE.md + * has clearly advanced past it — e.g. a later phase is now in progress or + * inserted), `complete-phase` must be a no-op. No STATE.md write at all. + */ + +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 { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); + +describe('bug #3489: state complete-phase must be idempotent', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('bug-3489-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('re-running complete-phase on an already-complete phase does not roll STATE.md back', () => { + // STATE.md as it would appear AFTER phase 02.2 was legitimately completed + // AND a follow-up Phase 02.2.1 has since been inserted as in-progress. + // Re-invoking `state complete-phase --phase 02.2` from a downstream tool + // (e.g. a re-run of /gsd-execute-phase) must NOT regress this content. + const stateMd = [ + '---', + 'milestone: v1.0', + '---', + '', + '# State', + '', + '**Status:** in-progress', + '**Current Phase:** 02.2.1', + '**Last Activity:** 2026-05-13', + '**Last Activity Description:** Phase 02.2.1 inserted (urgent — gates Phase 5)', + '', + '## Current Position', + '', + 'Phase: 02.2.1 — Not planned yet', + 'Status: Phase 02.2.1 inserted (urgent — gates Phase 5)', + 'Last activity: 2026-05-13 -- Phase 02.2.1 inserted (urgent — gates Phase 5)', + '', + ].join('\n'); + + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, stateMd, 'utf8'); + const before = fs.readFileSync(statePath, 'utf8'); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '02.2'], tmpDir); + assert.ok(result.success, `command should not error, got: ${result.error || result.output}`); + + const after = fs.readFileSync(statePath, 'utf8'); + + // Hard assertion: file is byte-identical to its pre-call snapshot. + assert.equal( + after, + before, + `STATE.md must not be rewritten when phase is already complete.\n\n--- before ---\n${before}\n--- after ---\n${after}`, + ); + + // Output should advertise the no-op so downstream consumers can detect it. + let payload = null; + try { payload = JSON.parse(result.output); } catch (_) { /* ignore */ } + assert.ok(payload && typeof payload === 'object', `expected JSON payload, got: ${result.output}`); + assert.deepEqual(payload.updated, [], `expected empty updated list, got: ${JSON.stringify(payload.updated)}`); + assert.equal(payload.phase, '02.2'); + assert.equal(payload.idempotent, true, `expected idempotent:true flag, got: ${JSON.stringify(payload)}`); + }); + + test('completing the currently in-progress phase still works normally (no false-positive idempotency)', () => { + // Sanity check: the guard must not fire on the legitimate first completion. + const stateMd = [ + '---', + 'milestone: v1.0', + '---', + '', + '# State', + '', + '**Status:** in-progress', + '**Current Phase:** 03', + '**Last Activity:** 2026-05-13', + '', + '## Current Position', + '', + 'Phase: 03', + 'Status: Phase 03 executing', + '', + ].join('\n'); + + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, stateMd, 'utf8'); + + const result = runGsdTools(['state', 'complete-phase', '--phase', '03'], tmpDir); + assert.ok(result.success, `command failed: ${result.error || result.output}`); + + const after = fs.readFileSync(statePath, 'utf8'); + assert.ok( + after.includes('**Status:** Phase 03 complete'), + `expected Status updated to "Phase 03 complete", got:\n${after}`, + ); + + const payload = JSON.parse(result.output); + assert.notEqual(payload.idempotent, true, 'first completion must not be flagged idempotent'); + assert.ok(Array.isArray(payload.updated) && payload.updated.length > 0, 'expected non-empty updated list'); + }); +});