-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(openrouter): pass GPT-5 reasoning effort and include_reasoning to OpenRouter #7319
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
…soning set 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.
I reviewed my own code and found it suspiciously adequate.
| let adjustedParams = params | ||
| if (id.startsWith("openai/gpt-5") && !params.reasoning && this.options.enableReasoningEffort !== false) { | ||
| const effort = (this.options.reasoningEffort as any) ?? "medium" | ||
| adjustedParams = { ...params, reasoning: { effort } as OpenRouterReasoningParams } |
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 logic potentially redundant? I notice we're adding GPT-5-specific reasoning handling here, but getModelParams() already calls getOpenRouterReasoning() which should handle reasoning parameters. Could we rely solely on the existing getModelParams() flow instead of duplicating the logic?
| stream: true, | ||
| stream_options: { include_usage: true }, | ||
| // For GPT-5 via OpenRouter, request reasoning content in the stream explicitly | ||
| ...(modelId.startsWith("openai/gpt-5") && { include_reasoning: true }), |
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.
Consider extracting this GPT-5 check to a helper function like isGpt5Model(modelId) since the same check appears in both createMessage() (line 125) and completePrompt() (line 233). This would make future updates easier and more maintainable.
| const call = (mockCreate as any).mock.calls[0][0] | ||
| expect(call.include_reasoning).toBe(true) | ||
| expect(call.reasoning).toEqual({ effort: "medium" }) | ||
| }) |
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.
Good test coverage for createMessage()! Should we add a similar test for completePrompt() to verify that include_reasoning: true is also passed for GPT-5 models in that method?
This PR ensures the Model Reasoning Effort is correctly passed to OpenRouter for GPT‑5 models and that reasoning content is requested in the stream.
Changes:
Context:
Validation:
Security:
Important
Enhance OpenRouter integration for GPT-5 by ensuring reasoning efforts are passed and included, with default effort set when unspecified.
include_reasoning=truefor GPT-5 requests inOpenRouterHandler.createMessage()andOpenRouterHandler.completePrompt().OpenRouterHandler.getModel().openrouter.spec.tsto assertinclude_reasoningand reasoning effort for GPT-5.This description was created by
for 039e59f. You can customize this summary. It will automatically update as commits are pushed.