fix(web-shell): keep reasoning effort controls available - #10244
Merged
Conversation
Collaborator
Author
WebShell E2E reportEnvironment: local qwen-code WebShell, its fake daemon, and Playwright Chromium. No DataWorks page or environment was used. Post-rebase verification:
Repository build, typecheck, and lint passed before rebasing onto the latest 中文补充：以上验证全部通过 qwen-code WebShell 完成，没有使用 DataWorks 页面。重点覆盖了首屏、消息发送后、运行中三个生命周期，以及强制思考模型“开关不可关闭，但强度始终可切换”的行为。 |
Collaborator
Collaborator
Author
CI base blocker updateAll failed Node/WebShell jobs stopped during dependency installation, before this PR's tests ran, on the same base-only error:
This PR has no diff in that file. The dedicated three-line upstream fix is #10243. Once it lands, this branch will merge the updated 中文：当前红灯均在依赖安装触发的仓库构建阶段被 |
Collaborator
Code reviewMy independent proposal for this problem was the minimal version: teach the WebShell mapper to accept mandatory-thinking options, and project the effective effort from the daemon. This PR does that and goes further — documented effort aliases, static Verified against the base code:
No critical blockers. One non-blocking question:
How it fits togethersequenceDiagram
participant P1 as User
participant P2 as WebShell model popover
participant P3 as Daemon ACP agent
participant P4 as Generation config
participant P5 as WebUI mapper
P1->>P2: pick effort medium
P2->>P3: setSessionConfigOption reasoning_effort medium
P3->>P4: copy parameter maps, drop competing thinking fields
P3->>P4: apply effort medium
P3-->>P2: fresh configOptions with qwenCode reasoning meta
P2->>P5: mapReasoningControls
P5-->>P2: effort medium, canDisable false when thinking is mandatory
Files changed (14)
Test evidence — the PR's own CICI on
Classification: all six failures are pre-existing main breakage, not caused by this PR. Evidence:
The author's local results quoted in the description (33/33 WebShell smoke, 560/560 projection, 146/146 mapping, 635/635 App/ChatEditor) are the author's claim, not independently verified — CI could not reproduce them for the reason above. Sandboxed verification would settle this: Real-scenario testing (local run): N/A for this surface. The change is WebShell browser UI, not the TUI, so tmux cannot drive it, and triage never executes PR-derived code; the browser-level evidence for this surface is the Playwright smoke suite above. Not verified: browser-level behavior on this commit — blocked on main being fixed and CI running green here (or a 中文说明代码审查：方案与我的独立设想一致且更完整（别名、静态 thinking_budget 区间、静态覆盖字段、首屏/实时统一元数据），每一部分都有对应测试，不算范围膨胀。已核对：所有被改辅助函数的调用方都在 diff 内；消费的 API 均已在 main 上存在，未新增核心接口； 无阻塞问题。一个非阻塞疑问：对 toggle-only 模型（qwen3.5/3.6/3.7 系列）且用户配置了静态 测试证据：本提交的全部 6 个失败均为 main 预先存在的构建错误（ — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 3/5 — clean code review and a genuinely needed fix, but zero verified test evidence on this commit: every CI pipeline dies on a pre-existing main build break before this PR's suites can run. Reflection: the PR matches my independent proposal and exceeds it — each addition beyond the minimal fix (alias projection, budget mapping, static-override handling, welcome/live unification) answers a named regression case with a test behind it. Every edit in the diff is necessary for the stated goal; there are no drive-bys. The problem is real and the mechanism was verified in code, not accepted on faith. If this landed and I read it in six months, the projection logic is dense but each branch has a named test — I'd thank the author, not curse them. Why I'm not approving anyway: the gate approves on evidence, not on reading. This PR touches the high-risk ⏸️ Deferring to @ytahdn — approval withheld until this commit gets a real test signal. This is not a request-changes: nothing in the diff is defective. Two concrete things unblock it:
中文说明置信度：3/5 —— 代码审查干净、修复确有必要，但本提交没有任何已验证的测试证据：所有 CI 流水线都在 main 预先存在的构建错误处终止，本 PR 的测试根本未能运行。 结论：方案与我的独立设想一致且更完整，diff 中每处改动都服务于既定目标，无夹带。问题是真实的，机制已在代码中核实。不批准的原因是证据而非代码：本 PR 触及高风险的 ⏸️ 转交 @ytahdn —— 在本提交获得真实测试信号前暂缓批准。这不是 request-changes：diff 本身没有缺陷。解锁只需两步：① 修复 main（删除 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real regression, and the mechanism is verifiable right in the code. The old Direction: aligned. This makes the controls track model capability instead of session lifecycle, which is exactly what the feature promised. CHANGELOG: no direct reference (qwen-code WebShell-specific surface). Size: cross-package (cli / webui / web-shell), so core scope under the two-tier rule. ~206 production lines (acpAgent.ts 114, model-configuration.ts 38, App.tsx 25, mappers.ts 16, workspace-providers-status.ts 9, ChatEditor.tsx 2, types.ts 2), ~729 test lines, ~30 docs lines. Below every threshold — no maintainer awareness needed on size. Approach: scope feels right. Projection (ACP), mapping (daemon → WebShell), and presentation (web-shell) genuinely must move together for this fix; the copy-on-write strip of competing thinking fields is a reasonable minimal mechanism; fail-closed paths (older daemons, opaque routes, runtime models) stay covered with tests; the design doc is updated in the same diff. No drive-by changes spotted. Risk: high-risk path matched — Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 真实回归，机制可在代码中直接验证。旧版 方向： 对齐。这让控件跟随模型能力而非会话生命周期，正是该功能本来的承诺。CHANGELOG：无直接引用（qwen-code WebShell 专属能力）。 规模： 跨包改动（cli / webui / web-shell），按两级规则属于核心范围。生产代码约 206 行，测试约 729 行，文档约 30 行，未达任何阈值，无需规模层面的维护者关注。 方案： 范围合理。投影（ACP）、映射（daemon → WebShell）、展示（web-shell）必须一起改动才能完成此修复；copy-on-write 清理冲突思考字段是合理的最小机制；fail-closed 路径（旧 daemon、不透明路由、runtime model）有测试覆盖；设计文档同步更新。未发现夹带改动。 风险： 命中高风险路径—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Author
Base repaired and synchronizedThe upstream build break is repaired and this branch now contains the latest Post-merge local verification on this exact head:
The full CI matrix is rerunning now. After it is green, I will rerun triage against the verified head and continue through approval and merge. 中文：上游构建错误已经修复，本分支已正常合并最新 |
Collaborator
Author
Post-merge WebShell verificationVerified on exact head
The browser coverage includes the pre-session welcome state, completed-message state, active prompt, mandatory-thinking switch behavior, model switching, unsupported/old-daemon/missing-capability fail-closed paths, and the legacy toggle-only path. Port 5184 was released after the run and the working tree remains clean. 中文：以上结果均基于最新提交 |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
Contributor
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. |
yiliang114
approved these changes
Aug 27, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at head 9bad139 (static pass over the source diff; tests and e2e smoke read for intent):
- Mandatory-thinking handling is consistent across layers: the setter rejects
noneand drops it from the choice list,buildModelReasoningConfigOptionnever yields anonecurrentValue when mandatory, the webui mapper exposescanDisable: false(and returns no controls on the inconsistentmandatory + nonecombo), and the web-shell clears a pendingnoneintent via effect plus guards the setter path. - The copy-on-write cleanup only clones the
extra_body/samplingParamslayer objects before deleting the competing thinking keys, so shared source objects stay intact; the in-placegeneration.*writes follow the existing pattern in this handler. getModelReasoningConfigurationno longer disappears on override/mandatory — the effort validity check moved to the call site (efforts.find(...) ?? defaultEffort), which keeps controls live after messages and mid-prompt.- CI green (only the review-pr bot lane still running). No correctness issues found.
chiga0
approved these changes
Aug 27, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings.
Scope: source diff, test changes, e2e smoke spec, webui mapper, App.tsx session logic. Design doc hunk reviewed for consistency.
Not covered: getCurrentAcpModelId and REASONING_EFFORT_TIERS implementations (not in diff; behavior inferred from the passing test suite) · macOS/Windows CI — SKIPPED due to upstream TS1117 in an unchanged core test mock · Integration Tests — SKIPPED same reason · mutation testing — no toolchain available.
What I traced:
Mandatory-thinking consistency across layers (model-configuration.ts → acpAgent.ts → mappers.ts → App.tsx → ChatEditor.tsx)
buildModelReasoningConfigOptionwiththinkingMandatory:true:enabled===falseno longer maps toREASONING_EFFORT_NONE; thenoneoption is dropped from the list;_meta.thinkingMandatoryis forwarded. Verified inmodel-configuration.test.ts.buildSessionConfigOptions:effectiveModelEffortcorrectly resolves todefaultEffortwhenmandatoryUsesDefaultEffort(thinking mandatory + override disables reasoning). ThereasoningEnabledpath is consistent —enabled:falsepassed tobuildModelReasoningConfigOptionis masked by!thinkingMandatory, socurrentValueremains the mapped effort rather thannone.- Webui mapper (
mappers.ts):thinkingMandatory:truebypasses thevalues.includes('none')gate; thethinkingMandatory+currentValue==='none'guard is defensive dead-code (server cannot produce that combination) but harmless.canDisable:falsethreaded intoDaemonReasoningControls. App.tsx:validPendingReasoningIntentguardscanDisable===falsesynchronously in the render; theuseEffectis a clean-up for stale stored state, safe and redundant.ChatEditor.tsx: Thinking switchdisabledcondition correctly extended with|| reasoning.canDisable === false.
Copy-on-write cleanup in setSessionConfigOption
The extra_body/samplingParams fields are spread-copied before the thinking keys are deleted, so the original shared source objects are never mutated. Test re-enables qwen3.8-max after a static thinking disable verifies this with not.toBe(extraBody) / not.toBe(samplingParams) assertions.
Effort projection for overrides
Static thinking_budget → low/medium/xhigh mapping (boundaries 4096, 16384) verified by the parameterized test with values 4096/4097/16384/16385. minimal → low alias in normalizedEffortOverride is a straightforward string check. high/max generic efforts fall back to currentModelEffort === undefined → buildModelReasoningConfigOption maps to defaultEffort = 'xhigh' via the !efforts.includes(state.effort) path — confirmed by the presents inherited %s as the qwen3.8-max xhigh alias test.
Opaque-route guard
getModelReasoningConfiguration now returns undefined for completeModelId.startsWith(ACP_ROUTE_ID_PREFIX). The setSessionConfigOption path for opaque routes goes through innerConfig.setReasoningEffort directly (generic effort, not model-specific); the smoke test exercises this and passes.
CI: Ubuntu test suite 14/14 green · web-shell E2E smoke green · Desktop Shell (ubuntu/windows) green · Serve A/B green. macOS and Windows unit tests, and Integration Tests, are SKIPPED due to an upstream TS1117 in an unchanged core test mock — this branch has no diff in that file.
Cross-check: One prior review (APPROVED) confirmed the same areas; no mismatches, no missed blockers.
Reviewed with AI assistance.
Collaborator
|
Released in v0.22.3. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This keeps the model-specific reasoning controls available for qwen3.8-max before session creation, after messages have been sent, and while a prompt is running. Models that require thinking keep the Thinking switch checked and disabled while all supported effort tiers remain selectable.
The live session now reports the effective model-specific tier for documented effort aliases, thinking budgets, and request-level overrides. Choosing a tier clears only competing thinking fields through copy-on-write, so unrelated request parameters and shared source objects remain intact.
Welcome-state reasoning uses the same mandatory-thinking metadata as the live session. If refreshed metadata makes thinking mandatory, a pending Thinking-off choice is discarded and cannot reappear or be sent with the first prompt.
Why it's needed
After the first message, the authoritative live session could downgrade qwen3.8-max to a generic reasoning option or hide it entirely when thinking was mandatory or a static request field was present. WebShell then showed only the model list, even though the model still supported changing reasoning effort. This made the control depend on session lifecycle instead of model capability.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: the reported post-message popover contained only the model list; the live mandatory/no-off option was rejected by the WebShell mapper, so effort controls disappeared.
After: the qwen-code WebShell fake-daemon Chromium smoke suite passes 33/33, including welcome-state selection, replayed messages, mandatory thinking, and a configuration change during an active prompt. ACP/model projection passes 560/560 tests, WebUI session mapping passes 146/146, and WebShell App/ChatEditor passes 635/635. Two independent source/wire reviews completed with Critical 0 and Suggestion 0.
Repository build, typecheck, and lint passed before the final rebase. On the current origin/main base, the repository build is blocked by an upstream TS1117 duplicate property in an unchanged core test mock; this branch has no diff in that file. All focused suites and the full WebShell smoke suite above pass on the rebased commit, and CI will be followed through resolution.
Tested on
Environment (optional)
Local qwen-code WebShell with its fake daemon and Playwright Chromium. No DataWorks page or environment was used.
Risk & Scope
Linked Issues
N/A — fixes the reported WebShell reasoning-selector regression.
中文说明
本 PR 做了什么
本 PR 让 qwen3.8-max 的模型专属思考控制在首屏尚未创建会话、已经发送消息以及提示词运行中都保持可用。对于强制思考模型，Thinking 开关保持选中且禁用，但所有受支持的思考强度仍可选择。
实时会话现在会根据官方定义的强度别名、thinking budget 和请求级覆盖项展示真实生效的模型专属档位。用户选择档位时只通过 copy-on-write 清理冲突的思考字段，不会破坏无关请求参数或共享的源对象。
首屏思考能力与实时会话使用同一份强制思考元数据。如果刷新后的模型元数据变为强制思考，待应用的“关闭思考”选择会被丢弃，不会在之后恢复，也不会随首条提示词发送。
为什么需要
发送首条消息后，权威实时会话在强制思考或存在静态请求字段时，可能把 qwen3.8-max 降级成通用思考选项，或者完全隐藏它。WebShell 随后只显示模型列表，尽管该模型仍支持切换思考强度。这让控件是否出现取决于会话生命周期，而不是模型能力。
Reviewer 测试计划
如何验证
前后证据
修复前：用户反馈的发送消息后弹层只包含模型列表；实时的强制思考/无关闭选项会被 WebShell 映射层拒绝，因此思考强度消失。
修复后：qwen-code WebShell fake daemon 的 Chromium smoke 33/33 通过，覆盖首屏选择、历史消息、强制思考以及提示词运行中的配置切换。ACP/模型投影 560/560、WebUI 会话映射 146/146、WebShell App/ChatEditor 635/635 均通过。两次独立 source/wire 复审结果均为 Critical 0、Suggestion 0。
仓库级 build、typecheck、lint 在最终 rebase 前均已通过。当前 origin/main 基线上，仓库 build 被上游 core 测试 mock 中重复属性导致的 TS1117 阻塞；本分支对该文件没有任何 diff。上述聚焦测试和完整 WebShell smoke 已在 rebase 后提交上通过，并会继续跟进 CI 直至阻塞解除。
测试系统
环境
本地 qwen-code WebShell、其自带 fake daemon 以及 Playwright Chromium。未使用任何 DataWorks 页面或环境。
风险与范围
关联 Issue
无——修复本次反馈的 WebShell 思考选择器回归。