diff --git a/.changeset/indented-atx-headings.md b/.changeset/indented-atx-headings.md new file mode 100644 index 0000000000..7c12cbc681 --- /dev/null +++ b/.changeset/indented-atx-headings.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +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 2f9c1a5e3d..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,6 +413,31 @@ export async function buildUpdatedSpec( keptOrder.push(replacement); seen.add(key); } + // 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} - "${foreign.heading}" 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()) { @@ -442,10 +469,50 @@ export async function buildUpdatedSpec( }; } +/** + * 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. + * + * 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. + */ +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}(?:[ \t]|$)/.test(lines[index])) { + return { + heading: lines[index].trim(), + raw: lines.slice(index).join('\n').trimEnd(), + }; + } + } + return undefined; +} + 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. */ @@ -567,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 new file mode 100644 index 0000000000..af82c2be6e --- /dev/null +++ b/test/core/specs-apply.salvage.test.ts @@ -0,0 +1,235 @@ +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 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-orphan-')); + }); + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + async function build(specBody: string[], deltaBody: string[]) { + const specsDir = path.join(tempDir, 'openspec', 'specs', 'demo'); + 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'), 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')); + 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 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()); + expect(warnings.join('\n')).toContain('goes with it'); + }); + + 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'); + }); + + 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('ignores a heading inside a fenced example', async () => { + const { warnings } = await build( + SPEC(['```markdown', '### Requirement: Example', '```']), + REMOVE + ); + expect(warnings.join('\n')).not.toContain('goes with it'); + }); + + 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 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 + // is the pre-existing behaviour this warning exists to surface. + expect(rebuilt).not.toContain('Kept by hand.'); + expect(rebuilt).toContain('### Requirement: Other'); + }); +});