From 1b3606c88af43712a2fedd5f04c8d4c9b3f42b1d Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 11:45:00 -0500 Subject: [PATCH 1/5] fix(specs): keep content absorbed into a removed requirement A requirement block's `raw` runs to the next header the parser RECOGNISES, so a heading it does not - one indented by the 0-3 spaces CommonMark allows, or a plain `### Notes` - is absorbed into the requirement above it. Removing that requirement deleted the absorbed content with it. Silently: nothing counted it, so nothing warned, and the spec left behind still validated. Reproducible on main with no marker and no capability retirement involved. Anything from the first `#`/`##`/`###` heading after a removed block's own header is now kept in place. `####` is excluded deliberately - a requirement's `#### Scenario:` headings are its own and go with it. This replaces an earlier attempt on this branch that widened every heading pattern in both parsers to accept indentation. That was wrong twice over. It reclassified content, so a spec that was valid became invalid - commented-out and indented examples started parsing as real requirements, taking `list` from 1 requirement to 3. And it did not even fix the bug: moving the line out of the block only meant the reconstruction dropped it at a different step, since `rebuilt` is assembled from `before + header + kept blocks + after` and anything skipped is simply gone. So nothing is reclassified now. An indented heading is still not a requirement, exactly as before; it just survives its neighbour's removal, which is all this ever needed to do. The repo's own corpus produces byte-identical `list`, `validate --specs --strict` and `validate --changes --strict` output. Four regressions, each mutation-verified: removing the salvage fails the three absorbed-content cases, and counting `####` as a boundary fails the scenario case. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/indented-atx-headings.md | 5 ++ src/core/specs-apply.ts | 35 ++++++++++ test/core/specs-apply.salvage.test.ts | 94 +++++++++++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 .changeset/indented-atx-headings.md create mode 100644 test/core/specs-apply.salvage.test.ts diff --git a/.changeset/indented-atx-headings.md b/.changeset/indented-atx-headings.md new file mode 100644 index 0000000000..62b38bb63d --- /dev/null +++ b/.changeset/indented-atx-headings.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Stop deleting content that sits next to a removed requirement. A requirement block runs until the next heading OpenSpec recognises, so a heading it doesn't — one indented by the one-to-three spaces Markdown allows, or a plain `### Notes` — was absorbed into the requirement above it and deleted along with it when a change removed that requirement. Silently: nothing counted the content, so nothing warned, and the spec left behind still validated. That content is now kept in place. Nothing is reclassified — an indented heading still isn't a requirement, exactly as before — and a requirement's own `#### Scenario:` blocks still travel with it. diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 2f9c1a5e3d..5fbe341138 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -404,12 +404,18 @@ export async function buildUpdatedSpec( // Recompose requirements section preserving original ordering where possible const keptOrder: RequirementBlock[] = []; const seen = new Set(); + // Content that was never part of a requirement but sat inside its block, kept + // in place when that requirement goes. See `salvageForeignTail`. + const salvaged: string[] = []; for (const block of parts.bodyBlocks) { const key = normalizeRequirementName(block.name); const replacement = nameToBlock.get(key); if (replacement) { keptOrder.push(replacement); seen.add(key); + } else { + const tail = salvageForeignTail(block.raw); + if (tail) salvaged.push(tail); } } // Append any newly added that were not in original order @@ -422,6 +428,7 @@ export async function buildUpdatedSpec( const reqBody = [parts.preamble && parts.preamble.trim() ? parts.preamble.trimEnd() : ''] .filter(Boolean) .concat(keptOrder.map((b) => b.raw)) + .concat(salvaged) .join('\n\n') .trimEnd(); @@ -442,6 +449,34 @@ export async function buildUpdatedSpec( }; } +/** + * The part of a requirement block's `raw` that was never the requirement's own. + * + * A block runs to the next header the parser RECOGNISES, so a heading it does + * not - one indented by the 0-3 spaces CommonMark allows, or a plain + * `### Notes` - is absorbed into the requirement above it. Removing that + * requirement then deleted the absorbed content too, silently, because nothing + * counted it and nothing reported it. + * + * Anything from the first `#`/`##`/`###` heading after the block's own header is + * returned so the caller can keep it. `####` is excluded on purpose: a + * requirement's `#### Scenario:` headings are its own and go with it. + * + * Nothing is reclassified. An indented heading is still not a requirement - it + * simply survives its neighbour's removal, which is all this ever needed to do. + */ +function salvageForeignTail(raw: string): string { + const lines = raw.replace(/\r\n?/g, '\n').split('\n'); + const fenceMask = buildCodeFenceMask(lines); + for (let index = 1; index < lines.length; index++) { + if (fenceMask[index]) continue; + if (/^ {0,3}#{1,3}\s/.test(lines[index])) { + return lines.slice(index).join('\n').trimEnd(); + } + } + return ''; +} + function normalizeBlockRaw(raw: string): string { return raw.replace(/\r\n?/g, '\n').trim(); } diff --git a/test/core/specs-apply.salvage.test.ts b/test/core/specs-apply.salvage.test.ts new file mode 100644 index 0000000000..faa27fa6be --- /dev/null +++ b/test/core/specs-apply.salvage.test.ts @@ -0,0 +1,94 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; +import { buildUpdatedSpec, findSpecUpdates } from '../../src/core/specs-apply.js'; + +// A requirement block runs to the next header the parser RECOGNISES, so a +// heading it does not - one indented by the 0-3 spaces CommonMark allows, or a +// plain `### Notes` - is absorbed into the requirement above it. Removing that +// requirement deleted the absorbed content too. Silently: nothing counted it, so +// nothing warned, and the spec that remained still validated. +describe('buildUpdatedSpec (content absorbed into a removed requirement)', () => { + let tempDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-salvage-')); + }); + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + async function rebuild(foreign: string[]): Promise { + const specsDir = path.join(tempDir, 'openspec', 'specs', 'demo'); + const changeDir = path.join(tempDir, 'openspec', 'changes', 'drop'); + await fs.mkdir(specsDir, { recursive: true }); + await fs.mkdir(path.join(changeDir, 'specs', 'demo'), { recursive: true }); + await fs.writeFile( + path.join(specsDir, 'spec.md'), + [ + '# demo Specification', + '', + '## Purpose', + 'Why this exists.', + '', + '## Requirements', + '', + '### Requirement: Doomed', + 'The system SHALL do the doomed thing.', + '', + '#### Scenario: One', + '- **WHEN** a', + '- **THEN** b', + '', + ...foreign, + '', + '### Requirement: Survivor', + 'The system SHALL survive.', + '', + '#### Scenario: Two', + '- **WHEN** c', + '- **THEN** d', + '', + ].join('\n') + ); + await fs.writeFile( + path.join(changeDir, 'specs', 'demo', 'spec.md'), + [ + '# demo - Changes', + '', + '## REMOVED Requirements', + '', + '### Requirement: Doomed', + '**Reason**: Superseded.', + '**Migration**: None.', + '', + ].join('\n') + ); + const [update] = await findSpecUpdates(changeDir, path.join(tempDir, 'openspec', 'specs')); + const built = await buildUpdatedSpec(update, 'drop', { silent: true }); + return built.rebuilt; + } + + it.each([ + { what: 'an indented requirement header', foreign: [' ### Requirement: Audit trail', ' The system SHALL retain it.'] }, + { what: 'a heading that is not a requirement', foreign: ['### Notes', 'Kept by hand, never delete.'] }, + { what: 'an indented non-requirement heading', foreign: [' ### Notes', 'Indented, kept by hand.'] }, + ])('keeps $what when the requirement above it is removed', async ({ foreign }) => { + const rebuilt = await rebuild(foreign); + for (const line of foreign) { + expect(rebuilt).toContain(line.trim()); + } + // The removal itself still happened, and the neighbour is untouched. + expect(rebuilt).not.toContain('The system SHALL do the doomed thing.'); + expect(rebuilt).toContain('### Requirement: Survivor'); + }); + + it("keeps a requirement's own scenarios with it when it is removed", async () => { + // `####` must NOT count as a boundary, or every requirement would be severed + // from its scenarios and they would survive as orphans. + const rebuilt = await rebuild([]); + expect(rebuilt).not.toContain('#### Scenario: One'); + expect(rebuilt).toContain('#### Scenario: Two'); + }); +}); From c25bc799bd1ebb09522085cacd3bf219fcf0bb19 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 12:04:54 -0500 Subject: [PATCH 2/5] fix(specs): keep notes absorbed into a modified or removed requirement A slow audit of the previous commit found the fix covered one of three paths. A requirement block absorbs anything below it that the parser does not read as a new header - a note indented by the 0-3 spaces CommonMark allows, say - so that content rides inside the block. The previous commit salvaged it when the requirement was REMOVED and missed MODIFIED entirely: that path rebuilds the block from the delta, which never carried the note, so it was dropped exactly as before. Verified against the real CLI: main loses it on both paths. RENAMED was the opposite trap. It rewrites the original block's header line in place, so the note is already there - but it also deletes the original key from the block map, which made the requirement look REMOVED to the salvage and produced a duplicate. Tracking which operation applied is therefore not reliable at this point in the merge, so the salvage now asks the assembled result instead: re-insert a note only when nothing else in the rebuilt section already carries it. That is correct for all three paths by construction. Salvaged content also keeps its position now, next to the requirement it was written beside, rather than being appended at the end of the section. Six regressions, three of them mutation-verified against this logic: never re-inserting fails four, always re-inserting duplicates on rename, and appending at the end loses the position. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/indented-atx-headings.md | 2 +- src/core/specs-apply.ts | 34 ++++++++++--- test/core/specs-apply.salvage.test.ts | 71 ++++++++++++++++++++++----- 3 files changed, 87 insertions(+), 20 deletions(-) diff --git a/.changeset/indented-atx-headings.md b/.changeset/indented-atx-headings.md index 62b38bb63d..c16b35c1a2 100644 --- a/.changeset/indented-atx-headings.md +++ b/.changeset/indented-atx-headings.md @@ -2,4 +2,4 @@ "@fission-ai/openspec": patch --- -Stop deleting content that sits next to a removed requirement. A requirement block runs until the next heading OpenSpec recognises, so a heading it doesn't — one indented by the one-to-three spaces Markdown allows, or a plain `### Notes` — was absorbed into the requirement above it and deleted along with it when a change removed that requirement. Silently: nothing counted the content, so nothing warned, and the spec left behind still validated. That content is now kept in place. Nothing is reclassified — an indented heading still isn't a requirement, exactly as before — and a requirement's own `#### Scenario:` blocks still travel with it. +Stop deleting notes written next to a requirement. A requirement absorbs anything below it that OpenSpec doesn't recognise as a new heading — a note indented by the one to three spaces Markdown allows, for example — so removing or modifying that requirement silently deleted the note too. Nothing counted it, so nothing warned, and the spec left behind still validated. Such content is now kept, in place. Nothing is reclassified: an indented heading still isn't a requirement, and a requirement's own `#### Scenario:` blocks still travel with it. diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 5fbe341138..930626cde8 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -404,31 +404,49 @@ export async function buildUpdatedSpec( // Recompose requirements section preserving original ordering where possible const keptOrder: RequirementBlock[] = []; const seen = new Set(); - // Content that was never part of a requirement but sat inside its block, kept - // in place when that requirement goes. See `salvageForeignTail`. - const salvaged: string[] = []; + // What the section will actually contain, in order. Kept apart from + // `keptOrder` because that one answers "are there any requirements left", + // which salvaged content must not influence. + const orderedBody: string[] = []; + // Content that was never a requirement's own but sat inside its block, paired + // with the position it should keep. See `salvageForeignTail`. + const pendingTails: Array<{ at: number; tail: string }> = []; for (const block of parts.bodyBlocks) { const key = normalizeRequirementName(block.name); const replacement = nameToBlock.get(key); + // Read off the ORIGINAL block - the only copy that still has it. + const foreignTail = salvageForeignTail(block.raw); if (replacement) { keptOrder.push(replacement); + orderedBody.push(replacement.raw); seen.add(key); - } else { - const tail = salvageForeignTail(block.raw); - if (tail) salvaged.push(tail); } + if (foreignTail) pendingTails.push({ at: orderedBody.length, tail: foreignTail }); } // Append any newly added that were not in original order for (const [key, block] of nameToBlock.entries()) { if (!seen.has(key)) { keptOrder.push(block); + orderedBody.push(block.raw); + } + } + // Re-insert only the content that did NOT survive on its own. A RENAMED block + // is the original with its header line swapped, so it still carries its tail + // and re-adding it would duplicate the text; a MODIFIED block is rebuilt from + // the delta and does not, and a REMOVED one is gone entirely. Asking the + // assembled result, rather than tracking which operation applied, is what + // makes this correct for all three - the rename bookkeeping deletes the + // original key, so the operation is not reliably knowable here. + for (let index = pendingTails.length - 1; index >= 0; index--) { + const { at, tail } = pendingTails[index]; + if (!orderedBody.some((part) => part.includes(tail))) { + orderedBody.splice(at, 0, tail); } } const reqBody = [parts.preamble && parts.preamble.trim() ? parts.preamble.trimEnd() : ''] .filter(Boolean) - .concat(keptOrder.map((b) => b.raw)) - .concat(salvaged) + .concat(orderedBody) .join('\n\n') .trimEnd(); diff --git a/test/core/specs-apply.salvage.test.ts b/test/core/specs-apply.salvage.test.ts index faa27fa6be..70c600b170 100644 --- a/test/core/specs-apply.salvage.test.ts +++ b/test/core/specs-apply.salvage.test.ts @@ -19,7 +19,7 @@ describe('buildUpdatedSpec (content absorbed into a removed requirement)', () => await fs.rm(tempDir, { recursive: true, force: true }); }); - async function rebuild(foreign: string[]): Promise { + async function rebuild(foreign: string[], delta?: string[]): Promise { const specsDir = path.join(tempDir, 'openspec', 'specs', 'demo'); const changeDir = path.join(tempDir, 'openspec', 'changes', 'drop'); await fs.mkdir(specsDir, { recursive: true }); @@ -54,16 +54,18 @@ describe('buildUpdatedSpec (content absorbed into a removed requirement)', () => ); await fs.writeFile( path.join(changeDir, 'specs', 'demo', 'spec.md'), - [ - '# demo - Changes', - '', - '## REMOVED Requirements', - '', - '### Requirement: Doomed', - '**Reason**: Superseded.', - '**Migration**: None.', - '', - ].join('\n') + ( + delta ?? [ + '# demo - Changes', + '', + '## REMOVED Requirements', + '', + '### Requirement: Doomed', + '**Reason**: Superseded.', + '**Migration**: None.', + '', + ] + ).join('\n') ); const [update] = await findSpecUpdates(changeDir, path.join(tempDir, 'openspec', 'specs')); const built = await buildUpdatedSpec(update, 'drop', { silent: true }); @@ -91,4 +93,51 @@ describe('buildUpdatedSpec (content absorbed into a removed requirement)', () => expect(rebuilt).not.toContain('#### Scenario: One'); expect(rebuilt).toContain('#### Scenario: Two'); }); + + // A RENAMED block is the original with its header swapped, so it still holds + // the absorbed content. A MODIFIED one is rebuilt from the delta and does not + // - dropping it there loses the content exactly as removing the requirement + // would, which the first version of this fix missed. + it('keeps absorbed content when the requirement above it is MODIFIED', async () => { + const rebuilt = await rebuild( + [' ### Notes', ' Kept by hand, never delete.'], + [ + '# demo - Changes', + '', + '## MODIFIED Requirements', + '', + '### Requirement: Doomed', + 'The system SHALL do the doomed thing, now better.', + '', + '#### Scenario: One', + '- **WHEN** a', + '- **THEN** b', + '', + ] + ); + expect(rebuilt).toContain('Kept by hand, never delete.'); + // Exactly once - a rename path that already carries the tail must not + // duplicate it. + expect(rebuilt.match(/Kept by hand/g)).toHaveLength(1); + expect(rebuilt).toContain('now better'); + // And it stays where the author put it, not appended at the end. + expect(rebuilt.indexOf('Kept by hand')).toBeLessThan(rebuilt.indexOf('Requirement: Survivor')); + }); + + it('does not duplicate absorbed content when the requirement is RENAMED', async () => { + const rebuilt = await rebuild( + [' ### Notes', ' Kept by hand, never delete.'], + [ + '# demo - Changes', + '', + '## RENAMED Requirements', + '', + '- FROM: `### Requirement: Doomed`', + '- TO: `### Requirement: Renamed`', + '', + ] + ); + expect(rebuilt.match(/Kept by hand/g)).toHaveLength(1); + expect(rebuilt).toContain('### Requirement: Renamed'); + }); }); From 2e231ec843ba321fbade7006c1a1a09bd69ba180 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 12:16:53 -0500 Subject: [PATCH 3/5] fix(specs): decide salvage by identity, not by matching text Another audit pass, another defect in my own fix. Deciding whether a note survived by searching the rebuilt section for its text is wrong when two requirements carry the same note: the first copy is found, and the second is dropped. Reproduced - two removed requirements each followed by an identical `### Notes`, one note destroyed. Survival is a question about the block, not about text. An untouched block is the same object the parser produced and still carries its note; a replaced one is a different object and does not. The RENAMED path previously blurred that by copying the whole raw, so it now carries only the requirement's own lines and the salvage puts the note back like every other path. With every replacement uniformly lacking the tail, `replacement !== block` decides it exactly, and no text is compared at all. Four properties, each mutation-verified: matching text instead of identity loses the duplicate note, always re-inserting doubles an untouched block's note, letting RENAMED keep the tail doubles it on rename, and counting `####` as a boundary severs a requirement from its scenarios. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/specs-apply.ts | 47 +++++++++----- test/core/specs-apply.salvage.test.ts | 94 +++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 17 deletions(-) diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 930626cde8..4bc0503cf0 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -310,7 +310,11 @@ export async function buildUpdatedSpec( } const block = nameToBlock.get(from)!; const newHeader = `### Requirement: ${to}`; - const rawLines = block.raw.split('\n'); + // Only the requirement's own lines are carried over. Anything absorbed + // below it is put back by the salvage in the recomposition step, which + // keeps every path - renamed, modified, removed - uniform: the replacement + // never holds the tail, so the salvage never has to guess whether it does. + const rawLines = requirementOwnLines(block.raw); rawLines[0] = newHeader; const renamedBlock: RequirementBlock = { headerLine: newHeader, @@ -421,7 +425,12 @@ export async function buildUpdatedSpec( orderedBody.push(replacement.raw); seen.add(key); } - if (foreignTail) pendingTails.push({ at: orderedBody.length, tail: foreignTail }); + // Re-insert the tail unless this block came through untouched, in which + // case it still carries it. Identity, not text: two requirements can carry + // the same note, and a containment check would drop the second copy. + if (foreignTail && replacement !== block) { + pendingTails.push({ at: orderedBody.length, tail: foreignTail }); + } } // Append any newly added that were not in original order for (const [key, block] of nameToBlock.entries()) { @@ -430,18 +439,11 @@ export async function buildUpdatedSpec( orderedBody.push(block.raw); } } - // Re-insert only the content that did NOT survive on its own. A RENAMED block - // is the original with its header line swapped, so it still carries its tail - // and re-adding it would duplicate the text; a MODIFIED block is rebuilt from - // the delta and does not, and a REMOVED one is gone entirely. Asking the - // assembled result, rather than tracking which operation applied, is what - // makes this correct for all three - the rename bookkeeping deletes the - // original key, so the operation is not reliably knowable here. + // Put each salvaged note back where it was written. Walked in reverse so the + // recorded positions are still valid as earlier entries shift. for (let index = pendingTails.length - 1; index >= 0; index--) { const { at, tail } = pendingTails[index]; - if (!orderedBody.some((part) => part.includes(tail))) { - orderedBody.splice(at, 0, tail); - } + orderedBody.splice(at, 0, tail); } const reqBody = [parts.preamble && parts.preamble.trim() ? parts.preamble.trimEnd() : ''] @@ -483,16 +485,27 @@ export async function buildUpdatedSpec( * Nothing is reclassified. An indented heading is still not a requirement - it * simply survives its neighbour's removal, which is all this ever needed to do. */ -function salvageForeignTail(raw: string): string { +/** A block's own lines, up to whatever was absorbed below it. */ +function requirementOwnLines(raw: string): string[] { const lines = raw.replace(/\r\n?/g, '\n').split('\n'); + const boundary = foreignTailIndex(lines); + return boundary === -1 ? lines : lines.slice(0, boundary); +} + +/** Index of the first line that was never the requirement's own, or -1. */ +function foreignTailIndex(lines: string[]): number { const fenceMask = buildCodeFenceMask(lines); for (let index = 1; index < lines.length; index++) { if (fenceMask[index]) continue; - if (/^ {0,3}#{1,3}\s/.test(lines[index])) { - return lines.slice(index).join('\n').trimEnd(); - } + if (/^ {0,3}#{1,3}\s/.test(lines[index])) return index; } - return ''; + return -1; +} + +function salvageForeignTail(raw: string): string { + const lines = raw.replace(/\r\n?/g, '\n').split('\n'); + const boundary = foreignTailIndex(lines); + return boundary === -1 ? '' : lines.slice(boundary).join('\n').trimEnd(); } function normalizeBlockRaw(raw: string): string { diff --git a/test/core/specs-apply.salvage.test.ts b/test/core/specs-apply.salvage.test.ts index 70c600b170..e683841cef 100644 --- a/test/core/specs-apply.salvage.test.ts +++ b/test/core/specs-apply.salvage.test.ts @@ -140,4 +140,98 @@ describe('buildUpdatedSpec (content absorbed into a removed requirement)', () => expect(rebuilt.match(/Kept by hand/g)).toHaveLength(1); expect(rebuilt).toContain('### Requirement: Renamed'); }); + + // Whether a note survived cannot be decided by looking for its text in the + // result: two requirements may carry the same note, and a containment check + // drops the second copy. Survival is decided by whether the block came + // through untouched, which is a question about identity, not text. + it('keeps both copies when two removed requirements carry the same note', async () => { + const specsDir = path.join(tempDir, 'openspec', 'specs', 'demo'); + const changeDir = path.join(tempDir, 'openspec', 'changes', 'drop'); + await fs.mkdir(specsDir, { recursive: true }); + await fs.mkdir(path.join(changeDir, 'specs', 'demo'), { recursive: true }); + const note = [' ### Notes', ' Owned by payments.']; + await fs.writeFile( + path.join(specsDir, 'spec.md'), + [ + '# demo Specification', + '', + '## Purpose', + 'Why this exists.', + '', + '## Requirements', + '', + '### Requirement: Alpha', + 'The system SHALL alpha.', + '', + '#### Scenario: A', + '- **WHEN** a', + '- **THEN** b', + '', + ...note, + '', + '### Requirement: Beta', + 'The system SHALL beta.', + '', + '#### Scenario: B', + '- **WHEN** c', + '- **THEN** d', + '', + ...note, + '', + '### Requirement: Gamma', + 'The system SHALL gamma.', + '', + '#### Scenario: G', + '- **WHEN** e', + '- **THEN** f', + '', + ].join('\n') + ); + await fs.writeFile( + path.join(changeDir, 'specs', 'demo', 'spec.md'), + [ + '# demo - Changes', + '', + '## REMOVED Requirements', + '', + '### Requirement: Alpha', + '**Reason**: x.', + '**Migration**: None.', + '', + '### Requirement: Beta', + '**Reason**: y.', + '**Migration**: None.', + '', + ].join('\n') + ); + const [update] = await findSpecUpdates(changeDir, path.join(tempDir, 'openspec', 'specs')); + const { rebuilt } = await buildUpdatedSpec(update, 'drop', { silent: true }); + + // Two notes were written; two must survive. + expect(rebuilt.match(/### Notes/g)).toHaveLength(2); + expect(rebuilt).toContain('### Requirement: Gamma'); + }); + + it('does not duplicate a note when its requirement is untouched', async () => { + // An untouched block is the original object and still carries its note, so + // re-inserting would double it. + const rebuilt = await rebuild( + [' ### Notes', ' Kept by hand, never delete.'], + [ + '# demo - Changes', + '', + '## ADDED Requirements', + '', + '### Requirement: Fresh', + 'The system SHALL be fresh.', + '', + '#### Scenario: F', + '- **WHEN** a', + '- **THEN** b', + '', + ] + ); + expect(rebuilt.match(/Kept by hand/g)).toHaveLength(1); + }); }); From 459a6fde93b8674ea65f67e71d7248c5ee48aa13 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 12:44:23 -0500 Subject: [PATCH 4/5] fix(specs): warn when a note absorbed into a requirement will be deleted An adversarial review found the previous approach was worse than the bug. Salvaging the "foreign tail" out of a requirement block relied on a positional rule: everything after the first heading-shaped line is not the requirement's. That is not true. A `# comment` inside a scenario bullet, or a markdown example, matches the same shape - and on MODIFIED the old text was then spliced back in after the new, so the spec asserted both. The validator called the result valid, and re-applying the same delta grew the file every time. Reproduced end to end. It also turned a working archive into a hard abort: preserving an unindented `### Notes` made the rebuilt spec fail validation as a scenario-less requirement, so changes that archived cleanly on main stopped archiving, with an error that never mentioned the note. Measured before choosing: 3 of 742 requirement blocks in this repo contain a heading-shaped line, and the repro shows those are false positives. Trading a rare silent deletion for silent corruption on the most common operation is a bad trade. So the merge is left exactly as it was - byte-identical output, verified against main - and the loss is reported instead. That fixes the part of the bug that actually hurt: it was silent. A wrong warning costs a line of output; acting on a wrong answer rewrites the spec. Eight tests. Dropping the warning fails three; ignoring the fence mask fails one - the fence case the previous version left unpinned. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/indented-atx-headings.md | 2 +- src/core/specs-apply.ts | 85 +++----- test/core/specs-apply.salvage.test.ts | 302 +++++++++----------------- 3 files changed, 129 insertions(+), 260 deletions(-) diff --git a/.changeset/indented-atx-headings.md b/.changeset/indented-atx-headings.md index c16b35c1a2..3ee95c6a31 100644 --- a/.changeset/indented-atx-headings.md +++ b/.changeset/indented-atx-headings.md @@ -2,4 +2,4 @@ "@fission-ai/openspec": patch --- -Stop deleting notes written next to a requirement. A requirement absorbs anything below it that OpenSpec doesn't recognise as a new heading — a note indented by the one to three spaces Markdown allows, for example — so removing or modifying that requirement silently deleted the note too. Nothing counted it, so nothing warned, and the spec left behind still validated. Such content is now kept, in place. Nothing is reclassified: an indented heading still isn't a requirement, and a requirement's own `#### Scenario:` blocks still travel with it. +Say when archiving a change will delete a note written next to a requirement. A requirement absorbs anything below it that OpenSpec doesn't recognise as a new heading — a note indented by the one to three spaces Markdown allows, for example — so removing or modifying that requirement took the note with it, silently. `openspec archive` now names the content and where to move it to keep it. The merge itself is unchanged: nothing is relocated, because a `#` line inside a scenario looks identical to a note and moving one of those would rewrite the spec wrongly. diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 4bc0503cf0..8260febee3 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -310,11 +310,7 @@ export async function buildUpdatedSpec( } const block = nameToBlock.get(from)!; const newHeader = `### Requirement: ${to}`; - // Only the requirement's own lines are carried over. Anything absorbed - // below it is put back by the salvage in the recomposition step, which - // keeps every path - renamed, modified, removed - uniform: the replacement - // never holds the tail, so the salvage never has to guess whether it does. - const rawLines = requirementOwnLines(block.raw); + const rawLines = block.raw.split('\n'); rawLines[0] = newHeader; const renamedBlock: RequirementBlock = { headerLine: newHeader, @@ -408,47 +404,41 @@ export async function buildUpdatedSpec( // Recompose requirements section preserving original ordering where possible const keptOrder: RequirementBlock[] = []; const seen = new Set(); - // What the section will actually contain, in order. Kept apart from - // `keptOrder` because that one answers "are there any requirements left", - // which salvaged content must not influence. - const orderedBody: string[] = []; - // Content that was never a requirement's own but sat inside its block, paired - // with the position it should keep. See `salvageForeignTail`. - const pendingTails: Array<{ at: number; tail: string }> = []; for (const block of parts.bodyBlocks) { const key = normalizeRequirementName(block.name); const replacement = nameToBlock.get(key); - // Read off the ORIGINAL block - the only copy that still has it. - const foreignTail = salvageForeignTail(block.raw); if (replacement) { keptOrder.push(replacement); - orderedBody.push(replacement.raw); seen.add(key); } - // Re-insert the tail unless this block came through untouched, in which - // case it still carries it. Identity, not text: two requirements can carry - // the same note, and a containment check would drop the second copy. - if (foreignTail && replacement !== block) { - pendingTails.push({ at: orderedBody.length, tail: foreignTail }); + // A block's raw runs to the next header the parser RECOGNISES, so anything + // else - a note indented by the 0-3 spaces CommonMark allows, say - is + // absorbed into the requirement above it and goes when that requirement is + // rewritten or removed. Reported rather than moved: a heading-shaped line + // inside a scenario (`# comment`, a markdown example) is indistinguishable + // from a real note here, and relocating one of those corrupts the spec + // silently. Saying what will go is useful whichever it is; moving it is + // only safe for one. + if (replacement !== block) { + const orphan = firstForeignLine(block.raw); + if (orphan) { + warn( + `${specName} - "${orphan}" sits inside requirement "${block.name}" and goes with it. ` + + 'Move it under its own requirement, or above `## Requirements`, to keep it.' + ); + } } } // Append any newly added that were not in original order for (const [key, block] of nameToBlock.entries()) { if (!seen.has(key)) { keptOrder.push(block); - orderedBody.push(block.raw); } } - // Put each salvaged note back where it was written. Walked in reverse so the - // recorded positions are still valid as earlier entries shift. - for (let index = pendingTails.length - 1; index >= 0; index--) { - const { at, tail } = pendingTails[index]; - orderedBody.splice(at, 0, tail); - } const reqBody = [parts.preamble && parts.preamble.trim() ? parts.preamble.trimEnd() : ''] .filter(Boolean) - .concat(orderedBody) + .concat(keptOrder.map((b) => b.raw)) .join('\n\n') .trimEnd(); @@ -470,42 +460,25 @@ export async function buildUpdatedSpec( } /** - * The part of a requirement block's `raw` that was never the requirement's own. - * - * A block runs to the next header the parser RECOGNISES, so a heading it does - * not - one indented by the 0-3 spaces CommonMark allows, or a plain - * `### Notes` - is absorbed into the requirement above it. Removing that - * requirement then deleted the absorbed content too, silently, because nothing - * counted it and nothing reported it. + * The first line of a requirement block that was never the requirement's own - + * a heading at `#`, `##` or `###` after the block's own header - or undefined. * - * Anything from the first `#`/`##`/`###` heading after the block's own header is - * returned so the caller can keep it. `####` is excluded on purpose: a - * requirement's `#### Scenario:` headings are its own and go with it. + * `####` is excluded: a requirement's `#### Scenario:` headings are its own. + * Fenced lines are skipped, so a heading inside an example does not count. * - * Nothing is reclassified. An indented heading is still not a requirement - it - * simply survives its neighbour's removal, which is all this ever needed to do. + * Approximate on purpose, and only ever used to WARN. A `#` line inside a + * scenario looks the same as a note written below the requirement, and no + * line-based rule separates them; a wrong warning costs a line of output, while + * acting on a wrong answer would rewrite the spec. */ -/** A block's own lines, up to whatever was absorbed below it. */ -function requirementOwnLines(raw: string): string[] { +function firstForeignLine(raw: string): string | undefined { const lines = raw.replace(/\r\n?/g, '\n').split('\n'); - const boundary = foreignTailIndex(lines); - return boundary === -1 ? lines : lines.slice(0, boundary); -} - -/** Index of the first line that was never the requirement's own, or -1. */ -function foreignTailIndex(lines: string[]): number { const fenceMask = buildCodeFenceMask(lines); for (let index = 1; index < lines.length; index++) { if (fenceMask[index]) continue; - if (/^ {0,3}#{1,3}\s/.test(lines[index])) return index; + if (/^ {0,3}#{1,3}\s/.test(lines[index])) return lines[index].trim(); } - return -1; -} - -function salvageForeignTail(raw: string): string { - const lines = raw.replace(/\r\n?/g, '\n').split('\n'); - const boundary = foreignTailIndex(lines); - return boundary === -1 ? '' : lines.slice(boundary).join('\n').trimEnd(); + return undefined; } function normalizeBlockRaw(raw: string): string { diff --git a/test/core/specs-apply.salvage.test.ts b/test/core/specs-apply.salvage.test.ts index e683841cef..aaed3e8736 100644 --- a/test/core/specs-apply.salvage.test.ts +++ b/test/core/specs-apply.salvage.test.ts @@ -4,234 +4,130 @@ import path from 'path'; import os from 'os'; import { buildUpdatedSpec, findSpecUpdates } from '../../src/core/specs-apply.js'; -// A requirement block runs to the next header the parser RECOGNISES, so a -// heading it does not - one indented by the 0-3 spaces CommonMark allows, or a -// plain `### Notes` - is absorbed into the requirement above it. Removing that -// requirement deleted the absorbed content too. Silently: nothing counted it, so -// nothing warned, and the spec that remained still validated. -describe('buildUpdatedSpec (content absorbed into a removed requirement)', () => { +// A requirement block runs to the next header the parser RECOGNISES, so a note +// written below it - indented by the 0-3 spaces CommonMark allows, say - is +// absorbed into that requirement and goes when the requirement is rewritten or +// removed. The loss was silent: nothing counted the note, so nothing said a +// word, and the spec left behind still validated. +// +// It is reported, not moved. A heading-shaped line inside a scenario (a +// `# comment`, a markdown example) is indistinguishable from a real note by any +// line-based rule, and relocating one of those rewrites the spec wrongly - +// resurrecting superseded text on MODIFIED, and growing the file on every +// re-apply. A wrong warning costs a line of output instead. +describe('buildUpdatedSpec (content absorbed into a requirement)', () => { let tempDir: string; beforeEach(async () => { - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-salvage-')); + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-orphan-')); }); afterEach(async () => { await fs.rm(tempDir, { recursive: true, force: true }); }); - async function rebuild(foreign: string[], delta?: string[]): Promise { + async function build(specBody: string[], deltaBody: string[]) { const specsDir = path.join(tempDir, 'openspec', 'specs', 'demo'); - const changeDir = path.join(tempDir, 'openspec', 'changes', 'drop'); + const changeDir = path.join(tempDir, 'openspec', 'changes', 'c'); await fs.mkdir(specsDir, { recursive: true }); await fs.mkdir(path.join(changeDir, 'specs', 'demo'), { recursive: true }); - await fs.writeFile( - path.join(specsDir, 'spec.md'), - [ - '# demo Specification', - '', - '## Purpose', - 'Why this exists.', - '', - '## Requirements', - '', - '### Requirement: Doomed', - 'The system SHALL do the doomed thing.', - '', - '#### Scenario: One', - '- **WHEN** a', - '- **THEN** b', - '', - ...foreign, - '', - '### Requirement: Survivor', - 'The system SHALL survive.', - '', - '#### Scenario: Two', - '- **WHEN** c', - '- **THEN** d', - '', - ].join('\n') - ); - await fs.writeFile( - path.join(changeDir, 'specs', 'demo', 'spec.md'), - ( - delta ?? [ - '# demo - Changes', - '', - '## REMOVED Requirements', - '', - '### Requirement: Doomed', - '**Reason**: Superseded.', - '**Migration**: None.', - '', - ] - ).join('\n') - ); + await fs.writeFile(path.join(specsDir, 'spec.md'), specBody.join('\n')); + await fs.writeFile(path.join(changeDir, 'specs', 'demo', 'spec.md'), deltaBody.join('\n')); const [update] = await findSpecUpdates(changeDir, path.join(tempDir, 'openspec', 'specs')); - const built = await buildUpdatedSpec(update, 'drop', { silent: true }); - return built.rebuilt; + return buildUpdatedSpec(update, 'c', { silent: true }); } + const REQUIREMENT = [ + '### Requirement: Target', + 'The system SHALL target.', + '', + '#### Scenario: S', + '- **WHEN** a', + '- **THEN** b', + ]; + const SPEC = (middle: string[]) => [ + '# demo Specification', + '', + '## Purpose', + 'Why this exists.', + '', + '## Requirements', + '', + ...REQUIREMENT, + '', + ...middle, + '', + '### Requirement: Other', + 'The system SHALL other.', + '', + '#### Scenario: T', + '- **WHEN** c', + '- **THEN** d', + '', + ]; + const REMOVE = [ + '# demo - Changes', + '', + '## REMOVED Requirements', + '', + '### Requirement: Target', + '**Reason**: x.', + '**Migration**: None.', + '', + ]; + it.each([ - { what: 'an indented requirement header', foreign: [' ### Requirement: Audit trail', ' The system SHALL retain it.'] }, - { what: 'a heading that is not a requirement', foreign: ['### Notes', 'Kept by hand, never delete.'] }, - { what: 'an indented non-requirement heading', foreign: [' ### Notes', 'Indented, kept by hand.'] }, - ])('keeps $what when the requirement above it is removed', async ({ foreign }) => { - const rebuilt = await rebuild(foreign); - for (const line of foreign) { - expect(rebuilt).toContain(line.trim()); - } - // The removal itself still happened, and the neighbour is untouched. - expect(rebuilt).not.toContain('The system SHALL do the doomed thing.'); - expect(rebuilt).toContain('### Requirement: Survivor'); + { what: 'an indented note', line: ' ### Notes' }, + { what: 'an unindented note', line: '### Notes' }, + { what: 'an indented requirement header', line: ' ### Requirement: Absorbed' }, + ])('warns that $what goes with the requirement it sits in', async ({ line }) => { + const { warnings } = await build(SPEC([line, 'Kept by hand.']), REMOVE); + expect(warnings.join('\n')).toContain(line.trim()); + expect(warnings.join('\n')).toContain('goes with it'); }); - it("keeps a requirement's own scenarios with it when it is removed", async () => { - // `####` must NOT count as a boundary, or every requirement would be severed - // from its scenarios and they would survive as orphans. - const rebuilt = await rebuild([]); - expect(rebuilt).not.toContain('#### Scenario: One'); - expect(rebuilt).toContain('#### Scenario: Two'); + it('says nothing when a requirement holds only its own content', async () => { + const { warnings } = await build(SPEC([]), REMOVE); + expect(warnings.join('\n')).not.toContain('goes with it'); }); - // A RENAMED block is the original with its header swapped, so it still holds - // the absorbed content. A MODIFIED one is rebuilt from the delta and does not - // - dropping it there loses the content exactly as removing the requirement - // would, which the first version of this fix missed. - it('keeps absorbed content when the requirement above it is MODIFIED', async () => { - const rebuilt = await rebuild( - [' ### Notes', ' Kept by hand, never delete.'], - [ - '# demo - Changes', - '', - '## MODIFIED Requirements', - '', - '### Requirement: Doomed', - 'The system SHALL do the doomed thing, now better.', - '', - '#### Scenario: One', - '- **WHEN** a', - '- **THEN** b', - '', - ] - ); - expect(rebuilt).toContain('Kept by hand, never delete.'); - // Exactly once - a rename path that already carries the tail must not - // duplicate it. - expect(rebuilt.match(/Kept by hand/g)).toHaveLength(1); - expect(rebuilt).toContain('now better'); - // And it stays where the author put it, not appended at the end. - expect(rebuilt.indexOf('Kept by hand')).toBeLessThan(rebuilt.indexOf('Requirement: Survivor')); + it('does not warn about a requirement left untouched', async () => { + // The note sits in `Target`, which this delta does not mention. + const { warnings } = await build(SPEC([' ### Notes', 'Kept by hand.']), [ + '# demo - Changes', + '', + '## ADDED Requirements', + '', + '### Requirement: Fresh', + 'The system SHALL be fresh.', + '', + '#### Scenario: F', + '- **WHEN** a', + '- **THEN** b', + '', + ]); + expect(warnings.join('\n')).not.toContain('goes with it'); }); - it('does not duplicate absorbed content when the requirement is RENAMED', async () => { - const rebuilt = await rebuild( - [' ### Notes', ' Kept by hand, never delete.'], - [ - '# demo - Changes', - '', - '## RENAMED Requirements', - '', - '- FROM: `### Requirement: Doomed`', - '- TO: `### Requirement: Renamed`', - '', - ] + it('ignores a heading inside a fenced example', async () => { + const { warnings } = await build( + SPEC(['```markdown', '### Requirement: Example', '```']), + REMOVE ); - expect(rebuilt.match(/Kept by hand/g)).toHaveLength(1); - expect(rebuilt).toContain('### Requirement: Renamed'); + expect(warnings.join('\n')).not.toContain('goes with it'); }); - // Whether a note survived cannot be decided by looking for its text in the - // result: two requirements may carry the same note, and a containment check - // drops the second copy. Survival is decided by whether the block came - // through untouched, which is a question about identity, not text. - it('keeps both copies when two removed requirements carry the same note', async () => { - const specsDir = path.join(tempDir, 'openspec', 'specs', 'demo'); - const changeDir = path.join(tempDir, 'openspec', 'changes', 'drop'); - await fs.mkdir(specsDir, { recursive: true }); - await fs.mkdir(path.join(changeDir, 'specs', 'demo'), { recursive: true }); - const note = [' ### Notes', ' Owned by payments.']; - await fs.writeFile( - path.join(specsDir, 'spec.md'), - [ - '# demo Specification', - '', - '## Purpose', - 'Why this exists.', - '', - '## Requirements', - '', - '### Requirement: Alpha', - 'The system SHALL alpha.', - '', - '#### Scenario: A', - '- **WHEN** a', - '- **THEN** b', - '', - ...note, - '', - '### Requirement: Beta', - 'The system SHALL beta.', - '', - '#### Scenario: B', - '- **WHEN** c', - '- **THEN** d', - '', - ...note, - '', - '### Requirement: Gamma', - 'The system SHALL gamma.', - '', - '#### Scenario: G', - '- **WHEN** e', - '- **THEN** f', - '', - ].join('\n') - ); - await fs.writeFile( - path.join(changeDir, 'specs', 'demo', 'spec.md'), - [ - '# demo - Changes', - '', - '## REMOVED Requirements', - '', - '### Requirement: Alpha', - '**Reason**: x.', - '**Migration**: None.', - '', - '### Requirement: Beta', - '**Reason**: y.', - '**Migration**: None.', - '', - ].join('\n') - ); - const [update] = await findSpecUpdates(changeDir, path.join(tempDir, 'openspec', 'specs')); - const { rebuilt } = await buildUpdatedSpec(update, 'drop', { silent: true }); - - // Two notes were written; two must survive. - expect(rebuilt.match(/### Notes/g)).toHaveLength(2); - expect(rebuilt).toContain('### Requirement: Gamma'); + it("leaves a requirement's own scenarios alone", async () => { + // `####` must not count, or every requirement would look like it holds + // foreign content. + const { warnings } = await build(SPEC([]), REMOVE); + expect(warnings.join('\n')).not.toContain('Scenario'); }); - it('does not duplicate a note when its requirement is untouched', async () => { - // An untouched block is the original object and still carries its note, so - // re-inserting would double it. - const rebuilt = await rebuild( - [' ### Notes', ' Kept by hand, never delete.'], - [ - '# demo - Changes', - '', - '## ADDED Requirements', - '', - '### Requirement: Fresh', - 'The system SHALL be fresh.', - '', - '#### Scenario: F', - '- **WHEN** a', - '- **THEN** b', - '', - ] - ); - expect(rebuilt.match(/Kept by hand/g)).toHaveLength(1); + it('rewrites the spec exactly as before - nothing is moved', async () => { + const { rebuilt } = await build(SPEC([' ### Notes', 'Kept by hand.']), REMOVE); + // The note is reported, not relocated: it goes with the requirement, which + // is the pre-existing behaviour this warning exists to surface. + expect(rebuilt).not.toContain('Kept by hand.'); + expect(rebuilt).toContain('### Requirement: Other'); }); }); From 10e54d63d9855a8f2f4f8a18d53274352378deb5 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 19:38:10 -0500 Subject: [PATCH 5/5] fix(archive): warn before actual content loss --- .changeset/indented-atx-headings.md | 2 +- src/core/archive.ts | 48 +++++++--- src/core/specs-apply.ts | 61 +++++++++---- test/core/archive.test.ts | 126 ++++++++++++++++++++++++++ test/core/specs-apply.salvage.test.ts | 102 +++++++++++++++++++++ 5 files changed, 306 insertions(+), 33 deletions(-) diff --git a/.changeset/indented-atx-headings.md b/.changeset/indented-atx-headings.md index 3ee95c6a31..7c12cbc681 100644 --- a/.changeset/indented-atx-headings.md +++ b/.changeset/indented-atx-headings.md @@ -2,4 +2,4 @@ "@fission-ai/openspec": patch --- -Say when archiving a change will delete a note written next to a requirement. A requirement absorbs anything below it that OpenSpec doesn't recognise as a new heading — a note indented by the one to three spaces Markdown allows, for example — so removing or modifying that requirement took the note with it, silently. `openspec archive` now names the content and where to move it to keep it. The merge itself is unchanged: nothing is relocated, because a `#` line inside a scenario looks identical to a note and moving one of those would rewrite the spec wrongly. +Say before confirmation when archiving a change will delete a note written next to a requirement. A requirement absorbs anything below it that OpenSpec doesn't recognize as a new heading — a note indented by the one to three spaces Markdown allows, for example — so removing or modifying that requirement took the note with it, silently. `openspec archive` now names content the rebuilt spec would actually drop and where to move it to keep it. The merge itself is unchanged: nothing is relocated, because a `#` line inside a scenario looks identical to a note and moving one of those would rewrite the spec wrongly. diff --git a/src/core/archive.ts b/src/core/archive.ts index ef340937f8..4bafd51cd4 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -558,6 +558,32 @@ export class ArchiveCommand { } } + // Build the proposed updates before asking permission to apply them. + // buildUpdatedSpec also reports content that the merge would drop, so + // the confirmation must come after this preview. + const prepared: Array<{ + update: SpecUpdate; + rebuilt: string; + counts: { added: number; modified: number; removed: number; renamed: number }; + }> = []; + let prepareError: unknown; + try { + for (const update of specUpdates) { + const built = await buildUpdatedSpec(update, changeName!, { silent: true }); + prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts }); + specWarnings.push(...built.warnings); + } + } catch (err: unknown) { + // A user may still decline spec updates and archive the change, as + // before this preview existed. Defer the error until they accept. + prepareError = err; + } + if (prepareError === undefined && !json) { + for (const warning of specWarnings) { + console.log(chalk.yellow(`⚠️ Warning: ${warning}`)); + } + } + let shouldUpdateSpecs = true; if (!options.yes) { if (json) { @@ -585,32 +611,24 @@ export class ArchiveCommand { } if (shouldUpdateSpecs) { - // Prepare all updates first (validation pass, no writes) - const prepared: Array<{ update: SpecUpdate; rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number } }> = []; - try { - for (const update of specUpdates) { - const built = await buildUpdatedSpec(update, changeName!, { silent: json }); - prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts }); - // Carried into the result so JSON mode (where nothing was - // printed) still surfaces them; human mode discards the result. - specWarnings.push(...built.warnings); - } - } catch (err: any) { + if (prepareError !== undefined) { + const message = + prepareError instanceof Error ? prepareError.message : String(prepareError); if (json) { throw new ArchiveBlockedError( 'archive_spec_update_failed', - String(err.message || err), + message, 'Fix the change delta specs and rerun. No files were changed.' ); } - console.log(String(err.message || err)); + console.log(message); console.log('Aborted. No files were changed.'); process.exitCode = 1; return null; } - // Validate every rebuilt spec before writing any of them, so a - // late validation failure really does leave all targets unchanged. + // Validate every rebuilt spec before writing any of them, so a late + // validation failure really does leave all targets unchanged. if (!skipValidation) { for (const p of prepared) { const specName = p.update.id; diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 8260febee3..0d50d90494 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -282,6 +282,7 @@ export async function buildUpdatedSpec( // Apply operations in order: RENAMED → REMOVED → MODIFIED → ADDED // RENAMED let renamedApplied = 0; + const renamedTargets = new Map(); for (const r of plan.renamed) { const from = normalizeRequirementName(r.from); const to = normalizeRequirementName(r.to); @@ -319,6 +320,7 @@ export async function buildUpdatedSpec( }; nameToBlock.delete(from); nameToBlock.set(to, renamedBlock); + renamedTargets.set(from, to); renamedApplied++; } @@ -411,19 +413,27 @@ export async function buildUpdatedSpec( keptOrder.push(replacement); seen.add(key); } - // A block's raw runs to the next header the parser RECOGNISES, so anything - // else - a note indented by the 0-3 spaces CommonMark allows, say - is - // absorbed into the requirement above it and goes when that requirement is - // rewritten or removed. Reported rather than moved: a heading-shaped line - // inside a scenario (`# comment`, a markdown example) is indistinguishable - // from a real note here, and relocating one of those corrupts the spec - // silently. Saying what will go is useful whichever it is; moving it is - // only safe for one. - if (replacement !== block) { - const orphan = firstForeignLine(block.raw); - if (orphan) { + // A block's raw runs to the next header the parser RECOGNISES, so a note + // under an unrecognized heading can be absorbed into the requirement. + // Warn only when the replacement from this same original block drops the + // full absorbed suffix. RENAMED carries the original raw content under a + // new map key, and MODIFIED may repeat the suffix deliberately; neither is + // data loss. + const renamedTarget = renamedTargets.get(key); + const replacementFromOriginal = + replacement ?? (renamedTarget ? nameToBlock.get(renamedTarget) : undefined); + if (replacementFromOriginal !== block) { + const foreign = firstForeignTail(block.raw); + const replacementRaw = replacementFromOriginal?.raw; + const normalizedForeign = foreign ? normalizeBlockRaw(foreign.raw) : ''; + const keepsForeignTail = + foreign !== undefined && + replacementRaw !== undefined && + countOccurrences(normalizeBlockRaw(replacementRaw), normalizedForeign) >= + countOccurrences(normalizeBlockRaw(block.raw), normalizedForeign); + if (foreign && !keepsForeignTail) { warn( - `${specName} - "${orphan}" sits inside requirement "${block.name}" and goes with it. ` + + `${specName} - "${foreign.heading}" sits inside requirement "${block.name}" and goes with it. ` + 'Move it under its own requirement, or above `## Requirements`, to keep it.' ); } @@ -460,8 +470,9 @@ export async function buildUpdatedSpec( } /** - * The first line of a requirement block that was never the requirement's own - - * a heading at `#`, `##` or `###` after the block's own header - or undefined. + * The suffix of a requirement block that begins with content the requirement + * parser did not recognize as a boundary: a `#`, `##`, or `###` heading after + * the block's own header. * * `####` is excluded: a requirement's `#### Scenario:` headings are its own. * Fenced lines are skipped, so a heading inside an example does not count. @@ -471,12 +482,17 @@ export async function buildUpdatedSpec( * line-based rule separates them; a wrong warning costs a line of output, while * acting on a wrong answer would rewrite the spec. */ -function firstForeignLine(raw: string): string | undefined { +function firstForeignTail(raw: string): { heading: string; raw: string } | undefined { const lines = raw.replace(/\r\n?/g, '\n').split('\n'); const fenceMask = buildCodeFenceMask(lines); for (let index = 1; index < lines.length; index++) { if (fenceMask[index]) continue; - if (/^ {0,3}#{1,3}\s/.test(lines[index])) return lines[index].trim(); + if (/^ {0,3}#{1,3}(?:[ \t]|$)/.test(lines[index])) { + return { + heading: lines[index].trim(), + raw: lines.slice(index).join('\n').trimEnd(), + }; + } } return undefined; } @@ -485,6 +501,18 @@ function normalizeBlockRaw(raw: string): string { return raw.replace(/\r\n?/g, '\n').trim(); } +/** Count non-overlapping copies so one retained duplicate cannot mask another copy's loss. */ +function countOccurrences(haystack: string, needle: string): number { + if (!needle) return 0; + let count = 0; + let start = 0; + while ((start = haystack.indexOf(needle, start)) !== -1) { + count++; + start += needle.length; + } + return count; +} + /** * Write an updated spec to disk. */ @@ -606,4 +634,3 @@ export function buildSpecSkeleton(specFolderName: string, changeName: string, pu purpose?.trim() || `TBD - created by archiving change ${changeName}. Update Purpose after archive.`; return `# ${titleBase} Specification\n\n## Purpose\n${purposeBody}\n\n## Requirements\n`; } - diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 75c4d85f43..7bf3014cd2 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -1842,6 +1842,132 @@ Then expected result happens`; expect(archives[0]).toMatch(new RegExp(`\\d{4}-\\d{2}-\\d{2}-${changeName}`)); }); + it('warns about absorbed content before asking to apply the destructive spec update', async () => { + const { confirm } = await import('@inquirer/prompts'); + const mockConfirm = confirm as unknown as ReturnType; + const changeName = 'warn-before-spec-update'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'demo'); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'demo'); + await fs.mkdir(changeSpecDir, { recursive: true }); + await fs.mkdir(mainSpecDir, { recursive: true }); + + const mainSpec = `# demo Specification + +## Purpose +This capability exists to exercise archive warning behavior. + +## Requirements + +### Requirement: Target +The system SHALL target. + +#### Scenario: Target works +- **WHEN** it runs +- **THEN** it works + + ### Notes +Keep this note. + +### Requirement: Survivor +The system SHALL survive. + +#### Scenario: Survivor works +- **WHEN** it runs +- **THEN** it survives +`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec); + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `# demo - Changes + +## REMOVED Requirements + +### Requirement: Target +**Reason**: It is obsolete. +` + ); + + mockConfirm.mockReset(); + mockConfirm.mockImplementationOnce(async () => { + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('"### Notes" sits inside requirement "Target"') + ); + return false; + }); + + await archiveCommand.execute(changeName); + + expect(mockConfirm).toHaveBeenCalledWith({ + message: 'Proceed with spec updates?', + default: true, + }); + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(mainSpec); + await expect(fs.access(changeDir)).rejects.toThrow(); + }); + + it('prints the loss warning before --yes writes the spec', async () => { + const changeName = 'warn-before-yes-write'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'demo'); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'demo'); + await fs.mkdir(changeSpecDir, { recursive: true }); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `# demo Specification + +## Purpose +This capability exists to exercise archive warning behavior. + +## Requirements + +### Requirement: Target +The system SHALL target. + +#### Scenario: Target works +- **WHEN** it runs +- **THEN** it works + + ### Notes +Keep this note. + +### Requirement: Survivor +The system SHALL survive. + +#### Scenario: Survivor works +- **WHEN** it runs +- **THEN** it survives +` + ); + await fs.writeFile( + path.join(changeSpecDir, 'spec.md'), + `# demo - Changes + +## REMOVED Requirements + +### Requirement: Target +**Reason**: It is obsolete. +` + ); + + await archiveCommand.execute(changeName, { yes: true }); + + const output = ( + console.log as unknown as { mock: { calls: unknown[][] } } + ).mock.calls.flat().map(String); + const warningIndex = output.findIndex((line) => + line.includes('"### Notes" sits inside requirement "Target"') + ); + const successIndex = output.indexOf('Specs updated successfully.'); + expect(warningIndex).toBeGreaterThanOrEqual(0); + expect(successIndex).toBeGreaterThan(warningIndex); + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.not.toContain( + 'Keep this note.' + ); + await expect(fs.access(changeDir)).rejects.toThrow(); + }); + it('should support header trim-only normalization for matching', async () => { const changeName = 'normalize-headers'; const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); diff --git a/test/core/specs-apply.salvage.test.ts b/test/core/specs-apply.salvage.test.ts index aaed3e8736..af82c2be6e 100644 --- a/test/core/specs-apply.salvage.test.ts +++ b/test/core/specs-apply.salvage.test.ts @@ -79,6 +79,7 @@ describe('buildUpdatedSpec (content absorbed into a requirement)', () => { { what: 'an indented note', line: ' ### Notes' }, { what: 'an unindented note', line: '### Notes' }, { what: 'an indented requirement header', line: ' ### Requirement: Absorbed' }, + { what: 'an empty ATX heading', line: '###' }, ])('warns that $what goes with the requirement it sits in', async ({ line }) => { const { warnings } = await build(SPEC([line, 'Kept by hand.']), REMOVE); expect(warnings.join('\n')).toContain(line.trim()); @@ -123,6 +124,107 @@ describe('buildUpdatedSpec (content absorbed into a requirement)', () => { expect(warnings.join('\n')).not.toContain('Scenario'); }); + it('does not warn when RENAMED carries the full absorbed tail forward', async () => { + const tail = [' ### Notes', 'Kept by hand.']; + const { rebuilt, counts, warnings } = await build(SPEC(tail), [ + '# demo - Changes', + '', + '## RENAMED Requirements', + '', + '- FROM: `### Requirement: Target`', + '- TO: `### Requirement: Renamed`', + '', + ]); + + expect(rebuilt).toContain(tail.join('\n')); + expect(counts.renamed).toBe(1); + expect(warnings.join('\n')).not.toContain('goes with it'); + }); + + it('does not warn when MODIFIED carries the full absorbed tail forward', async () => { + const tail = [' ### Notes', 'Kept by hand.']; + const { rebuilt, counts, warnings } = await build(SPEC(tail), [ + '# demo - Changes', + '', + '## MODIFIED Requirements', + '', + ...REQUIREMENT, + '', + ...tail, + '', + ]); + + expect(rebuilt).toContain(tail.join('\n')); + expect(counts.modified).toBe(0); + expect(warnings.join('\n')).not.toContain('goes with it'); + }); + + it('warns when MODIFIED keeps the heading but drops part of the absorbed tail', async () => { + const tail = [' ### Notes', 'Kept by hand.']; + const { rebuilt, warnings } = await build(SPEC(tail), [ + '# demo - Changes', + '', + '## MODIFIED Requirements', + '', + ...REQUIREMENT, + '', + tail[0], + '', + ]); + + expect(rebuilt).not.toContain(tail[1]); + expect(warnings.join('\n')).toContain(tail[0].trim()); + expect(warnings.join('\n')).toContain('goes with it'); + }); + + it('does not let an identical earlier copy mask loss of the absorbed tail', async () => { + const repeated = [' ### Notes', 'Kept by hand.']; + const requirementWithExample = [ + '### Requirement: Target', + 'The system SHALL target.', + '', + '```markdown', + ...repeated, + '```', + '', + '#### Scenario: S', + '- **WHEN** a', + '- **THEN** b', + ]; + const spec = [ + '# demo Specification', + '', + '## Purpose', + 'Why this exists.', + '', + '## Requirements', + '', + ...requirementWithExample, + '', + ...repeated, + '', + '### Requirement: Other', + 'The system SHALL other.', + '', + '#### Scenario: T', + '- **WHEN** c', + '- **THEN** d', + '', + ]; + const { rebuilt, warnings } = await build(spec, [ + '# demo - Changes', + '', + '## MODIFIED Requirements', + '', + ...requirementWithExample, + '', + ]); + + expect(rebuilt).toContain(repeated.join('\n')); + expect(warnings.join('\n')).toContain(repeated[0].trim()); + expect(warnings.join('\n')).toContain('goes with it'); + }); + it('rewrites the spec exactly as before - nothing is moved', async () => { const { rebuilt } = await build(SPEC([' ### Notes', 'Kept by hand.']), REMOVE); // The note is reported, not relocated: it goes with the requirement, which