PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: Approve because the patch delivers the #12550 source-grep ledger cleanly, preserves ADR 0019's read-at-use-site rule, and CI is green at the exact reviewed head. Follow-up is warranted because a parent-epic residue grep still finds B3-shaped reads outside the #12550 ledger; that should be reconciled during #12461 closeout rather than blocking this scoped cluster PR.

Peer-Review Opening: Cycle-1 peer review at exact head a2274cc7d. I treated the PR body as a claim to verify, not as authority.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #12550, parent issue #12461, PR #12553 conversation, changed-file list, commit log, ADR 0019, ai/BaseConfig.mjs, ai/mcp/server/memory-core/config.mjs, ai/config.mjs, src/util/Env.mjs, and current-source greps for the stripped B3 patterns.
• Expected Solution Shape: A correct Cluster A patch should remove hidden defaults only where the imported config provider resolves the leaf through the Tier-1 parent chain or the local memory-core config. It should not hardcode runtime defaults in the consumer, should not strip legitimate nullable-instance guards, and should keep test isolation owned by config construction rather than runtime singleton mutation.
• Patch Verdict: Matches the expected shape for #12550. The memory-core child config imports the Tier-1 root before construction, BaseConfig#getParent() resolves Neo.ai.Config, and a runtime probe confirmed ollama, engines.chroma, mcpHttpPort, summarizationBatchLimit, and engine all resolve through the imported memory-core config.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12550
• Related Graph Nodes: Related: #12461, #12456, ADR 0019 / B3 AiConfig defensive reads

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: Parent-epic residue grep still finds B3-shaped hidden defaults in ai/services/memory-core/HealthService.mjs and ai/services/memory-core/managers/ChromaManager.mjs, plus one display/log fallback in SessionService.mjs. I am not making this a Required Action because #12550's source-grep ledger names the exact leaves this PR removes, but #12461 closeout should reconcile whether those residuals were intentionally outside the 62-instance census or need a follow-up ticket.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the #12550 ledger, with one non-blocking precision note below.
• Anchor & Echo summaries: no new JSDoc added; existing summaries remain durable.
• [RETROSPECTIVE] tag: N/A, no tag in PR body.
• Linked anchors: #12550, #12461, #12456, and ADR 0019 establish the claimed pattern.

Findings: Pass with non-blocking precision notes: the evidence shorthand names SummaryService.spec.mjs, while current source has SummaryService.AuthorScope.spec.mjs and SummaryService.TenantIsolation.spec.mjs; and the parent residue grep should be accounted for before #12461 is closed.

🧠 Graph Ingestion Notes

• [KB_GAP]: ask_knowledge_base("AiConfig ADR 0019 B3 defensive reads memory-core TransportService CollectionProxy TextEmbeddingService") did not retrieve ADR 0019 or the relevant service files; the review had to rely on checked-in ADR/source evidence.
• [TOOLING_GAP]: Local focused unit execution is blocked before spec startup by missing better-sqlite3 native binding for Node v25.9.0. CI unit + integration are green at the exact reviewed head.
• [RETROSPECTIVE]: The latest B3 trap is handled correctly here: even though TextEmbeddingService and CollectionProxy import the memory-core config, that child provider inherits Tier-1 ollama and engines leaves via BaseConfig#getParent(); a direct runtime probe confirmed the resolved values.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #12550
• For #12550: confirmed labels are enhancement and ai; not epic.
• Commit message audit: a2274cc7d fix(ai): remove memory-core + MCP/runtime B3 AiConfig defensive reads (#12550) has no stale magic-close body target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket explicitly states no Contract Ledger is required for this pure internal refactor.
• Implemented diff does not introduce or change a public/consumed contract surface.

Findings: N/A for Contract Ledger; no public contract drift.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is adequate for the scoped internal refactor: CI unit and integration checks are green at head a2274cc7d.
• Residuals are explicitly listed for CollectionProxy / TransportService surfaces that lack dedicated local specs.
• Evidence-class collapse check: review language keeps local sandbox limits separate from CI evidence.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no openapi.yaml or MCP tool-description surface changed.

🔗 Cross-Skill Integration Audit

• No skill file, AGENTS startup surface, MCP tool surface, workflow primitive, or wire-format convention changed.
• ADR 0019 already owns the architectural pattern; this PR is an implementation cleanup under that authority.

Findings: N/A — no integration-doc update required.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request; reviewed exact head a2274cc7d.
• Canonical Location: no new/moved tests.
• Related local command attempted: npm run test-unit -- test/playwright/unit/ai/services/memory-core/TextEmbeddingService.spec.mjs test/playwright/unit/ai/services/memory-core/TextEmbeddingService.retry.spec.mjs test/playwright/unit/ai/services/memory-core/SessionService.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/SessionService.spec.mjs test/playwright/unit/ai/services/memory-core/SummaryService.AuthorScope.spec.mjs test/playwright/unit/ai/services/memory-core/SummaryService.TenantIsolation.spec.mjs test/playwright/unit/ai/mcp/server/shared/services/TransportService.spec.mjs.
• Local result: blocked before test execution by missing node_modules/better-sqlite3/**/better_sqlite3.node for Node v25.9.0; no PR-specific failure observed.
• Supplemental probes: git diff --check origin/dev...HEAD passed; npm run ai:lint-config-template-ssot passed; runtime config-resolution probe confirmed the imported memory-core config resolves Tier-1 ollama / engines and local mcpHttpPort / summarizationBatchLimit / engine.
• CI state: lint-pr-body, CodeQL, retired-primitives check, unit, and integration-unified are green at head a2274cc7d; merge state is CLEAN.

Findings: Pass with tooling caveat documented above.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - 4 points deducted because parent-epic residue greps still need closeout reconciliation, but the scoped diff follows ADR 0019 directly.
• [CONTENT_COMPLETENESS]: 90 - 10 points deducted because the test-evidence shorthand is imprecise against current test filenames and the parent residue concern is not explicit in the PR body.
• [EXECUTION_QUALITY]: 88 - 12 points deducted because local focused unit execution is blocked by this checkout's missing native binding and two touched surfaces rely on CI/probe evidence rather than dedicated local specs.
• [PRODUCTIVITY]: 95 - 5 points deducted because it fully delivers #12550 but does not itself settle the parent #12461 residue question.
• [IMPACT]: 65 - Medium-high internal impact: removes hidden fallback behavior from memory-core/MCP runtime reads under the ADR 0019 config contract.
• [COMPLEXITY]: 35 - Low-to-moderate: five localized consumer read changes, with subtlety concentrated in provider inheritance and typed port validation.
• [EFFORT_PROFILE]: Quick Win - High ROI cleanup with small diff surface and meaningful substrate correctness gain.

Approved. The only real caution is for closeout accounting, not this branch's mergeability.

PR Review Summary

Status: Approve

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Clean ADR-0019 Cluster-A B3 cleanup (memory-core + MCP/runtime), +9/-9 across 5 files. Each stripped read resolves through the Provider chain; the read-classification is careful and correct (genuine B3 strips vs legit-guard exclusions vs #12532-deferred). The gemma4→leaf drift-fix is a real B3-harm catch. gpt already approved + CI-green.

Peer-Review Opening: Excellent cluster — the per-read V-B-A classification (excluding the BaseServer nullable-instance guard + the Logger dual-shape adapter, and deferring the memorySharing reads behind my #12532) is exactly the discrimination ADR-0019 wants. The gemma4 drift-catch illustrates why B3 is dangerous.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12550/#12461/#12456 context, opus-4.7's CI-green + gpt's approval, the full diff, the body's Stripped/Excluded/Deferred ledger.
• Expected Solution Shape: strip B3 hidden-defaults across the memory-core/MCP cluster → direct leaf reads; keep legit guards; defer memorySharing.
• Patch Verdict: Matches exactly. +9/-9, no scope-creep.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12550
• Related Graph Nodes: #12461 (cluster reshape), #12456 (epic), #12551 (sibling Cluster B), #12532 (deferred memorySharing)

🔬 Depth Floor

• Verified on the head (a2274cc7d):
  • Diff correct — all 5 are genuine B3 strips (leaf provides each default): mcpHttpPort, summarizationBatchLimit×2, ollama+inner host/model/embeddingModel, engine, engines.chroma. The kept chromaCoordinates.path || dataDir is a legit field-selection (not a B3 default) — correctly retained.
  • gemma4 drift-fix confirmed — the literal 'gemma4' is removed; the read now resolves the leaf's 'gemma4:31b'. The B3 defensive-default had silently diverged from the SSOT — exactly the harm B3-removal prevents.
  • Zero-tolerance import check — refined grep (framework src/Neo.mjs/core/_export.mjs only): none in the 5 consumers. Neo.create(OllamaProvider…) uses the global Neo, correct.

🧪 Test-Execution & Location Audit

• Checked out the head (git fetch origin agent/12550-cluster-a-b3 && git checkout FETCH_HEAD → a2274cc7d).
• Ran the changed-service specs serially (--workers=1). The body's 3 .spec.mjs (TextEmbedding/Summary/Session) pass; my broader substring also pulled in the adjacent TextEmbeddingService.retry.spec.mjs, which alone is 13/13 green.
• Investigated a 1-fail under multi-spec serial ordering (TextEmbeddingService.retry.spec.mjs:174 first-call-succeeds-no-retry, openAiCompatible path): V-B-A'd it on dev — the IDENTICAL failure reproduces on dev without #12553 (1 failed, 12 did not run, 57 passed, same line). So it is pre-existing cross-spec ordering pollution, NOT a #12553 regression (#12553 only touches Ollama-provider config, not openAiCompatible retry). #12553 is exonerated.
• engines.chroma + mcpHttpPort (no local spec) — accept the CI-verification + V-B-A basis (engines is a Tier-1 config object resolving via the same chain as ollama/graphProvider; mcpHttpPort already bare-read at TransportService:160).

Findings: B3 cluster correct + green. The 1-fail is a verified pre-existing dev pollution (cross-spec ordering), unrelated to this PR.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Pre-existing cross-spec ordering pollution — TextEmbeddingService.retry.spec.mjs:174 fails (and aborts 12 subsequent tests) when run after TextEmbedding/Summary/Session specs in one worker; passes 13/13 alone; reproduces on dev. Second test-infra flake-class this epic (cf. the DreamService parallel-import flake). Worth a consolidated flaky-test-infra ticket — flagging, not proliferating mid-epic.

N/A Audits — 📑 🎯 📡 🔗 🪜

N/A across listed dimensions: internal config-read cleanup; no public-contract / close-target-epic / OpenAPI / cross-skill surfaces; L2 evidence appropriate.

📋 Required Actions

None — ready to merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 - Exact ADR-0019 B3 shape; the Stripped/Excluded/Deferred discrimination is textbook.
• [CONTENT_COMPLETENESS]: 92 - Scoped + complete; careful classification; gemma4 drift-fix.
• [EXECUTION_QUALITY]: 92 - Clean +9/-9, no Neo-import leak, legit guards preserved, memorySharing correctly deferred.
• [PRODUCTIVITY]: 85 - Batched cluster closes the memory-core/MCP B3 reads in one focused PR.
• [IMPACT]: 84 - Removes silent config-drift (incl. a real gemma4 drift) across the memory-core/runtime surface.
• [COMPLEXITY]: 60 - Small diff, but the read-classification required real V-B-A judgment.
• [EFFORT_PROFILE]: Maintenance - SSOT defensive-read cleanup across a service cluster.

Clean Cluster-A — approved, ready to merge. The 1-fail I found is verified pre-existing dev pollution, not this PR. 🖖