Fix the walker: obstacle handling, recovery, and route-order correctness - #1827
Open
infuse21 wants to merge 64 commits into
Open
Fix the walker: obstacle handling, recovery, and route-order correctness#1827infuse21 wants to merge 64 commits into
infuse21 wants to merge 64 commits into
Conversation
Release Microbot 2.6.16
…oom door transport Sync of two live-debugged fixes (PluginTesting 5cf454bdab + 829a6a45f6): - The walled-recovery-target guard had its replan cooldown wrapping the ENTIRE check, so during the cooldown window the guard was skipped and recovery clicked the walled tile anyway (through the shop's west wall, parking the player outside while a valid 64-tile route through the door sat unused). The cooldown now throttles only the replan; a walled target always breaks out of the click path (recovery-target-walled-waiting when throttled). - Wydin's food-shop back room (Port Sarim, Pirate's Treasure) door added to transports.tsv — the door is a graph edge the collision map cannot express. - Adds ClockTowerEdgeProbeTest (walker regression probe from the door-route work). Full unit suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sion-table-tested function Sync of PluginTesting a48da124d3. RouteRecovery.decideRecoveryClick(...) makes the recovery block's click/wait/replan choice purely — action-in-flight preemption first, then the walled-target check where the replan cooldown selects REPLAN vs WAIT and can structurally never yield CLICK, else CLICK. Rs2Walker's block is now a flat shell executing the chosen action (identical side effects, logs, exit reasons). RecoveryClickDecisionTest pins every guard interaction as a decision- table row, including both live incidents (Clock Tower stale-click preemption; Port Sarim walled-during-cooldown). Full unit suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ges known + passable Sync of PluginTesting 7c5b33905d. Door-owned edges are now recorded KNOWN + PASSABLE (both endpoint tiles standable) instead of unknown: matches the static map's doorway convention and makes doors the static map wrongly holds as walls routable once seen. Object-doors whose closed footprint blocks their tile stay unknown (rockfall behavior unchanged); curated/learned blocked edges still veto in neighbor generation; no CAPTURE_VERSION bump needed (v2 stores hold no door edges). Tests updated + blocked-footprint safety case added. Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e walled route clicks Sync of PluginTesting 8c57e1324d (Wydin's shop trace): (1) the wrong-traversal blacklist/learn now requires the end sample NOT to be taken while the player is still walking (mid-walk samples poisoned the shop's front door into the learned blocked-edge store; plane changes stay trusted; settled wrong-way displacement still blacklists); all call sites pass Rs2Player.isMoving(); +3 decision rows. (2) Output-side net in the gated route click wrapper: refuse any selected tile Euclidean-near yet absent from the player BFS (route_click_walled diagnostic). Full unit suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…0-tile detour Sync of PluginTesting d58e22d273. Southbound Shantay gate rows now exist ticket- gated (1854) AND currency-gated (5 Coins), so lacking a ticket no longer drops the gate from planning and forces a several-hundred-tile detour with route flapping. Rs2Walker.ensureShantayPassBeforeGate buys the ticket from Shantay at the gate when the inventory lacks one (inventory ticket used as-is; northbound free). Coins in bank qualify under banked-transport walking like charter fees. + ShortestPathCoreTest pin for both row variants. Guardrail baseline regenerated (Rs2Walker lambda renumbering). Full unit suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…hPortal) Sync of the PohPortal backfill from PluginTesting (Stage-4 upstream parity work): Barbarian Outpost, Fishing Guild, Port Khazard, Marim, Civitas illa Fortis, Ourania, Cemetery, Dareeyak, Trollheim, Lassar, Ice Plateau, Paddewwa — arrival tiles from upstream teleportation_portals_poh.tsv with compiler-verified MAG/MARBLE/TEAK object ids (PohPortal 28 -> 40 entries). These flow into the transport graph via PohPortal.findPortalsInPoh() -> PohTransport, so house-portal routing gains the missing destinations. Pathing data only — no quest-helper code. Full unit suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sync of PluginTesting c9c3aa2783: real pathfinder routes asserting the properties past live incidents depended on — Unkah via the ferry, Tempoross cove honest partial, Shantay gate with ticket / with coins / never without, Wydin's shop enterable without circling + its door row pinned in the catalog, GE baseline. Corpus green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s timeout Sync of PluginTesting dfba00bf05: transport settles end once the player stands at the planned destination idle (pure transportSettlePending, unit-tested); door settles end once the door's far-side tile becomes reachable (the edge opened); both keep a 300ms one-tick floor with 900ms demoted to a ceiling. Removes the fixed ~900ms freeze after every door and transport. Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…alador crumbling wall) Sync of PluginTesting b1bde1bbcc: recovery's unified dispatch now takes a transport/agility shortcut whose origin the player already stands on/beside, scanning a window around the player's true raw index — the case TransportResolver declines by design and the normal loop never reached (the far side is an adjacent unreachable frontier, so recovery intercepted every tick and the player oscillated on the origin). Gated to non-instance/POH; inInstance threaded as a parameter so client reads stay on the caller's side. Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… starts stop thrashing Sync of PluginTesting bfc7212454: the transport-refresh memo is now a 4-entry LRU keyed by cache-key hash instead of a single slot, so the banked-walk compare's two alternating config states stay warm (previously every flip was a full ~2.5s re-filter; four per banked start = ~13s). Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… off-path Sync of PluginTesting 9395bafe64: the interim-continuation click logged at debug (invisible) and fired while off-path, sustaining a click->moving->defer-recalc-> stale-interim->click spiral that ran the player minutes down wrong corridors without replanning. All movement clicks now log at info; continuation clicks are gated on isNearPath(); the stuck-sidestep click logs too. Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sync of PluginTesting b73c999863: pure currency transports (charter fares, carpets, the Shantay coin row) have empty itemIdRequirements, so the withdrawal collector's item loop never added their coins — bank trip happened, no gold fetched, post-bank replan dropped the transport. New pure-currency branch sums fares across route hops; regression test pins it. Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sync of PluginTesting 04be8b4bd8 (walker scope only): routine walk-lifecycle exits (cancel:*/interrupted-exception) log at info instead of WARN, so GameChatAppender no longer mirrors every script retarget into the game chatbox; partial_seg demoted to debug (identical lines many times/sec). Genuine anomalies keep WARN + chat visibility. Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ndings Sync of PluginTesting 41d51de75e: Debug Options gains verboseWalkerLogging — flips util.walker + shortestpath loggers to DEBUG at runtime (console only, chat mirror unaffected), applied at startUp and live on config change. Audit: all use* toggles gate their TransportType; buttons consumed by keyName; one dead option identified (preferNonConsumableTeleportAndSpells, value never read). Full suite green on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the walker passive Two passivity bugs from live incidents: Rogue drift (Gu'Tanoth): combat retaliation dragged the player a tile per second for 27s while the walker deferred every off-path recalc with "moving" and preempted every recovery click — busy state had no time bound and no notion of WHOSE movement it was. Busy-state branches (moving/ animating/interacting/recent-movement) now defer, and recovery clicks now yield, only within a 10s ownership window of the walker's last issued action (route/recovery click, door interact, transport handoff). External movement lets the recalc fire and replan from the drift position; the recalc reason "off_path_unowned_movement" makes future incidents self-identifying. Shantay gate: walkWithBankedTransports skipped the whole compare/withdraw pipeline for targets within 100 chebyshev, but straight-line proximity says nothing about the walkable route — the gate is ~30 tiles away and ~700 by inventory-only path, so gold was never withdrawn and the buy-at-gate handler was never reached. The short-circuit now probes the direct path once and falls through to the full bank compare when the path detours past max(60, 3x chebyshev) or goes partial. The flow's silent exits now log at INFO. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e lookup found
Live run: the buy-at-gate step fired correctly but
Rs2Npc.interact("Shantay", "Buy-pass") partial-matched the nearer
"Shantay Guard" (Actions=[Talk-to, null, Pass]) and failed with
"Action not found", so the walker clicked the gate ticketless. Interact
by id (NpcID.SHANTAY) — immune to the name overlap.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… in 115 rows The 2.8s transport refilter was not spread across 5726 rows: ~115 varplayer-gated rows each paid cross-thread client hops per condition, because varplayerChecks read varps live (bypassing every cache) and the global state cache never retains zero-valued varps, so the pre-warm pass never helped exactly the rows that needed it. Capture all referenced varp values in the refresh's existing single client-thread block (the refreshBoostedLevels/refreshCurrencyCache pattern) and check against the snapshot. Expected: filter ~2.3s -> tens of ms; bank-vs-direct compare drops accordingly. Also pins the walk-1 "hairpin" investigation result in the route corpus: the loop was the honest static-map path to a goal tile inside the fenced field (cold reproduction matched live; no gate exists; the executor correctly terminated within-distance). farmFieldGoal_arrivesViaTheHonestTour keeps the field reachable so a collision regression fails loudly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…che on hop The varp put-guard (value > 0) was a leftover from the original Rs2PlayerCache sentinel design where 0 meant 'not cached'; it kept zero-valued varps permanently uncached, so every read paid a cross-thread hop (the 2.3s transport-refresh slowdown). Varbit caching had the opposite bug: unconditional puts could cache stale pre-login reads that the login varp sync never corrects (the server only re-sends non-zero varps). Both paths now cache any value, but only while LOGGED_IN, checked atomically on the client thread. The varbit/varp maps are also flushed on HOPPING and CONNECTION_LOST so values that dropped to zero while out of sync cannot stick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…data
Replace ensureShantayPassBeforeGate (hardcoded object 4031 / item 1854 /
NPC 4642) with a data-driven framework so the next buy-at-the-gate
transport is a TSV row, not code:
- purchasable_items.tsv: item, cost ("5 Coins" transport currency
format), vendor NPC id + action, vendor location, radius.
- PurchasableItemCatalog: lenient loader; forTransport matches when the
vendor stands near the transport origin AND the row is gated on the
item or is its currency twin — free rows (a gate's exit direction)
carry neither, which scopes buying to the paying direction without
coordinate special-cases.
- Rs2Walker.ensureRequiredItemBeforeTransport: generic pre-transport buy
(by NPC id — a name lookup once matched "Shantay Guard" and failed),
logs purchasable_buy | item/vendor.
- Banked planner: an item requirement the bank cannot supply but the
catalog can falls back to withdrawing the fare, so the buy step has
its coins even when the planner held the item row.
Routing is unchanged: transports.tsv keeps the duplicate-row OR pattern
(item row + currency twin) pinned by the existing corpus scenarios.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nger poisons the store A single wrong-traversal observation could permanently blacklist a real door (the Wydin shop door needed a hand-edit to un-poison). Split the two concerns the old design conflated: - Session: the observing session still blocks the edge immediately — it just watched the failure, and anything less loops the walker into the same door. - Persistence: the row is written on probation (strike 1, new TSV columns Strikes / Last strike ms). Later sessions do NOT enforce it on load; a second observation at least 10 minutes independent confirms it to strike 2, which enforces forever. A one-off poison self-heals on restart. Legacy rows without the columns (or hand-copied from blocked_edges.tsv) parse as already-confirmed, so existing files behave identically. LearnedBlockedEdges gains save() (full rewrite for strike bumps); PathfinderConfig keeps row bookkeeping and turns loadLearnedBlockedEdges into a true reload so the test seam can simulate restarts. Pinned behaviorally through learnBlockedEdge's return value + on-disk rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Quantify how much each fresh live capture disagrees with the shipped map so the persistent-live-store decision gets magnitudes instead of anecdotes (the MLM rockfall / kebbit "falsely locked" reports were this class of conflict). LiveCollisionConflicts.tally splits disagreements into liveOpensStatic (stale wall / open door) and liveBlocksStatic (new obstacle / closed door); the plugin logs one throttled collision_conflict line per 30s capture window when nonzero. Runs once per capture on the immutable snapshot — never on the pathfinder hot path. Log-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ndSpells The config audit found its value is read by nowhere — toggling it triggered a path recalculation and then changed nothing. The working equivalent is the "Use teleportation items" enum's non-consumable mode. Also dropped from PATH_REFRESH_CONFIG_KEYS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rRouteState (P1) Twelve fields — settle windows (doorInteractionSettleUntilMs/StartedAtMs, doorSettleFarSideWp), focused-door raw-scan state (rawScanFocusedDoor*), attempt tracking (lastDoorAttemptFrom/To/AtMs), cooldowns (lastDoorEdgePassSkipAtMs, lastDoorPathAdjAttemptAtMs) and the global door throttle (nextDoorInteractionAllowedAtMs) — move into the holder. Mechanical, logic-preserving: same volatile semantics, no external refs (verified per field), compile + walker suite as the net. lastDoorFallbackAttemptAtMs and lastDoorLosAttemptAtMs were dead (declaration only, no reads or writes) and are deleted, not migrated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…outeState (P1) The last scattered cluster: stuckCount, lastPosition, lastMovedTimeMs, prevAnimatingForStuckCheck (the stuck check's evidence that the player is actually moving) plus the two cooldowns recovery consults when it is not — lastWalledRecoveryReplanAtMs and lastPartialTransRecalcMs. Mechanical and logic-preserving, same volatile semantics, no external references (verified per field; the lastPosition hits elsewhere in the tree are unrelated locals). Compile plus the walker suite are the net. With this, the P1 state consolidation is complete: the processWalk loop, recovery and transport handling now share one holder instead of ~45 static fields, which is what makes the WalkExecutor / TransportService extractions possible without threading parameters everywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First live data set: ~44k "conflicts" in every single scene, including ordinary overworld ones reporting zero blocks — consistently 1.4-2.1 planes' worth of the 21632 edges a scene plane holds. Mechanism: SplitFlagMap.get returns false for tiles it has no data for (see CollisionMap.hasRegion's javadoc), so every floorless upper-plane tile reads as solid wall while the live scene's empty collision flags read as walkable. The tally counted all of it. Live-walkable edges on tiles the shipped map seals on all four sides now go to their own sealedOpens bucket, and a sealed-only tally logs nothing. The bucket is reported rather than discarded because a genuinely sealed tile that live says is open is the "falsely locked" rockfall case worth seeing. Regression-tested with the exact noise pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Before using banked transports the walker bootstraps a bank mirror. When
Rs2Bank.getNearestBank returns nothing it logged no_nearest_bank and returned
EXIT, and the caller propagated that — abandoning the entire walk, not just the
banked part. walkTo routes through here, so ordinary walking stopped dead and the
tick re-ran the same failing bootstrap forever:
Finding nearest bank...
Nearest bank point (3026,3506,0) did not match any BankLocation
bank_cache_bootstrap | no_nearest_bank start=(3025,3508,0)
That is a gap in BankLocation coverage, not a routing failure. The mirror only
unlocks transports needing banked items; the ordinary route is usually fine
without it. Continue unbanked instead, and back off for a minute before retrying
— the attempt runs a pathfind, so repeating it every tick cost as much as it
achieved.
The underlying coverage gap around (3025,3508) is left alone: it deserves real
data, not a guessed BankLocation entry.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
getPathAndBankToNearestBank took the last tile of whatever the pathfinder returned and looked for a bank within 2 tiles of it. The pathfinder hands back its BEST EFFORT, not only complete routes: with no bank reachable it returns a partial path that stops short — observed stopping 2 tiles from the player at (3025,3508) while the player was moving and climbing stairs. Nothing matched that stub endpoint, so it logged "did not match any BankLocation", which reads like missing catalog coverage and sent the previous investigation hunting for a bank entry that already existed (EDGEVILLE sits 69 tiles east of that spot). Report the unreachable / partial route as what it is. Second defect fixed by the same change: getPathToNearestBank returns the entry's path, so in this case it handed callers the two-tile stub while its own javadoc promises "empty list if no accessible bank could be reached". Returning nothing from the helper honours that contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…RouteState (P1) lastBankBootstrapMissAtMs arrived as a loose mutable static in Rs2Walker — the pattern the two P1 cluster migrations removed earlier today. Move it into the holder so the consolidation stays whole; the cooldown constant stays a constant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Opening it moves the player through and shuts it behind them — the Al
Kharid toll-gate pattern — so the walker must CLICK it; walking into it
does nothing. Collision cannot reveal this: static and resolved both
report every edge at that tile passable (live capture records door-owned
edges as passable by design), so the planner already routed through and
the executor then had nothing to act on.
Measured live through the agent server rather than inferred — walk onto a
known tile, open, sample position, repeat from the far side:
(3026,3511,p1) -> (3025,3511,p1)
(3025,3511,p1) -> (3026,3511,p1)
Added as a bidirectional pair and pinned in the route corpus. No
requirement columns: none were observed, and guessing them is how a
planner ends up routing accounts through doors they cannot open.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The FIRST plane-change transport of a walk costs ~9.5s while the same kind mid-route costs ~2.2s — measured at 9504ms and 9183ms across two Falador castle runs, against 2200ms for the second staircase in the same walk. That single event is a fifth of a 48s route. The path bounds at 1800ms (did anything start) + 5000ms (did the plane change) + jitter, and a failed start returns false to be retried, so two attempts would explain it. That is inference, and inference has cost three rounds on the door gate today, so measure instead: transport_plane_change reports startWaitMs, planeWaitMs, totalMs and whether it bailed for retry. Log-only; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
doorProbe was still 1219ms in a single scan after the previous commit memoised the segment-independent predicates — that fix helped less than claimed, because the predicate it cached returns false cheaply for ordinary walls. The real cost is wallDoorTouchesSegment, which resolves getWorldLocation() three times per call, over every wall in the scene snapshot, for every route segment. The scan already captured every object's location on the client thread and the probe is already holding it, so pass it in. Adds location-taking overloads for isDoorOnSegment and wallDoorTouchesSegment; the nine existing call sites keep their signature and delegate. Only the probe's hot path is switched over. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The first transport of a walk costs ~12.7s in the segment handler while the same transport mid-route costs ~1.8s; the plane-change waits account for only ~1.5s (three Falador castle runs: 12267ms, 12723ms handler vs 685ms, 1471ms plane change). The handler is entered ~0.3s into the walk, so the missing ~11s is inside handleTransports, before the click. The structural suspect is visible: the 10-tile Rs2GameObject.getAll scan sits inside the loop over candidate transports at the tile, and a staircase tile carries several rows. N candidates would mean N scans. Logs scanMs, candidatesAtTile and matches when a single scan exceeds 400ms, which distinguishes "many scans" from "one slow scan" — the two have different fixes. Log-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…escending stairs Measured: a single transport object scan cost 10938ms, 9599ms and 5483ms in one Falador castle walk. The instrumentation showed candidatesAtTile=1, so it was ONE scan being slow, not many scans. Cause: allowClosedVariant is set only for Climb-down, and when set the predicate resolved an ObjectComposition — a client-thread hop — for EVERY object within 10 tiles that was not an id match, hunting for a trapdoor/manhole/grate/hatch. Inside a castle that is hundreds of round trips. Ascending never took the branch, which is exactly why the user reported descending stairs as slow. Now: id-only pass first (plain field reads, no compositions), falling back to the composition pass only when that finds nothing AND the action is Climb-down, restricted to radius 2 — a closed variant sits on the transport's own tile, never ten tiles away. Live result on the same route: first transport 12021ms -> 2264ms, whole walk 48s -> 40s, and no transport_object_scan slow lines at all. Guardrail baseline regenerated. NOT pure renumbering this time and checked accordingly: splitting one predicate into two moved the same three API calls (getActions, getName, getId) across two lambdas with different captured parameters. Same owning method, no new off-thread call types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Interact-at-range gated the raw scene scan's transport branch, but that branch rarely runs first: the route click puts the player on the transport origin before the scan reaches it. Every transport in the last four live walks was dispatched by handleTransportsInRawSegment or the current-tile handler, and no ranged_transport_dispatch line appeared at all — so the walker still walked its four tiles to the foot of the stairs and only then clicked, which is the exact behaviour the feature was meant to remove. handleTransportsInRawSegment now takes the same shouldDispatchTransportAtRange decision. Route order is held the same way as the scan: the caller says whether this is the nearest segment, and a transport passed over denies the ranged branch to everything behind it. The local-reachability recovery caller passes true — it acts on the edge blocking the player right now, so it is the nearest obstacle by construction. Same guardrails as before: object-interaction transports only, not in instances, not mid-settle, and an edge whose ranged attempt produced no movement falls back to walking onto the origin. Third dispatch path found by grepping every proximity gate rather than fixing the one in front of me — the mistake that cost three rounds on the door gate this morning. Guardrail baseline regenerated; verified pure renumbering (10 added, 10 removed, identical once indices are stripped). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Doors are the last slow part: door_interaction_done at 1779-3181ms, with the waits bounded at 700ms (start) + 2200ms (traversal). The traversal wait already returns the instant the door EDGE resolves, so a wait that runs near its cap means the edge is not being OBSERVED quickly — the live overlay needs a scene recapture before it sees the door open — rather than the door itself being slow. Those are different fixes, and the log currently cannot tell them apart. door_await now reports releasedBy (edge-resolved / arrived-far-side / progress / idle-accepted / timeout) plus the split between the two phases, above a 900ms combined threshold. Log-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…h for a transport
Coming down one staircase left the NEXT one — two tiles away, inside the
existing near band — unhandled. The walker idled, the idle nudge
minimap-clicked onto its origin (2959,3339), and only then did the
current-tile handler take it. That is the "walks the 4 tiles instead of
clicking the stairs" report, caught on video and in the log.
Cause: inside the post-transport window the raw scene scan was passed
allowTransportHandlers=false. But the gate immediately above already skips
the entire scan during that window UNLESS a planned transport is nearby —
so reaching that call inside the window means one IS nearby, and disabling
transport handling there is self-defeating.
The stated reason for the suppression ("broad raw transport scans can
enter long false-negative waits") was the 10s object scan fixed earlier
today: a composition lookup per nearby object whenever the action was
Climb-down. Ranged dispatch also carries a no-progress fallback and a
per-edge cooldown, so an unresponsive transport degrades rather than
loops.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enabling transports in the post-transport window did not fix the second staircase: the scan still returns handled=false with ms=0, i.e. before it captures its ~60ms snapshot, so it is bailing at one of the guards rather than scanning and finding nothing. The idle nudge then walks onto the transport origin, which is the reported "minimap clicks 4 tiles instead of clicking the stairs". handled=false ms=0 cannot distinguish recovery-move-in-flight from interim-active from moving from the empty-scan cooldown, and those have different fixes. post_transport_raw_scene_scan_why now names the guard. Log-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regression from allowing door interaction while moving. The door probe has always searched ten tiles, which was harmless while interaction required standing still — arriving implied proximity. Acting mid-walk removed that implication. Observed at Falador: the walker opened the door at (2985,3341) from nine tiles out while (2981,3340) was still shut directly in front of it. The interaction timed out against the closed near door (door_await releasedBy=timeout traversalWaitMs=2276), the player drifted backwards to (2970,3343), and the walk spent ~15s on recovery clicks before the real blocker was finally handled. While moving, a door must now be within DOOR_APPROACH_INTERACT_MAX_TILES (4) to be interacted with. Stationary behaviour is untouched, so the walker still opens doors at probe range once it has stopped, and the approach win from the moving relaxation is kept for the door it is actually walking into. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g startup Root cause of "runs the four tiles instead of clicking the stairs", found by instrumentation rather than the three guesses before it. currentWalkerPhase() stays STARTUP until the first movement click, and StartupObstaclePolicy.allowBroadRawHandlers() is false — so the raw scene scan is not merely gated, it is never called. The first transport of a walk is routinely taken with no click at all (the player already stands on its origin), so the phase never advances and the scan stays off straight through the SECOND transport. Nothing dispatches it; the walker idles until the idle nudge minimap-clicks onto the origin, and the current-tile handler picks it up from there. That also explains why the earlier fix (enabling transports inside the scan) changed nothing: the scan never ran to reach it. The segment loop already carves out this case with hasImmediatePlannedTransportStep. This is the raw scan's equivalent and is deliberately just as narrow — the scan is allowed during startup only when a planned transport origin sits within the near band of the player. Also fixes the instrumentation that found this: "not-attempted" could not distinguish "policy suppressed the call" from "called and bailed", which is why it read not-attempted every time. Now reports policy-startup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… does The root of the "minimap-walks to the second staircase" symptom, and the reason two carve-outs before it did not land. currentWalkerPhase() only left STARTUP on a movement click, via markFirstMovementClick. A walk that begins standing on a staircase origin takes the transport with no click at all, so the phase never advances and StartupObstaclePolicy keeps the broad raw handlers suppressed through every FURTHER transport on that route. Measured, not inferred: at (2958,3337,p2), two tiles from the next staircase origin, the raw scan reported why=policy-startup on three consecutive passes, and the idle nudge then walked onto the origin. The startup phase exists to hold the broad handlers back until the walker has committed to the route. A transport handoff commits at least as hard as a minimap click, so it ends the phase too. Scoped to the current walk session, since lastTransportHandledAtMs deliberately outlives one. This replaces guessing at gates with fixing the condition that drives them. The post-transport carve-out is reverted in 0c10b483cc — the helper it added returned false in exactly the case it was written for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… negative Rs2Random.gaussRand is an unbounded Box-Muller draw, so mean 300 / dev 120 returns a negative value past ~2.5 sigma — about one call in 160 — and Global.sleep hands it straight to Thread.sleep, which throws IllegalArgumentException and kills the entire walk. Seen live on a Falador castle run: all three plane-change transports had already succeeded and the player was standing on plane 0 when the SETTLE SLEEP after the last one threw, exiting the walker (exception- IllegalArgumentException) into ShortestPathScript auto-retry 1/3. Predates this branch — git blame puts the line upstream in May. It needs a plane-change transport plus a tail draw to fire, which is why it surfaced only now that transports are dispatched reliably. Clamping to a 60ms floor removes only the impossible tail; the jitter this sleep exists to provide is unchanged, so antiban character is preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lly clicked The post-door stall, measured rather than guessed. POST_DOOR_NUDGE_SUPPRESS_TRY_DIRECT_MS (2200ms) exists so a minimap click does not land on the heels of a CANVAS click. It was armed after every door handle — but maybeCanvasNudgeAfterDoor deliberately declines unless the player is on the final approach to the goal (see its own comment: "avoids immediate scene-click jumps after ordinary mid-route door opens"), which means it never clicks for an ordinary mid-route door. So every mid-route door froze movement for 2.2s waiting out a hold-off owed to a scene click that was never made, and the idle nudge then supplied a minimap click anyway — slower AND still minimap, defeating the anti-robotic intent in both directions. Falador castle: door done 15:25:46, idle nudge 15:25:48, twice in one walk. The method now reports whether it clicked, and the full window is armed only then; otherwise a short 500ms beat, enough to keep the next movement off the door interaction's own tick. Guardrail baseline: the two Rs2Walker entries change key only because the return type is part of it (void -> boolean). Same method, same two API calls, 1:1 pairing — no new client-thread exposure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s branch
The cherry-pick of the walker work conflicted only on this generated file.
It was resolved by taking the incoming side to get through the sequence,
which pulled PluginTesting's baseline onto a branch that does not have
PluginTesting's code — so this regenerates it against what is actually here.
The delta is not lambda renumbering and was checked entry by entry:
- Rs2Dialogue#computeHasSelectAnOption / #widgetHasContent / #allChildren
come from the dialogue-caching work, which is not on this branch.
+ Rs2Dialogue#hasSelectAnOption is this branch's uncached version.
- The ObjectComposition#getActions() entries drop out because the
scanner's client-thread inference runs over the whole codebase and
differs between the two branches.
Note for bisect: the intermediate cherry-pick commit carries the incoming
baseline and would fail the guardrail test on its own; the branch tip is
correct. Full suite green here.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… door work playerLoc is read at the top of the processWalk pass. The door/transport cascade that runs before the recovery decision blocks for seconds inside interaction awaits, so by the time recovery runs the position it reasons about can be badly out of date. Measured at Falador castle: recovery_target_walled reported player=(2974,3339) while the early_exit emitted immediately afterwards read the live position as (2985,3340) — eleven tiles and eight seconds apart, spanning two door interactions. It declared the route target walled and discarded the route with a full recalculatePath(), costing a five-second stall for a target that was not walled from where the player actually was. Refreshing playerLoc in place would be worse than leaving it alone: the unreachability verdict for currentWorldPoint, the route index i, and the reachable-tile cache were all derived from the old position, so a fresh position would pair with stale analysis. The pass is abandoned instead and re-derives from the true position on re-entry, like every other break in this block. Self-limiting by construction: with no blocking work the captured position is milliseconds old and the check does not fire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…om a stalled walker
Two ten-second freezes before the same Falador ladder, diagnosed twice from
the log alone, both diagnoses wrong:
- waitUntilIdleAfterSceneWalk with its plane-gated arrival escape. Fixed
and instrumented; the freeze recurred and scene_walk_idle_wait never
printed, so that wait is not on the path at all.
- the interim target held to INTERIM_MAX_AGE_MS. shouldClearInterimTarget
clears at INTERIM_CLOSE_TILES (5) and the player was 2 tiles from the
interim, so it clears immediately and never yields.
Both guesses were possible only because nothing logs in that window: the
post_transport_* tmarks stop 15s after a transport and the rest of the walk
is event-driven, so an idle loop and a blocked thread look identical.
One throttled line per processWalk pass settles it. Heartbeats across the gap
mean the loop is spinning without acting, and the state on each line says
which timer is holding it. Heartbeats stopping means the script thread is
blocked inside a wait, and the last line before the silence says which pass
entered it. Either way the next log answers it without another guess.
One line per second, only while a walk is active.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… from An interim checkpoint could only ever die of old age. shouldClearInterimTarget has three escapes — arrived within INTERIM_CLOSE_TILES, progress stalled for INTERIM_PROGRESS_TIMEOUT_MS, or the 10s hard cap — and the progress timestamp is renewed whenever the ROUTE INDEX advances. A player making honest progress along the route, in the opposite direction to a checkpoint the route has since moved past, renews it every pass, so stale-progress can never fire and the walker yields to a dead checkpoint for the full ten seconds. Caught by the heartbeat, which is the only reason this was visible at all: 18:48:04 at=2961,3349 moving=true interim=2973,3350 interimAgeMs=6539 18:48:05 at=2961,3345 moving=true interim=2973,3350 interimAgeMs=7718 18:48:07 at=2960,3343 moving=true interim=2973,3350 interimAgeMs=8940 18:48:08 at=2960,3343 moving=false interim=2973,3350 interimAgeMs=9999 18:48:08 interim_clear | reason=expired 18:48:09 ranged_transport_dispatch Distance to the checkpoint grew the whole time and the transport dispatch sat behind it. Now cleared once the player is INTERIM_ABANDON_MARGIN_TILES beyond their closest approach, using interimLastDistanceToTarget which was already tracked and never read for this. The margin tolerates rounding a wall on the way in; a 5-arg overload keeps the abandon check inert when no best distance is known, so the existing decision table is unchanged and still green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ined Route order was enforced with "first segment handler to run this pass", which is not the same claim as "nearest unresolved obstacle on the route". Segments skipped by the post-transport window or the startup pre-click gate never touch that flag, so the first segment that actually runs inherits nearest-door status and may click a door at range with an unexamined segment — and any door on it — still ahead of us. That is precisely the failure the interact-at-range rule exists to prevent, and it reproduced on the Falador ground floor: 19:34:47 segment_handler_skip i=11 reason=no_nearby_planned_transport 19:34:47 segment_handler_skip i=12 reason=no_nearby_planned_transport 19:34:51 Found Door (2985,3341) <- door 3, clicked at range 19:34:54 door_await releasedBy=timeout traversalWaitMs=2275 19:34:56 interim close route click ... player=2959,3332 19:35:03 Castle door (2981,3340) <- door 2, which was between us Door 2 was shut, so the server began routing around the building and dragged the player south to (2960,3330); the traversal wait could never be satisfied and burned its full budget. Roughly ten seconds and a visible U-turn. A skipped segment now disqualifies the ranged dispatch, so doors fall back to the stationary requirement — the behaviour from before ranged door dispatch existed — until no earlier segment is being skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…asurement "doorProbe" in the slow-raw-scan line was computed as doorMs minus doorWaitMs, where doorMs wraps the whole handleDoors call and doorWaitMs counts only awaitDoorInteractionProgress. Everything else in door handling therefore landed in it and read as probe cost: the edge-resolution wait, the menu interaction, and the post-interaction verification. The edge-resolution wait alone has been measured at 1897ms in this log (door_edge_wait_done result=FAILED_TIMEOUT), which is larger than the entire figure I have been quoting as "the door probe is slow". Two rounds of geometry optimisation were aimed at that number and moved it far less than expected; this is why. Now reported as four separate figures: doorFind the segment probe itself (snapshot + geometry) doorEdgeWait waitForDoorEdgeResolution doorWait awaitDoorInteractionProgress doorOther the remainder — menu interaction and verification An expensive scan and an expensive wait want opposite fixes, and the old metric could not tell them apart. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WebWalkLog's own contract is "INFO/WARN for outcomes and decisions; DEBUG for per-run volume", and the walker had drifted well off it. A single 45-second Falador walk emitted about 60 INFO lines, of which: 35 tmark phase=post_transport_* several per pass for the whole 15s window 5 Found WallObject door re-logged for the SAME door every pass 5 local reachability miss entry into a check, not its result 2 path-adj blocker-scan: no ... scans that found nothing None of those are outcomes. The outcomes — door_await, door_interaction_done, early_exit, the movement clicks, transport handoffs, slow-scan timings — stay at INFO, as does the blocker scan that actually picks something (score=). New WebWalkLog.tmarkDebug for phase markers that explain a pass rather than report a result; tmark itself is unchanged for lifecycle marks (walk_start, path_snapshot, first_minimap_click). All of it returns under "Verbose console logging", which is where it was useful today and where it should have been all along. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request updates player-state cache invalidation and client-thread reads. It adds catalog-backed purchasable transport handling and banking integration. Pathfinder refreshes now use bounded snapshot caching, and learned blocked edges persist strike metadata. Live collision capture and conflict telemetry are expanded. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/door/Rs2WalkerAwaits.java (1)
62-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the idle-accepted door-await release reachable.
shouldAcceptIdleDoorAwaitonly returnstrueafteredgeResolved, but this call reaches it after theedgeResolvedcase already returns. The idle-stationary release path is unreachable until edge resolution occurs, so it must not require the same edge state. Update the helper/call site/test to accept idle state over time when the edge is still unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/door/Rs2WalkerAwaits.java` around lines 62 - 95, The idle-accepted path in the door traversal await is unreachable because `shouldAcceptIdleDoorAwait` is called only after the `edgeResolved` early return. Update `shouldAcceptIdleDoorAwait` and its call in the traversal wait around `Rs2WalkerAwaits` so elapsed, stationary idle state can be accepted while the edge remains unresolved; preserve the existing edge-resolved, arrival, progress, interruption, and conversation releases.
🧹 Nitpick comments (1)
runelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/ClockTowerEdgeProbeTest.java (1)
13-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test assert an expected route.
The test passes when the target is unreachable at Line 38 and when it is reachable at Lines 42-49. It cannot detect a collision-map regression.
Replace the output-only probe with an assertion after recording the expected result. Otherwise, remove this scratch probe from the test suite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/ClockTowerEdgeProbeTest.java` around lines 13 - 50, Update printStaticRouteIntoTower to assert the expected reachability and route rather than only printing diagnostic output. Record the expected path, assert the target is reachable, and compare the reconstructed route against it; remove the scratch probe behavior and System.out output so collision-map regressions fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/api/playerstate/Rs2PlayerStateCache.java`:
- Around line 76-81: Update onVarbitChanged in Rs2PlayerStateCache so varp and
varbit cache writes occur only when client.getGameState() is LOGGED_IN, keeping
both caches empty in all other states. Add a test that posts a varp event during
HOPPING and verifies the subsequent read calls client.getVarpValue() instead of
returning a cached value.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathConfig.java`:
- Around line 767-780: Update the position of
interactWithRouteObstaclesAtRange() in sectionAdvanced from the conflicting slot
to the unused position 3, preserving maxSimilarTransportDistance() at position 5
and ensuring every item in the section has a unique position.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/dialogues/Rs2Dialogue.java`:
- Around line 323-336: Update keyPressForDialogueOption(int index) so it returns
true only when the dialogue option key press succeeds; do not unconditionally
return true after the digit fallback. Preserve the existing widget-binding
attempt and out-of-range handling, and return the actual result of the fallback
key press so callers such as clickOption(String...) and clickOption(String,
boolean) can retry failed selections.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/banking/Rs2WalkerBankingPlanner.java`:
- Around line 218-234: Update the purchasable fallback in the transport-planning
logic to multiply purchasable.costAmount by requiredQuantity before merging into
itemQuantityMap. Keep the existing currency lookup, logging, and fallback
behavior unchanged so the withdrawal plan requests enough fare for all required
purchases.
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java`:
- Around line 5331-5361: Update the ranged-dispatch branch in the raw-path
transport scan to set sawUndispatchedTransportStep after every handleTransports
attempt, including when it returns false or reports no progress. Keep the
existing rangedAllowed=false handling and ensure later route indices cannot
dispatch past an unresolved ranged transport.
In
`@runelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/live/LiveCollisionConflictsTest.java`:
- Around line 62-65: Update the else branch in LiveCollisionConflictsTest so it
does not assume statik == false implies a partially-open static tile or assert
liveOpensStatic. Preserve the existing one-bucket assertion, or determine the
sealed condition before selecting between liveOpensStatic and liveOpensSealed.
In
`@runelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathCoreTest.java`:
- Around line 265-266: Update the currency-variant check in the relevant
ShortestPathCoreTest test to require getCurrencyAmount() == 5 instead of merely
being positive, and assert that the currency name is exactly "Coins" before
setting hasCoinVariant.
In
`@runelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/WalkerRouteCorpusTest.java`:
- Around line 199-201: Update the negative gate assertion in
WalkerRouteCorpusTest to use the same visits predicate as the positive tests:
check the gate tile at (3304, 3116, 0) with radius 2 and negate that result for
the no-ticket/no-coins case. Remove the separate adjacent-tile checks so
diagonal crossings are detected consistently.
In
`@runelite-client/src/test/java/net/runelite/client/plugins/microbot/util/walker/banking/BankedTransportItemPlanningTest.java`:
- Around line 99-111: Update the pureCurrency selection in
BankedTransportItemPlanningTest to also require Transport types accepted by
Rs2WalkerBankingPlanner.isCurrencyBasedTransport, matching
getMissingTransportItemIdsWithQuantities. Keep the existing currency and
item-requirement conditions, and select only from this aligned set so the
assertion does not depend on unsupported catalog rows.
---
Outside diff comments:
In
`@runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/door/Rs2WalkerAwaits.java`:
- Around line 62-95: The idle-accepted path in the door traversal await is
unreachable because `shouldAcceptIdleDoorAwait` is called only after the
`edgeResolved` early return. Update `shouldAcceptIdleDoorAwait` and its call in
the traversal wait around `Rs2WalkerAwaits` so elapsed, stationary idle state
can be accepted while the edge remains unresolved; preserve the existing
edge-resolved, arrival, progress, interruption, and conversation releases.
---
Nitpick comments:
In
`@runelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/ClockTowerEdgeProbeTest.java`:
- Around line 13-50: Update printStaticRouteIntoTower to assert the expected
reachability and route rather than only printing diagnostic output. Record the
expected path, assert the target is reachable, and compare the reconstructed
route against it; remove the scratch probe behavior and System.out output so
collision-map regressions fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef6e348e-00db-40f7-b678-c0d41a6a9617
⛔ Files ignored due to path filters (2)
runelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/purchasable_items.tsvis excluded by!**/*.tsvrunelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/transports.tsvis excluded by!**/*.tsv
📒 Files selected for processing (33)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/api/playerstate/Rs2PlayerStateCache.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PurchasableItemCatalog.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathConfig.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/LearnedBlockedEdges.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/live/LiveCollisionCapture.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/live/LiveCollisionConflicts.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/bank/Rs2Bank.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/dialogues/Rs2Dialogue.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/PohPortal.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/WebWalkLog.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/banking/Rs2WalkerBankingPlanner.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/door/DoorProbeContext.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/door/Rs2DoorGeometry.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/door/Rs2DoorProbe.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/door/Rs2WalkerAwaits.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/recovery/RouteRecovery.javarunelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/state/WalkerRouteState.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/api/playerstate/Rs2PlayerStateCacheTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/ClockTowerEdgeProbeTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/LiveCollisionTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/PurchasableItemCatalogTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathCoreTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/WalkerRouteCorpusTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/LearnedBlockedEdgeStrikesTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/LearnedBlockedEdgesTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/live/LiveCollisionConflictsTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/util/walker/Rs2WalkerUnitTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/util/walker/banking/BankedTransportItemPlanningTest.javarunelite-client/src/test/java/net/runelite/client/plugins/microbot/util/walker/recovery/RecoveryClickDecisionTest.javarunelite-client/src/test/resources/threadsafety/client-thread-guardrail-baseline.txt
Nine of eleven findings were valid against current code. Correctness: - Rs2PlayerStateCache cached varps/varbits regardless of game state, so a VarbitChanged arriving after the HOPPING flush and before the next LOGGED_IN repopulated the value the flush existed to discard. Writes now require LOGGED_IN; test posts a varp during HOPPING and asserts the next read hits the client. - Rs2Walker: the raw-path ranged dispatch only set sawUndispatchedTransportStep when it DECLINED. An attempt that ran and failed, or reported success without moving us, left the flag clear and a later index could dispatch past the obstacle in front of us. Now set on every unresolved outcome. - Rs2WalkerBankingPlanner: the purchasable fallback withdrew one fare regardless of requiredQuantity, so a step needing two passes was funded for one. - Rs2WalkerAwaits.shouldAcceptIdleDoorAwait ended in `return edgeResolved`, which made it dead code — the traversal wait returns the moment the edge resolves, so it only ever ran with edgeResolved false. A stalled interaction burned the full 2200ms instead of releasing at 1200ms. Movement, animation and the minimum elapsed time still veto. - Rs2Dialogue.keyPressForDialogueOption(int) reported success for an out-of-range index after pressing a digit for an option that was not there. Config: interactWithRouteObstaclesAtRange shared position 5 with maxSimilarTransportDistance; moved to the unused slot 3. Tests: Shantay coin variant now pins amount == 5 and currency "Coins" rather than merely positive; the negative gate test uses the same visits() predicate as the positive ones (the old two-tile form missed diagonal crossings); the banked-fare test selects rows the collector actually accepts (isCurrencyBasedTransport is now package-private for this); ClockTowerEdgeProbe asserts reachability and validates every step of the reconstructed route instead of printing to stdout. Guardrail baseline: one added entry for the new getGameState() call, in a @subscribe handler the baseline already accepts — subscribers are dispatched on the client thread. Skipped: - LiveCollisionConflictsTest bucket assertion: the probe tile is fixed and demonstrably not sealed, and staticTileSealed is private, so branching on it would widen API for a case this test cannot currently hit. - "Return the fallback key press result" in Rs2Dialogue: Rs2Keyboard.keyPress returns void, so there is no result to propagate. Fixed the reachable half. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… on statik Reviewer follow-up, and they were right — I skipped this claiming the test could not determine sealedness without widening API. staticTileSealed is a pure function of four SplitFlagMap.get calls, all public, and the test sits in the same package, so it is trivially mirrored. I asserted rather than read it. statik == false only says the north edge is blocked. If all four edges are blocked the tile is sealed and tally() increments liveOpensSealed, so asserting liveOpensStatic off statik == false could fail on a perfectly correct result. The probe tile is not sealed today, which is why this passed; the assumption was simply never checked. The one-bucket invariant is unchanged and both specific buckets are still asserted, now under the right condition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s failure
Following the reviewer's point about the unconditional-true pattern, audited the
three keyPress sites in Rs2Dialogue. Two were already correct:
pressDialogueOptionWidget guards every step and its return true is the honest
maximum given Rs2Keyboard.keyPress is void, and handleQuestOptionDialogueSelection
already returns that helper's result.
The third was worse than the pattern being discussed. keyPressForDialogueOption
(String, boolean) repeated the raw access:
Rs2Keyboard.keyPress(dialogueOption.getOnKeyListener()[7].toString().charAt(0));
with no check on the listener array being null, having fewer than 8 entries, a
null element, or an empty token. Each of those throws — NPE,
ArrayIndexOutOfBounds, StringIndexOutOfBounds — where pressDialogueOptionWidget
returns false. This overload is what clickOption(String...) calls, so an
unreadable key binding did not merely skip the retry, it took the caller down
with an exception.
Now delegates to the guarded helper, which also removes the duplicated access.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fishing Contest routed through it and stalled: the walker reached (2650,3469), found the gate on the frontier, and logged door_recovery_suppressed on repeat while the quest's real entrance sat unused at (2642,3441). The shipped map has the edge right — static n=false from (2649,3469)/(2650,3469), s=false from the tiles above. The live capture overrides it to passable, because it records door-owned edges as known+passable so ordinary openable doors stop reading as walls. This gate is never openable, so that override turns a wall into a permanent detour the pathfinder keeps choosing. A blocked_edges row is the intended answer for exactly this: it survives the live override and forces the route to the guarded gate the quest actually wants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ort cache key The transport-refresh cache key hashed every inventory, equipment and bank item, so any item change minted a new key — and a new key is a full re-evaluation of ~5,700 transports. Measured over a questing session: 0 hits, 22 misses, every one invChanged=true, at ~2.3s typical and 7.8s worst, on the questing thread inside the tick. Questing is the pathological case because it does both things that defeat this cache at once: churns the inventory constantly (pick up garlic, use garlic, take fish, spend coins) and walks after nearly every step. A fishing script churns items too but walks once a trip, so it only ate the cold start occasionally — which is why this presented as a questing problem when it is a walker one. The same argument was already applied to skills directly above this method: a skill no transport requires cannot change any transport's usability, so it must not participate. Items are no different. Usability depends on item state in exactly four places, all now collected: - transport itemIdRequirements - restriction itemIdRequirements (a separate list, easily missed) - the hardcoded fairy-ring staves and Chronicle, which appear on no row - currency, matched by NAME rather than id That last one is the trap: narrowing to ids alone would silently stop coin changes invalidating the cache, leaving a stale "you can afford this" verdict — a wrong route rather than a crash. Pinned by a test. Falls back to hashing everything while the relevant sets are null, which is the case for the first key of a session (computed before the first refresh), so behaviour there is unchanged. Not yet live-verified: the expected signature is cache_hit climbing above zero with invChanged=false on repeat walks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s never unlock
Completing Fishing Contest unlocks the tunnel under White Wolf Mountain, but the
walker kept routing over the mountain until relog.
The row, the collision map and the pathfinder were all fine — measured with the
corpus harness: the cave is walkable end to end (72 tiles) and a surface crossing
does route under the mountain once the transport is allowed. The failure was the
runtime quest gate.
Rs2PlayerStateCache.getQuestState is a pure cache read with no live fallback. The
map is filled once by populateQuests() on LOGGED_IN and maintained incrementally
by updateQuest(VarbitChanged) — which matched only quests carrying a VARBIT:
.filter(x -> x.getVarbit() != null)
.filter(x -> x.getVarbit().getId() == event.getVarbitId())
Fishing Contest is tracked by a VARPLAYER (QuestVarPlayer.QUEST_FISHING_CONTEST),
and VarbitChanged carries both ids — this cache already reads getVarpId() for its
varp map twenty lines below. So every varplayer-tracked quest stayed frozen at
whatever login saw, and TransportRequirementPolicy.completedQuests fails closed on
a stale or null state (indexOf(null) == -1), dropping the transport silently.
Not Fishing-Contest-specific: this affected every varplayer-tracked quest, so any
quest-gated transport unlocked mid-session was invisible until relog.
Also hoists the two event getters out of the stream filter — they were being
invoked once per QuestHelperQuest value on every VarbitChanged, a few hundred
redundant accessor calls per tick.
Pinned by a matcher decision table, plus two corpus routes: the tunnel must be
used when the gate opens, and must NOT be used without the quest. Guardrail
baseline moves the same two VarbitChanged accessors from the lambda to
updateQuest, which is the hoist.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…quests MICROBOT annotation-only change to the vendored package: varbit already carries @Getter, varPlayer did not, so getVarPlayer() was never generated on this branch. Rs2PlayerStateCache.updateQuest needs it to match VARPLAYER-tracked quests when refreshing cached quest state from VarbitChanged. Without it every such quest — Fishing Contest, and the White Wolf Mountain tunnel it unlocks — stays frozen at its login value and the transports it gates never unlock mid-session. PluginTesting already has this via the quest-helper reorganisation (4198710), which is deliberately not on this branch; this is the one line of it the walker work actually depends on. No behaviour change to quest-helper itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
55 commits · 35 files · +3,452 / −394 · 12 test classes touched
Scoped to
util/walker+shortestpath. Every change was driven by a reproducedlive incident, and the log evidence is quoted in the relevant commit message.
Why
The walker reached obstacles fine but repeatedly failed at deciding what to do
about them: which tile to stand on, which obstacle is next, and whether an
earlier decision is still valid. That produced stalls, backtracking, U-turns and
one hard crash — all of which looked like pathfinding faults but were not. Route
generation was healthy throughout; this PR is almost entirely executor work.
The main behavioural change: act on obstacles at range
Previously the walker detected obstacles up to 13 tiles ahead but only acted
on them from ~0–2 tiles, so it walked to an approach tile it had guessed at, then
interacted. The approach-tile guess was the source of several failures, because
the game server already solves that problem correctly when you click the object.
It now clicks the first unresolved obstacle on the route when it is in range
and lets the server walk us there. "First" is what keeps this safe — clicking a
door with a closed door between makes the server path around the whole building.
Two guards enforce that, both added after the failure they prevent was observed:
a skipped segment was never examined, so a door on it is neither resolved nor
ruled out
past a closed door to open the next one
Measured on the Falador castle route: 56s → 39s, doors handled in route
order, and every
door_await releasedBy=timeoutdisappeared — those turned outto be doors clicked past a nearer closed one, not slow doors.
Correctness fixes
gaussRand(300,120)is unbounded, so the post-plane-change settlesleep went negative roughly one call in 160 and
Thread.sleepthrew, killingthe walk. Pre-existing; needs a plane change plus a tail draw to fire.
before the door cascade ran — observed 11 tiles and 8 seconds out of date, and
it discarded the route on that basis.
index advanced, so a player walking away from one renewed it every pass and it
could only die of old age — holding up a transport dispatch for its full 10s.
starting on a staircase never clicked, never left STARTUP, and kept the broad
obstacle handlers suppressed for the whole route.
learned as blocked from mid-walk samples, and bank-lookup failures aborting
otherwise-valid walks.
Performance
Features and data
Purchasable-transport catalog (
purchasable_items.tsv) — the Shantay passpattern as data rather than hardcoded logic. Two-strike learned-blocked edges so
one bad sample no longer poisons the store. Static-vs-live collision conflict
telemetry.
nearestReachablepicks a tile by route rather than by distance.Observability
All per-pass diagnostics sit behind the existing "Verbose console logging"
toggle. A walk emits ~13 INFO lines rather than ~60; outcomes stay at INFO,
explanations are DEBUG. This matters beyond tidiness — most fixes here were found
by instrumentation, and one metric (
doorProbe) was actively misleading: it wasa residual that absorbed two blocking waits, so the real geometry cost is ~60ms,
not the ~1.4s the old figure implied.
Testing
Full suite green. New/extended decision-table tests for the recovery click,
interim-checkpoint clearing, purchasable transports, learned-edge strikes and
live-collision conflicts.
WalkerRouteCorpusTestpins every live incident as aheadless route regression and is unchanged by this PR — path generation is
not what moved.
Risk and rollback
The interact-at-range behaviour is behind a config toggle, so a misbehaving run
can be reverted live without a rebuild. Everything else is a targeted fix with
its failure case named in the commit message.
Reviewer notes
Two commits sit outside the walker package and are load-bearing:
Rs2PlayerStateCache— caches zero varp values while logged in; required bythe transport-refresh perf work, ships with its own test suite
Rs2Dialogue— presses the option's own key binding; required for guardeddoors, which previously cancelled the conversation they had just opened