feat(16): Intelligent ambient NPCs + multi-region world#3
Merged
Conversation
Add LLM-driven ambient intents, background NPC tier, village plaza region, and cross-layer sync so scheduled NPCs wander with visible intent labels while bg NPCs stay muted. Includes verify:phase16 (P16-00–P16-11) and registry fixes. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR adds Phase 16 ambient NPC support across shared schemas, server runtime, web UI, worker flows, world data, and verification scripts. It replaces waypoint-based schedules with zone/mobility segments, introduces background villagers, streams partial speak responses, and wires new ambient intent and region walkability paths end to end. ChangesPhase 16 ambient NPC flow
Sequence Diagram(s)sequenceDiagram
participant GameRoom
participant ambientIntent
participant job_context
participant worker
participant ChatPage
participant RoomScene
GameRoom->>ambientIntent: enqueue segment_change / speak_end intent
worker->>job_context: set partial_emit and phase_timing
worker->>GameRoom: emit speakPartial and done
ChatPage->>RoomScene: pass npcAmbientById and streamingReply
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 315c735768
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+85
to
+86
| setIntent(roomId, npcId, { | ||
| intent: parsed.data, |
There was a problem hiding this comment.
When the worker/LLM returns a zone intent, this route only validates target intents and then caches any non-empty zoneId. A malformed or unqualified zone such as market passes safeParseAmbientIntent, but the next ambient tick calls pickZoneTarget → parseZoneId, which throws on that value and can break the room's ambient simulation. Please reject unknown/malformed zone IDs here (or make the wander lookup non-throwing) before calling setIntent.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major 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)
apps/game-server/src/room/executor.ts (1)
162-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
playersas the default human occupancy source, notroom.player.The current fallback only tracks one human cell. In multiplayer, NPC snap/path blocking can place NPCs onto live player cells when
otherPlayerCellsis omitted.Proposed fix
- const otherPlayers = - options?.otherPlayerCells ?? [{ x: next.player.x, y: next.player.y }]; + const otherPlayers = + options?.otherPlayerCells ?? + [...next.players.values()].map((p) => ({ x: p.x, y: p.y }));As per coding guidelines, Multiplayer: human positions live in
players, not loneRoomState.player.🤖 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 `@apps/game-server/src/room/executor.ts` around lines 162 - 164, The fallback for otherPlayerCells currently uses room.player (via next.player) which only yields a single human cell; change the default source to the room's players collection so multiplayer human positions are respected: when options?.otherPlayerCells is undefined, build otherPlayers from the room.players entries (collecting each player's x,y and excluding the acting player by id) and keep gridOpts = { excludeNpcId: acting.id } unchanged so NPC path/snap blocking won't place NPCs onto live players.Source: Coding guidelines
🟡 Minor comments (4)
apps/game-server/src/routes/internal-ambient-intent.ts-59-63 (1)
59-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire integer
gameMinute.Line 60 allows fractional values; minutes should be integer to keep queue ids and temporal logic consistent.
Proposed fix
- if (!Number.isFinite(gameMinute) || gameMinute < 0 || gameMinute > 1439) { + if (!Number.isInteger(gameMinute) || gameMinute < 0 || gameMinute > 1439) {🤖 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 `@apps/game-server/src/routes/internal-ambient-intent.ts` around lines 59 - 63, The code currently accepts fractional gameMinute values; change the validation in the route (the gameMinute variable and its check) to require an integer by replacing Number.isFinite(gameMinute) with Number.isInteger(gameMinute) (or add Number.isInteger(gameMinute) to the condition), while keeping the bounds check (>=0 && <=1439) and still calling clearPendingNpcIntentJob(roomId, npcId, jobId) and returning the 400 error when validation fails.apps/game-server/src/sse/hub.colyseus.test.ts-18-47 (1)
18-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest name says “only to initiator,” but non-initiator is not asserted.
Add a negative assertion for session
bso this test actually enforces one-recipient routing.Suggested patch
expect( aSends.some( (s) => (s as { type: string }).type === COLYSEUS_SERVER_MESSAGES.speakPartial && (s as { payload: { text?: string } }).payload?.text === "你好呀", ), ).toBe(true); + expect((mockRoom.clients[1]!.send as ReturnType<typeof vi.fn>).mock.calls.length).toBe(0); });🤖 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 `@apps/game-server/src/sse/hub.colyseus.test.ts` around lines 18 - 47, The test currently verifies that the initiator ("a") received a speakPartial message but doesn't assert that non-initiator "b" did not; update the test around registerJob and emitJobEvent to also check that the mock client with sessionId "b" did not receive a COLYSEUS_SERVER_MESSAGES.speakPartial message (e.g., inspect the mock send for "b" or the sends map for "b" and assert absence of a speakPartial payload) so the test enforces one-recipient routing.apps/web/src/lib/colyseusAmbientSnapshot.ts-108-113 (1)
108-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard background NPC inclusion on explicit active=true.
Current logic only skips when
active === false; missingbgNpc{N}Activeis treated as active and can create phantom background ambient/activity entries. Require explicittruebefore inserting.🔧 Proposed fix
for (const [npcId, prefix] of bgSlots) { const active = state[`${prefix}Active` as keyof AmbientSchemaState]; - if (active === false) continue; + if (active !== true) continue; const activityKey = state[`${prefix}ActivityKey` as keyof AmbientSchemaState] ?? "wandering"; npcAmbientById[npcId] = { activityKey: typeof activityKey === "string" ? activityKey : "wandering",Also applies to: 129-133
🤖 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 `@apps/web/src/lib/colyseusAmbientSnapshot.ts` around lines 108 - 113, The current insertion into npcAmbientById uses `active` from state[`${prefix}Active`] and only skips when `active === false`, which treats missing/undefined as active and creates phantom entries; change the guard in the loop to only proceed when `active === true` (i.e., require explicit true) before building the object (the block that reads `const active = state[\`${prefix}Active\` as keyof AmbientSchemaState]; if (active === false) continue;` and then sets `activityKey` and assigns `npcAmbientById[npcId] = { ... }`), and apply the same explicit-true guard to the similar block around the code referenced at 129-133 so missing keys do not create ambient entries.packages/shared/src/speakIntent.ts-53-53 (1)
53-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
旁白from movement intent regex.Including
旁白in move matching will misclassify narration-related text asPHYSICAL.💡 Suggested patch
- /左侧|右侧|左边|右边|上方|下方|下面|下边|旁边|附近|旁白|到我|来我|过来/, + /左侧|右侧|左边|右边|上方|下方|下面|下边|旁边|附近|到我|来我|过来/,🤖 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 `@packages/shared/src/speakIntent.ts` at line 53, The movement-intent regex in speakIntent.ts incorrectly includes the token "旁白", causing narration to be matched as PHYSICAL; edit the movement intent pattern (the regex line containing /左侧|右侧|左边|右边|上方|下方|下面|下边|旁边|附近|旁白|到我|来我|过来/) and remove the "旁白" alternative so narration is not classified as movement.
🧹 Nitpick comments (4)
workers/agent-worker/src/graph/action_intent.py (1)
89-104: 💤 Low valueNPC name disambiguation may still be ambiguous for equal-length names.
When multiple NPCs match, sorting by name length and picking the first works for nested names (e.g., "费" vs "费雪"), but if two NPCs have equal-length names that both appear in the message, the result depends on iteration order from
room.get("npcs"). This is acceptable for now since it's unlikely, but worth noting.🤖 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 `@workers/agent-worker/src/graph/action_intent.py` around lines 89 - 104, The disambiguation in _npc_named_in_message can yield nondeterministic results when multiple NPC names have equal length; update the matching tie-breaker so sorting is deterministic by adding a secondary key (e.g., the name string) after length. Change the sort call on matches to sort by (-len(name), name) or equivalent using str(n.get("name") or "") as the secondary key so equal-length names are resolved consistently rather than by input iteration order.apps/game-server/src/ambient/segment-intent.test.ts (1)
26-44: ⚡ Quick winThe “ordering contract” test is currently a false-positive risk.
enqueueis called by the test itself, so it doesn’t prove SUT ordering, and thenot.toBe("")check can pass whenfindNpc(...)isundefined. Tighten this by asserting NPC existence first and wiring a real observable ordering point from the SUT (e.g., injectable enqueue/spies with call-order checks).🤖 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 `@apps/game-server/src/ambient/segment-intent.test.ts` around lines 26 - 44, The test is a false positive because it calls enqueue itself and allows findNpc(...) to be undefined; change the test to assert NPC existence and wire a real observable enqueue from the SUT instead of calling enqueue manually: stop calling enqueue("npc-2", "segment_change") in the test, inject or mock the module-level enqueue used by applySegmentStartIntentFallback (or update applySegmentStartIntentFallback to accept an enqueue callback) so the spy you create is invoked by the SUT, then assert findNpc(getOrCreate(roomId).state, "npc-2") is defined, expect its intentReasonZh toBeNonEmpty, and verify the injected enqueue spy was called by the SUT and assert call order (e.g., spy call order or toHaveBeenCalledBefore) relative to any state changes to ensure the ordering contract.apps/web/src/ChatPage.tsx (1)
36-50: ⚡ Quick winUnused parameter
_previnmergeRoomStateIntoMoveMap.The
_prevparameter is declared but never referenced in the function body. Consider removing it to clarify intent, or document if it's reserved for future use.♻️ Proposed fix
function mergeRoomStateIntoMoveMap( roomState: RoomState, - _prev: RoomState, grids: Record<string, { x: number; y: number }>, ): RoomState {🤖 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 `@apps/web/src/ChatPage.tsx` around lines 36 - 50, The function mergeRoomStateIntoMoveMap declares an unused parameter _prev; remove _prev from the parameter list and any corresponding typings (RoomState) so the signature becomes (roomState: RoomState, grids: Record<string, { x: number; y: number }>) and update all callers to drop the second argument, or if callers must keep the argument for API compatibility, rename it to _prevUsed and add a brief comment explaining it's intentionally unused; ensure references to mergeRoomStateIntoMoveMap and its type usage are updated accordingly (including any imports/exports) so the unused-parameter warning is resolved.apps/web/src/game/regionCollision.ts (1)
35-41: ⚡ Quick winAdd explicit local-bounds checks before grid indexing.
Line 38 computes
idxwithout validatinglocal.lx/local.lybounds. If those values ever land outside[0,width)/[0,height), the flat-index math can read a wrong in-range cell instead of returningundefined.Suggested patch
const local = fromLocal(region, gx, gy); if (!local) return undefined; + if (local.lx < 0 || local.ly < 0) return undefined; + if (local.lx >= grid.width || local.ly >= grid.height) return undefined; const idx = local.ly * grid.width + local.lx; const cell = grid.cells[idx]; if (cell === undefined) return undefined; return cell === 0;🤖 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 `@apps/web/src/game/regionCollision.ts` around lines 35 - 41, The code computes idx from local.lx/local.ly without validating those coordinates, which can produce a valid flat index for out-of-bounds local coordinates; before computing idx in the regionCollision logic (where local is returned by fromLocal and variables local.lx, local.ly, grid.width, grid.height and grid.cells are used), add explicit bounds checks ensuring 0 <= local.lx < grid.width and 0 <= local.ly < grid.height and return undefined if either check fails, then compute idx and proceed as before.
🤖 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 `@apps/game-server/src/ambient/intent-cache.ts`:
- Around line 114-121: The code currently sets npc.joinVicinityPlayerId by
falling back to nearestPlayerId(roomId, npc) when initiatorPlayerId is missing
inside the joinVicinity branch; change this so joinVicinity strictly requires an
explicit initiator and fails closed when absent: in the joinVicinity handling
(symbols: joinVicinity, joinVicinityPlayerId, initiatorPlayerId,
nearestPlayerId, npc.joinVicinityActive, npc.joinVicinityStartedAt,
npc.joinVicinityUntil, JOIN_VICINITY_MS) remove the nearestPlayerId fallback and
set npc.joinVicinityPlayerId only from initiatorPlayerId (trimmed) or leave
undefined and ensure the code path treats missing initiator as an
error/early-return so no implicit nearest-player inference occurs.
In `@apps/game-server/src/ambient/schedule.ts`:
- Around line 74-90: The mapping that builds segments in schedule.ts currently
casts segment.fromMinute and segment.toMinute with "as number" without runtime
checks; add validation before using them: ensure segment.fromMinute and
segment.toMinute exist, are of type "number" (or coercible to a finite number),
are not NaN, and fall within the valid minute range (e.g., 0 <= minute < 1440)
and that fromMinute < toMinute; if any check fails, throw a descriptive Error
referencing raw.npcId. Update the returned object in that map (the block that
also calls validateActivityKey) to use these validated numeric values instead of
the unchecked casts.
In `@apps/game-server/src/ambient/tick.test.ts`:
- Around line 210-211: The test seeds a human player's position by mutating
map.player (map.player.x / map.player.y) which violates the multiplayer
contract; replace those assignments with the multiplayer state object used by
the code under test (the players map/array) so the test reads/seeds positions
via players (e.g., players[<id>].x / players[<id>].y) and adjust any
assertions/readers to use players instead of map.player; update any test setup
that creates the human player to populate the same player id used by the spatial
logic (so headers like X-Player-Id in the real flow would map to the players
entry).
In `@apps/game-server/src/colyseus/workerStateCache.ts`:
- Around line 15-37: The cache currently only removes expired entries on reads
which allows unbounded growth; update the cache maintenance in
setCachedWorkerState to prune expired entries and enforce a size cap: add a
MAX_CACHE_ENTRIES constant and before inserting in setCachedWorkerState iterate
cache (the Map named cache with values of type CacheEntry) to delete entries
whose expiresAt < Date.now(), then if cache.size >= MAX_CACHE_ENTRIES evict
oldest entries (e.g., by sorting Map entries by expiresAt or tracking insertion
order) until size < MAX_CACHE_ENTRIES, and finally insert the new entry; keep
workerStateCacheKey and getCachedWorkerState unchanged except relying on the
added pruning to prevent memory leaks (you may reference TTL_MS for expiration
calculations).
In `@apps/game-server/src/queue/npc-ambient-intent.ts`:
- Around line 93-99: When replacing or clearing pending jobs you currently
delete only from pendingByNpc but leave entries in mockJobs, causing an
unbounded map; update both code paths that remove or replace pending entries
(the branch handling replace where you call pendingByNpc.delete(pending) and the
branch that returns existingPending/clears pending around lines using
existingPending/pending) to also remove the corresponding key from mockJobs (use
the same pending or existingPending identifier used as the key), ensuring
mockJobs.delete(...) is called whenever pendingByNpc.delete(...) is called so
replaced/cleared jobs are pruned.
In `@apps/game-server/src/routes/chat.ts`:
- Around line 42-54: The race comes from checking isNpcSpeakBusy(npcId) then
awaiting startNpcChatTurn(...) before calling acquireNpcSpeakJob(npcId, jobId),
letting two requests both start jobs and the later Map.set overwrite the earlier
jobId; fix by making slot reservation atomic: introduce a short-lived
reservation token and methods reserveNpcSpeakSlot(npcId, token) and
acquireNpcSpeakJobIfTokenMatches(npcId, jobId, token) (or change
acquireNpcSpeakJob to accept an expected token and only set when the token
matches), then in the route generate a token, call reserveNpcSpeakSlot before
awaiting startNpcChatTurn, pass the token into acquireNpcSpeakJobIfTokenMatches
after you get jobId, and if acquisition fails cancel/cleanup the started job so
the original job can clear the slot reliably; reference getColyseusRoom,
isNpcSpeakBusy, startNpcChatTurn, acquireNpcSpeakJob, and the npcSpeakJobs Map
in your changes.
In `@apps/game-server/src/routes/internal-ambient-intent.ts`:
- Around line 50-56: Guard the intent update so stale job callbacks can't
overwrite newer pending jobs: before calling setIntent in the route that
processes ambient intent callbacks, check that the incoming jobId matches the
currently pending job for that NPC by using a helper like
isPendingNpcIntentJob(roomId, npcId, jobId) (implemented by comparing
pendingByNpc.get(pendingKey(roomId, npcId)) === jobId); if the check fails, call
clearPendingNpcIntentJob(roomId, npcId, jobId) and return a 409/400 without
applying setIntent. Ensure this same guard is used for the block handling 85-92
so only the latest jobId can mutate intent state.
In `@apps/game-server/src/routes/rooms.ts`:
- Around line 379-395: The worker-state cache (built via
workerStateCacheKey/getCachedWorkerState/setCachedWorkerState) can serve stale
data after room mutations; add explicit cache invalidation for the affected room
whenever state-changing endpoints run (e.g., the handlers that implement
apply-actions and reset). Implement a helper like
clearWorkerStateCacheForRoom(roomId) that removes all cache entries for that
room (or a targeted invalidate(key) if keys are tracked), and call it at the end
of the apply-actions handler and the reset handler (and any other mutation
functions) so subsequent calls to the router.get("/:roomId/worker-state") will
rebuild fresh payloads. Ensure the helper references workerStateCacheKey
semantics so it removes all player/flag variants for that room.
In `@apps/game-server/src/world/chunk-loader.ts`:
- Around line 176-179: getWalkability currently short-circuits on
regionWalkabilityAt(gx, gy) and can ignore persisted per-tile overrides; update
getWalkability to first consult the chunk/delta cache that stores patched tile
walkability (the persisted override structure used elsewhere in this file) and
only fall back to regionWalkabilityAt(gx, gy) when no persisted override exists,
i.e., remove/relocate the early return using regionWalk so patched tile state is
respected before returning region data.
In `@apps/web/src/game/activityLabelLogic.ts`:
- Around line 102-115: shouldShowActivity currently returns early when dist >
PROXIMITY_CELLS, preventing resolveActivityLabel from applying the join-vicinity
rule (<= 5); remove that early return and always call resolveActivityLabel with
playerDistanceCells (dist), letting resolveActivityLabel implement the proximity
logic (including the join-vicinity <= 5 rule), or alternatively change the gate
to allow distances up to the join-vicinity threshold before returning false;
update shouldShowActivity so it delegates all distance-based decisions to
resolveActivityLabel (referencing shouldShowActivity, resolveActivityLabel,
PROXIMITY_CELLS and the join-vicinity <= 5 rule).
In `@apps/web/src/hooks/useNpcChat.ts`:
- Around line 292-300: onSpeakPartial currently accepts late/out-of-order
partial packets when there's no active turn; add an early guard in the
onSpeakPartial handler to return immediately if there is no active turn (e.g.
both pendingJobIdRef.current and thinkingNpcIdRef.current are falsy) so stale
speakPartial events cannot resurrect ghost streaming text; keep the existing
jobId and npcId checks (pendingJobIdRef, thinkingNpcIdRef, activeNpcIdRef) but
ensure the new "no active turn" guard runs before processing text or resolving
replyNpcId.
- Around line 545-549: The POST to `${chatBase}/rooms/${mapRoomId}/nl/parse` in
useNpcChat.ts omits per-player headers; update the fetch call in the useNpcChat
hook to include an "X-Player-Id" header (e.g., headers: { "Content-Type":
"application/json", "X-Player-Id": playerId }) so spatial NL parsing is executed
in the correct player context; ensure you reference the existing player
identifier variable in scope (playerId or currentPlayer.id) and add the header
to the same headers object passed to fetch.
In `@packages/shared/src/speakIntent.ts`:
- Line 97: The current intent detection in speakIntent (the check using
msg.includes("帮") || msg.includes("请")) overcaptures polite wording like "请问"
and misclassifies narrative questions; change the logic to match explicit
request patterns instead (e.g., require "请问" or "请" followed by a request verb
like "帮", "帮忙", "给", "把", or "你" or patterns like "帮我") using a regex or
startsWith checks on msg rather than any includes, so only true help requests
(e.g., "请问...", "请帮我...", "帮我...") return "help" and generic appearances of "请"
do not.
In `@packages/shared/src/stableStringHash.ts`:
- Around line 1-7: The current stableStringHash uses UTF-16 code units via
charCodeAt causing mismatches for non-BMP characters; update stableStringHash to
iterate over Unicode code points instead (e.g. using for...of or manual index
with s.codePointAt(i) and advancing by 1 or 2 depending on the code point) and
feed each full code point value into the same hash update h = (Math.imul(31, h)
+ codePoint) | 0, finally returning h >>> 0 so the function (stableStringHash)
produces the same 32-bit unsigned hash as Python's code-point-based
stable_string_hash.
In `@scripts/benchmark-speak-browser.mjs`:
- Around line 124-187: The response handler added in runSpeakRound (onResponse
via page.on("response", onResponse)) must be removed in a finally block so it
always detaches even if an awaited operation throws; wrap the main logic after
registering the handler in a try { ... } finally { page.off("response",
onResponse) } so page.off is executed unconditionally, referencing the existing
onResponse function and the runSpeakRound function to locate the change.
In `@scripts/lib/wait-for-stack.mjs`:
- Around line 28-29: The health-check conditional wrongly uses && and thus
throws when any single field is unexpected; update the logic around the check
that inspects body.service, body.status and body.ok in wait-for-stack.mjs so it
rejects only when none of the positive indicators are present — e.g., change the
compound condition to use ORs (body.service !== "game-server" || body.status !==
"ok" || body.ok !== true) or invert a grouped positive check (e.g., if
(!(body.service === "game-server" || body.status === "ok" || body.ok === true))
throw ...) so a response is accepted if at least one indicator is positive.
In `@scripts/migrate-schedules-phase16.mjs`:
- Around line 13-30: The script currently only validates whatever it finds
(files variable) and can falsely report success if one or more expected NPC
schedule files are missing; before iterating, explicitly verify that the three
expected filenames (e.g., "npc-1.json", "npc-2.json", "npc-3.json") exist in
SCHEDULES_DIR by comparing against the filtered files produced by
/^npc-[1-3]\.json$/; if any are missing, log which are missing and exit non-zero
(matching the existing pattern of using console.error + process.exit(1)) so
migrate-schedules-phase16.mjs does not report success when expected schedule
files are absent.
In `@scripts/run-phase16-e2e.mjs`:
- Line 48: The env spread leaves LLM_MOCK set to the string "undefined" for the
child process; instead construct an environment object that excludes the
LLM_MOCK key (e.g., clone process.env into a newEnv and remove newEnv.LLM_MOCK
or use
Object.fromEntries(Object.entries(process.env).filter(([k])=>k!=='LLM_MOCK'))),
then pass that newEnv as the env in the spawn/options where the current env: {
...process.env, LLM_MOCK: undefined } is used in scripts/run-phase16-e2e.mjs.
In `@scripts/verify-phase16.mjs`:
- Around line 342-363: The current write of verify-report.json happens only in
the inner finally after the browser is launched, so exceptions that occur
earlier in main() (before the inner finally at the page/browser scope) skip
writing the report; wrap the whole body of main() in an outer try/finally (or
move the report-write to a new outer finally) so the report is always written,
and in that outer finally: serialize and write the report object to
verify-report.json, attempt to gracefully close/cleanup chromium/browser/page
only if they are defined (check chromium, browser, page variables), and preserve
the existing inner finally behavior; reference main(), report, chromium,
browser, page to locate where to add the outer try/finally and safe
cleanup/write logic.
In `@workers/agent-worker/src/graph/nodes/llm_social_turn.py`:
- Around line 110-143: The function _extract_reply_from_json_stream incorrectly
advances the index for \uXXXX escapes: change the inner increment from i += 5 to
i += 4 so that the subsequent i += 2 yields a total advance of 6 characters
(backslash, 'u', and four hex digits) instead of 7; keep the existing bounds
check (i + 5 < len(buffer)) and update only the i += 5 -> i += 4 inside the \u
handling block to avoid skipping the character after the escape.
---
Outside diff comments:
In `@apps/game-server/src/room/executor.ts`:
- Around line 162-164: The fallback for otherPlayerCells currently uses
room.player (via next.player) which only yields a single human cell; change the
default source to the room's players collection so multiplayer human positions
are respected: when options?.otherPlayerCells is undefined, build otherPlayers
from the room.players entries (collecting each player's x,y and excluding the
acting player by id) and keep gridOpts = { excludeNpcId: acting.id } unchanged
so NPC path/snap blocking won't place NPCs onto live players.
---
Minor comments:
In `@apps/game-server/src/routes/internal-ambient-intent.ts`:
- Around line 59-63: The code currently accepts fractional gameMinute values;
change the validation in the route (the gameMinute variable and its check) to
require an integer by replacing Number.isFinite(gameMinute) with
Number.isInteger(gameMinute) (or add Number.isInteger(gameMinute) to the
condition), while keeping the bounds check (>=0 && <=1439) and still calling
clearPendingNpcIntentJob(roomId, npcId, jobId) and returning the 400 error when
validation fails.
In `@apps/game-server/src/sse/hub.colyseus.test.ts`:
- Around line 18-47: The test currently verifies that the initiator ("a")
received a speakPartial message but doesn't assert that non-initiator "b" did
not; update the test around registerJob and emitJobEvent to also check that the
mock client with sessionId "b" did not receive a
COLYSEUS_SERVER_MESSAGES.speakPartial message (e.g., inspect the mock send for
"b" or the sends map for "b" and assert absence of a speakPartial payload) so
the test enforces one-recipient routing.
In `@apps/web/src/lib/colyseusAmbientSnapshot.ts`:
- Around line 108-113: The current insertion into npcAmbientById uses `active`
from state[`${prefix}Active`] and only skips when `active === false`, which
treats missing/undefined as active and creates phantom entries; change the guard
in the loop to only proceed when `active === true` (i.e., require explicit true)
before building the object (the block that reads `const active =
state[\`${prefix}Active\` as keyof AmbientSchemaState]; if (active === false)
continue;` and then sets `activityKey` and assigns `npcAmbientById[npcId] = {
... }`), and apply the same explicit-true guard to the similar block around the
code referenced at 129-133 so missing keys do not create ambient entries.
In `@packages/shared/src/speakIntent.ts`:
- Line 53: The movement-intent regex in speakIntent.ts incorrectly includes the
token "旁白", causing narration to be matched as PHYSICAL; edit the movement
intent pattern (the regex line containing
/左侧|右侧|左边|右边|上方|下方|下面|下边|旁边|附近|旁白|到我|来我|过来/) and remove the "旁白" alternative so
narration is not classified as movement.
---
Nitpick comments:
In `@apps/game-server/src/ambient/segment-intent.test.ts`:
- Around line 26-44: The test is a false positive because it calls enqueue
itself and allows findNpc(...) to be undefined; change the test to assert NPC
existence and wire a real observable enqueue from the SUT instead of calling
enqueue manually: stop calling enqueue("npc-2", "segment_change") in the test,
inject or mock the module-level enqueue used by applySegmentStartIntentFallback
(or update applySegmentStartIntentFallback to accept an enqueue callback) so the
spy you create is invoked by the SUT, then assert
findNpc(getOrCreate(roomId).state, "npc-2") is defined, expect its
intentReasonZh toBeNonEmpty, and verify the injected enqueue spy was called by
the SUT and assert call order (e.g., spy call order or toHaveBeenCalledBefore)
relative to any state changes to ensure the ordering contract.
In `@apps/web/src/ChatPage.tsx`:
- Around line 36-50: The function mergeRoomStateIntoMoveMap declares an unused
parameter _prev; remove _prev from the parameter list and any corresponding
typings (RoomState) so the signature becomes (roomState: RoomState, grids:
Record<string, { x: number; y: number }>) and update all callers to drop the
second argument, or if callers must keep the argument for API compatibility,
rename it to _prevUsed and add a brief comment explaining it's intentionally
unused; ensure references to mergeRoomStateIntoMoveMap and its type usage are
updated accordingly (including any imports/exports) so the unused-parameter
warning is resolved.
In `@apps/web/src/game/regionCollision.ts`:
- Around line 35-41: The code computes idx from local.lx/local.ly without
validating those coordinates, which can produce a valid flat index for
out-of-bounds local coordinates; before computing idx in the regionCollision
logic (where local is returned by fromLocal and variables local.lx, local.ly,
grid.width, grid.height and grid.cells are used), add explicit bounds checks
ensuring 0 <= local.lx < grid.width and 0 <= local.ly < grid.height and return
undefined if either check fails, then compute idx and proceed as before.
In `@workers/agent-worker/src/graph/action_intent.py`:
- Around line 89-104: The disambiguation in _npc_named_in_message can yield
nondeterministic results when multiple NPC names have equal length; update the
matching tie-breaker so sorting is deterministic by adding a secondary key
(e.g., the name string) after length. Change the sort call on matches to sort by
(-len(name), name) or equivalent using str(n.get("name") or "") as the secondary
key so equal-length names are resolved consistently rather than by input
iteration order.
🪄 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: f1783d02-1937-4118-a0be-bad03f29e52d
📒 Files selected for processing (137)
.gitignoreapps/game-server/data/schedules/README.mdapps/game-server/data/schedules/npc-1.jsonapps/game-server/data/schedules/npc-2.jsonapps/game-server/data/schedules/npc-3.jsonapps/game-server/data/schedules/schema.jsonapps/game-server/data/world/beginning-fields@v1/collision.jsonapps/game-server/data/world/beginning-fields@v1/pois.jsonapps/game-server/data/world/beginning-fields@v1/spawns.jsonapps/game-server/data/world/beginning-fields@v1/zones.jsonapps/game-server/data/world/regions.jsonapps/game-server/data/world/village-plaza@v1/collision.jsonapps/game-server/data/world/village-plaza@v1/pois.jsonapps/game-server/data/world/village-plaza@v1/spawns.jsonapps/game-server/data/world/village-plaza@v1/zones.jsonapps/game-server/src/ambient/README.mdapps/game-server/src/ambient/intent-cache.test.tsapps/game-server/src/ambient/intent-cache.tsapps/game-server/src/ambient/intent-fallback.test.tsapps/game-server/src/ambient/intent-fallback.tsapps/game-server/src/ambient/schedule.test.tsapps/game-server/src/ambient/schedule.tsapps/game-server/src/ambient/segment-intent.test.tsapps/game-server/src/ambient/segment-intent.tsapps/game-server/src/ambient/tick.test.tsapps/game-server/src/ambient/tick.tsapps/game-server/src/ambient/zone-wander.test.tsapps/game-server/src/ambient/zone-wander.tsapps/game-server/src/colyseus/GameRoom.tsapps/game-server/src/colyseus/bridge.test.tsapps/game-server/src/colyseus/bridge.tsapps/game-server/src/colyseus/npc-chat.test.tsapps/game-server/src/colyseus/npc-chat.tsapps/game-server/src/colyseus/schema.tsapps/game-server/src/colyseus/server.tsapps/game-server/src/colyseus/workerStateCache.tsapps/game-server/src/index.test.tsapps/game-server/src/index.tsapps/game-server/src/memory/service.tsapps/game-server/src/queue/npc-ambient-intent.test.tsapps/game-server/src/queue/npc-ambient-intent.tsapps/game-server/src/queue/npc-turn.tsapps/game-server/src/room/executor.test.tsapps/game-server/src/room/executor.tsapps/game-server/src/routes/chat.tsapps/game-server/src/routes/internal-ambient-intent.tsapps/game-server/src/routes/internal-memories.tsapps/game-server/src/routes/internal.tsapps/game-server/src/routes/rooms.tsapps/game-server/src/speak/casual-stub.test.tsapps/game-server/src/sse/hub.colyseus.test.tsapps/game-server/src/sse/hub.tsapps/game-server/src/world/chunk-loader.test.tsapps/game-server/src/world/chunk-loader.tsapps/game-server/src/world/cross-region.test.tsapps/game-server/src/world/lore-repository.test.tsapps/game-server/src/world/region-walkability.test.tsapps/game-server/src/world/region-walkability.tsapps/game-server/src/world/registry-boot.tsapps/web/public/assets/one-city/BeginningFields.jsonapps/web/public/maps/village-plaza/v1.jsonapps/web/public/world/beginning-fields/collision.jsonapps/web/public/world/village-plaza/collision.jsonapps/web/src/ChatPage.tsxapps/web/src/components/ExploreCoordsStrip.tsxapps/web/src/components/MessageList.test.tsapps/web/src/components/MessageList.tsxapps/web/src/components/PhaserGame.tsxapps/web/src/game/HomeMapBackground.tsapps/web/src/game/RoomScene.activity.test.tsapps/web/src/game/RoomScene.tsapps/web/src/game/activityLabelLogic.tsapps/web/src/game/activityLabels.tsapps/web/src/game/bgNpcLabels.test.tsapps/web/src/game/bgNpcLabels.tsapps/web/src/game/entitySprites.tsapps/web/src/game/floorBlocked.test.tsapps/web/src/game/floorBlocked.tsapps/web/src/game/intentLabels.tsapps/web/src/game/oneCityTilesetManifest.tsapps/web/src/game/regionCollision.tsapps/web/src/hooks/useColyseusRoom.tsapps/web/src/hooks/useNpcChat.test.tsapps/web/src/hooks/useNpcChat.tsapps/web/src/index.cssapps/web/src/lib/chunkWalkability.tsapps/web/src/lib/colyseusAmbientSnapshot.test.tsapps/web/src/lib/colyseusAmbientSnapshot.tsapps/web/src/lib/speakLatencyTrace.tsassets/one-city/BeginningFields.jsondocs/CONTRACTS.mddocs/LLM-E2E-FLOW-AND-LATENCY.mdpackage.jsonpackages/shared/src/ambientIntentSchema.tspackages/shared/src/backgroundNpc.tspackages/shared/src/casualSpeakStub.tspackages/shared/src/colyseus.tspackages/shared/src/homeMap.tspackages/shared/src/index.tspackages/shared/src/intentContent.test.tspackages/shared/src/intentContent.tspackages/shared/src/npcActivity.test.tspackages/shared/src/npcActivity.tspackages/shared/src/room.test.tspackages/shared/src/room.tspackages/shared/src/speakIntent.test.tspackages/shared/src/speakIntent.tspackages/shared/src/stableStringHash.tspackages/shared/src/worldRegion.test.tspackages/shared/src/worldRegion.tsscripts/bake-beginning-fields.mjsscripts/bake-village-plaza.mjsscripts/benchmark-speak-browser.mjsscripts/e2e-full-run.mjsscripts/lib/agent-verify-map.mjsscripts/lib/wait-for-stack.mjsscripts/migrate-schedules-phase16.mjsscripts/run-phase16-e2e.mjsscripts/verify-phase16.mjsworkers/agent-worker/src/graph/action_intent.pyworkers/agent-worker/src/graph/ambient_intent.pyworkers/agent-worker/src/graph/casual_fast_lane.pyworkers/agent-worker/src/graph/job_context.pyworkers/agent-worker/src/graph/nodes/llm_social_turn.pyworkers/agent-worker/src/graph/npc_loop.pyworkers/agent-worker/src/graph/speak_intent.pyworkers/agent-worker/src/graph/stable_string_hash.pyworkers/agent-worker/src/graph/state.pyworkers/agent-worker/src/main.pyworkers/agent-worker/src/memory/client.pyworkers/agent-worker/tests/test_action_intent.pyworkers/agent-worker/tests/test_ambient_intent.pyworkers/agent-worker/tests/test_casual_fast_lane.pyworkers/agent-worker/tests/test_casual_reply_pool.pyworkers/agent-worker/tests/test_fetch_state_and_memory.pyworkers/agent-worker/tests/test_llm_social_degrade.pyworkers/agent-worker/tests/test_speak_intent.py
💤 Files with no reviewable changes (1)
- apps/web/src/game/oneCityTilesetManifest.ts
Comment on lines
+114
to
+121
| if (intent.joinVicinity) { | ||
| const now = Date.now(); | ||
| npc.joinVicinityActive = true; | ||
| npc.joinVicinityStartedAt = now; | ||
| npc.joinVicinityUntil = now + JOIN_VICINITY_MS; | ||
| npc.joinVicinityPlayerId = | ||
| initiatorPlayerId?.trim() || nearestPlayerId(roomId, npc) || undefined; | ||
| } else { |
There was a problem hiding this comment.
Do not infer joinVicinityPlayerId from nearest player when initiator is missing.
This fallback can target the wrong human in multiplayer and hides missing X-Player-Id propagation. For joinVicinity, require explicit initiator ID and fail closed when absent.
Suggested fix
if (intent.joinVicinity) {
+ const playerId = initiatorPlayerId?.trim();
const now = Date.now();
- npc.joinVicinityActive = true;
- npc.joinVicinityStartedAt = now;
- npc.joinVicinityUntil = now + JOIN_VICINITY_MS;
- npc.joinVicinityPlayerId =
- initiatorPlayerId?.trim() || nearestPlayerId(roomId, npc) || undefined;
+ if (playerId) {
+ npc.joinVicinityActive = true;
+ npc.joinVicinityStartedAt = now;
+ npc.joinVicinityUntil = now + JOIN_VICINITY_MS;
+ npc.joinVicinityPlayerId = playerId;
+ } else {
+ npc.joinVicinityActive = false;
+ npc.joinVicinityUntil = 0;
+ npc.joinVicinityStartedAt = 0;
+ npc.joinVicinityPlayerId = undefined;
+ }
} else {As per coding guidelines, **/*.{ts,tsx}: Multiplayer: human positions live in players, not lone RoomState.player; spatial NL needs X-Player-Id header.
📝 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
| if (intent.joinVicinity) { | |
| const now = Date.now(); | |
| npc.joinVicinityActive = true; | |
| npc.joinVicinityStartedAt = now; | |
| npc.joinVicinityUntil = now + JOIN_VICINITY_MS; | |
| npc.joinVicinityPlayerId = | |
| initiatorPlayerId?.trim() || nearestPlayerId(roomId, npc) || undefined; | |
| } else { | |
| if (intent.joinVicinity) { | |
| const playerId = initiatorPlayerId?.trim(); | |
| const now = Date.now(); | |
| if (playerId) { | |
| npc.joinVicinityActive = true; | |
| npc.joinVicinityStartedAt = now; | |
| npc.joinVicinityUntil = now + JOIN_VICINITY_MS; | |
| npc.joinVicinityPlayerId = playerId; | |
| } else { | |
| npc.joinVicinityActive = false; | |
| npc.joinVicinityUntil = 0; | |
| npc.joinVicinityStartedAt = 0; | |
| npc.joinVicinityPlayerId = undefined; | |
| } | |
| } else { |
🤖 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 `@apps/game-server/src/ambient/intent-cache.ts` around lines 114 - 121, The
code currently sets npc.joinVicinityPlayerId by falling back to
nearestPlayerId(roomId, npc) when initiatorPlayerId is missing inside the
joinVicinity branch; change this so joinVicinity strictly requires an explicit
initiator and fails closed when absent: in the joinVicinity handling (symbols:
joinVicinity, joinVicinityPlayerId, initiatorPlayerId, nearestPlayerId,
npc.joinVicinityActive, npc.joinVicinityStartedAt, npc.joinVicinityUntil,
JOIN_VICINITY_MS) remove the nearestPlayerId fallback and set
npc.joinVicinityPlayerId only from initiatorPlayerId (trimmed) or leave
undefined and ensure the code path treats missing initiator as an
error/early-return so no implicit nearest-player inference occurs.
Source: Coding guidelines
Comment on lines
74
to
90
| const segments = raw.segments.map((segment) => { | ||
| if (segment.waypoints.length > MAX_WAYPOINTS) { | ||
| throw new Error(`Schedule ${raw.npcId} segment exceeds max waypoints (${MAX_WAYPOINTS})`); | ||
| if ("waypoints" in segment || "stationary" in segment) { | ||
| throw new Error(`Schedule ${raw.npcId}: legacy waypoints/stationary fields are not supported`); | ||
| } | ||
| if (!segment.zoneId || typeof segment.zoneId !== "string") { | ||
| throw new Error(`Schedule ${raw.npcId}: segment missing zoneId`); | ||
| } | ||
| if (!MOBILITY_SET.has(segment.mobility as Mobility)) { | ||
| throw new Error(`Schedule ${raw.npcId}: invalid mobility ${segment.mobility}`); | ||
| } | ||
| return { | ||
| ...segment, | ||
| activityKey: validateActivityKey(segment.activityKey), | ||
| waypoints: segment.waypoints.map((wp) => ({ gx: wp.gx, gy: wp.gy })), | ||
| fromMinute: segment.fromMinute as number, | ||
| toMinute: segment.toMinute as number, | ||
| activityKey: validateActivityKey(segment.activityKey as string), | ||
| zoneId: segment.zoneId as string, | ||
| mobility: segment.mobility as Mobility, | ||
| }; |
There was a problem hiding this comment.
Validate fromMinute/toMinute before casting.
segment.fromMinute and segment.toMinute are cast with as number but never runtime-validated. Invalid JSON values (string/NaN/out-of-range) can silently pass and break segment resolution instead of failing fast at load time.
Proposed fix
const MOBILITY_SET = new Set<Mobility>(["wander", "stationary", "poi"]);
+function parseMinute(value: unknown, field: "fromMinute" | "toMinute", npcId: string): number {
+ if (!Number.isInteger(value) || (value as number) < 0 || (value as number) >= 1440) {
+ throw new Error(`Schedule ${npcId}: invalid ${field} ${String(value)}`);
+ }
+ return value as number;
+}
+
function parseScheduleFile(filePath: string): NpcSchedule {
@@
return {
- fromMinute: segment.fromMinute as number,
- toMinute: segment.toMinute as number,
+ fromMinute: parseMinute(segment.fromMinute, "fromMinute", raw.npcId),
+ toMinute: parseMinute(segment.toMinute, "toMinute", raw.npcId),
activityKey: validateActivityKey(segment.activityKey as string),
zoneId: segment.zoneId as string,
mobility: segment.mobility as Mobility,
};
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const segments = raw.segments.map((segment) => { | |
| if (segment.waypoints.length > MAX_WAYPOINTS) { | |
| throw new Error(`Schedule ${raw.npcId} segment exceeds max waypoints (${MAX_WAYPOINTS})`); | |
| if ("waypoints" in segment || "stationary" in segment) { | |
| throw new Error(`Schedule ${raw.npcId}: legacy waypoints/stationary fields are not supported`); | |
| } | |
| if (!segment.zoneId || typeof segment.zoneId !== "string") { | |
| throw new Error(`Schedule ${raw.npcId}: segment missing zoneId`); | |
| } | |
| if (!MOBILITY_SET.has(segment.mobility as Mobility)) { | |
| throw new Error(`Schedule ${raw.npcId}: invalid mobility ${segment.mobility}`); | |
| } | |
| return { | |
| ...segment, | |
| activityKey: validateActivityKey(segment.activityKey), | |
| waypoints: segment.waypoints.map((wp) => ({ gx: wp.gx, gy: wp.gy })), | |
| fromMinute: segment.fromMinute as number, | |
| toMinute: segment.toMinute as number, | |
| activityKey: validateActivityKey(segment.activityKey as string), | |
| zoneId: segment.zoneId as string, | |
| mobility: segment.mobility as Mobility, | |
| }; | |
| const segments = raw.segments.map((segment) => { | |
| if ("waypoints" in segment || "stationary" in segment) { | |
| throw new Error(`Schedule ${raw.npcId}: legacy waypoints/stationary fields are not supported`); | |
| } | |
| if (!segment.zoneId || typeof segment.zoneId !== "string") { | |
| throw new Error(`Schedule ${raw.npcId}: segment missing zoneId`); | |
| } | |
| if (!MOBILITY_SET.has(segment.mobility as Mobility)) { | |
| throw new Error(`Schedule ${raw.npcId}: invalid mobility ${segment.mobility}`); | |
| } | |
| return { | |
| fromMinute: parseMinute(segment.fromMinute, "fromMinute", raw.npcId), | |
| toMinute: parseMinute(segment.toMinute, "toMinute", raw.npcId), | |
| activityKey: validateActivityKey(segment.activityKey as string), | |
| zoneId: segment.zoneId as string, | |
| mobility: segment.mobility as Mobility, | |
| }; |
🤖 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 `@apps/game-server/src/ambient/schedule.ts` around lines 74 - 90, The mapping
that builds segments in schedule.ts currently casts segment.fromMinute and
segment.toMinute with "as number" without runtime checks; add validation before
using them: ensure segment.fromMinute and segment.toMinute exist, are of type
"number" (or coercible to a finite number), are not NaN, and fall within the
valid minute range (e.g., 0 <= minute < 1440) and that fromMinute < toMinute; if
any check fails, throw a descriptive Error referencing raw.npcId. Update the
returned object in that map (the block that also calls validateActivityKey) to
use these validated numeric values instead of the unchecked casts.
Comment on lines
+210
to
+211
| map.player.x = 31; | ||
| map.player.y = 12; |
There was a problem hiding this comment.
Use players state instead of map.player in multiplayer-targeting tests.
Lines 210-211 seed player position through map.player, which conflicts with the multiplayer state contract and can hide regressions in player-cell-dependent behavior. Please seed/read human positions via players for this test path.
As per coding guidelines, "Multiplayer: human positions live in players, not lone RoomState.player; spatial NL needs X-Player-Id header".
🤖 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 `@apps/game-server/src/ambient/tick.test.ts` around lines 210 - 211, The test
seeds a human player's position by mutating map.player (map.player.x /
map.player.y) which violates the multiplayer contract; replace those assignments
with the multiplayer state object used by the code under test (the players
map/array) so the test reads/seeds positions via players (e.g., players[<id>].x
/ players[<id>].y) and adjust any assertions/readers to use players instead of
map.player; update any test setup that creates the human player to populate the
same player id used by the spatial logic (so headers like X-Player-Id in the
real flow would map to the players entry).
Source: Coding guidelines
Comment on lines
+15
to
+37
| const cache = new Map<string, CacheEntry>(); | ||
|
|
||
| export function workerStateCacheKey( | ||
| roomId: string, | ||
| playerId: string, | ||
| skipNearbyLore: boolean, | ||
| ): string { | ||
| return `${roomId}:${playerId}:${skipNearbyLore ? 1 : 0}`; | ||
| } | ||
|
|
||
| export function getCachedWorkerState(key: string): WorkerStatePayload | null { | ||
| const entry = cache.get(key); | ||
| if (!entry) return null; | ||
| if (Date.now() > entry.expiresAt) { | ||
| cache.delete(key); | ||
| return null; | ||
| } | ||
| return entry.payload; | ||
| } | ||
|
|
||
| export function setCachedWorkerState(key: string, payload: WorkerStatePayload): void { | ||
| cache.set(key, { expiresAt: Date.now() + TTL_MS, payload }); | ||
| } |
There was a problem hiding this comment.
Unbounded cache growth can leak memory over time.
Expired entries are only removed on read, so keys that are never read again remain indefinitely. Add pruning on write (or a size cap/LRU) to prevent unbounded growth on long-lived servers.
Suggested patch
const cache = new Map<string, CacheEntry>();
+let writes = 0;
+
+function pruneExpired(now: number): void {
+ for (const [k, v] of cache) {
+ if (now > v.expiresAt) cache.delete(k);
+ }
+}
@@
export function setCachedWorkerState(key: string, payload: WorkerStatePayload): void {
- cache.set(key, { expiresAt: Date.now() + TTL_MS, payload });
+ const now = Date.now();
+ writes += 1;
+ if (writes % 100 === 0) pruneExpired(now);
+ cache.set(key, { expiresAt: now + TTL_MS, payload });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const cache = new Map<string, CacheEntry>(); | |
| export function workerStateCacheKey( | |
| roomId: string, | |
| playerId: string, | |
| skipNearbyLore: boolean, | |
| ): string { | |
| return `${roomId}:${playerId}:${skipNearbyLore ? 1 : 0}`; | |
| } | |
| export function getCachedWorkerState(key: string): WorkerStatePayload | null { | |
| const entry = cache.get(key); | |
| if (!entry) return null; | |
| if (Date.now() > entry.expiresAt) { | |
| cache.delete(key); | |
| return null; | |
| } | |
| return entry.payload; | |
| } | |
| export function setCachedWorkerState(key: string, payload: WorkerStatePayload): void { | |
| cache.set(key, { expiresAt: Date.now() + TTL_MS, payload }); | |
| } | |
| const cache = new Map<string, CacheEntry>(); | |
| let writes = 0; | |
| function pruneExpired(now: number): void { | |
| for (const [k, v] of cache) { | |
| if (now > v.expiresAt) cache.delete(k); | |
| } | |
| } | |
| export function workerStateCacheKey( | |
| roomId: string, | |
| playerId: string, | |
| skipNearbyLore: boolean, | |
| ): string { | |
| return `${roomId}:${playerId}:${skipNearbyLore ? 1 : 0}`; | |
| } | |
| export function getCachedWorkerState(key: string): WorkerStatePayload | null { | |
| const entry = cache.get(key); | |
| if (!entry) return null; | |
| if (Date.now() > entry.expiresAt) { | |
| cache.delete(key); | |
| return null; | |
| } | |
| return entry.payload; | |
| } | |
| export function setCachedWorkerState(key: string, payload: WorkerStatePayload): void { | |
| const now = Date.now(); | |
| writes += 1; | |
| if (writes % 100 === 0) pruneExpired(now); | |
| cache.set(key, { expiresAt: now + TTL_MS, payload }); | |
| } |
🤖 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 `@apps/game-server/src/colyseus/workerStateCache.ts` around lines 15 - 37, The
cache currently only removes expired entries on reads which allows unbounded
growth; update the cache maintenance in setCachedWorkerState to prune expired
entries and enforce a size cap: add a MAX_CACHE_ENTRIES constant and before
inserting in setCachedWorkerState iterate cache (the Map named cache with values
of type CacheEntry) to delete entries whose expiresAt < Date.now(), then if
cache.size >= MAX_CACHE_ENTRIES evict oldest entries (e.g., by sorting Map
entries by expiresAt or tracking insertion order) until size <
MAX_CACHE_ENTRIES, and finally insert the new entry; keep workerStateCacheKey
and getCachedWorkerState unchanged except relying on the added pruning to
prevent memory leaks (you may reference TTL_MS for expiration calculations).
Comment on lines
+93
to
+99
| console.warn( | ||
| `[npc-ambient-intent] replacing stale pending room=${input.roomId} npc=${input.npcId} ` + | ||
| `old=${existingPending} trigger=${existing.trigger} minute=${existing.gameMinute}`, | ||
| ); | ||
| pendingByNpc.delete(pending); | ||
| } else { | ||
| return existingPending; |
There was a problem hiding this comment.
Prune mockJobs when pending jobs are replaced/cleared.
Line 97 and Line 130 clear only pendingByNpc; old entries remain in mockJobs indefinitely, so this map grows unbounded in a long-running server.
Proposed fix
@@
if (
existing &&
(existing.gameMinute !== input.gameMinute || existing.trigger !== input.trigger)
) {
@@
- pendingByNpc.delete(pending);
+ pendingByNpc.delete(pending);
+ mockJobs.delete(existingPending);
@@
export function clearPendingNpcIntentJob(
roomId: string,
npcId: string,
jobId?: string,
): void {
@@
if (!pending) return;
if (jobId && pending !== jobId) return;
pendingByNpc.delete(key);
+ mockJobs.delete(pending);
}Also applies to: 121-131
🤖 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 `@apps/game-server/src/queue/npc-ambient-intent.ts` around lines 93 - 99, When
replacing or clearing pending jobs you currently delete only from pendingByNpc
but leave entries in mockJobs, causing an unbounded map; update both code paths
that remove or replace pending entries (the branch handling replace where you
call pendingByNpc.delete(pending) and the branch that returns
existingPending/clears pending around lines using existingPending/pending) to
also remove the corresponding key from mockJobs (use the same pending or
existingPending identifier used as the key), ensuring mockJobs.delete(...) is
called whenever pendingByNpc.delete(...) is called so replaced/cleared jobs are
pruned.
Comment on lines
+28
to
+29
| if (body.service !== "game-server" && body.status !== "ok" && body.ok !== true) { | ||
| throw new Error("unexpected game-server health body"); |
There was a problem hiding this comment.
Fix health check validation logic.
The condition on line 28 uses AND operators (&&) when it should use OR (||). The current logic throws an error if any single field is incorrect, even when service === "game-server". This would incorrectly reject valid health responses where one field matches expectations.
The intent appears to be: reject only when none of the expected indicators are present.
🐛 Proposed fix
- if (body.service !== "game-server" && body.status !== "ok" && body.ok !== true) {
+ if (body.service !== "game-server" || (body.status !== "ok" && body.ok !== true)) {
throw new Error("unexpected game-server health body");
}Or more simply, require at least one positive indicator:
- if (body.service !== "game-server" && body.status !== "ok" && body.ok !== true) {
+ if (body.service !== "game-server" || (body.status !== "ok" && body.ok !== true)) {
throw new Error("unexpected game-server health body");
}📝 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
| if (body.service !== "game-server" && body.status !== "ok" && body.ok !== true) { | |
| throw new Error("unexpected game-server health body"); | |
| if (body.service !== "game-server" || (body.status !== "ok" && body.ok !== true)) { | |
| throw new Error("unexpected game-server health body"); | |
| } |
🤖 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 `@scripts/lib/wait-for-stack.mjs` around lines 28 - 29, The health-check
conditional wrongly uses && and thus throws when any single field is unexpected;
update the logic around the check that inspects body.service, body.status and
body.ok in wait-for-stack.mjs so it rejects only when none of the positive
indicators are present — e.g., change the compound condition to use ORs
(body.service !== "game-server" || body.status !== "ok" || body.ok !== true) or
invert a grouped positive check (e.g., if (!(body.service === "game-server" ||
body.status === "ok" || body.ok === true)) throw ...) so a response is accepted
if at least one indicator is positive.
Comment on lines
+13
to
+30
| const files = readdirSync(SCHEDULES_DIR).filter((f) => /^npc-[1-3]\.json$/.test(f)); | ||
|
|
||
| for (const file of files) { | ||
| const raw = JSON.parse(readFileSync(join(SCHEDULES_DIR, file), "utf8")); | ||
| if (raw.segments.some((s) => "waypoints" in s || "stationary" in s)) { | ||
| console.error(`${file}: legacy waypoints/stationary still present`); | ||
| process.exit(1); | ||
| } | ||
| for (const s of raw.segments) { | ||
| if (!s.zoneId?.includes("@v1:") || !s.mobility) { | ||
| console.error(`${file}: missing zoneId or mobility`); | ||
| process.exit(1); | ||
| } | ||
| } | ||
| console.log(`OK ${file} (${raw.segments.length} segments)`); | ||
| } | ||
|
|
||
| console.log("migrate-schedules-phase16: all schedules valid"); |
There was a problem hiding this comment.
Fail fast when expected schedule files are missing.
At Line [13], the script only validates what is found. If one or more of npc-1/2/3.json is absent, Line [30] still reports success, producing a false pass for migration validation.
Suggested fix
-const files = readdirSync(SCHEDULES_DIR).filter((f) => /^npc-[1-3]\.json$/.test(f));
+const EXPECTED = ["npc-1.json", "npc-2.json", "npc-3.json"];
+const dirFiles = new Set(readdirSync(SCHEDULES_DIR));
+const missing = EXPECTED.filter((f) => !dirFiles.has(f));
+if (missing.length > 0) {
+ console.error(`missing required schedules: ${missing.join(", ")}`);
+ process.exit(1);
+}
+const files = EXPECTED;🤖 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 `@scripts/migrate-schedules-phase16.mjs` around lines 13 - 30, The script
currently only validates whatever it finds (files variable) and can falsely
report success if one or more expected NPC schedule files are missing; before
iterating, explicitly verify that the three expected filenames (e.g.,
"npc-1.json", "npc-2.json", "npc-3.json") exist in SCHEDULES_DIR by comparing
against the filtered files produced by /^npc-[1-3]\.json$/; if any are missing,
log which are missing and exit non-zero (matching the existing pattern of using
console.error + process.exit(1)) so migrate-schedules-phase16.mjs does not
report success when expected schedule files are absent.
| cwd: root, | ||
| stdio: "ignore", | ||
| detached: true, | ||
| env: { ...process.env, LLM_MOCK: undefined }, |
There was a problem hiding this comment.
Environment variable not unset correctly.
Setting LLM_MOCK: undefined in the environment object does not remove the variable. Node.js child_process converts undefined to the string "undefined", so the spawned process receives LLM_MOCK=undefined rather than having the variable unset entirely.
To properly unset the variable, filter it out or use a two-step approach.
🔧 Proposed fix
+ const cleanEnv = { ...process.env };
+ delete cleanEnv.LLM_MOCK;
const child = spawn("pnpm", ["dev:stack"], {
cwd: root,
stdio: "ignore",
detached: true,
- env: { ...process.env, LLM_MOCK: undefined },
+ env: cleanEnv,
});📝 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
| env: { ...process.env, LLM_MOCK: undefined }, | |
| const cleanEnv = { ...process.env }; | |
| delete cleanEnv.LLM_MOCK; | |
| const child = spawn("pnpm", ["dev:stack"], { | |
| cwd: root, | |
| stdio: "ignore", | |
| detached: true, | |
| env: cleanEnv, | |
| }); |
🤖 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 `@scripts/run-phase16-e2e.mjs` at line 48, The env spread leaves LLM_MOCK set
to the string "undefined" for the child process; instead construct an
environment object that excludes the LLM_MOCK key (e.g., clone process.env into
a newEnv and remove newEnv.LLM_MOCK or use
Object.fromEntries(Object.entries(process.env).filter(([k])=>k!=='LLM_MOCK'))),
then pass that newEnv as the env in the spawn/options where the current env: {
...process.env, LLM_MOCK: undefined } is used in scripts/run-phase16-e2e.mjs.
Comment on lines
+342
to
+363
| async function main() { | ||
| assertE2eNoMock("verify:phase16"); | ||
| console.log(`verify:phase16 → ${webUrl} WORLD_SEED=${process.env.WORLD_SEED}`); | ||
| await healthOk(); | ||
| record("P16-00", "game-server health", true); | ||
|
|
||
| const bgState = await assertBackgroundNpcSpeakBlocked(); | ||
| report.bgNpcState = bgState; | ||
| record( | ||
| "P16-11", | ||
| "background NPC speak blocked (HTTP chat 400)", | ||
| true, | ||
| `${bgState.bgCount} bg ids: ${bgState.bgIds.join(", ")}`, | ||
| ); | ||
|
|
||
| const chromium = await loadPlaywright(); | ||
| const browser = await chromium.launch({ headless: true }); | ||
| let page; | ||
| try { | ||
| page = await browser.newPage(); | ||
| const bootStart = Date.now(); | ||
| await page.goto(webUrl, { waitUntil: "domcontentloaded", timeout: 30_000 }); |
There was a problem hiding this comment.
Ensure verify-report.json is written for pre-browser failures too.
Right now, failures before Line [357] skip the current finally path that writes the report. That drops key diagnostics for exactly the failures most likely to happen in CI setup.
Suggested fix
async function main() {
- assertE2eNoMock("verify:phase16");
- console.log(`verify:phase16 → ${webUrl} WORLD_SEED=${process.env.WORLD_SEED}`);
- await healthOk();
- record("P16-00", "game-server health", true);
+ let browser;
+ let page;
+ try {
+ assertE2eNoMock("verify:phase16");
+ console.log(`verify:phase16 → ${webUrl} WORLD_SEED=${process.env.WORLD_SEED}`);
+ await healthOk();
+ record("P16-00", "game-server health", true);
@@
- const chromium = await loadPlaywright();
- const browser = await chromium.launch({ headless: true });
- let page;
- try {
+ const chromium = await loadPlaywright();
+ browser = await chromium.launch({ headless: true });
page = await browser.newPage();
@@
- } finally {
- await browser.close();
- await writeReport();
- }
+ } finally {
+ if (browser) await browser.close();
+ await writeReport();
+ }Also applies to: 560-563
🤖 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 `@scripts/verify-phase16.mjs` around lines 342 - 363, The current write of
verify-report.json happens only in the inner finally after the browser is
launched, so exceptions that occur earlier in main() (before the inner finally
at the page/browser scope) skip writing the report; wrap the whole body of
main() in an outer try/finally (or move the report-write to a new outer finally)
so the report is always written, and in that outer finally: serialize and write
the report object to verify-report.json, attempt to gracefully close/cleanup
chromium/browser/page only if they are defined (check chromium, browser, page
variables), and preserve the existing inner finally behavior; reference main(),
report, chromium, browser, page to locate where to add the outer try/finally and
safe cleanup/write logic.
Comment on lines
+110
to
+143
| def _extract_reply_from_json_stream(buffer: str) -> str: | ||
| """Best-effort incremental reply field extraction from streaming JSON.""" | ||
| match = re.search(r'"reply"\s*:\s*"', buffer) | ||
| if not match: | ||
| return "" | ||
| i = match.end() | ||
| chars: list[str] = [] | ||
| while i < len(buffer): | ||
| ch = buffer[i] | ||
| if ch == "\\": | ||
| if i + 1 >= len(buffer): | ||
| break | ||
| nxt = buffer[i + 1] | ||
| if nxt == "n": | ||
| chars.append("\n") | ||
| elif nxt == "t": | ||
| chars.append("\t") | ||
| elif nxt in {'"', "\\", "/"}: | ||
| chars.append(nxt) | ||
| elif nxt == "u" and i + 5 < len(buffer): | ||
| try: | ||
| chars.append(chr(int(buffer[i + 2 : i + 6], 16))) | ||
| i += 5 | ||
| except ValueError: | ||
| break | ||
| else: | ||
| chars.append(nxt) | ||
| i += 2 | ||
| continue | ||
| if ch == '"': | ||
| break | ||
| chars.append(ch) | ||
| i += 1 | ||
| return "".join(chars) |
There was a problem hiding this comment.
Off-by-one error in \uXXXX escape handling skips the next character.
When processing a \uXXXX escape sequence, i += 5 followed by i += 2 advances by 7 total, but the sequence is only 6 characters long (\u + 4 hex digits). This causes the character immediately after the escape to be skipped.
For example, parsing "test\u0041more" would produce "testAore" instead of "testAmore".
🐛 Proposed fix
elif nxt == "u" and i + 5 < len(buffer):
try:
chars.append(chr(int(buffer[i + 2 : i + 6], 16)))
- i += 5
+ i += 6
+ continue
except ValueError:
break
else:
chars.append(nxt)
- i += 2
- continue
+ i += 2Alternatively, restructure to explicitly track the consumed length for each escape type.
🤖 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 `@workers/agent-worker/src/graph/nodes/llm_social_turn.py` around lines 110 -
143, The function _extract_reply_from_json_stream incorrectly advances the index
for \uXXXX escapes: change the inner increment from i += 5 to i += 4 so that the
subsequent i += 2 yields a total advance of 6 characters (backslash, 'u', and
four hex digits) instead of 7; keep the existing bounds check (i + 5 <
len(buffer)) and update only the i += 5 -> i += 4 inside the \u handling block
to avoid skipping the character after the escape.
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Jun 11, 2026
Docstrings generation was requested by @moyunzero. * #3 (comment) The following files were modified: * `apps/game-server/src/ambient/intent-cache.ts` * `apps/game-server/src/ambient/intent-fallback.ts` * `apps/game-server/src/ambient/schedule.ts` * `apps/game-server/src/ambient/segment-intent.ts` * `apps/game-server/src/ambient/tick.ts` * `apps/game-server/src/ambient/zone-wander.ts` * `apps/game-server/src/colyseus/bridge.ts` * `apps/game-server/src/colyseus/npc-chat.ts` * `apps/game-server/src/colyseus/server.ts` * `apps/game-server/src/colyseus/workerStateCache.ts` * `apps/game-server/src/index.ts` * `apps/game-server/src/queue/npc-ambient-intent.ts` * `apps/game-server/src/queue/npc-turn.ts` * `apps/game-server/src/room/executor.ts` * `apps/game-server/src/routes/chat.ts` * `apps/game-server/src/routes/internal-ambient-intent.ts` * `apps/game-server/src/routes/internal-memories.ts` * `apps/game-server/src/routes/internal.ts` * `apps/game-server/src/routes/rooms.ts` * `apps/game-server/src/sse/hub.ts` * `apps/game-server/src/world/region-walkability.ts` * `apps/game-server/src/world/registry-boot.ts` * `apps/web/src/ChatPage.tsx` * `apps/web/src/components/ExploreCoordsStrip.tsx` * `apps/web/src/components/MessageList.tsx` * `apps/web/src/components/PhaserGame.tsx` * `apps/web/src/game/activityLabelLogic.ts` * `apps/web/src/game/activityLabels.ts` * `apps/web/src/game/bgNpcLabels.ts` * `apps/web/src/game/entitySprites.ts` * `apps/web/src/game/floorBlocked.ts` * `apps/web/src/game/intentLabels.ts` * `apps/web/src/game/regionCollision.ts` * `apps/web/src/hooks/useColyseusRoom.ts` * `apps/web/src/hooks/useNpcChat.ts` * `apps/web/src/lib/chunkWalkability.ts` * `apps/web/src/lib/colyseusAmbientSnapshot.ts` * `apps/web/src/lib/speakLatencyTrace.ts` * `packages/shared/src/ambientIntentSchema.ts` * `packages/shared/src/backgroundNpc.ts` * `packages/shared/src/casualSpeakStub.ts` * `packages/shared/src/homeMap.ts` * `packages/shared/src/intentContent.ts` * `packages/shared/src/room.ts` * `packages/shared/src/speakIntent.ts` * `packages/shared/src/stableStringHash.ts` * `packages/shared/src/worldRegion.ts` * `scripts/bake-beginning-fields.mjs` * `scripts/bake-village-plaza.mjs` * `scripts/benchmark-speak-browser.mjs` * `scripts/e2e-full-run.mjs` * `scripts/lib/wait-for-stack.mjs` * `scripts/run-phase16-e2e.mjs` * `scripts/verify-phase16.mjs` * `workers/agent-worker/src/graph/action_intent.py` * `workers/agent-worker/src/graph/ambient_intent.py` * `workers/agent-worker/src/graph/casual_fast_lane.py` * `workers/agent-worker/src/graph/job_context.py` * `workers/agent-worker/src/graph/nodes/llm_social_turn.py` * `workers/agent-worker/src/graph/npc_loop.py` * `workers/agent-worker/src/graph/speak_intent.py` * `workers/agent-worker/src/graph/stable_string_hash.py` * `workers/agent-worker/src/main.py` * `workers/agent-worker/src/memory/client.py` * `workers/agent-worker/tests/test_ambient_intent.py` * `workers/agent-worker/tests/test_casual_fast_lane.py`
4 tasks
This was referenced Jun 12, 2026
6 tasks
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
bg-villager-*), and village plaza cross-region travel with world registry + collision data.ambient_intentgraph, web intent/bg nameplate rendering, sharedworldRegion/ambientIntentSchemacontracts.pnpm verify:phase16(P16-00–P16-11) including bg nameplate proximity search and cross-region HUD checks; fix schedule registry test for missing zone refs.Test plan
pnpm agent:verify— game-server 170, web 81, shared 96, worker 180 pytestpnpm verify:phase16— all P16-00…P16-11 pass (real LLM stack,pnpm dev:stack, noLLM_MOCK)agent:verifyon pushPlanning (local)
.planning/ROADMAP.md+.planning/STATE.mdupdated locally: Phase 16 SHIPPED 2026-06-11 (gitignored per repo policy).Made with Cursor
Summary by CodeRabbit