LearnNewsExamplesServices
Frontmatter
title>-
fixMemory Core semantic search crash & active session summarization leak
authortobiu
stateMerged
createdAtApr 13, 2026, 12:26 PM
updatedAtApr 13, 2026, 12:31 PM
closedAtApr 13, 2026, 12:31 PM
mergedAtApr 13, 2026, 12:31 PM
branchesdev ← agent/9959-memory-core-query-fix
urlhttps://github.com/neomjs/neo/pull/9960
Merged
tobiu
tobiu commented on Apr 13, 2026, 12:26 PM
  • StorageRouter: wrap re-ranker Pass 1 query in try/catch with graceful fallback to empty results, add optional chaining on all ChromaDB result property accesses, add logger import
  • SessionService: move currentSessionId exclusion to top of findSessionsToSummarize loop (was only applied to Case B count mismatch, not Case A missing summary)
  • Add 7 new Playwright tests in QueryReRanker.spec.mjs covering both bugs plus ChromaDB timestamp filtering validation
tobiu
tobiu commented on Apr 13, 2026, 12:28 PM

Input from Claude Opus 4.6 (Antigravity):

❋ # PR Review Summary

Status: Approved

Self-review via the pr-review skill. Stepping back from "Implementer" to "Navigator" mode.

📊 Evaluation Metrics

  • [ARCH_ALIGNMENT]: 90 - Both fixes respect the existing singleton service architecture. The try/catch fallback returns a ChromaDB-shaped empty result {ids: [[]], distances: [[]], metadatas: [[]]}, which is the idiomatic contract for downstream consumers. The currentSessionId early-return pattern follows established Iterator/Guard patterns in SessionService.
  • [CONTENT_COMPLETENESS]: 80 - The inline comments are adequate and explain the why (not just the what). However, the StorageRouter class-level JSDoc was not updated to document the new error-resilience behavior of the re-ranker. Minor gap.
  • [EXECUTION_QUALITY]: 95 - Both bugs were reproduced test-first before fixing. 7 targeted tests covering empty results, API failures, end-to-end query paths, drift detection, active session exclusion, and ChromaDB $gt timestamp filtering. All 18/18 memory-core tests pass. Live MCP server verified post-restart.
  • [PRODUCTIVITY]: 100 - All ticket goals achieved: semantic search restored, active session leak plugged, tests added.
  • [IMPACT]: 85 - These bugs rendered the entire query_raw_memories and query_summaries MCP tools non-functional during embedding provider failures, and wasted summarization tokens on incomplete sessions. Direct impact on agent continuity and token budget.
  • [COMPLEXITY]: 35 - Surgical 2-file fix plus a new test file. Low cognitive load, high diagnostic accuracy.
  • [EFFORT_PROFILE]: Quick Win - High impact (broken semantic search + token waste) resolved with minimal code churn. Classic defensive programming fix.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #9959
  • Related Graph Nodes: memory-core-mcp-server, StorageRouter, SessionService, DreamService

🧠 Graph Ingestion Notes

  • [KB_GAP]: The StorageRouter.injectQueryReRanker() method lacked documentation about error handling expectations. The monkey-patching pattern implicitly assumed that the underlying originalQuery would always succeed — a fragile assumption when the backing embedding function delegates to an external API (Gemini). The KB should document that all ChromaDB query wrappers must be error-resilient since embedding functions are the most common failure point.

  • [TOOLING_GAP]: The dynamic_text_embedding_service ChromaDB warning spam in test output is noisy but cosmetic (16 warnings per test run). This is a known ChromaDB schema deserialization quirk when custom embedding functions are registered. Not actionable for this PR but worth noting for future test harness improvements.

  • [RETROSPECTIVE]: The test-first methodology proved its value here — the initial test run revealed an additional bug (active session exclusion in Case A) that wasn't part of the original diagnosis. The SessionService.findSessionsToSummarize() logic had a subtle asymmetry where Case A (missing summary) and Case B (count mismatch) applied different exclusion rules. Without the test explicitly injecting memories for the currentSessionId, this secondary bug would have remained latent. This reinforces that behavioral tests should always explicitly exercise boundary conditions (e.g., "what happens when the Actor IS the Subject?").

📋 Required Actions

No blocking actions. Minor polish opportunities:

  • (Optional) Add @summary JSDoc tag to injectQueryReRanker documenting the try/catch fallback behavior
  • (Optional) Consider a follow-up ticket to suppress or redirect the dynamic_text_embedding_service ChromaDB warnings in test output

[Ready for human QA merge.]