PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The shared buildChatModel extraction and SearchService provider dispatch are the right implementation direction, but the PR does not yet satisfy #12741's MCP-level contract because the Knowledge Base health gate can still reject ask_knowledge_base before SearchService.ask() runs when GEMINI_API_KEY is absent. This is the same release-safety class the PR is meant to remove, so it needs one focused correction cycle.

Peer-Review Opening: Strong core extraction, Vega. Moving the chat wrapper beside the providers is the right boundary, and the tests prove the most important selector property: a present Gemini key does not override an explicitly local provider. The remaining problem is one layer above the service: MCP pre-dispatch health still encodes the old Gemini-key worldview.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12741, PR body, changed-file list, ADR 0019, learn/benefits/ArchitectureOverview.md, prior KB provider tickets from non-synthesis query_documents, BaseServer health-gate source, KB Server.mjs, KB HealthService.mjs, SearchService.mjs, the new buildChatModel.mjs, focused unit specs, exact-head CI state, and close-target issue metadata. I intentionally did not call ask_knowledge_base during this review because this PR reviews the stale remote-synthesis path itself.
• Expected Solution Shape: KB ask synthesis should route through the configured chat provider and remain callable on the local-provider path without requiring GEMINI_API_KEY. The solution must not add a parallel provider SSOT; it should read resolved AiConfig leaves at the use site per ADR 0019, and tests must cover the server/tool boundary that can block SearchService.ask() before provider dispatch.
• Patch Verdict: Partially matches. SearchService.construct() now calls shared buildChatModel() with aiConfig.modelProvider, and buildChatModel correctly supports openAiCompatible, ollama, and explicit gemini. But ai/services/knowledge-base/HealthService.mjs still performs an env-level key/host check and ensureHealthy() still throws for any non-healthy status; BaseServer.mjs calls that before every non-exempt KB tool. That leaves the local-provider ask path blocked by the old health predicate in configurations where NEO_MODEL_PROVIDER=openAiCompatible is set but GEMINI_API_KEY and provider host env overrides are not.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12741
• Related Graph Nodes: #12740, #12742, #12738, ADR 0019, Neo.ai.services.knowledge-base.SearchService, Neo.ai.services.knowledge-base.HealthService, Neo.ai.mcp.server.BaseServer, ai/provider/buildChatModel.mjs

🔬 Depth Floor

Challenge: The diff fixes the synthesis client but not the MCP health gate that decides whether the synthesis tool can run. BaseServer calls healthService.ensureHealthy() before dispatch; KB Server.getHealthExemptTools() does not exempt ask_knowledge_base; KB HealthService.ensureHealthy() throws unless status is exactly healthy; and #checkApiKeyConfigured() still falls back to !!process.env.GEMINI_API_KEY unless specific provider host env vars are present. That means a deployment can choose the local provider through NEO_MODEL_PROVIDER=openAiCompatible and still get blocked before the new provider-routed SearchService code is reached.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly matches the intended SearchService dispatch change.
• Anchor & Echo summaries: drift remains in the touched source. SearchService class-level docs still say synthesis sends context to "the Google Gemini model"; KB HealthService comments/details still say GEMINI_API_KEY is required for all KB operations / embedding features. Those strings now contradict the provider-routed contract.
• [RETROSPECTIVE] tag: N/A, none present.
• Linked anchors: pass. #12741 and ADR 0019 are the right source anchors.

Findings: Request Changes. Tighten the health gate and provider wording so the MCP-visible contract matches the service-level routing.

🧠 Graph Ingestion Notes

• [KB_GAP]: Provider routing has two layers: the service's synthesis model and the MCP server's pre-dispatch health gate. A local-capable SearchService is insufficient if HealthService.ensureHealthy() still requires Gemini-era conditions.
• [TOOLING_GAP]: The tech-debt-radar workflow normally mandates ask_knowledge_base, but this review deliberately used non-synthesis query_documents because stale KB ask synthesis is the incident surface under review. Memory Core raw-memory search also returned a degraded Chroma path (chroma-error-finding-id), not a genuine no-match.
• [RETROSPECTIVE]: Extracting buildChatModel into ai/provider/ is the right shared primitive boundary; consumers avoid importing Memory Core service dependencies just to reuse provider dispatch.

🎯 Close-Target Audit

• Close-targets identified: #12741
• #12741 confirmed not epic-labeled (bug, ai, regression, architecture, model-experience)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift. The ticket says ask_knowledge_base should run through a local provider without GEMINI_API_KEY, but the server health gate still rejects degraded health before tool dispatch in the local-provider/no-key shape unless separate host env vars happen to satisfy the legacy check. Required Action 1 covers this.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers all close-target behavior.
• No L3/L4 evidence needed for the dispatch logic itself; L1 is appropriate once the pre-dispatch gate is covered.

Findings: Current L1 tests cover buildChatModel selector behavior and SearchService degraded-reference behavior, but they do not cover KB MCP/health pre-dispatch for the no-Gemini local-provider path.

🛂 Provenance Audit

Findings: Pass. The new shared helper is an internal extraction from the existing SessionService.buildChatModel provider selector, not an imported external abstraction. Its new home in ai/provider/ is justified by dependency direction: Knowledge Base can reuse provider dispatch without importing Memory Core service modules.

🔗 Cross-Skill Integration Audit

• No skill/startup workflow update needed for a pure provider helper extraction.
• Downstream server health integration is incomplete.

Findings: Integration gap: SearchService and Memory Core now share the provider helper, but Knowledge Base health/readiness still expresses the old Gemini-key contract. This is not a skill-doc issue; it is a runtime integration miss.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head 8ab8c709f2afcd0d927252da508a0af52f5536ed.
• Canonical Location: existing focused unit test file remains under test/playwright/unit/ai/services/..., appropriate for Agent OS service tests.
• Ran the specific changed/related unit specs:

npm run test-unit -- test/playwright/unit/ai/services/memory-core/SessionService.buildChatModel.spec.mjs test/playwright/unit/ai/services/knowledge-base/SearchService.spec.mjs test/playwright/unit/ai/services/knowledge-base/SearchService.noModel.spec.mjs

Result: 19 passed.

• Exact-head CI via GitHub checks is green: CodeQL, Analyze, lint-pr-body, unit, and integration-unified all success.
• Coverage gap: no focused test proves KB HealthService.ensureHealthy() / MCP pre-dispatch allows local-provider ask without GEMINI_API_KEY.

Findings: Tests pass, but they miss the blocking health-gate contract.

📋 Required Actions

To proceed with merging, please address the following:

• Make Knowledge Base health/pre-dispatch provider-aware. HealthService should read the resolved aiConfig.modelProvider / local provider leaves instead of using an env-only GEMINI_API_KEY predicate for the whole server. With modelProvider=openAiCompatible or ollama, missing GEMINI_API_KEY must not make ensureHealthy() reject ask_knowledge_base when Chroma/collections are healthy. Add focused coverage for this no-key local-provider shape.
• Update the remaining Gemini-specific KB prose and error/detail strings touched by this contract. At minimum, SearchService class docs should no longer say the RAG context is sent to "the Google Gemini model", and KB health comments/details should not claim GEMINI_API_KEY is required for all KB operations in local-provider mode.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 78 - 22 points deducted because the service-level provider extraction aligns with ADR 0019, but the adjacent KB health gate still re-implements provider readiness via direct env checks instead of the resolved AiConfig provider contract.
• [CONTENT_COMPLETENESS]: 72 - 28 points deducted because the PR body is strong, but touched source docs still describe Gemini-only synthesis/health semantics that the PR is supposed to retire.
• [EXECUTION_QUALITY]: 70 - 30 points deducted because exact-head CI and focused unit tests pass, but the missing health-gate coverage leaves a real MCP-level failure path for the primary AC.
• [PRODUCTIVITY]: 75 - 25 points deducted because the PR makes substantial progress on #12741 but does not yet make the tool reliably local-capable at the MCP boundary.
• [IMPACT]: 88 - 12 points deducted only because #12742 still owns the default-local policy; this PR remains high-impact incident containment by removing hardwired Gemini construction from KB ask synthesis.
• [COMPLEXITY]: 68 - Moderate-high: the change crosses provider abstraction, KB service construction, Memory Core imports, and test coverage, but it is still one clear dispatch axis rather than a full config redesign.
• [EFFORT_PROFILE]: Heavy Lift - High-value Agent OS provider-routing work with a narrow remaining integration blocker.

The core patch is close. Fix the health gate and stale Gemini wording, and this should be an approval candidate rather than a reshape.