From 6724581285c22b9c84d506faf5668952571561b4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 14 Jun 2026 20:51:53 -0400 Subject: [PATCH] fix(#1230): preserve frontmatter status/stopped_at when a state write doesn't change the body source (#1252) * fix(#1230): preserve frontmatter status/stopped_at when a state write doesn't change the body source Adds a delta heuristic in `readModifyWriteStateMd`: snapshots the body Status and Stopped At fields before the transform, then after syncStateFrontmatter runs, restores the existing frontmatter values for any field whose body source was not changed by this write. Integrates cleanly with the existing !resync progress-restore block (computes postFm once, applies both restorations, reconstructs frontmatter once). Regression tests added to bug-905 test file (same theme, no new file). Co-Authored-By: Claude Sonnet 4.6 * chore(#1230): add Fixed changeset fragment --------- Co-authored-by: Claude Sonnet 4.6 --- .changeset/sharp-wasps-fly.md | 5 + src/state.cts | 75 ++++- ...statefrontmatter-preserve-scalars.test.cjs | 296 ++++++++++++++++++ 3 files changed, 374 insertions(+), 2 deletions(-) create mode 100644 .changeset/sharp-wasps-fly.md 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)}`, + ); + }); +});