Improve bot hunting routes and party recruitment - #82
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds level-aware hunting rules, controlled spot relocation, gear-source caching, required-party prioritization, scheduler profiles, resolve limits, and population telemetry. It also adds tests for hunting, travel, gear acquisition, party recruitment, and scheduler behavior. ChangesBot hunting and travel
Population scheduling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HuntingState
participant BotSpotTravel
participant BotSession
HuntingState->>BotSpotTravel: Start spot relocation
BotSpotTravel->>BotSession: Start movement or SoE cast
BotSpotTravel->>BotSession: Teleport and assign destination spot
BotSpotTravel->>HuntingState: Complete arrival after settling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
pmbstyle
marked this pull request as ready for review
August 6, 2026 12:53
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/GameServer/Bot/Population/HotActivation.js (1)
75-88: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale saved spot can still relocate a bot that has coordinates.
The comment on lines 76-77 states that coordinates are authoritative. That holds only when
SpotService.findCurrentSpot(state.loc)resolves. If the bot stands in an area with no mapped spot,physicalSpotisnullandspotfalls back tosavedSpot. Line 86 then callsSpotService.randomPointNear(spot, ...)and places the bot at the remote saved field, which is the case this change intends to prevent.Restrict the saved-spot placement path to states without coordinates.
🐛 Proposed fix
for (let i = 0; i < Config.activationPlacementAttempts; i++) { - candidate = spot && !options.playerLoc + candidate = spot && !options.playerLoc && (physicalSpot || !state?.loc) ? SpotService.randomPointNear(spot, Config.activationPlacementRadius) : randomAround(baseLoc, Config.activationPlacementRadius);🤖 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 `@src/GameServer/Bot/Population/HotActivation.js` around lines 75 - 88, Update the spot selection and placement logic around physicalSpot, savedSpot, and the candidate loop so savedSpot is used for randomPointNear only when state.loc is absent. When coordinates exist but findCurrentSpot returns null, use the coordinate-based randomAround path instead of relocating via the stale saved spot.
🧹 Nitpick comments (2)
src/GameServer/Bot/Population/SpotProfiles.js (1)
108-125: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider dropping the unconditional
profiles[0]fallback.Lines 120-125 return
profiles[0]when no candidate matches the level band.profiles[0]is arbitrary and can be a starter field thatSpotService.isSuitablerejects. That result contradicts the level-aware rules this change introduces, andPopulationService.beginHuntingTravelwill then route the bot to a wrong-level ground instead of recordingmissing_spot.Return
nullwhen no suitable or in-band candidate exists, so callers can record the miss.♻️ Proposed change
- return (suitable.length ? suitable : candidates).sort((a, b) => { + const pool = suitable.length ? suitable : candidates; + return pool.sort((a, b) => { const aGap = Math.abs(a.avgLevel - targetLevel); const bGap = Math.abs(b.avgLevel - targetLevel); if (aGap !== bGap) return aGap - bGap; return b.density - a.density; - })[0] || profiles[0] || null; + })[0] || null;🤖 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 `@src/GameServer/Bot/Population/SpotProfiles.js` around lines 108 - 125, Remove the unconditional profiles[0] fallback from the final return in the spot-selection flow, preserving the sorted suitable/candidates result but returning null when neither list contains a valid entry. Keep the existing guided-spot behavior unchanged so callers can record missing_spot when no level-aware candidate exists.src/GameServer/Bot/Population/PopulationService.js (1)
34-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the location guard.
The
hasOwnPropertyclause on line 36 adds nothing. Ifstate.locomitslocX, thenNumber(undefined)isNaNandNumber.isFinitealready returnsfalse. The clause is unreachable protection.The real gap is coercion.
Number(null)andNumber('')both return0, so a state withloc: { locX: null, locY: null }passes the guard.SpotService.findCurrentSpotthen resolves grid0_0and the bot starts travel from the world origin.♻️ Proposed change
const from = { ...(state.loc || {}) }; - const hasLocation = Number.isFinite(Number(from.locX)) && Number.isFinite(Number(from.locY)) - && (Object.prototype.hasOwnProperty.call(from, 'locX') || Object.prototype.hasOwnProperty.call(from, 'locY')); + const hasLocation = typeof from.locX === 'number' && Number.isFinite(from.locX) + && typeof from.locY === 'number' && Number.isFinite(from.locY); if (!hasLocation) return null;🤖 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 `@src/GameServer/Bot/Population/PopulationService.js` around lines 34 - 37, Update the location guard in the population flow to reject null and empty-string coordinate values before numeric coercion, preventing invalid locations from resolving to grid 0_0; remove the redundant hasOwnProperty checks and continue accepting only finite, genuinely provided locX and locY values.
🤖 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 `@src/GameServer/Bot/AI/BotSpotTravel.js`:
- Around line 22-35: Update start in BotSpotTravel.js to validate the resolved
destination coordinates from targetLoc or spot.center before assigning
session.spotRelocation. Reject and return false when the destination is missing
coordinates or any coordinate is non-finite; only create the relocation state
and begin the cast for valid destinations.
- Around line 44-49: The delayed relocation check in the setTimeout callback
must cancel spot relocation before returning from either PK-combat branch.
Update both PK branches to call BotSpotTravel.cancel (or the local cancel
helper) before their returns, preserving the existing combat-interrupt
cancellation behavior and preventing relocation during the SoE cast or flee.
In `@src/GameServer/Bot/AI/SpotService.js`:
- Around line 32-45: Update huntBand to normalize targetLevel and both level-gap
inputs to finite numeric values before calculating min and max, ensuring
Infinity and other non-finite values cannot produce an unbounded band. Preserve
the existing defaults and clamping behavior, and keep eligibleDensity’s level
iteration bounded.
In `@src/GameServer/Bot/AI/States/HuntingState.js`:
- Line 275: Reorder the control flow in HuntingState so the incoming-threat/PK
handling block executes before the session.spotRelocation.arrivalPending early
return, allowing combat, fleeing, and relocation cancellation. Move that
relocation gate below the existing HP/MP recovery check so low-health or
low-resource bots can rest before returning; preserve the documented death-loop
prevention behavior.
- Around line 150-163: Update tickSpotRelocation to use relocation.startedAt as
a maximum-duration deadline, clearing session.spotRelocation and returning false
when the deadline expires before arrival. Apply equivalent expiration handling
to the soe_gatekeeper branch so it cannot remain active indefinitely, while
preserving successful arrival completion and normal walk-command throttling.
In `@src/GameServer/Bot/Population/BotLifeState.js`:
- Around line 1989-2012: Update the cache filtering in coldDueSummary to reuse
the stale equipment-plan predicate used by dueCold(), allowing cold hunting
states with an outdated rateModelVersion to count as due even when nextResolveAt
is later. Apply this check before the nextResolveAt timestamp return while
preserving existing exclusions and summary calculations.
In `@src/GameServer/Bot/Population/PopulationConfig.js`:
- Around line 142-145: Cap the scheduler resolve limits used by
schedulerProfile() at the same 100-state maximum enforced by dueCold(). Update
the configuration handling for schedulerIdleMaxResolvesPerTick and
schedulerPlayerMaxResolvesPerTick, or reuse a shared limit, so configured values
above 100 cannot produce mismatched telemetry or exceed the query cap.
In `@src/GameServer/Bot/Population/PopulationService.js`:
- Around line 1103-1112: Update the lag-budget logic around lagThrottle,
lagAbort, and pressure so configuring schedulerLagThrottleMs still reduces
budget when schedulerLagAbortMs is zero or unset. Use a proportional throttle
fallback based on the configured throttle threshold, while preserving the
existing abort behavior and interpolation when lagAbort is configured.
In `@tests/test_bot_cold_travel_without_spot.js`:
- Line 93: Remove the unused _value and _result parameters from the callback
near the planning request handling, and remove the planningAtlasRequests reset
unless this scenario adds an assertion validating the expected count afterward.
Keep the test behavior unchanged otherwise.
---
Outside diff comments:
In `@src/GameServer/Bot/Population/HotActivation.js`:
- Around line 75-88: Update the spot selection and placement logic around
physicalSpot, savedSpot, and the candidate loop so savedSpot is used for
randomPointNear only when state.loc is absent. When coordinates exist but
findCurrentSpot returns null, use the coordinate-based randomAround path instead
of relocating via the stale saved spot.
---
Nitpick comments:
In `@src/GameServer/Bot/Population/PopulationService.js`:
- Around line 34-37: Update the location guard in the population flow to reject
null and empty-string coordinate values before numeric coercion, preventing
invalid locations from resolving to grid 0_0; remove the redundant
hasOwnProperty checks and continue accepting only finite, genuinely provided
locX and locY values.
In `@src/GameServer/Bot/Population/SpotProfiles.js`:
- Around line 108-125: Remove the unconditional profiles[0] fallback from the
final return in the spot-selection flow, preserving the sorted
suitable/candidates result but returning null when neither list contains a valid
entry. Keep the existing guided-spot behavior unchanged so callers can record
missing_spot when no level-aware candidate exists.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ddea9fd6-ceac-4794-b6bc-cac35a6ba246
📒 Files selected for processing (22)
scripts/run-tests.jssrc/GameServer/Bot/AI/BotDecisionService.jssrc/GameServer/Bot/AI/BotSpotTravel.jssrc/GameServer/Bot/AI/BotStatus.jssrc/GameServer/Bot/AI/BotTargetScorer.jssrc/GameServer/Bot/AI/GearAcquisitionPlanner.jssrc/GameServer/Bot/AI/LevelingRoutes.jssrc/GameServer/Bot/AI/SpotService.jssrc/GameServer/Bot/AI/States/HuntingState.jssrc/GameServer/Bot/Population/BotLifeState.jssrc/GameServer/Bot/Population/HotActivation.jssrc/GameServer/Bot/Population/PopulationConfig.jssrc/GameServer/Bot/Population/PopulationMetrics.jssrc/GameServer/Bot/Population/PopulationService.jssrc/GameServer/Bot/Population/PopulationStatus.jssrc/GameServer/Bot/Population/SpotProfiles.jstests/test_bot_background_party_recruitment.jstests/test_bot_cold_travel_without_spot.jstests/test_bot_gear_acquisition.jstests/test_bot_hunting_ground_rules.jstests/test_bot_population_scheduler_slices.jstests/test_bot_population_state.js
Comment on lines
+142
to
+145
| schedulerIdleBudgetMs: 'BOT_POPULATION_SCHEDULER_IDLE_BUDGET_MS', | ||
| schedulerPlayerBudgetMs: 'BOT_POPULATION_SCHEDULER_PLAYER_BUDGET_MS', | ||
| schedulerIdleMaxResolvesPerTick: 'BOT_POPULATION_SCHEDULER_IDLE_MAX_RESOLVES', | ||
| schedulerPlayerMaxResolvesPerTick: 'BOT_POPULATION_SCHEDULER_PLAYER_MAX_RESOLVES', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cap scheduler resolve limits at the cold-query limit.
dueCold() clamps its limit to 100, but these environment variables can set a larger value. schedulerProfile() then reports the larger limit, while the query returns at most 100 states. This makes coldBatch saturation telemetry incorrect and makes the configured cap ineffective.
Clamp the profile limits to 100, or use one shared limit in both paths.
🤖 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 `@src/GameServer/Bot/Population/PopulationConfig.js` around lines 142 - 145,
Cap the scheduler resolve limits used by schedulerProfile() at the same
100-state maximum enforced by dueCold(). Update the configuration handling for
schedulerIdleMaxResolvesPerTick and schedulerPlayerMaxResolvesPerTick, or reuse
a shared limit, so configured values above 100 cannot produce mismatched
telemetry or exceed the query cap.
Comment on lines
+1103
to
+1112
| const lagThrottle = Math.max(0, Number(Config.schedulerLagThrottleMs) || 0); | ||
| const lagAbort = Math.max(0, Number(Config.schedulerLagAbortMs) || 0); | ||
| return lagAbort > 0 && Metrics.currentEventLoopLag() >= lagAbort | ||
| ? 0 | ||
| : Math.min(budget, Math.max(25, Config.schedulerIntervalMs - 25)); | ||
| let budget = baseBudget; | ||
|
|
||
| if (lagAbort > 0 && lagMs >= lagAbort) { | ||
| budget = 0; | ||
| } else if (lagAbort > lagThrottle && lagMs > lagThrottle) { | ||
| const pressure = Math.min(1, (lagMs - lagThrottle) / (lagAbort - lagThrottle)); | ||
| budget = Math.round(baseBudget * (1 - pressure)); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Lag throttling is silently disabled when schedulerLagAbortMs is unset.
Line 1109 requires lagAbort > lagThrottle. If an operator configures schedulerLagThrottleMs but leaves schedulerLagAbortMs at 0, lagAbort is 0, the condition is false, and no throttling occurs at any lag value. The scheduler then keeps its full budget under load.
Fall back to a proportional throttle when lagAbort is not configured.
♻️ Proposed change
if (lagAbort > 0 && lagMs >= lagAbort) {
budget = 0;
} else if (lagAbort > lagThrottle && lagMs > lagThrottle) {
const pressure = Math.min(1, (lagMs - lagThrottle) / (lagAbort - lagThrottle));
budget = Math.round(baseBudget * (1 - pressure));
+ } else if (lagAbort === 0 && lagThrottle > 0 && lagMs > lagThrottle) {
+ const pressure = Math.min(1, (lagMs - lagThrottle) / lagThrottle);
+ budget = Math.round(baseBudget * (1 - pressure));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const lagThrottle = Math.max(0, Number(Config.schedulerLagThrottleMs) || 0); | |
| const lagAbort = Math.max(0, Number(Config.schedulerLagAbortMs) || 0); | |
| return lagAbort > 0 && Metrics.currentEventLoopLag() >= lagAbort | |
| ? 0 | |
| : Math.min(budget, Math.max(25, Config.schedulerIntervalMs - 25)); | |
| let budget = baseBudget; | |
| if (lagAbort > 0 && lagMs >= lagAbort) { | |
| budget = 0; | |
| } else if (lagAbort > lagThrottle && lagMs > lagThrottle) { | |
| const pressure = Math.min(1, (lagMs - lagThrottle) / (lagAbort - lagThrottle)); | |
| budget = Math.round(baseBudget * (1 - pressure)); | |
| } | |
| const lagThrottle = Math.max(0, Number(Config.schedulerLagThrottleMs) || 0); | |
| const lagAbort = Math.max(0, Number(Config.schedulerLagAbortMs) || 0); | |
| let budget = baseBudget; | |
| if (lagAbort > 0 && lagMs >= lagAbort) { | |
| budget = 0; | |
| } else if (lagAbort > lagThrottle && lagMs > lagThrottle) { | |
| const pressure = Math.min(1, (lagMs - lagThrottle) / (lagAbort - lagThrottle)); | |
| budget = Math.round(baseBudget * (1 - pressure)); | |
| } else if (lagAbort === 0 && lagThrottle > 0 && lagMs > lagThrottle) { | |
| const pressure = Math.min(1, (lagMs - lagThrottle) / lagThrottle); | |
| budget = Math.round(baseBudget * (1 - pressure)); | |
| } |
🤖 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 `@src/GameServer/Bot/Population/PopulationService.js` around lines 1103 - 1112,
Update the lag-budget logic around lagThrottle, lagAbort, and pressure so
configuring schedulerLagThrottleMs still reduces budget when schedulerLagAbortMs
is zero or unset. Use a proportional throttle fallback based on the configured
throttle threshold, while preserving the existing abort behavior and
interpolation when lagAbort is configured.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/GameServer/Bot/AI/BotSpotTravel.js (1)
63-70: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCommit arrival after the teleport completes.
TeleportToreturns immediately and updates the actor position after 1 second. The current code assignscurrentSpotand recordstravel_completebefore that update. Use a completion callback orPromisebefore committing arrival state.🤖 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 `@src/GameServer/Bot/AI/BotSpotTravel.js` around lines 63 - 70, Update the travel flow around TeleportTo so arrival state is committed only after the teleport’s delayed position update completes. Defer SpotService.assignSpot, initialSpawnCoord, townRoutePlan, and spotRelocation updates until TeleportTo signals completion via its supported callback or Promise, while preserving the existing destination and relocation values.
🤖 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 `@src/GameServer/Bot/Population/BotLifeState.js`:
- Around line 24-31: Update hasStaleRateModelPlan() to require plan?.status ===
'active' alongside the existing hunting activity and expectedKills checks.
Preserve the current stale rateModelVersion detection and prevent inactive plans
from being treated as ready for early resolution.
---
Outside diff comments:
In `@src/GameServer/Bot/AI/BotSpotTravel.js`:
- Around line 63-70: Update the travel flow around TeleportTo so arrival state
is committed only after the teleport’s delayed position update completes. Defer
SpotService.assignSpot, initialSpawnCoord, townRoutePlan, and spotRelocation
updates until TeleportTo signals completion via its supported callback or
Promise, while preserving the existing destination and relocation values.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cfa825a0-39e1-45b8-b4fa-5b3870d4c136
📒 Files selected for processing (11)
src/GameServer/Bot/AI/BotSpotTravel.jssrc/GameServer/Bot/AI/SpotService.jssrc/GameServer/Bot/AI/States/HuntingState.jssrc/GameServer/Bot/Population/BotLifeState.jssrc/GameServer/Bot/Population/HotActivation.jssrc/GameServer/Bot/Population/PopulationService.jssrc/GameServer/Bot/Population/SpotProfiles.jstests/test_bot_cold_travel_without_spot.jstests/test_bot_hunting_ground_rules.jstests/test_bot_population_scheduler_slices.jstests/test_bot_population_state.js
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/test_bot_cold_travel_without_spot.js
- tests/test_bot_population_scheduler_slices.js
- src/GameServer/Bot/Population/SpotProfiles.js
- tests/test_bot_population_state.js
- src/GameServer/Bot/Population/HotActivation.js
- src/GameServer/Bot/AI/SpotService.js
- src/GameServer/Bot/Population/PopulationService.js
- src/GameServer/Bot/AI/States/HuntingState.js
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
1 participant
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.
Summary
Root cause
The previous selection path could keep eligible bots on early fields even when better level-appropriate spots existed. Cold travel and gear planning also caused avoidable state churn. For parties, ordinary eligible solo candidates could fill the bounded query before bots with required objectives were considered, leaving requests waiting despite available capacity.
Validation
npm testnpm run check(780 JavaScript files)git diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests