feat(api): add abort signal support to all completePrompt#780
Open
easonLiangWorldedtech wants to merge 9 commits into
Open
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds ChangesCompletePromptOptions feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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: 11
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/zoo-gateway.ts (1)
279-306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnnecessary
as anycasts and inconsistenttimeoutMshandling vs. sibling providers.
optionsis already typed asCompletePromptOptions(withtimeoutMs?: number), so theas anycasts at Line 302-303 are unnecessary and bypass type checking. More importantly, the added> 0guard meanstimeoutMs: 0is silently dropped here, whereaslite-llm.tsandopencode-go.tsforwardtimeoutMsunconditionally whenever it's notundefined. This is a hidden behavioral divergence across providers implementing the sameCompletePromptOptionscontract.🔧 Suggested fix to align with sibling providers
// Build request options with abortSignal and/or timeout const createOptions: OpenAI.RequestOptions = {} if (options?.abortSignal) { createOptions.signal = options.abortSignal } - if ((options as any)?.timeoutMs !== undefined && (options as any).timeoutMs > 0) { - createOptions.timeout = (options as any).timeoutMs + if (options?.timeoutMs !== undefined) { + createOptions.timeout = options.timeoutMs }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/zoo-gateway.ts` around lines 279 - 306, The `completePrompt` implementation in `ZooGatewayProvider` is using unnecessary `as any` casts for `options.timeoutMs` and also diverges from sibling providers by skipping `timeoutMs: 0`. Update the `createOptions` building logic to use the typed `CompletePromptOptions` directly, read `timeoutMs` without casts, and forward it whenever it is not `undefined`, matching the behavior in `lite-llm.ts` and `opencode-go.ts`.
🧹 Nitpick comments (12)
src/api/providers/__tests__/gemini-handler.spec.ts (1)
58-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest title says "via httpOptions" but asserts
config.abortSignal.The test name is misleading — it doesn't verify anything about
httpOptions; it verifiesabortSignalis passed onconfig.abortSignaldirectly.✏️ Suggested title fix
- it("completePrompt should pass abort signal through to client via httpOptions", async () => { + it("completePrompt should pass abort signal through to client via config.abortSignal", async () => {🤖 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__/gemini-handler.spec.ts` around lines 58 - 79, Update the GeminiHandler test name to match what it actually asserts: it currently checks that completePrompt forwards the abort signal to client.models.generateContent via config.abortSignal, not through httpOptions. Rename the test in gemini-handler.spec.ts to reflect abortSignal propagation on config, and keep the assertion aligned with the existing GeminiHandler.completePrompt call path.src/api/providers/gemini.ts (1)
587-593: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
httpOptstyped asRecord<string, any>loses type safety.Unlike the
httpOptionsliteral used increateMessage(Line 300), this is built as a loosely-typedRecord<string, any>, which won't catch typos in property names (e.g.timeoutvstimeouts) at compile time.♻️ Suggested typed alternative
- const httpOpts: Record<string, any> = {} + const httpOpts: NonNullable<GenerateContentConfig["httpOptions"]> = {}🤖 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 587 - 593, `httpOpts` in the Gemini provider is too loosely typed as `Record<string, any>`, so property-name mistakes won’t be caught. Update the `httpOpts` construction in `createMessage`/the Gemini request setup to use a typed object shape consistent with the `httpOptions` literal pattern used elsewhere, and keep the `timeout` and `baseUrl` assignments type-checked. Use the `httpOpts` variable and the surrounding `options?.timeoutMs` / `this.options.googleGeminiBaseUrl` logic to locate and replace the weak typing.src/api/providers/__tests__/vscode-lm.spec.ts (2)
757-780: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant test with weaker assertion than the existing abort-signal test.
This duplicates "should cancel token when signal is already aborted" (Lines 651-675) but only asserts
sendRequestwas called instead of verifyingcancel()on theCancellationTokenSource, providing no additional coverage.🤖 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 757 - 780, This test in vscode-lm.spec.ts duplicates the existing “should cancel token when signal is already aborted” case without adding coverage. Either remove the redundant “should cancel token immediately when signal is already aborted” test, or update it to assert the CancellationTokenSource cancel() behavior through completePrompt and the handler’s abort handling instead of only checking mockLanguageModelChat.sendRequest. Use the existing abort-related tests around completePrompt and sendRequest as the reference point for locating the duplicated logic.
699-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTimeout/combined-signal tests only assert
sendRequestwas called, not actual cancellation behavior.Both "should handle timeoutMs by creating a timeout-based cancellation" and "should handle both signal and timeoutMs together" only check that
sendRequestwas invoked — they don't verify theCancellationTokenSourceis actually wired to the timeout, or that cancellation propagates correctly (unlike the earlierdispose()/cancel()assertions at Lines 646-649 and 673-675). As per coding guidelines, spec files should validate state transitions, not just that a call happened.As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js}should "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/vscode-lm.spec.ts` around lines 699 - 744, The timeout-related specs in completePrompt only verify that sendRequest was called, so they do not prove cancellation wiring works. Update the two tests to assert the CancellationTokenSource behavior created for timeoutMs and combined abortSignal/timeoutMs cases, mirroring the earlier dispose/cancel assertions; specifically verify the token source is created, disposed, and that cancellation propagates when the timeout or external signal is triggered. Keep the checks focused on state transitions and cancellation effects rather than only call presence.Source: Coding guidelines
src/api/providers/__tests__/bedrock.spec.ts (1)
1629-1679: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the
timeoutMs: 0+abortSignalcase.None of these tests exercise
completePrompt("...", { abortSignal, timeoutMs: 0 }), which is the combination that triggers the immediate-abort bug flagged inbedrock.ts(Lines 841-875). Also, unlike the analogous test inpoe.spec.ts("should prefer signal over timeoutMs when both are provided"), the merge test here doesn't assertsendOptions.abortSignal !== controller.signalto confirm a genuinely merged signal is produced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/bedrock.spec.ts` around lines 1629 - 1679, Add regression coverage in AwsBedrockHandler.completePrompt for the timeoutMs: 0 plus abortSignal case, since that combination currently triggers the immediate-abort path. Update the existing merge test to verify the composed sendOptions.abortSignal is not the same object as the caller’s controller.signal, and add a new test that calls completePrompt with both abortSignal and timeoutMs: 0 to confirm the request still proceeds with a merged signal. Use AwsBedrockHandler, completePrompt, and the mockSend/sendOptions assertions to locate the right test block.src/api/providers/poe.ts (1)
141-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSolid implementation — correctly handles
timeoutMs: 0and cleans up the timer infinally.Unlike the Bedrock implementation, this correctly guards the merge branch with
timeoutMs > 0(Line 151) sotimeoutMs: 0never triggers an immediate abort, and it hoiststimeoutIdto the function scope with afinallycleanup (Lines 143, 172-175), avoiding the dangling-timer issue seen elsewhere.This merge-abort-signal-with-timeout logic is now duplicated nearly verbatim across
poe.ts,bedrock.ts,vscode-lm.ts, and other providers in this PR stack, with each reimplementation subtly diverging (e.g. the Bedrock bug noted separately). Consider extracting a sharedmergeAbortSignal(abortSignal?, timeoutMs?): AbortSignal | undefinedutility so all providers share one correct, tested implementation instead of re-deriving it per file.🤖 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/poe.ts` around lines 141 - 176, The abort-signal/timeout merge logic in completePrompt is duplicated across providers and has already diverged in behavior, so extract it into a shared mergeAbortSignal helper and use it from PoeProvider.completePrompt (and the other provider implementations). Keep the current correct behavior in poe.ts, but move the controller/timeout handling into the shared utility so all callers reuse one tested implementation instead of maintaining separate copies.src/api/providers/__tests__/lite-llm.spec.ts (1)
1185-1193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSignal-forwarding assertion doesn't verify identity.
expect.objectContaining({ signal: controller.signal })insidetoHaveBeenCalledWithperforms deep/structural equality, not reference equality. Vitest's docs notetoEqual-style matching recursively compares every field of an object or element of an array, ignoring object identity. Since a freshAbortSignalhas no distinguishing own enumerable properties, this assertion could pass even if a different (but similarly unaborted) signal were forwarded instead ofcontroller.signal. Given the PR's own acceptance criteria calls fortoBe-based identity checks, consider asserting on the captured call args directly.♻️ Suggested stronger identity assertion
- await handler.completePrompt("test prompt", { abortSignal: controller.signal }) - expect(mockCreate).toHaveBeenCalledWith( - expect.objectContaining({ model: expect.any(String) }), - expect.objectContaining({ signal: controller.signal }), - ) + await handler.completePrompt("test prompt", { abortSignal: controller.signal }) + const [, callOptions] = mockCreate.mock.calls[0] + expect(callOptions.signal).toBe(controller.signal)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/lite-llm.spec.ts` around lines 1185 - 1193, The abort-signal test in handler.completePrompt is only doing structural matching, so it may pass without proving the exact AbortSignal instance was forwarded. Update the assertion in lite-llm.spec.ts to inspect the captured mockCreate call arguments directly and verify the signal by identity with the controller.signal reference, using the existing completePrompt and mockCreate test setup.src/api/providers/opencode-go.ts (1)
549-558: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent "no options" handling vs. the Anthropic branch above.
createOptions || undefinedis dead code —createOptionsis always a truthy{}object, so this branch always passes at least{}tocreate(). This differs from the Anthropic branch (Line 517) just above, which correctly passesundefinedwhen no options were supplied viaObject.keys(requestOptions).length > 0 ? requestOptions : undefined. Aligning both branches would make the abort/timeout forwarding behavior consistent within this handler.♻️ Suggested fix for consistency
const createOptions: OpenAI.RequestOptions = {} if (options?.abortSignal) { createOptions.signal = options.abortSignal } if (options?.timeoutMs !== undefined) { createOptions.timeout = options.timeoutMs } - const response = await this.client.chat.completions.create(requestOptions, createOptions || undefined) + const response = await this.client.chat.completions.create( + requestOptions, + Object.keys(createOptions).length > 0 ? createOptions : undefined, + )Note: this would require updating the corresponding "backward compatible" assertions in
opencode-go.spec.ts(Lines 662-666, 717-721) that currently expect{}instead ofundefined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/opencode-go.ts` around lines 549 - 558, The OpenAI path in opencode-go.ts always passes an empty options object because createOptions is never undefined, so the `createOptions || undefined` fallback is dead code. Update the `chat.completions.create` call in the handler to mirror the Anthropic branch’s “no options” behavior by passing undefined when no abortSignal/timeoutMs were provided, using the `createOptions` assembly around the `createOptions` variable and `this.client.chat.completions.create` call. Also adjust the related backward-compatible expectations in opencode-go.spec.ts that currently assert an empty object instead of undefined.src/api/providers/fake-ai.ts (1)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnnecessary
as anycast now thatFakeAI.completePromptmatches the signature.
FakeAI(line 36) already declarescompletePrompt(prompt: string, options?: CompletePromptOptions): Promise<string>, sothis.ai.completePrompt(prompt, options)should type-check directly. Theas anycast bypasses type safety for no remaining benefit.♻️ Suggested cleanup
completePrompt(prompt: string, options?: CompletePromptOptions): Promise<string> { - return (this.ai as any).completePrompt(prompt, options) + return this.ai.completePrompt(prompt, options) }🤖 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 83 - 85, The `FakeAI.completePrompt` implementation still uses an unnecessary `as any` cast even though `FakeAI` already matches the `completePrompt(prompt, options)` signature. Update the `completePrompt` method in `FakeAI` to call `this.ai.completePrompt(prompt, options)` directly, removing the cast so the existing type definition is enforced.src/api/providers/qwen-code.ts (1)
341-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType
fetchOptionsasOpenAI.RequestOptionsinstead ofRecord<string, unknown>+as any.Every sibling provider updated in this cohort (openrouter.ts, vercel-ai-gateway.ts, xai.ts) types this object as
OpenAI.RequestOptionsand passes it without anas anycast. Here the loose typing plus cast bypasses compile-time checking ofsignal/timeoutfield names against the SDK's actualRequestOptionsshape.♻️ Proposed fix
- const fetchOptions: Record<string, unknown> = {} + const fetchOptions: OpenAI.RequestOptions = {} if (options?.abortSignal) { fetchOptions.signal = options.abortSignal } if (options?.timeoutMs !== undefined) { fetchOptions.timeout = options.timeoutMs } const response = await this.callApiWithRetry(() => - client.chat.completions.create(requestOptions, fetchOptions as any), + client.chat.completions.create(requestOptions, fetchOptions), )🤖 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` around lines 341 - 353, The request options in qwen-code.ts are too loosely typed and the `as any` cast bypasses SDK validation. Update the `fetchOptions` object in the `callApiWithRetry`/`client.chat.completions.create` path to use `OpenAI.RequestOptions` like the sibling providers, and pass it directly without casting. Keep the existing `options.abortSignal` and `options.timeoutMs` mapping, but ensure the property names match the `RequestOptions` shape so TypeScript checks them.src/api/providers/__tests__/base-openai-compatible-provider-timeout.spec.ts (1)
121-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeaker assertion than sibling provider tests — use exact signal identity.
This test uses
expect.any(AbortSignal)for thesignaloption instead of asserting identity againstcontroller.signal. Sibling provider tests in this same cohort (anthropic, anthropic-vertex, minimax) assert the exact instance viatoBe(controller.signal)/ literal object equality, consistent with the PR's stated acceptance criteria of verifying signal identity rather than just type. As written, this test would not catch a regression where the signal is wrapped/cloned before being passed to the client.🧪 Suggested tightening
await handler.completePrompt("test prompt", { abortSignal: controller.signal, timeoutMs: 5000 }) expect(mockCreate).toHaveBeenCalledWith( expect.objectContaining({ model: "test-model" }), - expect.objectContaining({ signal: expect.any(AbortSignal), timeout: 5000 }), + expect.objectContaining({ signal: controller.signal, timeout: 5000 }), )As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js}should "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling," which for request construction should verify exact values, not just types.🤖 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__/base-openai-compatible-provider-timeout.spec.ts` around lines 121 - 134, The timeout-forwarding test in TestOpenAiCompatibleProvider is too weak because it only checks that signal is an AbortSignal type. Tighten the assertion in completePrompt’s client call to verify the exact abortSignal instance from the local AbortController (matching the sibling provider tests) while still checking timeout: 5000, so regressions that clone or wrap the signal are caught.Source: Coding guidelines
src/api/providers/openai-compatible.ts (1)
200-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage included for this merge logic in this batch.
This is the only provider in the current cohort whose
completePromptabort/timeout merge logic (branching on both/either ofabortSignal/timeoutMs, plus thefinallycleanup) has no accompanying spec update in the reviewed files. Given the branching complexity here (and the edge-case bug flagged separately), dedicated tests for each combination (signal-only, timeout-only, both, negative timeout) would help catch regressions.🤖 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 200 - 253, Add spec coverage for the abort/timeout merge logic in completePrompt on OpenAICompatibleProvider, since this is the only provider without tests for these branches. Update the relevant test file to cover the completePrompt method with cases for abortSignal only, timeoutMs only, both together, and negative/zero timeout behavior, and verify the merged signal path plus the timeout cleanup in the finally block. Use the unique symbols completePrompt, generateOptions.abortSignal, and the timeoutId cleanup path to locate the code.
🤖 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__/fireworks.spec.ts`:
- Around line 612-639: Update the fireworks provider tests in the completePrompt
cases so they assert the exact AbortSignal instance is forwarded, not just an
object containing a signal field. In the abort-signal and merged signal/timeout
tests, locate handler.completePrompt and mockCreate, then verify the second
argument’s signal with strict identity against controller.signal using the
recorded mock call instead of expect.objectContaining, while keeping the timeout
assertion unchanged.
In `@src/api/providers/__tests__/lmstudio-native-tools.spec.ts`:
- Around line 396-407: The test in completePrompt is too loose because
objectContaining on the options object can still allow a different AbortSignal
instance. Update the assertion in lmstudio-native-tools.spec.ts to inspect the
second argument passed to mockCreate and verify its signal with strict identity
against controller.signal, using the completePrompt and mockCreate call site as
the reference points.
In `@src/api/providers/__tests__/mistral.spec.ts`:
- Around line 546-554: The test name in `mistral.spec.ts` contradicts what
`handler.completePrompt` is asserting: the case with `timeoutMs: 0` should
verify that zero is forwarded, not omitted. Update the `it(...)` description to
match the existing expectation and the `completePrompt` behavior, keeping the
assertion unchanged so the test clearly reflects that `timeoutMs=0` is passed
through.
In `@src/api/providers/__tests__/openai-native.spec.ts`:
- Around line 248-313: The OpenAI native spec contains duplicate and
near-duplicate test coverage in the same describe block. Consolidate the
repeated “should work without options (backward compatible)” cases and merge the
two signal-forwarding assertions into one test each so the behavior is covered
once per scenario. Update the relevant `handler.completePrompt` and
`mockResponsesCreate` tests in `openai-native.spec.ts` to remove redundancy
while preserving both backward-compatibility and abort-signal coverage.
- Around line 315-330: The completePrompt spec in openai-native.spec.ts only
checks that an AbortSignal exists, so it does not prove timeoutMs is wired into
request construction. Strengthen the assertions around handler.completePrompt
and mockResponsesCreate to verify the signal’s behavior or identity when
timeoutMs is passed, such as confirming it aborts after the timeout or differs
from a signal built from abortSignal alone. Update both affected tests to use
meaningful expectations that would fail if completePrompt ignored timeoutMs.
In `@src/api/providers/__tests__/zai.spec.ts`:
- Around line 430-438: The completePrompt test in zai.spec.ts should assert the
abort signal by identity rather than relying on objectContaining. Update the
expectation around handler.completePrompt and mockCreate so you inspect
mockCreate.mock.calls[0][1].signal and verify it is exactly controller.signal
with toBe, ensuring the signal passed through from completePrompt is the same
AbortSignal instance.
In `@src/api/providers/bedrock.ts`:
- Around line 841-875: Fix the request-signal merge logic in the Bedrock send
path so it matches the rest of the providers: in the `sendOptions` IIFE inside
`completePrompt`, treat `timeoutMs <= 0` as “no timeout” even when
`options.abortSignal` is present, and only create/merge a timeout signal when
`timeoutMs > 0`. Also hoist the timeout handle out of the inline abort listener
and ensure it is cleared on normal completion in the `await
this.client.send(command, sendOptions)` flow, mirroring the cleanup pattern used
in `poe.ts` and `vscode-lm.ts`.
In `@src/api/providers/native-ollama.ts`:
- Around line 350-352: The completePrompt path in native-ollama.ts ignores
caller abort/timeout options, so requests cannot be cancelled or bounded. Update
completePrompt to honor CompletePromptOptions by creating a per-request
cancellation path for the Ollama chat/generate call, since the shared
client.abort() is client-scoped; use a separate client instance or equivalent
request-scoped cancellation handling tied to the options passed into
completePrompt.
In `@src/api/providers/openai-codex.ts`:
- Around line 1157-1189: In completePrompt, the abort wiring misses the case
where options.abortSignal is already aborted before the listener is attached, so
the local controller never gets canceled. Update the request-local abort setup
to check options.abortSignal.aborted immediately (or otherwise propagate from
options.abortSignal directly) before creating the merged AbortSignal.any
listener, and ensure localAbortController is aborted and timeoutId cleared in
that path as well.
In `@src/api/providers/openai-compatible.ts`:
- Around line 235-243: In the OpenAI-compatible provider’s timeout handling
block, negative timeoutMs values are currently skipped when abortSignal is not
provided, leaving generateOptions.abortSignal unset. Update the same logic that
handles options.timeoutMs so any timeoutMs less than or equal to 0 creates an
immediately aborted signal, matching the existing behavior used when both
timeoutMs and abortSignal are supplied; keep the positive case using
AbortSignal.timeout(options.timeoutMs).
In `@src/api/providers/requesty.ts`:
- Around line 219-229: The `RequestyProvider` in `requesty.ts` handles
`timeoutMs` differently from the other provider implementations by rejecting
`0`, which makes the shared `CompletePromptOptions` contract inconsistent.
Update the `createOptions` setup in the completion flow so the `timeout` field
is forwarded whenever `options.timeoutMs` is defined, not only when it is
greater than zero, matching the behavior used in the Mistral and Unbound
provider code paths.
---
Outside diff comments:
In `@src/api/providers/zoo-gateway.ts`:
- Around line 279-306: The `completePrompt` implementation in
`ZooGatewayProvider` is using unnecessary `as any` casts for `options.timeoutMs`
and also diverges from sibling providers by skipping `timeoutMs: 0`. Update the
`createOptions` building logic to use the typed `CompletePromptOptions`
directly, read `timeoutMs` without casts, and forward it whenever it is not
`undefined`, matching the behavior in `lite-llm.ts` and `opencode-go.ts`.
---
Nitpick comments:
In `@src/api/providers/__tests__/base-openai-compatible-provider-timeout.spec.ts`:
- Around line 121-134: The timeout-forwarding test in
TestOpenAiCompatibleProvider is too weak because it only checks that signal is
an AbortSignal type. Tighten the assertion in completePrompt’s client call to
verify the exact abortSignal instance from the local AbortController (matching
the sibling provider tests) while still checking timeout: 5000, so regressions
that clone or wrap the signal are caught.
In `@src/api/providers/__tests__/bedrock.spec.ts`:
- Around line 1629-1679: Add regression coverage in
AwsBedrockHandler.completePrompt for the timeoutMs: 0 plus abortSignal case,
since that combination currently triggers the immediate-abort path. Update the
existing merge test to verify the composed sendOptions.abortSignal is not the
same object as the caller’s controller.signal, and add a new test that calls
completePrompt with both abortSignal and timeoutMs: 0 to confirm the request
still proceeds with a merged signal. Use AwsBedrockHandler, completePrompt, and
the mockSend/sendOptions assertions to locate the right test block.
In `@src/api/providers/__tests__/gemini-handler.spec.ts`:
- Around line 58-79: Update the GeminiHandler test name to match what it
actually asserts: it currently checks that completePrompt forwards the abort
signal to client.models.generateContent via config.abortSignal, not through
httpOptions. Rename the test in gemini-handler.spec.ts to reflect abortSignal
propagation on config, and keep the assertion aligned with the existing
GeminiHandler.completePrompt call path.
In `@src/api/providers/__tests__/lite-llm.spec.ts`:
- Around line 1185-1193: The abort-signal test in handler.completePrompt is only
doing structural matching, so it may pass without proving the exact AbortSignal
instance was forwarded. Update the assertion in lite-llm.spec.ts to inspect the
captured mockCreate call arguments directly and verify the signal by identity
with the controller.signal reference, using the existing completePrompt and
mockCreate test setup.
In `@src/api/providers/__tests__/vscode-lm.spec.ts`:
- Around line 757-780: This test in vscode-lm.spec.ts duplicates the existing
“should cancel token when signal is already aborted” case without adding
coverage. Either remove the redundant “should cancel token immediately when
signal is already aborted” test, or update it to assert the
CancellationTokenSource cancel() behavior through completePrompt and the
handler’s abort handling instead of only checking
mockLanguageModelChat.sendRequest. Use the existing abort-related tests around
completePrompt and sendRequest as the reference point for locating the
duplicated logic.
- Around line 699-744: The timeout-related specs in completePrompt only verify
that sendRequest was called, so they do not prove cancellation wiring works.
Update the two tests to assert the CancellationTokenSource behavior created for
timeoutMs and combined abortSignal/timeoutMs cases, mirroring the earlier
dispose/cancel assertions; specifically verify the token source is created,
disposed, and that cancellation propagates when the timeout or external signal
is triggered. Keep the checks focused on state transitions and cancellation
effects rather than only call presence.
In `@src/api/providers/fake-ai.ts`:
- Around line 83-85: The `FakeAI.completePrompt` implementation still uses an
unnecessary `as any` cast even though `FakeAI` already matches the
`completePrompt(prompt, options)` signature. Update the `completePrompt` method
in `FakeAI` to call `this.ai.completePrompt(prompt, options)` directly, removing
the cast so the existing type definition is enforced.
In `@src/api/providers/gemini.ts`:
- Around line 587-593: `httpOpts` in the Gemini provider is too loosely typed as
`Record<string, any>`, so property-name mistakes won’t be caught. Update the
`httpOpts` construction in `createMessage`/the Gemini request setup to use a
typed object shape consistent with the `httpOptions` literal pattern used
elsewhere, and keep the `timeout` and `baseUrl` assignments type-checked. Use
the `httpOpts` variable and the surrounding `options?.timeoutMs` /
`this.options.googleGeminiBaseUrl` logic to locate and replace the weak typing.
In `@src/api/providers/openai-compatible.ts`:
- Around line 200-253: Add spec coverage for the abort/timeout merge logic in
completePrompt on OpenAICompatibleProvider, since this is the only provider
without tests for these branches. Update the relevant test file to cover the
completePrompt method with cases for abortSignal only, timeoutMs only, both
together, and negative/zero timeout behavior, and verify the merged signal path
plus the timeout cleanup in the finally block. Use the unique symbols
completePrompt, generateOptions.abortSignal, and the timeoutId cleanup path to
locate the code.
In `@src/api/providers/opencode-go.ts`:
- Around line 549-558: The OpenAI path in opencode-go.ts always passes an empty
options object because createOptions is never undefined, so the `createOptions
|| undefined` fallback is dead code. Update the `chat.completions.create` call
in the handler to mirror the Anthropic branch’s “no options” behavior by passing
undefined when no abortSignal/timeoutMs were provided, using the `createOptions`
assembly around the `createOptions` variable and
`this.client.chat.completions.create` call. Also adjust the related
backward-compatible expectations in opencode-go.spec.ts that currently assert an
empty object instead of undefined.
In `@src/api/providers/poe.ts`:
- Around line 141-176: The abort-signal/timeout merge logic in completePrompt is
duplicated across providers and has already diverged in behavior, so extract it
into a shared mergeAbortSignal helper and use it from PoeProvider.completePrompt
(and the other provider implementations). Keep the current correct behavior in
poe.ts, but move the controller/timeout handling into the shared utility so all
callers reuse one tested implementation instead of maintaining separate copies.
In `@src/api/providers/qwen-code.ts`:
- Around line 341-353: The request options in qwen-code.ts are too loosely typed
and the `as any` cast bypasses SDK validation. Update the `fetchOptions` object
in the `callApiWithRetry`/`client.chat.completions.create` path to use
`OpenAI.RequestOptions` like the sibling providers, and pass it directly without
casting. Keep the existing `options.abortSignal` and `options.timeoutMs`
mapping, but ensure the property names match the `RequestOptions` shape so
TypeScript checks them.
🪄 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: 3bf76c5e-965a-49a6-9547-478202ddd3e0
📒 Files selected for processing (62)
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__/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/fake-ai.tssrc/api/providers/gemini.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/minimax.tssrc/api/providers/mistral.tssrc/api/providers/native-ollama.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai-native.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/poe.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/vscode-lm.tssrc/api/providers/xai.tssrc/api/providers/zoo-gateway.tssrc/core/task/__tests__/Task.spec.tssrc/utils/__tests__/enhance-prompt.spec.tssrc/utils/single-completion-handler.ts
Comment on lines
+315
to
+330
| it("completePrompt should pass timeoutMs through to client", async () => { | ||
| mockResponsesCreate.mockResolvedValue({ | ||
| output: [ | ||
| { | ||
| type: "message", | ||
| content: [{ type: "output_text", text: "response" }], | ||
| }, | ||
| ], | ||
| }) | ||
|
|
||
| await handler.completePrompt("Test prompt", { timeoutMs: 5000 }) | ||
| expect(mockResponsesCreate).toHaveBeenCalledWith( | ||
| expect.objectContaining({ model: expect.any(String) }), | ||
| expect.objectContaining({ signal: expect.any(AbortSignal) }), | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assertions are too weak to verify timeoutMs is actually used.
Both tests only assert signal: expect.any(AbortSignal), which is true even when timeoutMs is never read by the implementation (as is currently the case — see the companion issue raised on openai-native.ts). Strengthen these to assert on signal identity/behavior (e.g., that the signal aborts after the timeout, or that it differs from a signal produced with abortSignal-only) so a regression like the current bug is actually caught. As per coding guidelines, spec files should validate request construction with meaningful assertions.
Also applies to: 332-348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/__tests__/openai-native.spec.ts` around lines 315 - 330,
The completePrompt spec in openai-native.spec.ts only checks that an AbortSignal
exists, so it does not prove timeoutMs is wired into request construction.
Strengthen the assertions around handler.completePrompt and mockResponsesCreate
to verify the signal’s behavior or identity when timeoutMs is passed, such as
confirming it aborts after the timeout or differs from a signal built from
abortSignal alone. Update both affected tests to use meaningful expectations
that would fail if completePrompt ignored timeoutMs.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/api/providers/__tests__/openai-compatible.spec.ts (1)
146-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNear-duplicate
timeoutMs: 0tests."should pass timeoutMs=0 as valid value" (146-161) and "should handle timeoutMs=0 without abortSignal" (163-178) exercise the identical setup and only differ in that the latter adds a
signalcheck. Consider consolidating into a single test with both assertions.🤖 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.spec.ts` around lines 146 - 178, The two `timeoutMs: 0` cases in `openai-compatible.spec.ts` are near-duplicates, so consolidate them into one test. Keep the shared setup around `TestOpenAiCompatibleProvider` and `mockCreate`, then assert both that `completePrompt("test prompt", { timeoutMs: 0 })` passes `timeout: 0` to `client.chat.completions.create` and that the second call argument has no `signal` set. This should replace the separate `should pass timeoutMs=0 as valid value` and `should handle timeoutMs=0 without abortSignal` tests with a single, clearer test.
🤖 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__/openai-codex.spec.ts`:
- Around line 233-297: The abort-signal merge tests in OpenAiCodexHandler are
too weak because they only check that fetchOptions.signal is defined. Strengthen
the tests in openai-codex.spec.ts by exercising the actual merged signal
behavior: after calling completePrompt with an external AbortController and/or
timeoutMs, trigger the external abort and assert the fetch request signal
becomes aborted, proving the AbortSignal.any merge and listener wiring in
OpenAiCodexHandler are working rather than just creating a local controller.
In `@src/api/providers/__tests__/openai-compatible.spec.ts`:
- Around line 50-66: The OpenAI-compatible provider tests are only matching the
signal shape, not proving the exact AbortSignal instance is forwarded. Update
the assertions in the `TestOpenAiCompatibleProvider` completePrompt tests to
capture the arguments passed to `client.chat.completions.create` and verify the
forwarded `signal` by identity with `toBe(controller.signal)`, including the
`signal + timeout` case below, so the test confirms the same AbortController
signal is passed through unchanged.
---
Nitpick comments:
In `@src/api/providers/__tests__/openai-compatible.spec.ts`:
- Around line 146-178: The two `timeoutMs: 0` cases in
`openai-compatible.spec.ts` are near-duplicates, so consolidate them into one
test. Keep the shared setup around `TestOpenAiCompatibleProvider` and
`mockCreate`, then assert both that `completePrompt("test prompt", { timeoutMs:
0 })` passes `timeout: 0` to `client.chat.completions.create` and that the
second call argument has no `signal` set. This should replace the separate
`should pass timeoutMs=0 as valid value` and `should handle timeoutMs=0 without
abortSignal` tests with a single, clearer test.
🪄 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: 239454f2-b40a-4a31-b4ef-fe2c5b6bb0ce
📒 Files selected for processing (3)
src/api/providers/__tests__/bedrock.spec.tssrc/api/providers/__tests__/openai-codex.spec.tssrc/api/providers/__tests__/openai-compatible.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/openai-codex.ts (1)
1162-1172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat non-positive
timeoutMsas “no timeout”, not “abort now”.
AwsBedrockHandler.completePrompt()in this PR only arms a timeout whentimeoutMs > 0, but this branch aborts the Codex request immediately for0or negative values. That makes the sharedCompletePromptOptionscontract provider-specific and breaks callers that use0to disable the timeout.Proposed fix
- if (options?.timeoutMs !== undefined || options?.abortSignal) { + if ((options?.timeoutMs !== undefined && options.timeoutMs > 0) || options?.abortSignal) { localAbortController = new AbortController() // Handle timeout first - if (options.timeoutMs !== undefined) { - if (options.timeoutMs > 0) { - timeoutId = setTimeout(() => localAbortController?.abort(), options.timeoutMs) - } else { - // timeoutMs is 0 or negative, abort immediately - localAbortController.abort() - } + if (options.timeoutMs !== undefined && options.timeoutMs > 0) { + timeoutId = setTimeout(() => localAbortController?.abort(), 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/openai-codex.ts` around lines 1162 - 1172, Treat non-positive timeoutMs as disabling the timeout rather than aborting immediately. In AwsBedrockHandler.completePrompt() / the timeout handling branch in openai-codex.ts, keep creating the AbortController for an explicit abortSignal, but only schedule setTimeout when timeoutMs > 0 and skip any abort action for 0 or negative values. Make sure the shared CompletePromptOptions behavior stays consistent across providers and does not call localAbortController.abort() for non-positive timeoutMs.
🤖 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 843-858: The merged abort listener in bedrock.ts’s request setup
is never removed, so shared caller signals can accumulate closures across
repeated completePrompt() calls. Update the abort-signal merge logic in the
sendOptions block to retain a reference to the listener, and in the request
cleanup/finally path remove that listener in addition to clearing
mergeTimeoutId. Use the unique symbols completePrompt, sendOptions, and
mergeTimeoutId to locate the merge-and-cleanup flow.
In `@src/api/providers/openai-codex.ts`:
- Around line 1181-1189: The per-request timeout and abort listener in
completePrompt() are not fully cleaned up after a successful request, which can
accumulate across reused task signals. Move the timer and listener cleanup into
the function’s finally path so timeoutId is always cleared and the abort handler
registered on options.abortSignal is always removed, including the related abort
wiring in the other completePrompt() flow. Use the existing
localAbortController, timeoutId, and options.abortSignal references to locate
the cleanup points.
---
Outside diff comments:
In `@src/api/providers/openai-codex.ts`:
- Around line 1162-1172: Treat non-positive timeoutMs as disabling the timeout
rather than aborting immediately. In AwsBedrockHandler.completePrompt() / the
timeout handling branch in openai-codex.ts, keep creating the AbortController
for an explicit abortSignal, but only schedule setTimeout when timeoutMs > 0 and
skip any abort action for 0 or negative values. Make sure the shared
CompletePromptOptions behavior stays consistent across providers and does not
call localAbortController.abort() for non-positive timeoutMs.
🪄 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: 98114667-4764-467b-a4b4-8be2f4d175b1
📒 Files selected for processing (11)
src/api/providers/__tests__/fireworks.spec.tssrc/api/providers/__tests__/lmstudio-native-tools.spec.tssrc/api/providers/__tests__/mistral.spec.tssrc/api/providers/__tests__/openai-codex.spec.tssrc/api/providers/__tests__/openai-compatible.spec.tssrc/api/providers/__tests__/openai-native.spec.tssrc/api/providers/__tests__/zai.spec.tssrc/api/providers/bedrock.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-compatible.tssrc/api/providers/requesty.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/api/providers/tests/zai.spec.ts
- src/api/providers/tests/lmstudio-native-tools.spec.ts
- src/api/providers/tests/mistral.spec.ts
- src/api/providers/tests/fireworks.spec.ts
- src/api/providers/openai-compatible.ts
- src/api/providers/requesty.ts
- src/api/providers/tests/openai-codex.spec.ts
- src/api/providers/tests/openai-compatible.spec.ts
github-actions
Bot
added
the
awaiting-review
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/complete-prompt-abort-pr
branch
from
71c9508 to
8d06710
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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__/native-ollama.spec.ts`:
- Around line 373-420: The abort assertions in native-ollama.spec.ts are too
global and can pass even if cancellation hits the shared client instead of the
request-local one. Update the Ollama constructor mock to return a per-instance
abort spy and keep the chat mock tied to that instance, then assert inside the
completePrompt timeout and abortSignal cases that the instance-specific abort
was called. Use the existing handler.completePrompt, mockChat, and mockAbort
setup as the entry point, but switch the expectations to the fresh client
created for each request.
- Around line 422-443: The test in native-ollama.spec.ts is only verifying the
timeout delay, so it does not prove the finally-block cleanup in completePrompt.
Update the test around handler.completePrompt and the setTimeout/clearTimeout
spies to capture the returned timer handle and assert that clearTimeout is
called with that exact handle after a successful completion. Keep the existing
timeout setup in place, but tighten the assertion so the cleanup behavior is
validated directly.
In `@src/api/providers/native-ollama.ts`:
- Around line 353-367: Use an isolated Ollama client for every cancellation path
in native-ollama.ts, not just when abortSignal is set. Update the client
selection in the request flow around fetchModel()/timeoutMs so timeout handling
also uses a per-request Ollama instance instead of this.ensureClient(). Make
sure the local client is created with the same host and auth headers as
ensureClient() so authenticated Ollama endpoints continue to work, and keep
client.abort() scoped to that request only.
- Around line 371-384: The abort handling in native-ollama’s request flow still
allows execution to continue into `client.chat()` after
`options.abortSignal.aborted` is already true. In the method that sets up the
request and registers the abort listener, throw an abort error immediately when
the signal is pre-aborted, before constructing or sending the chat request, and
keep the listener cleanup/removal in the existing `finally` block by
unregistering the `onAbort` callback there.
🪄 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: 6a02b2fe-e66c-4603-ba86-33765ad106f3
📒 Files selected for processing (6)
src/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/bedrock.tssrc/api/providers/native-ollama.tssrc/api/providers/openai-codex.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/api/providers/tests/openai-codex-native-tool-calls.spec.ts
- src/api/providers/tests/openai-native.spec.ts
- src/api/providers/bedrock.ts
- src/api/providers/openai-codex.ts
Comment on lines
+353
to
+367
| const hasAbortSignal = options?.abortSignal !== undefined | ||
| let localClient: Ollama | undefined | ||
| let timeoutId: ReturnType<typeof setTimeout> | undefined | ||
|
|
||
| try { | ||
| const client = this.ensureClient() | ||
| // Use local client if abortSignal is provided (per-request isolation) | ||
| const client = hasAbortSignal | ||
| ? (localClient ??= new Ollama({ host: this.options.ollamaBaseUrl })) | ||
| : this.ensureClient() | ||
| const { id: modelId } = await this.fetchModel() | ||
| const useR1Format = modelId.toLowerCase().includes("deepseek-r1") | ||
|
|
||
| // Handle timeoutMs if provided | ||
| if (options?.timeoutMs !== undefined && options.timeoutMs > 0) { | ||
| timeoutId = setTimeout(() => client.abort(), options.timeoutMs) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file structure first
ast-grep outline src/api/providers/native-ollama.ts --view expanded
# Read the relevant section around the reported lines
sed -n '300,430p' src/api/providers/native-ollama.ts | cat -n
# Find the shared client setup
rg -n "ensureClient|ollamaApiKey|ollamaBaseUrl|new Ollama|abortSignal|timeoutMs" src/api/providers/native-ollama.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 6698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '161,180p' src/api/providers/native-ollama.ts | cat -n
sed -n '350,380p' src/api/providers/native-ollama.ts | cat -nRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2374
Use an isolated Ollama client for all cancellation paths.
timeoutMs still goes through this.ensureClient(), so client.abort() can cancel unrelated requests sharing the provider client. The local client path also needs the same auth headers as ensureClient(), otherwise authenticated Ollama endpoints will fail.
🤖 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 353 - 367, Use an isolated
Ollama client for every cancellation path in native-ollama.ts, not just when
abortSignal is set. Update the client selection in the request flow around
fetchModel()/timeoutMs so timeout handling also uses a per-request Ollama
instance instead of this.ensureClient(). Make sure the local client is created
with the same host and auth headers as ensureClient() so authenticated Ollama
endpoints continue to work, and keep client.abort() scoped to that request only.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
…dling - bedrock.spec.ts: cover pre-aborted signal, timeoutMs=0, and empty response scenarios - openai-codex.spec.ts: add completePrompt tests for abortSignal merging and timeout scenarios - openai-compatible.spec.ts: new test file for BaseOpenAiCompatibleProvider completePrompt method
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/complete-prompt-abort-pr
branch
from
ba77fe2 to
8ab7156
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes #615 — Core plumbing for abort signal in
completePromptpath. AddsCompletePromptOptionsinterface withabortSignalandtimeoutMs, updatesSingleCompletionHandler.completePrompt()to accept optional metadata, and wires it through all 30+ provider implementations.Closes #615 (part of #404)
Depends on: #674 (already merged ✅)
Changes at a Glance
complete-prompt-options.spec.tsAcceptance Criteria (#615)
abortSignalin metadata (ApiHandlerCreateMessageMetadata)SingleCompletionHandler.completePromptaccepts optional metadataCompletePromptOptionsinterfacecurrentRequestAbortController.signalinto metadatasingle-completion-handler.tsforwards options to provideroptions?: CompletePromptOptionsparamFiles Changed
Core Interface Changes (3 files)
src/api/index.ts— NewCompletePromptOptionsinterface,completePrompt(prompt, metadata?)signaturesrc/utils/single-completion-handler.ts— Accepts and forwardsoptionsto providersrc/core/task/__tests__/Task.spec.ts— Strengthened assertions: signal identity checks instead oftoBeInstanceOf(AbortSignal), new test for fresh AbortController per requestProvider Implementations (27 files)
All providers updated in
completePrompt()path to forwardoptions.abortSignal:Direct pass-through (assign
requestOptions.signal = options.abortSignal):anthropic, anthropic-vertex, deepseek, lite-llm, lm-studio, minimax, mistral, openai, openrouter, poe, qwen-code, requesty, unbound, vercel-ai-gateway, xai, zoo-gateway
SDK-specific patterns:
AbortSignal.any([localController.signal, options.abortSignal])— merges with internal controllerAbortSignal.any([localAbortController.signal, options.abortSignal])+ local controller fallbackconfig.abortSignal(nothttpOptions.signal) for AI SDK compatibilityBase class:
requestOptions.signalTest Files (32 files)
Each provider test file includes:
New test file
complete-prompt-options.spec.tscovers the interface contract.Review Guide
Quick Checklist for Reviewers
Focus Areas (highest impact)
src/api/index.ts— New interface definition, review for completenessbedrock.ts,openai-codex.ts,openai-native.ts— Signal merge logic is most complexTask.spec.ts— Strengthened assertions fromtoBeInstanceOf(AbortSignal)to signal identity checksWhat to Skip (low risk)
Summary by CodeRabbit
CompletePromptOptions) with per-callabortSignalandtimeoutMs, enabling cancellation and timeouts across supported providers.timeoutMs = 0handling and backward compatibility when options are omitted).