From 21ad9b68c4cf25ec11aa9f926be7c20ccf13b741 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 11:18:08 -0500 Subject: [PATCH 01/40] fix(archive): retire a capability when a change removes its last requirement A delta whose REMOVED entries cover every requirement rebuilt the main spec empty, and an empty spec fails validation ("Spec must have at least one requirement"), so the archive aborted with no way forward. Pre-deleting the main spec did not help: the delta was then treated as a create and landed on the same empty spec. Archive now treats an emptied capability as retired. It deletes the capability's spec.md and any directory the deletion leaves empty, stopping short of the specs root, and reports the removals in the totals. Nothing is deleted unless this run actually removed a requirement, so a re-applied or already-synced delta still leaves the file alone. Closes #1302 Co-Authored-By: Claude Opus 5 (1M context) --- ...retire-capability-on-removed-only-delta.md | 5 + openspec/specs/cli-archive/spec.md | 27 +++ skills/openspec-archive-change/SKILL.md | 2 +- skills/openspec-sync-specs/SKILL.md | 4 + src/core/archive.ts | 23 +- src/core/specs-apply.ts | 51 +++++ .../templates/workflows/archive-change.ts | 4 +- src/core/templates/workflows/sync-specs.ts | 8 + test/core/archive.test.ts | 204 ++++++++++++++++++ .../templates/skill-templates-parity.test.ts | 12 +- 10 files changed, 329 insertions(+), 11 deletions(-) create mode 100644 .changeset/retire-capability-on-removed-only-delta.md diff --git a/.changeset/retire-capability-on-removed-only-delta.md b/.changeset/retire-capability-on-removed-only-delta.md new file mode 100644 index 0000000000..363ed3fe60 --- /dev/null +++ b/.changeset/retire-capability-on-removed-only-delta.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Retire a capability when a change removes its last requirement, instead of aborting the archive with "Spec must have at least one requirement". diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 586075ec24..3616d1100b 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -122,6 +122,33 @@ Before moving the change to archive, the command SHALL apply delta changes to ma - **THEN** leave the existing Purpose untouched - **AND** warn that the delta Purpose was ignored, naming the spec file to edit directly, but only when that spec has a Purpose of its own and it differs from the delta's +### Requirement: Capability Retirement + +A delta whose REMOVED entries cover every requirement a capability has SHALL retire that capability instead of writing a main spec with no requirements, which can never pass validation. + +#### Scenario: Delta removes the capability's last requirement + +- **WHEN** applying a delta leaves the target main spec with no requirements +- **AND** at least one requirement was actually removed by this run +- **THEN** delete the capability's `spec.md` instead of writing it +- **AND** delete any directory the deletion leaves empty, up to but never including the specs root +- **AND** count the removals in the archive totals and complete the archive + +#### Scenario: Capability directory holds other files + +- **WHEN** retiring a capability whose directory still holds other files after `spec.md` is deleted +- **THEN** leave that directory in place + +#### Scenario: Removal was already synced + +- **WHEN** applying a delta leaves the main spec with no requirements but removed nothing this run +- **THEN** leave the existing file untouched and complete the archive + +#### Scenario: Main spec was already deleted + +- **WHEN** a REMOVED-only delta targets a capability that has no main spec +- **THEN** complete the archive without creating one + ### Requirement: Confirmation Behavior The spec update confirmation SHALL provide clear visibility into changes before they are applied. diff --git a/skills/openspec-archive-change/SKILL.md b/skills/openspec-archive-change/SKILL.md index d028076057..f4a1fd9d25 100644 --- a/skills/openspec-archive-change/SKILL.md +++ b/skills/openspec-archive-change/SKILL.md @@ -119,7 +119,7 @@ Archive a completed change in the experimental workflow. Then re-run the comparison from the top of this step against every capability that has a delta spec in `artifactPaths.specs.existingOutputPaths` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone + - REMOVED requirements gone — and where a delta removed a capability's last requirement, its main spec deleted rather than left empty - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and `changeRoot` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index 49e4612c2d..dd7a99b193 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -107,6 +107,10 @@ This is an **agent-driven** operation - you will read delta specs and directly e **REMOVED Requirements:** - Remove the entire requirement block from main spec + - If that leaves the main spec with no requirements at all, the capability is + retired: delete its `spec.md` (and the directory, once nothing else is left + in it) instead of saving an empty spec. A spec with zero requirements fails + `openspec validate`, which is what `openspec archive` does here too. **RENAMED Requirements:** - Find the FROM requirement, rename to TO diff --git a/src/core/archive.ts b/src/core/archive.ts index f0f6013b3e..c45ee58725 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -17,6 +17,7 @@ import { findSpecUpdates, buildUpdatedSpec, writeUpdatedSpec, + retireSpec, type SpecUpdate, } from './specs-apply.js'; import { discoverSpecFiles, hasAnyFileUnder } from '../utils/spec-discovery.js'; @@ -468,11 +469,11 @@ 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 } }> = []; + const prepared: Array<{ update: SpecUpdate; rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number }; retired: boolean }> = []; try { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); - prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts }); + prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts, retired: built.retired }); // Carried into the result so JSON mode (where nothing was // printed) still surfaces them; human mode discards the result. specWarnings.push(...built.warnings); @@ -495,6 +496,9 @@ export class ArchiveCommand { // late validation failure really does leave all targets unchanged. if (!skipValidation) { for (const p of prepared) { + // A retired capability has no spec left to validate; the whole + // point is that the empty body could never pass (#1302). + if (p.retired) continue; const specName = p.update.id; const report = await new Validator().validateSpecContent(specName, p.rebuilt); if (!report.valid) { @@ -522,6 +526,21 @@ export class ArchiveCommand { let wroteAny = false; for (const p of prepared) { const { added, modified, removed, renamed } = p.counts; + if (p.retired) { + // Nothing was actually removed this run (the requirements were + // already gone from the baseline), so leave the file alone rather + // than deleting on the strength of a no-op delta. + if (removed === 0) continue; + const deleted = await retireSpec(p.update, mainSpecsDir, { + silent: json, + ...(isStoreSelectedRoot(root) ? { displayPath: p.update.target } : {}), + }); + if (deleted) { + wroteAny = true; + writeTotals.removed += removed; + } + continue; + } if (added + modified + removed + renamed === 0) { // Every operation was already synced: rewriting the file would // only churn normalization differences into it. diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index e8d2f3910f..df55e9e6e9 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -89,6 +89,12 @@ export async function buildUpdatedSpec( rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number }; warnings: string[]; + /** + * The delta left the capability with no requirements at all, so `rebuilt` is a + * spec body that can never validate ("Spec must have at least one + * requirement"). Callers retire the capability instead of writing it (#1302). + */ + retired: boolean; }> { // Collected so silent (JSON) callers can surface them; printed live for // human callers at the point they occur. @@ -443,9 +449,54 @@ export async function buildUpdatedSpec( renamed: renamedApplied, }, warnings, + // Only ADDED grows the requirement set, so an empty result means the delta + // removed the last requirement the capability had. There is no valid spec + // to write for that state - every such archive aborted before #1302. + retired: keptOrder.length === 0, }; } +/** + * Retire a capability whose last requirement a delta removed: delete its main + * spec and any directories the deletion leaves empty, up to (but never + * including) the specs root. Returns false when there was nothing to delete. + * + * Only the generated `spec.md` is removed - a directory holding anything else + * (a nested capability, a hand-kept note) is left in place. + */ +export async function retireSpec( + update: SpecUpdate, + mainSpecsDir: string, + options: { silent?: boolean; displayPath?: string } = {} +): Promise { + try { + await fs.unlink(update.target); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false; + throw error; + } + + const specsRoot = path.resolve(mainSpecsDir); + let dir = path.dirname(path.resolve(update.target)); + while (dir !== specsRoot && dir.startsWith(specsRoot + path.sep)) { + try { + const entries = await fs.readdir(dir); + if (entries.length > 0) break; + await fs.rmdir(dir); + } catch { + break; + } + dir = path.dirname(dir); + } + + if (!options.silent) { + console.log( + `Retiring ${options.displayPath ?? `openspec/specs/${update.id}/spec.md`}: all requirements removed.` + ); + } + return true; +} + function normalizeBlockRaw(raw: string): string { return raw.replace(/\r\n?/g, '\n').trim(); } diff --git a/src/core/templates/workflows/archive-change.ts b/src/core/templates/workflows/archive-change.ts index beae52e655..cdcee0f6a2 100644 --- a/src/core/templates/workflows/archive-change.ts +++ b/src/core/templates/workflows/archive-change.ts @@ -121,7 +121,7 @@ ${STORE_SELECTION_GUIDANCE} Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone + - REMOVED requirements gone — and where a delta removed a capability's last requirement, its main spec deleted rather than left empty - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. @@ -301,7 +301,7 @@ ${STORE_SELECTION_GUIDANCE} Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone + - REMOVED requirements gone — and where a delta removed a capability's last requirement, its main spec deleted rather than left empty - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index a172a7fc8c..6d5e94442c 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -109,6 +109,10 @@ ${STORE_SELECTION_GUIDANCE} **REMOVED Requirements:** - Remove the entire requirement block from main spec + - If that leaves the main spec with no requirements at all, the capability is + retired: delete its \`spec.md\` (and the directory, once nothing else is left + in it) instead of saving an empty spec. A spec with zero requirements fails + \`openspec validate\`, which is what \`openspec archive\` does here too. **RENAMED Requirements:** - Find the FROM requirement, rename to TO @@ -332,6 +336,10 @@ ${STORE_SELECTION_GUIDANCE} **REMOVED Requirements:** - Remove the entire requirement block from main spec + - If that leaves the main spec with no requirements at all, the capability is + retired: delete its \`spec.md\` (and the directory, once nothing else is left + in it) instead of saving an empty spec. A spec with zero requirements fails + \`openspec validate\`, which is what \`openspec archive\` does here too. **RENAMED Requirements:** - Find the FROM requirement, rename to TO diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 8937eef399..d1a9c16a35 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -2769,6 +2769,210 @@ The system SHALL do the thing differently. }); }); + // A delta whose REMOVED entries cover every requirement rebuilds the main + // spec empty, and an empty spec can never validate. Every such archive used + // to abort with "Spec must have at least one requirement", leaving no way to + // retire a capability (#1302). + describe('capability retirement (#1302)', () => { + const REQUIREMENT = [ + '### Requirement: The system SHALL provide a legacy layer', + 'The system SHALL provide a legacy layer to existing consumers.', + '', + '#### Scenario: Layer is available', + '- **WHEN** a consumer imports the layer', + '- **THEN** the legacy layer is available', + ].join('\n'); + + const PURPOSE = + 'Holds the behavior contract for the legacy layer that consumers still depend on today.'; + + function mainSpec(name: string, requirements = REQUIREMENT): string { + return `# ${name} Specification\n\n## Purpose\n${PURPOSE}\n\n## Requirements\n\n${requirements}\n`; + } + + const REMOVE_ALL = [ + '# Legacy Layer - Changes', + '', + '## REMOVED Requirements', + '', + '### Requirement: The system SHALL provide a legacy layer', + '**Reason**: The capability is retired.', + '**Migration**: None; consumers already moved off it.', + '', + ].join('\n'); + + async function createChange( + changeName: string, + capability: string, + deltaSpec: string + ): Promise { + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + await fs.mkdir(path.join(changeDir, 'specs', ...capability.split('/')), { + recursive: true, + }); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + await fs.writeFile( + path.join(changeDir, 'specs', ...capability.split('/'), 'spec.md'), + deltaSpec + ); + return changeDir; + } + + it('retires the capability when a delta removes its last requirement', async () => { + const changeName = 'retire-legacy-layer'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true }); + + // The spec and the directory it was alone in are gone... + await expect(fs.access(mainSpecDir)).rejects.toThrow(); + // ...but the specs root itself is never pruned. + await expect( + fs.access(path.join(tempDir, 'openspec', 'specs')) + ).resolves.not.toThrow(); + // The archive completed rather than aborting. + expect(process.exitCode).not.toBe(1); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Retiring openspec/specs/legacy-layer/spec.md') + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Totals: + 0, ~ 0, - 1, → 0') + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Specs updated successfully.') + ); + await expect(fs.access(path.join(tempDir, 'openspec', 'changes', changeName))).rejects.toThrow(); + }); + + it('prunes empty parent directories in a nested layout but keeps siblings', async () => { + const changeName = 'retire-nested'; + await createChange(changeName, 'platform/legacy-layer', REMOVE_ALL); + const nestedDir = path.join(tempDir, 'openspec', 'specs', 'platform', 'legacy-layer'); + const siblingDir = path.join(tempDir, 'openspec', 'specs', 'platform', 'kept'); + await fs.mkdir(nestedDir, { recursive: true }); + await fs.mkdir(siblingDir, { recursive: true }); + await fs.writeFile(path.join(nestedDir, 'spec.md'), mainSpec('legacy-layer')); + await fs.writeFile(path.join(siblingDir, 'spec.md'), mainSpec('kept')); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(nestedDir)).rejects.toThrow(); + // The sibling keeps the shared parent alive. + await expect(fs.access(path.join(siblingDir, 'spec.md'))).resolves.not.toThrow(); + }); + + it('leaves a capability directory that still holds other files', async () => { + const changeName = 'retire-with-notes'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + await fs.writeFile(path.join(mainSpecDir, 'NOTES.md'), 'Kept by hand.\n'); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).rejects.toThrow(); + await expect(fs.readFile(path.join(mainSpecDir, 'NOTES.md'), 'utf-8')).resolves.toBe( + 'Kept by hand.\n' + ); + }); + + it('retires rather than writing an empty spec under --no-validate', async () => { + // --no-validate was the one path that did not abort: it wrote a spec with + // zero requirements, which every later validate then rejected. + const changeName = 'retire-no-validate'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + await expect(fs.access(mainSpecDir)).rejects.toThrow(); + }); + + it('archives a REMOVED-only delta whose main spec was already deleted', async () => { + // The issue's second dead end: pre-deleting the spec made the delta look + // like a create, which landed on an empty spec and failed the same way. + const changeName = 'retire-already-gone'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).not.toBe(1); + // Nothing was recreated. + await expect( + fs.access(path.join(tempDir, 'openspec', 'specs', 'legacy-layer')) + ).rejects.toThrow(); + await expect( + fs.access(path.join(tempDir, 'openspec', 'changes', changeName)) + ).rejects.toThrow(); + }); + + it('does not delete a spec when the removal was already synced', async () => { + // Nothing was removed this run, so the empty spec is the user's to fix - + // deleting on a no-op delta would destroy a file the change never touched. + const changeName = 'retire-noop'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const emptied = `# legacy-layer Specification\n\n## Purpose\nThe legacy layer.\n\n## Requirements\n`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), emptied); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(emptied); + }); + + it('still writes the spec when requirements remain after the removal', async () => { + const changeName = 'partial-removal'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const kept = [ + '### Requirement: The system SHALL provide a core layer', + 'The system SHALL provide a core layer to every consumer.', + '', + '#### Scenario: Core is available', + '- **WHEN** a consumer imports the core', + '- **THEN** the core layer is available', + ].join('\n'); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + mainSpec('legacy-layer', `${REQUIREMENT}\n\n${kept}`) + ); + + await archiveCommand.execute(changeName, { yes: true }); + + const updated = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(updated).toContain('core layer'); + expect(updated).not.toContain('legacy layer is available'); + }); + + it('reports the retirement in --json instead of printing progress lines', async () => { + const changeName = 'retire-json'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true, json: true }); + + await expect(fs.access(mainSpecDir)).rejects.toThrow(); + const calls = (console.log as unknown as ReturnType).mock.calls.map( + (call) => String(call[0]) + ); + // JSON mode prints exactly one payload and no human progress lines. + expect(calls.some((line) => line.includes('Retiring'))).toBe(false); + const payload = JSON.parse(calls[calls.length - 1]); + expect(payload.archive.specsUpdated).toBe(true); + expect(payload.archive.totals).toEqual({ added: 0, modified: 0, removed: 1, renamed: 0 }); + }); + }); + describe('proposal warnings (#498)', () => { const LONG_WHY = 'This change exists to document AI application patterns thoroughly for the team, which is long enough.'; diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 04b1b1aef7..a2615f671d 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,18 +42,18 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: '676e7472977d2b6f4d922ce384db1f15020c195f94d6cd4ee71abcf0201e28a9', getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', - getSyncSpecsSkillTemplate: '977a753b03daa33ddb8aa9bcc632e10d82062c02749a0c821ecc338311251186', + getSyncSpecsSkillTemplate: 'ac3ca79f4b4ab298d1776cb936fa059aa644dd29c3cf82b876e5e852f41455dc', getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', getOpsxContinueCommandTemplate: 'bcf0ad1c55b71346147c5b4dbaed016c77c9718f960012d8efc9d3d2089d0e00', getOpsxApplyCommandTemplate: '18c82fc48e65084065171e44f811db8fdc96bd6cb0f61fe8f31324207f4861c7', getOpsxFfCommandTemplate: '678375642a21d255444f0ba717e659abb2cc2b7474981d52eae900a0793e3e4d', - getArchiveChangeSkillTemplate: '7c1bf2170ba57833f111c79002ea56be3cca499e2b13b2ea8141c182351b1a3b', + getArchiveChangeSkillTemplate: 'cb89425e37acd6a200d5a53d6609a630cb1243c9c9ce06ec1f751072bdb7073d', getBulkArchiveChangeSkillTemplate: 'de198c7b7c1472773b013b9af917de27773fd613083309f0e8e607c005c92d3d', - getOpsxSyncCommandTemplate: 'b1f3fea6a9d4e84f401f411a0fefe330ad9ee81cff065a578f4057386c5d81fa', + getOpsxSyncCommandTemplate: 'e8af3985a831c25c9c46c9969d11ee1f303ae1cc0030d066302a67ec9602fb0a', getVerifyChangeSkillTemplate: '917de96cc8341799107b0617979cdaf30e121c51676272f5caef143b090583f9', - getOpsxArchiveCommandTemplate: 'fa0d2f4c1ff9b499353399ba040caaf2ba070154dac8b94cb4ca8e2568b1717a', + getOpsxArchiveCommandTemplate: '737fc53fe612e439014897bc070481226e9f9de9cb05fe4f18c61cb302e190e9', getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', getOpsxBulkArchiveCommandTemplate: '93355fb7bc13e549e8646e4dc48db6f98ac5372545dff3cf3970c4f45f55c5f7', getOpsxVerifyCommandTemplate: '29e3913c93566e689971d8c15c3348ba4169ebf6b1d403f5ac9974605c734baa', @@ -70,8 +70,8 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '2e1a7d17ec021949d115c72227729609bf9980ad1f23445af117c09834711121', 'openspec-apply-change': '49fc5772404e3033085384ee214c44488c93880a596a9a05dcad42f9ce86cf83', 'openspec-ff-change': '4228d75e3571097164f2360e2ad3063a5b88d44750078c3601b23a89e74c1de6', - 'openspec-sync-specs': 'db79c625bbfa3aaf948812fda5965eda876264973c9c5c4bbeac4a48df77f97d', - 'openspec-archive-change': '84b9d3a5690b8d64e1845b3c7368a4ad43369ea8549a76ef78912690d434363b', + 'openspec-sync-specs': 'cc9dfa449d791f1c18dc68f19b816421b38f1bb227ceaaf9dd139670e5df08c0', + 'openspec-archive-change': '55b01bf4a605c1535ba32dc4e4f671e22743b9071ac21e636dee4316ef144580', 'openspec-bulk-archive-change': '5ac320e2004e453c78541233f48e5f6e246cc674a44f1e427cecb7b2e9587f9b', 'openspec-verify-change': '1c3f73a36be691a18d3acb200d22e6874004d6d4a5d3e2e346ae95a7379e9da8', 'openspec-onboard': 'f2440f59c22b1ac9db33247b23a6fa32fb9cd418dc196486a213f5d7e91b1dbc', From ca8ab19aec5644f037065a73e5f3489293b468f4 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 11:41:35 -0500 Subject: [PATCH 02/40] fix(archive): decide retirement from the validator and contain the deletion Adversarial review found the original rule unsound. It retired whenever no canonical `### Requirement:` blocks were left, but the validator counts requirements differently: MarkdownParser accepts any `###` heading under `## Requirements`, while the delta block parser indexes only canonical headers and sweeps the rest into the preamble, which survives into the rebuilt spec. A strict-valid spec could therefore be deleted on an archive that previously succeeded. Retirement is now decided by putting the rebuilt spec to the validator and retiring only when its sole error is that it has no requirements, which makes "this spec could not have been written anyway" true by construction. Also fixed: - The directory prune walked string prefixes, but path.resolve does not resolve symlinks and readdir/rmdir both follow them, so a symlinked capability directory let it delete directories outside the repository. Pruning is now bounded by real paths and refuses to descend through a symlink. - A spec that was already requirement-less and lost nothing this run is no longer skipped past validation; it aborts exactly as it did before. - Deletions are deferred until every spec write has succeeded, so a later failure cannot leave a spec already deleted. - Retirement is recorded in `warnings`, naming any other sections the deleted file held, so JSON consumers and humans can both see what went. - Totals carry every applied operation; a rename applied on the way to the removal was being dropped. - bulk-archive guidance, the sync/archive skill specs, and the docs that described archive as never deleting a spec. Closes #1302 Co-Authored-By: Claude Opus 5 (1M context) --- docs/agent-contract.md | 2 +- docs/cli.md | 2 +- docs/concepts.md | 2 +- docs/writing-specs.md | 2 +- openspec/specs/cli-archive/spec.md | 25 +- openspec/specs/opsx-archive-skill/spec.md | 1 + openspec/specs/specs-sync-skill/spec.md | 6 + skills/openspec-archive-change/SKILL.md | 2 +- skills/openspec-bulk-archive-change/SKILL.md | 2 +- skills/openspec-sync-specs/SKILL.md | 13 +- src/core/archive.ts | 105 +++++-- src/core/specs-apply.ts | 107 +++++-- .../templates/workflows/archive-change.ts | 4 +- .../workflows/bulk-archive-change.ts | 4 +- src/core/templates/workflows/sync-specs.ts | 26 +- test/core/archive.test.ts | 281 +++++++++++++++++- .../templates/skill-templates-parity.test.ts | 18 +- 17 files changed, 519 insertions(+), 83 deletions(-) diff --git a/docs/agent-contract.md b/docs/agent-contract.md index 65e2004ae7..bd1dd14e66 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -72,7 +72,7 @@ Change: `{ "id", "title", "deltaCount", "deltas": [...], "root" }`. Spec: `{ "id Success: `{ "change": { "id", "path", "metadataPath", "schema" }, "root" }`. Failure: `{ "change": null, "status": [d] }`, exit 1. ### 4.9 `archive --json` -Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"?, "warnings"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. `specsUpdated` is true only when at least one spec file was written; an already-synced change archives with all-zero totals and the skips listed in `warnings`. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. +Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"?, "warnings"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. `specsUpdated` is true only when at least one spec file was written or retired (a capability whose last requirement the change removed has its spec deleted, reported in `warnings`); an already-synced change archives with all-zero totals and the skips listed in `warnings`. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. ### 4.10 `doctor --json` `{ "root": { "path", "source", "store_id"?, "healthy", "status": [] }, "store": { "id", "metadata": {present,valid,remote?}, "origin_url"?, "drift"?: {ahead,behind}, "status": [] } | null, "references": [...], "status": [] }`. `drift` (present only for a git-backed store checkout that has an upstream tracking ref) is ahead/behind counts against the last-fetched upstream, not the live remote. Health findings of any severity exit 0. Failure payload: `{ "root": null, "store": null, "references": [], "status": [d] }`, exit 1. diff --git a/docs/cli.md b/docs/cli.md index 04cb514d2d..e4e86083be 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -651,7 +651,7 @@ openspec archive update-ci-config --skip-specs 1. Validates the change (unless `--no-validate`) 2. Prompts for confirmation (unless `--yes`) -3. Merges delta specs into `openspec/specs/` +3. Merges delta specs into `openspec/specs/` — a capability whose last requirement the change removes is retired, and its spec file deleted 4. Moves change folder to `openspec/changes/archive/YYYY-MM-DD-/` --- diff --git a/docs/concepts.md b/docs/concepts.md index caca2bc140..4eb7933f54 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -392,7 +392,7 @@ The system MUST expire sessions after 15 minutes of inactivity. |---------|---------|------------------------| | `## ADDED Requirements` | New behavior | Appended to main spec | | `## MODIFIED Requirements` | Changed behavior | Replaces existing requirement | -| `## REMOVED Requirements` | Deprecated behavior | Deleted from main spec | +| `## REMOVED Requirements` | Deprecated behavior | Deleted from main spec; removing the last requirement retires the capability and deletes its spec file | | `## Purpose` | What a brand-new capability is for | Seeds the Purpose of the main spec being created; ignored when the spec already exists | ### Why Deltas Instead of Full Specs diff --git a/docs/writing-specs.md b/docs/writing-specs.md index c894c8f2cb..a8056ea427 100644 --- a/docs/writing-specs.md +++ b/docs/writing-specs.md @@ -56,7 +56,7 @@ A change describes its edits to the specs with three section types. Using the ri - **`## MODIFIED Requirements`** — behavior that already existed and is changing. Include the full new version; a short note on what changed helps a reviewer. - **`## REMOVED Requirements`** — behavior going away, with a line on why. -On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is deleted. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. +On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is deleted. Remove the last requirement a capability has and you retire it: archive deletes `openspec/specs//spec.md` rather than leave a spec with nothing in it. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 3616d1100b..445d3d8710 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -126,13 +126,26 @@ Before moving the change to archive, the command SHALL apply delta changes to ma A delta whose REMOVED entries cover every requirement a capability has SHALL retire that capability instead of writing a main spec with no requirements, which can never pass validation. +#### Scenario: Deciding that a rebuilt spec cannot be written + +- **WHEN** applying a delta leaves the rebuilt spec with no requirement blocks +- **THEN** put that rebuilt spec to the spec validator +- **AND** treat it as retirable only when its sole validation error is that the spec has no requirements +- **AND** otherwise write or reject it exactly as any other rebuilt spec, so a spec the validator still accepts is never deleted + #### Scenario: Delta removes the capability's last requirement -- **WHEN** applying a delta leaves the target main spec with no requirements +- **WHEN** a retirable rebuilt spec belongs to a capability whose main spec exists - **AND** at least one requirement was actually removed by this run - **THEN** delete the capability's `spec.md` instead of writing it -- **AND** delete any directory the deletion leaves empty, up to but never including the specs root -- **AND** count the removals in the archive totals and complete the archive +- **AND** delete any directory the deletion leaves empty, resolving symlinks so nothing outside the real specs root is removed, and never the specs root itself +- **AND** count every operation the delta applied in the archive totals +- **AND** report the retirement as a warning, naming any sections the deleted file held besides Purpose and Requirements + +#### Scenario: Retirement is deferred until every spec is written + +- **WHEN** an archive both retires one capability and updates another +- **THEN** perform the deletion only after every spec write has succeeded, so a failure part-way leaves nothing deleted #### Scenario: Capability directory holds other files @@ -141,13 +154,13 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret #### Scenario: Removal was already synced -- **WHEN** applying a delta leaves the main spec with no requirements but removed nothing this run -- **THEN** leave the existing file untouched and complete the archive +- **WHEN** a retirable rebuilt spec removed nothing this run and its main spec exists +- **THEN** leave the file untouched and abort the archive with the validation error, as for any other unwritable spec #### Scenario: Main spec was already deleted - **WHEN** a REMOVED-only delta targets a capability that has no main spec -- **THEN** complete the archive without creating one +- **THEN** complete the archive without creating or deleting one ### Requirement: Confirmation Behavior diff --git a/openspec/specs/opsx-archive-skill/spec.md b/openspec/specs/opsx-archive-skill/spec.md index 5ebf37a88d..24f0594035 100644 --- a/openspec/specs/opsx-archive-skill/spec.md +++ b/openspec/specs/opsx-archive-skill/spec.md @@ -78,6 +78,7 @@ The skill SHALL prompt to sync delta specs before archiving if specs exist. - **AND** if user cancels, stop without archiving - **AND** if user confirms, execute `/opsx:sync` logic inline and wait for it to complete - **AND** verify every capability that has a delta spec, not only those the sync reports it touched: ADDED requirements present, MODIFIED requirements carrying the changes named in the delta, REMOVED requirements absent, RENAMED requirements present under the new name and absent under the old one +- **AND** treat a capability whose last requirement the sync removed as verified when its main spec was deleted rather than left empty - **AND** stop without archiving if the sync fails or any capability does not verify - **AND** archive only after verification passes, or when the user explicitly chose to archive without syncing or to archive already-synced specs diff --git a/openspec/specs/specs-sync-skill/spec.md b/openspec/specs/specs-sync-skill/spec.md index 1b925049e2..bc7a2e829a 100644 --- a/openspec/specs/specs-sync-skill/spec.md +++ b/openspec/specs/specs-sync-skill/spec.md @@ -48,6 +48,12 @@ The agent SHALL reconcile main specs with delta specs using the delta operation - **AND** the requirement exists in main spec - **THEN** remove the requirement from main spec +#### Scenario: REMOVED requirements retire the capability +- **WHEN** removing the requirements named in the delta leaves the main spec with no requirements at all +- **THEN** delete that capability's `spec.md`, and its directory once nothing else remains in it +- **AND** report the deletion, naming any other sections the file held +- **AND** report, rather than delete, a main spec that was already empty before this sync + #### Scenario: RENAMED requirements - **WHEN** delta contains `## RENAMED Requirements` with FROM:/TO: format - **AND** the FROM requirement exists in main spec diff --git a/skills/openspec-archive-change/SKILL.md b/skills/openspec-archive-change/SKILL.md index f4a1fd9d25..71d542bbcb 100644 --- a/skills/openspec-archive-change/SKILL.md +++ b/skills/openspec-archive-change/SKILL.md @@ -119,7 +119,7 @@ Archive a completed change in the experimental workflow. Then re-run the comparison from the top of this step against every capability that has a delta spec in `artifactPaths.specs.existingOutputPaths` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where a delta removed a capability's last requirement, its main spec deleted rather than left empty + - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and `changeRoot` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. diff --git a/skills/openspec-bulk-archive-change/SKILL.md b/skills/openspec-bulk-archive-change/SKILL.md index 8bd2ebdf6e..0cbb45be9a 100644 --- a/skills/openspec-bulk-archive-change/SKILL.md +++ b/skills/openspec-bulk-archive-change/SKILL.md @@ -190,7 +190,7 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone + - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) - RENAMED requirements present under the new name and absent under the old one - Do not verify delta specs in `excludedDeltas`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's `changeRoot` — do not archive that change. `changeRoot` remains intact. diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index dd7a99b193..84c534b44c 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -107,10 +107,13 @@ This is an **agent-driven** operation - you will read delta specs and directly e **REMOVED Requirements:** - Remove the entire requirement block from main spec - - If that leaves the main spec with no requirements at all, the capability is - retired: delete its `spec.md` (and the directory, once nothing else is left - in it) instead of saving an empty spec. A spec with zero requirements fails - `openspec validate`, which is what `openspec archive` does here too. + - If removing it *this run* leaves the main spec with no requirements at all, + the capability is retired: delete its `spec.md`, and the directory once + nothing else is left in it. An empty main spec fails `openspec validate`, + so saving one is never the right outcome. + - If the main spec was already empty before this sync - you removed nothing - + change nothing. Report the empty spec and let the user decide; do not delete + a file this change never touched. `openspec archive` draws the same line. **RENAMED Requirements:** - Find the FROM requirement, rename to TO @@ -133,6 +136,8 @@ This is an **agent-driven** operation - you will read delta specs and directly e - What changes were made (requirements added/modified/removed/renamed) - Any new main spec left with a TBD Purpose placeholder, so it gets written now rather than lingering + - Any capability retired, naming the `spec.md` you deleted and any other + sections it held, since deleting the file takes those with it **Delta Spec Format Reference** diff --git a/src/core/archive.ts b/src/core/archive.ts index c45ee58725..de5a3ad209 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -3,6 +3,7 @@ import path from 'path'; import { formatLocalDate } from '../utils/date.js'; import { getTaskProgressForChange, formatTaskStatus } from '../utils/task-progress.js'; import { Validator } from './validation/validator.js'; +import { VALIDATION_MESSAGES } from './validation/constants.js'; import chalk from 'chalk'; import { emitStoreRootBanner, @@ -39,6 +40,29 @@ function isMissingPathError(error: unknown): boolean { */ const ARCHIVE_DATE_PREFIX_PATTERN = /^\d{4}-\d{2}-\d{2}-/; +/** + * True when the ONLY thing wrong with a rebuilt spec is that it has no + * requirements. That is the exact failure retiring a capability replaces + * (#1302); anything else means the spec is broken in a way the author still has + * to fix, so archive must abort exactly as it always did instead of deleting. + * + * Asking the validator - rather than counting requirement blocks a second time - + * is what makes "this spec could not have been written anyway" true by + * construction. The two counts genuinely disagree: `MarkdownParser` accepts any + * `###` heading under `## Requirements` as a requirement, while the delta block + * parser only indexes canonical `### Requirement:` headers and sweeps the rest + * into the preamble, which survives into the rebuilt spec. + */ +async function isRetirableSpec(specName: string, rebuilt: string): Promise { + const report = await new Validator().validateSpecContent(specName, rebuilt); + if (report.valid) return false; + const errors = report.issues.filter((issue) => issue.level === 'ERROR'); + return ( + errors.length > 0 && + errors.every((issue) => issue.message === VALIDATION_MESSAGES.SPEC_NO_REQUIREMENTS) + ); +} + async function listActiveChangeNames(changesDir: string): Promise { try { const entries = await fs.readdir(changesDir, { withFileTypes: true }); @@ -469,11 +493,30 @@ 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 }; retired: boolean }> = []; + const prepared: Array<{ update: SpecUpdate; rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number }; retired: boolean; otherSections: string[] }> = []; try { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); - prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts, retired: built.retired }); + // Retirement is decided by the validator, never by a second + // opinion about what counts as a requirement: the block parser + // sweeps some shapes the validator accepts into the preamble, so + // "no blocks left" alone would delete specs that validate fine. + const retirable = + built.noRequirementBlocks && (await isRetirableSpec(update.id, built.rebuilt)); + // There are two ways to need no spec written. The spec is already + // gone, so the capability is retired and there is nothing to do; + // or it exists and this run removed its last requirement, so it + // gets deleted. A spec that is already requirement-less and lost + // nothing this run is neither: it stays the author's to fix, and + // falls through to the same abort it has always produced. + const retired = retirable && (!update.exists || built.counts.removed > 0); + prepared.push({ + update, + rebuilt: built.rebuilt, + counts: built.counts, + retired, + otherSections: built.otherSections, + }); // Carried into the result so JSON mode (where nothing was // printed) still surfaces them; human mode discards the result. specWarnings.push(...built.warnings); @@ -496,8 +539,9 @@ export class ArchiveCommand { // late validation failure really does leave all targets unchanged. if (!skipValidation) { for (const p of prepared) { - // A retired capability has no spec left to validate; the whole - // point is that the empty body could never pass (#1302). + // A retirement was already put to the validator, and failed on + // nothing but "no requirements" - there is no spec left to write, + // so re-reporting that one error would just abort the fix (#1302). if (p.retired) continue; const specName = p.update.id; const report = await new Validator().validateSpecContent(specName, p.rebuilt); @@ -525,22 +569,11 @@ export class ArchiveCommand { const writeTotals = { added: 0, modified: 0, removed: 0, renamed: 0 }; let wroteAny = false; for (const p of prepared) { + // Retirements are deferred to the end: they are the only + // irreversible step here, and the write loop is not transactional, + // so a later write that throws must not find a spec already deleted. + if (p.retired) continue; const { added, modified, removed, renamed } = p.counts; - if (p.retired) { - // Nothing was actually removed this run (the requirements were - // already gone from the baseline), so leave the file alone rather - // than deleting on the strength of a no-op delta. - if (removed === 0) continue; - const deleted = await retireSpec(p.update, mainSpecsDir, { - silent: json, - ...(isStoreSelectedRoot(root) ? { displayPath: p.update.target } : {}), - }); - if (deleted) { - wroteAny = true; - writeTotals.removed += removed; - } - continue; - } if (added + modified + removed + renamed === 0) { // Every operation was already synced: rewriting the file would // only churn normalization differences into it. @@ -557,6 +590,40 @@ export class ArchiveCommand { writeTotals.removed += removed; writeTotals.renamed += renamed; } + + for (const p of prepared) { + if (!p.retired) continue; + const deleted = await retireSpec(p.update, mainSpecsDir, { + silent: json, + ...(isStoreSelectedRoot(root) ? { displayPath: p.update.target } : {}), + }); + if (!deleted) continue; + wroteAny = true; + // A rename applied on the way to the removal still happened; folding + // every count in keeps the totals honest about the whole delta. + writeTotals.added += p.counts.added; + writeTotals.modified += p.counts.modified; + writeTotals.removed += p.counts.removed; + writeTotals.renamed += p.counts.renamed; + // Deleting a file is the one archive outcome a JSON consumer cannot + // infer from the totals, so it is recorded the way every other + // spec-merge divergence is. + const retirementNote = + `${p.update.id} - capability retired; deleted the main spec (all requirements removed).` + + (p.otherSections.length > 0 + ? ` Its other section(s) went with it: ${p.otherSections.join(', ')}.` + : ''); + specWarnings.push(retirementNote); + // The "Retiring ..." line already told a human the file is gone; the + // sections it took along are the part they cannot see. + if (!json && p.otherSections.length > 0) { + console.log( + chalk.yellow( + `⚠️ Warning: ${p.update.id} - the deleted spec also held section(s): ${p.otherSections.join(', ')}.` + ) + ); + } + } specsUpdated = wroteAny; totals = writeTotals; if (!json) { diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index df55e9e6e9..201e3ce0c6 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -90,11 +90,19 @@ export async function buildUpdatedSpec( counts: { added: number; modified: number; removed: number; renamed: number }; warnings: string[]; /** - * The delta left the capability with no requirements at all, so `rebuilt` is a - * spec body that can never validate ("Spec must have at least one - * requirement"). Callers retire the capability instead of writing it (#1302). + * Every canonical `### Requirement:` block the delta could act on is gone. + * This is only a *candidate* signal for retirement (#1302): the validator, not + * this count, decides whether `rebuilt` is actually unwritable - it recognises + * requirement shapes this parser sweeps into the preamble, so a spec can be + * blockless here and still validate. See `isRetirableSpec` in archive.ts. */ - retired: boolean; + noRequirementBlocks: boolean; + /** + * Authored `## ` sections other than Purpose and Requirements. Retirement + * deletes the whole file, so callers name these in a warning rather than + * discarding hand-written prose silently. + */ + otherSections: string[]; }> { // Collected so silent (JSON) callers can surface them; printed live for // human callers at the point they occur. @@ -449,20 +457,47 @@ export async function buildUpdatedSpec( renamed: renamedApplied, }, warnings, - // Only ADDED grows the requirement set, so an empty result means the delta - // removed the last requirement the capability had. There is no valid spec - // to write for that state - every such archive aborted before #1302. - retired: keptOrder.length === 0, + // Only ADDED grows the block set, so an empty result means the delta removed + // the last requirement block the capability had. Whether that spec is + // genuinely unwritable is the validator's call, not this one. + noRequirementBlocks: keptOrder.length === 0, + otherSections: findOtherSections(rebuilt), }; } +/** + * Authored `## ` headings other than Purpose and Requirements, outside fenced + * code. Named so a retirement can say what it is deleting along with the spec. + */ +function findOtherSections(content: string): string[] { + const lines = content.replace(/\r\n?/g, '\n').split('\n'); + const mask = buildCodeFenceMask(lines); + const sections: string[] = []; + for (let i = 0; i < lines.length; i++) { + if (mask[i]) continue; + const match = lines[i].match(/^##\s+(.+?)\s*$/); + if (!match) continue; + const title = match[1]; + if (/^(Purpose|Requirements)$/i.test(title)) continue; + sections.push(title); + } + return sections; +} + /** * Retire a capability whose last requirement a delta removed: delete its main - * spec and any directories the deletion leaves empty, up to (but never - * including) the specs root. Returns false when there was nothing to delete. + * spec and any directories the deletion leaves empty. Returns false when there + * was nothing to delete. * * Only the generated `spec.md` is removed - a directory holding anything else - * (a nested capability, a hand-kept note) is left in place. + * (a nested capability, a hand-kept note) is left in place. Unlinking `spec.md` + * follows the same path the write path would have written to, so a symlinked + * spec file loses the link and leaves its target alone. + * + * Directory pruning is bounded by REAL paths, not by string prefixes: + * `path.resolve` collapses `..` but does not resolve symlinks, and `readdir` and + * `rmdir` both follow them, so a symlinked capability directory would otherwise + * let the walk delete directories outside the specs root entirely. */ export async function retireSpec( update: SpecUpdate, @@ -476,18 +511,7 @@ export async function retireSpec( throw error; } - const specsRoot = path.resolve(mainSpecsDir); - let dir = path.dirname(path.resolve(update.target)); - while (dir !== specsRoot && dir.startsWith(specsRoot + path.sep)) { - try { - const entries = await fs.readdir(dir); - if (entries.length > 0) break; - await fs.rmdir(dir); - } catch { - break; - } - dir = path.dirname(dir); - } + await pruneEmptyDirs(path.dirname(update.target), mainSpecsDir); if (!options.silent) { console.log( @@ -497,6 +521,43 @@ export async function retireSpec( return true; } +/** Remove now-empty directories from `startDir` upward, never leaving the real specs root. */ +async function pruneEmptyDirs(startDir: string, mainSpecsDir: string): Promise { + let specsRoot: string; + try { + specsRoot = await fs.realpath(mainSpecsDir); + } catch { + return; + } + + let dir = startDir; + for (;;) { + let realDir: string; + try { + // lstat first: rmdir on a symlink fails anyway, but resolving one would + // walk us out of the tree, and the parent we then step to would be wrong. + const link = await fs.lstat(dir); + if (link.isSymbolicLink()) return; + realDir = await fs.realpath(dir); + } catch { + return; + } + + // Strictly inside the real specs root - the root itself is never pruned. + if (realDir === specsRoot || !realDir.startsWith(specsRoot + path.sep)) return; + + try { + const entries = await fs.readdir(dir); + if (entries.length > 0) return; + await fs.rmdir(dir); + } catch { + return; + } + + dir = path.dirname(dir); + } +} + function normalizeBlockRaw(raw: string): string { return raw.replace(/\r\n?/g, '\n').trim(); } diff --git a/src/core/templates/workflows/archive-change.ts b/src/core/templates/workflows/archive-change.ts index cdcee0f6a2..d1bf96c605 100644 --- a/src/core/templates/workflows/archive-change.ts +++ b/src/core/templates/workflows/archive-change.ts @@ -121,7 +121,7 @@ ${STORE_SELECTION_GUIDANCE} Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where a delta removed a capability's last requirement, its main spec deleted rather than left empty + - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. @@ -301,7 +301,7 @@ ${STORE_SELECTION_GUIDANCE} Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where a delta removed a capability's last requirement, its main spec deleted rather than left empty + - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. diff --git a/src/core/templates/workflows/bulk-archive-change.ts b/src/core/templates/workflows/bulk-archive-change.ts index 97211d39e1..896df9c87f 100644 --- a/src/core/templates/workflows/bulk-archive-change.ts +++ b/src/core/templates/workflows/bulk-archive-change.ts @@ -192,7 +192,7 @@ ${STORE_SELECTION_GUIDANCE} - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone + - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) - RENAMED requirements present under the new name and absent under the old one - Do not verify delta specs in \`excludedDeltas\`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. @@ -530,7 +530,7 @@ ${STORE_SELECTION_GUIDANCE} - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone + - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) - RENAMED requirements present under the new name and absent under the old one - Do not verify delta specs in \`excludedDeltas\`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index 6d5e94442c..c5f963f4ef 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -109,10 +109,13 @@ ${STORE_SELECTION_GUIDANCE} **REMOVED Requirements:** - Remove the entire requirement block from main spec - - If that leaves the main spec with no requirements at all, the capability is - retired: delete its \`spec.md\` (and the directory, once nothing else is left - in it) instead of saving an empty spec. A spec with zero requirements fails - \`openspec validate\`, which is what \`openspec archive\` does here too. + - If removing it *this run* leaves the main spec with no requirements at all, + the capability is retired: delete its \`spec.md\`, and the directory once + nothing else is left in it. An empty main spec fails \`openspec validate\`, + so saving one is never the right outcome. + - If the main spec was already empty before this sync - you removed nothing - + change nothing. Report the empty spec and let the user decide; do not delete + a file this change never touched. \`openspec archive\` draws the same line. **RENAMED Requirements:** - Find the FROM requirement, rename to TO @@ -135,6 +138,8 @@ ${STORE_SELECTION_GUIDANCE} - What changes were made (requirements added/modified/removed/renamed) - Any new main spec left with a TBD Purpose placeholder, so it gets written now rather than lingering + - Any capability retired, naming the \`spec.md\` you deleted and any other + sections it held, since deleting the file takes those with it **Delta Spec Format Reference** @@ -336,10 +341,13 @@ ${STORE_SELECTION_GUIDANCE} **REMOVED Requirements:** - Remove the entire requirement block from main spec - - If that leaves the main spec with no requirements at all, the capability is - retired: delete its \`spec.md\` (and the directory, once nothing else is left - in it) instead of saving an empty spec. A spec with zero requirements fails - \`openspec validate\`, which is what \`openspec archive\` does here too. + - If removing it *this run* leaves the main spec with no requirements at all, + the capability is retired: delete its \`spec.md\`, and the directory once + nothing else is left in it. An empty main spec fails \`openspec validate\`, + so saving one is never the right outcome. + - If the main spec was already empty before this sync - you removed nothing - + change nothing. Report the empty spec and let the user decide; do not delete + a file this change never touched. \`openspec archive\` draws the same line. **RENAMED Requirements:** - Find the FROM requirement, rename to TO @@ -362,6 +370,8 @@ ${STORE_SELECTION_GUIDANCE} - What changes were made (requirements added/modified/removed/renamed) - Any new main spec left with a TBD Purpose placeholder, so it gets written now rather than lingering + - Any capability retired, naming the \`spec.md\` you deleted and any other + sections it held, since deleting the file takes those with it **Delta Spec Format Reference** diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index d1a9c16a35..da4cd495a7 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { ArchiveCommand } from '../../src/core/archive.js'; +import { retireSpec } from '../../src/core/specs-apply.js'; import { Validator } from '../../src/core/validation/validator.js'; import { MarkdownParser } from '../../src/core/parsers/markdown-parser.js'; import { findMainSpecStructureIssues } from '../../src/core/parsers/spec-structure.js'; @@ -2912,19 +2913,96 @@ The system SHALL do the thing differently. ).rejects.toThrow(); }); - it('does not delete a spec when the removal was already synced', async () => { - // Nothing was removed this run, so the empty spec is the user's to fix - - // deleting on a no-op delta would destroy a file the change never touched. + // The requirement-block count and the validator do NOT agree on what a + // requirement is: MarkdownParser accepts any `###` heading under + // `## Requirements`, while the delta block parser only indexes canonical + // `### Requirement:` headers and sweeps the rest into the preamble - which + // survives into the rebuilt spec. Retiring on the block count alone deleted + // specs that validate cleanly, so the validator is the only oracle. + it('does not retire a spec that still validates without any requirement blocks', async () => { + const changeName = 'retire-preamble-heading'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const preambleRequirement = [ + '### Notes on scope', + 'The system SHALL treat the notes below as normative for the legacy layer.', + '', + '#### Scenario: Notes apply', + '- **WHEN** a reader consults the notes', + '- **THEN** the notes apply', + ].join('\n'); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + mainSpec('legacy-layer', `${preambleRequirement}\n\n${REQUIREMENT}`) + ); + + await archiveCommand.execute(changeName, { yes: true }); + + const updated = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(updated).toContain('### Notes on scope'); + expect(process.exitCode).not.toBe(1); + // The rebuilt spec is still a valid spec, so it is written, not deleted. + const report = await new Validator().validateSpecContent('legacy-layer', updated); + expect(report.valid).toBe(true); + }); + + it('aborts, exactly as before, when the removal was already synced', async () => { + // Nothing was removed this run, so this is not a retirement: the spec is + // already requirement-less and stays the author's to fix. Deleting on a + // no-op delta would destroy a file the change never touched, and archiving + // anyway would leave a main spec that `validate` rejects. const changeName = 'retire-noop'; await createChange(changeName, 'legacy-layer', REMOVE_ALL); const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); await fs.mkdir(mainSpecDir, { recursive: true }); - const emptied = `# legacy-layer Specification\n\n## Purpose\nThe legacy layer.\n\n## Requirements\n`; + const emptied = `# legacy-layer Specification\n\n## Purpose\n${PURPOSE}\n\n## Requirements\n`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), emptied); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(emptied); + // The change is still there to fix and retry. + await expect( + fs.access(path.join(tempDir, 'openspec', 'changes', changeName)) + ).resolves.not.toThrow(); + }); + + it('leaves the file alone on a no-op delta under --no-validate', async () => { + const changeName = 'retire-noop-unvalidated'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const emptied = `# legacy-layer Specification\n\n## Purpose\n${PURPOSE}\n\n## Requirements\n`; await fs.writeFile(path.join(mainSpecDir, 'spec.md'), emptied); await archiveCommand.execute(changeName, { yes: true, noValidate: true }); await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(emptied); + await expect( + fs.access(path.join(tempDir, 'openspec', 'changes', changeName)) + ).rejects.toThrow(); + }); + + it('aborts instead of retiring when the emptied spec is also broken another way', async () => { + // "No requirements" is the only error retirement replaces. A spec that is + // additionally malformed is the author's to fix, so archive must abort as + // it always did rather than delete the evidence. + const changeName = 'retire-also-broken'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + // No `## Purpose` section at all: the rebuilt spec fails on that too. + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `# legacy-layer Specification\n\n## Requirements\n\n${REQUIREMENT}\n` + ); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).resolves.not.toThrow(); }); it('still writes the spec when requirements remain after the removal', async () => { @@ -2952,6 +3030,201 @@ The system SHALL do the thing differently. expect(updated).not.toContain('legacy layer is available'); }); + it('keeps a nested capability alive under a retiring parent', async () => { + const changeName = 'retire-parent-of-nested'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + const nestedDir = path.join(mainSpecDir, 'sub'); + await fs.mkdir(nestedDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + await fs.writeFile(path.join(nestedDir, 'spec.md'), mainSpec('sub')); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).rejects.toThrow(); + await expect(fs.access(path.join(nestedDir, 'spec.md'))).resolves.not.toThrow(); + }); + + // path.resolve collapses `..` but does NOT resolve symlinks, and readdir and + // rmdir both follow them. A string-prefix bound therefore let the prune walk + // delete directories anywhere on disk through a symlinked capability path. + it.skipIf(process.platform === 'win32')( + 'never prunes directories outside the real specs root through a symlink', + async () => { + const changeName = 'retire-through-symlink'; + await createChange(changeName, 'platform/legacy-layer', REMOVE_ALL); + const outside = path.join(tempDir, 'outside', 'platform'); + const linkedCapability = path.join(outside, 'legacy-layer'); + await fs.mkdir(linkedCapability, { recursive: true }); + await fs.writeFile(path.join(linkedCapability, 'spec.md'), mainSpec('legacy-layer')); + await fs.symlink(outside, path.join(tempDir, 'openspec', 'specs', 'platform'), 'dir'); + + await archiveCommand.execute(changeName, { yes: true }); + + // The spec file itself goes, exactly where a write would have landed... + await expect(fs.access(path.join(linkedCapability, 'spec.md'))).rejects.toThrow(); + // ...but no directory outside the real specs root is removed. + await expect(fs.access(linkedCapability)).resolves.not.toThrow(); + await expect(fs.access(outside)).resolves.not.toThrow(); + } + ); + + it('does not delete anything until every spec write has succeeded', async () => { + // Retirement is the only irreversible step, and the write loop is not + // transactional, so a sibling that fails validation must leave the + // retiring spec on disk and the change unarchived. + const changeName = 'retire-with-failing-sibling'; + const changeDir = await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const badDeltaDir = path.join(changeDir, 'specs', 'other-layer'); + await fs.mkdir(badDeltaDir, { recursive: true }); + await fs.writeFile( + path.join(badDeltaDir, 'spec.md'), + // A requirement with no scenario: rebuilds fine, fails spec validation. + '# Other Layer - Changes\n\n## ADDED Requirements\n\n### Requirement: The system SHALL do a new thing\nThe system SHALL do a new thing.\n' + ); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).resolves.not.toThrow(); + await expect( + fs.access(path.join(tempDir, 'openspec', 'changes', changeName)) + ).resolves.not.toThrow(); + }); + + it('applies a retirement and an ordinary update in the same archive', async () => { + const changeName = 'retire-and-add'; + const changeDir = await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const addDeltaDir = path.join(changeDir, 'specs', 'core-layer'); + await fs.mkdir(addDeltaDir, { recursive: true }); + await fs.writeFile( + path.join(addDeltaDir, 'spec.md'), + [ + '# Core Layer - Changes', + '', + '## ADDED Requirements', + '', + '### Requirement: The system SHALL provide a core layer', + 'The system SHALL provide a core layer to every consumer.', + '', + '#### Scenario: Core is available', + '- **WHEN** a consumer imports the core', + '- **THEN** the core layer is available', + '', + ].join('\n') + ); + const legacyDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(legacyDir, { recursive: true }); + await fs.writeFile(path.join(legacyDir, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(legacyDir)).rejects.toThrow(); + await expect( + fs.access(path.join(tempDir, 'openspec', 'specs', 'core-layer', 'spec.md')) + ).resolves.not.toThrow(); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Totals: + 1, ~ 0, - 1, → 0') + ); + }); + + it('counts a rename applied on the way to the removal', async () => { + const changeName = 'retire-after-rename'; + await createChange( + changeName, + 'legacy-layer', + [ + '# Legacy Layer - Changes', + '', + '## RENAMED Requirements', + '', + '- FROM: `### Requirement: The system SHALL serve old clients`', + '- TO: `### Requirement: The system SHALL provide a legacy layer`', + '', + '## REMOVED Requirements', + '', + '### Requirement: The system SHALL provide a legacy layer', + '**Reason**: The capability is retired.', + '**Migration**: None.', + '', + ].join('\n') + ); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + mainSpec( + 'legacy-layer', + [ + '### Requirement: The system SHALL serve old clients', + 'The system SHALL serve old clients over the v1 endpoint.', + '', + '#### Scenario: Old client calls v1', + '- **WHEN** an old client calls v1', + '- **THEN** the response is served', + ].join('\n') + ) + ); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(mainSpecDir)).rejects.toThrow(); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Totals: + 0, ~ 0, - 1, → 1') + ); + }); + + it('names the sections a retirement deletes along with the spec', async () => { + const changeName = 'retire-with-sections'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `${mainSpec('legacy-layer')}\n## Why These Decisions\nThe v1 endpoint predates the routing layer.\n` + ); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('the deleted spec also held section(s): Why These Decisions') + ); + }); + + it('deletes nothing when the user declines the spec update', async () => { + const { confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(false); + const changeName = 'retire-declined'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const original = mainSpec('legacy-layer'); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), original); + + await archiveCommand.execute(changeName, {}); + + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(original); + }); + + it('reports nothing to delete when the spec vanished before the write', async () => { + // Guards the `if (deleted)` branch: a racing deletion must not be counted + // as a retirement this run. + const update = { + id: 'legacy-layer', + source: path.join(tempDir, 'nope', 'spec.md'), + target: path.join(tempDir, 'openspec', 'specs', 'gone', 'spec.md'), + exists: false, + }; + + await expect( + retireSpec(update, path.join(tempDir, 'openspec', 'specs')) + ).resolves.toBe(false); + expect(console.log).not.toHaveBeenCalledWith(expect.stringContaining('Retiring')); + }); + it('reports the retirement in --json instead of printing progress lines', async () => { const changeName = 'retire-json'; await createChange(changeName, 'legacy-layer', REMOVE_ALL); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index a2615f671d..6ccaebb8c5 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,20 +42,20 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: '676e7472977d2b6f4d922ce384db1f15020c195f94d6cd4ee71abcf0201e28a9', getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', - getSyncSpecsSkillTemplate: 'ac3ca79f4b4ab298d1776cb936fa059aa644dd29c3cf82b876e5e852f41455dc', + getSyncSpecsSkillTemplate: 'e44563eac8af7fdb094f5a67d0a3eb57009a8e9bfa39b2f7ee3302983c0ad944', getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', getOpsxContinueCommandTemplate: 'bcf0ad1c55b71346147c5b4dbaed016c77c9718f960012d8efc9d3d2089d0e00', getOpsxApplyCommandTemplate: '18c82fc48e65084065171e44f811db8fdc96bd6cb0f61fe8f31324207f4861c7', getOpsxFfCommandTemplate: '678375642a21d255444f0ba717e659abb2cc2b7474981d52eae900a0793e3e4d', - getArchiveChangeSkillTemplate: 'cb89425e37acd6a200d5a53d6609a630cb1243c9c9ce06ec1f751072bdb7073d', - getBulkArchiveChangeSkillTemplate: 'de198c7b7c1472773b013b9af917de27773fd613083309f0e8e607c005c92d3d', - getOpsxSyncCommandTemplate: 'e8af3985a831c25c9c46c9969d11ee1f303ae1cc0030d066302a67ec9602fb0a', + getArchiveChangeSkillTemplate: '4fcadc813e9b16dfef40b8b1204e9a6c4314fdc6173d6014ee4a7340c4f7b774', + getBulkArchiveChangeSkillTemplate: '67d86844d10cfbb40a3155d3cfb8f5f63f98891ef9a42312e079df255fd7a358', + getOpsxSyncCommandTemplate: 'e846eb02aacd9951add38e7b0bfb3dd0eddad99ce97c258425485e5df5bebf20', getVerifyChangeSkillTemplate: '917de96cc8341799107b0617979cdaf30e121c51676272f5caef143b090583f9', - getOpsxArchiveCommandTemplate: '737fc53fe612e439014897bc070481226e9f9de9cb05fe4f18c61cb302e190e9', + getOpsxArchiveCommandTemplate: '699912daa4f722f9b0de2af423649bc795d116565f6edf897f28640308d8e807', getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', - getOpsxBulkArchiveCommandTemplate: '93355fb7bc13e549e8646e4dc48db6f98ac5372545dff3cf3970c4f45f55c5f7', + getOpsxBulkArchiveCommandTemplate: '36c48354cf8640adda2642521e20f41dce38beb62227cee1c28ec85ef99b1e0c', getOpsxVerifyCommandTemplate: '29e3913c93566e689971d8c15c3348ba4169ebf6b1d403f5ac9974605c734baa', getOpsxProposeSkillTemplate: '06a8f7d272db8d3cb113dc05d606630d1e5aedd267c2722e971d1175e0d8bb40', getOpsxProposeCommandTemplate: 'ed3ad596d9bb238830b4fcbe566e3c1ba9d0db62f4a92cdb28c38262dc3f04df', @@ -70,9 +70,9 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '2e1a7d17ec021949d115c72227729609bf9980ad1f23445af117c09834711121', 'openspec-apply-change': '49fc5772404e3033085384ee214c44488c93880a596a9a05dcad42f9ce86cf83', 'openspec-ff-change': '4228d75e3571097164f2360e2ad3063a5b88d44750078c3601b23a89e74c1de6', - 'openspec-sync-specs': 'cc9dfa449d791f1c18dc68f19b816421b38f1bb227ceaaf9dd139670e5df08c0', - 'openspec-archive-change': '55b01bf4a605c1535ba32dc4e4f671e22743b9071ac21e636dee4316ef144580', - 'openspec-bulk-archive-change': '5ac320e2004e453c78541233f48e5f6e246cc674a44f1e427cecb7b2e9587f9b', + 'openspec-sync-specs': 'f5eadc57a35219153eb270c89c7c7175ea826bce0d75cc94c71e1b49ce86d13a', + 'openspec-archive-change': '90ee49e029901a4105e6083dabbdcac4038b2eab99657c6b765fdfda23760128', + 'openspec-bulk-archive-change': 'a452812ce3a9eb807a90c797225e2c381ef87905cc8c7e22454d9d822a1e4d96', 'openspec-verify-change': '1c3f73a36be691a18d3acb200d22e6874004d6d4a5d3e2e346ae95a7379e9da8', 'openspec-onboard': 'f2440f59c22b1ac9db33247b23a6fa32fb9cd418dc196486a213f5d7e91b1dbc', 'openspec-propose': '6b49634d3672e7fef4750a8c7572a661fec0dafe6d52a0075b41a2c87a793871', From 4503f6b73c3bf1b39688fd980426f05975932f16 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 12:11:53 -0500 Subject: [PATCH 03/40] fix(archive): close the retirement gaps a second review round found Five adversarial reviews, mutation testing and CodeRabbit went at the reworked retirement. The findings, all verified by repro before fixing: - The archive-name collision check ran AFTER the spec merge, so archiving twice in one day deleted the capability's spec and then failed, leaving the change unarchived and the file gone. The destination depends only on the change name, so it is now settled before any spec is written or deleted - which also closes the same, older window for ordinary writes. - `--no-validate` retired too, but the whole safety argument is the validator's verdict, and that path produces none. It now writes the spec exactly as it did before this feature existed, leaving no exception to the claim that nothing previously working changes. - The validator can be talked out of seeing a requirement: a stray `### Requirements` under Purpose captures its section lookup, so a spec still holding a real requirement reported "no requirements" and was deleted. Any `###` heading left under `## Requirements` now vetoes retirement outright - a reader is not fooled by the stray heading even when the parser is. - A dangling symlink made `update.exists` false (`fs.access` follows links, `unlink` does not), skipping the "removed something this run" guard: a run that removed nothing deleted an entry and reported a removal. The no-target case is now an explicit branch that never deletes, instead of an ENOENT probe. - `findOtherSections` reported `## ` headings that were inside HTML comments and listed duplicates; it now masks comments like every other structural scan here and dedupes. The warning also names the `## Purpose`, which the deletion always takes, and the resolved path when a symlink puts the file outside the repo. - A failed `unlink` surfaced a bare errno; it now says what was being attempted and what to do. Tests grew from 19 to 33, killing every surviving mutant the review found: deferral proven against a failing write (not just a failing validation), the warnings payload, the already-gone path's output, multi-level pruning, the `+ path.sep` boundary, a symlinked specs root, two retirements in one archive, and `isRetirableSpec` unit-tested directly - including the two-error shape that proves `every` rather than `some`. Agent guidance, the three living specs and the docs now state the same conditions the CLI applies, so a sync agent cannot delete a spec archive keeps. Closes #1302 Co-Authored-By: Claude Opus 5 (1M context) --- ...retire-capability-on-removed-only-delta.md | 2 +- docs/agent-contract.md | 2 +- openspec/specs/cli-archive/spec.md | 18 +- openspec/specs/opsx-archive-skill/spec.md | 2 +- openspec/specs/specs-sync-skill/spec.md | 10 +- skills/openspec-archive-change/SKILL.md | 2 +- skills/openspec-bulk-archive-change/SKILL.md | 2 +- skills/openspec-sync-specs/SKILL.md | 19 +- src/core/archive.ts | 106 ++++-- src/core/specs-apply.ts | 113 ++++-- .../templates/workflows/archive-change.ts | 4 +- .../workflows/bulk-archive-change.ts | 4 +- src/core/templates/workflows/sync-specs.ts | 38 +- test/core/archive.test.ts | 350 +++++++++++++++++- .../templates/skill-templates-parity.test.ts | 18 +- 15 files changed, 561 insertions(+), 129 deletions(-) diff --git a/.changeset/retire-capability-on-removed-only-delta.md b/.changeset/retire-capability-on-removed-only-delta.md index 363ed3fe60..2b06f3c534 100644 --- a/.changeset/retire-capability-on-removed-only-delta.md +++ b/.changeset/retire-capability-on-removed-only-delta.md @@ -2,4 +2,4 @@ "@fission-ai/openspec": patch --- -Retire a capability when a change removes its last requirement, instead of aborting the archive with "Spec must have at least one requirement". +Retire a capability when a change removes its last requirement: `openspec archive` now deletes that capability's main spec instead of aborting with "Spec must have at least one requirement". This is the first case where archiving deletes a file under `openspec/specs/`; it happens only when the emptied spec could not have been written at all, and every retirement is named in the archive output. diff --git a/docs/agent-contract.md b/docs/agent-contract.md index bd1dd14e66..7945823cf8 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -72,7 +72,7 @@ Change: `{ "id", "title", "deltaCount", "deltas": [...], "root" }`. Spec: `{ "id Success: `{ "change": { "id", "path", "metadataPath", "schema" }, "root" }`. Failure: `{ "change": null, "status": [d] }`, exit 1. ### 4.9 `archive --json` -Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"?, "warnings"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. `specsUpdated` is true only when at least one spec file was written or retired (a capability whose last requirement the change removed has its spec deleted, reported in `warnings`); an already-synced change archives with all-zero totals and the skips listed in `warnings`. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. +Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"?, "warnings"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. `specsUpdated` is true only when at least one spec file was written or retired (a capability whose last requirement the change removed has its spec deleted, and every retirement is named in `warnings`); an already-synced change archives with all-zero totals and the skips listed in `warnings`. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. ### 4.10 `doctor --json` `{ "root": { "path", "source", "store_id"?, "healthy", "status": [] }, "store": { "id", "metadata": {present,valid,remote?}, "origin_url"?, "drift"?: {ahead,behind}, "status": [] } | null, "references": [...], "status": [] }`. `drift` (present only for a git-backed store checkout that has an upstream tracking ref) is ahead/behind counts against the last-fetched upstream, not the live remote. Health findings of any severity exit 0. Failure payload: `{ "root": null, "store": null, "references": [], "status": [d] }`, exit 1. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 445d3d8710..3d98d22d4f 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -128,10 +128,16 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret #### Scenario: Deciding that a rebuilt spec cannot be written -- **WHEN** applying a delta leaves the rebuilt spec with no requirement blocks +- **WHEN** applying a delta leaves the rebuilt spec with no requirement blocks and no other `###` heading under `## Requirements` - **THEN** put that rebuilt spec to the spec validator - **AND** treat it as retirable only when its sole validation error is that the spec has no requirements -- **AND** otherwise write or reject it exactly as any other rebuilt spec, so a spec the validator still accepts is never deleted +- **AND** otherwise write or reject it exactly as any other rebuilt spec, so a spec the validator still accepts, one broken in some further way, and one still holding a `###` heading are all left alone + +#### Scenario: Validation was skipped + +- **WHEN** the archive runs with validation disabled +- **THEN** retire nothing, because no verdict was produced to justify a deletion +- **AND** write the rebuilt spec exactly as an archive without this behavior would #### Scenario: Delta removes the capability's last requirement @@ -140,12 +146,13 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret - **THEN** delete the capability's `spec.md` instead of writing it - **AND** delete any directory the deletion leaves empty, resolving symlinks so nothing outside the real specs root is removed, and never the specs root itself - **AND** count every operation the delta applied in the archive totals -- **AND** report the retirement as a warning, naming any sections the deleted file held besides Purpose and Requirements +- **AND** record the retirement in the archive warnings, naming the sections the deleted file held, and the resolved path when a symlink placed it elsewhere #### Scenario: Retirement is deferred until every spec is written - **WHEN** an archive both retires one capability and updates another -- **THEN** perform the deletion only after every spec write has succeeded, so a failure part-way leaves nothing deleted +- **THEN** settle the archive destination before touching any spec, so a name collision cannot strand a deletion +- **AND** perform the deletion only after every spec write has succeeded #### Scenario: Capability directory holds other files @@ -155,7 +162,8 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret #### Scenario: Removal was already synced - **WHEN** a retirable rebuilt spec removed nothing this run and its main spec exists -- **THEN** leave the file untouched and abort the archive with the validation error, as for any other unwritable spec +- **THEN** leave the file untouched +- **AND** abort the archive with the validation error, as for any other unwritable spec, unless validation was skipped #### Scenario: Main spec was already deleted diff --git a/openspec/specs/opsx-archive-skill/spec.md b/openspec/specs/opsx-archive-skill/spec.md index 24f0594035..2c76461e54 100644 --- a/openspec/specs/opsx-archive-skill/spec.md +++ b/openspec/specs/opsx-archive-skill/spec.md @@ -78,7 +78,7 @@ The skill SHALL prompt to sync delta specs before archiving if specs exist. - **AND** if user cancels, stop without archiving - **AND** if user confirms, execute `/opsx:sync` logic inline and wait for it to complete - **AND** verify every capability that has a delta spec, not only those the sync reports it touched: ADDED requirements present, MODIFIED requirements carrying the changes named in the delta, REMOVED requirements absent, RENAMED requirements present under the new name and absent under the old one -- **AND** treat a capability whose last requirement the sync removed as verified when its main spec was deleted rather than left empty +- **AND** treat a capability whose last requirement the sync removed as verified when its main spec was deleted rather than left empty, and a spec the sync deliberately kept and reported as verified too - **AND** stop without archiving if the sync fails or any capability does not verify - **AND** archive only after verification passes, or when the user explicitly chose to archive without syncing or to archive already-synced specs diff --git a/openspec/specs/specs-sync-skill/spec.md b/openspec/specs/specs-sync-skill/spec.md index bc7a2e829a..5551192085 100644 --- a/openspec/specs/specs-sync-skill/spec.md +++ b/openspec/specs/specs-sync-skill/spec.md @@ -49,10 +49,14 @@ The agent SHALL reconcile main specs with delta specs using the delta operation - **THEN** remove the requirement from main spec #### Scenario: REMOVED requirements retire the capability -- **WHEN** removing the requirements named in the delta leaves the main spec with no requirements at all +- **WHEN** removing the requirements named in the delta leaves `## Requirements` completely empty, with no other `###` heading or prose under it +- **AND** the rest of the spec is well-formed and it was not already empty before this sync - **THEN** delete that capability's `spec.md`, and its directory once nothing else remains in it -- **AND** report the deletion, naming any other sections the file held -- **AND** report, rather than delete, a main spec that was already empty before this sync +- **AND** report the deletion, naming the `## Purpose` and any other sections the file held + +#### Scenario: Something is left under Requirements +- **WHEN** any of those conditions fails - content remains under `## Requirements`, the spec is malformed, or nothing was removed this run +- **THEN** keep the file and report what is left, rather than deleting it #### Scenario: RENAMED requirements - **WHEN** delta contains `## RENAMED Requirements` with FROM:/TO: format diff --git a/skills/openspec-archive-change/SKILL.md b/skills/openspec-archive-change/SKILL.md index 71d542bbcb..fac60b5f37 100644 --- a/skills/openspec-archive-change/SKILL.md +++ b/skills/openspec-archive-change/SKILL.md @@ -119,7 +119,7 @@ Archive a completed change in the experimental workflow. Then re-run the comparison from the top of this step against every capability that has a delta spec in `artifactPaths.specs.existingOutputPaths` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) + - REMOVED requirements gone — and where this sync retired a capability (removed its last requirement, leaving `## Requirements` empty), its main spec deleted rather than left empty; a spec the sync deliberately kept and reported is also a match - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and `changeRoot` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. diff --git a/skills/openspec-bulk-archive-change/SKILL.md b/skills/openspec-bulk-archive-change/SKILL.md index 0cbb45be9a..5d7289d812 100644 --- a/skills/openspec-bulk-archive-change/SKILL.md +++ b/skills/openspec-bulk-archive-change/SKILL.md @@ -190,7 +190,7 @@ This skill allows you to batch-archive changes, handling spec conflicts intellig - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) + - REMOVED requirements gone — and where this sync retired a capability (removed its last requirement, leaving `## Requirements` empty), its main spec deleted rather than left empty; a spec the sync deliberately kept and reported is also a match - RENAMED requirements present under the new name and absent under the old one - Do not verify delta specs in `excludedDeltas`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's `changeRoot` — do not archive that change. `changeRoot` remains intact. diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index 84c534b44c..ccadc9e3bc 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -107,13 +107,18 @@ This is an **agent-driven** operation - you will read delta specs and directly e **REMOVED Requirements:** - Remove the entire requirement block from main spec - - If removing it *this run* leaves the main spec with no requirements at all, - the capability is retired: delete its `spec.md`, and the directory once - nothing else is left in it. An empty main spec fails `openspec validate`, - so saving one is never the right outcome. - - If the main spec was already empty before this sync - you removed nothing - - change nothing. Report the empty spec and let the user decide; do not delete - a file this change never touched. `openspec archive` draws the same line. + - Retiring the capability. Delete the whole `spec.md` - and the directory once + nothing else is left in it - only when ALL of these hold: + 1. removing the requirements *this run* left `## Requirements` completely + empty: no requirement blocks and no other `###` headings or prose under it; + 2. the rest of the spec is well-formed (it still has a `## Purpose`); + 3. the main spec was not already empty before this sync - if you removed + nothing, change nothing. + Otherwise keep the file: report what is left and let the user decide. An + empty `## Requirements` fails `openspec validate`, so say so rather than + saving one silently. `openspec archive` draws exactly these lines. + - Deleting the file also deletes its `## Purpose` and every other section it + held. Name them when you report the retirement. **RENAMED Requirements:** - Find the FROM requirement, rename to TO diff --git a/src/core/archive.ts b/src/core/archive.ts index de5a3ad209..0d901bc614 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -53,7 +53,7 @@ const ARCHIVE_DATE_PREFIX_PATTERN = /^\d{4}-\d{2}-\d{2}-/; * parser only indexes canonical `### Requirement:` headers and sweeps the rest * into the preamble, which survives into the rebuilt spec. */ -async function isRetirableSpec(specName: string, rebuilt: string): Promise { +export async function isRetirableSpec(specName: string, rebuilt: string): Promise { const report = await new Validator().validateSpecContent(specName, rebuilt); if (report.valid) return false; const errors = report.issues.filter((issue) => issue.level === 'ERROR'); @@ -450,6 +450,32 @@ export class ArchiveCommand { } } + // Settle the archive destination BEFORE touching any spec. The name depends + // only on the change, and a collision is routine (archiving twice in a day, + // a restored change), so discovering it after the merge would leave specs + // rewritten - or a capability deleted - for an archive that never happened. + // + // Names that already carry a date prefix keep it: re-prefixing would stutter + // the name, and when the archive runs on a later day the folder would sort + // under a day on which the change did not happen (#1309). + const archiveName = ARCHIVE_DATE_PREFIX_PATTERN.test(changeName) + ? changeName + : `${formatLocalDate()}-${changeName}`; + const archivePath = path.join(archiveDir, archiveName); + + let archiveExists = false; + try { + await fs.access(archivePath); + archiveExists = true; + } catch (error: any) { + if (error.code !== 'ENOENT') { + throw error; + } + } + if (archiveExists) { + throw new ArchiveBlockedError('archive_target_exists', `Archive '${archiveName}' already exists.`); + } + // Handle spec updates unless skipSpecs flag is set let specsUpdated = false; let totals: ArchiveResult['totals']; @@ -493,7 +519,7 @@ 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 }; retired: boolean; otherSections: string[] }> = []; + const prepared: Array<{ update: SpecUpdate; rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number }; retired: boolean; deletes: boolean; otherSections: string[] }> = []; try { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); @@ -501,20 +527,35 @@ export class ArchiveCommand { // opinion about what counts as a requirement: the block parser // sweeps some shapes the validator accepts into the preamble, so // "no blocks left" alone would delete specs that validate fine. + // + // Residual `###` headings veto it outright. The validator can be + // talked out of seeing them - a stray `### Requirements` under + // Purpose captures the section lookup - but a reader cannot, and + // deleting the file would take them with it. + // + // Under --no-validate there is no verdict to lean on, so nothing + // is retired: the author opted out of the check that makes this + // safe, and the old behavior (write the spec) loses nothing. const retirable = - built.noRequirementBlocks && (await isRetirableSpec(update.id, built.rebuilt)); - // There are two ways to need no spec written. The spec is already - // gone, so the capability is retired and there is nothing to do; - // or it exists and this run removed its last requirement, so it - // gets deleted. A spec that is already requirement-less and lost - // nothing this run is neither: it stays the author's to fix, and - // falls through to the same abort it has always produced. - const retired = retirable && (!update.exists || built.counts.removed > 0); + !skipValidation && + built.noRequirementBlocks && + built.residualRequirementHeadings.length === 0 && + (await isRetirableSpec(update.id, built.rebuilt)); + // Two ways to need no spec written, and only one of them deletes. + // `deletes`: the spec is there and this run removed its last + // requirement. `!update.exists`: nothing to write and nothing to + // delete - the capability is already retired. + // Neither covers a spec that is already requirement-less and lost + // nothing this run: that stays the author's to fix and falls + // through to the same abort it has always produced. + const deletes = retirable && update.exists && built.counts.removed > 0; + const retired = deletes || (retirable && !update.exists); prepared.push({ update, rebuilt: built.rebuilt, counts: built.counts, retired, + deletes, otherSections: built.otherSections, }); // Carried into the result so JSON mode (where nothing was @@ -591,12 +632,19 @@ export class ArchiveCommand { writeTotals.renamed += renamed; } + // Deletions run only after every write has succeeded - they are the + // one irreversible step, and the write loop is not transactional. + // Deleting several capabilities is still not atomic against itself: if + // a second deletion fails the first is already done, which the thrown + // message names so the state is at least legible. for (const p of prepared) { - if (!p.retired) continue; - const deleted = await retireSpec(p.update, mainSpecsDir, { + if (!p.deletes) continue; + const { deleted, retiredPath } = await retireSpec(p.update, mainSpecsDir, { silent: json, ...(isStoreSelectedRoot(root) ? { displayPath: p.update.target } : {}), }); + // The file was gone before we got to it, so nothing was retired by + // this run and nothing should be reported as though it had been. if (!deleted) continue; wroteAny = true; // A rename applied on the way to the removal still happened; folding @@ -607,12 +655,13 @@ export class ArchiveCommand { writeTotals.renamed += p.counts.renamed; // Deleting a file is the one archive outcome a JSON consumer cannot // infer from the totals, so it is recorded the way every other - // spec-merge divergence is. + // spec-merge divergence is. Purpose is always lost with the file, so + // it is named too rather than left to the reader to work out. + const lost = ['Purpose', ...p.otherSections]; const retirementNote = - `${p.update.id} - capability retired; deleted the main spec (all requirements removed).` + - (p.otherSections.length > 0 - ? ` Its other section(s) went with it: ${p.otherSections.join(', ')}.` - : ''); + `${p.update.id} - capability retired; deleted the main spec (all requirements removed)` + + (retiredPath ? ` at ${retiredPath}` : '') + + `. Its section(s) went with it: ${lost.join(', ')}.`; specWarnings.push(retirementNote); // The "Retiring ..." line already told a human the file is gone; the // sections it took along are the part they cannot see. @@ -640,29 +689,6 @@ export class ArchiveCommand { } } - // Create archive directory with date prefix. Names that already carry - // one keep it: re-prefixing would stutter the name, and when the archive - // runs on a later day the folder would sort under a day on which the - // change did not happen (#1309). - const archiveName = ARCHIVE_DATE_PREFIX_PATTERN.test(changeName) - ? changeName - : `${formatLocalDate()}-${changeName}`; - const archivePath = path.join(archiveDir, archiveName); - - // Check if archive already exists - let archiveExists = false; - try { - await fs.access(archivePath); - archiveExists = true; - } catch (error: any) { - if (error.code !== 'ENOENT') { - throw error; - } - } - if (archiveExists) { - throw new ArchiveBlockedError('archive_target_exists', `Archive '${archiveName}' already exists.`); - } - // Create archive directory if needed await fs.mkdir(archiveDir, { recursive: true }); diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 201e3ce0c6..f9393cca69 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -97,6 +97,13 @@ export async function buildUpdatedSpec( * blockless here and still validate. See `isRetirableSpec` in archive.ts. */ noRequirementBlocks: boolean; + /** + * `###` headings still sitting in the rebuilt requirements body. A reader sees + * these as requirements whatever the parsers make of them, so their presence + * disqualifies a retirement: something is left to keep, and deleting the file + * would take it silently. + */ + residualRequirementHeadings: string[]; /** * Authored `## ` sections other than Purpose and Requirements. Retirement * deletes the whole file, so callers name these in a warning rather than @@ -461,27 +468,46 @@ export async function buildUpdatedSpec( // the last requirement block the capability had. Whether that spec is // genuinely unwritable is the validator's call, not this one. noRequirementBlocks: keptOrder.length === 0, + // Read off the rebuilt requirements body, which is the preamble alone once + // every block is gone. + residualRequirementHeadings: findHeadings(reqBody, /^###\s+(.+?)\s*$/), otherSections: findOtherSections(rebuilt), }; } /** - * Authored `## ` headings other than Purpose and Requirements, outside fenced - * code. Named so a retirement can say what it is deleting along with the spec. + * Structural headings matching `pattern`, ignoring anything inside a fenced code + * block or an HTML comment - the same two things every other structural scan in + * this file masks, because a commented-out or fenced heading is invisible to the + * spec parsers but still sits in the file (#1413). */ -function findOtherSections(content: string): string[] { - const lines = content.replace(/\r\n?/g, '\n').split('\n'); - const mask = buildCodeFenceMask(lines); - const sections: string[] = []; - for (let i = 0; i < lines.length; i++) { - if (mask[i]) continue; - const match = lines[i].match(/^##\s+(.+?)\s*$/); - if (!match) continue; - const title = match[1]; - if (/^(Purpose|Requirements)$/i.test(title)) continue; - sections.push(title); +function findHeadings(content: string, pattern: RegExp): string[] { + const normalized = content.replace(/\r\n?/g, '\n'); + // Structure is read from the masked copy; titles come from the real lines so + // an author's own wording is reported verbatim. + const lines = normalized.split('\n'); + const masked = maskHtmlComments(normalized).split('\n'); + const fenceMask = buildCodeFenceMask(masked); + const found: string[] = []; + for (let i = 0; i < masked.length; i++) { + if (fenceMask[i]) continue; + if (!pattern.test(masked[i])) continue; + const match = lines[i].match(pattern); + if (match) found.push(match[1]); } - return sections; + return found; +} + +/** + * Authored `## ` headings other than Purpose and Requirements. Named so a + * retirement can say what it is deleting along with the spec, so duplicates are + * collapsed - this list is read as prose, not as a count. + */ +function findOtherSections(content: string): string[] { + const titles = findHeadings(content, /^##\s+(.+?)\s*$/).filter( + (title) => !/^(Purpose|Requirements)$/i.test(title) + ); + return [...new Set(titles)]; } /** @@ -490,11 +516,16 @@ function findOtherSections(content: string): string[] { * was nothing to delete. * * Only the generated `spec.md` is removed - a directory holding anything else - * (a nested capability, a hand-kept note) is left in place. Unlinking `spec.md` - * follows the same path the write path would have written to, so a symlinked - * spec file loses the link and leaves its target alone. + * (a nested capability, a hand-kept note) is left in place. * - * Directory pruning is bounded by REAL paths, not by string prefixes: + * The unlink is deliberately NOT bounded to the specs root: it targets exactly + * the path a write would have written to, so a symlinked capability directory + * resolves the same way for both. That does mean a symlinked directory lets the + * unlink reach a file outside the repository, which `retiredPath` surfaces so the + * report never hides where the file really was. A symlinked `spec.md` itself + * loses the link and leaves its target alone. + * + * Directory pruning IS bounded, by REAL paths rather than string prefixes: * `path.resolve` collapses `..` but does not resolve symlinks, and `readdir` and * `rmdir` both follow them, so a symlinked capability directory would otherwise * let the walk delete directories outside the specs root entirely. @@ -503,25 +534,55 @@ export async function retireSpec( update: SpecUpdate, mainSpecsDir: string, options: { silent?: boolean; displayPath?: string } = {} -): Promise { +): Promise<{ deleted: boolean; retiredPath?: string }> { + // Resolved before the unlink, while the link still exists, so the report can + // name the file that actually goes when a symlink points out of the tree. + let realTarget: string | undefined; + try { + realTarget = await fs.realpath(update.target); + } catch { + realTarget = undefined; + } + try { await fs.unlink(update.target); } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false; - throw error; + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { deleted: false }; + // A bare errno here reads as an internal failure; say what was being + // attempted so the message is actionable on its own. + throw new Error( + `Could not retire capability '${update.id}': failed to delete ${update.target} ` + + `(${(error as Error).message}). Remove it by hand, then rerun the archive.` + ); } await pruneEmptyDirs(path.dirname(update.target), mainSpecsDir); + const nominal = options.displayPath ?? `openspec/specs/${update.id}/spec.md`; + // Only worth showing when the two differ - otherwise it is the same path twice. + const resolvedNote = + realTarget && realTarget !== path.resolve(update.target) ? ` (resolved to ${realTarget})` : ''; if (!options.silent) { - console.log( - `Retiring ${options.displayPath ?? `openspec/specs/${update.id}/spec.md`}: all requirements removed.` - ); + console.log(`Retiring ${nominal}${resolvedNote}: all requirements removed.`); } - return true; + return { deleted: true, ...(resolvedNote ? { retiredPath: realTarget } : {}) }; } -/** Remove now-empty directories from `startDir` upward, never leaving the real specs root. */ +/** + * Remove now-empty directories from `startDir` upward, never leaving the real + * specs root. + * + * The guard re-runs every iteration, so stepping to the LEXICAL parent is safe: + * a parent that is not the real one is simply re-resolved and rejected. Errors + * are swallowed and end the walk - ENOTEMPTY and ENOENT are correct outcomes (a + * file arriving mid-walk must win), and a permissions failure leaves an empty + * directory behind, which the next successful archive clears. + * + * Not race-free: an attacker who can swap an ancestor between the check and the + * `rmdir` could get an empty directory outside the root removed. Closing that + * needs fd-relative syscalls Node does not expose, and it requires local write + * access to `openspec/specs` during an archive. + */ async function pruneEmptyDirs(startDir: string, mainSpecsDir: string): Promise { let specsRoot: string; try { diff --git a/src/core/templates/workflows/archive-change.ts b/src/core/templates/workflows/archive-change.ts index d1bf96c605..8c09666b4e 100644 --- a/src/core/templates/workflows/archive-change.ts +++ b/src/core/templates/workflows/archive-change.ts @@ -121,7 +121,7 @@ ${STORE_SELECTION_GUIDANCE} Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) + - REMOVED requirements gone — and where this sync retired a capability (removed its last requirement, leaving \`## Requirements\` empty), its main spec deleted rather than left empty; a spec the sync deliberately kept and reported is also a match - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. @@ -301,7 +301,7 @@ ${STORE_SELECTION_GUIDANCE} Then re-run the comparison from the top of this step against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` — not only the ones the sync reports it touched. A successful sync leaves nothing left to apply, so each capability must now read as already synced: - ADDED requirements present - MODIFIED requirements carrying the scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) + - REMOVED requirements gone — and where this sync retired a capability (removed its last requirement, leaving \`## Requirements\` empty), its main spec deleted rather than left empty; a spec the sync deliberately kept and reported is also a match - RENAMED requirements present under the new name and absent under the old one If the sync failed, or any capability does not match, report what differs and stop — do not archive. Nothing has moved and \`changeRoot\` is intact, so the user can fix the mismatch or re-run the sync and start the archive again. diff --git a/src/core/templates/workflows/bulk-archive-change.ts b/src/core/templates/workflows/bulk-archive-change.ts index 896df9c87f..5585fa77ba 100644 --- a/src/core/templates/workflows/bulk-archive-change.ts +++ b/src/core/templates/workflows/bulk-archive-change.ts @@ -192,7 +192,7 @@ ${STORE_SELECTION_GUIDANCE} - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) + - REMOVED requirements gone — and where this sync retired a capability (removed its last requirement, leaving \`## Requirements\` empty), its main spec deleted rather than left empty; a spec the sync deliberately kept and reported is also a match - RENAMED requirements present under the new name and absent under the old one - Do not verify delta specs in \`excludedDeltas\`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. @@ -530,7 +530,7 @@ ${STORE_SELECTION_GUIDANCE} - Verify that main specs are updated: - ADDED requirements present - MODIFIED requirements carrying scenario and description changes named in the delta, with their other scenarios intact - - REMOVED requirements gone — and where this sync removed a capability's last requirement, its main spec deleted rather than left empty (a main spec that was already empty beforehand is left alone, so treat that as matching too) + - REMOVED requirements gone — and where this sync retired a capability (removed its last requirement, leaving \`## Requirements\` empty), its main spec deleted rather than left empty; a spec the sync deliberately kept and reported is also a match - RENAMED requirements present under the new name and absent under the old one - Do not verify delta specs in \`excludedDeltas\`; they are intentionally left unsynced. - If sync failed or any capability does not match verification, report what differs and fail/skip moving that change's \`changeRoot\` — do not archive that change. \`changeRoot\` remains intact. diff --git a/src/core/templates/workflows/sync-specs.ts b/src/core/templates/workflows/sync-specs.ts index c5f963f4ef..80853fbd31 100644 --- a/src/core/templates/workflows/sync-specs.ts +++ b/src/core/templates/workflows/sync-specs.ts @@ -109,13 +109,18 @@ ${STORE_SELECTION_GUIDANCE} **REMOVED Requirements:** - Remove the entire requirement block from main spec - - If removing it *this run* leaves the main spec with no requirements at all, - the capability is retired: delete its \`spec.md\`, and the directory once - nothing else is left in it. An empty main spec fails \`openspec validate\`, - so saving one is never the right outcome. - - If the main spec was already empty before this sync - you removed nothing - - change nothing. Report the empty spec and let the user decide; do not delete - a file this change never touched. \`openspec archive\` draws the same line. + - Retiring the capability. Delete the whole \`spec.md\` - and the directory once + nothing else is left in it - only when ALL of these hold: + 1. removing the requirements *this run* left \`## Requirements\` completely + empty: no requirement blocks and no other \`###\` headings or prose under it; + 2. the rest of the spec is well-formed (it still has a \`## Purpose\`); + 3. the main spec was not already empty before this sync - if you removed + nothing, change nothing. + Otherwise keep the file: report what is left and let the user decide. An + empty \`## Requirements\` fails \`openspec validate\`, so say so rather than + saving one silently. \`openspec archive\` draws exactly these lines. + - Deleting the file also deletes its \`## Purpose\` and every other section it + held. Name them when you report the retirement. **RENAMED Requirements:** - Find the FROM requirement, rename to TO @@ -341,13 +346,18 @@ ${STORE_SELECTION_GUIDANCE} **REMOVED Requirements:** - Remove the entire requirement block from main spec - - If removing it *this run* leaves the main spec with no requirements at all, - the capability is retired: delete its \`spec.md\`, and the directory once - nothing else is left in it. An empty main spec fails \`openspec validate\`, - so saving one is never the right outcome. - - If the main spec was already empty before this sync - you removed nothing - - change nothing. Report the empty spec and let the user decide; do not delete - a file this change never touched. \`openspec archive\` draws the same line. + - Retiring the capability. Delete the whole \`spec.md\` - and the directory once + nothing else is left in it - only when ALL of these hold: + 1. removing the requirements *this run* left \`## Requirements\` completely + empty: no requirement blocks and no other \`###\` headings or prose under it; + 2. the rest of the spec is well-formed (it still has a \`## Purpose\`); + 3. the main spec was not already empty before this sync - if you removed + nothing, change nothing. + Otherwise keep the file: report what is left and let the user decide. An + empty \`## Requirements\` fails \`openspec validate\`, so say so rather than + saving one silently. \`openspec archive\` draws exactly these lines. + - Deleting the file also deletes its \`## Purpose\` and every other section it + held. Name them when you report the retirement. **RENAMED Requirements:** - Find the FROM requirement, rename to TO diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index da4cd495a7..b79d87b856 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { ArchiveCommand } from '../../src/core/archive.js'; +import { ArchiveCommand, isRetirableSpec } from '../../src/core/archive.js'; import { retireSpec } from '../../src/core/specs-apply.js'; import { Validator } from '../../src/core/validation/validator.js'; import { MarkdownParser } from '../../src/core/parsers/markdown-parser.js'; @@ -2802,6 +2802,12 @@ The system SHALL do the thing differently. '', ].join('\n'); + /** The last thing printed, which in JSON mode is the one payload. */ + function lastJsonPayload(): string { + const calls = (console.log as unknown as ReturnType).mock.calls; + return String(calls[calls.length - 1][0]); + } + async function createChange( changeName: string, capability: string, @@ -2881,20 +2887,6 @@ The system SHALL do the thing differently. ); }); - it('retires rather than writing an empty spec under --no-validate', async () => { - // --no-validate was the one path that did not abort: it wrote a spec with - // zero requirements, which every later validate then rejected. - const changeName = 'retire-no-validate'; - await createChange(changeName, 'legacy-layer', REMOVE_ALL); - const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); - await fs.mkdir(mainSpecDir, { recursive: true }); - await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); - - await archiveCommand.execute(changeName, { yes: true, noValidate: true }); - - await expect(fs.access(mainSpecDir)).rejects.toThrow(); - }); - it('archives a REMOVED-only delta whose main spec was already deleted', async () => { // The issue's second dead end: pre-deleting the spec made the delta look // like a create, which landed on an empty spec and failed the same way. @@ -3221,10 +3213,336 @@ The system SHALL do the thing differently. await expect( retireSpec(update, path.join(tempDir, 'openspec', 'specs')) - ).resolves.toBe(false); + ).resolves.toEqual({ deleted: false }); expect(console.log).not.toHaveBeenCalledWith(expect.stringContaining('Retiring')); }); + // The archive destination is settled from the change name alone, so a + // collision is knowable before anything is touched. Discovering it after the + // merge deleted a spec for an archive that then never happened. + it('checks the archive destination before deleting anything', async () => { + const changeName = 'retire-colliding'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + await fs.mkdir( + path.join(tempDir, 'openspec', 'changes', 'archive', `${formatLocalDate()}-${changeName}`), + { recursive: true } + ); + + await expect(archiveCommand.execute(changeName, { yes: true })).rejects.toThrow( + /already exists/ + ); + + await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).resolves.not.toThrow(); + await expect( + fs.access(path.join(tempDir, 'openspec', 'changes', changeName)) + ).resolves.not.toThrow(); + }); + + it('keeps the retiring spec on disk when a later spec write fails', async () => { + // The validation pass runs before both loops, so only a failing WRITE + // proves deletions really are deferred to the end. + const changeName = 'retire-with-failing-write'; + const changeDir = await createChange(changeName, 'legacy-layer', REMOVE_ALL); + // `zz-` keeps the retirement first in the prepared order, so an + // undeferred deletion would land before the failing write. + const otherDelta = path.join(changeDir, 'specs', 'zz-other-layer'); + await fs.mkdir(otherDelta, { recursive: true }); + await fs.writeFile( + path.join(otherDelta, 'spec.md'), + [ + '# Other - Changes', + '', + '## ADDED Requirements', + '', + '### Requirement: The system SHALL do a new thing', + 'The system SHALL do a new thing.', + '', + '#### Scenario: It happens', + '- **WHEN** invoked', + '- **THEN** it happens', + '', + ].join('\n') + ); + const legacyDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(legacyDir, { recursive: true }); + await fs.writeFile(path.join(legacyDir, 'spec.md'), mainSpec('legacy-layer')); + // Make the second spec's write throw. + const readOnlyDir = path.join(tempDir, 'openspec', 'specs', 'zz-other-layer'); + await fs.mkdir(readOnlyDir, { recursive: true }); + await fs.chmod(readOnlyDir, 0o555); + + try { + await archiveCommand.execute(changeName, { yes: true }).catch(() => undefined); + await expect(fs.access(path.join(legacyDir, 'spec.md'))).resolves.not.toThrow(); + } finally { + await fs.chmod(readOnlyDir, 0o755); + } + }); + + it('prunes a whole chain of emptied parents, not just one level', async () => { + const changeName = 'retire-deep'; + await createChange(changeName, 'a/b/legacy-layer', REMOVE_ALL); + const deep = path.join(tempDir, 'openspec', 'specs', 'a', 'b', 'legacy-layer'); + await fs.mkdir(deep, { recursive: true }); + await fs.writeFile(path.join(deep, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(path.join(tempDir, 'openspec', 'specs', 'a'))).rejects.toThrow(); + await expect(fs.access(path.join(tempDir, 'openspec', 'specs'))).resolves.not.toThrow(); + }); + + it('never prunes a sibling directory that merely shares the specs-root prefix', async () => { + const specsRoot = path.join(tempDir, 'openspec', 'specs'); + const sibling = path.join(tempDir, 'openspec', 'specs-extra', 'legacy-layer'); + await fs.mkdir(sibling, { recursive: true }); + await fs.writeFile(path.join(sibling, 'spec.md'), mainSpec('legacy-layer')); + + await expect( + retireSpec( + { id: 'legacy-layer', source: 'x', target: path.join(sibling, 'spec.md'), exists: true }, + specsRoot, + { silent: true } + ) + ).resolves.toMatchObject({ deleted: true }); + + await expect(fs.access(sibling)).resolves.not.toThrow(); + await expect( + fs.access(path.join(tempDir, 'openspec', 'specs-extra')) + ).resolves.not.toThrow(); + }); + + it.skipIf(process.platform === 'win32')( + 'prunes even when the specs root is itself named through a symlink', + async () => { + const realRoot = path.join(tempDir, 'openspec', 'specs'); + const linkedRoot = path.join(tempDir, 'specs-link'); + await fs.symlink(realRoot, linkedRoot, 'dir'); + const capability = path.join(realRoot, 'legacy-layer'); + await fs.mkdir(capability, { recursive: true }); + await fs.writeFile(path.join(capability, 'spec.md'), mainSpec('legacy-layer')); + + await retireSpec( + { + id: 'legacy-layer', + source: 'x', + target: path.join(capability, 'spec.md'), + exists: true, + }, + linkedRoot, + { silent: true } + ); + + await expect(fs.access(capability)).rejects.toThrow(); + } + ); + + it('retires both capabilities when one archive empties two', async () => { + const changeName = 'retire-two'; + const changeDir = await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const secondDelta = path.join(changeDir, 'specs', 'second-layer'); + await fs.mkdir(secondDelta, { recursive: true }); + await fs.writeFile( + path.join(secondDelta, 'spec.md'), + REMOVE_ALL.replace('Legacy Layer', 'Second Layer') + ); + for (const capability of ['legacy-layer', 'second-layer']) { + const dir = path.join(tempDir, 'openspec', 'specs', capability); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'spec.md'), mainSpec(capability)); + } + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(path.join(tempDir, 'openspec', 'specs', 'legacy-layer'))).rejects.toThrow(); + await expect(fs.access(path.join(tempDir, 'openspec', 'specs', 'second-layer'))).rejects.toThrow(); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Totals: + 0, ~ 0, - 2, → 0') + ); + }); + + it('does not retire under --no-validate, since nothing checked the result', async () => { + // The safety argument is the validator's verdict. With validation off + // there is none, so the pre-#1302 behavior stands: write the spec. + const changeName = 'retire-unvalidated'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `${mainSpec('legacy-layer')}\n## Notes\nHand-written notes worth keeping.\n` + ); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + const written = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(written).toContain('## Notes'); + expect(written).not.toContain('### Requirement:'); + }); + + it('refuses to retire while any ### heading remains under Requirements', async () => { + // A stray `### Requirements` under Purpose captures the validator's + // section lookup, so it reports "no requirements" for a spec that plainly + // still has one. A reader is not fooled, and neither is this guard. + const changeName = 'retire-residual-heading'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + [ + '# legacy-layer Specification', + '', + '## Purpose', + PURPOSE, + '', + '### Requirements', + '(a stray sub-heading a previous author left behind)', + '', + '## Requirements', + '', + '### Legacy note', + 'The system SHALL keep the legacy note until migration completes.', + '', + '#### Scenario: Note applies', + '- **WHEN** a reader consults the note', + '- **THEN** it applies', + '', + REQUIREMENT, + '', + ].join('\n') + ); + + await archiveCommand.execute(changeName, { yes: true }); + + const survived = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(survived).toContain('### Legacy note'); + }); + + it('reports the retirement, and what it took, in the --json warnings', async () => { + const changeName = 'retire-json-warnings'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + `${mainSpec('legacy-layer')}\n## Why These Decisions\nBecause v1 predates routing.\n` + ); + + await archiveCommand.execute(changeName, { yes: true, json: true }); + + const payload = JSON.parse(lastJsonPayload()); + expect(payload.archive.warnings).toEqual( + expect.arrayContaining([ + expect.stringContaining('legacy-layer - capability retired; deleted the main spec'), + ]) + ); + // Purpose always goes with the file, so it is named alongside the rest. + expect(payload.archive.warnings.join('\n')).toContain('Purpose, Why These Decisions'); + }); + + it('claims no retirement for a spec that was already gone', async () => { + const changeName = 'retire-already-gone-json'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + + await archiveCommand.execute(changeName, { yes: true, json: true }); + + const payload = JSON.parse(lastJsonPayload()); + expect(payload.archive.specsUpdated).toBe(false); + expect(payload.archive.totals).toEqual({ added: 0, modified: 0, removed: 0, renamed: 0 }); + expect(JSON.stringify(payload.archive.warnings ?? [])).not.toContain('capability retired'); + }); + + it('does not name headings hidden in fences or HTML comments, nor repeat one', async () => { + const changeName = 'retire-masked-sections'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + [ + mainSpec('legacy-layer'), + '## Notes', + 'A sample of the format:', + '', + '```markdown', + '## Not A Real Section', + '```', + '', + '', + '', + '## Notes', + 'A second block under the same heading.', + '', + ].join('\n') + ); + + await archiveCommand.execute(changeName, { yes: true, json: true }); + + const warnings = JSON.parse(lastJsonPayload()).archive.warnings.join('\n'); + expect(warnings).toContain('Notes'); + expect(warnings).not.toContain('Not A Real Section'); + expect(warnings).not.toContain('CommentedOut'); + // Deduped: the repeated heading is named once. + expect(warnings.match(/Notes/g)).toHaveLength(1); + }); + + describe('isRetirableSpec', () => { + const REQUIREMENTLESS = `# legacy-layer Specification\n\n## Purpose\n${PURPOSE}\n\n## Requirements\n`; + + it('is false for a spec that validates', async () => { + await expect( + isRetirableSpec('legacy-layer', mainSpec('legacy-layer')) + ).resolves.toBe(false); + }); + + it('is true when the only error is that it has no requirements', async () => { + await expect(isRetirableSpec('legacy-layer', REQUIREMENTLESS)).resolves.toBe(true); + }); + + it('is false for a different single error', async () => { + // No Purpose section: a real failure, but not the one retirement replaces. + await expect( + isRetirableSpec( + 'legacy-layer', + `# legacy-layer Specification\n\n## Requirements\n\n${REQUIREMENT}\n` + ) + ).resolves.toBe(false); + }); + + it('is false when another error accompanies the missing requirements', async () => { + // A requirement stranded under a trailing section: "no requirements" + // AND "header outside the main ## Requirements section". + const stranded = [ + '# legacy-layer Specification', + '', + '## Purpose', + PURPOSE, + '', + '## Requirements', + '', + '## Appendix', + '', + REQUIREMENT, + '', + ].join('\n'); + const report = await new Validator().validateSpecContent('legacy-layer', stranded); + const errors = report.issues.filter((issue) => issue.level === 'ERROR'); + // Guards the `every` rather than `some`: this shape carries the + // no-requirements error alongside at least one other. + expect(errors.length).toBeGreaterThan(1); + expect(errors.map((issue) => issue.message)).toContain( + VALIDATION_MESSAGES.SPEC_NO_REQUIREMENTS + ); + await expect(isRetirableSpec('legacy-layer', stranded)).resolves.toBe(false); + }); + }); + it('reports the retirement in --json instead of printing progress lines', async () => { const changeName = 'retire-json'; await createChange(changeName, 'legacy-layer', REMOVE_ALL); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 6ccaebb8c5..cce2dcd07d 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,20 +42,20 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: '676e7472977d2b6f4d922ce384db1f15020c195f94d6cd4ee71abcf0201e28a9', getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', - getSyncSpecsSkillTemplate: 'e44563eac8af7fdb094f5a67d0a3eb57009a8e9bfa39b2f7ee3302983c0ad944', + getSyncSpecsSkillTemplate: 'ee8cc2853726ecd8a989e363633756da0c559f0932d906315636eee8961b9e57', getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', getOpsxContinueCommandTemplate: 'bcf0ad1c55b71346147c5b4dbaed016c77c9718f960012d8efc9d3d2089d0e00', getOpsxApplyCommandTemplate: '18c82fc48e65084065171e44f811db8fdc96bd6cb0f61fe8f31324207f4861c7', getOpsxFfCommandTemplate: '678375642a21d255444f0ba717e659abb2cc2b7474981d52eae900a0793e3e4d', - getArchiveChangeSkillTemplate: '4fcadc813e9b16dfef40b8b1204e9a6c4314fdc6173d6014ee4a7340c4f7b774', - getBulkArchiveChangeSkillTemplate: '67d86844d10cfbb40a3155d3cfb8f5f63f98891ef9a42312e079df255fd7a358', - getOpsxSyncCommandTemplate: 'e846eb02aacd9951add38e7b0bfb3dd0eddad99ce97c258425485e5df5bebf20', + getArchiveChangeSkillTemplate: 'ee27b4c15a2f13bbb0ab0ceb5f4b10fa5e19dd70128ca58dd3f482c1f2a8f97f', + getBulkArchiveChangeSkillTemplate: 'e67a6fae6553e01c9930bd08f11465a205637fec6c72726b0cfa1a735920bba4', + getOpsxSyncCommandTemplate: 'd971937d2b7a839af656da02c75960ab6942ed21912e083304c934b3684c494f', getVerifyChangeSkillTemplate: '917de96cc8341799107b0617979cdaf30e121c51676272f5caef143b090583f9', - getOpsxArchiveCommandTemplate: '699912daa4f722f9b0de2af423649bc795d116565f6edf897f28640308d8e807', + getOpsxArchiveCommandTemplate: '729fcdc9be6af7abb65f4ed3400ce6e95eef256d3660cfaeac3ef07e89144671', getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', - getOpsxBulkArchiveCommandTemplate: '36c48354cf8640adda2642521e20f41dce38beb62227cee1c28ec85ef99b1e0c', + getOpsxBulkArchiveCommandTemplate: '87a003ac49d0303a5b77dc935bcff1d830ca5434b129ba788d5d44253f814f87', getOpsxVerifyCommandTemplate: '29e3913c93566e689971d8c15c3348ba4169ebf6b1d403f5ac9974605c734baa', getOpsxProposeSkillTemplate: '06a8f7d272db8d3cb113dc05d606630d1e5aedd267c2722e971d1175e0d8bb40', getOpsxProposeCommandTemplate: 'ed3ad596d9bb238830b4fcbe566e3c1ba9d0db62f4a92cdb28c38262dc3f04df', @@ -70,9 +70,9 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '2e1a7d17ec021949d115c72227729609bf9980ad1f23445af117c09834711121', 'openspec-apply-change': '49fc5772404e3033085384ee214c44488c93880a596a9a05dcad42f9ce86cf83', 'openspec-ff-change': '4228d75e3571097164f2360e2ad3063a5b88d44750078c3601b23a89e74c1de6', - 'openspec-sync-specs': 'f5eadc57a35219153eb270c89c7c7175ea826bce0d75cc94c71e1b49ce86d13a', - 'openspec-archive-change': '90ee49e029901a4105e6083dabbdcac4038b2eab99657c6b765fdfda23760128', - 'openspec-bulk-archive-change': 'a452812ce3a9eb807a90c797225e2c381ef87905cc8c7e22454d9d822a1e4d96', + 'openspec-sync-specs': 'b25bcaf45a4490dbb8db1ba0340bda26ab90a53b56e89cdfab5861ed85e55d17', + 'openspec-archive-change': '7fde55c06ae896b5f628b00e0dddceb9f86fdbf431e3f4dcd5fc0aacac16d808', + 'openspec-bulk-archive-change': '789d60d5874eab9714aef6a0b1109af0af2f9f23f57767b4641db89849dd0fce', 'openspec-verify-change': '1c3f73a36be691a18d3acb200d22e6874004d6d4a5d3e2e346ae95a7379e9da8', 'openspec-onboard': 'f2440f59c22b1ac9db33247b23a6fa32fb9cd418dc196486a213f5d7e91b1dbc', 'openspec-propose': '6b49634d3672e7fef4750a8c7572a661fec0dafe6d52a0075b41a2c87a793871', From 72e770b8b7c0766fd4a0bc7e10ad56915356b1df Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 12:23:53 -0500 Subject: [PATCH 04/40] fix(archive): make the write-failure test platform-neutral and the path note meaningful Windows CI and CodeRabbit each caught one: - `chmod 0o555` is not a write barrier on Windows, so the test that proves deletions are deferred until every write succeeds never failed a write there: the archive completed, the spec was retired, and the assertion blew up. It now puts a directory where the second spec's file belongs, which fails the write on every platform. Verified it still kills the reordering mutant. - The "resolved to" note compared a canonicalized path against a merely resolved one, so any symlinked ancestor - the platform's own /var -> /private/var is enough - decorated an ordinary retirement with a path that says nothing. It now fires only when the spec really lived outside the specs tree, which is the fact the nominal path hides. Both directions are pinned by tests. Closes #1302 Co-Authored-By: Claude Opus 5 (1M context) --- src/core/specs-apply.ts | 21 +++++++++++-- test/core/archive.test.ts | 63 ++++++++++++++++++++++++++++++++------- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index f9393cca69..6a8a849fda 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -559,15 +559,30 @@ export async function retireSpec( await pruneEmptyDirs(path.dirname(update.target), mainSpecsDir); const nominal = options.displayPath ?? `openspec/specs/${update.id}/spec.md`; - // Only worth showing when the two differ - otherwise it is the same path twice. - const resolvedNote = - realTarget && realTarget !== path.resolve(update.target) ? ` (resolved to ${realTarget})` : ''; + // Worth showing only when the file really lived outside the specs tree, which + // is the thing the nominal path hides. Comparing the resolved target against + // the merely-resolved one would fire on any canonicalization difference - the + // platform's own `/var` -> `/private/var` link is enough - and say nothing. + const escaped = realTarget !== undefined && !(await isInsideRealDir(realTarget, mainSpecsDir)); + const resolvedNote = escaped ? ` (resolved to ${realTarget})` : ''; if (!options.silent) { console.log(`Retiring ${nominal}${resolvedNote}: all requirements removed.`); } return { deleted: true, ...(resolvedNote ? { retiredPath: realTarget } : {}) }; } +/** Whether `realPath` (already canonical) sits under the real `dir`. */ +async function isInsideRealDir(realPath: string, dir: string): Promise { + let realDir: string; + try { + realDir = await fs.realpath(dir); + } catch { + // No root to measure against: say nothing rather than claim an escape. + return true; + } + return realPath.startsWith(realDir + path.sep); +} + /** * Remove now-empty directories from `startDir` upward, never leaving the real * specs root. diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index b79d87b856..a4c8ff9758 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3269,17 +3269,19 @@ The system SHALL do the thing differently. const legacyDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); await fs.mkdir(legacyDir, { recursive: true }); await fs.writeFile(path.join(legacyDir, 'spec.md'), mainSpec('legacy-layer')); - // Make the second spec's write throw. - const readOnlyDir = path.join(tempDir, 'openspec', 'specs', 'zz-other-layer'); - await fs.mkdir(readOnlyDir, { recursive: true }); - await fs.chmod(readOnlyDir, 0o555); + // Make the second spec's write throw, by putting a directory where its + // file belongs. Read-only permissions would be a no-op on Windows; this + // fails the write on every platform. + await fs.mkdir(path.join(tempDir, 'openspec', 'specs', 'zz-other-layer', 'spec.md'), { + recursive: true, + }); - try { - await archiveCommand.execute(changeName, { yes: true }).catch(() => undefined); - await expect(fs.access(path.join(legacyDir, 'spec.md'))).resolves.not.toThrow(); - } finally { - await fs.chmod(readOnlyDir, 0o755); - } + await archiveCommand.execute(changeName, { yes: true }).catch(() => undefined); + + await expect(fs.access(path.join(legacyDir, 'spec.md'))).resolves.not.toThrow(); + await expect( + fs.access(path.join(tempDir, 'openspec', 'changes', changeName)) + ).resolves.not.toThrow(); }); it('prunes a whole chain of emptied parents, not just one level', async () => { @@ -3422,6 +3424,47 @@ The system SHALL do the thing differently. expect(survived).toContain('### Legacy note'); }); + it('does not claim a resolved path for an ordinary retirement', async () => { + // The temp root is itself reached through a symlink on macOS + // (/var -> /private/var), so comparing resolved-vs-canonical paths would + // decorate every retirement with a note that means nothing. + const changeName = 'retire-plain-path'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true, json: true }); + + const payload = JSON.parse(lastJsonPayload()); + // No " at " suffix: the nominal path told the whole story. + expect(payload.archive.warnings.join('\n')).not.toContain('removed) at '); + }); + + it.skipIf(process.platform === 'win32')( + 'names the resolved path when a symlink put the spec outside the specs tree', + async () => { + const changeName = 'retire-outside'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const outside = path.join(tempDir, 'outside', 'legacy-layer'); + await fs.mkdir(outside, { recursive: true }); + await fs.writeFile(path.join(outside, 'spec.md'), mainSpec('legacy-layer')); + await fs.symlink(outside, path.join(tempDir, 'openspec', 'specs', 'legacy-layer'), 'dir'); + + await archiveCommand.execute(changeName, { yes: true, json: true }); + + const payload = JSON.parse(lastJsonPayload()); + // The warning names where the file really was, not the nominal path. + expect(payload.archive.warnings.join('\n')).toContain( + await fs.realpath(path.join(tempDir, 'outside')) + ); + // The unlink follows the link exactly where a write would have gone... + await expect(fs.access(path.join(outside, 'spec.md'))).rejects.toThrow(); + // ...but the directory outside the tree is left alone. + await expect(fs.access(outside)).resolves.not.toThrow(); + } + ); + it('reports the retirement, and what it took, in the --json warnings', async () => { const changeName = 'retire-json-warnings'; await createChange(changeName, 'legacy-layer', REMOVE_ALL); From bdd8d1855946716f44fb0770ab878a9874740d74 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 12:54:23 -0500 Subject: [PATCH 05/40] fix(archive): make the residual-heading veto position-independent A third review round, scoped to the code the earlier rounds never saw. The veto that is supposed to stop a retirement deleting hand-written content only worked when that content sat ABOVE the first requirement. `parts.preamble` is by definition the text before the first `### Requirement:` header; anything after the last one belongs to that block's raw and is discarded with it, so the rebuilt-body scan never saw it. Identical content, different position: one aborted, the other was deleted silently. The veto now reads the original Requirements section - preamble plus every block - so position does not matter. Also: - `realpath` follows a symlinked `spec.md` but `unlink` removes the link, so the warning declared it had deleted a file outside the repo that was still there. The note is now skipped when the target is itself a symlink. - `findHeadings` masked HTML comments before code fences, so an unterminated `', + '', + '### Seven year retention', + 'The system SHALL retain audit entries for seven years.', + '', + '#### Scenario: Early purge refused', + '- **WHEN** a purge is attempted early', + '- **THEN** it is refused', + '', + ].join('\n'); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), original); + + // Valid as written, which is what made the deletion silent. + expect((await new Validator().validateSpecContent('audit', original, 'strict')).valid).toBe( + true + ); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe( + original + ); + // And the author is told why their marker was refused. + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('past the end of its `## Requirements` section') + ); + }); + it('names the marker only when retiring would really fix it', async () => { // The same two-section spec, with no marker. The hint must stay quiet: // adding the marker would not have made this spec writable. diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 4323ba3793..198ef8cb76 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -42,7 +42,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getContinueChangeSkillTemplate: '676e7472977d2b6f4d922ce384db1f15020c195f94d6cd4ee71abcf0201e28a9', getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', - getSyncSpecsSkillTemplate: '5fe7265e065ab5bc9f78017b8779394270142fb2ee9c235ef74c209804ae71d0', + getSyncSpecsSkillTemplate: '3172c8869838b467a134da19ffb1947145e54a64cea78c61945dd02ea8ec1bd9', getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', @@ -51,7 +51,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxFfCommandTemplate: '678375642a21d255444f0ba717e659abb2cc2b7474981d52eae900a0793e3e4d', getArchiveChangeSkillTemplate: 'ee27b4c15a2f13bbb0ab0ceb5f4b10fa5e19dd70128ca58dd3f482c1f2a8f97f', getBulkArchiveChangeSkillTemplate: 'e67a6fae6553e01c9930bd08f11465a205637fec6c72726b0cfa1a735920bba4', - getOpsxSyncCommandTemplate: 'f7300763cbec51112df7c3fb376e5fb96b86e574cc9e83dda37929b6967eb7c1', + getOpsxSyncCommandTemplate: '51baab5279cdd241f5e85a20db841ee7f8e7109faa5f9967fd287543bcfa4de6', getVerifyChangeSkillTemplate: '917de96cc8341799107b0617979cdaf30e121c51676272f5caef143b090583f9', getOpsxArchiveCommandTemplate: '729fcdc9be6af7abb65f4ed3400ce6e95eef256d3660cfaeac3ef07e89144671', getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', @@ -70,7 +70,7 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-continue-change': '2e1a7d17ec021949d115c72227729609bf9980ad1f23445af117c09834711121', 'openspec-apply-change': '49fc5772404e3033085384ee214c44488c93880a596a9a05dcad42f9ce86cf83', 'openspec-ff-change': '4228d75e3571097164f2360e2ad3063a5b88d44750078c3601b23a89e74c1de6', - 'openspec-sync-specs': '7580a09a9406f0b985df51d20b07808b0ce09dc976f49878c6d1fb2fcb16be1e', + 'openspec-sync-specs': '2947f7b9ce6f2e85133a4a7923fecdb6704c45522ebea495c6eb23323f9d1a4a', 'openspec-archive-change': '7fde55c06ae896b5f628b00e0dddceb9f86fdbf431e3f4dcd5fc0aacac16d808', 'openspec-bulk-archive-change': '789d60d5874eab9714aef6a0b1109af0af2f9f23f57767b4641db89849dd0fce', 'openspec-verify-change': '1c3f73a36be691a18d3acb200d22e6874004d6d4a5d3e2e346ae95a7379e9da8', From 990d6914faa8b9d98a3cb7fafa025e24c5a055a2 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 19:47:32 -0500 Subject: [PATCH 21/40] docs(archive): say the marker needs the schema key beside it `.openspec.yaml` requires `schema:`, so a file holding only `retire_capabilities: true` is not honorable metadata and the marker does nothing. The docs and the abort hint both described adding one line, which sends anyone creating that file from scratch into a dead end. The message did explain itself once you were there ("schema: Invalid input: expected string, received undefined"), but it should not need to. Pre-existing shared behavior - `skip_specs` has the same requirement - so this is wording, not a behavior change. --- .changeset/retire-capability-on-removed-only-delta.md | 2 +- docs/cli.md | 2 +- docs/writing-specs.md | 2 +- src/core/archive.ts | 3 ++- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.changeset/retire-capability-on-removed-only-delta.md b/.changeset/retire-capability-on-removed-only-delta.md index 1c37fee5f6..5321d7e6c1 100644 --- a/.changeset/retire-capability-on-removed-only-delta.md +++ b/.changeset/retire-capability-on-removed-only-delta.md @@ -2,4 +2,4 @@ "@fission-ai/openspec": minor --- -Retire a capability when a change removes its last requirement. A change that declares `retire_capabilities: true` in its `.openspec.yaml` may now be archived even when its REMOVED entries take a capability's last requirement: `openspec archive` deletes that capability's main spec instead of aborting with "Spec must have at least one requirement". Without the marker nothing changes — the archive aborts exactly as before, except the message now names the marker as the way out. Retirement happens only when the emptied spec could not have been written at all, every one is named in the archive output alongside the `git checkout` that restores it if the file was committed, and `--no-validate` never retires. One thing to know before retiring: a capability's spec is the base another change's MODIFIED block is checked against, so an in-flight change that modifies the capability you just retired will keep validating clean and then refuse to archive ("target spec does not exist; only ADDED requirements are allowed for new specs") — close or rework that change alongside the retirement. +Retire a capability when a change removes its last requirement. A change that declares `retire_capabilities: true` in its `.openspec.yaml` (alongside the `schema:` that file requires) may now be archived even when its REMOVED entries take a capability's last requirement: `openspec archive` deletes that capability's main spec instead of aborting with "Spec must have at least one requirement". Without the marker nothing changes — the archive aborts exactly as before, except the message now names the marker as the way out. Retirement happens only when the emptied spec could not have been written at all, every one is named in the archive output alongside the `git checkout` that restores it if the file was committed, and `--no-validate` never retires. One thing to know before retiring: a capability's spec is the base another change's MODIFIED block is checked against, so an in-flight change that modifies the capability you just retired will keep validating clean and then refuse to archive ("target spec does not exist; only ADDED requirements are allowed for new specs") — close or rework that change alongside the retirement. diff --git a/docs/cli.md b/docs/cli.md index a656857819..fc54661e50 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -652,7 +652,7 @@ openspec archive update-ci-config --skip-specs 1. Validates the change (unless `--no-validate`) 2. Prompts for confirmation (unless `--yes`) -3. Merges delta specs into `openspec/specs/` — a capability whose last requirement the change removes is retired, and its spec file deleted, but only when the change declares `retire_capabilities: true` +3. Merges delta specs into `openspec/specs/` — a capability whose last requirement the change removes is retired, and its spec file deleted, but only when the change's `.openspec.yaml` declares `retire_capabilities: true` next to its `schema:` 4. Moves change folder to `openspec/changes/archive/YYYY-MM-DD-/` --- diff --git a/docs/writing-specs.md b/docs/writing-specs.md index 7f95004427..cff75cdb62 100644 --- a/docs/writing-specs.md +++ b/docs/writing-specs.md @@ -56,7 +56,7 @@ A change describes its edits to the specs with three section types. Using the ri - **`## MODIFIED Requirements`** — behavior that already existed and is changing. Include the full new version; a short note on what changed helps a reviewer. - **`## REMOVED Requirements`** — behavior going away, with a line on why. -On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`. Without it the archive aborts and tells you so, and the archive output names the `git checkout` that restores the file. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. +On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`, alongside the `schema:` that file already needs. Without it the archive aborts and tells you so, and the archive output names the `git checkout` that restores the file. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. diff --git a/src/core/archive.ts b/src/core/archive.ts index 3d37af4cda..446057c85d 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -647,7 +647,8 @@ export class ArchiveCommand { const retirementHint = retirementWouldFix ? `This change removes the last requirement '${specName}' has. To retire the` + ` capability and delete its spec, add \`retire_capabilities: true\` to the` + - ` change's ${METADATA_FILENAME}, then rerun.` + + ` change's ${METADATA_FILENAME} (alongside its \`schema:\`, which that file` + + ` requires), then rerun.` + (retirementMarker.invalidReason ? ` The marker present now cannot be honored (${retirementMarker.invalidReason}).` : '') From f9401acd07799a3deb3f8532d49f6ee1a906ae52 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Wed, 29 Jul 2026 20:06:17 -0500 Subject: [PATCH 22/40] chore: merge main (#1483) and keep both archive test suites #1483 landed while this branch was in review. Three conflicts: - `archive.ts`: one import line, both sides' imports kept. - `skill-templates-parity.test.ts`: hash constants, resolved by key-union and then regenerated from the merged source, which is the only authority once two branches have edited the same template. - `archive.test.ts`: the trap this repo documents. Both branches appended a DIFFERENT describe block at the same place - `capability retirement (#1302)` here, `non-interactive prompts (#1479)` on main - so taking either side would have dropped 16 or 133 tests with a green suite. Both are kept. The conflict boundary also cut the retirement describe's last two closing braces, which `tsc --noEmit` accepted and only esbuild caught as "Unexpected end of file". Restored by brace-balance against both parents. Verified after: every one of main's 91 archive titles and 19 parity titles is present, #1483's describe still holds its 16 tests, and its own non-interactive repro still behaves as it does on main. --- test/core/archive.test.ts | 3 +++ test/core/templates/skill-templates-parity.test.ts | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 05c1782581..b1eef44c80 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -4127,6 +4127,9 @@ The system SHALL do the thing differently. const payload = JSON.parse(calls[calls.length - 1]); expect(payload.archive.specsUpdated).toBe(true); expect(payload.archive.totals).toEqual({ added: 0, modified: 0, removed: 1, renamed: 0 }); + }); + }); + describe('non-interactive prompts (#1479)', () => { // An AI agent (or any script) runs the CLI with stdin closed, so every // prompt rejects with @inquirer's "User force closed the prompt with 0 diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 14043b44d0..4c800e7f92 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -43,7 +43,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getApplyChangeSkillTemplate: '031cf8f8ffc2937fc4051651bd5e1fc6159bfd225605d8e4c3181054a4e52b38', getFfChangeSkillTemplate: '225a8eaf1b3769ac5d43e079297c5fa9cc20fc2e34fec9bb0d887c8c1fb0ea71', getSyncSpecsSkillTemplate: '3172c8869838b467a134da19ffb1947145e54a64cea78c61945dd02ea8ec1bd9', - getOnboardSkillTemplate: '31dffc7c3b8d75ffbd59ed751d6a1550b885b20ef90e12d236262127ee4021e9', + getOnboardSkillTemplate: '856b5f451f45093f8906967da29b4e0479c7c271e401eab2ef58165800a67284', getOpsxExploreCommandTemplate: 'e9674ddace813e685b0e9fe37149140a3d33d48aa20b9ba2b0963a7c49c9aea7', getOpsxNewCommandTemplate: '652adc870f16bb260d54436356132b6ee051a9ed7cc0464603fb31f4db259762', getOpsxContinueCommandTemplate: 'bcf0ad1c55b71346147c5b4dbaed016c77c9718f960012d8efc9d3d2089d0e00', @@ -54,7 +54,7 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxSyncCommandTemplate: '51baab5279cdd241f5e85a20db841ee7f8e7109faa5f9967fd287543bcfa4de6', getVerifyChangeSkillTemplate: '917de96cc8341799107b0617979cdaf30e121c51676272f5caef143b090583f9', getOpsxArchiveCommandTemplate: '729fcdc9be6af7abb65f4ed3400ce6e95eef256d3660cfaeac3ef07e89144671', - getOpsxOnboardCommandTemplate: 'e69a5aa37749727290c05b687981dd69f3b17a55514a118d088c4124c5fd8505', + getOpsxOnboardCommandTemplate: '3fda1bb6ce52cdb240d1ade84319ea44160aef79573052ce58b77eb662de98a1', getOpsxBulkArchiveCommandTemplate: '87a003ac49d0303a5b77dc935bcff1d830ca5434b129ba788d5d44253f814f87', getOpsxVerifyCommandTemplate: '29e3913c93566e689971d8c15c3348ba4169ebf6b1d403f5ac9974605c734baa', getOpsxProposeSkillTemplate: '06a8f7d272db8d3cb113dc05d606630d1e5aedd267c2722e971d1175e0d8bb40', From e0b540ea265fc3f6226321f3440b7e6710230e62 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 08:22:14 -0500 Subject: [PATCH 23/40] fix(archive): only print a recovery command that would actually run Both blockers from the last review. The recovery line offered `git checkout HEAD -- ` for every retirement, including ones where the file never lived under the directory archive was run from: a selected store, or a symlinked capability directory. Git rejects an absolute path from a different worktree however it is quoted, and an unquoted path containing a space splits when pasted - a real store path reproduced both. Those cases now say where the file was and leave recovery to the reader, rather than handing them a command that cannot work. The ordinary case still gets the command, quoted when the path needs it, via the portable quoting #1483 already established for change names. And `openspec/specs/specs-sync-skill/spec.md` still authorised deletion from the four original conditions, with no mention of the tail-heading veto the CLI gained - so the living spec permitted something the code refuses. It now carries that condition, and a parity test pins it in the generated guidance so the two cannot drift apart again. Both fixes are mutation-verified: restoring the unconditional command fails the escaped-path regression, and rewording the veto out of the template fails the guidance test. --- openspec/specs/specs-sync-skill/spec.md | 3 +- src/core/archive.ts | 48 +++++++++++++------ test/core/archive.test.ts | 25 ++++++++++ .../templates/skill-templates-parity.test.ts | 4 +- 4 files changed, 63 insertions(+), 17 deletions(-) diff --git a/openspec/specs/specs-sync-skill/spec.md b/openspec/specs/specs-sync-skill/spec.md index 716d67b9d4..5e76a0ac90 100644 --- a/openspec/specs/specs-sync-skill/spec.md +++ b/openspec/specs/specs-sync-skill/spec.md @@ -50,6 +50,7 @@ The agent SHALL reconcile main specs with delta specs using the delta operation #### Scenario: REMOVED requirements retire the capability - **WHEN** removing the requirements named in the delta leaves `## Requirements` with no requirement blocks and no other `###` heading under it +- **AND** the spec holds no `###` heading past the end of that section either, whatever put it there - a second `## Requirements` section, or a `##` line inside an HTML comment - **AND** the rest of the spec is well-formed and it was not already empty before this sync - **AND** the change declares `retire_capabilities: true` in its metadata - **THEN** delete that capability's `spec.md`, and its directory once nothing else remains in it @@ -57,7 +58,7 @@ The agent SHALL reconcile main specs with delta specs using the delta operation - **AND** leave the file in place and say the marker is missing when it is not declared #### Scenario: Something is left under Requirements -- **WHEN** any of those conditions fails - content remains under `## Requirements`, the spec is malformed, or nothing was removed this run +- **WHEN** any of those conditions fails - content remains under `## Requirements`, a `###` heading sits past the end of that section, the spec is malformed, or nothing was removed this run - **THEN** keep the file in place and report what is left, rather than deleting it #### Scenario: RENAMED requirements diff --git a/src/core/archive.ts b/src/core/archive.ts index c356568054..f59b6d6a5f 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -198,9 +198,22 @@ class ArchiveBlockedError extends Error { * has to fill in. */ function quoteChangeName(name: string): string { - if (/^[A-Za-z0-9._-]+$/.test(name)) return name; - if (!/["\\$`\r\n%!]/.test(name)) return `"${name}"`; - return ''; + return quoteForShell(name) ?? ''; +} + +/** + * Quotes an argument for a line the reader is meant to paste, or returns + * undefined when no portable spelling exists. + * + * Double quotes are the one form bash, zsh, PowerShell and cmd.exe all read the + * same way. A value holding a character that stays special INSIDE double quotes + * in any of them has no portable spelling, so callers say something else rather + * than emit a command that expands to something the reader did not intend. + */ +function quoteForShell(value: string): string | undefined { + if (/^[A-Za-z0-9._\/-]+$/.test(value)) return value; + if (!/["\\$`\r\n%!]/.test(value)) return `"${value}"`; + return undefined; } /** @@ -873,19 +886,24 @@ export class ArchiveCommand { (isStoreSelectedRoot(root) ? p.update.target : path.relative(root.path, p.update.target).split(path.sep).join('/')); - // Deliberately conditional. Whether this file is in `HEAD` is not + // A command is offered only when pasting it where archive was run + // would actually work. An absolute path here means the file did not + // live under that directory - a selected store, or a symlinked + // capability directory - and `git checkout HEAD -- ` is rejected + // from a different worktree however it is quoted, so that case gets + // guidance instead of a command that cannot run. A path with no + // portable shell spelling is handled the same way. + // + // Conditional on purpose, too: whether the file is in `HEAD` is not // something archive knows - a spec an earlier archive CREATED and - // nobody has committed yet is not, and for that one the command - // below cannot work. Promising recovery outright would be the one - // claim this feature must not get wrong, so it is phrased as the - // condition it really is. - const recovery = - `If it was committed, restore it with: git checkout HEAD -- ${deletedPath}` + - // An absolute path here means the file did not live under the - // directory archive was run from - a selected store, or a - // symlinked capability directory - so the command has to be run - // in the checkout that actually holds it. - (path.isAbsolute(deletedPath) ? ' (from the checkout that holds it)' : ''); + // nobody has committed yet is not - and promising recovery is the one + // claim this feature must not get wrong. + const pasteablePath = path.isAbsolute(deletedPath) + ? undefined + : quoteForShell(deletedPath); + const recovery = pasteablePath + ? `If it was committed, restore it with: git checkout HEAD -- ${pasteablePath}` + : `It was deleted from ${deletedPath}; if it was committed, restore it from that checkout's history.`; const retirementNote = `${p.update.id} - capability retired; deleted the main spec (all requirements removed` + `, declared by retire_capabilities) at ${deletedPath}` + diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index b1eef44c80..8defb778e9 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3151,6 +3151,31 @@ The system SHALL do the thing differently. ); }); + it.skipIf(process.platform === 'win32')( + 'gives guidance, not a broken command, when the spec lived outside the repo', + async () => { + // `git checkout HEAD -- ` is rejected from a different + // worktree however it is quoted, and an unquoted path with a space + // splits when pasted. A store-selected root and a symlinked capability + // directory both produce exactly that path, so those cases say where the + // file was instead of offering a command that cannot run. + const outside = path.join(tempDir, 'out side'); + await fs.mkdir(outside, { recursive: true }); + await fs.writeFile(path.join(outside, 'spec.md'), mainSpec('legacy-layer')); + await fs.mkdir(path.join(tempDir, 'openspec', 'specs'), { recursive: true }); + await fs.symlink(outside, path.join(tempDir, 'openspec', 'specs', 'legacy-layer'), 'dir'); + const changeName = 'retire-outside'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + + await archiveCommand.execute(changeName, { yes: true, json: true }); + + const notes = JSON.parse(lastJsonPayload()).archive.warnings.join('\n'); + expect(notes).toContain('out side/spec.md; if it was committed, restore it from'); + // No command at all, so nothing can be pasted and silently mis-run. + expect(notes).not.toContain('git checkout HEAD --'); + } + ); + it('does not promise git recovery outright, and names the real path', async () => { // Archive cannot know whether the file is in HEAD - a spec an earlier // archive created and nobody committed is not - so the recovery line is diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 4c800e7f92..15085feb96 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -731,7 +731,9 @@ describe('skill templates split parity', () => { const text = sync!.template.instructions; expect(text).toContain('retire_capabilities: true'); // And that the CLI draws the same lines, so an agent syncing by hand does - // not delete a spec the CLI would have kept. + // not delete a spec the CLI would have kept - including the tail veto, which + // the skill cannot infer and the CLI will not budge on. expect(text).toContain('no other `###` heading under it'); + expect(text).toContain('past the end of the'); }); }); From 1fead66e71eee0323638d272e65a1107362f0bd8 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 09:46:39 -0500 Subject: [PATCH 24/40] fix(archive): retire only what the merge can account for Replaces the tail-heading veto with a rule that does not read Markdown at all. Six review rounds each found a different way to dress content so a heading scan would miss it: a second `## Requirements` section, a `##` inside an HTML comment ending the section early, a three-space indent, a setext underline. Every fix was another regex approximating a parser, and every round found the next skin. `extractRequirementsSection` has already split the file into the parts this merge understands. So instead of asking "does anything here look like a requirement" - a question a regex and a renderer answer differently - the guard now asks where content ended up: anything non-blank between the `## Requirements` header and the first requirement, or after the section ends, is content the merge carried through without understanding, and a retirement that would delete the file is refused. There is no second opinion to disagree with the first, because there is no second parse. The in-block heading guard stays, and its comment now says why: a `###` heading that is not a requirement header is absorbed into the block above it, so it never reaches the preamble or the tail. Folding that into the rule above needs a parser that ends a block at any `###` heading, which belongs in the parser. This narrows the feature: a spec carrying an authored section beyond Purpose can no longer be retired automatically. That is deliberate. The abort names the lines that stood in the way, and deleting a file whose contents this merge cannot enumerate is exactly the case a person should decide. Depends on #1490 for indented requirement headers, which are swallowed by the block parser before any of this runs. Co-Authored-By: Claude Opus 5 (1M context) --- openspec/specs/cli-archive/spec.md | 8 +-- src/core/archive.ts | 32 +++++----- src/core/specs-apply.ts | 76 ++++++++++++------------ test/core/archive.test.ts | 94 +----------------------------- 4 files changed, 59 insertions(+), 151 deletions(-) diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index 77db204a66..b1bfdaaa93 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -182,11 +182,11 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret - **THEN** leave the file untouched - **AND** abort the archive with the validation error, as for any other unwritable spec, unless validation was skipped -#### Scenario: A heading past the end of the Requirements section +#### Scenario: Content the merge cannot account for -- **WHEN** the spec holds a `###` heading beyond the `## Requirements` section this merge reads, whatever put it there - a second `## Requirements` section, or a `##` line inside an HTML comment that ends the section for the merge but not for a scan that masks comments -- **THEN** refuse the retirement, because that heading is a requirement to any reader and would be deleted with the file and named nowhere -- **AND** say so when the change declared the marker, rather than aborting on the bare validation error +- **WHEN** the spec holds any non-blank content outside the parts this merge understands - between the `## Requirements` header and the first requirement, or after the section ends, whatever put it there +- **THEN** refuse the retirement, because deleting the file would take that content with it +- **AND** say which lines stood in the way when the change declared the marker, rather than aborting on the bare validation error #### Scenario: Main spec is already gone diff --git a/src/core/archive.ts b/src/core/archive.ts index f59b6d6a5f..98a51506c7 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -98,12 +98,12 @@ async function decideSpecOutcome( const retirable = !skipValidation && built.noRequirementBlocks && + // Nothing in the file this merge cannot account for. Asked as "did anything + // land outside the parts I understand" rather than "does anything look like + // a requirement" - the second question is the one six review rounds each + // found a new way to answer wrongly. + built.unaccountedContent.length === 0 && built.residualRequirementHeadings.length === 0 && - // Anything requirement-shaped past the merged section's boundary is content - // every parser here stops short of - the validator's lookup, the block - // parser, the residual-heading veto and the lost-section report all bind to - // the first section. It would be deleted with the file and named nowhere. - !built.hasUnmergedRequirementHeadings && (await isRetirableSpec(update.id, built.rebuilt)); if (!retirable) return 'write'; @@ -715,7 +715,7 @@ 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 }; outcome: SpecOutcome; otherSections: string[]; noRequirementBlocks: boolean; residualRequirementHeadings: string[]; hasUnmergedRequirementHeadings: boolean }> = []; + const prepared: Array<{ update: SpecUpdate; rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number }; outcome: SpecOutcome; otherSections: string[]; noRequirementBlocks: boolean; unaccountedContent: string[]; residualRequirementHeadings: string[] }> = []; try { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); @@ -726,8 +726,8 @@ export class ArchiveCommand { outcome: await decideSpecOutcome(update, built, skipValidation, retirementDeclared), otherSections: built.otherSections, noRequirementBlocks: built.noRequirementBlocks, + unaccountedContent: built.unaccountedContent, residualRequirementHeadings: built.residualRequirementHeadings, - hasUnmergedRequirementHeadings: built.hasUnmergedRequirementHeadings, }); // Carried into the result so JSON mode (where nothing was // printed) still surfaces them; human mode discards the result. @@ -767,8 +767,8 @@ export class ArchiveCommand { const retirementWouldFix = !retirementDeclared && p.noRequirementBlocks && + p.unaccountedContent.length === 0 && p.residualRequirementHeadings.length === 0 && - !p.hasUnmergedRequirementHeadings && p.update.exists && p.counts.removed > 0 && (await isRetirableSpec(specName, p.rebuilt)); @@ -785,17 +785,17 @@ export class ArchiveCommand { // nothing left the author who did exactly what the docs asked // back in the original dead end with no signal that their // marker had been read at all. - // Only the unmerged-tail veto can reach this abort. A residual - // heading INSIDE the section still counts as a requirement to - // the validator, so that spec is valid and simply gets written - - // there is no dead end there to explain. + // The author asked for a retirement and got the bare + // validation abort. Name the lines that stood in the way. const refusalReason = retirementDeclared && - p.hasUnmergedRequirementHeadings && + p.unaccountedContent.length > 0 && (await isRetirableSpec(specName, p.rebuilt)) - ? `'${specName}' declares retire_capabilities, but the spec holds ### heading(s) past the end of ` + - 'its `## Requirements` section, which this merge does not read. Move them into that section, ' + - 'or delete the spec by hand.' + ? `'${specName}' declares retire_capabilities, but the spec holds content outside its ` + + `requirements that deleting the file would take with it: ` + + `${p.unaccountedContent.slice(0, 3).map((line) => `"${line}"`).join(', ')}` + + `${p.unaccountedContent.length > 3 ? `, and ${p.unaccountedContent.length - 3} more line(s)` : ''}. ` + + 'Move it under `## Requirements`, or delete the spec by hand.' : undefined; if (json) { throw new ArchiveBlockedError( diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index ebaccdd29d..810a663d2d 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -94,32 +94,37 @@ export async function buildUpdatedSpec( */ noRequirementBlocks: boolean; /** - * `###` headings still sitting in the rebuilt requirements body. A reader sees - * these as requirements whatever the parsers make of them, so their presence - * disqualifies a retirement: something is left to keep, and deleting the file - * would take it along silently. + * Content the merge carried through without understanding it: anything + * non-blank sitting between the `## Requirements` header and the first + * requirement, or after the section ends. + * + * Retirement deletes the whole file, so every byte in it has to be one this + * merge can account for - the title, the `## Purpose` section, the + * `## Requirements` header, and the requirement blocks themselves. These two + * slices are the parts that are none of those. + * + * Deliberately NOT a search for requirement-shaped text. Six review rounds + * each found a different way to dress content so a heading scan would miss it: + * a second `## Requirements` section, a `##` inside an HTML comment ending the + * section early, a three-space indent, a setext underline. Every one of those + * lands here regardless of how it is spelled, because this asks where content + * ended up rather than what it looks like - and `extractRequirementsSection` + * has already drawn the boundaries, so there is no second opinion to disagree + * with the first. */ - residualRequirementHeadings: string[]; + unaccountedContent: string[]; /** - * The tail this merge copies through untouched still holds a `###` heading. + * `###` headings inside the requirements section that are not requirement + * headers. A block's `raw` runs to the next header the parser RECOGNISES, so + * one of these is absorbed into the requirement above it and would be deleted + * with the file - it never reaches the preamble or the tail, which is why + * `unaccountedContent` cannot see it. * - * `extractRequirementsSection` binds to the FIRST `## `-terminated section, so - * anything past that boundary rides through unexamined: the residual-heading - * veto above only sees the section body, `findOtherSections` reports `## ` - * titles and filters `Requirements` out entirely, and the validator's own - * section lookup stops at the first one too. A `SHALL` with a scenario down - * there therefore passes `validate --strict` and would be deleted with the - * file, named nowhere. - * - * Read with the fence-only mask, deliberately, because that is the mask - * `extractRequirementsSection` used to choose the boundary. `findHeadings` - * masks HTML comments as well, and that one-mask difference was the bug: a - * multi-line comment holding a `## ` line ends the section for the merge while - * staying invisible to a comment-masking scan, so the tail it created could - * not be seen. Whatever the boundary turned out to be, this asks the same - * question about what ended up beyond it. + * The clean version of this is a parser that ends a block at any `###` + * heading, which would fold this into the check above. That belongs in the + * parser, not here. */ - hasUnmergedRequirementHeadings: boolean; + residualRequirementHeadings: string[]; /** * Authored `## ` sections other than Purpose and Requirements. Retirement * deletes the whole file, so callers name these rather than discarding @@ -486,14 +491,21 @@ export async function buildUpdatedSpec( // is discarded with it, so a rebuilt-body scan only ever sees headings above // the first requirement - it would veto `### Notes` written before the // requirements and miss the identical heading written after them. + unaccountedContent: [parts.preamble, parts.after] + .flatMap((part) => part.split('\n')) + .map((line) => line.trim()) + .filter((line) => line.length > 0), + // Read off the ORIGINAL section body, not the rebuilt one: a heading below + // the last requirement lives in that block's raw and is discarded with it, + // so a rebuilt-body scan would only ever see headings written above the + // first requirement. residualRequirementHeadings: findHeadings( [parts.preamble, ...parts.bodyBlocks.map((block) => block.raw)] .filter((part) => part && part.trim()) .join('\n\n'), - /^###\s+(.+?)\s*$/ + /^ {0,3}###\s+(.+?)\s*$/ ).filter((title) => !/^Requirement:/i.test(title)), otherSections: findOtherSections(rebuilt), - hasUnmergedRequirementHeadings: hasHeadingsBeyondMergedSection(parts.after), }; } @@ -523,22 +535,6 @@ function findHeadings(content: string, pattern: RegExp): string[] { return found; } -/** - * Whether the untouched tail after the merged Requirements section still holds a - * `###` heading - a requirement to any reader, whatever the parsers make of it. - * - * Masks fenced blocks and nothing else, matching the mask - * `extractRequirementsSection` used to pick the boundary. Masking HTML comments - * here too would reintroduce the blind spot this exists to close: a `## ` inside - * a multi-line comment is a boundary for that function, so the tail is real even - * though a comment-masking scan cannot see what created it. - */ -function hasHeadingsBeyondMergedSection(after: string): boolean { - const lines = after.replace(/^\uFEFF/, '').replace(/\r\n?/g, '\n').split('\n'); - const fenceMask = buildCodeFenceMask(lines); - return lines.some((line, index) => !fenceMask[index] && /^###\s+/.test(line)); -} - /** * Authored `## ` headings other than Purpose and Requirements. Named so a * retirement can say what moved along with the spec, so duplicates are diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 8defb778e9..79a2427853 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3122,7 +3122,7 @@ The system SHALL do the thing differently. ); // And the author is told why their marker was refused. expect(console.log).toHaveBeenCalledWith( - expect.stringContaining('past the end of its `## Requirements` section') + expect.stringContaining('content outside its requirements that deleting the file would take with it') ); }); @@ -3584,26 +3584,6 @@ The system SHALL do the thing differently. ); }); - it('names the sections a retirement deletes along with the spec', async () => { - const changeName = 'retire-with-sections'; - await createChange(changeName, 'legacy-layer', REMOVE_ALL); - const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); - await fs.mkdir(mainSpecDir, { recursive: true }); - await fs.writeFile( - path.join(mainSpecDir, 'spec.md'), - `${mainSpec('legacy-layer')}\n## Why These Decisions\nThe v1 endpoint predates the routing layer.\n` - ); - - await archiveCommand.execute(changeName, { yes: true }); - - expect(console.log).toHaveBeenCalledWith( - expect.stringContaining('the deleted spec also held section(s): Why These Decisions') - ); - // Named, not silently dropped - and the note says how to get them back. - expect(console.log).toHaveBeenCalledWith( - expect.stringContaining('git checkout HEAD -- openspec/specs/legacy-layer/spec.md') - ); - }); it('deletes nothing when the user declines the spec update', async () => { const { confirm } = await import('@inquirer/prompts'); @@ -3948,36 +3928,6 @@ The system SHALL do the thing differently. } ); - it('still names every lost section when a fence holds an unterminated comment', async () => { - // Masking comments before fences let a `', - '', - '## Notes', - 'A second block under the same heading.', - '', - ].join('\n') - ); - - await archiveCommand.execute(changeName, { yes: true, json: true }); - - const warnings = JSON.parse(lastJsonPayload()).archive.warnings.join('\n'); - expect(warnings).toContain('Notes'); - expect(warnings).not.toContain('Not A Real Section'); - expect(warnings).not.toContain('CommentedOut'); - // Deduped: the repeated heading is named once. - expect(warnings.match(/Notes/g)).toHaveLength(1); - }); describe('isRetirableSpec', () => { const REQUIREMENTLESS = `# legacy-layer Specification\n\n## Purpose\n${PURPOSE}\n\n## Requirements\n`; From 7ef2c15e097de59b781def4eb601a67de333e122 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 10:19:48 -0500 Subject: [PATCH 25/40] fix(archive): account for the whole spec, not two slices of it Defect eight, same class as the seven before it. The guard asked where content landed, which was the right question, but it only read two of the five slices `extractRequirementsSection` produces: the preamble and the tail. Content simply moved somewhere nobody looked. Reproduced: a hand-written migration runbook and a table written below a requirement's scenarios live inside that requirement's `raw` - the block runs to the next header the parser RECOGNISES - so removing the requirement deleted them, and the report said "Its section(s) went with it: Purpose". Not silence: a false statement the reader can act on. The same hole covered anything written above the `## Requirements` section. And because the abort hint is gated on the same checks, an unmarked run RECOMMENDED adding the marker that destroys it. The audit now covers the whole file. Expected: the title, the `## Purpose` section, the `## Requirements` header, and inside each block a requirement's own parts - its header, its statement, its scenarios' bullets. Every other non-blank line is reported and refuses the retirement. That folds in the `###`-heading guard, which was a patch on this same leak using the technique the rewrite was meant to abandon. One reported shape is deliberately not a case: prose between `## Purpose` and `## Requirements` IS the Purpose body, since the section runs to the next `##`, and the warning already names Purpose as going with the file. The test says so. Both regressions fail against the two-slice version. Co-Authored-By: Claude Opus 5 (1M context) --- openspec/specs/cli-archive/spec.md | 2 +- src/core/archive.ts | 5 +- src/core/specs-apply.ts | 133 ++++++++++++++++++++--------- test/core/archive.test.ts | 63 ++++++++++++++ 4 files changed, 157 insertions(+), 46 deletions(-) diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index b1bfdaaa93..d3a7d441d9 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -184,7 +184,7 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret #### Scenario: Content the merge cannot account for -- **WHEN** the spec holds any non-blank content outside the parts this merge understands - between the `## Requirements` header and the first requirement, or after the section ends, whatever put it there +- **WHEN** the spec holds any non-blank line the merge cannot name - anywhere in the file, including above the requirements section and inside a requirement block, where content the parser did not read as a new header rides along - **THEN** refuse the retirement, because deleting the file would take that content with it - **AND** say which lines stood in the way when the change declared the marker, rather than aborting on the bare validation error diff --git a/src/core/archive.ts b/src/core/archive.ts index 98a51506c7..dc5969a621 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -103,7 +103,6 @@ async function decideSpecOutcome( // a requirement" - the second question is the one six review rounds each // found a new way to answer wrongly. built.unaccountedContent.length === 0 && - built.residualRequirementHeadings.length === 0 && (await isRetirableSpec(update.id, built.rebuilt)); if (!retirable) return 'write'; @@ -715,7 +714,7 @@ 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 }; outcome: SpecOutcome; otherSections: string[]; noRequirementBlocks: boolean; unaccountedContent: string[]; residualRequirementHeadings: string[] }> = []; + const prepared: Array<{ update: SpecUpdate; rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number }; outcome: SpecOutcome; otherSections: string[]; noRequirementBlocks: boolean; unaccountedContent: string[] }> = []; try { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); @@ -727,7 +726,6 @@ export class ArchiveCommand { otherSections: built.otherSections, noRequirementBlocks: built.noRequirementBlocks, unaccountedContent: built.unaccountedContent, - residualRequirementHeadings: built.residualRequirementHeadings, }); // Carried into the result so JSON mode (where nothing was // printed) still surfaces them; human mode discards the result. @@ -768,7 +766,6 @@ export class ArchiveCommand { !retirementDeclared && p.noRequirementBlocks && p.unaccountedContent.length === 0 && - p.residualRequirementHeadings.length === 0 && p.update.exists && p.counts.removed > 0 && (await isRetirableSpec(specName, p.rebuilt)); diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 810a663d2d..469ea98cf8 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -15,6 +15,7 @@ import { parseDeltaSpec, normalizeRequirementName, type RequirementBlock, + type RequirementsSectionParts, } from './parsers/requirement-blocks.js'; import { findMainSpecStructureIssues } from './parsers/spec-structure.js'; import { buildCodeFenceMask } from './parsers/code-fence.js'; @@ -94,37 +95,26 @@ export async function buildUpdatedSpec( */ noRequirementBlocks: boolean; /** - * Content the merge carried through without understanding it: anything - * non-blank sitting between the `## Requirements` header and the first - * requirement, or after the section ends. + * Every non-blank line of the spec this merge cannot name. * - * Retirement deletes the whole file, so every byte in it has to be one this - * merge can account for - the title, the `## Purpose` section, the - * `## Requirements` header, and the requirement blocks themselves. These two - * slices are the parts that are none of those. + * Retirement deletes the whole file, so the only safe question is whether the + * merge can account for all of it. `extractRequirementsSection` splits a spec + * into five slices, and auditing a subset is how this guard kept failing: for + * seven rounds it looked for requirement-SHAPED text and was beaten by a new + * disguise each time, and when it started asking where content landed it + * still read only the preamble and the tail - so content simply moved into a + * slice nobody checked, and authored prose sitting inside a removed block's + * raw was deleted while the report said only "Purpose" was lost. * - * Deliberately NOT a search for requirement-shaped text. Six review rounds - * each found a different way to dress content so a heading scan would miss it: - * a second `## Requirements` section, a `##` inside an HTML comment ending the - * section early, a three-space indent, a setext underline. Every one of those - * lands here regardless of how it is spelled, because this asks where content - * ended up rather than what it looks like - and `extractRequirementsSection` - * has already drawn the boundaries, so there is no second opinion to disagree - * with the first. - */ - unaccountedContent: string[]; - /** - * `###` headings inside the requirements section that are not requirement - * headers. A block's `raw` runs to the next header the parser RECOGNISES, so - * one of these is absorbed into the requirement above it and would be deleted - * with the file - it never reaches the preamble or the tail, which is why - * `unaccountedContent` cannot see it. + * So this accounts for the whole file: the title, the `## Purpose` section, + * the `## Requirements` header, and, inside each requirement block, the parts + * that make up a requirement - its header, its statement, and its scenarios' + * bullets. Every other non-blank line is reported and refuses the retirement. * - * The clean version of this is a parser that ends a block at any `###` - * heading, which would fold this into the check above. That belongs in the - * parser, not here. + * Fails safe in every direction: a line this cannot classify counts as + * unaccounted, which refuses rather than deletes. */ - residualRequirementHeadings: string[]; + unaccountedContent: string[]; /** * Authored `## ` sections other than Purpose and Requirements. Retirement * deletes the whole file, so callers name these rather than discarding @@ -491,20 +481,7 @@ export async function buildUpdatedSpec( // is discarded with it, so a rebuilt-body scan only ever sees headings above // the first requirement - it would veto `### Notes` written before the // requirements and miss the identical heading written after them. - unaccountedContent: [parts.preamble, parts.after] - .flatMap((part) => part.split('\n')) - .map((line) => line.trim()) - .filter((line) => line.length > 0), - // Read off the ORIGINAL section body, not the rebuilt one: a heading below - // the last requirement lives in that block's raw and is discarded with it, - // so a rebuilt-body scan would only ever see headings written above the - // first requirement. - residualRequirementHeadings: findHeadings( - [parts.preamble, ...parts.bodyBlocks.map((block) => block.raw)] - .filter((part) => part && part.trim()) - .join('\n\n'), - /^ {0,3}###\s+(.+?)\s*$/ - ).filter((title) => !/^Requirement:/i.test(title)), + unaccountedContent: contentTheMergeCannotName(parts), otherSections: findOtherSections(rebuilt), }; } @@ -535,6 +512,80 @@ function findHeadings(content: string, pattern: RegExp): string[] { return found; } +/** + * The non-blank lines of a spec that are not part of what a retirement is able + * to name: the title, the `## Purpose` section, the `## Requirements` header, + * and each requirement block's own header, statement and scenario bullets. + * + * Deliberately whole-file. Auditing a subset of the slices is what let authored + * prose inside a removed block, and content above the requirements section, be + * deleted unmentioned. + */ +function contentTheMergeCannotName(parts: RequirementsSectionParts): string[] { + const leftovers: string[] = []; + + // Above the requirements section: the title and the Purpose section are + // expected; anything else is authored content the deletion would take. + const beforeLines = parts.before.replace(/^\uFEFF/, '').replace(/\r\n?/g, '\n').split('\n'); + const beforeMask = buildCodeFenceMask(beforeLines); + let inPurpose = false; + let titleSeen = false; + for (let index = 0; index < beforeLines.length; index++) { + const line = beforeLines[index]; + if (!line.trim()) continue; + if (!beforeMask[index]) { + const section = line.match(/^ {0,3}##\s+(.+?)\s*$/); + if (section) { + inPurpose = /^purpose$/i.test(section[1].trim()); + if (!inPurpose) leftovers.push(line.trim()); + continue; + } + if (!titleSeen && !inPurpose && /^ {0,3}#\s+.+$/.test(line)) { + titleSeen = true; + continue; + } + } + if (inPurpose) continue; + leftovers.push(line.trim()); + } + + // Between the header and the first requirement, and past the section's end. + for (const slice of [parts.preamble, parts.after]) { + for (const line of slice.split('\n')) { + if (line.trim()) leftovers.push(line.trim()); + } + } + + // Inside each requirement block, everything the block parser did not treat as + // a new header rides along in `raw` - tables, fences, comments, prose written + // below the scenarios. Only a requirement's own parts are expected here. + for (const block of parts.bodyBlocks) { + const lines = block.raw.replace(/\r\n?/g, '\n').split('\n'); + const mask = buildCodeFenceMask(lines); + let seenScenario = false; + for (let index = 0; index < lines.length; index++) { + const line = lines[index]; + if (!line.trim()) continue; + if (index === 0) continue; // the `### Requirement:` header itself + if (mask[index]) { + leftovers.push(line.trim()); + continue; + } + if (/^ {0,3}####\s+Scenario:/i.test(line)) { + seenScenario = true; + continue; + } + // Bullets belong to a scenario; free prose belongs to the requirement + // statement, which sits above the first scenario. + if (/^\s*[-*]\s/.test(line)) continue; + if (!seenScenario && !/^\s*[|`<]/.test(line)) continue; + leftovers.push(line.trim()); + } + } + + return leftovers; +} + /** * Authored `## ` headings other than Purpose and Requirements. Named so a * retirement can say what moved along with the spec, so duplicates are diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 79a2427853..91397835c9 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3126,6 +3126,69 @@ The system SHALL do the thing differently. ); }); + // The guard audits the WHOLE file, not a couple of its slices. A block's + // raw carries everything the parser did not read as a new header - prose, + // tables, fences - and that content was deleted while the report said only + // "Purpose" was lost. Content above the requirements section had the same + // hole. + it.each([ + { + where: 'inside a removed block', + spec: [ + '# legacy-layer Specification', + '', + '## Purpose', + PURPOSE, + '', + '## Requirements', + '', + REQUIREMENT, + '', + 'MIGRATION RUNBOOK (authored by hand, not a heading):', + 'Step 1: rotate the customer keys before 2026-08-01.', + '', + '| host | owner |', + '| --- | --- |', + '| db-1 | payments |', + '', + ].join('\n'), + quoted: 'MIGRATION RUNBOOK', + }, + { + where: 'above the requirements section', + spec: [ + '# legacy-layer Specification', + '', + 'NOTE TO MAINTAINERS: the escrow keys live in the "legacy" vault.', + '', + '## Purpose', + PURPOSE, + '', + '## Requirements', + '', + REQUIREMENT, + '', + ].join('\n'), + quoted: 'NOTE TO MAINTAINERS', + }, + // Not a case: prose between `## Purpose` and `## Requirements` IS the + // Purpose body - the section runs to the next `##` - and the retirement + // warning already names Purpose as going with the file. + ])('refuses to retire with authored content $where', async ({ spec, quoted }) => { + const changeName = `retire-authored-${quoted.split(' ')[0].toLowerCase()}`; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), spec); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(spec); + // And the author is told which lines stood in the way. + expect(console.log).toHaveBeenCalledWith(expect.stringContaining(quoted)); + }); + it('names the marker only when retiring would really fix it', async () => { // The same two-section spec, with no marker. The hint must stay quiet: // adding the marker would not have made this spec writable. From 1b3606c88af43712a2fedd5f04c8d4c9b3f42b1d Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 11:45:00 -0500 Subject: [PATCH 26/40] 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 27/40] 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 28/40] 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 29/40] 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 870b32274c6ad495e0a37eb3d86c560fca99b5bf Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 14:23:45 -0500 Subject: [PATCH 30/40] fix(archive): scope a scenario's bullets, and stop refusing ordinary prose Defect nine, plus the over-refusal it exposed. Every bullet counted as a scenario's own, anywhere in the block. So an operational note bulleted below the last scenario - "IMPORTANT: escrow keys live in the legacy vault" - was deleted with the file, on a spec that passes `validate --strict`, and the report named only "Purpose". A scenario's bullets run unbroken beneath its header; a blank line after them ends the run, and bullets past that point are the author's own note. Measuring the guard against this repo's 36 specs then showed the opposite failure was already there: 7 of them could never be retired, almost entirely because every fenced line inside a requirement was treated as foreign. A code example inside a scenario is that requirement's own content - a `### Requirement:` inside a fence is not a heading to any reader - so fenced lines are now accounted for, as are numbered lists and a statement that opens with inline code. One ambiguity is left deliberately unresolved: a scenario whose bullets are split by a blank line reads exactly like a note bulleted below it, and no line-based rule separates them. Those specs are REFUSED, never deleted. The abort quotes the lines, and the author moves them or removes the file by hand. Refusing costs a message; the alternative costs the file. Two regressions: the bulleted note must refuse, and a requirement using a numbered list, a fenced example and an inline-code statement must still retire. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/specs-apply.ts | 40 ++++++++++++++++----- test/core/archive.test.ts | 75 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 8 deletions(-) diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 469ea98cf8..68aedde42c 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -563,22 +563,46 @@ function contentTheMergeCannotName(parts: RequirementsSectionParts): string[] { const lines = block.raw.replace(/\r\n?/g, '\n').split('\n'); const mask = buildCodeFenceMask(lines); let seenScenario = false; + // A scenario's bullets run unbroken beneath its header. A blank line after + // them ends the scenario, so bullets written past that point are a note the + // author added, not part of the scenario - and deleting the file would take + // them. Treating every bullet as a scenario's own is what let an + // operational note below the last scenario be deleted unmentioned. + let inScenarioBullets = false; + let bulletsSeen = false; for (let index = 0; index < lines.length; index++) { const line = lines[index]; - if (!line.trim()) continue; - if (index === 0) continue; // the `### Requirement:` header itself - if (mask[index]) { - leftovers.push(line.trim()); + if (!line.trim()) { + // Only a blank that follows actual bullets closes the run, so a blank + // between a scenario header and its first bullet is not a boundary. + if (bulletsSeen) inScenarioBullets = false; continue; } + if (index === 0) continue; // the `### Requirement:` header itself + // Fenced lines render as a code block inside the requirement, so they are + // its own content however they are spelled - a `### Requirement:` in an + // example is not a heading to any reader. Flagging them made a spec that + // merely documents a command unretirable. + if (mask[index]) continue; if (/^ {0,3}####\s+Scenario:/i.test(line)) { seenScenario = true; + inScenarioBullets = true; + bulletsSeen = false; + continue; + } + if (/^\s*(?:[-*]|\d+[.)])\s/.test(line)) { + if (inScenarioBullets) { + bulletsSeen = true; + continue; + } + // A bullet outside a scenario. Before the first scenario it is part of + // the requirement statement; after one it is the author's own note. + if (!seenScenario) continue; + leftovers.push(line.trim()); continue; } - // Bullets belong to a scenario; free prose belongs to the requirement - // statement, which sits above the first scenario. - if (/^\s*[-*]\s/.test(line)) continue; - if (!seenScenario && !/^\s*[|`<]/.test(line)) continue; + // Free prose above the first scenario is the requirement statement. + if (!seenScenario && !/^\s*[|<]/.test(line)) continue; leftovers.push(line.trim()); } } diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 91397835c9..f6c1ba84a3 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3189,6 +3189,81 @@ The system SHALL do the thing differently. expect(console.log).toHaveBeenCalledWith(expect.stringContaining(quoted)); }); + it('refuses to retire when a note is bulleted below the scenarios', async () => { + // Every bullet used to count as a scenario's own, so an operational note + // written under the last scenario was deleted with the file and named + // nowhere. A scenario's bullets run unbroken beneath its header; a blank + // line ends them. + const changeName = 'retire-bulleted-note'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const spec = [ + '# legacy-layer Specification', + '', + '## Purpose', + PURPOSE, + '', + '## Requirements', + '', + REQUIREMENT, + '', + '- IMPORTANT: escrow keys live in the "legacy" vault; rotate before deleting.', + '', + ].join('\n'); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), spec); + expect((await new Validator().validateSpecContent('legacy-layer', spec, 'strict')).valid).toBe( + true + ); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(spec); + expect(console.log).toHaveBeenCalledWith(expect.stringContaining('escrow keys')); + }); + + it('still retires a spec whose requirement uses lists and code examples', async () => { + // The guard must not refuse ordinary spec prose: a numbered list, a fenced + // example, and a statement opening with inline code are all a + // requirement's own content. + // + // Known limitation, deliberate: a scenario whose bullets are split by a + // blank line reads the same as a note bulleted below the scenario, and no + // line-based rule separates them. Such a spec is REFUSED, never deleted - + // the abort names the lines and the author moves them or deletes the file + // by hand. + const changeName = 'retire-rich-requirement'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile( + path.join(mainSpecDir, 'spec.md'), + [ + '# legacy-layer Specification', + '', + '## Purpose', + PURPOSE, + '', + '## Requirements', + '', + '### Requirement: The system SHALL provide a legacy layer', + '`openspec legacy` SHALL provide a legacy layer to existing consumers.', + '', + '#### Scenario: Layer is available', + '- **WHEN** a consumer runs `openspec legacy --check`', + '- **THEN** these happen in order:', + ' 1. the layer loads', + ' 2. the consumer proceeds', + '', + ].join('\n') + ); + + await archiveCommand.execute(changeName, { yes: true }); + + await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).rejects.toThrow(); + }); + it('names the marker only when retiring would really fix it', async () => { // The same two-section spec, with no marker. The hint must stay quiet: // adding the marker would not have made this spec writable. From 01ebf7471595cb69716fc4a4530e01aff99738c9 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 15:26:24 -0500 Subject: [PATCH 31/40] fix(archive): a section is not only an ATX heading Defect nine, from a deep adversarial pass, and it is the same species as the eight before it: the guard decided what a section IS by one syntax while a reader recognises three. Once `## Purpose` was seen, every later line in the pre-requirements slice was accepted as its body until the next ATX `##`. But a setext underline turns the line above it into a heading, and raw HTML says so outright - a reader sees a sibling of `## Purpose`, not more of it. So a whole authored section could sit between Purpose and Requirements, pass `validate --specs --strict`, and be deleted with the file while the report said only "Purpose". On main the same archive aborts and loses nothing. Reproduced with a `Data Migration Notes` section underlined with dashes: the capability retired, the notes gone, unnamed. Now refused, with the lines quoted. Two path defects from the same review, one fix: the reported path was rebuilt from the capability id, so on a case-insensitive filesystem it differed in case from the file actually unlinked and git rejected the printed command; and a capability directory symlinked to a sibling deleted one spec while naming another. `retireSpec` now always returns the path it unlinked, and archive reports that. Whether to print a command at all is decided against the REAL repo root, so a symlink that stays inside the repo still gets a working command and only a path that genuinely leaves it falls back to prose. Also pins `!skipValidation` in isolation. The existing --no-validate test passed for the wrong reason - its fixture was blocked by the content guard - so the conjunct itself was unpinned. Four regressions, all mutation-verified. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/archive.ts | 27 +++++++++++++--- src/core/specs-apply.ts | 34 ++++++++++++++++++-- test/core/archive.test.ts | 67 +++++++++++++++++++++++++++++++++++---- 3 files changed, 114 insertions(+), 14 deletions(-) diff --git a/src/core/archive.ts b/src/core/archive.ts index dc5969a621..e309abd54e 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -848,7 +848,7 @@ export class ArchiveCommand { // names so the state is at least legible. for (const p of prepared) { if (p.outcome !== 'retire') continue; - const { retired, sourcePath } = await retireSpec(p.update, mainSpecsDir, { + const { retired, sourcePath, resolvedPath } = await retireSpec(p.update, mainSpecsDir, { silent: json, ...(isStoreSelectedRoot(root) ? { displayPath: p.update.target } : {}), }); @@ -878,11 +878,28 @@ export class ArchiveCommand { // real directory can differ in case, and git is case-sensitive, so // an id-derived path is one git rejects. `sourcePath` is set only // when the file escaped the specs tree, so it wins when present. + // `update.target` is built from the capability id, so on a + // case-insensitive filesystem it can differ in case from the file + // that was actually unlinked - and git is case-sensitive, so the + // printed command is one git rejects. A capability directory + // symlinked to a sibling has the same problem without leaving the + // tree. `retiredPath` carries the resolved path, so it wins + // whenever it disagrees, not only when it escapes. + const unlinkedPath = resolvedPath ?? p.update.target; + // Measured against the REAL root, so the platform's own + // `/var` -> `/private/var` link does not read as an escape. A path + // that genuinely sits outside stays absolute, which is what routes + // it to prose guidance instead of a command git would reject. + const realRoot = await fs.realpath(root.path).catch(() => root.path); + const relativeToRoot = path.relative(realRoot, unlinkedPath); + const insideRoot = + relativeToRoot !== '' && + !relativeToRoot.startsWith('..') && + !path.isAbsolute(relativeToRoot); const deletedPath = - sourcePath ?? - (isStoreSelectedRoot(root) - ? p.update.target - : path.relative(root.path, p.update.target).split(path.sep).join('/')); + isStoreSelectedRoot(root) || !insideRoot + ? unlinkedPath + : relativeToRoot.split(path.sep).join('/'); // A command is offered only when pasting it where archive was run // would actually work. An absolute path here means the file did not // live under that directory - a selected store, or a symlinked diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 68aedde42c..a2db1aedab 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -530,21 +530,41 @@ function contentTheMergeCannotName(parts: RequirementsSectionParts): string[] { const beforeMask = buildCodeFenceMask(beforeLines); let inPurpose = false; let titleSeen = false; + let previousLine = ''; for (let index = 0; index < beforeLines.length; index++) { const line = beforeLines[index]; - if (!line.trim()) continue; + if (!line.trim()) { + previousLine = ''; + continue; + } if (!beforeMask[index]) { const section = line.match(/^ {0,3}##\s+(.+?)\s*$/); if (section) { inPurpose = /^purpose$/i.test(section[1].trim()); if (!inPurpose) leftovers.push(line.trim()); + previousLine = line; + continue; + } + // `##` is not the only way to open a section. A setext underline turns + // the line above it into a heading, and raw HTML says so outright - a + // reader sees a sibling of `## Purpose`, not more of its body. Treating + // everything up to the next ATX `##` as Purpose swallowed those whole and + // deleted them, reported as nothing but "Purpose". + const setext = inPurpose && previousLine.trim() && /^ {0,3}(=+|-+)\s*$/.test(line); + const htmlHeading = /^ {0,3} { +): Promise<{ retired: boolean; sourcePath?: string; resolvedPath?: string }> { // Resolved before the unlink, while the link still exists, so the report can // name the file that actually goes when a symlink points out of the tree. // A symlinked `spec.md` is excluded: `realpath` would follow it, but `unlink` @@ -693,7 +713,15 @@ export async function retireSpec( if (!options.silent) { console.log(`Retiring ${nominal}${resolvedNote}: all requirements removed.`); } - return { retired: true, ...(resolvedNote ? { sourcePath: realSource } : {}) }; + // `resolvedPath` is always the file that was actually unlinked - callers need + // it to report a path git will accept, since the nominal one is built from + // the capability id and can differ in case, or point through a symlink. + // `sourcePath` stays the narrower "this escaped the specs tree" signal. + return { + retired: true, + ...(realSource ? { resolvedPath: realSource } : {}), + ...(resolvedNote ? { sourcePath: realSource } : {}), + }; } /** Whether `realPath` (already canonical) sits under the real `dir`. */ diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index f6c1ba84a3..96eb09f4d0 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -3264,6 +3264,59 @@ The system SHALL do the thing differently. await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).rejects.toThrow(); }); + it.each([ + { what: 'a setext heading', body: ['Data Migration Notes', '--------------------', 'Export the table by hand first.'] }, + { what: 'a raw HTML heading', body: ['

Data Migration Notes

', 'Export the table by hand first.'] }, + ])('refuses to retire when $what opens a section inside Purpose', async ({ body }) => { + // `##` is not the only way to open a section. Treating everything up to + // the next ATX `##` as Purpose body swallowed these whole and deleted + // them, reported as nothing but "Purpose". + const changeName = `retire-purpose-span-${body.length}`; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const spec = [ + '# legacy-layer Specification', + '', + '## Purpose', + PURPOSE, + '', + ...body, + '', + '## Requirements', + '', + REQUIREMENT, + '', + ].join('\n'); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), spec); + expect((await new Validator().validateSpecContent('legacy-layer', spec, 'strict')).valid).toBe( + true + ); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + await expect(fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8')).resolves.toBe(spec); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Data Migration Notes') + ); + }); + + it('never retires under --no-validate, whatever else the spec holds', async () => { + // Isolates that conjunct: the spec is otherwise a clean retirement + // candidate, so only the flag can be stopping it. + const changeName = 'retire-novalidate-isolated'; + await createChange(changeName, 'legacy-layer', REMOVE_ALL); + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'legacy-layer'); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainSpec('legacy-layer')); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + // Written, not deleted. + await expect(fs.access(path.join(mainSpecDir, 'spec.md'))).resolves.not.toThrow(); + }); + it('names the marker only when retiring would really fix it', async () => { // The same two-section spec, with no marker. The hint must stay quiet: // adding the marker would not have made this spec writable. @@ -3308,9 +3361,10 @@ The system SHALL do the thing differently. await archiveCommand.execute(changeName, { yes: true, json: true }); const notes = JSON.parse(lastJsonPayload()).archive.warnings.join('\n'); - expect(notes).toContain('out side/spec.md; if it was committed, restore it from'); - // No command at all, so nothing can be pasted and silently mis-run. - expect(notes).not.toContain('git checkout HEAD --'); + // The symlink points elsewhere in the SAME repo, so a command still + // works there - and it names the path that was really unlinked, quoted, + // rather than the nominal one git would reject. + expect(notes).toContain('git checkout HEAD -- "out side/spec.md"'); } ); @@ -4004,9 +4058,10 @@ The system SHALL do the thing differently. const payload = JSON.parse(lastJsonPayload()); // The warning names where the file really was, not the nominal path. - expect(payload.archive.warnings.join('\n')).toContain( - await fs.realpath(path.join(tempDir, 'outside')) - ); + // Root-relative, because this symlink still points inside the repo: an + // absolute path is reserved for one that genuinely leaves it, and is + // what routes the message to prose instead of a command. + expect(payload.archive.warnings.join('\n')).toContain('outside/legacy-layer/spec.md'); // The unlink follows the link exactly where a write would have gone... await expect(fs.access(path.join(outside, 'spec.md'))).rejects.toThrow(); // ...but the directory outside the tree is left alone. From c1e77785a7cc08e54d702c93e77ccc9ac3afecae Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 30 Jul 2026 19:44:05 -0500 Subject: [PATCH 32/40] fix(archive): close remaining capability retirement gaps --- ...retire-capability-on-removed-only-delta.md | 2 +- docs/agent-contract.md | 2 +- docs/faq.md | 2 +- docs/writing-specs.md | 2 +- openspec/specs/cli-archive/spec.md | 7 +- openspec/specs/specs-sync-skill/spec.md | 10 +- skills/openspec-sync-specs/SKILL.md | 23 +- src/core/archive.ts | 263 +++++++++++++----- src/core/specs-apply.ts | 75 ++--- src/core/templates/workflows/sync-specs.ts | 46 ++- src/utils/change-metadata.ts | 15 +- test/core/archive.test.ts | 127 +++++++-- .../templates/skill-templates-parity.test.ts | 22 +- 13 files changed, 371 insertions(+), 225 deletions(-) diff --git a/.changeset/retire-capability-on-removed-only-delta.md b/.changeset/retire-capability-on-removed-only-delta.md index 5321d7e6c1..9eef62bbc5 100644 --- a/.changeset/retire-capability-on-removed-only-delta.md +++ b/.changeset/retire-capability-on-removed-only-delta.md @@ -2,4 +2,4 @@ "@fission-ai/openspec": minor --- -Retire a capability when a change removes its last requirement. A change that declares `retire_capabilities: true` in its `.openspec.yaml` (alongside the `schema:` that file requires) may now be archived even when its REMOVED entries take a capability's last requirement: `openspec archive` deletes that capability's main spec instead of aborting with "Spec must have at least one requirement". Without the marker nothing changes — the archive aborts exactly as before, except the message now names the marker as the way out. Retirement happens only when the emptied spec could not have been written at all, every one is named in the archive output alongside the `git checkout` that restores it if the file was committed, and `--no-validate` never retires. One thing to know before retiring: a capability's spec is the base another change's MODIFIED block is checked against, so an in-flight change that modifies the capability you just retired will keep validating clean and then refuse to archive ("target spec does not exist; only ADDED requirements are allowed for new specs") — close or rework that change alongside the retirement. +Retire a capability when a change removes its last requirement. A change that declares `retire_capabilities: true` in its `.openspec.yaml` (alongside the `schema:` that file requires) may now be archived even when its REMOVED entries take a capability's last requirement: `openspec archive` deletes that capability's main spec instead of aborting with "Spec must have at least one requirement". Without the marker nothing changes — the archive aborts exactly as before, except the message now names the marker as the way out. Retirement happens only when the emptied spec could not have been written at all, every one is named in the archive output, a pasteable `git checkout` is included when the spec lived in the caller's checkout, and `--no-validate` never retires. One thing to know before retiring: a capability's spec is the base another change's MODIFIED block is checked against, so an in-flight change that modifies the capability you just retired will keep validating clean and then refuse to archive ("target spec does not exist; only ADDED requirements are allowed for new specs") — close or rework that change alongside the retirement. diff --git a/docs/agent-contract.md b/docs/agent-contract.md index a343bfdc0c..6f1d0c0402 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -72,7 +72,7 @@ Change: `{ "id", "title", "deltaCount", "deltas": [...], "root" }`. Spec: `{ "id Success: `{ "change": { "id", "path", "metadataPath", "schema" }, "root" }`. Failure: `{ "change": null, "status": [d] }`, exit 1. ### 4.9 `archive --json` -Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"?, "warnings"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. `specsUpdated` is true only when at least one spec file was written or retired (a capability whose last requirement the change removed has its spec deleted, which requires `retire_capabilities: true` in the change's `.openspec.yaml`; every retirement is named in `warnings` with the `git checkout` that restores it); an already-synced change archives with all-zero totals and the skips listed in `warnings`. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. +Success: `{ "archive": { "change", "archivedAs": "YYYY-MM-DD-name", "path", "specsUpdated", "totals"?, "warnings"? }, "root" }`. Failure: `{ "archive": null, "root"?, "status": [d] }`, exit 1. `specsUpdated` is true only when at least one spec file was written or retired (a capability whose last requirement the change removed has its spec deleted, which requires `retire_capabilities: true` in the change's `.openspec.yaml`; every retirement is named in `warnings`, with a pasteable Git recovery command only when the spec lived in the caller's checkout); an already-synced change archives with all-zero totals and the skips listed in `warnings`. JSON mode is strictly non-interactive: every prompt point becomes an `archive_*` code. ### 4.10 `doctor --json` `{ "root": { "path", "source", "store_id"?, "healthy", "status": [] }, "store": { "id", "metadata": {present,valid,remote?}, "origin_url"?, "drift"?: {ahead,behind}, "status": [] } | null, "references": [...], "status": [] }`. `drift` (present only for a git-backed store checkout that has an upstream tracking ref) is ahead/behind counts against the last-fetched upstream, not the live remote. Health findings of any severity exit 0. Failure payload: `{ "root": null, "store": null, "references": [], "status": [d] }`, exit 1. diff --git a/docs/faq.md b/docs/faq.md index 9afd9afcf7..770479aa3e 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -108,7 +108,7 @@ A spec that describes only what's changing, using `ADDED`, `MODIFIED`, and `REMO ### Where do archived changes go? -To `openspec/changes/archive/YYYY-MM-DD-/`, with all artifacts preserved. Nothing is deleted; the change just moves out of your active list. +To `openspec/changes/archive/YYYY-MM-DD-/`, with all change artifacts preserved. The change moves out of your active list. A change that explicitly declares `retire_capabilities: true` can also delete a main capability spec when it removes that capability's final requirement. ## Configuration and customization diff --git a/docs/writing-specs.md b/docs/writing-specs.md index cff75cdb62..501c129cd1 100644 --- a/docs/writing-specs.md +++ b/docs/writing-specs.md @@ -56,7 +56,7 @@ A change describes its edits to the specs with three section types. Using the ri - **`## MODIFIED Requirements`** — behavior that already existed and is changing. Include the full new version; a short note on what changed helps a reviewer. - **`## REMOVED Requirements`** — behavior going away, with a line on why. -On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`, alongside the `schema:` that file already needs. Without it the archive aborts and tells you so, and the archive output names the `git checkout` that restores the file. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. +On archive, ADDED gets appended to the main spec, MODIFIED replaces the old version, and REMOVED is dropped from it. Remove the last requirement a capability has and you retire it: rather than leave a spec with nothing in it, archive deletes `openspec/specs//spec.md`. Because that is the one archive step that removes a file, it has to be asked for — add `retire_capabilities: true` to the change's `.openspec.yaml`, alongside the `schema:` that file already needs. Without it the archive aborts and tells you so. For a spec in the caller's checkout, the archive output also names the `git checkout` that restores a committed file; selected stores receive checkout-scoped recovery guidance instead. If you mark a real change as ADDED, you end up with two competing requirements; if you describe new behavior as MODIFIED, there's nothing to replace. When in doubt, open the current spec and see whether the requirement is already there. One more section is worth knowing about. When your delta creates a capability that doesn't exist yet, open it with `## Purpose` — a sentence or two on what the capability is for. Archive uses it as the Purpose of the main spec it creates; skip it and you get a `TBD` placeholder to fill in by hand. An existing spec already has a Purpose, so a delta's is ignored there — edit `openspec/specs//spec.md` directly to change one. diff --git a/openspec/specs/cli-archive/spec.md b/openspec/specs/cli-archive/spec.md index d3a7d441d9..3d19f1b835 100644 --- a/openspec/specs/cli-archive/spec.md +++ b/openspec/specs/cli-archive/spec.md @@ -160,9 +160,10 @@ A delta whose REMOVED entries cover every requirement a capability has SHALL ret - **AND** at least one requirement was actually removed by this run - **AND** the change declares `retire_capabilities: true` - **THEN** delete the capability's `spec.md` instead of writing it -- **AND** delete any directory the deletion leaves empty, resolving symlinks so nothing outside the real specs root is removed, and never the specs root itself +- **AND** refuse to delete when the target resolves outside the real specs root +- **AND** delete any in-root directory the deletion leaves empty, and never the specs root itself - **AND** count every operation the delta applied in the archive totals -- **AND** record the retirement in the archive warnings, naming the sections the deleted file held, the command that restores it from git, and the resolved path when a symlinked directory placed the file outside the specs tree +- **AND** record the retirement in the archive warnings, naming what the deleted file held and giving a pasteable Git recovery command only when the spec lived in the caller's checkout #### Scenario: Retirement is deferred until every spec is written @@ -334,4 +335,4 @@ The archive command SHALL validate changes before applying them to ensure data i **No overwrite**: Preserves historical archives and prevents data loss **Spec updates before archiving**: Specs in the main directory represent current reality; when a change is deployed and archived, its future state specs become the new reality and must replace the main specs **Confirmation for spec updates**: Provides visibility into what will change, prevents accidental overwrites, and ensures users understand the impact before specs are modified -**--yes flag for automation**: Allows CI/CD pipelines to archive without interactive prompts while maintaining safety by default for manual use \ No newline at end of file +**--yes flag for automation**: Allows CI/CD pipelines to archive without interactive prompts while maintaining safety by default for manual use diff --git a/openspec/specs/specs-sync-skill/spec.md b/openspec/specs/specs-sync-skill/spec.md index 5e76a0ac90..619de70f9d 100644 --- a/openspec/specs/specs-sync-skill/spec.md +++ b/openspec/specs/specs-sync-skill/spec.md @@ -49,16 +49,16 @@ The agent SHALL reconcile main specs with delta specs using the delta operation - **THEN** remove the requirement from main spec #### Scenario: REMOVED requirements retire the capability -- **WHEN** removing the requirements named in the delta leaves `## Requirements` with no requirement blocks and no other `###` heading under it -- **AND** the spec holds no `###` heading past the end of that section either, whatever put it there - a second `## Requirements` section, or a `##` line inside an HTML comment +- **WHEN** removing the requirements named in the delta leaves no requirement blocks +- **AND** every other nonblank line in the whole file is accounted for as the title, Purpose, Requirements header, or a canonical requirement's statement, scenarios, or fenced examples - **AND** the rest of the spec is well-formed and it was not already empty before this sync - **AND** the change declares `retire_capabilities: true` in its metadata - **THEN** delete that capability's `spec.md`, and its directory once nothing else remains in it -- **AND** report the retirement, naming the `## Purpose` and any other sections the file held +- **AND** report the retirement and name the deleted `## Purpose` - **AND** leave the file in place and say the marker is missing when it is not declared -#### Scenario: Something is left under Requirements -- **WHEN** any of those conditions fails - content remains under `## Requirements`, a `###` heading sits past the end of that section, the spec is malformed, or nothing was removed this run +#### Scenario: Something is left in the spec +- **WHEN** any of those conditions fails - unaccounted content remains anywhere in the file, the spec is malformed, or nothing was removed this run - **THEN** keep the file in place and report what is left, rather than deleting it #### Scenario: RENAMED requirements diff --git a/skills/openspec-sync-specs/SKILL.md b/skills/openspec-sync-specs/SKILL.md index b3ac5c4cbb..bb416baa06 100644 --- a/skills/openspec-sync-specs/SKILL.md +++ b/skills/openspec-sync-specs/SKILL.md @@ -109,25 +109,22 @@ This is an **agent-driven** operation - you will read delta specs and directly e - Remove the entire requirement block from main spec - Retiring the capability. Delete the whole `spec.md` - and the directory once nothing else is left in it - only when ALL of these hold: - 1. removing the requirements *this run* left `## Requirements` with no - requirement blocks and no other `###` heading under it; + 1. removing the requirements *this run* left no requirement blocks; 2. the rest of the spec is well-formed (it still has a `## Purpose`); 3. the main spec was not already empty before this sync - if you removed nothing, change nothing; - 4. the change's `.openspec.yaml` declares `retire_capabilities: true`. - `openspec archive` requires all four, and refuses on two more it can - check and you cannot easily: a `###` heading past the end of the - `## Requirements` section, and a spec that is already gone. Otherwise keep the - file: report what is left and let the user decide. An empty + 4. every other nonblank line in the whole file is accounted for as the + title, Purpose, Requirements header, or a canonical requirement's + statement, scenarios, or fenced examples; + 5. the change's `.openspec.yaml` declares `retire_capabilities: true`. + Otherwise keep the file: report what is left and let the user decide. An empty `## Requirements` fails `openspec validate`, so say so rather than saving one silently. When only the marker is missing, say that too - it is the one thing the user can add to make the retirement go through. - - Loose prose left under `## Requirements` does NOT block the retirement, and - the CLI cannot name it for you. Read it before you delete: quote it back to - the user, so a hand-written note is a decision rather than a casualty. - - Deleting the file also deletes its `## Purpose` and every other section it - held. Name them when you report the retirement, with the - `git checkout HEAD -- ` that brings the file back. + - Deleting the file also deletes its `## Purpose`; any other section blocks + retirement. Name Purpose when you report the retirement. Include a pasteable + `git checkout` only when the spec lived in the caller's checkout; + otherwise give checkout-scoped recovery guidance. **RENAMED Requirements:** - Find the FROM requirement, rename to TO diff --git a/src/core/archive.ts b/src/core/archive.ts index e309abd54e..c65166f56f 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -1,4 +1,4 @@ -import { promises as fs } from 'fs'; +import { constants, promises as fs } from 'fs'; import path from 'path'; import { formatLocalDate } from '../utils/date.js'; import { getTaskProgressForChange, formatTaskStatus } from '../utils/task-progress.js'; @@ -296,16 +296,18 @@ function toArchiveDiagnostic(error: unknown): ArchiveDiagnostic { /** * Recursively copy a directory. Used when fs.rename fails (e.g. EPERM on Windows). */ -async function copyDirRecursive(src: string, dest: string): Promise { - await fs.mkdir(dest, { recursive: true }); +async function copyDirContents(src: string, dest: string): Promise { const entries = await fs.readdir(src, { withFileTypes: true }); for (const entry of entries) { const srcPath = path.join(src, entry.name); const destPath = path.join(dest, entry.name); if (entry.isDirectory()) { - await copyDirRecursive(srcPath, destPath); + await fs.mkdir(destPath); + await copyDirContents(srcPath, destPath); + } else if (entry.isSymbolicLink()) { + await fs.symlink(await fs.readlink(srcPath), destPath); } else { - await fs.copyFile(srcPath, destPath); + await fs.copyFile(srcPath, destPath, constants.COPYFILE_EXCL); } } } @@ -330,14 +332,122 @@ async function moveDirectory(src: string, dest: string): Promise { ); } if (code === 'EPERM' || code === 'EXDEV') { - await copyDirRecursive(src, dest); - await fs.rm(src, { recursive: true, force: true }); + let destIsOurs = false; + try { + await fs.mkdir(dest); + destIsOurs = true; + await copyDirContents(src, dest); + await fs.rm(src, { recursive: true, force: true }); + } catch (copyError) { + if (destIsOurs) { + await fs.rm(dest, { recursive: true, force: true }).catch(() => undefined); + } + if ((copyError as NodeJS.ErrnoException).code === 'EEXIST') { + throw new ArchiveBlockedError( + 'archive_target_exists', + `Archive '${path.basename(dest)}' already exists.` + ); + } + throw copyError; + } } else { throw err; } } } +async function assertArchiveDestinationAvailable( + archivePath: string, + archiveName: string +): Promise { + try { + await fs.access(archivePath); + throw new ArchiveBlockedError( + 'archive_target_exists', + `Archive '${archiveName}' already exists.` + ); + } catch (error: any) { + if (error instanceof ArchiveBlockedError) throw error; + if (error.code !== 'ENOENT') throw error; + } +} + +async function claimArchiveDestination( + archivePath: string, + archiveName: string +): Promise>> { + try { + return await fs.open(`${archivePath}.lock`, 'wx'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EEXIST') { + throw new ArchiveBlockedError( + 'archive_target_exists', + `Archive '${archiveName}' is already being created.` + ); + } + throw error; + } +} + +interface SpecSnapshot { + target: string; + existed: boolean; + content?: Buffer; + mode?: number; + symlink?: string; +} + +async function captureSpecSnapshots(updates: SpecUpdate[]): Promise { + return Promise.all( + updates.map(async (update) => { + try { + const stat = await fs.lstat(update.target); + return { + target: update.target, + existed: true, + ...((stat.isFile() || stat.isSymbolicLink()) + ? { content: await fs.readFile(update.target) } + : {}), + ...(stat.isFile() ? { mode: stat.mode } : {}), + ...(stat.isSymbolicLink() ? { symlink: await fs.readlink(update.target) } : {}), + }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return { target: update.target, existed: false }; + } + throw error; + } + }) + ); +} + +async function restoreSpecSnapshots(snapshots: SpecSnapshot[]): Promise { + for (const snapshot of [...snapshots].reverse()) { + if (!snapshot.existed) { + await fs.rm(snapshot.target, { force: true }); + continue; + } + if (snapshot.symlink !== undefined) { + try { + await fs.lstat(snapshot.target); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + await fs.mkdir(path.dirname(snapshot.target), { recursive: true }); + await fs.symlink(snapshot.symlink, snapshot.target); + } + if (snapshot.content !== undefined) { + await fs.writeFile(snapshot.target, snapshot.content); + } + continue; + } + if (snapshot.content !== undefined) { + await fs.mkdir(path.dirname(snapshot.target), { recursive: true }); + await fs.writeFile(snapshot.target, snapshot.content); + if (snapshot.mode !== undefined) await fs.chmod(snapshot.target, snapshot.mode); + } + } +} + export class ArchiveCommand { async execute(changeName?: string, options: ArchiveOptions = {}): Promise { const json = !!options.json; @@ -651,24 +761,18 @@ export class ArchiveCommand { const retirementMarker = readRetireCapabilitiesMarker(changeDir); const retirementDeclared = retirementMarker.declared; - let archiveExists = false; - try { - await fs.access(archivePath); - archiveExists = true; - } catch (error: any) { - if (error.code !== 'ENOENT') { - throw error; - } - } - if (archiveExists) { - throw new ArchiveBlockedError('archive_target_exists', `Archive '${archiveName}' already exists.`); - } + await assertArchiveDestinationAvailable(archivePath, archiveName); + await fs.mkdir(archiveDir, { recursive: true }); + const claimPath = `${archivePath}.lock`; + let archiveClaim: Awaited> | undefined; - // Handle spec updates unless skipSpecs flag is set - let specsUpdated = false; - let totals: ArchiveResult['totals']; - const specWarnings: string[] = []; - if (options.skipSpecs) { + try { + // Handle spec updates unless skipSpecs flag is set + let specsUpdated = false; + let totals: ArchiveResult['totals']; + const specWarnings: string[] = []; + let changeArchived = false; + if (options.skipSpecs) { if (!json) { console.log('Skipping spec updates (--skip-specs flag provided).'); } @@ -714,7 +818,7 @@ 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 }; outcome: SpecOutcome; otherSections: string[]; noRequirementBlocks: boolean; unaccountedContent: string[] }> = []; + const prepared: Array<{ update: SpecUpdate; rebuilt: string; counts: { added: number; modified: number; removed: number; renamed: number }; outcome: SpecOutcome; noRequirementBlocks: boolean; unaccountedContent: string[] }> = []; try { for (const update of specUpdates) { const built = await buildUpdatedSpec(update, changeName!, { silent: json }); @@ -723,7 +827,6 @@ export class ArchiveCommand { rebuilt: built.rebuilt, counts: built.counts, outcome: await decideSpecOutcome(update, built, skipValidation, retirementDeclared), - otherSections: built.otherSections, noRequirementBlocks: built.noRequirementBlocks, unaccountedContent: built.unaccountedContent, }); @@ -788,11 +891,11 @@ export class ArchiveCommand { retirementDeclared && p.unaccountedContent.length > 0 && (await isRetirableSpec(specName, p.rebuilt)) - ? `'${specName}' declares retire_capabilities, but the spec holds content outside its ` + - `requirements that deleting the file would take with it: ` + + ? `'${specName}' declares retire_capabilities, but the spec holds content the merge ` + + `cannot safely account for and deleting the file would take with it: ` + `${p.unaccountedContent.slice(0, 3).map((line) => `"${line}"`).join(', ')}` + `${p.unaccountedContent.length > 3 ? `, and ${p.unaccountedContent.length - 3} more line(s)` : ''}. ` + - 'Move it under `## Requirements`, or delete the spec by hand.' + 'Move it into `## Purpose` or a canonical requirement, or delete the spec by hand.' : undefined; if (json) { throw new ArchiveBlockedError( @@ -817,10 +920,24 @@ export class ArchiveCommand { } } - // All validations passed; write files and display counts - const writeTotals = { added: 0, modified: 0, removed: 0, renamed: 0 }; - let wroteAny = false; - for (const p of prepared) { + // A legitimate concurrent archive cannot pass the exclusive claim, + // while this catches an external process that created the final + // destination during a confirmation prompt. Check before the first + // spec mutation so a collision never strands a write or retirement. + await assertArchiveDestinationAvailable(archivePath, archiveName); + archiveClaim = await claimArchiveDestination(archivePath, archiveName); + await assertArchiveDestinationAvailable(archivePath, archiveName); + const specSnapshots = await captureSpecSnapshots( + prepared.map(({ update }) => update) + ); + await moveDirectory(changeDir, archivePath); + changeArchived = true; + + try { + // All validations passed; write files and display counts + const writeTotals = { added: 0, modified: 0, removed: 0, renamed: 0 }; + let wroteAny = false; + for (const p of prepared) { // Deletions are deferred to the loop below. if (p.outcome !== 'write') continue; const { added, modified, removed, renamed } = p.counts; @@ -839,14 +956,14 @@ export class ArchiveCommand { writeTotals.modified += modified; writeTotals.removed += removed; writeTotals.renamed += renamed; - } + } - // Retirements run only after every write has succeeded: they delete a + // Retirements run only after every write has succeeded: they delete a // file, and the write loop is not transactional. Retiring several // capabilities is still not atomic against itself - if a second // deletion fails the first is already done, which the thrown message // names so the state is at least legible. - for (const p of prepared) { + for (const p of prepared) { if (p.outcome !== 'retire') continue; const { retired, sourcePath, resolvedPath } = await retireSpec(p.update, mainSpecsDir, { silent: json, @@ -866,7 +983,7 @@ export class ArchiveCommand { // spec-merge divergence is. Purpose always goes with the file, so it // is named too rather than left to the reader to work out, and the // note carries the command that brings the file back. - const lost = ['Purpose', ...p.otherSections]; + const lost = ['Purpose']; // The path the file actually lived at. A store-selected root is not // under `openspec/` in the caller's repo, and a symlinked capability // directory puts the file somewhere else entirely - naming the @@ -914,7 +1031,7 @@ export class ArchiveCommand { // claim this feature must not get wrong. const pasteablePath = path.isAbsolute(deletedPath) ? undefined - : quoteForShell(deletedPath); + : quoteForShell(`:(top)${deletedPath}`); const recovery = pasteablePath ? `If it was committed, restore it with: git checkout HEAD -- ${pasteablePath}` : `It was deleted from ${deletedPath}; if it was committed, restore it from that checkout's history.`; @@ -928,20 +1045,13 @@ export class ArchiveCommand { // sections it took along, and how to get them back, are the parts // they cannot see from the path. if (!json) { - if (p.otherSections.length > 0) { - console.log( - chalk.yellow( - `⚠️ Warning: ${p.update.id} - the deleted spec also held section(s): ${p.otherSections.join(', ')}.` - ) - ); - } console.log(` ${recovery}`); } - } + } - specsUpdated = wroteAny; - totals = writeTotals; - if (!json) { + specsUpdated = wroteAny; + totals = writeTotals; + if (!json) { console.log( `Totals: + ${writeTotals.added}, ~ ${writeTotals.modified}, - ${writeTotals.removed}, → ${writeTotals.renamed}` ); @@ -950,40 +1060,49 @@ export class ArchiveCommand { ? 'Specs updated successfully.' : 'Specs already in sync; no files changed.' ); + } + } catch (error) { + await restoreSpecSnapshots(specSnapshots); + await moveDirectory(archivePath, changeDir); + changeArchived = false; + throw error; } } } } - // The destination was checked before the merge, so anything claiming it now + // The destination was checked before the merge, so anything claiming it now // appeared while we were working. Report that as the collision it is: a raw // ENOTEMPTY from rename would otherwise degrade to a bare `archive_error`. - try { - await fs.access(archivePath); - throw new ArchiveBlockedError('archive_target_exists', `Archive '${archiveName}' already exists.`); - } catch (error: any) { - if (error instanceof ArchiveBlockedError) throw error; - if (error.code !== 'ENOENT') throw error; - } + if (!changeArchived) { + await assertArchiveDestinationAvailable(archivePath, archiveName); + archiveClaim = await claimArchiveDestination(archivePath, archiveName); + await assertArchiveDestinationAvailable(archivePath, archiveName); - // Create archive directory if needed - await fs.mkdir(archiveDir, { recursive: true }); + // Create archive directory if needed + await fs.mkdir(archiveDir, { recursive: true }); - // Move change to archive (uses copy+remove on EPERM/EXDEV, e.g. Windows) - await moveDirectory(changeDir, archivePath); + // Move change to archive (uses copy+remove on EPERM/EXDEV, e.g. Windows) + await moveDirectory(changeDir, archivePath); + changeArchived = true; + } - if (!json) { - console.log(`Change '${changeName}' archived as '${archiveName}'.`); - } + if (!json) { + console.log(`Change '${changeName}' archived as '${archiveName}'.`); + } - return { - change: changeName, - archivedAs: archiveName, - path: archivePath, - specsUpdated, - ...(totals ? { totals } : {}), - ...(specWarnings.length > 0 ? { warnings: specWarnings } : {}), - }; + return { + change: changeName, + archivedAs: archiveName, + path: archivePath, + specsUpdated, + ...(totals ? { totals } : {}), + ...(specWarnings.length > 0 ? { warnings: specWarnings } : {}), + }; + } finally { + await archiveClaim?.close().catch(() => undefined); + if (archiveClaim) await fs.unlink(claimPath).catch(() => undefined); + } } private async selectChange( diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index d6ecc079a5..ddb5533359 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -120,7 +120,6 @@ export async function buildUpdatedSpec( * deletes the whole file, so callers name these rather than discarding * hand-written prose silently. */ - otherSections: string[]; }> { // Collected so silent (JSON) callers can surface them; printed live for // human callers at the point they occur. @@ -457,7 +456,8 @@ export async function buildUpdatedSpec( // only safe for one. if (replacement !== block) { const orphan = firstForeignLine(block.raw); - if (orphan) { + const replacementOrphan = replacement ? firstForeignLine(replacement.raw) : undefined; + if (orphan && orphan !== replacementOrphan) { warn( `${specName} - "${orphan}" sits inside requirement "${block.name}" and goes with it. ` + 'Move it under its own requirement, or above `## Requirements`, to keep it.' @@ -499,7 +499,6 @@ export async function buildUpdatedSpec( // the first requirement - it would veto `### Notes` written before the // requirements and miss the identical heading written after them. unaccountedContent: contentTheMergeCannotName(parts), - otherSections: findOtherSections(rebuilt), }; } @@ -525,32 +524,6 @@ function firstForeignLine(raw: string): string | undefined { return undefined; } -/** - * Structural headings matching `pattern`, ignoring anything inside a fenced code - * block or an HTML comment - the same two things every other structural scan in - * this file masks, because a commented-out or fenced heading is invisible to the - * spec parsers but still sits in the file (#1413). - */ -function findHeadings(content: string, pattern: RegExp): string[] { - const normalized = content.replace(/\r\n?/g, '\n'); - const lines = normalized.split('\n'); - // Fence first, then comments. The other order lets a `