CDM: don't trust a FocusKick interrupt this character can't cast - #1194
Merged
EllesmereGaming merged 5 commits intoAug 5, 2026
Merged
Conversation
focusKickInterruptSpellID lives on the bar definition, which is profile-level, while the spell it names is per-spec spellbook content. Nothing reconciled the two, so a Ret Paladin's Rebuke rode the shared profile onto a Holy Paladin, who has no interrupt at all. The same leak produced the earlier report of a bare "47528" (Mind Freeze) in the dropdown. Two consequences, both fixed by validating on read: - The cast-sound handler took the stale id straight to its "is my kick ready" gate. A spell you do not know is never on cooldown, so the gate passed every time and the player was pinged to interrupt casts they cannot interrupt. - The options dropdown rendered the phantom pick as a real selection. Validated on read and never written back: the id is still correct for the spec that set it, so clearing it would destroy that spec's setting the first time the player logged in on another one. Pet-bank interrupts (Axe Toss, pet kicks) are legitimate picks that IsPlayerSpell cannot see, so both banks are checked. The check deliberately sits at fire time rather than at arming time. Arming runs during loading screens, when the spellbook reads empty for reasons unrelated to the player's spec, and folding "not loaded yet" into "cannot cast it" is the exact collapse that unregistered a working proxy in EllesmereGaming#1178.
Field report: the sound fires on a Holy Paladin. That only happens if the handler resolved an interrupt id, and it has two sources -- the explicit profile-level pick and the bar's assignedSpells. The previous commit only validated the first, so if Rebuke reached this character through the bar's spell list instead the sound still fired. assignedSpells is per-spec, but per-spec is not the same as castable: importing a shared profile with "Include CDM Spell Layout" writes another character's layout into these spec keys wholesale, and talent changes strand entries the same way. The rule is the same whichever list supplies the id -- a spell this character cannot cast is never on cooldown, so it reads as permanently ready to the gate that decides whether to play the sound.
…cast Hardening the same gate from the other side. The check now returns the castable id rather than a yes/no, and the caller uses what it returns. A talent swap moves an interrupt between its base and override forms while the stored id stays put. Answering "yes, known" off the base form but leaving the caller holding the un-castable stored id would feed the readiness gate a spell that is never on cooldown -- the same always-ready failure this branch is about, reached from the other direction. Walking to the override, then to the base, and handing back whichever one the spellbook actually has closes both. It also protects the feature it is guarding: without the base/override walk, a legitimate kick stored under the form the player is not currently talented into would read as "cannot cast" and silently kill the sound for someone who can kick. That is the regression risk of adding a spellbook check at all, and it is the worse of the two failures. Resolution stays spellbook-driven throughout. Which specs carry an interrupt is Blizzard's to change -- Preservation has Quell, Mistweaver has Spear Hand Strike, Resto Shaman has Wind Shear, most other healers have nothing -- and a hardcoded table of that goes stale on a patch.
…viour change The inner helper was a closure rebuilt on every call, and this runs on every cast start of the tracked unit. Lifted to ns.IsSpellInPlayerBook. Also trimmed the header comment down to the three things that are not obvious from the code: validate on read and never write, ask the spellbook rather than a spec table, and return the resolved id rather than a boolean. The Rebuke story it repeated is already at the call site.
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.
The bug
Reported on a Holy Paladin, which has no interrupt at all: the FocusKick bar showed Rebuke as its Interrupt Spell, and the focus cast sound fired for a spell the character cannot cast.
Root cause
focusKickInterruptSpellIDlives on the bar definition, which is profile-level and therefore shared by every spec and every character on that profile. The spell it names is per-spec spellbook content. Nothing reconciled the two.A Ret Paladin sets Rebuke, the profile carries it, and Holy reads it straight back. An earlier field report of a bare
47528in the same dropdown was a Death Knight's Mind Freeze doing exactly the same thing.The sound path made it worse rather than catching it. Its only guard was
C_Spell.GetSpellCooldown(spellID).isActive— and a spell you do not know is never on cooldown, so an unknown id looks permanently ready and the gate passes every time. An unknown spell is the worst possible input for a readiness check.Two changes in #1178 are why this surfaced now, though neither created it:
focusKickInterruptSpellIDarm the cast-sound proxy on its own, without the spell being on the bar, meant a spec that cannot kick now gets pinged.The fix
ns.ResolveCastableInterrupt(sid)returns the id this character can actually cast, or nil:IsPlayerSpell/C_SpellBook.IsSpellKnownOrInSpellBookFindSpellOverrideByID) — talented into a replacement, stored id is the baseC_Spell.GetBaseSpell) — talented back out, stored id is the replacementIsPlayerSpellcannot seeBoth readers of that id are now validated: the cast-sound handler resolves the explicit pick and otherwise falls through to the first castable spell on the bar, and the options dropdown only labels a stale id when it is castable, treating "no label" as "not a selection".
Three decisions worth stating
It returns the resolved id, not a boolean. The caller feeds it to a cooldown check, and a talent swap moves an interrupt between its base and override forms while the stored id stays put. Answering "yes, known" off the base form while leaving the caller holding the un-castable form would feed the readiness gate a spell that is never on cooldown — the same always-ready failure, reached from the other side. The base/override walk is also what stops the guard silencing a legitimate kick stored under the form the player is not currently talented into, which would regress the fix in #1178.
It validates on read and never writes back. The stored id is still correct for the spec that set it. Clearing it during a Holy login would silently destroy the Ret setting the first time the player switched characters. That applies to any "clean up the stale value" instinct on profile-level data shared across specs.
The check runs at fire time, not arming time. Arming happens during loading screens, when the spellbook reads empty for reasons unrelated to spec. Gating arming on
IsPlayerSpellwould fold "not loaded yet" into "cannot cast", reintroducing the teardown #1178 fixed. The cast handler only runs on a liveUNIT_SPELLCAST_START, by which point the spellbook has settled.It is also deliberately spellbook-driven rather than a class/spec table: which specs carry an interrupt is Blizzard's to change, and a hardcoded list goes stale on a patch. (Worth noting the common assumption that Resto Shaman is the only healer with a kick is not right either — Preservation has Quell, Mistweaver has Spear Hand Strike.)
Scope
Two files in the Cooldown Manager, +96/−15. No change to arming, no change to the nameplate/unit-frame kick tint (
EllesmereUI_Kick.luaresolves from the spellbook on everySPELLS_CHANGEDand was always correct here); only the CDM FocusKick bar had the profile-level leak.The branch also carries a separate housekeeping commit regenerating
Locales/_keys.txt, which was stale against main.Testing
Confirmed fixed in game by the reporter on a Holy Paladin: the stale interrupt no longer appears in the dropdown and the focus cast sound no longer fires for a spell the character cannot cast.