test(providers): add completePrompt signal/timeout tests for all 25 providers#712
Draft
easonLiangWorldedtech wants to merge 14 commits into
Conversation
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangescompletePrompt options propagation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/api/providers/openai-codex.ts (1)
1156-1227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
timeoutMsis not enforced in this Codex request.This
completePromptpath only wires cancellation.options.timeoutMsis unused, so the non-streaming Codex request can outlive the caller's requested timeout even though the new API surface now exposes that option.🤖 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 1156 - 1227, The completePrompt flow in openai-codex.ts only forwards cancellation and ignores options.timeoutMs, so the Codex request can run past the caller’s timeout. Update completePrompt to enforce timeoutMs alongside the existing abort handling by creating a timeout-driven abort signal/controller, merging it with the current signals before the fetch call, and making sure the request is canceled when the timeout elapses while preserving the existing mergedSignal and abortController behavior.src/api/providers/anthropic-vertex.ts (1)
273-302: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winForward or reject
timeoutMshere too.
CompletePromptOptions.timeoutMsis unused in this path, so Vertex-backed Anthropic completions do not honor the new per-call timeout contract. That leavescompletePromptbehavior inconsistent across providers.🤖 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 273 - 302, The Vertex Anthropic path in completePrompt is ignoring CompletePromptOptions.timeoutMs, so per-call timeouts are not honored. Update completePrompt in anthropic-vertex to forward timeoutMs alongside the existing signal handling when calling this.client.messages.create, matching the timeout behavior used by other providers and keeping the CompletePromptOptions contract consistent.src/api/providers/openai-native.ts (1)
1487-1563: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe new timeout option is still dropped here.
completePromptnow honors caller abort signals, butoptions.timeoutMsnever affects theresponses.createcall. That leaves OpenAI Native without the per-call timeout behavior this PR is adding elsewhere.🤖 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 1487 - 1563, The new per-call timeout is not being applied in completePrompt, so the responses.create request only respects abort signals and ignores options.timeoutMs. Update the OpenAI Native path in completePrompt to wire the timeout into the request lifecycle alongside the existing mergedSignal logic, using the same timeout handling pattern added elsewhere in this provider. Make sure the fix is localized to the completePrompt method and its requestOptions passed to responses.create.src/api/providers/gemini.ts (1)
579-599: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
timeoutMsis still dropped incompletePrompt.Only the abort signal is threaded into
httpOptions. Callers that passCompletePromptOptions.timeoutMswill not get the requested timeout behavior on Gemini requests.🤖 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/gemini.ts` around lines 579 - 599, The timeout handling in completePrompt is incomplete because only options.signal is copied into httpOptions, so CompletePromptOptions.timeoutMs is ignored. Update the Gemini request setup in completePrompt to thread timeoutMs through the HTTP options alongside signal, using the existing httpOpts/promptConfig flow so Gemini requests honor caller-provided timeouts.src/api/providers/native-ollama.ts (1)
347-378: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThis provider currently drops
CompletePromptOptionsentirely.Accepting
_optionsbut ignoring bothsignalandtimeoutMsmeans abort/timeout callers get no effect at all. Even if the Ollama client cannot cancel the transport, this method still needs caller-visible timeout/abort handling to match the newcompletePromptcontract.🤖 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/native-ollama.ts` around lines 347 - 378, The completePrompt implementation in native-ollama ignores CompletePromptOptions, so callers cannot abort or time out requests. Update completePrompt to honor the options contract by using signal and timeoutMs from _options in the native-ollama provider flow, and ensure the client.chat call in completePrompt still returns promptly with a caller-visible abort/timeout error even if the Ollama transport itself cannot be canceled.src/api/providers/bedrock.ts (1)
799-844: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate
timeoutMshere as well.This path forwards
options.signalbut silently ignoresoptions.timeoutMs, socompletePromptfalls back to the client default instead of honoring the caller's per-request timeout. That breaks the new cross-providerCompletePromptOptionscontract.🤖 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 799 - 844, Propagate the per-request timeout through `BedrockProvider.completePrompt` the same way `options.signal` is already forwarded, since this method currently ignores `options.timeoutMs` and falls back to the client default. Update the `completePrompt` call path around `this.client.send` to include the timeout handling expected by `CompletePromptOptions`, matching the behavior used elsewhere in the provider so caller-supplied timeouts are honored consistently.src/api/providers/vscode-lm.ts (1)
578-600: 🎯 Functional Correctness | 🔴 CriticalMove the
catchback insidecompletePrompt.
The method closes beforecatch, so this block is invalid TypeScript syntax and will not parse.🤖 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 578 - 600, The `completePrompt` method is missing its `catch` in the correct scope, which makes the TypeScript invalid. Move the existing `catch(error: any)` block back inside `completePrompt`, after the `try/finally` that uses `client.sendRequest` and `tokenSource.dispose()`, so the method is syntactically complete and still rethrows the wrapped `VSCode LM completion error`.
🧹 Nitpick comments (3)
src/api/providers/config-builder/README.md (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroken relative links to
request-config-builder.ts.This README lives in
config-builder/alongsiderequest-config-builder.ts, so../request-config-builder.ts:13and:101point one directory too high. Use./request-config-builder.ts.🤖 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 53 - 54, The README links to RequestConfigBuilder are using the wrong relative path and point one directory too high. Update the markdown references in the config-builder README so they point to the local request-config-builder.ts file from the same folder, keeping the existing symbol references for RequestConfigBuilder and its methods but changing the path target to the correct relative location.src/api/providers/__tests__/poe.spec.ts (1)
328-340: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueBackward-compat test doesn't actually verify
signalis absent.
expect.objectContaining({ model, prompt })only checks those keys are present and permits additional properties, so this assertion would still pass even if asignalwere incorrectly forwarded. If the intent is to confirm nosignalleaks through when options are omitted, assert the property is undefined explicitly.♻️ Stronger assertion for signal absence
const result = await handler.completePrompt("test prompt") expect(result).toBe("response") expect(mockGenerateText).toHaveBeenCalledWith( expect.objectContaining({ model: mockLanguageModel, prompt: "test prompt", }), ) + expect(mockGenerateText.mock.calls[0][0]).not.toHaveProperty("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__/poe.spec.ts` around lines 328 - 340, The backward-compatibility test in PoeHandler.completePrompt currently only checks for model and prompt, so it would still pass if signal is unintentionally forwarded. Update the assertion in poe.spec.ts to explicitly verify that no signal is present when completePrompt is called without options, using the existing mockGenerateText expectation so the test fails if a signal leaks through.src/api/providers/fake-ai.ts (1)
78-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the new contract type-checked.
this.aiis already typed asFakeAI, so theas anycast just disables compile-time enforcement of the updatedcompletePrompt(prompt, options)signature for test doubles. Callingthis.ai.completePrompt(prompt, options)directly keeps that contract honest.🤖 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 is bypassing the typed contract by casting `this.ai` to `any`, which defeats compile-time checking of the new `completePrompt(prompt, options)` signature. Update the `completePrompt` method in `FakeAI` to call `this.ai.completePrompt(prompt, options)` directly so the `FakeAI` type remains enforced for test doubles.
🤖 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__/anthropic.spec.ts`:
- Around line 505-520: The `handler.completePrompt` merge test only checks that
`controller.signal` is forwarded, so it can still pass even if `timeoutMs` is
ignored. Tighten the test in `anthropic.spec.ts` by asserting the merged abort
behavior from `completePrompt`/`mockCreate`: verify the signal passed to
`mockCreate` is a distinct merged signal that aborts after the timeout window,
or that it aborts when the timeout elapses even without the controller being
aborted.
In `@src/api/providers/__tests__/native-ollama.spec.ts`:
- Around line 238-245: The native Ollama test currently only checks a partial
payload with objectContaining, so it can still pass if a signal property is
present. Harden the expectation in native-ollama.spec.ts around mockChat by
asserting the request payload does not include signal explicitly, using the
existing mockChat assertion block and the same model/messages/options fields as
anchors. Keep the current positive checks, but add a separate negative assertion
on the chat call argument so any forwarded signal option causes the test to
fail.
In `@src/api/providers/__tests__/openai-codex-native-tool-calls.spec.ts`:
- Around line 551-553: The test around the mock fetch call in
openai-codex-native-tool-calls.spec should assert the exact AbortSignal instance
instead of only checking that a signal property exists. Update the expectation
on mockFetch.mock.calls[0] so it verifies the passed request options contain the
same signal object that was provided to the API path, using the fetchCallArgs
variable and the surrounding test setup to compare identity rather than property
presence.
In `@src/api/providers/anthropic.ts`:
- Around line 401-416: The completePrompt method in anthropic.ts handles abort
signals but still ignores per-call timeoutMs, so update the client call path to
either pass the timeout through using the existing request options or explicitly
reject unsupported timeoutMs values. Use completePrompt and
this.client.messages.create as the main touchpoints, and make sure the behavior
matches the new CompletePromptOptions contract consistently with signal
handling.
In `@src/api/providers/base-openai-compatible-provider.ts`:
- Around line 229-238: Preserve explicit timeout values in the provider request
builder: the truthy check in base-openai-compatible-provider’s completePrompt
path drops timeoutMs = 0, so change the guard to forward any defined timeout
value while still omitting only undefined/null inputs. Apply the same fix
anywhere the PR added similar timeout propagation logic in other completePrompt
implementations, and verify RequestOptions.timeout is set from options.timeoutMs
even when it is zero.
In `@src/api/providers/config-builder/README.md`:
- Around line 200-230: The README documentation for `mergeAbortSignals` is
inconsistent with `request-config-builder.ts` and currently documents the
secondary-aborted bug. Update the behavior description to match the real
`mergeAbortSignals` contract: if `secondarySignal` is missing or already
aborted, the returned signal should reflect the correct abort state instead of
being described as `primarySignal` unchanged, and the primary-aborted branch
should be documented as returning the primary signal directly. Keep the table
and the `mergeAbortSignals` summary aligned with the implementation in
`request-config-builder.ts`.
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 119-134: The mergeAbortSignals method in RequestConfigBuilder is
returning the primary signal when secondarySignal is already aborted, which
breaks the “any aborted signal aborts the result” contract. Update
mergeAbortSignals so it returns an already-aborted signal whenever either
primarySignal or secondarySignal is aborted, and keep the listener-based merged
AbortController path only for the case where both signals are still active. Make
sure the fix is applied in mergeAbortSignals and preserves the existing abort
propagation behavior for both signals.
In `@src/api/providers/minimax.ts`:
- Around line 292-304: The completePrompt implementation in Minimax still
ignores timeoutMs, so the request can exceed the caller’s deadline. Update
completePrompt to honor CompletePromptOptions.timeoutMs in addition to signal by
wiring both into the client.messages.create call, using the existing options
handling in completePrompt and the MiniMax client request path so non-streaming
completions are aborted when the timeout is reached.
In `@src/api/providers/mistral.ts`:
- Around line 196-207: The completePrompt flow in mistral.ts is dropping the
caller’s timeout setting, so requests can outlive the deadline. Update the
Mistral provider’s completePrompt method to translate options.timeoutMs into a
request timeout/abort path and pass it through alongside the existing signal
handling when calling this.client.chat.complete. Make sure the timeout behavior
matches the contract used by other providers in the same completePrompt
implementation.
In `@src/api/providers/openai-compatible.ts`:
- Around line 200-208: The per-call timeout override is not being passed through
in completePrompt, so callers using options.timeoutMs still fall back to the
provider default. Update the completePrompt method in openai-compatible.ts to
forward the timeout override into the generateText call alongside
options.signal, using the existing CompletePromptOptions shape and the
getLanguageModel/getMaxOutputTokens flow.
In `@src/api/providers/opencode-go.ts`:
- Around line 491-509: The Anthropic-format path in completePrompt currently
ignores timeoutMs, unlike the OpenAI-format branch, so add timeout handling here
as well. Update the this.anthropicClient.messages.create call path to forward
options.timeoutMs when supported, or explicitly reject/fail fast in the
Anthropic branch if the timeout cannot be applied. Use the completePrompt method
and the anthropicClient.messages.create call site to locate the change.
In `@src/api/providers/poe.ts`:
- Around line 137-143: The Poe completePrompt implementation in completePrompt
currently forwards only options.signal, so CompletePromptOptions.timeoutMs is
being ignored. Update this method to apply the per-request timeout behavior as
well, using the same timeout/abort handling pattern used elsewhere in the
provider API, and ensure generateText receives both the signal and
timeout-derived abort control when options.timeoutMs is provided.
In `@src/api/providers/xai.ts`:
- Around line 145-156: The xAI request path in completePrompt still ignores the
per-request timeout option, so update the request setup in xai.ts to forward
options.timeoutMs along with options.signal when calling
this.client.responses.create. Use the completePrompt method and the
client.responses.create invocation as the integration point, and ensure the
timeout is translated into the request options in the same way the new API
contract expects.
---
Outside diff comments:
In `@src/api/providers/anthropic-vertex.ts`:
- Around line 273-302: The Vertex Anthropic path in completePrompt is ignoring
CompletePromptOptions.timeoutMs, so per-call timeouts are not honored. Update
completePrompt in anthropic-vertex to forward timeoutMs alongside the existing
signal handling when calling this.client.messages.create, matching the timeout
behavior used by other providers and keeping the CompletePromptOptions contract
consistent.
In `@src/api/providers/bedrock.ts`:
- Around line 799-844: Propagate the per-request timeout through
`BedrockProvider.completePrompt` the same way `options.signal` is already
forwarded, since this method currently ignores `options.timeoutMs` and falls
back to the client default. Update the `completePrompt` call path around
`this.client.send` to include the timeout handling expected by
`CompletePromptOptions`, matching the behavior used elsewhere in the provider so
caller-supplied timeouts are honored consistently.
In `@src/api/providers/gemini.ts`:
- Around line 579-599: The timeout handling in completePrompt is incomplete
because only options.signal is copied into httpOptions, so
CompletePromptOptions.timeoutMs is ignored. Update the Gemini request setup in
completePrompt to thread timeoutMs through the HTTP options alongside signal,
using the existing httpOpts/promptConfig flow so Gemini requests honor
caller-provided timeouts.
In `@src/api/providers/native-ollama.ts`:
- Around line 347-378: The completePrompt implementation in native-ollama
ignores CompletePromptOptions, so callers cannot abort or time out requests.
Update completePrompt to honor the options contract by using signal and
timeoutMs from _options in the native-ollama provider flow, and ensure the
client.chat call in completePrompt still returns promptly with a caller-visible
abort/timeout error even if the Ollama transport itself cannot be canceled.
In `@src/api/providers/openai-codex.ts`:
- Around line 1156-1227: The completePrompt flow in openai-codex.ts only
forwards cancellation and ignores options.timeoutMs, so the Codex request can
run past the caller’s timeout. Update completePrompt to enforce timeoutMs
alongside the existing abort handling by creating a timeout-driven abort
signal/controller, merging it with the current signals before the fetch call,
and making sure the request is canceled when the timeout elapses while
preserving the existing mergedSignal and abortController behavior.
In `@src/api/providers/openai-native.ts`:
- Around line 1487-1563: The new per-call timeout is not being applied in
completePrompt, so the responses.create request only respects abort signals and
ignores options.timeoutMs. Update the OpenAI Native path in completePrompt to
wire the timeout into the request lifecycle alongside the existing mergedSignal
logic, using the same timeout handling pattern added elsewhere in this provider.
Make sure the fix is localized to the completePrompt method and its
requestOptions passed to responses.create.
In `@src/api/providers/vscode-lm.ts`:
- Around line 578-600: The `completePrompt` method is missing its `catch` in the
correct scope, which makes the TypeScript invalid. Move the existing
`catch(error: any)` block back inside `completePrompt`, after the `try/finally`
that uses `client.sendRequest` and `tokenSource.dispose()`, so the method is
syntactically complete and still rethrows the wrapped `VSCode LM completion
error`.
---
Nitpick comments:
In `@src/api/providers/__tests__/poe.spec.ts`:
- Around line 328-340: The backward-compatibility test in
PoeHandler.completePrompt currently only checks for model and prompt, so it
would still pass if signal is unintentionally forwarded. Update the assertion in
poe.spec.ts to explicitly verify that no signal is present when completePrompt
is called without options, using the existing mockGenerateText expectation so
the test fails if a signal leaks through.
In `@src/api/providers/config-builder/README.md`:
- Around line 53-54: The README links to RequestConfigBuilder are using the
wrong relative path and point one directory too high. Update the markdown
references in the config-builder README so they point to the local
request-config-builder.ts file from the same folder, keeping the existing symbol
references for RequestConfigBuilder and its methods but changing the path target
to the correct relative location.
In `@src/api/providers/fake-ai.ts`:
- Around line 78-79: The `FakeAI.completePrompt` implementation is bypassing the
typed contract by casting `this.ai` to `any`, which defeats compile-time
checking of the new `completePrompt(prompt, options)` signature. Update the
`completePrompt` method in `FakeAI` to call `this.ai.completePrompt(prompt,
options)` directly so the `FakeAI` type remains enforced for test doubles.
🪄 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: a551a1e6-63c3-4248-b88e-d3db1412f58a
📒 Files selected for processing (60)
src/api/index.tssrc/api/providers/__tests__/anthropic-vertex.spec.tssrc/api/providers/__tests__/anthropic.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.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__/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/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/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/utils/single-completion-handler.ts
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/openai-compatible.ts`:
- Around line 210-217: The abort handling in the OpenAI-compatible provider
currently drops timeoutMs whenever options.signal is present, so merge both
inputs instead of choosing one. Update the logic in openai-compatible.ts around
generateOptions.abortSignal to combine options.signal and options.timeoutMs
using RequestConfigBuilder.mergeAbortSignals (or the same equivalent helper used
elsewhere), so the generated request keeps both cancellation sources active.
🪄 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: a07d2d67-353b-4eaa-9af5-b73e2665bcd2
📒 Files selected for processing (17)
src/api/providers/__tests__/anthropic.spec.tssrc/api/providers/__tests__/bedrock.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__/poe.spec.tssrc/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/anthropic.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/config-builder/request-config-builder.tssrc/api/providers/minimax.tssrc/api/providers/mistral.tssrc/api/providers/openai-compatible.tssrc/api/providers/opencode-go.tssrc/api/providers/poe.tssrc/api/providers/vscode-lm.tssrc/api/providers/xai.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/api/providers/minimax.ts
- src/api/providers/poe.ts
- src/api/providers/tests/moonshot.spec.ts
- src/api/providers/tests/native-ollama.spec.ts
- src/api/providers/tests/bedrock.spec.ts
- src/api/providers/tests/poe.spec.ts
- src/api/providers/base-openai-compatible-provider.ts
- src/api/providers/opencode-go.ts
- src/api/providers/vscode-lm.ts
- src/api/providers/mistral.ts
- src/api/providers/config-builder/request-config-builder.ts
- src/api/providers/tests/request-config-builder.spec.ts
Contributor
There was a problem hiding this comment.
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/openai-compatible.ts (1)
211-232: 🩺 Stability & Availability | 🟠 MajorHandle pre-aborted signals and clear the merged timeout
options.signal.addEventListener("abort", ...)won’t fire for an already-aborted signal, so the merged controller can miss immediate cancellation. The timeout also stays pending aftergenerateTextreturns unless the external signal aborts. Add anoptions.signal.abortedguard and clean up the timer/listener infinally.🤖 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-compatible.ts` around lines 211 - 232, The abort/timeout merge in openai-compatible.ts’s generateText flow does not handle already-aborted signals and can leave the merged timeout pending after the request completes. Update the logic around the options.signal / timeoutMs handling to check options.signal.aborted before attaching the listener, and ensure the merged controller’s timer and abort listener are always cleaned up in a finally block after generateText. Keep the fix localized to the generateOptions.abortSignal setup and the surrounding await generateText(generateOptions) call.
🧹 Nitpick comments (1)
src/api/providers/__tests__/vscode-lm.spec.ts (1)
618-662: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen these timeout tests with cancellation-path assertions (not just call existence).
toHaveBeenCalled()alone won’t catch regressions wheretimeoutMs/merged signal wiring is ignored; assert the cancellation token/bridging side effects explicitly.🤖 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__/vscode-lm.spec.ts` around lines 618 - 662, The timeout-related tests in vscodelm.spec are only checking that completePrompt calls sendRequest, which won’t detect regressions in the cancellation wiring. Update the tests around completePrompt and the timeout/abort handling to assert the actual cancellation-path side effects for timeoutMs and the combined signal case, using the mocked sendRequest invocation and any timeout/abort bridging behavior so the test verifies that the cancellation token is created and passed through correctly.
🤖 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.
Outside diff comments:
In `@src/api/providers/openai-compatible.ts`:
- Around line 211-232: The abort/timeout merge in openai-compatible.ts’s
generateText flow does not handle already-aborted signals and can leave the
merged timeout pending after the request completes. Update the logic around the
options.signal / timeoutMs handling to check options.signal.aborted before
attaching the listener, and ensure the merged controller’s timer and abort
listener are always cleaned up in a finally block after generateText. Keep the
fix localized to the generateOptions.abortSignal setup and the surrounding await
generateText(generateOptions) call.
---
Nitpick comments:
In `@src/api/providers/__tests__/vscode-lm.spec.ts`:
- Around line 618-662: The timeout-related tests in vscodelm.spec are only
checking that completePrompt calls sendRequest, which won’t detect regressions
in the cancellation wiring. Update the tests around completePrompt and the
timeout/abort handling to assert the actual cancellation-path side effects for
timeoutMs and the combined signal case, using the mocked sendRequest invocation
and any timeout/abort bridging behavior so the test verifies that the
cancellation token is created and passed through correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76223aa0-26d4-4737-be03-b872b7252b71
📒 Files selected for processing (12)
src/api/providers/__tests__/anthropic.spec.tssrc/api/providers/__tests__/gemini.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__/opencode-go.spec.tssrc/api/providers/__tests__/vercel-ai-gateway.spec.tssrc/api/providers/__tests__/vscode-lm.spec.tssrc/api/providers/config-builder/README.mdsrc/api/providers/openai-compatible.tssrc/api/providers/poe.tssrc/api/providers/vercel-ai-gateway.ts
💤 Files with no reviewable changes (1)
- src/api/providers/tests/openai-codex-native-tool-calls.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/api/providers/config-builder/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/api/providers/poe.ts
- src/api/providers/tests/vercel-ai-gateway.spec.ts
- src/api/providers/tests/anthropic.spec.ts
- src/api/providers/tests/opencode-go.spec.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/tests/native-ollama.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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__/poe.spec.ts`:
- Around line 378-388: The assertion in PoeHandler completePrompt is
tautological because abortSignal is never an AbortController, so it does not
verify merged-signal behavior. Update the test in poe.spec.ts to compare
callArgs.abortSignal against the original controller.signal and assert they are
not the same object while still checking it is defined, using completePrompt and
mockGenerateText to confirm the handler creates a distinct merged AbortSignal
when both signal and timeoutMs are provided.
🪄 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: 930311e2-7b14-4203-8aca-aea1b3077ad6
📒 Files selected for processing (5)
src/api/providers/__tests__/minimax.spec.tssrc/api/providers/__tests__/mistral.spec.tssrc/api/providers/__tests__/moonshot.spec.tssrc/api/providers/__tests__/poe.spec.tssrc/api/providers/__tests__/xai.spec.ts
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/complete-prompt-abort
branch
2 times, most recently
from
d0d5d8f to
8fe2b1c
Compare
Contributor
|
I'll review more in depth once #701 lands |
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/complete-prompt-abort
branch
from
8fe2b1c to
b33e17c
Compare
…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)
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/complete-prompt-abort
branch
from
5f2900d to
8b83fda
Compare
…tial-request test
…-code, lmstudio-native
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/api/providers/fetchers/__tests__/modelCache.spec.ts (1)
474-485: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen
writeModelsassertion to verify serialization/path.Since
safeWriteJsonis mocked,resolves.toBeUndefined()passes almost unconditionally and doesn't exercise the function's actual contract: building the${router}_models.jsonfilename and forwarding the data payload. Assert the mock was called with the expected filename and data.♻️ Suggested stronger assertion
await expect(writeModels("openrouter", mockModels)).resolves.toBeUndefined() + expect(safeWriteJson).toHaveBeenCalledWith( + expect.stringContaining("openrouter_models.json"), + mockModels, + )As per coding guidelines: "Use package-local unit tests for ... serialization, request construction ...".
🤖 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 writeModels test only checks for a resolved promise, which is too weak because safeWriteJson is mocked. Update the writeModels spec to assert the mock receives the expected ${router}_models.json filename and the same model payload, so the test verifies serialization/path construction instead of just completion. Use the writeModels and safeWriteJson symbols in src/api/providers/fetchers/__tests__/modelCache.spec.ts to locate the assertion.Source: Coding guidelines
src/api/providers/qwen-code.ts (1)
330-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a top-level named import over an inline
import(...)type.Other OpenAI-compatible providers import
CompletePromptOptionsfrom../indexdirectly; the inlineimport("../index").CompletePromptOptionshere is inconsistent and harder to read. Consider importing it normally.🤖 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/qwen-code.ts` at line 330, The completePrompt method in qwen-code.ts uses an inline import type for CompletePromptOptions, which is inconsistent with the other OpenAI-compatible providers. Update the provider module to use a top-level named import from ../index and reference CompletePromptOptions directly in completePrompt so the signature matches the rest of the codebase and is easier to read.
🤖 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__/lmstudio.spec.ts`:
- Around line 174-182: Add a package-local unit test in the lmstudio spec to
cover the zero-timeout path, since LmStudioHandler.completePrompt currently
relies on a truthy check and may drop timeoutMs: 0 instead of forwarding it.
Extend the existing timeout forwarding test in completePrompt to call the
handler with { timeoutMs: 0 } and assert that mockCreate receives { timeout: 0
}, so the regression is caught alongside the current nonzero timeout case.
In `@src/api/providers/__tests__/poe.spec.ts`:
- Around line 390-401: The test name in PoeHandler’s completePrompt suite does
not match the behavior it verifies, since it never aborts the AbortController
and therefore does not cover timeout cleanup. Update the test in poe.spec.ts to
actually trigger controller.abort(), then assert the merged abortSignal behavior
and that the timeout is cleared or the signal is aborted as intended. Use the
existing completePrompt and mockGenerateText setup to keep the check focused on
the abort/timeout path.
In `@src/api/providers/opencode-go.ts`:
- Around line 498-500: The timeout override in opencode-go is using a truthy
check, so a caller-supplied timeoutMs of 0 is ignored and the SDK default is
used instead. Update the option handling in the relevant request-building paths
(the timeout assignment near the current requestOptions logic and the second
provider path mentioned in the review) to check for explicit presence of
timeoutMs rather than truthiness, so zero is treated as an intentional value and
applied by both code paths.
In `@src/api/providers/poe.ts`:
- Around line 144-166: The merged abort handling in `generateText` setup leaks
the timeout and misses already-aborted input signals. Update the `abortSignal +
timeoutMs` branch in `src/api/providers/poe.ts` so the timeout is always cleared
in a `finally` after `generateText(generateOptions)` completes, and ensure a
pre-aborted `options.abortSignal` immediately aborts the merged controller
before the request starts. Use the existing
`RequestConfigBuilder.mergeAbortSignals` behavior as the reference for the
correct merging semantics.
In `@src/api/providers/qwen-code.ts`:
- Around line 341-353: The timeout guard in the Qwen code provider currently
uses a truthy check, which drops an explicit timeoutMs value of 0 and diverges
from the base provider behavior. Update the fetchOptions setup in qwen-code.ts
to gate timeout assignment with an explicit undefined check, matching the logic
used by the base OpenAI-compatible provider and keeping
callApiWithRetry/client.chat.completions.create behavior consistent.
---
Nitpick comments:
In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts`:
- Around line 474-485: The writeModels test only checks for a resolved promise,
which is too weak because safeWriteJson is mocked. Update the writeModels spec
to assert the mock receives the expected ${router}_models.json filename and the
same model payload, so the test verifies serialization/path construction instead
of just completion. Use the writeModels and safeWriteJson symbols in
src/api/providers/fetchers/__tests__/modelCache.spec.ts to locate the assertion.
In `@src/api/providers/qwen-code.ts`:
- Line 330: The completePrompt method in qwen-code.ts uses an inline import type
for CompletePromptOptions, which is inconsistent with the other
OpenAI-compatible providers. Update the provider module to use a top-level named
import from ../index and reference CompletePromptOptions directly in
completePrompt so the signature matches the rest of the codebase and is easier
to read.
🪄 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: d58847bd-768c-4020-a9cf-a6bcdc19e811
📒 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
✅ Files skipped from review due to trivial changes (2)
- src/api/providers/index.ts
- src/api/providers/config-builder/README.md
🚧 Files skipped from review as they are similar to previous changes (48)
- src/utils/tests/enhance-prompt.spec.ts
- src/api/providers/tests/sambanova.spec.ts
- src/utils/single-completion-handler.ts
- src/api/providers/tests/gemini-handler.spec.ts
- src/api/providers/openrouter.ts
- src/api/providers/lm-studio.ts
- src/api/providers/tests/complete-prompt-options.spec.ts
- src/api/providers/xai.ts
- src/api/providers/anthropic-vertex.ts
- src/api/providers/mistral.ts
- src/api/providers/gemini.ts
- src/api/providers/lite-llm.ts
- src/api/providers/tests/zai.spec.ts
- src/api/providers/tests/openai.spec.ts
- src/api/providers/base-openai-compatible-provider.ts
- src/api/providers/tests/openrouter.spec.ts
- src/api/providers/tests/unbound.spec.ts
- src/api/providers/openai-codex.ts
- src/api/providers/zoo-gateway.ts
- src/api/providers/fake-ai.ts
- src/api/providers/bedrock.ts
- src/api/providers/tests/minimax.spec.ts
- src/api/providers/unbound.ts
- src/api/providers/tests/mistral.spec.ts
- src/api/providers/tests/zoo-gateway.spec.ts
- src/api/providers/requesty.ts
- src/api/providers/tests/anthropic.spec.ts
- src/api/providers/native-ollama.ts
- src/api/providers/vscode-lm.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/tests/requesty.spec.ts
- src/api/providers/anthropic.ts
- src/api/providers/tests/gemini.spec.ts
- src/api/providers/tests/vertex.spec.ts
- src/api/providers/tests/xai.spec.ts
- src/api/providers/tests/openai-codex-native-tool-calls.spec.ts
- src/api/providers/openai.ts
- src/api/providers/tests/deepseek.spec.ts
- src/api/providers/openai-native.ts
- src/api/providers/config-builder/request-config-builder.ts
- src/api/providers/tests/native-ollama.spec.ts
- src/api/providers/tests/mimo.spec.ts
- src/api/providers/openai-compatible.ts
- src/api/providers/tests/vercel-ai-gateway.spec.ts
- src/api/providers/tests/openai-native.spec.ts
- src/api/providers/minimax.ts
- src/api/providers/tests/moonshot.spec.ts
- src/api/providers/tests/request-config-builder.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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)
572-576: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
timeoutMs: 0is still ignored here.Line 574 uses a truthy guard, so
completePrompt(..., { timeoutMs: 0 })never cancels. The other providers in this PR moved to defined checks specifically to preserve0, so this breaks the sharedCompletePromptOptionscontract.🛠️ Proposed fix
- if (options?.timeoutMs && options.timeoutMs > 0) { - timeoutTimeout = setTimeout(() => tokenSource.cancel(), options.timeoutMs) + if (options?.timeoutMs !== undefined && options.timeoutMs >= 0) { + if (options.timeoutMs === 0) { + tokenSource.cancel() + } else { + timeoutTimeout = setTimeout(() => tokenSource.cancel(), 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/vscode-lm.ts` around lines 572 - 576, The timeout handling in completePrompt still ignores timeoutMs = 0 because it uses a truthy check before creating the cancellation timer. Update the guard in the vscode-lm provider’s completePrompt flow to check for a defined timeoutMs value instead of truthiness, while still only scheduling the timer when the value is non-negative as intended. Keep the fix aligned with the shared CompletePromptOptions behavior used by the other provider implementations.
🤖 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/bedrock.ts`:
- Around line 842-875: The request options builder in bedrock.ts does not abort
immediately when timeoutMs is 0 without a caller-provided signal, and the
timeout timer created for the merged AbortSignal is not cleaned up after
client.send finishes. Update the sendOptions/AbortController logic to ensure
timeoutMs: 0 always produces an already-aborted signal even when
options.abortSignal is absent, and refactor the timeout path in the send() call
so any setTimeout handle is cleared once the request resolves or rejects. Use
the existing sendOptions builder and this.client.send flow as the place to fix
it.
In `@src/api/providers/openai-codex.ts`:
- Around line 1165-1168: The request-local timeout in openai-codex’s fetch flow
is left pending when the request finishes early, so it can later abort an
already completed controller. Update the timeout handling around the
`options.timeoutMs` / `timeoutId` logic in `fetch` to always clear the timer in
the `finally` block, and make sure the `localAbortController` is only aborted by
an active request timeout.
---
Outside diff comments:
In `@src/api/providers/vscode-lm.ts`:
- Around line 572-576: The timeout handling in completePrompt still ignores
timeoutMs = 0 because it uses a truthy check before creating the cancellation
timer. Update the guard in the vscode-lm provider’s completePrompt flow to check
for a defined timeoutMs value instead of truthiness, while still only scheduling
the timer when the value is non-negative as intended. Keep the fix aligned with
the shared CompletePromptOptions behavior used by the other provider
implementations.
🪄 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: e204c493-54e6-434e-994f-c36ad6de967a
📒 Files selected for processing (29)
src/api/providers/__tests__/anthropic-vertex.spec.tssrc/api/providers/__tests__/anthropic.spec.tssrc/api/providers/__tests__/bedrock.spec.tssrc/api/providers/__tests__/gemini-handler.spec.tssrc/api/providers/__tests__/gemini.spec.tssrc/api/providers/__tests__/minimax.spec.tssrc/api/providers/__tests__/mistral.spec.tssrc/api/providers/__tests__/moonshot.spec.tssrc/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/__tests__/vertex.spec.tssrc/api/providers/__tests__/xai.spec.tssrc/api/providers/anthropic-vertex.tssrc/api/providers/bedrock.tssrc/api/providers/config-builder/README.mdsrc/api/providers/fetchers/__tests__/modelCache.spec.tssrc/api/providers/gemini.tssrc/api/providers/lite-llm.tssrc/api/providers/minimax.tssrc/api/providers/mistral.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/openrouter.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.ts
✅ Files skipped from review due to trivial changes (1)
- src/api/providers/config-builder/README.md
🚧 Files skipped from review as they are similar to previous changes (17)
- src/api/providers/tests/vertex.spec.ts
- src/api/providers/unbound.ts
- src/api/providers/minimax.ts
- src/api/providers/openrouter.ts
- src/api/providers/anthropic-vertex.ts
- src/api/providers/openai.ts
- src/api/providers/zoo-gateway.ts
- src/api/providers/tests/bedrock.spec.ts
- src/api/providers/xai.ts
- src/api/providers/lite-llm.ts
- src/api/providers/mistral.ts
- src/api/providers/tests/gemini-handler.spec.ts
- src/api/providers/tests/moonshot.spec.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/fetchers/tests/modelCache.spec.ts
- src/api/providers/requesty.ts
- src/api/providers/tests/request-config-builder.spec.ts
Comment on lines
+842
to
+875
| // Build request options with abortSignal and/or timeoutMs | ||
| const sendOptions: { abortSignal?: AbortSignal } | undefined = (() => { | ||
| let signal: AbortSignal | undefined = options?.abortSignal | ||
| if (options?.timeoutMs !== undefined) { | ||
| if (signal) { | ||
| // When both are provided, create a merged signal that aborts when either fires | ||
| const controller = new AbortController() | ||
| if (signal.aborted) { | ||
| controller.abort() | ||
| } else if (options.timeoutMs > 0) { | ||
| const timeoutId = setTimeout(() => controller.abort(), options.timeoutMs) | ||
| signal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| clearTimeout(timeoutId) | ||
| controller.abort() | ||
| }, | ||
| { once: true }, | ||
| ) | ||
| } else { | ||
| // timeoutMs is 0, abort immediately | ||
| controller.abort() | ||
| } | ||
| signal = controller.signal | ||
| } else if (options.timeoutMs !== undefined) { | ||
| if (options.timeoutMs > 0) { | ||
| signal = AbortSignal.timeout(options.timeoutMs) | ||
| } | ||
| } | ||
| } | ||
| return signal ? { abortSignal: signal } : undefined | ||
| })() | ||
|
|
||
| const response = await this.client.send(command, sendOptions) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle timeoutMs: 0 without a caller signal and clear merged timeout timers.
Line 866 leaves signal unset for { timeoutMs: 0 }, so the Bedrock request runs instead of aborting immediately. Also, the timer created on Line 852 is not cleared when client.send completes before the timeout.
Proposed fix
async completePrompt(prompt: string, options?: import("../index").CompletePromptOptions): Promise<string> {
+ let timeoutId: ReturnType<typeof setTimeout> | undefined
try {
const modelConfig = this.getModel()
@@
- const timeoutId = setTimeout(() => controller.abort(), options.timeoutMs)
+ timeoutId = setTimeout(() => controller.abort(), options.timeoutMs)
@@
} else if (options.timeoutMs !== undefined) {
if (options.timeoutMs > 0) {
signal = AbortSignal.timeout(options.timeoutMs)
+ } else {
+ const controller = new AbortController()
+ controller.abort()
+ signal = controller.signal
}
}
@@
throw enhancedError
+ } finally {
+ if (timeoutId !== undefined) {
+ clearTimeout(timeoutId)
+ }
}
}📝 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
| // Build request options with abortSignal and/or timeoutMs | |
| const sendOptions: { abortSignal?: AbortSignal } | undefined = (() => { | |
| let signal: AbortSignal | undefined = options?.abortSignal | |
| if (options?.timeoutMs !== undefined) { | |
| if (signal) { | |
| // When both are provided, create a merged signal that aborts when either fires | |
| const controller = new AbortController() | |
| if (signal.aborted) { | |
| controller.abort() | |
| } else if (options.timeoutMs > 0) { | |
| const timeoutId = setTimeout(() => controller.abort(), options.timeoutMs) | |
| signal.addEventListener( | |
| "abort", | |
| () => { | |
| clearTimeout(timeoutId) | |
| controller.abort() | |
| }, | |
| { once: true }, | |
| ) | |
| } else { | |
| // timeoutMs is 0, abort immediately | |
| controller.abort() | |
| } | |
| signal = controller.signal | |
| } else if (options.timeoutMs !== undefined) { | |
| if (options.timeoutMs > 0) { | |
| signal = AbortSignal.timeout(options.timeoutMs) | |
| } | |
| } | |
| } | |
| return signal ? { abortSignal: signal } : undefined | |
| })() | |
| const response = await this.client.send(command, sendOptions) | |
| let timeoutId: ReturnType<typeof setTimeout> | undefined | |
| // Build request options with abortSignal and/or timeoutMs | |
| const sendOptions: { abortSignal?: AbortSignal } | undefined = (() => { | |
| let signal: AbortSignal | undefined = options?.abortSignal | |
| if (options?.timeoutMs !== undefined) { | |
| if (signal) { | |
| // When both are provided, create a merged signal that aborts when either fires | |
| const controller = new AbortController() | |
| if (signal.aborted) { | |
| controller.abort() | |
| } else if (options.timeoutMs > 0) { | |
| timeoutId = setTimeout(() => controller.abort(), options.timeoutMs) | |
| signal.addEventListener( | |
| "abort", | |
| () => { | |
| clearTimeout(timeoutId) | |
| controller.abort() | |
| }, | |
| { once: true }, | |
| ) | |
| } else { | |
| // timeoutMs is 0, abort immediately | |
| controller.abort() | |
| } | |
| signal = controller.signal | |
| } else if (options.timeoutMs !== undefined) { | |
| if (options.timeoutMs > 0) { | |
| signal = AbortSignal.timeout(options.timeoutMs) | |
| } else { | |
| const controller = new AbortController() | |
| controller.abort() | |
| signal = controller.signal | |
| } | |
| } | |
| } | |
| return signal ? { abortSignal: signal } : undefined | |
| })() | |
| const response = await this.client.send(command, sendOptions) |
🤖 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 842 - 875, The request options
builder in bedrock.ts does not abort immediately when timeoutMs is 0 without a
caller-provided signal, and the timeout timer created for the merged AbortSignal
is not cleaned up after client.send finishes. Update the
sendOptions/AbortController logic to ensure timeoutMs: 0 always produces an
already-aborted signal even when options.abortSignal is absent, and refactor the
timeout path in the send() call so any setTimeout handle is cleared once the
request resolves or rejects. Use the existing sendOptions builder and
this.client.send flow as the place to fix it.
Comment on lines
+1165
to
+1168
| if (options.timeoutMs !== undefined) { | ||
| if (options.timeoutMs > 0) { | ||
| timeoutId = setTimeout(() => localAbortController?.abort(), options.timeoutMs) | ||
| } else { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear the request-local timeout in finally.
If fetch completes before timeoutMs, the timer from Line 1167 remains pending and later aborts a completed request controller.
Proposed fix
} finally {
+ if (timeoutId !== undefined) {
+ clearTimeout(timeoutId)
+ }
this.abortController = 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
| if (options.timeoutMs !== undefined) { | |
| if (options.timeoutMs > 0) { | |
| timeoutId = setTimeout(() => localAbortController?.abort(), options.timeoutMs) | |
| } else { | |
| } finally { | |
| if (timeoutId !== undefined) { | |
| clearTimeout(timeoutId) | |
| } | |
| this.abortController = 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/openai-codex.ts` around lines 1165 - 1168, The
request-local timeout in openai-codex’s fetch flow is left pending when the
request finishes early, so it can later abort an already completed controller.
Update the timeout handling around the `options.timeoutMs` / `timeoutId` logic
in `fetch` to always clear the timer in the `finally` block, and make sure the
`localAbortController` is only aborted by an active request timeout.
Contributor
|
I'm going to convert this PR to draft while we wait for a fix to #615 to land. |
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
3 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
This PR establishes the
CompletePromptOptionscontract and adds comprehensive test coverage across all 25 API providers, building on top of #701 (RequestConfigBuilder).#701 introduced the
RequestConfigBuilderfor SDK-agnostic request configuration. This PR ensures every provider correctly passes abort signals and timeouts throughcompletePrompt.Scope & Strategy
CompletePromptOptions) + Test coverage + Basic signal/timeout forwardingRequestConfigBuilderin provider implementationsThis separation ensures each PR has a single focus: verifying the contract first, then optimizing the implementation.
Changes at a Glance
complete-prompt-options.spec.ts)it()/test()casesTest Coverage Breakdown
Each provider test file includes:
Shared test file:
complete-prompt-options.spec.ts(4 tests) — validates common options behavior across all providers.Files Changed
Test Files (26 files)
Provider Implementations (25 files)
All providers updated to support
completePrompt(prompt, options)whereoptionsincludes:signal?: AbortSignal— abort signal passthroughtimeoutMs?: number— per-call timeout controlKey files:
Utilities (2 files)
Relationship to other PRs
Review Guide
Quick Checklist for Reviewers
completePromptchanges consistent across all 25 files?Focus Areas (highest impact)
What to Skip (low risk)
Test Plan
Summary by CodeRabbit
CompletePromptOptions(abortSignal,timeoutMs) and forwards them per request across supported providers.RequestConfigBuilderand re-exported it for fluent, typed request configuration (including abort-signal merging).RequestConfigBuilder, including usage patterns and signal-merging behavior.