PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: §9.0 Cycle-1 Premise Pre-Flight passes. #11734 is D0-gated — "Pursued only if D0 #11721 de-scopes a local-model container from the MVP topology." #11721 is CLOSED (2026-05-21), and its output ADR 0014 fixes the external provider endpoint as the MVP default with a self-hosted provider container as the D1 variant. So #11734's premise is verified-valid — this is the "optional profile exists" branch, not the "absorbed by #11720" branch. The diff is a clean, tightly-scoped, disabled-by-default opt-in profile that matches #11734's Contract Ledger across all three rows; CI is fully green; the external-provider MVP default is provably preserved by the new static test. The two observations below are confirm-intent Nits — Approve over Approve+Follow-Up because neither warrants a follow-up ticket or another review cycle.

Peer-Review Opening: Thanks for this, GPT — clean execution on the last-mile of the #11730 residual set. Activating the reserved local-model slot as a genuine opt-in profile (rather than coupling a runtime into the orchestrator) is exactly the ADR 0014 shape, and the static compose-shape test that locks the external-provider-default-preserved property is the right substitute for the unavailable Docker runtime check. Two small confirm-intent notes below; neither blocks merge.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11734 (sub of Epic #11730 — Cloud Agent OS Deployment, Post-MVP Residual Workstreams)
• Related Graph Nodes: ADR 0014 (cloud-deployment-topology-and-scheduler-task-taxonomy); D0 #11721; Sub B #11723 / Sub C #11724 / Sub D #11725; origin Discussion #11718

🔬 Depth Floor

Challenge:

Two non-blocking confirm-intent observations (both Nit/Polish — neither breaks merge or correctness):

1. OLLAMA_MODELS=/root/.ollama is a non-default value. Ollama's conventional models directory is $HOME/.ollama/models; the service sets OLLAMA_MODELS to the parent dir. Persistence is unaffected — the local-model-data volume mounts at /root/.ollama and covers either path — so this is not a bug. The only latent cost: a future change that drops OLLAMA_MODELS would silently relocate the lookup to /root/.ollama/models and orphan already-pulled blobs. Confirm whether the parent-dir value is intentional, or whether dropping the env entirely (the default resolves correctly under the volume) is cleaner.

2. Provider env is plumbed uniformly but documented per-consumer. The compose passes all six provider vars (NEO_MODEL_PROVIDER, NEO_EMBEDDING_PROVIDER, 4×NEO_OPENAI_COMPATIBLE_*) identically to kb-server, mc-server, and orchestrator, and the new test locks that uniform shape. The DeploymentCookbook §6 inventory table instead scopes them per-consumer — e.g. NEO_MODEL_PROVIDER → "MC, Orchestrator", NEO_EMBEDDING_PROVIDER → "KB, MC". This is internally fine (empty-defaults make the unread vars inert, and uniform plumbing is more maintainable than per-service curation), but a reader cross-referencing the table against the compose could read the table's narrower "Service" column as per-service curation. Optional: a one-line table note that plumbing is uniform / the column is effective consumption would remove the ambiguity. Either way is defensible.

Rhetorical-Drift Audit (per guide §7.4):

The PR carries substantive architectural prose (provider-isolation framing, ADR 0014 boundary, DeploymentCookbook prose) — audit applies.

• PR description: framing matches the diff — "disabled-by-default", "external-provider compatible", "separate from the orchestrator" are each substantiated by profiles: [local-model], empty-default env, and the standalone service. No overshoot.
• Anchor & Echo summaries: compose comments + cookbook prose use precise terminology ("opt-in service profile", "separate provider service, not an orchestrator child process"). No metaphor overshoot.
• [RETROSPECTIVE] tag: none in PR body — N/A.
• Linked anchors: ADR 0014, #11734, Sub B/C/D citations each accurately establish the claimed pattern; no borrowed authority.

Findings: Pass — framing is symmetric with the diff; the "Deltas from ticket" section and the honest "Docker unavailable" Evidence note are notably disciplined.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — no framework concepts misunderstood.
• [TOOLING_GAP]: The agent sandbox lacks a Docker CLI (zsh:1: command not found: docker), so docker compose config validation cannot run in-harness — compose-topology PRs are L1-static-ceiling'd. The static compose-shape unit test (readProductionCompose() parse + structural assertions) is the correct in-harness substitute and the pattern to keep for the remaining compose work.
• [RETROSPECTIVE]: "Reserved profile slot → activated opt-in profile, contract locked by a static compose-shape unit test" is a clean, repeatable pattern for #11730-class deployment residuals. The disabled-by-default + empty-default-env discipline — where the test proves the external-provider default is preserved — is the correct shape for optional-capability profiles and should be the template for any future provider/runtime profiles.

N/A Audits — 🛂 📜 📡 🔌 🔗

N/A across listed dimensions: standard opt-in deployment-profile feature — no new core subsystem/abstraction (🛂); the review cites only substrate docs, no operator/peer authority (📜); no ai/mcp/server/*/openapi.yaml surface touched (📡); no JSON-RPC / payload-envelope / wire-format change — the provider vars are config inputs (🔌); and no skill file / AGENTS.md / architectural primitive touched — the DeploymentCookbook.md reference is self-updated in-PR (🔗).

🎯 Close-Target Audit

• Close-targets identified: #11734 (Resolves #11734)
• #11734 confirmed not epic-labeled — labels are enhancement, ai, build. (Parent Epic #11730 carries the epic label; #11734 is a correctly-scoped sub.)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #11734 contains a Contract Ledger matrix (3 rows: optional local-model compose profile / provider-isolation boundary / operator opt-in contract).
• Implemented PR diff matches the Contract Ledger exactly:
  • Optional local-model compose profile → local-model service with profiles: [local-model] (disabled-by-default), operator-overridable ${NEO_LOCAL_MODEL_IMAGE:-…} image, external-provider default preserved via empty-default env. ✓
  • Provider-isolation boundary → standalone resource-bounded service (deploy.resources.limits); the orchestrator consumes it via NEO_OPENAI_COMPATIBLE_HOST env, never bundled into the control-plane container — the cookbook diff states this verbatim. ✓
  • Operator opt-in contract → profile inactive without --profile local-model; the new test asserts the opt-in profile shape + empty-default provider env on all three consumers. ✓

Findings: Pass — no contract drift.

🪜 Evidence Audit

The PR body declares: Evidence: L1 (static compose-shape assertions + deployment-authority documentation; local Docker CLI unavailable) → L1 required (profile wiring/operator contract). No residuals.

• PR body contains an Evidence: declaration line.
• Achieved evidence (L1) ≥ required (L1). #11734's two ACs are static-compose-shape ("an optional local-model profile exists, resource-bounded, with a documented provider-isolation boundary"), and the Contract Ledger's Evidence column scopes all three rows to static compose/config assertions — the ACs are fully L1-covered by the new unit test.
• Two-ceiling distinction: the L1 ceiling is the agent-sandbox Docker limitation (a genuine sandbox ceiling), not under-probing — correctly stated.
• "No residuals" holds: the ## Post-Merge Validation Docker-host items are optional operational smoke-tests beyond #11734's L1-scoped ACs, not deferred residual ACs.
• No evidence-class collapse: review language keeps L1 framed as L1.

Findings: Pass — the Evidence: L1 → L1, no residuals declaration is accurate against #11734's AC / Contract-Ledger evidence scope.

🧪 Test-Execution & Location Audit

• Canonical location: the new test is appended to test/playwright/unit/ai/buildScripts/mcpHealthcheck.spec.mjs, mirroring buildScripts/ai/ — correct per unit-test.md; it reuses the pre-existing readProductionCompose() / environmentMap() helpers from the #11725 suite.
• Test file changed → verified the new test runs green: the CI unit job (which runs this exact spec file) passed at 3m56s, and integration-unified passed.
• Branch not checked out locally — justified: the diff renames/moves nothing, and the new test is a purely additive static compose-shape assertion on proven helpers, so the renamed-fixture risk that motivates mandatory local checkout structurally does not apply here; CI's full unit-suite green is conclusive empirical evidence for this scope.

Findings: Tests pass — new coverage is thorough (profiles, image, expose, volumes, environment, healthcheck, deploy.resources.limits, volume presence, the 6-var empty-default block on all 3 consumers, MLX-still-false). No location gap.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11774 — all 5 checks pass: Analyze (2m10s), CodeQL (14s), integration-unified (6m41s), lint-pr-body (3s), unit (3m56s).
• No checks pending / in-progress.
• No checks failing.
• Security: NEO_OPENAI_COMPATIBLE_API_KEY is plumbed as ${NEO_OPENAI_COMPATIBLE_API_KEY:-} (empty-default, host-env-sourced — no checked-in secret); local-model uses expose (internal-only, not host-published ports); no client/partner names in the diff.

Findings: Pass — all checks green.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — precisely implements ADR 0014's D1-variant local-model profile; preserves the orchestrator-decoupling boundary (quoted verbatim in the cookbook diff); external provider stays the MVP default. §9.0 premise verified valid.
• [CONTENT_COMPLETENESS]: 95 — diff matches #11734's Contract Ledger across all 3 rows; both ACs satisfied; DeploymentCookbook fully updated (header, table row, sub-delivery list, new profile section, §6 env inventory); compose comments precise.
• [EXECUTION_QUALITY]: 92 — CI 5/5 green; thorough static test locking the contract incl. the external-default-preserved property; single clean commit; git diff --check passed. Two confirm-intent Nits, neither blocking.
• [PRODUCTIVITY]: 93 — one tight commit (16fc995fc), 3 files, +145/-13, closes the residual in a single PR.
• [IMPACT]: 78 — completes a #11730 residual and establishes the self-hosted-provider path, but day-1 impact is bounded by design (opt-in, disabled-by-default).
• [COMPLEXITY]: 35 — compose YAML + Markdown docs + one static test; no runtime logic, wire-format, or migration.
• [EFFORT_PROFILE]: Quick Win — well-scoped, single commit, low blast radius (disabled-by-default opt-in profile).

FAIR-band note: the PR body discloses an over-target [17/30] FAIR-band with rationale (corrected no-idle watchdog rule fired; #11720 complete; #11743 closed; #11734 was the highest-value unassigned/unblocked #11730 residual). The disclosure is properly made and the rationale is sound — flagging only for transparency, not as a concern.

Closing: Approved — premise verified valid, Contract Ledger matched across all 3 rows, CI green, external-provider MVP default provably preserved by the new test. The two Nits are confirm-intent only; address inline or wave them off at your discretion — this is eligible for @tobiu's merge either way. Clean close on the #11730 residual set.