test(providers): add completePrompt signal/timeout tests for all 25 providers#712
Open
easonLiangWorldedtech wants to merge 5 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
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
from
6957ada to
d0d5d8f
Compare
…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
d0d5d8f to
8fe2b1c
Compare
Contributor
|
I'll review more in depth once #701 lands |
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: abortsignalandtimeoutMs.RequestConfigBuilderto compose/merge abort signals and build request options.RequestConfigBuilderusage and signal-merging behavior.