feat(api): pass abortSignal to streaming API calls for all providers (#404)#434
Closed
easonLiangWorldedtech wants to merge 3 commits into
Closed
Conversation
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:
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 (20)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughWalkthroughAdds optional AbortSignal to ApiHandler metadata; Task attaches per-request signal; providers forward metadata.abortSignal into streaming and non-streaming SDK request options; many tests updated/added to assert option shapes and runtime cancellation behavior. ChangesAbort Signal Propagation
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
easonLiangWorldedtech
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/openai.ts (1)
247-250: ⚡ Quick winMain non-streaming path omits
abortSignal.The O3 non-streaming flow (Line 408) and both streaming flows forward
metadata?.abortSignal, but this main non-streaming request still passes onlypath. For consistency and to cancel non-streaming requests too, forward the signal here as well.♻️ Proposed change
- response = await this.client.chat.completions.create( - requestOptions, - this._isAzureAiInference(modelUrl) ? { path: OPENAI_AZURE_AI_INFERENCE_PATH } : {}, - ) + response = await this.client.chat.completions.create(requestOptions, { + ...(this._isAzureAiInference(modelUrl) ? { path: OPENAI_AZURE_AI_INFERENCE_PATH } : {}), + signal: metadata?.abortSignal, + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/openai.ts` around lines 247 - 250, The non-streaming call to this.client.chat.completions.create omits the abort signal; update the call site that currently passes only { path: OPENAI_AZURE_AI_INFERENCE_PATH } so it also forwards metadata?.abortSignal (or include abortSignal when not undefined) alongside path when _isAzureAiInference(modelUrl) is true, and ensure the same abortSignal is passed in the alternate (non-Azure) branch as well so requestOptions and the signal are both honored by this.client.chat.completions.create.
🤖 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.
Nitpick comments:
In `@src/api/providers/openai.ts`:
- Around line 247-250: The non-streaming call to
this.client.chat.completions.create omits the abort signal; update the call site
that currently passes only { path: OPENAI_AZURE_AI_INFERENCE_PATH } so it also
forwards metadata?.abortSignal (or include abortSignal when not undefined)
alongside path when _isAzureAiInference(modelUrl) is true, and ensure the same
abortSignal is passed in the alternate (non-Azure) branch as well so
requestOptions and the signal are both honored by
this.client.chat.completions.create.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5124a34-2d10-49c1-a063-7e83f8ab99f7
📒 Files selected for processing (18)
src/api/index.tssrc/api/providers/__tests__/openai-compatible-abort-signal.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/deepseek.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/zai.tssrc/core/task/Task.tssrc/core/task/__tests__/task-abort-signal-passing.spec.ts
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
ef0a6de to
ee47fbb
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts (3)
73-82: ⚡ Quick winRemove unused helper function and variable.
The
createMockStreamfunction (lines 73-80) andmockUsagevariable (line 82) are defined but never used in this test. The mock is set up directly on line 84-87 instead.♻️ Suggested cleanup
- function createMockStream(yieldValue: any) { - return { - fullStream: (async function* () { - yield yieldValue - })(), - usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), - } - } - - const mockUsage = Promise.resolve({ inputTokens: 10, outputTokens: 5 }) - mockStreamText.mockReturnValue({ fullStream: mockFullStream(), - usage: mockUsage, + usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts` around lines 73 - 82, Remove the unused helper and variable: delete the createMockStream function and the mockUsage constant since they are never referenced in the test; ensure any tests still rely on the explicit mock set up later (the inline mock at lines that replace createMockStream) and run tests to confirm nothing else depends on createMockStream or mockUsage.
108-117: ⚡ Quick winRemove duplicate unused helper function.
The
createMockStreamfunction (lines 108-115) is defined but never used. This is a duplicate of the unused helper from the previous test.♻️ Suggested cleanup
- function createMockStream(yieldValue: any) { - return { - fullStream: (async function* () { - yield yieldValue - })(), - usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), - } - } - - const mockUsage = Promise.resolve({ inputTokens: 10, outputTokens: 5 }) - mockStreamText.mockReturnValue({ fullStream: mockFullStream(), - usage: mockUsage, + usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts` around lines 108 - 117, The test contains a duplicate, unused helper function createMockStream (and its adjacent mockUsage Promise) that duplicates a helper from the previous test; remove the createMockStream declaration (and the unused mockUsage Promise if it is also unused) so the test file only keeps one shared helper and avoids dead code; search for createMockStream and mockUsage to ensure no usages remain before deleting.
142-151: ⚡ Quick winRemove third duplicate unused helper function.
Yet another
createMockStreamfunction (lines 142-149) that is defined but never called.♻️ Suggested cleanup
- function createMockStream(yieldValue: any) { - return { - fullStream: (async function* () { - yield yieldValue - })(), - usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), - } - } - - const mockUsage = Promise.resolve({ inputTokens: 10, outputTokens: 5 }) - mockStreamText.mockReturnValue({ fullStream: mockFullStream(), - usage: mockUsage, + usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts` around lines 142 - 151, Remove the duplicate, unused helper function createMockStream (the async generator returning fullStream and usage) from the test; leave the existing mockUsage constant in place if tests use it. Locate the duplicate createMockStream definition in the file (the one that returns { fullStream: (async function*(){ yield yieldValue })(), usage: Promise.resolve(...) }) and delete it so there is only the intended helper(s) remaining and no unused symbol shadowing.
🤖 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.
Nitpick comments:
In `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts`:
- Around line 73-82: Remove the unused helper and variable: delete the
createMockStream function and the mockUsage constant since they are never
referenced in the test; ensure any tests still rely on the explicit mock set up
later (the inline mock at lines that replace createMockStream) and run tests to
confirm nothing else depends on createMockStream or mockUsage.
- Around line 108-117: The test contains a duplicate, unused helper function
createMockStream (and its adjacent mockUsage Promise) that duplicates a helper
from the previous test; remove the createMockStream declaration (and the unused
mockUsage Promise if it is also unused) so the test file only keeps one shared
helper and avoids dead code; search for createMockStream and mockUsage to ensure
no usages remain before deleting.
- Around line 142-151: Remove the duplicate, unused helper function
createMockStream (the async generator returning fullStream and usage) from the
test; leave the existing mockUsage constant in place if tests use it. Locate the
duplicate createMockStream definition in the file (the one that returns {
fullStream: (async function*(){ yield yieldValue })(), usage:
Promise.resolve(...) }) and delete it so there is only the intended helper(s)
remaining and no unused symbol shadowing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efeee00b-056f-4dc5-aadf-f93a7464e61f
📒 Files selected for processing (24)
packages/types/src/tool-params.tssrc/api/index.tssrc/api/providers/__tests__/fireworks.spec.tssrc/api/providers/__tests__/openai-compatible-abort-signal.spec.tssrc/api/providers/__tests__/sambanova.spec.tssrc/api/providers/__tests__/zai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/deepseek.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/zai.tssrc/core/task/Task.tssrc/core/task/__tests__/task-abort-signal-passing.spec.tssrc/core/tools/ReadFileTool.tssrc/core/tools/__tests__/readFileTool.spec.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/api/index.ts
- src/api/providers/zai.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/tests/sambanova.spec.ts
- src/api/providers/opencode-go.ts
- src/api/providers/unbound.ts
- src/api/providers/lite-llm.ts
- src/api/providers/lm-studio.ts
- src/api/providers/mimo.ts
- src/api/providers/tests/fireworks.spec.ts
- src/api/providers/tests/zai.spec.ts
- src/api/providers/deepseek.ts
- src/core/task/Task.ts
- src/api/providers/openrouter.ts
- src/api/providers/qwen-code.ts
- src/core/task/tests/task-abort-signal-passing.spec.ts
- src/api/providers/openai.ts
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
4e0b0e7 to
71da495
Compare
easonLiangWorldedtech
pushed a commit
to easonLiangWorldedtech/Zoo-Code
that referenced
this pull request
Jun 9, 2026
…oo-Code-Org#434) - Add { signal } parameter to chat.completions.create() in all provider handlers - Update test assertions to expect.any(Object) for the second parameter - Add new tests for abort signal passing (abort-signal.spec.ts, task-abort-signal-passing.spec.ts) - Fix fireworks.spec.ts streaming test assertions 27 files changed, 122 insertions(+), 54 deletions(-)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/api/providers/__tests__/unbound.spec.ts (1)
91-91: 💤 Low valueConsider a more specific assertion for request options.
The test now uses
expect.any(Object)to match the second argument (request options), which is loose. While dedicated abort signal tests exist elsewhere, this test could be more precise about the expected shape.✨ Optional: More specific assertion
- expect.any(Object), + expect.objectContaining({ signal: expect.anything() }),This verifies the options object has a
signalproperty without coupling the test to specific abort signal values.🤖 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__/unbound.spec.ts` at line 91, Replace the loose expect.any(Object) used for the request options in the test with a more specific matcher that asserts the presence of an abort signal; for example change the second-argument matcher to expect.objectContaining({ signal: expect.any(Object) }) (or expect.anything() if preferred) so the test verifies the options object includes a signal property while leaving other fields flexible — update the assertion that currently uses expect.any(Object) in the unbound.spec.ts test to use expect.objectContaining({ signal: expect.any(Object) }).Source: Coding guidelines
src/api/providers/__tests__/vercel-ai-gateway.spec.ts (1)
209-209: 💤 Low valueConsider more specific assertions for request options.
Multiple test assertions now use
expect.any(Object)to match the second argument (request options containing the abort signal). While dedicated abort signal tests exist elsewhere, these assertions could be more precise about the expected shape. As per coding guidelines, unit tests should verify request construction details.✨ Optional: More specific assertion pattern
Replace instances of:
- expect.any(Object), + expect.objectContaining({ signal: expect.anything() }),This verifies the options object has a
signalproperty without coupling tests to specific abort signal values, improving request construction verification while keeping tests focused on their primary assertions (temperature, tools, etc.).Also applies to: 225-225, 256-256, 335-335, 353-353, 371-371, 389-389, 489-489
🤖 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__/vercel-ai-gateway.spec.ts` at line 209, Tests use broad expect.any(Object) for the request options; update each assertion (the mocked fetch/HTTP request assertions in vercelAiGateway tests such as those around temperature/tools checks) to assert the options contain an abort signal instead of matching any object — replace expect.any(Object) with a more specific matcher like expect.objectContaining({ signal: expect.any(Object) }) (or expect.objectContaining({ signal: expect.any(AbortSignal) }) if the runtime provides AbortSignal) so the tests verify the presence/shape of the signal without coupling to its concrete value; apply this change to the multiple occurrences noted in the spec.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/api/providers/__tests__/unbound.spec.ts`:
- Line 91: Replace the loose expect.any(Object) used for the request options in
the test with a more specific matcher that asserts the presence of an abort
signal; for example change the second-argument matcher to
expect.objectContaining({ signal: expect.any(Object) }) (or expect.anything() if
preferred) so the test verifies the options object includes a signal property
while leaving other fields flexible — update the assertion that currently uses
expect.any(Object) in the unbound.spec.ts test to use expect.objectContaining({
signal: expect.any(Object) }).
In `@src/api/providers/__tests__/vercel-ai-gateway.spec.ts`:
- Line 209: Tests use broad expect.any(Object) for the request options; update
each assertion (the mocked fetch/HTTP request assertions in vercelAiGateway
tests such as those around temperature/tools checks) to assert the options
contain an abort signal instead of matching any object — replace
expect.any(Object) with a more specific matcher like expect.objectContaining({
signal: expect.any(Object) }) (or expect.objectContaining({ signal:
expect.any(AbortSignal) }) if the runtime provides AbortSignal) so the tests
verify the presence/shape of the signal without coupling to its concrete value;
apply this change to the multiple occurrences noted in the spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19e09453-6e0e-433d-bec0-4ce04430def6
📒 Files selected for processing (29)
src/api/index.tssrc/api/providers/__tests__/base-openai-compatible-provider.spec.tssrc/api/providers/__tests__/fireworks.spec.tssrc/api/providers/__tests__/lmstudio-native-tools.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/openai-compatible-abort-signal.spec.tssrc/api/providers/__tests__/opencode-go.spec.tssrc/api/providers/__tests__/qwen-code-native-tools.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__/zai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/deepseek.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/zai.tssrc/core/task/Task.tssrc/core/task/__tests__/task-abort-signal-passing.spec.ts
✅ Files skipped from review due to trivial changes (4)
- src/api/providers/tests/lmstudio-native-tools.spec.ts
- src/api/providers/tests/base-openai-compatible-provider.spec.ts
- src/api/providers/tests/qwen-code-native-tools.spec.ts
- src/api/providers/tests/requesty.spec.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/api/providers/tests/sambanova.spec.ts
- src/api/providers/openrouter.ts
- src/api/providers/mimo.ts
- src/api/providers/lm-studio.ts
- src/api/providers/opencode-go.ts
- src/core/task/Task.ts
- src/api/index.ts
- src/api/providers/openai-compatible.ts
- src/api/providers/qwen-code.ts
- src/api/providers/tests/openai-compatible-abort-signal.spec.ts
- src/api/providers/lite-llm.ts
- src/core/task/tests/task-abort-signal-passing.spec.ts
- src/api/providers/tests/fireworks.spec.ts
- src/api/providers/openai.ts
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
71da495 to
c52904d
Compare
easonLiangWorldedtech
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
easonLiangWorldedtech
pushed a commit
to easonLiangWorldedtech/Zoo-Code
that referenced
this pull request
Jun 9, 2026
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
2 times, most recently
from
086f8e1 to
0a86942
Compare
edelauna
reviewed
Jun 10, 2026
edelauna
left a comment
edelauna
left a comment
Contributor
There was a problem hiding this comment.
looks like as part of a merge conflict/resolution you're removing some code from main, can you re-review to ensure the diff is minimized to only the files related to your change.
Contributor
There was a problem hiding this comment.
lets not remove the diagnostic changes please.
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
4 times, most recently
from
79dd3fb to
c5a6f53
Compare
easonLiangWorldedtech
pushed a commit
to easonLiangWorldedtech/Zoo-Code
that referenced
this pull request
Jun 13, 2026
…ode-Org#434) - Add AbortController support to openai-compatible provider - Pass abort signal through Task execution chain - Add comprehensive tests for abort signal behavior across all providers
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
c5a6f53 to
b5b554d
Compare
easonLiangWorldedtech
pushed a commit
to easonLiangWorldedtech/Zoo-Code
that referenced
this pull request
Jun 13, 2026
…ode-Org#434) - Add AbortController support to openai-compatible provider - Pass abort signal through Task execution chain - Add comprehensive tests for abort signal behavior across all providers
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
b5b554d to
fab4533
Compare
easonLiangWorldedtech
pushed a commit
to easonLiangWorldedtech/Zoo-Code
that referenced
this pull request
Jun 13, 2026
…ode-Org#434) - Add AbortController support to openai-compatible provider - Pass abort signal through Task execution chain - Add comprehensive tests for abort signal behavior across all providers
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
67b226c to
9e3130f
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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/gemini.ts`:
- Around line 346-350: The non-stream Gemini call is passing
metadata?.abortSignal as a top-level signal which `@google/genai` ignores; update
the non-stream call (completePrompt / models.generateContent usage) to pass the
abort signal inside config by using config: { ...config, abortSignal:
metadata?.abortSignal } instead of signal: metadata?.abortSignal so cancellation
works the same as the streaming path (models.generateContentStream).
🪄 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: b666a8fe-43e0-4dfd-9e2d-6ae6fcb29a8a
📒 Files selected for processing (14)
src/api/providers/__tests__/deepseek.spec.tssrc/api/providers/__tests__/vercel-ai-gateway.spec.tssrc/api/providers/bedrock.tssrc/api/providers/gemini.tssrc/api/providers/native-ollama.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-native.tssrc/api/providers/openai.tssrc/api/providers/qwen-code.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 as they are similar to previous changes (11)
- src/api/providers/tests/deepseek.spec.ts
- src/api/providers/openai-native.ts
- src/api/providers/openai-codex.ts
- src/api/providers/xai.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/zoo-gateway.ts
- src/api/providers/bedrock.ts
- src/api/providers/openai.ts
- src/api/providers/native-ollama.ts
- src/api/providers/tests/vercel-ai-gateway.spec.ts
- src/api/providers/vscode-lm.ts
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
2 times, most recently
from
a140cd5 to
b3e4008
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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/__tests__/native-ollama.spec.ts (1)
642-672:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbort-bridge behavior is not actually verified in these abortSignal tests.
Both tests only assert that
chatis called without asignaloption. That checks call-shape, but not the contract implied by Line 642 title:controller.abort()should invoke the per-request client'sabort()method. This leaves the key cancellation path untested.Suggested test hardening
const mockedData = vi.hoisted(() => ({ mockChat: vi.fn(), mockGetOllamaModels: vi.fn(), + mockAbort: vi.fn(), })) vi.mock("ollama", () => { return { Ollama: vi.fn().mockImplementation(() => ({ chat: mockedData.mockChat, + abort: mockedData.mockAbort, })), Message: vi.fn(), } })it("should wire abortSignal to per-request client's abort() method", async () => { + mockedData.mockAbort.mockClear() ... const controller = new AbortController() - for await (const _chunk of handlerWithSignal.createMessage( + const stream = handlerWithSignal.createMessage( "system", [{ role: "user", content: "Hello!" }], { taskId: "test", abortSignal: controller.signal }, - )) { + ) + controller.abort() + for await (const _chunk of stream) { break } - - expect(mockedData.mockChat).toHaveBeenCalled() - const callArgs = mockedData.mockChat.mock.calls[0][0] - expect(callArgs.signal).toBeUndefined() + expect(mockedData.mockAbort).toHaveBeenCalled() })Also applies to: 674-697
🤖 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__/native-ollama.spec.ts` around lines 642 - 672, The test only checks that chat was called without a signal but doesn't verify the abort bridge; update the "should wire abortSignal to per-request client's abort() method" test (and the similar test at the other site) so mockedData.mockChat's implementation returns/uses a per-request client object with an abort spy (or capture the client created inside the mock), start the async iteration from NativeOllamaHandler.createMessage, then call controller.abort(), await any iteration cleanup, and assert that the captured per-request client's abort spy was invoked (i.e., controller.abort() triggers the client's abort()); keep the existing assertion that callArgs.signal is undefined.
🤖 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/__tests__/native-ollama.spec.ts`:
- Around line 642-672: The test only checks that chat was called without a
signal but doesn't verify the abort bridge; update the "should wire abortSignal
to per-request client's abort() method" test (and the similar test at the other
site) so mockedData.mockChat's implementation returns/uses a per-request client
object with an abort spy (or capture the client created inside the mock), start
the async iteration from NativeOllamaHandler.createMessage, then call
controller.abort(), await any iteration cleanup, and assert that the captured
per-request client's abort spy was invoked (i.e., controller.abort() triggers
the client's abort()); keep the existing assertion that callArgs.signal is
undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60472a9b-aea0-449a-8bf4-fe633c3d43ce
📒 Files selected for processing (17)
src/api/providers/__tests__/deepseek.spec.tssrc/api/providers/__tests__/gemini.spec.tssrc/api/providers/__tests__/native-ollama.spec.tssrc/api/providers/__tests__/vercel-ai-gateway.spec.tssrc/api/providers/__tests__/zoo-gateway.spec.tssrc/api/providers/bedrock.tssrc/api/providers/gemini.tssrc/api/providers/native-ollama.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-native.tssrc/api/providers/openai.tssrc/api/providers/qwen-code.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/tests/zoo-gateway.spec.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/api/providers/unbound.ts
- src/api/providers/tests/deepseek.spec.ts
- src/api/providers/openai-native.ts
- src/api/providers/tests/gemini.spec.ts
- src/api/providers/openai-codex.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/gemini.ts
- src/api/providers/bedrock.ts
- src/api/providers/zoo-gateway.ts
- src/api/providers/xai.ts
- src/api/providers/openai.ts
- src/api/providers/qwen-code.ts
- src/api/providers/vscode-lm.ts
- src/api/providers/native-ollama.ts
- src/api/providers/tests/vercel-ai-gateway.spec.ts
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
3 times, most recently
from
e8aab5f to
963e058
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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/__tests__/native-ollama.spec.ts (1)
241-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
completePromptabort test does not verifyabort()bridging despite the test intent.At Line 249, you pass
abortSignal, but the test never triggers abort or checkscapturedAbortSpies; it only validatescallArgs.signalis undefined. A brokenrequestClient.abort()path would still pass.Suggested fix
it("should wire abortSignal to per-request client's abort() method", async () => { + mockedData.capturedAbortSpies.length = 0 const controller = new AbortController() @@ await handler.completePrompt("Test prompt", { taskId: "test", abortSignal: mockAbortSignal }) @@ const callArgs = mockedData.mockChat.mock.calls[0][0] expect(callArgs.signal).toBeUndefined() + + const abortSpy = mockedData.capturedAbortSpies[mockedData.capturedAbortSpies.length - 1] + expect(abortSpy).toBeDefined() + expect(abortSpy!).toHaveBeenCalledTimes(0) + + controller.abort() + await new Promise((r) => setTimeout(r, 0)) + expect(abortSpy!).toHaveBeenCalledTimes(1) })🤖 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__/native-ollama.spec.ts` around lines 241 - 254, The test for completePrompt should actually trigger the abort and assert that the per-request client's abort() bridging is invoked: create an AbortController (controller), spy or capture the per-request client's abort method via capturedAbortSpies (or a jest.spyOn on requestClient.abort) before calling handler.completePrompt("Test prompt", { taskId: "test", abortSignal: controller.signal }), then call controller.abort() (or controller.abort(new Error(...))) and await any promise resolution, and finally expect the captured abort spy to have been called (e.g., expect(capturedAbortSpies[0]).toHaveBeenCalled()) in addition to the existing expectation that callArgs.signal is undefined; ensure the abort is triggered after invoking completePrompt so the bridge path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/providers/__tests__/bedrock.spec.ts`:
- Around line 1585-1606: The tests for AwsBedrockHandler.createMessage currently
only iterate the generator and never assert cancellation propagation; update
both tests (the pre-aborted case and the mid-stream abort case) to explicitly
assert that the request/abort signal was triggered: for the pre-aborted test,
assert controller.signal.aborted is true immediately after consuming the
generator or assert that the generator iteration throws an
AbortError/DOMException; for the mid-stream-abort test, set up a mock that
records whether the internal request was aborted (or assert the thrown
AbortError) and assert that the internal controller.signal (or the passed
controller.signal) becomes aborted after you call controller.abort(); reference
AwsBedrockHandler.createMessage and the test generator variable to locate where
to add these assertions.
In `@src/api/providers/__tests__/openai-native.spec.ts`:
- Around line 1850-1891: The tests for OpenAiNativeHandler fallback currently
only assert fetchOptions.signal is defined; update both affected test cases to
assert signal propagation and abortion semantics: in the "should reuse
existingSignal when SDK fails and fallback to fetch" test (and the similar
SDK→fallback test), check that fetchOptions.signal === controller.signal (i.e.,
the same AbortSignal instance is passed) and add an assertion that after
controller.abort() the signal's aborted property is true (or await a
fetch/stream result that observes the abort) to prove the fallback signal flips
to aborted; locate the tests by the test name strings and update the assertions
accordingly.
---
Outside diff comments:
In `@src/api/providers/__tests__/native-ollama.spec.ts`:
- Around line 241-254: The test for completePrompt should actually trigger the
abort and assert that the per-request client's abort() bridging is invoked:
create an AbortController (controller), spy or capture the per-request client's
abort method via capturedAbortSpies (or a jest.spyOn on requestClient.abort)
before calling handler.completePrompt("Test prompt", { taskId: "test",
abortSignal: controller.signal }), then call controller.abort() (or
controller.abort(new Error(...))) and await any promise resolution, and finally
expect the captured abort spy to have been called (e.g.,
expect(capturedAbortSpies[0]).toHaveBeenCalled()) in addition to the existing
expectation that callArgs.signal is undefined; ensure the abort is triggered
after invoking completePrompt so the bridge path is exercised.
🪄 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: 8248e33f-6c3c-4e16-b131-5b114f1f9a72
📒 Files selected for processing (20)
src/api/providers/__tests__/bedrock.spec.tssrc/api/providers/__tests__/deepseek.spec.tssrc/api/providers/__tests__/gemini.spec.tssrc/api/providers/__tests__/native-ollama.spec.tssrc/api/providers/__tests__/openai-native.spec.tssrc/api/providers/__tests__/vercel-ai-gateway.spec.tssrc/api/providers/__tests__/vscode-lm.spec.tssrc/api/providers/__tests__/zoo-gateway.spec.tssrc/api/providers/bedrock.tssrc/api/providers/gemini.tssrc/api/providers/native-ollama.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-native.tssrc/api/providers/openai.tssrc/api/providers/qwen-code.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 as they are similar to previous changes (16)
- src/api/providers/unbound.ts
- src/api/providers/openai-native.ts
- src/api/providers/openai-codex.ts
- src/api/providers/tests/deepseek.spec.ts
- src/api/providers/tests/zoo-gateway.spec.ts
- src/api/providers/tests/gemini.spec.ts
- src/api/providers/gemini.ts
- src/api/providers/qwen-code.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/xai.ts
- src/api/providers/bedrock.ts
- src/api/providers/vscode-lm.ts
- src/api/providers/openai.ts
- src/api/providers/tests/vercel-ai-gateway.spec.ts
- src/api/providers/zoo-gateway.ts
- src/api/providers/native-ollama.ts
Comment on lines
+1585
to
+1606
| it("should handle pre-aborted signals by calling controller.abort() immediately", async () => { | ||
| const handler = new AwsBedrockHandler({ | ||
| apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", | ||
| awsAccessKey: "test-access-key", | ||
| awsSecretKey: "test-secret-key", | ||
| awsRegion: "us-east-1", | ||
| }) | ||
|
|
||
| const controller = new AbortController() | ||
| controller.abort() // Pre-abort the signal | ||
|
|
||
| const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Hello" }] | ||
| const generator = handler.createMessage("System prompt", messages, { | ||
| taskId: "test-task", | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| // Consume the stream - pre-aborted signal should trigger internal abort | ||
| for await (const _ of generator) { | ||
| // consume | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Abort tests currently pass without proving cancellation propagation.
At Line 1603 and Line 1627, both tests only consume/suppress iteration; they never assert that the request signal became aborted. With the current mock stream shape, these can pass even if abort wiring breaks.
Suggested assertion tightening
it("should handle pre-aborted signals by calling controller.abort() immediately", async () => {
@@
- // Consume the stream - pre-aborted signal should trigger internal abort
- for await (const _ of generator) {
- // consume
- }
+ await generator.next()
+
+ const clientInstance =
+ vi.mocked(BedrockRuntimeClient).mock.results[vi.mocked(BedrockRuntimeClient).mock.results.length - 1]?.value
+ const mockSendFn = clientInstance?.send as ReturnType<typeof vi.fn>
+ const sendOptions = mockSendFn.mock.calls[0][1]
+ expect(sendOptions.abortSignal).toBeDefined()
+ expect(sendOptions.abortSignal.aborted).toBe(true)
})
@@
it("should use { once: true } listener for external abort signal", async () => {
@@
- controller.abort()
+ const clientInstance =
+ vi.mocked(BedrockRuntimeClient).mock.results[vi.mocked(BedrockRuntimeClient).mock.results.length - 1]?.value
+ const mockSendFn = clientInstance?.send as ReturnType<typeof vi.fn>
+ const sendOptions = mockSendFn.mock.calls[0][1]
+ expect(sendOptions.abortSignal.aborted).toBe(false)
+
+ controller.abort()
+ expect(sendOptions.abortSignal.aborted).toBe(true)
@@
- await consumePromise.catch(() => {})
+ await consumePromise.catch(() => {})
})Also applies to: 1608-1639
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/__tests__/bedrock.spec.ts` around lines 1585 - 1606, The
tests for AwsBedrockHandler.createMessage currently only iterate the generator
and never assert cancellation propagation; update both tests (the pre-aborted
case and the mid-stream abort case) to explicitly assert that the request/abort
signal was triggered: for the pre-aborted test, assert controller.signal.aborted
is true immediately after consuming the generator or assert that the generator
iteration throws an AbortError/DOMException; for the mid-stream-abort test, set
up a mock that records whether the internal request was aborted (or assert the
thrown AbortError) and assert that the internal controller.signal (or the passed
controller.signal) becomes aborted after you call controller.abort(); reference
AwsBedrockHandler.createMessage and the test generator variable to locate where
to add these assertions.
Fix 6 major issues and 6 nitpick inconsistencies from CodeRabbit review:
Major fixes (Stop functionality broken for some providers):
- openai-codex.ts: Pass full metadata in retry loop, not just taskId
- openai-native.ts: Reuse existing controller signal in fallback path
- vscode-lm.ts: Bridge external abortSignal to VSCode CancellationToken
- bedrock.ts: Handle pre-aborted signals + use {once: true} listener
- gemini.ts: Move abortSignal to config.abortSignal for both streaming and non-streaming calls
- native-ollama.ts: Use per-request client with abort() method
Nitpick fixes (consistent signal forwarding pattern):
- openai.ts, unbound.ts, qwen-code.ts, xai.ts, vercel-ai-gateway.ts, zoo-gateway.ts:
Normalize all completePrompt methods to use {signal: metadata?.abortSignal}
Test fixes (improve coverage for abort signal paths):
- bedrock.spec.ts: Assert controller.signal.aborted state for pre-aborted and mid-stream tests
- openai-native.spec.ts: Capture fetchOptions before abort, verify external signal becomes aborted
- native-ollama.spec.ts: Use spyCountBefore to track per-request client abort spy correctly
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
963e058 to
ad82a44
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jun 14, 2026
Contributor
Closing in favor of smaller, focused PRsThanks for the work here — the intent is right and most of the provider-level wiring is solid. However, at 59 files this is too large to review effectively, and the review surfaced several bugs that would have been obvious in smaller PRs (missing We've broken this into 4 sub-issues on #404, ordered by dependency:
#615 must land first. #616, #617, #618 can go in parallel after that. Each issue includes the specific review findings and acceptance criteria from this PR's review. The code from this branch can be cherry-picked into each — most of the work is already done, it just needs to be split and the bugs fixed. |
This was referenced Jun 14, 2026
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
Fixes #404 — When the user clicks Stop during streaming, the HTTP request to the provider continues running because the abort signal wasn't being passed through. This wastes API tokens/compute on the provider side.
Changes
Commit 1:
feat(api): add abort signal support for API providers and task{ signal: metadata?.abortSignal }to streaming.chat.completions.create()calls in 16 providers:abortSignal?: AbortSignaltoApiHandlerCreateMessageMetadatainterfaceCommit 2:
feat(api): integrate external abort signal with internal controllersmetadataparameter withabortSignaltocompletePromptmethods across 22 provider implementations:completePromptdoes not support non-streaming abort (Mistral SDK limitation)Test Coverage
task-abort-signal-passing.spec.ts— Comprehensive tests for abort signal flow through Task execution chainopenai-compatible-abort-signal.spec.ts— Provider-level abort signal verification for OpenAI Compatible handlerArchitecture
test video ref(not latest)
2026-06-01.230625.mp4
Summary by CodeRabbit
New Features
Tests