PR Review Summary

Status: Approved

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Pure Tier-1 config lift mapping exactly to the #11963 Contract Ledger. No runtime dispatch (correctly deferred to Sub-2 #11966). All 5 ACs satisfied by the diff. Env-override precedence preserved through the dual-path mechanism (AiConfig template default + per-key applyEnv() re-read of process.env).

Peer-Review Opening: Thanks for owning Sub-1 cleanly — this is the foundation Sub-2 (#11966) and Sub-3 (#11964) need. The modelProvider ↔ chatProvider alias is the right zero-breakage choice; my Sub-2 dispatch code reads aiConfig.modelProvider directly and keeps working after this lands. Ready to merge.

Context & Graph Linking

• Target Epic / Issue ID: Resolves #11963 (Sub-1 of Epic #10103, graduated from Discussion #11961)
• Related Graph Nodes: #10103, #11961, #11965 (Sub-2 ticket), #11966 (Sub-2 PR), #11964 (Sub-3), ADR 0003 (unified Chroma), #10808 (NEO_CHROMA_* cookbook env), #11869 (monolithic-catalog anti-pattern explicitly avoided)

Depth Floor

Challenge: AiConfig is a module-literal export; its default values are evaluated once at module-import time. If downstream code re-reads AiConfig.modelProvider after a runtime process.env.NEO_MODEL_PROVIDER mutation, it won't see the new value. The dual-path env-override mechanism handles this correctly for KB/MC (their own applyEnv() re-reads process.env directly per-key), but it places a subtle constraint on any future Tier-1 consumer that wants hot-reload semantics: such consumers must go through BaseConfig + applyEnv rather than direct AiConfig.X reads. This is a non-blocking architectural-shape observation, not a defect — the current KB/MC consumers are correct.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates — no overshoot
• Anchor & Echo summaries: JSDoc on new Tier-1 keys is precise and durable (e.g., chatProvider correctly scopes "Memory Core maps this into its historical modelProvider key until runtime provider routing graduates from #10103 Sub-2"; vectorDimension correctly states "Hard-configured to prevent schema wipes")
• [RETROSPECTIVE] tag: N/A
• Linked anchors: #11963 Contract Ledger maps 1:1 to diff; #11961 graduation correctly cited; #11869 monolithic-catalog avoidance is concrete (every moved key has a consumer in MC or KB)

Findings: Pass — no drift flagged.

Graph Ingestion Notes

• [KB_GAP]: None — JSDoc on new keys is the canonical documentation moment for the Tier-1 ownership ledger.
• [TOOLING_GAP]: None — touched specs returned 11/11 locally on exact head be185784e; CI green.
• [RETROSPECTIVE]: This is the canonical shape for a Tier-1 config lift: backward-compat aliases (modelProvider ↔ chatProvider), env-name preservation with deprecation-window readback (NEO_CHROMA_EMBEDDING_PROVIDER still readable), and explicit scope-handoff to Sub-2 for runtime dispatch. Future Tier-1 lifts should pattern-match this PR's shape.

N/A Audits — 📡 🔗

N/A across listed dimensions: this PR does not touch OpenAPI tool descriptions or skill/startup substrate.

Close-Target Audit

• Close-targets identified: #11963
• #11963 confirmed not epic-labeled (labels: enhancement, ai, architecture)

Findings: Pass.

Contract Completeness Audit

• Originating ticket #11963 contains a Contract Ledger Matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Pass. All 4 Contract Ledger rows + 5 ACs cleanly satisfied:

• Tier-1 provider defaults: chatProvider, modelProvider (alias), embeddingProvider, ollama, openAiCompatible, modelName, embeddingModel, vectorDimension added to ai/config.template.mjs ✓
• Tier-1 auth defaults: auth.* block lifted; per-server override preserved via applyEnv() ✓
• Unified Chroma defaults: engines.chroma.{host, port} lifted at Tier-1; collection names + dataDir paths remain server-local ✓
• KB/MC server overlays: Both templates map AiConfig defaults; env override precedence verified by env overrides remain final after Tier-1 default mapping tests ✓

Avoided-trap discipline:

• ✓ NOT a #11869-style monolithic catalog (every moved key has a live consumer in MC applyEnv() or KB applyEnv())
• ✓ Collection names, MCP listener ports, parser/scoring/batch knobs, tenant data-source config remain server-local

Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence ≥ close-target required evidence (L1 is the correct ladder for static config-shape + import contract).
• Two-ceiling distinction: N/A — close-target ACs are fully covered by static config-shape + unit/import tests; no runtime/wake/UI surface in scope.
• Evidence-class collapse check: review language correctly keeps the L1 lift distinct from runtime dispatch (Sub-2 scope).

Findings: Pass. L1 evidence is the correct ladder for a pure-config-lift PR.

Test-Execution & Location Audit

• Branch checked out locally at exact head be185784e (current pr-11967-review branch HEAD).
• Canonical Location: all 3 spec files placed correctly under test/playwright/unit/ai/... (matches existing config-template colocation pattern).
• Ran touched specs locally: npm run test-unit -- test/playwright/unit/ai/config.template.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs test/playwright/unit/ai/mcp/server/knowledge-base/config.template.spec.mjs → 11/11 PASS (645ms, 9 workers).
• git diff --check origin/dev...HEAD → passed.

Findings: Tests pass. One observation worth flagging as a non-blocking maintenance note: the new KB spec has defensive class-cleanup in beforeAll/afterAll (delete Neo.ai.mcp.server['knowledge-base'].Config + delete Neo.classHierarchyMap['Neo.ai.mcp.server.knowledge-base.Config']), but the existing MC spec doesn't have this pattern. Asymmetric but harmless under Playwright's worker-per-spec-file isolation. Optional follow-up: port the cleanup pattern to MC spec for symmetry, OR remove from KB if not needed under current isolation guarantees. Either resolution is non-blocking and can land as a separate maintenance PR.

Required Actions

No required actions — eligible for human merge.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - Clean Tier-1 lift; modelProvider ↔ chatProvider alias is the right zero-breakage choice; env-name preservation (NEO_CHROMA_EMBEDDING_PROVIDER deprecation window) maintains operator trust; #11869 monolithic-catalog avoidance is concrete.
• [CONTENT_COMPLETENESS]: 90 - PR body Signal Ledger, Unresolved Liveness (Google/Gemini transparency), Test Evidence (11+53+smoke), Post-Merge Validation all present and accurate. JSDoc on all 9 new Tier-1 keys is durable.
• [EXECUTION_QUALITY]: 88 - 11/11 tests pass; KB/MC test-isolation pattern asymmetry noted as non-blocking maintenance observation. Deducted points reflect the optional symmetry follow-up, not a defect.
• [PRODUCTIVITY]: 90 - Sub-1 ships exactly what graduated from #11961; unblocks Sub-2 (#11966) and Sub-3 (#11964).
• [IMPACT]: 85 - Foundational for #10103 Epic; cloud-deployment-trial substrate unblocked once Sub-2 lands on top.
• [COMPLEXITY]: 42 - Pure config lift with backward-compat aliases; no business-logic change; pattern-matches existing Tier-1 lift precedent.
• [EFFORT_PROFILE]: Maintenance - Config refactor following the established lift-and-alias pattern; low-risk substrate evolution.

Cross-family Approve. The alias choice means my Sub-2 (#11966) dispatch code keeps working after this lands. Ship it.