Make refactor-break-bw-compat fully aggressive: demolish, never preserve#1
Conversation
Rewrite the skill from a cautious "decide whether to break" posture into a scorched-earth doctrine: breaking is the default verdict, total migration is mandatory, and any preservation of the old shape is the failure state. - Drop the "Decision: Break or Not?" matrix entirely — the "Do NOT Break" column (external consumers, missing tests, unclear ownership, in-progress migration) was the hedge the new doctrine forbids. - Replace cautious principles with a 6-point demolition doctrine: break first, no adapters, total migration, callers are not sacred, delete never disable, old tests are old assumptions. - Reframe blast-radius mapping as a demolition manifest, not a veto list; legacy-behavior tests get rewritten or deleted rather than written-first to gate removal. - Recast targets, anti-patterns, gates, and exit codes around zero residue and the half-migrated state as the single worst outcome. Skill behavior change → patch bump on plugin.json and marketplace.json. Op: extend
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request bumps the version of the ODIN marketplace and core plugin to 1.15.42 and significantly rewrites the refactor-break-bw-compat skill documentation to adopt an aggressive, "scorched earth" doctrine for removing backward compatibility and legacy layers. The review feedback focuses on improving the clarity of the updated markdown file, suggesting a fix for a typo ("leaves on" to "is migrated to") and recommending more explicit phrasing ("new/modernized branch/path" instead of "winner") to prevent AI agents from mistakenly preserving legacy code paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ## Reconnaissance Checklist | ||
| 1. **Break first, apologize never.** Do not ask whether a thing can be broken. Assume it must be. The default verdict is demolition. | ||
| 2. **One shape, no adapters.** The new design owns the entire surface. Never write new-to-old bridges, fallbacks, or translation layers — they resurrect the corpse you came to bury. | ||
| 3. **Total migration or nothing.** A half-migrated codebase is the only true failure state. If you touch a subsystem, every caller leaves on the new API before you stop. |
There was a problem hiding this comment.
The phrase 'leaves on' appears to be a typo for 'is migrated to' or 'lives on'. Clarifying this improves readability and ensures the instruction is unambiguous for the AI agent.
| 3. **Total migration or nothing.** A half-migrated codebase is the only true failure state. If you touch a subsystem, every caller leaves on the new API before you stop. | |
| 3. **Total migration or nothing.** A half-migrated codebase is the only true failure state. If you touch a subsystem, every caller is migrated to the new API before you stop. |
| 2. **One shape, no adapters.** The new design owns the entire surface. Never write new-to-old bridges, fallbacks, or translation layers — they resurrect the corpse you came to bury. | ||
| 3. **Total migration or nothing.** A half-migrated codebase is the only true failure state. If you touch a subsystem, every caller leaves on the new API before you stop. | ||
| 4. **Callers are not sacred.** "Unknown external consumers" is not a reason to preserve — it is a reason to rewrite their contract and let them adapt. You change the interface; the world changes with it. | ||
| 5. **Delete, never disable.** Flags, gates, version checks, `if legacy:` — collapse to the winning branch and erase the fork entirely. Version control is the only museum. |
There was a problem hiding this comment.
To prevent the AI agent from mistakenly selecting the legacy path as the 'winner' (e.g., if the legacy path is currently the default or has more active callers), explicitly specify that the 'winning branch' is the new/modernized behavior.
| 5. **Delete, never disable.** Flags, gates, version checks, `if legacy:` — collapse to the winning branch and erase the fork entirely. Version control is the only museum. | |
| 5. **Delete, never disable.** Flags, gates, version checks, 'if legacy:' — collapse to the new/modernized branch and erase the legacy fork entirely. Version control is the only museum. |
| - Version-gated paths (`if version >= X`, `#[cfg(feature = "legacy")]`, capability checks) — collapse to the new branch. | ||
| - Adapter / shim / bridge / wrapper / compat layers — delete wholesale, rewrite callers against the real interface. | ||
| - Dual serialization formats (v1/v2 JSON, protobuf legacy fields, `oneof` fallbacks) — keep one format, drop the reader/writer for the rest. | ||
| - Feature flags and config toggles that select old vs new — delete the flag, hardwire the winner. |
There was a problem hiding this comment.
Clarify 'hardwire the winner' to explicitly specify hardwiring the new/modernized behavior to avoid any ambiguity for the agent.
| - Feature flags and config toggles that select old vs new — delete the flag, hardwire the winner. | |
| - Feature flags and config toggles that select old vs new — delete the flag, hardwire the new/modernized path. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e2a1b525e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --- | ||
| name: refactor-break-bw-compat | ||
| description: "Refactor by removing backward compatibility and legacy layers. Use when modernizing APIs, cleaning up migration debt, removing compat shims, or eliminating stale feature flags." | ||
| description: "Aggressively refactor by tearing out all backward-compatibility and legacy layers. Use when modernizing APIs, demolishing migration debt, ripping out compat shims, killing feature flags, or rewriting a subsystem clean. Assume no callers deserve protection — break everything, rebuild it right." |
There was a problem hiding this comment.
When this skill auto-triggers from its frontmatter cues, the broadened Use when modernizing APIs ... or rewriting a subsystem clean wording can load the new scorched-earth doctrine for ordinary modernization or rewrite requests that did not explicitly ask to remove compatibility. README.md documents that many skills trigger from natural-language frontmatter, and this skill has no disable-model-invocation: true, so a vague API cleanup can now route into instructions to break public callers and delete legacy contracts; either make this explicit-only or restrict the description to requests that clearly ask for a breaking refactor.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
skills/refactor-break-bw-compat/SKILL.md (1)
66-73: ⚡ Quick winUnify exit-code taxonomy with other skills to avoid automation drift.
This file uses
0..3, while other skill docs in-repo use a reserved failure range (e.g.,11+). If tooling parses exit codes across skills, this inconsistency can cause ambiguous handling. Consider standardizing the numbering scheme repo-wide (or documenting why this skill is intentionally different).🤖 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/refactor-break-bw-compat/SKILL.md` around lines 66 - 73, The Exit Codes table currently uses codes 0–3 (entries labelled "0", "1", "2", "3") which is inconsistent with other skills that reserve a failure range (e.g., 11+); update the "Exit Codes" section in SKILL.md to match the repo-wide taxonomy by renumbering failure codes into the reserved range (for example map residue/build/migration failures to 11, 12, 13 or follow the established pattern), and add a one-line note explaining alignment with the global exit-code scheme; alternatively, if this skill intentionally diverges, add a brief justification comment in the same "Exit Codes" section stating why it uses 0–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/refactor-break-bw-compat/SKILL.md`:
- Around line 10-19: Update the "Doctrine" section in SKILL.md to add a clear
exception for stateful/schema/protocol changes: keep the existing
demolition-first rules (items 1–6) but append a required safety carve-out that
mandates an explicit migration plan, backfill strategy, and
rollback/compatibility track whenever a change crosses persisted or externally
consumed boundaries (DB schemas, event/message formats, queues, public
contracts); reference the Doctrine header and the numbered rules so readers know
this overrides them for those specific cases and mirror the same addition for
the corresponding paragraph referenced (the block also present later at the same
spot as noted).
---
Nitpick comments:
In `@skills/refactor-break-bw-compat/SKILL.md`:
- Around line 66-73: The Exit Codes table currently uses codes 0–3 (entries
labelled "0", "1", "2", "3") which is inconsistent with other skills that
reserve a failure range (e.g., 11+); update the "Exit Codes" section in SKILL.md
to match the repo-wide taxonomy by renumbering failure codes into the reserved
range (for example map residue/build/migration failures to 11, 12, 13 or follow
the established pattern), and add a one-line note explaining alignment with the
global exit-code scheme; alternatively, if this skill intentionally diverges,
add a brief justification comment in the same "Exit Codes" section stating why
it uses 0–3.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f8abd72-7b73-4a16-a226-bcf2522b2e08
📒 Files selected for processing (3)
.claude-plugin/marketplace.json.claude-plugin/plugin.jsonskills/refactor-break-bw-compat/SKILL.md
| There is no migration plan. There is no deprecation window. There is the old shape, and there is the right shape, and the only acceptable end state is the right shape with zero residue of the old. | ||
|
|
||
| 1. **No half-measures.** A partial migration is worse than no migration — it doubles the surface area and confuses every reader. | ||
| 2. **One migration direction.** Old-to-new only. Never add new-to-old adapters; that entrenches the old path. | ||
| 3. **Blast radius awareness.** Map every consumer before removing anything. Surprise breakage is a planning failure, not a courage signal. | ||
| 4. **Dead code is a lie.** "Just in case" code is not dead — it actively misleads readers about what the system does. | ||
| 5. **Compat shims are temporary.** If a shim has no removal date, it is permanent. If it is permanent, it is architecture. Decide which. | ||
| ## Doctrine | ||
|
|
||
| ## Reconnaissance Checklist | ||
| 1. **Break first, apologize never.** Do not ask whether a thing can be broken. Assume it must be. The default verdict is demolition. | ||
| 2. **One shape, no adapters.** The new design owns the entire surface. Never write new-to-old bridges, fallbacks, or translation layers — they resurrect the corpse you came to bury. | ||
| 3. **Total migration or nothing.** A half-migrated codebase is the only true failure state. If you touch a subsystem, every caller leaves on the new API before you stop. | ||
| 4. **Callers are not sacred.** "Unknown external consumers" is not a reason to preserve — it is a reason to rewrite their contract and let them adapt. You change the interface; the world changes with it. | ||
| 5. **Delete, never disable.** Flags, gates, version checks, `if legacy:` — collapse to the winning branch and erase the fork entirely. Version control is the only museum. | ||
| 6. **Old tests are old assumptions.** Tests that assert the legacy behavior encode the contract you are abolishing. Rewrite them to the new behavior or delete them. Do not let them veto the refactor. |
There was a problem hiding this comment.
Add a hard safety carve-out for stateful/schema/protocol migrations.
The current doctrine forbids migration planning outright, which is unsafe for persisted-data changes (DB schema, event formats, queues, external contracts). This can turn a refactor into data loss or prolonged outage. Keep demolition as default, but require an explicit migration/backfill/rollback track whenever the old/new shape crosses a persisted or externally consumed boundary.
Also applies to: 37-43
🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...n plan. There is no deprecation window. There is the old shape, and there is the righ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~10-~10: You have already used ‘right’ in nearby sentences. Consider using an alternative word to let your writing stand out and sound more polished.
Context: ...nd the only acceptable end state is the right shape with zero residue of the old. ##...
(REP_RIGHT)
[style] ~14-~14: The adverb ‘never’ is usually put before the verb ‘apologize’.
Context: ...he old. ## Doctrine 1. Break first, apologize never. Do not ask whether a thing can be br...
(ADVERB_WORD_ORDER)
🤖 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/refactor-break-bw-compat/SKILL.md` around lines 10 - 19, Update the
"Doctrine" section in SKILL.md to add a clear exception for
stateful/schema/protocol changes: keep the existing demolition-first rules
(items 1–6) but append a required safety carve-out that mandates an explicit
migration plan, backfill strategy, and rollback/compatibility track whenever a
change crosses persisted or externally consumed boundaries (DB schemas,
event/message formats, queues, public contracts); reference the Doctrine header
and the numbered rules so readers know this overrides them for those specific
cases and mirror the same addition for the corresponding paragraph referenced
(the block also present later at the same spot as noted).
There was a problem hiding this comment.
Pull request overview
This PR overhauls the refactor-break-bw-compat skill to shift from a cautious backward-compatibility removal guide to an explicitly aggressive, break-first doctrine, and bumps the plugin/marketplace patch versions to reflect the behavior change.
Changes:
- Rewrites
skills/refactor-break-bw-compat/SKILL.mdto a “scorched earth” breaking-refactor doctrine (targets, strategy, validation gates, and exit codes). - Removes the prior “Break or Not?” decision matrix and reframes blast-radius mapping as a demolition manifest.
- Bumps patch version
1.15.41 → 1.15.42in both plugin and marketplace manifests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| skills/refactor-break-bw-compat/SKILL.md | Replaces the skill’s guidance with an aggressive breaking-refactor doctrine, including updated gates and exit codes. |
| .claude-plugin/plugin.json | Patch version bump to reflect skill behavior change. |
| .claude-plugin/marketplace.json | Patch version bump (including embedded plugin version) to match the release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Backward compatibility is the tax the present pays to the past. This skill stops paying. Every compat shim, every legacy adapter, every "v1 still supported" branch is debt that compounds against the reader. Tear it all out. Break everything. Rebuild the system as if the old shape never existed. | ||
|
|
||
| ## Principles | ||
| There is no migration plan. There is no deprecation window. There is the old shape, and there is the right shape, and the only acceptable end state is the right shape with zero residue of the old. |
| 1. **Break first, apologize never.** Do not ask whether a thing can be broken. Assume it must be. The default verdict is demolition. | ||
| 2. **One shape, no adapters.** The new design owns the entire surface. Never write new-to-old bridges, fallbacks, or translation layers — they resurrect the corpse you came to bury. | ||
| 3. **Total migration or nothing.** A half-migrated codebase is the only true failure state. If you touch a subsystem, every caller leaves on the new API before you stop. | ||
| 4. **Callers are not sacred.** "Unknown external consumers" is not a reason to preserve — it is a reason to rewrite their contract and let them adapt. You change the interface; the world changes with it. |
What
Rewrites the
refactor-break-bw-compatskill from a cautious "decide whether it's safe to break" posture into a scorched-earth doctrine. Breaking is now the default verdict; total migration is mandatory; any preservation of the old shape is treated as the failure state.Why
The previous skill hedged: a "Decision: Break or Not?" matrix with a whole Do NOT Break column (external consumers, missing tests, unclear ownership, in-progress migration) plus "write tests first, then break" gating. That cautious framing is the opposite of an aggressive breaking-refactor tool — it talked the agent out of breaking things.
Changes
1.15.41 → 1.15.42onplugin.json+marketplace.json(skill behavior change).https://claude.ai/code/session_01CyaTX2N5M7APSfWBs9LNMV
Generated by Claude Code
Summary by CodeRabbit
Chores
Documentation