fix(core): preserve disabled reasoning effort - #7541
Conversation
6257299 to
572e823
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
3c75468 to
496dc8f
Compare
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with a clear failure mode — configuring Direction: aligned. OpenAI-compatible provider support is core to qwen-code, and Size: 2 production lines changed in Approach: the scope is exactly right — one condition guard that preserves Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，失败模式清晰——为 OpenAI 兼容的 GPT-5 模型配置 方向：对齐。OpenAI 兼容提供商支持是 qwen-code 的核心功能，而 规模： 方案：范围恰好——一个条件守卫，保留 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
496dc8f to
e959a85
Compare
Code ReviewIndependent proposal: given the problem (stripping Comparison with the diff: the PR does exactly this. The one-line condition change at line 900 of No correctness bugs, security issues, or regressions found. The change follows the existing code style and the comment above the strip block already explains the intent. No over-abstraction, no duplication, no unrelated edits. TestingUnit tests: 112/112 pass on the PR branch (including the new regression test Real-scenario testing (mock OpenAI server): Set up a mock OpenAI-compatible server on Before (installed qwen v0.20.1): After (PR code via Both builds preserve 中文说明代码审查独立方案： 鉴于问题（剥离 与 diff 的比较： PR 完全这样做了。 未发现正确性 bug、安全问题或回归。更改遵循现有代码风格。 测试单元测试： PR 分支上 112/112 通过（包括新的回归测试）。类型检查通过。 真实场景测试（模拟 OpenAI 服务器）： 搭建了模拟服务器，配置了 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean, minimal fix for a real provider-compatibility bug; would merge without hesitation. This is exactly the kind of PR I like to see: one line of production code that fixes a concrete problem, backed by a focused regression test. The author correctly identified that The test is well-placed alongside the existing reasoning-strip tests, uses the same mock patterns, and directly exercises the side-query code path. 112/112 tests pass, typecheck is clean. No unrelated changes, no scope creep, no over-engineering. 中文说明置信度：5/5 — 干净、最小的修复，解决了真实的提供商兼容性问题；毫不犹豫地合并。 这是一个理想的 PR：一行生产代码修复了一个具体问题，配有聚焦的回归测试。作者正确识别了 测试放置得当，使用了相同的 mock 模式，直接验证了侧查询代码路径。112/112 测试通过，类型检查通过。无无关改动，无范围蔓延，无过度工程。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
| expect(apiCall.reasoning).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should preserve reasoning_effort none when thinking is disabled', async () => { |
There was a problem hiding this comment.
[Suggestion] The new condition typed['reasoning_effort'] !== 'none' has two branches (preserve when 'none', strip otherwise), but only the preserve branch is tested. No test verifies that reasoning_effort with a non-'none' value (e.g., 'high') is still stripped when thinking is disabled. — Failure scenario: a future change accidentally widens the preservation condition (e.g., removes the !== 'none' guard), causing non-none reasoning_effort values to leak through when thinking is disabled, contradicting the disable signal and adding unwanted reasoning latency/cost.
| it('should preserve reasoning_effort none when thinking is disabled', async () => { | |
| it('should preserve reasoning_effort none when thinking is disabled', async () => { | |
| mockContentGeneratorConfig = { | |
| ...mockContentGeneratorConfig, | |
| samplingParams: { reasoning_effort: 'none' }, | |
| } as ContentGeneratorConfig; | |
| mockConfig = { | |
| ...mockConfig, | |
| contentGeneratorConfig: mockContentGeneratorConfig, | |
| }; | |
| pipeline = new ContentGenerationPipeline(mockConfig); | |
| const request: GenerateContentParameters = { | |
| model: 'gpt-5', | |
| contents: [{ parts: [{ text: 'Classify action' }], role: 'user' }], | |
| config: { thinkingConfig: { includeThoughts: false } }, | |
| }; | |
| (mockConverter.convertGeminiRequestToOpenAI as Mock).mockReturnValue([ | |
| { role: 'user', content: 'Classify action' }, | |
| ]); | |
| (mockConverter.convertOpenAIResponseToGemini as Mock).mockReturnValue( | |
| new GenerateContentResponse(), | |
| ); | |
| (mockClient.chat.completions.create as Mock).mockResolvedValue({ | |
| id: 'response-id', | |
| choices: [{ message: { content: 'safe' }, finish_reason: 'stop' }], | |
| } as OpenAI.Chat.ChatCompletion); | |
| await pipeline.execute(request, 'side-query:permission-classifier'); | |
| const apiCall = (mockClient.chat.completions.create as Mock).mock | |
| .calls[0][0]; | |
| expect(apiCall.reasoning_effort).toBe('none'); | |
| }); | |
| it('should still strip non-none reasoning_effort when thinking is disabled', async () => { | |
| mockContentGeneratorConfig = { | |
| ...mockContentGeneratorConfig, | |
| samplingParams: { reasoning_effort: 'high' }, | |
| } as ContentGeneratorConfig; | |
| mockConfig = { | |
| ...mockConfig, | |
| contentGeneratorConfig: mockContentGeneratorConfig, | |
| }; | |
| pipeline = new ContentGenerationPipeline(mockConfig); | |
| const request: GenerateContentParameters = { | |
| model: 'gpt-5', | |
| contents: [{ parts: [{ text: 'Classify action' }], role: 'user' }], | |
| config: { thinkingConfig: { includeThoughts: false } }, | |
| }; | |
| (mockConverter.convertGeminiRequestToOpenAI as Mock).mockReturnValue([ | |
| { role: 'user', content: 'Classify action' }, | |
| ]); | |
| (mockConverter.convertOpenAIResponseToGemini as Mock).mockReturnValue( | |
| new GenerateContentResponse(), | |
| ); | |
| (mockClient.chat.completions.create as Mock).mockResolvedValue({ | |
| id: 'response-id', | |
| choices: [{ message: { content: 'safe' }, finish_reason: 'stop' }], | |
| } as OpenAI.Chat.ChatCompletion); | |
| await pipeline.execute(request, 'side-query:permission-classifier'); | |
| const apiCall = (mockClient.chat.completions.create as Mock).mock | |
| .calls[0][0]; | |
| expect(apiCall.reasoning_effort).toBeUndefined(); | |
| }); |
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Minimal, well-scoped fix. Preserving only 'none' is the right granularity — other effort values ('low'/'medium'/'high') still get stripped as before. Test precisely covers the side-query + thinking-disabled scenario.
| delete typed['reasoning']; | ||
| } | ||
| if ('reasoning_effort' in typed) { | ||
| if ('reasoning_effort' in typed && typed['reasoning_effort'] !== 'none') { |
There was a problem hiding this comment.
Nit: the comparison is case-sensitive (!== 'none'). OpenAI only uses lowercase, but a user config with "None" would be silently stripped. Consider String(typed['reasoning_effort']).toLowerCase() !== 'none' for robustness. Non-blocking.
| const apiCall = (mockClient.chat.completions.create as Mock).mock | ||
| .calls[0][0]; | ||
| expect(apiCall.reasoning_effort).toBe('none'); | ||
| }); |
There was a problem hiding this comment.
Suggest adding a reverse test: reasoning_effort: 'high' with thinking disabled should still be stripped. The existing test at L829 only covers the nested reasoning object removal, not the top-level string. This would prevent a future regression where the guard is accidentally widened to preserve all values.
Review & Local Verification Report代码审查设计评价：精准最小修复。 本 PR 仅改动 1 行生产代码 + 1 个测试用例，解决了一个语义微妙的问题： 问题：当 修复： // Before: 无条件删除
if ('reasoning_effort' in typed) { delete typed['reasoning_effort']; }
// After: 保留 'none'（显式禁用信号）
if ('reasoning_effort' in typed && typed['reasoning_effort'] !== 'none') { delete typed['reasoning_effort']; }语义分析：
测试覆盖： 新增测试验证 本地验证由于多 agent 并行工作目录冲突，未能在本地独立运行测试。但基于代码分析：
结论LGTM。 最小改动解决真实问题，语义正确，不引入副作用。典型的"less is more"修复。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Two non-blocking follow-ups inline. Nothing here changes my approval.
| delete typed['reasoning']; | ||
| } | ||
| if ('reasoning_effort' in typed) { | ||
| if ('reasoning_effort' in typed && typed['reasoning_effort'] !== 'none') { |
There was a problem hiding this comment.
One thing worth noting: reasoningDisabled also fires on config-level reasoning: false, not just side queries. So a user who sets both reasoning: false and samplingParams.reasoning_effort: 'none' now ships the literal none on every request. On providers that reject that literal (DeepSeek's chat API only documents high/max), that combo goes from silently working to a 400. Sending the user's explicit value is still the right call — but the Risk section only mentions side queries, so might be worth calling this out there. Non-blocking.
| delete typed['reasoning']; | ||
| } | ||
| if ('reasoning_effort' in typed) { | ||
| if ('reasoning_effort' in typed && typed['reasoning_effort'] !== 'none') { |
There was a problem hiding this comment.
The block comment above still says "we strip both shapes here", which is no longer accurate for the flat shape. And the 'none' exemption is exactly the kind of guard someone will simplify away later — the gateway constraint (function tools on GPT-5 rejected unless reasoning is explicitly disabled) isn't inferable from the code. Might be worth a one-line why next to the condition. Non-blocking.
5 participants
What this PR does
Preserves an explicitly configured
reasoning_effort: "none"when a side query disables thinking. Other reasoning effort values and nested reasoning configuration continue to be removed as before.Why it's needed
Structured side queries use a forced function call to produce schema-constrained output. Some OpenAI-compatible gateways reject function tools for GPT-5 models unless reasoning is explicitly disabled with
reasoning_effort: "none". The current pipeline removes that explicit opt-out, causing Auto Mode classification and other structured side queries to fail with a 400 response.Reviewer Test Plan
How to verify
Configure an OpenAI-compatible GPT-5 model with
samplingParams.reasoning_effortset tonone, then trigger a structured side query with thinking disabled. Confirm the outbound request retainsreasoning_effort: "none". Also confirm non-disabled reasoning effort values are still removed when thinking is disabled.Evidence (Before & After)
Before: the gateway rejected structured side queries because the explicit
nonevalue was removed before the request was sent.After: the same Auto Mode classifier request succeeds, and the focused pipeline suite passes all 112 tests including the new regression case.
Tested on
Environment (optional)
Node.js 26.5.0; focused core unit tests, formatting, lint, and core typecheck.
Risk & Scope
nonevalue differently will now receive the user-configured value instead of having it removed, matching the explicit configuration.Linked Issues
N/A
中文说明
此 PR 的作用
当侧查询禁用思考时，保留显式配置的
reasoning_effort: "none"。其他推理强度值和嵌套推理配置仍会像以前一样被移除。为什么需要它
结构化侧查询使用强制函数调用来生成符合 schema 的输出。某些 OpenAI 兼容网关会拒绝 GPT-5 模型的函数工具调用，除非通过
reasoning_effort: "none"显式禁用推理。当前管道会删除这个显式关闭值，导致 Auto Mode 分类和其他结构化侧查询返回 400 错误。Reviewer Test Plan
如何验证
配置一个 OpenAI 兼容的 GPT-5 模型，将
samplingParams.reasoning_effort设置为none，然后触发一个禁用思考的结构化侧查询。确认发出的请求仍包含reasoning_effort: "none"。同时确认在禁用思考时，其他非禁用的推理强度值仍会被移除。证据（修改前后）
修改前：由于显式的
none值在发送请求前被删除，网关拒绝结构化侧查询。修改后：同一个 Auto Mode 分类器请求成功，聚焦的管道测试套件全部 112 个测试通过，包括新的回归测试。
测试平台
环境（可选）
Node.js 26.5.0；已运行聚焦的 core 单元测试、格式检查、lint 和 core 类型检查。
风险与范围
none有不同解释，它现在会收到用户显式配置的值，而不是删除该值；这与用户配置一致。关联 Issue
无