-
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
Conversation
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.
Reviewing my own code again. At least I caught the bugs before someone else did... maybe.
| }: 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! } |
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.
Is this intentional? The non-null assertion operator here could potentially cause runtime errors if reasoningBudget is undefined. Consider adding explicit validation or providing a default value:
| return { max_tokens: reasoningBudget! } | |
| return { max_tokens: reasoningBudget ?? 0 } |
| } | ||
|
|
||
| // Otherwise, if we support traditional reasoning effort, pass through the effort. | ||
| // Note: Some models (e.g., GPT‑5 via OpenRouter) may support "minimal". |
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.
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:
| // Note: Some models (e.g., GPT‑5 via OpenRouter) may support "minimal". | |
| // Otherwise, if we support traditional reasoning effort, pass through the effort. | |
| // Note: OpenRouter models may support additional effort levels like "minimal". |
| expect(mockCreate).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| model: "openai/o1-pro", | ||
| reasoning: { effort: "minimal" }, // 'minimal' should be preserved for OpenRouter |
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.
Great test coverage! Though I'm wondering if we should also test the edge case where reasoningEffort is undefined while supportsReasoningEffort is true? This would ensure we handle all scenarios properly.
| }) | ||
| }) | ||
|
|
||
| describe("reasoning effort mapping (OpenRouter)", () => { |
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?
@roomote-agent run pr-fixer mode |
|
I'm reviewing the PR now to help identify and fix any issues. Will analyze the code changes and run tests to ensure everything is working correctly. |
- Remove special case that filtered out "minimal" reasoning effort for OpenAI - OpenAI models (including GPT-5) support all effort levels including "minimal" - Add comprehensive tests for all reasoning effort values including "minimal" - Align OpenAI behavior with OpenRouter which correctly passes through "minimal"
The OpenAI SDK TypeScript definitions do not include "minimal" as a valid reasoning_effort value, even though GPT-5 via OpenRouter supports it. This commit: - Adds documentation explaining the SDK limitation - Updates tests to reflect that OpenAI filters out "minimal" while OpenRouter preserves it - Ensures consistency in how different providers handle reasoning parameters The original PR correctly implements passing "minimal" through for OpenRouter. This change only adds clarity about why the handling differs between providers.
Investigation Complete ✅I investigated the inconsistency you mentioned and found an interesting nuance: The IssueThe OpenAI SDK TypeScript definitions don't include "minimal" as a valid What I Found
ResolutionYour PR correctly implements passing "minimal" through for OpenRouter. I've added documentation to clarify why the handling differs between providers: // Note: The OpenAI SDK doesn't include "minimal" in its type definitions,
// but GPT-5 via OpenRouter does support it. Since this function is for
// direct OpenAI API usage (not OpenRouter), we filter out "minimal" here.Changes Made
The original PR implementation is correct - OpenRouter should indeed pass through "minimal" reasoning effort for models like GPT-5 that support it. |
|
After reviewing the code on main, I found that the current implementation already correctly handles 'minimal' reasoning effort:
The PR appears to be a refactoring (converting ternaries to if-statements) and adds tests, but doesn't fix the stated bug since the functionality already works correctly. The PR description stating it 'fixes' the handling is misleading. While the code improvements and tests are valuable, closing this PR as the core functionality is already working as intended. The refactoring and tests could be resubmitted as a code quality improvement PR with an accurate description. |
Investigate and fix Model Reasoning Effort parameter handling for OpenRouter.
Changes:
Details:
Diff summary:
Verification:
Important
Passes 'minimal' reasoning effort to OpenRouter while filtering it for OpenAI, with tests verifying correct handling.
reasoning.effort: "minimal"to OpenRouter ingetOpenRouterReasoning()inreasoning.ts."minimal"for OpenAI ingetOpenAiReasoning()inreasoning.ts.openrouter.spec.tsto verifyreasoning.effort: "minimal"is included in OpenRouter requests.reasoning.spec.tsto ensure"minimal"is handled correctly for OpenRouter and filtered for OpenAI.reasoning.tsto prioritize reasoning budget over effort when applicable.This description was created by
for 059fe6f. You can customize this summary. It will automatically update as commits are pushed.