* 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.
This commit is contained in:
Tom Boucher
@@ -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',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 = [
|
||||
|
||||
Reference in New Issue
Block a user