Harden hot bot party reliability - #77
Merged
Merged
Conversation
📝 WalkthroughWalkthroughParty companion invitations, loot pickup, detachment, background-party lifecycle recovery, activation cleanup, near-player eligibility, and critical-health retreat behavior were updated with corresponding regression coverage. ChangesParty companion flow
Background-party lifecycle
Companion survival behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerSession
participant World
participant BotSession
participant PartyCompanionService
PlayerSession->>World: askForTeamUp
World->>BotSession: store pendingPartyInvite
BotSession->>World: answerForTeamUp id=1
World->>PartyCompanionService: inviteBotCompanion
PartyCompanionService-->>PlayerSession: joinParty result
sequenceDiagram
participant HotActivation
participant PartyState
participant LifeState
participant BotManager
HotActivation->>PartyState: dissolve background party
HotActivation->>LifeState: clear and refresh party state
HotActivation->>HotActivation: verify release and reserve activation
HotActivation->>BotManager: loadAndSpawnBot
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
July 30, 2026 14:43
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/test_party_bot_loot.js (2)
304-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hard-coding
pickupCalls[10].Capture the call index immediately before
reconcileGroundLoot; otherwise unrelated fixture changes can make this test inspect or complete the wrong pickup.Proposed fix
+ const pickupCallIndex = pickupCalls.length; PartyCompanionService.reconcileGroundLoot(botSession); assert.deepStrictEqual( - pickupCalls[10] && { session: pickupCalls[10].session, actor: pickupCalls[10].actor, data: pickupCalls[10].data }, + pickupCalls[pickupCallIndex] && { session: pickupCalls[pickupCallIndex].session, actor: pickupCalls[pickupCallIndex].actor, data: pickupCalls[pickupCallIndex].data }, { session: botSession, actor: closestBot, data: { id: 500011 } }, 'By Turn loot should still be collected from the ground by an available companion' ); - pickupCalls[10].onComplete(); + pickupCalls[pickupCallIndex].onComplete();🤖 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 `@tests/test_party_bot_loot.js` around lines 304 - 311, Update the test around PartyCompanionService.reconcileGroundLoot to capture the pickupCalls length immediately before reconciliation, then inspect and complete the newly added call using that captured index instead of hard-coded pickupCalls[10].
291-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover distribution 4 as well.
The production change enables distributions 3 and 4, but this regression only exercises 3. Parameterize the case over both modes or add a separate By Turn Including Spoil case.
🤖 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 `@tests/test_party_bot_loot.js` around lines 291 - 295, Extend the regression test around leaderSession.partyCompanionSettings to cover distribution modes 3 and 4, preferably by parameterizing the existing case while preserving its current assertions and setup for each mode.src/GameServer/Bot/Population/HotActivation.js (1)
229-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
craft_recipe_sync_failedover-reports. The flag stays true for everything after the craft profile lookup, so aloadAndSpawnBotthrow is also reported as a recipe-sync failure (and the reservation is dropped even though a spawn may have been requested). Scope the flag to theensureRecipescall itself.♻️ Narrow the flag to the recipe sync
- const recipesReady = craftShop - ? CraftShopService.ensureRecipes(state.characterId, craftShop) - : Promise.resolve(); + const recipesReady = craftShop + ? CraftShopService.ensureRecipes(state.characterId, craftShop) + .catch((error) => { craftActivation = true; throw error; }) + : Promise.resolve();with
craftActivationinitialized tofalseand no longer set from!!craftShop.🤖 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 229 - 233, Scope the craft-specific failure flag to the ensureRecipes call in the activation flow: initialize craftActivation to false and do not set it from !!craftShop during the profile lookup. Set it only while ensureRecipes is executing, so loadAndSpawnBot failures retain activation_prepare_failed handling.
🤖 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/States/FollowingState.js`:
- Around line 444-459: Update retreatFromThreat so the rooted case is handled
before the retreatInProgress early return: clear session.partyRetreatUntil,
abort active automation, and return false when rooted. Only preserve the
existing retreat route and return true for non-rooted retreatInProgress cases.
In `@src/GameServer/Bot/Population/HotActivation.js`:
- Around line 124-134: Update the cached-state fast path in the promise callback
to normalize refreshed.activity from 'grouped' to 'hunting' before returning it,
matching the constructed state path while preserving all other refreshed state
fields.
- Around line 118-136: Update releaseBackgroundParty to compensate when the
post-setStatus clearParty validation fails: restore the background party’s prior
active status or enqueue the established retry/reconciliation path before
propagating the release error. Keep the existing successful refresh and
state-clearing behavior unchanged, and ensure compensation covers both a zero
clear count and a remaining cached party link.
In `@src/GameServer/World/World.js`:
- Around line 317-336: Update the invitation-answer handling around
pendingPartyInvite to preserve the existing player-to-player response flow:
validate and consume the pending invite, then route human invitations through
the normal party-formation path instead of inviteBotCompanion. Call
inviteBotCompanion only when the pending invitation targets a bot, while
retaining the existing rejection behavior for invalid or declined answers.
---
Nitpick comments:
In `@src/GameServer/Bot/Population/HotActivation.js`:
- Around line 229-233: Scope the craft-specific failure flag to the
ensureRecipes call in the activation flow: initialize craftActivation to false
and do not set it from !!craftShop during the profile lookup. Set it only while
ensureRecipes is executing, so loadAndSpawnBot failures retain
activation_prepare_failed handling.
In `@tests/test_party_bot_loot.js`:
- Around line 304-311: Update the test around
PartyCompanionService.reconcileGroundLoot to capture the pickupCalls length
immediately before reconciliation, then inspect and complete the newly added
call using that captured index instead of hard-coded pickupCalls[10].
- Around line 291-295: Extend the regression test around
leaderSession.partyCompanionSettings to cover distribution modes 3 and 4,
preferably by parameterizing the existing case while preserving its current
assertions and setup for each mode.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 514dedaf-789f-415f-a1ed-f05fd88824a8
📒 Files selected for processing (11)
src/GameServer/Bot/AI/PartyCompanionService.jssrc/GameServer/Bot/AI/States/FollowingState.jssrc/GameServer/Bot/BotManager.jssrc/GameServer/Bot/Population/BotLifeState.jssrc/GameServer/Bot/Population/HotActivation.jssrc/GameServer/Bot/Population/PopulationService.jssrc/GameServer/World/World.jstests/test_bot_background_party_recruitment.jstests/test_bot_population_state.jstests/test_party_bot_loot.jstests/test_party_companion_rest_follow.js
Comment on lines
+444
to
+459
| function retreatFromThreat(session, bot, threat, player, rooted) { | ||
| const retreatInProgress = Date.now() < Number(session.partyRetreatUntil || 0) && | ||
| (!!session.moveTimer || bot.state?.fetchTowards?.()); | ||
| session.currentTargetId = undefined; | ||
| bot.unselect(); | ||
| bot.attack?.abortCast?.(session, bot); | ||
| bot.attack?.clearTimers?.(); | ||
| bot.state?.setHits?.(false); | ||
|
|
||
| // Damage wakeups can run this state several times before a 500-unit route | ||
| // completes. Keep the existing escape movement instead of cancelling it | ||
| // and returning without a replacement route on every cooldown tick. | ||
| if (retreatInProgress) return true; | ||
|
|
||
| bot.automation?.abortAll?.(bot); | ||
| if (rooted) return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Abort an active retreat when root is applied.
retreatInProgress returns before the rooted check. If the bot is rooted during the 1.5-second retreat window, its existing route remains active and moved is reported as true. Check rooted first, clear partyRetreatUntil, and abort automation before preserving a route.
Proposed fix
function retreatFromThreat(session, bot, threat, player, rooted) {
const retreatInProgress = Date.now() < Number(session.partyRetreatUntil || 0) &&
(!!session.moveTimer || bot.state?.fetchTowards?.());
session.currentTargetId = undefined;
bot.unselect();
bot.attack?.abortCast?.(session, bot);
bot.attack?.clearTimers?.();
bot.state?.setHits?.(false);
+ if (rooted) {
+ session.partyRetreatUntil = 0;
+ bot.automation?.abortAll?.(bot);
+ return false;
+ }
+
if (retreatInProgress) return true;
bot.automation?.abortAll?.(bot);
- if (rooted) return false;📝 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
| function retreatFromThreat(session, bot, threat, player, rooted) { | |
| const retreatInProgress = Date.now() < Number(session.partyRetreatUntil || 0) && | |
| (!!session.moveTimer || bot.state?.fetchTowards?.()); | |
| session.currentTargetId = undefined; | |
| bot.unselect(); | |
| bot.attack?.abortCast?.(session, bot); | |
| bot.attack?.clearTimers?.(); | |
| bot.state?.setHits?.(false); | |
| // Damage wakeups can run this state several times before a 500-unit route | |
| // completes. Keep the existing escape movement instead of cancelling it | |
| // and returning without a replacement route on every cooldown tick. | |
| if (retreatInProgress) return true; | |
| bot.automation?.abortAll?.(bot); | |
| if (rooted) return false; | |
| function retreatFromThreat(session, bot, threat, player, rooted) { | |
| const retreatInProgress = Date.now() < Number(session.partyRetreatUntil || 0) && | |
| (!!session.moveTimer || bot.state?.fetchTowards?.()); | |
| session.currentTargetId = undefined; | |
| bot.unselect(); | |
| bot.attack?.abortCast?.(session, bot); | |
| bot.attack?.clearTimers?.(); | |
| bot.state?.setHits?.(false); | |
| if (rooted) { | |
| session.partyRetreatUntil = 0; | |
| bot.automation?.abortAll?.(bot); | |
| return false; | |
| } | |
| // Damage wakeups can run this state several times before a 500-unit route | |
| // completes. Keep the existing escape movement instead of cancelling it | |
| // and returning without a replacement route on every cooldown tick. | |
| if (retreatInProgress) return true; | |
| bot.automation?.abortAll?.(bot); |
🤖 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/States/FollowingState.js` around lines 444 - 459,
Update retreatFromThreat so the rooted case is handled before the
retreatInProgress early return: clear session.partyRetreatUntil, abort active
automation, and return false when rooted. Only preserve the existing retreat
route and return true for non-rooted retreatInProgress cases.
Comment on lines
+118
to
+136
| function releaseBackgroundParty(state, reason) { | ||
| const partyId = state?.party?.partyId; | ||
| if (!partyId) return Promise.resolve(state); | ||
|
|
||
| return BackgroundPartyState.setStatus(partyId, 'dissolved') | ||
| .then(() => LifeState.clearParty(partyId, `hot_activation_${reason}`)) | ||
| .then((cleared) => { | ||
| const refreshed = LifeState.cachedState(state.characterId); | ||
| if (refreshed && !refreshed.party?.partyId) return refreshed; | ||
| if (Number(cleared || 0) <= 0 || refreshed?.party?.partyId) { | ||
| throw new Error(`background_party_release_failed:${partyId}`); | ||
| } | ||
| return { | ||
| ...state, | ||
| activity: state.activity === 'grouped' ? 'hunting' : state.activity, | ||
| party: { ...(state.party || {}), partyId: null, leaderId: null } | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
No compensation when clearParty fails after the party is already marked dissolved.
setStatus(partyId, 'dissolved') is committed before clearParty. If the clear returns 0 or the cache still shows the link, the thrown error aborts activation but the background party stays dissolved while member rows still reference partyId. Those members are then orphaned at runtime — nothing reconciles them until recoverDissolvedPartyMembers() runs on the next restart. Consider restoring the party status (or enqueuing a retry) in a failure handler here.
🛠️ Sketch of a compensating path
return BackgroundPartyState.setStatus(partyId, 'dissolved')
.then(() => LifeState.clearParty(partyId, `hot_activation_${reason}`))
.then((cleared) => {
@@
return {
...state,
activity: state.activity === 'grouped' ? 'hunting' : state.activity,
party: { ...(state.party || {}), partyId: null, leaderId: null }
};
- });
+ })
+ .catch((error) => BackgroundPartyState.setStatus(partyId, 'active')
+ .catch(() => null)
+ .then(() => { throw error; }));
}📝 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
| function releaseBackgroundParty(state, reason) { | |
| const partyId = state?.party?.partyId; | |
| if (!partyId) return Promise.resolve(state); | |
| return BackgroundPartyState.setStatus(partyId, 'dissolved') | |
| .then(() => LifeState.clearParty(partyId, `hot_activation_${reason}`)) | |
| .then((cleared) => { | |
| const refreshed = LifeState.cachedState(state.characterId); | |
| if (refreshed && !refreshed.party?.partyId) return refreshed; | |
| if (Number(cleared || 0) <= 0 || refreshed?.party?.partyId) { | |
| throw new Error(`background_party_release_failed:${partyId}`); | |
| } | |
| return { | |
| ...state, | |
| activity: state.activity === 'grouped' ? 'hunting' : state.activity, | |
| party: { ...(state.party || {}), partyId: null, leaderId: null } | |
| }; | |
| }); | |
| } | |
| function releaseBackgroundParty(state, reason) { | |
| const partyId = state?.party?.partyId; | |
| if (!partyId) return Promise.resolve(state); | |
| return BackgroundPartyState.setStatus(partyId, 'dissolved') | |
| .then(() => LifeState.clearParty(partyId, `hot_activation_${reason}`)) | |
| .then((cleared) => { | |
| const refreshed = LifeState.cachedState(state.characterId); | |
| if (refreshed && !refreshed.party?.partyId) return refreshed; | |
| if (Number(cleared || 0) <= 0 || refreshed?.party?.partyId) { | |
| throw new Error(`background_party_release_failed:${partyId}`); | |
| } | |
| return { | |
| ...state, | |
| activity: state.activity === 'grouped' ? 'hunting' : state.activity, | |
| party: { ...(state.party || {}), partyId: null, leaderId: null } | |
| }; | |
| }) | |
| .catch((error) => BackgroundPartyState.setStatus(partyId, 'active') | |
| .catch(() => null) | |
| .then(() => { throw error; })); | |
| } |
🤖 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 118 - 136,
Update releaseBackgroundParty to compensate when the post-setStatus clearParty
validation fails: restore the background party’s prior active status or enqueue
the established retry/reconciliation path before propagating the release error.
Keep the existing successful refresh and state-clearing behavior unchanged, and
ensure compensation covers both a zero clear count and a remaining cached party
link.
Comment on lines
+124
to
+134
| .then((cleared) => { | ||
| const refreshed = LifeState.cachedState(state.characterId); | ||
| if (refreshed && !refreshed.party?.partyId) return refreshed; | ||
| if (Number(cleared || 0) <= 0 || refreshed?.party?.partyId) { | ||
| throw new Error(`background_party_release_failed:${partyId}`); | ||
| } | ||
| return { | ||
| ...state, | ||
| activity: state.activity === 'grouped' ? 'hunting' : state.activity, | ||
| party: { ...(state.party || {}), partyId: null, leaderId: null } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cached-state fast path skips the grouped → hunting normalization.
Line 126 returns refreshed as-is, so a cache entry with partyId cleared but activity === 'grouped' propagates into activationPlan and backgroundActivity, unlike the constructed state at Line 132. Normalize both paths.
♻️ Proposed fix
- const refreshed = LifeState.cachedState(state.characterId);
- if (refreshed && !refreshed.party?.partyId) return refreshed;
+ const refreshed = LifeState.cachedState(state.characterId);
+ if (refreshed && !refreshed.party?.partyId) {
+ return {
+ ...refreshed,
+ activity: refreshed.activity === 'grouped' ? 'hunting' : refreshed.activity
+ };
+ }📝 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
| .then((cleared) => { | |
| const refreshed = LifeState.cachedState(state.characterId); | |
| if (refreshed && !refreshed.party?.partyId) return refreshed; | |
| if (Number(cleared || 0) <= 0 || refreshed?.party?.partyId) { | |
| throw new Error(`background_party_release_failed:${partyId}`); | |
| } | |
| return { | |
| ...state, | |
| activity: state.activity === 'grouped' ? 'hunting' : state.activity, | |
| party: { ...(state.party || {}), partyId: null, leaderId: null } | |
| }; | |
| .then((cleared) => { | |
| const refreshed = LifeState.cachedState(state.characterId); | |
| if (refreshed && !refreshed.party?.partyId) { | |
| return { | |
| ...refreshed, | |
| activity: refreshed.activity === 'grouped' ? 'hunting' : refreshed.activity | |
| }; | |
| } | |
| if (Number(cleared || 0) <= 0 || refreshed?.party?.partyId) { | |
| throw new Error(`background_party_release_failed:${partyId}`); | |
| } | |
| return { | |
| ...state, | |
| activity: state.activity === 'grouped' ? 'hunting' : state.activity, | |
| party: { ...(state.party || {}), partyId: null, leaderId: 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/HotActivation.js` around lines 124 - 134,
Update the cached-state fast path in the promise callback to normalize
refreshed.activity from 'grouped' to 'hunting' before returning it, matching the
constructed state path while preserving all other refreshed state fields.
Comment on lines
+317
to
+336
| const pending = session.pendingPartyInvite; | ||
| session.pendingPartyInvite = null; | ||
|
|
||
| if (!pending?.requestorSession || !pending?.requestorActor) { | ||
| session.dataSendToMe(ServerResponse.actionFailed()); | ||
| return false; | ||
| } | ||
|
|
||
| if (Number(data?.id) !== 1) { | ||
| pending.requestorSession.dataSendToMe(ServerResponse.joinParty(0)); | ||
| return false; | ||
| } | ||
|
|
||
| return this.inviteBotCompanion( | ||
| pending.requestorSession, | ||
| pending.requestorActor, | ||
| session, | ||
| pending.distribution, | ||
| pending.source || 'invite' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Restore player-to-player invitation answers.
The non-bot path still sends askForTeamUp without a pending invite, so its native answer reaches Line 320 and is rejected. Even if pending state were added, this method now always calls inviteBotCompanion, so accepted player invitations cannot form a normal party. Preserve the prior human-answer path and branch to bot attachment only for bot invitations.
🤖 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/World/World.js` around lines 317 - 336, Update the
invitation-answer handling around pendingPartyInvite to preserve the existing
player-to-player response flow: validate and consume the pending invite, then
route human invitations through the normal party-formation path instead of
inviteBotCompanion. Call inviteBotCompanion only when the pending invitation
targets a bot, while retaining the existing rejection behavior for invalid or
declined answers.
This was referenced Jul 30, 2026
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
This change hardens hot-bot party lifecycle and companion behavior so persisted parties, native invitations, loot rules, and emergency combat movement remain consistent after activation and restarts.
What changed
huntcommand changes their autonomous behaviorWhy
Hot-bot activation previously had an asynchronous gap where the same character could be activated more than once, while stale persisted party state could survive long enough to produce invalid membership or recruitment decisions. Some companion actions also bypassed native party semantics, and emergency retreat logic restarted itself every AI tick instead of allowing movement to complete.
These fixes keep party state authoritative across cold/hot transitions and make companion behavior match the game-facing C4 flow more closely.
Player impact
Validation
npm testnpm run checkgit diff --check origin/main..HEADSummary by CodeRabbit