feat(api): abort signal bridging for openai-native, openai-codex, bedrock, native-ollama#731
Draft
easonLiangWorldedtech wants to merge 15 commits into
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:
📝 WalkthroughWalkthroughThe PR introduces ChangesCompletion Options Plumbing
Model Cache Export
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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! |
![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: 10
🧹 Nitpick comments (10)
src/api/providers/fetchers/__tests__/modelCache.spec.ts (1)
474-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the
writeModelsassertion.Since
safeWriteJsonis fully mocked, this test only confirms the promise resolves. Consider asserting thatsafeWriteJsonwas called with the expected filename (openrouter_models.json) andmockModelsto verify the construction logic itself.🤖 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 - 486, The writeModels test currently only checks that the promise resolves, so it does not verify the filename or payload being passed through. Update the writeModels spec in modelCache.spec.ts to assert that safeWriteJson is called with the expected generated filename openrouter_models.json and the mockModels object, using the writeModels function as the entry point so the cache-file construction logic is validated.src/api/providers/base-openai-compatible-provider.ts (1)
229-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor cleanup: misleading comment and redundant
|| undefined.Line 229 references
RequestConfigBuilder, but the options are built inline here. Also,requestOptionsis always a non-null object literal, sorequestOptions || undefinedon Line 238 never evaluates toundefined.♻️ Suggested cleanup
- // Build request options with abortSignal and/or timeout using RequestConfigBuilder + // Build request options with abortSignal and/or timeout const requestOptions: OpenAI.RequestOptions = {} if (options?.abortSignal) { requestOptions.signal = options.abortSignal } if (options?.timeoutMs !== undefined) { requestOptions.timeout = options.timeoutMs } - const response = await this.client.chat.completions.create(params, requestOptions || undefined) + const response = await this.client.chat.completions.create(params, requestOptions)🤖 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/base-openai-compatible-provider.ts` around lines 229 - 238, The request-options block in base-openai-compatible-provider is doing the work inline, so remove the misleading Reference to RequestConfigBuilder in the nearby comment and update it to describe the actual inline construction. Also simplify the chat.completions.create call in the same method by dropping the redundant requestOptions || undefined since requestOptions is always an object; keep the logic in the provider method that builds OpenAI.RequestOptions from options.abortSignal and options.timeoutMs.src/api/providers/unbound.ts (1)
207-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
|| undefined
createOptionsis always an object here, socreateOptions || undefinednever changes the call. PasscreateOptionsdirectly.🤖 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 207 - 218, The chat completion call in unbound provider uses a redundant fallback when passing request options, because createOptions is always initialized as an object. Update the request in the chat.completions.create path to pass createOptions directly from the unbound provider logic, keeping the existing abortSignal and timeoutMs handling intact.src/api/providers/openai-native.ts (1)
1514-1528: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant dummy controller allocation.
When
this.abortControlleris undefined,this.abortController?.signal ?? new AbortController().signalallocates a throwaway controller whose signal can never abort, somergeAbortSignalseffectively just tracksoptions?.abortSignal. The merge against the freshly-created (then immediately overwritten) controller adds no behavior. This is functionally correct but obscures intent; consider merging directly againstoptions?.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/openai-native.ts` around lines 1514 - 1528, The merged abort-signal setup in completePrompt is creating a throwaway AbortController when this.abortController is undefined, which adds no behavior and obscures the intent. Simplify the logic in completePrompt by removing the dummy controller allocation and merging directly with options?.abortSignal (while preserving the existing abortController reset and abort listener behavior) so the cancellation flow remains clear and equivalent.src/api/providers/__tests__/openai-codex-native-tool-calls.spec.ts (1)
632-638: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBridge test doesn't confirm propagation to the internal signal.
createCallArgs.signalis captured, but aftercontroller.abort()the test only assertscontroller.signal.aborted(always true). It never re-checks that the bridged internal signal aborts. As the comment notes, by this point the stream has finished, so nothing here proves bridging works. Either assert oncreateCallArgs.signalwhile it's still live, or attach anabortlistener to it (as done in the fetch test at lines 556-565).🤖 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 632 - 638, The bridge test is only asserting the external controller signal after abort, so it never verifies that the captured internal signal from createCallArgs.signal is actually aborted. Update the test around the openai codex native tool call flow to assert on createCallArgs.signal while it is still valid, or add an abort listener to that internal signal before calling controller.abort(), similar to the fetch test pattern. Use the existing createCallArgs capture and the abort-controller bridge logic in the test to confirm propagation.src/api/providers/__tests__/openai-native.spec.ts (2)
345-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test.
This
"should work without options (backward compatible)"block is identical to the one at lines 312-324 (same name and body). Likewise the"should pass signal through to client via createOptions"test (326-343) duplicates"should merge incoming signal with existing controller"(293-310). Remove the redundant cases.🤖 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 345 - 357, There are duplicate test cases in openai-native.spec.ts: the “should work without options (backward compatible)” block duplicates an earlier test with the same behavior, and “should pass signal through to client via createOptions” duplicates the signal-merging test. Remove the redundant `it(...)` cases and keep a single representative test for each behavior, using the existing test names and `handler.completePrompt`/`mockResponsesCreate` coverage as the reference points.
216-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test does not actually verify abort propagation into the fallback fetch.
The only abort-related assertions are
expect(controller.signal.aborted)before and aftercontroller.abort(), which are tautologically true and tell you nothing about whether the internal/fetchsignalwas bridged. The captured fetch signal is never inspected. Per the PR's acceptance criteria, tests should verify actual cancellation propagation (as the sibling codex fetch test at lines 531-566 does by attaching anabortlistener tofetchCallArgs[1].signal).Consider capturing the
signalpassed tomockFetchand asserting it aborts whencontroller.abort()fires.🤖 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 216 - 226, The openai-native abort test is only checking the external controller’s own state, so it never proves the fallback fetch received the bridged signal. Update the test in openai-native.spec.ts around the stream/abort assertions to capture the signal passed into mockFetch and verify that it aborts when controller.abort() is called, mirroring the existing fetch-signal pattern used in the sibling codex fetch test. Use the relevant test case and mockFetch/fetch call args to assert actual cancellation propagation rather than tautological controller.signal checks.src/api/providers/zoo-gateway.ts (1)
298-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
|| undefinedbranch.
createOptionsis initialized to{}and is always a truthy object, socreateOptions || undefinedalways resolves tocreateOptions. The fallback never fires. This is harmless (passing an empty{}to the OpenAI SDK is fine, and the spec at Line 448 expects{}), but the expression is misleading. Either drop the|| undefinedor mirror the conditional pattern used elsewhere (e.g. Anthropic'sObject.keys(requestOptions).length > 0 ? requestOptions : undefined).♻️ Suggested simplification
- const response = await this.client.chat.completions.create(requestOptions, createOptions || undefined) + const response = await this.client.chat.completions.create(requestOptions, createOptions)🤖 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/zoo-gateway.ts` around lines 298 - 306, The `createOptions || undefined` fallback in `zoo-gateway.ts` is dead code because `createOptions` is always an object, so the expression is misleading. Update the `this.client.chat.completions.create(...)` call to pass `createOptions` directly or use the same conditional-empty-object pattern used in the Anthropic path, and keep the behavior aligned with the existing `OpenAI.RequestOptions` handling in this method.src/api/providers/openai-codex.ts (1)
1172-1182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
options.timeoutMsis silently ignored incompletePrompt.
completePromptacceptsCompletePromptOptions(which includestimeoutMs) but only wiresabortSignal; thefetchat Line 1239 has no timeout. Every other handler in this cohort (qwen-code, opencode-go, openai-compatible, minimax) honorstimeoutMs, so a caller passing it here gets no cancellation-on-timeout. Consider merging a timeout intomergedSignal(e.g. viaAbortSignal.timeout/ the same merge used elsewhere) for parity.🤖 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/openai-codex.ts` around lines 1172 - 1182, The completePrompt flow in openai-codex is ignoring CompletePromptOptions.timeoutMs because it only merges abortSignal and never applies a timeout to the request. Update completePrompt to combine timeoutMs into the request cancellation path, preferably by extending the mergedSignal logic used with RequestConfigBuilder.mergeAbortSignals or AbortSignal.timeout so the fetch is aborted on timeout as in the other providers. Keep the fix localized around completePrompt and the request setup that feeds the fetch call.src/api/providers/anthropic-vertex.ts (1)
299-302: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
options.timeoutMsis silently dropped here.
CompletePromptOptionscarriestimeoutMs, and the sibling handlers in this PR (e.g.minimax.ts,opencode-go.ts) forward it to the Anthropic SDK via the request-optionstimeoutfield. This path only wiressignal, so a per-request timeout override is ignored.♻️ Suggested change to also forward the timeout
- const response = await this.client.messages.create( - params, - options?.abortSignal ? { signal: options.abortSignal } : undefined, - ) + const requestOptions: Anthropic.RequestOptions = {} + if (options?.abortSignal) { + requestOptions.signal = options.abortSignal + } + if (options?.timeoutMs !== undefined) { + requestOptions.timeout = options.timeoutMs + } + const response = await this.client.messages.create( + params, + Object.keys(requestOptions).length > 0 ? requestOptions : 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/anthropic-vertex.ts` around lines 299 - 302, The Anthropic Vertex request in messages.create is dropping CompletePromptOptions.timeoutMs, so per-request timeouts are ignored. Update the request-options object in this path to forward options.timeoutMs through the Anthropic SDK timeout field, alongside the existing abort signal handling, and keep the behavior consistent with the sibling provider implementations like minimax and opencode-go.
🤖 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__/bedrock.spec.ts`:
- Around line 1629-1687: The two tests in AwsBedrockHandler are misleading
because they assert behavior that completePrompt does not implement. Update the
timeoutMs test to match the actual pass-through behavior in completePrompt and
remove the claim that it creates AbortSignal.timeout unless you retarget it to
the method that באמת handles timeout logic. For the abort-signal test,
completePrompt forwards the provided abortSignal directly, so either move the
test to createMessage where the internal controller bridge exists or change the
assertions to verify direct pass-through from the caller’s
AbortController.signal instead of checking for a separate internal signal.
In `@src/api/providers/__tests__/request-config-builder.spec.ts`:
- Around line 300-423: The current `RequestConfigBuilder.mergeAbortSignals`
tests cover abort propagation but miss the stale-listener cleanup case that can
leak when one source signal outlives the request. Add a package-local unit test
in `request-config-builder.spec.ts` that exercises `mergeAbortSignals` with two
active signals, aborts one of them, and then verifies the sibling signal’s abort
listener has been removed or no longer fires after the merged signal aborts. Use
the existing `RequestConfigBuilder.mergeAbortSignals` helper and its abort-event
setup to locate the behavior and assert listener cleanup directly.
In `@src/api/providers/bedrock.ts`:
- Around line 853-856: The Bedrock request path in completePrompt is ignoring
options.timeoutMs, so update the this.client.send call path to honor caller
timeouts the same way other providers do. In the Bedrock provider’s
completePrompt logic, either convert timeoutMs into an AbortSignal using
AbortSignal.timeout(...) and pass it alongside any existing abortSignal, or
explicitly remove/document timeoutMs support if it should not be used here. Make
sure the change is localized around the existing options.abortSignal handling so
both timeout and manual abort behavior remain consistent.
In `@src/api/providers/config-builder/README.md`:
- Around line 45-47: The Bedrock examples in the config-builder README are using
the wrong cancellation property, which can mislead users of
BedrockRequestConfigBuilder. Update the Bedrock-specific snippets and any
related examples in the README to use the same option name that
BedrockRequestConfigBuilder forwards to AWS SDK v3 send(), and keep the
Anthropic/Vertex AI examples unchanged so the provider-specific API is clear.
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 141-145: Detach the abort listeners added in
requestConfigBuilder’s merge logic so they do not մն to the caller-owned signal
after the merged AbortController settles. In the branch that creates the
controller and wires primarySignal and secondarySignal, add cleanup tied to the
merged controller’s abort path or remove the sibling listener inside each abort
handler. Keep the fix localized to the signal-merging logic in
request-config-builder.ts.
- Around line 13-18: `RequestConfigBuilder` is treating partially assembled
options as if they were a complete `TOptions`, so tighten the contract by
changing the internal `options` state and the `build()`/`fromMetadata()` flow in
`RequestConfigBuilder` to either keep `Partial<TOptions>` until the end or
validate required fields before returning `TOptions`. Also update
`mergeAbortSignals()` so both abort listeners are always cleaned up: remove the
sibling listener when either signal aborts and on normal completion, not just
the one that fired, to avoid leaving per-request listeners attached to
long-lived signals.
In `@src/api/providers/minimax.ts`:
- Around line 300-302: The timeout handling in minimax.ts uses a truthy guard,
which skips forwarding timeoutMs when it is 0. Update the check in the Minimax
request options path to match the other handlers (for example, the same
undefined check used in gemini.ts, mistral.ts, and opencode-go.ts) so
options.timeoutMs is applied even when it is 0.
In `@src/api/providers/opencode-go.ts`:
- Line 558: The `createOptions || undefined` guard in `chat.completions.create`
is a no-op because `createOptions` is always an object, so empty options still
get passed. Update the `opencode-go.ts` request flow to mirror the Anthropic
path by only passing `createOptions` when it contains at least one key,
otherwise pass `undefined`; use the `response` call site and the `createOptions`
construction nearby to locate the change.
In `@src/api/providers/requesty.ts`:
- Around line 223-229: The timeout handling in requesty’s chat completion path
is inconsistent with the rest of the providers because the `timeoutMs` check in
`RequestyProvider` drops a valid `0` value. Update the guard in the
`createOptions` setup inside the `chat.completions.create` flow to match the
base provider and `openai.ts` behavior by checking for `undefined` instead of
truthiness, so `timeoutMs: 0` is forwarded. Also simplify the `createOptions ||
undefined` usage in the same call, since `createOptions` is already a concrete
object.
In `@src/api/providers/xai.ts`:
- Around line 154-155: Update the timeout handling in xai.ts so a
caller-provided timeoutMs of 0 is preserved instead of being skipped by the
truthy check. In the request-building logic that sets requestOptions.timeout,
change the condition to distinguish undefined from an explicit zero (or validate
and reject zero consistently) so the shared options contract is honored when
options.timeoutMs is passed into the provider.
---
Nitpick comments:
In `@src/api/providers/__tests__/openai-codex-native-tool-calls.spec.ts`:
- Around line 632-638: The bridge test is only asserting the external controller
signal after abort, so it never verifies that the captured internal signal from
createCallArgs.signal is actually aborted. Update the test around the openai
codex native tool call flow to assert on createCallArgs.signal while it is still
valid, or add an abort listener to that internal signal before calling
controller.abort(), similar to the fetch test pattern. Use the existing
createCallArgs capture and the abort-controller bridge logic in the test to
confirm propagation.
In `@src/api/providers/__tests__/openai-native.spec.ts`:
- Around line 345-357: There are duplicate test cases in openai-native.spec.ts:
the “should work without options (backward compatible)” block duplicates an
earlier test with the same behavior, and “should pass signal through to client
via createOptions” duplicates the signal-merging test. Remove the redundant
`it(...)` cases and keep a single representative test for each behavior, using
the existing test names and `handler.completePrompt`/`mockResponsesCreate`
coverage as the reference points.
- Around line 216-226: The openai-native abort test is only checking the
external controller’s own state, so it never proves the fallback fetch received
the bridged signal. Update the test in openai-native.spec.ts around the
stream/abort assertions to capture the signal passed into mockFetch and verify
that it aborts when controller.abort() is called, mirroring the existing
fetch-signal pattern used in the sibling codex fetch test. Use the relevant test
case and mockFetch/fetch call args to assert actual cancellation propagation
rather than tautological controller.signal checks.
In `@src/api/providers/anthropic-vertex.ts`:
- Around line 299-302: The Anthropic Vertex request in messages.create is
dropping CompletePromptOptions.timeoutMs, so per-request timeouts are ignored.
Update the request-options object in this path to forward options.timeoutMs
through the Anthropic SDK timeout field, alongside the existing abort signal
handling, and keep the behavior consistent with the sibling provider
implementations like minimax and opencode-go.
In `@src/api/providers/base-openai-compatible-provider.ts`:
- Around line 229-238: The request-options block in
base-openai-compatible-provider is doing the work inline, so remove the
misleading Reference to RequestConfigBuilder in the nearby comment and update it
to describe the actual inline construction. Also simplify the
chat.completions.create call in the same method by dropping the redundant
requestOptions || undefined since requestOptions is always an object; keep the
logic in the provider method that builds OpenAI.RequestOptions from
options.abortSignal and options.timeoutMs.
In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts`:
- Around line 474-486: The writeModels test currently only checks that the
promise resolves, so it does not verify the filename or payload being passed
through. Update the writeModels spec in modelCache.spec.ts to assert that
safeWriteJson is called with the expected generated filename
openrouter_models.json and the mockModels object, using the writeModels function
as the entry point so the cache-file construction logic is validated.
In `@src/api/providers/openai-codex.ts`:
- Around line 1172-1182: The completePrompt flow in openai-codex is ignoring
CompletePromptOptions.timeoutMs because it only merges abortSignal and never
applies a timeout to the request. Update completePrompt to combine timeoutMs
into the request cancellation path, preferably by extending the mergedSignal
logic used with RequestConfigBuilder.mergeAbortSignals or AbortSignal.timeout so
the fetch is aborted on timeout as in the other providers. Keep the fix
localized around completePrompt and the request setup that feeds the fetch call.
In `@src/api/providers/openai-native.ts`:
- Around line 1514-1528: The merged abort-signal setup in completePrompt is
creating a throwaway AbortController when this.abortController is undefined,
which adds no behavior and obscures the intent. Simplify the logic in
completePrompt by removing the dummy controller allocation and merging directly
with options?.abortSignal (while preserving the existing abortController reset
and abort listener behavior) so the cancellation flow remains clear and
equivalent.
In `@src/api/providers/unbound.ts`:
- Around line 207-218: The chat completion call in unbound provider uses a
redundant fallback when passing request options, because createOptions is always
initialized as an object. Update the request in the chat.completions.create path
to pass createOptions directly from the unbound provider logic, keeping the
existing abortSignal and timeoutMs handling intact.
In `@src/api/providers/zoo-gateway.ts`:
- Around line 298-306: The `createOptions || undefined` fallback in
`zoo-gateway.ts` is dead code because `createOptions` is always an object, so
the expression is misleading. Update the
`this.client.chat.completions.create(...)` call to pass `createOptions` directly
or use the same conditional-empty-object pattern used in the Anthropic path, and
keep the behavior aligned with the existing `OpenAI.RequestOptions` handling in
this method.
🪄 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: b2ae313b-0e5d-4150-a938-73fa39c7aa65
📒 Files selected for processing (68)
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__/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/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/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/zoo-gateway.tssrc/core/task/__tests__/Task.spec.tssrc/utils/__tests__/enhance-prompt.spec.tssrc/utils/single-completion-handler.ts
Comment on lines
+1629
to
+1687
| it("completePrompt should pass timeoutMs through to client", async () => { | ||
| const mockSend = vi.fn() | ||
|
|
||
| const handler = new AwsBedrockHandler({ | ||
| apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", | ||
| awsAccessKey: "test-access-key", | ||
| awsSecretKey: "test-secret-key", | ||
| awsRegion: "us-east-1", | ||
| }) | ||
|
|
||
| const clientInstance = (handler as any).client | ||
| expect(clientInstance).toBeDefined() | ||
| clientInstance.send = mockSend | ||
|
|
||
| mockSend.mockResolvedValueOnce({ | ||
| output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, | ||
| }) | ||
|
|
||
| await handler.completePrompt("test prompt", { timeoutMs: 5000 }) | ||
|
|
||
| // bedrock.ts uses truthy check for timeoutMs, so it creates AbortSignal.timeout | ||
| expect(mockSend).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should abort internal controller when external abortSignal is triggered", async () => { | ||
| const mockResult = { | ||
| output: { message: { content: [{ type: "text", text: "response" }] }, stopReason: null }, | ||
| } | ||
| const mockSend = vi.fn().mockResolvedValue(mockResult) | ||
|
|
||
| const handler = new AwsBedrockHandler({ | ||
| apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", | ||
| awsAccessKey: "test-access-key", | ||
| awsSecretKey: "test-secret-key", | ||
| awsRegion: "us-east-1", | ||
| }) | ||
|
|
||
| const clientInstance = (handler as any).client | ||
| clientInstance.send = mockSend | ||
|
|
||
| const controller = new AbortController() | ||
| let internalSignalCaptured: AbortSignal | undefined | ||
|
|
||
| // Spy on the send call to capture the abortSignal | ||
| mockSend.mockImplementation(async (command, options) => { | ||
| internalSignalCaptured = options?.abortSignal | ||
| return mockResult | ||
| }) | ||
|
|
||
| await handler.completePrompt("test prompt", { abortSignal: controller.signal }) | ||
|
|
||
| expect(internalSignalCaptured).toBeDefined() | ||
| expect(internalSignalCaptured).toBeInstanceOf(AbortSignal) | ||
|
|
||
| // Abort the external signal and verify it propagates to the internal signal | ||
| controller.abort() | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| expect(internalSignalCaptured!.aborted).toBe(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two misleading tests give false confidence.
- Line 1649-1650: the comment states bedrock "creates
AbortSignal.timeout" fortimeoutMs, butcompletePromptdoes not handletimeoutMsat all (see bedrock.ts Line 853-856). The assertion only checksmockSendwas called, so it doesn't actually exercise timeout behavior. Fix the comment (and ideally assert thattimeoutMshas no effect, matching current behavior). - Line 1653-1687: this test is named "abort internal controller when external abortSignal is triggered," but
completePromptpassesoptions.abortSignalstraight through with no bridging controller.internalSignalCapturedis the same instance ascontroller.signal, soexpect(internalSignalCaptured!.aborted).toBe(true)is tautological. The internal-controller bridge only exists increateMessage(Line 537-548). Either retarget this test atcreateMessageor rename/assert it as a direct pass-through (expect(internalSignalCaptured).toBe(controller.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/__tests__/bedrock.spec.ts` around lines 1629 - 1687, The
two tests in AwsBedrockHandler are misleading because they assert behavior that
completePrompt does not implement. Update the timeoutMs test to match the actual
pass-through behavior in completePrompt and remove the claim that it creates
AbortSignal.timeout unless you retarget it to the method that באמת handles
timeout logic. For the abort-signal test, completePrompt forwards the provided
abortSignal directly, so either move the test to createMessage where the
internal controller bridge exists or change the assertions to verify direct
pass-through from the caller’s AbortController.signal instead of checking for a
separate internal signal.
Comment on lines
+300
to
+423
| describe("static mergeAbortSignals", () => { | ||
| test("should return primarySignal when secondarySignal is undefined", () => { | ||
| const controller = new AbortController() | ||
| const result = RequestConfigBuilder.mergeAbortSignals(controller.signal) | ||
| expect(result).toBe(controller.signal) | ||
| }) | ||
|
|
||
| test("should return an aborted signal when secondarySignal is already aborted but primary is not", () => { | ||
| const primaryController = new AbortController() | ||
| const secondaryController = new AbortController() | ||
| secondaryController.abort() | ||
|
|
||
| const result = RequestConfigBuilder.mergeAbortSignals(primaryController.signal, secondaryController.signal) | ||
|
|
||
| // Result should be aborted since secondary was already aborted | ||
| expect(result.aborted).toBe(true) | ||
| // Should NOT be the primary signal (which is not aborted) | ||
| expect(result).not.toBe(primaryController.signal) | ||
| }) | ||
|
|
||
| test("should return primarySignal when both signals are already aborted", () => { | ||
| const primaryController = new AbortController() | ||
| const secondaryController = new AbortController() | ||
| primaryController.abort() | ||
| secondaryController.abort() | ||
|
|
||
| const result = RequestConfigBuilder.mergeAbortSignals(primaryController.signal, secondaryController.signal) | ||
| expect(result).toBe(primaryController.signal) | ||
| }) | ||
|
|
||
| test("should return merged signal when both signals are active", () => { | ||
| const primaryController = new AbortController() | ||
| const secondaryController = new AbortController() | ||
|
|
||
| const result = RequestConfigBuilder.mergeAbortSignals(primaryController.signal, secondaryController.signal) | ||
| expect(result).not.toBe(primaryController.signal) | ||
| expect(result).not.toBe(secondaryController.signal) | ||
| }) | ||
|
|
||
| test("should abort merged signal when primarySignal is aborted", async () => { | ||
| const primaryController = new AbortController() | ||
| const secondaryController = new AbortController() | ||
|
|
||
| const mergedSignal = RequestConfigBuilder.mergeAbortSignals( | ||
| primaryController.signal, | ||
| secondaryController.signal, | ||
| ) | ||
|
|
||
| let aborted = false | ||
| mergedSignal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| aborted = true | ||
| }, | ||
| { once: true }, | ||
| ) | ||
|
|
||
| primaryController.abort() | ||
|
|
||
| // Wait for event to propagate | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| expect(aborted).toBe(true) | ||
| }) | ||
|
|
||
| test("should abort merged signal when secondarySignal is aborted", async () => { | ||
| const primaryController = new AbortController() | ||
| const secondaryController = new AbortController() | ||
|
|
||
| const mergedSignal = RequestConfigBuilder.mergeAbortSignals( | ||
| primaryController.signal, | ||
| secondaryController.signal, | ||
| ) | ||
|
|
||
| let aborted = false | ||
| mergedSignal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| aborted = true | ||
| }, | ||
| { once: true }, | ||
| ) | ||
|
|
||
| secondaryController.abort() | ||
|
|
||
| // Wait for event to propagate | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| expect(aborted).toBe(true) | ||
| }) | ||
|
|
||
| test("should not abort merged signal when neither signal is aborted", async () => { | ||
| const primaryController = new AbortController() | ||
| const secondaryController = new AbortController() | ||
|
|
||
| const mergedSignal = RequestConfigBuilder.mergeAbortSignals( | ||
| primaryController.signal, | ||
| secondaryController.signal, | ||
| ) | ||
|
|
||
| let aborted = false | ||
| mergedSignal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| aborted = true | ||
| }, | ||
| { once: true }, | ||
| ) | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| expect(aborted).toBe(false) | ||
| }) | ||
|
|
||
| test("should handle primary already aborted before merge", () => { | ||
| const primaryController = new AbortController() | ||
| const secondaryController = new AbortController() | ||
|
|
||
| primaryController.abort() | ||
|
|
||
| const mergedSignal = RequestConfigBuilder.mergeAbortSignals( | ||
| primaryController.signal, | ||
| secondaryController.signal, | ||
| ) | ||
| expect(mergedSignal.aborted).toBe(true) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a regression for merged-listener cleanup.
These cases prove abort propagation, but they do not catch the stale-listener leak from mergeAbortSignals when one source signal outlives the request. Please add a package-local unit test that asserts the helper removes the sibling listener once the merged signal aborts. As per coding guidelines, **/*.{test,spec}.{ts,tsx,js}: Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling.
🤖 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__/request-config-builder.spec.ts` around lines 300
- 423, The current `RequestConfigBuilder.mergeAbortSignals` tests cover abort
propagation but miss the stale-listener cleanup case that can leak when one
source signal outlives the request. Add a package-local unit test in
`request-config-builder.spec.ts` that exercises `mergeAbortSignals` with two
active signals, aborts one of them, and then verifies the sibling signal’s abort
listener has been removed or no longer fires after the merged signal aborts. Use
the existing `RequestConfigBuilder.mergeAbortSignals` helper and its abort-event
setup to locate the behavior and assert listener cleanup directly.
Source: Coding guidelines
Comment on lines
+853
to
+856
| const response = await this.client.send( | ||
| command, | ||
| options?.abortSignal ? { abortSignal: options.abortSignal } : undefined, | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the AWS SDK Bedrock client request/command send path accepts a timeout option directly,
# using installed type definitions if present in the repo environment.
node - <<'JS'
const fs = require('fs');
const path = require('path');
function findFile(start, target) {
const stack = [start];
while (stack.length) {
const dir = stack.pop();
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
const p = path.join(dir, entry.name);
if (entry.isDirectory()) stack.push(p);
else if (entry.isFile() && entry.name === target) return p;
}
}
return null;
}
const root = process.cwd();
const pkg = findFile(root, 'package.json');
console.log('package.json:', pkg || 'not found');
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '830,870p' src/api/providers/bedrock.ts
printf '\n---\n'
sed -n '390,430p' src/api/providers/anthropic.ts
printf '\n---\n'
sed -n '285,315p' src/api/providers/zoo-gateway.ts
printf '\n---\n'
sed -n '1,120p' src/api/providers/bedrock.ts | rg -n "CompletePromptOptions|timeoutMs|abortSignal" -n
printf '\n---\n'
rg -n "interface CompletePromptOptions|type CompletePromptOptions|timeoutMs|abortSignal" src/api -g '!**/*.map'
printf '\n---\n'
sed -n '1635,1660p' src/api/providers/bedrock.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the shared CompletePromptOptions type and any Bedrock-specific timeout comments
rg -n "CompletePromptOptions|timeoutMs|AbortSignal\.timeout|10-minute|creates an AbortSignal\.timeout" src
# Show the definition around CompletePromptOptions
sed -n '1,220p' src/api/index.ts
# Show the Bedrock comment around the referenced line
sed -n '1638,1655p' src/api/providers/bedrock.ts
# Look for any other Bedrock send calls to compare request options usage
rg -n "client\.send\(" src/api/providers/bedrock.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 32765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1618,1660p' src/api/providers/__tests__/bedrock.spec.ts
printf '\n---\n'
sed -n '800,860p' src/api/providers/bedrock.ts
printf '\n---\n'
sed -n '1,90p' src/api/providers/__tests__/complete-prompt-options.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4635
Wire timeoutMs through Bedrock completePrompt options.timeoutMs is still ignored on this path, so Bedrock won’t honor the caller’s timeout like the other providers do. Either bridge it to an AbortSignal.timeout(...) here or remove/document timeoutMs for this provider.
🤖 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/bedrock.ts` around lines 853 - 856, The Bedrock request
path in completePrompt is ignoring options.timeoutMs, so update the
this.client.send call path to honor caller timeouts the same way other providers
do. In the Bedrock provider’s completePrompt logic, either convert timeoutMs
into an AbortSignal using AbortSignal.timeout(...) and pass it alongside any
existing abortSignal, or explicitly remove/document timeoutMs support if it
should not be used here. Make sure the change is localized around the existing
options.abortSignal handling so both timeout and manual abort behavior remain
consistent.
Comment on lines
+45
to
+47
| | AWS Bedrock | `BedrockRequestConfigBuilder extends RequestConfigBuilder<BedrockInvokeOptions>` | SDK-specific type | `BedrockRequestConfigBuilder` (future) | | ||
| | Anthropic | `AnthropicRequestConfigBuilder extends RequestConfigBuilder<Anthropic.RequestOptions>` | `Anthropic.RequestOptions` | `AnthropicRequestConfigBuilder` (future) | | ||
| | Vertex AI | `VertexAiRequestConfigBuilder extends RequestConfigBuilder<VertexAiOptions>` | Custom interface | `VertexAiRequestConfigBuilder` (future) | |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The Bedrock examples use the wrong cancellation field.
These snippets model Bedrock options with signal, but the Bedrock path in this PR forwards { abortSignal } to AWS SDK v3 send(). Copying this example would drop cancellation for that provider, so the README should use the Bedrock-specific option name here.
Also applies to: 300-318
🤖 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 45 - 47, The Bedrock
examples in the config-builder README are using the wrong cancellation property,
which can mislead users of BedrockRequestConfigBuilder. Update the
Bedrock-specific snippets and any related examples in the README to use the same
option name that BedrockRequestConfigBuilder forwards to AWS SDK v3 send(), and
keep the Anthropic/Vertex AI examples unchanged so the provider-specific API is
clear.
Comment on lines
+13
to
+18
| export class RequestConfigBuilder<TOptions extends Record<string, any> = Record<string, any>> { | ||
| protected options: TOptions | ||
|
|
||
| constructor(defaultOptions?: Partial<TOptions>) { | ||
| this.options = (defaultOptions ? { ...defaultOptions } : {}) as TOptions | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and locate related symbols/usages
ast-grep outline src/api/providers/config-builder/request-config-builder.ts --view expanded || true
printf '\n--- usages ---\n'
rg -n "RequestConfigBuilder|fromMetadata\(|build\(" src/api/providers -S
printf '\n--- file slice ---\n'
cat -n src/api/providers/config-builder/request-config-builder.ts | sed -n '1,220p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 16810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests outline ---'
ast-grep outline src/api/providers/__tests__/request-config-builder.spec.ts --view expanded || true
echo '--- tests slice 1 ---'
cat -n src/api/providers/__tests__/request-config-builder.spec.ts | sed -n '1,220p'
echo '--- tests slice 2 ---'
cat -n src/api/providers/__tests__/request-config-builder.spec.ts | sed -n '221,520p'
echo '--- base provider usage ---'
cat -n src/api/providers/base-openai-compatible-provider.ts | sed -n '200,260p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 22447
Tighten the builder contract and clean up merged abort listeners
RequestConfigBuilderaccumulatesPartial<TOptions>internally, butbuild()/fromMetadata()expose the result asTOptions | undefined. That makes the generic promise completeness it can’t enforce; storePartial<TOptions>or add a final validation step before returningTOptions.mergeAbortSignals()only removes the listener that fires. If one source aborts or the request finishes normally, the sibling listener stays attached to the other signal, which leaks per-request listeners on long-lived signals.
🤖 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 13 -
18, `RequestConfigBuilder` is treating partially assembled options as if they
were a complete `TOptions`, so tighten the contract by changing the internal
`options` state and the `build()`/`fromMetadata()` flow in
`RequestConfigBuilder` to either keep `Partial<TOptions>` until the end or
validate required fields before returning `TOptions`. Also update
`mergeAbortSignals()` so both abort listeners are always cleaned up: remove the
sibling listener when either signal aborts and on normal completion, not just
the one that fired, to avoid leaving per-request listeners attached to
long-lived signals.
Comment on lines
+141
to
+145
| // Both are active: merge with event listeners | ||
| const controller = new AbortController() | ||
|
|
||
| primarySignal.addEventListener("abort", () => controller.abort(), { once: true }) | ||
| secondarySignal.addEventListener("abort", () => controller.abort(), { once: true }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Detach the source listeners after the merged signal settles.
These listeners are never removed from the non-aborted source signal. In the current downstream usage, each request merges a fresh internal signal with a caller-owned signal, so successful requests accumulate stale listeners/controllers on that long-lived external signal. Hook cleanup off the merged controller’s abort path (or explicitly remove the sibling listener in each handler).
🤖 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
- 145, Detach the abort listeners added in requestConfigBuilder’s merge logic so
they do not մն to the caller-owned signal after the merged AbortController
settles. In the branch that creates the controller and wires primarySignal and
secondarySignal, add cleanup tied to the merged controller’s abort path or
remove the sibling listener inside each abort handler. Keep the fix localized to
the signal-merging logic in request-config-builder.ts.
Comment on lines
+300
to
+302
| if (options?.timeoutMs) { | ||
| requestOptions.timeout = options.timeoutMs | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent timeoutMs guard — truthy check drops timeoutMs: 0.
Other handlers in this PR (gemini.ts, mistral.ts, opencode-go.ts) gate on options?.timeoutMs !== undefined, and the Mistral spec even asserts timeoutMs: 0 is forwarded. Here the truthy check filters out 0, producing divergent behavior for that edge case.
♻️ Align with the other handlers
- if (options?.timeoutMs) {
+ if (options?.timeoutMs !== undefined) {
requestOptions.timeout = options.timeoutMs
}📝 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
| if (options?.timeoutMs) { | |
| requestOptions.timeout = options.timeoutMs | |
| } | |
| if (options?.timeoutMs !== undefined) { | |
| requestOptions.timeout = options.timeoutMs | |
| } |
🤖 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/minimax.ts` around lines 300 - 302, The timeout handling in
minimax.ts uses a truthy guard, which skips forwarding timeoutMs when it is 0.
Update the check in the Minimax request options path to match the other handlers
(for example, the same undefined check used in gemini.ts, mistral.ts, and
opencode-go.ts) so options.timeoutMs is applied even when it is 0.
| createOptions.timeout = options.timeoutMs | ||
| } | ||
|
|
||
| const response = await this.client.chat.completions.create(requestOptions, createOptions || undefined) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
createOptions || undefined never yields undefined.
createOptions is always an object literal ({}), which is truthy, so createOptions || undefined always passes the object — even when no abortSignal/timeoutMs were set. This contradicts the empty-guard intent and diverges from the Anthropic path above (which uses Object.keys(...).length > 0 ? ... : undefined). Passing an empty {} is harmless to the SDK, but the guard is a misleading no-op.
♻️ Mirror the Anthropic-path guard
- 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,
+ )📝 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 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/opencode-go.ts` at line 558, The `createOptions ||
undefined` guard in `chat.completions.create` is a no-op because `createOptions`
is always an object, so empty options still get passed. Update the
`opencode-go.ts` request flow to mirror the Anthropic path by only passing
`createOptions` when it contains at least one key, otherwise pass `undefined`;
use the `response` call site and the `createOptions` construction nearby to
locate the change.
Comment on lines
+223
to
+229
| if (options?.timeoutMs) { | ||
| createOptions.timeout = options.timeoutMs | ||
| } | ||
|
|
||
| let response: OpenAI.Chat.ChatCompletion | ||
| try { | ||
| response = await this.client.chat.completions.create(completionParams) | ||
| response = await this.client.chat.completions.create(completionParams, createOptions || undefined) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent timeoutMs guard drops timeoutMs: 0.
The base provider and openai.ts use options?.timeoutMs !== undefined, which intentionally forwards timeoutMs: 0 (the base provider even has a dedicated test for it). Here the truthy check if (options?.timeoutMs) silently drops a 0 value, diverging from the rest of the cohort. The createOptions || undefined is also redundant since createOptions is always a non-null object.
🐛 Suggested fix
const createOptions: OpenAI.RequestOptions = {}
if (options?.abortSignal) {
createOptions.signal = options.abortSignal
}
- if (options?.timeoutMs) {
+ if (options?.timeoutMs !== undefined) {
createOptions.timeout = options.timeoutMs
}
let response: OpenAI.Chat.ChatCompletion
try {
- response = await this.client.chat.completions.create(completionParams, createOptions || undefined)
+ response = await this.client.chat.completions.create(completionParams, createOptions)📝 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
| if (options?.timeoutMs) { | |
| createOptions.timeout = options.timeoutMs | |
| } | |
| let response: OpenAI.Chat.ChatCompletion | |
| try { | |
| response = await this.client.chat.completions.create(completionParams) | |
| response = await this.client.chat.completions.create(completionParams, createOptions || undefined) | |
| if (options?.timeoutMs !== undefined) { | |
| createOptions.timeout = options.timeoutMs | |
| } | |
| let response: OpenAI.Chat.ChatCompletion | |
| try { | |
| response = await this.client.chat.completions.create(completionParams, createOptions) |
🤖 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/requesty.ts` around lines 223 - 229, The timeout handling
in requesty’s chat completion path is inconsistent with the rest of the
providers because the `timeoutMs` check in `RequestyProvider` drops a valid `0`
value. Update the guard in the `createOptions` setup inside the
`chat.completions.create` flow to match the base provider and `openai.ts`
behavior by checking for `undefined` instead of truthiness, so `timeoutMs: 0` is
forwarded. Also simplify the `createOptions || undefined` usage in the same
call, since `createOptions` is already a concrete object.
Comment on lines
+154
to
+155
| if (options?.timeoutMs) { | ||
| requestOptions.timeout = options.timeoutMs |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve explicit timeoutMs: 0 instead of treating it as omitted.
This truthy check drops a caller-supplied 0 and makes it indistinguishable from undefined, even though the shared contract only falls back when the option is omitted. Use an explicit undefined check here (or reject zero up front) so the API does not silently ignore a provided override.
Suggested fix
- if (options?.timeoutMs) {
+ if (options?.timeoutMs !== undefined) {
requestOptions.timeout = options.timeoutMs
}📝 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
| if (options?.timeoutMs) { | |
| requestOptions.timeout = options.timeoutMs | |
| if (options?.timeoutMs !== undefined) { | |
| requestOptions.timeout = options.timeoutMs |
🤖 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/xai.ts` around lines 154 - 155, Update the timeout handling
in xai.ts so a caller-provided timeoutMs of 0 is preserved instead of being
skipped by the truthy check. In the request-building logic that sets
requestOptions.timeout, change the condition to distinguish undefined from an
explicit zero (or validate and reject zero consistently) so the shared options
contract is honored when options.timeoutMs is passed into the provider.
Contributor
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
… signal propagation
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/bridging-providers
branch
from
90bc3ea to
c290504
Compare
- openai-native.ts: add Bedrock pattern ({ once: true } + pre-aborted guard)
to executeRequest() and makeResponsesApiRequest() fallback path
- openai-codex.ts: add metadata param to executeRequest() with Bedrock
pattern abort bridging
- bedrock.ts: add externalAbortSignal bridging to createMessage() using
{ once: true } + pre-aborted guard
- native-ollama.ts: replace ensureClient() singleton with per-request
_createOllamaClient(), use constructor headers option for API key
Tests:
- openai-native.spec.ts: add fallback fetch signal abort test
- openai-codex-native-tool-calls.spec.ts: add createMessage abort signal
tests (bridge + pre-aborted)
- bedrock.spec.ts: add completePrompt abort propagation test
- native-ollama.spec.ts: add per-request client creation and headers
option tests
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/bridging-providers
branch
from
c290504 to
337dca9
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
Adds custom
addEventListenerbridging and per-request client workarounds for 4 providers whose SDKs do not acceptAbortSignaldirectly.Closes #617 (part of #404, depends on #615)
Providers
openai-native.ts & openai-codex.ts
{ once: true }+ pre-aborted guardcreateMessageANDcompletePromptwired with abort signal supportopenai-native.ts: respects caller-ownedthis.abortController, no premature clearing infinallybedrock.ts
completePromptforwards signal via AWS SDK v3:this.client.send(command, { abortSignal }){ once: true }patternnative-ollama.ts
new Ollama()client with constructorheadersoption (no more(requestClient as any).configmutation)ensureClient()/ singleton removed, extracted to private helperChanges at a Glance
Closes #617
Summary by CodeRabbit
New Features
Bug Fixes