feat(go2.lic): v2.5.0 expose trip status to supervisors; bound the restart loop

[Nisugi]

Summary

go2 v2.5.0: make a trip's progress observable to supervising scripts, and stop the restart loop from running forever.

• Go2.status struct: phase, blocked reason, its cause symbol, the failing command, destination, rooms_left, last_room, last_progress_at, restarts, max_restarts, eta, started_at
• every change is emitted as go2.status on Lich::Common::Events when that primitive exists; Go2.on_status is the fallback on older Lich, and both receive a frozen snapshot
• Go2.last_result keeps the final :arrived / :failed / :aborted status after the script exits
• --max-restarts=<N> (default 30, 0 = unlimited) bounds the restart loop with a clear give-up message naming the last blocker
• ;go2 status prints a one-line summary

Motivation

A script driving go2 had one signal, Script.running?('go2'). Travelling out of [Hinterwilds, Beaten Path - 29882] while wounded, the exit's search failed with "You are in far too much agony to do that", go path failed, and go2 restarted itself roughly fifteen times over several minutes. Throughout, the supervisor saw "trip underway". A wedged go2 was indistinguishable from a walking one, and the loop had no exit other than success: error_count was never compared to a ceiling and most $go2_restart = true sites never touched it.

The blocked reason is captured from the game lines since the last room title, first failure line wins, so the root cause is reported rather than the cascade of "You can't go there" that typeahead sends afterwards.

Depends on

Draft until elanthia-online/lich-5#1619 (the Lich::Common::Events primitive) lands. The script runs fine without it: the Events.emit is behind a defined? guard and rescued, and Go2.on_status works on any Lich. Once 1619 is released, supervisors should prefer

Events.on('go2.status', name: 'mysupervisor') { |_topic, st| @need_heal = true if st.phase == :blocked }

because that subscription is cleaned up automatically when the listening script dies.

And elanthia-online/lich-5#1622 for the cause field: when that Lich's move() records Lich::Common::Move.last_failure, go2 takes the exact failing line and its classified cause (:injured, :encumbered, :position, :map, ...) from there instead of scraping the buffer, and ;go2 status prints it. On an older Lich cause is nil and the buffer scrape still supplies reason. go2 itself calls move through the unchanged top-level name, so the bounded retries in #1622 apply to every go2 step with no change here.

Behavior notes

• A nested go2 started for a bank detour reports under the outer trip's status rather than resetting it.
• status_finish emits only the terminal phase; the reset to :idle is silent since it is not a transition a supervisor can act on.
• error_count and its lag-check / timeto logic are unchanged; the new ceiling counts restart cycles.

Verification

• ruby -c and rubocop --only Lint clean
• helper logic exercised standalone with stubs: reason selection after the last room title, hidden-title fallback, progress cursor, restart ceiling, frozen payloads, mutation attempts by a listener, emit sequence on the real events.rb, and a raising Events.emit being logged rather than propagated
• not yet exercised on a live trip; the Hinterwilds case above is the intended manual test

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 510baac0-08ac-4722-b169-9b516b4bfb6e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mrhoribu]

PR Review: feat(go2.lic): v2.5.0 expose trip status to supervisors; bound the restart loop (#2471)

Reviewed: 1 file, +272/-3 at 3d50ac6 on 2026-09-17. Based on: full clone of scripts plus the lich-5 checkout at 236a9a2c (which already carries lib/common/events.rb and lib/common/move.rb, i.e. the lich-5#1619/#1622 surfaces this PR depends on).
Verdict: Request changes — one blocker that stops go2 loading at all on GemStone.
Summary: Genuinely useful observability work with a clean design (read model + edge events, frozen snapshots, graceful degradation on older Lich), but the Status constant guard collides with Lich::Gemstone::Status and takes the whole script down on GS, and three smaller issues undercut the signal the feature exists to provide.

---

Blockers

1. const_defined?(:Status) finds Lich::Gemstone::Status, so Status is never defined and go2 dies at load on GemStone — scripts/go2.lic:266-268

  Status = Struct.new(:phase, :reason, :cause, :command, :destination, :rooms_left, :last_room,
                      :last_progress_at, :restarts, :max_restarts, :eta, :started_at,
                      keyword_init: true) unless const_defined?(:Status)
  @@status ||= Status.new(phase: :idle, restarts: 0, max_restarts: 0)

Module#const_defined? defaults to inherit: true, and for a module receiver that search falls back to Object and its ancestors. On a GemStone session Lich runs include Lich::Gemstone at the top level of lib/main/main.rb:232 (and again at :581 / :667 for the pipe and --gemstone paths), which puts Lich::Gemstone into Object's ancestry; lib/gemstone/infomon/status.rb — required unconditionally by GameLoader.gemstone at lib/common/gameloader.rb:31 — defines Lich::Gemstone::Status. So inside module Go2, const_defined?(:Status) is true, the Struct.new is skipped, and line 268 resolves Status to the Gemstone module.

Reproduced against Ruby 4.0.5 with nothing but the two lines of context Lich supplies:

$ ruby repro.rb
repro.rb:11:in '<module:Go2>': undefined method 'new' for module Lich::Gemstone::Status (NoMethodError)

This is at module-body scope, above every CLI branch, so it is not a trip-time failure — ;go2 <anything> stops working for every GemStone character the moment this merges. DragonRealms is unaffected (Lich::DragonRealms has no Status), which is also why a DR-side smoke test would not catch it. Ironically the PR's own motivating case (Hinterwilds) is GS.

Fix: pass inherit: false — unless const_defined?(:Status, false) — or sidestep the lookup entirely with Go2.const_defined?(:Status, false). BLOCKED_LINE at scripts/go2.lic:384 uses the identical guard; no top-level BLOCKED_LINE exists today, but it is the same latent trap and worth fixing in the same pass. This is the same class of bug as the briefcombat fix merged last week (7fd0b971, "prevent constant redefinition on subsequent runs"), so it is probably worth a repo-wide convention rather than a one-off.

---

Major

2. ;go2 status is refused while a trip is running, and ;force go2 status perturbs the trip it is reporting on — scripts/go2.lic:1383-1386

  elsif Script.current.vars[1] =~ /^status$/i
    echo Go2.status_summary

Script.start rejects a second instance of a running script (lich-5/lib/common/script.rb:159: "--- Lich: go2 is already running (use ;force [scriptname] if desired)."), so during a trip — the only time the status is interesting — ;go2 status prints that refusal instead of the status. The help text added at scripts/go2.lic:1094 doesn't mention ;force.

;force go2 status does run, but the dispatch at 1383 sits well below the module body's side effects, so a status query mutates live trip state before it prints anything:

• scripts/go2.lic:224 — @@mounted = false is unconditional. If the running trip is mounted, Go2.mounted is cleared underneath it; the trip's unless standing? or ... or Go2.mounted guard then fires a stand, gets "You cannot do that while mounted.", and if UserVars.mapdb_use_urchins is on that path now also flips urchins off and forces a re-path (:2596-2604).
• scripts/go2.lic:1199-1201 — the forced instance registers before_dying { update_urchin_expire.call }, which sends urchin status to the game on exit (:1180-1196).

Both are pre-existing hazards of the dispatch's position (the bank-detour force_start_script at :2493 already trips the @@mounted one), but status is the first subcommand whose entire purpose is to be run mid-trip, so it turns a latent edge into the normal usage. Two options: move the status branch above the module-body side effects, or drop the subcommand and document ;e echo Go2.status_summary, which reads the same class variables with no script start at all.

3. A bank-detour go2 that hits the ceiling declares the outer trip failed — scripts/go2.lic:2397-2403

      if max_restarts > 0 and Go2.status.restarts > max_restarts
        ...
        Go2.status_finish(:failed)
        exit
      end

Every other terminal-state site is guarded against the nested case — status_reset and before_dying behind unless $go2_started_go2_bank at :2371-2382, and the arrival at :2785 — but this one is not. The nested instance started by force_start_script at :2493 runs the same code with $go2_started_go2_bank true and shares @@status with its parent, so when a bank detour exhausts the ceiling it: emits go2.status with phase: :failed, writes Go2.last_result, and replaces the live @@status with a fresh idle struct.

The outer go2 does not check the detour's outcome — it falls through to unless (start_room = Room.current) at :2498 and keeps walking. From that point its destination, started_at and max_restarts are gone from the status, restarts has been zeroed, and its eventual status_finish(:arrived) at :2785 reports arrival with a nil destination. A supervisor subscribed to go2.status — the intended consumer — is handed a terminal :failed for a trip that is still in progress.

Same root, smaller edge: Go2.status.restarts += 1 at :2396 increments the shared counter, so a nested bank trip inherits the outer's restart count and compares it against its own local max_restarts. A go2 that has already restarted 30 times will make its bank detour give up on the first restart, and the message will read "giving up after 31 restarts".

Guarding this site the same way the others are (unless $go2_started_go2_bank, echoing and exiting without touching status) fixes both.

4. notify_status iterates the hook hash unprotected, so registering a hook during an emit raises — scripts/go2.lic:319

    @@status_hooks.each_value { |blk|
      begin
        blk.call(snapshot)
      rescue StandardError => e

Ruby raises RuntimeError: can't add a new key into hash during iteration when a key is added to a Hash that is being iterated, and Go2.on_status at :289 is a bare @@status_hooks[name] = block with no mutex. Delivery is synchronous on go2's thread and hooks are expected to do real work, so the GVL is released inside blk.call and another script's thread can land its registration in that window. Confirmed on Ruby 4.0.5:

race.rb:4:in 'block in <main>': can't add a new key into hash during iteration (RuntimeError)

Two shapes: a supervisor calling Go2.on_status from its own thread while go2 is mid-emit dies at registration with that error (the raise lands in the registering thread), and a hook that registers a new named hook from inside its own callback raises out of each_value on go2's thread — outside the per-hook begin/rescue, so it propagates into the movement loop and kills the trip. That directly contradicts the guarantee stated at :307-309 ("Neither route may raise into the movement loop, whatever a listener ... does"). Re-registering under an existing name is fine (no new key), so this only bites first registrations — i.e. supervisor startup, which is exactly when it happens.

@@status_hooks.values.each { ... } closes it; a mutex around on_status / off_status / the snapshot would be tidier. (Deletion during iteration is legal in MRI, so off_status is safe as written.)

---

Minor

5. Restart increments are invisible to event subscribers — scripts/go2.lic:2396

Go2.status.restarts += 1 mutates the struct directly rather than going through status_update, so nothing is emitted for it. The following Go2.status_update(phase: :pathing) unless Go2.status.phase == :blocked at :2405 re-emits and carries the new count along — except when the phase is :blocked, which is the wedged case this PR was written for. A supervisor watching go2.status events on the Hinterwilds scenario therefore never sees restarts climb; only pollers of Go2.status do. Routing it through status_update(restarts: Go2.status.restarts + 1) would make the two views agree.

6. The "frozen snapshot" does not protect String fields — scripts/go2.lic:311

snapshot = st.dup.freeze freezes the struct, not its members, and reason and command are the same String objects the live status holds. snapshot.reason << "..." in a listener mutates go2's own state. Verified on 4.0.5: after a listener appends to snapshot.a, the live struct reads back the mutated string. The comment at :305-307 ("nothing they do can alter go2's own trip state") overstates what the freeze buys. Either freeze the strings when they are set, or soften the comment.

7. on_status hooks are never reaped when the registering script dies — scripts/go2.lic:286-296

The PR body correctly notes that Events subscriptions are cleaned up by ScriptDeath when the owner dies (lich-5/lib/common/events.rb:31-36), but @@status_hooks has no equivalent. A supervisor that crashes without calling off_status leaves its block registered for the life of the Lich process, invoked on go2's thread on every future trip, holding its dead script's binding alive. Recording Script.current&.object_id alongside the block and skipping (or dropping) entries whose owner is no longer in Script.running would match the Events contract the docstring holds itself up against.

8. @@seen_move_failure is process-wide but Move.last_failure is per-thread — scripts/go2.lic:271, 344-350

Lich::Common::Move stores the record in Thread.current[LAST_FAILURE_KEY] (lich-5/lib/common/move.rb:76-84) precisely because several scripts may be moving one character at once. @@seen_move_failure is a class variable, so the nested bank instance's fresh_move_failure writes over the parent's cursor with a Failure from a different thread. Since the comparison is equal?, the parent's next mark_blocked can then treat its own already-consumed failure as fresh and report a stale reason once. Narrow, and the cursor self-corrects on the next real failure — but a Thread.current[:go2_seen_move_failure] would match the primitive it is tracking.

9. Changelog entry is longer and more technical than this repo's convention — scripts/go2.lic:17-29

Thirteen lines naming Lich::Common::Move.last_failure, Events.on('go2.status') and "Lich 5.22+". The surrounding entries (2.4.0, 2.3.0) are one to three player-facing lines; the module docstring at :235-263 and the PR description already carry the API detail for script authors. Something closer to "go2 now reports trip progress to other scripts, and gives up after 30 restarts instead of looping forever (--max-restarts=<N>)" fits the file better.

---

Nits

• scripts/go2.lic:1094 — the ;go2 status help line is inserted between --max-restarts and --disable-confirm=<on|off>, inside the flag block, rather than with the other subcommands.
• scripts/go2.lic:2553-2554 — two consecutive status_update calls emit two events for one logical transition; merging them into one call would halve the churn on every re-path.
• scripts/go2.lic:2564-2571 — checksleeping / checkbound / checkstunned / checkwebbed are each called twice, once to build muckled_by and once for the echo. Building the list and echoing from it would keep them in sync if a future check is added.

---

Open questions

• status_summary on last_result after a nested failure. If finding 3 is fixed by guarding the nested status_finish, does the outer trip still want a way to know its detour gave up? Right now it doesn't check, and the detour's failure is silently followed by "You're too poor to go to the bank" on the next pass through :2486. Settles it: is a failed bank detour meant to abort the outer trip, or to fall through as it does today?
• mark_blocked in the StringProc branch (:2663). Go2.mark_blocked(last_sent) if Room.current.id == room_id_before_proc and room_id_before_proc != next_id.to_i marks blocked whenever the proc left us in the same room. Are there wayto procs that legitimately take more than one path step, or that return to the same room as an intermediate state (a lift, a boat, a queue)? Settles it: one example of a multi-step StringProc in the map DB that would false-positive here — if none exists, the check is sound as written.
• --max-restarts default of 30 against the reported failure. The Hinterwilds case restarted ~15 times over several minutes; 30 restarts is therefore roughly ten minutes of wedged walking before the ceiling fires. Settles it: is ten minutes the intended patience, or was 30 chosen as "comfortably above normal" without a wall-clock estimate?

---

What this PR gets right

• The read-model-plus-edge-events split is the right shape, and it matches what lich-5/lib/common/events.rb:44-48 explicitly recommends. Polling Go2.status and subscribing to go2.status are both first-class rather than one being a bolt-on.
• Degradation is handled properly everywhere it matters: defined?(Lich::Common::Events), Move.respond_to?(:last_failure), and defined?(Lich.log) all guard, and blocked_reason's buffer scrape is a real fallback rather than a stub. The script genuinely runs on an older Lich.
• blocked_reason picking the first failure line after the last room title is the non-obvious detail that makes the reason field worth having — reporting the typeahead cascade instead of the root cause would have made the whole feature noise.
• Preserving :blocked across re-paths (:2405, :2554) so the phase only clears when a room actually changes in note_progress is the correct semantics, and easy to get wrong in the other direction.
• Movement semantics are left alone. The room_id_before_proc comparison is explicitly status-only with $go2_restart = true still commented out at :2665, which keeps this reviewable as an additive change.
• ruby -c and rubocop (repo config, Ruby 4.0.5) are both clean — confirmed locally, not taken on trust.

---

Coverage notes

• Full clone of elanthia-online/scripts at 9a8e789b with PR head 3d50ac62 fetched, plus the local lich-5 checkout at 236a9a2c for the framework contracts (Script.start, Script#vars, Events, Move, GameLoader, StringProc).
• ruby -c → Syntax OK. rubocop with the repo's .rubocop.yml (TargetRubyVersion 4.0, custom ascii_only_source cop) → no offenses. Ruby 4.0.5 via rbenv.
• Findings 1, 4 and 6 were reproduced with standalone Ruby 4.0.5 scripts rather than reasoned about; findings 2 and 3 were traced through the file and the lich-5 source but not run against a live game. Nothing here was exercised on a real trip.
• Not reviewed: GUI paths, the Gtk setup block, and the pre-2.5.0 body of go2.lic except where the diff touches it. The Lich::Common::Move.last_failure field names (dir, line, cause) were verified against lich-5/lib/common/move.rb:43 in this checkout; if lich-5#1622 changes that Struct before merging, mark_blocked at :401-407 needs re-checking.
• The PR is still marked draft and blocked on lich-5#1619/#1622 per its own description, so the dependency gap is the author's stated position, not a finding.

[Nisugi]

Thanks for the review, and for the reproductions. Everything is addressed at e3851ca1.

Blocker 1. The struct is Go2::TripStatus and both guards pass false to const_defined? (0b141660).

Major 2. The status dispatch runs at the top of the module, before @@mounted = false and the urchin before_dying, so ;force go2 status reads and exits without touching the live trip. Help lists it with the other commands and says ;force.

Major 3. A nested bank detour counts its own restarts, and when it gives up it records why and exits without status_finish. On the open question: a failed detour now fails the outer trip. The silvers were needed to make the trip, so walking on only meant being refused at the first toll; the outer finishes :failed with reason bank detour failed: <why>. Not having enough in the bank fails the same way instead of an exit that read as :aborted.

Major 4. @@status_lock guards on_status/off_status; notify_status snapshots and copies the hook list under it and calls hooks outside it. Reproduced your race first, then confirmed it gone.

Minor 5 to 9. Restart increments go through status_update so subscribers see the count climb while blocked; String fields are frozen on write (your << case now raises FrozenError, live state untouched); hooks record their registering script and are dropped when it is no longer in Script.list (not Script.running, which would reap a hidden supervisor); the failure cursor is Thread.current; the changelog is two player-facing lines. Nits: help line moved, the re-path emits merged, the muckled echo reads from the same list as the reason.

Patience. Agreed that ten minutes is far too long, and a restart count is a poor clock. There is now --max-stuck=<seconds> (CharSettings max_stuck_seconds, default 60, 0 = never), measured from the last room change and checked beside the restart ceiling, which stays at 30 as a backstop. Either ends the trip the same way: echo the last blocker, status_finish(:failed) with the reason, exit, and the supervisor decides from Go2.last_result or the go2.status event.

StringProc. I could not settle it. The check is status-only, so a proc that legitimately ends in its starting room would show a transient :blocked that clears on the next room change; left as written unless you know of a case.