-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(openrouter): pass minimal reasoning effort to OpenRouter #7402
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 1 commit
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 |
|---|---|---|
|
|
@@ -321,3 +321,71 @@ describe("OpenRouterHandler", () => { | |
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe("reasoning effort mapping (OpenRouter)", () => { | ||
| it("passes 'minimal' through in reasoning.effort for OpenRouter requests", async () => { | ||
| const handler = new OpenRouterHandler({ | ||
| openRouterApiKey: "test-key", | ||
| openRouterModelId: "openai/o1-pro", | ||
| reasoningEffort: "minimal", | ||
| } as ApiHandlerOptions) | ||
|
|
||
| // Prepare a model that supports reasoning effort (not budget) | ||
| ;(handler as any).models = { | ||
| "openai/o1-pro": { | ||
| maxTokens: 8192, | ||
| contextWindow: 200000, | ||
| supportsImages: true, | ||
| supportsPromptCache: true, | ||
| inputPrice: 0.0, | ||
| outputPrice: 0.0, | ||
| description: "o1-pro test", | ||
| supportsReasoningEffort: true, | ||
| }, | ||
| } | ||
|
|
||
| // Ensure endpoints map is empty so base model info is used | ||
| ;(handler as any).endpoints = {} | ||
|
|
||
| // Mock OpenAI client call | ||
| const mockCreate = vitest.fn().mockResolvedValue({ | ||
| async *[Symbol.asyncIterator]() { | ||
| yield { | ||
| id: "openai/o1-pro", | ||
| choices: [{ delta: { content: "ok" } }], | ||
| } | ||
| yield { | ||
| id: "usage-id", | ||
| choices: [{ delta: {} }], | ||
| usage: { prompt_tokens: 1, completion_tokens: 1, cost: 0 }, | ||
| } | ||
| }, | ||
| }) | ||
| ;(OpenAI as any).prototype.chat = { | ||
| completions: { create: mockCreate }, | ||
| } as any | ||
|
|
||
| const systemPrompt = "system" | ||
| const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "hello" }] | ||
|
|
||
| // Stub fetchModel to use the handler's getModel (which applies getModelParams -> getOpenRouterReasoning) | ||
| const realGetModel = (handler as any).getModel.bind(handler) | ||
| ;(handler as any).fetchModel = vitest.fn().mockImplementation(async () => realGetModel()) | ||
|
|
||
| // Trigger a request | ||
| const gen = handler.createMessage(systemPrompt, messages) | ||
| // Drain iterator to ensure call is made | ||
| for await (const _ of gen) { | ||
| // noop | ||
| } | ||
|
|
||
| // Verify the API call included the normalized effort | ||
| expect(mockCreate).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| model: "openai/o1-pro", | ||
| reasoning: { effort: "minimal" }, // 'minimal' should be preserved for OpenRouter | ||
|
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. Great test coverage! Though I'm wondering if we should also test the edge case where |
||
| stream: true, | ||
| }), | ||
| ) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -30,14 +30,21 @@ export const getOpenRouterReasoning = ({ | |||||||
| reasoningBudget, | ||||||||
| reasoningEffort, | ||||||||
| settings, | ||||||||
| }: GetModelReasoningOptions): OpenRouterReasoningParams | undefined => | ||||||||
| shouldUseReasoningBudget({ model, settings }) | ||||||||
| ? { max_tokens: reasoningBudget } | ||||||||
| : shouldUseReasoningEffort({ model, settings }) | ||||||||
| ? reasoningEffort | ||||||||
| ? { effort: reasoningEffort } | ||||||||
| : undefined | ||||||||
| : undefined | ||||||||
| }: GetModelReasoningOptions): OpenRouterReasoningParams | undefined => { | ||||||||
| // If the model uses a budget-style reasoning config on OpenRouter, pass it through. | ||||||||
| if (shouldUseReasoningBudget({ model, settings })) { | ||||||||
| return { max_tokens: reasoningBudget! } | ||||||||
|
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. Is this intentional? The non-null assertion operator here could potentially cause runtime errors if
Suggested change
|
||||||||
| } | ||||||||
|
|
||||||||
| // Otherwise, if we support traditional reasoning effort, pass through the effort. | ||||||||
| // Note: Some models (e.g., GPT‑5 via OpenRouter) may support "minimal". | ||||||||
|
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. The comment mentions "GPT-5 via OpenRouter" specifically, but this change affects all models using OpenRouter with reasoning effort support. Could we make this comment more generic to avoid confusion? Something like:
Suggested change
|
||||||||
| if (shouldUseReasoningEffort({ model, settings })) { | ||||||||
| if (!reasoningEffort) return undefined | ||||||||
| return { effort: reasoningEffort } | ||||||||
| } | ||||||||
|
|
||||||||
| return undefined | ||||||||
| } | ||||||||
|
|
||||||||
| export const getAnthropicReasoning = ({ | ||||||||
| model, | ||||||||
|
|
||||||||
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.
Minor organizational suggestion: This new test is in a separate describe block at the end of the file. Would it make more sense to integrate it with the existing "OpenRouterHandler" describe block above for better test organization?