PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back (cycle-1 premise pre-flight: no premise-invalid / not-graduated / bypassed / anti-pattern / misalignment / better-substrate / stale-source trigger fires — so not Drop+Supersede):

• Decision: Approve+Follow-Up
• Rationale: Approve because it ships the per-call timeout bound + detect signal for the production embedding path (#14036's core — the freeze/runaway vector behind #13999), correct + ADR-0019-clean + test-verified. Follow-Up rather than Request Changes because the only gap (the native-ollama client embed path being unbounded) is a non-default path, better-tracked as a confirm/fast-follow than an incremental cycle, and does not gate the production-path value.

Peer-Review Opening: Thanks for this — it's the clean upstream complement to #13551's WAL-backlog watchdog: a bounded per-call timeout that converts a silent embed freeze into a machine-readable signal. Notes below; one non-blocking follow-up question.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: the full PR diff, #14036 AC, config.template.mjs embeddingProvider/openAiCompatible leaves, TextEmbeddingService provider routing (embedTexts, the line-101 Ollama dispatch), and the retry spec.
• Expected Solution Shape: bound each embed call with a duration timeout, abort + emit a signal on timeout, as a proper ConfigProvider leaf with fail-loud validation; tests proving the bound is enforced, distinct from the interactive contention timeout, and that the friction fires. Must not hardcode a deployment population assumption.
• Patch Verdict: Matches for the production path. batchEmbeddingTimeoutMs (300s) is a proper leaf; assertPositiveTimeoutMs fails loud on an invalid resolved value; requestTimeoutMs is threaded into the batch enqueue; #emitOpenAiCompatibleTimeoutFriction emits a structured timeout ConsumerFriction (post-invocation-failure), aggregated.
• Premise Coherence: Coheres with verify-before-assert + friction→gold — it converts an embed freeze into a per-call upstream detect signal. Production embeddingProvider defaults to openAiCompatible (config.template.mjs:157), so the bounded path IS the production path; the klarso ollama-backed freeze reaches the provider through it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14036
• Related Graph Nodes: #13999, #14027, #13551 (WAL-backlog watchdog — the aggregate signal this per-call signal explains), #12838/#12840 (deferred-embed WAL), ADR-0025

🔬 Depth Floor

Challenge: #14036 says "bound EACH embed call." This PR bounds the openAiCompatible path (the production default ✓). The native-ollama client embed dispatch (TextEmbeddingService.mjs:101, reachable via embedTexts(texts, 'ollama')) is not bounded here. Since production defaults to openAiCompatible the freeze vector is covered — but if any deployment routes embeddings through the native-ollama client, that path can still hang unbounded. Non-blocking, raised as the follow-up below.

Rhetorical-Drift Audit: PR framing ("bound each batch request", "must not hold the provider queue forever") matches what the diff substantiates (300s bound + timeout-friction); no overshoot. Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — correct use of the ConsumerFriction primitive and the ConfigProvider leaf pattern.
• [TOOLING_GAP]: None observed.
• [RETROSPECTIVE]: The per-call timeout+friction is the upstream "why-the-drain-stalled" signal that pairs with #13551's downstream "the-drain-is-stalled" watchdog — together they bracket the metadata-without-vector pathway.

N/A Audits — 📑 📡

N/A across listed dimensions: the PR adds one additive, defaulted config leaf + uses the existing emitConsumerFriction helper (no new/modified consumed contract requiring a ledger); it touches no openapi.yaml MCP-tool surface.

🎯 Close-Target Audit

• Close-targets identified: #14036
• #14036 confirmed not epic-labeled (enhancement/ai/testing/architecture).

Findings: Pass. Core AC (bound each call + signal on timeout + abort) met for the production path; the CPU-saturation arm is "and/or" (optional), satisfied by the timeout-watchdog arm. See the Depth Floor challenge for the native-ollama residual.

🪜 Evidence Audit

Findings: N/A — #14036's observable AC (a bounded embed call that times out + emits a signal) is covered at L2: the timeout-all test proves the 25ms bound rejects, the renamed test proves batch uses the batch bound (not contention), and the new (#14036) test asserts 3 timeouts → one aggregated friction (count: 3). No runtime surface beyond unit reach.

🔗 Cross-Skill Integration Audit

Findings: All checks pass — no integration gaps. A new env-driven config leaf following the existing openAiCompatible.* pattern; introduces no new MCP tool, skill convention, or predecessor-step linkage.

🧪 Test-Execution & Location Audit

• Canonical Location: test/playwright/unit/ai/services/memory-core/TextEmbeddingService.retry.spec.mjs — correct unit tree.
• Branch NOT checked out locally — reviewed via the diff. The bound + friction are asserted in the added/renamed tests (read in the diff); I did not re-run the suite locally.

Findings: Tests in canonical location; coverage reads as sufficient for the bound + signal. (Honest evidence ceiling: diff-read, not a local re-run.)

📋 Required Actions

No merge-blocking actions — eligible for human merge on the production path.

Follow-up (non-blocking): confirm whether the native-ollama client embed path (embedTexts(texts, 'ollama') / the line-101 dispatch) is reachable in any deployment. If yes → a fast-follow ticket for the symmetric timeout-bound there completes #14036's "each call" coverage; if it's non-production → #14036 is fully resolved as-is. Your call as author.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - The right immune-system per-call detect primitive (timeout + ConsumerFriction), ADR-0019-clean, pairs with #13551.
• [CONTENT_COMPLETENESS]: 80 - Production path fully covered; the native-ollama client path is the residual behind the follow-up.
• [EXECUTION_QUALITY]: 88 - Fail-loud assertPositiveTimeoutMs, end-to-end timeout enforcement test, friction-aggregation assertion.
• [PRODUCTIVITY]: 85 - Focused, bounded change closing a release-critical freeze vector.
• [IMPACT]: 88 - Bounds a #13999-class freeze/runaway (CPU saturation + WAL stall) for the production embedder + makes it observable.
• [COMPLEXITY]: 45 - Small surface, but the timeout/friction semantics cross the shared embed path.
• [EFFORT_PROFILE]: Quick Win - Small patch, high-value detect primitive.

Solid work — approving; just confirm the ollama-path scope for the #14036 close.

Reviewed by Vega (Claude Opus 4.8).