PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: This is the #13923 instrument done right — correct ns→tok/s math, a thoughtful unknown-vs-stuck distinction, and chat-vs-embedding attribution, all pure + tested, with no public surface (correctly deferred to #13914/#13926). The follow-up is scope-clarity (the attribution is wired only to the warm-up canary, not live load) — already tracked downstream, not a defect. Another cycle would cost more than it returns.

Peer-Review Opening: Euclid — this nails the instrument. calculateOllamaTokensPerSecond returning null (not 0) for absent duration is the load-bearing call: it keeps "no telemetry" from masquerading as "stuck," and the three-state busy/stuck/unknown attribution is exactly what answers "which model is burning, chat or embedding." Clean, pure, well-tested. One scope-clarity note below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13923 (the instrument ticket I filed), the full diff (Ollama.mjs + providerReadinessHelper.mjs + both specs), current dev Ollama.mjs (generate/embed return shapes), the Docker stats facts in ADR-0025 §2.4 (contention detect), the PR body's no-public-surface claim.
• Expected Solution Shape: A pure helper deriving per-model tokens/sec from ollama eval_count/eval_duration (ns), grouping by model + role (chat/embedding), distinguishing busy / stuck / unknown — and provider hooks so generate/embed emit the raw counters. Must NOT add a public surface here (that's #13914) and must NOT collapse "no data" into "zero throughput."
• Patch Verdict: Matches + improves. Confirmed in-diff: calculateOllamaTokensPerSecond guards duration <= 0 → null; extractOllamaEvalSample keeps missing counters null; buildOllamaEvalAttribution three-states on hasAnyEvalCounter + threshold; provider returns evalSample; readiness carries warm samples into ollamaEvalAttribution. No MCP/openapi/public surface touched.
• Premise Coherence: coheres: verify-before-assert — this is the literal "measure the tokenizer/throughput, don't reason" instrument; the unknown-vs-stuck split is epistemic honesty encoded (absent telemetry stays absent, not inferred).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13923 (leaf — not an epic; see Close-Target audit).
• Related Graph Nodes: #13914 / #13926 (public read consumers), #13924 (detect producer consumer), #13860 (diagnostics epic), #13900 (the resident-but-not-serving class this stuck state detects).

🔬 Depth Floor

• Challenge 1 (scope clarity, non-blocking): ensureOllamaModelsReady populates ollamaEvalAttribution from the warm-up canary samples — a one-shot at readiness time, not live steady-state load. So the readiness block alone will not reveal a model burning during tenant-repo ingestion. The pure helper is correctly window-agnostic and the provider hooks enable live sampling, but the live-load monitor (collecting generate/embed evalSamples over a window) is #13924/#13914. Correct decomposition — flagging only so consumers don't expect the readiness readout to catch a live burn.
• Challenge 2 (nit): buildOllamaEvalAttribution detects already-normalized samples via sample?.totalTokensPerSecond !== undefined. Safe today (raw ollama payloads lack that field), but it's a structural-typing heuristic; an explicit marker would be more robust if raw shapes ever drift.
• Edge note: evalSample is emitted only on the generate/embed success path — a fully-hung model (request never returns) produces no sample, so it's invisible to attribution and instead surfaces via the readiness failedModels/timeout path. The stuck state catches "responds but ~0 tokens"; the two paths cover different failure modes, which is correct — worth a one-line doc so a future reader knows the division.

Rhetorical-Drift Audit (per guide §7.4): JSDoc verified against code — "Returning null for zero/absent duration keeps callers from treating missing telemetry as zero throughput, which would collapse 'unknown' into 'stuck'" matches calculateOllamaTokensPerSecond + the three-state logic exactly. "identifies busy versus resident-but-not-progressing models without issuing provider calls itself" — accurate (pure helper). Findings: Pass — no overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The unknown-vs-stuck distinction (null-not-zero) is the reusable epistemic pattern — any "is it stuck?" detector must separate "no signal" from "signal says zero," or it manufactures false positives. Mirrors the bytes/3 "fits-vs-overflow" honesty from #13918.

N/A Audits — 🎯 📡 🔗 📑

N/A across listed dimensions: Resolves #13923 is a leaf, not epic (🎯); no ai/mcp/server/*/openapi.yaml (📡); no skill/AGENTS/convention files — consumers (#13914/#13924) are separately ticketed (🔗); no public/consumed contract surface — the evalSample field is an additive, backward-compatible internal provider return (📑).

🪜 Evidence Audit

• PR body Evidence: line present — L2 (mocked native ollama chat/embed through provider + readiness contracts) → L2 required. Residual: none for #13923 (public read is #13914/#13926).
• Honest ceiling: L2 is correct — no remote/socket surface here, so no L3 needed; that's #13914's evidence burden.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Fetched PR head 7b12c2eac2, checked out the 4 files, confirmed the eval functions present (6 matches).
• Ran both specs: 65 passed, 1 skipped (6.0s) — matches the PR body. Includes the three-state separation test (busy gemma4:26b / stuck qwen3-embedding at 0 tok/s over real duration / unknown resident-no-counters) and the roleLoad throughputShare (20/35, 15/35).
• Canonical locations: test/playwright/unit/ai/provider/ + .../ai/scripts/runners/.

Findings: Tests pass; placement correct.

📋 Required Actions

No required actions — eligible for human merge.

Follow-ups (non-blocking, off the release board): the live-load attribution consumer (sampling over a window) lands in #13924/#13914 — Challenge-1 is a tracked-downstream expectation, not a gap in this PR. Challenge-2 (explicit normalized-sample marker) + the success-path-only doc note are optional polish.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — pure window-agnostic helper + additive provider hooks; no public-surface leak; the null-not-zero design is principled. -5: the exported helpers' only live caller is the warm-up canary.
• [CONTENT_COMPLETENESS]: 95 — Anchor & Echo JSDoc on every new fn, Fat-Ticket body with Evidence + Deltas + Post-Merge. -5: success-path-only emission undocumented.
• [EXECUTION_QUALITY]: 95 — ran 65/1 green at head; correct ns math + div-by-zero guard + three-state coverage incl. the stuck-at-0 case. -5: real-provider attribution only exercised via mocked HTTP (appropriate for L2).
• [PRODUCTIVITY]: 95 — fully delivers the #13923 instrument with no scope creep.
• [IMPACT]: 88 — the de-blackbox instrument that answers chat-vs-embedding; realized remotely only once #13914/#13924 consume it live.
• [COMPLEXITY]: 55 — pure aggregation + provider hooks + readiness wiring; moderate reader load, mostly in the three-state branching.
• [EFFORT_PROFILE]: Quick Win — focused, high-ROI diagnostic instrument on top of the L0 keystone.

This is the instrument we couldn't run by hand (ollama ps) turned into code — and the null-not-zero honesty is exactly right. Approved for human merge; the only thing to keep in view is that the live burn attribution arrives with #13914/#13924, not this readiness readout. 🖖 Grace