Add LLM conversations and actions for player bots - #81
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds structured bot inference, Langfuse tracing, persistent conversations and journals, party routing, ambient scenes, policy-controlled tools, supply errands, negotiations, atomic trades, lifecycle integration, and expanded automated tests. ChangesBot AI platform
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
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/BotManager.js (1)
1396-1408: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not report a cancelled ambient scene as completed.
deliverskips output whenambientScene.cancelledis true, but the timeout at line 1406 still callsBotAmbientDirector.finish(ambientScene, 'completed'). A scene that was cancelled mid-flight is then recorded as completed. Apply the same check before you finish the scene.🐛 Proposed fix
- setTimeout(() => ambientScene - ? BotAmbientDirector.finish(ambientScene, 'completed') - : BotConversation.finish(conversation), 6500); + setTimeout(() => { + if (!ambientScene) return BotConversation.finish(conversation); + if (ambientScene.cancelled || ambientScene.finished) return undefined; + return BotAmbientDirector.finish(ambientScene, 'completed'); + }, 6500);🤖 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/BotManager.js` around lines 1396 - 1408, Update the final timeout in the conversation delivery flow around deliver so it does not call BotAmbientDirector.finish with 'completed' when ambientScene.cancelled is true. Preserve completion for active scenes and the existing BotConversation.finish behavior when no ambient scene is present.
🟠 Major comments (24)
src/GameServer/Session.js-285-285 (1)
285-285: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the new cleanup call so a throw cannot abort session teardown.
The placement is correct.
cleanupruns beforethis.actor.destructor()and beforethis.actor = null, so it can still read the actor.The call is not guarded.
error()is the connection-close and error handler. Ifcleanupthrows, the remaining teardown on Lines 289-306 does not run: the character status is not persisted, effects are not cleared, companions are not detached, the actor is not destroyed, andWorld.removeUser(this)never executes. The session then stays inWorld.user.sessionswith a dead socket, anddataSendToOtherswrites to it for every other visible player.A throw is plausible.
BotNegotiationService.cleanupreachesactiveFor, which callsstockValid, which accessessession.actor.backpackand callsitem.fetchSelfId()without optional chaining. A partially initialized or partially torn-down actor raises aTypeErrorthere.Wrap the call in a try/catch and log the failure.
🛡️ Proposed fix
if (this.actor) { - invoke('GameServer/Bot/BotTradeService').cleanup(this, 'disconnect'); + try { + invoke('GameServer/Bot/BotTradeService').cleanup(this, 'disconnect'); + } catch (cleanupError) { + utils.infoWarn('GameServer', 'bot trade cleanup failed on disconnect: %s', cleanupError.message || cleanupError); + }🤖 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/Session.js` at line 285, Guard the invoke('GameServer/Bot/BotTradeService').cleanup call in the session teardown path with try/catch so any cleanup exception is logged and does not prevent the remaining error() teardown steps from running. Keep the call before this.actor.destructor() and this.actor = null, and use the existing error logging mechanism to record the failure.src/GameServer/Bot/TradeService.js-251-259 (1)
251-259: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLog a failed Adena refund. The current code can drop the evidence.
If
giveAdenafails, the actor has paid and received nothing. Line 255 preserves that fact only whenerroris an object.deductAdenarejects with the string"Not enough Adena.", andgiveItemrejects with the string`Unknown item ${selfId}.`. For a string rejection, Line 255 isfalseandrollbackErroris discarded, so the currency loss leaves no trace.Add a warning log inside the
catchblock so every refund failure is recorded with the actor id and the amount, independent of the rejection type.🛠️ Proposed fix
if (adenaDeducted) { try { await giveAdena(actor, totalCost); } catch (rollbackError) { + utils.infoWarn( + 'TradeService', + 'adena refund failed for actor %s: amount=%d item=%s error=%s', + actor.fetchId(), + totalCost, + selfId, + rollbackError?.message || String(rollbackError) + ); if (error && typeof error === 'object') { error.rollbackError = rollbackError; } } }🤖 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/TradeService.js` around lines 251 - 259, Update the giveAdena rollback catch block in the trade flow to always emit a warning containing the actor id and totalCost when the refund fails, regardless of the original rejection type; preserve the existing rollbackError attachment behavior for object errors.src/GameServer/Bot/Economy/BotNegotiationService.js-451-481 (1)
451-481: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
acceptPricereturns a Promise on the merchant-store path but callers do not await it.
acceptPriceis not markedasync, so its return type depends onnegotiation.storeRevision. WhenstoreRevisionis set, line 478 returnsrepublishAcceptedStore(bot, negotiation), which isasync. Otherwise, line 480 returns a plain object.Callers do not await. In
BotAgentTools.js(line 767–773), the function callsBotNegotiationService.acceptPrice()and immediately readsresult.okwithoutawait. The same occurs intest_bot_negotiation_flow.js(line 61) andtest_bot_negotiation_database.js(line 41).When
storeRevisionis set andacceptPricereturns a Promise, the caller readsresult.okon the Promise object, which isundefined. The caller then misinterprets a pending or completed store republish as a failure.Make
acceptPriceasyncand update all callers toawaitthe result.🤖 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/Economy/BotNegotiationService.js` around lines 451 - 481, Make acceptPrice asynchronous so it consistently returns a Promise, including the non-store path, and update every caller—including BotAgentTools and the negotiation flow/database tests—to await its result before reading result.ok or other fields.Source: Linters/SAST tools
src/GameServer/Network/Request/Purchase.js-59-68 (1)
59-68: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake multi-entry merchant purchases atomic. Each
buyFromStorecall commits independently, so a later revision or price failure leaves earlier entries purchased while the handler sendsactionFailed(). Process the full list through one transaction or roll back all committed entries, including inventory, Adena, store stock, and seller 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/Network/Request/Purchase.js` around lines 59 - 68, Update the purchase flow surrounding the loop in the request handler so all data.list entries are processed atomically through a single transaction, or ensure complete rollback when any buyFromStore call fails. Preserve all-or-nothing state across inventory, Adena, store stock, and seller state, and only send actionFailed() after the entire operation has been reverted.src/GameServer/Bot/Population/Cooldown.js-36-37 (1)
36-37: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove cleanup after a successful state save.
Lines 36–37 call
BotTradeService.cleanupandBotAmbientDirector.cleanupbeforeLifeState.upsertStateon line 38. Both cleanup methods mutate the session's active trade and ambient scene state. If the state save fails on line 38 (returningnull), the method returnsstate_save_failedon line 39. The session remains in memory with its trade and ambient state already cleared, but the cold state was not persisted to the database. This leaves the session in an inconsistent state.Move the cleanup calls into the
.then()chain after the!savedcheck succeeds, so cleanup only occurs after a successful save.🤖 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/Cooldown.js` around lines 36 - 37, Move the BotTradeService.cleanup and BotAmbientDirector.cleanup calls from before LifeState.upsertState into the successful-save branch of its .then() chain, after the !saved failure check. Preserve the state_save_failed return path so both cleanups run only when the cold state has been persisted successfully.tests/test_llm_equipment_tools.js-50-50 (1)
50-50: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRestore
BotManager.sessionsand correct thefinallycomment.Line 50 overwrites
BotManager.sessions, which is shared module state, and never restores it. Thefinallyblock at lines 76-78 is empty and its comment states that the fixture changes no persistent state. That statement is wrong.Any later test in the same process that reads
BotManager.sessionsobserves this fakebotfixture. The leak is silent and can produce a false pass or a false failure elsewhere.tests/test_cold_bot_chat.jslines 106-129 already saves and restoresBotManager.findSessionByNamefor the same reason.🐛 Proposed fix
+const originalSessions = BotManager.sessions; BotManager.sessions = [bot];console.log('LLM equipment tool checks passed'); } finally { - // no persistent world state is changed by this fixture + BotManager.sessions = originalSessions; }Also applies to: 76-78
🤖 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_llm_equipment_tools.js` at line 50, Update the test fixture around BotManager.sessions so it saves the original shared sessions value before assigning [bot], then restores that value in the finally block. Replace the incorrect finally comment with one describing the restoration, ensuring later tests see the original BotManager.sessions state.src/GameServer/Bot/AI/PartyLLMRouter.js-149-157 (1)
149-157: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not set
timeoutMs: 0for this request.OpenRouterGatewaydisables the abort timer whentimeoutMsis0. A stalled provider request can keep the router lock active until the session ends. Preserve the configured positive timeout instead.🤖 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/PartyLLMRouter.js` around lines 149 - 157, Update the OpenRouterGateway.request call in PartyLLMRouter so it does not override the request timeout with timeoutMs: 0; instead, preserve the existing positive timeout from the cfg/config path when building the config object. Keep the rest of the routing options in the same request flow unchanged, and adjust only the timeout handling on the request sent from the PartyLLMRouter logic.src/GameServer/Bot/AI/PartyDialogueRouter.js-134-162 (1)
134-162: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict candidates to bot sessions.
buildCandidatesappliesisOnlineand theneligible. For the party channel,eligiblerequiresisCompanion, so only companion bots pass. For local chat,eligibleis onlydistance <= hearingRadius. No predicate requires that the session belongs to a bot.
BotManagerpassessessions: this.sessionsand then starts an LLM reply fordeterministicRoute.candidate(seesrc/GameServer/Bot/BotManager.jslines 774-804). Ifthis.sessionscontains human player sessions, a nearby player name, or the speaker's own name, resolves to a non-bot candidate and the reply pipeline runs for that session. Add an explicit bot check tobuildCandidates.#!/bin/bash # Description: Determine whether the sessions passed to PartyDialogueRouter.select include human players. set -euo pipefail rg -n -C10 'PartyDialogueRouter.select' --type=js src rg -n -C4 'this\.sessions\s*=' --type=js src/GameServer/Bot/BotManager.js rg -n -C3 '\bisBot\b|botSession|isBotSession' --type=js src/GameServer/Bot | head -60🐛 Proposed fix
function buildCandidates({ sessions = [], playerSession, partyChannel = false, hearingRadius = HEARING_RADIUS } = {}) { const player = playerSession?.actor; const configuredPullerId = Number(playerSession?.partyCompanionSettings?.pullerId || 0); return sessions + .filter((session) => session !== playerSession && isBotSession(session)) .filter((session) => isOnline(session))🤖 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/PartyDialogueRouter.js` around lines 134 - 162, Update buildCandidates to explicitly retain only bot sessions before applying the existing isOnline and eligible filters. Reuse the repository’s established bot-session predicate or bot marker identified in the surrounding BotManager code, ensuring human sessions—including the speaker’s own session—cannot become candidates while preserving the current party-channel and hearing-radius eligibility behavior.src/GameServer/Bot/AI/PartyDialogueState.js-74-91 (1)
74-91: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd expiry to the in-flight and router locks.
beginRouterrefuses a new router call whilerouterInFlightAtis set, and onlyclearRouterresets it.inFlightBotIdis only cleared byclearInFlightorrecordDeliveredReply. Both values persist on the player session for its whole lifetime.If a reply path ends without reaching its clear call, two effects follow: the cheap router stays disabled for that player, and
PartyDialogueRouter.selectkeeps returning the stalein_flightcandidate for every later message. Compare the stored timestamp against a deadline so a stuck lock self-recovers.#!/bin/bash # Description: Check that every beginRouter/beginRequest path has a guaranteed clear. set -euo pipefail rg -n -C12 'beginRouter\(|clearRouter\(' --type=js src rg -n -C8 'clearInFlight\(|beginRequest\(' --type=js src🛡️ Proposed fix
+const ROUTER_LOCK_TTL_MS = 30 * 1000; +const IN_FLIGHT_TTL_MS = 60 * 1000; + function beginRouter(playerSession, at = Date.now()) { const state = ensure(playerSession); if (!state) return false; - if (state.routerInFlightAt) return false; + const startedAt = Number(state.routerInFlightAt || 0); + if (startedAt && (Number(at || Date.now()) - startedAt) < ROUTER_LOCK_TTL_MS) return false; state.routerInFlightAt = Number(at || Date.now()); return true; }Apply the same deadline check where
inFlightBotIdis read, sosnapshotnever reports a stale in-flight 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/PartyDialogueState.js` around lines 74 - 91, Add expiry handling to the router and bot in-flight locks: update beginRouter to clear routerInFlightAt and allow a new request when the stored timestamp exceeds the configured deadline, and update the snapshot/read path used by PartyDialogueRouter.select to ignore and clear stale inFlightBotId values. Preserve current lock behavior while timestamps remain within the deadline, using the existing time/deadline configuration symbols where available.src/GameServer/Bot/BotTradeService.js-340-345 (1)
340-345: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
incomingSlotsnever counts a new stackable stack, so the capacity check can pass on a full inventory.Line 343 requires
!line.stackable. A stackable line that the recipient does not yet hold is therefore excluded fromincomingNew, although it needs a free inventory slot. A non-stackable line is counted even when a same-selfIditem is already present, which is also wrong because non-stackable items never merge.The intended rule is: a line needs a new slot when it is not stackable, or when it is stackable and no matching stack exists.
🐛 Proposed fix
function incomingSlots(session, outgoingLines) { const inventory = session.actor.backpack.fetchItems(); const existingSelfIds = new Set(inventory.filter((item) => item.fetchStackable?.()).map((item) => Number(item.fetchSelfId()))); - const incomingNew = outgoingLines.filter((line) => !line.stackable && !existingSelfIds.has(Number(line.selfId))).length; + const incomingNew = outgoingLines.filter((line) => !line.stackable || !existingSelfIds.has(Number(line.selfId))).length; return inventory.length + incomingNew <= MAX_INVENTORY_ITEMS; }🤖 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/BotTradeService.js` around lines 340 - 345, Update incomingSlots so incomingNew counts each outgoing line that requires a new inventory slot: every non-stackable line, plus stackable lines whose selfId is absent from existingSelfIds. Preserve the existing inventory-length capacity calculation and avoid counting stackable lines that can merge with an existing matching stack.src/GameServer/Bot/BotManager.js-985-990 (1)
985-990: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
deterministicRoute.candidatesbefore you pass it to the router.Line 822 reads
deterministicRoute.candidates?.length, so the field is treated as optional there. Lines 988 and 989 use it directly, and line 989 calls.find(...)on it. IfPartyDialogueRouter.selectreturnsneeds_routerorambiguouswithout acandidatesarray, this throws aTypeErrorinsidehandlePlayerSpeakNow, the router flag stays set throughbeginRouter, and the player receives no reply.Normalize the list once.
🐛 Proposed fix
if (PartyDialogueState.beginRouter(playerSession)) { + const candidates = deterministicRoute.candidates || []; return PartyLLMRouter.route({ text: rawText, playerSession, - candidates: deterministicRoute.candidates, - selectedBotId: deterministicRoute.candidates.find((candidate) => candidate.selected)?.id || null, + candidates, + selectedBotId: candidates.find((candidate) => candidate.selected)?.id || null, dialogueState: PartyDialogueState.snapshot(playerSession) }).then((routerResult) => {🤖 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/BotManager.js` around lines 985 - 990, Normalize deterministicRoute.candidates once before the PartyLLMRouter.route call, defaulting a missing value to an empty array. Use that normalized list for both the candidates property and selectedBotId lookup, preserving handlePlayerSpeakNow behavior when the route has no candidates.src/GameServer/Bot/AI/States/ShoppingState.js-130-143 (1)
130-143: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard the purchased item before you read
fetchId().
purchased.itemcan be undefined, andbot.backpack.fetchItemFromSelfId(companionErrand.itemId)can return null when the stack was merged under a different lookup key. Line 138 then throws aTypeError. Thecatchblock at line 168 treats that throw as a purchase failure, so the bot reports "I could not complete that supply purchase", clearssession.pendingResourceDelivery, and recordsfailedtelemetry — after the Adena was already spent and the item was added to the bot inventory. The item is then never delivered.Fail with an explicit reason only when the item is really missing, and keep the delivery record when the purchase succeeded.
🐛 Proposed fix to validate the purchased item
const purchasedItem = purchased.item || bot.backpack.fetchItemFromSelfId(companionErrand.itemId); + if (!purchasedItem?.fetchId) { + throw new Error('purchased_item_missing'); + } session.pendingResourceDelivery = {🤖 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/ShoppingState.js` around lines 130 - 143, Validate the resolved purchasedItem in the purchase flow before calling fetchId(), and throw an explicit missing-item reason only when both purchased.item and fetchItemFromSelfId return no item. Preserve the successful purchase state by retaining session.pendingResourceDelivery and allowing delivery to proceed whenever an item was resolved; only treat a genuinely missing item as a failure.src/GameServer/Bot/BotManager.js-741-762 (1)
741-762: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the ingress chain and stop the rejection from escaping.
Two concerns in this serialization:
trackedrethrows the failure at line 758. Callers ofhandlePlayerSpeakin the packet path normally ignore the return value, so a throw insidehandlePlayerSpeakNowbecomes an unhandled promise rejection.- The chain has no depth limit. Each party-chat line appends another link that waits for the previous one. A player who sends lines faster than the router resolves grows the chain without bound, and the last queued line answers minutes later.
Log and swallow the failure at the tail, and drop the turn when a chain is already pending beyond a small limit.
🛡️ Proposed fix sketch
const previous = playerSession.partyDialogueIngressPromise || Promise.resolve(); + playerSession.partyDialogueIngressDepth = Number(playerSession.partyDialogueIngressDepth || 0); + if (playerSession.partyDialogueIngressDepth >= 3) { + return Promise.resolve({ ok: false, reason: 'ingress_busy' }); + } + playerSession.partyDialogueIngressDepth += 1; const run = Promise.resolve(previous) .catch(() => {}) .then(() => this.handlePlayerSpeakNow(playerSession, data)); let tracked; const clear = () => { + playerSession.partyDialogueIngressDepth = Math.max(0, Number(playerSession.partyDialogueIngressDepth || 1) - 1); if (playerSession.partyDialogueIngressPromise === tracked) { delete playerSession.partyDialogueIngressPromise; } }; tracked = run.then( (result) => { clear(); return result; }, (error) => { clear(); - throw error; + utils.infoWarn('BotDialogue', 'party ingress turn failed: %s', error?.message || error); + return { ok: false, reason: 'ingress_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/BotManager.js` around lines 741 - 762, Update the party dialogue serialization around handlePlayerSpeak to swallow failures at the tracked promise tail after logging them, rather than rethrowing and creating unhandled rejections. Before appending a new turn, detect when playerSession.partyDialogueIngressPromise already represents a queue beyond a small bounded limit and drop the new turn; preserve cleanup of the tracked promise when it settles.src/GameServer/Bot/BotTradeService.js-308-313 (1)
308-313: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe gift ledger is charged at offer time and never refunded on cancel.
offerBotItemaddsdeltatoledger.unitswhen the line is created.cancelTradecallsreleaseReservations, which removes the reservation entries but leavesledger.unitsunchanged. A repeated open-and-cancel sequence therefore consumes the wholeMAX_BOT_GIFT_UNITSbudget for the hour although no item ever changed hands, and the bot then refuses legitimate gifts withgift_budget_exceeded.Charge the ledger on successful commit, or record the charged amount per trade and refund it in
releaseReservations.🐛 Proposed direction: refund on release
const delta = Math.max(0, nextCount - (current?.count || 0)); if (!trade.supplyDelivery) { const ledger = botGiftLedger(botSession); if (ledger.units + delta > MAX_BOT_GIFT_UNITS) return { ok: false, reason: 'gift_budget_exceeded' }; ledger.units += delta; + trade.giftUnitsCharged = Number(trade.giftUnitsCharged || 0) + delta; }function releaseReservations(trade) { const bot = trade?.botSession; if (!bot) return; + if (trade.state !== 'committed' && Number(trade.giftUnitsCharged || 0) > 0) { + const ledger = botGiftLedger(bot); + ledger.units = Math.max(0, ledger.units - Number(trade.giftUnitsCharged)); + trade.giftUnitsCharged = 0; + } const reservations = botReservations(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/BotTradeService.js` around lines 308 - 313, Update the gift-budget accounting across offerBotItem and releaseReservations so ledger.units is not permanently consumed when a trade is cancelled. Track each offer’s charged delta by trade, then refund that amount when releaseReservations removes the reservation, while preserving the MAX_BOT_GIFT_UNITS check and preventing double refunds.src/GameServer/Bot/AI/BotToolRegistry.js-182-183 (1)
182-183: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
session.botToolExecutionsgrows without bound.Line 183 creates the map once per session and nothing ever removes entries from it. Every tool execution adds one entry keyed by turn, player, and action. A long-lived bot session accumulates entries for the whole session lifetime.
Two consequences:
- The map retains a normalized result object per executed tool for as long as the session lives. This is a memory leak on the hot-bot path.
- Line 230 materializes the full entry list with
[...mutationStore.entries()]on every mutating call. Cost grows linearly with the number of past executions, so the total work over a session is quadratic.Store the executions per turn and drop the previous turn when a new turn starts.
🛡️ Proposed fix sketch
- const mutationStore = session && (session.botToolExecutions ||= new Map()); + let mutationStore = null; + if (session) { + if (!session.botToolExecutions || session.botToolExecutionsTurn !== currentTurn) { + session.botToolExecutions = new Map(); + session.botToolExecutionsTurn = currentTurn; + } + mutationStore = session.botToolExecutions; + }With the store scoped to the current turn, the prefix scan at Lines 230-231 reduces to
mutationStore.size > 0.Also applies to: 229-237
🤖 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/BotToolRegistry.js` around lines 182 - 183, Update the mutation store initialization and lookup logic around mutationKey and the mutating-call handling to scope executions to the current turn: when currentTurn changes, discard the prior turn’s map and create a fresh store, while preserving entries within the same turn. Replace the full mutationStore.entries() prefix scan with a direct non-empty check, ensuring only current-turn executions are retained and reused.src/GameServer/Bot/AI/BotWorkflowTelemetry.js-2-2 (1)
2-2: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
activeWorkflowsnever releases abandoned workflows.Line 32 adds an entry on the first phase of a workflow. Line 62 removes it only when the caller passes
options.terminal === true. A workflow that never reaches a terminal phase leaves its entry in the map permanently. Player logout, bot death, an errand timeout, and any thrown exception all produce that outcome.Two consequences:
- The map grows without bound for the whole server uptime.
- The root observation created at Line 25 is never ended, so the trace stays open in the Langfuse backend and the phase spans of later workflows are the only ones that close.
Add a TTL sweep and a cancellation entry point.
🛡️ Proposed fix sketch
const activeWorkflows = new Map(); +const WORKFLOW_TTL_MS = 10 * 60 * 1000; + +function sweep(now) { + activeWorkflows.forEach((workflow, id) => { + if (now - workflow.startedAt < WORKFLOW_TTL_MS) return; + workflow.root?.end( + { workflowId: id, outcome: 'abandoned', durationMs: now - workflow.startedAt }, + LangfuseTracing.observationStatus({ applied: false, reason: 'abandoned' }) + ); + activeWorkflows.delete(id); + }); +} + +function cancel(workflowId, reason = 'cancelled') { + const id = text(workflowId, 128); + const workflow = activeWorkflows.get(id); + if (!workflow) return false; + workflow.root?.end( + { workflowId: id, outcome: 'cancelled', reason, durationMs: Math.max(0, Date.now() - workflow.startedAt) }, + LangfuseTracing.observationStatus({ applied: false, reason }) + ); + activeWorkflows.delete(id); + return true; +}Then call
sweep(Date.now())at the start ofrecordSupply, and exportcancelso the bot lifecycle can close a workflow when a session ends.Also applies to: 23-33, 51-63
🤖 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/BotWorkflowTelemetry.js` at line 2, Add TTL-based cleanup to the activeWorkflows management around recordSupply, including a sweep(timestamp) helper that ends and removes workflows exceeding the configured TTL; invoke sweep(Date.now()) at the start of recordSupply. Add and export cancel so bot lifecycle code can explicitly end and remove a workflow when its session ends, while preserving terminal cleanup behavior.src/GameServer/Bot/AI/BotToolRegistry.js-34-53 (1)
34-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBind the actor when reading coordinates.
Creature.fetchLocX(),fetchLocY(), andfetchLocZ()accessthis.model, so passing them as unbound callbacks makesloccatch aTypeErrorand return0. This preventsworldRevisionfrom detecting actor movement and can makestale_world_stateineffective.🤖 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/BotToolRegistry.js` around lines 34 - 53, Update worldRevision’s coordinate reads so fetchLocX, fetchLocY, and fetchLocZ are invoked with actor as their receiver, preserving the existing loc normalization and fallback behavior while ensuring movement changes the computed revision.src/GameServer/Bot/AI/BotRemoteChat.js-11-31 (1)
11-31: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDenial of Service (CWE-770): Allocation of Resources Without Limits or Throttling
Reachability: External · Exploitability: Moderate
Bound cold-chat work per player–bot pair.
timeoutMs: 0disables the gateway deadline. A hung request keeps the queue head and its budget reservation active.enqueueand the global interactive waiter list have no depth cap.bypass: trueskips request and token quotas, whilecircuitBreaker: falseallows new requests after failures. Set a finite timeout, cap queued tells with a fallback response, and apply a cold-chat budget.🤖 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/BotRemoteChat.js` around lines 11 - 31, Update the cold-chat flow around config(), enqueue(), and the interactive waiter handling to use a finite gateway timeout, limit queued tells per player–bot pair with a fallback response when the cap is reached, and enforce the cold-chat request/token budget. Remove quota bypass behavior and enable circuit-breaker enforcement so hung or repeatedly failing requests cannot hold resources indefinitely.src/Database.js-507-522 (1)
507-522: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTwo transfer entries for the same
sourceItemIdcan duplicate items.The validation loop (Lines 507-516) reads every source row before the mutation loop (Lines 519-522) writes. If two entries reference the same
sourceItemId, both validate against the same pre-write snapshot, and both computeremainingfrom the stalesource.amount. The secondUPDATEoverwrites the first. The target then receivesamount1 + amount2while the source loses onlymax(amount1, amount2).Aggregate the requested amount per source item and validate the total.
🐛 Proposed fix: reject or aggregate duplicate source rows
const sources = entries.map((entry) => { if (!entry.fromCharacterId || !entry.toCharacterId || !entry.sourceItemId || !entry.selfId || entry.amount <= 0) { throw new Error('invalid inventory transfer'); } const source = one('SELECT id, selfId, name, amount, equipped, slot, petData FROM items WHERE id = ? AND characterId = ?', [entry.sourceItemId, entry.fromCharacterId]); if (!source || Number(source.selfId) !== entry.selfId || Number(source.amount) < entry.amount || Number(source.equipped) !== 0) { throw new Error('inventory item changed'); } return { entry, source }; }); + + // A batch must never read the same source row twice: both checks + // would pass against the pre-write snapshot and the second UPDATE + // would overwrite the first, duplicating the item. + const requested = new Map(); + sources.forEach(({ entry }) => { + const key = `${entry.fromCharacterId}:${entry.sourceItemId}`; + requested.set(key, (requested.get(key) || 0) + entry.amount); + }); + sources.forEach(({ entry, source }) => { + if (requested.get(`${entry.fromCharacterId}:${entry.sourceItemId}`) > Number(source.amount)) { + throw new Error('inventory item changed'); + } + });Note that this rejects over-allocation but still leaves the stale
remainingcomputation for duplicates. Derivingremainingfrom a running per-row balance instead ofsource.amountremoves the overwrite completely.🤖 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/Database.js` around lines 507 - 522, Update the transfer validation and mutation flow around the sources mapping and moved loop to aggregate requested amounts by sourceItemId before validating or mutating. Validate each unique source against its total requested amount, then apply a single deletion or update using that aggregate so duplicate entries cannot overwrite one another or transfer more than the source holds.src/GameServer/Bot/AI/OpenRouterGateway.js-455-457 (1)
455-457: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAn interactive request runs with no client timeout.
cfg.timeoutMs > 0gates the abort timer.BotBrain.requestDecisionpassestimeoutMs: 0, so hot dialogue turns get noAbortControllerdeadline and no completion-token limit (completionLimitreturnsnullforinteractive === true). A stalled provider connection then holdssession.brainInFlightuntil the runtime's own socket timeout expires, and the pending-turn queue inBotBrainkeeps growing during that window.Apply an upper bound for interactive requests instead of disabling the timeout completely.
🛡️ Proposed fix
+ // An interactive turn may take longer than a background one, but it must + // still have a deadline: the caller holds brainInFlight until it resolves. + interactiveTimeoutMs: 60000, maxTokens: 320, timeoutMs: 3500,- const timeout = cfg.timeoutMs > 0 - ? setTimeout(() => controller.abort(), Math.max(1, cfg.timeoutMs)) - : null; + const deadlineMs = cfg.timeoutMs > 0 + ? cfg.timeoutMs + : (requestData.interactive === true ? cfg.interactiveTimeoutMs : 0); + const timeout = deadlineMs > 0 + ? setTimeout(() => controller.abort(), Math.max(1, deadlineMs)) + : 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/AI/OpenRouterGateway.js` around lines 455 - 457, Update the timeout calculation near the request’s AbortController setup so interactive requests receive a finite upper-bound deadline even when cfg.timeoutMs is 0. Preserve configured positive timeouts, and apply the interactive fallback only for the BotBrain.requestDecision path without changing unrelated request behavior.src/GameServer/Bot/AI/BotBrain.js-852-864 (1)
852-864: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe pending-turn queue has no bound.
Every chat message that arrives while
session.brainInFlightis true is pushed tosession.pendingBrainTurns. Nothing caps the array length, and nothing drops old entries. Each queued entry later becomes one provider request, andBotInferenceBudget.reserveis called withbypass: trueat Line 882, so the budget does not reject it.One player who sends chat faster than a turn completes therefore grows the array and the outbound inference cost without limit. Cap the queue and drop or coalesce the oldest entries when the cap is reached.
🛡️ Proposed bound on the queue
+const MAX_PENDING_BRAIN_TURNS = 3; +if (session.brainInFlight) { const pending = { event, status, text, requestContext }; const queue = session.pendingBrainTurns || (session.pendingBrainTurns = []); + if (queue.length >= MAX_PENDING_BRAIN_TURNS) { + debugSkip(session, cfg, 'pending_queue_full'); + fallbackReply(session, requestContext, 'pending_queue_full'); + return true; + } queue.push(pending); session.pendingBrainTurn = queue[0]; debugSkip(session, cfg, 'request_queued'); return true; }Run the following script to check whether the chat ingress path already caps admission per player or per bot:
#!/bin/bash set -euo pipefail fd -t f -e js . src/GameServer/Network/Request src/GameServer/Bot/AI \ | xargs -r rg -n -C5 'pendingBrainTurns|admission|maxPending|queueLimit|admit\(' fd -t f 'BotConversationService.js|ChatArrivalState.js|Speak.js' src | xargs -r rg -n -C5 'limit|max|queue|admit'🤖 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/BotBrain.js` around lines 852 - 864, Bound session.pendingBrainTurns in the brainInFlight branch before appending new pending turns. When the configured cap is reached, drop or coalesce the oldest queued entries so the queue remains bounded, while preserving the existing pendingBrainTurn and request_queued behavior for retained entries.src/GameServer/Bot/AI/BotAmbientDirector.js-273-294 (1)
273-294: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStore the participant sessions on the scene instead of recovering them from conversation lines.
finishrebuilds the participant sessions fromscene.conversation?.lines?.[0]?.speakerandlines?.[1]?.speaker.startalready holds the authoritativeinitiatorandresponderpair but does not keep them on the scene. Note thatscene.participantsholds display names (Line 254), not sessions.If the conversation has fewer than two lines, or if
speakeris not the session object,participantsis empty. Thensession.ambientSceneis never cleared.expireSceneIfNeededcallsfinishagain,finishreturns early onscene.finished, andambientScenestays set forever. Every latereligiblecall returnsscene_activeat Line 205, so that bot never joins another ambient scene for the rest of its session.Keep the session pair on the scene and iterate it.
🐛 Proposed fix
const scene = { id: `ambient-${actorId(initiator)}-${actorId(responder)}-${now}`, topic: conversation.topic, participants: [actorName(initiator), actorName(responder)], + sessions: [initiator, responder], startedAt: now,scene.finished = true; BotConversation.finish(scene.conversation || scene); activeScenes.delete(scene.id); - const participants = [scene.conversation?.lines?.[0]?.speaker, scene.conversation?.lines?.[1]?.speaker] - .filter(Boolean); + const participants = (scene.sessions || []).filter(Boolean); participants.forEach((session) => {🤖 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/BotAmbientDirector.js` around lines 273 - 294, Update the scene construction in start to store the authoritative initiator and responder session objects on a dedicated scene field, then change finish to iterate that stored participant pair instead of deriving sessions from conversation lines. Preserve the existing cleanup, event recording, and refresh behavior while ensuring both sessions have ambientScene cleared even when the conversation has fewer than two lines.src/GameServer/Bot/AI/BotConversationStore.js-149-185 (1)
149-185: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not latch a conversation into memory-only mode after a transient database error.
Two caches latch together and prevent recovery:
ensureSchemastores its result inschemaPromise(Lines 130-135). One early failure, for example the database is not ready at server start, fixes the result atfalsefor the process lifetime.ensureConversationreturns the cached entry at Line 156 before any database work. Line 180 callsmemoryEntry, which creates an entry whoseconversation.idstarts withmemory:.After a transient failure the pair is cached with a
memory:id. Every later call returns that entry at Line 156.appendTurnthen skips persistence at Line 307, andloadTurnsskips the database at Line 192. The conversation never persists again, even after the database recovers. OnlyresetMemoryclears it.Cache the schema probe result only on success, and re-attempt the database lookup when the cached entry is still a
memory:placeholder.🛡️ Proposed fix
function ensureSchema() { if (!databaseReady()) return Promise.resolve(false); if (!schemaPromise) { schemaPromise = Database.execute([ 'SELECT 1 FROM bot_conversations LIMIT 1', [] - ], 'schema:bot-conversations').then(() => true).catch(() => false); + ], 'schema:bot-conversations').then(() => true).catch(() => { + // Allow a later retry once the database becomes available. + schemaPromise = null; + return false; + }); } return schemaPromise; }const key = pairKey(player, bot); const cached = memory.get(key); - if (cached) return cached; + // A `memory:` id means persistence was unavailable earlier. Retry the + // database so the pair does not stay memory-only for the whole process. + if (cached && !String(cached.conversation?.id || '').startsWith('memory:')) return cached;🤖 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/BotConversationStore.js` around lines 149 - 185, Update ensureSchema so schemaPromise is retained only after a successful schema probe; clear or avoid caching it when the probe fails so later calls can retry. In ensureConversation, do not immediately return cached entries whose conversation id has the memory: placeholder; re-run the database lookup and creation flow for those entries, while preserving the fast return for persisted conversations.src/GameServer/Bot/AI/BotConversationStore.js-247-265 (1)
247-265: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAllocate the turn ordinal atomically.
Lines 249-257 run
UPDATE bot_conversations SET nextTurnOrdinal = nextTurnOrdinal + 1and a separateSELECT nextTurnOrdinalwith no transaction. Two concurrent appends for the same conversation can both increment and then both read the same larger value, so two messages receive the sameturnOrdinal.The store orders model-visible history by
turnOrdinal, messageOrder, id(Lines 79-85 and 209-212), andmessageOrderderives from the role. A collision interleaves player and bot messages in the history sent to the model.setSummaryalso markscompacted = 1for every row withturnOrdinal <= throughOrdinal(Lines 424-427), so a collision can compact a turn that was never summarized.Allocate and read in one statement, or wrap both statements in a transaction.
Run the following script to confirm whether concurrent same-pair appends are reachable and whether
Database.executesupports transactions:#!/bin/bash # Description: Trace appendTurn callers, per-pair write serialization, and transaction support. set -euo pipefail fd -t f 'Database.js' src ast-grep outline src/Database.js --items all rg -nP -C4 '\b(BEGIN|transaction|serialize|RETURNING)\b' src/Database.js rg -nP -C4 'appendTurn\s*\(' src rg -nP -C8 'queueConversationWrite' src ast-grep run --pattern 'function queueConversationWrite($$$) { $$$ }' --lang javascript src || true🤖 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/BotConversationStore.js` around lines 247 - 265, The turn ordinal allocation in the append flow must be atomic: update the database and obtain the newly allocated value in one statement, or execute the existing UPDATE and SELECT within a transaction. Update the logic surrounding the visible Database.execute calls, preserving the memory: fallback and assignment to entry.conversation.nextTurnOrdinal while ensuring concurrent appends for the same conversation receive distinct ordinals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd2192a0-30df-4ab4-8ce8-633d7ab39f6f
📒 Files selected for processing (122)
config/default.iniconfig/local.example.inidatabase/sql/sqlite.sqlpackage.jsonscripts/run-tests.jssrc/Database.jssrc/GameServer/Actor/Attack.jssrc/GameServer/Actor/Backpack.jssrc/GameServer/Actor/Generics/Die.jssrc/GameServer/Actor/Generics/LevelUp.jssrc/GameServer/Actor/Generics/NpcDied.jssrc/GameServer/Actor/Generics/Revive.jssrc/GameServer/Actor/Generics/Select.jssrc/GameServer/Bot/AI/BotAgentTools.jssrc/GameServer/Bot/AI/BotAmbientDirector.jssrc/GameServer/Bot/AI/BotBrain.jssrc/GameServer/Bot/AI/BotBrainContext.jssrc/GameServer/Bot/AI/BotCombatUtility.jssrc/GameServer/Bot/AI/BotContextAssembler.jssrc/GameServer/Bot/AI/BotConversationService.jssrc/GameServer/Bot/AI/BotConversationStore.jssrc/GameServer/Bot/AI/BotConversationSummarizer.jssrc/GameServer/Bot/AI/BotDialogueArbiter.jssrc/GameServer/Bot/AI/BotEquipmentUpgrade.jssrc/GameServer/Bot/AI/BotEventJournal.jssrc/GameServer/Bot/AI/BotInferenceBudget.jssrc/GameServer/Bot/AI/BotLLMTurnStore.jssrc/GameServer/Bot/AI/BotLootEtiquette.jssrc/GameServer/Bot/AI/BotRemoteChat.jssrc/GameServer/Bot/AI/BotSkillCapabilities.jssrc/GameServer/Bot/AI/BotStatus.jssrc/GameServer/Bot/AI/BotSupplyErrand.jssrc/GameServer/Bot/AI/BotSupportPlanner.jssrc/GameServer/Bot/AI/BotToolAudit.jssrc/GameServer/Bot/AI/BotToolRegistry.jssrc/GameServer/Bot/AI/BotTownTravel.jssrc/GameServer/Bot/AI/BotWorkflowTelemetry.jssrc/GameServer/Bot/AI/ChatArrivalState.jssrc/GameServer/Bot/AI/HotBotPolicyOverlay.jssrc/GameServer/Bot/AI/LangfuseTracing.jssrc/GameServer/Bot/AI/OpenRouterGateway.jssrc/GameServer/Bot/AI/PartyAddressResolver.jssrc/GameServer/Bot/AI/PartyCompanionService.jssrc/GameServer/Bot/AI/PartyDialogueRouter.jssrc/GameServer/Bot/AI/PartyDialogueState.jssrc/GameServer/Bot/AI/PartyLLMRouter.jssrc/GameServer/Bot/AI/PartyPulling.jssrc/GameServer/Bot/AI/States/FollowingState.jssrc/GameServer/Bot/AI/States/ShoppingState.jssrc/GameServer/Bot/BotAI.jssrc/GameServer/Bot/BotManager.jssrc/GameServer/Bot/BotTradeService.jssrc/GameServer/Bot/Economy/BotMerchantStoreService.jssrc/GameServer/Bot/Economy/BotNegotiationService.jssrc/GameServer/Bot/Economy/MarketOpportunity.jssrc/GameServer/Bot/Population/BackgroundResolver.jssrc/GameServer/Bot/Population/BotLifeState.jssrc/GameServer/Bot/Population/Cooldown.jssrc/GameServer/Bot/Population/HotActivation.jssrc/GameServer/Bot/Population/PopulationService.jssrc/GameServer/Bot/TradeService.jssrc/GameServer/Network/Request/Purchase.jssrc/GameServer/Network/Request/Speak.jssrc/GameServer/Network/Request/TradeDone.jssrc/GameServer/Session.jssrc/GameServer/World/Generics/NpcShopBuyLists.jssrc/GameServer/World/TownRespawn.jssrc/GameServer/World/World.jssrc/NodeL2.jstests/test_ai_config_surface.jstests/test_bot_activity_journal.jstests/test_bot_agent_support_confirmation.jstests/test_bot_ambient_director.jstests/test_bot_brain_state_change.jstests/test_bot_chat_commands.jstests/test_bot_context_assembler.jstests/test_bot_conversation_store.jstests/test_bot_conversation_summary.jstests/test_bot_dialogue_arbiter.jstests/test_bot_inference_budget.jstests/test_bot_inference_interactive_queue.jstests/test_bot_llm_party_policy.jstests/test_bot_merchant_store_negotiation.jstests/test_bot_name_suggestion.jstests/test_bot_negotiation_database.jstests/test_bot_negotiation_flow.jstests/test_bot_negotiation_policy.jstests/test_bot_outbound_trade.jstests/test_bot_party_chat.jstests/test_bot_support_planner.jstests/test_bot_tool_authorization.jstests/test_bot_tool_pending_audit.jstests/test_bot_tool_registry.jstests/test_bot_town_travel.jstests/test_bot_trade_atomicity.jstests/test_bot_trade_database.jstests/test_bot_trade_reservations.jstests/test_chat_arrival_state.jstests/test_cold_bot_chat.jstests/test_hot_bot_conversation_flow.jstests/test_hot_bot_policy_overlay.jstests/test_hot_bot_queue_failure.jstests/test_hot_bot_schema_repair.jstests/test_hot_conversation_history_queue.jstests/test_langfuse_tracing.jstests/test_llm_configured_supply_store.jstests/test_llm_equipment_tools.jstests/test_llm_negotiation_tools.jstests/test_llm_party_regroup.jstests/test_llm_pull_policy_tools.jstests/test_llm_skill_priority_tools.jstests/test_llm_supply_errand.jstests/test_llm_trade_tools.jstests/test_openrouter_gateway.jstests/test_party_address_resolver.jstests/test_party_chat_routing_integration.jstests/test_party_dialogue_router.jstests/test_party_dialogue_state.jstests/test_party_llm_router.jstests/test_sqlite_bot_conversation_migration.jstests/test_supply_trade_lifecycle.jstests/test_trade_store_atomicity.js
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/TradeService.js (1)
263-270: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake a failed refund recoverable.
If
giveItemfails afterdeductAdenasucceeds, Line 265 attempts a refund. If that database write also fails, this code only attachesrollbackErrorand rethrows. The actor then has no item and a persisted Adena deduction.Use one database transaction for the debit and item transfer. If that is not possible, persist and retry a compensating refund before reporting the purchase as failed.
🤖 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/TradeService.js` around lines 263 - 270, Update the trade flow around deductAdena, giveItem, and the adenaDeducted rollback so the Adena debit and item transfer execute within one database transaction and commit or roll back together. If a shared transaction is unavailable, persist the failed refund and retry the compensating giveAdena operation before rethrowing the purchase error, rather than only attaching rollbackError.
🤖 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 `@tests/test_trade_store_atomicity.js`:
- Around line 90-95: Update the stale-price rejection test around
TradeService.buyFromStore to retain the buyer returned by actor(4) instead of
passing it inline, then assert the buyer’s Adena balance remains 100 after the
rejection. Preserve the existing assertion that the repriced lot count remains
unchanged.
---
Outside diff comments:
In `@src/GameServer/Bot/TradeService.js`:
- Around line 263-270: Update the trade flow around deductAdena, giveItem, and
the adenaDeducted rollback so the Adena debit and item transfer execute within
one database transaction and commit or roll back together. If a shared
transaction is unavailable, persist the failed refund and retry the compensating
giveAdena operation before rethrowing the purchase error, rather than only
attaching rollbackError.
🪄 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: 5069502c-ced7-4dc3-bfad-92f3e8fa1155
📒 Files selected for processing (6)
src/GameServer/Bot/AI/BotInferenceBudget.jssrc/GameServer/Bot/AI/States/FollowingState.jssrc/GameServer/Bot/TradeService.jstests/test_bot_inference_interactive_queue.jstests/test_llm_supply_errand.jstests/test_trade_store_atomicity.js
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_llm_supply_errand.js
- src/GameServer/Bot/AI/States/FollowingState.js
1 participant
Summary
Why
Bot dialogue previously had no durable conversational context or safe bridge into server actions. Direct messages could bypass the LLM, cold bots were effectively disconnected from chat, party messages could prompt several bots at once, and generated replies could claim actions that had not actually completed. Merchant negotiation also could not update the public store that the player was interacting with.
This change adds one progressive AI layer around the existing bot runtime: the LLM handles player-facing communication and high-level intent, while authoritative game services validate and execute every action.
Player and developer impact
Reliability and safety
Validation
npm testsuitegit diff --checkSummary by CodeRabbit