PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: All five #13879 ACs met, the formal Contract Ledger Matrix followed row-by-row, and the conservative unload policy explicitly decided + tested. The one real follow-up (OpenAI-compatible probe coverage) is a narrow test-gap, better tracked than cycled. Directly sharpens the Ollama-contention diagnosis that feeds the fire-relief path.

Peer-Review Opening: Clean, well-scoped fix, Euclid — the observedRequiredCount reframing is exactly the right truthful-readiness move (a stale resident must not masquerade as capacity), and the ollama pull <tag> guidance is a real operator-experience win over the old OLLAMA_MAX_LOADED_MODELS-only hint.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13879 (ACs + the formal Contract Ledger Matrix), the diff, createParallelModelCapacityWarning current source, and the Ollama-contention cluster context (#13852 / #13700 / #13851).
• Expected Solution Shape: Report extraModels separately; compute capacity from required resident models (not total); missing-tag warning names ollama pull <tag>; conservative no-unload policy for shared residents; focused tests. Must NOT blind-unload shared Ollama models.
• Patch Verdict: Matches all 5 ACs. observedRequiredCount = getRequiredAvailable(...).length → capacityReady is now required-aware (the core fix); extraModels surfaced in every envelope; the ollama pull warning branch; conservative report-don't-unload policy explicitly chosen.
• Premise Coherence: Coheres — ADR-0019-aligned (per ticket; config resolved at use-site, no second source), and "make the readiness contract truthful first" is the right ordering before any unload decision.

🕸️ Context & Graph Linking

• Target Issue: Resolves #13879
• Related Graph Nodes: #13852 (parent live-proof lane), #13854 (parity parent), #13700 (the parallel-N KV twin), #13889 (separate — the message-ingestion epic; unrelated to this diagnostics fix).

🔬 Depth Floor

Challenge (test-coverage gap on the transitively-changed path): The capacity-from-required fix is applied not only in ensureOllamaModelsReady but also in probeProviderParallelModelCapacity, which serves both Ollama and the OpenAI-compatible provider (LM Studio). There it changes the readiness semantics — ready = observedRequiredCount >= requiredResidentModels (new) vs observedCount >= requireParallelModels (old) — a correct generalization of the same latent bug, but a behavior change the Ollama-titled PR doesn't advertise. The new tests only exercise the Ollama ensureOllamaModelsReady path; the OpenAI-compatible probe's new required-aware ready + requiredResidentModels aren't directly covered. → Follow-up: add a focused test for the OpenAI-compatible probe's required-aware readiness (stale extra + missing required), or confirm an existing spec exercises it.

Also checked: the conservative no-unload decision is sound for shared Ollama infra; the helper factoring (getRequiredAvailable / getExtraModels / requiredModelSet) is clean and consistent across both envelopes; backward-compat preserved (extraModels defaults to []).

Rhetorical-Drift Audit: Pass. The body accurately describes the conservative report-don't-unload decision and the extraModels reporting; no overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The fix's spine is "capacity = required-resident, not total-resident" — a truthfulness correction that prevents a stale shared model from reading as readiness progress. The right ordering (make the envelope truthful before deciding any unload policy) is the reusable lesson.
• [KB_GAP]: The shared probeProviderParallelModelCapacity quietly governs both Ollama and OpenAI-compatible readiness; a fix framed as "Ollama" propagates to the OpenAI-compatible path. Worth a doc note that this probe is the shared capacity-readiness primitive across providers.

🎯 Close-Target Audit

• Close-targets identified: Resolves #13879 (newline-isolated, line 1).
• #13879 labels: bug / ai / architecture / model-experience — not epic-labeled. Valid leaf close-target. No stray Closes/Fixes; Related: refs are non-closing.

Findings: Pass.

📑 Contract Completeness Audit

#13879 carries a formal Contract Ledger Matrix:

• Row 1 (ensureOllamaModelsReady envelope — required/missing/insufficient/extra separately): ✅ implemented + tested.
• Row 2 (createParallelModelCapacityWarning — missing-tag vs extra-resident): ✅ implemented (extra= + ollama pull branch) + asserted in tests.
• Row 3 (orchestrator-owned readiness via ConfiguredTaskDefinitionsService — don't treat stale extras as success): satisfied transitively — the helper is the source of ready/capacityReady, now required-aware; no direct change in the diff.

Findings: Pass. → Verify-note (folded into follow-ups): confirm ConfiguredTaskDefinitionsService postSpawn/liveness keys on ready/capacityReady (now required-aware) rather than reading observedCount directly.

N/A Audits — 🪜 📡 🛂 🔗

N/A: no observable-runtime-AC beyond the listed post-merge L4 residual (ACs are unit-coverable), no OpenAPI/MCP surface, no new architectural-abstraction provenance, no skill/convention surface.

🧪 Test-Execution & Location Audit

• New test reports stale resident Ollama models separately (#13879) covers the exact failing scenario from the ticket's empirical probe (qwen3-embedding resident, gemma4:26b missing → observedRequiredCount:0, extraModels:[qwen3-embedding:latest], warning ollama pull gemma4:26b, NOT OLLAMA_MAX_LOADED_MODELS). Existing tests updated to assert the new fields.
• CI green at head (no failing/pending). Verified the AC mapping + logic from the diff; did not re-run locally (CI-green + diff-read).

Findings: Tests present + targeted; the one gap is the OpenAI-compatible probe path (follow-up above).

📋 Required Actions

Non-blocking follow-ups (do not gate human merge):

• Add focused coverage for the OpenAI-compatible probeProviderParallelModelCapacity required-aware readiness (the change generalized there too), or confirm an existing spec exercises it.
• Verify ConfiguredTaskDefinitionsService postSpawn/liveness consumes ready/capacityReady (now required-aware), not observedCount directly — closes the Contract-Ledger row-3 transitive guarantee.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — truthful-readiness-first, ADR-0019-aligned, clean shared-helper factoring across both envelopes.
• [CONTENT_COMPLETENESS]: 90 — Fat-Ticket body + Evidence line + the conservative-policy decision documented; -10 because the OpenAI-path behavior change isn't called out in the body.
• [EXECUTION_QUALITY]: 88 — correct required-aware capacity + backward-compatible envelopes; -12 for the untested OpenAI-compatible probe semantics.
• [PRODUCTIVITY]: 95 — resolves all five ACs in one focused slice.
• [IMPACT]: 65 — directly sharpens the Ollama-contention diagnosis (a stale resident no longer reads as capacity) that the recovery fire-relief path consumes.
• [COMPLEXITY]: 40 — single helper file + spec; threaded one new count + one new array through several envelopes.
• [EFFORT_PROFILE]: Quick Win — high-ROI truthfulness fix with targeted coverage.

Approving — clean and AC-complete; the follow-ups just close the transitive OpenAI-path coverage. Merge is the operator's gate.

flipping to approve, since we have a follow-up linter enhancement ticket. the block-formatting is NOT resolved here.