feat(26): Council map presence + 全员 LPC character visuals#17
Merged
Conversation
MapSchema migration dropped mainNpcGridById/bgNpcGridById state but leave cleanup still called the removed setters, crashing React on room unmount. Replace with setRoomNpcs([]). Add verify:phase26 screenshots + uat script. Co-authored-by: Cursor <cursoragent@cursor.com>
…erify gate Place all 12 council members on the authoritative map via shuffleCouncilSpawnAssignments and breaking MapSchema v2. Web renders the full roster in Phaser; speak/ambient cover all seats; REL-08 leaningDrift in worker. Ship gate verify:phase26 plus E2E regression fixes for phase7 reset-snap and phase16 after background NPC removal. Co-authored-by: Cursor <cursoragent@cursor.com>
Record HTTP speak initiator in GameRoom, align phase16/26 scripts with COUNCIL_NPC_IDS, fix ambient leash and bucket tests, and stop truncating dialogue overlay text while salvaging partial social JSON from the worker. Co-authored-by: Cursor <cursoragent@cursor.com>
Detect non-canonical NPCs in MAP-05 snapshot checks, fail verify:phase26 when leaning_drift pytest fails, and clean up ship-gate script lint/CSS. Co-authored-by: Cursor <cursoragent@cursor.com>
Unify all players and npc-1 on lpc-npc-1.png; rescale grid to 32px with 64px character height. Add sync script, nameplate layout refresh, and document the product decision (npc-asset/ stays local-only). 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. |
📝 WalkthroughWalkthroughThis PR moves player and ChangesCouncil spawn and LPC visual refresh
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant createDefaultRoom
participant getCouncilSpawnSlots
participant shuffleCouncilSpawnAssignments
createDefaultRoom->>getCouncilSpawnSlots: regionId
getCouncilSpawnSlots-->>createDefaultRoom: council spawn slots
createDefaultRoom->>shuffleCouncilSpawnAssignments: roomId, slots
shuffleCouncilSpawnAssignments-->>createDefaultRoom: npcId to slot assignments
sequenceDiagram
participant RoomScene
participant entitySprites
participant lpcNpc1Sheet
RoomScene->>entitySprites: registerLpcNpc1Anims(scene)
entitySprites->>lpcNpc1Sheet: lpcNpc1FrameIndex / lpcNpc1AnimKey
RoomScene->>entitySprites: createPlayerSprite / createNpcSprite
entitySprites-->>RoomScene: spriteProfile-aware sprite
RoomScene->>entitySprites: spriteNameplateY(spriteProfile)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
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)
packages/shared/src/worldRegion.ts (1)
239-262: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject Beginning Fields bundles that omit
spawnsByRegionIdentirely.Line 239 only validates council spawns after
spawnsRawexists. A bundle with nospawnsByRegionId[BEGINNING_FIELDS_ID]still passesloadWorldRegistry(), then fails later when room creation asks for council slots. Fail fast here before the guard.Suggested fix
const spawnsRaw = bundle.spawnsByRegionId?.[region.id]; + if (region.id === BEGINNING_FIELDS_ID && !spawnsRaw) { + throw new Error( + `world registry: ${region.id} requires exactly 12 councilSpawns`, + ); + } if (spawnsRaw) { const spawnsParsed = SpawnsFileSchema.parse(spawnsRaw);🤖 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/worldRegion.ts` around lines 239 - 262, The `loadWorldRegistry` flow in `worldRegion.ts` only validates council spawns when `spawnsByRegionId[region.id]` exists, so Beginning Fields bundles can omit that entry and still pass. Update the `spawnsRaw`/`SpawnsFileSchema.parse` path to explicitly require `BEGINNING_FIELDS_ID` in `bundle.spawnsByRegionId`, and throw the same registry error before any later guards if it is missing. Keep the existing `validateLocalCellInRegion` and `councilSpawns` length checks in `loadWorldRegistry` unchanged for the present entry case.
🟡 Minor comments (9)
apps/game-server/src/world/region-walkability.test.ts-46-115 (1)
46-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert parity with the baked spawn JSON
These checks only readdefaultBeginningFieldsBundle(), so drift inapps/game-server/data/world/beginning-fields@v1/spawns.jsonwould still pass. Add an explicit assertion against the baked file, or load it here, to keep the “sync spawns.json after bake:one-city” contract covered.🤖 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/world/region-walkability.test.ts` around lines 46 - 115, The walkability tests in region-walkability.test.ts only validate the runtime bundle from defaultBeginningFieldsBundle(), so they can miss drift from the baked spawns JSON. Update the existing council spawn tests around loadWorldRegistry, BEGINNING_FIELDS_ID, and the councilSpawns/defaultPlayerSpawn assertions to also read or assert against the baked beginning-fields@v1/spawns.json content. Keep the same parity checks, but make one test explicitly verify the loaded baked spawn data matches the bundle so the “sync spawns.json after bake:one-city” contract is covered.apps/web/src/components/DialogueBar.tsx-163-169 (1)
163-169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep a status message for active-NPC busy state.
The composer is still disabled for any
composerBusyForActiveNpc, but this block now only explains the “other player” case. Restore a status for the active NPC thinking/sending path so the disabled composer has visible/live feedback.Proposed fix
- {composerSpeakBusyOtherPlayer ? ( + {composerBusyForActiveNpc ? ( <p className="composer__speak-status" data-testid="composer-speak-status" role="status" > - 该 NPC 正在响应其他玩家的指令，请稍候再试。 + {composerSpeakBusyOtherPlayer + ? "该 NPC 正在响应其他玩家的指令，请稍候再试。" + : `${activeNpcName}正在思考，请稍候…`} </p> ) : null}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/DialogueBar.tsx` around lines 163 - 169, The busy-status rendering in DialogueBar should cover both composerBusyForActiveNpc and composerSpeakBusyOtherPlayer, but the current message only explains the “other player” case. Update the status block around the composer status text in DialogueBar to restore a visible/live message for the active NPC thinking/sending path, using the existing busy-state flags so the disabled composer always has feedback. Keep the current other-player message and add a separate active-NPC status message in the same conditional flow.apps/web/src/index.css-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMatch the repo’s
@importnotation rule.This line currently trips stylelint’s
import-notationrule, so it will keep failing lint until theurl(...)wrapper is removed.🤖 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/index.css` at line 1, The stylesheet import in the top-level CSS entry is using the wrong import notation and is failing stylelint’s import-notation rule. Update the existing Google Fonts import in index.css to use the repo’s preferred bare `@import` form without the url(...) wrapper, keeping the same font URL and leaving the rest of the stylesheet unchanged.Source: Linters/SAST tools
apps/web/src/game/roomSceneInput.ts-61-177 (1)
61-177: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTear down the grid-debug scene hooks on shutdown.
setupGridDebugPickeradds ascale.on("resize", layoutHud)listener and window-scoped debug helpers, butRoomSceneonly unregisters its own resize handler on shutdown. In?gridDebug=1, recreating the scene will stack stale resize callbacks and keep globals pointing at destroyed Phaser objects.Suggested fix
layoutHud(); ctx.scene.scale.on("resize", layoutHud); @@ ctx.input.on("pointerup", (pointer: Phaser.Input.Pointer) => { if (!pointer.event.shiftKey) return; const world = worldFromPointer(pointer); const { x, y } = worldToGrid(world.x, world.y); const picks = w.__aetherlife_gridPicks!; picks.push({ x, y }); drawPickMarkers(); const info = w.__aetherlife_spawnCellInfo!(x, y); console.log(`[gridDebug] pick #${picks.length}: (${x}, ${y})`, info, picks); hud.setText(gridDebugHudText(x, y, picks.length)); }); + + ctx.scene.events.once("shutdown", () => { + ctx.scene.scale.off("resize", layoutHud); + delete w.__aetherlife_clearGridPicks; + delete w.__aetherlife_spawnCellInfo; + overlayGfx.destroy(); + hoverGfx.destroy(); + markerGfx.destroy(); + hud.destroy(); + }); drawGridOverlay(); drawPickMarkers(); updateHover(ctx.input.activePointer); }🤖 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/roomSceneInput.ts` around lines 61 - 177, The grid-debug setup in setupGridDebugPicker leaves behind scene-specific listeners and globals after RoomScene shuts down, which can stack stale callbacks on recreation. Add a shutdown/cleanup path inside setupGridDebugPicker that unregisters the ctx.scene.scale "resize" handler, removes the ctx.input pointer listeners, destroys the hud/graphics objects, and clears the window helpers (__aetherlife_clearGridPicks, __aetherlife_spawnCellInfo, __aetherlife_gridPicks) so they don’t reference destroyed Phaser objects. Ensure the cleanup is wired to the scene shutdown/destroy lifecycle alongside the existing resize logic.AGENTS.md-127-128 (1)
127-128: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the guardrail reference below this table.
This section now introduces Guardrail
#106forentitySprites.ts, but the note immediately under the table still points readers only to Guardrails#57–#59, so the new contract is not traceable from here.🤖 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 `@AGENTS.md` around lines 127 - 128, Update the note beneath the guardrail table so it references the new Guardrail `#106` introduced for entitySprites.ts, not just Guardrails `#57`–#59. Edit the AGENTS.md guidance near the table and make sure the referenced symbols and related entry for apps/web/src/game/entitySprites.ts are reflected consistently so readers can trace the current contract from this section.apps/web/src/game/bgNpcLabels.ts-6-7 (1)
6-7: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t let background nameplates outgrow the main nameplates at 32px grids.
Line 6 clamps
BG_NAMEPLATE_FONT_SIZE_PXto 12px, butnameplateFontPx()now resolves to 10px atCELL_PX = 32(andsceneLabelLayout.test.tscodifies that compact size). That makes the background tier larger than the foreground nameplates in the small-cell layout and is likely to reintroduce overlap. Please cap the floor at the main-nameplate size instead of hard-coding12here.🤖 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/bgNpcLabels.ts` around lines 6 - 7, The background nameplate size in BG_NAMEPLATE_FONT_SIZE_PX is hard-coded with a 12px minimum, which can exceed the main nameplate size in compact layouts. Update the clamp in bgNpcLabels.ts to use the foreground nameplate size as the upper floor instead of a fixed 12, so BG_NAMEPLATE_FONT_SIZE_PX always stays at or below nameplateFontPx() for 32px cells and preserves the intended ordering.workers/agent-worker/tests/test_world_vote.py-554-559 (1)
554-559: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the room id is actually forwarded.
The fake
get_leaning_drift()ignoresroom_id, so this still passes if_leaning_default_vote()regresses to calling it withNone. Since the behavior change here is specifically room-aware, make the stub validate"room-drift"(or use a mock andassert_called_with) so the test pins that contract.🤖 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/tests/test_world_vote.py` around lines 554 - 559, The _leaning_default_vote test is not verifying that room_id is forwarded into get_leaning_drift, so a regression passing None would still pass. Update the test around wv._leaning_default_vote and src.graph.world_vote.get_leaning_drift to assert the call includes "room-drift" (or make the stub fail unless that room_id is received), so the room-aware contract is pinned.docs/ISSUE-LOG.md-2603-2605 (1)
2603-2605: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winISSUE-100 points to the wrong guardrail.
Line 2605 cites Guardrail
#106as the reset-home protection, but#106in this new guardrail block is the LPC skin rule. Please either add the missing reset-home guardrail entry or correct this reference so future readers land on the right contract.🤖 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 `@docs/ISSUE-LOG.md` around lines 2603 - 2605, The guardrail reference is pointing to the wrong contract: the reset-home protection should not be labeled as Guardrail `#106` because that symbol now maps to the LPC skin rule. Update the ISSUE-100 note in this guardrail block to either add the missing reset-home guardrail entry with its own unique number or change the reference to the existing reset-home guardrail symbol so readers can navigate to the correct rule.docs/DEVELOPMENT-HISTORY.zh-CN.md-832-832 (1)
832-832: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPhase 26 still reads as "planned" in the same block.
Line 832 now marks the phase complete, but the neighboring deliverables and acceptance rows still say
planned/规划. Please update those labels too so the history page doesn't describe the same phase as both shipped and planned.🤖 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 `@docs/DEVELOPMENT-HISTORY.zh-CN.md` at line 832, The Phase 26 block is inconsistent because the status now says completed while the related deliverables and acceptance rows still read planned/规划. Update the remaining Phase 26 labels in the same history section to match the completed state, using the surrounding Phase 26 table entries in DEVELOPMENT-HISTORY.zh-CN.md as the target for the edit.
🧹 Nitpick comments (6)
apps/web/src/lib/colyseusAmbientSnapshot.ts (1)
94-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConstrain
roomNpcsto council IDs before exposing it.
iterateNpcMapcurrently pushes every MapSchema key, andindexOf()sorts unknown ids before known council ids. A stale legacy key could be selected by proximity UI even though this PR’s dialogue path is council-only.Proposed guard
+ const councilOrder = new Map<string, number>( + COUNCIL_NPC_IDS.map((npcId, index) => [npcId, index]), + ); + iterateNpcMap(state.npcs, (npcId, entry) => { + if (!councilOrder.has(npcId)) return; const snap = snapshotNpcFromEntry(npcId, entry); roomNpcs.push(snap); @@ roomNpcs.sort( (a, b) => - COUNCIL_NPC_IDS.indexOf(a.id as (typeof COUNCIL_NPC_IDS)[number]) - - COUNCIL_NPC_IDS.indexOf(b.id as (typeof COUNCIL_NPC_IDS)[number]), + (councilOrder.get(a.id) ?? Number.MAX_SAFE_INTEGER) + - (councilOrder.get(b.id) ?? Number.MAX_SAFE_INTEGER), );🤖 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 94 - 110, Filter `roomNpcs` in `colyseusAmbientSnapshot` so only council NPCs are exposed before any downstream selection or sorting. In the `iterateNpcMap(state.npcs, ...)` flow, guard on `COUNCIL_NPC_IDS` and skip unknown/stale IDs when building `roomNpcs`, `npcAmbientById`, and `npcActivityById`. Keep the existing `roomNpcs.sort(...)` logic for council ordering, but ensure non-council entries never reach proximity UI or council-only dialogue paths.apps/web/src/lib/colyseusAmbientSnapshot.test.ts (1)
5-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShuffle the fixture so this actually tests council ordering.
snapshotAmbientStateFromSchema()sortsroomNpcsby council id, but this fixture already emits the NPCs in that same order. If the sort regressed, this test would still pass. Emit at least one NPC out of order and assert the normalized id sequence explicitly.🤖 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.test.ts` around lines 5 - 57, The fixture in snapshotAmbientStateFromSchema’s test currently matches the same council-id order that the sorter produces, so it does not verify normalization. Update the test data emitted through the npcs.forEach path to include at least one NPC out of order, then assert the exact ordered id sequence in snap.roomNpcs (using the snapshotAmbientStateFromSchema output) so the test fails if the council sorting regresses.packages/npc-memory/src/schema.ts (1)
135-145: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the Drizzle
driftcolumn type aligned with the migration.Line 140 models
driftasinteger, butpackages/npc-memory/migrations/0010_npc_leaning_drift.sql:1-8createsnpc_leaning_drift.driftasSMALLINT. That mismatch is easy to miss now, but it will make future schema diffs drift from the actual table definition and obscures the persisted range contract for leaning drift. Please switch this field to the matching Drizzle column builder as well.🤖 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` around lines 135 - 145, The npcLeaningDrift schema definition has a type mismatch for the drift field: it is currently declared with the integer column builder while the migration defines it as SMALLINT. Update the drift property in npcLeaningDrift to use the matching Drizzle smallint builder so the schema stays aligned with the existing npc_leaning_drift table definition and future diffs remain consistent.docs/BEGINNING-FIELDS.md (1)
3-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the grid scale change.
The text states "16×2 整数缩放" resulting in 32px/cell, but doesn't explain the prior 48px value mentioned in the PR summary. Consider adding a brief note that this replaces an earlier 48px experiment or miscalculation, or confirm the 48px reference was from a different context.
🤖 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 `@docs/BEGINNING-FIELDS.md` around lines 3 - 21, Clarify the grid scale wording in BEGINNING-FIELDS by explaining the 32px/格 decision relative to the earlier 48px reference so readers can see whether it was a reverted experiment or a different context. Update the relevant “角色视觉” row and opening paragraph near HomeMapBackground/RoomScene to make the scale transition explicit, using the existing symbols CELL_PX, CHAR_DISPLAY_PX, and gridLayout.ts as anchors.scripts/uat-phase7-reset-snap.mjs (2)
21-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider dynamic
FARcoordinate validation.The static
FAR = { x: 15, y: 10 }may be too close to some council spawn assignments depending onroomIdshuffle. Consider verifyingchebyshevDistance(FAR, DEFAULT) > THRESHOLDafter resolvingDEFAULT, or dynamically computing a far point, to ensure the reset-snap test actually tests meaningful distance.🤖 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/uat-phase7-reset-snap.mjs` around lines 21 - 27, The hardcoded FAR coordinate in uat-phase7-reset-snap.mjs may not remain sufficiently distant after roomId shuffles, so the reset-snap test can lose its intended coverage. Update the logic around FAR, resolveDefaultHome(), and the default home calculation so the chosen point is validated against the resolved DEFAULT using chebyshevDistance and a threshold, or compute FAR dynamically until it satisfies the distance requirement. Keep the existing constants and test flow, but ensure the selected reset point is always meaningfully far from the council home.
4-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueESLint false positives —
processandconsoleare Node.js globals.The
no-undeferrors forprocess(lines 17–19) andconsole(line 117) are false positives in a Node.js script context. Consider adding/* eslint-env node */at the top or configuring ESLint to recognize.mjsfiles as Node environment.🤖 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/uat-phase7-reset-snap.mjs` around lines 4 - 28, The no-undef findings for Node globals in this script are false positives; update the ESLint context so this `.mjs` file is treated as Node. Add a Node environment hint at the top of the script or adjust the ESLint config for `.mjs` scripts, so references to `process` and `console` in the `loadRootEnv`/`resolveDefaultHome` module are recognized without changing the runtime code.Source: Linters/SAST tools
🤖 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/web/src/game/lpcNpc1Sheet.ts`:
- Around line 63-70: The LPC NPC frame lookup is still using the generic
facing-to-row mapping instead of the LPC sheet’s declared row order. Update
lpcNpc1FrameIndex to derive the row from the LPC row map used by the sheet
definition (up/left/down/right) rather than facingToIndex(), and keep the
walk/idle offset logic intact. Also adjust lpcNpc1Sheet.test.ts so it verifies
the explicit LPC row ordering, not the generic facing order, by asserting the
mapping used by lpcNpc1FrameIndex and the sheet constants.
In `@apps/web/src/game/roomSceneSync.ts`:
- Around line 311-333: The new speaking halo tween on ent.ring must be cleaned
up in the shared teardown path. Update RoomScene’s stopEntityMotion() to also
stop and clear ent.speakHaloTween, and reset the ring state before the entity is
destroyed or removed. Make sure the cleanup covers the same reset/removal flows
that already call stopEntityMotion(), so no live tween remains attached to a
destroyed NPC ring.
In `@apps/web/src/index.css`:
- Around line 211-214: The transcript scroll area is still blocked by the parent
overlay’s pointer-events setting, so the new `.dialogue-overlay__last-line`
styles alone won’t make it usable. Update the dialogue overlay/scroll container
setup in `index.css` so the actual scrollable element (the one with
`max-height`, `overflow-y: auto`, and `white-space: pre-wrap`) can receive
pointer events, or remove the height cap if that better matches the layout. Use
the `.dialogue-overlay` and `.dialogue-overlay__last-line` selectors to locate
and adjust the affected styles.
In `@packages/shared/src/council/spawn.ts`:
- Around line 33-36: The fallback in ensureRegistry currently seeds global state
with defaultBeginningFieldsBundle(), which leaves getWorldRegistry() incomplete
on cold starts. Update ensureRegistry to load and register the full default
world bundle instead of the partial beginning-fields bundle, so paths like
getCouncilSpawnSlots() always see VILLAGE_PLAZA_ID and the other required
regions.
In `@packages/shared/src/room.ts`:
- Around line 65-69: The legacy starter inventory in LEGACY_STARTER_INVENTORY is
being reused by reference when building room state, so each room can end up
sharing the same inventory arrays for npc-1/npc-2/npc-3. Update the room
initialization logic in room.ts to clone the starter inventory arrays before
assigning them to a room’s inventory, so every room gets its own copy and
in-place mutations stay isolated. Use the LEGACY_STARTER_INVENTORY symbol and
the room setup code around the inventory assignment to locate the fix.
- Around line 71-76: `ensureCouncilSpawnsReady()` is overwriting the global
world registry on any `getCouncilSpawnSlots()` failure, which can discard an
already-initialized registry. Update the logic so it only bootstraps with
`loadWorldRegistry(defaultBeginningFieldsBundle())` when the registry is
actually unset, and do not catch-and-replace for lookup/data errors. Keep the
fallback local to the initialization path and let malformed registry state
surface instead of silently resetting `setWorldRegistry()`.
In `@scripts/lib/dialogue-engage.mjs`:
- Around line 71-100: The engagement flow in dialogue-engage.mjs is treating any
visible dialogue bar as success, even when it belongs to a different NPC. Update
the checks in the main loop and the early-return path in the engage logic so
success is gated on the active chip for npcId (for example, the activeChip /
chip locator or another NPC-specific indicator), not just dialogueBar
visibility. Make the same NPC-specific verification before returning in the
while loop and after clicking the corner menu trigger so sendSpeakOverlay(..., {
skipEngage: true }) only proceeds when the correct council member is engaged.
In `@scripts/migrate-room-council-npcs.mjs`:
- Around line 1-8: This new migration script is being linted as if it were
browser code, so Node globals like console and process are flagged as undefined.
Update the script entrypoint to explicitly indicate Node usage or import/declare
the Node globals, and ensure the references used in the script’s main flow
(migrateRoomCouncilNpcs and the process/console calls) are recognized by ESLint
so CI passes.
In `@scripts/sync-npc-lpc-assets.mjs`:
- Around line 19-21: The baked atlas layout constants in the NPC/LPC asset sync
script no longer match the web runtime contract, so the client reads the wrong
columns for idle frames. Update the frame layout used by the baker and keep it
consistent with the corresponding constants in assetManifest (WALK_FRAMES,
IDLE_FRAMES, and FRAMES_PER_FACING), so both sides agree on the same per-facing
column arrangement. Make sure the symbols in the sync script and the web
manifest are aligned after the change.
In `@scripts/verify-phase16.mjs`:
- Around line 5-13: The verify script is introducing no-undef lint failures
because it uses runtime globals like fetch, window, and console without the
appropriate environment coverage. Update scripts/verify-phase16.mjs so its
globals are recognized by the correct Node/browser lint scope, or extend the
lint override for *.mjs scripts to include those globals; make sure the fix is
applied consistently around the existing helpers like assertE2eNoMock,
gameServerHttpBase, and loadRootEnv.
In `@scripts/verify-phase26.mjs`:
- Around line 13-31: The new verify-phase26.mjs script is triggering no-undef
because it relies on globals like process, fetch, AbortSignal, URLSearchParams,
window, document, localStorage, and console without declaring them. Update the
script’s ESLint scope or file-level overrides to recognize the intended runtime
globals, or refactor the script so the relevant helpers and entrypoint in
verify-phase26.mjs do not reference undeclared globals. Keep the fix localized
to this script and its lint configuration so the gate passes cleanly.
- Around line 301-345: The early return in engageCouncilNpc() is too coarse: it
treats any visible dialogue bar as success even when the currently active NPC is
still the previous one. Update engageCouncilNpc() and/or speakToCouncilNpc() so
they verify the targeted npcId is actually selected before proceeding, using the
active NPC state exposed through the dialogue flow in ChatPage. If the dialogue
bar is already open, re-click the target avatar or otherwise confirm activeNpcId
matches npcId before calling sendSpeakOverlay(), so multi-NPC checks cannot
reuse the previous speaker.
In `@workers/agent-worker/src/council/leaning_drift.py`:
- Around line 72-73: The in-memory toggle in _use_in_memory only checks the
LLM_MOCK environment variable, so it ignores Settings.llm_mock and can
incorrectly call collective_connection() during mock runs. Update _use_in_memory
to honor the existing Settings.llm_mock setting as well, keeping its behavior
aligned with the rest of the worker and the tests in test_leaning_drift.py.
---
Outside diff comments:
In `@packages/shared/src/worldRegion.ts`:
- Around line 239-262: The `loadWorldRegistry` flow in `worldRegion.ts` only
validates council spawns when `spawnsByRegionId[region.id]` exists, so Beginning
Fields bundles can omit that entry and still pass. Update the
`spawnsRaw`/`SpawnsFileSchema.parse` path to explicitly require
`BEGINNING_FIELDS_ID` in `bundle.spawnsByRegionId`, and throw the same registry
error before any later guards if it is missing. Keep the existing
`validateLocalCellInRegion` and `councilSpawns` length checks in
`loadWorldRegistry` unchanged for the present entry case.
---
Minor comments:
In `@AGENTS.md`:
- Around line 127-128: Update the note beneath the guardrail table so it
references the new Guardrail `#106` introduced for entitySprites.ts, not just
Guardrails `#57`–#59. Edit the AGENTS.md guidance near the table and make sure the
referenced symbols and related entry for apps/web/src/game/entitySprites.ts are
reflected consistently so readers can trace the current contract from this
section.
In `@apps/game-server/src/world/region-walkability.test.ts`:
- Around line 46-115: The walkability tests in region-walkability.test.ts only
validate the runtime bundle from defaultBeginningFieldsBundle(), so they can
miss drift from the baked spawns JSON. Update the existing council spawn tests
around loadWorldRegistry, BEGINNING_FIELDS_ID, and the
councilSpawns/defaultPlayerSpawn assertions to also read or assert against the
baked beginning-fields@v1/spawns.json content. Keep the same parity checks, but
make one test explicitly verify the loaded baked spawn data matches the bundle
so the “sync spawns.json after bake:one-city” contract is covered.
In `@apps/web/src/components/DialogueBar.tsx`:
- Around line 163-169: The busy-status rendering in DialogueBar should cover
both composerBusyForActiveNpc and composerSpeakBusyOtherPlayer, but the current
message only explains the “other player” case. Update the status block around
the composer status text in DialogueBar to restore a visible/live message for
the active NPC thinking/sending path, using the existing busy-state flags so the
disabled composer always has feedback. Keep the current other-player message and
add a separate active-NPC status message in the same conditional flow.
In `@apps/web/src/game/bgNpcLabels.ts`:
- Around line 6-7: The background nameplate size in BG_NAMEPLATE_FONT_SIZE_PX is
hard-coded with a 12px minimum, which can exceed the main nameplate size in
compact layouts. Update the clamp in bgNpcLabels.ts to use the foreground
nameplate size as the upper floor instead of a fixed 12, so
BG_NAMEPLATE_FONT_SIZE_PX always stays at or below nameplateFontPx() for 32px
cells and preserves the intended ordering.
In `@apps/web/src/game/roomSceneInput.ts`:
- Around line 61-177: The grid-debug setup in setupGridDebugPicker leaves behind
scene-specific listeners and globals after RoomScene shuts down, which can stack
stale callbacks on recreation. Add a shutdown/cleanup path inside
setupGridDebugPicker that unregisters the ctx.scene.scale "resize" handler,
removes the ctx.input pointer listeners, destroys the hud/graphics objects, and
clears the window helpers (__aetherlife_clearGridPicks,
__aetherlife_spawnCellInfo, __aetherlife_gridPicks) so they don’t reference
destroyed Phaser objects. Ensure the cleanup is wired to the scene
shutdown/destroy lifecycle alongside the existing resize logic.
In `@apps/web/src/index.css`:
- Line 1: The stylesheet import in the top-level CSS entry is using the wrong
import notation and is failing stylelint’s import-notation rule. Update the
existing Google Fonts import in index.css to use the repo’s preferred bare
`@import` form without the url(...) wrapper, keeping the same font URL and leaving
the rest of the stylesheet unchanged.
In `@docs/DEVELOPMENT-HISTORY.zh-CN.md`:
- Line 832: The Phase 26 block is inconsistent because the status now says
completed while the related deliverables and acceptance rows still read
planned/规划. Update the remaining Phase 26 labels in the same history section to
match the completed state, using the surrounding Phase 26 table entries in
DEVELOPMENT-HISTORY.zh-CN.md as the target for the edit.
In `@docs/ISSUE-LOG.md`:
- Around line 2603-2605: The guardrail reference is pointing to the wrong
contract: the reset-home protection should not be labeled as Guardrail `#106`
because that symbol now maps to the LPC skin rule. Update the ISSUE-100 note in
this guardrail block to either add the missing reset-home guardrail entry with
its own unique number or change the reference to the existing reset-home
guardrail symbol so readers can navigate to the correct rule.
In `@workers/agent-worker/tests/test_world_vote.py`:
- Around line 554-559: The _leaning_default_vote test is not verifying that
room_id is forwarded into get_leaning_drift, so a regression passing None would
still pass. Update the test around wv._leaning_default_vote and
src.graph.world_vote.get_leaning_drift to assert the call includes "room-drift"
(or make the stub fail unless that room_id is received), so the room-aware
contract is pinned.
---
Nitpick comments:
In `@apps/web/src/lib/colyseusAmbientSnapshot.test.ts`:
- Around line 5-57: The fixture in snapshotAmbientStateFromSchema’s test
currently matches the same council-id order that the sorter produces, so it does
not verify normalization. Update the test data emitted through the npcs.forEach
path to include at least one NPC out of order, then assert the exact ordered id
sequence in snap.roomNpcs (using the snapshotAmbientStateFromSchema output) so
the test fails if the council sorting regresses.
In `@apps/web/src/lib/colyseusAmbientSnapshot.ts`:
- Around line 94-110: Filter `roomNpcs` in `colyseusAmbientSnapshot` so only
council NPCs are exposed before any downstream selection or sorting. In the
`iterateNpcMap(state.npcs, ...)` flow, guard on `COUNCIL_NPC_IDS` and skip
unknown/stale IDs when building `roomNpcs`, `npcAmbientById`, and
`npcActivityById`. Keep the existing `roomNpcs.sort(...)` logic for council
ordering, but ensure non-council entries never reach proximity UI or
council-only dialogue paths.
In `@docs/BEGINNING-FIELDS.md`:
- Around line 3-21: Clarify the grid scale wording in BEGINNING-FIELDS by
explaining the 32px/格 decision relative to the earlier 48px reference so readers
can see whether it was a reverted experiment or a different context. Update the
relevant “角色视觉” row and opening paragraph near HomeMapBackground/RoomScene to
make the scale transition explicit, using the existing symbols CELL_PX,
CHAR_DISPLAY_PX, and gridLayout.ts as anchors.
In `@packages/npc-memory/src/schema.ts`:
- Around line 135-145: The npcLeaningDrift schema definition has a type mismatch
for the drift field: it is currently declared with the integer column builder
while the migration defines it as SMALLINT. Update the drift property in
npcLeaningDrift to use the matching Drizzle smallint builder so the schema stays
aligned with the existing npc_leaning_drift table definition and future diffs
remain consistent.
In `@scripts/uat-phase7-reset-snap.mjs`:
- Around line 21-27: The hardcoded FAR coordinate in uat-phase7-reset-snap.mjs
may not remain sufficiently distant after roomId shuffles, so the reset-snap
test can lose its intended coverage. Update the logic around FAR,
resolveDefaultHome(), and the default home calculation so the chosen point is
validated against the resolved DEFAULT using chebyshevDistance and a threshold,
or compute FAR dynamically until it satisfies the distance requirement. Keep the
existing constants and test flow, but ensure the selected reset point is always
meaningfully far from the council home.
- Around line 4-28: The no-undef findings for Node globals in this script are
false positives; update the ESLint context so this `.mjs` file is treated as
Node. Add a Node environment hint at the top of the script or adjust the ESLint
config for `.mjs` scripts, so references to `process` and `console` in the
`loadRootEnv`/`resolveDefaultHome` module are recognized without changing the
runtime code.
🪄 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: 94488132-6024-47be-bcb2-2ef0723e7a17
⛔ Files ignored due to path filters (1)
apps/web/public/assets/sprites/lpc-npc-1.pngis excluded by!**/*.png
📒 Files selected for processing (103)
.gitignoreAGENTS.mdapps/game-server/data/world/beginning-fields@v1/spawns.jsonapps/game-server/data/world/village-plaza@v1/spawns.jsonapps/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/schema.test.tsapps/game-server/src/ambient/tick.test.tsapps/game-server/src/ambient/tick.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/speak-schema.tsapps/game-server/src/index.test.tsapps/game-server/src/room/council-migrate.tsapps/game-server/src/room/executor.test.tsapps/game-server/src/room/store.test.tsapps/game-server/src/room/store.tsapps/game-server/src/routes/chat.tsapps/game-server/src/sse/hub.colyseus.test.tsapps/game-server/src/sse/hub.tsapps/game-server/src/world/region-walkability.test.tsapps/web/AGENTS.mdapps/web/public/assets/CREDITS.mdapps/web/src/ChatPage.tsxapps/web/src/components/CouncilRosterPanel.test.tsapps/web/src/components/CouncilRosterPanel.tsxapps/web/src/components/DialogueBar.tsxapps/web/src/components/DialogueOverlay.tsxapps/web/src/components/PhaserGame.tsxapps/web/src/game/FloorRenderer.tsapps/web/src/game/HomeMapBackground.tsapps/web/src/game/LocalPlayerMovementController.tsapps/web/src/game/RoomScene.tsapps/web/src/game/activityLabels.tsapps/web/src/game/assetManifest.tsapps/web/src/game/bgNpcLabels.test.tsapps/web/src/game/bgNpcLabels.tsapps/web/src/game/entityLabels.test.tsapps/web/src/game/entityLabels.tsapps/web/src/game/entityLayout.tsapps/web/src/game/entitySprites.tsapps/web/src/game/gridLayout.tsapps/web/src/game/gridMovement.tsapps/web/src/game/intentLabels.tsapps/web/src/game/lpcNpc1Sheet.test.tsapps/web/src/game/lpcNpc1Sheet.tsapps/web/src/game/roomSceneInput.tsapps/web/src/game/roomSceneSync.tsapps/web/src/game/roomSceneTypes.tsapps/web/src/game/roomSceneViewport.test.tsapps/web/src/game/sceneLabelLayout.test.tsapps/web/src/game/sceneLabelLayout.tsapps/web/src/hooks/useColyseusRoom.tsapps/web/src/index.cssapps/web/src/lib/colyseusAmbientSnapshot.test.tsapps/web/src/lib/colyseusAmbientSnapshot.tsdocs/BEGINNING-FIELDS.mddocs/CONTRACTS.mddocs/DEVELOPMENT-HISTORY.mddocs/DEVELOPMENT-HISTORY.zh-CN.mddocs/INVARIANTS-MULTIPLAYER.mddocs/ISSUE-LOG.mdpackage.jsonpackages/npc-memory/migrations/0010_npc_leaning_drift.sqlpackages/npc-memory/migrations/meta/_journal.jsonpackages/npc-memory/src/schema.tspackages/shared/src/council/migrate.test.tspackages/shared/src/council/migrate.tspackages/shared/src/council/spawn.tspackages/shared/src/homeMap.tspackages/shared/src/index.tspackages/shared/src/pathfind.test.tspackages/shared/src/room.test.tspackages/shared/src/room.tspackages/shared/src/worldRegion.tsscripts/lib/council-spawn.mjsscripts/lib/dialogue-engage.mjsscripts/lib/home-spawn.mjsscripts/migrate-room-council-npcs.mjsscripts/sync-npc-lpc-assets.mjsscripts/uat-phase26-playwright.mjsscripts/uat-phase6-move-flash.mjsscripts/uat-phase7-reset-snap.mjsscripts/verify-phase13.mjsscripts/verify-phase16.mjsscripts/verify-phase26.mjsworkers/agent-worker/src/config.pyworkers/agent-worker/src/council/leaning_drift.pyworkers/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/graph/world_vote.pyworkers/agent-worker/tests/test_ambient_intent.pyworkers/agent-worker/tests/test_fetch_state_and_memory.pyworkers/agent-worker/tests/test_leaning_drift.pyworkers/agent-worker/tests/test_persona_runtime.pyworkers/agent-worker/tests/test_social_stream_extract.pyworkers/agent-worker/tests/test_world_vote.py
💤 Files with no reviewable changes (1)
- apps/game-server/src/ambient/schedule.test.ts
Comment on lines
+63
to
+70
| export function lpcNpc1FrameIndex( | ||
| facing: CardinalFacing, | ||
| kind: "walk" | "idle", | ||
| frameInAnim: number, | ||
| ): number { | ||
| const facingIndex = facingToIndex(facing); | ||
| const offset = kind === "walk" ? frameInAnim : LPC_NPC1_WALK_FRAMES + frameInAnim; | ||
| return facingIndex * LPC_NPC1_FRAMES_PER_FACING + offset; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the LPC row map here instead of facingToIndex().
Lines 6-16 define the LPC sheet order as up/left/down/right, but this helper still indexes rows with the generic facing order. That makes the new LPC animations read the wrong source rows and the companion test currently locks that bug in.
Suggested fix
-import { facingToIndex } from "./facing.js";
@@
export function lpcNpc1FrameIndex(
facing: CardinalFacing,
kind: "walk" | "idle",
frameInAnim: number,
): number {
- const facingIndex = facingToIndex(facing);
+ const facingIndex = LPC_SOURCE_ROW_BY_FACING[facing];
const offset = kind === "walk" ? frameInAnim : LPC_NPC1_WALK_FRAMES + frameInAnim;
return facingIndex * LPC_NPC1_FRAMES_PER_FACING + offset;
}Please update apps/web/src/game/lpcNpc1Sheet.test.ts to assert the declared LPC row order as well.
📝 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
| export function lpcNpc1FrameIndex( | |
| facing: CardinalFacing, | |
| kind: "walk" | "idle", | |
| frameInAnim: number, | |
| ): number { | |
| const facingIndex = facingToIndex(facing); | |
| const offset = kind === "walk" ? frameInAnim : LPC_NPC1_WALK_FRAMES + frameInAnim; | |
| return facingIndex * LPC_NPC1_FRAMES_PER_FACING + offset; | |
| export function lpcNpc1FrameIndex( | |
| facing: CardinalFacing, | |
| kind: "walk" | "idle", | |
| frameInAnim: number, | |
| ): number { | |
| const facingIndex = LPC_SOURCE_ROW_BY_FACING[facing]; | |
| const offset = kind === "walk" ? frameInAnim : LPC_NPC1_WALK_FRAMES + frameInAnim; | |
| return facingIndex * LPC_NPC1_FRAMES_PER_FACING + offset; |
🤖 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/lpcNpc1Sheet.ts` around lines 63 - 70, The LPC NPC frame
lookup is still using the generic facing-to-row mapping instead of the LPC
sheet’s declared row order. Update lpcNpc1FrameIndex to derive the row from the
LPC row map used by the sheet definition (up/left/down/right) rather than
facingToIndex(), and keep the walk/idle offset logic intact. Also adjust
lpcNpc1Sheet.test.ts so it verifies the explicit LPC row ordering, not the
generic facing order, by asserting the mapping used by lpcNpc1FrameIndex and the
sheet constants.
Comment on lines
+211
to
+214
| white-space: pre-wrap; | ||
| overflow-wrap: anywhere; | ||
| max-height: min(40vh, 12rem); | ||
| overflow-y: auto; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The new transcript scroll area still can’t be scrolled.
.dialogue-overlay keeps pointer-events: none on Line 125, so making .dialogue-overlay__last-line scrollable just clips long text behind an inert scrollbar. Either re-enable pointer events on the scroll target/frame or remove the max-height cap.
Possible fix
.dialogue-overlay__last-line {
white-space: pre-wrap;
overflow-wrap: anywhere;
max-height: min(40vh, 12rem);
overflow-y: auto;
+ pointer-events: auto;
}📝 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
| white-space: pre-wrap; | |
| overflow-wrap: anywhere; | |
| max-height: min(40vh, 12rem); | |
| overflow-y: auto; | |
| white-space: pre-wrap; | |
| overflow-wrap: anywhere; | |
| max-height: min(40vh, 12rem); | |
| overflow-y: auto; | |
| pointer-events: auto; |
🤖 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/index.css` around lines 211 - 214, The transcript scroll area is
still blocked by the parent overlay’s pointer-events setting, so the new
`.dialogue-overlay__last-line` styles alone won’t make it usable. Update the
dialogue overlay/scroll container setup in `index.css` so the actual scrollable
element (the one with `max-height`, `overflow-y: auto`, and `white-space:
pre-wrap`) can receive pointer events, or remove the height cap if that better
matches the layout. Use the `.dialogue-overlay` and
`.dialogue-overlay__last-line` selectors to locate and adjust the affected
styles.
Comment on lines
+19
to
+21
| const WALK_FRAMES = 9; | ||
| const IDLE_FRAMES = 2; | ||
| const FRAMES_PER_FACING = WALK_FRAMES + IDLE_FRAMES; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the baked atlas layout with the web runtime contract.
This script now emits 9 walk frames per facing, but apps/web/src/game/assetManifest.ts still declares WALK_FRAMES = 4, IDLE_FRAMES = 2, FRAMES_PER_FACING = 6. With the current layout, the web client will read columns 4-5 as idle even though this baker writes idle at columns 9-10, so idle states will render the wrong frames.
Suggested fix
- * Layout: 4 facings × (9 walk + 2 idle) frames, 64×64 each → 704×256 PNG.
+ * Layout: 4 facings × (4 walk + 2 idle) frames, 64×64 each → 384×256 PNG.
@@
-const WALK_FRAMES = 9;
+const WALK_FRAMES = 4;
const IDLE_FRAMES = 2;
const FRAMES_PER_FACING = WALK_FRAMES + IDLE_FRAMES;Also applies to: 83-98
🤖 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/sync-npc-lpc-assets.mjs` around lines 19 - 21, The baked atlas layout
constants in the NPC/LPC asset sync script no longer match the web runtime
contract, so the client reads the wrong columns for idle frames. Update the
frame layout used by the baker and keep it consistent with the corresponding
constants in assetManifest (WALK_FRAMES, IDLE_FRAMES, and FRAMES_PER_FACING), so
both sides agree on the same per-facing column arrangement. Make sure the
symbols in the sync script and the web manifest are aligned after the change.
Comment on lines
+5
to
+13
| * Phase 26+: 12 council NPCs on map; bg-villager tier removed — P16-10/11 assert council presence + invalid speak 400. | ||
| * Output: .planning/phases/16-intelligent-ambient-npcs/verify-screenshots/ + verify-report.json | ||
| */ | ||
| import { mkdir, writeFile } from "node:fs/promises"; | ||
| import { dirname, resolve, relative } from "node:path"; | ||
| import { fileURLToPath, pathToFileURL } from "node:url"; | ||
| import { assertE2eNoMock } from "./lib/e2e-policy.mjs"; | ||
| import { gameServerHttpBase, loadRootEnv } from "./lib/env.mjs"; | ||
| import { assertCanonicalCouncilRoster, COUNCIL_NPC_IDS } from "./lib/council-spawn.mjs"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This script currently introduces no-undef lint errors.
Static analysis is already flagging new uses of fetch, window, and console in this file. Unless scripts/verify-phase16.mjs is covered by the correct Node/browser globals (or the lint override is updated for *.mjs scripts), this phase gate lands red in CI.
🤖 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 5 - 13, The verify script is
introducing no-undef lint failures because it uses runtime globals like fetch,
window, and console without the appropriate environment coverage. Update
scripts/verify-phase16.mjs so its globals are recognized by the correct
Node/browser lint scope, or extend the lint override for *.mjs scripts to
include those globals; make sure the fix is applied consistently around the
existing helpers like assertE2eNoMock, gameServerHttpBase, and loadRootEnv.
Source: Linters/SAST tools
Comment on lines
+13
to
+31
| import { spawnSync } from "node:child_process"; | ||
| import { mkdir } from "node:fs/promises"; | ||
| import { join } from "node:path"; | ||
| import { dirname, resolve } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { | ||
| assertE2eNoMock, | ||
| assertE2eRealLlm, | ||
| e2eSpeakTimeoutMs, | ||
| } from "./lib/e2e-policy.mjs"; | ||
| import { engageDialogue } from "./lib/dialogue-engage.mjs"; | ||
| import { | ||
| closeShellDrawer, | ||
| openShellDrawerCollective, | ||
| sendSpeakOverlay, | ||
| } from "./lib/e2e-memory-helpers.mjs"; | ||
| import { gameServerHttpBase, loadRootEnv } from "./lib/env.mjs"; | ||
| import { loadPlaywright } from "./lib/speak-browser-stack.mjs"; | ||
| import { COUNCIL_NPC_IDS } from "./lib/council-spawn.mjs"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
verify-phase26.mjs is failing no-undef as written.
Static analysis is already reporting process, fetch, AbortSignal, URLSearchParams, window, document, localStorage, and console as undefined in this new file. Please align the script or ESLint overrides before landing; otherwise the new gate is red on arrival.
🤖 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-phase26.mjs` around lines 13 - 31, The new verify-phase26.mjs
script is triggering no-undef because it relies on globals like process, fetch,
AbortSignal, URLSearchParams, window, document, localStorage, and console
without declaring them. Update the script’s ESLint scope or file-level overrides
to recognize the intended runtime globals, or refactor the script so the
relevant helpers and entrypoint in verify-phase26.mjs do not reference
undeclared globals. Keep the fix localized to this script and its lint
configuration so the gate passes cleanly.
Source: Linters/SAST tools
Comment on lines
+72
to
+73
| def _use_in_memory() -> bool: | ||
| return os.getenv("LLM_MOCK") == "1" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor Settings.llm_mock in the in-memory toggle.
This helper only checks LLM_MOCK, but the rest of the worker already treats Settings.llm_mock as mock mode. A run started with Settings(llm_mock=True) and no env var will still hit collective_connection() here, and workers/agent-worker/src/graph/npc_loop.py then swallows that failure so drift silently stops applying. It also makes workers/agent-worker/tests/test_leaning_drift.py depend on external env instead of its own fixture.
🤖 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/council/leaning_drift.py` around lines 72 - 73, The
in-memory toggle in _use_in_memory only checks the LLM_MOCK environment
variable, so it ignores Settings.llm_mock and can incorrectly call
collective_connection() during mock runs. Update _use_in_memory to honor the
existing Settings.llm_mock setting as well, keeping its behavior aligned with
the rest of the worker and the tests in test_leaning_drift.py.
- engageNpcDialogue: require aria-selected on target chip (shared + verify:phase26) - stopEntityMotion: clear speakHaloTween and killTweensOf(ring) - room/spawn: clone starter inventory; bootstrap full defaultWorldRegistryBundle Co-authored-by: Cursor <cursoragent@cursor.com>
Expose data-active-npc-id on dialogue-bar and __aetherlife_engageNpc dev hook so engageNpcDialogue works when corner-menu chips unmount; ignore stale overlay replies when switching speak targets between council NPCs. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/lib/e2e-memory-helpers.mjs (1)
194-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBaseline-diff can false-negative on identical replies, causing a timeout even after a genuine new response.
isNewReplytreats any text equal tobaselineTextas "not new." If the NPC's fresh reply happens to match the pre-existing text verbatim (repeated/canned response, generic fallback), the loop runs out the fulltimeoutMsand throws, even though a distinct response cycle actually completed.thinkingMs(already tracked insendSpeakOverlay) proves a new cycle occurred and could be used to safely bypass the baseline-equality check.🩹 Suggested fix: bypass baseline check once "thinking" was observed
-async function waitSpeakFirstText(page, t0, timeoutMs, baselineText = "") { +async function waitSpeakFirstText(page, t0, timeoutMs, baselineText = "", hadThinking = false) { const baseline = baselineText.trim(); const deadline = Date.now() + timeoutMs; let overlayPartialMs = null; const isNewReply = (text) => { const trimmed = text.trim(); if (!trimmed || /^思考/.test(trimmed)) return false; - return !baseline || trimmed !== baseline; + return !baseline || hadThinking || trimmed !== baseline; };const { firstTextMs, overlayPartialMs } = await waitSpeakFirstText( page, t0, speakTimeoutMs, baselineReply, + thinkingMs !== null, );Also applies to: 305-318
🤖 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/e2e-memory-helpers.mjs` around lines 194 - 236, The baseline comparison in waitSpeakFirstText can falsely treat a legitimate fresh reply as old when the NPC returns the same text verbatim, so update the new-reply detection to stop relying on baseline equality after a new response cycle has been confirmed. Use the already-tracked thinkingMs from sendSpeakOverlay (or an equivalent “thinking observed” signal) to bypass the baselineText check once the cycle has started, while still ignoring empty/思考 text and preserving the existing overlay/dialogue-bar checks.
🤖 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.
Outside diff comments:
In `@scripts/lib/e2e-memory-helpers.mjs`:
- Around line 194-236: The baseline comparison in waitSpeakFirstText can falsely
treat a legitimate fresh reply as old when the NPC returns the same text
verbatim, so update the new-reply detection to stop relying on baseline equality
after a new response cycle has been confirmed. Use the already-tracked
thinkingMs from sendSpeakOverlay (or an equivalent “thinking observed” signal)
to bypass the baselineText check once the cycle has started, while still
ignoring empty/思考 text and preserving the existing overlay/dialogue-bar checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8fe2066-f3da-4053-95ef-1b9362c19447
📒 Files selected for processing (4)
apps/web/src/components/DialogueBar.tsxapps/web/src/game/roomSceneSync.tsscripts/lib/dialogue-engage.mjsscripts/lib/e2e-memory-helpers.mjs
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
MapSchemav2 (schemaVersion=2); removes legacy flatnpc1/2/3+bg-villagermodel; addsverify:phase26/uat:phase26gates, dispersedcouncilSpawns, leaning-drift migration, and roster/speak hardening.npc-1use bakedlpc-npc-1.png(walk/idle); display gridCELL_PX=32, character height 64px (2 cells); council NPCsnpc-2…npc-12remain on Stardewnpcs.png.BEGINNING-FIELDS.md§角色视觉,ISSUE-LOG#106–#108 / ISSUE-102, frozen UX contracts updated;npc-asset/gitignored (local source only — bake viapnpm assets:sync:lpc-npc1).Test plan
pnpm --filter @aetherlife/game-server testpnpm --filter @aetherlife/web test(146)cd workers/agent-worker && LLM_MOCK=1 uv run pytest -qpnpm agent:verify(pre-push)pnpm verify:phase6:move-onlypnpm verify:phase13pnpm verify:phase26(needspnpm dev:stack+ real LLM keys)pnpm uat:phase26:playwright(optional full UAT)Made with Cursor
Summary by CodeRabbit
New Features
npc-1, including updated nameplates, intents/activities, and dialogue bubble placement.Bug Fixes