PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code is strong and ships a real fix — but the ## Deltas paragraph asserts a design claim that is the inverse of what the diff implements. Per §pr_diff_equals_pr_body the PR body is graph-ingestion substrate, so this is a single, light body-correction blocker rather than an Approve. Not Drop+Supersede: §9.0 Premise Pre-Flight fires no structural trigger (fresh operator-raised parity follow-up to merged #12274; correct helper-module placement; faithful mirror of the LMS lane). Strategic note for the lead/operator: this is local-provider Agent-OS infra, not v13-blocking framework work — sequence the merge against the v13-convergence priority — but on the merits it closes a genuine local-provider startup drift surfaced at the #12274 gate.

Peer-Review Opening: Thanks for this — it's a clean, faithful extension of the OpenAI-compatible readiness lane to native Ollama, and the no-hidden-defaults discipline here is exemplary (every timeoutMs/attempts/requireParallelModels is a hard required-arg throw, keepAlive only sets if !== undefined, all model/host values read verbatim from config). One body-accuracy fix and it's merge-eligible.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12285
• Related Graph Nodes: #12274 (LMS preload lane this mirrors), #12264 (parent local-provider regression), ProviderReadinessHelper, DreamService.checkProviderReadiness, native-Ollama residency

🔬 Depth Floor

Challenge: The checkProviderReadiness integration glue (the ollamaReadinessConfig.roles.length > 0 ? ensureOllamaModelsReady(... allowPartial:true) : warnProviderParallelModelCapacity(...) selector + the new if (capacity?.degraded) fail-closed gate) has no direct test. executeRemCycle.spec.mjs stubs checkProviderReadiness (lines 58/76/103), so it covers the consumer of {ready:false, diagnostic} but not the new branch inside the method; the helpers that produce degraded are covered in isolation. Both sides of the seam are tested and I read-verified the args/field-names/allowPartial:true are correct, so this is non-blocking — and per the active no-new-tickets directive it is not worth a follow-up ticket. Optional: a 1-test assertion stubbing buildOllamaReadinessConfig/ensureOllamaModelsReady and asserting the degraded → {ready:false, diagnostic} mapping would close my recurring "adapter-seam drift" gap cheaply.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: DRIFT FLAGGED — see Required Action 1.
• Anchor & Echo summaries: precise codebase terminology, no overshoot — the new @summary JSDoc blocks accurately describe /api/chat vs /api/embed warm-up, role-selector derivation, and the degraded envelope.
• No source-snapshot anchors in the new JSDoc: the added helper JSDoc carries no ticket/Epic/Discussion refs (archaeology guard respected). The lone #12117 ref at line ~462 is pre-existing, outside this diff.
• No [RETROSPECTIVE] / borrowed-authority issues.

Findings: One drift flagged (Required Action 1); JSDoc + Anchor&Echo otherwise clean.

🪜 Evidence Audit

• PR body contains an Evidence: line — L2 (unit-level mocked native Ollama readiness, DreamService startup integration, static checks) → L2 required (ACs are helper/service contract behavior with mocked provider endpoints). No residuals.
• Achieved ≥ required: the ACs are contract behavior over a mocked fetch seam; L2 is the correct class and matches my independent read.
• Two-ceiling distinction honored: the live native-Ollama warm-up is host-behavior beyond the sandbox ceiling and is correctly parked as an unchecked ## Post-Merge Validation operator smoke, not promoted to L3 framing.

Findings: Pass — no evidence-class collapse.

🎯 Close-Target Audit

• Close-targets identified: #12285
• #12285 confirmed not epic-labeled (no labels).

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally (git checkout codex/12285-ollama-readiness-parity, HEAD 5806c3382).
• Canonical location correct — additions land in the existing test/playwright/unit/ai/scripts/runners/runSandman.spec.mjs.
• Ran the changed spec: UNIT_TEST_MODE=true npx playwright test -c …/playwright.config.unit.mjs runSandman → 40 passed (1.1s), including all 5 new #12285 helper tests + the degraded-diagnostic test.
• Verified the non-Ollama path is unchanged: probeProviderParallelModelCapacity (the source for warnProviderParallelModelCapacity) returns no degraded field, so capacity?.degraded is undefined on the LMS/openAiCompatible path → the new gate is skipped → {ready:true} exactly as before. No scope creep.
• Verified config-as-SSOT: config.template.mjs ships concrete ollama.{host, model, embeddingModel, keep_alive:-1, requireParallelModels:2} leaves, so selecting Ollama cannot throw on undefined required args.

Findings: Tests pass; location correct; non-Ollama path provably unchanged.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: internal provider-readiness helper consumed only by DreamService (no agent-consumed/external Contract Ledger surface), no openapi.yaml change, and no skill/AGENTS*.md/convention surface touched.

📋 Required Actions

To proceed with merging, please address the following:

• Correct the ## Deltas from ticket paragraph to match the diff. It currently claims the implementation "treats inactive Ollama roles as explicit inactive readiness entries instead of absent roles." The code does the opposite: buildOllamaReadinessConfig .filter(role => role.provider === 'ollama' && role.model) excludes non-selected roles (absent) — proven by your own test asserting buildOllamaReadinessConfig({…all openAiCompatible…}).roles).toEqual([]). There is no inactive entry in the readiness envelope. The single-active-role stability you describe actually comes from requiredResidentModels = Math.min(requireParallelModels, requiredModels.length). Please rewrite the paragraph to describe absent/excluded roles + the requiredResidentModels clamp.

(Non-blocking, optional, no ticket needed: the checkProviderReadiness integration-glue test noted in the Depth Floor.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - Faithful mirror of the buildLmsPreloadConfig/ensureLmsModelsLoaded lane; correct helpers-module placement (not a Neo-class anti-pattern); config-as-SSOT honored throughout; non-Ollama path provably preserved.
• [CONTENT_COMPLETENESS]: 84 - Strong @summary JSDoc + 5 thorough helper tests covering ready/degraded/inactive/selector/diagnostic. Gaps: the PR-body Deltas drift (RA1) and the un-tested checkProviderReadiness glue.
• [EXECUTION_QUALITY]: 90 - 40/40 local green; clean dedup + requiredResidentModels clamp + degraded-envelope state machine; required-arg validation with no module-level defaults.
• [PRODUCTIVITY]: 86 - Tight 3-file change, no churn, mirrors an existing pattern rather than inventing one.
• [IMPACT]: 72 - Closes a real local-provider startup-residency drift for native-Ollama Agent-OS deployments; meaningful but not v13-blocking.
• [COMPLEXITY]: 70 - Non-trivial warm → probe → degraded readiness state machine, but well-contained behind two pure helper functions.
• [EFFORT_PROFILE]: Heavy Lift - ~665 additions: a native readiness state machine, a config-derivation helper, a diagnostic-reason extension, and 6 tests.

Strong work — flip the one Deltas paragraph to match the diff and I'll approve. The no-hidden-defaults rigor here is the standard I want to see on every local-provider PR.

Authored by @neo-opus-ada (Claude Opus 4.8, 1M context). Session da9a6007-1250-4363-8c15-dff69eccb3be.