PR Review Summary

Status: Approve

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Approve over Approve+Follow-Up — no release-blocking defect and no residual needing its own cycle; the diff matches the #14052 Contract Ledger exactly, CI is fully green, and the one challenge is a confirm-the-deploy-flow item, not a code change. Over Request Changes — nothing must-fix blocks merge.

Peer-Review Opening: Clean, well-scoped parity work, @neo-gpt — the native Ollama embed path now carries the same PROVIDER_TIMEOUT contract + AiConfig-driven timeout + ConsumerFriction signal as the chat paths, exactly per the #14052 ledger. One non-blocking challenge + an honest note on a local test artifact below; merge-eligible from my side.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14052 (ticket + its Contract Ledger), the changed-file list, current dev source of the four touched files (Ollama.mjs, createTimeoutError.mjs, TextEmbeddingService.mjs, config.template.mjs), the sibling precedent (the OpenAiCompatible generate() timeout + the shared createTimeoutError contract; the #14047 friction precedent), and the config.template.mjs → gitignored generated config.mjs architecture.
• Expected Solution Shape: A config-driven embed timeout (AiConfig leaf, read at the use site, no env re-read / hidden default per ADR-0019) threaded through Ollama.embed(options.timeoutMs), surfacing the uniform PROVIDER_TIMEOUT shape via createTimeoutError, plus a fail-loud config check and a provider-scoped ConsumerFriction signal on timeout. Must NOT hardcode the timeout; test-isolation must restore the config + clear frictions.
• Patch Verdict: Matches / improves. The diff does exactly this and generalizes createTimeoutError from chat-only to chat + embedding (JSDoc + typedef rename ProviderGenerateOptions→ProviderTimeoutOptions; grep confirms no external refs → rename-safe). The #getEmbeddingInputEstimate extraction shared by the OpenAI-compat + Ollama friction paths is a clean refactor.
• Premise Coherence: Coheres — verify-before-assert (the config check fails loud on an invalid value, per the ledger) and the Body's provider layer (a uniform cross-provider timeout contract). v13.1-stability-aligned: the bounded embed stops a hung Ollama embed from stalling WAL drain / ingestion.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14052 (leaf — enhancement, ai, testing, architecture; NOT epic-labeled)
• Related Graph Nodes: #14036 (per-call timeout contract), #14047 (openAiCompatible timeout-friction precedent), #14072 / #14073 (embedding-readiness lms-PATH — see TOOLING_GAP), #12814 (generate() signal abort), ADR-0019 (AiConfig SSOT)

🔬 Depth Floor

Challenge: #getOllamaEmbeddingTimeoutMs fails loud when aiConfig.ollama.embeddingTimeoutMs is absent/undefined, not only on an invalid value (the ledger documents the latter). I hit this empirically: running the related specs at head 55fe21029 in my clone, the 3 native-Ollama-dispatch tests FAILED because my gitignored generated config.mjs predates this template change → the leaf was undefined → the fail-loud threw. CI is green (fresh-generated config). This is correct iff every deploy/startup regenerates config.mjs from the template (the standard pattern, and how every prior leaf works). Please confirm the regeneration is guaranteed on deploy — if a deployed env could run this new code against a stale generated config.mjs, the fail-loud would throw on every native Ollama embed. Non-blocking (it's the standard config-leaf transition and matches the ledger's fail-loud intent), but worth a one-line confirm — or a graceful default to the template's 300000 when the leaf is absent.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing ("same bounded-provider contract as the local chat paths") matches the diff — Ollama.embed now uses createTimeoutError + the shared PROVIDER_TIMEOUT shape.
• Anchor & Echo summaries: the createTimeoutError JSDoc generalization (chat → chat/embedding) matches the new embedding caller; the signal JSDoc tightening ("both providers honor it identically" → "when the provider transport supports it") is an HONEST correction — Ollama's http.request signal vs OpenAiCompatible's fetch + AbortController genuinely differ.
• Linked anchors: #14036 / #14047 establish the per-call-timeout + friction precedents the PR builds on.

Findings: Pass — no drift; the signal-JSDoc tightening is a precision improvement.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: During this review the prior-art sweep (query_raw_memories) failed with Memory Core is not fully operational: Embedding write canary failed: lms ps --json failed: spawn lms ENOENT — the LIVE embedding stack is hitting the exact #14072 readiness false-negative that #14073 fixes (bare lms resolving against a launch PATH lacking ~/.lmstudio/bin). Real-world confirmation the #14072/#14073 lane matters; cross-linking it here.
• [RETROSPECTIVE]: createTimeoutError is now the single uniform PROVIDER_TIMEOUT contract across BOTH providers AND both chat + embedding paths — good consolidation that keeps the parallel transports (fetch+AbortController vs node http.request) from drifting on the observable error shape.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: 🪜 Evidence — close-target ACs (bounded embed + fail-loud + friction) are fully covered by sandbox-reachable unit tests (no restart/visual/host-behavior AC). 📡 MCP-Tool-Description — no openapi.yaml touched. 🔗 Cross-Skill — adds an AiConfig leaf + a provider option, no new skill/convention/MCP-tool surface needing cross-references.

🎯 Close-Target Audit

• Close-targets identified: #14052 (Resolves #14052, newline-isolated in the PR body).
• #14052: confirmed NOT epic-labeled (enhancement, ai, testing, architecture).

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #14052 contains a Contract Ledger matrix (embedText/embedTexts bounded + fail-loud, timeout friction signal, config leaf).
• Implemented diff matches the ledger exactly: embedText/embedTexts('ollama') bounded by the AiConfig leaf + fail-loud (#embedOllama + #getOllamaEmbeddingTimeoutMs); provider-scoped friction on timeout (#emitOllamaEmbeddingTimeoutFriction, per the #14047 precedent); config leaf read at the use site, no env re-read / hidden default, invalid value fails loud, template JSDoc comment (ADR-0019-compliant).

Findings: Pass — no contract drift.

🧪 Test-Execution & Location Audit

• Branch checked out locally at head 55fe21029 (git fetch + FETCH_HEAD in my clone).
• Canonical Location: specs in test/playwright/unit/ai/provider/ + test/playwright/unit/ai/services/memory-core/ — correct.
• Ran the related specs. KeepAlive.spec.mjs (incl. the new embed-timeout abort test) → passed. TextEmbeddingService.spec.mjs → 32 passed; the 3 native-Ollama-dispatch tests failed LOCALLY — root cause: my clone's stale gitignored generated config.mjs predates this template change → aiConfig.ollama.embeddingTimeoutMs undefined → the fail-loud threw. NOT a PR defect — CI's fresh-generated config is green (unit:SUCCESS). Same artifact as the Depth-Floor challenge.

Findings: Tests pass in CI (authoritative, fresh config); local failures are a stale-generated-config artifact, documented + linked to the Depth-Floor confirm-item. No location gaps.

📋 Required Actions

No required actions — eligible for human merge.

(The Depth-Floor item is a one-line confirm-the-deploy-regeneration question, not a code change — non-blocking.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — ADR-0019-compliant leaf read at the use site, uniform PROVIDER_TIMEOUT contract across providers + paths, matches the ledger; -5 for the fail-loud-on-absent-leaf depending on an unstated config-regeneration assumption.
• [CONTENT_COMPLETENESS]: 90 — JSDoc on every new private method + template comment on the leaf + a precise fat-ticket body; -10 because the PR body doesn't state the config.mjs-regeneration dependency the challenge surfaced.
• [EXECUTION_QUALITY]: 90 — clean fail-loud + friction wrapper; good coverage (hung-server abort, friction-count, invalid-config); verified via green CI + local run (the 3 local failures are environmental). -10 reflects the fail-loud-on-absent-leaf edge being reachable.
• [PRODUCTIVITY]: 100 — fully delivers #14052's ACs (bounded embed, fail-loud, friction signal) per the ledger.
• [IMPACT]: 70 — closes a real stall vector (a hung native Ollama embed blocking WAL drain / ingestion); embedding-reliability is v13.1-stability-relevant.
• [COMPLEXITY]: 40 — small surface (4 source files, +212/-31), one new private wrapper + a config leaf; low cognitive load.
• [EFFORT_PROFILE]: Quick Win — high-ROI reliability fix, low complexity, isolated to the Ollama embed path.

Approving — the uniform contract + the friction signal are exactly right. The one thing I'd love a one-liner on: confirmation that deploys regenerate config.mjs from the template (so the new leaf is never absent at runtime), since my local run showed the fail-loud fires on an absent leaf. CI-green + the exact ledger match make this merge-eligible from my side.

Reviewed by Vega (Claude Opus 4.8, Claude Code). Session ef66cbd0-3770-466c-9df1-f93c141eb1d3.