PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The canonical Compose reachability defect is real and the empty pass-through shape is salvageable in place, so Drop+Supersede would discard useful work. The release blocker is narrower: the new guard collapses three service contracts into one global name set and never verifies the operator-overridable value shape, so it stays green when the repaired coordinate is broken.

Peer-Review Opening: Thanks for tracing a real deployment gap back to the canonical profile and for correcting the first “unbuildable” claim. The pass-through itself is close; one coordinate-level repair will make its regression guard tell the same truth as the deployment contract.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16850; the three changed-file names; current origin/dev Compose/config source; ADR-0019; the local-model precedent introduced by #11774; and Memory Core searches for native-Ollama Compose wiring and the earlier local-agent-os deployment.
• Expected Solution Shape: Expose only the resolved Ollama inputs each consuming service needs, retain AiConfig’s empty/unset fallback ownership, and avoid hardcoding the optional local-model service name. The guard must prove each (service, env leaf) coordinate and its operator-overridable interpolation independently; a union of names across services is not the consumed contract.
• Patch Verdict: Partially matches. docker-compose.yml adds empty pass-through entries, but composeEnvNames() in LoopbackDefaultReachability.spec.mjs:51-77 unions every service before checking names, while the production claim is per service. It also ignores the configured value entirely.
• Premise Coherence: Coheres with verify-before-assert in finding and narrowing a real canonical-profile gap, but the shipped guard conflicts with that same value: its mutation proof is weaker than the “every service that consumes them” contract it claims to protect.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16850
• Related Graph Nodes: #16706, #16830, PR #11774, ADR-0019, native-Ollama deployment reachability
• Origin Session ID: d8332b13-5d97-4839-ac11-d2de4602a989

🔬 Depth Floor

Challenge: I ran two exact-head, in-memory mutations over the Git blobs at be29257967341458aa4bc1f20094a6ac91639aa7:

1. Removing NEO_OLLAMA_HOST from both kb-server and mc-server, while leaving it only on orchestrator, still produced unreachable=[] and passed the new guard. The resulting service presence was {kb-server:false, mc-server:false, orchestrator:true}.
2. Replacing all three values with NEO_OLLAMA_HOST=http://wrong-process.invalid:11434 also produced unreachable=[] and passed.

Those are the two boundaries the PR says it protects: service reachability and an operator-supplied target. The current test proves only that a variable name appears somewhere in the file.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: “all six to every service,” “no configuration path,” and the claimed positive-control census exceed what the guard and historical deployment surface establish
• Anchor & Echo summaries: the repeated Compose comments at docker-compose.yml:93-104,226-237,335-346 say every native embed failed “on every deployment” and “no config path” could name the target, although out-of-band/custom Compose wiring already could
• [RETROSPECTIVE] tag: N/A — none added
• Linked anchors: #11774 deliberately configured the bundled local-model as openAiCompatible; it does not establish that native Ollama requires all six leaves on all three services

Findings: Drift is real and shares the same repair boundary as the behavioral guard; truth-fold it during that repair rather than creating a separate documentation round.

🧠 Graph Ingestion Notes

• [KB_GAP]: The canonical local-model profile contract and native-Ollama provider support are distinct. #11774 made the bundled model service reachable through the OpenAI-compatible path; this PR adds a missing native-provider override path.
• [TOOLING_GAP]: A union-of-env-names parser cannot protect a per-service deployment contract and ignores whether the value remains operator-overridable.
• [RETROSPECTIVE]: Deployment reachability guards need coordinates—consumer service, leaf, and value shape—not a file-wide occurrence count.

🎯 Close-Target Audit

• Close-targets identified: #16850
• #16850 is not epic-labeled

Findings: Pass. #16850 is a valid leaf; closure is blocked only by the contract/guard mismatch above.

📑 Contract Completeness Audit

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

Findings: Contract drift. The ledger still specifies a file-wide declared-leaf subset, while the corrected AC and production claim are per-consuming-service; it also treats all six leaves as symmetric even though NEO_OLLAMA_REQUIRE_PARALLEL_MODELS has production callers in the orchestrator path, not the KB/MC path. Fold the ledger and durable comments into the same coordinate-level repair.

🪜 Evidence Audit

• PR body contains the canonical one-line Evidence: L<X> ... → L<Y> required ... declaration
• The L3 plane check is listed under Post-Merge Validation rather than promoted to exact-head merge evidence
• #16850 annotates that runtime AC as [L3-deferred — operator handoff needed]
• The author distinguishes the unavailable running-plane ceiling from completed structural evidence
• No external receipt is used as exact-unmerged-head deployment causality

Findings: The L3 deferral is honest. The claimed L1 structural receipt is not yet sufficient, because both exact mutations above pass. This is collateral to the same behavioral Required Action, not an independent paperwork blocker.

N/A Audits — 📡 🔗

N/A across listed dimensions: this PR changes Compose pass-through and a unit guard, not OpenAPI descriptions, skill conventions, or cross-substrate primitives.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI is green at be29257967341458aa4bc1f20094a6ac91639aa7; L3 runtime validation is correctly deferred
• Reviewer falsifier: node --input-type=module over exact-head Git blobs removed the host leaf from two consumers and then hardcoded a wrong target; both mutations still passed the new guard
• Test location: test/playwright/unit/ai/deploy/LoopbackDefaultReachability.spec.mjs is correctly placed

Findings: Falsifier failed the claimed regression boundary despite green CI.

📋 Required Actions

To proceed with merging, please address the following:

• Make the guard and pass-through contract coordinate-aware. Derive the required (service, NEO_OLLAMA_* leaf) pairs from actual consumers, assert each service independently, and assert the value retains an operator-overridable form such as ${VAR:-} rather than merely finding the name somewhere. Add mutation controls that delete a required leaf from one service and replace a required interpolation with a hardcoded wrong target; both must go red. Give non-consuming services explicit dispositions instead of copying all six by symmetry—especially NEO_OLLAMA_REQUIRE_PARALLEL_MODELS, whose production use is orchestrator-side. In the same repair, align the Contract Ledger and repeated source/PR prose to the bounded truth: the canonical profile lacked the native-Ollama pass-through, while custom or out-of-band deployment wiring could already supply it.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 55 - Correct deployment layer and AiConfig default ownership, but the test erases the service boundary and the env matrix is copied by symmetry rather than consumer authority.
• [CONTENT_COMPLETENESS]: 55 - Functional pass-through exists, while ledger, durable comments, and evidence language remain broader than the delivered contract.
• [EXECUTION_QUALITY]: 45 - CI is green, but two exact-head mutations falsify the only new regression guard.
• [PRODUCTIVITY]: 60 - The patch unlocks useful native-provider configuration; the guard must become trustworthy before this high-blast profile change lands.
• [IMPACT]: 85 - These settings decide whether containerized embedding can reach its provider.
• [COMPLEXITY]: 35 - Three small surfaces and one focused in-place repair.
• [EFFORT_PROFILE]: Quick Win - The production pass-through is mostly present; the remaining work is a bounded contract/test correction.

The central fix is worth keeping. Once the guard proves the same per-service, operator-overridable coordinates the Compose profile promises—and the prose stops universalizing beyond that profile—this should be a straightforward re-review.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z