-
Notifications
You must be signed in to change notification settings - Fork 212
fix: apply apiRequestTimeout consistently across providers #567
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 5 commits
4c82de4
e9461af
94344e7
59ff0ed
591efef
eee6cb0
1bb1827
0bad013
cca8e68
5411599
9be9cec
09be296
ca18a8e
2aeb510
ad547d2
8cba19e
b25a2d0
72ee779
fa1cb19
2bbc093
760f2ee
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 |
|---|---|---|
|
|
@@ -64,6 +64,7 @@ describe("VertexHandler", () => { | |
| expect(AnthropicVertex).toHaveBeenCalledWith({ | ||
| projectId: "test-project", | ||
| region: "us-central1", | ||
| timeout: expect.any(Number), | ||
|
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. Same
Contributor
Author
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. Updated the Anthropic Vertex tests to use the same concrete timeout sentinel across the bare, JSON credentials, and key file credentials paths. |
||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,12 @@ | ||
| // pnpm --filter roo-cline test api/providers/__tests__/openrouter.spec.ts | ||
|
|
||
| vitest.mock("vscode", () => ({})) | ||
| vitest.mock("vscode", () => ({ | ||
| workspace: { | ||
| getConfiguration: () => ({ | ||
| get: (_key: string, defaultValue?: unknown) => defaultValue, | ||
| }), | ||
| }, | ||
| })) | ||
|
|
||
| import { Anthropic } from "@anthropic-ai/sdk" | ||
| import OpenAI from "openai" | ||
|
|
@@ -104,6 +110,7 @@ describe("OpenRouterHandler", () => { | |
| "X-Title": "Zoo Code", | ||
| "User-Agent": `ZooCode/${Package.version}`, | ||
| }, | ||
| timeout: expect.any(Number), | ||
|
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. Could this use a concrete value instead of
Contributor
Author
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. Updated. I mocked |
||
| }) | ||
| }) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ describe("RequestyHandler", () => { | |
| "X-Title": "Zoo Code", | ||
| "User-Agent": `ZooCode/${Package.version}`, | ||
| }, | ||
| timeout: expect.any(Number), | ||
|
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. Same question as the openrouter test —
Contributor
Author
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. Updated Requesty as well. Both client-construction assertions now verify the exact mocked timeout value rather than accepting any number. |
||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -77,6 +78,7 @@ describe("RequestyHandler", () => { | |
| "X-Title": "Zoo Code", | ||
| "User-Agent": `ZooCode/${Package.version}`, | ||
| }, | ||
| timeout: expect.any(Number), | ||
| }) | ||
| }) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import { getModelParams } from "../transform/model-params" | |
| import { filterNonAnthropicBlocks } from "../transform/anthropic-filter" | ||
| import { getAnthropicProviderReasoning } from "../transform/reasoning" | ||
| import { handleProviderError } from "./utils/error-handler" | ||
| import { getApiRequestTimeout } from "./utils/timeout-config" | ||
|
|
||
| import { BaseProvider } from "./base-provider" | ||
| import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index" | ||
|
|
@@ -44,6 +45,7 @@ export class AnthropicHandler extends BaseProvider implements SingleCompletionHa | |
| this.client = new Anthropic({ | ||
| baseURL: this.options.anthropicBaseUrl || undefined, | ||
| [apiKeyFieldName]: this.options.apiKey, | ||
| timeout: getApiRequestTimeout(), | ||
|
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. The
Contributor
Author
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. I fixed this in two places: the configuration schema now uses |
||
| }) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ import { isMcpTool } from "../../utils/mcp-name" | |
| import { sanitizeOpenAiCallId } from "../../utils/tool-id" | ||
| import { openAiCodexOAuthManager } from "../../integrations/openai-codex/oauth" | ||
| import { t } from "../../i18n" | ||
| import { getApiRequestTimeout } from "./utils/timeout-config" | ||
|
|
||
| export type OpenAiCodexModel = ReturnType<OpenAiCodexHandler["getModel"]> | ||
|
|
||
|
|
@@ -371,6 +372,7 @@ export class OpenAiCodexHandler extends BaseProvider implements SingleCompletion | |
| apiKey: accessToken, | ||
| baseURL: CODEX_API_BASE_URL, | ||
| defaultHeaders: codexHeaders, | ||
| timeout: getApiRequestTimeout(), | ||
|
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. Every other provider calls
Contributor
Author
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. Not intentional. I updated OpenAI Codex to cache the timeout at handler construction time and pass that cached value when the lazy client is created, so it now matches the behavior of the other providers. |
||
| }) | ||
|
|
||
| const stream = (await (client as any).responses.create(requestBody, { | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
This only tests the bare construction path (
{ projectId, region, timeout }).anthropic-vertex.tshas two other branches —parsedVertexCredentialsandvertexKeyFile— that also passtimeout. Are those covered somewhere, or worth adding here?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.
I added explicit coverage for both additional Anthropic Vertex constructor branches:
vertexJsonCredentialsandvertexKeyFile.The tests now assert that
GoogleAuthreceives the expected credentials/keyFile options and that the resultingAnthropicVertexconstructor config includestimeout. I also adjusted theGoogleAuthmock to use a function expression so it can be invoked withnew, matching the handler path.