Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 14, 2026, 2:15 PM |
| updatedAt | Jun 14, 2026, 2:35 PM |
| closedAt | Jun 14, 2026, 2:35 PM |
| mergedAt | Jun 14, 2026, 2:35 PM |
| branches | dev ← fix/query-include-distances-13222 |
| url | https://github.com/neomjs/neo/pull/13223 |
Merged
neo-opus-ada
APPROVED reviewed on Jun 14, 2026, 2:27 PM
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: This is the right small fix for the observed Memory Core score-collapse path: the services were explicitly suppressing
distances, whileStorageRouter.injectQueryReRanker()consumes those distances as the semantic component. The PR restores the missing input and adds deterministic regression guards on the exact include payload rather than depending on seeded Chroma behavior.
Peer-Review Opening: This is a tight correction. I’m approving because the source path, Chroma client contract, focused tests, and CI all line up.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Issue #13222 body and Contract Ledger; PR changed-file list; current
origin/devsource forMemoryService.mjs,SummaryService.mjs,StorageRouter.mjs, andQueryReRanker.spec.mjs; installedchromadbclient include/default behavior; Memory Core mining for the score-reporting / #12450 context. - Expected Solution Shape: Correct behavior should preserve the existing query/re-ranker pipeline and only ensure the service-level explicit include arrays request the vector distances that downstream scoring already expects. It should not hardcode topology weights, rewrite the re-ranker, or require live seeded Chroma data for a unit regression guard. Test isolation should capture the query arguments sent by each service path.
- Patch Verdict: Matches. The diff adds
'distances'to both service include arrays, leavesStorageRouterranking semantics unchanged, and tests both raw-memory and summary query include payloads.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #13222
- Related Graph Nodes: #12450, #12828, #12830
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The PR intentionally diverges from the ticket’s sketched behavioral assertion and uses include-capture tests instead. I checked this against the failure mode and accept it: the contract to guard is “the services must request
distances,” and the live/seeded Chroma behavioral cases are already environment-sensitive in this spec. The local full-spec control run onorigin/devreproduces the same substrate-dependent failures, while the new #13222 guards pass.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates. The claim is not “all summary vector corruption fixed”; it stays scoped to score-reporting / semantic-signal include loss.
- Anchor & Echo summaries: inline comments are specific to the load-bearing include and do not overstate the fix.
-
[RETROSPECTIVE]tag: N/A — no review tag in author body. - Linked anchors: #12450 / #12828 / #12830 are used as scope boundaries, not borrowed authority.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A.[TOOLING_GAP]: Local full-fileQueryReRanker.spec.mjsstill depends on seeded/queryable Chroma state for older tests; this PR avoids that for the new regression guards, and the same failures reproduce onorigin/dev.[RETROSPECTIVE]: Small explicit Chromaincludearrays can be semantic-ranking inputs, not just response-shape optimization. Guard the exact include when downstream code treats the omitted field as a meaningful score.
🎯 Close-Target Audit
- Close-targets identified: #13222
- #13222 labels checked:
bug,ai,regression,architecture,model-experience; notepic. - Branch commit history checked with
git log origin/dev..HEAD --format=%h%x09%s%n%b; no stale magic-close body targets beyond the valid subject ticket reference.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix.
- Implemented PR diff matches the intended contract:
query_raw_memoriesandquery_summariesnow request distances, andStorageRouterreceives the semantic-score input it already consumes.
Findings: Pass. The test-shape delta is acceptable because it guards the exact service contract deterministically while CI covers the broader unit/integration surface.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is adequate for the close target: L2 include-capture tests directly verify both service paths request
distances. - No residuals claimed; no operator-only validation is required for this leaf.
- Evidence-class collapse check passed: PR body keeps post-merge live-score variation as validation, not as already-proven local evidence.
Findings: Pass.
N/A Audits — 📡 🛂 🔗
N/A across listed dimensions: the PR does not touch OpenAPI tool descriptions, introduce a major new architecture/provenance surface, or require cross-skill workflow integration.
🧪 Test-Execution & Location Audit
- Branch checked out locally via
gh pr checkout 13223. - Exact head verified: local
HEADequals GitHub head576c34b3455722a8486ba3535c5f6c304e623187. - Canonical location: modified tests remain under
test/playwright/unit/ai/services/memory-core/. - Related focused tests run:
npm run test-unit -- test/playwright/unit/ai/services/memory-core/QueryReRanker.spec.mjs -g "#13222"→ 2 passed. - Full touched spec attempted locally:
npm run test-unit -- test/playwright/unit/ai/services/memory-core/QueryReRanker.spec.mjsfailed in three pre-existing substrate-dependent tests; the same three failures reproduce on detachedorigin/dev, so they are not introduced by this PR. - CI status checked current-head: lint-pr-body, retired primitive check, unit, integration-unified, and CodeQL are green.
Findings: Pass with local substrate caveat documented.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 - I actively considered re-ranker changes, service-level response-shape changes, and live-substrate test coupling; none were needed because the existing architecture already expects distances and only the explicit include arrays suppressed them.[CONTENT_COMPLETENESS]: 95 - 5 points deducted because the PR body carries the necessary delta rationale rather than updating the ticket ledger’s Evidence cell; acceptable here because the public contract itself did not drift.[EXECUTION_QUALITY]: 95 - 5 points deducted for local full-spec environmental failures remaining in adjacent pre-existing tests; the PR’s focused guards pass and the failures reproduce onorigin/dev.[PRODUCTIVITY]: 100 - Restores the missing semantic-score input for both raw-memory and summary query paths with minimal code and direct regression coverage.[IMPACT]: 65 - Moderate-high: a two-line service fix restores meaningful ranking scores for Memory Core query consumers, but it is localized and not a new subsystem.[COMPLEXITY]: 25 - Low: two include-array edits plus isolated unit guards; the conceptual importance is higher than the mechanical change size.[EFFORT_PROFILE]: Quick Win - High value relative to a very small diff and deterministic tests.
Approved. Per §0, this is merge-eligible only; merge execution remains human-only.
Resolves #13222
query_summariesandquery_raw_memoriespassed a chromaincludethat omitted'distances', so the response carried no distance values. The Dual-Pass re-ranker (StorageRouter.injectQueryReRanker) reads those distances as its Pass-1 semantic score — with none returned,vectorDistdefaulted to0→semanticScorea constant1, silently degrading ranking to topology-only and reportingdistance:0 / relevanceScore:1on every result. Split from #12450 (the bug-b score-reporting half, independent of the summary-vector corruption tracked in #12828 / #12830).Evidence: L2 (substrate-free unit guards capture the
includeeach service sends and assert'distances'is present — 2/2 green) → L2 required (a 2-line include fix plus a regression guard the priorrelevanceScore > 0assertion could not provide). No residuals.The fix
MemoryService.mjs:1105:include: ['metadatas']→['metadatas', 'distances']SummaryService.mjs:374:include: ['metadatas', 'documents']→['metadatas', 'documents', 'distances']Restores real vector distances end-to-end: Pass-1 distances → real
semanticScore→ composite rank →relevanceScore(MemoryService.mjs:1186/SummaryService.mjs:452). A load-bearing inline comment documents why'distances'must stay in the include, so a future tidy does not strip it.Deltas from ticket
distance > 0/relevanceScore < 1). Switched to substrate-free include-assertion guards: the behavioral path needs seeded, queryable chroma data that only runs in CI's Docker MCintegration-unified— verified, the 3 pre-existingcount > 0tests in this same spec (lines 116 / 134 / 400) fail identically on the base branch with my changes stashed, so it is an environment limitation, not the fix. The include-assertion runs everywhere and deterministically guards the exact regression ('distances'present in the include), so it is the stronger guard. Both query paths covered.query_summariesrecency-fallback stays out of scope (couples to #12828 / #12830); this leaf fixes the score-reporting plus the re-ranker semantic-signal collapse.Test Evidence
QueryReRanker.spec.mjs -g "#13222": 2 passed (971ms) —queryMemoriesandquerySummarieseach assert'distances'is in the chromainclude.git stashed): the 3 pre-existing env-gatedcount > 0tests fail identically with and without my change → my change introduces no regression; those execute in CIintegration-unified(Docker MC seeded substrate).Post-Merge Validation
query_raw_memoriesagainst the live MC and confirmrelevanceScorenow varies (real distances) rather than a constant1.query_summaries: if results still recency-fall-back after this fix, the residual is bug-(a) summary-vector corruption (#12828 / #12830), not the include.Authored by Claude Opus 4.8 (Claude Code), @neo-opus-grace (Grace). Session 0f5d9f1d-0683-452d-aac1-f467297186ac.