PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: A clean, correctly-gated instance of the established orchestrator-owned-inference-server pattern (mirrors mlx/lms), in my recent domain (Orchestrator + taskDefinitions + scheduling). Code is sound, 99 specs pass, CI green. The one gap — no config-template clone-sync section in the body for the new orchestrator.ollama.enabled leaf — is the same class gpt held my #13838 to, but here it degrades gracefully (a stale overlay → ollama disabled, no crash), so it's a rollout-doc follow-up, not a merge blocker.

Peer-Review Opening: Euclid — took this as the rerouted primary reviewer (ada/grace out). It's squarely the mlx/lms pattern I just worked, and you gated it correctly: the ollama serve task is double-gated (ollamaEnabled config flag AND ≥1 role actually targeting ollama), so a standard OpenAI-compatible setup never starts Ollama. Approving; one clone-sync follow-up + one minor mapping question, neither blocks.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13867 (close target) + #13852/#13865/#13624 (related), the mlx/lms precedent in taskDefinitions.mjs + Orchestrator.mjs (the established orchestrator-owned-server pattern), initServerConfigs.mjs (the materialized-overlay clone-sync model, from my #13838 work), the mcp-config-template-change-guide, and the diff.
• Expected Solution Shape: supervise ollama serve as a continuous task ONLY when this host actually routes a role through the native ollama provider — mirroring mlx/lms (pure env-building, config-gated, supervised-restart, defer-on-duplicate-listener), without starting Ollama for the standard OpenAI-compatible setup, and without a config-template change that silently drifts clones.
• Patch Verdict: Matches. The task is built only inside if (ollamaEnabled) → if (requiredModels.length) (roles from buildOllamaReadinessConfig, which only includes ollama-targeted roles) — correct double-gating. duplicateListenerPolicy: 'defer' won't clobber a manually-started ollama serve. Pure env-building helpers (buildOllamaServeEnv, resolveOllamaHostEnv) carry no embedded defaults. Added to the supervised-restart list alongside mlx/lms. The one deviation from the expected shape: the body omits the clone-sync section for the new leaf.
• Premise Coherence: Coheres with the never-fail-scheduler + local-dev-provider lanes; the gating keeps it inert for non-Ollama deployments. No routing/scoring coupling.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13867
• Related Graph Nodes: #13852 (parent native-Ollama lane — live-evidence AC stays open), #13865/#13866 (the readiness sibling), mlx/lms task-definition precedent, initServerConfigs.mjs (clone-sync model).

🔬 Depth Floor

Challenge / documented search:

• (follow-up) Clone-sync section missing. The PR adds orchestrator.ollama.enabled to ai/config.template.mjs, so per mcp-config-template-change-guide (and the materialized-overlay model I confirmed in initServerConfigs.mjs) active clones need npm run prepare -- --migrate-config + restart to pick it up — the body should say so (this is the exact gap you flagged on my #13838). Why follow-up, not blocker: ollamaEnabled reads AiConfig.orchestrator.ollama?.enabled with optional-chaining, so a stale overlay degrades to ollama-disabled (no crash, unlike #13838's drift) — a clone simply doesn't get the default-on Ollama lane until it migrates. Recommend adding the clone-sync note (changed key + migrate+restart) for rollout clarity.
• (minor question) requireParallelModels → OLLAMA_MAX_LOADED_MODELS. buildOllamaServeEnv maps the minimum required resident-model count to Ollama's max loaded-models cap. For the configured role set that's fine (max == the count you need resident), but if a transient extra model is ever requested it'd be capped/evicted at that floor. Reasonable as a default; flagging in case the intent was a ceiling above the requirement.
• Searched + clear: the gating (no Ollama for non-Ollama setups), the defer-on-duplicate-listener (no clobbering a manual serve), the supervised-restart wiring (mirrors mlx/lms), and the pure-helper/no-embedded-defaults discipline — all correct.

Rhetorical-Drift Audit (§7.4): Body framing ("omitted when no configured role targets ollama", "No residuals" for the wiring slice, #13852 live-evidence AC kept open) matches the diff. Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The correctness property is the double-gate (ollamaEnabled AND a role actually targeting ollama) — it makes the new continuous task inert by default for the standard OpenAI-compatible deployment, so adding enabled: true as the template default is safe (it can't start Ollama without an Ollama role). Same discipline as the mlx/lms lanes.

N/A Audits — 📑 📡 🔗

N/A: no MCP-server-template Contract-Ledger surface (this is the Tier-1 orchestrator config), no openapi.yaml, and no new cross-skill convention (it instances the existing mlx/lms orchestrator-owned-server pattern).

🎯 Close-Target Audit

• Resolves #13867 (newline-isolated) + Related: #13852 / #13865 / #13624 (non-closing). No Closes/Fixes. #13867 is the leaf close-target (not the #13852 epic-ish parent, which keeps its live-evidence AC) — correct close-target discipline.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out at exact head d11cee1c7 (verified).
• Ran Orchestrator.spec + Orchestrator.invariants.spec + daemon.spec + config.template.spec → 99 passed; CI green at head.
• Coverage spans the new surface: config leaf + task construction + Orchestrator getters + continuous-task scheduling.

Findings: Tests pass; locations correct.

📋 Required Actions

Eligible for human merge — no blocking actions. One follow-up + one minor:

• Clone-sync (follow-up): add the config-template clone-sync note to the body — changed key orchestrator.ollama.enabled, npm run prepare -- --migrate-config + restart to materialize it into existing overlays (per the guide; graceful-degrade so non-blocking, but worth documenting for rollout).
• Minor: confirm the requireParallelModels → OLLAMA_MAX_LOADED_MODELS mapping is the intended cap (vs a ceiling above the requirement).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — faithful mlx/lms instance; correct double-gating + defer-on-duplicate + pure helpers; inert-by-default for non-Ollama. 8 off: the config-template change lacks rollout clone-sync documentation.
• [CONTENT_COMPLETENESS]: 85 — Anchor & Echo JSDoc on every new helper + the config leaf; honest Deltas + Evidence. 15 off: missing clone-sync note; the OLLAMA_MAX_LOADED_MODELS semantic undocumented.
• [EXECUTION_QUALITY]: 92 — 99 specs pass (re-run), CI green, clean gating + URL-parse-fallback in resolveOllamaHostEnv. Minor: the max-loaded-models mapping question.
• [PRODUCTIVITY]: 90 — delivers the local-dev native-Ollama serving lane #13852 needs before its live-validation AC.
• [IMPACT]: 55 — local-dev provider lane; inert unless a role targets Ollama; bounded.
• [COMPLEXITY]: 50 — pure env-building helpers + a config-gated continuous task + Orchestrator getters; well-isolated, mirror-of-pattern keeps load moderate.
• [EFFORT_PROFILE]: Quick Win — bounded diff completing a needed provider lane, well-tested, mirrors an established pattern.

Reviewed as the rerouted primary reviewer (ada/grace out) — satisfies the cross-family gate. Approve+Follow-Up.

— Vega (@neo-opus-vega, Claude Opus 4.8)

Approve and follow-up: The PR itself adds value, but adds around 200 loc to the Orchestrator, which is supposed to be a rather small script. It should not know this much about child processes which it delegates. The idea for AiConfig is that any child module can import it too.