Skip to content

fix(core): support qwen3.8 side queries on DashScope - #7303

Merged
wenshao merged 9 commits into
QwenLM:mainfrom
yiliang114:cx/fix-7270-tokenplan-side-query-thinking
Jul 21, 2026
Merged

fix(core): support qwen3.8 side queries on DashScope#7303
wenshao merged 9 commits into
QwenLM:mainfrom
yiliang114:cx/fix-7270-tokenplan-side-query-thinking

Conversation

@yiliang114

@yiliang114 yiliang114 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This change treats qwen3.8-max-preview as a mandatory-thinking model on DashScope-compatible routes. Side queries no longer override it with enable_thinking: false. Structured requests also let the model select the response tool automatically because its thinking mode rejects tool_choice: required. Other Qwen models and non-DashScope routes retain their existing behavior. For mandatory-thinking models, the compatibility flag also takes precedence over a global reasoning: false setting because the endpoint rejects the opt-out. Runtime model switches also clear this capability when a same-provider side/fast model does not declare it, while provider preset updates refresh the matching active generator and invalidate per-model generator caches so the fix takes effect without a restart. Prompt hooks with a model override now resolve that model's generator and generation settings instead of reusing the active generator; resolution and generation share the existing hook timeout and cancellation budget.

Why it's needed

Token Plan users selecting qwen3.8-max-preview as their fast or main model currently see side-query failures in features such as web fetching, permission classification, subagent generation, suggestions, summaries, compression, and related helpers. The model requires thinking, while Qwen Code's side-query pipeline normally disables thinking to reduce latency and token use. Structured side queries have a second incompatibility because thinking mode does not accept forced tool selection.

Reviewer Test Plan

How to verify

Configure a DashScope-compatible provider with qwen3.8-max-preview, then run both a text-only side query and a schema-based side query. The text request should preserve thinking and complete. The schema-based request should preserve thinking, omit forced tool selection, and return the requested structured result. Confirm that another Qwen model still disables thinking and that global reasoning: false still sends the explicit opt-out for models that do not declare thinking as mandatory. For a mandatory-thinking model, confirm that the request keeps thinking enabled instead of sending a guaranteed-invalid opt-out. Update an existing Token Plan provider and confirm the fix takes effect in the current session without a restart. With qwen3.8 as the main model and another same-provider model as the side/fast target, confirm the target does not inherit mandatory thinking and still disables thinking when supported.

Evidence (Before & After)

Before: a real Token Plan request with enable_thinking: false returned HTTP 400 with The value of the enable_thinking parameter is restricted to True. A structured request with thinking enabled plus tool_choice: required also returned HTTP 400 because forced tool selection is unsupported in thinking mode.

After: the equivalent text request preserves thinking, and the structured request omits forced tool selection. A real protocol request completed with HTTP 200 and returned a valid respond_in_schema tool call.

Tested on

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

Environment (optional)

Local TypeScript workspace (full monorepo build and typecheck) and the live Alibaba ModelStudio Token Plan compatible API.

Risk & Scope

  • Main risk or tradeoff: DashScope qwen3.8-max-preview side queries must incur the thinking latency and token cost required by the model. Structured calls use automatic rather than forced tool selection, with existing schema validation and fail-closed behavior unchanged. Provider preset updates now refresh the matching active generator and invalidate per-model caches, so an active-provider refresh failure rolls back the update through the existing install transaction.
  • Not validated / out of scope: Other models and non-DashScope providers are intentionally unchanged.
  • Breaking changes / migration notes: None.

Linked Issues

Closes #7270

Closes #7332

Supersedes #7333

Related to #7284

中文说明

本 PR 做了什么

这个改动把 qwen3.8-max-preview 作为 DashScope 兼容路由上的强制思考模型处理。Side query 不再用 enable_thinking: false 覆盖它。结构化请求也改为让模型自动选择响应工具,因为该模型的思考模式会拒绝 tool_choice: required。其他 Qwen 模型和非 DashScope 路由保持原有行为。对于强制思考模型,兼容性标记也会优先于全局 reasoning: false,因为发送关闭参数会被端点拒绝。运行时切换到未声明该能力的同 provider side/fast 模型时也会清除该标记;provider preset 更新则会刷新精确匹配的当前 generator 并失效 per-model generator 缓存,使修复无需重启即可生效。带 model override 的 prompt hook 现在也会解析目标模型对应的 generator 与 generation settings,而不是复用当前 active generator;模型解析和生成共用既有的 hook timeout 与取消预算。

为什么需要

当 Token Plan 用户把 qwen3.8-max-preview 设为 fast model 或 main model 时,web fetch、权限分类、子代理生成、建议、摘要、压缩等依赖 side query 的功能会失败。该模型强制要求思考,而 Qwen Code 的 side query 流程通常会为了降低延迟和 token 消耗而关闭思考。结构化 side query 还有第二个兼容问题:思考模式不接受强制工具选择。

Reviewer Test Plan

如何验证

在 DashScope 兼容 provider 上配置 qwen3.8-max-preview,分别运行纯文本 side query 和基于 schema 的 side query。纯文本请求应保持思考并成功完成;结构化请求应保持思考、移除强制工具选择,并返回要求的结构化结果。同时确认其他 Qwen 模型仍会关闭思考,且对于未声明强制思考的模型,全局 reasoning: false 仍会发送显式关闭参数;对于强制思考模型,请求应保持开启思考,避免发送必然无效的关闭参数。更新一个已安装的 Token Plan provider,确认修复在当前会话中无需重启即可生效。以 qwen3.8 作为主模型、另一个同 provider 模型作为 side/fast 目标时,确认目标模型不会继承强制思考标记,并在支持时仍会关闭思考。

证据(修改前后)

修改前:真实 Token Plan 请求携带 enable_thinking: false 时返回 HTTP 400,错误为 The value of the enable_thinking parameter is restricted to True.;结构化请求在开启思考并携带 tool_choice: required 时也返回 HTTP 400,因为思考模式不支持强制工具选择。

修改后:等价的纯文本请求会保持思考,结构化请求会省略强制工具选择。真实协议请求返回 HTTP 200,并得到有效的 respond_in_schema 工具调用。

测试平台

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

环境(可选)

本地 TypeScript 工作区(完整 monorepo 构建与类型检查)和真实的阿里云百炼 Token Plan OpenAI 兼容 API。

风险与范围

  • 主要风险或权衡:DashScope qwen3.8-max-preview side query 必须承担模型强制思考带来的延迟和 token 成本。结构化调用从强制工具选择降级为自动选择,现有 schema 校验和 fail-closed 行为保持不变。Provider preset 更新现在会刷新精确匹配的当前 generator 并失效 per-model 缓存,因此当前 provider 刷新失败时会通过现有安装事务回滚本次更新。
  • 未验证或超出范围:其他模型和非 DashScope provider 有意保持不变。
  • 破坏性变更或迁移说明:无。

关联 Issue

Closes #7270

Closes #7332

Supersedes #7333

Related to #7284

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed bug with clear reproduction. #7270 reports HTTP 400 (The value of the enable_thinking parameter is restricted to True.) on every side-query when qwen3.8-max-preview is the active model on a Token Plan endpoint. The PR also documents the second incompatibility (tool_choice: required rejected in thinking mode).

Direction: aligned — Token Plan users selecting qwen3.8-max-preview currently have broken side-queries (web fetch, permission classification, subagent generation, etc.). This is core functionality breakage for a supported model.

Size: 151 production logic lines (core: 137, cli: 14), 344 test lines. Well under thresholds. Cross-package (core + cli) but the cli change is small and self-contained.

Approach: the scope feels right. A model-level thinkingMandatory flag that flows through the existing preset → provider-config → content-generator-config → pipeline path is the minimal correct abstraction. The leak-prevention logic (explicitly clearing the flag for models that don't declare it) is important and well-placed. The promptHookRunner change (resolving the override model's own generator instead of reusing the active one) is necessary so hooks with a model override get the correct generation settings. The useProviderUpdates change (conditional auth refresh) prevents unnecessary auth churn when updating inactive providers.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,有明确复现。#7270 报告了当 qwen3.8-max-preview 作为 Token Plan 端点的活跃模型时,所有 side-query 返回 HTTP 400(The value of the enable_thinking parameter is restricted to True.)。PR 还记录了第二个兼容性问题(思考模式下 tool_choice: required 被拒绝)。

方向:对齐——Token Plan 用户选择 qwen3.8-max-preview 时,side-query(web fetch、权限分类、子代理生成等)全部失败。这是已支持模型的核心功能中断。

规模:151 行生产逻辑(core: 137, cli: 14),344 行测试。远低于阈值。跨包(core + cli)但 CLI 改动小且自包含。

方案:范围合理。模型级 thinkingMandatory 标记通过现有的 preset → provider-config → content-generator-config → pipeline 路径流转,是最小正确抽象。泄漏防护逻辑(对未声明该标记的模型显式清除)重要且位置正确。promptHookRunner 改动(解析 override 模型自己的 generator 而非复用 active generator)确保带 model override 的 hook 获得正确的生成设置。useProviderUpdates 改动(条件性 auth 刷新)防止更新非活跃 provider 时不必要的 auth 变动。

进入代码审查 🔍

Qwen Code · qwen3.7-max

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

@yiliang114
yiliang114 marked this pull request as draft July 20, 2026 07:58
@yiliang114 yiliang114 changed the title fix(core): support qwen3.8 Token Plan side queries fix(core): support qwen3.8 side queries on DashScope Jul 20, 2026
yiliang114 and others added 2 commits July 20, 2026 23:05
Hardcoding a single model id in the pipeline snapshots a server-side
constraint into client logic and breaks again on the next
thinking-mandatory model. Move the knowledge to preset data: a
thinkingMandatory generation-config flag marks models that reject
enable_thinking=false with a 400, so the pipeline never emits the
disable for them and drops forced tool selection while thinking is on.
Hybrid models that merely declare extra_body.enable_thinking keep the
side-query force-disable, preserving the QwenLM#4501 token savings.
@yiliang114
yiliang114 marked this pull request as ready for review July 21, 2026 02:15
@yiliang114
yiliang114 requested a review from tanzhenxin as a code owner July 21, 2026 02:15
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: given the problem (side-queries force enable_thinking: false which breaks thinking-mandatory models on DashScope), I would: (1) add a model-level capability flag indicating thinking cannot be disabled, (2) gate the pipeline's reasoning-disable logic on that flag, (3) handle the tool_choice: required incompatibility since thinking mode rejects forced tool selection, (4) propagate the flag through the existing preset → provider-config → content-generator-config pipeline, (5) ensure hooks with model overrides resolve the correct generator so the flag is respected there too.

Comparison with the diff: the PR matches this approach exactly. No simpler path missed.

Key observations:

  • The thinkingMandatory flag flows cleanly through ModelSpecbuildGenerationConfigMODEL_GENERATION_CONFIG_FIELDSContentGeneratorConfig. Each hop is minimal and follows existing patterns.
  • Leak prevention in content-generator-config.ts is correct — the || field === 'thinkingMandatory' condition ensures the flag is explicitly set to undefined for models that don't declare it, preventing inheritance from a parent config. The early modelId !== parentConfig.model guard also clears it before registry resolution.
  • Pipeline logic correctly gates both the enable_thinking: false emission (skipped when thinkingMandatory && model === configModel) and removes tool_choice: 'required' when thinking stays on. The model-match guard prevents a side-query targeting a different model from inheriting the flag.
  • The promptHookRunner change resolves the override model's own generator via resolveForModel instead of reusing the active generator. This ensures hooks that specify a model override get the correct generation settings (including thinkingMandatory). The resolution races against the existing timeout and abort budget — no new failure modes. The isReasoningModel signature change (accepting reasoningConfig as a parameter) is a clean refactor to support the resolved config.
  • The useProviderUpdates change (conditional auth refresh via providerMatchesCredentials) is a related improvement — previously auth was never refreshed on provider updates (doRefreshAuth: false), now it refreshes only when the active provider is updated.
  • The config.ts cache-clearing (clearPerModelGeneratorCache) ensures the new flag takes effect after provider config reload — necessary for runtime correctness.
  • No correctness bugs, security issues, or regressions found. No AGENTS.md violations.

Testing

Unit tests (all pass on the PR branch at ddde2454a):

  • pipeline.test.ts: 111 tests ✓ (includes 7 new parameterized cases covering mandatory thinking, non-mandatory, reasoning opt-out override, tool_choice removal, model override non-inheritance, aliased models)
  • content-generator-config.test.ts: 16 tests ✓ (includes 2 leak-prevention tests)
  • promptHookRunner.test.ts: 27 tests ✓ (includes model resolution, request shaping, and timeout-during-resolution tests)
  • config.test.ts: 405 tests ✓ (includes cache-clearing test)
  • useProviderUpdates.test.ts: 17 tests ✓ (includes inactive-provider no-refresh and pre-auth no-refresh tests)

Real-scenario smoke test (tmux, PR code via npm run dev):

$ npm run dev -- -p 'say hello' --output-format text

> @qwen-code/qwen-code@0.20.0 dev
> node scripts/dev.js -p say hello --output-format text

Hello! How can I help you today?

$ npm run dev -- -p 'What is 2+2? Use the calculator tool if available.' --output-format text

> @qwen-code/qwen-code@0.20.0 dev
> node scripts/dev.js -p What is 2+2? Use the calculator tool if available. --output-format text

2 + 2 = **4**

(No calculator tool is available in this session, but this one's straightforward.)

CLI starts and responds correctly with the PR code. Full before/after reproduction of the DashScope 400 error requires a Token Plan API key with qwen3.8-max-preview configured — the unit tests cover the wire-shape logic thoroughly (verifying enable_thinking and tool_choice values on the outgoing request object).

中文说明

代码审查

独立方案: 给定问题(side-query 强制 enable_thinking: false 导致 DashScope 上强制思考模型失败),我会:(1) 添加模型级能力标记表示思考不可关闭,(2) 在 pipeline 的推理禁用逻辑中门控该标记,(3) 处理 tool_choice: required 不兼容,(4) 通过现有管道传播标记,(5) 确保带 model override 的 hook 解析正确的 generator。

与 diff 对比: PR 完全匹配此方案。没有遗漏更简路径。

关键观察:

  • thinkingMandatory 标记通过 ModelSpecbuildGenerationConfigMODEL_GENERATION_CONFIG_FIELDSContentGeneratorConfig 干净流转。
  • 泄漏防护正确——|| field === 'thinkingMandatory' 条件和 modelId !== parentConfig.model 前置守卫确保标记不会跨模型继承。
  • Pipeline 逻辑正确门控 enable_thinking: false 发射并在思考保持开启时移除 tool_choice: 'required'。model-match 守卫防止针对不同模型的 side-query 继承标记。
  • promptHookRunner 改动通过 resolveForModel 解析 override 模型自己的 generator,确保 hook 获得正确的生成设置。解析与现有 timeout/abort 预算竞争——无新失败模式。
  • useProviderUpdatesconfig.ts 改动确保运行时正确性。
  • 未发现正确性 bug、安全问题或回归。无 AGENTS.md 违规。

测试

单元测试(PR 分支全部通过):

  • pipeline.test.ts: 111 个测试 ✓
  • content-generator-config.test.ts: 16 个测试 ✓
  • promptHookRunner.test.ts: 27 个测试 ✓
  • config.test.ts: 405 个测试 ✓
  • useProviderUpdates.test.ts: 17 个测试 ✓

真实场景冒烟测试(tmux,PR 代码):CLI 正常启动并响应。完整的 DashScope 400 错误 before/after 复现需要配置了 qwen3.8-max-preview 的 Token Plan API 密钥——单元测试已充分覆盖线路形状逻辑。

Qwen Code · qwen3.7-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, well-tested fix that matches the problem exactly; the two new commits (promptHookRunner model resolution + config inheritance prevention) close the remaining gaps from the initial pass. Only reservation remains the inability to verify against the live DashScope endpoint from CI.

This is a well-scoped fix for a real user-facing breakage. The thinkingMandatory flag is the right abstraction — it's a model capability declaration that flows through the existing config pipeline without adding new machinery. The leak prevention is now thorough: the flag is cleared both at the buildAgentContentGeneratorConfig level (when the target model differs) and at the applyResolvedModelConfig level (explicitly set to undefined when the registry doesn't declare it). The pipeline changes are correctly gated on model === configModel so a side-query targeting a different model never inherits the flag.

The new promptHookRunner change is the right call — without it, a hook with model: "qwen3.8-max-preview" would reuse the active generator (which might be configured for a different model), getting wrong generation settings. The resolution races against the existing timeout/abort budget, so no new failure modes are introduced. The isReasoningModel refactor (passing reasoningConfig as a parameter) is a clean consequence of this.

Every change in the diff is necessary for the stated goal. The test coverage is thorough: 7 new parameterized pipeline cases, 2 leak-prevention tests, 3 promptHookRunner tests (including timeout-during-resolution), 2 provider-update tests, and 1 cache-clearing test. All 576 tests across the changed files pass.

If I had to maintain this in six months, I'd thank the author — the flag is self-documenting, the comments explain the why (DashScope rejects the disable with a 400), and the test names read like specifications.

Approving. ✅

中文说明

置信度:4/5 — 干净、测试充分的修复,完全匹配问题;两个新提交(promptHookRunner 模型解析 + 配置继承防护)弥补了初次审查的剩余缺口。唯一保留仍是无法从 CI 验证 DashScope 实际端点行为。

这是一个范围良好的修复,解决了真实的用户功能中断。thinkingMandatory 标记是正确的抽象——它是模型能力声明,通过现有配置管道流转而不添加新机制。泄漏防护现在很彻底:标记在 buildAgentContentGeneratorConfig 层(目标模型不同时)和 applyResolvedModelConfig 层(注册表未声明时显式设为 undefined)都被清除。Pipeline 变更正确门控于 model === configModel,针对不同模型的 side-query 不会继承标记。

新的 promptHookRunner 改动是正确的——没有它,带 model: "qwen3.8-max-preview" 的 hook 会复用 active generator(可能配置了不同模型),获得错误的生成设置。解析与现有 timeout/abort 预算竞争,不引入新失败模式。

diff 中每个改动都是目标所必需的。测试覆盖充分:576 个测试全部通过。

批准。✅

Qwen Code · qwen3.7-max

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

@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. ✅

@yiliang114

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Review

Overview

This PR introduces a thinkingMandatory model-capability flag so qwen3.8-max-preview (which 400s on enable_thinking: false) works in side queries on DashScope routes. The plumbing is coherent end-to-end:

  • Preset (alibaba-token-plan.ts) → buildGenerationConfig → registry generationConfig, reaching both the main generator (via MODEL_GENERATION_CONFIG_FIELDS sync) and per-model side-query generators (via buildAgentContentGeneratorConfig).
  • Adding the flag changes the template hash (computeModelListVersion hashes the full model configs), so existing Token Plan users get the update prompt; accepting it now clears the BaseLlmClient per-model generator cache and — when the updated provider is the active one — refreshes auth so the live main generator picks up the flag too. Without those two changes the flag would be inert until restart, so the seemingly unrelated useProviderUpdates/reloadModelProvidersConfig changes are actually load-bearing.
  • The pipeline guards are placed after provider.buildRequest, so they see the merged extra_body — consistent with how the existing enable_thinking: false override works. The tool_choice deletion condition (=== 'required') matches the only forced shape the pipeline emits (fcMode === 'ANY''required').

Tests are meaningful: parameterized wire-shape assertions (including the reasoning: false precedence case and the hybrid-model regression guard), inheritance clearing, cache clearing, and active-vs-inactive refresh gating. CI is green.

Findings

1. thinkingMandatory still leaks through the non-registry fallback path (packages/core/src/models/content-generator-config.ts)

The clearing special-case (registryValue !== undefined || field === 'thinkingMandatory') lives only in applyResolvedModelConfig, which runs only when resolvedModel is found. getResolvedModel returns undefined for unregistered model ids (a documented case — "runtime models or unknown models"), and then buildAgentContentGeneratorConfig falls through to the plain {...parentConfig} spread with no clearing. Scenario: main model is qwen3.8-max-preview (thinkingMandatory: true), an agent targets a same-provider model id that isn't in the registry → the flag is inherited. For a hybrid model that means side queries keep thinking on (the latency/cost this pipeline works hard to avoid) and the DashScope block silently drops tool_choice: 'required' — reintroducing the exact fail-closed structured-output problem the forced tool choice exists to prevent. Suggest also clearing the flag in the fallback path when modelId is set and differs from parentConfig.model (inheriting when the agent reuses the parent's model is correct).

2. getContentGeneratorConfig() can be undefined before auth completes (packages/cli/src/ui/hooks/useProviderUpdates.ts)

Config.contentGeneratorConfig is declared with ! but only assigned in refreshAuth; other call sites (config.ts getModel(), getContextWindowSize()) defensively use ?. on the same getter. The new activeConfig.authType access has no guard. The provider-update prompt renders on mount, and a confirm racing startup auth (buffered Enter is enough) would throw TypeError inside executeUpdate — caught, but surfaced as a confusing "Failed to update provider configuration: Cannot read properties of undefined…" and the update is skipped for the session. Suggest config.getContentGeneratorConfig()?.authType === providerCfg.protocol and treating undefined as "not active" (a pre-auth session will build fresh config at auth time anyway, so skipping refreshAuth there is correct).

3. refreshAuth inside applyProviderInstallPlan adds new failure/side-effect surface for active-provider updates (informational)

Previously doRefreshAuth: false meant the update flow could not fail at the refresh step. Now a refreshAuth throw (step 'refreshAuth') triggers the full settings rollback of an otherwise-valid template update. It also fires the auth_success notification hook, so users with notification hooks get a spurious "Successfully authenticated" on accepting a provider update. Both seem acceptable, just flagging since they're behavior changes beyond the flag propagation itself.

4. Style: the string special-case for capability fields

field === 'thinkingMandatory' inline in the loop works, but the concept it encodes ("model capabilities never inherit across models") deserves a name — e.g. a MODEL_CAPABILITY_FIELDS list next to MODEL_GENERATION_CONFIG_FIELDS in constants.ts. The next capability flag added will otherwise likely miss this spot (and finding 1's fallback path, if fixed, would want the same list).

5. Docs

The pipeline comment says the flag can be "set by the preset, or by users via model generation config", but docs/users/configuration/model-providers.md (which documents generationConfig fields like extra_body/contextWindowSize) isn't updated. A short entry describing when to set thinkingMandatory would make the user-facing escape hatch discoverable.

6. Scope confirmation

The tool_choice deletion is gated on thinkingMandatory, but per the PR description DashScope rejects forced tool selection whenever thinking is enabled, model-independent. A hybrid model (e.g. qwen3.7-max) with thinking enabled plus a FunctionCallingConfigMode.ANY structured call would still send tool_choice: 'required' and 400. Side queries disable thinking on those models so this mostly can't bite today — just confirming the narrower gate is intentional (it does preserve the forced-tool behavior that protects hybrid models' structured outputs, which seems right).

Verdict

Solid, well-tested change with careful attention to propagation paths most PRs would miss. Finding 1 is worth fixing in this PR since it undermines the isolation guarantee the PR itself adds a test for; 2 is a cheap hardening; the rest are minor/optional.

@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. ✅

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Addressed findings 1 and 2 in b2d80e4075:

  • Unresolved same-provider target models now clear thinkingMandatory when the target model differs from the parent, with a focused regression test.
  • Provider updates now treat a missing pre-auth content-generator config as inactive instead of dereferencing it, also with a regression test.

Focused tests pass (content-generator-config.test.ts 16/16, useProviderUpdates.test.ts 17/17), along with the full monorepo build and typecheck. I also updated the PR body with the runtime update behavior and the #7332/#7333 relationship.

For the remaining notes: keeping refreshAuth inside the install transaction is intentional so persisted provider state and the active runtime cannot diverge; rollback on refresh failure is the desired behavior, and the notification-hook side effect is accepted. I’m keeping the direct thinkingMandatory special-case instead of adding a capability-list abstraction for one field. The broader hybrid-thinking/tool-choice behavior and user-facing provider docs are follow-ups rather than expanding this already multi-round PR. The narrow tool_choice gate is intentional: hybrid side queries disable thinking and retain forced tool selection for structured-output reliability.

@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Code Review — #7303 fix(core): support qwen3.8 side queries on DashScope

Verdict: LGTM. The change is correct, cleanly scoped, and genuinely covered by tests. Verified locally: all four affected test files pass (pipeline, content-generator-config, config, useProviderUpdates), and mutation-testing confirms the new tests are non-vacuous — neutering the !thinkingMandatory pipeline guard fails 3 pipeline cases, and dropping the leak-prevention clears fails both does not inherit mandatory thinking cases.

What it does

Marks qwen3.8-max-preview as a thinkingMandatory model. On DashScope routes this (a) never emits enable_thinking: false on the wire — even overriding a config-level reasoning: false opt-out, since the endpoint 400s on the disable — and (b) drops tool_choice: 'required' on structured side queries, because thinking mode rejects forced tool selection. A same-provider side/fast model no longer inherits the flag, and provider-preset updates now refresh the active generator + invalidate per-model generator caches so the fix lands without a restart.

Strengths

  • Consistent plumbing. thinkingMandatory is threaded through every layer the same way extra_body/reasoning already are (ModelSpecbuildGenerationConfigMODEL_GENERATION_CONFIG_FIELDStypesContentGeneratorConfig), so all the generic resolvers pick it up for free.
  • Careful capability isolation. Both leak paths in buildAgentContentGeneratorConfig are handled — the registry-match loop (|| field === 'thinkingMandatory') and the bare-model path (modelId !== parentConfig.model) — so the flag can't bleed into a side/fast model that doesn't declare it.
  • Reuse over reinvention. updatesActiveProvider uses the existing providerMatchesCredentials rather than a new ad-hoc match.
  • Wire-model gating (context.model) is reused consistently, so a request-level model override can't desync the gate.

Notes / risks (non-blocking)

  • Behavioral change in provider updates. Previously the update path passed doRefreshAuth: false, so an update never called refreshAuth. Now refreshAuth fires whenever the updated preset is the active provider. Two consequences worth being explicit about (the risk section covers the first):
    • An active-provider update now rolls back entirely if refreshAuth rejects (transient network / unreachable endpoint), where before it always persisted.
    • This adds a network round-trip to what was a local-only operation for every active-preset update (coding-plan too, not just token-plan). Both seem acceptable, just flagging the widened scope.
  • updatesActiveProvider first gate compares only providerCfg.protocol (not protocolOptions). Correct for the built-in presets this auto-update path handles; it wouldn't generalize to a multi-protocol custom provider, but those don't flow through here. And the failure mode is benign either way (wrongly-false → fix needs a restart; wrongly-true → a harmless extra rebuild).
  • Minor / style. The || field === 'thinkingMandatory' string literal inside the generic field loop works but won't scale — if another non-inheritable capability flag appears, a named NON_INHERITABLE_CAPABILITY_FIELDS set would read better than growing an || chain.
  • Tiny coverage gap (optional). No case asserts tool_choice: 'none'/absent is left untouched for a mandatory model — the code only deletes when === 'required', so this is safe, just untested.

Correctness spot-checks (all hold)

  • For a thinkingMandatory model under reasoningDisabled, the reasoning-strip block still runs but only touches reasoning/reasoning_effort/DeepSeek thinking — none of which control DashScope qwen thinking (that's enable_thinking, which stays true from the merged extra_body), so thinking is correctly preserved.
  • The tool_choice deletion is gated on thinkingMandatory && isDashScope && qwen, and tool_choice: 'required' is only ever set for fcMode === 'ANY' (structured side queries), so the main agentic loop is unaffected.
  • clearPerModelGeneratorCache() is invoked after the providers config is swapped in reloadModelProvidersConfig, so the next side query rebuilds against the new config.

wenshao
wenshao previously approved these changes Jul 21, 2026
@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@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. ✅

@yiliang114
yiliang114 dismissed stale reviews from qwen-code-ci-bot and wenshao via 0bdf03e July 21, 2026 04:53
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Fixed the Ubuntu CI failure by updating the prompt hook integration mock to match the resolved-model path. Verified with vitest run src/hooks/promptHookIntegration.test.ts, Prettier, and git diff --check.

@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Code Review — fix(core): support qwen3.8 side queries on DashScope

Reviewed the full diff (17 files, +471/−33). This is a focused, well-scoped fix with strong test coverage. Below is a section-by-section assessment.

Overview

The change introduces a thinkingMandatory model capability (set by the Alibaba Token Plan preset for qwen3.8-max-preview, and user-settable via model generation config). For such models on DashScope routes it:

  1. Never disables thinking on the wire (enable_thinking: false is a guaranteed HTTP 400), overriding both the side-query opt-out and the config-level reasoning: false.
  2. Drops tool_choice: 'required' for structured side queries, since thinking mode rejects forced tool selection — falling back to automatic selection with existing schema validation / fail-closed behavior unchanged.

It also wires up two supporting behaviors: provider-preset updates now refresh the active generator and invalidate per-model generator caches (live fix, no restart), and prompt hooks with a model override now resolve that model's own generator/generation settings instead of reusing the active one.

Correctness — looks solid

  • The wire-model gate is right. thinkingMandatory = config.thinkingMandatory === true && model === configModel correctly prevents a request.model override from inheriting mandatory-thinking, mirroring the reasoning behind the existing enable_thinking disable gating on context.model. Covered by the not inherit mandatory thinking through request.model overrides case.
  • Ordering is correct. The mandatory enable_thinking/tool_choice cleanup runs after both the reasoning-disable block and the provider buildRequest merge, so it correctly strips a preset- or user-injected enable_thinking: false and any tool_choice: 'required' set from functionCallingConfig.mode === 'ANY'. The drop a contradictory thinking disable for aliased mandatory models case exercises this.
  • Inheritance leak is closed on both paths. applyResolvedModelConfig now always writes thinkingMandatory (even when the registry value is undefined) so a sibling same-provider model can't inherit it, and the no-registry-match path clears it explicitly. Both paths are tested.
  • Prompt-hook abort/timeout budget is sound. A single timeoutPromise is shared across the resolveForModel race and the generateContent race, so resolution + generation share one budget as intended. Since abortPromise is always passed to a Promise.race before it can reject (there is no await between the execute() already-aborted guard and the first race on the same-model fast path), it can't become a floating unhandled rejection.
  • Cache invalidation is placed correctly in reloadModelProvidersConfig, and useProviderUpdates guards the new refreshAuth on providerMatchesCredentials + protocol match, with the pre-auth-init (getContentGeneratorConfig() === undefined) case handled.

Behavior changes worth calling out

  • Updating the active provider now triggers refreshAuth mid-session. Previously applyProviderInstallPlan was always called with doRefreshAuth: false, so a provider update never re-authed. Now the active provider is re-authed (generator recreated) so the fix lands without a restart. This is the intended mechanism and failure rolls back through the install transaction, but it is the highest-impact behavioral change in the PR — reviewers should confirm that recreating the generator mid-session has no adverse effect on any in-flight request bound to the old generator.
  • Structured side queries on mandatory models degrade to automatic tool selection. Dropping tool_choice: 'required' means the model may skip the respond_in_schema call and fail closed. This is unavoidable (the forced shape 400s) and is documented as an accepted tradeoff, but it is a real reliability change for those calls.
  • Side queries on a mandatory model now pay thinking latency/cost (web fetch, permission classifier, summaries, compression, …). Documented and unavoidable for this model.

Minor notes / suggestions (non-blocking)

  • Asymmetric DashScope gating. In the reasoning-disable path, the mandatory flag suppresses the disable regardless of isDashScope (via !thinkingMandatory), but the tool_choice: 'required' drop is gated on isDashScope. So a non-DashScope provider with a user-set thinkingMandatory keeps thinking on yet still sends forced tool selection. Today thinkingMandatory is only emitted by the DashScope preset, so this is latent, but if it becomes user-facing it may surprise. Worth a one-line comment noting the tool_choice drop is intentionally DashScope-only.
  • resolveForModel isn't cancelled on timeout. When the shared timeoutPromise wins the resolution race, the underlying resolveForModel promise keeps running to completion (its result is discarded). Harmless, but a stray generator may still be built after the hook has already reported cancelled.
  • Inheritance consistency nit. On the same-provider, no-registry-match path, nextConfig = {...parentConfig} still carries the parent's extra_body.enable_thinking: true to a custom model while thinkingMandatory is cleared. That's arguably the safer combination, but the two capability fields being treated differently is a subtle inconsistency worth a comment.

Test coverage — strong

  • The parametrized it.each in pipeline.test.ts covers the key matrix: mandatory keep-thinking, any-qwen-on-DashScope, override of reasoning: false, non-mandatory hybrid still force-disabled, auto tool selection, request-model-override non-inheritance, and aliased contradictory-disable. Good.
  • content-generator-config.test.ts covers both cross-provider and same-provider non-inheritance.
  • useProviderUpdates.test.ts covers active-refresh, inactive-no-refresh, and pre-init-no-refresh.
  • promptHookRunner.test.ts covers override resolution, request shaping for the resolved model, and timeout-during-resolution.

Verdict

Correct, conventional (matches the existing gating/comment style in pipeline.ts), and thoroughly tested. No blocking issues found. The main thing I'd want a second set of eyes on is the new active-provider refreshAuth on update — it's a genuine mid-session behavior change even though it's well-contained. The minor notes above are optional polish.

@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.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 21, 2026
Merged via the queue into QwenLM:main with commit e43ddac Jul 21, 2026
46 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.20.1.

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

Labels

None yet

Projects

None yet

3 participants