fix(templates): don't archive a change before its spec sync finishes#1394
Merged
Conversation
The generated openspec-archive-change skill dispatched the spec sync to a subagent via the Task tool and then moved changeRoot in the very next step, with nothing requiring it to wait. Where subagents run asynchronously, the archive relocates the delta specs out from under the running sync, so the change is archived while openspec/specs/ is never updated — and the success summary still reports "Specs: ✓ Synced". Step 4 now requires waiting for the dispatched sync to return, verifying the synced requirements are present in the main spec, and stopping without archiving if either check fails. Adds a matching guardrail bullet and a parity assertion so the gate cannot silently disappear again. Fixes #1393 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughArchive workflows now run spec synchronization inline, verify synced requirements in main specs, and stop before archiving on cancellation, sync failure, or mismatches. Templates, parity tests, the archive specification, and patch release metadata were updated. ChangesArchive sync gating
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ArchiveWorkflow
participant openspec-sync-specs
participant MainSpec
ArchiveWorkflow->>openspec-sync-specs: Run delta spec synchronization inline
openspec-sync-specs-->>ArchiveWorkflow: Return completion status
ArchiveWorkflow->>MainSpec: Verify capability requirements
MainSpec-->>ArchiveWorkflow: Return match status
ArchiveWorkflow->>ArchiveWorkflow: Archive only after successful verification
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Addresses review on #1394. The first pass asked the agent to "wait" for a dispatched subagent, but subagents run in the background by default and the wait is not reliably expressible in prose — the race survived. It also gated the archive on the synced requirements being *present*, which a correct REMOVED-only or RENAMED-only sync does not satisfy, turning a successful sync into a hard block. The sync now runs inline via the Skill tool, with a synchronous-subagent fallback for harnesses that need one. Verification follows delta semantics: ADDED/MODIFIED present, REMOVED gone, RENAMED under the new name, checked across every capability the sync touched. Also resolves the opsx command variant's contradiction with its own guardrail, stops the summary reporting a checkmark that step 4 never verified, and updates openspec/specs/opsx-archive-skill/spec.md, which still said the skill proceeds with the archive regardless of the sync choice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@openspec/specs/opsx-archive-skill/spec.md`:
- Around line 77-80: Clarify the archive workflow requirements around inline
/opsx:sync: verify ADDED and MODIFIED requirements are present, REMOVED
requirements are absent, and RENAMED requirements exist under the new name.
Distinguish an explicit “Archive without syncing” choice from a requested sync
that fails or remains unverified; only the former may proceed without
verification, while the latter must stop archiving. Update the conditions around
the archive decision to encode this contract.
🪄 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
Run ID: 0e719f9c-9456-4b5b-8aa1-25609687cfc1
📒 Files selected for processing (5)
.changeset/archive-waits-for-spec-sync.mdopenspec/specs/opsx-archive-skill/spec.mdskills/openspec-archive-change/SKILL.mdsrc/core/templates/workflows/archive-change.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/archive-waits-for-spec-sync.md
- skills/openspec-archive-change/SKILL.md
- src/core/templates/workflows/archive-change.ts
- test/core/templates/skill-templates-parity.test.ts
… spec The scenario said the agent verifies each capability "matches its delta", which is ambiguous about what a match means — and a REMOVED-only sync correctly leaves requirements absent. Spell out the predicate the template implements, and separate an explicit "Archive without syncing" choice from a requested sync that failed or could not be verified: only the former may skip verification, the latter must stop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
Second review pass on #1394. The gate was weaker than it looked. "MODIFIED requirements present" is vacuous — a MODIFIED requirement exists in the main spec before the sync runs, so a no-op sync passed the check for the most common delta shape, which is the exact symptom #1393 reports. "RENAMED under their new name" passed a sync that copied rather than renamed, leaving both names behind. And scoping the re-check to "every capability it touched" derived the verification set from the artifact being verified, so a silently skipped capability escaped it. Verification is now bound to the delta specs in artifactPaths.specs, covers the changes each MODIFIED delta names, and requires RENAMED requirements to be gone from the old name. Separately, the previous pass named the Claude Code "Skill tool" and run_in_background in a template that is also the slash-command source for ~28 other tools, where skills are removed entirely for commands-only delivery. Both variants now use the runtime-neutral phrasing bulk-archive-change already uses. Also: route the prompt options explicitly instead of defaulting unknown answers to archive, tell the user a stopped archive is recoverable, and mark the summary line as a conditional rather than literal text to copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The verification predicate restated delta semantics in its own words, which could drift from what openspec-sync-specs actually does. Anchor it instead to the comparison step 4 already performs before prompting: a successful sync leaves nothing to apply, so every capability must read as already synced. The explicit ADDED/MODIFIED/REMOVED/RENAMED bullets stay as the definition of what "nothing left to apply" means. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
approved these changes
Jul 20, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The latest commits close the prior verification holes: MODIFIED content is checked, RENAMED requires the old name to disappear, and the verification set comes from the delta inputs rather than the sync report. The focused seven-test parity suite passes locally at 035039f and the full CI/CodeQL matrix is green, so this is approved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
LGTM — ready to review. Fixes #1393. Agent-facing template prose plus the repo's own spec; no CLI or parser code is touched.
What was wrong
The generated
openspec-archive-changeskill told the agent to hand the spec sync to a background task, then archive:Step 5 then runs
mv "<changeRoot>" ".../archive/YYYY-MM-DD-<name>". Nothing in between required the sync to have finished.Background tasks don't block, so the archive moves the delta specs out from under a sync that is still reading them. The change lands in
archive/,openspec/specs/never receives the requirements, and the summary still printsSpecs: ✓ Synced— the user is told the sync happened when it did not. Where the delegation happens to block, it works, so correctness depended on scheduling rather than on the instructions.How it was fixed
The sync runs inline. Step 4 now runs the
openspec-sync-specsworkflow inline and waits, instead of delegating it. Removing the background hop removes the race at its source rather than asking prose to win a scheduling argument.Verification follows delta semantics. Before step 5, the agent re-checks every capability that has a delta spec — not just the ones the sync claims it touched, so a silently skipped capability can't escape:
This is the same comparison step 4 already runs before prompting, so the check can't drift from what
openspec-sync-specsactually does: a successful sync leaves nothing left to apply.If the sync failed or a capability doesn't match, the agent reports what differs and stops. Nothing has moved at that point, so the user can fix it and retry.
Getting this predicate right took two passes, and both mistakes are now covered by tests:
MODIFIEDfor mere presence is vacuous — a MODIFIED requirement already exists before the sync — so a no-op sync would have passed the gate for the most common delta shape.Tool-neutral wording. This template is also the slash-command source for ~28 non-Claude tools, and skills are removed entirely under commands-only delivery. An earlier revision named the Claude Code "Skill tool" and
run_in_background; both variants now use the runtime-neutral phrasingbulk-archive-changealready uses.Two consistency fixes fall out of the same area: the prompt options are now routed explicitly (an unrecognized answer re-asks instead of defaulting to archive), and the summary's
✓ Syncedline is marked conditional on the step 4 verification.Scope
src/core/templates/workflows/archive-change.tsopenspec/specs/opsx-archive-skill/spec.mdskills/openspec-archive-change/SKILL.mdpnpm build && pnpm generate:skills)test/core/templates/skill-templates-parity.test.ts.changeset/archive-waits-for-spec-sync.mdbulk-archive-changenever delegates its sync, so it doesn't have this race and is left alone.Proof it works
openspec validate --specs --strict→ 36 passed, 0 failed.The parity test
gates the archive on a completed spec sync (#1393)pins the invariants in both the generated skill and the command template: the sync runs inline, delegation to a background task is prohibited, verification is bound to the delta specs on disk, and the MODIFIED/REMOVED/RENAMED predicates hold. All seven asserted strings are absent frommain, so it fails without this change — verified, not assumed.Test suite: 2028 tests. Local failures are the 17 known environment-only
zsh-installercases (oh-my-zsh present, per CLAUDE.md); under heavy parallel load a few CLI-spawn tests can also time out. CI is green on Linux, macOS and Windows.Getting the fix
These are generated assets, so existing projects pick this up by upgrading and re-running
openspec update, which regenerates the skills and commands from these templates. New projects get it atopenspec init.Notes / nits
skill-templates-parity.test.tsand needspnpm build && pnpm generate:skillsplus fresh hashes.sync-specsstep 4d creates a new main spec from ADDED requirements only. Today that surfaces as a blocked archive with a report naming the mismatch, instead of a silently incomplete spec. "Archive without syncing" remains available as the escape hatch.bulk-archive-changereports "N delta specs synced" with no verification, so the silent-false-success half of Generated archive skill can race the spec sync: archive proceeds without waiting for the dispatched sync subagent #1393 survives there.🤖 Generated with Claude Code using Claude Opus 4.8
Summary by CodeRabbit
openspec-archive-changeandopsx:archiveworkflows to run spec synchronization inline and wait for it to complete before archiving.