-
Notifications
You must be signed in to change notification settings - Fork 164
feat(api): abort signal pass-through for openai-native, openai-codex, bedrock, native-ollama #623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3fbb572
4ab7a33
f2377ec
d08ce41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -216,7 +216,7 @@ describe("OpenAiNativeHandler", () => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect.objectContaining({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal: expect.any(Object), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal: expect.any(AbortSignal), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -245,6 +245,49 @@ describe("OpenAiNativeHandler", () => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toBe("") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should use metadata.abortSignal when provided in completePrompt", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mockResponsesCreate.mockResolvedValue({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: "message", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content: [{ type: "output_text", text: "This is the completion response" }], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const controller = new AbortController() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = await handler.completePrompt("Test prompt", { abortSignal: controller.signal }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toBe("This is the completion response") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(mockResponsesCreate).toHaveBeenCalledWith( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect.any(Object), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect.objectContaining({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal: controller.signal, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should use default abortController signal when metadata.abortSignal not provided", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mockResponsesCreate.mockResolvedValue({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: "message", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content: [{ type: "output_text", text: "This is the completion response" }], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = await handler.completePrompt("Test prompt") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(result).toBe("This is the completion response") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(mockResponsesCreate).toHaveBeenCalledWith( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect.any(Object), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect.objectContaining({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal: handler["abortController"].signal, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+271
to
+290
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test will crash when accessing undefined abortController. Line 287 accesses 🔧 Recommended fix: check the mock call arguments directly it("should use default abortController signal when metadata.abortSignal not provided", async () => {
mockResponsesCreate.mockResolvedValue({
output: [
{
type: "message",
content: [{ type: "output_text", text: "This is the completion response" }],
},
],
})
const result = await handler.completePrompt("Test prompt")
expect(result).toBe("This is the completion response")
expect(mockResponsesCreate).toHaveBeenCalledWith(
expect.any(Object),
expect.objectContaining({
- signal: handler["abortController"].signal,
+ signal: expect.any(AbortSignal),
}),
)
})📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe("getModel", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1624,7 +1667,7 @@ describe("GPT-5 streaming event coverage (additional)", () => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| store: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect.objectContaining({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal: expect.any(Object), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal: expect.any(AbortSignal), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,14 +234,14 @@ export class NativeOllamaHandler extends BaseProvider implements SingleCompletio | |
| chatOptions.num_ctx = this.options.ollamaNumCtx | ||
| } | ||
|
|
||
| // Create the actual API request promise | ||
| const stream = await client.chat({ | ||
| const controller = new AbortController() | ||
| const stream = (await client.chat({ | ||
| model: modelId, | ||
| messages: ollamaMessages, | ||
| stream: true, | ||
| options: chatOptions, | ||
| tools: this.convertToolsToOllama(metadata?.tools), | ||
| }) | ||
| })) as any | ||
|
Comment on lines
+238
to
+244
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the Ollama SDK type signature for the chat method
rg -A 10 'chat\s*\(' --type ts --iglob '**/node_modules/ollama/**/*.d.ts'Repository: Zoo-Code-Org/Zoo-Code Length of output: 214 🏁 Script executed: # Check package.json for ollama dependency
cat package.json | grep -A 2 -B 2 '"ollama"'Repository: Zoo-Code-Org/Zoo-Code Length of output: 47 🏁 Script executed: # Look at the actual code in native-ollama.ts around the problematic lines
cat -n src/api/providers/native-ollama.ts | sed -n '230,250p'Repository: Zoo-Code-Org/Zoo-Code Length of output: 857 🏁 Script executed: # Search for any comments or explanations around the type cast
rg -B 5 -A 5 'client\.chat.*as any' src/api/providers/native-ollama.tsRepository: Zoo-Code-Org/Zoo-Code Length of output: 47 🏁 Script executed: # Check if there are any type definitions for Ollama chat in the codebase
rg -i 'chat.*function|chat.*method|OllamaChat' --type tsRepository: Zoo-Code-Org/Zoo-Code Length of output: 3383 🏁 Script executed: # Check package.json for ollama dependency version
rg '"ollama"' package.json -A 1 -B 1Repository: Zoo-Code-Org/Zoo-Code Length of output: 47 🏁 Script executed: # Look at how client is initialized to understand its type
rg -B 10 'const client' src/api/providers/native-ollama.ts | head -30Repository: Zoo-Code-Org/Zoo-Code Length of output: 815 🏁 Script executed: # Check the test file to see how the mock is set up
cat -n src/api/providers/__tests__/native-ollama.spec.ts | head -100Repository: Zoo-Code-Org/Zoo-Code Length of output: 3425 🏁 Script executed: # Search for imports of Ollama to understand the SDK
rg 'import.*ollama|from.*ollama' --type ts -iRepository: Zoo-Code-Org/Zoo-Code Length of output: 2079 🏁 Script executed: # Check how the stream variable is used after line 244
cat -n src/api/providers/native-ollama.ts | sed -n '244,280p'Repository: Zoo-Code-Org/Zoo-Code Length of output: 1659 🏁 Script executed: # Look for the actual return type of client.chat in the Ollama SDK
rg -i 'export.*chat|declare.*chat' --type ts src/Repository: Zoo-Code-Org/Zoo-Code Length of output: 1854 🏁 Script executed: # Get the full function signature to understand the context better
cat -n src/api/providers/native-ollama.ts | sed -n '206,260p'Repository: Zoo-Code-Org/Zoo-Code Length of output: 2154 🏁 Script executed: # Check package.json and package-lock.json for the exact Ollama version
cat package.json | grep -A 5 -B 5 'ollama' || cat package-lock.json | grep -A 10 '"ollama"' | head -20Repository: Zoo-Code-Org/Zoo-Code Length of output: 115 🏁 Script executed: # Search for any GitHub issues or comments about Ollama type casting
rg -i 'ollama.*type|any.*chat|stream.*any' --type ts -C 2Repository: Zoo-Code-Org/Zoo-Code Length of output: 42748 🏁 Script executed: # Check if there are type definitions or interfaces defined for Ollama chat response
rg -i 'interface.*chat|type.*chat.*response|ollama.*response' --type tsRepository: Zoo-Code-Org/Zoo-Code Length of output: 2450 🏁 Script executed: # Get the full package.json to find ollama version
cat package.json | grep -A 50 '"dependencies"' | grep -E '"ollama"|"version"'Repository: Zoo-Code-Org/Zoo-Code Length of output: 47 🏁 Script executed: # Look for any comments or issues related to type casting in the Ollama provider
cat -n src/api/providers/native-ollama.ts | sed -n '1,50p'Repository: Zoo-Code-Org/Zoo-Code Length of output: 2613 🏁 Script executed: # Check if there are any other cast to any in similar streaming contexts
rg '\)\s*as\s+any' src/api/providers --type ts -B 2 -A 2Repository: Zoo-Code-Org/Zoo-Code Length of output: 16509 Add explanatory comment or remove the unexplained The
🤖 Prompt for AI Agents |
||
|
|
||
| let totalInputTokens = 0 | ||
| let totalOutputTokens = 0 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -424,8 +424,9 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio | |
|
|
||
| try { | ||
| // Use the official SDK with per-request headers | ||
| const signal = metadata?.abortSignal ?? this.abortController.signal | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All four provider handlers read
📍 Affects 3 files
🤖 Prompt for AI Agents |
||
| const stream = (await (this.client as any).responses.create(requestBody, { | ||
| signal: this.abortController.signal, | ||
| signal, | ||
| headers: requestHeaders, | ||
| })) as AsyncIterable<any> | ||
|
|
||
|
|
@@ -576,7 +577,7 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio | |
| "User-Agent": userAgent, | ||
| }, | ||
| body: JSON.stringify(requestBody), | ||
| signal: this.abortController.signal, | ||
| signal: metadata?.abortSignal ?? this.abortController.signal, | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
|
|
@@ -1482,7 +1483,7 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio | |
| return this.lastResponseId | ||
| } | ||
|
|
||
| async completePrompt(prompt: string): Promise<string> { | ||
| async completePrompt(prompt: string, metadata?: ApiHandlerCreateMessageMetadata): Promise<string> { | ||
| // Create AbortController for cancellation | ||
| this.abortController = new AbortController() | ||
|
|
||
|
|
@@ -1544,9 +1545,9 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio | |
| requestBody.prompt_cache_retention = promptCacheRetention | ||
| } | ||
|
|
||
| // Make the non-streaming request | ||
| // Make the non-streaming request with conditional signal pass-through | ||
| const response = await (this.client as any).responses.create(requestBody, { | ||
| signal: this.abortController.signal, | ||
| signal: metadata?.abortSignal ?? this.abortController.signal, | ||
| }) | ||
|
|
||
| // Extract text from the response | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are non-functional: vi.mock() inside test cases has no effect.
Lines 1583 and 1606 call
vi.mock()inside individual test functions. Vitest hoistsvi.mock()calls to module scope before imports resolve, so callingvi.mock()inside a test (after imports have already been processed) has no effect. Additionally,mockConverseCommandis defined as a local variable (lines 1582, 1605) that is never connected to the actualConverseCommandconstructor, so the expectations at lines 1601 and 1623 will always fail.The module-level mock infrastructure already exists (lines 22-37). Use the existing
vi.mocked(ConverseCommand)andBedrockRuntimeClientmocks instead.🔧 Recommended fix: use module-level mocks
describe("abortSignal pass-through", () => { + const mockConverseCommand = vi.mocked(ConverseCommand) + + beforeEach(() => { + mockConverseCommand.mockClear() + }) + it("should use metadata.abortSignal when provided in completePrompt", async () => { - const mockConverseCommand = vi.fn() - vi.mock("`@aws-sdk/client-bedrock-runtime`", async (importOriginal) => { - const actual = await importOriginal<typeof import("`@aws-sdk/client-bedrock-runtime`")>() - return { - ...actual, - BedrockRuntimeClient: vi.fn().mockReturnValue({ send: vi.fn().mockResolvedValue({ stream: null }) }), - } - }) - const handler = new AwsBedrockHandler({ apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", awsAccessKey: "test", awsSecretKey: "test", awsRegion: "us-east-1", }) + // Get the mock send from the handler's client + const clientInstance = + vi.mocked(BedrockRuntimeClient).mock.results[vi.mocked(BedrockRuntimeClient).mock.results.length - 1]?.value + const mockSendFn = clientInstance?.send as ReturnType<typeof vi.fn> + mockSendFn.mockResolvedValueOnce({ output: [{ type: "message", content: [{ type: "text", text: "Response" }] }] }) + const controller = new AbortController() await handler.completePrompt("Test prompt", { abortSignal: controller.signal }) expect(mockConverseCommand).toHaveBeenCalled() + const commandArg = mockConverseCommand.mock.calls[0][0] as any + // The upstream contract passes abortSignal via client.send options, not the command itself + // Verify the command was created (signal is passed separately to send) }) it("should use default controller signal when metadata.abortSignal not provided", async () => { - const mockConverseCommand = vi.fn() - vi.mock("`@aws-sdk/client-bedrock-runtime`", async (importOriginal) => { - const actual = await importOriginal<typeof import("`@aws-sdk/client-bedrock-runtime`")>() - return { - ...actual, - BedrockRuntimeClient: vi.fn().mockReturnValue({ send: vi.fn().mockResolvedValue({ stream: null }) }), - } - }) - const handler = new AwsBedrockHandler({ apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0", awsAccessKey: "test", awsSecretKey: "test", awsRegion: "us-east-1", }) + const clientInstance = + vi.mocked(BedrockRuntimeClient).mock.results[vi.mocked(BedrockRuntimeClient).mock.results.length - 1]?.value + const mockSendFn = clientInstance?.send as ReturnType<typeof vi.fn> + mockSendFn.mockResolvedValueOnce({ output: [{ type: "message", content: [{ type: "text", text: "Response" }] }] }) + await handler.completePrompt("Test prompt") expect(mockConverseCommand).toHaveBeenCalled() }) })Note: The Bedrock implementation passes the
abortSignaloption toclient.send(), not in theConverseCommandconstructor. You may need to spy on thesendcall's second argument to fully verify the signal threading.🤖 Prompt for AI Agents