From 46967baae84bc3690bc235c2d7f46298dc9a4dad Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 10 Jun 2026 00:22:29 -0400 Subject: [PATCH] fix(#948): guard STATE.md no-op writes; preserve milestone_name/stopped_at (closes #944) (#952) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#948): add regression tests for no-op write guard and record-session auto-create (#944) Red before fix: 11/15 tests fail. Green after: 15/15. Covers zero-match patch byte-identity, milestone_name preservation, stopped_at frontmatter-wins, record-session auto-create fallback, and adversarial fixtures (CRLF, empty body, non-canonical labels). Also registers bug-948-state-noop-write-guard.test.cjs in the state bucket of lint-test-file-count.allowlist.json. Co-Authored-By: Claude Opus 4.8 * fix(#948): guard STATE.md no-op writes; preserve milestone_name/stopped_at (closes #944) Shared root cause: `readModifyWriteStateMd` wrote STATE.md unconditionally even when the transform produced no change, and `syncStateFrontmatter` re-derived frontmatter from the possibly-stale body on every write. Three coordinated fixes in src/state.cts: 1. readModifyWriteStateMd: add no-op guard — when transform result === input content, skip the write entirely (no platformWriteSync, no last_updated bump, no frontmatter re-derive). Fixes #948 zero-match phantom write and the #944 phantom last_updated bump. 2. syncStateFrontmatter: extend existing-frontmatter preserve logic — fall back to existingFm['milestone_name'] / existingFm['milestone'] when the derived value is the template placeholder 'milestone' (getMilestoneInfo returns this literal when it cannot match the version in ROADMAP.md); prefer existingFm['stopped_at'] / existingFm['paused_at'] over a body-derived value (the frontmatter value, written by the canonical record-session path, wins over stale historical body lines). Mirrors the fallback already in cmdStateJson. 3. cmdStateRecordSession: when --stopped-at / --resume-file are supplied but body labels are absent, DWIM auto-create a canonical ## Session section (mirroring how add-decision / add-blocker / record-metric auto-create their sections). Never return a silent recorded:false when the caller supplied values. SDK check: no sdk/src/state.ts exists in this repo (the comment in cmdStateSnapshot references a sibling concern in the TypeScript SDK codebase, which is a separate repo not present here). Co-Authored-By: Claude Opus 4.8 * chore: add changeset for PR #952 (fix #948/#944) Co-Authored-By: Claude Opus 4.8 * fix(#948): correct stopped_at preserve rule; adjust test for sync behaviour The "always prefer frontmatter stopped_at" rule in syncStateFrontmatter was too aggressive — it broke phase.complete which intentionally updates stopped_at in the body and expects syncStateFrontmatter to pick it up. The primary fix (no-op guard in readModifyWriteStateMd) already prevents the stale-body-overwrites-frontmatter scenario from #948: the file is not written when the transform produces no change, so syncStateFrontmatter never runs on a zero-match patch. The body-derived value can only win when an actual write occurs, which means the body was legitimately updated. Reverted to the original #905 rule for stopped_at/paused_at: fall back to existing frontmatter only when the derived value is absent (empty/null). Also adjusted the sync-suite test to assert what state sync actually does (milestone_name preservation) rather than a stopped_at-wins property that state sync does not have by design. Co-Authored-By: Claude Opus 4.8 * fix(#944): update existing session block in place (adversarial review) HIGH finding: the DWIM auto-create in cmdStateRecordSession was appending a second ## Session block unconditionally, even when one already existed with non-canonical content (e.g. a markdown table). Both buildStateFrontmatter and cmdStateSnapshot read only the FIRST ## Session block via regex, so the newly-written Stopped at / Resume file values landed in the second, invisible block — frontmatter stopped_at stayed stale and state-snapshot returned nulls. Fix: check for an existing ## Session heading. When one is present, normalize that section in place by replacing its body with canonical **Last session:** / **Stopped at:** / **Resume file:** bold-label lines. Only append a brand-new section when NO ## Session heading exists. LOW finding: the auto-create scaffold emits **Last session:** but cmdStateSnapshot only matched **Last Date:**, so session.last_date was null after auto-create despite a valid timestamp being written. Fix: extend the lastDateMatch regex in cmdStateSnapshot to also accept **Last session:** / Last session: (the form the scaffold writes). Tests: 3 new tests added to bug-948-state-noop-write-guard.test.cjs that confirmed failure against the previous HEAD and pass after this fix: - exactly one ## Session block after record-session with non-canonical existing block - state-snapshot sees correct stopped_at via first Session block (not a duplicate) - state-snapshot session.last_date is non-null after auto-create on body-less file Co-Authored-By: Claude Opus 4.8 * fix(#944): improve in-place section replace to cleanly remove old body content The previous regex `/(^## Session[ \t]*$)([\s\S]*?)(?=\n^## |\n*$)/im` with a lazy match consumed nothing after the heading, so old non-canonical body content (e.g. table rows) remained after the new canonical lines. While functionally correct (parsers found the canonical lines first in the FIRST ## Session block), it left stale content in the section. Replace with a negative-lookahead per-line pattern that consumes all content from the heading up to (but not including) the next ## heading, producing a clean section with only the canonical bold-label lines. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .changeset/clever-yaks-sprint.md | 5 + scripts/lint-test-file-count.allowlist.json | 1 + src/state.cts | 125 +++- tests/bug-948-state-noop-write-guard.test.cjs | 698 ++++++++++++++++++ 4 files changed, 827 insertions(+), 2 deletions(-) create mode 100644 .changeset/clever-yaks-sprint.md create mode 100644 tests/bug-948-state-noop-write-guard.test.cjs diff --git a/.changeset/clever-yaks-sprint.md b/.changeset/clever-yaks-sprint.md new file mode 100644 index 000000000..322b609e8 --- /dev/null +++ b/.changeset/clever-yaks-sprint.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 952 +--- +**`state patch` and `state record-session` no longer corrupt STATE.md** — a no-match patch no longer rewrites the file (was resetting `milestone_name` and resurrecting a stale `stopped_at`), and `record-session` now persists `--stopped-at`/`--resume-file` even when the body lacks the exact labels. diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index c61b9c64c..55b1a42ef 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -98,6 +98,7 @@ "bug-3454-state-dollar-backreference-growth.test.cjs", "bug-397-state-preserve-executor-authored.test.cjs", "bug-905-state-syncstatefrontmatter-preserve-scalars.test.cjs", + "bug-948-state-noop-write-guard.test.cjs", "state-acquirestatelock-non-eexist.test.cjs", "state-prune.test.cjs", "state.test.cjs" diff --git a/src/state.cts b/src/state.cts index a36d5abff..b7edac8c3 100644 --- a/src/state.cts +++ b/src/state.cts @@ -713,6 +713,7 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, const now = realClock.nowIso(); const updated: string[] = []; + let sessionCreated = false; readModifyWriteStateMd(statePath, (content) => { // Update Last session / Last Date @@ -755,11 +756,85 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, } } + // Bug #944: DWIM normalize/auto-create — when the caller supplied --stopped-at or + // --resume-file but the body lacks the canonical labels (in-place replace + // returned a miss), persist the values durably. Mirrors the DWIM pattern used + // by add-decision, add-blocker, and record-metric. Never silently drop + // caller-supplied values. + // + // Guard: only act when the caller actually supplied a value. When no + // --stopped-at / --resume-file are given and the body already had no session + // labels (nothing was updated), we return recorded:false — the existing + // behaviour for a no-op call that didn't supply any values. + // + // Correctness invariant: both buildStateFrontmatter and cmdStateSnapshot read + // only the FIRST `## Session` block (via a /##\s*Session\s*\n…/i regex). + // If we blindly append a second `## Session` block when one already exists, the + // newly-written Stopped at / Resume file end up in the second (invisible) block. + // Fix: when a `## Session` heading already exists, normalize THAT block in place + // (insert / replace canonical bold-label lines within the existing section). + // Only append a brand-new section when NO `## Session` heading exists at all. + const callerSuppliedValues = !!(options.stopped_at || (options.resume_file !== undefined && options.resume_file !== null)); + const needsStoppedAt = options.stopped_at && !updated.includes('Stopped At'); + const needsResumeFile = options.resume_file !== undefined && options.resume_file !== null && !updated.includes('Resume File'); + const needsLastSession = !updated.includes('Last session') && !updated.includes('Last Date'); + + if (callerSuppliedValues && (needsStoppedAt || needsResumeFile || needsLastSession)) { + const resumeValue = (options.resume_file !== undefined && options.resume_file !== null) + ? options.resume_file + : 'None'; + const stoppedAtValue = options.stopped_at || 'None'; + + // Determine whether a ## Session heading already exists in the body. + const existingSessionHeading = /^## Session\s*$/im.test(content); + + if (existingSessionHeading) { + // Normalize in place: replace the ENTIRE BODY of the existing ## Session + // section (heading + all content up to the next ## heading or EOF) with + // canonical bold-label lines. The negative-lookahead per-line pattern + // `(?!^## )[\s\S]` consumes every line that doesn't start with "## ", + // which correctly stops at the next section boundary without consuming it. + // A trailing blank line is added so the next ## heading keeps its spacing. + content = content.replace( + /^(## Session[ \t]*\n(?:(?!^## )[\s\S])*)/m, + [ + '## Session', + '', + `**Last session:** ${now}`, + `**Stopped at:** ${stoppedAtValue}`, + `**Resume file:** ${resumeValue}`, + '', + '', + ].join('\n'), + ); + } else { + // No ## Session heading exists at all — append a new canonical section. + const scaffold = [ + '', + '## Session', + '', + `**Last session:** ${now}`, + `**Stopped at:** ${stoppedAtValue}`, + `**Resume file:** ${resumeValue}`, + '', + ].join('\n'); + content = content.trimEnd() + '\n' + scaffold; + } + + sessionCreated = true; + + if (needsLastSession) updated.push('Last session'); + if (needsStoppedAt) updated.push('Stopped At'); + if (needsResumeFile) updated.push('Resume File'); + } + return content; }, cwd); if (updated.length > 0) { - output({ recorded: true, updated }, raw, 'true'); + const result: Record = { recorded: true, updated }; + if (sessionCreated) result['created'] = true; + output(result, raw, 'true'); } else { output({ recorded: false, reason: 'No session fields found in STATE.md' }, raw, 'false'); } @@ -850,8 +925,12 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { const sessionMatch = body.match(/##\s*Session\s*\n([\s\S]*?)(?=\n##|$)/i); if (sessionMatch) { const sessionSection = sessionMatch[1]; + // Accept both `**Last Date:**` (canonical template form) and `**Last session:**` + // (the form written by the DWIM auto-create / normalize path added for #944). const lastDateMatch = sessionSection.match(/\*\*Last Date:\*\*\s*(.+)/i) - || sessionSection.match(/^Last Date:\s*(.+)/im); + || sessionSection.match(/^Last Date:\s*(.+)/im) + || sessionSection.match(/\*\*Last session:\*\*\s*(.+)/i) + || sessionSection.match(/^Last session:\s*(.+)/im); const stoppedAtMatch = sessionSection.match(/\*\*Stopped At:\*\*\s*(.+)/i) || sessionSection.match(/^Stopped At:\s*(.+)/im); const resumeFileMatch = sessionSection.match(/\*\*Resume File:\*\*\s*(.+)/i) @@ -1073,12 +1152,42 @@ function syncStateFrontmatter(content: string, cwd: string | undefined): string derivedFm['status'] = existingFm['status']; } + // Bug #948: preserve `milestone_name` / `milestone` when the derived value + // is the template placeholder 'milestone'. getMilestoneInfo returns the + // literal string 'milestone' when it cannot match the version from the roadmap + // (e.g. no ROADMAP.md, roadmap lacks the heading for the stored version, or the + // milestone version read from STATE.md itself triggers the lookup before the + // file is fully written). A placeholder must never overwrite a real name that the + // existing frontmatter already holds; only an empty derived value falls through + // to this guard (the primary #905 preserve path below handles that). + const MILESTONE_NAME_PLACEHOLDER = 'milestone'; + if ( + derivedFm['milestone_name'] === MILESTONE_NAME_PLACEHOLDER && + existingFm['milestone_name'] && + existingFm['milestone_name'] !== MILESTONE_NAME_PLACEHOLDER + ) { + derivedFm['milestone_name'] = existingFm['milestone_name']; + // Keep the stored milestone version consistent with the preserved name. + if (existingFm['milestone']) { + derivedFm['milestone'] = existingFm['milestone']; + } + } + // Bug #905: preserve scalar fields that buildStateFrontmatter can only derive // from body annotations (Current Phase:, Current Plan:, etc.). When those // annotations are absent — e.g. after an agent or tool rewrites the body — // buildStateFrontmatter returns no value for those keys. Mirror the same // fallback pattern used in cmdStateJson so the existing frontmatter values // survive every writeStateMd call. + // + // For stopped_at / paused_at: the original #905 "fall back when derived is + // absent" rule is preserved here. The stale-body-overwrites-frontmatter + // scenario from #948 is prevented by the no-op guard in + // readModifyWriteStateMd: when the transform produces no change the file is + // never written, so syncStateFrontmatter never even runs. Attempting to + // "always prefer frontmatter" here breaks legitimate callers like phase.complete + // that intentionally write a new stopped_at value to the body and expect + // syncStateFrontmatter to pick it up. if (!derivedFm['stopped_at'] && existingFm['stopped_at']) { derivedFm['stopped_at'] = existingFm['stopped_at']; } @@ -1247,6 +1356,18 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string // restore it when resync is false. const preFm = resync ? null : extractFrontmatter(content) as Record; const modified = transformFn(content); + + // Bug #948: no-op guard — if the transform produced no change, do NOT write + // the file. An unconditional write would bump `last_updated`, reset + // `milestone_name` to the template placeholder, and resurrect stale + // body-derived `stopped_at` values via syncStateFrontmatter. Skipping the + // write when content is unchanged is safe because every caller that mutates + // content already returns the mutated string, and callers that detect a + // no-op explicitly return the original content unchanged. + if (modified === content) { + return; + } + let synced = syncStateFrontmatter(modified, cwd); if (!resync && preFm && preFm['progress']) { diff --git a/tests/bug-948-state-noop-write-guard.test.cjs b/tests/bug-948-state-noop-write-guard.test.cjs new file mode 100644 index 000000000..c2c4ae246 --- /dev/null +++ b/tests/bug-948-state-noop-write-guard.test.cjs @@ -0,0 +1,698 @@ +'use strict'; +/** + * Regression guard for bugs #948 and #944. + * + * #948 (data loss): a `state patch` whose fields all fail to match still + * rewrites STATE.md — bumping `last_updated`, resetting `milestone_name` to + * the template placeholder, and resurrecting a stale `stopped_at` from an + * old body `## Session` block (body-derived value overwrites a newer + * frontmatter value written by `record-session`). + * + * #944: `state record-session --stopped-at X --resume-file Y` silently + * drops the supplied values when the STATE.md body lacks the exact session + * labels the in-place replace expects, returning `{"recorded": false}` at + * exit 0 and only bumping `last_updated`. + * + * Shared root cause: `readModifyWriteStateMd` always writes STATE.md even + * when the transform produced no change, and `syncStateFrontmatter` + * re-derives frontmatter (including milestone_name / stopped_at) from the + * possibly-stale body on every write. + * + * Fixes: + * 1. No-op guard in `readModifyWriteStateMd`: when transform output === + * input, skip the write entirely. + * 2. `syncStateFrontmatter` preserves existing `milestone_name` / `milestone` + * when the derived value is the template placeholder `'milestone'`. + * 3. `syncStateFrontmatter` prefers existing frontmatter `stopped_at` / + * `paused_at` over a body-derived value (frontmatter wins). + * 4. `cmdStateRecordSession` auto-creates a canonical `## Session` section + * when `--stopped-at` / `--resume-file` are supplied but no labels exist. + */ + +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 { runGsdTools, createTempProject, cleanup, parseFrontmatter } = require('./helpers.cjs'); + +// ───────────────────────────────────────────────────────────────────────────── +// Fixture builders +// ───────────────────────────────────────────────────────────────────────────── + +/** + * STATE.md with: + * - real `milestone_name` in frontmatter (e.g. "My Real Milestone") + * - newer frontmatter `stopped_at` (written by a prior `record-session`) + * - stale `## Session` body section with an OLDER "Stopped at" line + * + * When a zero-match `state patch` runs on this file, NONE of these values + * should be disturbed — the file must be byte-identical afterward. + */ +function buildStateMdWithStaleSectionAndRealFrontmatter(opts) { + const { + milestoneName = 'My Real Milestone', + fmStoppedAt = 'Phase 3, Plan 2 — newer value', + bodyStoppedAt = 'Phase 1, Plan 1 — stale historical value', + lastUpdated = '2026-01-01T00:00:00.000Z', + } = opts || {}; + + return [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v2.0', + `milestone_name: ${milestoneName}`, + 'status: executing', + `stopped_at: ${fmStoppedAt}`, + `last_updated: ${lastUpdated}`, + 'progress:', + ' total_phases: 5', + ' completed_phases: 2', + ' total_plans: 10', + ' completed_plans: 4', + ' percent: 40', + '---', + '', + '# GSD State', + '', + '## Current Position', + '', + 'Status: Executing Phase 3', + 'Last Activity: 2026-01-01', + '', + '## Session', + '', + `**Last session:** 2026-01-01T00:00:00.000Z`, + `**Stopped at:** ${bodyStoppedAt}`, + '**Resume file:** None', + '', + '## Accumulated Context', + '', + '### Decisions', + '', + '- [Phase 1]: Use Node 22', + '', + ].join('\n'); +} + +/** + * STATE.md with NO session section at all — no "## Session" heading, + * no Stopped at / Resume file labels. This is the #944 scenario. + */ +function buildStateMdWithoutSessionSection() { + return [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Foundation', + 'status: executing', + 'last_updated: 2026-01-01T00:00:00.000Z', + '---', + '', + '# GSD State', + '', + '## Current Position', + '', + 'Status: Executing Phase 1', + 'Last Activity: 2026-01-01', + '', + '## Accumulated Context', + '', + '### Decisions', + '', + '- [Phase 1]: Use TypeScript', + '', + ].join('\n'); +} + +/** + * STATE.md with a canonical session section (the success path — must not regress). + */ +function buildStateMdWithCanonicalSessionSection() { + return [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Foundation', + 'status: executing', + 'last_updated: 2026-01-01T00:00:00.000Z', + '---', + '', + '# GSD State', + '', + '## Session', + '', + '**Last session:** 2026-01-01T00:00:00.000Z', + '**Stopped at:** Phase 1, Plan 1', + '**Resume file:** None', + '', + '## Accumulated Context', + '', + '### Decisions', + '', + '- Use TypeScript', + '', + ].join('\n'); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Bug #948: zero-match patch must leave STATE.md byte-identical +// ───────────────────────────────────────────────────────────────────────────── + +describe('#948: zero-match state patch must not rewrite STATE.md', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('STATE.md is byte-identical after a zero-match patch', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const original = buildStateMdWithStaleSectionAndRealFrontmatter({}); + fs.writeFileSync(statePath, original); + + // Patch a field that does NOT exist in the file — zero matches expected. + const result = runGsdTools('state patch --NonExistentFieldXYZ "some value"', tmpDir); + assert.ok(result.success, `state patch should exit 0: ${result.error}`); + + const patchOutput = JSON.parse(result.output); + assert.deepStrictEqual(patchOutput.updated, [], 'updated should be empty'); + assert.ok(Array.isArray(patchOutput.failed), 'failed should be an array'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.strictEqual(after, original, 'STATE.md must be byte-identical after zero-match patch'); + }); + + test('milestone_name is preserved after zero-match patch (not reset to template placeholder)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const original = buildStateMdWithStaleSectionAndRealFrontmatter({ + milestoneName: 'My Real Milestone', + }); + fs.writeFileSync(statePath, original); + + runGsdTools('state patch --NonExistentField "value"', tmpDir); + + const after = fs.readFileSync(statePath, 'utf-8'); + const fm = parseFrontmatter(after); + assert.strictEqual(fm['milestone_name'], 'My Real Milestone', + 'milestone_name must not be reset to template placeholder by zero-match patch'); + }); + + test('stopped_at frontmatter value is preserved after zero-match patch (via byte-identity)', () => { + // The no-op guard prevents ANY rewrite when nothing changed, so the + // frontmatter stopped_at is preserved because the file is never touched. + // The stale body value cannot win because syncStateFrontmatter is never called. + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const original = buildStateMdWithStaleSectionAndRealFrontmatter({ + fmStoppedAt: 'Phase 3, Plan 2 — newer value', + bodyStoppedAt: 'Phase 1, Plan 1 — stale historical value', + }); + fs.writeFileSync(statePath, original); + + runGsdTools('state patch --NonExistentField "value"', tmpDir); + + // The byte-identity test already covers this; this test confirms the key + // field specifically is intact. + const after = fs.readFileSync(statePath, 'utf-8'); + assert.strictEqual(after, original, + 'STATE.md must be byte-identical — stopped_at cannot be overwritten via a no-op patch'); + }); + + test('last_updated is not bumped by a zero-match patch', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const original = buildStateMdWithStaleSectionAndRealFrontmatter({ + lastUpdated: '2026-01-01T00:00:00.000Z', + }); + fs.writeFileSync(statePath, original); + + runGsdTools('state patch --NonExistentField "value"', tmpDir); + + const after = fs.readFileSync(statePath, 'utf-8'); + const fm = parseFrontmatter(after); + assert.strictEqual(fm['last_updated'], '2026-01-01T00:00:00.000Z', + 'last_updated must not be bumped when no fields were changed'); + }); + + test('a matching patch STILL updates STATE.md correctly (no regression)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const fixture = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Foundation', + 'status: executing', + 'last_updated: 2026-01-01T00:00:00.000Z', + '---', + '', + '# GSD State', + '', + '**Status:** In Progress', + '**Last Activity:** 2026-01-01', + '', + ].join('\n'); + fs.writeFileSync(statePath, fixture); + + const result = runGsdTools('state patch --Status "Phase complete — ready for verification"', tmpDir); + assert.ok(result.success, `state patch should succeed: ${result.error}`); + + const patchOutput = JSON.parse(result.output); + assert.ok(patchOutput.updated.includes('Status'), 'Status should be in updated list'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(after.includes('Phase complete — ready for verification'), + 'matching patch should update the field'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Bug #948: syncStateFrontmatter — milestone_name placeholder preservation +// ───────────────────────────────────────────────────────────────────────────── + +describe('#948: syncStateFrontmatter preserves milestone_name when derived is template placeholder', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('state sync preserves real milestone_name when disk yields only template placeholder', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + // Frontmatter has a real name, but no ROADMAP.md exists so getMilestoneInfo + // will fall back to the 'milestone' placeholder — must not overwrite. + const content = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v2.5', + 'milestone_name: Very Real Project Name', + 'status: executing', + '---', + '', + '# GSD State', + '', + 'Status: Executing Phase 1', + 'Last Activity: 2026-01-01', + '', + ].join('\n'); + fs.writeFileSync(statePath, content); + + const result = runGsdTools('state sync', tmpDir); + assert.ok(result.success, `state sync failed: ${result.error}`); + + const after = fs.readFileSync(statePath, 'utf-8'); + const fm = parseFrontmatter(after); + assert.strictEqual(fm['milestone_name'], 'Very Real Project Name', + 'milestone_name must not be reset to template placeholder by state sync'); + }); + + test('state sync runs successfully and preserves milestone_name (no corruption)', () => { + // state sync always rebuilds frontmatter from the body — the no-op guard + // applies to commands whose transform produces no change. state sync always + // writes because last_updated changes. This test verifies that a full sync + // cycle does not corrupt milestone_name when the placeholder is derived. + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const content = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v2.5', + 'milestone_name: Very Real Project Name', + 'status: executing', + '---', + '', + '# GSD State', + '', + 'Status: Executing Phase 1', + 'Last Activity: 2026-01-01', + '', + ].join('\n'); + fs.writeFileSync(statePath, content); + + const result = runGsdTools('state sync', tmpDir); + assert.ok(result.success, `state sync failed: ${result.error}`); + + const after = fs.readFileSync(statePath, 'utf-8'); + const fm = parseFrontmatter(after); + assert.strictEqual(fm['milestone_name'], 'Very Real Project Name', + 'state sync must not reset milestone_name to template placeholder'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Bug #944: record-session with no session section must persist supplied values +// ───────────────────────────────────────────────────────────────────────────── + +describe('#944: record-session persists values even when body lacks session labels', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('stopped-at and resume-file are present in STATE.md after record-session with no prior section', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateMdWithoutSessionSection()); + + const PINNED_MS = Date.parse('2026-06-09T12:00:00.000Z'); + const result = runGsdTools( + 'state record-session --stopped-at "Phase 2, Plan 3" --resume-file ".planning/phases/02/02-03-PLAN.md"', + tmpDir, + { GSD_TEST_MODE: '1', GSD_NOW_MS: String(PINNED_MS) }, + ); + assert.ok(result.success, `state record-session should exit 0: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.recorded, true, + 'recorded must be true when values were supplied and persisted'); + assert.ok(!output.reason || output.reason !== 'No session fields found in STATE.md', + 'must not return the silent no-op reason when values were supplied'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(after.includes('Phase 2, Plan 3'), + '--stopped-at value must appear in STATE.md'); + assert.ok(after.includes('.planning/phases/02/02-03-PLAN.md'), + '--resume-file value must appear in STATE.md'); + }); + + test('command does not silently no-op when values are supplied (recorded must not be false)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateMdWithoutSessionSection()); + + const result = runGsdTools( + 'state record-session --stopped-at "Phase 5, Plan 1"', + tmpDir, + ); + assert.ok(result.success, `should exit 0: ${result.error}`); + + const output = JSON.parse(result.output); + // The key contract: if values were supplied, recorded must be true. + assert.notStrictEqual(output.recorded, false, + 'recorded must not be false when --stopped-at was explicitly supplied'); + }); + + test('STATE.md with non-canonical session labels still persists supplied values', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + // Session section exists but uses non-canonical label shapes (table, alternate caps) + const nonCanonical = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Foundation', + 'status: executing', + 'last_updated: 2026-01-01T00:00:00.000Z', + '---', + '', + '# GSD State', + '', + '## Session Info', + '', + '| Field | Value |', + '|-------|-------|', + '| Last Session | 2026-01-01 |', + '| Stopped Here | Phase 1, Plan 1 |', + '', + ].join('\n'); + fs.writeFileSync(statePath, nonCanonical); + + const PINNED_MS = Date.parse('2026-06-09T15:00:00.000Z'); + const result = runGsdTools( + 'state record-session --stopped-at "Phase 3, Plan 2" --resume-file "none.md"', + tmpDir, + { GSD_TEST_MODE: '1', GSD_NOW_MS: String(PINNED_MS) }, + ); + assert.ok(result.success, `should exit 0: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.recorded, true, + 'recorded must be true when values are persisted via auto-create fallback'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(after.includes('Phase 3, Plan 2'), + '--stopped-at value must be present in STATE.md'); + assert.ok(after.includes('none.md'), + '--resume-file value must be present in STATE.md'); + }); + + test('record-session with no args against a body-less file returns recorded:false (no regression)', () => { + // When NO values are supplied and no session fields can be found/updated, + // recorded:false is the correct behaviour — we only changed the contract + // when the caller supplies values. + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateMdWithoutSessionSection()); + + const result = runGsdTools('state record-session', tmpDir); + assert.ok(result.success, `should exit 0: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.recorded, false, + 'recorded should still be false when no session fields exist AND no values were supplied'); + }); + + test('canonical session section still updates in place (no regression)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateMdWithCanonicalSessionSection()); + + const PINNED_MS = Date.parse('2026-06-09T18:00:00.000Z'); + const result = runGsdTools( + 'state record-session --stopped-at "Phase 2, Plan 4" --resume-file ".planning/phases/02/02-04-PLAN.md"', + tmpDir, + { GSD_TEST_MODE: '1', GSD_NOW_MS: String(PINNED_MS) }, + ); + assert.ok(result.success, `should exit 0: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.recorded, true, 'recorded should be true'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(after.includes('Phase 2, Plan 4'), 'stopped-at should be updated'); + assert.ok(after.includes('.planning/phases/02/02-04-PLAN.md'), 'resume-file should be updated'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Adversarial fixtures: malformed frontmatter, missing fields, CRLF +// ───────────────────────────────────────────────────────────────────────────── + +describe('#948/#944: adversarial fixture variants', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('zero-match patch on CRLF STATE.md leaves file unchanged', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + // Build with CRLF line endings + const original = buildStateMdWithStaleSectionAndRealFrontmatter({}).replace(/\n/g, '\r\n'); + fs.writeFileSync(statePath, original); + + runGsdTools('state patch --NonExistentFieldXYZ "value"', tmpDir); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.strictEqual(after, original, 'CRLF file must be byte-identical after zero-match patch'); + }); + + test('zero-match patch on STATE.md with missing frontmatter fields does not corrupt', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const minimal = [ + '---', + 'gsd_state_version: 1.0', + '---', + '', + '# GSD State', + '', + '**Status:** In Progress', + '', + ].join('\n'); + fs.writeFileSync(statePath, minimal); + + const result = runGsdTools('state patch --NonExistentField "value"', tmpDir); + assert.ok(result.success, `should exit 0: ${result.error}`); + + const patchOutput = JSON.parse(result.output); + assert.deepStrictEqual(patchOutput.updated, [], 'no fields should be updated'); + }); + + test('record-session with empty body still records when values supplied', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + // Body is entirely empty (only frontmatter) + const emptyBody = [ + '---', + 'gsd_state_version: 1.0', + 'status: planning', + '---', + '', + ].join('\n'); + fs.writeFileSync(statePath, emptyBody); + + const result = runGsdTools( + 'state record-session --stopped-at "Phase 1, Plan 1"', + tmpDir, + ); + assert.ok(result.success, `should exit 0: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.recorded, true, + 'should persist even into a body-less STATE.md'); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.ok(after.includes('Phase 1, Plan 1'), + '--stopped-at value must appear in STATE.md'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Adversarial review findings: in-place update for existing ## Session heading +// ───────────────────────────────────────────────────────────────────────────── + +describe('#944 adversarial: existing ## Session heading must be updated in place, not duplicated', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + /** + * HIGH finding: when a `## Session` heading already exists but uses + * non-canonical rows (e.g. a markdown table), the DWIM code was appending + * a second `## Session` block instead of normalizing the existing one. + * buildStateFrontmatter / cmdStateSnapshot both read only the FIRST match, + * so the newly-written Stopped at / Resume file end up in an ignored block. + */ + test('record-session with existing non-canonical ## Session block: exactly one ## Session block afterward', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const nonCanonicalWithHeading = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Foundation', + 'status: executing', + 'last_updated: 2026-01-01T00:00:00.000Z', + '---', + '', + '# GSD State', + '', + '## Session', + '', + '| Field | Value |', + '|-------|-------|', + '| Last Session | 2026-01-01 |', + '| Stopped Here | Phase 1, Plan 1 |', + '', + '## Accumulated Context', + '', + '- Decision: use TypeScript', + '', + ].join('\n'); + fs.writeFileSync(statePath, nonCanonicalWithHeading); + + const PINNED_MS = Date.parse('2026-06-09T20:00:00.000Z'); + const result = runGsdTools( + 'state record-session --stopped-at "Phase 4, Plan 2" --resume-file "resume.md"', + tmpDir, + { GSD_TEST_MODE: '1', GSD_NOW_MS: String(PINNED_MS) }, + ); + assert.ok(result.success, `record-session should exit 0: ${result.error}`); + + const after = fs.readFileSync(statePath, 'utf-8'); + + // (a) exactly ONE ## Session block — no duplicate + const sessionHeadingCount = (after.match(/^## Session\s*$/gm) || []).length; + assert.strictEqual(sessionHeadingCount, 1, + 'exactly ONE ## Session block must exist after record-session (no duplicate appended)'); + + // (b) supplied values are present in the file + assert.ok(after.includes('Phase 4, Plan 2'), + '--stopped-at value must be present in STATE.md'); + assert.ok(after.includes('resume.md'), + '--resume-file value must be present in STATE.md'); + }); + + test('record-session with existing non-canonical ## Session block: state-snapshot sees supplied stopped_at', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const nonCanonicalWithHeading = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Foundation', + 'status: executing', + 'last_updated: 2026-01-01T00:00:00.000Z', + '---', + '', + '# GSD State', + '', + '## Session', + '', + '| Field | Value |', + '|-------|-------|', + '| Last Session | 2026-01-01 |', + '| Stopped Here | Phase 1, Plan 1 |', + '', + ].join('\n'); + fs.writeFileSync(statePath, nonCanonicalWithHeading); + + const PINNED_MS = Date.parse('2026-06-09T20:30:00.000Z'); + runGsdTools( + 'state record-session --stopped-at "Phase 4, Plan 2" --resume-file "resume.md"', + tmpDir, + { GSD_TEST_MODE: '1', GSD_NOW_MS: String(PINNED_MS) }, + ); + + // (c) state-snapshot must see the written stopped_at in the session block + // (via buildStateFrontmatter frontmatter OR body Session section, first match) + const snapshotResult = runGsdTools('state-snapshot', tmpDir); + assert.ok(snapshotResult.success, `state-snapshot should exit 0: ${snapshotResult.error}`); + const snapshot = JSON.parse(snapshotResult.output); + assert.strictEqual( + snapshot.session && snapshot.session.stopped_at, + 'Phase 4, Plan 2', + `state-snapshot session.stopped_at must reflect "Phase 4, Plan 2", got: ${JSON.stringify(snapshot.session)}`, + ); + }); + + /** + * LOW finding: auto-created scaffold writes `**Last session:**` but + * cmdStateSnapshot only matched `**Last Date:**`, so session.last_date + * was null after auto-create despite a valid timestamp being written. + * Fix: teach the snapshot parser to also accept `**Last session:**`. + */ + test('state-snapshot returns non-null session.last_date after auto-create on body-less file', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateMdWithoutSessionSection()); + + const PINNED_MS = Date.parse('2026-06-09T21:00:00.000Z'); + const recResult = runGsdTools( + 'state record-session --stopped-at "Phase 1, Plan 1"', + tmpDir, + { GSD_TEST_MODE: '1', GSD_NOW_MS: String(PINNED_MS) }, + ); + assert.ok(recResult.success, `record-session should exit 0: ${recResult.error}`); + + const snapshotResult = runGsdTools('state-snapshot', tmpDir); + assert.ok(snapshotResult.success, `state-snapshot should exit 0: ${snapshotResult.error}`); + const snapshot = JSON.parse(snapshotResult.output); + assert.notStrictEqual( + snapshot.session && snapshot.session.last_date, + null, + `state-snapshot session.last_date must not be null after auto-create; got: ${JSON.stringify(snapshot.session)}`, + ); + }); +});