Skip to content

fix(core): harden Qwen 3.8 reasoning effort wire shape - #8488

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:fix/qwen38-reasoning-effort-followup
Aug 4, 2026
Merged

fix(core): harden Qwen 3.8 reasoning effort wire shape#8488
wenshao merged 5 commits into
QwenLM:mainfrom
wenshao:fix/qwen38-reasoning-effort-followup

Conversation

@wenshao

@wenshao wenshao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Follow-up to #8472 (Qwen 3.8 reasoning effort), addressing the review findings posted after merge. Four fixes to the DashScope wire shape:

  1. Competing thinking knobs: when an effort tier ships as reasoning_effort, the request now drops enable_thinking and thinking_budget after the user extra_body merge. Previously the bundled Token Plan preset made the flagship preview model ship reasoning_effort and enable_thinking: true together — the exact "two competing knobs" shape the nested-reasoning strip exists to prevent — and DashScope rejects reasoning_effort combined with thinking_budget.
  2. Model-family gate on the tool_choice strip: the clause added by feat(core): support Qwen 3.8 reasoning effort #8472 (drop tool_choice: 'required' while thinking is enabled) is now gated to qwen-family wire models, mirroring the disable path's gate. On non-qwen models sharing the DashScope endpoint, reasoning_effort is an opaque sampling override, not a thinking switch, and dropping forced tool selection there degraded structured side queries (permission classifier, compaction, title generation).
  3. Snapshot/alias coverage: the tiered family is now prefix-matched (qwen3.8-max*), consistent with the family match in the modality defaults. Dated snapshots and -latest aliases previously fell back to the boolean enable_thinking collapse and silently lost the selected tier.
  4. Observability & docs: the tool_choice strip now logs a debug line with the model and triggering condition; the effort-config JSDoc and the request-level-override copy comment are restored; the provider docs row now reflects the enforced thinking_budget drop and the prefix-matched family.

The deliberate decision on the asymmetric request-level override (a request-level reasoning_effort reaching a legacy qwen model that also gets enable_thinking from the effort config): the explicit tier wins and the boolean is dropped — the wire never carries both knobs.

Why it's needed

The preset shape in #8472 meant every Token Plan user who selected an /effort tier on the bundled preview model shipped two competing thinking knobs, with no live verification that the preview model accepts the pair. The ungated tool_choice strip was a behavioral regression for non-qwen models routed through the same endpoint, and the exact-match family gate silently dropped tiers for snapshot/alias model ids — reintroducing the exact bug #2876 describes.

Reviewer Test Plan

How to verify

All changes are covered by unit tests; the seven new regression tests fail on the pre-fix code (verified by reverting the production files to the merged commit and re-running) and pass with the fix:

cd packages/core && npx vitest run src/core/openaiContentGenerator
  • Preset shape regression: qwen3.8-max-preview + preset-style extra_body: { enable_thinking: true } + effort tier → wire carries reasoning_effort alone (plus the vision-branch and legacy-model variants of the same drop).
  • Non-qwen counter-case: glm-5.2 + user extra_body: { reasoning_effort: 'high' } in forced-tool mode keeps tool_choice: 'required' (was dropped before this fix).
  • Snapshot ids: qwen3.8-max-2026-01-15 / qwen3.8-max-latest now pass the tier through as reasoning_effort.
  • Disable paths: config-level reasoning: false and per-request includeThoughts: false both strip the tier for qwen3.8-max and keep tool_choice: 'required'.
  • thinking_budget: dropped when a tier ships; kept alongside enable_thinking on legacy hybrid models (a valid pair).

Evidence (Before & After)

N/A (wire-shape change, not user-visible; unit-test evidence above).

Tested on

OS Status
🍏 macOS
🪟 Windows N/A
🐧 Linux N/A

Environment (optional)

Unit tests only (vitest); full npm run typecheck and targeted eslint clean.

Risk & Scope

  • Main risk or tradeoff: a user who explicitly set both enable_thinking and reasoning_effort via extra_body now ships only reasoning_effort — that combination is contradictory (and DashScope-rejected for thinking_budget), so the tier wins.
  • Not validated / out of scope: live DashScope acceptance of the single-knob shape on qwen3.8-max-preview (no API access in this change); the pipeline disable path intentionally still strips the tier for thinking-mandatory models under an opt-out, leaving the model default.
  • Breaking changes / migration notes: none.

Linked Issues

Follow-up to #8472 (review comments posted post-merge); original user request #2876.

中文说明

本 PR 做了什么

#8472(Qwen 3.8 reasoning effort)的后续修复,处理合并后提交的 review 发现。对 DashScope 出网形态做四项修复:

  1. 竞争的思考旋钮:当以 reasoning_effort 发出档位时,请求现在会在用户 extra_body 合并之后丢弃 enable_thinkingthinking_budget。此前内置 Token Plan 预设会让旗舰 preview 模型同时携带 reasoning_effortenable_thinking: true——正是嵌套 reasoning 剥除逻辑要避免的"两个竞争旋钮"形态——且 DashScope 拒绝 reasoning_effortthinking_budget 的组合。
  2. tool_choice 剥除加模型家族门控feat(core): support Qwen 3.8 reasoning effort #8472 新增的条件(思考开启时剥掉 tool_choice: 'required')现在门控到 qwen 家族 wire 模型,与 disable 路径的门控一致。对共享 DashScope 端点的非 qwen 模型,reasoning_effort 是原样透传的采样覆盖项而非思考开关,在那里剥掉强制工具选择会退化结构化侧查询(权限分类器、压缩、标题生成)。
  3. 快照/别名覆盖:档位家族改为前缀匹配(qwen3.8-max*),与 modality defaults 中的家族匹配一致。带日期的快照与 -latest 别名此前退回布尔 enable_thinking 折叠,所选档位被静默丢弃。
  4. 可观测性与文档tool_choice 剥除现在记录带模型名与触发条件的 debug 日志;恢复 effort 配置的 JSDoc 与请求级覆盖拷贝的注释;provider 文档行更新为反映强制剥除 thinking_budget 与前缀匹配的家族。

对不对称的请求级覆盖(请求级 reasoning_effort 到达同时从 effort 配置获得 enable_thinking 的旧 qwen 模型)的明确决策:显式档位优先,布尔开关被剥掉——出网永远不同时携带两个旋钮。

为什么需要

#8472 的预设形态意味着每个在内置 preview 模型上选择 /effort 档位的 Token Plan 用户都会同时发出两个竞争的思考旋钮,而 preview 模型是否接受该组合没有任何真实验证。未加门控的 tool_choice 剥除对路由到同一端点的非 qwen 模型是行为回归;精确匹配的家族门控会让快照/别名模型 ID 静默丢失档位——重新引入 #2876 所描述的原始缺陷。

Reviewer 测试计划

如何验证

所有改动均有单元测试覆盖;七个新回归测试在修复前代码上失败(已通过将生产文件还原到合并提交并复跑验证),修复后通过:

cd packages/core && npx vitest run src/core/openaiContentGenerator
  • 预设形态回归:qwen3.8-max-preview + 预设形态 extra_body: { enable_thinking: true } + effort 档位 → 出网仅携带 reasoning_effort(另有 vision 分支与旧模型变体的同类剥除测试)。
  • 非 qwen 反例:glm-5.2 + 用户 extra_body: { reasoning_effort: 'high' }、强制工具模式下保留 tool_choice: 'required'(修复前被剥掉)。
  • 快照 ID:qwen3.8-max-2026-01-15 / qwen3.8-max-latest 现在以 reasoning_effort 透传档位。
  • Disable 路径:配置级 reasoning: false 与请求级 includeThoughts: false 都会为 qwen3.8-max 剥掉档位并保留 tool_choice: 'required'
  • thinking_budget:与档位同发时被剥掉;在旧 hybrid 模型上与 enable_thinking 并存(合法组合)。

前后对比证据

N/A(出网形态变更,非用户可见;证据为上述单元测试)。

测试环境

OS 状态
🍏 macOS
🪟 Windows N/A
🐧 Linux N/A

环境(可选)

仅单元测试(vitest);全仓 npm run typecheck 与定向 eslint 干净。

风险与范围

  • 主要风险或权衡:用户若通过 extra_body 显式同时设置 enable_thinkingreasoning_effort,现在只会发出 reasoning_effort——该组合本身互相矛盾(且 thinking_budget 组合会被 DashScope 拒绝),档位优先。
  • 未验证 / 超出范围:qwen3.8-max-preview 上单旋钮形态的真实 DashScope 接受性(本次改动无 API 访问);pipeline 的 disable 路径在 thinking-mandatory 模型遇到 opt-out 时仍按原逻辑剥掉档位,交由模型默认值。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

#8472 的后续(review 评论提交于合并后);原始用户请求 #2876

…ollow-up)

Follow-up to QwenLM#8472, addressing the post-merge review findings:

- Drop enable_thinking/thinking_budget after the extra_body merge whenever
  reasoning_effort ships: the Token Plan preset made qwen3.8-max-preview
  carry both thinking knobs, and DashScope rejects reasoning_effort
  combined with thinking_budget
- Family-gate the new tool_choice=required strip clause to qwen wire
  models: reasoning_effort is an opaque sampling override on non-qwen
  DashScope models, and dropping forced tool selection degraded their
  structured side queries
- Prefix-match the qwen3.8-max family so dated snapshots and -latest
  aliases receive the selected tier instead of silently collapsing to
  enable_thinking
- Log the tool_choice strip; restore the effort-config JSDoc and comment
  the request-level override copy
@wenshao

wenshao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /takeover

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Gate re-run — the head moved since my last pass (autofix round 1 has landed), so all three stages run again against the new code.

Template looks good ✓

Problem: observed, not theoretical. I verified this against #8472's merged commit (1c1ee23) on the previous pass and the findings still stand on current code: the Token Plan preset turns enableThinking into extra_body.enable_thinking, which the provider merges after the effort config — so every /effort tier on qwen3.8-max-preview shipped reasoning_effort and enable_thinking: true together; the reasoning_effort clause #8472 added to the tool_choice strip was ungated; and the exact-match family gate silently dropped tiers for dated snapshots / -latest aliases. All four fixes map 1:1 to the post-merge review findings on #8472.

Direction: aligned — wire-shape correctness for the effort tiers #8472 just landed. Finishing that feature, not adding scope.

Size: core paths touched (packages/core/src/core/**). At this head: 120 production lines (pipeline.ts 21, dashscope.ts 99) + 356 test lines + 16 docs lines. Small and focused, far below every threshold.

Approach: minimal — every edit closes one named finding, no drive-by changes. Autofix round 1 landed exactly the shape my Stage 3 hold asked for (shared isQwenFamilyWireModel predicate, family-gated split-scope drop, value-checked trigger, real-provider combined-shape test) and declined nothing that was blocking.

Risk: openaiContentGenerator is on this repo's high-revert-correlation path list, so this PR gets full-depth review and hard CI evidence before approval (both done this pass). The author has write access and /verify was already exercised once (passed, on the pre-autofix head — see Stage 2 for the gap that leaves). That's a depth setting, not a gate.

Moving on to code review. 🔍

中文说明

Gate 重跑——距上一轮 head 已变化(autofix 第 1 轮已合入),三个阶段对新代码全部重跑。

模板完整 ✓

问题:已观测而非理论。上一轮已对照 #8472 的合入提交(1c1ee23)验证,结论在当前代码上仍然成立:Token Plan 预设把 enableThinking 转成 extra_body.enable_thinking,而 provider 在 effort 配置之后合并它——因此 qwen3.8-max-preview 上每个 /effort 档位都同时携带 reasoning_effortenable_thinking: true#8472tool_choice 剥除新增的 reasoning_effort 条件未加门控;精确匹配的家族门控让日期快照 / -latest 别名静默丢档。四项修复与 #8472 合并后的 review 发现一一对应。

方向:对齐——为 #8472 刚落地的档位修正出网形态,是收尾而非扩 scope。

规模:触及核心路径(packages/core/src/core/**)。当前 head:120 行生产代码(pipeline.ts 21、dashscope.ts 99)+ 356 行测试 + 16 行文档。小而聚焦,远低于各阈值。

方案:最小化——每处改动对应一个已点名发现,无夹带改动。autofix 第 1 轮落地的正是我 Stage 3 暂缓所要求的形态(共享 isQwenFamilyWireModel 谓词、家族门控的分级剥除、值检查触发、真实 provider 的组合形态测试),且没有驳回任何阻断项。

风险openaiContentGenerator 位于本仓库高 revert 相关路径清单,因此本 PR 按全深度审查 + 硬性 CI 证据处理(本轮均已完成)。作者有写权限,/verify 已跑过一次(通过,但在 autofix 之前的 head 上——缺口见 Stage 2)。这是深度设定,不是门槛。

进入代码审查。🔍

Qwen Code · qwen3.8-max

Reviewed at 6c26ad7e2987d5eb768fe9bbfc9c4b02d8597092 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Code review

Re-run context: head moved 8bbf15966c26ad7e. That delta is autofix round 1, which took my Stage 3 hold list and the author's self-review head-on. I didn't take the autofix summary's word for any of it — every claim below is re-verified against the code at this head.

Prior blockers — all resolved as asked:

  • Family-gated drop (was the hold's item 1). dropConflictingThinkingKnobs early-returns for non-qwen wire models via the new shared static isQwenFamilyWireModel, which also replaces the three inline predicate copies (pipeline disable path, pipeline tool_choice gate, buildQwenEffortConfig). The lowercasing inside the predicate fixes a latent case-drift too (Qwen3.8-Max now matches, consistent with modalityDefaults.ts and requiresThinking). glm/kimi presets keep enable_thinking and the user's reasoning_effort override untouched — locked by the new glm test's no-log assertion.
  • Legacy qwen keeps its thinking switch (item 2). Split scope: outside qwen3.8-max* only thinking_budget is dropped; enable_thinking survives. The two tests that previously locked in the lossy shape are flipped to assert the switch survives, on both the text branch (qwen3.7-max) and the vision branch (qwen-vl-max).
  • Value check (item 3). Trigger is now typeof effort === 'string' && effort !== 'none', matching the pipeline's own guards — an explicit 'none' disable stays on the wire with every knob intact (tested).
  • Combined-shape test gap. The new pipeline test swaps the table's mocked buildRequest for the real DashScope provider, so the provider drop and the pipeline gate execute in one path. It honestly locks in the residual behavior below (glm-5.2 still loses tool_choice via the pre-existing boolean clause) rather than hiding it.

The author's second self-review (post-autofix) — my independent read of each point:

  1. Explicit enable_thinking: false is dropped on the tiered family — that is the PR's declared tie-break ("the explicit tier wins and the boolean is dropped — the wire never carries both knobs"), and under the PR's stated knob map the tiered family reads reasoning_effort, not the boolean, so the dropped false is the inert knob there. Genuine disable paths exist, are kept, and are tested: reasoning_effort: 'none' (the drop stands down) and reasoning: false (pipeline strips the tier). Worth a docs sentence; not a hold.
  2. Legacy hybrid + thinking_budget + reasoning_effort override with no enable_thinking anywhere — self-contradictory hand-built config. Pre-PR that exact pair was the server-rejected shape, so the request errored rather than shipping more thinking; post-PR it succeeds with thinking at the model default. Keeping the budget and dropping the tier instead is arguable, but this is an edge of contradictory manual input — follow-up, along with the test gap the author names.
  3. The enable_thinking === true clause of the tool_choice strip is still ungated — true, and I verified it is pre-existing: feat(core): support Qwen 3.8 reasoning effort #8472's own diff added only the reasoning_effort disjunct (the boolean clause predates it), so gating only the clause this follow-up came to fix is the correct minimal scope. One hygiene gap: the autofix disposition says this was parked in a follow-up issue, but I can't find one filed — worth filing so it isn't lost.
  4. Disable path writes enable_thinking: false for the tiered family — inherited: the disable path predates this PR (this PR only substituted the predicate, behavior preserved), and the tiered-family interaction arrived with feat(core): support Qwen 3.8 reasoning effort #8472. reasoning_effort: 'none' as the family-consistent disable shape is a legitimate design question for a follow-up, not a defect of this diff.
  5. debug vs warn on the dropdebug matches the sibling tool_choice strip log a few lines away in pipeline.ts. Nit.
  6. Layering / DRY — the family predicate is single-sourced now, which was the point of hoisting it; its home and the two small extractions are style calls. Nit.
  7. preserve_thinking ships alongside the tier — that field is multi-turn reasoning continuity, not a thinking knob; whether the server pairs it with reasoning_effort is part of the same unverified-live residual named below. Follow-up question, not a blocker.

Net: no correctness bug, security hole, or regression at this head. The four fixes do what the PR says, and the gating is exactly what the hold asked for. The residual points above are follow-ups, and I'd ask the author to file the one for point 3 (and ideally 1/2/4 together) so they don't evaporate.

Test evidence

CI on 6c26ad7eQwen Code CI run (event pull_request, 2026-08-04T06:54 UTC) completed with success; all checks settled, none pending:

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped — merge queue only
Test (windows-latest, Node 22.x) ⏭️ skipped — merge queue only
Integration Tests (CLI, No Sandbox) ⏭️ skipped — merge queue only
Desktop Shell (ubuntu-22.04) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success

The three skips are by design, not a gap: ci.yml gates test_macos, test_windows, and integration_cli on github.event_name == 'merge_group', so they never run on pull_request events for any PR — the Linux unit gate is the PR-level suite, and it is green.

What I cannot evidence from here: I don't run PR code myself. The approving maintainer's comment on this head reports 230/230 tests on the changed files and 721/721 across the whole openaiContentGenerator directory with typecheck + ESLint clean — that is the maintainer's claim, attributed as such, not independently re-run.

Sandboxed verification would settle the one open behavioral claim: @qwen-code /verify — the earlier ✅ run A/B-proved the wire shape at 8bbf1596, but that head predates the autofix family-gating (the drop's scope changed), and the re-triggered run failed on infrastructure without producing a report. The claim that glm/kimi and legacy-hybrid shapes pass through intact while the qwen3.8-max* tier ships alone is currently pinned at this head by unit tests only, not an A/B harness.

Real-scenario testing: N/A — unattended CI run, and the change is a wire-shape fix with no TUI surface to drive.

中文说明

代码审查

重跑背景:head 从 8bbf15966c26ad7e。这段增量是 autofix 第 1 轮,正面处理了我 Stage 3 的暂缓清单和作者的自我审查。我没有采信 autofix 总结的任何自述——以下每条都是在本 head 的代码上重新核过的。

此前的阻断项——全部按要求解决:

  • 家族门控剥除(暂缓清单第 1 项)dropConflictingThinkingKnobs 对非 qwen wire 模型提前返回,使用新的共享静态方法 isQwenFamilyWireModel,它同时替换了三处内联谓词副本(pipeline disable 路径、pipeline tool_choice 门、buildQwenEffortConfig)。谓词内的小写化还顺带修掉了大小写漂移(Qwen3.8-Max 现在能匹配,与 modalityDefaults.tsrequiresThinking 一致)。glm/kimi 预设保留 enable_thinking 和用户的 reasoning_effort 覆盖——由新 glm 测试的无日志断言锁定。
  • legacy qwen 保留思考开关(第 2 项):分级处理——qwen3.8-max* 之外只剥 thinking_budgetenable_thinking 保留。此前锁定丢失形态的两个测试翻转为断言开关保留,文本分支(qwen3.7-max)与视觉分支(qwen-vl-max)均有覆盖。
  • 值检查(第 3 项):触发条件改为 typeof effort === 'string' && effort !== 'none',与 pipeline 自身的守卫一致——显式 'none' 禁用留在出网中且所有旋钮完整(有测试)。
  • 组合形态测试缺口:新 pipeline 测试把表格里 mock 的 buildRequest 换成真实 DashScope provider,使 provider 剥除与 pipeline 门在同一路径上执行;它诚实地锁定了下面的残留行为(glm-5.2 仍经由既有的布尔条件丢掉 tool_choice),而不是掩盖。

作者第二轮自审(autofix 后)——我对每条的独立判断:

  1. 显式 enable_thinking: false 在档位家族被剥掉——这是 PR 声明的取舍("显式档位胜出,布尔被剥——出网永不同时携带两个旋钮"),且按 PR 自身的旋钮映射,档位家族读 reasoning_effort 而非布尔,所以被剥的 false 在该家族是惰性旋钮。真正的禁用路径存在、保留且有测试:reasoning_effort: 'none'(剥除不触发)与 reasoning: false(pipeline 剥掉档位)。值得补一句文档,不构成暂缓。
  2. legacy 混合模型 + thinking_budget + reasoning_effort 覆盖且无任何 enable_thinking——自相矛盾的手工配置。PR 前该组合正是服务端拒绝的形态,请求直接报错;PR 后请求成功、思考为模型默认值。改为保留 budget、剥掉档位也可争论,但这属于矛盾手工输入的边角——跟进项,连同作者点名的测试缺口。
  3. tool_choice 剥除的 enable_thinking === true 条件仍未加门控——属实,且我核实它是既有行为:feat(core): support Qwen 3.8 reasoning effort #8472 的 diff 新增了 reasoning_effort 析取项(布尔条件早于它),所以本次跟进只给它要修的条件加门控,是恰当的最小范围。一个卫生缺口:autofix 的处置说已挂进跟进 issue,但我没找到已建的 issue——建议建一个以免遗失。
  4. disable 路径对档位家族写 enable_thinking: false——继承来的:disable 路径早于本 PR(本 PR 只替换了谓词,行为不变),档位家族的交互是 feat(core): support Qwen 3.8 reasoning effort #8472 引入的。以 reasoning_effort: 'none' 作为家族一致的禁用形态是合理的跟进设计问题,不是本 diff 的缺陷。
  5. 剥除用 debug 还是 warn——debug 与几行之外 pipeline.tstool_choice 剥除的日志一致。小项。
  6. 分层 / DRY——家族谓词现已单一来源,这正是提取的目的;归属位置和两个小提取是风格取舍。小项。
  7. preserve_thinking 与档位同出——该字段是多轮推理连续性,不是思考旋钮;服务端是否将它与 reasoning_effort 配对,属于下面同名的"未经实测"残留。跟进问题,非阻断。

结论:本 head 无正确性 bug、安全漏洞或回归。四项修复如 PR 所述生效,门控正是暂缓清单所要求的形态。上述残留各点是跟进项——请作者为第 3 点(最好连同 1/2/4)建 issue,以免遗失。

测试证据

6c26ad7e 上的 CI——Qwen Code CI 运行(event pull_request,2026-08-04T06:54 UTC)以 success 完成;所有检查已落定,无 pending。表内三个 skipped 是设计使然:ci.ymltest_macostest_windowsintegration_cli 门控在 merge_group 事件,任何 PR 的 pull_request 事件都不会跑——Linux 单测门就是 PR 级套件,它是绿的。

我无法在此提供的证据:我本人不运行 PR 代码。本 head 上的 maintainer 批准评论报告改动文件 230/230、整个 openaiContentGenerator 目录 721/721、typecheck + ESLint 干净——那是 maintainer 的声明,如实转述,未经我独立复跑。

沙箱验证可以落定唯一开放的行为主张:@qwen-code /verify——此前 ✅ 的那次在 8bbf1596 上做了 A/B 出网证明,但该 head 早于 autofix 的家族门控(剥除范围已变),而重新触发的运行因基础设施故障未产出报告。"glm/kimi 与 legacy 混合形态原样通过、qwen3.8-max* 档位单独出网"这一主张,在本 head 上目前只有单测锁定,没有 A/B harness 证据。

真实场景测试:N/A——无人值守 CI 运行,且本改动是出网形态修复,没有可驱动的 TUI 面。

Qwen Code · qwen3.8-max

Reviewed at 6c26ad7e2987d5eb768fe9bbfc9c4b02d8597092 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the hold's three asks are implemented exactly as asked and independently re-verified at this head; what remains from the author's second review is real but second-order — contradictory-input edges and pre-#8472 behavior — follow-up material, not merge blockers.

Stepping back: my previous pass held this PR because the knob drop was ungated, and green CI couldn't settle that. The honest question this pass is whether I'm now approving because the fix is genuinely right, or because the thread wore the objection down. I checked by re-reading the code rather than the autofix summary: the drop is family-gated through a shared predicate, split-scoped (only thinking_budget outside qwen3.8-max*), value-checked ('none' stands down), runs after the extra_body merge on both branches, and each behavior — including the glm counter-case and the flipped legacy tests — is pinned by a test that would catch the pre-fix shape. My independent proposal for this problem (Stage 2a) was essentially what landed; I didn't find a simpler path the PR missed.

The author's second self-review raised seven points, and a gate should take those seriously — so I triaged each one independently (detail in Stage 2). The pattern: none is a regression introduced by this diff. Points 1–2 are edges where the user supplies contradictory knobs and the PR's declared tie-break decides; point 3 and 4 are behavior that predates #8472 or the PR itself (I verified #8472's diff added only the reasoning_effort disjunct); points 5–7 are nits. On a re-run, concerns like these get noted and followed up — they don't hold the PR. The one concrete ask: file the follow-up issue for the ungated enable_thinking === true clause (the disposition says it's parked, but no issue exists yet).

What I'm still not claiming: live DashScope acceptance of the single-knob shape is unverified — the PR says so itself, and the earlier ✅ /verify covered the pre-autofix head. But the blast radius of that residual is bounded to the qwen3.8-max* family that motivated the fix (everything else is pass-through at this head), and the maintainer's approval on this exact commit means the live check is owned by someone with API access.

My CHANGES_REQUESTED hold from the previous head stands superseded by this approval — same account, newer review, newer commit.

Follow-ups to file (author): ungated enable_thinking === true tool-choice clause for non-qwen presets; the contradictory-input semantics (points 1–2); disable shape for the tiered family (point 4).

中文说明

退一步看:上一轮我因旋钮剥除未加门控而暂缓,绿色 CI 无法落定那个问题。本轮的诚实问题是:我现在批准,是因为修复确实正确,还是因为线程把反对磨没了。我的核对方式是重读代码而不是读 autofix 的总结:剥除已按家族门控、走共享谓词、分级处理(qwen3.8-max* 之外只剥 thinking_budget)、值检查('none' 不触发)、在两个分支的 extra_body 合并之后执行,且每种行为——包括 glm 反例和翻转后的 legacy 测试——都由能捕获修复前形态的测试锁定。我对该问题的独立方案(Stage 2a)与落地形态基本一致;没有找到 PR 遗漏的更简路径。

作者第二轮自审提了七点,gate 应当认真对待——我逐条独立研判(详见 Stage 2)。共性是:没有一条是本 diff 引入的回归。第 1–2 点是用户提供矛盾旋钮、由 PR 声明的取舍裁决的边角;第 3、4 点早于 #8472 或本 PR 就存在(我已核实 #8472 的 diff 只新增了 reasoning_effort 析取项);第 5–7 点小项。重跑时,这类顾虑记下来跟进即可,不构成暂缓。唯一具体请求:为未加门控的 enable_thinking === true 条件建跟进 issue(处置说已挂起,但 issue 尚不存在)。

我仍不主张的:DashScope 服务端对单旋钮形态的接受性未经验证——PR 自己也这么说,且此前 ✅ 的 /verify 覆盖的是 autofix 之前的 head。但该残留的影响范围限于促成修复的 qwen3.8-max* 家族(其余在本 head 上均为原样透传),且 maintainer 已在本 commit 上批准,意味着实测由有 API 访问权限的人接手。

我上一轮在旧 head 上的 CHANGES_REQUESTED 暂缓由本次批准取代——同一账号、更新的评审、更新的 commit。

待建跟进 issue(作者):非 qwen 预设的 enable_thinking === true tool-choice 条件未加门控;矛盾输入语义(第 1–2 点);档位家族的禁用形态(第 4 点)。

Qwen Code · qwen3.8-max

Reviewed at 6c26ad7e2987d5eb768fe9bbfc9c4b02d8597092 · re-run with @qwen-code /triage

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Aug 3, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

中文说明

已审查。 建议见行内评论。

— qwen3.7-max via Qwen Code /review (v0.21.4)

Comment thread packages/core/src/core/openaiContentGenerator/provider/dashscope.ts Outdated
doudouOUC
doudouOUC previously approved these changes Aug 3, 2026

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Implementation is correct and well-tested; all three post-merge findings from #8472 are faithfully addressed with thorough regression tests. Sole suggestion (observability: add a debug log to dropConflictingThinkingKnobs) is inline and non-blocking.

@wenshao

wenshao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Review: fix(core): harden Qwen 3.8 reasoning effort wire shape

Overview

Four follow-up fixes to #8472 on the DashScope wire shape:

  1. dropConflictingThinkingKnobs() deletes enable_thinking / thinking_budget after the extra_body merge whenever reasoning_effort ships (both the text and vision branches).
  2. The pipeline's tool_choice: 'required' strip is gated to qwen-family wire models for the reasoning_effort clause.
  3. buildQwenEffortConfig prefix-matches qwen3.8-max* instead of exact-matching two ids.
  4. A debug line on the tool_choice drop, restored JSDoc/comments, docs table update.

Items 2 and 3 are clearly right. Item 3 in particular fixes a real silent-tier-loss bug for snapshot/alias ids, and the gate in item 2 mirrors the existing disable-path gate at pipeline.ts:885-887. The test additions are well-targeted and the comments carry real reasoning rather than restating the code — this matches the surrounding style well.

My concerns are all with item 1.


1. dropConflictingThinkingKnobs is not family-gated — contradicts this PR's own rationale

packages/core/src/core/openaiContentGenerator/provider/dashscope.ts:346-351

private dropConflictingThinkingKnobs(merged: Record<string, unknown>): void {
  if ('reasoning_effort' in merged) {
    delete merged['enable_thinking'];
    delete merged['thinking_budget'];
  }
}

This runs for every DashScope model. That is the exact opposite of the reasoning used to justify change #2, which the PR body states as: "on non-qwen models sharing the DashScope endpoint, reasoning_effort is an opaque sampling override, not a thinking switch." Here it is treated as a thinking switch for every model.

This is not hypothetical. In packages/core/src/providers/presets/alibaba-token-plan.ts, glm-5.2, glm-5.1, glm-5, kimi-k2.7-code, kimi-k2.6, and kimi-k2.5 all carry enableThinking: true, which provider-config.ts:69-71 turns into extra_body: { enable_thinking: true }. A user who sets samplingParams.reasoning_effort on one of those models — which docs/users/configuration/model-providers.md explicitly recommends ("Set via samplingParams … when the provider expects a different shape") — now silently loses the preset's thinking switch.

Suggested fix: gate on the same predicate used everywhere else, or narrow further to the family where reasoning_effort genuinely is the thinking knob:

private dropConflictingThinkingKnobs(model: string, merged: Record<string, unknown>): void {
  if (!isQwenFamilyWireModel(model)) return;
  ...
}

2. On legacy qwen models the conflict is resolved in the direction that loses thinking

dashscope.ts:328-333 is this PR's own statement of the model→knob map: only qwen3.8-max* reads reasoning_effort; older qwen hybrids read enable_thinking. Given that, dropping enable_thinking in favour of a reasoning_effort the model does not read means the wire carries no thinking signal, so the model falls back to its default (off for the qwen3 commercial hybrids). A user who asked for max effort ends up with less thinking than before this PR, not more.

The two new tests lock this in:

  • dashscope.test.ts"drops enable_thinking when a request-level reasoning_effort override ships on a legacy qwen model" (qwen3.7-max)
  • dashscope.test.ts"vision model: drops enable_thinking when extra_body ships a reasoning_effort override" (qwen-vl-max)

Both models carry enableThinking: true in the Token Plan preset (qwen3.7-max, qwen3.7-plus, qwen3.6-plus, qwen3.6-flash) and none is thinkingMandatory, so this is the live shape for those users.

Outside the qwen3.8-max* family I'd resolve the conflict the other way — keep enable_thinking: true and drop the tier the model can't read — or, more conservatively, drop only thinking_budget there (the pair the vendor actually rejects) and leave enable_thinking alone. The qwen3.8-max* case is the one that genuinely needs the enable_thinking drop.

Worth noting the flagship case is safe as written: qwen3.8-max-preview is thinkingMandatory: true, so thinking cannot be turned off by omitting the boolean. That's the one model where the single-knob shape is unambiguously correct.

3. Key-presence check where the pipeline uses a value check

'reasoning_effort' in merged fires on presence, not on a meaningful value. Compare pipeline.ts:927, which guards the same field with typed['reasoning_effort'] !== 'none', and pipeline.ts:972-974, which additionally requires typeof reasoningEffort === 'string'. An explicit reasoning_effort: 'none' — a disable, per the pipeline's own semantics — currently also strips enable_thinking and thinking_budget. For a legacy qwen model the correct disable shape is enable_thinking: false, so the result is a no-op rather than the requested opt-out.

Aligning with the pipeline guard is a one-liner:

const effort = merged['reasoning_effort'];
if (typeof effort === 'string' && effort !== 'none') { ... }

4. Vendor behaviour is unverified (acknowledged in the PR body)

The whole change rests on DashScope accepting reasoning_effort alone on qwen3.8-max*. The PR body is upfront that there's no live verification. Given point 1 and 2 above, the blast radius of that assumption being wrong is wider than the 3.8 family — a single smoke request against the preview model before merge would retire most of the risk, and narrowing the drop to qwen3.8-max* would bound it regardless.


Smaller items

  • Duplicated family predicate. model.startsWith('qwen') || model === 'coder-model' now appears at pipeline.ts:887, pipeline.ts:972, and (as wireModel.startsWith('qwen') || wireModel === 'coder-model') at dashscope.ts:331. Three copies of a gate whose correctness matters is worth one exported helper — e.g. isQwenFamilyWireModel(model) next to isDashScopeProvider. It would also give the fix for point 1 an obvious home.
  • Docs are incomplete on the user-visible part. The updated row says only "A configured thinking_budget is dropped whenever an effort tier ships." The enable_thinking drop is the more surprising change for anyone with an extra_body override and isn't mentioned. The same row's closing sentence — "Other Qwen models continue to map a selected effort to enable_thinking: true" — is no longer unconditionally true given point 2.
  • Test layering gap. pipeline.test.ts mocks buildRequest (pipeline.test.ts:880-883) to merge extra_body itself, so the real provider drop never executes in those tests, and dashscope.test.ts never runs the pipeline gate. The new glm-5.2 pipeline case therefore exercises the gate but not the interaction — the combined shape (real provider drop feeding the pipeline's enable_thinking === true clause) is untested in both files. A case with extra_body: { enable_thinking: true, reasoning_effort: 'high' } on glm-5.2 would have surfaced point 1.
  • Debug log. pipeline.ts:976-980 is well-placed and only fires on the drop. No concerns; consider including tool_choice in the payload so the log line is self-contained.
  • Minor: the PR body says "seven new regression tests" — I count 6 it blocks (7 cases via the it.each) in dashscope.test.ts plus 3 table rows in pipeline.test.ts.

Security / performance

Nothing of concern. No new I/O, no credential or user-content handling; the added work is a couple of deletes per request and a debug log on a cold branch. The debugLogger.debug payload carries only model id and effort tier — no user content.

Verdict

The prefix match (#3) and the family gate on the tool_choice strip (#2) I'd merge as-is — both fix real bugs and are properly covered. I'd hold on the enable_thinking drop until it's gated to the model family that actually reads reasoning_effort; as written it silently disables thinking for the glm/kimi and legacy-qwen presets that ship enableThinking: true, which is a wider behavioural change than the PR body describes.

@wenshao

wenshao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

1 similar comment
@wenshao

wenshao commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 2055 passed · 0 failed · 2055 total

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:2055 通过 · 0 失败 · 2055 总计

Verification report

PR #8488 Deep Verification — fix(core): harden Qwen 3.8 reasoning effort wire shape

Verdict: merge-ready — 2055 scripted assertions executed, 0 unexpected failures.
Verified head: 8bbf1596aea29e23fbba8cb996520d70efb4b9fe (merge commit c501b4fd6, base tip d6f55a1c9).

中文摘要
  • 结论merge-ready。2055 条脚本化断言全部通过,0 条意外失败。
  • A/B 结论:对中心主张做了真出网字节的 A/B 证明(loopback HTTP 服务器捕获真实请求体,head 编译产物 vs base 编译产物):
    • 竞争旋钮修复成立qwen3.8-max-preview + 预设形态 extra_body: {enable_thinking: true} + effort 档位时,head 出网仅携带 reasoning_effort,base 同时携带两个旋钮(enable_thinking + thinking_budget 冲突形态均复现)——8 个判别单元格全部按预测翻转,4 个平价单元格两臂一致。
    • tool_choice 门控成立glm-5.2 + 用户 reasoning_effort 强制工具模式下,head 保留 tool_choice: 'required',base 剥掉(回归修复)。
    • 快照/别名覆盖成立qwen3.8-max-2026-01-15 / -latest / 大小写变体在 head 上以 reasoning_effort 透传档位,base 静默折叠为 enable_thinking
  • 测试非空洞:七个新回归测试在 base 源码上恰好失败七个(与 PR 声明一致),失败信息均为预期值/实际值断言;变异矩阵三个变异体(删除 drop 调用、前缀匹配还原为精确匹配、移除 pipeline 家族门控)全部被对应测试杀死,无幸存者。
  • 未覆盖范围:DashScope 真实服务端对单旋钮形态的接受性(无 API 访问,PR 自身亦声明);逐提交归因(depth-2 检出);整仓 build/integration 门(PR 自身 CI 覆盖)。全量 packages/core 套件中 70 个失败经 base 对照逐名比对为完全一致的既有环境性失败(样本:真实 HOME installation-id 泄漏进 mock 测试),与本 PR 无关,head 相对 base 恰多通过 10 个(即全部新增测试)。

Scope selection

  • Central claim: when an effort tier ships for a qwen3.8-max* model, the wire body carries reasoning_effort alone — preset/user extra_body knobs (enable_thinking, thinking_budget) are dropped after the extra_body merge (new dropConflictingThinkingKnobs).
  • Secondary claim A: the tool_choice: 'required' strip clause is family-gated — non-qwen models on the DashScope endpoint (e.g. glm-5.2) keep forced tool selection.
  • Secondary claim B: snapshot/alias model ids (qwen3.8-max-2026-01-15, qwen3.8-max-latest) receive the selected tier via prefix match instead of silently collapsing to enable_thinking.

Central claim — A/B wire-oracle proof

Harness: ab-wire.mjs (in this dir) drives the real compiled ContentGenerationPipeline + DashScopeOpenAICompatibleProvider from each arm's dist/ — no mocks of the unit under test — with baseUrl pointed at a loopback HTTP server that captures the exact JSON body the OpenAI SDK posts (authType: 'qwen-oauth' is used to activate the DashScope detection path on loopback). 12 scenarios × per-field assertions, run once per arm. Head dist = CI-built packages/core/dist at HEAD; base dist = tsc --build of packages/core in a worktree at HEAD^1.

Witness: 01-ab-wire-cells-head-vs-base.png (cell table rendered from the raw wire dumps), 02-ab-arm-run-summaries.png (live run logs).

cell scenario head wire base wire flip
C1 qwen3.8-max-preview + preset extra_body:{enable_thinking:true} + tier high + forced tools (the Token Plan preset shape) reasoning_effort:"high", single knob reasoning_effort:"high" and enable_thinking:true
C2 C1 + thinking_budget:1024 in extra_body both knobs dropped ships the DashScope-rejected reasoning_effort+thinking_budget pair
C3 glm-5.2 + user extra_body:{reasoning_effort:'high'} + forced tools tool_choice:'required' kept tool_choice dropped (the regression)
C4 qwen3.8-max-2026-01-15 + tier reasoning_effort:"high" enable_thinking:true, tier lost
C5 qwen3.8-max-latest + tier max reasoning_effort:"max" enable_thinking:true, tier lost
C5b QWEN3.8-Max-Latest (case) reasoning_effort:"low" tier lost
C6 legacy qwen3.7-max + tier enable_thinking:true same parity
C7 qwen3.8-max + reasoning:false + extra_body tier + forced tools enable_thinking:false, tier stripped, tool_choice:'required' kept same (pre-existing disable path) parity
C8 vision branch: qwen-vl-max + tier + extra_body:{reasoning_effort:'max'} reasoning_effort:"max" alone, vl_high_resolution_images:true both knobs
C9 qwen3.8-max + tier, no extra_body reasoning_effort:"medium", tool strip fires same parity
S1 sibling: legacy qwen + extra_body:{reasoning_effort:'none'} reasoning_effort:"none" alone (see Observations) both knobs
S2 sibling: thinking_budget without any tier untouched same parity

Counts: head 106/106 assertions pass (exit 0); base 104/104 pass against the encoded pre-fix expectations (exit 0) — the base arm reproduced every predicted pre-fix shape, including both competing-knobs forms and the ungated tool_choice strip. Raw per-cell wire bodies: logs/head-wire.json, logs/base-wire.json; run logs logs/head-ab.log, logs/base-ab.log.

Test gates

Witness: 03-vitest-gate-head-716.png.

  • Targeted gate (head): npx vitest run src/core/openaiContentGenerator17 files, 716/716 pass (logs/head-vitest.log).
  • Typecheck (head): tsc --noEmit in packages/core — clean, exit 0 (logs/head-typecheck.log). Gate liveness proven: a planted const _gateProbe: number = "not-a-number" in a scratch copy was caught (TS2322), then reverted.
  • Full packages/core suite (head vs base): head 19113 pass / 70 fail / 10 skip; base 19103 pass / 70 fail / 10 skip. The 70 failing test names are byte-identical across arms (diff of the sorted unique name lists exits clean; 70/70 FAIL lines, 64 unique names) — pre-existing environmental failures of this container lane (sample cause: the real $HOME installation-id leaks into mocked-uuid expectations in installationManager.test.ts), not PR regressions. Head's +10 passing delta is exactly the ten new tests. Witness: 06-full-core-attribution.png.

Non-vacuity of the new tests

Witness: 04-vacuity-base-7-red.png.

The HEAD test files were overlaid on the base worktree and run against the pre-fix source: exactly 7 failed | 218 passed, matching the PR's "seven new regression tests fail on the pre-fix code" claim one-for-one. Failure messages are intended behavioral mismatches, not setup errors — e.g. expected true to be undefined (preset enable_thinking survives), expected undefined to be 'xhigh' (tier lost on snapshots). The remaining 3 new tests (config-level opt-out strip, per-request opt-out strip, legacy thinking_budget keep) pin pre-existing behavior and correctly pass on base.

Mutation matrix

Witness: 05-mutation-matrix.png. Suite: the two changed test files (225 tests) in a scratch worktree at HEAD; control green 225/225 first.

mutant mutation result killed by
control none 225/225 green
M1 both dropConflictingThinkingKnobs() call sites removed 221/225 red preset enable_thinking drop; legacy request-level override; thinking_budget drop; vision-branch drop
M2 startsWith('qwen3.8-max') reverted to exact-match pair 223/225 red snapshot 2026-01-15; -latest alias
M3 pipeline family gate removed (base clause shape) 224/225 red glm-5.2 keeps tool_choice='required'

Every guard the PR introduces is pinned; no survivors. Positive control: M3's single red is the same test that fails in the base-source vacuity run. Each mutant's reds match the predicted attribution with no collateral tests going red.

Reviewer Test Plan walkthrough (per step)

Every step of the PR's own plan was executed and held:

  1. Preset shape regression (qwen3.8-max-preview + preset-style extra_body + tier → single knob, plus vision-branch and legacy-model variants) — wire cells C1/C8 plus the M1 killer set (preset drop, legacy request-level override, vision drop). Holds at head, reproduces broken at base.
  2. Non-qwen counter-case (glm-5.2 + user reasoning_effort keeps tool_choice: 'required') — wire cell C3. Holds at head; base strips it.
  3. Snapshot ids (qwen3.8-max-2026-01-15 / -latest pass the tier through) — wire cells C4/C5 (+ C5b case-insensitive). Hold at head; base loses the tier.
  4. Disable paths (config-level reasoning: false and per-request includeThoughts: false strip the tier and keep tool_choice) — wire cell C7 + the two new pipeline cases. Holds at head; on base this behavior is pre-existing (both cells parity), so these tests pin rather than change.
  5. thinking_budget (dropped when a tier ships; kept with enable_thinking on legacy hybrids) — wire cells C2/S2 + new dashscope tests. Holds at head.

No step was unperformable.

Corrections

None (first verification round; no prior-round claims to correct).

Findings

No blocking findings.

Observations (not findings):

  1. 'none' sibling (cell S1): a user extra_body: { reasoning_effort: 'none' } on a legacy qwen model with a config tier ships reasoning_effort: 'none' alone (the boolean is dropped). This is consistent with the PR's stated invariant — "the explicit tier wins … the wire never carries both knobs" — but whether DashScope's legacy models accept the literal value 'none' is unverifiable without API access (same acknowledged limitation as the PR's own). If 'none' is rejected server-side there, the failure mode is loud (request error), not silent.
  2. The qwen3.8-max prefix admits any hypothetical future id starting with that string (e.g. qwen3.8-max-2); this is the intended family design and is consistent with the /^qwen3\.8-max/ family match in modalityDefaults.ts (verified) — no false-positive risk among model ids present in any preset (only alibaba-token-plan.ts carries qwen3.8 entries).

Not covered

  • Live DashScope acceptance of the single-knob shape on qwen3.8-max-preview / snapshots — no API access in this sandbox (explicitly acknowledged in the PR description as well). The A/B proves the client-side wire shape only (the shape of the fix, not server-side acceptance).
  • Per-commit attribution: depth-2 checkout — only HEAD^2 is locally reachable (git rev-list HEAD^1..HEAD^2 returns just the merge commit), while the metadata snapshot lists 2 commits; the fix commit 0252a23b is unreachable. Verified the aggregate HEAD^1..HEAD diff.
  • Non-DashScope OpenAI-compatible servers were not driven through the changed clause; the clause sits under the unchanged isDashScope && conjunct, so the change cannot alter their path (structural argument, not a driven cell).
  • Repo-wide gates not re-run here: full monorepo build/bundle, integration suites, repo-wide lint — the PR's own CI covers them; this round ran the affected workspace's suite, the full packages/core suite, and the workspace typecheck.
  • The 70 pre-existing environmental failures in the full packages/core suite were attributed (byte-identical on base) but not root-caused — they predate this PR and are orthogonal to it.

Methodology

Environment: node:22-bookworm container (Node v22.23.2), CI verify lane; working tree = refs/pull/8488/merge (depth 2), npm ci + npm run build pre-run at HEAD. The A/B harness (ab-wire.mjs) instantiates the real pipeline/provider from compiled dist/ output and posts through the real OpenAI SDK + undici stack to a loopback HTTP server that records method/path/headers/body — no stub of any unit under test. Base control: git worktree add tmp/base-tree HEAD^1, rebuilt packages/core there (tsc --build emitted despite unrelated type-noise from two worktree artifacts: the tsconfig paths entry for @lydell/node-pty and the nested dependency store, both absent under a worktree; the emitted JS was shape-verified by grep — no dropConflictingThinkingKnobs, exact-match family gate). packages/core has no internal @qwen-code/* dependencies, and the PR leaves package-lock.json untouched, so reusing the root node_modules is a clean control; the one resolution hazard found — nested per-package node_modules (ajv, mime, fdir, …) not being an ancestor of the worktree — was fixed by symlinking the repo's nested store into both scratch trees, giving both arms bit-identical dependency resolution. All raw logs and per-cell wire dumps live under logs/; harness scripts (ab-wire.mjs, evidence-table.mjs) are in this directory for rerun. Scratch worktrees were removed after capture; the main tree is clean (git status --porcelain empty; junit.xml/tmp/ are gitignored).

Evidence images

01-ab-wire-cells-head-vs-base

02-ab-arm-run-summaries

03-vitest-gate-head-716

04-vacuity-base-7-red

05-mutation-matrix

06-full-core-attribution

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding this one — item 1 (dropConflictingThinkingKnobs) needs another round. Full detail in my Stage 2/3 comments above; the short version:

  • The drop is not family-gated: on the glm/kimi and legacy-qwen presets that inject enable_thinking via extra_body, a user samplingParams.reasoning_effort override silently deletes the thinking switch — the same "opaque sampling override, not a thinking switch" reasoning this PR uses to gate the tool_choice strip.
  • On legacy qwen models the conflict resolves to no thinking signal on the wire, and the new tests lock that shape in.
  • 'reasoning_effort' in merged also fires on 'none', against the pipeline's own value-check semantics.

Items 2–4 look mergeable as-is. Suggested fixes (family gate / shared predicate helper, 'none' value check, one combined-shape test, docs row) are in the Stage 3 comment. 🙏

中文说明

先暂缓——item 1(dropConflictingThinkingKnobs)还需要一轮。细节见上方 Stage 2/3 评论,简述:

  • 剥除未加家族门控:对经 extra_body 注入 enable_thinking 的 glm/kimi 与旧 qwen 预设,用户的 samplingParams.reasoning_effort 覆盖会静默删掉思考开关——与本 PR 为 tool_choice 剥除加门控时"原样透传的采样覆盖项、不是思考开关"的理由自相矛盾。
  • 对旧 qwen 模型,冲突解决为出网不携带任何思考信号,且新测试将该形态固化。
  • 'reasoning_effort' in merged'none' 同样触发,与 pipeline 自身的值检查语义不一致。

Items 2–4 本身即可合并。建议的修复(家族门控 / 共享谓词 helper、'none' 值检查、一个组合形态测试、文档行)见 Stage 3 评论。🙏

Qwen Code · qwen3.8-max

@qwen-code-dev-bot

qwen-code-dev-bot commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

AutoFix round 1 finishedview run. See this round's report below.

中文说明

AutoFix 第 1 轮已完成 —— 查看运行。本轮报告见下方。

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下:

Autofix round summary — PR #8488 (address review feedback)

All blocking findings on item 1 (dropConflictingThinkingKnobs) are addressed; items 2–4 of the PR were already mergeable and are untouched. No base-conflict resolution was needed (--conflict false).

Feedback points and dispositions

  1. Ungated knob drop (C1 / Stage 3 item 1) — Fixed. dropConflictingThinkingKnobs is now family-gated. A new exported predicate DashScopeOpenAICompatibleProvider.isQwenFamilyWireModel() (next to isDashScopeProvider) replaces all three inline copies of model.startsWith('qwen') || model === 'coder-model' (pipeline disable path, pipeline tool_choice gate, buildQwenEffortConfig), and the drop uses it too:
    • non-qwen models on the endpoint (glm/kimi presets): nothing is dropped — a user samplingParams.reasoning_effort override stays an opaque sampling parameter and the preset's enable_thinking survives;
    • legacy qwen hybrids (qwen3.7-max, qwen-vl-max, ...): keep enable_thinking and drop only thinking_budget (the vendor-rejected pair), per the reviewer's conservative resolution;
    • qwen3.8-max* only: both knobs are still dropped so the tier ships alone (the flagship fix, unchanged).
  2. Legacy qwen lost the thinking signal (C2 / Stage 2) — Fixed. The two tests that locked in the lossy shape are flipped to assert enable_thinking: true survives alongside a request-level/extra_body reasoning_effort override (text branch qwen3.7-max and vision branch qwen-vl-max). The docs closing sentence ("Other Qwen models continue to map a selected effort to enable_thinking: true") is unconditionally true again.
  3. Presence check vs value check (C3 / Stage 3 item 2) — Fixed. The trigger is now typeof effort === 'string' && effort !== 'none', matching the pipeline's own reasoning_effort guards; an explicit 'none' disable no longer strips anything.
  4. Combined-shape test gap (Stage 3 item 3) — Fixed. New pipeline test wires the real DashScopeOpenAICompatibleProvider.buildRequest into the pipeline gate (glm-5.2 + extra_body: { enable_thinking: true, reasoning_effort: 'high' } + forced tools): the preset switch survives the provider drop and then trips the pipeline's enable_thinking === true clause. The table's mocked buildRequest is left as-is for the gate-only rows. Also added the provider-level glm-5.2 case (would have caught C1 directly) and a vision-branch thinking_budget case so the vision path keeps drop coverage. A mutation check (reverting the drop to the pre-fix ungated shape) turns exactly the 7 new/flipped tests red, so they are load-bearing.
  5. Docs row (Stage 3 item 4) — Fixed. The DashScope row now states that both enable_thinking and thinking_budget are dropped (with a debug log) for the qwen3.8-max family, and describes the legacy-qwen pass-through/thinking_budget-only behavior.
  6. Debug log on the drop (inline suggestion rc:3707939839) — Implemented. The drop emits debugLogger.debug('DashScope: dropping thinking knobs that conflict with reasoning_effort', { model, reasoningEffort, dropped }), mirroring the pipeline's tool_choice strip log; pinned by an assertion in the flagship preset-drop test and a no-log assertion in the glm case.
  7. Include tool_choice in the pipeline strip's debug payload (smaller item) — Declined. The clause only fires when tool_choice === 'required', and the message string already names it (dropping tool_choice=required while thinking is enabled); a payload field would be a constant, not information.
  8. Live DashScope smoke request (item 4) — Not runnable here. No DashScope credentials exist in this environment (acknowledged in the PR body). The blast radius is now bounded anyway: outside qwen3.8-max* the wire shape is the pre-PR pass-through, so a wrong server-side assumption about the single-knob shape can only affect the family that motivated the fix.
  9. Ungated enable_thinking === true sibling disjunct (Stage 2 observation) — Out of scope. The reviewer explicitly parked it in a follow-up issue; not touched.
  10. PR body test count (minor) — Informational. The body predates this round; after it, the PR's new tests are: 6 added + 2 flipped provider tests and 1 added pipeline test (plus the earlier 3 pipeline table rows).

Changed files

  • packages/core/src/core/openaiContentGenerator/provider/dashscope.ts — new isQwenFamilyWireModel helper; family-gated, value-checked, split-scope dropConflictingThinkingKnobs with debug log; both call sites pass the wire model; buildQwenEffortConfig uses the helper.
  • packages/core/src/core/openaiContentGenerator/pipeline.ts — both gate copies use the shared helper (coder-model rationale moved into the helper's JSDoc).
  • packages/core/src/core/openaiContentGenerator/provider/dashscope.test.ts — flipped 2 tests, added 4, debug-log assertions.
  • packages/core/src/core/openaiContentGenerator/pipeline.test.ts — added 1 combined real-provider/pipeline-gate test.
  • docs/users/configuration/model-providers.md — DashScope row updated.

Conflict notes

None — branch merges cleanly against origin/main (already merged; --conflict false).

Verification

  • npm run build — passed
  • npm run typecheck — passed
  • npm run lint — passed
  • npx prettier --check on the five touched files — passed (docs table re-aligned with prettier --write)
  • cd packages/core && npx vitest run src/core/openaiContentGenerator/provider/dashscope.test.ts src/core/openaiContentGenerator/pipeline.test.ts — 2 files, 230/230 passed
  • cd packages/core && npx vitest run src/core/openaiContentGenerator src/core/contentGenerator.test.ts src/providers — 32 files, 888/888 passed
  • cd packages/core && npx vitest run (full packages/core suite) — 19198 tests: 19106 passed / 82 failed / 10 skipped. All 82 failures are pre-existing environmental failures of this runner: they live in twelve test files untouched by this PR (verified via git diff origin/main...HEAD --name-only) whose sources are also unchanged, none of them imports a changed module, and every message shows the runner environment leaking in — QWEN_HOME (set to /home/runner/work/_temp/qwen-autofix-review-home here) overrides the os.homedir()-based expectations in storage.ts:184, and the real installation_id file defeats the mocked uuid. The PR's own sandboxed verification documented the same failure class as byte-identical on the base arm. Every PR-touching area is green (888/888 above).
  • Mutation check: temporarily reverted the drop to the pre-fix ungated/presence-based shape — exactly the 7 new/flipped tests failed (all other 223 passed); fix restored and re-verified green
  • Integration tests: not applicable — the changed behavior (provider request shaping) is exercised directly by the unit suites, not only through the bundled CLI
中文说明

Autofix 本轮摘要 — PR #8488(处理评审反馈)

item 1(dropConflictingThinkingKnobs)的全部阻断项已修复;PR 的 items 2–4 本身即可合并,未改动。无需解决 base 冲突(--conflict false)。

反馈点与处理

  1. 未加门控的旋钮剥除(C1 / Stage 3 item 1)— 已修复。 dropConflictingThinkingKnobs 现在加了家族门控。新增导出谓词 DashScopeOpenAICompatibleProvider.isQwenFamilyWireModel()(位于 isDashScopeProvider 旁),一次性替换三处内联的 model.startsWith('qwen') || model === 'coder-model'(pipeline disable 路径、pipeline tool_choice 门控、buildQwenEffortConfig),剥除逻辑本身也使用它:
    • 端点上的非 qwen 模型(glm/kimi 预设):不剥除任何旋钮——用户的 samplingParams.reasoning_effort 覆盖项保持原样透传的采样参数身份,预设的 enable_thinking 保留;
    • 旧 qwen 混合模型(qwen3.7-maxqwen-vl-max 等):保留 enable_thinking,仅剥除 thinking_budget(供应商真正拒绝的组合),即评审者给出的保守解法;
    • qwen3.8-max*:两个旋钮仍被剥除,档位单独出网(旗舰修复,不变)。
  2. 旧 qwen 模型丢失思考信号(C2 / Stage 2)— 已修复。 两个固化丢档形态的测试已反转为断言 enable_thinking: true 与请求级/extra_bodyreasoning_effort 覆盖共存(文本分支 qwen3.7-max 与视觉分支 qwen-vl-max)。文档结尾句("Other Qwen models continue to map a selected effort to enable_thinking: true")重新无条件成立。
  3. 存在性检查 vs 值检查(C3 / Stage 3 item 2)— 已修复。 触发条件改为 typeof effort === 'string' && effort !== 'none',与 pipeline 自身对 reasoning_effort 的守卫一致;显式的 'none' 禁用不再剥除任何旋钮。
  4. 组合形态测试缺口(Stage 3 item 3)— 已修复。 新增 pipeline 测试将真实DashScopeOpenAICompatibleProvider.buildRequest 接入 pipeline 门控(glm-5.2 + extra_body: { enable_thinking: true, reasoning_effort: 'high' } + 强制工具):预设开关在 provider 剥除后幸存,随后触发 pipeline 的 enable_thinking === true 分支。表格中仅测门控的行仍保留 mock 版 buildRequest。同时补充 provider 层的 glm-5.2 用例(可直接捕获 C1)与视觉分支 thinking_budget 用例,使视觉路径保有剥除覆盖。变异检查(将剥除还原为修复前未加门控的存在性形态)恰好使新增/反转的 7 个测试变红,证明其承重。
  5. 文档行(Stage 3 item 4)— 已修复。 DashScope 行现在说明 qwen3.8-max 家族下 enable_thinkingthinking_budget 均会被剥除(带 debug 日志),并描述旧 qwen 模型的透传/仅剥 thinking_budget 行为。
  6. 剥除时输出 debug 日志(行内建议 rc:3707939839)— 已实现。 剥除时输出 debugLogger.debug('DashScope: dropping thinking knobs that conflict with reasoning_effort', { model, reasoningEffort, dropped }),与 pipeline 的 tool_choice 剥除日志对齐;由旗舰预设剥除测试中的断言与 glm 用例中的"无日志"断言固化。
  7. 在 pipeline 剥除的 debug payload 中加入 tool_choice(小项)— 拒绝。 该分支只在 tool_choice === 'required' 时触发,且日志消息本身已写明(dropping tool_choice=required while thinking is enabled);加入 payload 字段只是常量,不携带信息。
  8. 真实 DashScope 冒烟请求(item 4)— 本环境无法执行。 此环境无任何 DashScope 凭证(PR 正文已声明)。且影响半径现已收窄:qwen3.8-max* 之外的出网形态即修复前的透传形态,关于单旋钮形态的服务端假设即使有误,也只影响触发本次修复的家族。
  9. 未加门控的兄弟条件 enable_thinking === true(Stage 2 观察项)— 超出范围。 评审者已明确留待后续 issue,未改动。
  10. PR 正文测试计数(小项)— 仅说明。 正文写于本轮之前;本轮之后,PR 的新测试为:provider 侧新增 6 个 + 反转 2 个,pipeline 侧新增 1 个(另有此前的 3 行 pipeline 表格用例)。

变更文件

  • packages/core/src/core/openaiContentGenerator/provider/dashscope.ts — 新增 isQwenFamilyWireModel helper;家族门控、值检查、分档剥除的 dropConflictingThinkingKnobs(带 debug 日志);两处调用点传入出网模型;buildQwenEffortConfig 改用 helper。
  • packages/core/src/core/openaiContentGenerator/pipeline.ts — 两处门控改用共享 helper(coder-model 的由来说明移入 helper 的 JSDoc)。
  • packages/core/src/core/openaiContentGenerator/provider/dashscope.test.ts — 反转 2 个测试、新增 4 个、debug 日志断言。
  • packages/core/src/core/openaiContentGenerator/pipeline.test.ts — 新增 1 个"真实 provider 剥除 → pipeline 门控"组合测试。
  • docs/users/configuration/model-providers.md — 更新 DashScope 行。

冲突说明

无——分支与 origin/main 可干净合并(已合并;--conflict false)。

验证

  • npm run build — 通过
  • npm run typecheck — 通过
  • npm run lint — 通过
  • 对五个变更文件执行 npx prettier --check — 通过(文档表格经 prettier --write 重新对齐)
  • cd packages/core && npx vitest run src/core/openaiContentGenerator/provider/dashscope.test.ts src/core/openaiContentGenerator/pipeline.test.ts — 2 个文件,230/230 通过
  • cd packages/core && npx vitest run src/core/openaiContentGenerator src/core/contentGenerator.test.ts src/providers — 32 个文件,888/888 通过
  • cd packages/core && npx vitest run(packages/core 全套件)— 19198 个测试:19106 通过 / 82 失败 / 10 跳过。全部 82 个失败均为本运行器既有的环境性失败:它们分布在本 PR 未改动的十二个测试文件中(经 git diff origin/main...HEAD --name-only 核实),这些文件测试的源码同样未改动,且均不 import 变更模块;每条失败信息都显示运行器环境泄漏——QWEN_HOME(此处被设为 /home/runner/work/_temp/qwen-autofix-review-home)在 storage.ts:184 覆盖了基于 os.homedir() 的期望值,真实 installation_id 文件使 mocked uuid 失效。PR 自身的沙箱验证已记录同类失败在 base 臂逐字节一致。PR 触及的全部区域为绿(上方 888/888)。
  • 变异检查:将剥除临时还原为修复前未加门控的存在性形态——恰好新增/反转的 7 个测试失败(其余 223 个全部通过);恢复修复后复核为绿
  • 集成测试:不适用——变更行为(provider 请求整形)由单测套件直接覆盖,并非只经 bundled CLI 执行

Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。


🧠 Handled by Qwen Code · model/模型 qwen3.8-max

@wenshao

wenshao commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Review: fix(core): harden Qwen 3.8 reasoning effort wire shape

Overview. Four follow-up fixes to #8472's DashScope wire shape: (1) a new dropConflictingThinkingKnobs pass in DashScopeOpenAICompatibleProvider.buildRequest that removes enable_thinking/thinking_budget after the extra_body merge when an effort tier ships; (2) family-gating the pipeline's tool_choice: 'required' strip on the reasoning_effort clause; (3) prefix-matching the tiered family (qwen3.8-max*) so snapshots/aliases keep their tier; (4) a shared isQwenFamilyWireModel predicate, a debug log, and docs.

Verification I ran — worktree at 6c26ad7, packages/core: vitest run provider/dashscope.test.ts pipeline.test.ts230 passed. Plus four probe cases against the real provider, results inline below.

What's right

  • The prefix match is the correct fix and is genuinely consistent with modalityDefaults.ts:48 (/^qwen3\.8-max/). The exact-match gate really did drop the tier for -latest/dated ids.
  • Hoisting isQwenFamilyWireModel removes the drift between the disable gate and the effort mapping, and lowercasing inside the predicate is a small correctness win (verified: Qwen3.8-Max now ships reasoning_effort).
  • Running the drop after the extra_body merge is the right insertion point — the preset case in feat(core): support Qwen 3.8 reasoning effort #8472 was real (presets/alibaba-token-plan.ts:36 gives qwen3.8-max-preview enableThinking: true, which provider-config.ts:70 turns into extra_body.enable_thinking).
  • The new pipeline test that swaps in the real provider is a good move — the existing table mocks buildRequest as a plain merge, so it never exercised the production path.

Findings

1. An explicit extra_body.enable_thinking: false is deleted, silently turning thinking back on. dashscope.ts:392 tests presence ('enable_thinking' in merged), not value. Probe on qwen3.8-max + reasoning: { effort: 'high' } + extra_body: { enable_thinking: false }:

reasoning_effort: "high",  'enable_thinking' in result: false

The user asked for thinking off via the documented escape hatch and gets reasoning_effort: 'high' with the off-switch gone. This is inconsistent with the care taken two lines up, where 'none' is deliberately value-checked precisely because it's an explicit disable (dashscope.ts:374-379). If a false reaches here it should either be honoured (map to reasoning_effort: 'none', which the pipeline already preserves at pipeline.ts:922) or left alone for the pipeline's disable path — not dropped.

2. On legacy hybrids the drop removes the knob the model reads and keeps the one it ignores. qwen3.7-max + extra_body: { thinking_budget: 4096, reasoning_effort: 'max' }, no config reasoning:

reasoning_effort: "max",  thinking_budget: undefined,  enable_thinking: undefined

The PR's own rationale says legacy hybrids "read enable_thinking, not reasoning_effort". By that logic reasoning_effort is the inert field here and thinking_budget is the meaningful one — yet the meaningful one is dropped, and with no config tier there's no enable_thinking either, so the wire carries a single ignored parameter and nothing else. If the pair really is server-rejected, dropping reasoning_effort on non-qwen3.8-max ids preserves user intent and still satisfies the constraint. (The existing test only covers the variant where a config tier supplies enable_thinking: true, which masks this.)

3. The tool_choice family gate doesn't cover the motivating case. The PR's stated harm is degraded forced-tool side queries (permission classifier, compaction, title generation) on non-qwen models. But pipeline.ts:966's enable_thinking === true clause is still ungated, and every non-qwen Token Plan preset ships exactly that (glm-5.2, glm-5.1, glm-5, kimi-k2.* all carry enableThinking: true). The new test at pipeline.test.ts asserts and locks in that glm-5.2 still loses tool_choice: 'required' — so those side queries remain degraded by the same code path this PR set out to protect. Either the DashScope constraint is endpoint-wide (in which case the reasoning_effort clause arguably shouldn't be gated either) or it's qwen-specific (in which case the enable_thinking clause needs the same gate). Worth resolving explicitly rather than leaving the two clauses on opposite sides of the argument.

4. Disable-path asymmetry on the tiered family. Under reasoning: false / includeThoughts: false, pipeline.ts:885 still writes enable_thinking: false for qwen3.8-max (non-preview, so not thinkingMandatory) — the very boolean this PR argues that family doesn't read. The new test rows codify it (expectedThinking: false). reasoning_effort: 'none' is already preserved by pipeline.ts:922, so the family-consistent disable shape is available.

5. debug vs warn for a silent user-config drop. dashscope.ts:401 logs at debug. This discards explicitly user-supplied extra_body keys, and extra_body is documented as the escape hatch that wins (model-providers.md:598 tells users to inject thinking knobs there). The comparable Anthropic 'max' → 'high' clamp uses debugLogger.warn. Suggest warn, once per generator.

6. Layering / DRY (minor).

  • isQwenFamilyWireModel is a model-family fact, but it now lives on the DashScope provider class and is called from pipeline.ts:882 inside the non-DashScope (vLLM/SGLang) branch. Consider modalityDefaults.ts or a models/ util as its home.
  • (model ?? this.contentGeneratorConfig.model ?? '').toLowerCase() and startsWith('qwen3.8-max') are each duplicated across buildQwenEffortConfig and dropConflictingThinkingKnobs — two small privates (resolveWireModel, isTieredEffortModel) would keep the family definition single-sourced, which is the stated point of the refactor.
  • The visionMerged/merged blocks are now identical modulo the extras object; they could share a tail helper.

7. Test gaps. No coverage for findings 1 and 2 — both are one-liner additions to dashscope.test.ts and both would pin whichever semantics you settle on. Also: preserve_thinking: true is still spread unconditionally alongside the tier, so "the tier ships alone" isn't literally true — intentional, or should it join the drop list for qwen3.8-max*?

Docs

The rewritten row is accurate against the code, including the "only a conflicting thinking_budget is dropped" clause for older Qwen models. Two things it doesn't say and probably should: that a user-supplied enable_thinking is dropped regardless of its value (finding 1), and that this is one of the few places extra_body does not win.

Risk

Low blast radius (DashScope-only, no API surface change), and the correctness of the prefix fix is not in doubt. The residual risk is concentrated in findings 1–2: both silently discard explicit user configuration, and the underlying premise (DashScope rejects reasoning_effort + thinking_budget) is listed in the PR as not live-validated. Given that, honouring the user's field and letting the server reject is arguably safer than guessing which field to delete.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.8-max via Qwen Code /review (v0.21.5)

Comment thread docs/users/configuration/model-providers.md
Comment thread packages/core/src/core/openaiContentGenerator/provider/dashscope.ts
Comment thread packages/core/src/core/openaiContentGenerator/provider/dashscope.ts
Comment thread packages/core/src/core/openaiContentGenerator/provider/dashscope.ts

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — verified: 230/230 tests pass on changed files, 721/721 on full openaiContentGenerator dir, typecheck + ESLint clean. The four fixes do what they claim: competing-knob drop is correct (post-merge, both branches), prefix match covers snapshots/aliases consistently with modalityDefaults, tool_choice gate correctly family-scoped, disable-path refactor semantically identical.

Non-blocking:

  1. (P2) PR body says 'the explicit tier wins and the boolean is dropped — the wire never carries both knobs' for the legacy-model override, but the code/test/docs all say enable_thinking is KEPT on legacy qwen models. Code is correct; fix the PR body wording before squash so the commit record isn't misleading.
  2. (P2) The enable_thinking===true clause in pipeline.ts still strips tool_choice for non-qwen presets (glm-5.2). The PR's own rationale for gating the reasoning_effort clause ('opaque sampling override, not a thinking switch') applies equally here. Confirm DashScope actually rejects tool_choice=required + enable_thinking=true for non-qwen models, or family-gate that clause too.
  3. (P3) 'none' exemption can ship reasoning_effort:'none' + enable_thinking:true together — add a comment confirming server rejection is tier-scoped.
  4. (P3) No direct unit test for isQwenFamilyWireModel edge cases (undefined/case).

@wenshao

wenshao commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ⚠️ incomplete — infrastructure failure - workflow run

The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details.

中文 — 判定:⚠️ 未完成 · 基础设施故障

验证作业未完成(检出、runner 或初始化错误),未生成报告。详见工作流运行日志。

Qwen Code · sandboxed verification

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Aug 4, 2026
Merged via the queue into QwenLM:main with commit 874e46d Aug 4, 2026
109 of 110 checks passed
wenshao added a commit to wenshao/qwen-code that referenced this pull request Aug 4, 2026
…M#8488 round 2)

Second review round on QwenLM#8488:

- Honour an explicit extra_body enable_thinking: false on the qwen3.8-max
  family as reasoning_effort: 'none' instead of silently deleting it and
  re-enabling thinking
- Legacy qwen hybrids now drop the inert reasoning_effort override when it
  conflicts with a meaningful thinking_budget, keeping the knobs the model
  actually reads
- Family-gate the pipeline's enable_thinking tool_choice clause like the
  reasoning_effort clause: on non-qwen models sharing the endpoint the
  field is an opaque no-op (GLM reads thinking.enabled), and stripping
  forced tool selection there degraded their side queries
- The tier-native disable path emits reasoning_effort: 'none' — the knob
  the family reads — instead of enable_thinking: false; the
  required-thinking retry trigger recognises the new shape so runtime
  learning still fires
- Warn once per generator (not debug per request) when user extra_body
  knobs are dropped; hoist the wire-model family predicates to
  modalityDefaults.ts and share the provider's extra_body merge tail
- Tests for every behavior above (all load-bearing, verified by targeted
  mutation); docs attribute the vendor rejection to thinking_budget only
  and document the extra_body exceptions
@wenshao

wenshao commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Round 2 response — 0db82047f5

All seven findings addressed; every new/flipped test verified load-bearing by targeted mutation (reverting each fix turns exactly its tests red). Verification: packages/core targeted suites 942/942, full npm run typecheck, targeted eslint, prettier --check — all clean.

1. Explicit enable_thinking: false deleted — fixed. The drop value-checks the switch on the tier-native family: an explicit false is honoured as the family's canonical disable — reasoning_effort becomes 'none' (preserved by the pipeline's strip, as you noted), the boolean is removed, thinking_budget likewise. Your probe shape now ships reasoning_effort: 'none' alone. New test pins it.

2. Legacy hybrids dropped the knob the model reads — fixed. Non-tier-native qwen models now drop the inert reasoning_effort when a thinking_budget is present, keeping the knobs the model reads. Covers both variants: the config-tier + override case and your no-config probe (qwen3.7-max + extra_body: { thinking_budget: 4096, reasoning_effort: 'max' } now ships thinking_budget: 4096 alone). Three tests (text branch, vision branch, no-config probe).

3. tool_choice gate asymmetry — resolved: family-gate both field clauses. Evidence trail: the #7659 400 is conditioned on "thinking mode"; enable_thinking is a qwen thinking switch — the disable-path evidence records that GLM reads thinking.enabled and DeepSeek thinking.type, so on glm/kimi the field is an opaque no-op that does not put the request in thinking mode, and stripping required there was pure harm (the motivating harm of this PR, still happening on every glm/kimi Token Plan side query). Both field clauses are now family-gated; thinkingMandatory stays ungated as explicit thinking-on knowledge. The glm end-to-end test now asserts tool_choice: 'required' survives. Residual risk, stated plainly: if the DashScope gateway does read enable_thinking on kimi presets and rejects required in thinking mode, kimi side queries would 400 instead of silently degrading — no codebase or live evidence supports that reading, while the status quo was observably discarding forced tools for non-qwen presets.

4. Disable-path asymmetry — fixed. The DashScope disable for the tier-native family deletes enable_thinking/thinking_budget and emits reasoning_effort: 'none' — the knob the family reads. This surfaced one interaction: the required-thinking retry trigger keyed on enable_thinking === false on the wire, so it now also recognises reasoning_effort === 'none'; both retry tests (execute + executeStream) re-pin runtime learning with the new shape — reverting the disable shape turns all four tests red.

5. debug vs warn — fixed. debugLogger.warn, once per generator (a conflictingKnobDropWarned flag), pinned by a two-build assertion in the flagship test.

6. Layering / DRY — fixed. isQwenFamilyWireModel + isTieredEffortWireModel moved to modalityDefaults.ts (next to the family regexes; neutral home since the pipeline calls them in the non-DashScope branch too); the DashScope static is gone. resolveWireModel dedupes the wire-model resolution; mergeExtraBodyAndResolveKnobs shares the vision/text merge tail.

7. Test gaps — closed. Tests for findings 1 and 2 (above), plus the three inline suggestions: multi-knob drop test (enable_thinking + thinking_budget together), snapshot/alias drop assertions, non-qwen budget-gate test. preserve_thinking: intentional — it controls multi-turn thinking-content retention (added by #7045 for reasoning continuity), orthogonal to the thinking on/off knob; not added to the drop list.

Docs — rewritten: the vendor rejection is attributed to thinking_budget only (the enable_thinking + reasoning_effort pair was live-accepted in #8472's E2E — no rejection claim); documents the enable_thinking: false honoring and flags this as an extra_body-does-not-win-verbatim exception.

Risk — agreed, and round 2 is strictly more conservative: outside qwen3.8-max* the wire is pass-through (non-qwen) or keeps the model-read knobs (legacy); only the vendor-documented rejected pair and competing-knob shapes are resolved; an explicit user opt-out now wins instead of being deleted. The un-live-validated surface shrank to the single family that motivated the fix.

中文说明

第 2 轮处理 — 0db82047f5

七项发现全部处理;每个新增/反转测试均经定向变异验证承重(还原对应修复恰好使其变红)。验证:packages/core 定向套件 942/942、全仓 npm run typecheck、定向 eslintprettier --check 全部干净。

1. 显式 enable_thinking: false 被删 — 已修复。 tier-native 家族对该开关做值检查:显式 false 被尊重为家族的标准禁用形态——reasoning_effort 变为 'none'(pipeline 的剥除逻辑按你指出的那样保留 'none'),布尔被移除,thinking_budget 同样处理。你的探针形态现在只发 reasoning_effort: 'none',新测试钉住。

2. 旧 hybrid 剥掉了模型读取的旋钮 — 已修复。 非 tier-native qwen 模型在与 thinking_budget 冲突时改为剥掉惰性的 reasoning_effort,保留模型读取的旋钮。覆盖两种变体:配置档位 + 覆盖、以及你的无配置探针(qwen3.7-max + extra_body: { thinking_budget: 4096, reasoning_effort: 'max' } 现在只发 thinking_budget: 4096)。三个测试(文本分支、视觉分支、无配置探针)。

3. tool_choice 门控不对称 — 已明确解决:两个字段条款都加家族门控。 证据链:#7659 的 400 以「thinking mode」为条件;enable_thinking 是 qwen 的思考开关——disable 路径的既有证据记录 GLM 读 thinking.enabled、DeepSeek 读 thinking.type,因此在 glm/kimi 上该字段是不透明的空操作,不会把请求置入思考模式,在那里剥掉 required 纯属损害(正是本 PR 要保护的动机场景,却仍发生在每个 glm/kimi Token Plan 侧查询上)。两个字段条款现在都加了家族门控;thinkingMandatory 作为显式的「思考开启」知识保持不加门控。glm 端到端测试现在断言 tool_choice: 'required' 保留。残留风险,明确陈述: 若 DashScope 网关确实在 kimi 预设上读取 enable_thinking 并在思考模式下拒绝 required,kimi 侧查询将以 400 替代静默退化——代码库与实测均无证据支持该解读,而现状是可观察地在非 qwen 预设上丢弃强制工具。

4. Disable 路径不对称 — 已修复。 DashScope 上 tier-native 家族的 disable 删除 enable_thinking/thinking_budget 并发出 reasoning_effort: 'none'——家族读取的旋钮。这暴露了一个交互:required-thinking 重试触发条件以 wire 上 enable_thinking === false 为键,现在同时识别 reasoning_effort === 'none';execute + executeStream 两个重试测试以新形态重新钉住运行时学习——还原 disable 形态使全部四个测试变红。

5. debug vs warn — 已修复。 debugLogger.warn,每 generator 一次(conflictingKnobDropWarned 标志),由旗舰测试的两次构建断言钉住。

6. 层析 / DRY — 已修复。 isQwenFamilyWireModel + isTieredEffortWireModel 移入 modalityDefaults.ts(与家族正则相邻的中立位置——pipeline 在非 DashScope 分支也调用它们);DashScope 类 static 移除。resolveWireModel 收敛 wire 模型解析;mergeExtraBodyAndResolveKnobs 共享 vision/文本合并尾部。

7. 测试缺口 — 已补齐。 发现 1、2 的测试(见上),另加三条行内建议:多旋钮剥除测试(enable_thinking + thinking_budget 同发)、快照/别名剥除断言、非 qwen budget 门控测试。preserve_thinking:有意保留——它控制多轮思考内容保留(#7045 为推理连续性引入),与思考开/关旋钮正交;不加入剥除列表。

文档 — 已重写:供应商拒绝只归因于 thinking_budgetenable_thinking + reasoning_effort 组合在 #8472 的真实 E2E 中被接受——不作拒绝声明);记录 enable_thinking: false 被尊重,并标明此处 extra_body 不原样生效的例外。

风险 — 认同,且第 2 轮严格更保守:qwen3.8-max* 之外的出网是透传(非 qwen)或保留模型读取的旋钮(旧模型);只有供应商文档化的拒绝组合与竞争旋钮形态被消解;显式的用户关闭现在获胜而不是被删除。未经实测验证的面收窄到唯一触发本次修复的家族。

@wenshao

wenshao commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

The round-2 fixes referenced in my response comment above (0db82047f5) landed on the fork after this PR was merged, so they are not in this merge — they are carried by the follow-up PR #8536 (rebased onto the post-merge main as 440047a928, re-verified: targeted suites 942/942 + full typecheck).

上面回复中引用的 round 2 修复(0db82047f5)在本 PR 合并后才推送到 fork,未包含在本次合并中——由后续 PR #8536 承接(已基于合并后的 main 重建为 440047a928 并重新验证:定向套件 942/942 + 全仓 typecheck)。

NickF93 pushed a commit to NickF93/qwen-code that referenced this pull request Aug 4, 2026
…M#8488 round 2) (QwenLM#8536)

Second review round on QwenLM#8488:

- Honour an explicit extra_body enable_thinking: false on the qwen3.8-max
  family as reasoning_effort: 'none' instead of silently deleting it and
  re-enabling thinking
- Legacy qwen hybrids now drop the inert reasoning_effort override when it
  conflicts with a meaningful thinking_budget, keeping the knobs the model
  actually reads
- Family-gate the pipeline's enable_thinking tool_choice clause like the
  reasoning_effort clause: on non-qwen models sharing the endpoint the
  field is an opaque no-op (GLM reads thinking.enabled), and stripping
  forced tool selection there degraded their side queries
- The tier-native disable path emits reasoning_effort: 'none' — the knob
  the family reads — instead of enable_thinking: false; the
  required-thinking retry trigger recognises the new shape so runtime
  learning still fires
- Warn once per generator (not debug per request) when user extra_body
  knobs are dropped; hoist the wire-model family predicates to
  modalityDefaults.ts and share the provider's extra_body merge tail
- Tests for every behavior above (all load-bearing, verified by targeted
  mutation); docs attribute the vendor rejection to thinking_budget only
  and document the extra_body exceptions
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants