feat(eloot.lic): add guarded room cleanup API - #2457
therealatari wants to merge 7 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Follow-up Verification: 29 room-API examples and all 157 eLoot directory examples pass; RuboCop is clean. @coderabbitai review |
|
|
|
Good catch—the dependency was present, but only as a duck-typed caller contract, so it was much harder to see than the PR description implied. The relationship is now explicit:
I pushed the marker/check, added regression coverage for a missing protocol, and expanded both PR descriptions with the exact seam. |
|
Resolved against current upstream
Verification: 166 eLoot examples pass, all three affected files pass RuboCop, and the focused diff passes whitespace checks. Tests ran with the locally installed Ruby 4.0.5 RSpec/RuboCop executables; Bundler's complete development group is unavailable locally ( |
…room-api # Conflicts: # scripts/eloot.lic
|
Reconciled the guarded room-cleanup API with current upstream
Verification on the merged head:
Latest head: |
mrhoribu
left a comment
mrhoribu
left a comment
There was a problem hiding this comment.
PR Review: feat(eloot.lic): add guarded room cleanup API (#2457)
Reviewed: 3 files, +855/-46 at 8d8e0585 on 2026-09-17. Based on: full local clone of scripts at the PR head, plus the lich-5 checkout at 236a9a2c for the execution-guard contract.
Verdict: Request changes
Summary: Adds ELoot.room_loot / ELoot.restore_room_hands — a lease-protected, execution-guard-gated entry point that reuses eLoot's skin/search/filter/stow machinery for room-only cleanup — plus a process-wide lease acquired at script load and refuse_room_operation! gates on sell/travel/bank/box/pool. The engineering is careful and the guard integration is genuinely correct against lich-5's contract, but the success criterion (verify_room_restoration!) asserts a posture and a hand ordering that the room pass itself never establishes, and a retained recovery record has no clearing path once the character leaves the room — which permanently disables the API for the rest of the Lich process.
Major
1. verify_room_restoration! demands standing, but the room pass never stands you up — scripts/eloot.lic:2552
raise RoomScopeError, 'eLoot equipment restoration was not verified' unless current == originals && standing?standing? is asserted here and established in exactly two places in the whole file: scripts/eloot.lic:6178, inside Loot.skin_obj_types, and scripts/eloot.lic:2618, inside restore_room_equipment!. Neither is on room_loot's success path in the general case:
- Line 6178 runs only when
ELoot.data.settings[:skin_enable]is true andobjssurvives the unskinnable/exclude/bounty filtering inLoot.skin(scripts/eloot.lic:6152–6170). With skinning off, or every admitted corpse filtered out, nothing stands. - Line 2618 is only reachable through
restore_room_hands.
So: a supervisor that is knocked prone during the fight — routine in GS, and the reason Loot.skin carries its own stand loop at all — calls room_loot, the cleanup completes correctly, and then line 2552 raises 'eLoot equipment restoration was not verified'. The caller cannot distinguish that from "your weapon is gone." Worse, with the documented default recoverable: false the ensure at scripts/eloot.lic:2670 discards the recovery record on the way out, so the follow-up restore_room_hands — the one path that would stand you up — raises 'No matching guarded eLoot recovery is available'.
Note also that posture is an absolute requirement rather than a restored original: build_room_recovery (scripts/eloot.lic:2525–2531) captures hands but not posture, so even a caller that was deliberately kneeling gets a failure. Either capture posture in the recovery record and compare it, or issue the same dothistimeout('stand', ...) on the success path that line 2618 already uses. confirmed.
2. A retained recovery record can never be cleared after the character moves, which bricks the API for the process lifetime — scripts/eloot.lic:2653
raise RoomScopeError, 'eLoot equipment recovery is pending; restore it before another room cleanup' if @room_recovery@room_recovery has exactly four assignment sites — scripts/eloot.lic:2659 (set), :2665 (success), :2670 (non-recoverable teardown), :2693 (successful recovery). Nothing else in the file clears it; a standalone ;eloot run does not, ELoot.load does not, and the early raise above happens before recovery is bound, so the ensure at line 2670 can't clear a pre-existing record either (recovery is nil there).
That leaves one exit: a fully successful restore_room_hands. And validate_room_recovery_context! gates that on
unless recovery[:data].equal?(data) && Room.current.id == recovery[:room] && XMLData.room_count == recovery[:epoch]
raise RoomScopeError, 'eLoot recovery context changed'
end(scripts/eloot.lic:2539). XMLData.room_count is incremented by lich-5 on every room-description commit — lich-5/lib/common/xmlparser.rb:579 and :1296 — so any movement makes recovery permanently impossible. A hunting supervisor that takes a guard cancellation mid-skin with recoverable: true and then walks to the next room has, from that moment, an @room_recovery that can never be satisfied and an ELoot.room_loot that raises on every subsequent call until Lich restarts.
Your own specs pin both halves of this: spec/scripts/eloot/room_scope_spec.rb:160–168 asserts 'context changed' after displacement and :242 asserts the record is retained, and :144–158 asserts room_loot refuses while it's pending. What's missing is any tested path out. The 'refusing duplicate' branch in reconcile_room_tool_stows! (scripts/eloot.lic:2598) wedges the same way. An explicit, caller-invokable abandon (ELoot.abandon_room_recovery(owner:) — clears the record, reports what was left held) would close this without weakening the "never resend an uncertain stow" rule you're protecting. confirmed.
3. The hand check is positional; eLoot's own restoration is not — scripts/eloot.lic:2550–2552 vs :4210–4220
originals = recovery.values_at(:right, :left).map { |item| item&.id }
current = [GameObj.right_hand, GameObj.left_hand].map { |item| item&.id }
raise RoomScopeError, '...' unless current == originals && standing?current == originals is an ordered array comparison, so right-hand and left-hand contents must land back in the same hands. But the routine that actually restores READY-list items does not target a hand at all:
10.times {
ELoot.get_res("ready #{ELoot.fix_item_key(ready_item.to_s)}", ELoot.data.get_regex)
sleep 0.2
return true if ELoot.in_hand?(item)
}Inventory.return_ready_list (scripts/eloot.lic:4213–4216) sends a bare READY <slot> and declares success as soon as ELoot.in_hand? sees the item in either hand (scripts/eloot.lic:3406). Inventory.return_hands (scripts/eloot.lic:4193, :4203) routes through it for any item matched by ready_item_key, and only falls back to the hand-specific Inventory.drag(item, 'right'/'left') for non-READY items. So the verifier is strictly stricter than the restorer, and the failure mode is Major 2 again: a "wrong hand" outcome fails verification, and restore_room_equipment! will not fix it either — both items are in originals, so the stow loop at scripts/eloot.lic:2606–2616 skips them and Inventory.return_hands is a no-op (its first line returns early only on an exact positional match, then re-issues the same hand-agnostic READY).
The code-level mismatch is confirmed. Whether it fires depends on one game fact I can't check from here. Settles it: can READY <slot> ever place an item in the opposite hand from where eLoot took it — e.g. an off-hand or 2weapon/ranged slot, or a right hand occupied at the moment the command lands? If yes this is a live bug; if READY is deterministic per slot, use Set/sorted comparison anyway so the verifier stops being tighter than the restorer. suspected on the trigger, confirmed on the inconsistency.
Minor
4. Debug-file logging is disabled on the one path with no live acceptance — scripts/eloot.lic:3067, :3089
return if type == "debug" && (!ELoot.data.settings[:debug] && (!ELoot.data.settings[:debug_file] || room_scope?))
...
if !room_scope? && ELoot.data.settings[:debug_file]
ELoot.data.debug_logger.log(text)
endI understand why: ELoot.data.debug_logger is only constructed in the standalone startup block (scripts/eloot.lic:8207), never in prepare_room_inventory!, so in room scope it is nil. (nil.log wouldn't even raise here — lich-5/lib/common/class_exts/nilclass.rb patches NilClass#method_missing — so this is a deliberate choice, not a crash guard.) But the effect is that the API whose "live room-scoped acceptance remains explicitly pending" is also the only path that produces no debug log, which is exactly the artifact you'll want from the first live run. Constructing the logger in prepare_room_inventory! alongside the rest of the transient state would cost two lines and make the pending acceptance debuggable.
5. The sorter is left running during a room pass — scripts/eloot.lic:8202
# Sorter shows inventory checks so we kill it and restart
ELoot.manage_sorterStandalone ;eloot kills and re-starts sorter at startup for the stated reason that its inventory output interferes. prepare_room_inventory! (scripts/eloot.lic:2562–2578) does not call manage_sorter and there is no refuse_room_operation! on it, so a guarded room pass runs the same ELoot.get_command parsing with the sorter live. The PR body lists "sorters" among rejected operations; the diff contains nothing about sorters either way. If the intent is "the room API must not kill the user's scripts", say so in the docstring and note the parsing exposure — but as written, the room path drops a mitigation the standalone path considers necessary, and the consequence lands in get_command's unbounded rescue => error / retry loop (scripts/eloot.lic:2926–2930).
6. A persisted disk hook runs the room-scoped sleep shim on Lich's downstream thread — scripts/eloot.lic:2855, :2865
The new return if ELoot.respond_to?(:room_scope?) && ELoot.room_scope? in disk_usage prevents installing the parser hook during a room pass, but the hook is registered persist: true (scripts/eloot.lic:2874) and survives the script that created it. Its proc closes over self == ELoot, so its sleep 0.2 while Disk.mine.nil? at line 2865 resolves through RoomScopedWait#sleep (scripts/eloot.lic:2734) — and during an active room pass that becomes ELoot.room_owner.execution_sleep(0.2), i.e. the caller's policy callback is invoked from Lich's downstream reader thread, which lich-5 explicitly scopes out ("The scope applies to this script's worker threads", lich-5/lib/common/script.rb:2963). If the guard is already cancelled, the resulting Interrupted is swallowed by DownstreamHook.run, which removes the hook and prints a Lich error (lich-5/lib/common/downstreamhook.rb:47–52) — self-healing on the next standalone run, but silent disk-tracking loss until then.
Narrow trigger (needs the "A small circular container suddenly appears" line while Disk.mine is nil, mid-pass), so low likelihood — but the shim's reach is wider than "eLoot receivers called from the owner's thread", which is what the comment at scripts/eloot.lic:2732 claims. Gating the shim on ELoot.room_owner.equal?(Script.current) and Thread.current, or extracting that hook body so it doesn't resolve sleep against ELoot, would tighten it.
7. PR title is missing the version, and the changelog entry is out of order and developer-voiced — scripts/eloot.lic:20–26
Three separate things against this repo's conventions (CLAUDE.md, and gh pr list --repo elanthia-online/scripts --state merged):
- The title is
feat(eloot.lic): add guarded room cleanup API. The convention istype(script): vX.Y.Z short description— e.g.fix(eloot): v2.11.9 Hinterwilds sell run no longer walks you to Icemule for the pawnshop (#2473), which is the commit directly under this branch's merge base. Needsv2.12.0, and(eloot)rather than(eloot.lic). v2.12.0 (2026-09-10)sits abovev2.11.9 (2026-09-14), so the list is no longer newest-first by date. The branch predates the merge; re-date it to the day it lands (today is 2026-09-17).- The entries read as API documentation, not player-facing notes — "bounded callers", "the admitted room-cleanup scope". Compare the v2.11.8/v2.11.7 entries right below them. Something closer to
feature: other scripts can now ask eloot to clean up just the current room, without selling or travellingfits the house style. While you're there, drop "sorters" from the safety line — see #5; nothing in the diff touches them.
Nits
- Header re-indentation. The diff shifts
Improvements:,Major_change.feature_addition.bugfix, and the entire untouched v2.11.9 block from 2-space to 3-space indent (scripts/eloot.lic:19–32), leaving them misaligned against v2.11.8 and everything older, and making an unrelated changelog entry show as modified. Nothing parses this (the version regex atscripts/eloot.lic:2954is whitespace-insensitive), but it's noise in the diff and inconsistent in the file. reconcile_room_tool_stows!never marks what it reconciled.scripts/eloot.lic:2586–2599waits out an unsettled attempt and then proceeds without settingattempt[:settled] = true, so a secondrestore_room_handsre-runs the full 2 s observation window per tool. Harmless, just wasted guarded time on the retry path.;force elootnow dies with a backtrace. lich-5 already refuses a duplicate instance and points at;force(lich-5/lib/common/script.rb:159), so;force elootis the supported way to run a second copy. It now hits the top-levelELoot.acquire_room_lease(scripts/eloot.lic:256) and raises an uncaughtELoot::RoomScopeErrorat load, which Lich reports with a trace rather than the perfectly good message inside it. Wrapping that call inrescue ELoot::RoomScopeError => e; ELoot.msg(...); exitwould keep the protection and lose the trace.
Open questions
- Nothing in the repo calls this API.
room_loot,restore_room_hands, andROOM_LOOT_API_VERSIONhave no consumer inscripts,dr-scripts, oreohunter; the body says the Bigshot expansion it was split from has been retired and that EO Hunter does not depend on it. That's ~320 lines of new surface in the repo's largest script, gated behind a lease taken by every;elootrun, with live acceptance explicitly pending. Settles it: which script will call it, and is it on a timeline that would let this land behind its first consumer instead of ahead of it? A maintainer may reasonably prefer to merge the two together so the API's shape is validated by a real caller. - Where does the caller install the guard, and how does it wait for the bootstrap child? The comment at
scripts/eloot.lic:8151says "the caller waits for this exact child to exit before acquiring the room lease" —Script.run_child(..., execution_guard:)(lich-5/lib/common/script.rb:1031) looks like the intended mechanism. Settles it: a short worked example of the caller side in the docstring atscripts/eloot.lic:2622–2634. Without it, the next person to use this has to reverse-engineer the handshake from the spec harness, which stubs the owner rather than demonstrating it. - Is the
@@data = nil unless @room_inventory_readyteardown (scripts/eloot.lic:2671) meant to be observable by a subsequent standalone;eloot? After a failed room pass that never finishedset_inventory,ELoot.datais nil, so the next;elootre-runs the full profile load — which also resets the process-global$sell_ignoreinData#initialize(scripts/eloot.lic:489). Same as today's behavior on a first run, so probably fine; flagging because it's a new way to reach it.
What this PR gets right
Several load-bearing decisions are worth protecting through revision:
- The guard integration is correct against lich-5's actual contract, not an assumed one.
ScriptExecutionGuard::InterruptedsubclassesStandardError(lich-5/lib/common/script_execution_guard.rb:8), so eLoot's pre-existingrescue+retryloops would have swallowed cancellation into an infinite retry. ThreadingELoot.room_checkpoint!(error)through all four of them (scripts/eloot.lic:2927,:3882,:4273, and the RoomScopeError re-raise) fixes that, and it works because everyInterruptedis raised with a latched@reason, so the re-check always re-raises. The:revokedspec case pins it. - The tool-stow journal is marked before dispatch, not after.
record_room_tool_stowis called ahead of the5.times_dragloop instore_item(scripts/eloot.lic:4382), so an exception genuinely cannot prove non-dispatch, andreconcile_room_tool_stows!observes rather than resends. That is the right conservatism for a command channel you can't take back, and the five-way parameterized spec overarrives/observed/missing/revoked/movedis the kind of coverage this deserves. ready_item_keypreserves the legacy crash. Factoring the threeReadyList.ready_list.find {...}[0]call sites into one helper that returnsmatch&.firstonly in room scope, and keepsmatch[0]otherwise (scripts/eloot.lic:3920–3925), means the standalone path's behavior is bit-identical. Easy thing to get wrong in a refactor of this size.refuse_room_operation!is placed before side effects, not after. Each gate is the first statement of its method (sell,go2,silver_deposit,box_loot,pool), and the full-container gate atscripts/eloot.lic:4279fires ahead of thegold ingot/ locksmith branch, so a room pass can't be lured into a sell trip by a full sack. The spec atroom_scope_spec.rb:472checks exactly that.- The spec harness
module_evals the real 8,000-line script into an anonymous namespace rather than stubbing a reimplementation, which is whyroom_scope_spec.rb:86can assert that loading the production file doesn't enter normal startup. That is a meaningful step up from theload_lic_classextraction the rest of the suite uses.
Coverage notes
- Access: full local clone of
elanthia-online/scriptswith the PR head fetched aspr-2457; diff taken against merge basefa4e9b33.ghwas unavailable, so metadata, files, reviews, and comments came from the public GitHub API. Cross-repo verification used the locallich-5checkout at236a9a2c, which already contains theelanthia-online/lich-5#1575surface the PR body lists as a dependency (Script::EXECUTION_GUARD_PROTOCOL,with_execution_guard,execution_guard_active?,check_execution_guard!,execution_sleep,ScriptExecutionGuard,Script.run_child) — so the dependency appears satisfied on current lich-5main. Worth confirming against the minimum Lich the header declares, which is stillrequired: Lich >= 5.15.0. - Ran locally, all green:
ruby -con both changed Ruby files;rubocoponscripts/eloot.licandspec/scripts/eloot/(3 files, no offenses);bundle exec rspec spec/scripts/eloot/(197 examples, 0 failures); and the full suite afterbin/migrate(22,244 examples, 0 failures). Withoutbin/migratethegameobj-dataspecs fail wholesale — environmental, not this PR. - Not verified — needs a live session or the author: every claim about game responses. Specifically whether
READY <slot>can cross hands (Major 3), whether a supervisor realistically callsroom_lootwhile prone (Major 1 — grounded in code, but its frequency is a gameplay question), and the interleaving of sorter output withELoot.get_commandparsing (#5). The PR states live room-scoped acceptance is still pending, and nothing here substitutes for it. - Reviewed by reading, not by tracing at runtime:
Loot.room/Loot.search/Loot.skinand their descendants were read for reachability of the refused operations and forsleepcall sites needing the shim (ELoot,ELoot::Inventory,ELoot::Lootare extended;ELoot::HoardandELoot::Sellare not, and appear unreachable from the room path only becausego2/pool/sellare gated). I did not exhaustively prove that no room-path branch reaches an unextended module'ssleep. - No prior review comments or submitted reviews exist on the PR, so nothing here knowingly repeats or contradicts an earlier reviewer. The author's own comments record a maintainer request (spec organization under
spec/scripts/eloot/) which the diff satisfies.
2 participants
Summary
Lich dependency
Ordinary
;elootbehavior is unchanged. Only callers of the newELoot.room_lootAPI require elanthia-online/lich-5#1575.The dependency is on the script-instance execution-guard protocol introduced there:
Script::EXECUTION_GUARD_PROTOCOL == 1owneris the exactScript.currentand thatowner.execution_guard_active?is trueowner.check_execution_guard!at room-cleanup boundariesowner.execution_sleepeLoot does not install a guard itself. A supervising caller installs the policy with
owner.with_execution_guard; eLoot consumes that already-active authority without broadening or clearing it.This was split from #2456 at maintainer request so eLoot history and blame remain script-scoped. That Bigshot expansion has since been retired in favor of EO Hunter. This PR is therefore an independent composable API proposal, not a current EO Hunter dependency; ordinary eLoot and EO Hunter behavior do not depend on its merge.
eLoot specs are organized under
spec/scripts/eloot/; smaller script suites remain flat.Review hardening
Verification
Live room-scoped eLoot acceptance remains explicitly pending; the completed refuge acceptance used loot disabled.