feat(web-shell): add model-specific reasoning controls - #8675
Conversation
E2E Test ReportEnvironment: macOS, Node.js 22, local WebShell development runtime, dark theme. Result: ✅ Passed.
The dedicated local verification harness and its model catalog were removed before submission and are not part of the PR file list. |
|
Thanks for the PR — clear description and a design doc, which helps a lot for a cross-cutting change like this.
Moving on to code review. 🔍 中文说明感谢贡献——描述清晰且附有设计文档，对这种跨模块改动帮助很大。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 6 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "session_reasoning_control" |
— Qwen Code · serve A/B
Code reviewMy independent baseline for "add thinking/effort controls for qwen3.8-max in WebShell" would have been: advertise No critical blockers found. The implementation is consistent with project conventions (kebab-case modules, collocated tests, defensive
sequenceDiagram
participant P1 as User (WebShell)
participant P2 as WebUI session actions
participant P3 as Daemon POST config-option
participant P4 as ACP bridge
participant P5 as QwenAgent
participant P6 as Session
participant P7 as Config + settings
P1->>P2: select effort tier or toggle thinking
P2->>P3: setConfigOption(configId, value)
P3->>P4: setSessionConfigOption
P4->>P5: setSessionConfigOption
P5->>P6: setEffort or setThinking (validated against registry)
P6->>P7: apply to live Config, persist model.reasoningPreferences
P5-->>P2: response configOptions
P6-->>P2: config_option_update event
P2->>P1: chip updates (Model · Extra High / Thinking Off)
The model-switch path is the second key flow: Files changed (30 of 51 shown)
Testing — the PR's own CI at the reviewed commitThe ubuntu
Not verified, explicitly: the unit suite on this commit (red gate above); the WebShell popover behavior in a real browser (unattended CI run — no live UI exercise here); the author's reported local test counts (Core 620, CLI/daemon 1,039, WebUI 66, ChatEditor 39) are the author's claim on macOS only, not independently re-run, and the PR body's Before/After screenshots are still unfilled placeholders. Sandboxed verification would settle the parts static review cannot: 中文说明代码审查我的独立基线方案（只读标题和动机时）大致是：agent 侧把 thinking/effort 作为 ACP session config option 暴露，UI 渲染会话上报的内容，按模型持久化偏好，并用 daemon 能力位兼容旧 daemon。PR 正是这么做的——另外加了三个各有理由的层：Core 能力注册表（让目录在会话建立前就能描述控件）、DashScope 线上档位钳制、以及 未发现关键阻塞项。实现符合项目约定（kebab-case、同目录测试、对 unknown 负载的防御性 mapper、中英 i18n、设计文档入库），并复用了既有原语（clampReasoningEffort、normalizeReasoningEffort、getPersistScopeForModelSelection）。要点：
时序图见上文英文版：用户选择档位或开关 → WebUI action → daemon 路由 → ACP bridge → QwenAgent → Session（按注册表校验）→ Config 与持久化 → 响应与 config_option_update 事件回流更新 chip。模型切换是第二条关键链路：setModel → 同步目标模型的持久化偏好 → 补发 configOptions 更新，web 端按事件顺序保留或等待目标状态。 测试（以 PR 自身 CI 为证据）ubuntu Test 任务红了，但失败不在测试套件——任务在测试前的门禁步骤挂掉：check:voice-guard-sync 脚本缺失。该脚本是 2026-08-06 由 #8350 进入 main 的，晚于本分支切出；PR 未触碰 package.json，属于分支过旧导致的失败而非代码问题——套件根本没有开始运行，也没有产出覆盖率产物。补救方式是把 main 合并进分支。由于 ubuntu 是矩阵门禁，macOS/Windows 测试为 skipped——当前 commit 在任何平台都没有绿色套件；web-shell 相关任务（visuals、Serve A/B、E2E smoke）在审查时仍在进行中。 明确未验证：本 commit 的单元测试（上述红色门禁）；真实浏览器中的 WebShell 弹层行为（无人值守 CI，不做实机 UI 操作）；作者自述的本地测试数字（Core 620、CLI/daemon 1,039、WebUI 66、ChatEditor 39）是作者在 macOS 上的单方陈述，未独立复核，且 PR 正文的 Before/After 截图仍是未填写的占位符。 沙箱验证可以补足静态审查看不到的部分：上文提到的 /verify 通道——config-option 变更与 config_option_update / model_switched 的顺序是否真的端到端保留了按模型的 reasoning 状态，是线上协议层面的断言，本 PR 的测试只是 mock，且本 commit 的 CI 根本没跑套件；WebShell 弹层界面本身（chip 文案、二级菜单布局、thinking 关闭时的禁用行）由于截图未贴出，需要维护者实际查看。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean static review and genuinely strong test coverage, but this is a 1,600-production-line cross-package core change, which policy holds at maintainer-awareness level, and the commit's CI is red — so a human takes it the rest of the way. Stepping back: the direction is right and the problem is real — Qwen 3.8 effort support already exists at the wire, and users have no model-specific way to touch it; both this repo's changelog and the reference agent's point the same way. The implementation is better than my independent baseline: the capability registry is small and honestly scoped, the old-daemon fallback is preserved by a capability flag, and the event-ordering hazard that this design creates (config_option_update racing model_switched) is not just handled but tested in both orders. The 1,131 lines of tests target exactly the places that would burn later — config-switch reasoning carry-over, mapper ordering, tier normalization, popover keyboard navigation. If I were maintaining this in six months, I'd thank the author for the design doc and the ordering tests. My reservations are about weight and evidence, not correctness. The PR spans seven packages for one registered model; the popover-into-flyout UX restructure was the natural seam to split, and the ⏸️ Deferring to @wenshao (CODEOWNER for
None of this reflects a defect found in the code — it's the gate doing its job on a large, core-touching PR with an unsettled CI state. 中文说明置信度：3/5 —— 静态审查干净、测试覆盖扎实，但这是一次约 1,600 行生产代码的跨包核心改动，按策略属于需要维护者知情的级别，且当前 commit 的 CI 是红的——后续需要人来拍板。 退一步看：方向正确、问题真实——Qwen 3.8 的 effort 支持在线上链路已经存在，用户却没有按模型触达它的入口；本仓库与参考产品的 changelog 都指向同一方向。实现优于我的独立基线方案：能力注册表小而克制，旧 daemon 降级由能力位保留，该设计引入的事件顺序风险（config_option_update 与 model_switched 竞态）不仅处理了，还按两种顺序都有测试。1,131 行测试恰好打在将来会痛的位置——模型切换的 reasoning 保留、mapper 顺序、档位归一、弹层键盘导航。如果六个月后由我维护，我会感谢作者的设计文档和顺序测试。 我的保留意见在体量与证据，而非正确性。PR 为一个注册模型横跨七个包；弹层改二级菜单的 UX 重构本是天然的拆分点，而它重做的 config.ts 模型切换逻辑正是本仓库反复修补过的区域（#7541、#8488、#8536）。产品问题仍未回答：注册默认值意味着用户从未选择时，每个 qwen3.8-max 请求都会带上 reasoning_effort=xhigh 且默认开启 thinking。行为层面的断言目前只有作者单方陈述——CI 在本 commit 上根本没跑套件（分支过旧的门禁失败），PR 正文的 Before/After 截图也仍是占位符。 ⏸️ 转交 @wenshao（packages/core/ 的 CODEOWNER）——需要维护者决定：
以上均非代码缺陷——这是门禁在大型、触及核心且 CI 未定的 PR 上正常履职。 — Qwen Code · qwen3.8-max Reviewed at |
|
CI failure root cause and fix:
A fresh CI run is now queued against the updated head. |
|
CI failure root cause and fix:\n\n- The failed job stopped at
Voice guard mirror check passed. because the PR head predated the script that the current base workflow invokes.\n- Synced the branch with latest via a normal merge commit (no force push, no feature-code workaround, no mock data added).\n- Verified locally: voice guard mirror check passed; Core 620 tests passed; CLI 1042 tests passed; WebUI 66 tests passed; WebShell 39 tests passed; production builds completed during
patch-package 8.0.1
Successfully copied files.
Building web-templates...
Successfully copied files.
Compiling input files...
Successfully copied files.
vite v5.4.21 building for production... [vite:dts] Start generate declaration files... dist/styles.css 83.64 kB │ gzip: 15.56 kB
vite v5.4.21 building for production...
Static assets synced -> dist/extension
Copied sandbox profiles to dist/ ✅ All bundle assets copied to dist/ added 160 packages, changed 4 packages, and audited 2069 packages in 2m 574 packages are looking for funding 67 vulnerabilities (3 low, 37 moderate, 24 high, 3 critical) To address issues that do not require attention, run: To address all issues (including breaking changes), run: Run |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
ReviewSingle-pass review of the 51-file diff. Focused on the non-test source files (~2.6k lines); test files and the design doc were read only for intent.
Trigger: attach to an already-loaded session ( Suggestion: A documented explicit Trigger: Suggestion: seed the preference from the resolved config, i.e. treat Model-switch completion can still drop the just-synced reasoning snapshot — The Suggestion: key the reset on the response payload / target model id rather than on the pre-existing A failed notification turns a successful model switch into a failure —
Suggestion: wrap the update in try/catch (debug-log on failure) so delivery never gates the mutation.
Not covered: the WebShell popover/submenu interaction (nested Radix popover focus + outside-click behaviour) — not verifiable from the diff; the |
|
Pushed a focused follow-up for the failing Node 22 test job. The failures came from four PR-introduced compatibility gaps: the ACP worktree test mock was missing the new core export, the serve capability expectation omitted Verification on
The follow-up contains no local model catalog, reasoning mock scenario, daemon fixture, screenshot artifact, or unrelated |
|
Thanks for the detailed review. All five findings were valid and are addressed in
Verification before merging main: Core 505/505, CLI 606/606, WebUI 240/240; focused ESLint, Prettier, and Core/CLI/WebUI typechecks passed. After merging current |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated) and its suite (integration-tests/, incl. the new qwen-serve-routes capabilities assertion) did not run in this review.
Not reviewed: build-and-test — the web-shell unit suite produced no verdict on either the PR tree or the merge-base tree (heap OOM measured pre-existing at the merge base; the red "Test (ubuntu-latest, Node 22.x)" check is its symptom).
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none (~11 of ~38 tool calls used; all planned checks completed).; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — finished within budget., and 5 more.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated) and its suite (integration-tests/, incl. the new qwen-serve-routes capabilities assertion) did not run in this review。
未审查：build-and-test — the web-shell unit suite produced no verdict on either the PR tree or the merge-base tree (heap OOM measured pre-existing at the merge base; the red "Test (ubuntu-latest, Node 22.x)" check is its symptom)。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none (~11 of ~38 tool calls used; all planned checks completed).；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — finished within budget.，另有 5 条。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const update = connectionRef.current.sessionId | ||
| ? sessionActions.setConfigOption(configId, value) | ||
| : (() => { | ||
| const scope = providersState.status?.modelConfigScope ?? 'user'; |
There was a problem hiding this comment.
[Suggestion] R8-12: the pre-session reasoning write falls back to 'user' scope whenever providersState.status is unavailable — but the daemon derives modelConfigScope from model-key ownership specifically for this key, and before providers status loads (startup race — the composer renders from the provider's own fetch while App's useProviders is a separate load) or after a providers-status failure, the write silently goes to user scope. — Concrete cost: in a trusted workspace that owns the model key, the user toggles a reasoning option before providers status resolves or after its load failed; the entry is written to user scope; if the workspace settings also define model.reasoningPreferences for that model, the workspace value wins in values.effective, so the control keeps showing the workspace value and appears dead — the exact shadowed-write hazard this PR's own workspace-providers-status test names.
Suggested fix: when providersState.status?.modelConfigScope is undefined, reject the write (the handler already rejects for missing models) rather than guessing 'user'.
中文说明
[Suggestion] R8-12：会话前的推理写入在 providersState.status 不可用时回退到 'user' scope —— 但 daemon 的 modelConfigScope 正是为这个键按 model 键归属推导的；在 providers 状态加载完成前（启动竞态——composer 用 provider 自己的 fetch 渲染，而 App 的 useProviders 是独立加载）或加载失败后，写入会静默落到 user scope。— 具体代价：在拥有 model 键的受信任工作区里，用户在 providers 状态未解析或加载失败时切换推理选项；条目被写入 user scope；若工作区 settings 也为该模型定义了 model.reasoningPreferences，workspace 值会在 values.effective 中胜出，控件持续显示 workspace 值、看起来像坏掉了——这正是本 PR 自己的 workspace-providers-status 测试命名的遮蔽写入风险。
建议修复：当 providersState.status?.modelConfigScope 为 undefined 时拒绝写入（handler 已有模型缺失时的拒绝分支），而不是猜测 'user'。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| onKeyDown={(event) => { | ||
| if (event.key !== 'ArrowLeft' || searchQuery) return; |
There was a problem hiding this comment.
[Suggestion] R8-13: the ArrowLeft back-affordance for the model submenu is wired only to this search input — the model item buttons have no keyboard handler, asymmetric with the ArrowRight-opens affordance this same diff adds on the submenu trigger. — Concrete cost: a keyboard-only user opens the model popover, presses ArrowRight on the submenu trigger (submenu opens, focus lands in the search input), then Tabs into the model item buttons; ArrowLeft does nothing there — the user must Shift+Tab back to the search input first (Escape still works via Radix defaults).
Suggested fix: add the same ArrowLeft handler to the item buttons inside dropdownItems (close the submenu and focus submenuTriggerRef), or put a capture-phase ArrowLeft handler on the submenu PopoverContent.
中文说明
[Suggestion] R8-13：模型子菜单的 ArrowLeft 返回只接在这个搜索框上——模型项按钮没有键盘处理，与同一 diff 在子菜单触发器上新增的 ArrowRight 打开不对称。— 具体代价：纯键盘用户打开模型弹层，在子菜单触发器上按 ArrowRight（子菜单打开，焦点落在搜索框），再 Tab 到模型项按钮；在那里按 ArrowLeft 无反应——用户必须先 Shift+Tab 回到搜索框（Escape 仍可经 Radix 默认行为关闭）。
建议修复：给 dropdownItems 内的模型项按钮加同样的 ArrowLeft 处理（关闭子菜单并聚焦 submenuTriggerRef），或在子菜单 PopoverContent 上加捕获阶段的 ArrowLeft 处理。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const rpcCode = (err as { code?: unknown } | null)?.code; | ||
| if (typeof rpcCode === 'number' && rpcCode <= -32000) { | ||
| return { code: rpcCode, message: errMsg(err) }; |
There was a problem hiding this comment.
[Suggestion] R8-14: the new numeric-code pass-through returns { code, message } and drops the error's data payload. RequestError.authRequired({ authMethods }) (code -32000, thrown by ensureAuthenticated) reaches this branch on the /acp HTTP surface, while the stdio SDK surface's toResult() includes data — the "both transport surfaces agree" comment is not achieved; the same drop applies to every data-carrying agent RequestError. — Concrete cost: an /acp HTTP client calling session/new with missing/expired credentials receives { code: -32000, message: 'Authentication required: …' } with no data.authMethods; a client that renders available auth methods from the error works over stdio but not over HTTP. (Pre-PR both surfaces collapsed to an opaque -32603, so this is an improvement — but parity is not achieved.)
Suggested fix: forward data when present: return { code: rpcCode, message: errMsg(err), ...((err as { data?: unknown })?.data !== undefined ? { data: (err as { data: Record<string, unknown> }).data } : {}) };
中文说明
[Suggestion] R8-14：新的数字码透传只返回 { code, message }，丢弃了错误的 data 载荷。RequestError.authRequired({ authMethods })（code -32000，由 ensureAuthenticated 抛出）在 /acp HTTP 面会进入该分支，而 stdio SDK 面的 toResult() 携带 data —— "both transport surfaces agree" 的注释并未达成；所有携带 data 的 agent 端 RequestError 都会被同样丢弃。— 具体代价：/acp HTTP 客户端在凭证缺失/过期时调用 session/new，收到 { code: -32000, message: 'Authentication required: …' } 但没有 data.authMethods；依赖错误渲染可用登录方式的客户端在 stdio 可用、在 HTTP 不可用。（PR 之前两个面都坍缩为不透明的 -32603，所以这是改进——但对齐尚未达成。）
建议修复：存在 data 时一并转发：return { code: rpcCode, message: errMsg(err), ...((err as { data?: unknown })?.data !== undefined ? { data: (err as { data: Record<string, unknown> }).data } : {}) };
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
|
||
| expect(onModelChange).toHaveBeenCalledWith(AuthType.USE_OPENAI, true); | ||
| expect(onModelChange).toHaveBeenCalledWith(AuthType.USE_OPENAI, true, { | ||
| sourceWasRuntimeSnapshot: false, |
There was a problem hiding this comment.
[Suggestion] R7-12: Round-7 ledger item, still standing at the reviewed commit — the only toHaveBeenCalledWith assertion of the new OnModelChangeCallback context checks sourceWasRuntimeSnapshot: false; the true case (leaving a runtime snapshot through a path that passes the pre-clear flag) is asserted nowhere. — Concrete cost: Config.handleModelChange consumes the flag to decide whether the pre-switch model's reasoning is model-scoped (registered) or a global preference. If a future change derived the flag from post-mutation state (e.g. after the snapshot id was cleared), a user leaving a runtime snapshot whose modelId collides with a registered id via a registry switch would have their reasoning effort misclassified and silently discarded instead of preserved as the global preference — and no test would fail.
Suggested fix: add one assertion for a leaving-a-snapshot transition, e.g. expect(onModelChange).toHaveBeenCalledWith(<authType>, <refresh>, { sourceWasRuntimeSnapshot: true }) after switchToRuntimeModel(...) followed by a switch to another model.
中文说明
[Suggestion] R7-12：第 7 轮 ledger 条目，在受审提交上仍然存在——新 OnModelChangeCallback context 的唯一 toHaveBeenCalledWith 断言检查的是 sourceWasRuntimeSnapshot: false；true 的情形（经由传递清除前标志的路径离开运行时快照）没有任何断言。— 具体代价：Config.handleModelChange 用该标志判断切换前模型的 reasoning 是模型作用域（注册）还是全局偏好。若未来改动从变更后状态推导该标志（例如在快照 id 被清除之后），用户经由注册表切换离开一个 modelId 与注册 id 冲突的运行时快照时，其 reasoning effort 会被误判并被静默丢弃，而不是作为全局偏好保留——且没有测试会失败。
建议修复：为离开快照的转换补一条断言，例如 switchToRuntimeModel(...) 后切换到另一模型时 expect(onModelChange).toHaveBeenCalledWith(<authType>, <refresh>, { sourceWasRuntimeSnapshot: true })。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| useEffect(() => { | ||
| if (!open) { | ||
| setSearchQuery(''); | ||
| setSubmenuOpen(false); | ||
| returningFromSubmenuRef.current = false; | ||
| } | ||
| }, [open]); |
There was a problem hiding this comment.
[Suggestion] R7-14: Round-7 ledger item, still standing and re-confirmed this round — the submenu/header state resets only when the outer popover closes (this reset effect keys on [open]), and the focus effects depend only on [open, submenuOpen]; nothing watches the submenu/header props, which can change while the popover stays open. — Concrete cost: with the model popover open and the reasoning submenu expanded (focus in the submenu search input), an SSE-driven update flips showReasoningOptions without any outside click (a peer's model switch to an unregistered model clears connection.reasoning; pre-session, providers/model changes flip composerReasoningState). The submenu portal unmounts with the focused input inside — focus falls to <body> and is never restored to the flat search input; submenuOpen stays true, so if reasoning controls become supported again during the same open session, the submenu silently re-mounts open without focus handling.
Suggested fix: key the reset/focus effects on the rendered mode as well — include Boolean(submenu) in the deps of the reset effect (clear submenuOpen/searchQuery/returningFromSubmenuRef when the mode changes), and restore focus to the flat search input when transitioning submenu→flat while open.
中文说明
[Suggestion] R7-14：第 7 轮 ledger 条目，仍然存在且本轮再次确认——子菜单/头部状态只在外层弹层关闭时重置（该重置 effect 依赖 [open]），焦点 effect 只依赖 [open, submenuOpen]；没有任何 effect 监听 submenu/header props，而它们可以在弹层保持打开时变化。— 具体代价：模型弹层打开且推理子菜单展开（焦点在子菜单搜索框）时，一次 SSE 驱动的更新在没有任何外部点击的情况下翻转 showReasoningOptions（同伴客户端切换到未注册模型会清空 connection.reasoning；会话前 providers/model 变化会翻转 composerReasoningState）。子菜单 portal 连同持有焦点的输入框一起卸载——焦点落到 <body>，且不会被恢复到平铺搜索框；submenuOpen 保持 true，若同一次打开期间推理控件重新可用，子菜单会静默重新挂载为打开状态且无焦点处理。
建议修复：让重置/焦点 effect 也依赖渲染模式——重置 effect 的依赖加入 Boolean(submenu)（模式变化时清除 submenuOpen/searchQuery/returningFromSubmenuRef），并在 open 状态下由子菜单切回平铺时把焦点恢复到平铺搜索框。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
@qwen-code /review All six Round 8 Criticals are addressed in 004bc55 and their conversations are resolved after evidence-bearing replies. The branch also contains a normal merge of current main; no history was rewritten. Verification: 1,197 affected tests pass across core config, CLI modelConfigUtils, webui session actions, and Web Shell App/ChatPane/ChatEditor; full build, full typecheck, full lint, focused format checks, two clean self-audit passes, and a local qreview pass are green. Post-fix exact-HEAD probes are still running and will be reported if they find any discrepancy. Per the repository rule for a PR beyond roughly five review rounds, the non-blocking Round 8 Suggestions R8-8 through R8-14 plus repeated R7-12 and R7-14 are explicitly deferred to follow-up work rather than widening this PR again. Please re-review the new HEAD and update the blocking review decision. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Code Review: PR #8675 — feat(web-shell): add model-specific reasoning controls
- Review timestamp: 2026-08-12 16:38
- Target: PR #8675 — QwenLM/qwen-code
- Head SHA:
004bc55d37518b31389618c974dfabba7b627043 - Merge base:
28ab8bae56f0c2db1f413a83fd1dfaf5a85171cf - Effort: high (verified)
- Model: deepseek-v4-flash
Provenance
| Gate | Result |
|---|---|
| Build | ✅ 17/17 workspaces |
| Test (acp-bridge) | ✅ 449 passed |
| Test (cli) | ❌ 21 failures — all pre-existing (0 files changed by the PR) |
| Test (core) | ❌ 28 failures — all pre-existing (0 files changed by the PR) |
| Test (sdk-typescript) | ⏰ timed out |
| Test (vscode-ide-companion, web-shell, webui) | ⏭ not run (budget) |
| Test-efficacy harness | ✅ validated |
| Script lint | ✅ No executable scripts changed |
| Test plan | 4 claims checked, 0 contradicted, 4 differing |
Diff Statistics
- 72 files changed, +6476/-223
- 3695 source lines, 4980 test lines, 78 docs lines
Findings
Suggestion: HTTP_ACP_CONFIG_OPTION_IDS omits 'thinking' and 'effort' (R1-1)
- File:
packages/cli/src/serve/acp-http/dispatch.ts:423 - Source: review
- Confidence: high
- Failure scenario: The constant is used in both the advertisement filter and the error message. The setter now routes thinking/effort via a separate else-if branch, so they are functionally supported, but the error message reports (supported: model, mode) — misleading a client developer.
- Suggested fix: Update
HTTP_ACP_CONFIG_OPTION_IDSto include 'thinking' and 'effort', or update the error message string to list them independently.
Suggestion: No test coverage for validateReasoningPreferencesValue (R1-2)
- File:
packages/cli/src/utils/settingsUtils.ts:322 - Source: review
- Confidence: high
- Failure scenario: The function validates the model.reasoningPreferences shape on API writes but has zero test coverage. A future refactor that breaks the validation would ship silently.
- Suggested fix: Add a test sub-block under validateSettingValue covering all edge cases.
Suggestion: setConfigOption error path uses addNotice instead of noticeForSession (R1-3)
- File:
packages/webui/src/daemon/session/actions.ts:793 - Source: review
- Confidence: high
- Failure scenario: Inconsistent with setModel (line 753). If the session becomes stale, the error notice is posted unconditionally rather than suppressed.
- Suggested fix: Replace
addNoticewithnoticeForSession(session).
Suggestion: defineModelReasoningControls validation not unit-tested (R1-4)
- File:
packages/core/src/core/model-reasoning-controls.ts - Source: review
- Confidence: high
- Failure scenario: Validation logic has no direct unit test. A regression that silently removes these checks would not be caught.
- Suggested fix: Add a unit test covering all validation paths.
Suggestion: Unreachable integration test — outside every npm workspace (R1-5)
- File:
integration-tests/cli/acp-integration.test.ts:791 - Source: test
- Confidence: high
- Failure scenario: This file sits outside every npm workspace, so the project's test command never collects it.
- Suggested fix: Move into a workspace or add a workspace-scoped collection.
Suggestion: Unreachable integration test — outside every npm workspace (R1-6)
- File:
integration-tests/cli/qwen-serve-routes.test.ts:365 - Source: test
- Confidence: high
- Failure scenario: Same as R1-5.
- Suggested fix: Move into a workspace or add a workspace-scoped collection.
Suggestion: Inert test — passes with source reverted (R1-7)
- File:
packages/cli/src/acp-integration/acpAgent.worktree.test.ts:114 - Source: test
- Confidence: high
- Failure scenario: All 3 tests still passed when the source change was reverted.
- Suggested fix: Review test assertions to ensure they exercise the specific behavior the PR adds.
Suggestion: validateReasoningPreferencesValue uses global effort ladder, not model-specific supported tiers (R1-8)
- File:
packages/cli/src/utils/settingsUtils.ts:335 - Source: review
- Confidence: high
- Failure scenario: Validates against the global ladder (low, medium, high, xhigh, max), but qwen3.8-max only supports ['low', 'medium', 'xhigh']. Values 'high' and 'max' pass validation but are silently normalized.
- Suggested fix: Make
validateReasoningPreferencesValueregistry-aware.
Verdict
Comment (0 Critical, 8 Suggestion) — reviewed by qwen /review (model: deepseek-v4-flash).
Note: qwen attempted to post this review directly but its sandbox gh credentials were invalid; posted by the review orchestrator on its behalf.
|
Triaged the 0-Critical / 8-Suggestion review against exact HEAD 004bc55:
Per AGENTS.md, after roughly five review rounds this PR accepts only Critical correctness/security/regression fixes. This review reports 0 Critical, so I am not widening the 72-file diff for these suggestions. The valid deferred items are recorded here rather than silently dropped. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at the reviewed commit and its suite (which covers this PR's changed integration-tests files) did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget.; chunk 4: none — all checks I started completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget., and 3 more.
Test Plan (not a blocker): 620 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed; 039 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed; 66 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed; 39 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at the reviewed commit and its suite (which covers this PR's changed integration-tests files) did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.；chunk 4：none — all checks I started completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.，另有 3 条。
Test Plan（非阻断）：620 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed; 039 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed; 66 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed; 39 tests passed — this review observed 1242, 19141, 19620, 1507, 483, 3242, 533 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; this review manually ran the two changed integration test files (surfacing the R9-1 Critical) but not the full integration suite.
Not explored to full depth (tool budget reached): chunk 12: none — all checks I intended completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — finished within budget.; chunk 24: a clean authoritative typecheck of the package could not be obtained in this worktree (raw tsc run is polluted by pre-existing errors unrelated to the PR); the …; chunk 3: could not execute acpAgent.worktree.test.ts / Session.test.ts (no node_modules in the review worktree; full monorepo install would exceed the chunk budget)., and 11 more.
Test Plan (not a blocker): 620 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed; 039 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed; 66 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed; 39 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; this review manually ran the two changed integration test files (surfacing the R9-1 Critical) but not the full integration suite。
未探索到全部深度（达到工具调用预算）：chunk 12：none — all checks I intended completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — finished within budget.；chunk 24：a clean authoritative typecheck of the package could not be obtained in this worktree (raw tsc run is polluted by pre-existing errors unrelated to the PR); the …；chunk 3：could not execute acpAgent.worktree.test.ts / Session.test.ts (no node_modules in the review worktree; full monorepo install would exceed the chunk budget).，另有 11 条。
Test Plan（非阻断）：620 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed; 039 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed; 66 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed; 39 tests passed — this review observed 1242, 19144, 19619, 1507, 483, 3242, 533 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
Round 9 Criticals are addressed in b3f6319, and every Round 9 thread has been replied to and resolved. Build, typecheck, lint, the affected full unit-test files, and targeted ACP integration coverage all pass. @doudouOUC @wenshao, could you please re-review the new head? Auto-merge remains enabled. |
|
@wenshao @doudouOUC The sole remaining merge blocker is the stale qwen-code-ci-bot CHANGES_REQUESTED review 4910505722, submitted against pre-fix commit 004bc55. Current HEAD b3f6319 contains the Round 9 fixes, all Round 9 Critical threads are resolved, and every runnable CI check is green. The replacement review job 93929288123 has remained in Run review since 2026-08-11 21:11Z. Please review the current HEAD and either submit a superseding review or, after verification, dismiss the stale bot review using maintainer authority. CI/thread resolution alone cannot clear this reviewDecision; auto-merge is already enabled. |
Dismissed as stale after all Round 9 Critical findings were addressed in b3f6319. Every Round 9 thread has an evidence-bearing reply and is resolved; build, typecheck, lint, affected full unit suites, targeted ACP integration coverage, and every runnable CI check pass on the current HEAD. This dismissal only removes the obsolete bot decision submitted against 004bc55 and does not replace any required human approval.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (integration-tests/, incl. the PR's changed acp-integration and qwen-serve-routes files) did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: did not execute the new config.test.ts test — verified its fidelity statically against setThinkingEnabled / refreshAuth (config.ts:3676-3740, 4262-4296) ins…; chunk 24: none — all checks I started completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — I did not truncate any check.; You are review agent reverse-audit — Reverse audit agen...: none — no check was left unfinished., and 18 more.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (integration-tests/, incl. the PR's changed acp-integration and qwen-serve-routes files) did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：did not execute the new config.test.ts test — verified its fidelity statically against setThinkingEnabled / refreshAuth (config.ts:3676-3740, 4262-4296) ins…；chunk 24：none — all checks I started completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — I did not truncate any check.；You are review agent reverse-audit — Reverse audit agen...：none — no check was left unfinished.，另有 18 条。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
@qwen-code /resolve |
# Conflicts: # packages/cli/src/ui/commands/effort-command.ts # packages/cli/src/ui/hooks/use-effort-command.test.ts # packages/cli/src/ui/hooks/use-effort-command.ts # packages/core/src/core/openaiContentGenerator/provider/dashscope.test.ts
Round 10 is fully triaged. All three Critical findings were fixed in f4a4592 and independently re-verified with their original probes plus focused regressions. The merge conflict with current main was resolved by a normal merge and the affected effort/DashScope suites pass. All 16 non-blocking Suggestions were replied to and explicitly deferred under the repository policy for review rounds beyond five, then their threads were resolved. Build, typecheck, lint, and focused tests pass.
|
Qwen Code resolved the merge conflicts, but the head branch changed while resolving, so the update was not pushed. Re-run /resolve. The resolved diff is attached as the Root causeMain merged #8525 ( Semantic, not textualBoth sides changed the same message block in applied && effectiveTier !== tier && !config.getReasoningEffortOverride?.()
? t('Reasoning effort: {{tier}} (normalized from {{requested}} for the active model).', …)
: formatEffortChangeMessage(config, effectiveTier)The PR's registry normalization + Load-bearing
Not verifiedNo build or tests ran here. 中文说明根因:main 在本 PR 分支后合入 #8525( 语义冲突:两侧修改了同一段消息逻辑与相同的 qwen3.8-max 测试。解决方式:保留 PR 的归一化与 关键点:归一化分支必须先于 helper 且检查 override,否则 qwen3.8-max 带静态旋钮时会误报档位生效(正是 #8525 修的问题);helper 必须传归一化后的 未验证:本次未跑构建/测试。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): PR summary: This PR adds a built-in model reasoning-contr...: none — all planned checks completed within budget.; chunk 12: running packages/cli vitest for modelConfigUtils.test.ts (worktree has no node_modules; full monorepo install exceeded the tool budget — tests were hand-traced …; chunk 19: none. Everything planned was completed (build verification was adapted due to shared worktree contention: full node scripts/build.js couldn't be completed due…; chunk 13: none — all checks I intended completed within budget.; chunk 7: none — could not inspect @agentclientprotocol/sdk internals (no node_modules); the dependent assertion is covered by the new test itself..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Test Plan (not a blocker): 620 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed; 039 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed; 66 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed; 39 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR summary: This PR adds a built-in model reasoning-contr...：none — all planned checks completed within budget.；chunk 12：running packages/cli vitest for modelConfigUtils.test.ts (worktree has no node_modules; full monorepo install exceeded the tool budget — tests were hand-traced …；chunk 19：none. Everything planned was completed (build verification was adapted due to shared worktree contention: full node scripts/build.js couldn't be completed due…；chunk 13：none — all checks I intended completed within budget.；chunk 7：none — could not inspect @agentclientprotocol/sdk internals (no node_modules); the dependent assertion is covered by the new test itself.。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：620 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed; 039 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed; 66 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed; 39 tests passed — this review observed 1251, 19177, 19721, 1512, 483, 3286, 539 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
All Round 11 Critical findings were fixed in 8cb1088, independently reproduced post-fix, replied with evidence, and their review threads resolved. Dismissing this superseded CHANGES_REQUESTED review so the PR can return to the current review/CI gate.
|
Round 11 Criticals are fixed in 8cb1088, independently post-fix verified, replied to individually, and all three current threads are resolved. The superseded CHANGES_REQUESTED review 4913162151 has been actively dismissed; |
|
Superseded by #8974, which rebuilds the WebShell Qwen 3.8 Max reasoning controls on the latest main with a narrower manifest-driven, live-session-only design. |
Pull request was closed
5 participants
What this PR does
This PR adds a built-in model reasoning-controls registry and uses it end to end across Core, ACP, daemon, SDK, and WebShell. Registrations independently declare optional Thinking and Effort controls, supported effort tiers, and defaults. The first exact registration is
qwen3.8-max, with Thinking enabled by default andlow,medium, andxhigheffort options defaulting toxhigh.WebShell now renders capability-driven reasoning controls in the existing model popover. The current model remains in the primary Options menu, while the searchable model list opens as a compact right-side flyout. Preferences are stored per model, restored after switching away and back, and synchronized from live ACP config-option updates without clearing the target model state during model-switch completion.
The PR contains no local reasoning mock scenario, mock-daemon extension, or development-only model catalog data.
Why it's needed
Models increasingly expose different reasoning controls and effort subsets. Encoding those differences directly in clients would duplicate model-ID checks and make future additions fragile. A shared server-side registry keeps clients capability-driven, preserves older-daemon fallback behavior, and lets future models expose Thinking-only, Effort-only, or combined controls without another UI-specific model branch.
Reviewer Test Plan
How to verify
qwen3.8-max. Open the model chip and confirm Thinking is enabled and Extra High is selected by default.qwen3.8-maxto another model and back. Confirm Thinking and Effort controls remain visible and the model-specific preference is restored.qwen3.8-max-preview, aliases, snapshots, unregistered models, and daemons withoutsession_reasoning_controlretain the existing model-only picker.Evidence (Before & After)
After — capability-driven Thinking and Effort controls:
After — compact right-side searchable model flyout:
Tested on
Environment (optional)
Node.js 22, local WebShell development runtime, dark theme.
Focused verification: Core 620 tests passed; CLI/daemon 1,039 tests passed; WebUI mapper/action 66 tests passed; ChatEditor 39 tests passed; WebShell and WebUI typecheck/build passed; focused ESLint and Prettier checks passed.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 新增内置的模型推理控制能力注册表，并贯通 Core、ACP、daemon、SDK 与 WebShell。注册项可以独立声明 Thinking、Effort、支持的 Effort 档位和默认值。首个精确注册模型为
qwen3.8-max：Thinking 默认开启，Effort 支持low、medium、xhigh，默认xhigh。WebShell 在现有模型弹层中按能力动态展示推理设置。当前模型保留在一级 Options 菜单，搜索模型列表通过右侧紧凑二级菜单打开。偏好按模型独立保存，切换到其他模型再切回时恢复，并从 ACP config-option 更新同步；模型切换完成事件不会再清空目标模型已经同步的推理状态。
本 PR 不包含本地 reasoning mock 场景、mock daemon 扩展或仅用于开发的模型目录数据。
为什么需要
不同模型会逐步提供不同的推理开关和 Effort 子集。如果由客户端直接判断模型 ID，会造成重复分支并增加后续扩展成本。统一的服务端能力注册表让客户端完全按能力渲染，保留旧 daemon 的降级行为，并支持未来模型只提供 Thinking、只提供 Effort，或同时提供两者。
Reviewer 测试计划
验证方式
qwen3.8-max，打开模型 chip，确认 Thinking 默认开启、Effort 默认选中 Extra High。qwen3.8-max切换到其他模型再切回，确认 Thinking、Effort 仍可见且恢复该模型的独立偏好。qwen3.8-max-preview、别名、snapshot、未注册模型，以及未声明session_reasoning_control的旧 daemon 仍使用原模型选择器。效果证据
After — 按能力展示 Thinking 与 Effort：
同上，见英文 Evidence 中的 Thinking / Effort 截图。
After — 右侧紧凑可搜索模型二级菜单：
同上，见英文 Evidence 中的右侧模型二级菜单截图。
测试环境
Node.js 22，本地 WebShell 开发运行时，深色主题。
Focused 验证：Core 620 个测试通过；CLI/daemon 1,039 个测试通过；WebUI mapper/action 66 个测试通过；ChatEditor 39 个测试通过；WebShell 与 WebUI typecheck/build 通过；focused ESLint 与 Prettier 检查通过。
风险与范围
关联 Issue
N/A