diff --git a/.changeset/plucky-wolves-run.md b/.changeset/plucky-wolves-run.md new file mode 100644 index 000000000..ab7ba11f2 --- /dev/null +++ b/.changeset/plucky-wolves-run.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1664 +--- +**`frontmatter set` on an object-list field now fails closed instead of silently doing nothing** — setting `must_haves` (or another object-list field) to a value whose lossy parse projection matched the original's was a silent no-op: the command reported `{updated:true}` but the change never applied (the writer's scalar-only parser had flattened both to the same shape). `frontmatter set` now detects a no-op write for dict-valued fields and surfaces a clear error directing the user to edit the file directly. Scalars and scalar arrays round-trip faithfully, so idempotent sets of those still report `{updated:true}` (no false positive). diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 590ed5414..5d4028736 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -503,10 +503,35 @@ function cmdFrontmatterSet(cwd: string, filePath: string, field: string | undefi try { parsedValue = JSON.parse(value as string); } catch { parsedValue = value; } fm[field as string] = parsedValue as FrontmatterValue; const newContent = spliceFrontmatter(content, fm); + // #1660: a no-op set (newContent unchanged) with a dict-valued field means the lossy + // frontmatter parser made the new value's projection equal the original's — the change + // did not apply (bites object-list fields like must_haves). Detection lives in the pure + // exported helper noOpObjectListSetError so the mutation gate (property/unit set) covers + // it — the cmd path itself is not in that set. + const noOpErr = noOpObjectListSetError(content, newContent, parsedValue); + if (noOpErr) { + output({ error: noOpErr, field }, raw, undefined); + return; + } platformWriteSync(fullPath, newContent); output({ updated: true, field, value: parsedValue }, raw, 'true'); } +/** + * #1660: detect a frontmatter `set` that would be a silent no-op on a dict-valued field. + * Returns an error message when the splice produced no content change but the new value + * is a dict (object-list fields like must_haves, whose `{path, provides}` items flatten to + * scalar strings under extractFrontmatter so a replacement can deep-equal the original's + * projection), else null. Scalars and scalar arrays round-trip faithfully, so idempotent + * sets of those are intentionally NOT flagged. Pure and unit-tested directly (the cmd path + * is not in Stryker's property/unit set, so the detection must be testable in isolation). + */ +function noOpObjectListSetError(originalContent: string, newContent: string, parsedValue: unknown): string | null { + if (newContent !== originalContent) return null; + if (parsedValue === null || typeof parsedValue !== 'object' || Array.isArray(parsedValue)) return null; + return 'frontmatter set had no effect — the supplied value is equivalent to the existing field under the frontmatter parser, which cannot faithfully round-trip object-list fields like must_haves. Edit the file directly.'; +} + function cmdFrontmatterMerge(cwd: string, filePath: string, data: string | undefined, raw: boolean): void { if (!filePath || !data) { error('file and data required'); } const fullPath = path.isAbsolute(filePath) ? filePath : path.join(cwd, filePath); @@ -543,6 +568,7 @@ export = { parseFrontmatter: extractFrontmatter, reconstructFrontmatter, spliceFrontmatter, + noOpObjectListSetError, parseMustHavesBlock, FRONTMATTER_SCHEMAS, cmdFrontmatterGet, diff --git a/tests/frontmatter-cli.test.cjs b/tests/frontmatter-cli.test.cjs index a28dea3c5..aed6987ab 100644 --- a/tests/frontmatter-cli.test.cjs +++ b/tests/frontmatter-cli.test.cjs @@ -402,3 +402,42 @@ describe('frontmatter set/merge preserves must_haves object-lists (#1572)', () = ); }); }); + +// Bug #1660 — frontmatter set of an object-list field (e.g. must_haves) is a silent no-op +// when the new value's lossy parse projection equals the original's. Folded into the owning +// frontmatter-cli test (no new top-level bug-NNNN file). +describe('Bug #1660: frontmatter set of an object-list field fails closed instead of a silent no-op', () => { + const PLAN_WITH_MUST_HAVES = [ + '---', 'phase: 1', 'wave: 1', + 'must_haves:', ' artifacts:', ' - path: src/foo.ts', ' provides: the foo', + '---', '# body', '', + ].join('\n'); + + test('setting must_haves to a value that flattens to the original projection fails closed (no silent no-op)', () => { + const file = writeTempFile(PLAN_WITH_MUST_HAVES); + const before = fs.readFileSync(file, 'utf-8'); + // New value {artifacts:["path: src/foo.ts"]} — its extractFrontmatter projection equals + // the original's flattened projection, so the set would otherwise be a silent no-op. + const result = runGsdTools(['frontmatter', 'set', file, '--field', 'must_haves', '--value', JSON.stringify({ artifacts: ['path: src/foo.ts'] })]); + const parsed = JSON.parse(result.output); + assert.ok(parsed.error, 'a no-op set of an object-list field must surface an error, not silent {updated:true}'); + const after = fs.readFileSync(file, 'utf-8'); + assert.equal(after, before, 'the file must be unchanged when the set is refused (no silent partial write)'); + }); + + test('an idempotent set of a scalar (wave, same value) still reports updated (no false positive)', () => { + const file = writeTempFile('---\nphase: 1\nwave: 1\n---\n# body\n'); + const result = runGsdTools(['frontmatter', 'set', file, '--field', 'wave', '--value', '1']); + const parsed = JSON.parse(result.output); + assert.equal(parsed.updated, true, 'an idempotent SCALAR set must still report {updated:true} (not fail-closed)'); + assert.ok(!parsed.error, 'an idempotent scalar set must not produce an error'); + }); + + test('an idempotent set of a scalar array (tags, same value) still reports updated (no false positive)', () => { + const file = writeTempFile('---\nphase: 1\ntags: ["a","b"]\n---\n# body\n'); + const result = runGsdTools(['frontmatter', 'set', file, '--field', 'tags', '--value', '["a","b"]']); + const parsed = JSON.parse(result.output); + assert.equal(parsed.updated, true, 'an idempotent scalar-ARRAY set must still report {updated:true} (arrays round-trip; not fail-closed)'); + assert.ok(!parsed.error, 'an idempotent scalar-array set must not produce an error'); + }); +}); diff --git a/tests/frontmatter.unit.test.cjs b/tests/frontmatter.unit.test.cjs index a3f9cff37..14eb0505f 100644 --- a/tests/frontmatter.unit.test.cjs +++ b/tests/frontmatter.unit.test.cjs @@ -20,6 +20,7 @@ const { extractFrontmatter, reconstructFrontmatter, spliceFrontmatter, + noOpObjectListSetError, parseMustHavesBlock, FRONTMATTER_SCHEMAS, } = require('../gsd-core/bin/lib/frontmatter.cjs'); @@ -1202,3 +1203,29 @@ describe('reconstructFrontmatter: nested subval plain string', () => { assert.equal(result, 'meta:\n tag: "issue#42"'); }); }); + +// noOpObjectListSetError (#1660) — pure detection helper, unit-tested directly because the +// cmdFrontmatterSet path is not in Stryker's property/unit set. +describe('noOpObjectListSetError (#1660)', () => { + const ORIG = '---\nphase: 1\n---\n'; + test('changed content (real update) → null', () => { + assert.equal(noOpObjectListSetError(ORIG, ORIG + 'x', { must_haves: 1 }), null); + }); + test('scalar value no-op → null (idempotent scalar sets are fine)', () => { + for (const v of [1, 'str', true, 0, '']) assert.equal(noOpObjectListSetError(ORIG, ORIG, v), null, `scalar ${JSON.stringify(v)}`); + }); + test('scalar-array value no-op → null (scalar arrays round-trip faithfully)', () => { + assert.equal(noOpObjectListSetError(ORIG, ORIG, ['a', 'b']), null); + assert.equal(noOpObjectListSetError(ORIG, ORIG, []), null); + }); + test('null value no-op → null', () => { + assert.equal(noOpObjectListSetError(ORIG, ORIG, null), null); + }); + test('dict value no-op → error message naming the object-list round-trip limit', () => { + const msg = noOpObjectListSetError(ORIG, ORIG, { artifacts: [{ path: 'p' }] }); + assert.equal(typeof msg, 'string'); + assert.ok(msg.includes('had no effect'), msg); + assert.ok(msg.includes('object-list'), msg); + assert.ok(msg.includes('Edit the file directly'), msg); + }); +});