diff --git a/.changeset/happy-jaguars-climb.md b/.changeset/happy-jaguars-climb.md new file mode 100644 index 000000000..bbafb56da --- /dev/null +++ b/.changeset/happy-jaguars-climb.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3952 +--- +**STATE.md frontmatter comments now survive every state write** — a column-0 comment no longer depends on an unrelated body line being present, and an indented comment above the `progress:` counters (the natural provenance spot) is preserved instead of silently stripped. (#3742) diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 0ebaef63e..942fa90a8 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -357,32 +357,72 @@ function normalizeParsedValue(value: unknown, inArray: boolean): unknown { */ function extractCommentChannel(yaml: string, orderedKeys: string[]): FullLineCommentChannel | undefined { const lines = splitLines(yaml); - let pending: string[] = []; + // #3742: pending full-line comments carry their indentation so an INDENTED + // comment (` # note` above a nested key) can attach to the nested key that + // follows it — recorded under a dotted path key (`progress.total_phases`) + // that reconstructFrontmatter re-emits at the same nesting depth. Column-0 + // comments keep the exact pre-#3742 behavior (top-level key attachment). + let pending: Array<{ indent: number; line: string }> = []; let channel: FullLineCommentChannel | undefined; let keyIdx = 0; + // Stack of enclosing mapping keys with their indentation, for dotted-path + // construction on nested key lines. Only indented keys push here. + const pathStack: Array<{ indent: number; key: string }> = []; + + const attach = (pathKey: string, comments: Array<{ indent: number; line: string }>): void => { + if (!channel) channel = { leading: Object.create(null) as Record, trailing: [] }; + // Null-prototype `leading` (post-#3881-review, finding 3): the path key is + // derived from arbitrary user-authored YAML keys — `constructor`, + // `__proto__`, `toString`, `valueOf`, `hasOwnProperty` all round-trip + // through here. On an ordinary `{}` those resolve to inherited + // Object.prototype members; the null prototype makes every lookup an + // own-property-or-undefined read. + channel.leading[pathKey] = comments.map((c) => c.line); + }; for (const line of lines) { if (line.trim() === '') continue; - if (/^#/.test(line)) { - pending.push(line); + const commentMatch = /^(\s*)#/.exec(line); + if (commentMatch) { + pending.push({ indent: commentMatch[1].length, line }); continue; } - if (!/^\s/.test(line) && keyIdx < orderedKeys.length) { - const key = orderedKeys[keyIdx]; - if (line.startsWith(`${key}:`) || line.startsWith(`"${key}"`) || line.startsWith(`'${key}'`)) { - if (pending.length) { - // Null-prototype `leading` (post-#3881-review, finding 3): `key` is an arbitrary - // user-authored YAML key — `constructor`, `__proto__`, `toString`, `valueOf`, - // `hasOwnProperty` all round-trip through here. On an ordinary `{}` those resolve - // to inherited Object.prototype members, and `reconstructFrontmatter`'s - // `commentChannel?.leading[key]` read then finds e.g. the `Object.prototype.toString` - // FUNCTION instead of `undefined`, throwing when it is iterated as if it were an - // array of comment strings. - if (!channel) channel = { leading: Object.create(null) as Record, trailing: [] }; - channel.leading[key] = pending; + // A list item (`- foo: bar`) is not a mapping key: its `- ` prefix would + // otherwise register as a key named `- foo` and corrupt the path stack + // (#3742 review). List items fall through to the pending-drop below. + const isListItem = /^\s*-\s/.test(line); + const keyLineMatch = isListItem + ? null + : /^(\s*)(?:"([^"]+)"|'([^']+)'|([^:\s][^:]*)):(?:\s|$)/.exec(line); + if (keyLineMatch) { + const indent = keyLineMatch[1].length; + const key = keyLineMatch[2] ?? keyLineMatch[3] ?? keyLineMatch[4]; + if (indent === 0) { + // Top-level: keep the pre-#3742 orderedKeys walk — the comment + // attaches only to the next EXPECTED top-level key. + if (keyIdx < orderedKeys.length && key === orderedKeys[keyIdx]) { + const col0 = pending.filter((c) => c.indent === 0); + if (col0.length) attach(key, col0); + keyIdx++; + // A top-level mapping key opens a nesting context for the indented + // keys that follow it (#3742 dotted-path attachment). + pathStack.length = 0; + pathStack.push({ indent: 0, key }); pending = []; + continue; } - keyIdx++; + } else { + // Nested key line: a pending comment at the SAME indentation attaches + // to this key under its dotted path. Deeper/misaligned pending + // comments were not leading this key — drop them, matching the + // top-level rule's "attach only when a key follows" discipline. + while (pathStack.length > 0 && pathStack[pathStack.length - 1].indent >= indent) pathStack.pop(); + const sameIndent = pending.filter((c) => c.indent === indent); + if (sameIndent.length && key.length > 0) { + attach([...pathStack.map((e) => e.key), key].join('.'), sameIndent); + } + pathStack.push({ indent, key }); + pending = []; continue; } } @@ -392,9 +432,10 @@ function extractCommentChannel(yaml: string, orderedKeys: string[]): FullLineCom pending = []; } - if (pending.length) { + const col0Trailing = pending.filter((c) => c.indent === 0); + if (col0Trailing.length) { if (!channel) channel = { leading: Object.create(null) as Record, trailing: [] }; - channel.trailing = pending; + channel.trailing = col0Trailing.map((c) => c.line); } return channel; } @@ -806,6 +847,10 @@ function reconstructFrontmatter(obj: Frontmatter): string { lines.push(`${key}:`); for (const [subkey, subval] of Object.entries(value)) { if (subval === null || subval === undefined) continue; + // #3742: re-emit a nested key's leading full-line comments (channel + // path key `parent.subkey`) at the subkey's own indentation. + const nestedLeading = commentChannel?.leading[`${key}.${subkey}`]; + if (nestedLeading) for (const c of nestedLeading) lines.push(` ${c.trimStart()}`); if (Array.isArray(subval)) { if (subval.length === 0) { lines.push(` ${subkey}: []`); @@ -821,6 +866,10 @@ function reconstructFrontmatter(obj: Frontmatter): string { lines.push(` ${subkey}:`); for (const [subsubkey, subsubval] of Object.entries(subval as Record)) { if (subsubval === null || subsubval === undefined) continue; + // #3742: same nested-comment re-emission one level deeper + // (`parent.sub.subsub`). + const deepLeading = commentChannel?.leading[`${key}.${subkey}.${subsubkey}`]; + if (deepLeading) for (const c of deepLeading) lines.push(` ${c.trimStart()}`); if (Array.isArray(subsubval)) { if (subsubval.length === 0) { lines.push(` ${subsubkey}: []`); @@ -869,18 +918,49 @@ function reconstructFrontmatter(obj: Frontmatter): string { function propagateCommentChannel(source: Frontmatter, target: Frontmatter): void { const channel = (source as Record)[FULL_LINE_COMMENTS as unknown as symbol] as FullLineCommentChannel | undefined; if (!channel) return; + // #3742: two changes, both about a target that is a PARTIAL rebuild. + // + // (a) Root-segment membership: a comment keyed by a dotted path + // (`progress.total_plans`) survives while its root section survives — + // requiring the full path to resolve inside `target` would drop every + // nested comment the moment the rebuild reconstructed the section + // object (a fresh object with the same leaf keys still matches at + // EMIT time; membership is about the section existing at all). + // (b) Merge, not clobber: `target` may already carry its own channel + // (extracted from content that kept some comments). Target entries win + // for the same key; source entries fill the gaps; trailing lists + // concatenate (source first, mirroring document order when the source + // is the earlier snapshot). + const hasOwn = (o: object, k: string) => Object.prototype.hasOwnProperty.call(o, k); + const rootAlive = (k: string): boolean => { + const root = k.split('.')[0]; + return hasOwn(target, root); + }; // Null-prototype `leading` (post-#3881-review, finding 3) — same rationale as // `extractCommentChannel`. `target` may be a plain `{}` built by a caller outside this - // module (e.g. `buildStateFrontmatter`), so `key in target` is checked via + // module (e.g. `buildStateFrontmatter`), so membership is checked via // `hasOwnProperty`, not the `in` operator: `in` walks target's OWN prototype chain too, // and a target key named `constructor`/`toString`/etc. would otherwise read as "present" // even when it was never actually set. - const filtered: FullLineCommentChannel = { leading: Object.create(null) as Record, trailing: channel.trailing }; - for (const [key, comments] of Object.entries(channel.leading)) { - if (Object.prototype.hasOwnProperty.call(target, key)) filtered.leading[key] = comments; + const existingChannel = (target as Record)[FULL_LINE_COMMENTS as unknown as symbol] as FullLineCommentChannel | undefined; + // Trailing: when the target already carries a channel, its trailing list is + // the SAME comments re-parsed from content this lineage already emitted — + // concatenating would duplicate them on every write (unbounded growth, + // #3742 review). Take the target's list; only a channel-less target + // (a fresh rebuild, e.g. buildStateFrontmatter output) inherits the + // source's trailing comments. + const merged: FullLineCommentChannel = { + leading: Object.create(null) as Record, + trailing: existingChannel ? existingChannel.trailing : channel.trailing, + }; + for (const [key, comments] of Object.entries(existingChannel?.leading ?? {})) { + merged.leading[key] = comments; } - if (filtered.trailing.length || Object.keys(filtered.leading).length) { - (target as Record)[FULL_LINE_COMMENTS as unknown as symbol] = filtered; + for (const [key, comments] of Object.entries(channel.leading)) { + if (!hasOwn(merged.leading, key) && rootAlive(key)) merged.leading[key] = comments; + } + if (merged.trailing.length || Object.keys(merged.leading).length) { + (target as Record)[FULL_LINE_COMMENTS as unknown as symbol] = merged; } } diff --git a/src/state.cts b/src/state.cts index f808744d1..4caaa34c3 100644 --- a/src/state.cts +++ b/src/state.cts @@ -3641,6 +3641,23 @@ function applyPostSyncPreservation( } if (preservation.mutated || authoritativeReasserted) { + // #3742: preservation RESTORES frontmatter keys the body-derived rebuild + // could not produce (e.g. `current_phase` on a layout with no body + // `**Current Phase:**` line) — but the comment channel was filtered + // against the pre-restore key set during sync, so a full-line comment + // attached to a restored key died with nothing to re-attach it. Propagate + // the channel from the PRE-WRITE snapshot here, after the restores, so a + // comment's survival depends on its key surviving the whole write — not + // on which body line happened to feed the rebuild. Merge semantics + // (propagateCommentChannel) keep any channel the synced content already + // carried. No resync gate: this is the RMW path, where `resync` is the + // DEFAULT (readModifyWriteStateMd derives it as `options.resync !== + // false`) and preservation itself runs regardless — the factory-reset + // semantic the #3742 review worried about lives in writeStateMd's + // `rebuild` transactions, which never reach this branch. + if (preFmSnapshot && !isUnparseableFrontmatter(preFmSnapshot)) { + propagateCommentChannel(preFmSnapshot as unknown as Frontmatter, preservation.postFm as unknown as Frontmatter); + } const yamlStr = reconstructFrontmatter(preservation.postFm as unknown as Frontmatter); const body = stripFrontmatter(syncedContent); return `---\n${yamlStr}\n---\n\n${body}`; diff --git a/tests/frontmatter.unit.test.cjs b/tests/frontmatter.unit.test.cjs index 8c200beed..1277ca9bc 100644 --- a/tests/frontmatter.unit.test.cjs +++ b/tests/frontmatter.unit.test.cjs @@ -27,6 +27,7 @@ const { FRONTMATTER_SCHEMAS, agentScalarNeedsDoubleQuoting, escapeDoubleQuotedScalar, + propagateCommentChannel, } = require('../gsd-core/bin/lib/frontmatter.cjs'); // ─── extractFrontmatter ─────────────────────────────────────────────────────── @@ -1741,3 +1742,118 @@ describe('escapeDoubleQuotedScalar: exact output strings', () => { assert.equal(escapeDoubleQuotedScalar(input), expected); }); }); + +// ─── #3742: nested (path-shaped) comment channel + merge propagation ───────── +// Direct unit coverage for the #3742 channel extension — the mutation shard +// for frontmatter runs THIS file, so each clause of the new code needs a +// paired positive/negative assertion here (the #1882/#3706/#3888 trap: tests +// living only in tests/state.test.cjs do not constrain this shard). +describe('#3742: extractCommentChannel — indented comments attach by dotted path', () => { + const channelOf = (doc) => { + const e = extractFrontmatter(doc); + const sym = Object.getOwnPropertySymbols(e).find((x) => String(x).includes('fullLineComments')); + return sym ? e[sym] : null; + }; + + test('an indented comment above a nested key attaches to parent.key', () => { + const ch = channelOf(['---','a:',' # note',' b: 1','---'].join('\n')); + assert.ok(ch, 'channel must exist'); + assert.deepEqual(ch.leading['a.b'], [' # note']); + assert.equal(ch.leading['b'], undefined, 'no top-level key named b exists'); + }); + + test('two levels deep: parent.sub.subsub path', () => { + const ch = channelOf(['---','a:',' b:',' # deep',' c: 1','---'].join('\n')); + assert.deepEqual(ch.leading['a.b.c'], [' # deep']); + }); + + test('a MISALIGNED indent comment before a shallower key is dropped, not misattached', () => { + const ch = channelOf(['---','a:',' b: 1',' # misplaced','c: 2','---'].join('\n')); + assert.equal(ch, null, 'no comment attached to any key'); + }); + + test('a comment above a LIST ITEM is dropped (list items are not keys)', () => { + const ch = channelOf(['---','a:',' # above item',' - one','b: 2','---'].join('\n')); + assert.equal(ch, null); + }); + + test('a list-item mapping line (- k: v) does not register a key', () => { + const ch = channelOf(['---','a:',' # n',' - k: v','---'].join('\n')); + assert.equal(ch, null, 'the comment must not attach to a "- k" pseudo-key'); + }); + + test('column-0 semantics unchanged: attach to next top-level key, drop on non-key', () => { + const ch = channelOf(['---','a: 1','# c1','# c2','b: 2','---'].join('\n')); + assert.deepEqual(ch.leading['b'], ['# c1', '# c2']); + }); + + test('quoted top-level keys still walk orderedKeys', () => { + const ch = channelOf(['---','"a b": 1','# q','c: 2','---'].join('\n')); + assert.deepEqual(ch.leading['c'], ['# q']); + }); +}); + +describe('#3742: reconstructFrontmatter — nested comments re-emit at their indent', () => { + test('level-1 and level-2 nested comments re-emit with matching indentation', () => { + const doc = ['---','a:',' # l1',' b:',' # l2',' c: 1','---'].join('\n'); + const out = reconstructFrontmatter(extractFrontmatter(doc)); + assert.ok(out.split('\n').includes(' # l1'), 'l1 comment re-emits at two-space indent'); + assert.ok(out.split('\n').includes(' # l2'), 'l2 comment re-emits at four-space indent'); + // order: l1 above b, l2 above c + assert.ok(out.indexOf(' # l1') < out.indexOf(' b:'), 'l1 comment precedes its key'); + assert.ok(out.indexOf(' # l2') < out.indexOf(' c:'), 'l2 comment precedes its key'); + }); + + test('round-trip is idempotent (extract→reconstruct twice is a fixpoint)', () => { + const doc = ['---','x:',' # keep',' y: 1','---'].join('\n'); + const once = '---\n' + reconstructFrontmatter(extractFrontmatter(doc)) + '\n---'; + const twice = '---\n' + reconstructFrontmatter(extractFrontmatter(once)) + '\n---'; + assert.equal(twice, once); + assert.equal((twice.match(/# keep/g) || []).length, 1); + }); +}); + +describe('#3742: propagateCommentChannel — merge, root filter, trailing dedupe', () => { + const symOf = (o) => Object.getOwnPropertySymbols(o).find((x) => String(x).includes('fullLineComments')); + + test('a dotted-path key survives while its root section exists in the target', () => { + const src = extractFrontmatter(['---','p:',' # n',' q: 1','---'].join('\n')); + const target = { p: { q: 9 } }; + propagateCommentChannel(src, target); + const ch = target[symOf(target)]; + assert.deepEqual(ch.leading['p.q'], [' # n']); + }); + + test('a dotted-path key is DROPPED when the root section is absent from the target', () => { + const src = extractFrontmatter(['---','p:',' # n',' q: 1','---'].join('\n')); + const target = { other: 1 }; + propagateCommentChannel(src, target); + const ch = target[symOf(target)]; + assert.ok(!ch || !ch.leading['p.q'], 'comment must die with its section'); + }); + + test('target channel wins per key; source fills gaps (merge, not clobber)', () => { + const src = extractFrontmatter(['---','# from-source','a: 1','# from-source-2','b: 2','---'].join('\n')); + const target = extractFrontmatter(['---','# from-target','a: 1','b: 2','---'].join('\n')); + propagateCommentChannel(src, target); + const ch = target[symOf(target)]; + assert.deepEqual(ch.leading['a'], ['# from-target'], 'target entry wins'); + assert.deepEqual(ch.leading['b'], ['# from-source-2'], 'source fills the gap for a key the target owns but has no comment for'); + }); + + test('trailing list comes from the target when it has a channel (no duplication)', () => { + const src = extractFrontmatter(['---','a: 1','# trail','---'].join('\n')); + const target = extractFrontmatter(['---','a: 1','# trail','---'].join('\n')); + propagateCommentChannel(src, target); + const ch = target[symOf(target)]; + assert.deepEqual(ch.trailing, ['# trail'], 'exactly one trailing comment after merge'); + }); + + test('a channel-less target inherits the source trailing comments', () => { + const src = extractFrontmatter(['---','a: 1','# trail','---'].join('\n')); + const target = { a: 2 }; + propagateCommentChannel(src, target); + const ch = target[symOf(target)]; + assert.deepEqual(ch.trailing, ['# trail']); + }); +}); diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 4df676a33..fc90f26c3 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -999,6 +999,114 @@ current_phase: 3 assert.ok(content.includes('03-03'), 'the state update still took effect'); }); + // #3742 — comment survival must not depend on the document body: a + // column-0 comment died whenever the body had no **Current Phase:** line + // (the preservation restore re-added the key but nothing re-attached the + // comment channel), and an indented comment under progress: died always + // (the channel only ever knew top-level keys). + test('#3742: comments survive begin-phase with and without a body Current Phase line', () => { + const COL0 = '# PROVENANCE-COL0: hand-counted; do not resync'; + const NESTED = '# PROVENANCE-NESTED: nested under progress'; + for (const withBodyLine of [true, false]) { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + COL0, + 'current_phase: 01', + 'current_phase_name: probe-phase', + 'status: executing', + 'last_updated: "2026-08-10T00:00:00.000Z"', + 'progress:', + ' ' + NESTED, + ' total_phases: 2', + ' completed_phases: 0', + ' total_plans: 1', + ' completed_plans: 0', + '---', + '', + '## Current Position', + '', + '**Status:** Executing', + ...(withBodyLine ? ['**Current Phase:** 01'] : []), + '', + ].join('\n'), + ); + + runGsdTools('state begin-phase --phase 01 --name probe-phase --plans 1', tmpDir); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + content.includes(COL0), + `#3742: column-0 comment must survive begin-phase (body Current Phase line: ${withBodyLine}); got:\n${content}`, + ); + assert.ok( + content.includes(NESTED), + `#3742: indented comment under progress must survive begin-phase (body Current Phase line: ${withBodyLine}); got:\n${content}`, + ); + } + }); + + test('#3742: comments survive state update (the issue\'s anomaly verb)', () => { + const COL0 = '# PROVENANCE-COL0: do not resync'; + const NESTED = '# PROVENANCE-NESTED: nested under progress'; + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + COL0, + 'current_phase: 3', + 'status: executing', + 'progress:', + ' ' + NESTED, + ' total_phases: 9', + '---', + '', + '# Project State', + '', + '**Current Phase:** 03', + '**Status:** Executing', + '', + ].join('\n'), + ); + + runGsdTools('state update Status Paused', tmpDir); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(content.includes(COL0), `#3742: column-0 comment must survive state update; got:\n${content}`); + assert.ok(content.includes(NESTED), `#3742: nested comment must survive state update; got:\n${content}`); + }); + + test('#3742: trailing comments do not duplicate across repeated writes', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + [ + '---', + 'gsd_state_version: 1.0', + 'current_phase: 3', + 'status: executing', + '# TRAILING: do not resync', + '---', + '', + '# Project State', + '', + '**Current Phase:** 03', + '**Status:** Executing', + '', + ].join('\n'), + ); + + for (let i = 0; i < 3; i++) { + runGsdTools('state update Status Paused', tmpDir); + } + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + const count = (content.match(/# TRAILING: do not resync/g) || []).length; + assert.strictEqual(count, 1, `#3742: trailing comment must appear exactly once after repeated writes, got ${count}:\n${content}`); + }); + test('round-trip: write then read via state json', () => { fs.writeFileSync( path.join(tmpDir, '.planning', 'STATE.md'),