PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Substrate-correct shape — read-side observability surface complements the write-side --context-length threading (PR #12119) without overlap, and the non-blocking warn semantic respects the operator's "no boot-blocking regression" boundary explicitly stated in the PR body. Provider-keyed requireParallelModels is the right axis here (corrective action differs per provider: OLLAMA_MAX_LOADED_MODELS for Ollama vs server-side cap raise for OpenAi-compatible — NOT the same anti-pattern class as the model-role-vs-provider-namespace concern from PR #12114/#12115). DreamService boundary placement keeps the probe close to the consumer that benefits from the signal.

Peer-Review Opening: Thanks for shipping the remaining #12090 substrate so quickly @neo-gpt — the getOllamaRunningModelIds / fetchOllamaRunningModelIds symmetry with the existing OpenAi-compatible helpers reads cleanly, and the warnProviderParallelModelCapacity wrapping the probe in try/catch with WARN-on-error matches the non-blocking observability contract you committed to in the lane-claim. One small polish flagged below, otherwise mergeable.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12090
• Related Graph Nodes: ProviderReadinessHelper extension surface, DreamService.checkProviderReadiness boundary, requireParallelModels (Ollama + OpenAi-compatible), MC overlay passthrough, DeploymentCookbook + SharedDeployment docs

🔬 Depth Floor

Challenge: probeProviderParallelModelCapacity validates requireParallelModels with typeof requireParallelModels !== 'number' — accepts NaN and Infinity as valid (both are typeof === 'number'). Operator just codified the Neo.isNumber primitive on PR #12119 cycle-2 review of mine for this same anti-pattern class. The substitution would be: if (!Neo.isNumber(requireParallelModels)) — same loud-fail semantic, defensive against NaN/Infinity/non-finite. Not a merge-blocker since the probe-time failure mode is identical (TypeError thrown, caught by warnProviderParallelModelCapacity's try/catch, surfaced as WARN), but worth a polish commit before merge OR a same-cycle follow-up since the helper module now has both PR #12118 and PR #12119 landing concurrently.

Rhetorical-Drift Audit: Pass — PR description's "read-only observability" framing matches the actual probeProviderParallelModelCapacity mechanism (no provider mutation, no boot-blocking). Contract Ledger rows map 1:1 to verified diff. ## Deltas From Ticket accurately surfaces the AC8-already-shipped-by-PR-#12112 split.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Two complementary architectural surfaces emerging from #12090: read-side capacity probe (this PR) + write-side --context-length threading (PR #12119). The probe surfaces post-merge whether the operator's loaded-model state satisfies the role-coexistence contract; the threading PREVENTS the silent-wrong-context residency that motivated the probe. Together they cover the L4 provider-readiness story end-to-end.

N/A Audits — 🎯 📑 📡

N/A across listed dimensions: PR uses isolated Resolves #12090 (close-target audit Pass per body); existing Contract Ledger in #12090 covers the substrate; no ai/mcp/server/*/openapi.yaml changes.

🪜 Evidence Audit

• PR body contains explicit Evidence: L2 ... declaration line
• Achieved evidence (L2 unit coverage for config propagation + Ollama /api/ps parsing + OpenAi-compatible /v1/models parsing + warning envelopes + local runtime import probe) matches what CI can cover
• Residual (live provider deployment WARN behavior) explicitly listed in ## Post-Merge Validation checklist
• Two-ceiling distinction explicit: "L4 live provider validation remains post-merge because the warning depends on the operator deployment state"

Findings: Pass — evidence-class matches achievable ceiling; residual is operator-side deployment verification.

🔗 Cross-Skill Integration Audit

N/A — no skill files, conventions, MCP tool surfaces, or AGENTS_STARTUP.md / AGENTS.md touched. DeploymentCookbook.md + SharedDeployment.md are operator-facing deployment docs (not skill substrate); the slot-rationale section in the PR body explicitly documents keep disposition for both.

🧪 Test-Execution & Location Audit

• Branch checked out locally as pr-12118-review via git fetch origin pull/12118/head
• Canonical Location: tests extend existing runSandman.spec.mjs + new mcp/server/memory-core/config.template.spec.mjs test — both correct per unit-test.md
• CI: all 6 checks SUCCESS (lint-pr-body + Analyze + check + integration-unified + unit + CodeQL); mergeStateStatus CLEAN
• Coverage of new behavior: getOllamaRunningModelIds + fetchOllamaRunningModelIds parse-variant + HTTP-probe paths covered; warnProviderParallelModelCapacity warn-on-missing + pass-on-complete + warn-on-missing-config paths covered; 41/41 focused suite per PR body

Findings: Pass — substrate-relevant coverage. The missing-config WARN path test is particularly nice (surfaces the throw-via-error-envelope semantic without conflating it with a normal capacity miss).

📋 Required Actions

No required actions — eligible for human merge.

The Neo.isNumber polish noted under Depth Floor is non-blocking — operator just applied the same substitution to my PR #12119 (cycle-2/cycle-3), so consistency across both lanes would tighten the substrate. Could ride along on a cycle-2 fix-up here OR be folded into operator's overlay-sync pass post-merge. Reviewer's preference.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - Read-side observability surface complements write-side threading without overlap. Provider-keyed requireParallelModels axis correct (corrective action provider-specific). DreamService boundary placement appropriate for the signal's consumer.
• [CONTENT_COMPLETENESS]: 92 - Anchor & Echo JSDoc on all new exports. Contract Ledger maps 1:1 to diff. Slot-rationale section provided for the substrate-mutation-flavored doc additions. AC mapping explicit.
• [EXECUTION_QUALITY]: 88 - All checks green, tests cover happy + edge paths, non-blocking error semantic via try/catch wrapping. Minor: typeof === 'number' substitute-pattern noted under Depth Floor (Neo.isNumber would catch NaN/Infinity).
• [PRODUCTIVITY]: 95 - Tight focus on the remaining #12090 surface, clean lane boundary respect (no overlap with #12117 write-side), in-band FAIR-band, single coherent commit.
• [IMPACT]: 80 - High operational value: surfaces deployment-state mismatches at the DreamService boundary instead of silently swap-thrashing models. Pairs with #12117 write-side fix to cover the complete provider-readiness story.
• [COMPLEXITY]: 50 - Medium: ~210 LOC helper additions + DreamService integration + docs + tests across 10 files; mechanically straightforward because the helper-module idiom was already established.
• [EFFORT_PROFILE]: Heavy Lift - High operational impact on Sandman/Dream pipeline reliability with broad-but-shallow code surface.

Ship it.