fix(core): avoid required tools in DashScope thinking

[hogeheer499-commits]

What this PR does

When a DashScope request has thinking enabled, it no longer sends a forced tool_choice: "required". Forced tool selection stays unchanged when thinking is off and for non-DashScope providers.

Why it's needed

Structured side queries such as memory recall use forced tool selection. DashScope rejects that setting while thinking is active, so users who enable thinking without also setting the separate mandatory-thinking option receive a 400 response. The previous safeguard only recognized the mandatory-thinking option instead of the final request state.

Reviewer Test Plan

How to verify

Configure a DashScope model with thinking enabled but without mandatory thinking, then issue a structured side query using forced function calling. The outgoing request should keep thinking enabled and omit forced tool selection. Repeat without thinking enabled and confirm that forced tool selection is still present.

Evidence (Before & After)

Before, the reproduced wire shape contained { enable_thinking: true, tool_choice: "required" }, which DashScope rejects. After, the focused pipeline suite confirms that the same thinking request omits tool_choice, while the non-thinking control keeps it. The focused suite passes 122/122 tests.

Tested on

OS	Status
🍏 macOS	⚠️
🪟 Windows	⚠️
🐧 Linux	✅

Environment (optional)

Node.js 22.22.1 and npm 10.9.4.

Risk & Scope

• Main risk or tradeoff: accidentally removing forced tool selection from requests where it remains valid; regression coverage protects the thinking and non-thinking branches.
• Not validated / out of scope: no live DashScope request was sent; runtime learning for other OpenAI-compatible providers remains separate follow-up work.
• Breaking changes / migration notes: none.

Linked Issues

Fixes #7659

中文说明

本 PR 的改动

当 DashScope 请求已启用思考时，不再发送强制的 tool_choice: "required"。未启用思考时，以及非 DashScope 提供商的行为保持不变。

为什么需要此改动

记忆召回等结构化侧查询会使用强制工具选择。DashScope 在启用思考时会拒绝该设置，因此只启用思考、但没有另外设置强制思考选项的用户会收到 400 响应。之前的保护逻辑只识别强制思考选项，而没有检查最终请求的实际状态。

审查者测试计划

如何验证

配置一个启用思考但未启用强制思考的 DashScope 模型，然后使用强制函数调用发起结构化侧查询。发出的请求应保留思考设置，并省略强制工具选择。随后在未启用思考的情况下重复测试，确认强制工具选择仍然存在。

证据（改动前后）

改动前，复现出的请求包含 { enable_thinking: true, tool_choice: "required" }，DashScope 会拒绝该组合。改动后，针对性的 pipeline 测试确认相同的思考请求会省略 tool_choice，而非思考对照请求仍然保留该字段。针对性测试共 122 项，全部通过。

测试平台

操作系统	状态
🍏 macOS	⚠️
🪟 Windows	⚠️
🐧 Linux	✅

环境（可选）

Node.js 22.22.1 和 npm 10.9.4。

风险与范围

• 主要风险或权衡：可能误删仍然有效的强制工具选择；回归测试覆盖了思考与非思考两个分支。
• 未验证或范围之外：没有发送真实的 DashScope 请求；其他 OpenAI 兼容提供商的运行时学习仍属于后续独立工作。
• 破坏性变更或迁移说明：无。

关联 Issue

Fixes #7659

[qwen-code-ci-bot]

✅ Qwen Triage finished — view run. See the stage comments above for the result.

✅ Qwen Triage 已完成 —— 查看运行。结果见上方各阶段评论。

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓

Problem: observed bug with solid evidence. Issue #7659 documents the exact 400 rejection from DashScope when enable_thinking: true is on the wire without thinkingMandatory, and a maintainer confirmed the root cause in the pipeline. This is not theoretical — users hit this on every memory recall side query with thinking-mode models.

Direction: aligned. The fix direction was explicitly endorsed by a maintainer in #7659, who provided the exact guard condition this PR implements. CHANGELOG has no direct reference, but DashScope provider compatibility is squarely within scope.

Size: 14 production lines (pipeline.ts: +10/−4), 22 test lines (pipeline.test.ts: +22). Well under any threshold.

Approach: the scope feels right — move the existing tool_choice stripping out of the if (thinkingMandatory) block and broaden its condition to also check the wire state. Two files, one logical change, two regression tests. No unrelated edits, no scope creep. This is exactly the minimal fix the issue calls for.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓

问题：已观测到的 bug，有充分证据。Issue #7659 记录了 DashScope 在 wire 上有 enable_thinking: true 但没有 thinkingMandatory 时返回 400 的确切错误，维护者已在 pipeline 中确认根因。这不是理论性问题——用户在使用思考模式模型时，每次记忆召回侧查询都会遇到此错误。

方向：对齐。修复方向已在 #7659 中获得维护者明确认可，维护者提供了与本 PR 实现完全一致的守卫条件。CHANGELOG 无直接引用，但 DashScope 提供商兼容性完全在范围内。

规模：14 行生产代码（pipeline.ts: +10/−4），22 行测试代码（pipeline.test.ts: +22）。远低于任何阈值。

方案：范围合理——将现有的 tool_choice 移除逻辑从 if (thinkingMandatory) 块中移出，扩展条件以检查 wire 状态。两个文件，一个逻辑变更，两个回归测试。无无关编辑，无范围蔓延。这正是 issue 所需的最小修复。

进入代码审查 🔍

— Qwen Code · qwen3.8-max-preview

_{Reviewed at c30de11fab222fce0f285cd4b0c55faea9eb9c08 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code Review

Independent proposal: given the problem (DashScope tool_choice: 'required' stripping is gated solely on thinkingMandatory, missing the enable_thinking: true wire state), I would move the stripping out of the if (thinkingMandatory) block and give it its own guard: isDashScope && typed['tool_choice'] === 'required' && (thinkingMandatory || typed['enable_thinking'] === true). Add tests for both the thinking-on-wire and no-thinking branches.

Comparison: the PR does exactly this. The tool_choice stripping is relocated from inside the thinkingMandatory block (where it only fired for config-level mandatory thinking) to a standalone guard that also checks typed['enable_thinking'] === true on the wire. The const typed re-declaration at the outer scope is clean — the inner one is block-scoped to if (thinkingMandatory), no shadowing conflict.

The two new test cases slot into the existing it.each parameterized suite and cover the right branches:

• enable_thinking: true via extraBody + includeThoughts: true (thinking stays on) → tool_choice stripped ✓
• No extraBody + includeThoughts: true (no thinking) → tool_choice: 'required' preserved ✓

The existing test "still force-disable hybrid models that only declare extra_body.enable_thinking" (includeThoughts: false, expectedToolChoice: 'required') still passes because the pipeline force-sets enable_thinking: false on the wire for side queries, so the new guard's === true check doesn't fire. No regression.

No correctness bugs, no security concerns, no convention violations. The change is the exact fix the maintainer prescribed in #7659.

Testing

CI check results for c30de11:

Check	Conclusion
Test (ubuntu-latest, Node 22.x)	✅ success
precheck-pr / precheck	✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x)	✅ success
Classify PR	✅ success
Test (windows-latest, Node 22.x)	⏭️ skipped
Test (macos-latest, Node 22.x)	⏭️ skipped

No failures. The ubuntu test suite (which includes the pipeline tests) passes.

Real-scenario testing: N/A — this is a provider-level wire format change (request body field stripping), not a TUI-visible behavior. The unit tests directly assert the outgoing request shape.

中文说明

代码审查

独立方案： 针对此问题（DashScope 的 tool_choice: 'required' 移除仅以 thinkingMandatory 为条件，遗漏了 wire 上的 enable_thinking: true 状态），我会将移除逻辑从 if (thinkingMandatory) 块中移出，给予独立守卫：isDashScope && typed['tool_choice'] === 'required' && (thinkingMandatory || typed['enable_thinking'] === true)。并为 thinking-on-wire 和 no-thinking 两个分支添加测试。

对比： PR 完全按照此方案实现。tool_choice 移除从 thinkingMandatory 块内（仅对配置级强制思考生效）重定位到独立守卫，同时检查 wire 上的 typed['enable_thinking'] === true。外层作用域的 const typed 重新声明是干净的——内部的 typed 是 if (thinkingMandatory) 的块级作用域，无遮蔽冲突。

两个新测试用例插入现有的 it.each 参数化测试套件，覆盖了正确的分支：

• 通过 extraBody 设置 enable_thinking: true + includeThoughts: true（思考保持开启）→ tool_choice 被移除 ✓
• 无 extraBody + includeThoughts: true（无思考）→ tool_choice: 'required' 保留 ✓

现有测试"still force-disable hybrid models that only declare extra_body.enable_thinking"（includeThoughts: false，expectedToolChoice: 'required'）仍然通过，因为 pipeline 对侧查询强制设置 enable_thinking: false，新守卫的 === true 检查不会触发。无回归。

无正确性 bug，无安全隐患，无规范违反。此变更正是维护者在 #7659 中开出的修复方案。

测试

c30de11 的 CI 检查结果：ubuntu 测试套件（含 pipeline 测试）通过，precheck 通过，无失败。

真实场景测试：不适用——这是提供商级别的 wire 格式变更（请求体字段移除），非 TUI 可见行为。单元测试直接断言了发出的请求形状。

— Qwen Code · qwen3.8-max-preview

_{Reviewed at c30de11fab222fce0f285cd4b0c55faea9eb9c08 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 5/5 — clean across every stage; the exact fix the maintainer prescribed, with regression tests for both branches.

This is a textbook minimal bugfix. The problem is real and well-documented (#7659, maintainer-confirmed), the fix is the one-line guard broadening the maintainer wrote out in the issue, and the tests cover the thinking-on-wire and no-thinking paths. Fourteen production lines, no unrelated edits, no new abstractions. If I had to maintain this in six months, I'd thank the author — the guard reads naturally and the comment explains the DashScope constraint.

The only thing I checked carefully was whether the relocated guard could regress the existing "force-disable hybrid models" test (where enable_thinking starts true in extraBody but the pipeline overwrites it to false for side queries). It doesn't — the === true strict check on the wire state is exactly right, and that test still expects tool_choice: 'required'.

LGTM, approving. ✅

中文说明

置信度：5/5 —— 各阶段均无问题；正是维护者开出的修复方案，两个分支均有回归测试。

这是一个教科书式的最小 bugfix。问题真实且有充分记录（#7659，维护者已确认），修复方案就是维护者在 issue 中写出的那一行守卫扩展，测试覆盖了 thinking-on-wire 和 no-thinking 两条路径。14 行生产代码，无无关编辑，无新抽象。如果半年后维护这段代码，我会感谢作者——守卫条件读起来自然，注释解释了 DashScope 的约束。

我唯一仔细检查的是重定位后的守卫是否会使现有的"force-disable hybrid models"测试回归（enable_thinking 在 extraBody 中初始为 true，但 pipeline 对侧查询覆写为 false）。不会——对 wire 状态的 === true 严格检查完全正确，该测试仍期望 tool_choice: 'required'。

LGTM，批准合并。✅

— Qwen Code · qwen3.8-max-preview

_{Reviewed at c30de11fab222fce0f285cd4b0c55faea9eb9c08 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[qwen-code-ci-bot]

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

[wenshao]

Code Review — fix(core): avoid required tools in DashScope thinking

Focused, well-scoped fix for #7659. I traced it against the actual base (origin/main) and the DashScope provider path. Verdict: merge-ready — correct, a strict superset of the prior behavior (no regression), and covered by two targeted tests. One documented limitation below is genuinely out of scope.

What it does

On origin/main the tool_choice: "required" strip lived inside if (thinkingMandatory), so it only fired for models explicitly flagged mandatory-thinking. Memory-recall side queries (functionCallingConfig.mode = ANY → tool_choice: "required") therefore still hit DashScope's HTTP 400 "tool_choice … does not support being set to required … in thinking mode" for any ordinary thinking-enabled model. The PR hoists the strip out of that block and re-gates it on the real wire state:

if (isDashScope && typed['tool_choice'] === 'required' &&
    (thinkingMandatory || typed['enable_thinking'] === true)) {
  delete typed['tool_choice'];
}

Correctness ✅

• Strict superset, no regression. The old trigger (thinkingMandatory && isDashScope && required) is fully retained via the thinkingMandatory disjunct; the new enable_thinking === true disjunct only adds coverage. Moving the block after the thinkingMandatory cleanup preserves ordering (enable_thinking cleanup → strip).
• Right wire signal. DashScope enables thinking on the wire via top-level enable_thinking: true (injected by dashscope.ts when an effort is set, and by provider-config.ts presets), so typed['enable_thinking'] === true is the correct gate.
• No over-removal. DashScope always rejects required while thinking is on, so stripping whenever enable_thinking === true can never break an otherwise-valid request.
• reasoningDisabled interplay is right: when reasoning is off, enable_thinking is set to false, so required is correctly preserved (covered by the "still force-disable…" case).

Test coverage ✅

The two new it.each rows are the right pair: the positive branch (enable_thinking:true, non-mandatory model → tool_choice dropped) and the negative control (no enable_thinking → required preserved). Both trace correctly through the pipeline.

Limitations / follow-ups (non-blocking)

• Implicit/default thinking isn't covered. The gate keys off the literal enable_thinking === true. A DashScope hybrid model that thinks by default with the field absent would still send required and 400. This is exactly the runtime-learning path (à la fix(core): retry requests when providers require thinking #7534) the issue lists as the more robust alternative, which the PR explicitly defers — reasonable, since qwen-code emits the field whenever thinking is configured, so the reported case is covered. Worth a code comment noting the gate is request-state-based, not capability-based.
• Only the 'required' string is handled, not object-form tool_choice (which DashScope also rejects in thinking mode). Fine in practice — this pipeline only ever emits 'required'/'none', never an object.

Minor nits

• There are now three const typed = providerRequest as unknown as Record<…> in buildRequest (the two block-scoped ones plus the new function-scope one at the tail). Not a lint error (no-shadow isn't enabled for packages/core), but hoisting a single declaration would read cleaner. Optional — matches existing style.

中文小结

• 改动：原先 tool_choice: "required" 的剥离逻辑嵌在 if (thinkingMandatory) 内，只对显式标记强制思考的模型生效；普通的思考模型（如记忆召回侧查询）仍会因 required 触发 DashScope 400。本 PR 把剥离逻辑上移，改为按真实请求状态 (thinkingMandatory || enable_thinking === true) 判定。
• 正确性：是旧行为的严格超集，无回归；enable_thinking === true 正是 DashScope 判定思考开启的线路信号；思考开启时 DashScope 必拒 required，因此不会误删有效请求。两个新增测试用例覆盖到位。
• 限制（非阻断）：仅覆盖显式 enable_thinking: true，不覆盖“字段缺省但服务端默认思考”的情形（属 fix(core): retry requests when providers require thinking #7534 式运行时学习的后续工作，PR 已明示为范围外）；仅处理字符串 'required'，但本管线本就不产生 object 形式，无影响。
• 小建议：buildRequest 内现有三处 const typed，可合并为一处以提升可读性（非 lint 错误，可选）。

结论：可合并。

[doudouOUC]

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

[gwinthis]

Review + live DashScope API verification report (real key, real build, tmux)

Verdict: the fix is correct, and I verified every link of its causal chain against the live DashScope API — the premise (400), the transformation's validity (200 + tool still called), and the control (required preserved without thinking).

Code review reasoning

1. The bug was a condition-scope error, and the fix targets the final wire state. The old guard lived inside if (thinkingMandatory), so a request that enabled thinking via extra_body.enable_thinking (without the separate mandatory-thinking option) kept tool_choice: "required" and got a guaranteed 400. The new guard runs on the assembled request and keys off thinkingMandatory || enable_thinking === true — the state that actually reaches the provider.
2. The top-level-only check is consistent with the codebase's own routing convention. This pipeline deliberately sends DashScope a top-level enable_thinking and routes chat_template_kwargs.enable_thinking to non-DashScope servers (vLLM/SGLang) only (see the thinking-disable branch a few lines above). So a DashScope request carrying thinking exclusively via chat_template_kwargs is outside the codebase's own wire contract — no gap.
3. Degradation is graceful. Removing tool_choice downgrades forced tool selection to best-effort. Live test B (below) shows the model still emits the tool call — so structured side queries degrade from "guaranteed 400" to "almost always works", strictly better.

Verification evidence (Linux, commit c30de11)

Live DashScope API (dashscope.aliyuncs.com/compatible-mode/v1, qwen-plus, real key):

Case	Request	Response	Meaning
A (old wire shape)	enable_thinking: true + tool_choice: "required"	400 InternalError.Algo.InvalidParameter: The tool_choice parameter does not support being set to required or object in thinking mode	Premise confirmed verbatim
B (new wire shape)	enable_thinking: true, no tool_choice	200, response contains a save_fact tool call	The fix's output is valid AND still yields the tool call
C (control)	enable_thinking: false + tool_choice: "required"	200 with tool call	Preserving required without thinking is correct

Unit: pipeline.test.ts 122/122 passed, including the two new wire-shape cases (thinking→tool_choice removed; no-thinking→required preserved).

tmux E2E (full build + bundle, isolated HOME, real DashScope provider): interactive session on qwen-plus executed a shell tool call end-to-end in Auto mode (whose permission classifier issues structured side queries) with no API errors.

Note

The linked issue's failure mode (thinking enabled via extra_body only) is now covered; if DashScope ever adds more tool_choice restrictions (e.g. object form, which the error message also mentions), the guard currently only strips the string "required" — worth keeping in mind, not blocking.

[qwen-code-ci-bot]

Released in v0.21.1.

[yiliang114]

⚠️ Failed to process this request. Please re-mention the bot to retry.