From 881670db91599296592c11c6c700cdce6a06e2d3 Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:01:49 +0200 Subject: [PATCH 1/9] Clear drainHold once the HP-bar drain actually finishes stepHPDrain counts drainHold down to 0 as the last step of every phase (pixel slide, HP-number step, closing frames) but never let go of the field afterward, so it sat at 0 -- not nil -- for the rest of the battle. BattleSafety.inspect uses drainHold ~= nil as its settled-presentation gate for checkpoint capture, so the very first HP change in a battle permanently refused every checkpoint after it with battle_phase_busy, even once the bar had long since caught up. Only nil the field when the whole drain is actually over (bar pixel, HP number and the closing-frame hold all settled), not on every mid-sequence 0 -- a fresh HP change still needs drainHold to read as busy so BattleSafety keeps refusing captures until that one settles too. --- src/battle/BattleState.lua | 9 +++++++++ tests/engine/battle_checkpoint_boundary.lua | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/battle/BattleState.lua b/src/battle/BattleState.lua index dee46a19..36e9bf5e 100644 --- a/src/battle/BattleState.lua +++ b/src/battle/BattleState.lua @@ -1103,6 +1103,15 @@ function BattleState:stepHPDrain() if not b.shownPx then b.shownPx = targetPx end if (b.drainHold or 0) > 0 then b.drainHold = b.drainHold - 1 + -- Once the count runs out with nothing left pending (bar and + -- number already on the final total), the drain is over, not just + -- between steps: leave the field at 0 and BattleSafety.inspect + -- reads it as still mid-animation for the rest of the battle, + -- since drainHold ~= nil is its settled-presentation gate. + if b.drainHold <= 0 and b.shownPx == targetPx and b.shownHP == goal + and not b.draining then + b.drainHold = nil + end busy = true elseif b.shownPx ~= targetPx then -- .barAnimationLoop redraws the bar one pixel at a time, `ld c, 2 / diff --git a/tests/engine/battle_checkpoint_boundary.lua b/tests/engine/battle_checkpoint_boundary.lua index c9bfea05..318c460c 100644 --- a/tests/engine/battle_checkpoint_boundary.lua +++ b/tests/engine/battle_checkpoint_boundary.lua @@ -131,4 +131,25 @@ T.same(Checkpoint.inspect(game), { canCapture = true, canRestore = true, kind = "overworld", }, "settled overworld remains supported") +-- drainHold gates capture (see the refused() case above) exactly because it +-- marks an HP bar mid-animation. Once stepHPDrain settles the bar it must +-- let go of that gate too, or the very first drain of a battle leaves the +-- checkpoint contract refused for everything after it. +do + local game3, _, battle3 = makeGame() + battle3.enemy.mon.hp = battle3.enemy.mon.hp - 5 + local frames = 0 + while battle3:stepHPDrain() and frames < 10000 do + frames = frames + 1 + end + T.eq(battle3.enemy.shownHP, battle3.enemy.mon.hp, + "the HP bar settles on the new total") + T.eq(battle3.enemy.drainHold, nil, + "drainHold releases the checkpoint gate once the bar finishes draining") + local capability = Checkpoint.inspect(game3) + T.check(capability.canCapture == true, + "a checkpoint is capturable again after the drain settles: " + .. tostring(capability.reason)) +end + T.finish() From 2b5229e73f5145fd8b1f4de4fe938b61d6d6ad44 Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:03:52 +0200 Subject: [PATCH 2/9] Deny require("jit.util") in the mod sandbox DENIED_PREFIX blocked love.* and ffi.* submodule requires but had no entry for jit, so require("jit.util") walked straight through to the real module. jit.util is LuaJIT's own equivalent of the debug library this file already denies by name: funcbc, funck and the rest read the bytecode and constants of any function a chunk can reach, which is enough to recover upvalues -- the real _G, love, io -- that the sandbox exists to keep out of a mod's hands. Adding "jit" to DENIED_PREFIX blocks jit.* submodule requires the same way love.* and ffi.* already are, while leaving the bare jit global (env.jit, handed over directly for jit.on/off/flush) and a bare require("jit") untouched -- jit.util is not a field of that table without its own require, so neither route was ever a way to reach it. --- src/mods/Sandbox.lua | 10 ++++++++-- tests/modkit/cases/sandbox.lua | 7 +++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/mods/Sandbox.lua b/src/mods/Sandbox.lua index e41190ba..f52a07ce 100644 --- a/src/mods/Sandbox.lua +++ b/src/mods/Sandbox.lua @@ -28,8 +28,14 @@ local DENIED = { } -- Same idea one level up: love.filesystem is reachable by name, and --- love.thread starts a Lua state this sandbox has no say over. -local DENIED_PREFIX = { ["love"] = true, ["ffi"] = true } +-- love.thread starts a Lua state this sandbox has no say over. jit.util +-- is the LuaJIT-specific equal of the debug library above -- funcbc, +-- funck and friends read the bytecode and constants of any function a +-- chunk can reach, which is enough to walk back to upvalues (the real +-- _G, love, io) the rest of this file exists to keep out of reach. The +-- bare `jit` table stays -- env.jit above hands it over directly for +-- jit.on/off/flush -- so only the submodule require is denied. +local DENIED_PREFIX = { ["love"] = true, ["ffi"] = true, ["jit"] = true } -- The wire, which is what the network permission governs. local NETWORK = { socket = true, enet = true, http = true, https = true, diff --git a/tests/modkit/cases/sandbox.lua b/tests/modkit/cases/sandbox.lua index 27133e15..5f9c06c8 100644 --- a/tests/modkit/cases/sandbox.lua +++ b/tests/modkit/cases/sandbox.lua @@ -44,6 +44,11 @@ local PROBE = [[ out.requireDebug = attempt(require, "debug") out.requirePackage = attempt(require, "package") out.requireFfi = attempt(require, "ffi") + -- jit.util exposes bytecode/constant introspection over any function this + -- chunk can reach -- the same class of escape the debug library is denied + -- for -- so it must fail the same way require("debug") does rather than + -- walking straight through under the bare "jit" global's cover. + out.requireJitUtil = attempt(require, "jit.util") out.requireSocket = attempt(require, "socket") out.requireSemver = select(2, pcall(require, "src.mods.Semver")) @@ -180,6 +185,8 @@ T.eq(out.getfenv, nil, "getfenv is still absent, so a mod cannot read the real _ T.eq(out.debug, nil, "the debug library is still absent") T.check(out.loveThread ~= false, "love.thread is still refused: it opens a full Lua state") T.check(out.requireFfi ~= false, "require(\"ffi\") is still refused: it is arbitrary C") +T.check(out.requireJitUtil ~= false, + "require(\"jit.util\") is still refused: it is bytecode/constant introspection") T.check(out.requireDebug ~= false, "require(\"debug\") is still refused") T.check(out.requirePackage ~= false, "require(\"package\") is still refused") T.eq(out.popen, nil, "io.popen refuses rather than spawning a process") From 82ae667611c4877e28555e3ccb5ae856bc966fa5 Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:04:23 +0200 Subject: [PATCH 3/9] Update the crossValidate comment for growth_rates / evolution_methods The comment above the gatedFor skip in Schemas.crossValidate still described growth_rates and evolution_methods as unconfirmable Gen 2 namespaces, the way they were before each got a real Gen 2 id space: growth_rates keeps its Gen 1 target and is seeded from the extractor's data.pokemon.growthRates (src/battle/gen2/Mon.lua), and evolution_methods routes to gen2EvolutionMethods, a fixed literal set (src/core/gen2/Evolution.lua) that exists with or without a ROM import. Schemas.GEN2 does not gate either name -- gate_gen2_mod_api.lua pins that directly, including a case that a bad evolution method on a Gold species is still caught -- so the two are validated like any other reference today, not skipped. Nothing here changes that behavior; only the comment, which was describing an earlier state of the code, is corrected. --- src/mods/Schemas.lua | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/mods/Schemas.lua b/src/mods/Schemas.lua index 23d34b37..9be13727 100644 --- a/src/mods/Schemas.lua +++ b/src/mods/Schemas.lua @@ -377,11 +377,19 @@ function Schemas.crossValidate(loader, data) and loader.content[ref.registry] -- A registry with no home in this generation has no id space to -- check against: its base view resolves to nothing, so EVERY - -- reference into it would read as dangling. Gold's species carry a - -- growthRate and an evolution method like Red's do; the ids are - -- fine, it is the Gen 1 `growth_rates` / `evolution_methods` - -- namespaces that are not there to confirm them. Skipped for the - -- same reason an undeclared registry is: unknown, not wrong. + -- reference into it would read as dangling. `transitions` is the + -- standing example -- Gold draws its own battle intro and never + -- reads the merged table, so a mod's transition id there is + -- unconfirmable, not wrong. `growth_rates` and `evolution_methods` + -- used to sit in that category too, back when Gold had no id space + -- for either. Both are routed now: `growth_rates` keeps its Gen 1 + -- path and is seeded from data.pokemon.growthRates (the extractor's + -- Gold curves, src/battle/gen2/Mon.lua), and `evolution_methods` + -- routes to gen2EvolutionMethods (src/core/gen2/Evolution.lua's + -- literal EVOLVE_* ids, present with or without a ROM import). A + -- Gold species' growthRate or evolution method is checked against + -- real ids exactly like a Red one's, so a genuine typo is still + -- caught here rather than waved through as "unknown, not wrong." if refRegistry and Schemas.gatedFor(ref.registry, loader.generation) then refRegistry = nil end From f62b1268c888bfce5bf591973e6a0c8364e788a6 Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:11:39 +0200 Subject: [PATCH 4/9] Add an item.use hook around BagMenu's item-use dispatch useOn was a plain Lua local: every result ItemEffects.use returned fell through to one unconditional showMessages with no seam a mod could reach, unlike menu.lua/boxmark.lua/formview.lua's screens, which wrap their own default behavior as a table field or a Runtime hook. A mod could not suppress a message, delay it behind a screen of its own, or substitute a different outcome for one item id -- exactly the gap noted against Ultra Burst's item-driven fusion, which had nowhere left to attach a bespoke animation once TextBox.new turned out to be the only other reachable seam. This wraps the whole dispatch in a Runtime.call("item.use", ...) hook, the same mechanism "battle.overlay", "ui.party.submenu" and the rest of src/ui already use, rather than exporting BagMenu.useOn as a table field. A hook is the smaller commitment: it is additive (a fresh Runtime.call site needs no schema or manifest change and costs nothing unsubscribed -- see tests/engine/gate_hooks.lua's null-object case) and a mod can still run the vanilla flow unchanged by calling the handed-in vanilla function, whereas a table field would fix useOn's exact signature as public API the moment it shipped. If the maintainer would rather match the sibling screens' convention directly, exporting BagMenu.useOn is the alternative and does not conflict with this hook existing alongside it. vanillaUseOn keeps the original function body; useOn is now the thin wrapper mods observe through, and every internal caller in this file still goes through useOn so the hook fires on every path into it. --- src/ui/BagMenu.lua | 17 +++++- tests/engine/item_use_hook.lua | 101 +++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 tests/engine/item_use_hook.lua diff --git a/src/ui/BagMenu.lua b/src/ui/BagMenu.lua index e8cb6630..5d6807f1 100644 --- a/src/ui/BagMenu.lua +++ b/src/ui/BagMenu.lua @@ -4,6 +4,7 @@ local ItemEffects = require("src.inventory.ItemEffects") local ListMenu = require("src.ui.ListMenu") +local Runtime = require("src.mods.Runtime") local TextBox = require("src.render.TextBox") local BagMenu = {} @@ -46,7 +47,16 @@ end -- the stack, so every exit that prints has to close it afterwards. For -- every other item the picker popped itself first and closePicker's identity -- check makes it a no-op (#252). -local function useOn(game, battle, id, target, list, moveIndex, picker) +-- +-- Every result string used to fall through to this one unconditional +-- function with no seam around it: a mod could not suppress a message, +-- delay it behind a screen of its own, or replace the outcome for one item +-- id. The "item.use" hook wraps the whole dispatch (not a name per +-- result -- a mod deciding what a Poké Doll or a stone does needs the +-- SAME reach a vanilla `if result == ...` branch has, not a narrower one), +-- the way "battle.overlay" and "ui.party.submenu" already wrap a +-- screen's own default behavior elsewhere in src/ui. +local function vanillaUseOn(game, battle, id, target, list, moveIndex, picker) local result, payload, extra = ItemEffects.use(game.data, game.save, id, target, battle, moveIndex, game.overworld) local function closePicker() @@ -375,6 +385,11 @@ local function useOn(game, battle, id, target, list, moveIndex, picker) showMessages(game, payload, closePicker) -- failed end +local function useOn(game, battle, id, target, list, moveIndex, picker) + return Runtime.call("item.use", vanillaUseOn, + game, battle, id, target, list, moveIndex, picker) +end + local function pickTargetAndUse(game, battle, id, list) -- pick a target from the party -- the ETHERs and PP UP open the move menu after picking a mon diff --git a/tests/engine/item_use_hook.lua b/tests/engine/item_use_hook.lua new file mode 100644 index 00000000..9107a32c --- /dev/null +++ b/tests/engine/item_use_hook.lua @@ -0,0 +1,101 @@ +-- Public mod-API coverage for the "item.use" hook (src/ui/BagMenu.lua). +-- +-- Before this hook existed, every result ItemEffects.use returned fell +-- through to one unconditional call with nothing wrapped around it: a mod +-- could not suppress a message, delay it behind a screen of its own, or +-- replace what a specific item id does after the bag decides to use it. +-- This exercises the seam end to end through the public mod API -- a real +-- BagMenu list, a real USE selection -- rather than calling the hook +-- machinery directly. + +package.path = "./?.lua;./?/init.lua;" .. package.path + +local T = require("tests.modkit") +local Bag = require("src.inventory.Bag") + +-- Real TextBoxes want a Font atlas; this only cares that useOn reaches the +-- no-effect fallthrough, so the same stand-in tests/parity_rare_candy_menu.lua +-- uses for a ROM-backed run works here too. +local realTextBox = package.loaded["src.render.TextBox"] +package.loaded["src.render.TextBox"] = { + new = function(_, text, done) return { textBox = true, text = text, done = done } end, +} +package.loaded["src.ui.BagMenu"] = nil +local BagMenu = require("src.ui.BagMenu") + +local FIXTURE = { + ["mods/item_hook_probe/manifest.json"] = [[{ + "id": "item_hook_probe", + "name": "Item Hook Probe", + "version": "1.0.0", + "entry": "main.lua", + "api": 2 + }]], + ["mods/item_hook_probe/main.lua"] = [[ + local mod = ... + mod.hooks:wrap("item.use", + function(vanilla, game, battle, id, target, list, moveIndex, picker) + mod.exports.calls = (mod.exports.calls or 0) + 1 + mod.exports.id = id + mod.exports.battle = battle + mod.exports.target = target + return vanilla(game, battle, id, target, list, moveIndex, picker) + end) + ]], +} + +local function newStack() + local stack = { states = {} } + function stack:push(s) self.states[#self.states + 1] = s end + function stack:pop() return table.remove(self.states) end + function stack:top() return self.states[#self.states] end + return stack +end + +local run = T.sdk.loadMods({ "mods/item_hook_probe" }, { + fs = T.sdk.memfs(FIXTURE), +}) +T.eq(#run.errors, 0, + "the probe mod loads clean (" .. tostring(run.errors[1]) .. ")") + +local game = { + data = run.data, + stack = newStack(), + save = { + player = { name = "RED" }, inventory = {}, money = 0, + options = { battleStyle = "set", battleAnim = "on" }, + pokedex = { seen = {}, owned = {} }, flags = {}, + }, +} +Bag.add(game.save, "FIX_POTION", 1) + +local list = BagMenu.new(game, {}) +game.stack:push(list) +local row +for i, r in ipairs(list.items) do + if r.value == "FIX_POTION" then row = i end +end +T.check(row ~= nil, "the fixture item is in the bag") +list.index = row +list.onChoose(list.items[row], list) + +-- out of battle the bag offers USE / TOSS first (start_sub_menus.asm) +local sub = game.stack:top() +T.check(sub ~= nil and sub.items and sub.items[1] and sub.items[1].onSelect, + "the USE/TOSS submenu opened") +sub.items[1].onSelect() + +local out = run.loader.exports.item_hook_probe or {} +T.eq(out.calls, 1, "the hook fires exactly once for a bag item use") +T.eq(out.id, "FIX_POTION", "the hook sees the item id") +T.eq(out.battle, nil, "the hook sees the field-use battle argument (nil)") + +local top = game.stack:top() +T.check(type(top) == "table" and top.textBox == true, + "vanilla still ran: the no-effect message box landed on the stack") + +run.release() +package.loaded["src.render.TextBox"] = realTextBox +package.loaded["src.ui.BagMenu"] = nil + +T.finish() From 2da2168dacca8f6ecc037d86fe0951bb1929f0c8 Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:13:14 +0200 Subject: [PATCH 5/9] Refuse a TM/HM on a species with no tmhm list instead of crashing ItemEffects.use walked speciesDef.tmhm with a bare ipairs() to check whether the species could learn the machine's move. A record with no tmhm field at all -- a mod species that never set one, or any record missing it for whatever reason -- hit ipairs(nil) and took the whole game down on the first TM/HM use, rather than reaching the ordinary "can't learn that move" refusal a species whose list simply omits the move already gets. An absent list now reads the same as an empty one: nothing to learn, same refusal, same sound, same text. --- src/inventory/ItemEffects.lua | 5 ++++- tests/engine/item_tmhm_nil_test.lua | 31 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 tests/engine/item_tmhm_nil_test.lua diff --git a/src/inventory/ItemEffects.lua b/src/inventory/ItemEffects.lua index a900fec9..691b859d 100644 --- a/src/inventory/ItemEffects.lua +++ b/src/inventory/ItemEffects.lua @@ -515,7 +515,10 @@ function ItemEffects.use(data, save, itemId, target, battle, moveIndex, ow) if not target then return "failed", { noEffect(data) } end local speciesDef = data.pokemon[target.species] local ok = false - for _, m in ipairs(speciesDef.tmhm) do + -- a species record with no tmhm list at all is "teaches nothing", the + -- same as one whose list just does not name this move -- not a reason + -- to crash instead of refusing normally + for _, m in ipairs(speciesDef.tmhm or {}) do if m == itemDef.machine.move then ok = true break end end if not ok then diff --git a/tests/engine/item_tmhm_nil_test.lua b/tests/engine/item_tmhm_nil_test.lua new file mode 100644 index 00000000..217db2ee --- /dev/null +++ b/tests/engine/item_tmhm_nil_test.lua @@ -0,0 +1,31 @@ +-- ItemEffects.use crashed instead of refusing when a species record carried +-- no tmhm list at all (ipairs(nil)), which is a different situation from a +-- species whose list simply does not name the move being taught -- that +-- case already refuses cleanly with MonCannotLearnMachineMoveText. A mod +-- species missing the field entirely took the whole game down on the very +-- first TM/HM use rather than reaching that refusal. + +package.path = "./?.lua;./?/init.lua;" .. package.path +if not _G.love then _G.love = require("tests.love_stub") end + +local T = require("tests.harness").suite("item effects tmhm nil") +local Fixtures = require("tests.modkit.fixtures") +local ItemEffects = require("src.inventory.ItemEffects") +local Pokemon = require("src.pokemon.Pokemon") + +local Data = Fixtures.fresh() +Data.pokemon.FIXMON_A.tmhm = nil + +local mon = Pokemon.new(Data, "FIXMON_A", 10) +local save = { player = { name = "RED" } } + +local ok, result, payload = pcall(ItemEffects.use, Data, save, "FIX_TM", mon) +T.check(ok, "using a TM on a species with no tmhm list does not crash: " + .. tostring(result)) +if ok then + T.eq(result, "failed", "the species refuses the move instead of crashing into it") + T.check(type(payload) == "table" and payload[1] ~= nil, + "a refusal message is still returned") +end + +T.finish() From 455ff21aff59ab8c6a634e8bd6663de661141edc Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:19:16 +0200 Subject: [PATCH 6/9] Validate a map object's pokemon field against the pokemon registry R.maps.objects was f.opt(f.list(f.any)): a static wild encounter's species (OverworldController.lua's d.pokemon, handed straight to BattleState.newWild) went completely unchecked at load time, unlike an encounter slot's species. A typo'd or removed id sat in a loaded mod and only surfaced as a crash the moment a player reached that object. Objects share one array across every kind -- NPCs, signs, warps and static encounters all coexist with no field the loader could use to tell them apart ahead of time -- so a strict f.rec covering the whole shape would reject every kind this schema does not enumerate. Added f.partial, an open counterpart to f.rec: it type-checks (and, through collectRefs, cross-reference-checks) only the fields it is given and leaves everything else on the value alone, the same extensibility f.rec already grants at a record's top level but nowhere further in. R.maps.objects now types just `pokemon` through it, so a bad species id is a load-time "unresolved reference" error instead of a runtime crash, while an NPC object's sprite/movement/range/... fields -- never named in this schema -- still pass through untouched. --- src/mods/Schemas.lua | 51 ++++++++++- tests/engine/map_object_pokemon_ref_test.lua | 89 ++++++++++++++++++++ 2 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 tests/engine/map_object_pokemon_ref_test.lua diff --git a/src/mods/Schemas.lua b/src/mods/Schemas.lua index 9be13727..dfa2fb97 100644 --- a/src/mods/Schemas.lua +++ b/src/mods/Schemas.lua @@ -91,6 +91,28 @@ function f.rec(fields, opts) desc = "{" .. table.concat(parts, ", ") .. "}" } end +-- An open record: the listed fields are typed (including f.id +-- cross-references) and everything else on the value passes through +-- unexamined, unlike f.rec's nested shapes, which reject any key they do +-- not name. Map objects are why this exists -- NPCs, signs, items, warps +-- and static encounters all share one array, and only a full union of +-- every kind's shape could describe it as f.rec; that is a lot of surface +-- to keep in sync with the loader for fields nothing here needs to check. +-- f.partial types just the field that actually names another registry and +-- leaves every kind-specific field around it alone. +function f.partial(fields) + local names = {} + for name in pairs(fields) do names[#names + 1] = name end + table.sort(names) + local parts = {} + for _, name in ipairs(names) do + local ft = fields[name] + parts[#parts + 1] = name .. (ft.kind == "opt" and "?" or "") + end + return { kind = "partial", fields = fields, + desc = "{" .. table.concat(parts, ", ") .. ", ...}" } +end + function f.union(alts) local parts = {} for _, alt in ipairs(alts) do parts[#parts + 1] = alt.desc end @@ -197,6 +219,23 @@ checkValue = function(t, value, path, patchMode, errors, top) end return end + if kind == "partial" then + -- the open counterpart of "rec": listed fields are checked exactly + -- like a rec's, and any key not listed is left alone rather than + -- flagged, so a heterogeneous blob (map objects) can have one field + -- typed without every other shape sharing the array being rejected + if type(value) ~= "table" then return fail(errors, path, t.desc, value) end + for key, ft in pairs(t.fields) do + local sub = value[key] + if sub ~= nil then + checkValue(ft, sub, path .. "." .. tostring(key), patchMode, errors) + elseif ft.kind ~= "opt" and not patchMode then + errors[#errors + 1] = ("%s.%s: missing required field (%s)") + :format(path, key, ft.desc) + end + end + return + end if kind == "union" then for _, alt in ipairs(t.alts) do local scratch = {} @@ -299,7 +338,7 @@ collectRefs = function(t, value, path, out) for k, v in pairs(value) do collectRefs(t.value, v, path .. "." .. tostring(k), out) end - elseif kind == "rec" and type(value) == "table" then + elseif (kind == "rec" or kind == "partial") and type(value) == "table" then for key, ft in pairs(t.fields) do collectRefs(ft, value[key], path .. "." .. tostring(key), out) end @@ -895,7 +934,15 @@ R.maps = { destMap = f.str, destWarp = f.int(0), destGroup = f.opt(f.int(0)), destMapNum = f.opt(f.int(0)) })), - objects = f.opt(f.list(f.any)), + -- NPCs, signs, items, warps and static wild encounters all share this + -- one array, with no field the loader could use to tell them apart + -- ahead of time -- an f.rec strict enough to describe every kind would + -- reject the others. f.partial types only `pokemon` (the static + -- encounter's species, OverworldController.lua's `d.pokemon` -> + -- BattleState.newWild) so a bad id is a load-time error, the same as + -- an encounter slot's species, instead of the crash newWild has no + -- guard against. Every other object field passes through untouched. + objects = f.opt(f.list(f.partial{ pokemon = f.opt(f.id("pokemon")) })), signs = f.opt(f.list(f.any)), connections = f.opt(f.map(f.enum{ "north", "south", "east", "west" }, f.any)), }, diff --git a/tests/engine/map_object_pokemon_ref_test.lua b/tests/engine/map_object_pokemon_ref_test.lua new file mode 100644 index 00000000..905f400d --- /dev/null +++ b/tests/engine/map_object_pokemon_ref_test.lua @@ -0,0 +1,89 @@ +-- A map object's `pokemon` field (the static wild encounter kind -- +-- OverworldController.lua's `d.pokemon`, handed straight to +-- BattleState.newWild with no existence check of its own) used to go +-- completely unchecked: R.maps.objects was f.opt(f.list(f.any)), so a +-- typo'd species sat in a loaded mod and only surfaced as a crash the +-- moment a player stepped up to that object. Every other kind sharing the +-- objects array (NPCs, signs-as-objects, warps) has fields this schema +-- still does not know about, which is what f.partial is for: it types only +-- `pokemon` and leaves the rest of an object's shape alone. + +package.path = "./?.lua;./?/init.lua;" .. package.path + +local T = require("tests.modkit") + +local function manifest(id) + return ([[{ + "id": "%s", "name": "%s", "version": "1.0.0", + "entry": "main.lua", "api": 2 + }]]):format(id, id) +end + +-- ------- a bad species id is caught as a load error, not left to crash + +local BAD = { + ["mods/bad_static_encounter/manifest.json"] = manifest("bad_static_encounter"), + ["mods/bad_static_encounter/main.lua"] = [[ + local mod = ... + mod.content.maps:patch("FIX_ROUTE", { + objects = { + { pokemon = "NOT_A_SPECIES", level = 30, text = "Gyaoo!" }, + }, + }) + ]], +} + +do + local run = T.sdk.loadMods({ "mods/bad_static_encounter" }, + { fs = T.sdk.memfs(BAD) }) + local dangling = {} + for _, message in ipairs(run.errors) do + if message:match("unresolved reference") then + dangling[#dangling + 1] = message + end + end + T.eq(#dangling, 1, + "a bad static-encounter species is reported once (" + .. table.concat(dangling, "; ") .. ")") + T.check(dangling[1] and dangling[1]:match("maps%.FIX_ROUTE%.objects") + and dangling[1]:match("pokemon"), + "the report names the map, the objects field and the pokemon registry: " + .. tostring(dangling[1])) + run.release() +end + +-- ------- a real species resolves, and an NPC-shaped object beside it (no +-- pokemon field at all, and fields this schema never named -- sprite, +-- movement, range) is untouched + +local GOOD = { + ["mods/good_static_encounter/manifest.json"] = manifest("good_static_encounter"), + ["mods/good_static_encounter/main.lua"] = [[ + local mod = ... + mod.content.maps:patch("FIX_ROUTE", { + objects = { + { index = 1, name = "FIXROUTE_TRAINER", sprite = "SPRITE_FIX_NPC", + movement = "STAY", range = "NONE", text = "TEXT_FIXROUTE_TRAINER", + x = 5, y = 9 }, + { pokemon = "FIXMON_A", level = 30, text = "Gyaoo!" }, + }, + }) + ]], +} + +do + local run = T.sdk.loadMods({ "mods/good_static_encounter" }, + { fs = T.sdk.memfs(GOOD) }) + T.eq(#run.errors, 0, + "a real species and an untyped NPC object both load clean (" + .. tostring(run.errors[1]) .. ")") + local objects = run.data.maps.FIX_ROUTE.objects + T.eq(#objects, 2, "both objects landed on the map") + T.eq(objects[1].sprite, "SPRITE_FIX_NPC", + "the NPC object's untyped fields passed through unexamined") + T.eq(objects[2].pokemon, "FIXMON_A", + "the static encounter's species field passed through too") + run.release() +end + +T.finish() From c23f85cba92b23daff0910e5cf82013787bd13cc Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:21:47 +0200 Subject: [PATCH 7/9] Bind gen2Constants in the save editor's Gold bootstrap bindGoldData points gen2Palettes, gen2Icons, gen2Pokedex, gen2Landmarks, gen2Roofs and gen2Sprites at the extractor's own Gold tables through loadGen, but never gen2Constants -- despite Schemas.GEN2 routing `constants` to that same namespaced-and-differently-shaped category palettes and icons are in. A save editor boot left data.gen2Constants unset, so mod.content.constants:get(...) read an empty table instead of the cart's ordered name lists, misreading the generation and rejecting every record a mod shaped off it. data.gen2Constants now goes through the same loadGen("constants") path the other five already use, falling back the same way they do when no ROM cache is active. --- tests/save_editor_gen2_tests.lua | 17 +++++++++++++++++ tools/save-editor/Gen.lua | 6 ++++++ 2 files changed, 23 insertions(+) diff --git a/tests/save_editor_gen2_tests.lua b/tests/save_editor_gen2_tests.lua index 365ec70e..db9474d8 100644 --- a/tests/save_editor_gen2_tests.lua +++ b/tests/save_editor_gen2_tests.lua @@ -324,6 +324,23 @@ do check(bound.gen2Tilesets == bound.tilesets, "bindGoldData aliases gen2Tilesets") check(Gen.tilesets({ gen2Tilesets = { TILESET_GYM = true } }).TILESET_GYM, "Gen.tilesets prefers gen2Tilesets") + + -- bindGoldData bound gen2Palettes/gen2Icons/gen2Pokedex/gen2Landmarks/ + -- gen2Roofs/gen2Sprites through loadGen but never gen2Constants, so any + -- mod reading mod.content.constants:get(...) under a save-editor Gold + -- bootstrap saw an empty table where it expected the cart's ordered name + -- lists. loadGen falls back to require("data.generated.constants") when + -- the ROM cache has nothing active, which is what a checkout with no + -- ROM imported hits too -- stub that module the same way to prove the + -- wiring without needing a real Gold extraction. + package.loaded["data.generated.constants"] = { badges = { "ZEPHYR" } } + local withConstants = Gen.bindGoldData({}) + package.loaded["data.generated.constants"] = nil + check(withConstants.gen2Constants ~= nil, + "bindGoldData populates gen2Constants") + check(withConstants.gen2Constants and withConstants.gen2Constants.badges + and withConstants.gen2Constants.badges[1] == "ZEPHYR", + "gen2Constants carries the extractor's own name lists") end do diff --git a/tools/save-editor/Gen.lua b/tools/save-editor/Gen.lua index 695aa9b8..56258a85 100644 --- a/tools/save-editor/Gen.lua +++ b/tools/save-editor/Gen.lua @@ -95,6 +95,12 @@ function Gen.bindGoldData(data) end data.gen2Palettes = data.gen2Palettes or loadGen("palettes") + -- Namespaced AND differently shaped in Schemas.GEN2 (the cart's ordered + -- name lists, not Gen 1's rule table), same as palettes/icons below -- + -- omitting it left mod.content.constants:get(...) reading an empty table + -- under a Gold save-editor boot, which is what misreads "generation" and + -- rejects every record a mod shapes off it. + data.gen2Constants = data.gen2Constants or loadGen("constants") data.gen2Icons = data.gen2Icons or loadGen("icons") data.gen2Pokedex = data.gen2Pokedex or loadGen("pokedex") data.gen2Landmarks = data.gen2Landmarks or loadGen("landmarks") From d03d2af5f8e7e61e66bf6776f6ab9625dc5337bc Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:28:38 +0200 Subject: [PATCH 8/9] Pass data through to ItemEffects.partyAction for Gen 2 pack items Both call sites -- Game2:usePartyItem (the field pack) and BattleState:useItem (the battle pack) -- asked ItemEffects.partyAction for an item's family with no `data` argument, even though every other call in the same functions (useOnMon, usePpItem, applyPartyItem) passed it through correctly. partyAction resolves through recordFor, which reads data.gen2ItemEffects when given a dataset and falls back to the module's own built-in RECORDS table when not -- so with no data, a mod's own item_effects record was invisible and every mod-defined Gen 2 field or battle item resolved to a nil action, falling straight through to "isn't going to help here" / "isn't going to help here" without ever opening the party picker. Both now pass the live dataset (self.data on Game2, self.game.data on the battle screen) the same way their sibling calls already did. --- src/core/Game2.lua | 5 +++- src/ui/gen2/BattleState.lua | 6 ++-- tests/gen2_battle_items_test.lua | 48 ++++++++++++++++++++++++++++++++ tests/gen2_field_items_test.lua | 36 ++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/src/core/Game2.lua b/src/core/Game2.lua index 112859eb..7871cbdd 100644 --- a/src/core/Game2.lua +++ b/src/core/Game2.lua @@ -683,7 +683,10 @@ end -- .SelectMon / PPRestoreItem_Cancel carry path: nothing spent. function Game2:usePartyItem(itemId) local ItemEffects = require("src.core.gen2.ItemEffects") - local action = ItemEffects.partyAction(itemId) + -- without the merged dataset this can only ever see RECORDS, the + -- module's own built-ins, so a mod's field item resolves to no action + -- at all and never gets past the .Oak refusal below + local action = ItemEffects.partyAction(itemId, self.data) if not action then return end local party = (self.save and self.save.party) or {} if #party == 0 then diff --git a/src/ui/gen2/BattleState.lua b/src/ui/gen2/BattleState.lua index 2d65eeda..bbbc248c 100644 --- a/src/ui/gen2/BattleState.lua +++ b/src/ui/gen2/BattleState.lua @@ -3024,8 +3024,10 @@ function BattleState:useItem(itemId) -- Everything else the pack can spend on a party mon runs the same -- item_effects.asm routine the field pack runs: the potion line and the -- drinks, the status cures and their berries, REVIVE / MAX REVIVE, and - -- the ETHER / ELIXER family. - local action = ItemEffects.partyAction(itemId) + -- the ETHER / ELIXER family. Without the merged dataset this can only + -- ever see RECORDS, the module's own built-ins, the same gap + -- Game2:usePartyItem had for the field pack. + local action = ItemEffects.partyAction(itemId, self.game and self.game.data) if action then return self:useOnPartyMon(itemId, action) end diff --git a/tests/gen2_battle_items_test.lua b/tests/gen2_battle_items_test.lua index dbfc8527..186e4596 100644 --- a/tests/gen2_battle_items_test.lua +++ b/tests/gen2_battle_items_test.lua @@ -117,6 +117,27 @@ local DATA = { fieldMenu = "ITEMMENU_PARTY", battleMenu = "ITEMMENU_NOUSE" }, OLD_ROD = { id = "OLD_ROD", pocket = "KEY", name = "OLD ROD", fieldMenu = "ITEMMENU_CURRENT", battleMenu = "ITEMMENU_NOUSE" }, + -- a mod's own battle-pack item: its action lives only in + -- gen2ItemEffects below, which ItemEffects.RECORDS (the module's + -- built-in table) has never heard of (#8) + MOD_ITEM = item("MOD_ITEM"), + }, + gen2ItemEffects = { + -- a status cure rather than an HP heal: HP is exposed to the wild + -- mon's own reply once the item spends the turn, which would make a + -- direct before/after HP check depend on incidental battle math this + -- fix has nothing to do with. Status is not. + MOD_ITEM = { + action = "status", field = true, needsTarget = true, + use = function(ctx) + local mon = ctx.mon + if mon.status ~= "poison" then + return { used = false, text = "It won't have\nany effect." } + end + mon.status = nil + return { used = true, text = "MOD ITEM used!" } + end, + }, }, } @@ -236,6 +257,33 @@ do eq(save.inventory.ANTIDOTE, 1, "with the ANTIDOTE untouched") end +-- ---- a mod's own battle-pack item (#8) ------------------------------------- +-- BattleState:useItem asked ItemEffects.partyAction for the item's family +-- with no `data` argument, the same omission Game2:usePartyItem had for the +-- field pack, so a mod item's action -- present only in the merged +-- gen2ItemEffects table -- resolved to nil and the pack fell straight to +-- "That isn't going to help here." instead of opening the party list. +do + local sick = Mon.new(DATA, "CYNDAQUIL", 10, { dvs = perfect }) + sick.moves = { { id = "TACKLE", pp = 35, maxPp = 35 } } + sick.status = "poison" + local screen, _, _, save, pushed = newScreen({ + player = sick, party = { sick }, inventory = { MOD_ITEM = 1 }, + }) + check(runToMenu(screen), "reached the menu") + + screen:useItem("MOD_ITEM") + eq(screen.phase, "submenu", + "a mod's own gen2ItemEffects record opens UseItem_SelectMon") + local picker = pushed[#pushed] + eq(getmetatable(picker), PartyMenu, "and the pick is the party screen") + if picker and picker.onChoose then + picker.onChoose(1, sick) + eq(sick.status, nil, "the mod item's own use() ran through the real screens") + eq(save.inventory.MOD_ITEM, nil, "and the mod item was spent") + end +end + -- ---- IsItemUsedOnConfusedMon: the battle-only arm -------------------------- do local screen, battle, player, save, pushed = newScreen({ diff --git a/tests/gen2_field_items_test.lua b/tests/gen2_field_items_test.lua index c3f9ab68..63e36568 100644 --- a/tests/gen2_field_items_test.lua +++ b/tests/gen2_field_items_test.lua @@ -102,6 +102,24 @@ local DATA = { TM01 = { id = "TM01", name = "TM01", pocket = "TM_HM", index = 191, fieldMenu = "ITEMMENU_PARTY", battleMenu = "ITEMMENU_NOUSE", teaches = "SWIFT" }, + -- a mod's own field item, whose action lives only in gen2ItemEffects + -- below -- ItemEffects.RECORDS (the module's built-in table) has never + -- heard of it, so resolving it at all requires the merged dataset (#8) + MOD_ITEM = { id = "MOD_ITEM", name = "MOD ITEM", pocket = "ITEM", + index = 250, fieldMenu = "ITEMMENU_PARTY", battleMenu = "ITEMMENU_PARTY" }, + }, + gen2ItemEffects = { + MOD_ITEM = { + action = "heal", field = true, needsTarget = true, + use = function(ctx) + local mon = ctx.mon + if mon.hp >= mon.maxHp then + return { used = false, text = "It won't have\nany effect." } + end + mon.hp = math.min(mon.maxHp, mon.hp + 5) + return { used = true, text = "MOD ITEM used!" } + end, + }, }, gen2MenuGfx = {}, gen2Icons = { @@ -461,6 +479,24 @@ do eq(host.save.inventory.HP_UP, nil, "and the HP UP was spent") end +do + -- #8 regression: Game2:usePartyItem asked ItemEffects.partyAction for the + -- item's family with no `data` argument, so it could only ever see + -- RECORDS -- the module's own built-ins. A mod's field item, whose + -- action exists only in the merged gen2ItemEffects table, resolved to a + -- nil action and fell straight through to the "isn't going to help here" + -- refusal instead of opening the party list at all. + local mon = fixtureMon(12, { hp = 10 }) + local host = newHost({ MOD_ITEM = 1 }, { mon }) + host:useFieldItem("MOD_ITEM") + local party = host.stack:top() + check(party ~= nil and party.prompt ~= nil, + "a mod's own gen2ItemEffects record opens the party list") + drive(host, function() return host.stack:top() ~= party end) + eq(mon.hp, 15, "the mod item's own use() ran through the real menu") + eq(host.save.inventory.MOD_ITEM, nil, "and the mod item was spent") +end + do local mon = fixtureMon(12, { statExp = { hp = 25600, attack = 0, defense = 0, speed = 0, special = 0 } }) From 70b9def0b0fed04855c50e160d0abca7e95262bb Mon Sep 17 00:00:00 2001 From: sanjinpepic Date: Sun, 16 Aug 2026 20:31:46 +0200 Subject: [PATCH 9/9] Make Mon.syncIdentity's shiny recompute monotonic syncIdentity unconditionally recomputed mon.shiny from the mon's DVs, overwriting whatever was there. It is wired into refreshStats, which SummaryMenu.new calls on every menu open, so a forced shiny -- Mon.new's opts.shiny path, DVs that do not themselves read as shiny -- got un-shinied the moment the summary screen opened, even though opts.shiny already wins over shiny.roll at construction for exactly this case (a scripted shiny is the cart overriding the roll, not a roll to be hooked). mon.shiny now only ever gets PROMOTED by the DV check, never demoted: `mon.shiny or Mon.isShiny(...)`. A naturally shiny mon and a plain one are unaffected -- the DV check still runs and still decides the first time -- and a mon whose DVs are edited to justify shininess later still promotes normally; only an already-true shiny stops being able to flip back to false on a later refresh. --- src/battle/gen2/Mon.lua | 8 +++++++- tests/engine/gen2_new_seams.lua | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/battle/gen2/Mon.lua b/src/battle/gen2/Mon.lua index 3caa03f5..4b79c460 100644 --- a/src/battle/gen2/Mon.lua +++ b/src/battle/gen2/Mon.lua @@ -112,7 +112,13 @@ function Mon.syncIdentity(mon, data) if mon.dvs then mon.gender = Mon.gender(def, mon.dvs, { species = mon.species, level = mon.level }) - mon.shiny = Mon.isShiny(mon.dvs, + -- shiny is monotonic once true, the same as opts.shiny winning over + -- shiny.roll at Mon.new: a forced shiny (the scripted-shiny path, DVs + -- that do not themselves read as shiny) must not un-shiny the moment + -- this runs again, and it runs on every SummaryMenu open via + -- refreshStats. A mon not already shiny still promotes normally if + -- its DVs justify it, e.g. after an edit. + mon.shiny = mon.shiny or Mon.isShiny(mon.dvs, { species = mon.species, def = def, level = mon.level }) if mon.species == Unown.SPECIES then mon.unownLetter = Unown.letterFromDVs(mon.dvs) diff --git a/tests/engine/gen2_new_seams.lua b/tests/engine/gen2_new_seams.lua index f4a54a75..39a3ed56 100644 --- a/tests/engine/gen2_new_seams.lua +++ b/tests/engine/gen2_new_seams.lua @@ -468,6 +468,32 @@ do T.eq(forced.shiny, true, "opts.shiny still wins over shiny.roll") end) + -- Mon.syncIdentity (wired into refreshStats, which SummaryMenu.new calls + -- on every menu open) used to recompute mon.shiny from DVs unconditionally, + -- so opening the summary screen on a forced shiny -- one whose DVs do not + -- happen to match the natural pattern -- un-shinied it the moment the menu + -- opened. shiny is monotonic once true: a natural roll or a forced one + -- both stay shiny through any later refresh, the way opts.shiny already + -- wins at construction. + do + local forced = Mon.new(DATA, "SEEDMON", 5, { dvs = plainDvs, shiny = true }) + T.eq(forced.shiny, true, "still shiny straight out of Mon.new") + Mon.syncIdentity(forced, DATA) + T.eq(forced.shiny, true, "syncIdentity does not clobber a forced shiny") + Mon.refreshStats(forced, DATA) + T.eq(forced.shiny, true, + "refreshStats (SummaryMenu.new's call) does not either") + + -- the natural cases are unaffected: DVs that read shiny stay shiny, + -- DVs that do not stay plain + local natural = Mon.new(DATA, "SEEDMON", 5, { dvs = shinyDvs }) + Mon.syncIdentity(natural, DATA) + T.eq(natural.shiny, true, "a naturally shiny mon still reads shiny") + local plain = Mon.new(DATA, "SEEDMON", 5, { dvs = plainDvs }) + Mon.syncIdentity(plain, DATA) + T.eq(plain.shiny, false, "a plain mon is not promoted to shiny") + end + local genderCtx withHook("gender.roll", function(nextFn, ctx) genderCtx = ctx