feat(archive): let a change retire a capability it empties - #1484
Conversation
…irement
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 Fission-AI#1302
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCapability retirement is added for deltas that remove a capability’s final requirement. Archive deletes eligible main specs and empty directories, preserves unrelated files, updates totals and warnings, and documents the behavior across specifications, skills, templates, and CLI contracts. ChangesCapability retirement
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/archive.ts`:
- Around line 529-543: Update the retired branch in the archive flow around
retireSpec so a successful retirement also adds p.counts.renamed to
writeTotals.renamed alongside the existing removed total, preserving the no-op
continue and only carrying counts forward when the file is actually deleted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b93edca3-90c7-43dd-8c06-555d8fbd8015
📒 Files selected for processing (10)
.changeset/retire-capability-on-removed-only-delta.mdopenspec/specs/cli-archive/spec.mdskills/openspec-archive-change/SKILL.mdskills/openspec-sync-specs/SKILL.mdsrc/core/archive.tssrc/core/specs-apply.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/sync-specs.tstest/core/archive.test.tstest/core/templates/skill-templates-parity.test.ts
…letion 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 Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/specs-apply.ts (1)
468-485: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
findOtherSectionsmasks fenced code but not HTML comments.A
##heading inside a<!-- ... -->block (common in spec templates/instructional stubs) is reported as a deleted section. Warning-only, so cosmetic, but the doc comment's claim ("outside fenced code") is the accurate one — worth masking comment regions if that text is meant to be authoritative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/specs-apply.ts` around lines 468 - 485, Update findOtherSections to ignore ## headings located inside HTML comment blocks, while preserving its existing fenced-code masking and Purpose/Requirements exclusions. Extend or reuse the line mask logic to track <!-- through --> regions before collecting section titles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/specs-apply.ts`:
- Around line 468-485: Update findOtherSections to ignore ## headings located
inside HTML comment blocks, while preserving its existing fenced-code masking
and Purpose/Requirements exclusions. Extend or reuse the line mask logic to
track <!-- through --> regions before collecting section titles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57afbfe1-111a-4b62-9124-924a26fb9567
📒 Files selected for processing (17)
docs/agent-contract.mddocs/cli.mddocs/concepts.mddocs/writing-specs.mdopenspec/specs/cli-archive/spec.mdopenspec/specs/opsx-archive-skill/spec.mdopenspec/specs/specs-sync-skill/spec.mdskills/openspec-archive-change/SKILL.mdskills/openspec-bulk-archive-change/SKILL.mdskills/openspec-sync-specs/SKILL.mdsrc/core/archive.tssrc/core/specs-apply.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tssrc/core/templates/workflows/sync-specs.tstest/core/archive.test.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- skills/openspec-archive-change/SKILL.md
- src/core/templates/workflows/archive-change.ts
- src/core/templates/workflows/sync-specs.ts
- skills/openspec-sync-specs/SKILL.md
- test/core/templates/skill-templates-parity.test.ts
- src/core/archive.ts
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 Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/specs-apply.ts`:
- Around line 561-568: Update the resolvedNote comparison in the retirement flow
to canonicalize the expected update.target path before comparing it with
realTarget. Reuse fs.realpath on the containing directory and rejoin the target
basename, preserving the existing note and retiredPath behavior only when the
canonical paths genuinely differ.
In `@test/core/archive.test.ts`:
- Around line 3272-3283: Make the archive deferral test’s forced write failure
deterministic instead of relying on chmod: update the test around
archiveCommand.execute to skip on Windows and when running with effective root
privileges, or mock the relevant write operation such as
writeUpdatedSpec/fs.writeFile to throw. Preserve the assertion that
legacyDir/spec.md remains available when the later spec write fails, and retain
cleanup for any temporary permissions or mocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32cedc4c-7df2-40e8-b96a-bc8daf0def93
📒 Files selected for processing (15)
.changeset/retire-capability-on-removed-only-delta.mddocs/agent-contract.mdopenspec/specs/cli-archive/spec.mdopenspec/specs/opsx-archive-skill/spec.mdopenspec/specs/specs-sync-skill/spec.mdskills/openspec-archive-change/SKILL.mdskills/openspec-bulk-archive-change/SKILL.mdskills/openspec-sync-specs/SKILL.mdsrc/core/archive.tssrc/core/specs-apply.tssrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tssrc/core/templates/workflows/sync-specs.tstest/core/archive.test.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- openspec/specs/opsx-archive-skill/spec.md
- skills/openspec-archive-change/SKILL.md
- skills/openspec-bulk-archive-change/SKILL.md
- .changeset/retire-capability-on-removed-only-delta.md
- src/core/templates/workflows/archive-change.ts
- docs/agent-contract.md
- src/core/templates/workflows/bulk-archive-change.ts
- skills/openspec-sync-specs/SKILL.md
- src/core/templates/workflows/sync-specs.ts
- openspec/specs/cli-archive/spec.md
- test/core/templates/skill-templates-parity.test.ts
…th 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 Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 `<!--` inside a fenced example blanked the rest of the document and truncated the very list of sections the deletion was reporting. Fence first, then comments. - Moving the collision check before the merge widened the window between it and the move, where a claimed destination surfaced as a raw ENOTEMPTY and degraded to `archive_error`. `moveDirectory` now reports that as `archive_target_exists`, the same diagnostic the pre-flight check gives. And a simplification the review asked for: the overlapping `retirable` / `deletes` / `retired` booleans are now one `decideSpecOutcome()` returning 'write' | 'delete' | 'skip'. Behavior is identical - same clauses, same order - but the fourth state that existed only as a comment is now a visible return. Both guards were kept: the review constructed inputs where each is the sole thing preventing a data-losing delete. Two tests the review found wanting are gone or rewritten: one killed no unique mutant, and one assertion straddled two editable message fragments and could have gone vacuously true. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/archive.ts (1)
499-524: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReserve the archive destination atomically before mutating specs.
fs.access()only checks; it does not reservearchivePath. If another process creates the target after Line 514, spec writes and retirements can complete before Lines 706-715 abort the archive. The change then remains active while its canonical specs were already changed/deleted, and a retry no longer operates on the original state. Use a lifecycle-managed atomic reservation/lock honored by archive invocations, or move-and-rollback staging that claims the destination before spec mutations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/archive.ts` around lines 499 - 524, Replace the non-atomic fs.access check in the archive flow with a lifecycle-managed atomic reservation of archivePath before any spec mutation. Ensure concurrent archive invocations honor the reservation, release or finalize it on success or failure, and preserve the existing ArchiveBlockedError behavior when the destination is already claimed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/core/archive.test.ts`:
- Around line 3491-3502: Canonicalize the shared path before the warning
assertion in the archive test, using the test-only fs realpathSync.native API
and the existing promises fs import pattern. Compare warnings against the
canonicalized shared path so the negative containment assertion remains
meaningful across macOS and other filesystem path representations, while leaving
the subsequent file-existence check unchanged.
---
Outside diff comments:
In `@src/core/archive.ts`:
- Around line 499-524: Replace the non-atomic fs.access check in the archive
flow with a lifecycle-managed atomic reservation of archivePath before any spec
mutation. Ensure concurrent archive invocations honor the reservation, release
or finalize it on success or failure, and preserve the existing
ArchiveBlockedError behavior when the destination is already claimed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dfccad96-b971-4624-b33c-83bde2a73998
📒 Files selected for processing (4)
openspec/specs/cli-archive/spec.mdsrc/core/archive.tssrc/core/specs-apply.tstest/core/archive.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- openspec/specs/cli-archive/spec.md
- src/core/specs-apply.ts
CodeRabbit caught that `expect(warnings).not.toContain(shared)` passed vacuously: on macOS the temp root lives under /var, whose realpath is /private/var, so the warning would print a form the assertion never compared against. The sibling assertion on `tempDir` had the same flaw. Both now canonicalize first, and both were confirmed to fail against a mutant - dropping the lstat guard, and forcing the resolved-path note on - which neither did before. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ad of deleting it Retiring a capability was the first case where archiving deleted a file under `openspec/specs/`. Nothing in the repo had ever removed spec content before, so the blast radius of a wrong verdict was a lost file with only the reflog to recover it. The spec now moves instead. It is staged into the change directory, which the archive step renames onto the archive path moments later, so it comes to rest at `<archive>/retired-specs/<capability>/spec.md` beside the proposal and tasks that retired it. `git` records a rename, and bringing a capability back is a `git mv` from the archive. Staged into the change rather than written to the archive path after the move, because the archive path must not exist yet and the ordering is safer: if a later step fails, the spec sits in a change that is still active and a rerun carries it through, versus stranding the live specs tree without a spec it still needs. A symlinked `spec.md` is copied by content and its link removed, rather than moved: relocating the link itself would archive a relative path that no longer resolves from where it landed. A spec already staged by an earlier aborted run is never overwritten - it is the only copy once the live one moves. The retirement verdict, its guards, and the deferral until every write has succeeded are all unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The staging directories are created before the move, so any failure left an empty `retired-specs/<capability>/` behind. That folder then rode into the archive with the change, where it reads as a retirement that never happened - a spec was supposedly retired here, and there is nothing to show for it. The failure path now prunes back up to the change directory. Only empty directories go, so a capability the same run already staged next to the failing one is untouched, and the guard that refuses to overwrite a staged spec still stops at a non-empty destination. Both cases are covered by tests that fail without the prune: a dangling symlink is the reproducible post-staging failure, since lstat sees a file and the copy then follows the link and finds nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ted" Three leftovers from the deletion version: the `residualRequirementHeadings` comment, `pruneEmptyDirs`'s `mainSpecsDir` parameter - now a boundary that is the change directory on the cleanup path, not the specs root - and a sentence in writing-specs.md that used "deleted" for the requirement and then again for the file, two lines apart. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The retirement move still leaves a retry-blocking staged copy when copying succeeds but removing the live source fails. At exact head 4ff21293, I reproduced this with a symlinked spec.md in a read-only capability directory: copyFile() succeeded, unlink() returned EACCES, and both the live symlink and retired-specs/legacy/spec.md remained, so the next run fails because the destination is already taken; please roll back only the destination created by this attempt on post-copy failure and add that regression for the symlink path (and the EXDEV/EPERM fallback, which has the same copy-then-unlink shape).
…moved Both non-atomic retirement routes - a symlinked main spec, and the EXDEV/EPERM rename fallback - copy the spec into staging first and remove the original second. A copy that landed before an `unlink` that failed left the spec in TWO places, and the staged one then tripped the "already staged" guard on every rerun. The error told the caller to rerun the archive, and the rerun could never work. Reproduced at the previous head with a symlinked `spec.md` in a read-only capability directory: `copyFile` succeeded, `unlink` returned EACCES, and both copies remained. The failure path now deletes the destination this attempt created, so the capability is left exactly as the attempt found it and the rerun works. The rollback is gated on a flag set only after the destination is proven free, so a spec staged by an EARLIER run is never the thing removed - the overwrite guard still fires ahead of it and rolls nothing back. A partially written copy is cleaned by the same call. The message no longer promises more than it delivers: it reports that the spec is still in place, or names the leftover copy when the rollback itself failed. Regression tests cover both routes and assert the rerun succeeds, not just that the copy is gone. Both fail without the rollback. The cross-device route injects EXDEV, which cannot be provoked inside one temp directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two post-copy rollback cases shared one `skipIf(win32)`, inherited from the symlink case, which needs privileges Windows does not grant by default. The rename-fallback case uses regular files and spies only, and the sibling errno it stands in for - EPERM - is the Windows case, so skipping it there left that route untested on the platform that produces it. Skipping is now per-case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The rollback fix still has a data-loss race because fs.access plus destIsOurs is not an ownership claim. On exact head 4126e21, two concurrent retireSpec calls both saw the destination as free; one renamed the live spec into staging, the other got ENOENT and then unlinked that staged file because its own destIsOurs was true. Across 40/40 isolated iterations both the live source and staged destination ended missing, while one caller reported retired: true and the other retired: false. Please claim the destination atomically before moving or copying, and only roll back a claim created by that invocation, with a concurrent regression. The isolated build and 147 existing focused tests pass, so this needs a race-specific guard.
`fs.access` followed by a write is not an ownership claim. Two concurrent retirements both saw the destination free and both set `destIsOurs`; one moved the spec into staging, and the other - equally convinced the file was its own - rolled it back out. The source and the staged copy both ended up gone. Reproduced at the previous head in 36 of 40 iterations. The claim and the content now arrive in one syscall: `copyFile` with `COPYFILE_EXCL` fails with EEXIST rather than overwriting, so exactly one caller can ever own the path. That is also the check that refuses to clobber a spec an earlier aborted run staged, now decided atomically rather than by a separate look beforehand. The losing caller fails two ways, and both used to destroy the winner's file. EEXIST is the obvious one. ENOENT is not: `copyFile` opens the source first, so a loser that arrives after the winner removed the source fails before creating anything - and treating that as "a partial copy of mine" unlinked the winner's file. Neither errno now claims ownership. Fixing only EEXIST left 4 of 40 iterations still losing both copies. Copying rather than renaming is what makes the claim possible: `rename` overwrites silently on every platform, so it cannot tell "I created this" from "I destroyed someone else's". It also crosses filesystems, which retires the EXDEV/EPERM fallback, and reads a symlink's content rather than moving the link - so the two routes collapse into one shape. Regression asserts the invariant over 25 rounds: exactly one caller retires, the spec survives once and intact, and the source is gone. It fails against the old access-then-write shape. Not crash-safe, which is a weaker promise and now documented: a process killed between the copy and the unlink leaves the spec in both places, and the next run refuses rather than guessing which to keep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The exclusive-copy fix closes the concurrent race but can now delete a pre-existing staged recovery copy on a source-side error. At exact head e3bd2cd, I made the live spec.md unreadable and pre-created retired-specs/legacy/spec.md; copyFile(..., COPYFILE_EXCL) failed with source EACCES, the non-EEXIST/ENOENT branch set destIsOurs = true, and cleanup unlinked the prior staged file, so destination ownership must come from an explicit atomic claim created by this invocation (for example open(dest, 'wx') and writing through that handle), never inferred from a later copy errno.
… an errno Claiming the destination with `copyFile(..., COPYFILE_EXCL)` closed the concurrent race but kept reading ownership out of a failure code, and that cannot be made correct however the errnos are partitioned. An errno says what went wrong, not what was created: a source-side EACCES is indistinguishable from a partial copy of our own, so the cleanup deleted a recovery copy an earlier run had staged - the last remaining copy of a spec whose live file could not even be read. Reproduced at the previous head with an unreadable `spec.md` and a pre-existing `retired-specs/legacy/spec.md`: the staged file was destroyed. Ownership now comes from `open(dest, 'wx')`. O_CREAT|O_EXCL returns a handle exactly when it created the file, so the question is answered by the syscall instead of inferred afterwards, and every failure path leaves the flag false. EEXIST remains the refusal that protects an earlier run's copy, now decided by the same operation. Content is written through the claimed handle, as bytes, and the handle is closed before any rollback so Windows can unlink it. The regression uses real mode bits, skipped on Windows and under root: the defect was a source-side errno being read as proof about the destination, and stubbing a JS-level read cannot reproduce it, because the copy it has to fool never went through one. Verified it fails against the errno- inference version. All three findings on this path now hold together: the pre-existing copy survives, 0 of 120 racing iterations lose a spec, and a post-copy unlink failure still rolls back and reruns cleanly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The destination is now claimed by an exclusive create, and cleanup is gated only on that successful claim. Exact head 3028990 passed the hosted CI/security matrix, an isolated build, and all 149 focused tests; additional exact-head probes completed 50 concurrent retirement races with zero loss, preserved both the live and pre-existing staged recovery copies on the hostile path, and rolled back staging cleanly when source removal failed.
The rollback exists for a copy that landed while the source survived - the two-places state that blocks every rerun. It must not fire once the source is gone: at that point the staged copy holds the only remaining content, and the end state the retirement was reaching for is already reached. An external delete landing between the read and the unlink produced exactly that, and the rollback destroyed the spec outright - `retired: false`, no live file, no staged copy, content gone. `unlink` returning ENOENT is now a success rather than a failure to roll back. Every other errno still throws: the source is still sitting there, and leaving the staged copy beside it is the state that blocks a rerun. Found reviewing the finished path rather than reported - the same class as the three review findings before it, all of them the rollback reaching a copy it should not have. Regression verified against the unconditional unlink. Also corrects a doc line that still credited the copy with claiming the destination; the claim is the exclusive create. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The ENOENT follow-up is correct: once the staged write is complete and the source is already gone, retaining the staged copy preserves the only remaining content and matches the intended retirement state, while every other unlink failure still rolls back. Exact head a48d03b passed the full hosted CI/security matrix, an isolated build, all 150 focused tests, and an additional 50 concurrent retirement races with zero failures.
…-specs/ Reworks Fission-AI#1302 to follow the design that already exists instead of adding one. The move-into-the-archive approach introduced two things OpenSpec did not have: capability retirement as a lifecycle state, and `retired-specs/` as an on-disk convention no schema declares - which a future unarchive command would have to know about. Its whole justification was preserving content that two existing mechanisms already preserve: the archived change carries the delta naming every REMOVED requirement with its Reason and Migration, and git carries the file. The approach even conceded the point by advertising `git mv` as the recovery path. The issue itself proposed neither. It asked for a delete, or an explicit retirement marker. This does both: archive deletes the emptied spec, and only when the change declares `retire_capabilities: true` in its `.openspec.yaml`. `skip_specs` is the precedent. The marker reader is the same function, parameterised by key, so the two can never drift apart on what counts as honorable metadata - a marker in unparseable YAML, or one whose schema does not load, is not a marker in either case. An explicit `false` is not an unhonorable marker, it is simply undeclared. Without the marker nothing changes: the unwritable spec aborts the archive exactly as before, except the abort now names the marker as the way out - and says nothing about it when retiring would not have made the spec writable anyway, so it never sends an author after the wrong fix. Applying REMOVED already deletes requirement content from a main spec, so deleting the spec once nothing is left is that same operation carried to its end. Every guard survives: the validator's verdict, the residual-heading veto, something-removed-this-run, and never under --no-validate. What goes is the exclusive claim, the rollback, the staging directories, and the four data-loss windows they created across four review rounds. Net 307 lines smaller than the move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
c1e7778 does not actually preserve approved #1490 head 10e54d6: it replaces the identity/full-tail loss check with a heading-only comparison and removes the final regressions, so exact-head tests reproduce both wrong directions: RENAMED keeps ### Notes plus Kept by hand. but falsely says it goes away, while MODIFIED keeps the heading, drops Kept by hand., and emits no warning. Archive also calls buildUpdatedSpec() only after Proceed with spec updates?, reverting #1490's warning-before-decision guarantee; please merge/rebase onto #1490's current head without overwriting those semantics and retain its actual-loss plus real-archive regressions.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Separate rollback blocker at c1e7778: captureSpecSnapshots() reads content through a symlink, then restoreSpecSnapshots() recreates the link and writes that stale snapshot through it. In an exact-head two-retirement repro, the first retirement removed a symlinked spec.md, its external target changed, the second retirement failed, and rollback overwrote the external update with the old spec; restore the symlink itself only and add this failure-path regression.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 71b307e. The rollback safety follow-ups preserve actual-loss detection and pass the focused archive, salvage, template-parity, and hosted checks.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Exact head 443a99e still has an authorization race: the new flow mutates main specs before moving/sealing the active change, but its pre-mutation checks fingerprint only the delta and target. In a focused exact-head regression, removing retire_capabilities: true immediately before the retirement displacement let archive succeed, delete the live spec, and archive .openspec.yaml without that authorization; please seal or fingerprint/recheck the metadata at the mutation boundary and after the final move, with this regression.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 2d3761f. Retirement authorization is now fingerprinted and rechecked at displacement and final-move boundaries, with both race regressions passing alongside the focused archive and template-parity suite.
Reject structurally ambiguous or symlinked inputs before mutation, serialize archive claims safely, and preserve permissions during verified fallback moves. Keep retired specs as inode-preserving backups until the archive commits, restore them on rollback, and retain any backup changed concurrently instead of deleting user data.
Add a per-claim nonce and verify stable claim contents before unlinking because Windows file IDs may not distinguish a replacement lock entry.
Skip the POSIX unlink-and-recreate claim simulation on Windows, where deletion of an open file remains pending until the original handle closes.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Exact head 6721f1a is clean. Retirement remains explicitly authorized, loss warnings are derived before mutation, every destructive boundary revalidates inputs and metadata, staged ownership and rollback preserve concurrent or replaced content, fallback copies are fingerprint-verified, and retained backups remain recoverable. A fresh exact-head build plus 223 focused archive, parity, and source-spec tests pass, alongside the full hosted matrix.
6721f1a to
0deaab2
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the refreshed head. The main merge is conflict-free, the symlink fixture updates correctly enforce the new path-containment boundary, all 230 focused archive/parity/source-spec tests pass locally, and the full hosted matrix is green.
2 participants
Closes #1302. Depends on #1490.
The problem
Remove every requirement from a capability and the change could never be archived. The rebuilt spec had nothing in it, an empty spec can't validate, and archive stopped:
Deleting the spec first didn't help — the delta then looked like a create and hit the same wall.
The fix
A change can say it's retiring a capability:
Without the marker nothing changes. Archive stops exactly as before — it just tells you the way out:
skip_specsalready works this way, so this reuses it rather than inventing anything — both markers share one reader and can't disagree about what counts as valid metadata.When it refuses
Archive only deletes a spec it can fully account for. It stops, exactly as before, if:
retire_capabilities: true--no-validateThat third one is deliberately strict. If your spec has an authored section beyond
## Purpose, archive names the lines and asks you to move them or delete the file yourself:Worth knowing
git checkoutthat restores it, but only when that command would actually work — for a store or a symlinked capability the file lives outside your repo, so it tells you where it was instead.target spec does not exist. Close or rework it alongside the retirement.Notes
No CLI flag, JSON key, or exit code changes for anyone who doesn't opt in.
minor, matching howskip_specsshipped in 1.7.0.