Phase 29: NPC memory retrieval quality - #23
Conversation
- Lock computeRecencyFactor / ageHours-weighted score / env defaults - Preserve two-arg computeWeightedScore compatibility expectations Co-authored-by: Cursor <cursoragent@cursor.com>
- Add computeRecencyFactor with S(importance) and floor/ε clamps - Extend computeWeightedScore with optional ageHours; two-arg unchanged - resolveRecencyConfig reads MEMORY_RECENCY_* with safe defaults Co-authored-by: Cursor <cursoragent@cursor.com>
- Fixture assert SQL-style expression ≡ TS helpers - Council 0.55 path lock; ranking flip requires seedMemoryForTests Co-authored-by: Cursor <cursoragent@cursor.com>
- Parameterize S0/floor/ε in searchSimilar SQL (no float concat) - TestMemoryBackend uses computeWeightedScore(ageHours from createdAt) - Document MEMORY_RECENCY_* defaults in .env.example Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover sync Map-only getRecentTurns, async rehydrate, fire-and-forget mirror TTL 7d - Cover missing REDIS_URL degrade and clear Map+Redis known keys Co-authored-by: Cursor <cursoragent@cursor.com>
- Sync getRecentTurns stays Map-only; getRecentTurnsAsync rehydrates via LRANGE - appendCompletedTurn updates Map then fire-and-forget RPUSH/LTRIM/EXPIRE 7d - clearDialogueForPlayer deletes Map + tracked Redis keys; missing REDIS_URL degrades Co-authored-by: Cursor <cursoragent@cursor.com>
- Await Redis rehydrate only on already-async startNpcChatTurn - Leave GameRoom/chat casual stub on sync getRecentTurns (no speak-path Redis RTT) Co-authored-by: Cursor <cursoragent@cursor.com>
- Wire package.json verify:pgvector before probe implementation (TDD RED) Co-authored-by: Cursor <cursoragent@cursor.com>
- Read-only SELECT extversion + halfvec cast; semver >= 0.7.0 - Structured verdict line; FAIL defers 0013 / 29-04 Co-authored-by: Cursor <cursoragent@cursor.com>
- 0013: expression HNSW on npc_memories embedding::halfvec(2048) - 0014: additive current_mood/key_beliefs/summary on npc_attitudes - Mirror columns in Drizzle schema; schema tests lock DDL contracts Co-authored-by: Cursor <cursoragent@cursor.com>
- resolveKOverfetch max(20, 4*k) with cap - rerankCandidatesByForgettingCurve top-k scoring Co-authored-by: Cursor <cursoragent@cursor.com>
- searchSimilar ORDER BY halfvec <=> ASC LIMIT k_overfetch, then weighted top-k - resolveKOverfetch max(20, 4*k) capped; pure rerank helpers + unit tests - pnpm verify:memory-recall for EXPLAIN + overlap gate Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover NPC_MOODS whitelist accept/reject and token caps - Illegal mood must omit so callers preserve prior values Co-authored-by: Cursor <cursoragent@cursor.com>
- Closed NPC_MOODS whitelist; illegal mood omitted for preserve-prior - Beliefs ≤5×40 and summary ≤200; export from shared barrel Co-authored-by: Cursor <cursoragent@cursor.com>
- Beliefs replace; omit preserves prior; reputation must not wipe mood Co-authored-by: Cursor <cursoragent@cursor.com>
- getAttitudeRow + upsertSemanticState; beliefs replace on success - Reputation onConflict sets only reputation + updated_at - MemoryService.upsertAttitudeSemantic clamps then writes via repo Co-authored-by: Cursor <cursoragent@cursor.com>
- Table-driven friend/rival/fanout/min-event/affection cases (D-PROP-10) Co-authored-by: Cursor <cursoragent@cursor.com>
- D-PROP-10 constants 0.3/30/3/5/5; friend same-sign, rival/enemy invert - Export from shared index; WitnessDeltaUpdate shape for applyReputationDelta Co-authored-by: Cursor <cursoragent@cursor.com>
- Event count===1, friend reputation delta, list failure degrade Co-authored-by: Cursor <cursoragent@cursor.com>
- After witness: listRelationshipsForRoom → prop deltas → applyReputationDelta only - try/catch degrades on list failure; never insertEvent for propagation Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover mock Chinese mood whitelist + first-person beliefs - Bad JSON omits semantic; illegal mood omits mood only - Wrapper str path still expected to work Co-authored-by: Cursor <cursoragent@cursor.com>
- Same reflect LLM call returns text + mood/beliefs JSON - Parse failure / illegal mood omits semantic fields - run_reflect_llm remains a prose str wrapper Co-authored-by: Cursor <cursoragent@cursor.com>
- Success path writes mood/beliefs/summary via attitudes - Illegal mood preserves prior mood; omit path unchanged Co-authored-by: Cursor <cursoragent@cursor.com>
- Optional mood/beliefs/summary on store_reflection + internal reflect - MemoryService clamps then upserts semantic after text store - Memory tail uses run_reflect_llm_structured (zero new LLM calls) Co-authored-by: Cursor <cursoragent@cursor.com>
- Extend CollectiveContext with currentMood/keyBeliefs/summary from getAttitudeRow - MemoryContext.collective forwards semantic for worker; public state still band-only Co-authored-by: Cursor <cursoragent@cursor.com>
- format_attitude_context mood/beliefs/summary lines - speak_system_context wiring; tools invariant across moods Co-authored-by: Cursor <cursoragent@cursor.com>
- format_attitude_context appends mood/beliefs/summary lines - speak_system_context + parse_collective_from_context + merge keys wire DB→prompt Co-authored-by: Cursor <cursoragent@cursor.com>
- Document internal MemoryContext keeps mood/beliefs/summary; public collective-state strips - Annotate MemoryContext + collective-state route (D-BELIEF-11) Co-authored-by: Cursor <cursoragent@cursor.com>
Overlay hides thinking when a displayLine exists (casual client stub / prior NPC line), so the old thinking-only locator timed out every round. Also wait for composer idle before the next send. Co-authored-by: Cursor <cursoragent@cursor.com>
- B1 total p50 13448 vs P4 baseline 4521; D-PERF not claimed - Attribute stack noise (memory-tail, 429/timeout) over blind revert Co-authored-by: Cursor <cursoragent@cursor.com>
Stop over-broad audit_reply false positives (ISSUE-112) and automate dialogue Redis rehydrate via Map-evict Playwright; document Phase 29 memory/D-SESS notes and Guardrails #120–#124. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Default listRelationshipsForRoom to [] so DATABASE_URL does not hang pre-push on real Postgres during recordRuleEvent propagation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughPhase 29 adds forgetting-curve memory retrieval, semantic NPC attitude storage and prompting, relationship-based reputation propagation, Redis-backed dialogue continuity, reply-audit guardrails, verification scripts, migrations, tests, and related documentation. ChangesMemory, semantic state, and propagation
Dialogue continuity
Reply audit and supporting updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workers/agent-worker/src/graph/reflect.py (1)
46-129: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winJSON parse failure now silently drops the entire reflection, not just the semantic fields.
run_reflect_llm_structuredrelies purely on prompt instructions ("Reply with JSON only") with noresponse_format/schema enforcement and no retry. If the model doesn't produce parseable JSON (or omits a validtextfield),_parse_reflect_jsonreturnsNone, andmaybe_reflect_turninnpc_loop.pythen skipsstore_reflectionentirely — so plain prose reflection text, which the prior unstructured implementation always captured, is now silently lost on any parse failure. Prompt-only JSON enforcement has a well-known non-trivial failure rate in production, so this is a real reliability regression to a core memory feature, not just a narrow edge case.Consider falling back to the raw LLM content as prose when JSON parsing fails, rather than dropping the turn outright:
🩹 Proposed fallback fix
content = str(getattr(response, "content", "") or "").strip() - return _parse_reflect_json(content) + parsed = _parse_reflect_json(content) + if parsed is not None: + return parsed + # Model didn't emit valid JSON — fall back to raw prose rather than + # silently dropping the whole reflection turn (D-BELIEF-13). + return ReflectStructured(text=content[:800]) if content else None🤖 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/reflect.py` around lines 46 - 129, Update run_reflect_llm_structured and _parse_reflect_json so unparseable JSON or JSON without a valid text field falls back to the raw LLM response as ReflectStructured.text instead of returning None. Preserve valid JSON parsing and optional mood, beliefs, and summary extraction, while ensuring non-empty plain prose still reaches maybe_reflect_turn and store_reflection.
🧹 Nitpick comments (13)
scripts/uat-phase29-dialogue-restart.mjs (1)
215-219: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed 1500ms sleep makes the hard gate timing-dependent.
The Redis mirror is fire-and-forget, so this sleep is the only thing guaranteeing the
rpushlanded before the Map is evicted. On a slow/loaded Redis the UAT fails spuriously. Polldialogue-turnsuntil the seed appears (or retry the post-evict fetch) instead of a single sleep.🤖 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-phase29-dialogue-restart.mjs` around lines 215 - 219, Replace the fixed 1500ms delay before evictDialogueMap in the dialogue restart flow with bounded polling that repeatedly fetches turns and waits until the seed appears, or retries the post-eviction fetch using the existing assertion behavior. Ensure the polling has a timeout or maximum attempts so failures remain explicit rather than hanging.apps/game-server/src/routes/rooms.ts (1)
416-432: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo length cap on seeded dialogue text.
playerMessage/npcReplyare accepted at arbitrary length and stored in the Map plus mirrored to Redis, then injected into the speak prompt as recent turns.apps/game-server/src/routes/internal-memories.ts(Line 43) already enforcesMAX_PLAYER_MESSAGE_LENfor comparable text input; mirroring that here keeps the contract consistent and bounds prompt/Redis growth.🛡️ Proposed validation
if (!roomId?.trim() || !playerId || !npcId || !playerMessage || !npcReply) { res.status(400).json({ ok: false, error: "roomId, playerId, npcId, playerMessage, npcReply required", }); return; } + if ( + playerMessage.length > MAX_PLAYER_MESSAGE_LEN || + npcReply.length > MAX_PLAYER_MESSAGE_LEN + ) { + res.status(400).json({ ok: false, error: "text too long" }); + return; + } appendCompletedTurn({ roomId, playerId, npcId, playerMessage, npcReply });Add
MAX_PLAYER_MESSAGE_LENto the existing@aetherlife/sharedimport at the top of the file.🤖 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/rooms.ts` around lines 416 - 432, Update the dialogue-append handler around appendCompletedTurn to import and apply MAX_PLAYER_MESSAGE_LEN from `@aetherlife/shared`, rejecting requests when playerMessage or npcReply exceeds that limit while preserving the existing required-field validation and 400 response behavior.packages/npc-memory/src/schema.test.ts (1)
51-61: 📐 Maintainability & Code Quality | 🔵 TrivialThis locks in a lock-taking index build.
Asserting the absence of
CREATE INDEX CONCURRENTLYpins migration 0013 to a build that holds anACCESS EXCLUSIVElock onnpc_memoriesfor its duration. That is fine for dev/small tables, but on a populated table it blocks reads and writes. If this is a deliberate choice (concurrent builds cannot run inside the transaction Drizzle wraps migrations in), consider documenting the manual/out-of-band path for production.🤖 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.test.ts` around lines 51 - 61, The migration test for 0013 currently enforces a locking index build without documenting the production alternative. Update the test or nearby migration documentation to describe the manual/out-of-band production procedure for building the index concurrently, while preserving the existing non-CONCURRENTLY assertion for the transaction-wrapped migration.packages/npc-memory/src/collective/repository.ts (2)
76-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefault mood literal is triplicated.
"平静"now appears here, at Line 273, and as the column default inpackages/npc-memory/src/schema.ts(Line 114). Exporting aDEFAULT_NPC_MOODconstant frompackages/shared/src/council/semanticState.ts(which already ownsNPC_MOODS) and referencing it in all three places keeps the default aligned with the whitelist.🤖 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/collective/repository.ts` around lines 76 - 78, Replace the duplicated "平静" default with an exported DEFAULT_NPC_MOOD constant from semanticState.ts, deriving it from the existing NPC_MOODS whitelist. Update defaultSemanticFields, the occurrence near line 273, and the schema column default to reference this shared constant.
302-310: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the
hasXguards forpatch.mood/patch.beliefs/patch.summary.TypeScript does not narrow non-null optional properties via
patch.mood !== undefined, sopatch.mood!and inlinenextMood = patch.mood!still require non-null assertions. Assign through a narrowed local variable (mood: string) to make the write and finalonConflictDoUpdate({ set })clear without assertions.🤖 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/collective/repository.ts` around lines 302 - 310, Update the conflict-set construction around hasMood, hasBeliefs, and hasSummary to assign each patch value through a locally narrowed variable inside its corresponding guard, such as a typed mood, beliefs, or summary value. Use those narrowed locals for conflictSet assignments and the final onConflictDoUpdate set so the flow no longer relies on non-null assertions.packages/npc-memory/src/collective/repository.test.ts (1)
128-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test exercises the early return, not the omit path.
upsertSemanticState(..., {})short-circuits atpackages/npc-memory/src/collective/repository.tsLine 268 before any per-key logic runs, so the D-BELIEF-07 "omitted keys are not written" branch is never hit. A single-key patch covers the real behavior. Note the DBconflictSetpath remains untested here since these tests all use the in-memory store.💚 Proposed stronger omit-path assertions
await repo.upsertSemanticState("r-omit", "npc-2", "p-a", {}); - const row = await repo.getAttitudeRow("r-omit", "npc-2", "p-a"); + let row = await repo.getAttitudeRow("r-omit", "npc-2", "p-a"); + expect(row!.currentMood).toBe("愉悦"); + expect(row!.keyBeliefs).toEqual(["我信任他"]); + expect(row!.summary).toBe("进展顺利"); + + // Partial patch: only mood is written, beliefs/summary preserved. + await repo.upsertSemanticState("r-omit", "npc-2", "p-a", { mood: "低落" }); + row = await repo.getAttitudeRow("r-omit", "npc-2", "p-a"); + expect(row!.currentMood).toBe("低落"); expect(row!.keyBeliefs).toEqual(["我信任他"]); expect(row!.summary).toBe("进展顺利"); - expect(row!.currentMood).toBe("愉悦");🤖 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/collective/repository.test.ts` around lines 128 - 140, Update the D-BELIEF-07 test around upsertSemanticState to exercise the per-key omit path instead of the empty-patch early return: perform a single-key semantic update while omitting the other fields, then assert the omitted columns retain their prior values and the updated key changes as expected. Keep the existing in-memory repository setup, and do not attempt to cover the database conflictSet path in this test.packages/shared/src/relationshipPropagation.ts (1)
21-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive propagation polarity from the relationship metadata SSOT.
relationshipPropogationPolarityForBaseTaguses its own friend/rival sets while the relationship metadata and initial affection mappings recognize additional kinds such astrade,chess,frenemy, andpeer. Those currently return0and never participate in reputation propagation; add coverage or derive polarization from the canonical relationship definition/source to prevent silent drift.🤖 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/relationshipPropagation.ts` around lines 21 - 44, Update propagationPolarityForBaseTag to use the canonical relationship metadata or shared initial-affection mapping as its source of truth instead of maintaining incomplete local tag sets. Ensure kinds such as trade, chess, frenemy, and peer receive the polarity defined by that canonical source, while unknown tags continue returning 0..env.example (1)
91-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
MEMORY_K_OVERFETCH_CAPalongside the other Phase 29 recency knobs.
resolveKOverfetchinpackages/npc-memory/src/repository.ts(Line 109) readsMEMORY_K_OVERFETCH_CAP, but it is absent here, so operators have no discoverable way to tune the ANN candidate cap.📝 Proposed addition
MEMORY_RECENCY_S_EPSILON=0.001 +# Hard cap for ANN overfetch candidate pool (default 200) +MEMORY_K_OVERFETCH_CAP=200🤖 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 @.env.example around lines 91 - 94, Add the MEMORY_K_OVERFETCH_CAP environment variable to .env.example alongside the Phase 29 recency settings, documenting the configuration consumed by resolveKOverfetch in repository.ts. Preserve the existing ordering and formatting of the related knobs.packages/npc-memory/src/repository.ts (1)
130-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftScoring formula now lives in three places; rerank helper isn't the path
searchSimilaruses.
searchSimilarre-scores in SQL,rerankCandidatesByForgettingCurvere-scores in TS, andscripts/verify-memory-recall.mjsrecomputes it again. The D-DECAY-04 parity test compares TS against a hand-written mirror of the SQL, not against the database, so SQL drift stays invisible. Either routesearchSimilarthrough this helper (selectdist/created_atand rerank in TS) or add an integration check that executes the actual SQL expression.🤖 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/repository.ts` around lines 130 - 145, Align the forgetting-curve scoring paths by either routing searchSimilar through rerankCandidatesByForgettingCurve using selected distance and creation-time data, or adding an integration test that executes and validates the actual SQL scoring expression. Update the D-DECAY-04 parity coverage so it verifies database results rather than only comparing TypeScript with a hand-written SQL mirror, and keep the verification script consistent with the single authoritative formula.packages/npc-memory/src/repository.test.ts (1)
132-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
MEMORY_K_OVERFETCH_CAPenv override.The env-cap branch in
resolveKOverfetch(packages/npc-memory/src/repository.tsLines 109-114) is untested, including the invalid/< 1fallback. Note theresolveKOverfetch(10_000)expectation also encodes the "overfetch may be smaller thank" behavior flagged onrepository.ts; it will need updating if that clamp changes.💚 Suggested additional cases
it("caps at DEFAULT_K_OVERFETCH_CAP", () => { expect(resolveKOverfetch(10_000)).toBe(DEFAULT_K_OVERFETCH_CAP); }); + + it("honors MEMORY_K_OVERFETCH_CAP and ignores invalid values", () => { + expect(resolveKOverfetch(100, { MEMORY_K_OVERFETCH_CAP: "50" })).toBe(50); + expect(resolveKOverfetch(100, { MEMORY_K_OVERFETCH_CAP: "nope" })).toBe( + DEFAULT_K_OVERFETCH_CAP, + ); + });🤖 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/repository.test.ts` around lines 132 - 142, Add tests in the “resolveKOverfetch (D-ANN-03)” suite covering a valid MEMORY_K_OVERFETCH_CAP override and invalid or less-than-one values falling back to the default cap, restoring the environment after each case. Update the large-k expectation only if the repository implementation changes its cap behavior, preserving the intended result for k exceeding the configured cap.packages/npc-memory/migrations/0013_npc_memories_halfvec_index.sql (1)
5-7: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBuild this HNSW index out-of-band for production
CREATE INDEXwithoutCONCURRENTLYlocksnpc_memoriesfor writes while the 2048-dim halfvec HNSW index is built. Keep theCREATE INDEX IF NOT EXISTShere as a no-op fallback after the index exists, and runCREATE INDEX CONCURRENTLYin a maintenance step for production. Also validatehalfvec_cosine_opsplus matching expression-index usage in the deployed pgvector/version before relying on full coverage.🤖 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/migrations/0013_npc_memories_halfvec_index.sql` around lines 5 - 7, Update the migration around npc_memories_embedding_halfvec_hnsw to retain CREATE INDEX IF NOT EXISTS as the post-maintenance fallback, while documenting or adding the production out-of-band CREATE INDEX CONCURRENTLY maintenance step. Validate that the deployed pgvector version supports halfvec_cosine_ops and that queries use the matching embedding::halfvec(2048) expression index.Source: Linters/SAST tools
scripts/verify-pgvector-capability.mjs (1)
16-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the shared
.envparser intoscripts/lib/.
loadEnvis repeated in these new DB verify scripts and already exists in several other verify scripts. Extracting it will reduce duplication and ensure consistent.envparsing without adding dependencies.🤖 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-pgvector-capability.mjs` around lines 16 - 33, Extract the shared loadEnv function from verify-pgvector-capability.mjs into a reusable module under scripts/lib/, then update this script and the other verify scripts that duplicate it to import and reuse that module. Preserve the existing missing-file error behavior and parsing semantics without adding dependencies.scripts/verify-memory-recall.mjs (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThird copy of the overfetch/forgetting-curve constants — import from
@aetherlife/npc-memoryinstead.
resolveKOverfetchand the recency defaults (72h / 0.3 / 1e-3) already exist inpackages/npc-memory/src/repository.ts, and this script duplicates both plus the curve SQL. The new Guardrail#121indocs/ISSUE-LOG.mdrequires a single SSOT for the decay formula, so this gate can silently pass against stale knobs after a production tweak. Importing the exported helper/constants (keeping only the raw SQL local) keeps the gate honest.Also applies to: 74-83
🤖 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-memory-recall.mjs` around lines 47 - 54, Update verify-memory-recall’s resolveKOverfetch and recency-default usage to import and reuse the exported helper/constants from `@aetherlife/npc-memory`, removing the duplicated 72h, 0.3, and 1e-3 values. Keep only the gate-specific curve SQL local, and preserve the existing environment override behavior.
🤖 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/guards/reply.py`:
- Around line 10-15: Constrain the duplicated state-change regex alternatives to
affirmative physical actions, preventing matches within figurative,
interrogative, or negated dialogue while preserving valid action detection.
Apply the identical refined English and Chinese patterns in
apps/ai-gateway/app/guards/reply.py lines 10-15 and
workers/agent-worker/src/guard/reply_audit.py lines 8-11, and add regression
tests covering question and negation cases at both sites.
In `@apps/game-server/src/collective/service.ts`:
- Around line 124-139: Increase the edge candidate limit used by the
relationship fetch in the propagation flow around listRelationshipsForRoom,
instead of pre-limiting it to PROPAGATION_MAX_FANOUT. Overfetch enough
candidates for computeRelationshipPropagationDeltas to filter alreadyUpdated,
low-affection, and neutral-polarity edges before applying the final
PROPAGATION_MAX_FANOUT selection.
In `@apps/game-server/src/npc/dialogue-session.test.ts`:
- Around line 279-313: Update the “Redis errors are logged and never thrown to
caller” test to assert that the console.error spy records calls after the
failing Redis operations. Keep the existing no-throw and fallback-result
assertions, and ensure the assertion uses the errSpy created in that test before
restoring it.
In `@apps/game-server/src/npc/dialogue-session.ts`:
- Around line 134-143: Update the rehydration flow around the post-await
sessions.set call: after Redis LRANGE and turn parsing, re-check whether
sessions already contains k and preserve the existing in-memory entry instead of
overwriting it. Only cache the Redis result when turns.length is greater than
zero, leaving genuine empty misses uncached so future calls can consult Redis
again; retain knownRedisKeys updates and return behavior for non-empty results.
In `@packages/npc-memory/migrations/0014_npc_attitudes_semantic_state.sql`:
- Around line 3-8: Update the migration’s ALTER TABLE statements for
current_mood, key_beliefs, and summary to apply their defaults and enforce NOT
NULL, matching the notNull() declarations in schema.ts and preventing NULL
values during partial upserts.
In `@packages/npc-memory/src/repository.ts`:
- Around line 203-218: The ANN query in searchSimilar must avoid returning too
few matches after applying its selective room_id, player_id, npc_id,
summarized_at, and embedding predicates. Update the search path to provide
sufficient filtered coverage—such as an appropriate predicate-aware
partial/index strategy, session-local hnsw.ef_search aligned with kOverfetch, or
an exact-scan fallback when fewer than k matches remain—while preserving the
existing result ordering and limit behavior.
- Around line 219-234: Use the same NULL importance default in both SQL score
terms: update the stability-term COALESCE in the query around the score
expression to match the existing importance factor default of 5. Keep
computeWeightedScore/computeRecencyFactor parity by ensuring NULL importance
contributes identically to both the importance multiplier and recency stability
calculation.
- Around line 104-116: Update resolveKOverfetch so its returned candidate pool
is never smaller than safeK, including when the configured overfetch cap is
below k; preserve the existing minimum overfetch behavior while ensuring
searchSimilar can satisfy the requested limit.
In `@packages/npc-memory/src/schema.ts`:
- Around line 113-118: The attitude schema and tests must match migration 0014’s
nullable columns. Update the Drizzle definitions for currentMood, keyBeliefs,
and summary in packages/npc-memory/src/schema.ts to allow null values, then
adjust the corresponding assertions in packages/npc-memory/src/schema.test.ts to
expect nullable types; preserve existing defaults for inserted rows.
In `@packages/shared/src/relationshipPropagation.ts`:
- Around line 99-100: Update the relationship-fetch flow used by
listRelationshipsForRoom and the propagation logic in relationshipPropagation.ts
to retrieve a small overbuffer beyond PROPAGATION_MAX_FANOUT with stable
ordering, so low-affection or neutral rows are removed before the final cap.
Preserve the existing filters, sorting, and candidates.slice(0,
PROPAGATION_MAX_FANOUT) limit for the resulting propagation targets.
- Around line 78-97: Deduplicate candidates by NPC ID in the edge-processing
loop, retaining only the candidate with the highest absAffection for each other
NPC. Update the candidate collection logic in the surrounding relationship
propagation function so mirrored or multiple baseTag edges cannot produce
repeated updates, while preserving the existing filtering and polarity behavior.
In `@scripts/uat-phase29-dialogue-restart.mjs`:
- Around line 33-44: Update the ESLint configuration and/or package lint command
so scripts/uat-phase29-dialogue-restart.mjs is linted with Node globals enabled,
including process, console, fetch, and AbortSignal. Prefer extending the
existing UAT script pattern or explicit script list, and ensure pnpm lint
includes the script.
In `@scripts/verify-memory-recall.mjs`:
- Around line 26-28: Update the ESLint configuration for scripts/**/*.mjs to
declare Node.js globals, including console and process, so
verify-memory-recall.mjs and verify-pgvector-capability.mjs pass no-undef
without changing their runtime logic.
- Around line 59-68: Update planLooksLikeAnn to remove the loose
lower.includes("ann") check and require evidence of the intended HNSW index or
ANN operator instead. Ensure unrelated plan text, including Seq Scan plans and
Bitmap Index Scan on room_id, cannot satisfy the D-ANN-03 gate; preserve only
matches that confirm the expected HNSW path.
---
Outside diff comments:
In `@workers/agent-worker/src/graph/reflect.py`:
- Around line 46-129: Update run_reflect_llm_structured and _parse_reflect_json
so unparseable JSON or JSON without a valid text field falls back to the raw LLM
response as ReflectStructured.text instead of returning None. Preserve valid
JSON parsing and optional mood, beliefs, and summary extraction, while ensuring
non-empty plain prose still reaches maybe_reflect_turn and store_reflection.
---
Nitpick comments:
In @.env.example:
- Around line 91-94: Add the MEMORY_K_OVERFETCH_CAP environment variable to
.env.example alongside the Phase 29 recency settings, documenting the
configuration consumed by resolveKOverfetch in repository.ts. Preserve the
existing ordering and formatting of the related knobs.
In `@apps/game-server/src/routes/rooms.ts`:
- Around line 416-432: Update the dialogue-append handler around
appendCompletedTurn to import and apply MAX_PLAYER_MESSAGE_LEN from
`@aetherlife/shared`, rejecting requests when playerMessage or npcReply exceeds
that limit while preserving the existing required-field validation and 400
response behavior.
In `@packages/npc-memory/migrations/0013_npc_memories_halfvec_index.sql`:
- Around line 5-7: Update the migration around
npc_memories_embedding_halfvec_hnsw to retain CREATE INDEX IF NOT EXISTS as the
post-maintenance fallback, while documenting or adding the production
out-of-band CREATE INDEX CONCURRENTLY maintenance step. Validate that the
deployed pgvector version supports halfvec_cosine_ops and that queries use the
matching embedding::halfvec(2048) expression index.
In `@packages/npc-memory/src/collective/repository.test.ts`:
- Around line 128-140: Update the D-BELIEF-07 test around upsertSemanticState to
exercise the per-key omit path instead of the empty-patch early return: perform
a single-key semantic update while omitting the other fields, then assert the
omitted columns retain their prior values and the updated key changes as
expected. Keep the existing in-memory repository setup, and do not attempt to
cover the database conflictSet path in this test.
In `@packages/npc-memory/src/collective/repository.ts`:
- Around line 76-78: Replace the duplicated "平静" default with an exported
DEFAULT_NPC_MOOD constant from semanticState.ts, deriving it from the existing
NPC_MOODS whitelist. Update defaultSemanticFields, the occurrence near line 273,
and the schema column default to reference this shared constant.
- Around line 302-310: Update the conflict-set construction around hasMood,
hasBeliefs, and hasSummary to assign each patch value through a locally narrowed
variable inside its corresponding guard, such as a typed mood, beliefs, or
summary value. Use those narrowed locals for conflictSet assignments and the
final onConflictDoUpdate set so the flow no longer relies on non-null
assertions.
In `@packages/npc-memory/src/repository.test.ts`:
- Around line 132-142: Add tests in the “resolveKOverfetch (D-ANN-03)” suite
covering a valid MEMORY_K_OVERFETCH_CAP override and invalid or less-than-one
values falling back to the default cap, restoring the environment after each
case. Update the large-k expectation only if the repository implementation
changes its cap behavior, preserving the intended result for k exceeding the
configured cap.
In `@packages/npc-memory/src/repository.ts`:
- Around line 130-145: Align the forgetting-curve scoring paths by either
routing searchSimilar through rerankCandidatesByForgettingCurve using selected
distance and creation-time data, or adding an integration test that executes and
validates the actual SQL scoring expression. Update the D-DECAY-04 parity
coverage so it verifies database results rather than only comparing TypeScript
with a hand-written SQL mirror, and keep the verification script consistent with
the single authoritative formula.
In `@packages/npc-memory/src/schema.test.ts`:
- Around line 51-61: The migration test for 0013 currently enforces a locking
index build without documenting the production alternative. Update the test or
nearby migration documentation to describe the manual/out-of-band production
procedure for building the index concurrently, while preserving the existing
non-CONCURRENTLY assertion for the transaction-wrapped migration.
In `@packages/shared/src/relationshipPropagation.ts`:
- Around line 21-44: Update propagationPolarityForBaseTag to use the canonical
relationship metadata or shared initial-affection mapping as its source of truth
instead of maintaining incomplete local tag sets. Ensure kinds such as trade,
chess, frenemy, and peer receive the polarity defined by that canonical source,
while unknown tags continue returning 0.
In `@scripts/uat-phase29-dialogue-restart.mjs`:
- Around line 215-219: Replace the fixed 1500ms delay before evictDialogueMap in
the dialogue restart flow with bounded polling that repeatedly fetches turns and
waits until the seed appears, or retries the post-eviction fetch using the
existing assertion behavior. Ensure the polling has a timeout or maximum
attempts so failures remain explicit rather than hanging.
In `@scripts/verify-memory-recall.mjs`:
- Around line 47-54: Update verify-memory-recall’s resolveKOverfetch and
recency-default usage to import and reuse the exported helper/constants from
`@aetherlife/npc-memory`, removing the duplicated 72h, 0.3, and 1e-3 values. Keep
only the gate-specific curve SQL local, and preserve the existing environment
override behavior.
In `@scripts/verify-pgvector-capability.mjs`:
- Around line 16-33: Extract the shared loadEnv function from
verify-pgvector-capability.mjs into a reusable module under scripts/lib/, then
update this script and the other verify scripts that duplicate it to import and
reuse that module. Preserve the existing missing-file error behavior and parsing
semantics without adding dependencies.
🪄 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: 04e2d5fb-4696-4511-b6c5-1f5f27a990e6
📒 Files selected for processing (51)
.env.exampleREADME.mdREADME.zh-CN.mdapps/ai-gateway/app/guards/reply.pyapps/ai-gateway/tests/test_check_reply.pyapps/game-server/src/collective/service.test.tsapps/game-server/src/collective/service.tsapps/game-server/src/colyseus/npc-chat.tsapps/game-server/src/memory/service.test.tsapps/game-server/src/memory/service.tsapps/game-server/src/npc/dialogue-session.test.tsapps/game-server/src/npc/dialogue-session.tsapps/game-server/src/routes/collective-state.tsapps/game-server/src/routes/internal-memories.tsapps/game-server/src/routes/rooms.tsdocs/ARCHITECTURE.mddocs/CONTRACTS.mddocs/ISSUE-LOG.mddocs/LLM-E2E-FLOW-AND-LATENCY.mdpackage.jsonpackages/npc-memory/migrations/0013_npc_memories_halfvec_index.sqlpackages/npc-memory/migrations/0014_npc_attitudes_semantic_state.sqlpackages/npc-memory/migrations/meta/_journal.jsonpackages/npc-memory/src/collective/repository.test.tspackages/npc-memory/src/collective/repository.tspackages/npc-memory/src/index.tspackages/npc-memory/src/repository.test.tspackages/npc-memory/src/repository.tspackages/npc-memory/src/schema.test.tspackages/npc-memory/src/schema.tspackages/shared/src/council/semanticState.test.tspackages/shared/src/council/semanticState.tspackages/shared/src/index.tspackages/shared/src/relationshipPropagation.test.tspackages/shared/src/relationshipPropagation.tsscripts/benchmark-speak-browser.mjsscripts/uat-phase29-dialogue-restart.mjsscripts/verify-memory-recall.mjsscripts/verify-pgvector-capability.mjsworkers/agent-worker/src/graph/npc_loop.pyworkers/agent-worker/src/graph/prompt.pyworkers/agent-worker/src/graph/reflect.pyworkers/agent-worker/src/graph/speak_fetch.pyworkers/agent-worker/src/graph/speak_system_context.pyworkers/agent-worker/src/graph/state.pyworkers/agent-worker/src/guard/reply_audit.pyworkers/agent-worker/src/memory/client.pyworkers/agent-worker/tests/test_prompt_attitude.pyworkers/agent-worker/tests/test_reflect_summarize.pyworkers/agent-worker/tests/test_reply_audit.pyworkers/agent-worker/tests/test_speak_system_context.py
| STATE_CHANGE_PATTERNS = [ | ||
| re.compile(r"\b(opened|closed|moved|picked up|picked|walked|went to|entered|left)\b", re.I), | ||
| re.compile(r"(打开了|关闭了|移动到|走向|拿起了|捡起了|去把门打开|就去打开|把门打开|我现在就去)", re.I), | ||
| re.compile(r"\b(opened|closed|picked up|walked|went to|entered|moved to)\b", re.I), | ||
| re.compile( | ||
| r"(打开了|关闭了|移动到|走向了|走向门|拿起了|捡起了|去把门打开|就去打开|把门打开|我现在就去(?:把门|打开|拿|捡|走|过去))", | ||
| re.I, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constrain duplicated state-change patterns to affirmative physical actions.
Both guards can replace legitimate dialogue with the fallback because unanchored phrases match figurative, interrogative, and negated text.
apps/ai-gateway/app/guards/reply.py#L10-L15: refine the English/Chinese alternatives and add question/negation regression cases.workers/agent-worker/src/guard/reply_audit.py#L8-L11: apply the identical refined patterns and tests.
📍 Affects 2 files
apps/ai-gateway/app/guards/reply.py#L10-L15(this comment)workers/agent-worker/src/guard/reply_audit.py#L8-L11
🤖 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/ai-gateway/app/guards/reply.py` around lines 10 - 15, Constrain the
duplicated state-change regex alternatives to affirmative physical actions,
preventing matches within figurative, interrogative, or negated dialogue while
preserving valid action detection. Apply the identical refined English and
Chinese patterns in apps/ai-gateway/app/guards/reply.py lines 10-15 and
workers/agent-worker/src/guard/reply_audit.py lines 8-11, and add regression
tests covering question and negation cases at both sites.
| it("Redis errors are logged and never thrown to caller", async () => { | ||
| const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); | ||
| const broken = { | ||
| on: vi.fn(), | ||
| rpush: vi.fn().mockRejectedValue(new Error("redis down")), | ||
| ltrim: vi.fn(), | ||
| expire: vi.fn(), | ||
| lrange: vi.fn().mockRejectedValue(new Error("redis down")), | ||
| del: vi.fn().mockRejectedValue(new Error("redis down")), | ||
| }; | ||
| setDialogueRedisForTests(broken as unknown as import("ioredis").default); | ||
|
|
||
| expect(() => | ||
| appendCompletedTurn({ | ||
| roomId: "default", | ||
| playerId: "p1", | ||
| npcId: "npc-1", | ||
| playerMessage: "a", | ||
| npcReply: "b", | ||
| }), | ||
| ).not.toThrow(); | ||
|
|
||
| await expect(getRecentTurnsAsync("default", "p1", "npc-1")).resolves.toEqual([ | ||
| { role: "player", text: "a" }, | ||
| { role: "npc", text: "b" }, | ||
| ]); | ||
|
|
||
| // Force Map miss for async rehydrate error path | ||
| clearDialogueSessions(); | ||
| setDialogueRedisForTests(broken as unknown as import("ioredis").default); | ||
| await expect(getRecentTurnsAsync("default", "p1", "npc-1")).resolves.toEqual([]); | ||
|
|
||
| expect(() => clearDialogueForPlayer("default", "p1")).not.toThrow(); | ||
| errSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test claims errors are "logged" but never asserts on the spy.
errSpy is only used to silence output. Add an assertion so the logging half of the contract is actually covered.
💚 Proposed assertion
expect(() => clearDialogueForPlayer("default", "p1")).not.toThrow();
+ await vi.waitFor(() => expect(errSpy).toHaveBeenCalled());
errSpy.mockRestore();📝 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.
| it("Redis errors are logged and never thrown to caller", async () => { | |
| const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); | |
| const broken = { | |
| on: vi.fn(), | |
| rpush: vi.fn().mockRejectedValue(new Error("redis down")), | |
| ltrim: vi.fn(), | |
| expire: vi.fn(), | |
| lrange: vi.fn().mockRejectedValue(new Error("redis down")), | |
| del: vi.fn().mockRejectedValue(new Error("redis down")), | |
| }; | |
| setDialogueRedisForTests(broken as unknown as import("ioredis").default); | |
| expect(() => | |
| appendCompletedTurn({ | |
| roomId: "default", | |
| playerId: "p1", | |
| npcId: "npc-1", | |
| playerMessage: "a", | |
| npcReply: "b", | |
| }), | |
| ).not.toThrow(); | |
| await expect(getRecentTurnsAsync("default", "p1", "npc-1")).resolves.toEqual([ | |
| { role: "player", text: "a" }, | |
| { role: "npc", text: "b" }, | |
| ]); | |
| // Force Map miss for async rehydrate error path | |
| clearDialogueSessions(); | |
| setDialogueRedisForTests(broken as unknown as import("ioredis").default); | |
| await expect(getRecentTurnsAsync("default", "p1", "npc-1")).resolves.toEqual([]); | |
| expect(() => clearDialogueForPlayer("default", "p1")).not.toThrow(); | |
| errSpy.mockRestore(); | |
| }); | |
| it("Redis errors are logged and never thrown to caller", async () => { | |
| const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); | |
| const broken = { | |
| on: vi.fn(), | |
| rpush: vi.fn().mockRejectedValue(new Error("redis down")), | |
| ltrim: vi.fn(), | |
| expire: vi.fn(), | |
| lrange: vi.fn().mockRejectedValue(new Error("redis down")), | |
| del: vi.fn().mockRejectedValue(new Error("redis down")), | |
| }; | |
| setDialogueRedisForTests(broken as unknown as import("ioredis").default); | |
| expect(() => | |
| appendCompletedTurn({ | |
| roomId: "default", | |
| playerId: "p1", | |
| npcId: "npc-1", | |
| playerMessage: "a", | |
| npcReply: "b", | |
| }), | |
| ).not.toThrow(); | |
| await expect(getRecentTurnsAsync("default", "p1", "npc-1")).resolves.toEqual([ | |
| { role: "player", text: "a" }, | |
| { role: "npc", text: "b" }, | |
| ]); | |
| // Force Map miss for async rehydrate error path | |
| clearDialogueSessions(); | |
| setDialogueRedisForTests(broken as unknown as import("ioredis").default); | |
| await expect(getRecentTurnsAsync("default", "p1", "npc-1")).resolves.toEqual([]); | |
| expect(() => clearDialogueForPlayer("default", "p1")).not.toThrow(); | |
| await vi.waitFor(() => expect(errSpy).toHaveBeenCalled()); | |
| errSpy.mockRestore(); | |
| }); |
🤖 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/npc/dialogue-session.test.ts` around lines 279 - 313,
Update the “Redis errors are logged and never thrown to caller” test to assert
that the console.error spy records calls after the failing Redis operations.
Keep the existing no-throw and fallback-result assertions, and ensure the
assertion uses the errSpy created in that test before restoring it.
| ALTER TABLE npc_attitudes | ||
| ADD COLUMN IF NOT EXISTS current_mood text DEFAULT '平静'; | ||
| ALTER TABLE npc_attitudes | ||
| ADD COLUMN IF NOT EXISTS key_beliefs jsonb DEFAULT '[]'::jsonb; | ||
| ALTER TABLE npc_attitudes | ||
| ADD COLUMN IF NOT EXISTS summary text DEFAULT ''; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the mood whitelist contains the SQL default and check drizzle schema nullability
fd -t f 'semanticState.ts' --exec cat -n
rg -n -C4 'currentMood|current_mood|keyBeliefs|key_beliefs' packages/npc-memory/src/schema.ts packages/npc-memory/src/collective/repository.tsRepository: moyunzero/AetherLife
Length of output: 12700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migration files around npc_attitudes =="
rg -n "ADD COLUMN IF NOT EXISTS|current_mood|key_beliefs|summary|npc_attitudes" packages/npc-memory/migrations -C 3
echo
echo "== schema full npc_attitudes definition =="
sed -n '100,122p' packages/npc-memory/src/schema.ts | cat -nRepository: moyunzero/AetherLife
Length of output: 7238
Keep the migration nullability in sync with schema.ts.
current_mood, key_beliefs, and summary are declared notNull() in packages/npc-memory/src/schema.ts, but this migration adds them nullable. Make these columns NOT NULL once the initial defaults are applied so partial upserts cannot persist 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 `@packages/npc-memory/migrations/0014_npc_attitudes_semantic_state.sql` around
lines 3 - 8, Update the migration’s ALTER TABLE statements for current_mood,
key_beliefs, and summary to apply their defaults and enforce NOT NULL, matching
the notNull() declarations in schema.ts and preventing NULL values during
partial upserts.
| for (const edge of input.edges) { | ||
| const other = | ||
| edge.npcAId === input.targetNpcId | ||
| ? edge.npcBId | ||
| : edge.npcBId === input.targetNpcId | ||
| ? edge.npcAId | ||
| : null; | ||
| if (!other || other === input.targetNpcId) continue; | ||
| if (input.alreadyUpdated.has(other)) continue; | ||
| if (Math.abs(edge.affection) < PROPAGATION_MIN_AFFECTION) continue; | ||
|
|
||
| const polarity = propagationPolarityForBaseTag(edge.baseTag); | ||
| if (polarity === 0) continue; | ||
|
|
||
| candidates.push({ | ||
| npcId: other, | ||
| polarity, | ||
| absAffection: Math.abs(edge.affection), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Candidates are not deduplicated by NPC id.
If edges ever contains two rows touching the same pair (mirrored npcAId/npcBId rows, or multiple edges with different baseTags), the same other NPC is pushed twice and the consumer applies applyReputationDelta once per update (apps/game-server/src/collective/service.ts lines 141-149), doubling the propagated delta. Keeping the highest-|affection| candidate per NPC makes the function robust regardless of edge-store shape.
🛡️ Proposed dedupe by npcId
- type Candidate = { npcId: string; polarity: 1 | -1; absAffection: number };
- const candidates: Candidate[] = [];
+ type Candidate = { npcId: string; polarity: 1 | -1; absAffection: number };
+ const byNpcId = new Map<string, Candidate>();
@@
- candidates.push({
- npcId: other,
- polarity,
- absAffection: Math.abs(edge.affection),
- });
+ const candidate: Candidate = {
+ npcId: other,
+ polarity,
+ absAffection: Math.abs(edge.affection),
+ };
+ const prev = byNpcId.get(other);
+ if (!prev || candidate.absAffection > prev.absAffection) {
+ byNpcId.set(other, candidate);
+ }
}
- candidates.sort((a, b) => b.absAffection - a.absAffection);
+ const candidates = [...byNpcId.values()].sort((a, b) => b.absAffection - a.absAffection);📝 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.
| for (const edge of input.edges) { | |
| const other = | |
| edge.npcAId === input.targetNpcId | |
| ? edge.npcBId | |
| : edge.npcBId === input.targetNpcId | |
| ? edge.npcAId | |
| : null; | |
| if (!other || other === input.targetNpcId) continue; | |
| if (input.alreadyUpdated.has(other)) continue; | |
| if (Math.abs(edge.affection) < PROPAGATION_MIN_AFFECTION) continue; | |
| const polarity = propagationPolarityForBaseTag(edge.baseTag); | |
| if (polarity === 0) continue; | |
| candidates.push({ | |
| npcId: other, | |
| polarity, | |
| absAffection: Math.abs(edge.affection), | |
| }); | |
| } | |
| type Candidate = { npcId: string; polarity: 1 | -1; absAffection: number }; | |
| const byNpcId = new Map<string, Candidate>(); | |
| for (const edge of input.edges) { | |
| const other = | |
| edge.npcAId === input.targetNpcId | |
| ? edge.npcBId | |
| : edge.npcBId === input.targetNpcId | |
| ? edge.npcAId | |
| : null; | |
| if (!other || other === input.targetNpcId) continue; | |
| if (input.alreadyUpdated.has(other)) continue; | |
| if (Math.abs(edge.affection) < PROPAGATION_MIN_AFFECTION) continue; | |
| const polarity = propagationPolarityForBaseTag(edge.baseTag); | |
| if (polarity === 0) continue; | |
| const candidate: Candidate = { | |
| npcId: other, | |
| polarity, | |
| absAffection: Math.abs(edge.affection), | |
| }; | |
| const prev = byNpcId.get(other); | |
| if (!prev || candidate.absAffection > prev.absAffection) { | |
| byNpcId.set(other, candidate); | |
| } | |
| } | |
| const candidates = [...byNpcId.values()].sort((a, b) => b.absAffection - a.absAffection); |
🤖 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/relationshipPropagation.ts` around lines 78 - 97,
Deduplicate candidates by NPC ID in the edge-processing loop, retaining only the
candidate with the highest absAffection for each other NPC. Update the candidate
collection logic in the surrounding relationship propagation function so
mirrored or multiple baseTag edges cannot produce repeated updates, while
preserving the existing filtering and polarity behavior.
| const WEB_BASE = process.env.WEB_URL || "http://localhost:5173"; | ||
| const GS = process.env.GAME_SERVER_URL || "http://127.0.0.1:2567"; | ||
| const UAT_NPC_ID = process.env.UAT_PHASE29_NPC_ID || "npc-5"; | ||
| const UAT_ROOM_ID = | ||
| process.env.UAT_PHASE29_ROOM_ID || `uat-p29-restart-${Date.now()}`; | ||
| const UAT_PLAYER_ID = | ||
| process.env.UAT_PHASE29_PLAYER_ID || `uatp29${String(Date.now()).slice(-10)}`; | ||
| const WEB = `${WEB_BASE}${WEB_BASE.includes("?") ? "&" : "?"}room=${encodeURIComponent(UAT_ROOM_ID)}`; | ||
|
|
||
| const SPEAK_WAIT_TIMEOUT_MS = Number(process.env.UAT_SPEAK_WAIT_TIMEOUT_MS || 180_000); | ||
| const ENGAGE_TIMEOUT_MS = Number(process.env.UAT_SPEAK_ENGAGE_TIMEOUT_MS || 90_000); | ||
| const SKIP_SPEAK_RECALL = process.env.UAT_PHASE29_SKIP_SPEAK === "1"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ESLint config coverage for scripts/*.mjs (globals + ignores)
fd -H -t f -e js -e mjs -e cjs -e json -e yaml --full-path -g '*eslint*' .
fd -H -t f --full-path -g '*eslint*' . --exec rg -n 'scripts|globals|node|ignores' {}Repository: moyunzero/AetherLife
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- files named eslint/eslint.config.* or package scripts ---\n'
git ls-files | grep -E '(^|/)(eslint\.config\.|.*eslint.*|package\.json)$' || true
printf '\n--- package scripts ---\n'
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts}, null,2))"
else
fd -a 'package\.json$' . -d 2 | while read -r f; do echo "### $f"; node -e "const p=require('$f'); console.log(JSON.stringify({scripts:p.scripts}, null,2))" 2>/dev/null || true; done
fi
printf '\n--- scripts/uat-phase29-dialogue-restart.mjs (top/bottom) ---\n'
wc -l scripts/uat-phase29-dialogue-restart.mjs || true
sed -n '1,80p' scripts/uat-phase29-dialogue-restart.mjs
sed -n '80,$p' scripts/uat-phase29-dialogue-restart.mjs | head -120
printf '\n--- all sibling scripts listing ---\n'
fd -t f . scripts/uat 2>/dev/null | sort || trueRepository: moyunzero/AetherLife
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files named eslint/eslint.config.* or package scripts ---'
git ls-files | grep -E '(^|/)(eslint\.config\.|.*eslint.*|package\.json)$' || true
printf '%s\n' '--- package scripts ---'
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts}, null,2))"
else
fd -a 'package\.json$' . -d 2 | while read -r f; do echo "### $f"; node -e "const p=require('$f'); console.log(JSON.stringify({scripts:p.scripts}, null,2))" 2>/dev/null || true; done
fi
printf '%s\n' '--- scripts/uat-phase29-dialogue-restart.mjs (top/bottom) ---'
wc -l scripts/uat-phase29-dialogue-restart.mjs || true
sed -n '1,80p' scripts/uat-phase29-dialogue-restart.mjs
sed -n '80,$p' scripts/uat-phase29-dialogue-restart.mjs | head -120
printf '%s\n' '--- all sibling scripts listing ---'
fd -t f . scripts/uat 2>/dev/null | sort || trueRepository: moyunzero/AetherLife
Length of output: 14988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- eslint.config.js ---'
sed -n '1,260p' eslint.config.js
printf '%s\n' '--- AGENTS lint/config mentions ---'
if [ -f AGENTS.md ]; then
rg -n 'AGENTS|eslint|lint|ESLint|scripts|node globals|globals' AGENTS.md || true
fiRepository: moyunzero/AetherLife
Length of output: 2082
Run UAT scripts through ESLint with Node globals enabled.
scripts/uat-phase29-dialogue-restart.mjs uses Node globals like process, console, fetch, and AbortSignal, but eslint.config.js only declares globals for a small explicit file list, and pnpm lint still only lints scripts/lib scripts/agent-verify.mjs. Add this UAT script (or the UAT script pattern) to the config/listing or update the lint script; otherwise future UAT changes can fail lint despite running under Node.
🧰 Tools
🪛 ESLint
[error] 33-33: 'process' is not defined.
(no-undef)
[error] 34-34: 'process' is not defined.
(no-undef)
[error] 35-35: 'process' is not defined.
(no-undef)
[error] 37-37: 'process' is not defined.
(no-undef)
[error] 39-39: 'process' is not defined.
(no-undef)
[error] 42-42: 'process' is not defined.
(no-undef)
[error] 43-43: 'process' is not defined.
(no-undef)
[error] 44-44: 'process' is not defined.
(no-undef)
🤖 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-phase29-dialogue-restart.mjs` around lines 33 - 44, Update the
ESLint configuration and/or package lint command so
scripts/uat-phase29-dialogue-restart.mjs is linted with Node globals enabled,
including process, console, fetch, and AbortSignal. Prefer extending the
existing UAT script pattern or explicit script list, and ensure pnpm lint
includes the script.
Source: Linters/SAST tools
| if (!existsSync(path)) { | ||
| console.error("Missing .env — copy .env.example and fill DATABASE_URL."); | ||
| process.exit(1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
ESLint reports console/process as undefined in this new script.
Lint flags every console/process use here (no-undef), so the file will fail the lint gate. The ESLint config needs Node globals for scripts/**/*.mjs (same problem in scripts/verify-pgvector-capability.mjs). See the consolidated note.
🧰 Tools
🪛 ESLint
[error] 27-27: 'console' is not defined.
(no-undef)
[error] 28-28: 'process' is not defined.
(no-undef)
🤖 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-memory-recall.mjs` around lines 26 - 28, Update the ESLint
configuration for scripts/**/*.mjs to declare Node.js globals, including console
and process, so verify-memory-recall.mjs and verify-pgvector-capability.mjs pass
no-undef without changing their runtime logic.
| function planLooksLikeAnn(planText) { | ||
| const lower = planText.toLowerCase(); | ||
| return ( | ||
| lower.includes("index scan") || | ||
| lower.includes("index only scan") || | ||
| lower.includes("bitmap index scan") || | ||
| lower.includes("hnsw") || | ||
| lower.includes("ann") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
lower.includes("ann") makes the D-ANN-03 gate falsely pass.
"ann" is a substring of ordinary plan words (cannot, planning), so a plain Seq Scan plan can satisfy the check. Also, EXPLAIN on a Seq Scan plan containing e.g. Bitmap Index Scan on room_id would pass without touching the HNSW index at all — match the index name/operator instead of loose keywords.
🐛 Suggested tightening
function planLooksLikeAnn(planText) {
const lower = planText.toLowerCase();
return (
- lower.includes("index scan") ||
- lower.includes("index only scan") ||
- lower.includes("bitmap index scan") ||
- lower.includes("hnsw") ||
- lower.includes("ann")
+ lower.includes("npc_memories_embedding_halfvec_hnsw") ||
+ lower.includes("hnsw")
);
}📝 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.
| function planLooksLikeAnn(planText) { | |
| const lower = planText.toLowerCase(); | |
| return ( | |
| lower.includes("index scan") || | |
| lower.includes("index only scan") || | |
| lower.includes("bitmap index scan") || | |
| lower.includes("hnsw") || | |
| lower.includes("ann") | |
| ); | |
| } | |
| function planLooksLikeAnn(planText) { | |
| const lower = planText.toLowerCase(); | |
| return ( | |
| lower.includes("npc_memories_embedding_halfvec_hnsw") || | |
| lower.includes("hnsw") | |
| ); | |
| } |
🤖 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-memory-recall.mjs` around lines 59 - 68, Update
planLooksLikeAnn to remove the loose lower.includes("ann") check and require
evidence of the intended HNSW index or ANN operator instead. Ensure unrelated
plan text, including Seq Scan plans and Bitmap Index Scan on room_id, cannot
satisfy the D-ANN-03 gate; preserve only matches that confirm the expected HNSW
path.
Align NULL importance to 5 in forgetting-curve SQL; overfetch relationship edges past FANOUT before filter; skip Map clobber/empty negative cache on dialogue Redis rehydrate. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/game-server/src/npc/dialogue-session.ts (2)
190-197: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDelete persisted turns beyond this process’s key cache.
Line 191 only sees keys observed since this process started. After a restart—or when another instance wrote the turns—
clearDialogueForPlayerleaves Redis transcripts until TTL expiry. Maintain a durable player-thread index, or safely enumerate that exact player namespace before deletion.🤖 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/npc/dialogue-session.ts` around lines 190 - 197, Update clearDialogueForPlayer to remove all persisted dialogue-turn keys for the player, not only entries present in knownRedisKeys. Use a durable player-thread index or safely enumerate the exact redisPrefix namespace before calling deleteRedisKeys, while preserving the existing in-memory cache cleanup.
36-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound Redis rehydration failures.
maxRetriesPerRequest: nulldisables ioredis’s per-command reconnect retry limit, so a Map miss that queuesLRANGEcan remain pending until Redis reconnects. Add a finite command timeout/retry budget here consistent with the NPC chat latency budget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/game-server/src/npc/dialogue-session.ts` at line 36, Update the Redis initialization in the dialogue session rehydration path to replace maxRetriesPerRequest: null with a finite retry/timeout budget aligned with the NPC chat latency budget, ensuring queued LRANGE commands fail promptly when Redis is unavailable.apps/game-server/src/collective/service.ts (1)
196-202: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRelationship propagation can leave event recording partially committed. Both paths perform propagation writes only after the event and witness updates are persisted; a rejected propagation write can surface as a failed operation despite those earlier writes succeeding.
apps/game-server/src/collective/service.ts#L196-L202: make rule-event propagation transactional or idempotent and non-failing to the primary event path.apps/game-server/src/collective/service.ts#L246-L252: apply the same failure-isolation strategy to worker-event propagation.🤖 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/collective/service.ts` around lines 196 - 202, Isolate relationship propagation failures so primary event recording remains successful after event and witness persistence. Update rule-event propagation at apps/game-server/src/collective/service.ts:196-202 and worker-event propagation at apps/game-server/src/collective/service.ts:246-252 to use a transactional or idempotent non-failing strategy, ensuring rejected propagation writes do not surface as operation failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/game-server/src/npc/dialogue-session.test.ts`:
- Around line 252-256: Update the fake.lrange implementation in the
dialogue-session test to copy the current list from fake.store before awaiting
gate, then return that snapshot afterward. Preserve the gate synchronization
while ensuring the simulated Redis read remains stale even if
appendCompletedTurn executes during the wait.
In `@packages/npc-memory/src/repository.ts`:
- Line 231: Normalize NULL importance to 5 consistently in the searchSimilar SQL
projection or result mapper, so returned importance matches the score
calculation. In packages/npc-memory/src/repository.ts lines 231-231, update the
relevant CTE/final projection or mapper; in
packages/npc-memory/src/repository.test.ts lines 131-143, add a searchSimilar
test using an actual NULL importance and assert both returned importance and
score use the default.
---
Outside diff comments:
In `@apps/game-server/src/collective/service.ts`:
- Around line 196-202: Isolate relationship propagation failures so primary
event recording remains successful after event and witness persistence. Update
rule-event propagation at apps/game-server/src/collective/service.ts:196-202 and
worker-event propagation at apps/game-server/src/collective/service.ts:246-252
to use a transactional or idempotent non-failing strategy, ensuring rejected
propagation writes do not surface as operation failures.
In `@apps/game-server/src/npc/dialogue-session.ts`:
- Around line 190-197: Update clearDialogueForPlayer to remove all persisted
dialogue-turn keys for the player, not only entries present in knownRedisKeys.
Use a durable player-thread index or safely enumerate the exact redisPrefix
namespace before calling deleteRedisKeys, while preserving the existing
in-memory cache cleanup.
- Line 36: Update the Redis initialization in the dialogue session rehydration
path to replace maxRetriesPerRequest: null with a finite retry/timeout budget
aligned with the NPC chat latency budget, ensuring queued LRANGE commands fail
promptly when Redis is unavailable.
🪄 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: 680ab248-6a44-4311-93a5-4c7547fdcafd
📒 Files selected for processing (6)
apps/game-server/src/collective/service.test.tsapps/game-server/src/collective/service.tsapps/game-server/src/npc/dialogue-session.test.tsapps/game-server/src/npc/dialogue-session.tspackages/npc-memory/src/repository.test.tspackages/npc-memory/src/repository.ts
| fake.lrange = vi.fn(async (key: string) => { | ||
| await gate; | ||
| const list = fake.store.get(key) ?? []; | ||
| return [...list]; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Snapshot Redis before releasing the race gate.
The fake reads store after appendCompletedTurn runs, so it returns the new pair even if Lines 136-139 are removed. Snapshot before awaiting gate to exercise the stale-Redis overwrite path.
Proposed test fix
fake.lrange = vi.fn(async (key: string) => {
+ const list = [...(fake.store.get(key) ?? [])];
await gate;
- const list = fake.store.get(key) ?? [];
- return [...list];
+ return list;
});📝 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.
| fake.lrange = vi.fn(async (key: string) => { | |
| await gate; | |
| const list = fake.store.get(key) ?? []; | |
| return [...list]; | |
| }); | |
| fake.lrange = vi.fn(async (key: string) => { | |
| const list = [...(fake.store.get(key) ?? [])]; | |
| await gate; | |
| return list; | |
| }); |
🤖 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/npc/dialogue-session.test.ts` around lines 252 - 256,
Update the fake.lrange implementation in the dialogue-session test to copy the
current list from fake.store before awaiting gate, then return that snapshot
afterward. Preserve the gate synchronization while ensuring the simulated Redis
read remains stale even if appendCompletedTurn executes during the wait.
| * LN(2) | ||
| / GREATEST( | ||
| ${cfg.sEpsilon}::float8, | ||
| ${cfg.s0}::float8 * COALESCE(importance, 5) / 5.0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep NULL importance normalization consistent across SQL results and tests. The SQL score now treats NULL as 5, but searchSimilar can still return Number(null) === 0, while the new parity test never exercises an actual NULL value.
packages/npc-memory/src/repository.ts#L231-L231: normalizeimportanceto5in the CTE/final projection or mapper before returning it, so the returned field matches the score and downstream scoring contract.packages/npc-memory/src/repository.test.ts#L131-L143: add coverage starting from an actual NULL importance, preferably throughsearchSimilar, and assert both returned importance and score use the same default.
📍 Affects 2 files
packages/npc-memory/src/repository.ts#L231-L231(this comment)packages/npc-memory/src/repository.test.ts#L131-L143
🤖 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/repository.ts` at line 231, Normalize NULL importance
to 5 consistently in the searchSimilar SQL projection or result mapper, so
returned importance matches the score calculation. In
packages/npc-memory/src/repository.ts lines 231-231, update the relevant
CTE/final projection or mapper; in packages/npc-memory/src/repository.test.ts
lines 131-143, add a searchSimilar test using an actual NULL importance and
assert both returned importance and score use the default.
1 participant
Summary
uat:phase29:dialogue-restart), and reply-audit false-positive fix (ISSUE-112).Test plan
pnpm --filter @aetherlife/npc-memory testpnpm --filter @aetherlife/shared testpnpm --filter @aetherlife/game-server testcd workers/agent-worker && LLM_MOCK=1 uv run pytest -qcd apps/ai-gateway && uv run pytest tests -qWEB_URL=http://localhost:5173 pnpm uat:phase29:dialogue-restart(needspnpm dev:stack+ Redis; noLLM_MOCK)pnpm benchmark:speak-browser— expect D-PERF still open until memory-tail/quota follow-upKnown gap
Made with Cursor
Summary by CodeRabbit