fix: preserve Qwen thinking_budget passthrough - #5836
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesQwen thinking budget handling
Adaptor unsupported-path behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAIRequest
participant RequestConversion
participant ConvertOpenAIRequest
participant requestOpenAI2Ali
OpenAIRequest->>RequestConversion: preserve EnableThinking and ThinkingBudget
RequestConversion-->>ConvertOpenAIRequest: converted request
ConvertOpenAIRequest->>requestOpenAI2Ali: pass UpstreamModelName
requestOpenAI2Ali-->>OpenAIRequest: serialize model-specific thinking fields
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@dto/openai_request_zero_value_test.go`:
- Around line 54-102: The tests in
GeneralOpenAIRequestPreserveQwenThinkingBudget,
GeneralOpenAIRequestDropsThinkingBudgetForNonQwenModel, and
TestIsQwenThinkingBudgetModel are using require for non-fatal value assertions;
switch those checks to assert while keeping require only for setup/fatal steps
like common.Unmarshal and common.Marshal. Update the value comparisons in this
file to follow the same assert/require split used by the sibling relayconvert
tests, and keep the helper IsQwenThinkingBudgetModel coverage unchanged aside
from the assertion style.
In `@relay/channel/ali/adaptor_test.go`:
- Around line 14-55: The test assertions on the marshaled request output are
using require where they should be non-fatal assert checks; update
TestConvertOpenAIRequestPreservesThinkingBudgetForQwen and
TestConvertOpenAIRequestDropsThinkingBudgetForNonQwen to keep require.NoError
for setup/fatal failures, but switch the value checks on gjson.GetBytes(...) to
assert-based assertions so the test reports all mismatches without stopping
early.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac7ef7a5-39ae-42b6-90ff-3c1681a38b29
📒 Files selected for processing (9)
dto/openai_request.godto/openai_request_zero_value_test.gorelay/channel/ali/adaptor.gorelay/channel/ali/adaptor_test.gorelay/channel/ali/text.goservice/relayconvert/chat_responses_compat_test.goservice/relayconvert/chat_to_responses.goservice/relayconvert/responses_request_to_chat.goservice/relayconvert/responses_request_to_chat_test.go
…ng-budget # Conflicts: # service/relayconvert/internal/oai_responses/to_oai_chat_resp_test.go
|
@Calcium-Ion 你好,麻烦有空帮忙 review 一下这个 PR~ |
…thinking-budget # Conflicts: # relaykit/dto/openai_request_zero_value_test.go # relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go
…ng-budget # Conflicts: # relaykit/dto/openai_request_zero_value_test.go # relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relaykit/dto/openai_request.go (1)
9-10: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
common/json.gowrappers for relaykit JSON operations.
relaykit/relayconvert/kitutil/json.gois a local relaykit wrapper, so importing it here violates the required business-code contract to usecommon/json.gofor JSON marshaling/unmarshaling. Updaterelaykit/dto/openai_request.go,relaykit/relayconvert/internal/oai_chat/to_oai_responses_req.go, andrelaykit/relayconvert/internal/oai_responses/to_oai_chat_req.goto usecommon.Marshal/Unmarshal/helpers instead ofkitutil.🤖 Prompt for 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. In `@relaykit/dto/openai_request.go` around lines 9 - 10, Replace relaykit/relayconvert/kitutil JSON usage with the common/json.go wrappers in relaykit/dto/openai_request.go, relaykit/relayconvert/internal/oai_chat/to_oai_responses_req.go, and relaykit/relayconvert/internal/oai_responses/to_oai_chat_req.go. Update the affected imports and calls to use common.Marshal, common.Unmarshal, or the corresponding common helpers while preserving existing request-conversion behavior.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (2)
relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go (1)
42-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover explicit zero in both conversion paths.
Both tests verify only
thinking_budget: 128; add explicit0cases so zero-value passthrough remains protected end to end.
relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go#L42-L59: add a Qwen request withThinkingBudget: json.RawMessage(\0`)`.relaykit/relayconvert/internal/oai_responses/to_oai_chat_req_test.go#L59-L72: add the corresponding Responses-to-Chat zero-value case.🤖 Prompt for 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. In `@relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go` around lines 42 - 59, Add explicit zero-budget test coverage in both conversion paths: extend TestChatCompletionsRequestToResponsesRequestPreservesQwenThinkingBudget in relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go (lines 42-59) with a Qwen request using ThinkingBudget set to JSON 0 and assert the converted value remains 0; add the corresponding Responses-to-Chat zero-value case in relaykit/relayconvert/internal/oai_responses/to_oai_chat_req_test.go (lines 59-72), preserving the existing 128 cases.Source: Coding guidelines
relaykit/dto/openai_request_zero_value_test.go (1)
85-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover provider-prefixed Qwen model names directly.
Qwen/Qwen3-...matches theHasPrefix("qwen")branch, so the/qwenand/qwqmatching paths are not actually tested. Add cases such asprovider/qwen-plusandprovider/qwq-32bto protect the model-aware serialization contract.🤖 Prompt for 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. In `@relaykit/dto/openai_request_zero_value_test.go` around lines 85 - 101, Extend TestIsQwenThinkingBudgetModel with provider-prefixed positive cases such as provider/qwen-plus and provider/qwq-32b, ensuring the slash-based matching paths are exercised while preserving the existing expectations.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@relaykit/dto/openai_request.go`:
- Around line 9-10: Replace relaykit/relayconvert/kitutil JSON usage with the
common/json.go wrappers in relaykit/dto/openai_request.go,
relaykit/relayconvert/internal/oai_chat/to_oai_responses_req.go, and
relaykit/relayconvert/internal/oai_responses/to_oai_chat_req.go. Update the
affected imports and calls to use common.Marshal, common.Unmarshal, or the
corresponding common helpers while preserving existing request-conversion
behavior.
---
Nitpick comments:
In `@relaykit/dto/openai_request_zero_value_test.go`:
- Around line 85-101: Extend TestIsQwenThinkingBudgetModel with
provider-prefixed positive cases such as provider/qwen-plus and
provider/qwq-32b, ensuring the slash-based matching paths are exercised while
preserving the existing expectations.
In `@relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go`:
- Around line 42-59: Add explicit zero-budget test coverage in both conversion
paths: extend
TestChatCompletionsRequestToResponsesRequestPreservesQwenThinkingBudget in
relaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.go (lines
42-59) with a Qwen request using ThinkingBudget set to JSON 0 and assert the
converted value remains 0; add the corresponding Responses-to-Chat zero-value
case in relaykit/relayconvert/internal/oai_responses/to_oai_chat_req_test.go
(lines 59-72), preserving the existing 128 cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3fb14c5-388b-4ff6-acff-836682ebc43e
📒 Files selected for processing (9)
relay/channel/ali/adaptor.gorelay/channel/ali/adaptor_test.gorelay/channel/ali/text.gorelaykit/dto/openai_request.gorelaykit/dto/openai_request_zero_value_test.gorelaykit/relayconvert/internal/oai_chat/to_oai_responses_req.gorelaykit/relayconvert/internal/oai_chat/to_oai_responses_req_test.gorelaykit/relayconvert/internal/oai_responses/to_oai_chat_req.gorelaykit/relayconvert/internal/oai_responses/to_oai_chat_req_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- relay/channel/ali/adaptor_test.go
- relay/channel/ali/text.go
- relay/channel/ali/adaptor.go
* fix: preserve qwen thinking budget * test: address qwen thinking budget review comments * chore: remove unreachable adaptor code * test: cover zero Qwen thinking budgets
* fix: preserve qwen thinking budget * test: address qwen thinking budget review comments * chore: remove unreachable adaptor code * test: cover zero Qwen thinking budgets
* fix: preserve qwen thinking budget * test: address qwen thinking budget review comments * chore: remove unreachable adaptor code * test: cover zero Qwen thinking budgets
* fix: preserve qwen thinking budget * test: address qwen thinking budget review comments * chore: remove unreachable adaptor code * test: cover zero Qwen thinking budgets
* v1.0.0-rc.24: (117 commits) CI: enhance release synchronization workflow with optional file syncing fix: 修复兑换码额度精度损失 (QuantumNous#6685) feat(rate-limit): add user critical rate limit middleware for access token and aff transfer routes fix: test Claude/Gemini endpoints with native request format (QuantumNous#6698) feat(channels): refine fetched model categorization (QuantumNous#6632) Merge commit from fork refactor(relay): move replay metadata onto request bodies fix(relay): set Request.GetBody so the HTTP/2 transport can transparently retry after an upstream stream reset (QuantumNous#6249) Feat/auto group (QuantumNous#6590) fix(aws): cancel Bedrock requests on client disconnect (QuantumNous#6589) fix(billing): harden tiered retry group-switch billing (QuantumNous#6570) fix(billing): settle tiered retries with final group (QuantumNous#6518) feat: deepseek responses api (QuantumNous#6562) style: use text-sm for public header nav links to match other nav components (QuantumNous#6557) fix(oauth): stop treating a foreign window.opener as a bind flow (QuantumNous#6425) fix(relay): preserve multipart image edits for New API channels (QuantumNous#6559) feat(logs): expose stream status to log owners (QuantumNous#6558) feat: support zstd request decompression (QuantumNous#6545) fix: preserve Qwen thinking_budget passthrough (QuantumNous#5836) feat(oidc): 支持自定义 OIDC 登录显示名称 (QuantumNous#6012) ... # Conflicts: # service/text_quota.go # web/src/features/models/components/drawers/model-mutate-drawer.tsx # web/src/features/pricing/components/model-details.tsx # web/src/features/pricing/lib/price.ts
* fix: preserve qwen thinking budget * test: address qwen thinking budget review comments * chore: remove unreachable adaptor code * test: cover zero Qwen thinking budgets
* fix: preserve qwen thinking budget * test: address qwen thinking budget review comments * chore: remove unreachable adaptor code * test: cover zero Qwen thinking budgets
背景
enable_thinking和thinking_budget。GeneralOpenAIRequest,再重新序列化后转发。GeneralOpenAIRequest之前没有thinking_budget字段,导致 Qwen 请求中的该参数在转发前被丢弃。修复内容
thinking_budget字段,类型使用json.RawMessage,保留显式0等值。thinking_budget,避免影响其它不支持该参数的模型。thinking_budget保留。测试
go test ./dto ./relay/channel/ali ./service/relayconvertgo test ./...:根包 setup 因缺少web/classic/dist嵌入目录失败;其余可运行 Go 包通过。Summary by CodeRabbit
New Features
enable_thinking,thinking_budget) and preserved them across compatible OpenAI/Responses, Chat Completions, and ALI conversions.Bug Fixes
thinking_budgetis now omitted for non-Qwen thinking-budget models to avoid sending incompatible payload fields.Tests
thinking_budgetbehavior across conversions.