feat(api): add CompletePromptOptions parameter to completePrompt method#901
Open
easonLiangWorldedtech wants to merge 3 commits into
Open
Conversation
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
📝 WalkthroughWalkthrough
ChangesComplete prompt options
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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: 1
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/base-openai-compatible-provider.ts (1)
215-229: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThread
CompletePromptOptionsinto these completion requests.
abortSignalandtimeoutMsare defined onCompletePromptOptions, but everycompletePrompthere dropsoptions, so caller cancellation and per-call timeouts never reach the SDK request.
src/api/providers/base-openai-compatible-provider.ts#L215-L229src/api/providers/opencode-go.ts#L488-L537src/api/providers/openrouter.ts#L577-L606src/api/providers/poe.ts#L141-L147src/api/providers/qwen-code.ts#L330-L341src/api/providers/requesty.ts#L207-L221src/api/providers/unbound.ts#L195-L209src/api/providers/vercel-ai-gateway.ts#L120-L136🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/base-openai-compatible-provider.ts` around lines 215 - 229, Thread CompletePromptOptions through each completePrompt implementation so abortSignal and timeoutMs reach the underlying SDK completion request. Update the request construction or invocation in src/api/providers/base-openai-compatible-provider.ts lines 215-229, src/api/providers/opencode-go.ts lines 488-537, src/api/providers/openrouter.ts lines 577-606, src/api/providers/poe.ts lines 141-147, src/api/providers/qwen-code.ts lines 330-341, src/api/providers/requesty.ts lines 207-221, src/api/providers/unbound.ts lines 195-209, and src/api/providers/vercel-ai-gateway.ts lines 120-136, reusing each provider’s existing request options and preserving current completion behavior.
🧹 Nitpick comments (1)
src/core/task/__tests__/Task.spec.ts (1)
2235-2252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the mock stream factory.
The custom mock implementation manually overrides the async iterator protocol, but
for awaitloops rely exclusively on the[Symbol.asyncIterator]()method. Since that method returns a native generator, your customnext(),return(), andthrow()methods are bypassed as dead code, andcallCountis never actually incremented here.You can safely simplify this to a standard async generator function, which naturally fulfills the
AsyncGeneratorcontract and correctly increments your counter as intended:♻️ Proposed refactor
- const mockStreamFactory = () => { - return { - async *[Symbol.asyncIterator]() { - yield { type: "text", text: `response ${callCount}` } - }, - async next() { - callCount++ - return { done: true, value: { type: "text", text: `response ${callCount - 1}` } } - }, - async return() { - return { done: true, value: undefined } - }, - async throw(e: any) { - throw e - }, - [Symbol.asyncDispose]: async () => {}, - } as AsyncGenerator<ApiStreamChunk> - } + const mockStreamFactory = async function* (): AsyncGenerator<ApiStreamChunk> { + yield { type: "text", text: `response ${callCount}` } + callCount++ + }🤖 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/core/task/__tests__/Task.spec.ts` around lines 2235 - 2252, Replace the custom object returned by mockStreamFactory with a standard async generator function. Move the callCount increment into the generator execution before yielding the response, remove the manually defined next, return, throw, and async iterator members, and preserve the existing response text and AsyncGenerator<ApiStreamChunk> contract.
🤖 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`:
- Line 579: All completePrompt implementations accept CompletePromptOptions but
do not propagate cancellation or timeout settings to their requests. Update
src/api/providers/gemini.ts:579 in completePrompt to pass abortSignal to
generateContent; src/api/providers/minimax.ts:292-295 in completePrompt to pass
the signal to client.messages.create; src/api/providers/mistral.ts:195 in
completePrompt to pass abortSignal and timeoutMs to client.chat.complete;
src/api/providers/native-ollama.ts:553-579 in completePrompt to pass abortSignal
to client.chat; src/api/providers/openai-codex.ts:1157 in completePrompt to use
options?.abortSignal for fetch, preserving the existing controller as fallback;
src/api/providers/openai-compatible.ts:200 in completePrompt to pass abortSignal
to generateText; and src/api/providers/openai-native.ts:1487-1585 in
completePrompt to use options?.abortSignal for responses.create instead of
relying solely on the shared controller.
---
Outside diff comments:
In `@src/api/providers/base-openai-compatible-provider.ts`:
- Around line 215-229: Thread CompletePromptOptions through each completePrompt
implementation so abortSignal and timeoutMs reach the underlying SDK completion
request. Update the request construction or invocation in
src/api/providers/base-openai-compatible-provider.ts lines 215-229,
src/api/providers/opencode-go.ts lines 488-537, src/api/providers/openrouter.ts
lines 577-606, src/api/providers/poe.ts lines 141-147,
src/api/providers/qwen-code.ts lines 330-341, src/api/providers/requesty.ts
lines 207-221, src/api/providers/unbound.ts lines 195-209, and
src/api/providers/vercel-ai-gateway.ts lines 120-136, reusing each provider’s
existing request options and preserving current completion behavior.
---
Nitpick comments:
In `@src/core/task/__tests__/Task.spec.ts`:
- Around line 2235-2252: Replace the custom object returned by mockStreamFactory
with a standard async generator function. Move the callCount increment into the
generator execution before yielding the response, remove the manually defined
next, return, throw, and async iterator members, and preserve the existing
response text and AsyncGenerator<ApiStreamChunk> contract.
🪄 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: 098310bc-c298-461e-b612-f08806baff20
📒 Files selected for processing (30)
src/api/index.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/kenari.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
github-actions
Bot
added
the
awaiting-review
Add optional second parameter to all provider completePrompt methods for future abort signal and timeout support. - Add CompletePromptOptions interface with abortSignal and timeoutMs fields - Update 25+ provider implementations to accept options?: CompletePromptOptions parameter - Simplify providers to use basic request calls without signal/timeout logic
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-core/complete-prompt
branch
from
37133de to
62ed671
Compare
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/base-openai-compatible-provider.ts (1)
215-243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnused
optionsparameter incompletePromptimplementations.Although the
optionsparameter (containingabortSignalandtimeoutMs) is now included in thecompletePromptsignature across all providers, it is never forwarded to the underlying SDK clients (e.g., OpenAI or Anthropic). As a result, network requests will not actually be aborted when the cancellation signal is triggered, defeating the objective of stopping requests after cancellation.
src/api/providers/base-openai-compatible-provider.ts#L215-L243: Pass the options to the OpenAI client call:await this.client.chat.completions.create(params, { signal: options?.abortSignal, timeout: options?.timeoutMs }).src/api/providers/opencode-go.ts#L488-L545: Pass the options to both the Anthropic and OpenAI client calls.src/api/providers/requesty.ts#L207-L226: Pass the options to the OpenAI client call.src/api/providers/vercel-ai-gateway.ts#L120-L144: Pass the options to the OpenAI client call.src/api/providers/xai.ts#L145-L163: Pass the options to the Responses API client call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/base-openai-compatible-provider.ts` around lines 215 - 243, Forward CompletePromptOptions.abortSignal and timeoutMs to every underlying SDK request: update completePrompt in src/api/providers/base-openai-compatible-provider.ts (215-243), both Anthropic and OpenAI calls in src/api/providers/opencode-go.ts (488-545), the OpenAI call in src/api/providers/requesty.ts (207-226), the OpenAI call in src/api/providers/vercel-ai-gateway.ts (120-144), and the Responses API call in src/api/providers/xai.ts (145-163). Use each client’s request options so cancellation and timeouts are applied.
🤖 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/base-openai-compatible-provider.ts`:
- Around line 215-243: Forward CompletePromptOptions.abortSignal and timeoutMs
to every underlying SDK request: update completePrompt in
src/api/providers/base-openai-compatible-provider.ts (215-243), both Anthropic
and OpenAI calls in src/api/providers/opencode-go.ts (488-545), the OpenAI call
in src/api/providers/requesty.ts (207-226), the OpenAI call in
src/api/providers/vercel-ai-gateway.ts (120-144), and the Responses API call in
src/api/providers/xai.ts (145-163). Use each client’s request options so
cancellation and timeouts are applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e539678-aff9-4a10-a391-e6ceb6abf6a6
📒 Files selected for processing (31)
src/api/index.tssrc/api/providers/__tests__/fake-ai.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/kenari.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/minimax.tssrc/api/providers/mistral.tssrc/api/providers/native-ollama.tssrc/api/providers/openai-codex.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai-native.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/poe.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/vscode-lm.tssrc/api/providers/xai.tssrc/api/providers/zoo-gateway.tssrc/core/task/__tests__/Task.spec.tssrc/utils/__tests__/enhance-prompt.spec.tssrc/utils/single-completion-handler.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/utils/single-completion-handler.ts
- src/api/providers/bedrock.ts
- src/api/providers/qwen-code.ts
- src/api/providers/vscode-lm.ts
- src/api/providers/openai-codex.ts
- src/api/providers/mistral.ts
- src/api/providers/zoo-gateway.ts
- src/api/providers/openai.ts
- src/api/providers/openrouter.ts
- src/api/providers/minimax.ts
- src/api/providers/anthropic-vertex.ts
- src/api/providers/kenari.ts
- src/api/providers/unbound.ts
- src/api/providers/openai-compatible.ts
- src/api/providers/native-ollama.ts
- src/core/task/tests/Task.spec.ts
- src/api/providers/openai-native.ts
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
Fixes #615 — Core plumbing for abort signal in
completePromptpath.This PR adds an optional second parameter (
CompletePromptOptions) to all providercompletePromptmethods, enabling future abort signal and timeout support without breaking existing callers.Changes
New:
CompletePromptOptionsinterfaceDesign note: Rather than passing the full
ApiHandlerCreateMessageMetadataas issue #615 described, this PR introduces a minimal interface that only exposesabortSignalandtimeoutMs. This keeps the API surface clean while still enabling abort signal pass-through. Themetadatapattern used in the stream path (viaApiHandlerCreateMessageMetadata) is already handled separately.Updated files
src/api/index.ts— AddedCompletePromptOptions, updatedSingleCompletionHandler.completePromptsignatureoptions?: CompletePromptOptions(backward compatible, optional)src/utils/single-completion-handler.ts— Forwards options to the handlerTests (
Task.spec.ts)toBeInstanceOf(AbortSignal)→toBe(task.currentRequestAbortController!.signal)for signal identity verificationAcceptance criteria (from #615)
SingleCompletionHandler.completePromptinterface accepts optional metadata/optionssingle-completion-handler.tsforwards options to the providerTask.tsattaches abortSignal to metadata (already wired in feat(api): pass abortSignal to streaming API calls for all providers (#404) #434, this PR completes the interface layer)Related
Summary by CodeRabbit
New Features
timeoutMsandabortSignal.completePromptto accept optional metadata/options and ensured the single-completion flow forwards these options to the selected provider.Tests
abortSignalinstance and confirmed retries use a fresh, initially un-aborted signal.completePromptto be called with an explicitundefinedoptions argument when omitted.FakeAIHandlerdelegatescompletePromptwith the correct prompt/options.