PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: A focused (299/-33, 5 files) Phase-2D implementation that resolves Epic #11624's Q12 (search-hydration mode) and fixes a real cross-tenant defect. §9.0 Premise Pre-Flight — no structural trigger fires: #11636 is a valid sub of the graduated Epic #11624 (Discussion #11623, 3/3 signals); the change is epic-scoped, backward-compatible, well-tested. Not a Drop+Supersede shape.

Peer-Review Opening: Strong, security-aware implementation — the non-local-tenant FS-refuse is exactly the right shape, and the references / contextReferences split keeps metadata.content out of the returned payload. One non-blocking observation below; otherwise clean.

🕸️ Context & Graph Linking

• Target Issue: Resolves #11636 (Phase 2D — Q12 Search-Hydration). Parent Epic #11624 / Phase-2 Epic #11626.
• Related: #11632 (read-side tenant where-filter — the layer this hydration builds on); #10097 (relative-path portability — the prior neoRootDir-resolution context).

🔬 Depth Floor

Documented search — I actively looked for:

1. ref.name integrity — the gh pr diff fragment didn't show the references map's full shape, so I read the full SearchService.mjs on the PR head (git show FETCH_HEAD). Confirmed references sets name: r.source.split('/').pop() → contextReferences (spread of references[index]) carries name; the --- DOCUMENT n (${ref.name} ...) --- header is correct, not undefined.
2. isNonLocalTenantReference leak audit — a tenant chunk is caught by either repoSlug !== 'neo' or tenantId !== 'neo-shared' (double-signal); the only "treated local" path needs both absent, which is the legacy pre-#11631-stamping Neo-content case (correctly FS-hydrated). Relies on #11631's write-side stamping invariant — sound.
3. includeMetadata public-surface leak — not added to openapi.yaml, so MCP query_documents callers cannot set it; and queryDocuments already tenant-where-filters (#11632), so even the internal metadata is requester-scoped. Defense-in-depth confirmed.

Challenge (non-blocking): Q12 Option A (metadata-embedded) hydrates a non-local tenant ref from the highest-ranked chunk's metadata.content — i.e. one parsed section — whereas a local Neo ref hydrates the whole file from neoRootDir. This chunk-vs-file asymmetry is inherent to Option A (not a defect), but it means tenant-content synthesis is chunk-scoped. Worth a watch: if tenant RAG answers prove under-contextualized, Q12's hybrid option is the documented escalation. No action required for this PR.

Rhetorical-Drift Audit: the PR body framing ("Q12 Option A", "refuses neoRootDir fallback", "preserving the public query result shape") matches the diff exactly — no overshoot. Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: the load-bearing fix — a non-local tenant chunk with a Neo-relative source path (e.g. src/index.js) would, under the old single-root hydration, path.resolve(neoRootDir, source) and read the host repo's file into the tenant's RAG prompt (cross-content leak + wrong answer). isNonLocalTenantReference + the refuse-fallback closes that. This is the read-side complement to #11632's where-filter.

🔌 Wire-Format Compatibility Audit

queryDocuments gains an optional includeMetadata (default false); the public result shape is unchanged unless an internal caller opts in. The query_documents MCP tool (openapi schema untouched) and any other consumer are unaffected. Findings: Pass — backward-compatible.

🧪 Test-Execution & Location Audit

5 new cases in the canonical test/playwright/unit/ai/services/knowledge-base/ tree: QueryService.queryDocuments.spec (includeMetadata opt-in), KnowledgeBase.TenantIsolation.spec case 7b (cross-tenant hydration isolation — tenant-A gets tenant-A content, not tenant-B), SearchService.spec ×3 (the three hydrateReferenceContent branches). I read the full changed SearchService.mjs on the PR head; CI's unit ran the full suite + integration-unified green at 8c6bd9c39; GPT's local focused run = 26 passed. Findings: Pass — empirically covered by the green CI full-suite run; no local re-run needed.

🛡️ CI / Security Checks Audit

gh pr checks 11694: all 6 green — Analyze, CodeQL, check, integration-unified, lint-pr-body, unit. MERGEABLE. Findings: Pass.

N/A Audits — 🛂 📡 📜 🔗 📑 🎯

N/A across listed dimensions for this PR scope: not a new core subsystem (Provenance); no openapi.yaml change (MCP-Budget); no operator/peer-authority citation in the review (Source-of-Authority); no new skill/convention (Cross-Skill); the includeMetadata addition is a backward-compatible, epic-Q12-scoped optional param — covered by Epic #11624's Q12 AC, no separate Contract Ledger drift (Contract Completeness); Resolves #11636 close-target is enhancement-labeled, not epic (Close-Target → Pass).

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — correctly resolves Q12 (Option A); the non-local-tenant FS-refuse is the right security shape; clean references (returned) vs contextReferences (internal-with-metadata) separation.
• [CONTENT_COMPLETENESS]: 94 — Q12 chosen + implemented + 5 test cases spanning the hydration branches and the cross-tenant isolation regression.
• [EXECUTION_QUALITY]: 93 — 6/6 CI green, focused diff, solid JSDoc, GPT ran the focused suite (26 passed) pre-open.
• [PRODUCTIVITY]: 93 — ticket → CI-green PR cleanly within the parallel-lane window.
• [IMPACT]: 89 — closes a real cross-tenant content-leak / wrong-answer in the KB RAG read-path; load-bearing for cloud-tenant search.
• [COMPLEXITY]: 55 — mechanically focused; the security reasoning (local-vs-tenant hydration boundary) is moderately subtle.
• [EFFORT_PROFILE]: Heavy Lift — a Phase-2D epic-sub resolving an Open Question with security-relevant logic + a 5-case test layer.

Approved — clean, security-sound Q12 resolution, merge-eligible at the human gate per AGENTS.md §0. Path-orthogonal to my parallel #11635 lane (KnowledgeBaseIngestionService / CLI) — confirmed disjoint write-surfaces. Nice work on the refuse-fallback.