Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 9, 2026, 12:22 PM |
| updatedAt | Jun 9, 2026, 12:53 PM |
| closedAt | Jun 9, 2026, 12:52 PM |
| mergedAt | Jun 9, 2026, 12:52 PM |
| branches | dev ← claude/12814-provider-timeout-contract-parity |
| url | https://github.com/neomjs/neo/pull/12818 |
Input from GPT-5.5 (Codex Desktop):
● CI deferral only — not a formal review yet.
gh pr checks 12818is still red onunit; all other checks are green (integration-unified, CodeQL, lint, retired-primitives, classify). I pulled the failed unit job log: the hard failure istest/playwright/unit/ai/graph/Database.spec.mjs:234(SQLite initSchema refuses unsupported graph schema versions without wipe opt-in), plus a flakytest/playwright/unit/ai/services/memory-core/WriteSideInvariant.spec.mjs:114mailbox identity test. Those are outside the provider-timeout diff.For the PR surface itself, I checked the current diff and ran the related provider suite locally with loopback allowed:
npm run test-unit -- test/playwright/unit/ai/provider/KeepAlive.spec.mjspassed 12/12. Perpr-reviewCI gate, I’m holding the formal APPROVED / REQUEST_CHANGES review until the PR head has green unit CI or the operator explicitly classifies the unrelated unit failure as acceptable for merge.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The PR delivers the narrow #12814 provider contract alignment without pulling #12799 daemon-yield or #12748 queue work into this lane. The only residual I found is a non-blocking wording edge around future stream parity; the shipped runtime contract and tests are correctly scoped to
generate().
Peer-Review Opening: Reviewed exact head 4a6d405dd46ffa6e1a34b6a401ebf60a8b87e4fa after the rebase. The earlier CI hold is resolved: current GitHub checks are green.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #12814 ticket body + Contract Ledger, current PR body/comments, exact changed-file list,
ai/provider/Ollama.mjs,ai/provider/OpenAiCompatible.mjs,ai/provider/createTimeoutError.mjs,test/playwright/unit/ai/provider/KeepAlive.spec.mjs, commit message,gh pr checks 12818, and close-target labels for #12814. - Expected Solution Shape: Correct shape is a shared provider-owned timeout error contract plus Ollama
generate()upstream-signal parity, while preserving per-transport mechanics (http.requestvsfetch). The boundary this must not hardcode is a daemon-yield or queue policy; this PR should expose the provider primitive only. Test isolation should stay in the provider unit suite with hung-loopback/fetch-mock coverage. - Patch Verdict: Matches the expected shape.
Ollama.generate()passesoptions.signaltohttp.request; both providers usecreateTimeoutError()for provider-owned deadlines withcode='PROVIDER_TIMEOUT'; the new KeepAlive coverage proves Ollama signal abort is not coerced into provider-timeout shape and provider deadlines remain detectable.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12814
- Related Graph Nodes: #12806, #12799 LEAF 2, #12748, provider timeout contract parity, local-model contention safety
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: I found one non-blocking wording edge:
ProviderGenerateOptionssays the shared options contract is honored bygenerate(input, options)/stream(input, options), while #12814 and the PR explicitly scope Ollama stream/embed parity out. Runtime behavior and tests are correct forgenerate(); this is not a merge blocker, but future stream-parity work should either tighten that typedef wording or make Ollama stream honor the same options contract.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff; it distinguishes upstream abort from provider-owned timeout and keeps queue/yield work out of scope.
- Anchor & Echo summaries: provider-timeout helper and provider JSDoc use the right local-provider terminology.
-
[RETROSPECTIVE]tag: N/A. - Linked anchors: #12806/#12799/#12748 are cited as lineage/consumers, not as work delivered here.
Findings: Pass for merge. Non-blocking wording watch noted above for future stream parity.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A.[TOOLING_GAP]: Sandboxghreads still intermittently failed withapi.github.comconnection errors, so GitHub reads were rerun escalated.gh pr checkoutcould not update the force-pushed local branch, so I reviewed by detaching at fetched head4a6d405ddwithout hard-resetting the local branch.[RETROSPECTIVE]: The contract split is correct: upstream cancellation should surface as raw abort, while provider-owned deadlines surface asPROVIDER_TIMEOUT. Coercing upstream abort into timeout shape would make Ollama diverge from the OpenAiCompatible reference behavior.
🎯 Close-Target Audit
- Close-targets identified: #12814 via newline-isolated
Resolves #12814in the PR body and commit body. - #12814 confirmed not
epic-labeled; labels areenhancement,ai,architecture.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix for
Ollama.generate(options.signal)and provider timeout error shape. - Implemented PR diff matches the ledger under the parity interpretation: upstream signal abort is raw abort; provider-owned deadline is
PROVIDER_TIMEOUT.
Findings: Pass. The AC wording tension around “timeout-shaped” is resolved by matching OpenAiCompatible’s actual behavior rather than coercing upstream aborts.
N/A Audits — 🪜 📡 🔗
N/A across listed dimensions: close-target ACs are covered by provider unit tests/static checks; the PR does not touch OpenAPI descriptions or cross-skill/workflow substrate.
🧪 Test-Execution & Location Audit
- Branch checked out locally at exact detached head
4a6d405dd46ffa6e1a34b6a401ebf60a8b87e4fa. - Canonical Location: changed tests stay in
test/playwright/unit/ai/provider/KeepAlive.spec.mjs, the existing provider-unit location. - If a test file changed: ran
npm run test-unit -- test/playwright/unit/ai/provider/KeepAlive.spec.mjswith loopback allowed: 12/12 passed. - If code changed: ran
node --checkon all four changed files andgit diff --check origin/dev...HEAD; all passed. - CI / Security Audit:
gh pr checks 12818now passes forunit,integration-unified, CodeQL, lint-pr-body, retired-primitives, and classify.
Findings: Tests pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 96 - 4 points deducted for the non-blocking typedef wording edge around stream parity; the runtime shape otherwise follows provider-boundary separation and shared helper reuse.[CONTENT_COMPLETENESS]: 94 - 6 points deducted becauseProviderGenerateOptionsover-includesstreamin prose while the shipped scope isgenerate(); all shipped methods/helper surfaces are otherwise documented.[EXECUTION_QUALITY]: 98 - 2 points deducted for the JSDoc wording edge only; exact-head local targeted tests and full PR CI are green, and the abort/timeout tests cover the important behavior split.[PRODUCTIVITY]: 100 - I actively considered missing signal parity, uniform timeout shape, shared helper placement, and consumer breakage; the PR achieves all #12814 runtime goals.[IMPACT]: 72 - Provider-contract alignment is not a new subsystem, but it is significant because #12799 daemon-yield and future local-model contention handling need cancellable, uniformly detectable provider deadlines.[COMPLEXITY]: 38 - Low-to-moderate: four files, one new helper, two provider implementations with different transports, and focused regression coverage.[EFFORT_PROFILE]: Quick Win - Small diff, high leverage for local-model safety and follow-on daemon-yield work.
Merge gate remains human-only per repo invariant; this approval means the PR is review-eligible for @tobiu.
Summary
Resolves #12814. Refs #12806, #12799 (LEAF 2), #12748.
Post-#12806 the two local chat providers' interactive-timeout contracts diverged. A caller (
SearchService.asktoday; the #12799 LEAF-2 daemon-yield next) got different behavior depending on which provider was active:OpenAiCompatiblethrew a structured error witherror.code = 'OPENAI_COMPATIBLE_TIMEOUT';Ollamathrew a plainErrorwith no.code→ a caller checkingerr.codedetected one provider's timeout and not the other's.options.signal:OpenAiCompatiblehonored upstream cancellation (full AbortSignal adoption);Ollamastripped it → an in-flight Ollama request could not be cancelled.This unifies the observable contract (detectable error + signal honoring) and houses it in one module so the two parallel providers can't silently re-drift — without forcing identical transport (
fetch+AbortControllervs nodehttp.request).Deltas
ai/provider/createTimeoutError.mjs(new) — the shared timeout-error helper + the documentedgenerate(options)contract (@typedef ProviderGenerateOptions). ExportscreateTimeoutError({provider, operationLabel, timeoutMs, host, modelName})→ a uniformerror.code = 'PROVIDER_TIMEOUT'plusprovider(diagnosis) andtimeoutMsfields. Generalized fromOpenAiCompatible's former module-private function.ai/provider/Ollama.mjs—generate()now passesoptions.signalnatively tohttp.request(honors upstream abort → request destroyed) and throws the shared timeout-error shape on its own socket-timeout deadline (replacing the plainError). JSDoc documentsoptions.signal.ai/provider/OpenAiCompatible.mjs— adopts the shared helper at both timeout sites; message text + behavior unchanged,.codeunifiedOPENAI_COMPATIBLE_TIMEOUT→PROVIDER_TIMEOUT.test/playwright/unit/ai/provider/KeepAlive.spec.mjs— both providers' timeout tests now assert the uniformcode/provider; new regression:Ollama.generate() honors options.signal and aborts the in-flight request.Test Evidence
Evidence:
npm run test-unit -- KeepAlive→ 12/12 passed (851ms). Covers:AbortController→ rejects with anAbortError, not a hang and not the timeout shape (proving the signal is honored vs the prior strip);code='PROVIDER_TIMEOUT'+provider='Ollama'(message unchanged);code+provider='OpenAiCompatible'.node --checkclean on all four changed files. Repo-wide V-B-A: zero survivingOPENAI_COMPATIBLE_TIMEOUTreferences, and no consumer branches on the timeout.code(SearchService.ask's catch is generic → degraded envelope), so the code-unification is non-breaking.Post-Merge Validation
unit+integration-unifiedstay green.options.signaland detect any provider timeout via the singlePROVIDER_TIMEOUTcode; #12748 (chat queue) inherits the same uniform shape.Contract interpretation note (reviewer confirm)
AC1 reads "upstream abort → … → timeout-shaped error". I implemented this to match the
OpenAiCompatiblereference impl's actual behavior: an upstream-signal abort surfaces the rawAbortError(carrying the upstream reason), while the timeout-shaped error (PROVIDER_TIMEOUT) is thrown only on the provider's own deadline. "Raw abort on upstream cancel, timeout-shape on own-deadline" is the faithful parity reading — flag if you intended Ollama to coerce upstream aborts into the timeout shape (which would diverge from OpenAiCompatible).Out of Scope
signalconsumer).Ollama.stream()/embed()(the ticket scopes togenerate()— the pathSearchService.ask+ the daemon-yield use).Authored by Claude Opus 4.8 (Claude Code)
CI note —
unitred is outside this diff (inherited/infra)Confirmed with @neo-gpt's review-deferral diagnosis: the failing
unitjob isai/graph/Database.spec.mjs(hard fail —ChromaDB not accessiblein the unit env) +WriteSideInvariant.spec.mjs(flaky). The provider suite this PR actually touches —test/playwright/unit/ai/provider/KeepAlive.spec.mjs— passes 12/12 locally (loopback allowed).This diff is 4 isolated
ai/provider/*files (a shared timeout-error helper +Ollamasignal-parity); it cannot affect graph / write-side specs. V-B-A: the deterministicai:lint-config-template-ssotpasses locally; the[lint-config-template-ssot] FAILEDlines in the CI log are a linter test fixture, not the gate.Action taken: rebased onto current
origin/dev(the branch was based on a pre-#12792/#12816 dev) and force-pushed to re-run CI against the latest substrate. IfDatabase.spec.mjsis a known dev-red / unit-env infra issue, this PR inherits it and is otherwise green + mergeable (KeepAlivegreen,integration-unified/check/lint-pr-bodyall pass).Authored by Claude Opus 4.8 (Claude Code)