fix(templates): make archive sync guidance profile-agnostic when sync skill is uninstalled (#913) - #1443
Conversation
… skill is uninstalled (Fission-AI#913)
📝 WalkthroughWalkthroughArchive and bulk-archive instructions now support direct delta-spec merging when the sync workflow is unavailable, require completed synchronization and main-spec verification before archiving, and update parity tests for the changed templates and generated skills. ChangesArchive spec synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🤖 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 `@skills/openspec-archive-change/SKILL.md`:
- Line 75: Update the direct spec-sync instruction in
skills/openspec-archive-change/SKILL.md to use the store-aware
<planningHome.root>/openspec/specs/ path instead of the literal openspec/specs/
path. Apply the same change in the archive-change template at
src/core/templates/workflows/archive-change.ts, then regenerate the installed
skill from the updated template.
In `@skills/openspec-bulk-archive-change/SKILL.md`:
- Line 133: Bulk synchronization must execute, complete, and verify inline
before archiving. Update skills/openspec-bulk-archive-change/SKILL.md:133 to
require synchronous execution rather than merely invoking the
openspec-sync-specs approach; apply the same requirement in
getBulkArchiveChangeSkillTemplate() at
src/core/templates/workflows/bulk-archive-change.ts:135 and
getOpsxBulkArchiveCommandTemplate() at
src/core/templates/workflows/bulk-archive-change.ts:395, ensuring each workflow
waits for completion and verification before archive.
In `@src/core/templates/workflows/archive-change.ts`:
- Line 132: Update getOpsxArchiveCommandTemplate() so its instructions for
syncing specs match getArchiveChangeSkillTemplate(): run openspec-sync-specs
inline when available, otherwise perform the spec sync directly, and wait
synchronously for completion before continuing. Apply this consistently to both
existing sync guidance locations in the template.
🪄 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: 9e3427b8-1bf9-494c-9728-d5d2507f5b99
📒 Files selected for processing (5)
skills/openspec-archive-change/SKILL.mdskills/openspec-bulk-archive-change/SKILL.mdsrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tstest/core/templates/skill-templates-parity.test.ts
| - Anything else — ask again rather than archiving | ||
|
|
||
| To sync, run the `openspec-sync-specs` workflow inline (agent-driven intelligent merge) for change '<name>', passing the delta spec analysis from above, and wait for it to finish. Do not delegate it to a background task — step 5 would move `changeRoot` out from under a sync that is still reading it, leaving the change archived and the main specs never updated. If your agent can only run it by delegation, delegate synchronously and wait for the result. | ||
| To sync, run the `openspec-sync-specs` workflow inline (agent-driven intelligent merge) for change '<name>' (if `openspec-sync-specs` is not installed, perform the spec sync directly by merging delta specs into main specs under `openspec/specs/`), passing the delta spec analysis from above, and wait for it to finish. Do not delegate it to a background task — step 5 would move `changeRoot` out from under a sync that is still reading it, leaving the change archived and the main specs never updated. If your agent can only run it by delegation, delegate synchronously and wait for the result. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the store-aware main-spec root for direct sync.
skills/openspec-archive-change/SKILL.md#L75-L75: replace literalopenspec/specs/with the<planningHome.root>/openspec/specs/location established earlier.src/core/templates/workflows/archive-change.ts#L77-L77: update the source template accordingly, then regenerate the installed skill.
📍 Affects 2 files
skills/openspec-archive-change/SKILL.md#L75-L75(this comment)src/core/templates/workflows/archive-change.ts#L77-L77
🤖 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 `@skills/openspec-archive-change/SKILL.md` at line 75, Update the direct
spec-sync instruction in skills/openspec-archive-change/SKILL.md to use the
store-aware <planningHome.root>/openspec/specs/ path instead of the literal
openspec/specs/ path. Apply the same change in the archive-change template at
src/core/templates/workflows/archive-change.ts, then regenerate the installed
skill from the updated template.
Code review — verdict: needs rework before this can be consideredThe mechanics of this PR are clean. My concern is the approach itself, and I think it introduces more risk than the bug it fixes. Details below. 1. Real need — the issue is real, but it's deliberately parked#913 describes a genuine failure: However, on that issue @alfred-openspec wrote:
So this is intentionally held pending a packaging decision, and #913 is earmarked as the acceptance test for it. A stopgap landing now pre-empts that, and worse, it would make #913 look resolved while the packaging gap remains. I'd want maintainer sign-off on the approach before any version of this merges. 2. Does the solution work — this is where I have a substantive objectionThe fallback is a single parenthetical:
That instruction is standing in for
Two of those rules exist because they were shipped bug fixes:
Telling a core-profile agent to "merge delta specs into main specs" with none of that context is a direct invitation to reproduce both. And because core is the default profile, the blast radius is every default install — which runs against the "must not break a user of OpenSpec" bar for this review. There's also a mechanical gap: the instruction never says how the agent should determine whether 3. A deterministic path already exists and isn't used
This is Option B in the issue's own triage notes ("recommended"), and it's the option that cannot regress the format rules above, because the merge is code rather than prose. Routing the no-sync-skill case at the deterministic CLI would be both smaller and safer than asking an LLM to hand-merge. 4. Scope — fineGuidance-only, no architecture expanded. No complaint here. Mechanics — verified cleanCredit where due, the regeneration discipline is correct, which is the usual failure mode for template PRs:
What I'd suggest instead
Happy to re-review whichever direction you take. To be clear, the underlying report is worth fixing — it's the "let the agent merge it by hand" mechanism I can't sign off on. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The core-profile archive command path still invokes the missing sync skill, the direct fallback hardcodes repo-local openspec/specs, and bulk archive still lacks inline completion plus verification before moving each change. Please make the command and bulk paths root-aware and synchronous, with semantic regressions for those generated surfaces.
…nous, and verified
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/templates/workflows/bulk-archive-change.ts`:
- Around line 135-146: Define a complete, idempotent direct-merge contract
covering preservation of main-spec purpose, partial MODIFIED updates, RENAMED
FROM/TO handling, delta-header removal, and all ADDED/MODIFIED/REMOVED
semantics, then apply the same contract at
src/core/templates/workflows/bulk-archive-change.ts lines 135-146 and 406-417
and src/core/templates/workflows/archive-change.ts line 77. Regenerate
skills/openspec-bulk-archive-change/SKILL.md lines 133-144 from the updated
guidance, and extend test/core/templates/skill-templates-parity.test.ts lines
227-277 to assert these fallback semantics for every variant.
🪄 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: 74211531-b167-4f00-b1a2-42d5ea2cb4dd
📒 Files selected for processing (4)
skills/openspec-bulk-archive-change/SKILL.mdsrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/bulk-archive-change.tstest/core/templates/skill-templates-parity.test.ts
| - Run the \`openspec-sync-specs\` workflow inline (agent-driven intelligent merge, or merge delta specs directly into main specs at \`<planningHome.root>/openspec/specs/<capability>/spec.md\` if sync workflow is not installed) for each change, passing the delta spec analysis, and wait for it to finish. | ||
| - For conflicts, apply in resolved order. | ||
| - Do not delegate to a background task — step 8c would move \`changeRoot\` out from under a sync that is still reading it. | ||
|
|
||
| b. **Perform the archive**: | ||
| b. **Verify main specs before moving changeRoot**: | ||
| - Re-run the comparison against every capability that has a delta spec in \`artifactPaths.specs.existingOutputPaths\` against main spec at \`<planningHome.root>/openspec/specs/<capability>/spec.md\` (use the store-aware \`planningHome.root\` from step 3 status JSON, not a hardcoded repo path). | ||
| - 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 | ||
| - RENAMED requirements present under the new name and absent under the old one | ||
| - 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. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Define the complete direct-merge contract before using it as the fallback.
“Merge delta specs directly” omits preservation of main-spec purpose, partial MODIFIED application, RENAMED FROM/TO processing, delta-header removal, and idempotency. Verification covers only part of this, so a malformed main spec can be archived permanently.
src/core/templates/workflows/bulk-archive-change.ts#L135-L146: specify the complete merge semantics.src/core/templates/workflows/archive-change.ts#L77-L77: apply the same contract to single archive fallback.src/core/templates/workflows/bulk-archive-change.ts#L406-L417: apply it to the OPSX bulk fallback.skills/openspec-bulk-archive-change/SKILL.md#L133-L144: regenerate after updating source guidance.test/core/templates/skill-templates-parity.test.ts#L227-L277: assert the required fallback semantics for every variant.
📍 Affects 4 files
src/core/templates/workflows/bulk-archive-change.ts#L135-L146(this comment)src/core/templates/workflows/archive-change.ts#L77-L77src/core/templates/workflows/bulk-archive-change.ts#L406-L417skills/openspec-bulk-archive-change/SKILL.md#L133-L144test/core/templates/skill-templates-parity.test.ts#L227-L277
🤖 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/templates/workflows/bulk-archive-change.ts` around lines 135 - 146,
Define a complete, idempotent direct-merge contract covering preservation of
main-spec purpose, partial MODIFIED updates, RENAMED FROM/TO handling,
delta-header removal, and all ADDED/MODIFIED/REMOVED semantics, then apply the
same contract at src/core/templates/workflows/bulk-archive-change.ts lines
135-146 and 406-417 and src/core/templates/workflows/archive-change.ts line 77.
Regenerate skills/openspec-bulk-archive-change/SKILL.md lines 133-144 from the
updated guidance, and extend test/core/templates/skill-templates-parity.test.ts
lines 227-277 to assert these fallback semantics for every variant.
|
Thanks for the follow-up. Maintainer call: let's not add either a freehand or CLI fallback here; #913's exact core failure was fixed by #1030 and shipped in v1.4.0 (current v1.6.0 core installs |
3 participants
Linked Issue
Fixes #913
Observed Failure
Under the default
coreprofile (propose,explore,apply,archive),openspec-sync-specsis not installed on user projects. However, the archive and bulk-archive workflow instructions unconditionally directed the agent to runopenspec-sync-specswhen syncing delta specs during archiving. When an agent attempted to executeopenspec-sync-specson the core profile, execution failed with a missing-skill error.Root Cause
archive-change.tsandbulk-archive-change.tstemplates contained hardcoded instructions pointing atopenspec-sync-specswithout providing a fallback when thesyncskill/workflow is not installed on the active profile.Implementation
src/core/templates/workflows/archive-change.tsandsrc/core/templates/workflows/bulk-archive-change.tsto provide profile-agnostic guidance: whenopenspec-sync-specsis not installed, the agent is instructed to perform the spec sync directly by merging delta specs into main specs underopenspec/specs/.pnpm run generate:skills) and updated parity hashes (pnpm run regen:parity-hashes).Regression Coverage & Validation
pnpm test-> 112 test files passed (2253 tests), including all skill template parity tests.pnpm build-> Build completed cleanly.git diff --check-> Passed with 0 whitespace issues.AI Assistance Disclosure
This pull request was prepared with AI pair-programming assistance (Antigravity). All template edits, generated skills updates, and parity test validations were verified locally before submission.
Limitations or Untested Platforms
None.
Unrelated Changes
No unrelated changes were included.
Summary by CodeRabbit