From f615eb9ef39477cbede130f4c2b9a099022dc4c6 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 24 Jun 2026 13:30:46 -0400 Subject: [PATCH] fix(#1572): preserve must_haves object-lists across frontmatter set/merge (#1656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1572): preserve must_haves object-lists across frontmatter set/merge spliceFrontmatter round-tripped the WHOLE frontmatter through extractFrontmatter (a scalar-only parser) then reconstructFrontmatter (a lossy serializer), so any must_haves object-list — artifacts {path, provides}, prohibitions {statement, status} — was flattened to scalar strings and re-emitted as a malformed inline array whenever an UNRELATED field changed, silently dropping every provides:/ status: value. The write now preserves the original raw text for any top-level key whose value is structurally unchanged between the original parse and the new object (generalizing the existing whole-document no-op guard to per-key fidelity), and regenerates only the key that actually changed. The key set is still defined by newObj (the cmdSet/cmdMerge flow always passes the full merged object). spliceFrontmatter's only callers are cmdFrontmatterSet/Merge — the STATE.md read-modify-write family calls reconstructFrontmatter directly and is unaffected. Regression cases folded into tests/frontmatter-cli.test.cjs: artifacts/prohibitions object-lists survive set and merge; idempotent on repeat sets. Asserted via parseMustHavesBlock (the structure-preserving parser). * chore(#1572): backfill changeset pr ref to 1656 * fix(#1572): fail-closed when set/merge would emit [object Object] (codex review) Adversarial review (codex, gpt-5.5/high) flagged that directly setting a must_haves object-list (a CHANGED key) still routed through the lossy reconstructFrontmatter, emitting literal "[object Object]" and destroying the data. The reported case (mutating an UNRELATED field) was already fixed by per-key raw-text preservation, but the changed-object-list path was still silently lossy. Add fail-closed: when a regenerated key's text contains the "[object Object]" sentinel, spliceFrontmatter throws — cmdFrontmatterSet/Merge error out WITHOUT writing, directing the user to edit the file directly. The no-frontmatter (generate-from-scratch) path is guarded the same way. Adds a test that a refused set leaves the file unchanged and the original object-list intact. Codex finding #2 (a contrived flattened-projection no-op) is a deeper limitation noted in the PR — non-destructive, and the fail-closed message already directs users to edit object-list blocks directly. * test(#1572): add spliceFrontmatter per-key preservation + fail-closed unit coverage Stryker mutates gsd-core/bin/lib/frontmatter.cjs against tests/frontmatter.{property,unit}.test.cjs (MinScore 62). The #1572 regression cases live in frontmatter-cli.test.cjs, which is NOT in Stryker's test set, so the new functions (sliceTopLevelFrontmatterSegments, the per-key preserve/regenerate/drop/append loop, regenerateFrontmatterKey's [object Object] fail-closed) had surviving mutants that dropped the module below threshold. Add unit-level coverage in frontmatter.unit.test.cjs exercising every new branch directly via spliceFrontmatter: unchanged object-list preserved (provides survives) when a scalar sibling changes; changed scalar regenerates only that key; orphan keys dropped; new keys appended; indented nested block stays attached to its parent key; whole-document no-op returns input verbatim; both fail-closed paths (changed object-list + no-frontmatter) throw. --- .changeset/agile-koalas-tumble.md | 5 ++ src/frontmatter.cts | 120 +++++++++++++++++++++++++--- tests/frontmatter-cli.test.cjs | 128 ++++++++++++++++++++++++++++++ tests/frontmatter.unit.test.cjs | 79 ++++++++++++++++++ 4 files changed, 319 insertions(+), 13 deletions(-) create mode 100644 .changeset/agile-koalas-tumble.md diff --git a/.changeset/agile-koalas-tumble.md b/.changeset/agile-koalas-tumble.md new file mode 100644 index 000000000..c9904dae5 --- /dev/null +++ b/.changeset/agile-koalas-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1656 +--- +**`frontmatter set` / `frontmatter merge` no longer destroy `must_haves` object-lists** — changing one frontmatter field (e.g. `wave`) silently dropped every `provides:` value and collapsed `must_haves.artifacts`/`.prohibitions` from a structured `[{path, provides}]` list into a malformed inline array, because the whole frontmatter was round-tripped through a lossy parse→serialize path that flattens object-list items to scalar strings. The write now preserves the original raw text for any structurally-unchanged top-level key and regenerates only the field that actually changed, so unrelated `must_haves` blocks survive verbatim. (#1572) diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 388d7ec4b..590ed5414 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -199,20 +199,61 @@ function reconstructFrontmatter(obj: Frontmatter): string { return lines.join('\n'); } +/** + * 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 + * end), capturing all nested indented content. Used by `spliceFrontmatter` for per-key + * identity preservation (#1572): a structurally-unchanged key keeps its original raw + * text, so the lossy `reconstructFrontmatter` never touches object-lists the caller did + * not modify (e.g. must_haves.artifacts / .prohibitions). + */ +function sliceTopLevelFrontmatterSegments(yaml: string): Array<{ key: string; raw: string }> { + const lines = yaml.split(/\r?\n/); + const segments: Array<{ key: string; raw: string }> = []; + let current: { key: string; raw: string[] } | null = null; + for (const line of lines) { + // A column-0 `key:` (no leading whitespace) starts a new top-level segment. + if (/^[A-Za-z0-9_-]+:/.test(line)) { + if (current) segments.push({ key: current.key, raw: current.raw.join('\n') }); + const keyName = (line.match(/^([A-Za-z0-9_-]+):/) as RegExpMatchArray)[1]; + current = { key: keyName, raw: [line] }; + } else if (current) { + current.raw.push(line); + } + // Stray lines before the first top-level key (rare in frontmatter) are dropped. + } + if (current) segments.push({ key: current.key, raw: current.raw.join('\n') }); + return segments; +} + +/** + * Regenerate one frontmatter key's serialization, fail-closed if the lossy + * `reconstructFrontmatter` cannot represent the value (#1572 codex review). Object-list + * items (e.g. must_haves.artifacts `{path, provides}` maps) serialize as the literal + * string "[object Object]"; rather than silently emit that and destroy the data, refuse + * so the caller (cmdFrontmatterSet/Merge) errors out WITHOUT writing — directing the + * user to edit the file directly. The reported #1572 case (mutating an UNRELATED field) + * is unaffected: unchanged keys preserve their original raw text and never reach here. + */ +function regenerateFrontmatterKey(key: string, value: FrontmatterValue): string { + const rendered = reconstructFrontmatter({ [key]: value }); + if (/\[object Object\]/.test(rendered)) { + throw new Error( + `frontmatter: cannot faithfully serialize key "${key}" — it contains a nested object-list ` + + `(e.g. must_haves.artifacts) the frontmatter writer cannot represent, and serializing it would ` + + `emit "[object Object]". Edit the file directly instead of using frontmatter set/merge.`, + ); + } + return rendered; +} + function spliceFrontmatter(content: string, newObj: Frontmatter): string { const match = content.match(/^---\r?\n[\s\S]+?\r?\n---/); if (match) { - // Identity-preservation (additive, lossless round-trip): `reconstructFrontmatter` is a - // deliberately lossy serializer — it cannot faithfully re-emit nested object-list items - // (e.g. must_haves.artifacts / must_haves.prohibitions, whose items are `{ path, provides }` - // / `{ statement, status, … }` maps). When the caller is writing back a value that is - // STRUCTURALLY UNCHANGED from the original parse (the canonical CRUD round-trip and the - // #644 prohibition schema round-trip both do this), regenerating from the lossy object would - // silently mangle those blocks. Detect that case by deep-equality against a re-parse of the - // original frontmatter and preserve the ORIGINAL raw text verbatim — a true no-op splice. - // This touches neither the parser (`extractFrontmatter`) nor `parseMustHavesBlock`; it only - // makes the existing splice faithful when nothing changed. A genuine mutation (different - // object) still flows through `reconstructFrontmatter` exactly as before. + const fmBlock = match[0]; + + // Whole-document no-op guard: a true no-op returns content verbatim (byte-exact, + // including any formatting the lossy serializer would normalize). try { if (frontmatterDeepEqual(extractFrontmatter(content), newObj)) { return content; @@ -220,10 +261,63 @@ function spliceFrontmatter(content: string, newObj: Frontmatter): string { } catch { /* fall through to regeneration on any comparison hiccup */ } - const yamlStr = reconstructFrontmatter(newObj); - return `---\n${yamlStr}\n---` + content.slice(match[0].length); + + // Per-key identity preservation (#1572). `reconstructFrontmatter` is a deliberately + // lossy serializer — it cannot faithfully re-emit nested object-list items (e.g. + // must_haves.artifacts / .prohibitions, whose items are `{ path, provides }` / + // `{ statement, status }` maps; `extractFrontmatter` flattens those to scalar + // strings, so a round-trip drops `provides:` and collapses the list to a malformed + // inline array). For any top-level key whose value is STRUCTURALLY UNCHANGED between + // the original parse and `newObj`, preserve that key's ORIGINAL raw text verbatim; + // regenerate only keys that actually changed. This generalizes the whole-document + // no-op guard above to per-key fidelity, so mutating `wave` no longer destroys an + // unrelated `must_haves` block. Keys absent from the original (genuinely new) are + // regenerated and appended; keys absent from `newObj` are preserved (never silently + // deleted by a set/merge). + const fmLines = fmBlock.split(/\r?\n/); + const inner = fmLines.slice(1, -1).join('\n'); // drop the opening `---` and closing `---` + let originalParsed: Frontmatter; + try { originalParsed = extractFrontmatter(fmBlock); } catch { originalParsed = {}; } + + const segments = sliceTopLevelFrontmatterSegments(inner); + const emitted: string[] = []; + const seen: Set = new Set(); + + for (const seg of segments) { + seen.add(seg.key); + if (Object.prototype.hasOwnProperty.call(newObj, seg.key)) { + // Key is in newObj: preserve original raw text if structurally unchanged, + // otherwise regenerate. The key SET is defined by newObj — keys that were in + // the original but are absent from newObj are intentionally dropped (the real + // cmdSet/cmdMerge flow always passes the full merged object, so this only + // matters for direct unit callers and matches spliceFrontmatter's contract: + // the result frontmatter IS newObj). + if (frontmatterDeepEqual(newObj[seg.key], originalParsed[seg.key])) { + emitted.push(seg.raw); // unchanged → preserve original raw text verbatim + } else { + emitted.push(regenerateFrontmatterKey(seg.key, newObj[seg.key])); // changed → regenerate (fail-closed on object-lists) + } + } + // else: key absent from newObj → drop (not emitted). + } + // Append genuinely-new keys not present in the original frontmatter. + for (const k of Object.keys(newObj)) { + if (!seen.has(k)) { + emitted.push(regenerateFrontmatterKey(k, newObj[k])); + } + } + + const yamlStr = emitted.join('\n'); + return `---\n${yamlStr}\n---` + content.slice(fmBlock.length); } + // No existing frontmatter — generate from scratch, fail-closed on unrepresentable values. const yamlStr = reconstructFrontmatter(newObj); + if (/\[object Object\]/.test(yamlStr)) { + throw new Error( + 'frontmatter: cannot faithfully serialize the requested frontmatter — it contains a nested ' + + 'object-list (e.g. must_haves.artifacts) the writer cannot represent. Edit the file directly.', + ); + } return `---\n${yamlStr}\n---\n\n` + content; } diff --git a/tests/frontmatter-cli.test.cjs b/tests/frontmatter-cli.test.cjs index 11d1b3e57..a28dea3c5 100644 --- a/tests/frontmatter-cli.test.cjs +++ b/tests/frontmatter-cli.test.cjs @@ -274,3 +274,131 @@ body`; assert.ok(parsed.error, 'Should have error field'); }); }); + +// ─── frontmatter set/merge: must_haves object-list preservation (#1572) ────── +// `frontmatter set`/`merge` round-tripped the WHOLE frontmatter through the lossy +// extractFrontmatter → reconstructFrontmatter pair, which flattens must_haves +// object-list items ({path, provides} maps) to scalar strings and re-emits them as a +// malformed inline array — destroying `provides:` whenever an UNRELATED field changed. +// The fix preserves the original raw text for any structurally-unchanged top-level key. +const { parseMustHavesBlock } = require('../gsd-core/bin/lib/frontmatter.cjs'); + +describe('frontmatter set/merge preserves must_haves object-lists (#1572)', () => { + const ARTIFACTS_PLAN = [ + '---', + 'phase: 1', + 'wave: 1', + 'plan: 01-01', + 'type: implementation', + 'depends_on: []', + 'files_modified: []', + 'autonomous: true', + 'must_haves:', + ' artifacts:', + ' - path: src/foo.ts', + ' provides: the foo', + ' - path: src/bar.ts', + ' provides: the bar', + '---', + '# body', + '', + ].join('\n'); + + const PROHIBITIONS_PLAN = [ + '---', + 'phase: 1', + 'wave: 1', + 'must_haves:', + ' prohibitions:', + ' - statement: no direct DB calls', + ' status: enforced', + ' - statement: no print statements', + ' status: pending', + '---', + '# body', + '', + ].join('\n'); + + function runAndParse(plan, cmdArgsForFile) { + const file = writeTempFile(plan); + runGsdTools(cmdArgsForFile(file)); + const after = fs.readFileSync(file, 'utf-8'); + return after; + } + + test('set on an unrelated scalar preserves every must_haves.artifacts entry (path + provides)', () => { + const after = runAndParse(ARTIFACTS_PLAN, f => ['frontmatter', 'set', f, '--field', 'wave', '--value', '2']); + assert.deepEqual( + parseMustHavesBlock(after, 'artifacts'), + [ + { path: 'src/foo.ts', provides: 'the foo' }, + { path: 'src/bar.ts', provides: 'the bar' }, + ], + 'must_haves.artifacts object-list must survive a set on an unrelated field (#1572)', + ); + }); + + test('merge of an unrelated field preserves every must_haves.artifacts entry', () => { + const after = runAndParse(ARTIFACTS_PLAN, f => ['frontmatter', 'merge', f, '--data', JSON.stringify({ wave: 2 })]); + assert.deepEqual( + parseMustHavesBlock(after, 'artifacts'), + [ + { path: 'src/foo.ts', provides: 'the foo' }, + { path: 'src/bar.ts', provides: 'the bar' }, + ], + 'must_haves.artifacts object-list must survive a merge of an unrelated field (#1572)', + ); + }); + + test('must_haves.prohibitions object-list is preserved on an unrelated set (same code path)', () => { + const after = runAndParse(PROHIBITIONS_PLAN, f => ['frontmatter', 'set', f, '--field', 'wave', '--value', '2']); + assert.deepEqual( + parseMustHavesBlock(after, 'prohibitions'), + [ + { statement: 'no direct DB calls', status: 'enforced' }, + { statement: 'no print statements', status: 'pending' }, + ], + 'must_haves.prohibitions object-list must survive a set on an unrelated field (#1572)', + ); + }); + + test('round-trip is stable: setting wave twice still preserves artifacts (per-key preservation is idempotent)', () => { + const file = writeTempFile(ARTIFACTS_PLAN); + runGsdTools(['frontmatter', 'set', file, '--field', 'wave', '--value', '2']); + runGsdTools(['frontmatter', 'set', file, '--field', 'wave', '--value', '3']); + const after = fs.readFileSync(file, 'utf-8'); + assert.deepEqual( + parseMustHavesBlock(after, 'artifacts'), + [ + { path: 'src/foo.ts', provides: 'the foo' }, + { path: 'src/bar.ts', provides: 'the bar' }, + ], + 'must_haves.artifacts must survive repeated sets on an unrelated field', + ); + }); + + test('directly setting must_haves to a new object-list fails closed instead of emitting [object Object] (#1572 codex review)', () => { + // A CHANGED key whose value is an object-list cannot be faithfully serialized by the + // lossy writer (it would emit "[object Object]"). Rather than silently destroy the + // data, spliceFrontmatter throws — the command fails and the file is left unchanged. + const file = writeTempFile(ARTIFACTS_PLAN); + const result = runGsdTools([ + 'frontmatter', 'set', file, '--field', 'must_haves', + '--value', JSON.stringify({ artifacts: [{ path: 'src/new.ts', provides: 'new thing' }] }), + ]); + assert.ok( + !result.success, + 'frontmatter set of a must_haves object-list must fail closed (refuse to emit "[object Object]")', + ); + const after = fs.readFileSync(file, 'utf-8'); + assert.ok(!/\[object Object\]/.test(after), 'the file must not contain "[object Object]" after a refused set'); + assert.deepEqual( + parseMustHavesBlock(after, 'artifacts'), + [ + { path: 'src/foo.ts', provides: 'the foo' }, + { path: 'src/bar.ts', provides: 'the bar' }, + ], + 'the original must_haves.artifacts must be intact after the refused set', + ); + }); +}); diff --git a/tests/frontmatter.unit.test.cjs b/tests/frontmatter.unit.test.cjs index 967ea07fd..a3f9cff37 100644 --- a/tests/frontmatter.unit.test.cjs +++ b/tests/frontmatter.unit.test.cjs @@ -943,6 +943,85 @@ describe('spliceFrontmatter: exact delimiter handling', () => { }); }); +// spliceFrontmatter per-key identity preservation + fail-closed (#1572). These exercise +// sliceTopLevelFrontmatterSegments, the per-key deepEqual/preserve/regenerate/drop/append +// loop, and regenerateFrontmatterKey's "[object Object]" fail-closed directly. +describe('spliceFrontmatter: per-key preservation + fail-closed (#1572)', () => { + const PLAN = [ + '---', 'phase: 1', 'wave: 1', + 'must_haves:', ' artifacts:', ' - path: src/foo.ts', ' provides: the foo', + '---', '# body', '', + ].join('\n'); + + test('unchanged object-list key keeps its original raw text (provides survives) when a scalar sibling changes', () => { + const parsed = extractFrontmatter(PLAN); + parsed.wave = '2'; // mutate one scalar; must_haves flattened-projection unchanged + const out = spliceFrontmatter(PLAN, parsed); + // must_haves.artifacts raw preserved verbatim (provides intact) — NOT regenerated. + assert.deepEqual(parseMustHavesBlock(out, 'artifacts'), [{ path: 'src/foo.ts', provides: 'the foo' }]); + // the changed scalar WAS regenerated. + assert.ok(/^wave: 2$/m.test(out), 'changed scalar wave must be regenerated to 2'); + // original ordering preserved (phase before wave before must_haves). + const phaseIdx = out.indexOf('phase:'); + const waveIdx = out.indexOf('wave:'); + const mhIdx = out.indexOf('must_haves:'); + assert.ok(phaseIdx < waveIdx && waveIdx < mhIdx, 'top-level key order preserved'); + }); + + test('a changed scalar regenerates only that key (no other key touched)', () => { + const out = spliceFrontmatter(PLAN, { ...extractFrontmatter(PLAN), phase: '9' }); + assert.ok(/^phase: 9$/m.test(out)); + // wave unchanged → still 1 + assert.ok(/^wave: 1$/m.test(out)); + }); + + test('keys absent from newObj are dropped (key set is defined by newObj)', () => { + const out = spliceFrontmatter(PLAN, { phase: '1' }); + assert.ok(/^phase: 1$/m.test(out)); + assert.ok(!/wave:/.test(out), 'wave (absent from newObj) must be dropped'); + assert.ok(!/must_haves:/.test(out), 'must_haves (absent from newObj) must be dropped'); + }); + + test('genuinely-new keys (not in original) are appended', () => { + const out = spliceFrontmatter(PLAN, { ...extractFrontmatter(PLAN), brand_new: 'x' }); + assert.ok(/^brand_new: x$/m.test(out), 'new key appended'); + // existing keys still present + assert.ok(/^phase: 1$/m.test(out)); + }); + + test('a nested indented block stays attached to its parent key (segment slicer respects indentation)', () => { + const multi = '---\na: 1\nmust_haves:\n artifacts:\n - path: x\n provides: y\nb: 2\n---\n'; + const out = spliceFrontmatter(multi, { ...extractFrontmatter(multi), b: '3' }); + // The indented artifacts block must be preserved as part of must_haves (not split off), + // and b regenerated. proves the slicer grouped the nested lines under must_haves. + assert.deepEqual(parseMustHavesBlock(out, 'artifacts'), [{ path: 'x', provides: 'y' }]); + assert.ok(/^b: 3$/m.test(out)); + assert.ok(/^a: 1$/m.test(out)); + }); + + test('whole-document no-op returns the input verbatim', () => { + const out = spliceFrontmatter(PLAN, extractFrontmatter(PLAN)); + assert.equal(out, PLAN); + }); + + test('changing must_haves to an unrepresentable object-list fails closed (throws, no [object Object])', () => { + const newObj = { ...extractFrontmatter(PLAN), must_haves: { artifacts: [{ path: 'p', provides: 'q' }] } }; + assert.throws( + () => spliceFrontmatter(PLAN, newObj), + /cannot faithfully serialize key "must_haves"/, + 'a changed object-list key must fail closed rather than emit [object Object]', + ); + }); + + test('no-frontmatter path also fails closed for an unrepresentable object-list value', () => { + assert.throws( + () => spliceFrontmatter('body only', { must_haves: { artifacts: [{ path: 'p' }] } }), + /cannot faithfully serialize the requested frontmatter/, + 'generating fresh frontmatter with an object-list must fail closed', + ); + }); +}); + describe('extractFrontmatter: complex real-world documents', () => { test('plan document', () => { const doc = [