From 2b20b7e2cda45e7d7888e740ecf876243d4e2488 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 12 Aug 2026 09:14:54 -0400 Subject: [PATCH] =?UTF-8?q?fix(#3257):=20preserve=20full-line=20frontmatte?= =?UTF-8?q?r=20comments=20through=20the=20parse=E2=86=92reconstruct=20pair?= =?UTF-8?q?=20+=20syncStateFrontmatter=20(#3387)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3257: full-line frontmatter comments survive the parse→reconstruct pair AND a mutating state verb parseYamlRegion dropped column-0 # comments and reconstructFrontmatter rebuilt from Object.entries alone, so full-line comments were silently destroyed on every mutating STATE verb. Add failing-first regressions: 3 unit tests for the public pair (comment between keys, leading+trailing, consecutive) and an e2e test running a state verb (state update) on a commented STATE.md — the e2e exercises syncStateFrontmatter's fresh-derivedFm rebuild path, which is the actual loss site the issue is filed against. RED — fails on next; fix follows. * fix(#3257: preserve full-line frontmatter comments through parse→reconstruct AND syncStateFrontmatter Carry column-0 # comments through the frontmatter pair via a Symbol-keyed channel (FULL_LINE_COMMENTS): parseYamlRegion captures ^# lines and attaches them to the next top-level key (leading) or a trailing slot; reconstructFrontmatter re-emits them in place. The Symbol is invisible to Object.entries/keys/JSON, so every existing reader is unchanged; the channel is created only when a comment is seen, so comment-less frontmatter is byte-identical. CRITICAL (isolated review): syncStateFrontmatter rebuilds its target via buildStateFrontmatter (fresh object) + an Object.keys carry-forward, both of which skip the Symbol — so the pair-preserving channel was lost on the very STATE verbs the issue names. Export propagateCommentChannel(source, target) from frontmatter.cts and call it in syncStateFrontmatter before reconstruct, copying the channel onto derivedFm (leading filtered to keys still present so a deleted key's annotation drops with it, trailing preserved). Decision A. * chore(#3257: add changeset fragment * chore(#3257: backfill changeset PR number (#3387) --------- Co-authored-by: sim --- .changeset/loud-jade-canyon.md | 5 +++ src/frontmatter.cts | 71 ++++++++++++++++++++++++++++++++++ src/state.cts | 8 +++- tests/frontmatter.test.cjs | 41 ++++++++++++++++++++ tests/state.test.cjs | 33 ++++++++++++++++ 5 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 .changeset/loud-jade-canyon.md diff --git a/.changeset/loud-jade-canyon.md b/.changeset/loud-jade-canyon.md new file mode 100644 index 000000000..0919d9c39 --- /dev/null +++ b/.changeset/loud-jade-canyon.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3387 +--- +**Full-line `#` comments in `.planning/STATE.md` (and every frontmatter surface) now survive a mutating write** — `parseYamlRegion` carries column-0 comments through to `reconstructFrontmatter` via a Symbol-keyed channel, and `syncStateFrontmatter` propagates that channel across its fresh-rebuild of the frontmatter object, so a comment like `# NOTE: current_phase is hand-maintained` is no longer silently destroyed on the next `state` verb. Comment-less frontmatter is unchanged; data identity (keys/values/arrays/nested) is preserved alongside the comments. (#3257) diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 985ae629c..ee77e4e39 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -93,6 +93,20 @@ function isFrontmatterShaped(region: string): boolean { )); } +/** + * #3257: a Symbol-keyed channel that carries full-line (column-0 `#`) YAML + * comments through a parse → reconstruct round-trip. Comments are otherwise + * unrepresentable on the Frontmatter object (Record) and were + * silently dropped by reconstructFrontmatter. The Symbol is invisible to + * Object.entries / Object.keys / JSON.stringify / for-in, so every existing + * reader is unchanged; only reconstructFrontmatter reads it. Leading comments + * are attached to the top-level key that follows them; comments after the last + * key go to `trailing`. Only set when a comment is actually seen, so comment-less + * frontmatter parses byte-identically to before. + */ +const FULL_LINE_COMMENTS = Symbol('fullLineComments'); +type FullLineCommentChannel = { leading: Record; trailing: string[] }; + /** * Parse one already-delimited YAML region into a Frontmatter object. * @@ -104,6 +118,10 @@ function parseYamlRegion(yaml: string): Frontmatter { const frontmatter: Frontmatter = {}; const lines = yaml.split(/\r?\n/); + // #3257: pending column-0 full-line comments, attached to the next top-level key. + let pendingComments: string[] = []; + let commentChannel: FullLineCommentChannel | undefined; + // Stack to track nested objects: [{obj, key, indent}] type StackEntry = { obj: Record | unknown[]; key: string | null; indent: number }; const stack: StackEntry[] = [{ obj: frontmatter, key: null, indent: -1 }]; @@ -112,6 +130,12 @@ function parseYamlRegion(yaml: string): Frontmatter { // Skip empty lines if (line.trim() === '') continue; + // #3257: capture column-0 full-line comments; attach them to the next top-level key. + if (/^#/.test(line)) { + pendingComments.push(line); + continue; + } + // Calculate indentation (number of leading spaces) const indentMatch = line.match(/^(\s*)/); const indent = indentMatch ? indentMatch[1].length : 0; @@ -127,6 +151,12 @@ function parseYamlRegion(yaml: string): Frontmatter { const keyMatch = line.match(/^(\s*)([a-zA-Z0-9_-]+):\s*(.*)/); if (keyMatch) { const key = keyMatch[2]; + // #3257: attach any pending comments to this (top-level) key. + if (pendingComments.length) { + if (!commentChannel) commentChannel = { leading: {}, trailing: [] }; + commentChannel.leading[key] = pendingComments; + pendingComments = []; + } const value = keyMatch[3].trim(); if (value === '' || value === '[') { @@ -168,6 +198,15 @@ function parseYamlRegion(yaml: string): Frontmatter { } } + // #3257: trailing comments (after the last key) + attach the channel if any comment was seen. + if (pendingComments.length) { + if (!commentChannel) commentChannel = { leading: {}, trailing: [] }; + commentChannel.trailing = pendingComments; + } + if (commentChannel) { + (frontmatter as Record)[FULL_LINE_COMMENTS as unknown as symbol] = commentChannel; + } + return frontmatter; } @@ -282,8 +321,14 @@ function scalarNeedsDoubleQuoting(s: string): boolean { function reconstructFrontmatter(obj: Frontmatter): string { const lines: string[] = []; + // #3257: read the full-line-comment channel (set by parseYamlRegion when comments + // were present). Object.entries skips the Symbol key, so the data loop is unchanged. + const commentChannel = (obj as Record)[FULL_LINE_COMMENTS as unknown as symbol] as FullLineCommentChannel | undefined; for (const [key, value] of Object.entries(obj)) { if (value === null || value === undefined) continue; + // #3257: re-emit this key's leading full-line comments before the key itself. + const leading = commentChannel?.leading[key]; + if (leading) for (const c of leading) lines.push(c); if (Array.isArray(value)) { if (value.length === 0) { lines.push(`${key}: []`); @@ -343,9 +388,34 @@ function reconstructFrontmatter(obj: Frontmatter): string { } } } + // #3257: re-emit any trailing full-line comments (those after the last key). + if (commentChannel?.trailing?.length) { + for (const c of commentChannel.trailing) lines.push(c); + } return lines.join('\n'); } +/** + * #3257: copy the full-line-comment channel from `source` onto `target`, filtering + * `leading` to keys still present in `target` (a deleted key's annotation goes with + * it — AC5). No-op when `source` carries no channel. Consumers that rebuild their + * target object fresh (syncStateFrontmatter builds derivedFm via buildStateFrontmatter + * and copies keys with Object.keys, which skips the Symbol) MUST call this before + * reconstructFrontmatter, or the channel parseYamlRegion attached to the extracted + * source is lost. + */ +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; + const filtered: FullLineCommentChannel = { leading: {}, trailing: channel.trailing }; + for (const [key, comments] of Object.entries(channel.leading)) { + if (key in target) filtered.leading[key] = comments; + } + if (filtered.trailing.length || Object.keys(filtered.leading).length) { + (target as Record)[FULL_LINE_COMMENTS as unknown as symbol] = filtered; + } +} + /** * Slice a frontmatter YAML body into per-top-level-key raw text segments. Each segment * runs from a column-0 `key:` line through the line before the next column-0 key (or the @@ -826,4 +896,5 @@ export = { cmdFrontmatterSet, cmdFrontmatterMerge, cmdFrontmatterValidate, + propagateCommentChannel, }; diff --git a/src/state.cts b/src/state.cts index dd7e6d4db..a7226114e 100644 --- a/src/state.cts +++ b/src/state.cts @@ -27,7 +27,7 @@ const { planningDir, planningPaths } = planningWorkspace; import { realClock } from './clock.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); -const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter } = frontmatter; +const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter, propagateCommentChannel } = frontmatter; // eslint-disable-next-line @typescript-eslint/no-require-imports import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -2362,6 +2362,12 @@ function syncStateFrontmatter(content: string, cwd: string | undefined, authorit } } + // #3257: propagate full-line frontmatter comments from the extracted source onto the + // rebuilt derivedFm (buildStateFrontmatter + the Object.keys carry-forward above both + // skip the Symbol-keyed channel, so without this the comments would be lost here even + // though parseYamlRegion/reconstructFrontmatter preserve them in isolation). + propagateCommentChannel(existingFm as unknown as Frontmatter, derivedFm as unknown as Frontmatter); + const yamlStr = reconstructFrontmatter(derivedFm as unknown as Frontmatter); return `---\n${yamlStr}\n---\n\n${body}`; } diff --git a/tests/frontmatter.test.cjs b/tests/frontmatter.test.cjs index a0e09ace9..009646c67 100644 --- a/tests/frontmatter.test.cjs +++ b/tests/frontmatter.test.cjs @@ -295,6 +295,47 @@ describe('reconstructFrontmatter', () => { const extracted2 = extractFrontmatter(roundTrip); assert.deepStrictEqual(extracted2, extracted1, 'round-trip should preserve multiple data types'); }); + + test('#3257: full-line comments survive an extract→reconstruct round-trip', () => { + const original = '---\ngsd_state_version: 1.0\n# NOTE: current_phase is hand-maintained here\ncurrent_phase: 3\nstatus: executing\n---'; + const extracted = extractFrontmatter(original); + assert.strictEqual(extracted['gsd_state_version'], '1.0'); + assert.strictEqual(extracted['current_phase'], '3'); + + const reconstructed = reconstructFrontmatter(extracted); + // #3257: the comment must survive in place (between gsd_state_version and current_phase). + assert.ok( + reconstructed.includes('# NOTE: current_phase is hand-maintained here'), + `comment should survive reconstruct; got:\n${reconstructed}`, + ); + // data identity preserved alongside the comment. + assert.ok(reconstructed.includes('gsd_state_version: 1.0')); + assert.ok(reconstructed.includes('current_phase: 3')); + assert.ok(reconstructed.includes('status: executing')); + // the reconstructed output re-parses to the same data (idempotent round-trip). + const reextracted = extractFrontmatter(`---\n${reconstructed}\n---`); + assert.strictEqual(reextracted['current_phase'], '3'); + }); + + test('#3257: leading (before first key) and trailing (after last key) comments survive', () => { + const original = '---\n# top comment\na: 1\nb: 2\n# trailing comment\n---'; + const extracted = extractFrontmatter(original); + const reconstructed = reconstructFrontmatter(extracted); + assert.ok(reconstructed.includes('# top comment'), `leading comment lost:\n${reconstructed}`); + assert.ok(reconstructed.includes('# trailing comment'), `trailing comment lost:\n${reconstructed}`); + }); + + test('#3257: multiple consecutive comments survive in order', () => { + const original = '---\na: 1\n# first note\n# second note\nb: 2\n---'; + const extracted = extractFrontmatter(original); + const reconstructed = reconstructFrontmatter(extracted); + assert.ok(reconstructed.includes('# first note') && reconstructed.includes('# second note'), + `consecutive comments lost:\n${reconstructed}`); + const aIdx = reconstructed.indexOf('a: 1'); + const firstIdx = reconstructed.indexOf('# first note'); + const secondIdx = reconstructed.indexOf('# second note'); + assert.ok(aIdx < firstIdx && firstIdx < secondIdx, `order wrong (a:${aIdx} first:${firstIdx} second:${secondIdx})`); + }); }); // ─── spliceFrontmatter ────────────────────────────────────────────────────── diff --git a/tests/state.test.cjs b/tests/state.test.cjs index aea8966fd..aeee20566 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -915,6 +915,39 @@ team: platform assert.ok(content.includes('status: executing'), 'schema-owned status still preserved'); }); + test('#3257: full-line frontmatter comments survive a mutating state verb', () => { + // syncStateFrontmatter rebuilds frontmatter via buildStateFrontmatter (fresh object) + // + an Object.keys carry-forward. Without propagating the comment channel, the + // comment is lost HERE even though the parse→reconstruct pair preserves it in + // isolation. This is the e2e path the issue is filed against. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `--- +status: executing +milestone: v1.0 +# NOTE: current_phase is hand-maintained here while the roadmap is in flux +current_phase: 3 +--- + +# Project State + +**Current Phase:** 03 +**Current Plan:** 03-02 +` + ); + + // Any writeStateMd triggers syncStateFrontmatter. + runGsdTools('state update "Current Plan" "03-03"', tmpDir); + + const content = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + content.includes('# NOTE: current_phase is hand-maintained here while the roadmap is in flux'), + `full-line frontmatter comment must survive a mutating verb; got:\n${content}`, + ); + // The mutation itself still applied. + assert.ok(content.includes('03-03'), 'the state update still took effect'); + }); + test('round-trip: write then read via state json', () => { fs.writeFileSync( path.join(tmpDir, '.planning', 'STATE.md'),