Fix bot party formation backlog - #80
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR updates gear acquisition with structured party requirements and safe fallbacks. It adds party-request cleanup, objective-based formation, session expiry, deadline-aware scheduling, database indexes, and expanded population telemetry. ChangesParty acquisition and population lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BotLifeState
participant PopulationService
participant GearAcquisitionPlanner
participant BackgroundPartyState
BotLifeState->>PopulationService: provide open party requests
PopulationService->>GearAcquisitionPlanner: evaluate party need and fallback
PopulationService->>BackgroundPartyState: group and persist objective party
BackgroundPartyState-->>BotLifeState: return normalized party state
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 1, 2026 21:01
pmbstyle
marked this pull request as draft
August 1, 2026 21:04
![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 (5)
src/GameServer/Bot/Population/PopulationService.js (1)
206-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
partyRequestActivehelper.ESLint reports
partyRequestActiveas defined but never used. No call site appears in the changed code, and it is not exported at lines 347-360.🤖 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 206 - 208, Remove the unused partyRequestActive function from PopulationService.js, leaving the surrounding party request logic and exports unchanged.Source: Linters/SAST tools
src/GameServer/Bot/AI/GearAcquisitionPlanner.js (2)
459-475: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
hasWeaponterm inunpreparedSupport.Line 470 returns
requiredfor every state without a weapon. The!readiness.hasWeaponterm insideunpreparedSupportis therefore never decisive. Keep the armor condition only, so the intent of the support check stays clear.♻️ Proposed simplification
- const unpreparedSupport = ['healer', 'buffer'].includes(readiness.role) - && (!readiness.hasWeapon || readiness.armorCount < 2); + const unpreparedSupport = ['healer', 'buffer'].includes(readiness.role) + && readiness.armorCount < 2; if (!readiness.hasWeapon) return { need: 'required', reason: 'missing_weapon' };🤖 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/GearAcquisitionPlanner.js` around lines 459 - 475, Simplify the unpreparedSupport condition in partyNeedAssessmentForSource by removing the redundant readiness.hasWeapon check and retaining only the support-role and armor-count conditions; leave the separate missing_weapon return unchanged.
627-630: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCompute the party-need assessment once per source.
Lines 627-630 evaluate the same source three times:
soloSafeForSource,partyNeedForSource, andpartyNeedReasonForSource. Each call re-runscombatReadiness, which scans and reduces the equipped inventory. Lines 679-684 repeat the same pattern fornext. Export the assessment helper, or compute it once and derive the three fields.♻️ Proposed refactor for lines 679-685
- const partyNeed = !readyToCraft && !componentReady && next - ? partyNeedForSource(state, next) - : 'solo_ok'; - const partyNeedReason = !readyToCraft && !componentReady && next - ? partyNeedReasonForSource(state, next) - : 'solo_ready'; - const requiresParty = partyNeed === 'required'; + const nextAssessment = !readyToCraft && !componentReady && next + ? partyNeedAssessmentForSource(state, next) + : { need: 'solo_ok', reason: 'solo_ready' }; + const partyNeed = nextAssessment.need; + const partyNeedReason = nextAssessment.reason; + const requiresParty = partyNeed === 'required';Also applies to: 679-685
🤖 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/GearAcquisitionPlanner.js` around lines 627 - 630, Update the source assessment logic in the planner block around the status object and its corresponding next-source block to compute the party-need assessment once per source, then derive soloSafe, partyNeed, partyNeedReason, and requiresParty from that shared result. Reuse the same assessment for both the current source and next source paths, or export and reuse the underlying assessment helper, avoiding repeated combatReadiness scans while preserving existing field values.tests/test_bot_background_party_recruitment.js (1)
86-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for candidate claiming across two recruiting parties.
The new assertions cover objective separation and craft NPC targeting. They do not cover the
claimedset inrecruitBackgroundMembers. Add a case with two active parties that both lackstats.objectiveand share a spot. Then assert that each candidate is assigned exactly once. That case fails today because of the precedence defect flagged insrc/GameServer/Bot/Population/PopulationService.jsat lines 1229-1237.🤖 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_bot_background_party_recruitment.js` around lines 86 - 98, Extend the recruitment tests around groupPartyCandidatesByObjective with two active parties sharing a spot and neither having stats.objective, then verify each candidate is assigned exactly once across both parties. Exercise recruitBackgroundMembers and its claimed-set handling so the regression exposes the precedence defect without changing the existing objective-separation or craft NPC assertions.src/GameServer/Bot/Population/BotLifeState.js (1)
1253-1279: 🚀 Performance & Scalability | 🔵 TrivialVerify index coverage for the widened
json_extract-based candidate queries.These queries now filter and group on
json_extract(statsJson, '$.partyRequest.status'),'$.partyRequest.priority'), and a COALESCE of three JSON/column expressions (objectiveSpot), across three activities (hunting,resting,party_wait) instead of one. SQLite cannot use a normal index onjson_extract()expressions unless a generated column with an index exists for them.Given the PR's stated goal of bounding formation cost at scale (x50 population), confirm whether
bot_life_statehas generated-column indexes coveringpartyRequest.status/priorityand the objective-spot expression, or whether this query now performs a wider full-table scan on every formation tick than before.Also applies to: 1341-1364
🤖 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/BotLifeState.js` around lines 1253 - 1279, Verify the indexes used by the candidate query in the bot_life_state schema/migrations, covering the partyRequest status, partyRequest priority, and objective-spot expression referenced by objectiveSpot and stateObjectiveSpot. If coverage is missing, add appropriate generated columns and indexes, including the states-table aliases where required, so the widened filters and grouping avoid full-table scans. Confirm the same optimization applies to the corresponding query block around the second formation path.
🤖 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 719-761: Update updateCache in expireStalePartyRequests to inspect
the live cached.stats.partyRequest.status before applying the deferred patch,
and skip the row unless that current status is still eligible for expiration
(open). Keep the existing snapshot validation and SQL update behavior unchanged,
but prevent stale rows excluded by the UPDATE WHERE clause from overwriting
newer cache state.
In `@src/GameServer/Bot/Population/PopulationService.js`:
- Around line 947-951: Update the objective-member selection in the population
service to require an active/open request status alongside priority ===
'required', excluding deferred requests. Preserve the fallback through
partyObjectiveForState(member) and leader so an available member with an open
objective is selected before creating the party objective and spot.
- Around line 1229-1237: Update the nearby candidate filter so the
!claimed.has(Number(state.characterId)) check wraps the entire
objective-versus-spot ternary, rather than being combined with its condition.
Preserve the existing objective matching and spot matching branches while
ensuring claimed candidates are always excluded, regardless of whether
partyObjective exists.
- Around line 876-887: Update formationWork so expireStalePartyRequests failures
are caught before finally, allowing the candidate-count and candidate-query
stages to continue; preserve scheduling nextPartyRequestCleanupAt in finally.
Change coldPartyCandidateCount in formationWork to omit the argument or pass
false so partyWaitBacklog reflects all candidates.
---
Nitpick comments:
In `@src/GameServer/Bot/AI/GearAcquisitionPlanner.js`:
- Around line 459-475: Simplify the unpreparedSupport condition in
partyNeedAssessmentForSource by removing the redundant readiness.hasWeapon check
and retaining only the support-role and armor-count conditions; leave the
separate missing_weapon return unchanged.
- Around line 627-630: Update the source assessment logic in the planner block
around the status object and its corresponding next-source block to compute the
party-need assessment once per source, then derive soloSafe, partyNeed,
partyNeedReason, and requiresParty from that shared result. Reuse the same
assessment for both the current source and next source paths, or export and
reuse the underlying assessment helper, avoiding repeated combatReadiness scans
while preserving existing field values.
In `@src/GameServer/Bot/Population/BotLifeState.js`:
- Around line 1253-1279: Verify the indexes used by the candidate query in the
bot_life_state schema/migrations, covering the partyRequest status, partyRequest
priority, and objective-spot expression referenced by objectiveSpot and
stateObjectiveSpot. If coverage is missing, add appropriate generated columns
and indexes, including the states-table aliases where required, so the widened
filters and grouping avoid full-table scans. Confirm the same optimization
applies to the corresponding query block around the second formation path.
In `@src/GameServer/Bot/Population/PopulationService.js`:
- Around line 206-208: Remove the unused partyRequestActive function from
PopulationService.js, leaving the surrounding party request logic and exports
unchanged.
In `@tests/test_bot_background_party_recruitment.js`:
- Around line 86-98: Extend the recruitment tests around
groupPartyCandidatesByObjective with two active parties sharing a spot and
neither having stats.objective, then verify each candidate is assigned exactly
once across both parties. Exercise recruitBackgroundMembers and its claimed-set
handling so the regression exposes the precedence defect without changing the
existing objective-separation or craft NPC assertions.
🪄 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: b9bab6f3-d35d-4df7-a56f-135aae8ae684
📒 Files selected for processing (14)
src/GameServer/Bot/AI/GearAcquisitionPlanner.jssrc/GameServer/Bot/Economy/CraftTelemetry.jssrc/GameServer/Bot/Population/BackgroundPartyState.jssrc/GameServer/Bot/Population/BotLifeState.jssrc/GameServer/Bot/Population/PersonaPartyPolicy.jssrc/GameServer/Bot/Population/PopulationConfig.jssrc/GameServer/Bot/Population/PopulationMetrics.jssrc/GameServer/Bot/Population/PopulationService.jssrc/GameServer/Bot/Population/PopulationStatus.jstests/test_bot_background_party_recruitment.jstests/test_bot_craft_telemetry.jstests/test_bot_gear_acquisition.jstests/test_bot_party_wait.jstests/test_bot_population_state.js
pmbstyle
marked this pull request as ready for review
August 1, 2026 21:12
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/GameServer/Bot/AI/GearAcquisitionPlanner.js (1)
459-475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for equipment-based party reasons.
Add cases for a weaponless bot and a
healerorbufferwith fewer than two equipped armor pieces. Assert both theneedvalue and the exact reason.🤖 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/GearAcquisitionPlanner.js` around lines 459 - 475, Add regression tests covering partyNeedAssessmentForSource for a weaponless bot and for an unprepared healer or buffer with fewer than two armor pieces. Assert the exact required need value and corresponding reasons missing_weapon and unprepared_support, while preserving existing readiness and level-margin test coverage.tests/test_bot_background_party_recruitment.js (1)
95-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that recruitment uses both active parties.
The current assertions prove that each candidate is assigned once. They do not prove that
bgp_shared_aandbgp_shared_bboth receive a candidate. Assert the assigned party IDs as well.Proposed assertion
assert.deepStrictEqual(sharedAssignments.map((entry) => entry.characterId).sort((a, b) => a - b), [60, 61]); + assert.deepStrictEqual( + sharedAssignments.map((entry) => entry.partyId).sort(), + ['bgp_shared_a', 'bgp_shared_b'], + 'each active party must receive one candidate' + );🤖 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_bot_background_party_recruitment.js` around lines 95 - 100, Extend the assertions in the shared-candidate recruitment test to verify that both active parties, bgp_shared_a and bgp_shared_b, appear in the resulting sharedAssignments party IDs. Preserve the existing candidate uniqueness and character ID assertions.
🤖 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 `@database/sql/sqlite.sql`:
- Around line 181-199: The JSON-expression indexes
bot_life_state_party_request_filter and bot_life_state_party_objective_spot are
created before legacy statsJson validation, causing initialization to fail on
malformed data. Remove these index definitions from the initial schema load and
create them in an appropriate schema migration after repairing or rejecting
invalid non-null statsJson rows; preserve both index expressions for validated
data.
---
Nitpick comments:
In `@src/GameServer/Bot/AI/GearAcquisitionPlanner.js`:
- Around line 459-475: Add regression tests covering
partyNeedAssessmentForSource for a weaponless bot and for an unprepared healer
or buffer with fewer than two armor pieces. Assert the exact required need value
and corresponding reasons missing_weapon and unprepared_support, while
preserving existing readiness and level-margin test coverage.
In `@tests/test_bot_background_party_recruitment.js`:
- Around line 95-100: Extend the assertions in the shared-candidate recruitment
test to verify that both active parties, bgp_shared_a and bgp_shared_b, appear
in the resulting sharedAssignments party IDs. Preserve the existing candidate
uniqueness and character ID assertions.
🪄 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: 29fc52e8-901e-4736-b6ba-837b37654329
📒 Files selected for processing (5)
database/sql/sqlite.sqlsrc/GameServer/Bot/AI/GearAcquisitionPlanner.jssrc/GameServer/Bot/Population/BotLifeState.jssrc/GameServer/Bot/Population/PopulationService.jstests/test_bot_background_party_recruitment.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/GameServer/Bot/Population/BotLifeState.js
- src/GameServer/Bot/Population/PopulationService.js
Comment on lines
+181
to
+199
| CREATE INDEX IF NOT EXISTS bot_life_state_party_request_filter | ||
| ON bot_life_state( | ||
| phase, | ||
| partyId, | ||
| activity, | ||
| json_extract(statsJson, '$.partyRequest.status'), | ||
| json_extract(statsJson, '$.partyRequest.priority') | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS bot_life_state_party_objective_spot | ||
| ON bot_life_state( | ||
| phase, | ||
| partyId, | ||
| activity, | ||
| COALESCE( | ||
| json_extract(statsJson, '$.partyRequest.spotId'), | ||
| json_extract(statsJson, '$.equipmentPlan.next.spotId'), | ||
| spotId | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
db=${1:?Usage: $0 /path/to/bot.sqlite}
sqlite3 "$db" <<'SQL'
SELECT sqlite_version() AS sqlite_version;
SELECT json_extract('{"probe":1}', '$.probe') AS json1_probe;
SELECT COUNT(*) AS invalid_stats_json
FROM bot_life_state
WHERE statsJson IS NOT NULL
AND json_valid(statsJson) = 0;
PRAGMA index_list('bot_life_state');
SQL
rg -n -C 6 \
'sqlite\.sql|CREATE INDEX IF NOT EXISTS|migration' \
--glob '*.js' \
--glob '*.sql' \
.Repository: pmbstyle/L2Solo
Length of output: 202
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(database/sql/sqlite\.sql|.*(migration|schema|sqlite).*\.(js|ts|sql))$' | head -200
printf '%s\n' '--- schema context ---'
sed -n '150,215p' database/sql/sqlite.sql
printf '%s\n' '--- schema runner references ---'
rg -n -C 5 'database/sql/sqlite\.sql|sqlite\.sql|CREATE INDEX IF NOT EXISTS|executescript|migration' . \
--glob '*.js' --glob '*.ts' --glob '*.mjs' --glob '*.cjs' --glob '*.sql' --glob '*.json' \
| head -300
printf '%s\n' '--- SQLite tooling ---'
command -v sqlite3 || true
sqlite3 --version 2>/dev/null || trueRepository: pmbstyle/L2Solo
Length of output: 27493
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- migration implementation ---'
sed -n '175,215p' src/Database.js
printf '%s\n' '--- runtime declarations ---'
rg -n -C 4 '"engines"|node:sqlite|better-sqlite|sqlite' package.json package-lock.json npm-shrinkwrap.json 2>/dev/null | head -200 || true
printf '%s\n' '--- behavioral SQLite probe ---'
python3 - <<'PY'
import sqlite3
print("sqlite_version", sqlite3.sqlite_version)
con = sqlite3.connect(":memory:")
for sql in [
"SELECT json_extract('{\"probe\":1}', '$.probe')",
"SELECT json_valid('{\"probe\":1}')",
"SELECT json_valid('{bad}')",
]:
try:
print(sql, "=>", con.execute(sql).fetchone()[0])
except Exception as e:
print(sql, "=> ERROR:", type(e).__name__, str(e))
con.executescript("""
CREATE TABLE bot_life_state (
phase TEXT NOT NULL DEFAULT 'cold',
partyId TEXT,
activity TEXT NOT NULL DEFAULT 'hunting',
spotId TEXT,
statsJson TEXT
);
INSERT INTO bot_life_state(statsJson) VALUES
('{\"partyRequest\":{\"status\":\"open\",\"priority\":3}}'),
(NULL),
('{bad}');
""")
for name, expression in [
("party_request_filter", """phase, partyId, activity,
json_extract(statsJson, '$.partyRequest.status'),
json_extract(statsJson, '$.partyRequest.priority')"""),
("party_objective_spot", """phase, partyId, activity,
COALESCE(
json_extract(statsJson, '$.partyRequest.spotId'),
json_extract(statsJson, '$.equipmentPlan.next.spotId'),
spotId
)"""),
]:
try:
con.execute(f"CREATE INDEX {name} ON bot_life_state({expression})")
print("CREATE INDEX", name, "=> success")
except Exception as e:
print("CREATE INDEX", name, "=> ERROR:", type(e).__name__, str(e))
PYRepository: pmbstyle/L2Solo
Length of output: 12506
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- statsJson writes and JSON guards ---'
rg -n -C 4 'statsJson|json_valid|json_extract|JSON\.parse' src tests database --glob '*.js' --glob '*.sql' | head -400
printf '%s\n' '--- startup order ---'
sed -n '235,270p' src/Database.js
printf '%s\n' '--- schema migration metadata references ---'
rg -n -C 5 'schema_migrations|applySchemaMigrations|connection\.exec\(fs\.readFileSync' src scripts tests --glob '*.js'Repository: pmbstyle/L2Solo
Length of output: 40524
Validate legacy statsJson before creating these indexes.
Database.init() creates these indexes before applySchemaMigrations(). If any existing non-null statsJson value is malformed, SQLite raises malformed JSON and initialization fails. Move index creation into a migration that repairs or rejects invalid rows first, or clean the data before loading database/sql/sqlite.sql.
🤖 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 `@database/sql/sqlite.sql` around lines 181 - 199, The JSON-expression indexes
bot_life_state_party_request_filter and bot_life_state_party_objective_spot are
created before legacy statsJson validation, causing initialization to fail on
malformed data. Remove these index definitions from the initial schema load and
create them in an appropriate schema migration after repairing or rejecting
invalid non-null statsJson rows; preserve both index expressions for validated
data.
This was referenced Aug 6, 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.
This PR fixes the bot party backlog and the progression flow around it.
Bots now classify party needs from their actual gear and target level, with explicit required/preferred reasons. When a compatible group is not available, a bot can continue from a safe solo fallback instead of remaining idle in the party queue. Party requests now carry a concrete route/NPC objective, expire and cool down when stale, are cleaned up for passive or dead bots, and are cleared correctly when a bot joins or leaves a party.
Formation now groups compatible objectives, gives required requests priority without letting an incompatible request block other groups, reclaims unused capacity when needed, and rotates party sessions with jitter so groups do not dissolve in one synchronized wave.
The scheduler and formation work is bounded and cooperative to reduce event-loop spikes. Population telemetry now reports request reasons, formation stage timings, p95 duration, budget stops, queue age, scheduler lag, and overruns. Also added a small null-safety fix for craft travel telemetry.
Validation:
Summary by CodeRabbit
New Features
Bug Fixes
Monitoring