refactor(18): modularize RoomScene and code-health batch#6
Merged
Conversation
Split RoomScene sync/camera/floor/input/NPC motion into dedicated modules and extract useNpcChat into hooks/npcChat/. Cross-layer hygiene across game-server, worker, gateway, and packages; DRY verify scripts via scripts/lib; add ESLint gate and CODE-QUALITY-REVIEW documentation. Verified: pnpm agent:verify; pnpm agent:verify --e2e --base (fresh dev:stack) Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (37)
📝 WalkthroughWalkthroughThis PR adds scoped player request guards, non-blocking NPC speak acquisition and job rollback handling, worker LLM/queue hardening, extracted RoomScene impl modules, shared script/env utilities, tests, schema defaults, ESLint for scripts, and documentation updates. ChangesPhase 16-17 Core Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/game-server/src/routes/chat.ts (1)
56-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister the job before enqueuing the turn.
Line 59 starts the worker turn before the registry entry exists. That recreates the fast-path race
GameRoomis already guarding against: if the worker emits early events before Line 63 runs, room routing and/:roomId/eventslookup can miss the job entirely.Suggested fix
let speakAcquired = Boolean(colyseusRoom); try { + if (colyseusRoom) { + registerJob(jobId, colyseusRoom, roomId, undefined, { + npcId, + playerId, + playerMessage: message, + }); + } const casualStub = previewCasualSpeakStub(message); await startNpcChatTurn(roomId, message, npcId, playerId, jobId, { casualPreviewEmitted: Boolean(casualStub), }); - if (colyseusRoom) { - registerJob(jobId, colyseusRoom, roomId, undefined, { - npcId, - playerId, - playerMessage: message, - }); - } if (casualStub) { emitJobEvent(jobId, "speakPartial", { text: casualStub, npcId }); }🤖 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/chat.ts` around lines 56 - 68, The worker turn is started via startNpcChatTurn before the registry entry is created, causing a race; move the registerJob call to run before startNpcChatTurn so the job is registered prior to enqueuing work. Specifically, call registerJob(jobId, colyseusRoom, roomId, undefined, { npcId, playerId, playerMessage: message }) (preserving the colyseusRoom guard) immediately after computing casualStub/previewCasualSpeakStub and before invoking startNpcChatTurn, then proceed to await startNpcChatTurn as before to ensure routing and /:roomId/events lookups can find the job.scripts/agent-verify.mjs (1)
26-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad root env before resolving
AGENT_SCOPE.
scopeis captured beforeloadRootEnv(root)runs, so anAGENT_SCOPEdefined in.envis ignored and--scope-checkbehaves as if no scope was declared.Suggested fix
const root = resolve(dirname(fileURLToPath(import.meta.url)), ".."); +loadRootEnv(root); const args = process.argv.slice(2); const planOnly = args.includes("--plan"); const runE2e = args.includes("--e2e"); const scopeCheck = args.includes("--scope-check"); @@ function main() { - loadRootEnv(root); const files = changedFiles();Also applies to: 87-89
🤖 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/agent-verify.mjs` around lines 26 - 39, The AGENT_SCOPE env is being read into the local variable scope before calling loadRootEnv(root), so values from the project's .env are ignored; move the invocation of loadRootEnv(root) to run before computing scope (or recompute scope after loadRootEnv) so that process.env.AGENT_SCOPE can be populated and used; update the code paths around root, args, scopeIdx, and scope (and similarly the later occurrence at lines ~87-89) to ensure scope is resolved only after loadRootEnv(root) completes.
🧹 Nitpick comments (3)
workers/agent-worker/src/graph/nodes/llm_social_turn.py (1)
63-64: ⚡ Quick winStale comment contradicts the new value.
The comment says "One attempt per provider" but
SOCIAL_LLM_MAX_ATTEMPTSis now3. Update the comment to reflect the new retry budget.📝 Suggested comment update
-# One attempt per provider — APITimeout × 3 retries caused ~135s hangs before fallback. +# Up to 3 attempts per provider/model combo — balances resilience vs. timeout accumulation. SOCIAL_LLM_MAX_ATTEMPTS = 3🤖 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 63 - 64, Update the stale comment above SOCIAL_LLM_MAX_ATTEMPTS to reflect the current retry budget (now 3 attempts) instead of "One attempt per provider"; e.g., mention that there are three attempts per provider or a 3-attempt retry budget to match the constant name SOCIAL_LLM_MAX_ATTEMPTS and avoid contradiction.apps/web/src/hooks/npcChat/attitudeGate.ts (1)
5-7: 💤 Low valueConsider consolidating duplicate case bodies.
The
"interact"and"generic"cases return identical text. They can share a single case block for clarity.♻️ Consolidate duplicate cases
case "transfer": return `${npcName}拒绝配合这个请求。`; case "interact": case "generic": return `${npcName}现在不愿意帮忙。`;🤖 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/hooks/npcChat/attitudeGate.ts` around lines 5 - 7, The switch handling NPC attitudes contains duplicate bodies for the "interact" and "generic" cases; locate the switch in attitudeGate.ts and merge those two cases into a single case block (e.g., case "interact": case "generic": return `${npcName}现在不愿意帮忙。`;), removing the duplicate separate return so both labels share the same body referencing npcName.package.json (1)
73-80: Expandlintto include verify-phase entrypoints
package.jsoncurrently lints onlyscripts/libandscripts/agent-verify.mjs, so verify-phase entrypoints likescripts/verify-phase3.mjsandscripts/verify-phase15.mjsare left out of the lint gate."lint": "eslint scripts/lib scripts/agent-verify.mjs scripts/verify-phase3.mjs scripts/verify-phase15.mjs"🤖 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 `@package.json` around lines 73 - 80, The npm "lint" script in package.json only targets "scripts/lib" and "scripts/agent-verify.mjs", so add the missing verify-phase entrypoints (e.g., "scripts/verify-phase3.mjs" and "scripts/verify-phase15.mjs") to the "lint" command so ESLint runs against those files as well; update the "lint" script value to include those filenames (referencing the "lint" script key and the verify-phase script filenames) and run the linter to confirm no remaining issues.
🤖 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/ai-gateway/app/services/llm.py`:
- Around line 148-155: The except block currently breaks on a parse error for
the final key causing last_error to be raised immediately and preventing the
_heuristic_parse(message) fallback; change the flow so parse errors set
last_error but do not force an immediate raise: stop breaking/raising inside the
loop (remove the final break) and after the loop only raise last_error if there
is no usable message (e.g., if message is falsy), otherwise call and return
_heuristic_parse(message); update the except handling around last_error and the
post-loop check so functions like _heuristic_parse, message, last_error and the
keys/key_idx logic are used as described.
In `@apps/game-server/src/colyseus/job-registry.ts`:
- Around line 11-30: The registry currently evicts the oldest job blindly in
evictOldestJobIfNeeded, which can drop in-flight jobs; change the eviction
policy so active jobs are preserved: either (A) update evictOldestJobIfNeeded to
skip entries that indicate activity (e.g., entries with sessionId set or a
running flag) and only delete the oldest non-active entry, iterating until one
is found or none remain, or (B) move completed jobs into a separate bounded
store and only apply the MAX_JOBS cap to that completedJobs map—update
registerJob and the job-completion path to transfer entries to completedJobs and
leave active entries in jobs so getJobEntry/getJobRoom continue to work for live
jobs. Ensure references to evictOldestJobIfNeeded, registerJob, getJobEntry and
getJobRoom are adjusted accordingly.
In `@apps/game-server/src/queue/npc-turn.ts`:
- Around line 89-90: The enqueue of the BullMQ job (q.add) and the bridge
enqueue (pushBridgeJob) must be made failure-atomic: if q.add succeeds but
pushBridgeJob fails we must rollback the queue job so callers don't treat the
turn as failed while a backend actually accepted it. Wrap the two calls
together, and on pushBridgeJob failure call the queue removal API (remove job by
jobId from q) before rethrowing the error; use the same jobId passed to q.add
and ensure the catch paths in GameRoom/chat still see the original error. This
keeps q.add and pushBridgeJob logically atomic and prevents a concurrent second
turn from being admitted for the same NPC.
In `@apps/web/src/game/roomSceneInput.ts`:
- Around line 101-117: The pointerdown handler in setupRoomSceneInput currently
sends movement immediately (sendMoveTo, setPathTarget, drawPathPreview), which
triggers on the first finger of a multitouch pinch; change the logic to ignore
pointerdown when multiple touch pointers are active (i.e., only proceed if this
is a single-touch/tap intent) or, for full safety, move the tap-to-move
confirmation to pointerup instead of pointerdown: detect single-touch (no other
active pointers) before calling ctx.getMovementSync().sendMoveTo,
ctx.setPathTarget, and drawPathPreview inside the handler (or relocate that
sequence to the pointerup handler to confirm tap), and apply the same change to
the other similar handler block referenced around lines 134-144 so pinch-zoom no
longer dispatches unintended moves.
In `@docs/CODE-QUALITY-REVIEW.md`:
- Around line 96-99: Update the table row that bundles "verify 脚本 env/speak 重复"
into two separate rows: mark the env/bootstrap dedup as completed (✅) and
reference the consolidated implementation in scripts/lib/env.mjs and any
migrated phase scripts (e.g., the centralized env bootstrap changes), and create
a separate deferred row for the speak dedup (⏸ defer) with a note that
speak-specific refactor remains outstanding; ensure the row labels use the
original text "verify 脚本 env 重复" and "verify 脚本 speak 重复" (or similarly distinct
identifiers) so the backlog accurately reflects that env dedup is done while
speak dedup is still pending.
In `@docs/ISSUE-LOG.md`:
- Around line 1882-1885: The ISSUE-LOG entry for reset(roomId) uses non-enum
values; change the status line to "状态: open", remove "deferred" from that enum
field and instead mention the deferred note in the prose of the entry, and
replace "严重性: medium" with one of the documented buckets (blocker | major |
minor) — pick the appropriate mapped value (e.g., major) and ensure the enum
formatting matches other entries for consistency.
In `@eslint.config.js`:
- Around line 7-24: Update the ESLint override that currently targets files:
["scripts/lib/**/*.mjs", "scripts/agent-verify.mjs"] so the same
languageOptions.globals and rules apply to the broader script cohort (e.g.,
include patterns like scripts/verify-*.mjs and scripts/uat-*.mjs); add missing
global names in languageOptions.globals such as window, AbortSignal,
AbortController, KeyboardEvent (and any other browser-like globals referenced by
those scripts) and keep existing readonly entries (process, fetch, console,
TextDecoder, URL, setTimeout, clearTimeout) so no-undef/no-unused-vars warnings
are suppressed for those files.
In `@packages/npc-memory/src/schema.ts`:
- Line 36: The DB default for npc_id is inconsistent: schema.ts sets
DEFAULT_NPC_ID ("npc-1") for npcMemories.npcId and memorySummaries.npcId while
migrations/0001_npc_memory.sql uses DEFAULT '1', leaving legacy rows orphaned;
fix by adding a new migration that ALTERs both columns to DEFAULT 'npc-1' and
runs an UPDATE to convert existing rows WHERE npc_id = '1' → 'npc-1', and/or add
a short-term fallback in MemoryRepository/MemoryService to treat '1' as
LEGACY_DEFAULT_NPC_ID when reading until the migration is applied. Ensure you
reference DEFAULT_NPC_ID, npcMemories.npcId, memorySummaries.npcId,
migrations/0001_npc_memory.sql, and MemoryRepository/MemoryService in your
changes.
In `@scripts/lib/e2e-sse.mjs`:
- Around line 24-57: The SSE reader.read() can block indefinitely so enforce the
timeout per read by wrapping await reader.read() in a Promise.race that races
reader.read() against a timeout Promise based on timeoutMs and started; if the
timeout wins, cancel the reader (reader.cancel()) and throw a timeout error
mentioning jobId/timeoutMs, otherwise proceed with the returned {value, done} as
before; update references to reader.read(), timeoutMs, started, reader.cancel(),
and the outer loop so the helper fails fast when the stream goes idle.
In `@scripts/lib/env.mjs`:
- Around line 41-42: gameServerWsUrl() currently always returns
"ws://127.0.0.1:2567" when GAME_SERVER_WS is unset, causing mismatch with
gameServerHttpBase(); change gameServerWsUrl() to mirror the same derivation
logic as gameServerHttpBase(): if process.env.GAME_SERVER_WS is set use that,
otherwise compute the host/port from process.env.GAME_SERVER_URL and
process.env.GAME_SERVER_PORT (falling back to the same defaults used by
gameServerHttpBase()), and construct the ws(s) URL using ws:// or wss://
consistent with the HTTP scheme and the same host/port defaults so both helpers
point at the same server when env vars are partially customized.
- Around line 20-27: The loader is copying raw tail strings including
surrounding quotes into process.env; update the logic in loadRootEnv (the loop
using variables trimmed, eq, key) to compute value = trimmed.slice(eq +
1).trim(), then if value starts and ends with a matching single or double quote
strip those outer quotes before assigning — finally set process.env[key] =
unquotedValue; preserve existing behavior for empty/unquoted values and
comment/invalid lines.
In `@scripts/uat-phase7-reset-snap.mjs`:
- Around line 103-105: The script reads process.env.WEB_URL and
process.env.GAME_SERVER_URL before the .env is loaded, so move the call to
loadRootEnv(root) to run before those env vars are captured; specifically ensure
loadRootEnv(root) is invoked at the top (or before any reads of
WEB_URL/GAME_SERVER_URL) so main() and any code that sets local constants for
WEB_URL and GAME_SERVER_URL observe the loaded .env values, keeping
assertE2eNoMock("uat:phase7:reset-snap") in place.
---
Outside diff comments:
In `@apps/game-server/src/routes/chat.ts`:
- Around line 56-68: The worker turn is started via startNpcChatTurn before the
registry entry is created, causing a race; move the registerJob call to run
before startNpcChatTurn so the job is registered prior to enqueuing work.
Specifically, call registerJob(jobId, colyseusRoom, roomId, undefined, { npcId,
playerId, playerMessage: message }) (preserving the colyseusRoom guard)
immediately after computing casualStub/previewCasualSpeakStub and before
invoking startNpcChatTurn, then proceed to await startNpcChatTurn as before to
ensure routing and /:roomId/events lookups can find the job.
In `@scripts/agent-verify.mjs`:
- Around line 26-39: The AGENT_SCOPE env is being read into the local variable
scope before calling loadRootEnv(root), so values from the project's .env are
ignored; move the invocation of loadRootEnv(root) to run before computing scope
(or recompute scope after loadRootEnv) so that process.env.AGENT_SCOPE can be
populated and used; update the code paths around root, args, scopeIdx, and scope
(and similarly the later occurrence at lines ~87-89) to ensure scope is resolved
only after loadRootEnv(root) completes.
---
Nitpick comments:
In `@apps/web/src/hooks/npcChat/attitudeGate.ts`:
- Around line 5-7: The switch handling NPC attitudes contains duplicate bodies
for the "interact" and "generic" cases; locate the switch in attitudeGate.ts and
merge those two cases into a single case block (e.g., case "interact": case
"generic": return `${npcName}现在不愿意帮忙。`;), removing the duplicate separate return
so both labels share the same body referencing npcName.
In `@package.json`:
- Around line 73-80: The npm "lint" script in package.json only targets
"scripts/lib" and "scripts/agent-verify.mjs", so add the missing verify-phase
entrypoints (e.g., "scripts/verify-phase3.mjs" and "scripts/verify-phase15.mjs")
to the "lint" command so ESLint runs against those files as well; update the
"lint" script value to include those filenames (referencing the "lint" script
key and the verify-phase script filenames) and run the linter to confirm no
remaining issues.
In `@workers/agent-worker/src/graph/nodes/llm_social_turn.py`:
- Around line 63-64: Update the stale comment above SOCIAL_LLM_MAX_ATTEMPTS to
reflect the current retry budget (now 3 attempts) instead of "One attempt per
provider"; e.g., mention that there are three attempts per provider or a
3-attempt retry budget to match the constant name SOCIAL_LLM_MAX_ATTEMPTS and
avoid contradiction.
🪄 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: a94d95c0-9d62-4104-8858-4e32817d11c4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (76)
CONTRIBUTING.mdREADME.mdapps/ai-gateway/app/guards/content.pyapps/ai-gateway/app/main.pyapps/ai-gateway/app/services/llm.pyapps/ai-gateway/tests/test_openrouter_parse.pyapps/game-server/src/ambient/intent-cache.tsapps/game-server/src/collective/action-tracker.tsapps/game-server/src/collective/move-intent-tracker.tsapps/game-server/src/colyseus/GameRoom.tsapps/game-server/src/colyseus/bridge.test.tsapps/game-server/src/colyseus/bridge.tsapps/game-server/src/colyseus/job-registry.tsapps/game-server/src/queue/npc-turn.tsapps/game-server/src/room/store.tsapps/game-server/src/routes/audit.tsapps/game-server/src/routes/chat.tsapps/game-server/src/routes/collective-state.tsapps/game-server/src/routes/internal-memories.tsapps/game-server/src/routes/internal.tsapps/game-server/src/routes/npc-memory.tsapps/game-server/src/routes/rooms.tsapps/web/src/ChatPage.tsxapps/web/src/components/MessageList.tsxapps/web/src/components/NpcTabBar.tsxapps/web/src/components/PhaserGame.tsxapps/web/src/game/RoomScene.tsapps/web/src/game/roomSceneCamera.tsapps/web/src/game/roomSceneFloor.tsapps/web/src/game/roomSceneInput.tsapps/web/src/game/roomSceneNpcMotion.tsapps/web/src/game/roomSceneSync.tsapps/web/src/game/roomSceneTypes.tsapps/web/src/hooks/npcChat/attitudeGate.tsapps/web/src/hooks/npcChat/index.tsapps/web/src/hooks/npcChat/jobRegistry.tsapps/web/src/hooks/npcChat/speakQueue.tsapps/web/src/hooks/npcChat/types.tsapps/web/src/hooks/useColyseusRoom.tsapps/web/src/hooks/useNpcChat.test.tsapps/web/src/hooks/useNpcChat.tsdocs/CODE-QUALITY-REVIEW.mddocs/ISSUE-LOG.mdeslint.config.jspackage.jsonpackages/game-actions/README.mdpackages/game-actions/src/index.tspackages/npc-memory/src/repository.tspackages/npc-memory/src/schema.tspackages/shared/src/colyseus.tspackages/shared/src/index.tspackages/shared/src/player.tsscripts/agent-verify.mjsscripts/lib/e2e-sse.mjsscripts/lib/env.mjsscripts/lib/home-spawn.mjsscripts/lib/shared-pack.mjsscripts/uat-phase7-reset-snap.mjsscripts/verify-phase10.mjsscripts/verify-phase11.mjsscripts/verify-phase12.mjsscripts/verify-phase13.mjsscripts/verify-phase14.mjsscripts/verify-phase15.mjsscripts/verify-phase16.mjsscripts/verify-phase2.mjsscripts/verify-phase3.mjsscripts/verify-phase4.mjsscripts/verify-phase5.mjsscripts/verify-phase6.mjsscripts/verify-phase7.mjsscripts/verify-phase8.mjsworkers/agent-worker/src/graph/ambient_intent.pyworkers/agent-worker/src/graph/nodes/llm_social_turn.pyworkers/agent-worker/src/graph/npc_loop.pyworkers/agent-worker/src/main.py
| roomId: text("room_id").notNull(), | ||
| playerId: text("player_id").notNull().default("__legacy__"), | ||
| npcId: text("npc_id").notNull().default("1"), | ||
| npcId: text("npc_id").notNull().default(DEFAULT_NPC_ID), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a migration script addressing the npc_id value change
fd -t f -e sql -e ts -e js . --exec grep -l -i "npc.id.*=.*1\|UPDATE.*npc_memories\|UPDATE.*memory_summaries" {} \;
# Search for any migration or seed files referencing the old "1" default
rg -n "npc_id.*['\"]1['\"]" --type sql --type tsRepository: moyunzero/AetherLife
Length of output: 349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema.ts (npcId defaults) =="
sed -n '1,140p' packages/npc-memory/src/schema.ts | nl -ba | sed -n '1,140p' | sed -n '1,120p'
echo "----"
sed -n '30,90p' packages/npc-memory/src/schema.ts | nl -ba
echo "== migrations files =="
ls -la packages/npc-memory/migrations || true
echo "----"
for f in packages/npc-memory/migrations/*; do
echo "## $f"
sed -n '1,120p' "$f" | nl -ba
echo
done
echo "== search for old/new defaults =="
rg -n "npc_id text NOT NULL DEFAULT '1'" packages/npc-memory/migrations || true
rg -n "DEFAULT_NPC_ID|npc-1|npc_id.*DEFAULT" packages/npc-memory/migrations packages/npc-memory/src || true
rg -n "npc_id.*['\"]1['\"]" packages/npc-memory/migrations packages/npc-memory/src || trueRepository: moyunzero/AetherLife
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema.ts (npcId defaults) =="
# show the relevant table/columns area
sed -n '1,220p' packages/npc-memory/src/schema.ts | cat -n | sed -n '1,220p'
echo "== migrations files =="
ls -la packages/npc-memory/migrations || true
echo "----"
for f in packages/npc-memory/migrations/*; do
[ -f "$f" ] || continue
echo "## $f"
sed -n '1,140p' "$f" | cat -n | sed -n '1,140p'
echo
done
echo "== search for old/new defaults =="
rg -n "npc_id text NOT NULL DEFAULT '1'" packages/npc-memory/migrations || true
rg -n "DEFAULT_NPC_ID|npc-1|npc_id.*DEFAULT" packages/npc-memory/migrations packages/npc-memory/src || true
rg -n "npc_id.*['\"]1['\"]" packages/npc-memory/migrations packages/npc-memory/src || trueRepository: moyunzero/AetherLife
Length of output: 14179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DEFAULT_NPC_ID definition =="
rg -n "DEFAULT_NPC_ID" packages/shared/src -S
sed -n '1,120p' packages/shared/src/player.ts | cat -n
echo
echo "== Any migration updating npc_id default? =="
rg -n "ALTER TABLE .*npc_(memories|attitudes|summaries|memory)" packages/npc-memory/migrations -S || true
rg -n "SET DEFAULT|DROP DEFAULT|DEFAULT 'npc-1'|DEFAULT \"npc-1\"|DEFAULT '1'|DEFAULT \"1\"" packages/npc-memory/migrations -S || true
echo
echo "== Any existing schema/migration mismatch for npc_id default =="
rg -n "npc_id.*default\\(" packages/npc-memory/src -SRepository: moyunzero/AetherLife
Length of output: 2394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' packages/npc-memory/src/repository.ts | cat -n
echo "----"
rg -n "insert\\(|npcId\\s*\\:" packages/npc-memory/src/repository.ts -nRepository: moyunzero/AetherLife
Length of output: 9308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== insert usages in npc-memory =="
rg -n "insert\\(npcMemories|insert\\(memorySummaries|insert\\(collectiveEvents|insert\\(npc_attitudes" packages/npc-memory/src -S
echo
echo "== Any writes relying on DB default for npc_id? =="
rg -n "insert\\(.*\\)\\s*\\.values\\(\\s*\\{[^}]*npcId" packages/npc-memory/src -S || true
rg -n "insert\\(.*\\)\\s*\\.values\\(\\s*\\{(?![^}]*npcId)" packages/npc-memory/src -S || true
echo
echo "== Where LEGACY_DEFAULT_NPC_ID is used =="
rg -n "LEGACY_DEFAULT_NPC_ID" -S packages
echo
echo "== Any raw SQL using npc_id = '1' =="
rg -n "npc_id\\s*=?\\s*'1'|DEFAULT_NPC_ID|npc-1" packages/npc-memory/src -SRepository: moyunzero/AetherLife
Length of output: 4087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LEGACY_DEFAULT_NPC_ID usage =="
rg -n "LEGACY_DEFAULT_NPC_ID" -S packages | head -n 50 || true
echo
echo "== DEFAULT_NPC_ID usage in npc-memory =="
rg -n "DEFAULT_NPC_ID" -S packages/npc-memory/src
echo
echo "== collective/repository.ts (relevant parts) =="
sed -n '1,260p' packages/npc-memory/src/collective/repository.ts | cat -n
echo
echo "== search inserts into npc_attitudes / npc_memories / memory_summaries =="
rg -n "insert\\(npcAttitudes|insert\\(npcMemories|insert\\(memorySummaries" -S packages/npc-memory/src
echo
echo "== raw SQL involving npc_id = '1' or npc_id = \"npc-1\" in npc-memory =="
rg -n "npc_id\\s*=\\s*['\"](1|npc-1)['\"]" -S packages/npc-memory/src || trueRepository: moyunzero/AetherLife
Length of output: 10679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find appendMemory call sites =="
rg -n "appendMemory\\(" -S packages apps | head -n 50
echo
echo "== Find appendSummary call sites =="
rg -n "appendSummary\\(" -S packages apps | head -n 50
echo
echo "== Find where npcId is passed as '1' in code =="
rg -n "npcId\\s*:\\s*['\"]1['\"]" -S packages apps || true
echo
echo "== Find where npcId param is omitted when calling MemoryRepository methods =="
rg -n "appendMemory\\(\\{[^}]*roomId:[^}]*playerId:[^}]*text:" -S packages apps || trueRepository: moyunzero/AetherLife
Length of output: 1252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' apps/game-server/src/memory/service.ts | cat -n
echo "----"
sed -n '220,520p' apps/game-server/src/memory/service.ts | cat -nRepository: moyunzero/AetherLife
Length of output: 18447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DEFAULT_NPC_ID / LEGACY_DEFAULT_NPC_ID usage in apps/game-server =="
rg -n "DEFAULT_NPC_ID|LEGACY_DEFAULT_NPC_ID" apps/game-server -S || true
echo
echo "== Hardcoded npcId values \"npc-1\" or \"1\" in apps/game-server =="
rg -n "npcId\\s*[:=]\\s*['\"](npc-1|1)['\"]|['\"](npc-1|1)['\"]" apps/game-server/src/memory apps/game-server/src/collective apps/game-server/src -S || true
echo
echo "== Where npcId is passed into MemoryService calls (collective context) =="
rg -n "buildMemoryContext\\(|storeReflection\\(|storeBulkSummary\\(|getMemoryCount\\(|appendPlayerMemory\\(|appendNpcMemory\\(" apps/game-server/src -S | head -n 100Repository: moyunzero/AetherLife
Length of output: 18427
Major: npc_id default mismatch ('1' vs DEFAULT_NPC_ID="npc-1") can strand legacy rows
In packages/npc-memory/src/schema.ts, npcMemories.npcId / memorySummaries.npcId now default to DEFAULT_NPC_ID ("npc-1"). However, packages/npc-memory/migrations/0001_npc_memory.sql still creates the DB columns with npc_id ... DEFAULT '1', and there’s no later migration that updates the column default or migrates existing rows from '1' → 'npc-1'.
MemoryRepository/MemoryService use the provided npcId (typically "npc-1") when reading, with no LEGACY_DEFAULT_NPC_ID fallback, so rows written under the legacy default (npc_id='1') won’t be returned unless callers explicitly query with npcId='1'.
Add a migration to align DB defaults and/or update existing legacy rows, or document that '1' legacy data is intentionally orphaned.
🤖 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/npc-memory/src/schema.ts` at line 36, The DB default for npc_id is
inconsistent: schema.ts sets DEFAULT_NPC_ID ("npc-1") for npcMemories.npcId and
memorySummaries.npcId while migrations/0001_npc_memory.sql uses DEFAULT '1',
leaving legacy rows orphaned; fix by adding a new migration that ALTERs both
columns to DEFAULT 'npc-1' and runs an UPDATE to convert existing rows WHERE
npc_id = '1' → 'npc-1', and/or add a short-term fallback in
MemoryRepository/MemoryService to treat '1' as LEGACY_DEFAULT_NPC_ID when
reading until the migration is applied. Ensure you reference DEFAULT_NPC_ID,
npcMemories.npcId, memorySummaries.npcId, migrations/0001_npc_memory.sql, and
MemoryRepository/MemoryService in your changes.
Register HTTP speak jobs before enqueue, load .env before scope/URL reads, harden env helpers (quoted values, WS derivation), add SSE read timeouts, fall back to heuristic NL parse on malformed OpenRouter payloads, and defer tap-to-move until pointerup to avoid pinch-zoom conflicts. Co-authored-by: Cursor <cursoragent@cursor.com>
Return 503 for moderation outages instead of content_blocked, evict only stale jobs from the speak registry, rollback BullMQ when the Redis bridge fails, and migrate legacy npc_id defaults to npc-1. Co-authored-by: Cursor <cursoragent@cursor.com>
Route physical-reply fast lane by npc_id, extend move-intent patterns, and align client movement prediction with grid sync so speak stubs no longer collapse to identical help text. Co-authored-by: Cursor <cursoragent@cursor.com>
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
syncRoomEntitiesand related logic intoroomSceneSync.ts; split camera, floor, input, and NPC motion into dedicated modules.RoomScene.tsshrinks to lifecycle + entity factory (~1086 lines).apps/web/src/hooks/npcChat/; preserve frozen speak-busy UX contract.scripts/lib/*; fix@aetherlife/sharedimport in phase verify scripts viashared-pack.mjs.eslint.config.js,docs/CODE-QUALITY-REVIEW.md, and ISSUE-LOG guardrails.Test plan
pnpm agent:verify(pre-push hook on push)pnpm --filter @aetherlife/web test— 82 passed (incl.RoomScene.activity)pnpm --filter @aetherlife/game-server test— 182 passedcd workers/agent-worker && LLM_MOCK=1 uv run pytest -q— 201 passedpnpm agent:verify --e2e --baseon freshpnpm dev:stack— all mapped golden flows passed (~11 min, real LLM on phase12/8)Notes
assets/pet-cat/andapps/web/tsconfig.tsbuildinfointentionally excluded from this PR.dev:stackbefore re-runninguat:phase7:reset-snapif flaky.Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests