PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The fix correctly repairs the #12703 Brain-graph retrieval regression — vector ranking stays primary, a bounded lexical rescue surfaces exact-named local anchors, tenant isolation is preserved, and it is empirically green (7/7 on my checkout + full CI). Approve to unblock a P0 (ask_knowledge_base is the default KB tool every agent uses). The one non-blocking concern — the code-term rescue's file-scan I/O cost — is better tracked as a follow-up than as blocking Cycle-N churn.

Peer-Review Opening: Solid, well-scoped retrieval repair, and I verified it the right way — checked out the branch and ran the related test (7/7), not a diff-read approval. The design keeps vector primary and handles the tenant boundary, exactly where I'd worry for a cloud multi-tenant KB. One follow-up on the code-term scan's I/O cost below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12703 (the regression + its source-evidence list), the changed-file list, the config + test diffs, the QueryService.mjs rescue methods, and the PR body (treated as a claim to verify) — plus the empirical checkout.
• Expected Solution Shape: A retrieval-layer fix that surfaces exact-named Brain/graph anchors when semantic top-k misses them, WITHOUT (a) demoting vector ranking, (b) breaking existing queryDocuments behavior, or (c) leaking cross-tenant content; bounded I/O; a test proving the #12703 case.
• Patch Verdict: Matches. Lexical rescue complements vector (the JSDoc + addLexicalRescueScores adding to sourceScores confirm vector-primary); the 6 baseline tests still pass (no regression); rescued sources are stamped neo-shared (tenant-safe); the new test reproduces the #12703 query and asserts the anchors surface.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12703
• Related Graph Nodes: #12696 (v13 epic), Memory Core, ask_knowledge_base, QueryService, lexical-rescue, tenant-isolation

🔬 Depth Floor

Challenge: The primary non-blocking concern is the I/O cost of addCodeTermRescues. When the query contains any snake_case/kebab code term >5 chars (common in agent KB queries — mutate_frontier, query_recent_turns, etc.) it walks ai/services + ai/mcp/server + ai/graph (collectFiles limit:500/root ⇒ up to ~1,500 files), reads each file's full content, and normalizes it — per query. limit:500 bounds it, but reading + normalizing ~1,500 files adds real latency to a frequent query class, and a cloud/network FS would amplify it. Not a correctness defect (the rescue is right) — a perf-hardening target; see Required Actions follow-up.

(V-B-A self-correction: my initial false-positive worry was over-stated. The code-term regex [a-zA-Z][a-zA-Z0-9]*(?:[_-][a-zA-Z0-9]+)+ requires a _/- separator + >5 chars, so plain words like "database"/"pipeline" don't match — extraction precision is reasonable, and the path-hint rescue has a proper startsWith(neoRootDir) traversal guard.)

Secondary: there is no negative / false-positive-guard test — the new test covers the true-positive (#12703 anchors surface), but not "a plain semantic query with no anchors does NOT trigger the rescue scan" (which would lock in both the vector-primary contract and the I/O bound). The 6 passing baseline tests cover no-regression, but not the explicit no-rescue-on-no-anchor path.

Rhetorical-Drift Audit (per guide §7.4): PR carries substantive architectural prose — audited.

• PR-body framing matches the diff: "bounded local lexical-rescue... semantic ranking remains primary" — confirmed (rescue adds to sourceScores, doesn't replace vector results) + JSDoc.
• No overshoot: "default Neo shared corpus" tenant-stamping matches the fail-closed handling; the live-MCP residual (AC3) is honestly deferred to Post-Merge, not claimed as shipped.
• Linked anchors: #12703 is the non-epic bug; the named files (DreamPipeline.md, GoldenPathSynthesizer.mjs, GraphService.mjs) are the actual reproducer anchors.
• Findings: Pass — framing is precise; the residual is honest.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A clean hybrid-retrieval pattern — vector-primary + bounded lexical rescue for exact-named anchors — that repairs a default-tool regression without a tenant-isolation regression. The tenant-stamping (rescued = neo-shared) is the cloud-safety detail that's easy to miss and was handled.
• [TOOLING_GAP]: The regression itself (ask_knowledge_base missing exact Brain/graph anchors) is the live #12450/#12703 retrieval degradation that two reviewers (including me, on #12709) hit during this session — this PR is the direct repair.

N/A Audits — 📑 📡 🔌 🛂

N/A: no new public API/config-consumer contract beyond the documented lexicalRescueMatch weight (covered in the body's MCP Config Template Change section), no wire-format change, no new MCP tool, and not a novel external abstraction (lexical/exact-match rescue is a standard hybrid-retrieval complement).

🎯 Close-Target Audit

• Close-targets: #12703 (Resolves #12703 + closingIssuesReferences).
• #12703 confirmed NOT epic-labeled (bug, ai, regression, model-experience). Commit bodies clean (no stray close-keywords); subjects carry (#12703).
• Findings: Pass.

🪜 Evidence Audit

• PR body has the Evidence: line — L2 (unit service tests + stubbed Chroma + tenant guard) → L2 required (service retrieval contract AC1–6). Residual: AC3 (live MCP answer) needs a post-merge KB MCP restart.
• Achieved ≥ required: L2 achieved, and I independently confirmed it (7/7 on checkout). The L3 live-MCP-answer is correctly deferred (the running MCP process is stale until restart) and listed in Post-Merge Validation.
• Two-ceiling distinction honest: the L2-vs-live-MCP gap is a genuine process-staleness ceiling, not an unprobed shortcut.
• Findings: Pass — Evidence declaration accurate; AC3 residual legitimately post-merge.

🧪 Test-Execution & Location Audit

• Branch checked out (gh pr checkout 12710) — §2.2 empirical pass, not a static read.
• Ran the related test: npm run test-unit -- .../QueryService.queryDocuments.spec.mjs → 7 passed (839ms), including the new #12703 rescue test + the 6 pre-existing baseline tests (no regression).
• Canonical location: new test in test/playwright/unit/ai/services/knowledge-base/ ✓ (per unit-test.md).
• Portability fixup verified: fs.pathExists guard on the sandman_handoff.md assertion + serial mode (singleton-stub mutation) — sound.
• Findings: Tests pass (independently verified). No location issues.

📋 Required Actions

No required actions — eligible for human merge. One follow-up (non-blocking, per Strategic-Fit Approve+Follow-Up):

• Perf-harden addCodeTermRescues: the up-to-~1,500-file read+normalize per code-term query is the main scaling cost. Options for a follow-up ticket — an inverted index / cached term→file map, a tighter candidate set, or short-circuit after enough candidates. I'll file this as a follow-up sub under #12696 unless you'd rather take it (pull-request §6.3.1).
• (Fold into that follow-up) add a negative test: a no-anchor semantic query does NOT trigger the rescue scan — locks in the vector-primary contract + the I/O bound.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — sound hybrid-retrieval shape (vector-primary preserved, tenant-safe neo-shared stamping, config-driven weight, path-traversal guard on path hints); 5 deducted because the code-term rescue scans + reads all root files per anchor-query rather than using an index — heavier than the design ideal.
• [CONTENT_COMPLETENESS]: 95 — Fat-Ticket body (Evidence L2 line, Deltas, MCP Config Template Change, 3-spec Test Evidence, Post-Merge, Resolves #12703) + Anchor&Echo JSDoc (@summary/@param/@returns) on the new methods; 5 deducted because the method JSDoc doesn't note the I/O/perf characteristics of the scan (a maintainer-relevant property).
• [EXECUTION_QUALITY]: 88 — no defects; 7/7 tests pass on my independent checkout (the #12703 case + 6 baseline); 12 deducted for the addCodeTermRescues I/O cost (functional but heavy on a frequent query class) + the absent no-anchor negative test.
• [PRODUCTIVITY]: 95 — fully repairs the #12703 regression at the retrieval layer; the live-MCP answer (AC3) is honestly deferred to a post-merge restart, not over-claimed.
• [IMPACT]: 75 — repairs ask_knowledge_base (the default KB tool every agent uses) for Brain/graph queries — a P0 Brain-retrieval caveat in the v13 register; broad swarm reach.
• [COMPLEXITY]: 68 — Moderate-high: +413 lines of new lexical-rescue logic (6 methods + extractor regexes + bounded fs-walkers) integrated into the query-scoring path, plus tenant-stamping and path-traversal reasoning.
• [EFFORT_PROFILE]: Heavy Lift — substantial retrieval-layer addition (high complexity) fixing a core P0 (high impact).

Nice work — the tenant-stamping and vector-primary discipline are the parts I'd most worry about for a cloud KB, and both are right. Approving; the only thing I'd track beyond merge is the code-term scan's I/O cost.