fix(templates): stop the bulk archive when the user picks Cancel - #1398
Conversation
|
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 selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughBulk archive instructions now stop on “Cancel”, proceed for the two archive options, and re-prompt for other responses. Generated template hashes and parity tests are updated accordingly. ChangesBulk archive confirmation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
3c6c3e8 to
078a2f5
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The routing closes the destructive fall-through in both bulk templates: Cancel exits before sync or move, unknown responses re-prompt, and ready-only selection re-derives conflicts without skipped changes. All nine focused parity tests pass locally at 078a2f5 and the exact-head CI/CodeQL matrix is green, so this is approved.
The bulk archive confirmation offered a "Cancel" option but never told the agent what to do with it. Step 8 then archived every selected change, so an agent following the skill literally moved the changes even after the user cancelled. Route each answer by intent rather than by literal label: the option labels are written by the agent and carry an `N` placeholder, so matching them verbatim would send every legitimate answer down the "ask again" path. Cancel now stops without archiving and skips the remaining steps, the ready-only option is bound to the status table that decides what "ready" means (re-deriving conflict resolutions when a Ready* partner is skipped), and a guardrail repeats that a cancelled batch archives nothing. Regression tests cover both archive paths, so the single-change routing can no longer be silently reverted either. Instruction text only — no CLI behavior changes. Closes #1381 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
078a2f5 to
bbed2a3
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The rebase preserves the reviewed Cancel routing and cleanly composes with the current archive-path guidance. Exact-head build, all 10 focused parity tests, and the full CI/CodeQL matrix pass.
2 participants
Status: LGTM — ready for review.
What was wrong
The generated
openspec-bulk-archive-changeskill (and the matchingopsx:bulk-archivecommand) offered you a Cancel option at the confirmation prompt:Step 7 collected your answer, but nothing ever told the agent what to do with it. Step 8 followed immediately and archived every selected change. An agent reading the skill literally would
mvyour changes intoarchive/after you explicitly cancelled — and the only trace of the option was a vague "Skipped: user chose not to archive (if applicable)" bullet three steps later, after the move already happened.The single-change archive skill had the same hole; it was fixed in #1357 (
46a4d78) and reworded into its current routing block by #1394. The bulk variant was left behind. Neither has shipped yet — both are post-v1.6.0.How it was fixed
Both template bodies (skill and
opsxcommand) now route the answer:Four deliberate details, each from an adversarial review finding:
Nplaceholder the agent fills in ("Archive all 3 changes"). Matching the literal strings would have sent every legitimate answer into "Anything else — ask again", trading a destructive bug for an infinite re-prompt.Ready*row. Otherwise the agent can't tell which subset to archive."Cancel" — stop, do not archive), so one assertion guards all four templates — and "skip the remaining steps" also rules out printing a "Bulk Archive Complete" summary after a cancel.A guardrail line was added where agents skim:
Never archive after the user cancels the confirmation — a cancelled batch archives nothing.Proof it works
Two regression tests in
test/core/templates/skill-templates-parity.test.ts:honors Cancel at every archive confirmation (#1381)— covers all four templates (bulk skill, bulk command, single skill, single command). The single-change routing has been correct since feat(skills): publish workflow skills to skills.sh #1357 but was never pinned by a test; this closes the whole issue rather than half of it.routes the bulk archive confirmation by intent, not by literal label (#1381)— pins the intent-based wording, the ready-only binding, and the guardrail.Both verified failing-then-passing. With the template change reverted:
Full suite after
npm run build: 2029 passed / 2046, 104 of 105 files green. The sole failing file istest/core/completions/installers/zsh-installer.test.ts(17 failures) — the known environment-only oh-my-zsh issue noted inCLAUDE.mdand tracked in #1321, untouched by this branch and green in CI.Notes / nits
getBulkArchiveChangeSkillTemplate,getOpsxBulkArchiveCommandTemplate,openspec-bulk-archive-change), plus the committedskills/openspec-bulk-archive-change/SKILL.mdvianpm run generate:skills. No other hash drifted.Proceed to archive regardless of choice.inarchive-change.ts. Because it edits those same lines, git will raise a conflict rather than silently revert — but resolving that conflict in feat: support multi-level change domains and sibling archives #1367's favor would reopen Generated openspec-archive-change skill archives even when the user selects Cancel #1381. The new four-template test now fails loudly if that happens.Closes #1381
🤖 Generated with Claude Code
Summary by CodeRabbit
Ready/Ready*.Ready*conflicts whose partners are skipped, conflict resolution is re-derived from the archived set.