diff --git a/.changeset/calm-goats-bark.md b/.changeset/calm-goats-bark.md new file mode 100644 index 000000000..fa9021cbf --- /dev/null +++ b/.changeset/calm-goats-bark.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4453 +--- +**`state begin-phase` no longer rewrites prose that merely quotes a bold field label** — a `**Status:**` (or any served field label) quoted mid-sentence inside prose captured the field rewrite and silently destroyed the rest of its line; the bold form is now anchored to line start, so only the real field updates. Frontmatter round-trip through begin-phase (custom keys, progress subkeys, milestone identity without a ROADMAP) is pinned with regression tests. (#4243) diff --git a/src/state-document.cts b/src/state-document.cts index a8fc16bb8..15f6bb7cf 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -553,7 +553,22 @@ export function stateReplaceField(content: string, fieldName: string, newValue: // `(.*)` captured the following line and the rebuild discarded it — the #4010 // data-loss. ADR-3180 §7.7 makes stateExtractField the same-line-confined owner; // this aligns the writer to it. - const boldPattern = new RegExp(`(\\*\\*${escaped}:\\*\\*[ \\t]*)(.*)`, 'i'); + // + // #4243: the bold form is also ANCHORED to line start, with same-line leading + // whitespace only. The pre-fix pattern carried no `^` and no `m` flag, so a + // bold label quoted MID-SENTENCE inside prose — an Accumulated Context bullet + // mentioning `**Status:**` — captured the rewrite and destroyed the rest of + // its line, silently, whenever a whole-body caller fed this function every + // section (beginPhaseCore's tryField, advancePlanCore's Status/Current Plan + // writes). The plain branch below was always line-anchored; only the bold + // branch lagged. Anchoring reuses #4010's same-line confinement idiom (the + // leading class is `[ \t]*`, deliberately NOT the `\s*` the issue suggested — + // `^\s*\*\*` can consume the newlines before the label into the match and + // drop them on rebuild) and #4186's recognition-by-anchoring discipline: a + // write target must BE the whole declared line shape, never a substring + // guess inside prose. `$` is explicit-and-inert (`.` never crosses line + // terminators) and documents that the match ends at end-of-line. + const boldPattern = new RegExp(`^([ \\t]*\\*\\*${escaped}:\\*\\*[ \\t]*)(.*)$`, 'im'); if (boldPattern.test(content)) { return content.replace(boldPattern, (_match, prefix: string) => joinFieldReplacement(prefix, newValue)); } diff --git a/tests/state-document.test.cjs b/tests/state-document.test.cjs index 4a4ada130..9c397bdf2 100644 --- a/tests/state-document.test.cjs +++ b/tests/state-document.test.cjs @@ -221,6 +221,187 @@ describe('stateReplaceField — empty field preserves the following line (#4010) }); }); +// #4243: the bold branch of stateReplaceField was UNANCHORED +// (`(\*\*Field:\*\*[ \t]*)(.*)` with no ^ and no /m), so a bold label quoted +// MID-SENTENCE inside prose — the issue's `**Status:**` inside an Accumulated +// Context bullet — captured the rewrite and destroyed the rest of the line, +// silently, while the real field went stale or was updated elsewhere. The fix +// anchors the bold form to line start with same-line leading whitespace +// (`^([ \t]*\*\*Field:\*\*[ \t]*)`, 'im'), reusing #4010's same-line +// confinement idiom and #4186's recognition-by-anchoring discipline. These rows +// pin the corruption shapes; rows further down pin the negative space +// (legitimate line-start bold updates are byte-identical, branch order and +// first-occurrence-wins unchanged). +describe('stateReplaceField — anchored bold form leaves prose lookalikes untouched (#4243)', () => { + // The issue's verbatim prose line: a bold label quoted for documentation + // purposes inside a bullet, with the real field in the plain template form. + const ISSUE_PROSE_LINE = + '- [Phase 170]: archived files gained a `**Status:**Ready to execute` marker. Must not change.'; + + // ROW 1 — the failing-first regression from the issue. The lookalike must + // survive byte-identically and the REAL plain field must take the update. + test('issue repro: mid-sentence **Status:** lookalike survives, real plain field updates', () => { + const input = [ + '## Current Position', + '', + 'Phase: 5 of 9', + 'Plan: 2 of 6', + 'Status: Ready to execute', + 'Last activity: 2026-08-01 — did a thing', + '', + '## Accumulated Context', + '', + '### Decisions', + '', + ISSUE_PROSE_LINE, + '', + ].join('\n'); + const result = stateReplaceField(input, 'Status', 'Executing Phase 901'); + assert.notEqual(result, null, 'the real plain field must still match'); + assert.ok( + result.includes(ISSUE_PROSE_LINE), + `prose lookalike must survive byte-identically, got:\n${result}`, + ); + assert.ok( + /^Status: Executing Phase 901$/m.test(result), + 'the real plain Status line must take the update', + ); + assert.ok( + !result.includes('Executing Phase 901` marker'), + 'the rewrite must not bleed into the prose occurrence', + ); + }); + + test('lookalike ordered BEFORE the real bold field: prose survives, bold field updates', () => { + const input = [ + '## Accumulated Context', + '', + ISSUE_PROSE_LINE, + '', + '## Current Position', + '', + '**Status:** Ready to execute', + '', + ].join('\n'); + const result = stateReplaceField(input, 'Status', 'Executing Phase 901'); + assert.notEqual(result, null); + assert.ok(result.includes(ISSUE_PROSE_LINE), `prose lookalike must survive, got:\n${result}`); + assert.ok( + /^\*\*Status:\*\* Executing Phase 901$/m.test(result), + 'the real line-start bold field must take the update', + ); + }); + + test('mid-word lookalike with no real field: returns null (honest absence), never a rewrite', () => { + const input = 'Prose mentions text**Status:**tail mid-word and nothing else.'; + assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 901'), null); + }); + + // A list-item bold label (`- **Status:** value`) is prose-shaped for the + // writer: no STATE.md writer emits body fields as list items, and treating + // a bullet as a field write target is exactly the #4243 corruption class. + // The read side's own vocabulary (bold anywhere) is untouched; the writer + // reports honest absence instead. + test('list-item bold label is not a write target: returns null, bullet untouched', () => { + const input = '- **Status:** resolved in the archived review'; + assert.equal(stateReplaceField(input, 'Status', 'new'), null); + }); + + // Every field the regex serves: a document whose ONLY occurrence of the + // label is a mid-sentence lookalike must yield null — no served field may + // be rewritten from prose. + test('mid-sentence lookalike yields null for every served field', () => { + const servedFields = [ + 'Status', 'Phase', 'Plan', 'Current Plan', 'Current Phase', 'Current Phase Name', + 'Last Activity', 'Last Activity Description', 'Total Phases', 'Total Plans in Phase', + 'Progress', 'Completed Phases', 'Stopped At', + ]; + for (const field of servedFields) { + const input = `Some prose sentence quoting a **${field}:** label mid-sentence, plus trailing words.`; + assert.equal( + stateReplaceField(input, field, 'NEW'), + null, + `mid-sentence **${field}:** lookalike must not match (got a rewrite)`, + ); + } + }); + + test('lookalike plus real plain field: only the real plain line changes (representative fields)', () => { + const cases = [ + { field: 'Status', plain: 'Status: Ready to execute' }, + { field: 'Phase', plain: 'Phase: 5 of 9' }, + { field: 'Last Activity', plain: 'Last Activity: 2026-08-01 — did a thing' }, + ]; + for (const { field, plain } of cases) { + const lookalike = `- notes: the **${field}:** label was archived here. Keep it.`; + const input = [plain, '', '## Accumulated Context', '', lookalike, ''].join('\n'); + const result = stateReplaceField(input, field, 'NEW VALUE'); + assert.notEqual(result, null, `${field}: real plain field must match`); + assert.ok( + result.includes(lookalike), + `${field}: lookalike line must survive byte-identically, got:\n${result}`, + ); + } + }); + + // Negative space: an INDENTED line-start bold field is still a field (the + // doc's form ranking reads bold anywhere in the section; the writer keeps + // same-line indentation writable), and the indent is preserved. + test('indented line-start bold field still updates, indent preserved', () => { + const input = ' **Status:** old'; + const result = stateReplaceField(input, 'Status', 'new'); + assert.equal(result, ' **Status:** new'); + }); + + // Negative space + fix-shape pin: leading blank lines before the label are + // NOT swallowed. The anchor's leading class is same-line whitespace only + // (`[ \t]*`, #4010's idiom); the issue's suggested `^\s*` variant would + // consume the newlines into the match and drop them on rebuild. + test('leading blank lines before a bold label survive byte-identically', () => { + const input = '\n\n**Status:** Ready'; + const result = stateReplaceField(input, 'Status', 'Executing Phase 5'); + assert.equal(result, '\n\n**Status:** Executing Phase 5'); + }); + + test('CRLF document: lookalike survives with CRLF intact, real plain field updates', () => { + const input = [ + 'Status: Ready to execute', + '', + '## Accumulated Context', + '', + ISSUE_PROSE_LINE, + '', + ].join('\r\n'); + const result = stateReplaceField(input, 'Status', 'Executing Phase 901'); + assert.notEqual(result, null); + assert.ok(result.includes(ISSUE_PROSE_LINE), `prose lookalike must survive, got:\n${result}`); + assert.ok(result.includes('\r\n'), 'CRLF endings must be preserved'); + assert.ok(/^Status: Executing Phase 901\r?$/m.test(result), 'real plain field must update'); + }); + + // Negative space: branch ORDER is unchanged — a line-start bold field still + // beats the plain form, and only the first bold occurrence is replaced. + test('line-start bold still beats the plain form (branch order unchanged)', () => { + const input = '**Status:** old bold\nStatus: old plain'; + const result = stateReplaceField(input, 'Status', 'new'); + assert.equal(result, '**Status:** new\nStatus: old plain'); + }); + + test('two line-start bold occurrences: only the first is replaced', () => { + const input = '**Status:** first\n**Status:** second'; + const result = stateReplaceField(input, 'Status', 'new'); + assert.equal(result, '**Status:** new\n**Status:** second'); + }); + + // #4010 same-line adjacency under the anchor: an empty bold field's value + // lands on its own line and the following line survives. + test('anchored bold branch keeps the #4010 empty-field boundary', () => { + const input = ' **Status:**\n **Current Plan:** 2 of 5'; + const result = stateReplaceField(input, 'Status', 'Executing Phase 5'); + assert.equal(result, ' **Status:** Executing Phase 5\n **Current Plan:** 2 of 5'); + }); +}); + describe('stateExtractField (#2880)', () => { test('extracts from a two-cell row', () => { const input = '| Current Phase | 3 |'; diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 0e7daf716..e924f597b 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -3892,6 +3892,255 @@ Progress: [..........] 0% }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #4243 — begin-phase: prose bold-lookalikes stay untouched (anchored bold +// form in stateReplaceField) and frontmatter round-trips unknown keys +// ───────────────────────────────────────────────────────────────────────────── + +describe('#4243: begin-phase leaves prose lookalikes untouched, preserves unknown frontmatter', () => { + const ISSUE_PROSE_LINE = + '- [Phase 170]: archived files gained a `**Status:**Ready to execute` marker. Must not change.'; + + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // The issue's suggested regression fixture 1, verbatim shape: a bold + // `**Status:**` inside prose (## Accumulated Context), the real field in + // the plain template form. begin-phase must rewrite ONLY the real field. + test('issue fixture 1: prose **Status:** lookalike is byte-identical, real field updates', () => { + writeState(tmpDir, [ + '# Project State', + '', + '## Current Position', + 'Phase: 5 of 9 (Fifth)', + 'Plan: 2 of 6 in current phase', + 'Status: Ready to execute', + 'Last activity: 2026-08-01 — did a thing', + '', + 'Progress: [████░░░░░░] 40%', + '', + '## Accumulated Context', + '', + '### Decisions', + '', + ISSUE_PROSE_LINE, + '', + ].join('\n')); + + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '901', '--plans', '3'], + tmpDir, + ); + assert.ok(result.success, `begin-phase failed: ${result.error}`); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + content.includes(ISSUE_PROSE_LINE), + `prose lookalike must survive begin-phase byte-identically, got:\n${content}`, + ); + const pos = sectionMatchOf(content, 'Current Position'); + assert.ok(pos, 'Current Position section should exist'); + assert.match(pos[1], /^Status: Executing Phase 901$/m); + }); + + // Corruption shape 1b: the lookalike section ordered BEFORE ## Current + // Position, real field in the bold form — first-match-in-document-order is + // the prose under the unanchored pattern. + test('lookalike before Current Position: prose survives, real bold field updates', () => { + writeState(tmpDir, [ + '# Project State', + '', + '## Accumulated Context', + '', + ISSUE_PROSE_LINE, + '', + '## Current Position', + '', + 'Phase: 5 of 9', + 'Plan: 2 of 6', + '**Status:** Ready to execute', + 'Last activity: 2026-08-01 — did a thing', + '', + ].join('\n')); + + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '901', '--plans', '3'], + tmpDir, + ); + assert.ok(result.success, `begin-phase failed: ${result.error}`); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + content.includes(ISSUE_PROSE_LINE), + `prose lookalike must survive begin-phase byte-identically, got:\n${content}`, + ); + assert.match(content, /^\*\*Status:\*\* Executing Phase 901$/m); + }); + + // Corruption shape 1c: lookalikes for OTHER served fields — a mid-sentence + // `**Last Activity:**` must not capture the Last-activity refresh. + test('mid-sentence **Last Activity:** lookalike survives, real field refreshes', () => { + const lookalike = 'An earlier note mentions **Last Activity:** thresholds for archival. Keep.'; + writeState(tmpDir, [ + '# Project State', + '', + '## Current Position', + 'Phase: 5 of 9', + 'Plan: 2 of 6', + 'Status: Ready to execute', + 'Last activity: 2026-08-01 — did a thing', + '', + '## Accumulated Context', + '', + lookalike, + '', + ].join('\n')); + + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '901', '--plans', '3'], + tmpDir, + ); + assert.ok(result.success, `begin-phase failed: ${result.error}`); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(content.includes(lookalike), `lookalike must survive, got:\n${content}`); + const pos = sectionMatchOf(content, 'Current Position'); + assert.ok(pos, 'Current Position section should exist'); + assert.match(pos[1], /^Last activity: \d{4}-\d{2}-\d{2}/m); + }); + + // The issue's suggested regression fixture 2, no-ROADMAP arm: a custom + // frontmatter key, a populated progress block (with a custom subkey), a + // curated stopped_at, and milestone identity — all must survive begin-phase + // without a ROADMAP.md, and milestone/milestone_name must NOT be reset to + // invented defaults. (Already-correct behavior on next via the #2202 + // carry-forward + #3216 milestone-identity fix + the #4129 ratchet; pinned + // here so the class cannot regress.) + const FRONTMATTER_FIXTURE = [ + '---', + "gsd_state_version: '1.0'", + 'milestone: v2.1', + 'milestone_name: Real Curated Name', + 'status: planning', + "stopped_at: '2026-08-01 — curated stop note'", + 'custom_key: hand-added-by-agent', + 'progress:', + ' total_phases: 9', + ' completed_phases: 4', + ' total_plans: 30', + ' completed_plans: 12', + ' percent: 40', + ' custom_subkey: 77', + '---', + '', + '# Project State', + '', + '## Current Position', + 'Phase: 5 of 9 (Fifth)', + 'Plan: 2 of 6 in current phase', + 'Status: Ready to execute', + 'Last activity: 2026-08-01 — did a thing', + '', + ].join('\n'); + + test('issue fixture 2 (no ROADMAP): unknown keys, progress subkeys, stopped_at, milestone survive', () => { + writeState(tmpDir, FRONTMATTER_FIXTURE); + + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '901', '--name', 'Nine-Oh-One', '--plans', '3'], + tmpDir, + ); + assert.ok(result.success, `begin-phase failed: ${result.error}`); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(content); + assert.equal(fm['custom_key'], 'hand-added-by-agent', 'custom frontmatter key must survive'); + assert.equal(fm['milestone'], 'v2.1', 'milestone must not be reset to an invented default'); + assert.equal(fm['milestone_name'], 'Real Curated Name', 'curated milestone_name must survive'); + assert.ok(String(fm['stopped_at']).includes('curated stop note'), 'stopped_at must survive'); + const progress = fm['progress']; + assert.ok(progress && typeof progress === 'object', 'progress block must survive'); + // Numeric-tolerant: reconstructFrontmatter may serialize an unknown subkey + // as a quoted scalar, so it re-parses as a string — the VALUE surviving is + // the contract, not the YAML scalar shape. + assert.equal(Number(progress['custom_subkey']), 77, 'custom progress subkey must survive'); + // With ROADMAP.md absent and a milestone asserted, the #3573/#4094 withhold + // keeps all four counters at their STORED values (pinned below) and omits + // percent (an unmeasured scan must not assert one, #3233). `percent` is a + // DECLARED derived subkey governed by that recorded semantics — unlike the + // custom subkey above, its absence here is the documented behavior, so this + // row deliberately does not pin its value in either arm. + assert.equal(Number(progress['total_phases']), 9, 'stored total_phases kept under the #3573 withhold'); + assert.equal(Number(progress['completed_phases']), 4, 'stored completed_phases kept under the #3573 withhold'); + assert.equal(Number(progress['total_plans']), 30, 'stored total_plans kept under the #3573 withhold'); + assert.equal(Number(progress['completed_plans']), 12, 'stored completed_plans kept under the #3573 withhold'); + // Known keys still take the begin-phase update. + assert.equal(fm['status'], 'executing'); + assert.equal(String(fm['current_phase']), '901'); + }); + + test('issue fixture 2 (with ROADMAP): unknown keys and progress subkeys survive', () => { + writeState(tmpDir, FRONTMATTER_FIXTURE); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + ['# Roadmap', '', '## v2.1 — Real Curated Name', '', '### Phase 5: Fifth', '', 'complete.', ''].join('\n'), + ); + + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '901', '--name', 'Nine-Oh-One', '--plans', '3'], + tmpDir, + ); + assert.ok(result.success, `begin-phase failed: ${result.error}`); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(content); + assert.equal(fm['custom_key'], 'hand-added-by-agent', 'custom frontmatter key must survive'); + assert.equal(fm['milestone'], 'v2.1'); + assert.equal(fm['milestone_name'], 'Real Curated Name'); + const progress = fm['progress']; + assert.ok(progress && typeof progress === 'object', 'progress block must survive'); + assert.equal(Number(progress['custom_subkey']), 77, 'custom progress subkey must survive'); + assert.equal(fm['status'], 'executing'); + }); + + test('milestone without milestone_name, no ROADMAP: no invented identity appears', () => { + writeState(tmpDir, [ + '---', + "gsd_state_version: '1.0'", + 'milestone: v2.1', + 'custom_key: keep-me', + '---', + '', + '# Project State', + '', + '## Current Position', + 'Phase: 5 of 9', + 'Plan: 2 of 6', + 'Status: Ready to execute', + '', + ].join('\n')); + + const result = runGsdTools( + ['state', 'begin-phase', '--phase', '901', '--plans', '3'], + tmpDir, + ); + assert.ok(result.success, `begin-phase failed: ${result.error}`); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const fm = frontmatterLib.extractFrontmatter(content); + assert.equal(fm['milestone'], 'v2.1', 'milestone must survive without a ROADMAP'); + assert.equal(fm['milestone_name'], undefined, 'no fabricated milestone_name may appear'); + assert.equal(fm['custom_key'], 'keep-me'); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // Bug #1589 — progress counters not updated during plan execution // ─────────────────────────────────────────────────────────────────────────────