fix: max reasoning effort for deepseek v4 - #4330
Conversation
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR makes reasoning-effort normalization model-aware: it preserves ChangesDeepSeek V4 Reasoning Effort Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5Safe to merge; the change is a targeted short-circuit in a pure transformation function with no side effects, and the new test cases cover the expected paths well. The new helper is structurally identical to the two sibling helpers it sits alongside, the No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix: max reasoning effort for deepseek v..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/openai/utils.go`:
- Around line 133-136: supportsMaxReasoningEffort currently checks the raw model
string; normalize the input first using schemas.ParseModelString(model) and then
run the prefix check on the parsed model name (e.g., use parsed.Model or
equivalent field), e.g. lowercasing that parsed value and calling
strings.HasPrefix(..., "deepseek-v4") so provider-qualified forms are normalized
and "max" is preserved correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 729b5116-764f-40de-9732-ad518b26832b
📒 Files selected for processing (3)
core/providers/openai/chat_test.gocore/providers/openai/responses_test.gocore/providers/openai/utils.go
bdc1392 to
b6a2a61
Compare
Merge activity
|
## Summary DeepSeek V4 models (`deepseek-v4-pro`, `deepseek-v4-flash`) natively accept `"max"` as a reasoning effort value. Previously, the normalization logic would convert `"max"` to `"high"` (or `"xhigh"` for supported OpenAI models) for all models, which incorrectly stripped the valid `"max"` value when routing through DeepSeek's OpenAI-compatible provider. closes #4320 ## Changes - Added `supportsMaxReasoningEffort` helper in `utils.go` that identifies models with a `deepseek-v4` prefix as natively supporting `"max"` reasoning effort. - Updated `normalizeOpenAIReasoningEffort` to short-circuit and return `"max"` unchanged for those models before applying the OpenAI-specific `"high"`/`"xhigh"` mapping. - Extended tests in `chat_test.go` and `responses_test.go` to cover the `"max"` passthrough behavior for `deepseek-v4-pro` and `deepseek-v4-flash`. - Updated the responses test harness to accept a configurable `provider` field so DeepSeek-routed requests can be tested independently from OpenAI-routed ones. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/openai/... ``` Verify that the new test cases `"preserves max for deepseek-v4-pro"` and `"preserves max for deepseek-v4-flash"` pass in both `TestToOpenAIChatRequest_NormalizesReasoningEffort`, `TestOpenAIChatRequest_FilterOpenAISpecificParameters_NormalizesReasoningEffort`, and `TestToOpenAIResponsesRequest_NormalizesReasoningEffort`. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added official support for DeepSeek V4 models (deepseek-v4-pro, deepseek-v4-flash), including recognition of provider-prefixed model identifiers. * **Bug Fixes** * Reasoning-effort handling now preserves the "max" setting for DeepSeek V4 models across requests and responses, ensuring expected high-effort behavior is retained. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…4467) GLM-5.2 (Z.ai) natively supports reasoning_effort "max" (alongside "high"), but normalizeOpenAIReasoningEffort() silently downgraded "max" to "high" because GLM-5.2 was not in supportsMaxReasoningEffort(). This is the same normalization gap that previously affected DeepSeek V4 (issue #4320, PR #4330). Affected packages: core/providers/openai/ Closes #4460 Signed-off-by: is911 <archilles92@gmail.com> Signed-off-by: Akshay Deo <akshay@akshaydeo.com> Co-authored-by: Akshay Deo <akshay@akshaydeo.com>
## Summary DeepSeek V4 models (`deepseek-v4-pro`, `deepseek-v4-flash`) natively accept `"max"` as a reasoning effort value. Previously, the normalization logic would convert `"max"` to `"high"` (or `"xhigh"` for supported OpenAI models) for all models, which incorrectly stripped the valid `"max"` value when routing through DeepSeek's OpenAI-compatible provider. closes maximhq#4320 ## Changes - Added `supportsMaxReasoningEffort` helper in `utils.go` that identifies models with a `deepseek-v4` prefix as natively supporting `"max"` reasoning effort. - Updated `normalizeOpenAIReasoningEffort` to short-circuit and return `"max"` unchanged for those models before applying the OpenAI-specific `"high"`/`"xhigh"` mapping. - Extended tests in `chat_test.go` and `responses_test.go` to cover the `"max"` passthrough behavior for `deepseek-v4-pro` and `deepseek-v4-flash`. - Updated the responses test harness to accept a configurable `provider` field so DeepSeek-routed requests can be tested independently from OpenAI-routed ones. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/openai/... ``` Verify that the new test cases `"preserves max for deepseek-v4-pro"` and `"preserves max for deepseek-v4-flash"` pass in both `TestToOpenAIChatRequest_NormalizesReasoningEffort`, `TestOpenAIChatRequest_FilterOpenAISpecificParameters_NormalizesReasoningEffort`, and `TestToOpenAIResponsesRequest_NormalizesReasoningEffort`. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added official support for DeepSeek V4 models (deepseek-v4-pro, deepseek-v4-flash), including recognition of provider-prefixed model identifiers. * **Bug Fixes** * Reasoning-effort handling now preserves the "max" setting for DeepSeek V4 models across requests and responses, ensuring expected high-effort behavior is retained. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…aximhq#4467) GLM-5.2 (Z.ai) natively supports reasoning_effort "max" (alongside "high"), but normalizeOpenAIReasoningEffort() silently downgraded "max" to "high" because GLM-5.2 was not in supportsMaxReasoningEffort(). This is the same normalization gap that previously affected DeepSeek V4 (issue maximhq#4320, PR maximhq#4330). Affected packages: core/providers/openai/ Closes maximhq#4460 Signed-off-by: is911 <archilles92@gmail.com> Signed-off-by: Akshay Deo <akshay@akshaydeo.com> Co-authored-by: Akshay Deo <akshay@akshaydeo.com>
## Summary DeepSeek V4 models (`deepseek-v4-pro`, `deepseek-v4-flash`) natively accept `"max"` as a reasoning effort value. Previously, the normalization logic would convert `"max"` to `"high"` (or `"xhigh"` for supported OpenAI models) for all models, which incorrectly stripped the valid `"max"` value when routing through DeepSeek's OpenAI-compatible provider. closes maximhq#4320 ## Changes - Added `supportsMaxReasoningEffort` helper in `utils.go` that identifies models with a `deepseek-v4` prefix as natively supporting `"max"` reasoning effort. - Updated `normalizeOpenAIReasoningEffort` to short-circuit and return `"max"` unchanged for those models before applying the OpenAI-specific `"high"`/`"xhigh"` mapping. - Extended tests in `chat_test.go` and `responses_test.go` to cover the `"max"` passthrough behavior for `deepseek-v4-pro` and `deepseek-v4-flash`. - Updated the responses test harness to accept a configurable `provider` field so DeepSeek-routed requests can be tested independently from OpenAI-routed ones. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/openai/... ``` Verify that the new test cases `"preserves max for deepseek-v4-pro"` and `"preserves max for deepseek-v4-flash"` pass in both `TestToOpenAIChatRequest_NormalizesReasoningEffort`, `TestOpenAIChatRequest_FilterOpenAISpecificParameters_NormalizesReasoningEffort`, and `TestToOpenAIResponsesRequest_NormalizesReasoningEffort`. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added official support for DeepSeek V4 models (deepseek-v4-pro, deepseek-v4-flash), including recognition of provider-prefixed model identifiers. * **Bug Fixes** * Reasoning-effort handling now preserves the "max" setting for DeepSeek V4 models across requests and responses, ensuring expected high-effort behavior is retained. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…aximhq#4467) GLM-5.2 (Z.ai) natively supports reasoning_effort "max" (alongside "high"), but normalizeOpenAIReasoningEffort() silently downgraded "max" to "high" because GLM-5.2 was not in supportsMaxReasoningEffort(). This is the same normalization gap that previously affected DeepSeek V4 (issue maximhq#4320, PR maximhq#4330). Affected packages: core/providers/openai/ Closes maximhq#4460 Signed-off-by: is911 <archilles92@gmail.com> Signed-off-by: Akshay Deo <akshay@akshaydeo.com> Co-authored-by: Akshay Deo <akshay@akshaydeo.com>

Summary
DeepSeek V4 models (
deepseek-v4-pro,deepseek-v4-flash) natively accept"max"as a reasoning effort value. Previously, the normalization logic would convert"max"to"high"(or"xhigh"for supported OpenAI models) for all models, which incorrectly stripped the valid"max"value when routing through DeepSeek's OpenAI-compatible provider.closes #4320
Changes
supportsMaxReasoningEfforthelper inutils.gothat identifies models with adeepseek-v4prefix as natively supporting"max"reasoning effort.normalizeOpenAIReasoningEffortto short-circuit and return"max"unchanged for those models before applying the OpenAI-specific"high"/"xhigh"mapping.chat_test.goandresponses_test.goto cover the"max"passthrough behavior fordeepseek-v4-proanddeepseek-v4-flash.providerfield so DeepSeek-routed requests can be tested independently from OpenAI-routed ones.Type of change
Affected areas
How to test
go test ./core/providers/openai/...Verify that the new test cases
"preserves max for deepseek-v4-pro"and"preserves max for deepseek-v4-flash"pass in bothTestToOpenAIChatRequest_NormalizesReasoningEffort,TestOpenAIChatRequest_FilterOpenAISpecificParameters_NormalizesReasoningEffort, andTestToOpenAIResponsesRequest_NormalizesReasoningEffort.Breaking changes
Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Bug Fixes