diff --git a/.changeset/sharp-wasps-fly.md b/.changeset/sharp-wasps-fly.md new file mode 100644 index 000000000..2463f4f8b --- /dev/null +++ b/.changeset/sharp-wasps-fly.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1252 +--- +**`state.*` writes no longer silently revert the STATE.md frontmatter `status`/`stopped_at`** — an incidental write (e.g. `state record-session`) that doesn't change the body's `Status:`/`Stopped at:` source field now preserves the existing frontmatter value instead of re-deriving it from possibly-stale body text. Legitimate transitions (e.g. `begin-phase`/`complete-phase`, which do update the body Status) still re-derive normally, so a verified-complete phase can no longer be flipped back to `verifying` by an unrelated write. diff --git a/src/state.cts b/src/state.cts index 75e99d800..1c96934c9 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1579,6 +1579,28 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string // Snapshot the existing progress block BEFORE the transform so we can // restore it when resync is false. const preFm = resync ? null : extractFrontmatter(content) as Record; + + // Bug #1230: delta heuristic — snapshot pre-transform body source fields so + // we can detect whether THIS write changed them. syncStateFrontmatter + // re-derives frontmatter status/stopped_at from the body on every write; + // when the body's source field was NOT changed by the transform, the + // existing frontmatter value (e.g. a hand-set 'completed') must win over + // the body-derived value (e.g. 'verifying' from a stale "Status: Verifying + // Phase 3" line that an earlier tool wrote). We do NOT disturb `preFm` + // above (null when resync:true) — these are independent snapshots. + // Strip frontmatter before calling stateExtractField so the YAML `status:` + // key in the frontmatter block cannot shadow the body field we are tracking. + const preBody = stripFrontmatter(content); + const preFmSnapshot = extractFrontmatter(content) as Record; + const preBodyStatus = stateExtractField(preBody, 'Status'); + // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, + // mirroring buildStateFrontmatter's sessionBodyScope logic (line ~1172). + // A stale "Stopped at:" in a non-Session section (e.g. Session Continuity + // Archive prose) must not interfere with the delta comparison. + const preSessionMatch = matchSessionSection(preBody); + const preSessionScope = preSessionMatch ? preSessionMatch[1] : preBody; + const preBodyStoppedAt = stateExtractField(preSessionScope, 'Stopped At') || stateExtractField(preSessionScope, 'Stopped at'); + const modified = transformFn(content); // Bug #948: no-op guard — if the transform produced no change, do NOT write @@ -1594,14 +1616,63 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string let synced = syncStateFrontmatter(modified, cwd); - if (!resync && preFm && preFm['progress']) { + // Compute postFm once and apply BOTH the progress-restore (when !resync) + // AND the status/stopped_at preservation (#1230) before reconstructing. + // This avoids double-wrapping the frontmatter block. + const needsProgressRestore = !resync && preFm && preFm['progress']; + + // Post-transform body source fields used for the delta comparison (#1230). + // Use `modified` (not `synced`): syncStateFrontmatter only rewrites the frontmatter block, so the body is identical in both — and we need the body the transform produced. + // Strip frontmatter so the YAML status key cannot shadow the body field. + const postBody = stripFrontmatter(modified); + const postBodyStatus = stateExtractField(postBody, 'Status'); + // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, + // consistent with the pre-transform snapshot above and buildStateFrontmatter. + const postSessionMatch = matchSessionSection(postBody); + const postSessionScope = postSessionMatch ? postSessionMatch[1] : postBody; + const postBodyStoppedAt = stateExtractField(postSessionScope, 'Stopped At') || stateExtractField(postSessionScope, 'Stopped at'); + + let mutated = false; + const postFm = extractFrontmatter(synced) as Record; + + if (needsProgressRestore) { // Re-apply the curated progress block that syncStateFrontmatter just // overwrote with disk-derived values. Only restore keys that were present // in the snapshot — this preserves any new non-progress frontmatter fields // (e.g., status, current_phase) that syncStateFrontmatter legitimately // derived from the updated body. - const postFm = extractFrontmatter(synced) as Record; postFm['progress'] = preFm['progress']; + mutated = true; + } + + // Bug #1230: preserve existing frontmatter status when this write did NOT + // change the body's Status field. A write that doesn't touch Status must + // not silently revert a hand-set frontmatter status (e.g. 'completed') to + // whatever the stale body Status happens to derive (e.g. 'verifying'). + // Only apply when the existing frontmatter held a real, non-unknown status. + if ( + postBodyStatus === preBodyStatus && + typeof preFmSnapshot['status'] === 'string' && + preFmSnapshot['status'].length > 0 && + preFmSnapshot['status'] !== 'unknown' && + postFm['status'] !== preFmSnapshot['status'] + ) { + postFm['status'] = preFmSnapshot['status']; + mutated = true; + } + + // Bug #1230: same delta heuristic for stopped_at. + if ( + postBodyStoppedAt === preBodyStoppedAt && + typeof preFmSnapshot['stopped_at'] === 'string' && + preFmSnapshot['stopped_at'].length > 0 && + postFm['stopped_at'] !== preFmSnapshot['stopped_at'] + ) { + postFm['stopped_at'] = preFmSnapshot['stopped_at']; + mutated = true; + } + + if (mutated) { const yamlStr = reconstructFrontmatter(postFm as unknown as Frontmatter); const body = stripFrontmatter(synced); synced = `---\n${yamlStr}\n---\n\n${body}`; diff --git a/tests/bug-905-state-syncstatefrontmatter-preserve-scalars.test.cjs b/tests/bug-905-state-syncstatefrontmatter-preserve-scalars.test.cjs index c6f59f9c3..08b6d0bac 100644 --- a/tests/bug-905-state-syncstatefrontmatter-preserve-scalars.test.cjs +++ b/tests/bug-905-state-syncstatefrontmatter-preserve-scalars.test.cjs @@ -319,3 +319,299 @@ describe('#905: syncStateFrontmatter preserves scalars when body annotations are } }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// Bug #1230 regression suite +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Build a STATE.md where: + * - frontmatter has an explicit status (e.g. 'completed') and optional stopped_at + * - body has a Status: field that is STALE relative to the frontmatter + * (e.g. "Verifying Phase 3" would derive 'verifying') + * A subsequent incidental write must NOT revert the hand-set frontmatter status. + */ +function buildStateMd1230({ fmStatus = 'completed', fmStoppedAt = null, bodyStatus = 'Verifying Phase 3', bodyStoppedAt = null } = {}) { + const fmLines = [ + '---', + 'gsd_state_version: 1.0', + `status: ${fmStatus}`, + ]; + if (fmStoppedAt) fmLines.push(`stopped_at: "${fmStoppedAt}"`); + fmLines.push('---'); + + const bodyLines = [ + '', + '# GSD State', + '', + '## Configuration', + `Status: ${bodyStatus}`, + 'Last Activity: 2026-01-01', + `Current Phase: 3`, + '', + '## Session', + '', + '**Last session:** 2026-01-01T00:00:00.000Z', + ]; + if (bodyStoppedAt) bodyLines.push(`**Stopped at:** ${bodyStoppedAt}`); + bodyLines.push(''); + + return [...fmLines, ...bodyLines].join('\n'); +} + +describe('bug #1230: readModifyWriteStateMd preserves frontmatter status/stopped_at when write does not change body source field', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // (a) CORE: record-session with stale body Status leaves frontmatter status: completed intact + test('(a) record-session does NOT revert frontmatter status: completed when body Status is unchanged', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + // frontmatter status: completed; body Status: Verifying Phase 3 (derives 'verifying') + fs.writeFileSync(statePath, buildStateMd1230({ fmStatus: 'completed', bodyStatus: 'Verifying Phase 3' })); + + const result = runGsdTools( + ['state', 'record-session', '--stopped-at', 'Phase 3 final review checkpoint'], + tmpDir, + ); + assert.ok(result.success, `record-session failed: ${result.error}`); + + // Assert on raw file frontmatter — not state json (which re-derives) + const written = fs.readFileSync(statePath, 'utf8'); + const rawFm = parseFrontmatter(written); + assert.strictEqual( + rawFm.status, + 'completed', + `frontmatter status must remain 'completed' after record-session (body Status unchanged); got: ${JSON.stringify(rawFm.status)}`, + ); + }); + + // (b) add-decision (resync:true) with frontmatter status: completed, stale body → status preserved + test('(b) add-decision (resync:true) does NOT revert frontmatter status: completed when body Status unchanged', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateMd1230({ fmStatus: 'completed', bodyStatus: 'Verifying Phase 3' })); + + const result = runGsdTools( + ['state', 'add-decision', '--phase', '3', '--summary', 'Use Node 22'], + tmpDir, + ); + assert.ok(result.success, `add-decision failed: ${result.error}`); + + const written = fs.readFileSync(statePath, 'utf8'); + const rawFm = parseFrontmatter(written); + assert.strictEqual( + rawFm.status, + 'completed', + `frontmatter status must remain 'completed' after add-decision; got: ${JSON.stringify(rawFm.status)}`, + ); + }); + + // (c) LEGITIMATE UPDATE NOT FROZEN: begin-phase changes body Status → frontmatter re-derived correctly. + // + // stateReplaceField(content, 'Status', ...) replaces the FIRST ^Status: match in the + // full content (frontmatter + body). When the frontmatter has NO 'status:' key, the + // match falls through to the body 'Status:' line, which IS changed. The delta then + // fires (preBodyStatus ≠ postBodyStatus), the preservation guard is skipped, and + // syncStateFrontmatter re-derives from the new body value as intended. + test('(c) begin-phase changes body Status → frontmatter status reflects new body-derived value', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + // Frontmatter intentionally has NO 'status:' key so stateReplaceField targets the body + // 'Status:' line. After the transform, body Status becomes "Executing Phase 3" → + // normalizeStateStatus → 'executing' must be written to frontmatter. + const content = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Status: Ready to execute', + 'Last Activity: 2026-01-01', + 'Current Phase: 2', + 'Current Phase Name: Planning', + 'Current Plan: 1', + '', + '## Current Position', + '', + 'Phase: 2 (Planning) — READY', + 'Plan: 1 of 1', + 'Status: Ready to execute', + 'Last activity: 2026-01-01 -- Phase 2 planning complete', + '', + ].join('\n'); + fs.writeFileSync(statePath, content); + + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '3', '--name', 'Execution'], + tmpDir, + ); + assert.ok(result.success, `begin-phase failed: ${result.error}`); + + const written = fs.readFileSync(statePath, 'utf8'); + const rawFm = parseFrontmatter(written); + // begin-phase changed body Status to "Executing Phase 3" → delta fired → re-derived → 'executing' + assert.strictEqual( + rawFm.status, + 'executing', + `frontmatter status must be updated to 'executing' when begin-phase changes body Status; got: ${JSON.stringify(rawFm.status)}`, + ); + }); + + // (d) stopped_at: TRUE RED — frontmatter stopped_at preserved when body Stopped at differs + // + // Change C: this is a TRUE regression guard. Frontmatter stopped_at ("Phase 7 verified PASS") + // differs from the body ## Session "Stopped at:" value ("Phase 3 work"). The write operation + // (add-decision) does NOT touch the Session Stopped at line. The delta heuristic must detect + // that the Session Stopped at did NOT change (pre == post == "Phase 3 work") and therefore + // preserve the frontmatter value "Phase 7 verified PASS". Pre-fix code would REVERT to + // "Phase 3 work" (the body-derived value) — making this a genuine red. + test('(d) add-decision preserves frontmatter stopped_at when body Session Stopped at differs from frontmatter and is unchanged', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const content = [ + '---', + 'gsd_state_version: 1.0', + 'status: completed', + 'stopped_at: "Phase 7 verified PASS"', + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Status: Phase 3 complete', + 'Last Activity: 2026-01-01', + 'Current Phase: 3', + '', + '## Session', + '', + '**Last session:** 2026-01-01T00:00:00.000Z', + // body Session Stopped at is STALE (different from frontmatter stopped_at) + '**Stopped at:** Phase 3 work', + '**Resume file:** None', + '', + ].join('\n'); + fs.writeFileSync(statePath, content); + + // add-decision does NOT touch ## Session Stopped at → pre and post body value identical + // ("Phase 3 work" unchanged) → delta fires → frontmatter "Phase 7 verified PASS" preserved. + const result = runGsdTools( + ['state', 'add-decision', '--phase', '3', '--summary', 'Preserve stopped_at check'], + tmpDir, + ); + assert.ok(result.success, `add-decision failed: ${result.error}`); + + const written = fs.readFileSync(statePath, 'utf8'); + const rawFm = parseFrontmatter(written); + assert.strictEqual( + rawFm.stopped_at, + 'Phase 7 verified PASS', + `frontmatter stopped_at must be preserved ("Phase 7 verified PASS") when body Session Stopped at ` + + `is unchanged (stale "Phase 3 work"); got: ${JSON.stringify(rawFm.stopped_at)}`, + ); + }); + + // (f) PRODUCTION-PATH: legitimate status transition is NOT frozen by the delta heuristic. + // + // Change A: prove that begin-phase on a STATE.md with inline "Status: Executing Phase 1" + // (the standard template format) correctly transitions frontmatter status from + // 'executing' to a new 'executing' value when the body Status CHANGES. + // More critically: also verify a complete-phase → frontmatter becomes 'completed' + // when the body Status field IS changed. This locks in that the delta heuristic + // re-derives correctly whenever the body's Status source field actually changes. + test('(f) begin-phase changes inline body Status → delta fires → frontmatter status updated (not frozen)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + // Realistic STATE.md: frontmatter status: executing, body Status: Executing Phase 1 (inline format) + const content = [ + '---', + 'gsd_state_version: 1.0', + 'status: executing', + 'current_phase: 1', + 'current_phase_name: Planning', + 'current_plan: 1', + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Status: Executing Phase 1', + 'Last Activity: 2026-01-01', + 'Current Phase: 1', + 'Current Phase Name: Planning', + 'Current Plan: 1', + 'Total Plans in Phase: 2', + '', + ].join('\n'); + fs.writeFileSync(statePath, content); + + // begin-phase 2 changes body Status from "Executing Phase 1" to "Executing Phase 2" + // → pre and post body Status differ → delta does NOT fire → syncStateFrontmatter + // re-derives status from new body value → frontmatter status must reflect 'executing' + // (still 'executing' after begin-phase 2 is a correct transition). + const beginResult = runGsdTools( + ['state', 'begin-phase', '--phase', '2', '--name', 'Implementation'], + tmpDir, + ); + assert.ok(beginResult.success, `state begin-phase failed: ${beginResult.error}`); + + const written = fs.readFileSync(statePath, 'utf8'); + const rawFm = parseFrontmatter(written); + // Frontmatter status must have been updated (not frozen at original 'executing' for phase 1). + // After begin-phase 2, body Status becomes "Executing Phase 2" → re-derived → still 'executing' + // but it must NOT be the stale body-derived value from before the transform; the key check is + // that the write completed successfully and status is a known valid value. + assert.ok( + rawFm.status === 'executing', + `frontmatter status must be 'executing' after begin-phase 2 (delta fires, re-derived); got: ${JSON.stringify(rawFm.status)}`, + ); + + // Stronger check: if we then run a command that changes Status to a DIFFERENT value, + // the frontmatter MUST reflect the new body-derived status (not be frozen). + // Use state update to change Status to "Phase 2 complete" → derives 'completed'. + const updateResult = runGsdTools( + ['state', 'update', 'Status', 'Phase 2 complete'], + tmpDir, + ); + assert.ok(updateResult.success, `state update Status failed: ${updateResult.error}`); + + const written2 = fs.readFileSync(statePath, 'utf8'); + const rawFm2 = parseFrontmatter(written2); + assert.strictEqual( + rawFm2.status, + 'completed', + `frontmatter status must be 'completed' after body Status → 'Phase 2 complete' (delta fires, re-derived); ` + + `got: ${JSON.stringify(rawFm2.status)}. The delta heuristic must NOT freeze status when the body field changes.`, + ); + }); + + // (e) milestone-switch uses platformWriteSync directly (not RMW) — check it still resets status correctly + test('(e) milestone-switch still resets frontmatter status to planning (uses writeStateMd path, not RMW)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateMd1230({ fmStatus: 'completed', bodyStatus: 'Phase 3 complete' })); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n## v2.0 Next\n\n### Phase 4: Next steps\n', + 'utf-8', + ); + + const result = runGsdTools( + ['state', 'milestone-switch', '--milestone', 'v2.0', '--name', 'Next'], + tmpDir, + ); + assert.ok(result.success, `milestone-switch failed: ${result.error}`); + + const written = fs.readFileSync(statePath, 'utf8'); + const rawFm = parseFrontmatter(written); + assert.strictEqual( + rawFm.status, + 'planning', + `milestone-switch must reset frontmatter status to 'planning'; got: ${JSON.stringify(rawFm.status)}`, + ); + }); +});