fix(archive): stop failing on specs that were already synced before archiving#1376
Conversation
Archiving a change whose specs were synced to the baseline first (the early-sync pattern from the sync workflow) failed with 'ADDED failed - already exists'. An ADDED requirement that already exists in the target spec with identical content is now skipped; differing content still aborts as a genuine conflict. Fixes #1332. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesArchive idempotency
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the archive/spec application path, linked issue, CI, and focused local tests. The ADDED no-op is scoped correctly and preserves conflict safety. Looks good to merge.
Keep all three archive tests: the two idempotent-ADDED tests from this branch and the nested delta-spec test from main (#1355). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alfred-openspec
left a comment
There was a problem hiding this comment.
The archive fix itself remains correct, but the merge rewrote pnpm-lock.yaml from v9 to v6 with 3,870 unrelated changed lines and dependency-version churn despite no manifest change. This is causing Nix validation to fail on a missing offline tarball; please restore the lockfile from main and rerun CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Restored |
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified the remediation restores pnpm-lock.yaml byte-for-byte with main; the final diff is back to the intended archive fix, changeset, and focused tests. The identical-ADDED no-op remains correctly scoped, conflict safety is preserved, and CI is green.
Fixes #1332
What was wrong
Syncing a change's specs to the main specs early (to unblock dependent changes) and archiving later is a pattern OpenSpec's own
syncworkflow supports — butopenspec archivefailed on it:buildUpdatedSpecthrew whenever an ADDED requirement already existed in the target spec, even when the existing content was identical — i.e., when re-applying it would change nothing. The only way through was discovering the--skip-specsworkaround.How it was fixed
In the ADDED pass of
buildUpdatedSpec(src/core/specs-apply.ts), a requirement that already exists in the target with identical content (line-ending-normalized, trimmed) is now skipped as a no-op. A same-named requirement with different content still aborts the archive — the genuine-conflict safety check is unchanged. Reported counts now reflect what was actually applied, so an already-synced change shows+ 0 addedinstead of claiming to add requirements it didn't touch.Replication / proof
Repro from the issue: sync a change's ADDED requirement into
openspec/specs/, thenopenspec archive <change> --yes.ADDED failed ... - already exists, exit code 1, change stays active.archive/:Tests in
test/core/archive.test.ts:main, passes here): already-synced ADDED archives cleanly with no duplicate requirementFull suite: 1,860 passed; the only failures are the 17 known environment-only
zsh-installerones (#1321, unrelated, green in CI).Notes
Scoped to ADDED only, matching the issue. After an early sync, MODIFIED is naturally idempotent (replacing a block with itself), but REMOVED/RENAMED deltas referencing already-synced state would still error — those lack a content-identity check to distinguish "already done" from a typo, so loosening them trades away real safety. Can follow up if that pattern shows up in practice.
🤖 Generated with Claude Code
Summary by CodeRabbit
openspec archivefailures forADDEDrequirements that were already synced to main before archiving.ADDEDblocks are treated as a no-op to avoid duplicate requirements.ADDEDrequirement exists with different content.ADDEDrequirements during archiving.