feat(api): pass through abort signal to all providers#730
Draft
easonLiangWorldedtech wants to merge 16 commits into
Draft
Conversation
…nfiguration Body: Implement generic request configuration builder with chainable methods (addAbortSignal, addHeaders, setOption), static factory methods (fromMetadata, mergeAbortSignals), and 40 unit tests.
… and multi-SDK examples
…ls early-abort - Fix README TOC: change #how-mergesignals-works to #how-mergeabortsignals-works to match the actual heading anchor - Simplify mergeAbortSignals: return primarySignal directly when it's already aborted instead of creating a new AbortController
…onfigBuilder (Zoo-Code-Org#615) - Add default empty object parameter to addHeaders() so calling with undefined no longer throws TypeError from Object.keys(undefined) - Reorder mergeAbortSignals to check primarySignal.aborted before allocating AbortController, preventing unnecessary controller creation
…roviders - anthropic.spec.ts: timeout trigger test + signal instance verification - native-ollama.spec.ts: explicit assertion no second arg passed (no signal forwarding) - openai-codex-native-tool-calls.spec.ts: removed weak toHaveProperty(signal) assertion - vercel-ai-gateway.spec.ts: temperature test uses correct undefined second arg fix: add timeoutMs forwarding to completePrompt methods - vercel-ai-gateway.ts: use Object.keys(createOptions).length > 0 check instead of truthy check - poe.ts: merge signal and timeoutMs properly with combined abort logic - config-builder/README.md: update documentation for mergeAbortSignals behavior test: add timeoutMs coverage for poe, moonshot, minimax, mistral, xai providers - poe.spec.ts: signal+timeoutMs merge, timeoutMs only, timeoutMs=0 cases - moonshot.spec.ts: same timeoutMs tests for openai-compatible pattern - minimax.spec.ts: signal+timeoutMs, timeoutMs only, truthy check behavior - mistral.spec.ts: same timeoutMs coverage - xai.spec.ts: signal+timeoutMs, timeoutMs only, truthy check behavior test: add timeoutMs coverage for anthropic-vertex, base-openai-compatible, bedrock, openai-native - anthropic-vertex.spec.ts: signal passing test (no timeoutMs support) - base-openai-compatible-provider-timeout.spec.ts: completePrompt with signal+timeoutMs, timeoutMs only, truthy check behavior - bedrock.spec.ts: timeoutMs coverage for adaptive thinking path - openai-native.spec.ts: signal and timeoutMs merging tests test: add signal+timeoutMs merge tests for fireworks, lite-llm, lmstudio - fireworks.spec.ts: added merge signal and timeoutMs together test - lite-llm.spec.ts: added same combined signal+timeoutMs test - lmstudio.spec.ts: aligned with same abort signal pattern fix: replace tautological assertion in poe.spec.ts - poe.spec.ts: completePrompt should prefer signal over timeoutMs test now asserts abortSignal is a distinct AbortSignal from controller.signal instead of always-true instanceof check test: add missing error catch and timeout cleanup tests - vscode-lm.spec.ts: added 'should handle errors in completePrompt' test - poe.spec.ts: added 'completePrompt should clear timeout when user signal aborts' test - opencode-go.spec.ts: added OpenAI path completePrompt tests (signal, timeoutMs, merged)
…tial-request test
…-code, lmstudio-native
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds abort-signal and timeoutMs plumbing to completion APIs, introduces a shared request-config builder, forwards the new options through provider implementations and helper layers, and expands related tests. A separate model-cache helper export and its tests are also updated. ChangesAbort and timeout request plumbing
Model cache helper
Sequence Diagram(s)sequenceDiagram
participant Task
participant SingleCompletionHandler as singleCompletionHandler
participant BaseOpenAiCompatibleProvider
participant OpenAIClient as client.chat.completions.create
Task->>SingleCompletionHandler: promptText + metadata.abortSignal
SingleCompletionHandler->>BaseOpenAiCompatibleProvider: completePrompt(promptText, options)
BaseOpenAiCompatibleProvider->>OpenAIClient: request options with signal / timeout
OpenAIClient-->>BaseOpenAiCompatibleProvider: completion text
Estimated Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
easonLiangWorldedtech
force-pushed
the
origin/feat/abort-signal-core/pass-through-providers
branch
from
71f57b1 to
dcdce75
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/providers/vscode-lm.ts (1)
585-610: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe
catchis detached — error wrapping is silently dead code.
completePromptcloses at Line 604 (try { … } finally { … }with nocatch). Lines 605-610 are therefore parsed as a separate class method namedcatch, not a catch clause for thistry. Consequently any error fromclient.sendRequest(...)/ stream consumption propagates unwrapped (theVSCode LM completion error: …wrapping never runs), and the class gains an unreachablecatch()method.Fold the handler back into the
tryso it precedesfinally:🐛 Proposed fix
return result + } catch (error: any) { + if (error instanceof Error) { + throw new Error(`VSCode LM completion error: ${error.message}`) + } + throw error } finally { if (timeoutTimeout) { clearTimeout(timeoutTimeout) } tokenSource.dispose() } } - catch(error: any) { - if (error instanceof Error) { - throw new Error(`VSCode LM completion error: ${error.message}`) - } - throw error - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/vscode-lm.ts` around lines 585 - 610, The error handling in completePrompt is detached from the try/finally block, so the wrapping logic never runs and the extra catch() becomes a separate class method. Move the catch handler back onto the completePrompt flow so it is attached to the try before finally, and keep the Error wrapping that prefixes "VSCode LM completion error:" for failures from client.sendRequest and stream consumption.
🧹 Nitpick comments (10)
src/api/providers/fetchers/__tests__/modelCache.spec.ts (1)
474-485: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the
writeModelsassertion.Since
safeWriteJsonis mocked, resolving toundefinedis nearly guaranteed and doesn't validate the persistence contract. Consider assertingsafeWriteJsonwas called with the expected<router>_models.jsonfilename under the cache dir and withmockModels, which is the behavior this test should protect.♻️ Suggested stronger assertion
- await expect(writeModels("openrouter", mockModels)).resolves.toBeUndefined() + await expect(writeModels("openrouter", mockModels)).resolves.toBeUndefined() + expect(safeWriteJson).toHaveBeenCalledWith( + expect.stringContaining("openrouter_models.json"), + mockModels, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts` around lines 474 - 485, The current `writeModels` test only checks that `writeModels` resolves, which is too weak because `safeWriteJson` is mocked. Update the `writeModels` spec in `modelCache.spec.ts` to assert the persistence contract by verifying `safeWriteJson` is called with the expected `<router>_models.json` path under the cache directory and the same `mockModels` object. Use the existing `writeModels` and `safeWriteJson` symbols to locate the behavior being tested.src/api/providers/__tests__/openai-native.spec.ts (1)
268-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate test case. The block at Lines 301-313 is an exact copy of Lines 268-280 (same title and assertions). Remove one, or repurpose the second to cover a distinct scenario.
Also applies to: 301-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/openai-native.spec.ts` around lines 268 - 280, The test in openai-native.spec.ts is duplicated with the same title and assertions as another case in the same suite, so remove one copy or change the second block to cover a different scenario. Update the duplicate test around handler.completePrompt("Test prompt") and mockResponsesCreate.mockResolvedValue so the suite keeps only one backward-compatible coverage case and the other test validates a distinct behavior.src/api/providers/__tests__/openai-codex-native-tool-calls.spec.ts (1)
568-590: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate test title. Both tests are named
"completePrompt should work without options (backward compatible)"though they assert different things (POST method vs return value). Rename one for clarity in test output.Also applies to: 592-611
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/openai-codex-native-tool-calls.spec.ts` around lines 568 - 590, There is a duplicate test title in the `completePrompt` specs, which makes the test output ambiguous because two different cases share the same name. Rename the test in the `completePrompt` block around `openai-codex-native-tool-calls.spec.ts` to a distinct, descriptive title that reflects the behavior it checks, and keep the other `completePrompt` test name aligned with its specific assertion so the suite output clearly identifies each case.src/api/providers/config-builder/request-config-builder.ts (1)
141-148: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the manual merge with
AbortSignal.any(). The current listeners keep the non-aborting signal attached until it aborts, which can retain the merged controller/closure on long-lived parents. Node 20.20.2 already supportsAbortSignal.any([primarySignal, secondarySignal]); update the tests that asserttoBe(primaryController.signal)sinceany()returns a fresh signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/config-builder/request-config-builder.ts` around lines 141 - 148, The manual merge logic in request-config-builder’s signal combining path should be replaced with AbortSignal.any([primarySignal, secondarySignal]) instead of wiring abort listeners on a new AbortController. Update the merge branch in the signal-handling function to return the combined signal from AbortSignal.any, and adjust any tests that currently expect the returned value to be the same object as primaryController.signal since any() produces a new signal instance.src/api/providers/fake-ai.ts (1)
78-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
as anycast is now redundant.Since the
FakeAIinterface declarescompletePrompt(prompt, options?)(Line 31),this.aiis correctly typed and theas anycan be dropped for full type safety:♻️ Optional refactor
- completePrompt(prompt: string, options?: import("../index").CompletePromptOptions): Promise<string> { - return (this.ai as any).completePrompt(prompt, options) + completePrompt(prompt: string, options?: CompletePromptOptions): Promise<string> { + return this.ai.completePrompt(prompt, options) }(Add
CompletePromptOptionsto the existing import from../indexand reuse it on Line 31 as well.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/fake-ai.ts` around lines 78 - 79, The `FakeAI.completePrompt` implementation no longer needs the `as any` cast because `this.ai` is already typed by the `FakeAI` interface. Remove the cast in `completePrompt` and call the method directly for type safety, and if you touch the signature, import and reuse `CompletePromptOptions` from `../index` so the interface and implementation stay aligned.src/api/providers/config-builder/README.md (1)
200-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc/implementation branch ordering differs (functionally equivalent).
The documented
mergeAbortSignalscheckssecondarySignal.abortedbeforeprimarySignal.aborted, whereas the actual implementation checks the both-aborted and primary-aborted cases first. The resulting behavior is identical, but keeping the snippet byte-for-byte aligned withrequest-config-builder.tsavoids future confusion when readers diff the two.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/config-builder/README.md` around lines 200 - 230, The documented mergeAbortSignals example is out of order compared with the actual implementation in request-config-builder.ts, even though the behavior is the same. Update the README snippet for mergeAbortSignals so its branch ordering matches the real static method exactly, including the primarySignal and secondarySignal aborted checks and the both-aborted case, to keep the docs aligned with the code.src/api/providers/lm-studio.ts (1)
206-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
createOptions || undefinedis a no-op.
createOptionsis initialized to{}and only mutated, so it is always truthy; the|| undefinedfallback can never trigger. PassingcreateOptionsdirectly is clearer (and matches the existing tests asserting{}).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/lm-studio.ts` around lines 206 - 216, Remove the unnecessary `|| undefined` fallback in `LmStudioProvider`’s chat completion call: `createOptions` in `this.client.chat.completions.create(...)` is always an object, so pass it directly. Keep the existing `createOptions` construction in this method and update the call site to use the object as-is so it matches the current tests and behavior.src/api/providers/anthropic.ts (1)
153-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant inner switch —
defaultbranch (Lines 181-182) is unreachable.This IIFE only runs inside the outer
switch (modelId)case block (Lines 92-107), whose model IDs are identical to the innercaselabels (Lines 160-175). So every model reaching here matches acaseand pushes the caching beta; thedefaultbranch can never execute. You can drop the inner switch entirely and build the options inline.♻️ Simplify
- const requestOptions = (() => { - // prompt caching: https://x.com/alexalbert__/status/1823751995901272068 - // https://github.com/anthropics/anthropic-sdk-typescript?tab=readme-ov-file#default-headers - // https://github.com/anthropics/anthropic-sdk-typescript/commit/c920b77fc67bd839bfeb6716ceab9d7c9bbe7393 - - // Then check for models that support prompt caching - switch (modelId) { - case "claude-sonnet-4-6": - ... - case "claude-3-haiku-20240307": - betas.push("prompt-caching-2024-07-31") - return { - headers: { "anthropic-beta": betas.join(",") }, - ...(metadata?.abortSignal && { signal: metadata.abortSignal }), - } - default: - return metadata?.abortSignal ? { signal: metadata.abortSignal } : undefined - } - })() + // All models in this branch support prompt caching. + betas.push("prompt-caching-2024-07-31") + const requestOptions = { + headers: { "anthropic-beta": betas.join(",") }, + ...(metadata?.abortSignal && { signal: metadata.abortSignal }), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/anthropic.ts` around lines 153 - 184, The inner switch inside the requestOptions IIFE in anthropic.ts is redundant because it duplicates the same model IDs already handled by the outer modelId switch, making the default branch unreachable. Simplify the requestOptions logic by removing the nested switch in the requestOptions block and building the options inline from the already-matched model case, while preserving the prompt-caching beta header and optional abortSignal handling.src/api/providers/unbound.ts (1)
219-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
createOptions || undefinedis a no-op.
createOptionsis always a (possibly empty) object, so|| undefinednever triggers and an empty{}is always passed as the second argument. This is functionally harmless but misleading, and inconsistent withvercel-ai-gateway.ts(Line 150), which guards withObject.keys(createOptions).length > 0 ? createOptions : undefined. Consider aligning the two for clarity. Note the existing test at Line 203 currently asserts{}, so it would need updating if you switch to the guarded form.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/unbound.ts` around lines 219 - 221, The `createOptions || undefined` fallback in `UnboundProvider`’s chat completion call is misleading because `createOptions` is always an object, so an empty object is still passed. Update the `this.client.chat.completions.create(...)` call to match the guarded pattern used in `vercel-ai-gateway.ts` by only passing `createOptions` when it has keys, and adjust the existing test around the mocked `create` call in this provider to expect `undefined` instead of `{}` when no options are set.src/api/providers/lite-llm.ts (1)
350-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
createOptions || undefinedis dead —{}is always truthy.
createOptionsis initialized to{}and never reassigned, socreateOptions || undefinedalways evaluates to the object, even when nosignal/timeoutwas set. This forwards an empty options object rather thanundefined. It's harmless for the OpenAI SDK, but it's misleading and inconsistent withxai.ts, which usesObject.keys(...).length > 0 ? ... : undefined. Consider aligning for clarity.♻️ Optional consistency tweak
- const response = await this.client.chat.completions.create(requestOptions, createOptions || undefined) + const response = await this.client.chat.completions.create( + requestOptions, + Object.keys(createOptions).length > 0 ? createOptions : undefined, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/lite-llm.ts` at line 350, The `createOptions || undefined` fallback in `LiteLLMProvider` is unnecessary because `createOptions` is always an object, so the call always passes an empty options object even when no `signal` or `timeout` is set. Update the `chat.completions.create` call in `lite-llm.ts` to match the pattern used in `xai.ts` by only passing `createOptions` when it has keys, otherwise pass `undefined`, keeping the behavior explicit and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/providers/__tests__/gemini.spec.ts`:
- Around line 188-200: The test in `gemini.spec.ts` is misleading because it
claims to verify `timeoutMs` but only asserts the `abortSignal` is passed
through `httpOptions`. Update the `completePrompt` expectation in the `should
pass timeoutMs through to client via httpOptions` test to assert the timeout
value is forwarded as well, or rename the test to match what it actually checks.
Use the `handler.completePrompt` and `client.models.generateContent` assertions
to locate the expectation.
In `@src/api/providers/gemini.ts`:
- Around line 588-597: `CompletePromptOptions.timeoutMs` is not being forwarded
into Gemini request settings, so `httpOptions` only carries `abortSignal` and
`googleGeminiBaseUrl`. Update the `generateContent` path in
`src/api/providers/gemini.ts` to map `timeoutMs` onto `httpOpts.timeout`
alongside `signal` and `baseUrl`, and keep the `GenerateContentConfig` wiring
unchanged. Also update the `gemini.spec.ts` expectation for the Gemini request
options so it asserts the `timeout` value is passed through, not just `signal`.
In `@src/api/providers/mistral.ts`:
- Around line 200-216: The request options passed from the Mistral provider are
using the wrong shape, so abort and timeout settings are not applied. Update the
`chat.complete` call in `mistral.ts` to build options using the Mistral SDK’s
`RequestOptions` format, specifically placing the abort signal under
`fetchOptions.signal` and using `timeoutMs` for the timeout. Keep the change
localized to the request-building logic around `options`, `requestOptions`, and
`this.client.chat.complete` so the SDK receives the expected fields.
In `@src/api/providers/openai-compatible.ts`:
- Around line 212-231: In openai-compatible.ts, the merged abort handling in the
generateOptions setup must handle an already-aborted options.abortSignal
immediately and must clean up the timeout. Update the branch that combines
options.abortSignal with timeoutMs so it checks abortSignal.aborted before
adding the listener and aborts the merged controller right away, then ensure the
timeoutId is cleared after generateText completes or aborts by wrapping the call
path in try/finally. Refer to the generateOptions.abortSignal merge logic and
the generateText invocation when making the fix.
In `@src/api/providers/openai.ts`:
- Around line 321-327: In completePrompt within the OpenAI provider, the
createOptions timeout assignment is using a truthy check that drops an explicit
timeoutMs of 0, causing it to diverge from the base OpenAI-compatible behavior.
Update the conditional around options.timeoutMs so the OpenAI.RequestOptions
timeout is set whenever timeoutMs is provided, including 0, while keeping the
existing abortSignal handling unchanged.
In `@src/api/providers/openrouter.ts`:
- Around line 335-343: The requestOptions logic in openrouter’s provider path is
applying the Anthropic beta header whenever abortSignal is present, which causes
the header to leak onto non-Anthropic models. Update the conditional that builds
requestOptions so the x-anthropic-beta header is added only when modelId starts
with "anthropic/", and keep the abortSignal-only case separate so non-Anthropic
requests receive only the signal. Make the gating consistent with completePrompt
so both paths use the same Anthropic-only header rule.
In `@src/api/providers/poe.ts`:
- Around line 147-167: The combined abortSignal and timeoutMs handling in poe.ts
leaves the timeout timer pending when generateText completes normally. Update
the generateText flow so the timeoutId created alongside the AbortController is
always cleared after the request finishes, ideally in a finally block around the
generateText(generateOptions) call, while keeping the existing abort listener
behavior intact.
---
Outside diff comments:
In `@src/api/providers/vscode-lm.ts`:
- Around line 585-610: The error handling in completePrompt is detached from the
try/finally block, so the wrapping logic never runs and the extra catch()
becomes a separate class method. Move the catch handler back onto the
completePrompt flow so it is attached to the try before finally, and keep the
Error wrapping that prefixes "VSCode LM completion error:" for failures from
client.sendRequest and stream consumption.
---
Nitpick comments:
In `@src/api/providers/__tests__/openai-codex-native-tool-calls.spec.ts`:
- Around line 568-590: There is a duplicate test title in the `completePrompt`
specs, which makes the test output ambiguous because two different cases share
the same name. Rename the test in the `completePrompt` block around
`openai-codex-native-tool-calls.spec.ts` to a distinct, descriptive title that
reflects the behavior it checks, and keep the other `completePrompt` test name
aligned with its specific assertion so the suite output clearly identifies each
case.
In `@src/api/providers/__tests__/openai-native.spec.ts`:
- Around line 268-280: The test in openai-native.spec.ts is duplicated with the
same title and assertions as another case in the same suite, so remove one copy
or change the second block to cover a different scenario. Update the duplicate
test around handler.completePrompt("Test prompt") and
mockResponsesCreate.mockResolvedValue so the suite keeps only one
backward-compatible coverage case and the other test validates a distinct
behavior.
In `@src/api/providers/anthropic.ts`:
- Around line 153-184: The inner switch inside the requestOptions IIFE in
anthropic.ts is redundant because it duplicates the same model IDs already
handled by the outer modelId switch, making the default branch unreachable.
Simplify the requestOptions logic by removing the nested switch in the
requestOptions block and building the options inline from the already-matched
model case, while preserving the prompt-caching beta header and optional
abortSignal handling.
In `@src/api/providers/config-builder/README.md`:
- Around line 200-230: The documented mergeAbortSignals example is out of order
compared with the actual implementation in request-config-builder.ts, even
though the behavior is the same. Update the README snippet for mergeAbortSignals
so its branch ordering matches the real static method exactly, including the
primarySignal and secondarySignal aborted checks and the both-aborted case, to
keep the docs aligned with the code.
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 141-148: The manual merge logic in request-config-builder’s signal
combining path should be replaced with AbortSignal.any([primarySignal,
secondarySignal]) instead of wiring abort listeners on a new AbortController.
Update the merge branch in the signal-handling function to return the combined
signal from AbortSignal.any, and adjust any tests that currently expect the
returned value to be the same object as primaryController.signal since any()
produces a new signal instance.
In `@src/api/providers/fake-ai.ts`:
- Around line 78-79: The `FakeAI.completePrompt` implementation no longer needs
the `as any` cast because `this.ai` is already typed by the `FakeAI` interface.
Remove the cast in `completePrompt` and call the method directly for type
safety, and if you touch the signature, import and reuse `CompletePromptOptions`
from `../index` so the interface and implementation stay aligned.
In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts`:
- Around line 474-485: The current `writeModels` test only checks that
`writeModels` resolves, which is too weak because `safeWriteJson` is mocked.
Update the `writeModels` spec in `modelCache.spec.ts` to assert the persistence
contract by verifying `safeWriteJson` is called with the expected
`<router>_models.json` path under the cache directory and the same `mockModels`
object. Use the existing `writeModels` and `safeWriteJson` symbols to locate the
behavior being tested.
In `@src/api/providers/lite-llm.ts`:
- Line 350: The `createOptions || undefined` fallback in `LiteLLMProvider` is
unnecessary because `createOptions` is always an object, so the call always
passes an empty options object even when no `signal` or `timeout` is set. Update
the `chat.completions.create` call in `lite-llm.ts` to match the pattern used in
`xai.ts` by only passing `createOptions` when it has keys, otherwise pass
`undefined`, keeping the behavior explicit and consistent.
In `@src/api/providers/lm-studio.ts`:
- Around line 206-216: Remove the unnecessary `|| undefined` fallback in
`LmStudioProvider`’s chat completion call: `createOptions` in
`this.client.chat.completions.create(...)` is always an object, so pass it
directly. Keep the existing `createOptions` construction in this method and
update the call site to use the object as-is so it matches the current tests and
behavior.
In `@src/api/providers/unbound.ts`:
- Around line 219-221: The `createOptions || undefined` fallback in
`UnboundProvider`’s chat completion call is misleading because `createOptions`
is always an object, so an empty object is still passed. Update the
`this.client.chat.completions.create(...)` call to match the guarded pattern
used in `vercel-ai-gateway.ts` by only passing `createOptions` when it has keys,
and adjust the existing test around the mocked `create` call in this provider to
expect `undefined` instead of `{}` when no options are set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a422b91-add0-4756-b3bf-6d04315d0915
📒 Files selected for processing (72)
src/api/index.tssrc/api/providers/__tests__/anthropic-vertex.spec.tssrc/api/providers/__tests__/anthropic.spec.tssrc/api/providers/__tests__/base-openai-compatible-provider-timeout.spec.tssrc/api/providers/__tests__/base-openai-compatible-provider.spec.tssrc/api/providers/__tests__/bedrock.spec.tssrc/api/providers/__tests__/complete-prompt-options.spec.tssrc/api/providers/__tests__/deepseek.spec.tssrc/api/providers/__tests__/fireworks.spec.tssrc/api/providers/__tests__/gemini-handler.spec.tssrc/api/providers/__tests__/gemini.spec.tssrc/api/providers/__tests__/lite-llm.spec.tssrc/api/providers/__tests__/lmstudio-native-tools.spec.tssrc/api/providers/__tests__/lmstudio.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/minimax.spec.tssrc/api/providers/__tests__/mistral.spec.tssrc/api/providers/__tests__/moonshot.spec.tssrc/api/providers/__tests__/native-ollama.spec.tssrc/api/providers/__tests__/openai-codex-native-tool-calls.spec.tssrc/api/providers/__tests__/openai-native.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/__tests__/opencode-go.spec.tssrc/api/providers/__tests__/openrouter.spec.tssrc/api/providers/__tests__/poe.spec.tssrc/api/providers/__tests__/qwen-code-native-tools.spec.tssrc/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/__tests__/requesty.spec.tssrc/api/providers/__tests__/sambanova.spec.tssrc/api/providers/__tests__/unbound.spec.tssrc/api/providers/__tests__/vercel-ai-gateway.spec.tssrc/api/providers/__tests__/vertex.spec.tssrc/api/providers/__tests__/vscode-lm.spec.tssrc/api/providers/__tests__/xai.spec.tssrc/api/providers/__tests__/zai.spec.tssrc/api/providers/__tests__/zoo-gateway.spec.tssrc/api/providers/anthropic-vertex.tssrc/api/providers/anthropic.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/bedrock.tssrc/api/providers/config-builder/README.mdsrc/api/providers/config-builder/request-config-builder.tssrc/api/providers/deepseek.tssrc/api/providers/fake-ai.tssrc/api/providers/fetchers/__tests__/modelCache.spec.tssrc/api/providers/fetchers/modelCache.tssrc/api/providers/gemini.tssrc/api/providers/index.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/minimax.tssrc/api/providers/mistral.tssrc/api/providers/native-ollama.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai-native.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/poe.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/vscode-lm.tssrc/api/providers/xai.tssrc/api/providers/zai.tssrc/api/providers/zoo-gateway.tssrc/core/task/__tests__/Task.spec.tssrc/utils/__tests__/enhance-prompt.spec.tssrc/utils/single-completion-handler.ts
Comment on lines
+335
to
+343
| const requestOptions: OpenAI.RequestOptions | undefined = | ||
| modelId.startsWith("anthropic/") || metadata?.abortSignal | ||
| ? { | ||
| headers: { "x-anthropic-beta": "fine-grained-tool-streaming-2025-05-14" }, | ||
| ...(metadata?.abortSignal && { signal: metadata.abortSignal }), | ||
| } | ||
| : metadata?.abortSignal | ||
| ? { signal: metadata.abortSignal } | ||
| : undefined |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Anthropic beta header leaks onto non-Anthropic models when abortSignal is set.
The ternary's first branch fires when either modelId.startsWith("anthropic/") or metadata?.abortSignal is truthy. So for a non-Anthropic model with an abort signal, the request gets x-anthropic-beta: fine-grained-tool-streaming-... even though it isn't an Anthropic model. The intended signal-only branch (metadata?.abortSignal ? { signal }) is unreachable in that case. Note completePrompt (Line 605-612) already gates the header on anthropic/ only, so the two paths now disagree.
🐛 Proposed fix to gate the header on Anthropic models only
- const requestOptions: OpenAI.RequestOptions | undefined =
- modelId.startsWith("anthropic/") || metadata?.abortSignal
- ? {
- headers: { "x-anthropic-beta": "fine-grained-tool-streaming-2025-05-14" },
- ...(metadata?.abortSignal && { signal: metadata.abortSignal }),
- }
- : metadata?.abortSignal
- ? { signal: metadata.abortSignal }
- : undefined
+ const requestOptions: OpenAI.RequestOptions | undefined = modelId.startsWith("anthropic/")
+ ? {
+ headers: { "x-anthropic-beta": "fine-grained-tool-streaming-2025-05-14" },
+ ...(metadata?.abortSignal && { signal: metadata.abortSignal }),
+ }
+ : metadata?.abortSignal
+ ? { signal: metadata.abortSignal }
+ : undefined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const requestOptions: OpenAI.RequestOptions | undefined = | |
| modelId.startsWith("anthropic/") || metadata?.abortSignal | |
| ? { | |
| headers: { "x-anthropic-beta": "fine-grained-tool-streaming-2025-05-14" }, | |
| ...(metadata?.abortSignal && { signal: metadata.abortSignal }), | |
| } | |
| : metadata?.abortSignal | |
| ? { signal: metadata.abortSignal } | |
| : undefined | |
| const requestOptions: OpenAI.RequestOptions | undefined = modelId.startsWith("anthropic/") | |
| ? { | |
| headers: { "x-anthropic-beta": "fine-grained-tool-streaming-2025-05-14" }, | |
| ...(metadata?.abortSignal && { signal: metadata.abortSignal }), | |
| } | |
| : metadata?.abortSignal | |
| ? { signal: metadata.abortSignal } | |
| : undefined |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/openrouter.ts` around lines 335 - 343, The requestOptions
logic in openrouter’s provider path is applying the Anthropic beta header
whenever abortSignal is present, which causes the header to leak onto
non-Anthropic models. Update the conditional that builds requestOptions so the
x-anthropic-beta header is added only when modelId starts with "anthropic/", and
keep the abortSignal-only case separate so non-Anthropic requests receive only
the signal. Make the gating consistent with completePrompt so both paths use the
same Anthropic-only header rule.
551036e to
f728333
Compare
easonLiangWorldedtech
changed the title
easonLiangWorldedtech
force-pushed
the
origin/feat/abort-signal-core/pass-through-providers
branch
from
f728333 to
75adc30
Compare
… signal propagation
easonLiangWorldedtech
force-pushed
the
origin/feat/abort-signal-core/pass-through-providers
branch
from
75adc30 to
4c0dd5e
Compare
easonLiangWorldedtech
force-pushed
the
origin/feat/abort-signal-core/pass-through-providers
branch
from
4c0dd5e to
5055fe7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Passes
metadata?.abortSignalthrough to API calls in 19 provider implementations, so clicking Stop actually cancels the underlying HTTP request.Closes #616 (part of #404, depends on #615)
What Changed
Provider signal pass-through
Each provider now forwards
metadata?.abortSignalin bothcreateMessage(streaming) andcompletePrompt(non-streaming) paths:{ signal: metadata?.abortSignal }(OpenAI SDK) or{ abortSignal: metadata?.abortSignal }(AI SDK)Supporting changes
completePromptto usemetadata.abortSignalconsistently across all providersmergeAbortSignals()without unnecessary controller allocationisAuthScopedProviderandwriteModelsfrom modelCache for test accessStats
Closes #616