the spell owns its state, and a malformed file is refused, not a crash

- the spell owns its lifecycle state: the in-flight cast, the local cancel, the cooldowns and the spell-modifier tables live in one module (2328)
- a malformed MPQ, DBC, M2 or WDT file is refused instead of crashing the client, nine read sites bounded
- a test that needs the install or the addon corpus fails instead of skipping when the data is there; the Bagnon render suite runs again (2329)
- the dev crash injector is armed on every platform, and no instrument writes a path relative to the working directory

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Sam
2026-09-23 12:08:33 +07:00
co-authored by Claude Opus 5.5
parent ac1084783a
commit c2d6f7c858
91 changed files with 2266 additions and 1813 deletions
@@ -16,53 +16,19 @@
//! The director saw the first (their `Bagnon_Core` loaded at startup, so `currentPlayer` was
//! captured nil). Fix only that and you get the second. So the tests below pin **both**, and the
//! headline is a quad count, not an error count: [`bagnon_draws_a_slot_for_every_bag_slot`] is
//! green only when 16 item-slot quads actually reach the render list.
//! green only when every live slot's quad actually reaches the render list — the backpack's 16
//! and, since the reference's own `ContainerFrame.lua` runs off the chain (1751), the keyring's 12
//! beside them (Bagnon's default bag set is `{-2, 0, 1, 2, 3, 4}`, `Bagnon.lua:25`).
//!
//! Nothing from the corpus is committed, and every test here skips cleanly on a machine without
//! it — the `ui_chat::ace_gate_tests` rule.
use std::path::{Path, PathBuf};
use std::path::Path;
use benilla_formats::addon_corpus_or_skip as corpus_or_skip;
use benilla_ui::script::{AddOnInfo, ContainerSlot, ContainerState, QuadContent, UiScript};
use benilla_ui::toc::Toc;
/// Where the vanilla addon corpus might be — `$BENILLA_ADDON_CORPUS`, else a sibling checkout
/// resolved from this crate's manifest (a pool worktree's cwd is not stable across tool calls).
fn corpus_candidates() -> Vec<PathBuf> {
let mut out = Vec::new();
if let Some(over) = std::env::var_os("BENILLA_ADDON_CORPUS") {
out.push(PathBuf::from(over));
}
let manifest = Path::new(env!("CARGO_MANIFEST_DIR"));
for up in [2usize, 3, 4] {
if let Some(root) = manifest.ancestors().nth(up) {
out.push(root.join("wow-addons-vanilla"));
}
}
out
}
/// The corpus root, or `None` — **a skip, never a failure**: it is third-party content that is
/// deliberately not in this repo.
fn corpus() -> Option<PathBuf> {
corpus_candidates().into_iter().find(|c| c.is_dir())
}
macro_rules! corpus_or_skip {
() => {
match corpus() {
Some(root) => root,
None => {
eprintln!(
"skipping: no vanilla addon corpus — looked in {:?} (set $BENILLA_ADDON_CORPUS)",
corpus_candidates()
);
return;
}
}
};
}
/// The corpus **and** a client install — for the tests whose subject is a global that comes from
/// the reference file this client SOURCES off the player's own patch chain rather than shipping
/// ([`super::reference_ui`], decision 1234).
@@ -177,6 +143,29 @@ fn roster() -> crate::char_select::Roster {
crate::char_select::Roster::with_pending_pick(vec![row], 7)
}
/// The `"player"` snapshot the self descriptor builds when it lands — what `ui_unit::feed_units`
/// pushes (its `"player"` leg) and fires `PLAYER_ENTERING_WORLD` off. It is the roster seat plus
/// the fields only the descriptor can say: the unit EXISTS, at the roster's level, with health
/// and a live object. [`super::seat_from_roster`] deliberately leaves those at nil/0 (decision
/// 2263: the reference's `UnitExists("player")` is nil and `UnitLevel("player")` is 0 before the
/// object exists), so the seat can no longer stand in for the descriptor the way it did from 1230
/// until then — a fixture that never pushes this stays in the pre-object window for good, which
/// is a state the real client is never in when a player opens their bags.
fn descriptor_snapshot(seat: &benilla_ui::script::UnitState) -> benilla_ui::script::UnitState {
benilla_ui::script::UnitState {
exists: true,
has_object: true,
is_connected: true,
guid: 7,
level: 60,
health: 4_000,
max_health: 4_000,
power_type: 1, // a Warrior's rage
max_power: 1_000,
..seat.clone()
}
}
/// A VM shaped like the director's session: our whole interface, their installed addons walked in
/// dependency order the way `addons::load_third_party` walks them, and a real backpack.
fn seat(root: &Path, seat: Seat) -> UiScript {
@@ -191,6 +180,10 @@ fn seat(root: &Path, seat: Seat) -> UiScript {
s.set_realm_name("Harness");
let player = super::seat_from_roster(&roster()).expect("a pending pick seats a player");
if seat == Seat::BeforeAddons {
// The real entry load seeds the player RECORD beside the seat (`lifecycle.rs`, decisions
// 2261/2263); this one push does both, by `set_unit`'s keep-in-step rule — a `"player"`
// push that carries a name seeds the record — so `UnitName("player")` is "Harness" at
// addon file scope, which is what sends Bagnon down its LIVE path.
s.set_unit("player", Some(player.clone()));
}
let failures = super::load_default_ui(&s);
@@ -239,12 +232,14 @@ fn seat(root: &Path, seat: Seat) -> UiScript {
for event in ["VARIABLES_LOADED", "PLAYER_LOGIN"] {
s.fire_event(event, Vec::new());
}
// The self descriptor's arrival: `ui_unit::feed_units` pushes the player and fires
// PLAYER_ENTERING_WORLD off it. In the `AfterAddons` arm this is the FIRST time the VM has
// ever had a `"player"` — which is the state this file exists to reproduce.
if seat == Seat::AfterAddons {
s.set_unit("player", Some(player));
}
// The self descriptor's arrival: `ui_unit::feed_units` pushes the snapshot the descriptor
// builds and fires PLAYER_ENTERING_WORLD off it — in BOTH arms, because that is the world
// every real Bagnon open happens in. It matters: the reference's `GetKeyRingSize`
// (`ContainerFrame.lua:773-786`) reads `UnitLevel("player")`, and a fixture left in the
// pre-object window (level 0, the roster seat's answer since 2263) sizes the keyring at 4
// where the level-60 session sizes it at 12. In the `AfterAddons` arm this is the FIRST time
// the VM has ever had a `"player"` — which is the state this file exists to reproduce.
s.set_unit("player", Some(descriptor_snapshot(&player)));
s.fire_event("PLAYER_ENTERING_WORLD", Vec::new());
for _ in 0..10 {
s.tick(0.1);
@@ -381,17 +376,24 @@ fn bagnon_draws_a_slot_for_every_bag_slot() {
.unwrap(),
"the window must be on screen"
);
// 28 = the backpack's 16 + the keyring's 12. Bagnon's default bag set carries the keyring
// (`Bagnon.lua:25`, `{-2, 0, 1, 2, 3, 4}`), and its size is the reference's own
// `GetKeyRingSize` ladder off `UnitLevel("player")` (`ContainerFrame.lua:773-786`: 4, 8 at
// 40, 12 at 50, 16 only above 60) — 12 for the fixture's level-60 Warrior. The four bag slots
// hold nothing, so they add nothing.
assert_eq!(
s.eval::<i64>("return Bagnon.size").unwrap(),
16,
"Bagnon must size itself off the LIVE backpack (GetContainerNumSlots(0) == 16), not off \
Bagnon_Forever's empty offline cache — nil UnitName('player') is what sent it there"
28,
"Bagnon must size itself off the LIVE bags (GetContainerNumSlots(0) == 16 plus \
GetKeyRingSize() == 12 at level 60), not off Bagnon_Forever's empty offline cache — nil \
UnitName('player') is what sent it there"
);
assert_eq!(
bagnon_slot_quads(&mut s),
16,
"sixteen bag slots must actually be DRAWN — the director saw a title and a gold line and \
nothing else, while every column of the addon survey called this addon fine"
28,
"every live slot must actually be DRAWN (16 backpack + 12 keyring) — the director saw a \
title and a gold line and nothing else, while every column of the addon survey called \
this addon fine"
);
assert!(
s.errors().is_empty(),
@@ -859,8 +861,10 @@ fn the_whole_bagnon_window_survives_being_used() {
let mut s = open_bagnon(&root);
// 1 · Every slot, hovered and left again — the OnEnter/OnUpdate/OnLeave loop on both an
// occupied and an empty slot.
for i in 1..=16 {
// occupied and an empty slot, across every bag the window lays out (the keyring's slots
// come first in Bagnon's default order, the backpack's after them).
let size = s.eval::<i64>("return Bagnon.size").expect("Bagnon.size");
for i in 1..=size {
let name = format!("BagnonItem{i}");
if s.eval::<bool>(&format!("return getglobal(\"{name}\") ~= nil"))
.unwrap_or(false)
@@ -886,7 +890,8 @@ fn the_whole_bagnon_window_survives_being_used() {
// 3 · Bagnon's OWN bag buttons (the row it just revealed — `BagnonBags0..4` plus the
// keyring's `BagnonBags-2`, from `Bagnon.xml`'s `$parent<id>` naming): hover, click and
// drag each. `BagnonBag_OnClick` is `PutItemInBackpack`/`PutItemInBag`, `BagnonBag_OnDrag`
// is `PickupBagFromSlot` — three engine verbs, none of which existed here.
// is `PickupBagFromSlot` — three engine verbs that did not exist here when this test was
// written and are `script/cursor/bag_verbs.rs` now.
for id in ["0", "1", "2", "3", "4", "-2"] {
let name = format!("BagnonBags{id}");
if !s
@@ -906,8 +911,9 @@ fn the_whole_bagnon_window_survives_being_used() {
s.mouse_button(x + 12.0, y + 12.0, "LeftButton", false);
}
// 4 · The money frame's three coin buttons — `OpenCoinPickupFrame`, guarded in our own
// MoneyFrame.xml and *not* guarded in Bagnon's copy of the same handler.
// 4 · The money frame's three coin buttons — `OpenCoinPickupFrame`, which Bagnon's own
// `BagnonFrameMoney_OnClick` (`Frame.lua:537-543`) calls unguarded, exactly as the
// reference's `MoneyFrame.xml` coin buttons do.
for coin in ["Gold", "Silver", "Copper"] {
let name = format!("BagnonMoneyFrame{coin}Button");
if s.eval::<bool>(&format!(
@@ -949,32 +955,16 @@ fn the_whole_bagnon_window_survives_being_used() {
//
// To retire an entry: implement it and delete the line. To add one: you had better be able to
// say why, in the same sentence.
const KNOWN_GAPS: &[(&str, &str)] = &[
// The bag-BUTTON cursor trio (`Bag.lua:199-215` — Bagnon's own bag row, click and drag).
// Three 1.12 engine bindings benilla does not have: `PutItemInBackpack 0x4c8f70`,
// `PutItemInBag 0x4c8f00`, `PickupBagFromSlot 0x4c8fa0` (wow-re `system/ui/ledger.tsv`
// l.1962-1964 has the registrations verified; the DECOMPILATION on disk is partial —
// Ghidra removed unreachable blocks from two of the three — so what each one actually
// DOES is not read).
//
// **Deliberately not guessed.** The open question is whether `PutItemInBag(inv)`
// auto-stores the held item into that bag's CONTENTS or places it onto the equipment slot,
// and those are different opcodes. Writing a cursor transition on the wrong one moves a
// player's items to the wrong place, which is the most expensive kind of wrong here
// (the contract §4), so an RE cross-check is dispatched into wow-5875-re and these land on
// its verdict, not before.
("PutItemInBackpack", "Bagnon_Core\\core\\Bag.lua:202"),
("PutItemInBag", "Bagnon_Core\\core\\Bag.lua:204"),
("PickupBagFromSlot", "Bagnon_Core\\core\\Bag.lua:214"),
// Money on the cursor. `Frame.lua:537-543` — Bagnon's money frame calls this UNGUARDED
// where benilla's own `MoneyFrame.xml:411` guards its copy of the same reference handler.
// It is the head of a whole feature, not a missing line: the reference's
// `CoinPickupFrame.lua`/`.xml` need `GetCursorMoney`, `PickupPlayerMoney`, a MONEY cursor
// payload (wow-re `cursor-dragdrop-payload.md` row 2 — benilla's cursor has Item/Spell/
// Action only) and a keyboard-focused frame with `OnChar`/`OnKeyDown`, which this engine
// deliberately does not have (see `EnableKeyboard` in the same landing's report).
("OpenCoinPickupFrame", "Bagnon_Core\\core\\Frame.lua:543"),
];
//
// **Empty, and that is the list's best state, not a reason to delete it.** Four rows sat here
// when it was written, and each retired the way the rule above says: the bag-BUTTON cursor
// trio (`PutItemInBackpack`, `PutItemInBag`, `PickupBagFromSlot` — `Bag.lua:199-215`) once
// wow-5875-re's verdict landed and `script/cursor/bag_verbs.rs` bound all three; and money on
// the cursor (`OpenCoinPickupFrame`, `Frame.lua:537-543`) when the money kit went stock and
// the reference's own `CoinPickupFrame.xml` joined the manifest with the money cursor under
// it (decision 1962). With nothing on the list, "unexplained" is every error, which is the
// assertion this test was always reaching for.
const KNOWN_GAPS: &[(&str, &str)] = &[];
let raised = s.errors();
let unexplained: Vec<&String> = raised
@@ -1357,13 +1347,20 @@ fn bagnon_forevers_records_survive_the_logout_boundary() {
// The first-login scan (PLAYER_LOGIN, inside `seat`) recorded the live backpack: 16 slots,
// the pouch in slot 1, in Bagnon_Forever's own short-link shape.
//
// **`16,1,` — the 1 is the reference's, not a stray.** The recorder's count is
// `GetInventoryItemCount("player", ContainerIDToInventoryID(0))`, and `ContainerIDToInventoryID`
// has no backpack arm (`0x4f94e0`: `id - 1 < 4` → `+20`), so bag 0 asks inventory slot 19 —
// the tabard slot. `GetInventoryItemCount 0x4c8680` pushes **1** for an empty slot
// (`0x4c8797`; wow-re `inventory-item-count-law.md` §3, carved at 5a377f4bb), and 1 for a
// worn tabard's stack count too, so the real client writes this row as `16,1,` either way.
let record = "local r = BagnonForeverData[GetRealmName()][UnitName('player')] \
return tostring(r[0] and r[0].s), tostring(r[0] and r[0][1])";
let (size, item) = s
.eval::<(String, String)>(record)
.expect("the record reads");
assert_eq!(
size, "16,0,",
size, "16,1,",
"the login scan records the backpack's size row"
);
assert_eq!(
@@ -1422,7 +1419,7 @@ fn bagnon_forevers_records_survive_the_logout_boundary() {
.eval::<(String, String)>(record)
.expect("the record reads");
assert_eq!(
size, "16,0,",
size, "16,1,",
"the logout boundary must not erase the size row"
);
assert_eq!(item, "4496", "the logout boundary must not erase the slot");
@@ -1441,7 +1438,10 @@ fn bagnon_forevers_records_survive_the_logout_boundary() {
/// here with a real mouse click, an equipped bag in slot 2, and Bagnon fully hooked.
#[test]
fn a_bag_slot_click_toggles_bagnon_not_the_native_window() {
let root = corpus_or_skip!();
// The install too: "is a native bag window open" is asked of the reference's own `IsBagOpen`
// scan (`ContainerFrame.lua:177`, off the chain) — the windows are RECYCLED (1751), so there
// is no frame to name, and `bag_tests.rs` asks the same way.
let root = corpus_and_install_or_skip!();
let mut s = open_bagnon(&root);
// An equipped bag behind CharacterBag1Slot (bagId 2): the inventory surface (the bag ITEM in
@@ -1489,8 +1489,7 @@ fn a_bag_slot_click_toggles_bagnon_not_the_native_window() {
"the slot click must reach Bagnon's ToggleBag override and close its window"
);
assert!(
!s.eval::<bool>("return BenillaBagFrame2:IsVisible() or false")
.unwrap(),
!super::test_ui::bag_open(&s, 2),
"the native bag window must NOT open — the click belongs to the addon's override"
);
@@ -1508,8 +1507,7 @@ fn a_bag_slot_click_toggles_bagnon_not_the_native_window() {
"the second click re-opens Bagnon through the same override"
);
assert!(
!s.eval::<bool>("return BenillaBagFrame2:IsVisible() or false")
.unwrap(),
!super::test_ui::bag_open(&s, 2),
"the native window stays shut on the re-open too"
);
assert!(
@@ -1517,29 +1515,37 @@ fn a_bag_slot_click_toggles_bagnon_not_the_native_window() {
.unwrap(),
"the slot button stays unlit with an addon holding the bags (the ref scan is native-only)"
);
// Nothing is filtered out any more: the bag-button cursor trio the round-trip reaches
// (`PutItemInBag` on the click) is built (`script/cursor/bag_verbs.rs`), so a raise on it is
// a real one.
let raised = s.errors();
let unexplained: Vec<&String> = raised
.iter()
.filter(|e| !e.contains("PutItemIn") && !e.contains("PickupBagFromSlot"))
.collect();
assert!(
unexplained.is_empty(),
"the click round-trip raised: {unexplained:#?}"
raised.is_empty(),
"the click round-trip raised: {raised:#?}"
);
}
/// **Bug 7 — the director's unlit backpack button.** Bagnon lights `MainMenuBarBackpackButton`
/// from its window's OnShow/OnHide (`Bagnon.lua:33/39` — `SetChecked(1/0)`), and OnShow dispatch
/// is synchronous (wow-re `onshow-onhide-dispatch-order.md`), so whoever writes checked AFTER the
/// toggle wins. Two of our seats overwrote it: the button's old click tail re-derived checked
/// from the NATIVE backpack window (the ref's own scan — which leaves the button unlit on the
/// real client's click path too, a ref wart our stated divergence closes), and the `B` binding
/// ran the BUTTON handler instead of the ref's bare `ToggleBackpack()` body, dragging that tail
/// onto the one path the real client lights. Now: a real click on the button ends lit while
/// Bagnon's window is open and unlit when it closes, and the bare-global path (the binding's
/// body) does the same.
/// **Bug 7 — the director's unlit backpack button — and where it stands since the bag bar went
/// stock.** Bagnon lights `MainMenuBarBackpackButton` from its window's OnShow/OnHide
/// (`Bagnon.lua:33/39` — `SetChecked(1/0)`), and OnShow dispatch is synchronous (wow-re
/// `onshow-onhide-dispatch-order.md`), so whoever writes checked AFTER the toggle wins. On the
/// reference's own click path the stock `BackpackButton_OnClick` tail
/// (`MainMenuBarBagButtons.lua:55-69`) runs last: it re-derives checked by scanning the NATIVE
/// container frames, finds none for bag 0 while an addon holds the bags, and writes 0 — so the
/// real client leaves the button unlit after a click with Bagnon, a wart of its own. Our own
/// `BagFrame.xml` once closed that wart on the director's call (Bug 7: an auto-toggle undo in
/// place of the scan, a stated divergence); decision 1783 sourced the stock file unedited and the
/// divergence went with it, unrecorded — and this suite, skipping at every land from 08-30
/// (2329), never said so.
///
/// What this pins now is the client as it is, which is the reference: the bare
/// `ToggleBackpack()` path (the `B` binding's body) ends lit, because nothing runs after OnShow
/// there; a real click ends **unlit** on open as well as on close, because the stock tail is the
/// last word. Whether to reinstate Bug 7's divergence as an adapter shim is the director's call
/// (1751 §2 reserves shims for engine differences, and this is a look); if they say lit, the
/// second assertion below flips back and the shim carries the why.
#[test]
fn the_backpack_button_lights_while_bagnon_holds_the_bags() {
fn the_backpack_button_lit_state_with_bagnon_holding_the_bags() {
let root = corpus_or_skip!();
let mut s = open_bagnon(&root);
@@ -1568,7 +1574,9 @@ fn the_backpack_button_lights_while_bagnon_holds_the_bags() {
"closed ⇒ unlit (Bagnon's OnHide write is the last word)"
);
// And the second click: open again, lit again — the director's report, closed.
// And the second click: open again — and UNLIT, the stock tail's native scan overwriting
// Bagnon's OnShow write, as the reference does. Bug 7's "lit" is the assertion this replaces;
// it comes back with the shim if the director asks for it (the doc above).
s.mouse_button(x, y, "LeftButton", true);
s.mouse_button(x, y, "LeftButton", false);
assert!(
@@ -1577,8 +1585,9 @@ fn the_backpack_button_lights_while_bagnon_holds_the_bags() {
"the second click re-opens Bagnon"
);
assert!(
s.eval::<bool>("return MainMenuBarBackpackButton:GetChecked() and true or false")
!s.eval::<bool>("return MainMenuBarBackpackButton:GetChecked() and true or false")
.unwrap(),
"open ⇒ lit (Bagnon's OnShow write is the last word)"
"open by click ⇒ unlit: the stock BackpackButton_OnClick tail's native scan is the last \
word, as on the reference (Bug 7's divergence retired with the stock bag bar, 1783)"
);
}
@@ -1410,7 +1410,7 @@ mod tests {
// …and the pair every `Instant`→`GetTime` conversion runs through moved with it: a
// 10-minute cooldown armed 30 s before the rebuild still derives its real start.
let clock = world.resource::<UiClock>();
let armed = crate::cooldowns::CooldownInfo {
let armed = crate::spell::cooldowns::CooldownInfo {
start: startup + Duration::from_secs(60),
remaining_ms: 570_000,
duration_ms: 600_000,
@@ -37,27 +37,12 @@ use std::path::{Path, PathBuf};
use benilla_ui::script::{ScriptValue, UiScript, UnitState};
use benilla_ui::toc::Toc;
/// Where the vanilla addon corpus might be — `$BENILLA_ADDON_CORPUS`, else a sibling checkout
/// resolved from this crate's manifest (a pool worktree's cwd is not stable across tool calls).
fn corpus_candidates() -> Vec<PathBuf> {
let mut out = Vec::new();
if let Some(over) = std::env::var_os("BENILLA_ADDON_CORPUS") {
out.push(PathBuf::from(over));
}
let manifest = Path::new(env!("CARGO_MANIFEST_DIR"));
for up in [2usize, 3, 4] {
if let Some(root) = manifest.ancestors().nth(up) {
out.push(root.join("wow-addons-vanilla"));
}
}
out
}
/// The corpus root **and** a Quiver in it, or `None` — a skip, never a failure. Quiver is a live
/// third-party addon (`github.com/SabineWren/Quiver`) whose shipped file is a generated bundle, so
/// a machine that wants this test builds it into the corpus once.
/// a machine that wants this test builds it into the corpus once. The corpus itself resolves
/// through `benilla_formats::addon_corpus` — the one resolver, whose doc carries the incident.
fn quiver_root() -> Option<PathBuf> {
corpus_candidates()
benilla_formats::addon_corpus_candidates()
.into_iter()
.find(|c| c.join("Quiver").join("Quiver.toc").is_file())
}
@@ -67,10 +52,10 @@ macro_rules! quiver_or_skip {
match quiver_root() {
Some(root) => root,
None => {
eprintln!(
"skipping: no Quiver in the addon corpus — looked in {:?} \
(set $BENILLA_ADDON_CORPUS; the folder needs Quiver.toc + Quiver.bundle.lua)",
corpus_candidates()
benilla_formats::skipped(
"no Quiver in the addon corpus (set $BENILLA_ADDON_CORPUS; the folder needs \
Quiver.toc + Quiver.bundle.lua)",
&benilla_formats::addon_corpus_candidates(),
);
return;
}
@@ -288,7 +288,7 @@ fn quitting_from_the_character_screen_does_not_blank_the_session_it_wrote() {
///
/// This is the regression the latch design could most easily have caused, so it is pinned rather
/// than argued. The obvious guard for the tail — "is there a player object?" — reads FALSE on a
/// `/logout` by the time the tail runs: `net::apply::session::logged_out` despawns our avatar in
/// `/logout` by the time the tail runs: `net::session::logged_out` despawns our avatar in
/// the same drain that writes `LoggedOutMessage`, `back_on_logout` sets `NextState` off that same
/// message, and `OnExit(InWorld)` does not run until the next frame's `StateTransition`. A
/// predicate would have silenced the event on every logout to fix a quit on a loading screen.