diff --git a/docs/adr/1769-state-md-transition-module.md b/docs/adr/1769-state-md-transition-module.md index 8e60714f1..2b8184b1c 100644 --- a/docs/adr/1769-state-md-transition-module.md +++ b/docs/adr/1769-state-md-transition-module.md @@ -213,5 +213,5 @@ alongside, leave callbacks — parallel worlds don't converge (ADR-857's failure | 3 | `completePhase` + `phase.cts:1770` | #1784 | — | | 4 | `plannedPhase` + `milestoneSwitch` | #1786 | — | | 5 | `milestoneComplete` + `milestone.cts:352` | #1789 | — | -| 6 | `patch` | TBD | #1743, #1695 | +| 6 | `patch` | #1791 | #1743, #1695 | | 7 | `sync`, `prune`, `update` | TBD | #1760, #1761 | diff --git a/gsd-core/bin/lib/state-transition.cjs b/gsd-core/bin/lib/state-transition.cjs index 4569d1fc9..bf7650c1c 100644 --- a/gsd-core/bin/lib/state-transition.cjs +++ b/gsd-core/bin/lib/state-transition.cjs @@ -125,6 +125,8 @@ function transitionCore(content, intent, deps) { return milestoneSwitchCore(content, intent, deps); case 'milestoneComplete': return milestoneCompleteCore(content, intent, deps); + case 'patch': + return patchCore(content, intent); } } // ---------------------------------------------------------------------------- @@ -878,9 +880,8 @@ function milestoneCompleteCore(content, intent, deps) { updated.push('Last Activity Description'); } // ## Current Position reset — stop resume/progress flows pointing at closed - // execution instructions. allow-adhoc-markdown: pre-seam section write-modify; - // pending collectSection migration #1372. - const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i; + // execution instructions. + const positionPattern = /(##\s*Current Position\s*\n)([\s\S]*?)(?=\n##|$)/i; // allow-adhoc-markdown: pre-seam section write-modify carried from milestone.cts; pending collectSection migration #1372 const closedPositionBody = `\nPhase: Milestone ${version} complete\n` + `Plan: —\n` + `Status: Awaiting next milestone\n` + @@ -893,8 +894,7 @@ function milestoneCompleteCore(content, intent, deps) { } updated.push('Current Position'); // ## Operator Next Steps — normalize stale tails that can persist after close. - // allow-adhoc-markdown: pre-seam section write-modify; pending collectSection migration #1372. - const operatorPattern = /(##\s*Operator Next Steps\s*\n)([\s\S]*?)(?=\n##|$)/i; + const operatorPattern = /(##\s*Operator Next Steps\s*\n)([\s\S]*?)(?=\n##|$)/i; // allow-adhoc-markdown: pre-seam section write-modify carried from milestone.cts; pending collectSection migration #1372 if (operatorPattern.test(body)) { body = body.replace(operatorPattern, `$1\n- Start the next milestone with ${intent.nextMilestoneCommand}\n\n`); } @@ -904,3 +904,40 @@ function milestoneCompleteCore(content, intent, deps) { updated.push('Operator Next Steps'); return { content: reassemble(body), updated }; } +// ---------------------------------------------------------------------------- +// patch — intent implementation (Phase 6) +// ---------------------------------------------------------------------------- +/** + * Apply a `patch` transition to STATE.md content. + * + * Migrates `cmdStatePatch` (state.cts) onto the substrate. Applies each + * caller-supplied `{field: value}` pair via `stateReplaceField` over the full + * content (body + frontmatter — patch can target either), tracking which fields + * were updated vs. not found. + * + * The curated-field preservation that fixes #1743/#1695 is NOT in this core — + * it lives in `readModifyWriteStateMd`'s post-sync delta (table-driven via + * `getFieldClassification('current_phase_name').preservation === 'preserve-always'`). + * `patch` consulting the table "refuses to overwrite" curated fields implicitly: + * when the patch does not change a curated field's body source line, the + * existing frontmatter value wins over the sync re-derivation. The adapter + * still owns field-name validation (security) and the resync-progress decision. + * + * `data.updated` / `data.failed` mirror the pre-migration CLI output shape. + */ +function patchCore(content, intent) { + const updated = []; + const failed = []; + let result = content; + for (const [field, value] of Object.entries(intent.patches)) { + const replaced = (0, state_document_cjs_1.stateReplaceField)(result, field, value); + if (replaced !== null) { + result = replaced; + updated.push(field); + } + else { + failed.push(field); + } + } + return { content: result, updated, data: { updated, failed } }; +} diff --git a/scripts/lint-regression-test-names.allowlist.json b/scripts/lint-regression-test-names.allowlist.json index 79fca1d2f..e4446e9e8 100644 --- a/scripts/lint-regression-test-names.allowlist.json +++ b/scripts/lint-regression-test-names.allowlist.json @@ -5,6 +5,7 @@ "bug-1367-claude-local-flat-command-layout.test.cjs", "bug-14-progress-auto-flag-dropped.test.cjs", "bug-167-query-meta-command.test.cjs", + "bug-1695-state-patch-clobbers-phase-name.test.cjs", "bug-17-askuserquestion-option-cap.test.cjs", "bug-170-workflow-fallback-install-hint.test.cjs", "bug-1736-local-install-commands.test.cjs", diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index dcf03c2b1..879d79bea 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -99,6 +99,7 @@ }, "state": { "files": [ + "bug-1695-state-patch-clobbers-phase-name.test.cjs", "bug-21-state-md-template-frontmatter.test.cjs", "bug-2630-state-frontmatter-milestone-switch.test.cjs", "bug-3127-state-begin-phase-idempotent.test.cjs", diff --git a/src/state-transition.cts b/src/state-transition.cts index babbcf36f..981ad12fe 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -179,8 +179,9 @@ export type StateTransitionIntent = version: string; /** Resolved runtime slash command for the Operator Next Steps hint (e.g. '/gsd:new-milestone'). */ nextMilestoneCommand: string; - }; -// Phases 6–7 add the remaining intent kinds to this discriminated union. + } + | { kind: 'patch'; patches: Record }; +// Phase 7 adds the remaining intent kinds to this discriminated union. export type StateTransitionResult = { content: string; @@ -221,6 +222,8 @@ export function transitionCore( return milestoneSwitchCore(content, intent, deps); case 'milestoneComplete': return milestoneCompleteCore(content, intent, deps); + case 'patch': + return patchCore(content, intent); } } @@ -1128,3 +1131,46 @@ function milestoneCompleteCore( return { content: reassemble(body), updated }; } + +// ---------------------------------------------------------------------------- +// patch — intent implementation (Phase 6) +// ---------------------------------------------------------------------------- + +/** + * Apply a `patch` transition to STATE.md content. + * + * Migrates `cmdStatePatch` (state.cts) onto the substrate. Applies each + * caller-supplied `{field: value}` pair via `stateReplaceField` over the full + * content (body + frontmatter — patch can target either), tracking which fields + * were updated vs. not found. + * + * The curated-field preservation that fixes #1743/#1695 is NOT in this core — + * it lives in `readModifyWriteStateMd`'s post-sync delta (table-driven via + * `getFieldClassification('current_phase_name').preservation === 'preserve-always'`). + * `patch` consulting the table "refuses to overwrite" curated fields implicitly: + * when the patch does not change a curated field's body source line, the + * existing frontmatter value wins over the sync re-derivation. The adapter + * still owns field-name validation (security) and the resync-progress decision. + * + * `data.updated` / `data.failed` mirror the pre-migration CLI output shape. + */ +function patchCore( + content: string, + intent: { kind: 'patch'; patches: Record }, +): StateTransitionResult { + const updated: string[] = []; + const failed: string[] = []; + let result = content; + + for (const [field, value] of Object.entries(intent.patches)) { + const replaced = stateReplaceField(result, field, value); + if (replaced !== null) { + result = replaced; + updated.push(field); + } else { + failed.push(field); + } + } + + return { content: result, updated, data: { updated, failed } }; +} diff --git a/src/state.cts b/src/state.cts index 935628a2b..1e0eda460 100644 --- a/src/state.cts +++ b/src/state.cts @@ -32,7 +32,7 @@ const { extractFrontmatter, reconstructFrontmatter } = frontmatter; import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import stateTransitionMod = require('./state-transition.cjs'); -const { transitionCore } = stateTransitionMod; +const { transitionCore, getFieldClassification } = stateTransitionMod; type StateTransitionIntent = stateTransitionMod.StateTransitionIntent; type StateTransitionDeps = stateTransitionMod.StateTransitionDeps; import { @@ -397,21 +397,19 @@ function cmdStatePatch(cwd: string, patches: Record, raw: boolea const statePath = planningPaths(cwd).state; try { - const results: { updated: string[]; failed: string[] } = { updated: [], failed: [] }; const shouldResync = shouldResyncStateProgress(Object.keys(patches)); - // Use atomic read-modify-write to prevent lost updates from concurrent agents + // ADR-1769 Phase 6: dispatches to the STATE.md Transition Module. The + // per-patch stateReplaceField loop is the pure `patchCore` in + // src/state-transition.cts. readModifyWriteStateMd still owns the lock, the + // #1230/#1264 post-sync preservation, AND the #1695 curated-current_phase_name + // delta (table-driven) that this phase adds. Field-name validation (security) + // and the resync-progress decision stay in this adapter. + let results: { updated: string[]; failed: string[] } = { updated: [], failed: [] }; readModifyWriteStateMd(statePath, (content) => { - for (const [field, value] of Object.entries(patches)) { - const result = stateReplaceField(content, field, value); - if (result) { - content = result; - results.updated.push(field); - } else { - results.failed.push(field); - } - } - return content; + const result = transitionCore(content, { kind: 'patch', patches }, { clock: realClock, progressProvider: () => null }); + results = (result.data as { updated: string[]; failed: string[] }) ?? results; + return result.content; }, cwd, { resync: shouldResync }); output(results, raw, results.updated.length > 0 ? 'true' : 'false'); @@ -1892,6 +1890,14 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string const preSessionScope = preSessionMatch ? preSessionMatch[1] : preBody; const preBodyStoppedAt = stateExtractField(preSessionScope, 'Stopped At') || stateExtractField(preSessionScope, 'Stopped at'); + // ADR-1769 Phase 6 / #1743 / #1695: snapshot the body source for the curated + // current_phase_name (the `Phase:` line parseProsePhaseField harvests). When + // this write does NOT change that line, the curated frontmatter value must + // win over syncStateFrontmatter's body re-derivation (which can harvest a + // wrong parenthetical aside — #1695). Gated by the field-classification + // table's preserve-always row so the rule lives in one place. + const preBodyPhaseSource = stateExtractField(preBody, 'Phase'); + const modified = transformFn(content); // Bug #948: no-op guard — if the transform produced no change, do NOT write @@ -1922,6 +1928,9 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string const postSessionMatch = matchSessionSection(postBody); const postSessionScope = postSessionMatch ? postSessionMatch[1] : postBody; const postBodyStoppedAt = stateExtractField(postSessionScope, 'Stopped At') || stateExtractField(postSessionScope, 'Stopped at'); + // ADR-1769 Phase 6 / #1695: post-transform body Phase source for the + // current_phase_name delta comparison. + const postBodyPhaseSource = stateExtractField(postBody, 'Phase'); let mutated = false; const postFm = extractFrontmatter(synced) as Record; @@ -1963,6 +1972,26 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string mutated = true; } + // ADR-1769 Phase 6 / #1743 / #1695: same delta heuristic for the curated + // current_phase_name. Gated by the field-classification table (preserve-always). + // When this write did NOT change the body `Phase:` source line, the curated + // frontmatter current_phase_name wins over syncStateFrontmatter's body + // re-derivation (parseProsePhaseField can harvest a wrong parenthetical + // aside — #1695). begin/planned/complete-phase rewrite their body Phase line, + // so the delta does not fire for them and current_phase_name still advances. + const phaseNameCls = getFieldClassification('current_phase_name'); + if ( + phaseNameCls !== null && + phaseNameCls.preservation === 'preserve-always' && + postBodyPhaseSource === preBodyPhaseSource && + typeof preFmSnapshot['current_phase_name'] === 'string' && + preFmSnapshot['current_phase_name'].length > 0 && + postFm['current_phase_name'] !== preFmSnapshot['current_phase_name'] + ) { + postFm['current_phase_name'] = preFmSnapshot['current_phase_name']; + mutated = true; + } + if (mutated) { const yamlStr = reconstructFrontmatter(postFm as unknown as Frontmatter); const body = stripFrontmatter(synced); diff --git a/tests/bug-1695-state-patch-clobbers-phase-name.test.cjs b/tests/bug-1695-state-patch-clobbers-phase-name.test.cjs new file mode 100644 index 000000000..30ec14b4c --- /dev/null +++ b/tests/bug-1695-state-patch-clobbers-phase-name.test.cjs @@ -0,0 +1,113 @@ +'use strict'; +// Regression test for issue #1695 — `state patch` of an unrelated field clobbers +// the curated `current_phase_name` frontmatter scalar. +// +// Root cause: readModifyWriteStateMd({resync:false}) still runs syncStateFrontmatter, +// which re-derives EVERY body-derived scalar from body prose. The #1264 restore +// covers `progress` only and #1230 covers `status`/`stopped_at`; `current_phase_name` +// was left exposed, and parseProsePhaseField's paren-over-dash preference made the +// re-derived value wrong (harvesting a parenthetical aside as the phase name). +// +// ADR-1769 Phase 6 fix: extend the #1230 delta heuristic to current_phase_name +// (gated by the field-classification table's preserve-always row). When the +// transform did NOT change the body Current Phase / Phase source line, the curated +// frontmatter value wins. + +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 } = require('./helpers.cjs'); +const { extractFrontmatter } = require('../gsd-core/bin/lib/frontmatter.cjs'); + +function buildStateWithCuratedPhaseName({ phaseName = 'Native Global Hotkey', aside = 'next; Phase 15 landed, UAT deferred' } = {}) { + return [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Test', + 'current_phase: "16"', + `current_phase_name: "${phaseName}"`, + 'status: executing', + 'progress:', + ' total_phases: 20', + ' completed_phases: 15', + ' total_plans: 40', + ' completed_plans: 30', + ' percent: 75', + '---', + '', + '# GSD State', + '', + '## Configuration', + 'Current Phase: 16', + 'Total Plans in Phase: 4', + 'Current Plan: 2', + 'Status: Executing Phase 16', + 'Last Activity: 2026-06-20', + '', + '## Current Position', + '', + `Phase: 16 — ${phaseName} (${aside})`, + 'Plan: 2 of 4', + 'Status: Executing Phase 16', + 'Last activity: 2026-06-20 — mid-flight', + '', + ].join('\n'); +} + +function readFm(statePath) { + return extractFrontmatter(fs.readFileSync(statePath, 'utf-8')); +} + +describe('#1695: state patch of an unrelated field preserves curated current_phase_name', () => { + let tmpDir; + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + test('patching Status does NOT clobber the curated current_phase_name', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateWithCuratedPhaseName()); + + const result = runGsdTools(['query', 'state.patch', JSON.stringify({ Status: 'Paused for review' })], tmpDir); + assert.ok(result.success, `state patch failed: ${result.error}`); + + const fm = readFm(statePath); + assert.strictEqual( + fm.current_phase_name, + 'Native Global Hotkey', + `current_phase_name must be preserved on an unrelated patch; got ${JSON.stringify(fm.current_phase_name)} (the paren-over-dash re-derivation clobbered it — #1695)`, + ); + }); + + test('patching Current Plan does NOT clobber the curated current_phase_name', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateWithCuratedPhaseName()); + + const result = runGsdTools(['query', 'state.patch', JSON.stringify({ 'Current Plan': '3' })], tmpDir); + assert.ok(result.success, `state patch failed: ${result.error}`); + + const fm = readFm(statePath); + assert.strictEqual(fm.current_phase_name, 'Native Global Hotkey'); + }); + + test('explicitly patching the body Phase name-source line still advances (delta does not over-pin)', () => { + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, buildStateWithCuratedPhaseName()); + + // Patching the body 'Phase' field (the parseProsePhaseField source for + // current_phase_name) changes the source line, so the #1230 delta must NOT + // fire — syncStateFrontmatter re-derives current_phase_name from the new line. + // (Acceptance criterion from #1743: the guard must not pin a scalar whose body + // source genuinely changed.) + const result = runGsdTools(['query', 'state.patch', JSON.stringify({ Phase: '17 — Brand New Phase Name' })], tmpDir); + assert.ok(result.success, `state patch failed: ${result.error}`); + + const fm = readFm(statePath); + // current_phase_name should be re-derived from the new body 'Phase' line + // (not pinned to the old curated value). + assert.notStrictEqual(fm.current_phase_name, 'Native Global Hotkey', + `current_phase_name must advance when the body Phase source changed; got ${JSON.stringify(fm.current_phase_name)}`); + }); +}); diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 0823b5c28..7e39a23ea 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -1032,3 +1032,56 @@ describe('ADR-1769 Phase 5: milestoneComplete transition — closure write', () assert.ok(/^milestone: v1\.0/m.test(result.content), 'frontmatter milestone preserved'); }); }); + +// ADR-1769 Phase 6: patch + +describe('ADR-1769 Phase 6: patch transition — field updates', () => { + const deps = { clock: fixedClock, progressProvider: noProgress }; + + test('applies each patched field and reports the updated set', () => { + const input = [ + '# Project State', + '', + '**Status:** Planning', + '**Current Plan:** 2', + '**Total Plans in Phase:** 5', + '', + ].join('\n'); + const result = transitionCore( + input, + { kind: 'patch', patches: { Status: 'Paused', 'Current Plan': '3' } }, + deps, + ); + assert.strictEqual(stateExtractField(result.content, 'Status'), 'Paused'); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '3'); + assert.deepStrictEqual(result.data && result.data.updated, ['Status', 'Current Plan']); + }); + + test('reports failed fields (no matching field in content)', () => { + const input = '# Project State\n\n**Status:** Planning\n'; + const result = transitionCore( + input, + { kind: 'patch', patches: { Status: 'Paused', Nonexistent: 'x' } }, + deps, + ); + assert.deepStrictEqual(result.data && result.data.updated, ['Status']); + assert.deepStrictEqual(result.data && result.data.failed, ['Nonexistent']); + }); + + test('leaves content unchanged when no patch matches (no-op)', () => { + const input = '# Project State\n\n**Status:** Planning\n'; + const result = transitionCore(input, { kind: 'patch', patches: { Nonexistent: 'x' } }, deps); + assert.strictEqual(result.content, input); + assert.deepStrictEqual(result.data && result.data.updated, []); + assert.deepStrictEqual(result.data && result.data.failed, ['Nonexistent']); + }); + + test('patching a frontmatter YAML key directly updates the YAML line', () => { + // patch operates on the full content (body + frontmatter), so a lowercase + // frontmatter key like `stopped_at` is matched and replaced. + const input = ['---', 'status: executing', 'stopped_at: 2026-01-01', '---', '', '# State', ''].join('\n'); + const result = transitionCore(input, { kind: 'patch', patches: { stopped_at: '2026-06-27' } }, deps); + assert.ok(/^stopped_at: 2026-06-27$/m.test(result.content), 'YAML stopped_at must be patched'); + assert.deepStrictEqual(result.data && result.data.updated, ['stopped_at']); + }); +});