Item cache: hook response handler instead of racing the engine

`Item:CreateFromItemID(X):ContinueOnItemLoad(cb)` was leaving items
permanently uncached when pfQuest called it at addon-load /
PLAYER_ENTERING_WORLD against an empty WDB. Three intertwined bugs:

1. `GetItemInfoInstant` returned no values for uncached items, so
   `ItemMixin:GetItemID()` returned nil and `AddCallback(nil, cb)`
   crashed with "table index is nil".
2. `DoesItemExistByID` returned false for uncached items, which made
   `IsItemEmpty()` return true and erroneously gated `ContinueOnItemLoad`
   on cache state — modern semantic is "is this a valid ID", independent
   of cache.
3. Our `RequestLoadItemData` path eagerly called `CacheFetch` with our
   own callback at addon-load. That created a cache entry before the
   engine's natural inventory prefetch had a chance to run; the engine
   then saw the entry already existed and skipped its own SMSG, leaving
   the entry permanently pending. `GetItemInfo` retries didn't help —
   subsequent `_GetRecord` calls just appended callbacks to the dead
   list, no new query was sent.

Fixes:

- `GetItemInfoInstant` always pushes the itemID (echoed from input) and
  the remaining 6 fields when cached / nils when not. No cache touch.
- `DoesItemExistByID` returns `itemID > 0`. No cache touch.
- `RequestLoadItemData` tracks itemIDs in a pending set without touching
  the cache. We hook `FUN_ITEMSTATS_CACHE_RESPONSE` (the engine's
  `SMSG_ITEM_QUERY_SINGLE_RESPONSE` handler at `0x0055BDB0`) and sweep
  pending after the engine fills entries — observing the natural
  prefetch instead of racing it. `WorldTick` only handles escalation
  (after ~60 ticks, kick a query for items the engine doesn't prefetch
  itself, e.g. quest rewards) and the hard timeout.

Also fix the truthiness bug in modern (id, success) events:
`Event::Custom::Fire("%d%d", id, success)` was pushing `success` as a
Lua number — both 0 and 1 are truthy, so `if success then` fired the
success branch on failed loads. New `Event::Custom::FireIdSuccess`
pushes `1` for success and `nil` for failure (via the engine's
`lua_pushstring(NULL) → lua_pushnil` tail-jump). `ITEM_DATA_LOAD_RESULT`,
`GET_ITEM_INFO_RECEIVED`, and `QUEST_DATA_LOAD_RESULT` now all use this.

src/Offsets.h

@@ -1068,6 +1068,26 @@ enum Offsets {
     // With `requestIfMissing=false`, returns NULL if not in cache (no server
     // round-trip) — exactly the "instant" semantics we want.
     FUN_DBCACHE_ITEMSTATS_GET_RECORD = 0x0055BA30,
+    // Item-cache response handler — the function called from
+    // `0x00555140` (success path for `SMSG_ITEM_QUERY_SINGLE_RESPONSE`)
+    // and `0x00555160` (failure path). Parses one or more itemIDs out
+    // of the packet, looks up (or creates) the cache entry per ID,
+    // copies the item data block via `FUN_007c9640` to `entry+0x18`,
+    // sets `[entry+0x1F0]=1` (the loaded byte), then walks the entry's
+    // pending callback list at `[entry+0x1FC]` and invokes each with
+    // `(userData, success)`. High-bit-on itemID in the packet signals
+    // a per-item failure for the batch.
+    //
+    // We hook this to observe any item-cache fill the engine performs
+    // — covers the engine's natural inventory prefetch and any
+    // addon-triggered queries without having to register our own
+    // engine callbacks. Lets `RequestLoadItemData` be purely passive:
+    // we track itemIDs, the engine (or whoever) issues the query, we
+    // see the fill via this hook and fire `ITEM_DATA_LOAD_RESULT`.
+    //
+    // Signature: `__thiscall(void *cache, void *packetReader, int flag)`.
+    // `cache` is the same `VAR_ITEMDB_CACHE` we already use.
+    FUN_ITEMSTATS_CACHE_RESPONSE = 0x0055BDB0,
     // ItemStats_C field offsets we read. Full struct layout in
     // VanillaHelpers's `Game.h` (`struct ItemStats_C`); we only need these.
     OFF_ITEMSTATS_CLASS = 0x00,

src/event/Custom.h

@@ -74,6 +74,22 @@ inline void Fire(int eventID, const char *format, Args... args) {
     fn(eventID, format, args...);
 }
 
+// Convenience for the modern WoW `(id: number, success: bool)` event
+// shape (ITEM_DATA_LOAD_RESULT, GET_ITEM_INFO_RECEIVED,
+// QUEST_DATA_LOAD_RESULT, …). The engine's printf-style dispatcher has
+// no `%b`, so a naive `%d%d` with `0` for failure surfaces `arg2 = 0` —
+// truthy in Lua, breaking the canonical `if success then` branch. We
+// push `1` for success and `nil` for failure instead, leaning on the
+// engine's `lua_pushstring(L, NULL) → lua_pushnil` tail-jump so the
+// dispatcher itself emits the nil without any pre-staging. Lua handlers
+// can then use the natural idiom: nil is falsy, `1` is truthy.
+inline void FireIdSuccess(int eventID, int id, bool success) {
+    if (success)
+        Fire(eventID, "%d%d", id, 1);
+    else
+        Fire(eventID, "%d%s", id, static_cast<const char *>(nullptr));
+}
+
 // Internal: try to claim a slot for every reservation that's still
 // unclaimed (`slot < 0`). Called from the `Frame::RegisterEvent` hook
 // in DllMain — every time Lua calls `frame:RegisterEvent(...)`, we

src/item/Data.cpp

@@ -17,6 +17,7 @@
 #include "item/Arg.h"
 #include "item/Data.h"
 #include "item/Location.h"
+#include "tick/WorldTick.h"
 
 #include <cstdint>
 
@@ -52,16 +53,14 @@ static constexpr const char *kGetItemInfoReceived = "GET_ITEM_INFO_RECEIVED";
 static const Event::Custom::AutoReserve _reserveItemDataLoadResult{kItemDataLoadResult};
 static const Event::Custom::AutoReserve _reserveGetItemInfoReceived{kGetItemInfoReceived};
 
-static void FireGetItemInfoReceived(int itemID, int success) {
-    const int slot = Event::Custom::Lookup(kGetItemInfoReceived);
-    if (slot >= 0)
-        Event::Custom::Fire(slot, "%d%d", itemID, success);
+static void FireGetItemInfoReceived(int itemID, bool success) {
+    Event::Custom::FireIdSuccess(Event::Custom::Lookup(kGetItemInfoReceived),
+                                 itemID, success);
 }
 
-static void FireItemDataLoadResult(int itemID, int success) {
-    const int slot = Event::Custom::Lookup(kItemDataLoadResult);
-    if (slot >= 0)
-        Event::Custom::Fire(slot, "%d%d", itemID, success);
+static void FireItemDataLoadResult(int itemID, bool success) {
+    Event::Custom::FireIdSuccess(Event::Custom::Lookup(kItemDataLoadResult),
+                                 itemID, success);
 }
 
 // Engine callback for **implicit** cache fills (transparent warmup
@@ -76,16 +75,7 @@ static void FireItemDataLoadResult(int itemID, int success) {
 // callback would crash the engine, so stdcall is verified by induction.
 static void __stdcall ItemLoadCallback_Implicit(void *userData, int success) {
     const auto itemID = static_cast<int>(reinterpret_cast<uintptr_t>(userData));
-    FireGetItemInfoReceived(itemID, static_cast<int>(success != 0));
-}
-
-// Engine callback for **explicit** cache fills (player-side
-// `C_Item.RequestLoadItemData(ByID)` calls). Fires ITEM_DATA_LOAD_RESULT
-// only — the implicit GET_ITEM_INFO_RECEIVED path is for ambient cache
-// fills, not for callers who explicitly asked.
-static void __stdcall ItemLoadCallback_Explicit(void *userData, int success) {
-    const auto itemID = static_cast<int>(reinterpret_cast<uintptr_t>(userData));
-    FireItemDataLoadResult(itemID, static_cast<int>(success != 0));
+    FireGetItemInfoReceived(itemID, success != 0);
 }
 
 using ItemLoadCallback_t = void(__stdcall *)(void *userData, int success);
@@ -148,18 +138,143 @@ static int __fastcall Script_IsItemDataCached(void *L) {
     return 1;
 }
 
-// Explicit-request path: kicks off the cache fill via the explicit
-// callback (which fires ITEM_DATA_LOAD_RESULT). If the item is already
-// cached, the engine won't invoke our callback — synthesize the event
-// so addons get the same notification regardless of cache state,
-// matching modern `C_Item.RequestLoadItemData(ByID)` semantics.
-static void RequestAndMaybeNotify(uint32_t itemID) {
-    const bool wasCached = (CacheFetch(itemID, nullptr) != nullptr);
-    CacheFetch(itemID, &ItemLoadCallback_Explicit);
-    if (wasCached)
-        FireItemDataLoadResult(static_cast<int>(itemID), 1);
+// Pending explicit-request tracking. We deliberately do NOT eagerly
+// create cache entries when an addon calls `C_Item.RequestLoadItemData`
+// at addon-load / PLAYER_ENTERING_WORLD time — empirically (pfQuest),
+// pre-creating the entry blocks the engine's natural prefetch path
+// from issuing a fresh `SMSG_ITEM_QUERY_SINGLE`. The entry stays in a
+// permanently-pending state because nothing ever sends the query.
+//
+// Instead we hook the engine's item-cache response handler at
+// `FUN_ITEMSTATS_CACHE_RESPONSE` (the function that fills cache
+// entries on `SMSG_ITEM_QUERY_SINGLE_RESPONSE`). After the engine
+// finishes filling any entries and walking its own callback lists,
+// we sweep our pending set and fire `ITEM_DATA_LOAD_RESULT(itemID, 1)`
+// for any tracked items that now resolve. The engine's natural
+// prefetch (for player-inventory items) takes care of the actual
+// `SMSG_ITEM_QUERY_SINGLE` for us.
+//
+// For items the engine *doesn't* naturally prefetch (e.g. quest
+// reward IDs the player has never owned), we escalate after
+// `kEscalateTicks` by issuing our own `CacheFetch` with a no-op
+// callback — that creates the entry and queues the query, and the
+// response handler hook still fires our event when the data lands.
+// `kMaxWaitTicks` is the hard timeout that gives up and fires
+// `ITEM_DATA_LOAD_RESULT(itemID, nil)`.
+struct Pending {
+    uint32_t itemID;
+    int ticksWaiting;
+    bool requestIssued;
+};
+
+constexpr int kMaxPending = 64;
+constexpr int kEscalateTicks = 60;
+constexpr int kMaxWaitTicks = 1200;
+
+static Pending g_pending[kMaxPending];
+static int g_pendingCount = 0;
+
+static int FindPending(uint32_t itemID) {
+    for (int i = 0; i < g_pendingCount; ++i) {
+        if (g_pending[i].itemID == itemID)
+            return i;
+    }
+    return -1;
 }
 
+static void RemovePendingAt(int idx) {
+    --g_pendingCount;
+    if (idx != g_pendingCount)
+        g_pending[idx] = g_pending[g_pendingCount];
+}
+
+// No-op callback for the escalation path — we just need a non-null
+// callback pointer so the engine's `_GetRecord` queues the query.
+// The response handler hook will detect the fill and fire the event;
+// this callback doesn't need to do anything.
+static void __stdcall ItemLoadCallback_NoOp(void * /*userData*/, int /*success*/) {}
+
+static void RequestAndMaybeNotify(uint32_t itemID) {
+    // Already loaded — fire success synchronously.
+    if (CacheFetch(itemID, nullptr) != nullptr) {
+        FireItemDataLoadResult(static_cast<int>(itemID), true);
+        return;
+    }
+    // Already tracking — second request just dedups.
+    if (FindPending(itemID) >= 0)
+        return;
+    if (g_pendingCount >= kMaxPending)
+        return;
+    g_pending[g_pendingCount].itemID = itemID;
+    g_pending[g_pendingCount].ticksWaiting = 0;
+    g_pending[g_pendingCount].requestIssued = false;
+    ++g_pendingCount;
+}
+
+// Response handler hook. After the engine fills cache entries from
+// `SMSG_ITEM_QUERY_SINGLE_RESPONSE` and walks the per-entry callback
+// lists, we sweep `g_pending` for anything now-loaded and fire the
+// event. This is the engine-aligned alternative to per-tick polling:
+// we observe fills exactly when they happen, with zero added latency
+// and no risk of racing the engine's own callback walk.
+//
+// `__thiscall` is mimicked as `__fastcall(ecx, edx_unused, ...)` per
+// the convention used elsewhere in this codebase (see
+// `KeyringDescChange_h`, `FrameRegisterEvent_h`).
+using ItemStatsCacheResponse_t = void(__thiscall *)(void *cache, void *packetReader, int flag);
+static ItemStatsCacheResponse_t ItemStatsCacheResponse_o = nullptr;
+
+static void __fastcall ItemStatsCacheResponse_h(void *cache, void * /*edx_unused*/,
+                                                 void *packetReader, int flag) {
+    using Trampoline_t = void(__fastcall *)(void *cache, void *edx, void *packetReader, int flag);
+    reinterpret_cast<Trampoline_t>(ItemStatsCacheResponse_o)(cache, nullptr, packetReader, flag);
+    // Sweep pending — the engine just finished filling any matching
+    // entries. Peek (no callback) returns non-null iff the entry's
+    // loaded flag is set.
+    for (int i = 0; i < g_pendingCount;) {
+        const uint32_t itemID = g_pending[i].itemID;
+        if (CacheFetch(itemID, nullptr) != nullptr) {
+            RemovePendingAt(i);
+            FireItemDataLoadResult(static_cast<int>(itemID), true);
+            continue;
+        }
+        ++i;
+    }
+}
+
+static const Game::HookAutoRegister _hookCacheResponse{
+    Offsets::FUN_ITEMSTATS_CACHE_RESPONSE,
+    reinterpret_cast<void *>(&ItemStatsCacheResponse_h),
+    reinterpret_cast<void **>(&ItemStatsCacheResponse_o)};
+
+// Escalation + timeout. The hook covers the happy path (engine fills
+// cache → we fire). This tick is only responsible for:
+//   1. Issuing a query for items the engine doesn't naturally prefetch
+//      (e.g. quest rewards) — wait `kEscalateTicks` so we don't race
+//      the engine's startup-time state machine, then call CacheFetch
+//      with a no-op callback. The engine sends the SMSG, the response
+//      handler hook fires the event when the response lands.
+//   2. Giving up after `kMaxWaitTicks` for items that never resolve
+//      (bad itemID, server doesn't respond, etc.) — fire failure.
+static void OnWorldTick() {
+    for (int i = 0; i < g_pendingCount;) {
+        if (!g_pending[i].requestIssued && g_pending[i].ticksWaiting >= kEscalateTicks) {
+            CacheFetch(g_pending[i].itemID, &ItemLoadCallback_NoOp);
+            g_pending[i].requestIssued = true;
+        }
+        if (g_pending[i].ticksWaiting >= kMaxWaitTicks) {
+            const uint32_t itemID = g_pending[i].itemID;
+            RemovePendingAt(i);
+            FireItemDataLoadResult(static_cast<int>(itemID), false);
+            continue;
+        }
+        ++g_pending[i].ticksWaiting;
+        ++i;
+    }
+}
+
+static const Tick::WorldTick::AutoSubscribe _tickSub{&OnWorldTick};
+
 // Public C++ API — see Item/Data.h. Implicit (transparent) warmup
 // path used by the `Script_GetItemInfo` hook and `SetItemByID`. Fires
 // the cache request only if the item isn't already cached; uses the

src/item/Exists.cpp

@@ -14,28 +14,12 @@
 #include "Game.h"
 #include "Offsets.h"
 #include "item/Arg.h"
-#include "item/Data.h"
 #include "item/Location.h"
 
 #include <cstdint>
 
 namespace Item::Exists {
 
-namespace {
-
-using GetItemRecord_t = const uint8_t *(__thiscall *)(void *cache, uint32_t itemID,
-                                                      const uint64_t *guid, void *callback,
-                                                      void *userData, int unused);
-
-const uint8_t *PeekItemRecord(uint32_t itemID) {
-    auto fn = reinterpret_cast<GetItemRecord_t>(Offsets::FUN_DBCACHE_ITEMSTATS_GET_RECORD);
-    auto *cache = reinterpret_cast<void *>(Offsets::VAR_ITEMDB_CACHE);
-    const uint64_t zeroGuid = 0;
-    return fn(cache, itemID, &zeroGuid, nullptr, nullptr, 0);
-}
-
-} // namespace
-
 // `C_Item.DoesItemExist(itemLocation)` — true iff the location resolves
 // to a populated inventory slot on the active player. Equipment-slot
 // locations check the character pane; bag-slot locations walk the
@@ -51,27 +35,32 @@ static int __fastcall Script_C_Item_DoesItemExist(void *L) {
     return 1;
 }
 
-// `C_Item.DoesItemExistByID(itemInfo)` — true iff the cache currently
-// has data for this itemID (i.e. `GetItemInfo` would return non-nil
-// right now). Same semantics as the modern function and the existing
-// Lua shim: cache miss → false, but we kick off the network query
-// so a subsequent call after `GET_ITEM_INFO_RECEIVED` will succeed.
+// `C_Item.DoesItemExistByID(itemInfo)` — true if `itemID` looks like a
+// valid item identifier. Modern WoW answers from the client-side item
+// database (Item-sparse.db2) and is independent of whether the network
+// cache has been warmed yet. 1.12 has no client-side DB, so we
+// optimistically accept any positive itemID; callers needing the
+// "is the cache populated?" check should use `IsItemDataCachedByID`.
+//
+// This split matters for `ItemMixin:IsItemEmpty` (FrameXML ItemUtil),
+// which uses `DoesItemExistByID` to gate `ContinueOnItemLoad` — the
+// whole point of `ContinueOnItemLoad` is to wait for an uncached item
+// to load, so the existence check must succeed for items that aren't
+// in the cache yet.
+//
+// We deliberately do NOT auto-warm the cache here. Existence is a pure
+// query about the ID, not a load request — and during early-login
+// (cold WDB, pfQuest's PLAYER_ENTERING_WORLD path) a spurious cache
+// touch can race the engine's natural inventory prefetch and leave the
+// item stuck. Callers wanting to actually load data should use
+// `RequestLoadItemDataByID`; passive readers (`GetItemInfo`,
+// `GetItemNameByID`) already warm on their own miss path.
 //
 // Accepts numeric itemID or `"item:NNN..."` string (including full
-// chat links). Returns false for any input we can't resolve to an
-// itemID; never raises.
+// chat links); never raises.
 static int __fastcall Script_C_Item_DoesItemExistByID(void *L) {
     const int itemID = Item::Arg::ResolveItemID(L, 1);
-    if (itemID <= 0) {
-        Game::Lua::PushBoolean(L, 0);
-        return 1;
-    }
-    if (PeekItemRecord(static_cast<uint32_t>(itemID)) == nullptr) {
-        Item::Data::WarmCache(static_cast<uint32_t>(itemID));
-        Game::Lua::PushBoolean(L, 0);
-        return 1;
-    }
-    Game::Lua::PushBoolean(L, 1);
+    Game::Lua::PushBoolean(L, itemID > 0);
     return 1;
 }
 

src/item/Info.cpp

@@ -93,13 +93,31 @@ static bool BuildIconPath(uint32_t displayInfoID, char *out, size_t outSize) {
     return true;
 }
 
+// `GetItemInfoInstant` is the modern "synchronous, never blocks" item
+// info call. In modern WoW the trailing 6 fields come from client-side
+// `Item-sparse.db2` and so the call always returns the full 7-tuple.
+// 1.12 has no client-side item DB — those fields live only in the
+// network-fed `DBCache_ItemStats`. We mirror the contract by:
+//   - always returning the itemID (echoed from input) so callers using
+//     `GetItemInfoInstant(link_or_id)` as an ID extractor never see nil
+//   - returning the remaining 6 fields from the cache when present,
+//     nil when not (preserving the 7-tuple shape)
+//   - not warming the cache; "Instant" implies no side effects. Callers
+//     that need the full info should use `GetItemInfo`, which already
+//     triggers the warmup via the `Script_GetItemInfo` hook.
 static int __fastcall Script_GetItemInfoInstant(void *L) {
     const int itemID = Item::Arg::ResolveItemID(L, 1);
     if (itemID <= 0)
         return 0;
+
+    Game::Lua::PushNumber(L, static_cast<double>(itemID));
+
     const uint8_t *record = FetchItemRecord(static_cast<uint32_t>(itemID));
-    if (record == nullptr)
-        return 0;
+    if (record == nullptr) {
+        for (int i = 0; i < 6; ++i)
+            Game::Lua::PushNil(L);
+        return 7;
+    }
 
     const uint32_t classID =
         *reinterpret_cast<const uint32_t *>(record + Offsets::OFF_ITEMSTATS_CLASS);
@@ -114,7 +132,6 @@ static int __fastcall Script_GetItemInfoInstant(void *L) {
     if (!BuildIconPath(displayInfoID, iconPath, sizeof(iconPath)))
         iconPath[0] = 0;
 
-    Game::Lua::PushNumber(L, static_cast<double>(itemID));
     Game::Lua::PushString(L, LookupItemClassName(classID));
     Game::Lua::PushString(L, LookupItemSubClassName(classID, subClassID));
     Game::Lua::PushString(L, LookupInvType(invType));

src/quest/Data.cpp

@@ -31,12 +31,10 @@ static const Event::Custom::AutoReserve _reserveQuestDataLoadResult{kQuestDataLo
 // `ret 8` cleans both args. We encode the questID into `userData` at
 // request time so the callback knows which quest completed.
 static void __stdcall QuestLoadCallback(void *userData, int success) {
-    const int slot = Event::Custom::Lookup(kQuestDataLoadResult);
-    if (slot < 0)
-        return;
     const auto questID =
         static_cast<int>(reinterpret_cast<uintptr_t>(userData));
-    Event::Custom::Fire(slot, "%d%d", questID, static_cast<int>(success != 0));
+    Event::Custom::FireIdSuccess(Event::Custom::Lookup(kQuestDataLoadResult),
+                                 questID, success != 0);
 }
 
 static void RequestAndMaybeNotify(uint32_t questID) {
@@ -47,9 +45,8 @@ static void RequestAndMaybeNotify(uint32_t questID) {
     // invoking our callback. Fire the event ourselves so addons get the
     // notification regardless of cache state, matching modern semantics.
     if (wasCached) {
-        const int slot = Event::Custom::Lookup(kQuestDataLoadResult);
-        if (slot >= 0)
-            Event::Custom::Fire(slot, "%d%d", static_cast<int>(questID), 1);
+        Event::Custom::FireIdSuccess(Event::Custom::Lookup(kQuestDataLoadResult),
+                                     static_cast<int>(questID), true);
     }
 }
 

src/spell/AtCursor.cpp

@@ -41,9 +41,11 @@
 
 #include "Game.h"
 #include "Offsets.h"
+#include "spell/MacroPrimarySpell.h"
 
 #include <cstdint>
 #include <cstdio>
+#include <cstring>
 
 namespace Spell::AtCursor {
 
@@ -170,6 +172,28 @@ void RegisterLuaFunctions() {
 
 const Game::ModuleAutoRegister _autoreg{&RegisterLuaFunctions};
 
+// Macro-parser pattern: tag macros that call
+// `C_Spell.CastAtCursor(<spellID>)` with the spellID, so action-bar
+// UIs highlight the slot the same way they do for `/cast SpellName`.
+// The Lua call only accepts a numeric spellID, so the extractor just
+// reads a numeric literal — no name resolution needed.
+int ExtractCastAtCursorArg(const char *p, const char *end) {
+    while (p < end && (*p == ' ' || *p == '\t'))
+        ++p;
+    int value = 0;
+    const char *digitStart = p;
+    while (p < end && *p >= '0' && *p <= '9') {
+        if (value > 100000000) // sanity cap before overflow
+            return 0;
+        value = value * 10 + (*p - '0');
+        ++p;
+    }
+    return (p > digitStart && value > 0) ? value : 0;
+}
+
+const Spell::MacroPrimarySpell::PatternAutoRegister _patreg{
+    "C_Spell.CastAtCursor(", &ExtractCastAtCursorArg};
+
 } // namespace
 
 } // namespace Spell::AtCursor

src/spell/CastNoToggle.cpp

@@ -42,6 +42,7 @@
 #include "Offsets.h"
 #include "spell/Arg.h"
 #include "spell/Lookup.h"
+#include "spell/MacroPrimarySpell.h"
 
 #include <cstdint>
 #include <cstdio>
@@ -208,141 +209,37 @@ void RegisterLuaFunctions() {
 const Game::ModuleAutoRegister _autoreg{&RegisterLuaFunctions};
 
 // ---------------------------------------------------------------------------
-// Macro parser hook
+// Macro parser pattern
 //
-// Without this hook, a macro whose only cast statement is
-// `CastSpellNoToggle("Shoot")` doesn't get tagged in the spell-state
-// cache with the resolved spellID, because the engine's parser
-// (`FUN_004EFE00`) only knows to look for `/cast` slash-aliases and
-// `CastSpellByName("...")`. `IsAutoRepeatAction(macroSlot)` then has
-// nothing to match against the active auto-repeat spell, so the slot
-// fails to highlight in action-bar UIs (pfUI etc.).
-//
-// We hook the parser, let the original run, and if it didn't recognize
-// any pattern (cache field stays 0), scan the body ourselves for our
-// function name and use the engine's own name→spellID resolver to
-// fill in the field.
+// Tags macros containing `CastSpellNoToggle("Name")` with the
+// resolved spellID, so `IsAutoRepeatAction(macroSlot)` reports
+// correctly and action-bar UIs (pfUI etc.) highlight the slot for
+// auto-repeat spells. `Spell::MacroPrimarySpell` owns the parser
+// hook and the line walk; we just declare our `(prefix, extractor)`.
 // ---------------------------------------------------------------------------
 
-using MacroParse_t = void(__fastcall *)(int macroEntry);
-MacroParse_t MacroParse_o = nullptr;
-
-
-// Scan a macro body for `CastSpellNoToggle("<name>")` and return the
-// resolved spellID, or 0 if no recognized invocation is present.
-// Mirrors the engine parser's "first hit wins, line-at-a-time" walk
-// order so behavior is consistent with how `/cast` and
-// `CastSpellByName` are matched.
-int ScanMacroBodyForOurPattern(const char *body) {
-    static constexpr char kPattern[] = "CastSpellNoToggle(";
-    static constexpr size_t kPatternLen = sizeof(kPattern) - 1;
-
-    const char *line = body;
-    while (*line != '\0') {
-        const char *eol = line;
-        while (*eol != '\0' && *eol != '\n')
-            ++eol;
-
-        const char *p = line;
-        const char *end = eol;
-        while (p + kPatternLen <= end) {
-            if (std::memcmp(p, kPattern, kPatternLen) == 0) {
-                p += kPatternLen;
-                while (p < end && (*p == ' ' || *p == '\t'))
-                    ++p;
-                if (p < end && *p == '"') {
-                    ++p;
-                    const char *q = p;
-                    while (q < end && *q != '"')
-                        ++q;
-                    if (q < end) {
-                        const size_t n = static_cast<size_t>(q - p);
-                        if (n > 0 && n < 128) {
-                            char name[128];
-                            std::memcpy(name, p, n);
-                            name[n] = '\0';
-                            const int spellID = Spell::Arg::NameToSpellID(name);
-                            if (spellID > 0)
-                                return spellID;
-                        }
-                    }
-                }
-                break; // matched our prefix on this line; don't double-match
-            }
-            ++p;
-        }
-
-        line = (*eol == '\n') ? eol + 1 : eol;
-    }
-    return 0;
+int ExtractCastSpellNoToggleArg(const char *p, const char *end) {
+    while (p < end && (*p == ' ' || *p == '\t'))
+        ++p;
+    if (p >= end || *p != '"')
+        return 0;
+    ++p;
+    const char *q = p;
+    while (q < end && *q != '"')
+        ++q;
+    if (q >= end)
+        return 0;
+    const size_t n = static_cast<size_t>(q - p);
+    if (n == 0 || n >= 128)
+        return 0;
+    char name[128];
+    std::memcpy(name, p, n);
+    name[n] = '\0';
+    return Spell::Arg::NameToSpellID(name);
 }
 
-// The vanilla parser's per-line stack buffer is 256 bytes. When a
-// macro line exceeds that, the parser's local at `[EBP-4]` (the saved
-// entry pointer) and the saved return address get clobbered with body
-// bytes — observed in a crash with `entry@[EBP-4]` reading as ASCII
-// `"onTi"` (fragment of `"expirationTime"` from a long `/run` line).
-// The bound-check on the line-read function reads correctly in
-// Ghidra, so the overflow path is somewhere we couldn't isolate from
-// static analysis — we settle for a conservative pre-check that
-// skips the original entirely when any line is long enough to risk
-// the bug.
-//
-// 240 (not 256) leaves a safety margin: the parser writes a NUL
-// terminator and may walk one or two bytes past in its loop logic,
-// so we keep clear of the boundary. Macros that hit this branch
-// can't have a `/cast` line anyway (`/cast SpellName(Rank N)` is
-// well under 100 bytes), so losing the engine's `/cast` /
-// `CastSpellByName` tagging is no loss. We still run our own
-// `CastSpellNoToggle` pattern scan since it bound-checks its own
-// extraction.
-bool BodyHasLongLine(const char *body) {
-    if (body == nullptr)
-        return false;
-    constexpr int kMaxSafeLineBytes = 240;
-    int lineLen = 0;
-    for (; *body != '\0'; ++body) {
-        if (*body == '\n') {
-            lineLen = 0;
-        } else if (++lineLen > kMaxSafeLineBytes) {
-            return true;
-        }
-    }
-    return false;
-}
-
-void __fastcall MacroParse_h(int macroEntry) {
-    const char *body = reinterpret_cast<const char *>(
-        macroEntry + Offsets::OFF_MACRO_BODY);
-    int *cacheField = reinterpret_cast<int *>(
-        macroEntry + Offsets::OFF_MACRO_PRIMARY_SPELL);
-
-    if (BodyHasLongLine(body)) {
-        // Skip the engine parser entirely — it overflows on lines
-        // longer than its 256-byte buffer. Mark the cache as "no
-        // recognized cast" (0); our own scan below may still tag it
-        // if it's a `CastSpellNoToggle("...")` macro.
-        *cacheField = 0;
-        const int spellID = ScanMacroBodyForOurPattern(body);
-        if (spellID > 0)
-            *cacheField = spellID;
-        return;
-    }
-
-    MacroParse_o(macroEntry);
-
-    if (*cacheField != 0)
-        return;
-
-    const int spellID = ScanMacroBodyForOurPattern(body);
-    if (spellID > 0)
-        *cacheField = spellID;
-}
-
-const Game::HookAutoRegister _hookreg{
-    Offsets::FUN_MACRO_PARSE_PRIMARY_SPELL,
-    reinterpret_cast<void *>(&MacroParse_h),
-    reinterpret_cast<void **>(&MacroParse_o)};
+const Spell::MacroPrimarySpell::PatternAutoRegister _patreg{
+    "CastSpellNoToggle(", &ExtractCastSpellNoToggleArg};
 
 } // namespace
 

src/spell/MacroPrimarySpell.cpp

@@ -0,0 +1,145 @@
+// This file is part of ClassicAPI.
+//
+// ClassicAPI is free software: you can redistribute it and/or modify it under the terms
+// of the GNU Lesser General Public License as published by the Free Software Foundation, either
+// version 3 of the License, or (at your option) any later version.
+//
+// ClassicAPI is distributed in the hope that it will be useful, but WITHOUT ANY
+// WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+// PURPOSE. See the GNU Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License along with
+// ClassicAPI. If not, see <https://www.gnu.org/licenses/>.
+
+// Owns the hook on `FUN_MACRO_PARSE_PRIMARY_SPELL` and dispatches to
+// feature-module-registered `(prefix, extractor)` pairs. The engine's
+// parser only recognizes `/cast SpellName` and `CastSpellByName(...)`
+// — addons that define new cast wrappers (`CastSpellNoToggle`,
+// `C_Spell.CastAtCursor`, etc.) need their macro slots tagged with
+// the resolved spellID too, otherwise action-bar UIs lose
+// auto-repeat highlight and cooldown overlay.
+//
+// Each feature module owns an extractor that knows how to read the
+// arg shape for its specific function (quoted string, numeric
+// literal, etc.). Patterns auto-register at static-init via
+// `PatternAutoRegister`. The hook walks each macro line ONCE,
+// checking every registered prefix at every position; on a prefix
+// match it calls the extractor to resolve a spellID.
+//
+// Long-line overflow guard: the vanilla parser has a 256-byte
+// per-line stack buffer that overflows on `/run` lines longer than
+// that. We pre-check and skip the original entirely when any line
+// exceeds the safe threshold — the original's tagging is lost in
+// that case (no `/cast` on a 240+ byte line anyway) but our
+// patterns still run since they bound-check their own extraction.
+
+#include "MacroPrimarySpell.h"
+
+#include "Game.h"
+#include "Offsets.h"
+
+#include <cstdint>
+#include <cstring>
+
+namespace Spell::MacroPrimarySpell {
+
+namespace {
+
+PatternAutoRegister *g_patternHead = nullptr;
+
+using MacroParse_t = void(__fastcall *)(int macroEntry);
+MacroParse_t MacroParse_o = nullptr;
+
+// Vanilla's parser uses a 256-byte stack buffer per line; bytes past
+// the buffer clobber the saved entry pointer at `[EBP-4]` and the
+// return address. 240 leaves a safety margin.
+constexpr int kMaxSafeLineBytes = 240;
+
+bool BodyHasLongLine(const char *body) {
+    if (body == nullptr)
+        return false;
+    int lineLen = 0;
+    for (; *body != '\0'; ++body) {
+        if (*body == '\n') {
+            lineLen = 0;
+        } else if (++lineLen > kMaxSafeLineBytes) {
+            return true;
+        }
+    }
+    return false;
+}
+
+// Walks each line once. At every byte position, checks every
+// registered prefix; on first match, invokes the extractor. First
+// extractor that returns a positive spellID wins for the whole macro
+// — matches the engine parser's "first hit, line-at-a-time" walk.
+int ScanBody(const char *body) {
+    if (body == nullptr)
+        return 0;
+    const char *line = body;
+    while (*line != '\0') {
+        const char *eol = line;
+        while (*eol != '\0' && *eol != '\n')
+            ++eol;
+
+        for (const char *p = line; p < eol; ++p) {
+            for (auto *node = g_patternHead; node != nullptr; node = node->next) {
+                if (p + node->prefixLen > eol)
+                    continue;
+                if (std::memcmp(p, node->prefix, node->prefixLen) != 0)
+                    continue;
+                const int spellID = node->extract(p + node->prefixLen, eol);
+                if (spellID > 0)
+                    return spellID;
+                // Prefix matched but extraction failed (malformed
+                // args). Don't keep checking other prefixes at this
+                // position — they couldn't possibly also match here.
+                break;
+            }
+        }
+
+        line = (*eol == '\n') ? eol + 1 : eol;
+    }
+    return 0;
+}
+
+void __fastcall MacroParse_h(int macroEntry) {
+    const char *body = reinterpret_cast<const char *>(
+        macroEntry + Offsets::OFF_MACRO_BODY);
+    int *cacheField = reinterpret_cast<int *>(
+        macroEntry + Offsets::OFF_MACRO_PRIMARY_SPELL);
+
+    if (BodyHasLongLine(body)) {
+        *cacheField = 0;
+        const int spellID = ScanBody(body);
+        if (spellID > 0)
+            *cacheField = spellID;
+        return;
+    }
+
+    MacroParse_o(macroEntry);
+
+    if (*cacheField != 0)
+        return;
+
+    const int spellID = ScanBody(body);
+    if (spellID > 0)
+        *cacheField = spellID;
+}
+
+const Game::HookAutoRegister _hookreg{
+    Offsets::FUN_MACRO_PARSE_PRIMARY_SPELL,
+    reinterpret_cast<void *>(&MacroParse_h),
+    reinterpret_cast<void **>(&MacroParse_o)};
+
+} // namespace
+
+PatternAutoRegister::PatternAutoRegister(const char *p, ArgExtractor e)
+    : prefix(p),
+      prefixLen(static_cast<unsigned>(std::strlen(p))),
+      extract(e),
+      next(g_patternHead) {
+    g_patternHead = this;
+}
+
+} // namespace Spell::MacroPrimarySpell

src/spell/MacroPrimarySpell.h

@@ -0,0 +1,44 @@
+// This file is part of ClassicAPI.
+//
+// ClassicAPI is free software: you can redistribute it and/or modify it under the terms
+// of the GNU Lesser General Public License as published by the Free Software Foundation, either
+// version 3 of the License, or (at your option) any later version.
+//
+// ClassicAPI is distributed in the hope that it will be useful, but WITHOUT ANY
+// WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+// PURPOSE. See the GNU Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License along with
+// ClassicAPI. If not, see <https://www.gnu.org/licenses/>.
+
+#pragma once
+
+namespace Spell::MacroPrimarySpell {
+
+// Parses the argument(s) immediately after a known function-call
+// prefix. `p` points to the first byte past the closing `(` of the
+// prefix (so for `CastSpellNoToggle("Shoot")`, `p` would be at the
+// `"`). `end` is end-of-line. Returns the resolved spellID, or 0 if
+// extraction failed (malformed args, unknown spell, etc.).
+using ArgExtractor = int(*)(const char *p, const char *end);
+
+// File-scope static auto-registers a `(prefix, extractor)` pair. Each
+// feature module that wants its Lua wrapper to tag macros declares:
+//
+//   static const Spell::MacroPrimarySpell::PatternAutoRegister _r{
+//       "CastSpellNoToggle(", &MyExtractor};
+//
+// The macro-parser hook walks every line once, checks each registered
+// prefix at each position, and on match invokes the extractor to
+// resolve a spellID. The resolved ID is written to the macro's
+// primary-spell cache field — drives auto-repeat highlight, cooldown
+// overlay, etc. in action-bar UIs (pfUI).
+struct PatternAutoRegister {
+    PatternAutoRegister(const char *prefix, ArgExtractor extract);
+    const char *prefix;
+    unsigned prefixLen;
+    ArgExtractor extract;
+    PatternAutoRegister *next;
+};
+
+} // namespace Spell::MacroPrimarySpell