feat(web-shell): persist reasoning effort - #10011
Conversation
…asoning-persistence
…asoning-persistence
…asoning-persistence
…asoning-persistence
Real-daemon E2E reportTested on macOS with Node.js 22.17.0 and npm 10.9.2. Both baseline and fixed builds ran a real local daemon and WebShell with separate temporary configuration directories; every restart stopped the previous daemon before starting a new process against the same scenario directory.
The four 1280×800 screenshots embedded in the PR body were captured from these real-daemon runs. Scenario directories were Automated verification
Two independent full-diff audits were run from scratch after the final upstream merge. Both were required to be clean before submission. Final PR head |
|
Thanks for the PR! The description is thorough and the reviewer test plan is concrete — appreciated.
Moving on to code review. 🔍 中文说明感谢贡献!PR 描述很完整,reviewer 测试计划也很具体。
进入代码审查 🔍 — 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 / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewReviewed statically against Four questions before this is mergeable, none of which I could settle from the diff alone:
One carry-over from the gate: this bundles a settings-contract widening (enum → open string) with the WebShell persistence plumbing, and shares 15 files with the open sibling #9590 in exactly this subsystem. The sequencing of the two is a maintainer decision that should happen before either merges. sequenceDiagram
participant P1 as WebShell UI
participant P2 as Daemon serve route
participant P3 as ACP bridge
participant P4 as ACP agent
participant P5 as Session settings
P1->>P2: select effort
P2->>P3: setSessionConfigOption with persist marker
P3->>P4: forward request
P4->>P4: apply effort to live session first
P4->>P5: persist model.reasoningEffort in model scope
P5-->>P4: written scope, or write failure
P4-->>P3: response with persistence scope meta
P3-->>P1: settings_changed refreshes welcome preview
Files changed (28 of 49 shown)
Testing evidenceThis is an unattended CI run — I do not execute PR code; the evidence below is the PR's own CI on the reviewed commit, quoted from the API. Real-scenario coverage of the WebShell surface can be triggered by a maintainer via the isolated jobs named below. The main unit suite, Serve A/B, and the web-shell visuals capture were still running at review time — the table region updates in place once CI settles. The one red check is classified as a pre-existing flake, not PR-caused: the SDK Java suite's Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Not verified: the central persistence-across-restart behavior itself. The author reports manual macOS verification only (Windows/Linux untested per the PR body), and the Before/After evidence images in the description are still placeholders — that is the author's claim, not evidence this review can point to. Sandboxed verification would settle it: 中文说明代码审查针对 四个无法仅凭 diff 定论的问题:
门禁阶段的遗留问题:本 PR 把设置契约放宽(枚举→开放字符串)与 WebShell 持久化管线捆绑在一起,且与未合并的 #9590 在该子系统共享 15 个文件。两者先后顺序需要维护者在合并任何一个之前决定。 测试证据无人值守 CI 运行——不执行 PR 代码;以上证据来自 API 引用的 PR 自身 CI。主单测套件、Serve A/B、web-shell 视觉捕获在审查时仍在运行,CI 落定后表格区域会原地更新。唯一红色检查判定为既有 flake:SDK Java 的 未验证:跨重启持久化这一核心行为本身。作者仅报告 macOS 手工验证(正文标注 Windows/Linux 未测),描述中的 Before/After 截图仍是占位符——那是作者声明,不是本审查可引用的证据。沙箱验证可以定论: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — solid, thoroughly tested work that is clean on most review axes, but the Stage 0 escalation (800+ production lines in core), four unresolved semantic questions, and the open sequencing conflict with #9590 mean this needs a maintainer's sign-off rather than a bot approval. Stepping back:
Verdict: defer to maintainer — this is not a rejection. Stage 0 policy (a 中文说明置信度:3/5 —— 扎实、测试充分,多数审查维度都干净;但 Stage 0 升级(800+ 行核心生产代码)、四个未决语义问题、以及与 #9590 的排序冲突,意味着它需要维护者签核,而不是机器人批准。 退一步看:
结论:转交维护者——这不是拒绝。按 Stage 0 政策(触及 core 且 500+ 生产行的 — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @doudouOUC — PR #10011 (persist reasoning effort) needs a maintainer's call; this is a hand-off, not a rejection. Nothing here reads as a blocking defect — the code quality is high and the test coverage is thorough. What needs a human owner:
中文说明⏸️ 转交 @doudouOUC —— PR #10011(持久化思考档位)需要维护者决定;这是转交,不是拒绝。审查未发现阻塞性缺陷——代码质量高、测试覆盖扎实。需要人类负责人的原因:
— Qwen Code · qwen3.8-max |
…asoning-persistence # Conflicts: # packages/webui/src/daemon/session/actions.test.ts # packages/webui/src/daemon/session/actions.ts
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. |
…asoning-persistence
🩺 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 |
…asoning-persistence
…asoning-persistence
Final upstream integration updateThe final PR head
The Java 11 failure on the superseded PR head was isolated to a one-off SDK detach connection-reset race outside this change. This normal push started a fresh CI run for the final head. |
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Scope: source diff + cross-file contract checks. NOT reviewed: design doc, VSCode schema, test files (titles only). macOS/Windows test jobs are SKIPPED so platform-specific behaviour is unverified.
Checked:
- Writer/reader contract for
model.reasoningEffort:persistReasoningEffort(Session.ts) writes the value;buildWorkspaceProvidersStatusreads it back viasettings.model?.reasoningEffort→resolveReasoningPreviewState→buildModelReasoningConfigPreview. Bounds are consistent:'none'maps to{ enabled: false }, an effort string maps to{ enabled: true, effort },'default'maps to{ enabled: true, effort: 'default' }which correctly setscurrentValue = 'default'in the config option, andundefinedfalls through to the model's configured reasoning default. Both the ACP agent path and the workspace-providers-status path use the sameresolveReasoningPreviewStatehelper — no divergence. effortstype migration (string[]→DaemonReasoningEffortOption[]): All consumers updated — App.tsx (4 sites), ChatEditor.tsx, mappers.ts, types.ts. TheChatEditorlabel falls back toreasoning.effort(the raw value) when the current effort is not in the list — correct for the custom-effort case.isReasoningEffortConfirmed:'none'checksreasoning.enabled === false;'default'always returnstrue(intentional — no specific value to echo back); other values checkenabled === true && effort === value. The'default'early-return is safe because if the daemon rejected the value thesetSessionConfigOptioncall would have thrown before reaching this check.- Persistence failure recovery path: If
persistReasoningEffortthrows (reasoning_effort_persistence_failed),actions.tsfetches live session context, applies the live state optimistically if confirmed, then returns without throwing (liveReasoningConfirmed = true) — the user sees a warning message but the session is updated. If the live refresh also fails, the original persistence error is rethrown. Logic is correct. _metaforwarding chain: WebShell route injects{ [PERSIST_REASONING_EFFORT_META_KEY]: true }; bridge reads it and, on success, broadcastssettings_changedwithkey: 'model.reasoningEffort'only when the response carriesqwenCode/reasoningEffortPersistence.scope. ACP agent setsresponseMetaonly inside theif (persistReasoningEffort)block. Contract is correctly gated.- CI failure (
Test ubuntu-latest Node 22.x): Root-caused to a pre-existing bug:TOOL_NAME_ALIASESinpackages/core/src/permissions/rule-parser.tsis missing theReportFindings → report_findingsentry, which is verified present intool-names.tson both the PR head andmain. The failure predates this PR and is not caused by it. getModelReasoningConfigurationlogic change: Old:toggleOnly ||short-circuited to always return reasoning for toggleOnly models. New: returnsundefinedfor toggleOnly models when acurrentEffortis set. This is intentional — toggleOnly models should not influence the effort-based config option, and the generic fallback path (which now includes agenericCustomEffortslot) handles the rendering correctly.
Ran: rung 0 CI check audit (17/19 green, 1 pre-existing failure, 1 in-progress automation).
Not run: rungs 1-3 (no toolchain in this environment).
Reviewed with AI assistance.
Dismissed as stale after all three Round 11 Criticals were fixed on the current head. Commit 77bd395 decouples live cleanup from persistence for ephemeral/managed model switches, clears opaque provider-specific efforts when switching to non-Qwen targets while preserving built-in tiers, and performs live-only cleanup when System/SystemDefaults or session-only state owns the incompatible value. Each path has regression coverage; commit 3dc4e4d restores the affected full-core mock after the fix, and CI on head 7092e8e is green. All associated threads are resolved and no live human objection remains.
ytahdn
left a comment
There was a problem hiding this comment.
独立静态审查,基于 head 7092e8e9b(干净的 main 合并,0 个合并冲突块;纯静态、未运行任何测试或构建)。84 个文件按模块分批审查,下列每条结论均在 head tree 逐条举证。
复核结论:此前各轮 Critical 的修复均在 head 独立验证成立
- 第 10 轮 R10-1(clear 路径漏查 SystemDefaults 拥有权):
Session.persistReasoningEffort的 clear 路径现在同时检查system与systemDefaults拥有权,冲突时抛 −32603reasoning_effort_persistence_failed;Session.test.ts含 system-defaults-only 用例(断言错误且setValue未被调用)。 - 第 11 轮 R11-1(live clear 被
persistDefault门控):Session.setModel在switchModel成功后无条件调用clearIncompatibleReasoningEffortForModel(Session.ts:9751-9756);persistDefault:false与 standalone/managed 测试均断言setReasoningEffort(undefined)且未写设置。 - 第 11 轮 R11-2(切换清理对非 Qwen 目标早退):helper 现在 target-aware——分级
qwen3.8-max保留任何 effort(含 opaque);toggle-only Qwen 清除任何字符串 effort;非 Qwen 仅清除 opaque 非内建值、保留内建五档;vendor.ultra → gemini-2.5-pro边界测试存在。 - 第 11 轮 R11-3(只读 scope 拥有者静默跳过):无可写 User/Workspace owner 时,仍清除不兼容的 live preference 且不写设置;
it.each(['system','systemDefaults'])参数化回归存在。 - 第 8 轮决定(保留 DashScope toggle-only 硬拒绝)得到遵守:错误信息可操作且指明持久化来源("Change or clear the active reasoning effort (model.reasoningEffort when persisted)…"),并在
dashscope.test.ts中逐字固定。 - 子入口导出(
./reasoningPersistence)、standalone 会话的持久化 meta 剥离、超时后广播均有实现与测试对应,无包导出错误风险。
新发现
🟡 Important
- 既有缺口被本 PR 持久化语义放大:
pipeline.ts的buildGenerateContentConfig在samplingParams !== undefined时早退返回,只存在于 else 分支的buildReasoningConfig注入被整体跳过——配置了samplingParams的模型在 generic OpenAI-compatible / DeepSeek / Z.ai wire 上收不到任何 reasoning 参数(default provider 的 clamp 与 DeepSeek 的reasoning扁平化都依赖该注入),静默失效且无告警。本 PR 未触碰该分支(属既有缺口),但 effort 现在跨会话/重启持久,"设过一次、永久静默失效"的粘性被显著放大。DashScope 分级模型(直接读cfg.reasoning)与 disable 路径(在 provider buildRequest 之后改写providerRequest)均不受影响;pipeline.test.ts的注释也承认该行为。可考虑在早退分支返回前合并buildReasoningConfig的结果。 - 文档化的有意取舍,请确认产品后果:a159cdcff 的早退守卫用前缀匹配
isQwenFamilyWireModel+ 精确查表getModelConfiguration,而 wire/持久化层全部前缀匹配——qwen3.8-max-latest、带日期快照与coder-model在运行时完全支持 reasoning,但 ACP 选择器与 welcome preview 均不投影。这是本 PR 设计文档(model-reasoning-capabilities.md "Deferred models")明确记录的延迟决定,故不作为缺陷报告;但coder-model是默认模型(DEFAULT_QWEN_MODEL),后果是默认模型用户完全看不到 reasoning 控件,建议明确这是否为有意的产品行为。
🟢 Nit
config.ts的 QWEN_OAUTH 热切换字段拷贝表漏extra_body——本 PR 刚把reasoning/thinkingMandatory加入同一张表,并把 extra_body 思考旋钮建模为 preference 敏感状态(strip/restore),却漏了载体本身:无显式 preference 时,从带enableThinking: truepreset 的模型热切到无该 preset 的模型(如 qwen3.8-max → glm-5.2),陈旧enable_thinking: true会并入新模型请求。modelCommand.ts两处switchMainModel:clearIncompatibleReasoningEffortForModel(内部throwOnWriteFailure: true)置于switchModel成功后、model.name/model.baseUrl持久化之前——clear 写盘失败会抛错并跳过模型持久化,且无 ModelDialog 那样的"已切换但未能保存"兜底文案。webui actions.ts:持久化失败但 live 已确认时,先无条件发severity:'error'的 "Set reasoning effort failed" toast 再正常 return——部分成功被渲染成完全失败,与已更新的 effort chip 状态矛盾;建议该路径改用 warning 级与"已应用于当前会话,但默认值未保存"措辞。- daemon 合成的
settings_changed不带originatorClientId(POST /session/:id/config-option也不解析 X-Client-Id),发起方会收到自回声并重复 refetch(幂等无害,但与model.name路径不对称)。 - 新设计文档(webshell-reasoning-effort-persistence.md)措辞 "the request reports that the current session changed but the default was not saved":实现是整个请求以错误终结(
reasoning_effort_persistence_failederrorKind 承载该语义);照文档预期"成功响应带警告"的集成方会落空,建议改为错误语义表述。 - 低置信:managed/standalone 会话的持久化拒绝(−32602)发生在 live 变更已应用之后,错误文案未说明"当前会话已生效",客户端无法与写失败路径区分。
- 低置信:
bridge.ts的Promise.race([withTimeout(...), transportClosed])中 transport-close 分支胜出时,子进程可能已写盘但.then()广播永不执行,其他工作区预览保持陈旧直到下次刷新(超时分支已有测试,close 分支无补偿;窗口极窄,与既有model.name路径同形)。
💡 建议
- 持久化标记的信任边界完全依赖 daemon 边界结构而非显式检查:daemon HTTP 路径安全(服务端自拼标记,HTTP-ACP 显式拒绝
reasoning_effort路由),但直接(stdio)ACP 客户端带公开常量标记即可写全局model.reasoningEffort(同用户信任域,影响有限)。建议设计文档明确"private 标记只是意图标记、非授权机制",或仅在 serve/daemon 宿主下接受该标记。 bridgeClient.ts接收端对value: null的清除通知静默丢弃(当前不可达——清除事件由 daemon 侧合成),建议加注释说明有意丢弃或显式处理,避免未来 child 侧发射清除时刷新静默丢失。- 未注册 Qwen 模型的"隐藏"仅在投影层(
buildConfigOptions早退),setSessionConfigOption写路径无对应能力守卫;若要"隐藏即禁用"语义,建议补齐守卫,否则注释说明隐藏仅为投影语义。
结论
无新的已验证 Critical;此前各轮 Critical 的修复均在 head 复验成立。判定:💬 评论。
Independent static review at head 7092e8e9b (clean merge of main, zero combined conflict hunks; fully static — no tests or builds run). The 84 files were reviewed in module batches; every conclusion below was cross-verified against the head tree with witnesses.
Re-verification: all prior-round Criticals are confirmed fixed at this head
- Round-10 R10-1 (clear path missed SystemDefaults ownership): the clear path of
Session.persistReasoningEffortnow checks bothsystemandsystemDefaultsownership and throws −32603reasoning_effort_persistence_failedon conflict;Session.test.tshas a system-defaults-only case asserting the error and thatsetValuewas not called. - Round-11 R11-1 (live clear gated on
persistDefault):Session.setModelnow callsclearIncompatibleReasoningEffortForModelunconditionally after a successfulswitchModel(Session.ts:9751-9756); thepersistDefault:falseand standalone/managed tests assertsetReasoningEffort(undefined)with no settings write. - Round-11 R11-2 (switch-time cleanup early-returned for non-Qwen targets): the helper is now target-aware — tiered
qwen3.8-maxpreserves any effort (including opaque); toggle-only Qwen clears any string effort; non-Qwen clears only opaque non-built-ins while preserving the built-in tiers; thevendor.ultra → gemini-2.5-proboundary test exists. - Round-11 R11-3 (read-only scope owners silently skipped): with no writable User/Workspace owner, an incompatible live preference is still cleared without any settings write; parameterized regression over
['system','systemDefaults']exists. - The round-8 decision (retain the DashScope toggle-only hard rejection) is honored: the error message is actionable and identifies the persisted source ("Change or clear the active reasoning effort (model.reasoningEffort when persisted)…"), pinned verbatim in
dashscope.test.ts. - Sub-entry export (
./reasoningPersistence), persistence-meta strip for standalone sessions, and the post-timeout broadcast all have matching implementation and tests; no package-export error risk.
New findings
🟡 Important
- Pre-existing gap amplified by this PR's persistence semantics:
pipeline.ts'sbuildGenerateContentConfigearly-returns whensamplingParams !== undefined, so thebuildReasoningConfiginjection (present only in the else branch) is skipped entirely — models configured withsamplingParamsreceive no reasoning parameter at all on generic OpenAI-compatible / DeepSeek / Z.ai wires (the default provider's clamp and DeepSeek'sreasoningflattening both depend on that injection), silently and without warning. This PR does not touch that branch (pre-existing), but effort now persists across sessions/restarts, making "set once, silently ineffective forever" far stickier. DashScope tiered models (readcfg.reasoningdirectly) and the disable path (rewritesproviderRequestafter provider buildRequest) are both unaffected; the PR's ownpipeline.test.tscomment acknowledges the behavior. Consider merging thebuildReasoningConfigresult into the early-return branch. - Documented trade-off — please confirm the product consequence: the a159cdc early-exit guard uses prefix-matched
isQwenFamilyWireModel+ exact-keygetModelConfigurationlookup, while the wire/persistence layers are all prefix-matched —qwen3.8-max-latest, dated snapshots andcoder-modelare fully reasoning-capable at runtime, yet get neither the ACP selector nor the welcome preview. This is explicitly documented in this PR's design doc (model-reasoning-capabilities.md "Deferred models"), so it is not reported as a defect; howevercoder-modelis the default model (DEFAULT_QWEN_MODEL), so the consequence is that default-model users see no reasoning controls at all — please confirm this is intentional.
🟢 Nit
- The QWEN_OAUTH hot-switch field copy list in
config.tsomitsextra_body— this PR just addedreasoning/thinkingMandatoryto that same list and models the extra_body thinking knobs as preference-sensitive state (strip/restore), yet misses the carrier: with no explicit preference, hot-switching from a model with anenableThinking: truepreset to one without it (e.g. qwen3.8-max → glm-5.2) merges the staleenable_thinking: trueinto the new model's request. modelCommand.tsswitchMainModel(both call sites):clearIncompatibleReasoningEffortForModel(internalthrowOnWriteFailure: true) runs after a successfulswitchModeland beforemodel.name/model.baseUrlpersistence — a clear write failure throws and skips model persistence, with no ModelDialog-style "switched but could not save" fallback message.webui actions.ts: when persistence fails but the live state is confirmed, an unconditionalseverity:'error'"Set reasoning effort failed" toast is emitted before returning normally — a partial success is rendered as a full failure, contradicting the already-updated effort chip; consider warning-level "applied to this session, but the default was not saved" wording on this path.- The daemon-synthesized
settings_changedcarries nooriginatorClientId(POST /session/:id/config-optionalso does not parse X-Client-Id), so the initiator receives its own echo and refetches redundantly (idempotent and harmless, but asymmetric with themodel.namepath). - The new design doc (webshell-reasoning-effort-persistence.md) says "the request reports that the current session changed but the default was not saved": the implementation rejects the whole request with an error (the
reasoning_effort_persistence_failederrorKind carries the semantics); integrators expecting a "success response with a warning" per the doc will be surprised; suggest rewording to the error semantics. - Low confidence: the persistence rejection (−32602) for managed/standalone sessions fires after the live change has been applied, and the message does not convey "active for the current session", so clients cannot distinguish it from the write-failure path.
- Low confidence: in
bridge.ts'sPromise.race([withTimeout(...), transportClosed]), when the transport-close branch wins, the child may have already written to disk but the.then()broadcast never fires, leaving other workspaces' previews stale until the next refresh (the timeout branch has a test; the close branch has no compensation; the window is extremely narrow and same-shaped as the existingmodel.namepath).
💡 Suggestions
- The persistence marker's trust boundary relies entirely on the daemon boundary's structure rather than an explicit check: the daemon HTTP path is safe (marker assembled server-side; HTTP-ACP explicitly rejects
reasoning_effortrouting), but a direct (stdio) ACP client can write globalmodel.reasoningEffortby attaching the publicly known constant marker (same-user trust domain, limited blast radius). Consider documenting that the private marker is an intent marker, not an authorization mechanism, or accepting the marker only under the serve/daemon host. - The
bridgeClient.tsreceiver silently discardsvalue: nullclear notifications (currently unreachable — clear events are synthesized daemon-side); add a comment that the drop is intentional or handle it explicitly, otherwise a future child-side clear would be silently lost. - The "hide" for unregistered Qwen models is projection-only (
buildConfigOptionsearly return); thesetSessionConfigOptionwrite path has no corresponding capability guard. If "hidden = disabled" is intended, add the guard; otherwise note that hiding is projection-only semantics.
Conclusion
No new verified Criticals; all prior-round Critical fixes re-verified as standing at this head. Verdict: 💬 Comment.
yiliang114
left a comment
There was a problem hiding this comment.
Approving. The post-round delta is small and reviewed directly: a159cdc hides the ACP reasoning selector for unregistered Qwen-family models via isQwenFamilyWireModel + registered-config check (with test and design-doc update); 77bd395 generalizes the clear-on-switch to incompatible opaque preferences with a session-only live-override path and adds tests (+81 persistence / +31 Session / +12 modelCommand); 3dc4e4d is a mock fixup. Earlier rounds' 53 threads are all resolved; CI green on head 7092e8e, MERGEABLE. No blockers found.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- zh-CN effort labels regress to hardcoded English daemon names / dead reasoning.effort.* i18n keys — already reported by maintainer @wenshao (issue comment 5438383044), still stands at the reviewed commit
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/cli/src/config/config.ts:2344 — [review] loadCliConfig reasoning pass-through exercised by no committed testpackages/acp-bridge/src/bridgeClient.ts:2757 — [review] settings-changed demux drops malformed/non-owned notifications silently (no drop logging)packages/cli/src/acp-integration/model-configuration.ts:110 — [probe] 'high' displays as 'xhigh' while the DashScope wire ships 'high' unchanged; re-pick escalates the effortpackages/cli/src/acp-integration/session/Session.ts:9751 — [probe] session-only clear never restores the still-persisted effort when switching back to a compatible modelpackages/cli/src/acp-integration/session/Session.ts:9577 — [probe] clear-guard bypass: a target-scope entry shadowed by a higher scope is deleted silently while the clear reports success
Convergence: round 12 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/cli/src/config/reasoning-effort-persistence.ts (findings in round 11; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
收敛姿态下延后(第 12 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 12 轮发布了 2 条行内评论,其中 2 条是首次提出;上一轮发布了 3 条(其中 3 条首次提出)。发现反复回到同一批文件:packages/cli/src/config/reasoning-effort-persistence.ts(第 11 轮已出过发现,本轮又有 2 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /review |
Both blocking findings were addressed in 74ab5e9, with regression tests for per-scope compatibility and full pre/post route identity. Both review threads are resolved, CI is running on the new head, and a replacement review has been requested.
|
Addressed the maintainer-reported zh-CN reasoning-effort label issue in 8d44a23. WebShell now localizes known built-in values by their stable value in both the model chip and reasoning menu, while unknown/provider-specific values continue to use the daemon-supplied name. The daemon payload and selection values remain unchanged. Added regression coverage for all built-in tiers plus a Vendor.Ultra fallback; ChatEditor 103/103, WebShell typecheck, lint, targeted Prettier, and build pass. |
|
@qwen-code /review |
|
Follow-up for the web-shell E2E regression on the previous head:
Verified on
|
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- SettingsDialog.tsx:503 untrimmed reasoning-effort save — already reported as R1-7 location 1/3 (comment 3857746242), kept by design
- ACP/Session untrimmed effort pass-through (acpAgent.ts value.trim() selection) — already reported as R1-7 siblings (comments 3857746275, 3857746284), kept by design
- modelConfigUtils.ts padded/case-variant sentinel pass-through — already recorded in the round-8 deferred list (modelConfigUtils.ts:435)
- config.test.ts knob-strip assertions use toMatchObject — already recorded in the round-8 deferred list (config.test.ts:5410)
- Gemini effort throw lacks recovery guidance — already recorded in the round-10 deferred list (geminiContentGenerator.ts:211)
- actions.ts providers refresh is the only unwrapped daemon await — already recorded in the round-5 deferred list (actions.ts:1158)
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): chunk 6: could not run cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts (no installed dependencies/built dist in the worktree); chunk 7: executing acpAgent.test.ts to confirm the traced tests pass — no node_modules in the worktree or main checkout; full install+build skipped as it exceeds the r…; chunk 3: execute the new setSessionConfigOption describe block in bridge.test.ts — the worktree has no node_modules , and a full npm ci + build exceeds this revie….
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
docs/design/webshell-reasoning-effort-persistence.md:19 — [review] Design doc overstates the toggle-only clear: a persisted none is preservedpackages/cli/src/acp-integration/session/Session.ts:9752 — [review] Model-switch auto-clear of the persisted effort emits no settings_changed notificationpackages/cli/src/acp-integration/session/Session.ts:9752 — [review] A cleanup write failure after switchModel commits strands a half-finished model switch
Convergence: round 13 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/config/reasoning-effort-persistence.ts (findings in rounds 11, 12; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 2, previous 2), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 6 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):chunk 6:could not run cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts (no installed dependencies/built dist in the worktree);chunk 7:executing acpAgent.test.ts to confirm the traced tests pass — no node_modules in the worktree or main checkout; full install+build skipped as it exceeds the r…;chunk 3:execute the new setSessionConfigOption describe block in bridge.test.ts — the worktree has no node_modules , and a full npm ci + build exceeds this revie…。
收敛姿态下延后(第 13 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 13 轮发布了 2 条行内评论,其中 2 条是首次提出;上一轮发布了 2 条(其中 2 条首次提出)。发现反复回到同一批文件:packages/cli/src/config/reasoning-effort-persistence.ts(第 11、12 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,可以避免循环反复推导同一组发现;本 PR 的评审已解析为 critical 发布下限。(仅为观察——本轮评审未因此扣留任何内容。)
残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 2 条 Critical),首次发现的速率没有下降(本轮 2,上一轮 2),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- design doc overstates the toggle-only clear (preserved 'none') — already deferred by the concurrent round-13 review body (docs/design/webshell-reasoning-effort-persistence.md:19)
- knob-strip assertions use toMatchObject — already recorded in the round-8 deferred list (config.test.ts:5410)
- disabled->effort pre-clear guard unpinned by any test — already recorded in the round-4/5/6/8 deferred lists (acpAgent.ts:5648)
- loadCliConfig reasoning wiring untested end-to-end — already recorded in the round-12 deferred list (packages/cli/src/config/config.ts:2344)
- real-process integration option list no longer pinned exactly — already recorded in the round-9 deferred list (acp-integration.test.ts:555)
- settings-reload and session-restore switches bypass the cleanup — already reported as Critical R13-1 by the concurrent round-13 review (comment 3883943442)
- integration file unreachable by workspace test commands — already recorded in the round-8 deferred list (acp-integration.test.ts:666)
- switch-time clear never propagates to attached clients — already deferred by the concurrent round-13 review body (Session.ts:9752)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — test-efficacy probing was inconclusive (probe-tree build guard stopped all mutant/hunk probes; harness could not be validated).
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
packages/cli/src/config/reasoning-effort-persistence.ts:43 — [probe] resetLivePreference's persisted-'none' restore branch unpinned by any testpackages/cli/src/ui/commands/modelCommand.ts:178 — [review] /model --project clear ignores scopeOverride and wipes the user-scope effort defaultdocs/users/configuration/settings.md:177 — [probe] settings row drops the provider clamp caveat and misstates blank handlingpackages/cli/src/config/reasoning-effort-persistence.ts:77 — [probe] partial multi-scope clear has no rollback on write failurepackages/cli/src/utils/modelConfigUtils.ts:431 — [probe] persisted-none warning never fires for built-in registry presets
Convergence: round 13 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 2 (2 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (3 Critical(s)), the rate of first-time findings is not falling (this round 3, previous 2), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 8 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — test-efficacy probing was inconclusive (probe-tree build guard stopped all mutant/hunk probes; harness could not be validated)。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
收敛姿态下延后(第 13 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 13 轮发布了 3 条行内评论,其中 3 条是首次提出;上一轮发布了 2 条(其中 2 条首次提出)。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送,可以避免循环反复推导同一组发现;本 PR 的评审已解析为 critical 发布下限。(仅为观察——本轮评审未因此扣留任何内容。)
残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 3 条 Critical),首次发现的速率没有下降(本轮 3,上一轮 2),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — test-efficacy probing was inconclusive (probe-tree build guard stopped all mutant/hunk probes; harness could not be validated).
Not explored to full depth (tool budget reached): "agent 1c": none — no check was cut short.; chunk 5: running the two changed test files under vitest — the review worktree has no node_modules or built dist/ , and a full install + build exceeded the remaining ….
Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.ts:5637 — [probe] D14-1: Opaque effort accepted for toggle-only models wedges every subsequent requestpackages/cli/src/acp-integration/acpAgent.ts:5661 — [probe] D14-3: Load-bearing clear-before-set re-enable branch unpinned; removal keeps 520/520 greenpackages/cli/src/acp-integration/acpAgent.ts:12089 — [probe] D14-8: Workspace-reload cleanup is session-only although the triggering selection is persisted — reboot re-infectspackages/cli/src/acp-integration/session-model-persistence.ts:262 — [probe] D14-9: Auth-rollback preference restore no-ops when reasoning was disabled mid-restorepackages/cli/src/config/reasoning-effort-persistence.test.ts:306 — [probe] D14-2: Multi-scope continue-past-failed-clear semantic unpinned; continue->break mutant ships greenpackages/cli/src/config/reasoning-effort-persistence.test.ts:315 — [probe] D14-7: Failed-write in-memory retention unpinned; unconditional-delete mutant ships greenpackages/cli/src/config/reasoning-effort-persistence.test.ts:353 — [probe] D14-10: Same-route guard placement before the session-only branch unpinned; reorder mutant survives 956 testspackages/cli/src/acp-integration/acpAgent.test.ts:2401 — [probe] D14-4: Builder-wiring test asserts only typeof function; () => [] mutant stays greenpackages/cli/src/acp-integration/session/Session.test.ts:5166 — [probe] D14-5: Clear-guard trusted-workspace-owns disjunct unpinned; mutant turns rejection into silent successpackages/cli/src/acp-integration/session-model-persistence.test.ts:198 — [probe] D14-6: Restore cleanup call site's 5th route-identity argument unpinned; removal survives all suites
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — test-efficacy probing was inconclusive (probe-tree build guard stopped all mutant/hunk probes; harness could not be validated)。
未探索到全部深度(达到工具调用预算):"agent 1c":none — no check was cut short.;chunk 5:running the two changed test files under vitest — the review worktree has no node_modules or built dist/ , and a full install + build exceeded the remaining …。
收敛姿态下延后(第 14 轮,非阻断)——已记录,本轮不要求修改:共 10 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- disabled->effort pre-clear guard unpinned by any test (acpAgent.ts:5663) — already reported in the round-4/5/6/8 deferred lists
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/session-model-persistence.ts:262 — [probe] rollback 'disabled preference' branch untestedpackages/cli/src/acp-integration/acpAgent.test.ts:2401 — [probe] config-option builder seam pinned only by toBeTypeOfpackages/cli/src/config/reasoning-effort-persistence.test.ts:306 — [probe] write-failure memory/disk consistency unpinnedpackages/cli/src/ui/components/ModelDialog.test.tsx:501 — [probe] capture-line deletion unwitnessed (clear always runs)packages/cli/src/acp-integration/session/Session.test.ts:4628 — [probe] setModel write-failure ordering unpinnedpackages/cli/src/acp-integration/session/Session.test.ts:5248 — [probe] disk-full test omits the errorKind payload assertionpackages/cli/src/acp-integration/session/Session.test.ts:5166 — [probe] User-persistScope ownership arm unpinnedpackages/cli/src/acp-integration/session/Session.test.ts:5150 — [probe] return-side blank normalization unwitnessedpackages/cli/src/ui/components/ModelDialog.test.tsx:501 — [probe] order-keyed mock cannot detect a late capturepackages/cli/src/acp-integration/session/Session.test.ts:5078 — [probe] shadow-check equality escape hatch unpinnedpackages/cli/src/acp-integration/session/Session.test.ts:4949 — [probe] standalone clear rejection untestedpackages/cli/src/config/reasoning-effort-persistence.test.ts:306 — [probe] multi-scope continue-on-failure unpinnedpackages/cli/src/ui/commands/modelCommand.ts:140 — [probe] /model capture timing has no state-keyed witnesspackages/cli/src/acp-integration/acpAgent.test.ts:8908 — [probe] opaque-route 'none' selection unwitnessedpackages/cli/src/acp-integration/session-model-persistence.ts:193 — [probe] restore-seam identity pass-through unwitnessedpackages/cli/src/acp-integration/session/Session.test.ts:5268 — [probe] config_option_update builder re-invocation unpinnedpackages/cli/src/config/reasoning-effort-persistence.test.ts:100 — [probe] disk clear's independence from live preference unpinnedpackages/cli/src/config/reasoning-effort-persistence.test.ts:344 — [probe] session-only route-guard unpinnedpackages/cli/src/ui/commands/modelCommand.ts:152 — [probe] provider-qualified branch clear call unwitnessed
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 14 轮,非阻断)——已记录,本轮不要求修改:共 19 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Superseded by #10489, which rebuilds the scoped WebShell model/reasoning persistence change from the latest main without reusing this PR's commits. The replacement keeps Welcome lazy, reuses model.reasoningEffort without adding model.reasoningEnabled, clears incompatible tiers instead of downgrading them, and includes the toggle-only/unsupported/i18n regression matrix. |
Pull request was closed
What this PR does
WebShell reasoning selections now update the active session immediately and persist the same value as the global
model.reasoningEffortdefault for future sessions and daemon restarts. Exact lowercasenonedisables reasoning, an absent value uses the model or provider default, and every other non-empty string is preserved as an opaque model-specific effort value without a global allowlist, normalization, ranking, clamping, or replacement.The persisted setting is only written when WebShell sends its private persistence marker; ordinary ACP clients remain session-only, and ACP
defaultclears the setting. Model capability options carry separate values and display names, provider adapters own protocol-specific conversion, the welcome preview follows persisted state without pre-creating a session, and persistence failures leave the active session changed while reporting that the default was not saved.Why it's needed
Users currently have to reselect Medium or Thinking Off after every new WebShell session or daemon restart. The former global effort enum also cannot represent model-specific values such as
ultra,minimal, mixed-case values, or future provider additions without silently changing user intent.Reviewer Test Plan
How to verify
model.reasoningEfforttoultra,minimal, a custom value, and a mixed-case value. Each value should round-trip unchanged; only exact lowercasenoneshould disable reasoning.Evidence (Before & After)
Medium across daemon restart and a new session
Thinking Off across daemon restart and a new session
Tested on
Environment (optional)
Real local daemon and WebShell on macOS, using separate temporary configuration directories for baseline and fixed builds. Focused unit and integration tests, WebShell Playwright reasoning scenarios, the full repository build, full typecheck, and full lint all pass locally.
Risk & Scope
/effortcommand's built-in five-tier selector are intentionally unchanged.Linked Issues
N/A
中文说明
本 PR 做了什么
WebShell 中的思考档位选择现在会立即更新当前会话,并将同一个值持久化为全局
model.reasoningEffort默认值,供后续新会话和 daemon 重启后恢复。精确的小写none表示关闭思考;字段缺省表示使用模型或 Provider 默认值;其他所有非空字符串都按模型专属的不透明值原样保留,不经过全局白名单、大小写归一化、强度排序、截断或替换。只有 WebShell 携带内部持久化标记时才会写入设置;普通 ACP 客户端仍只影响当前会话,ACP 的
default用于清除设置。模型能力选项使用独立的值和展示名,Provider 适配器负责协议级转换;欢迎页预览无需预创建会话即可跟随持久化状态;如果持久化失败,当前会话的选择仍然生效,并明确提示默认值未保存。为什么需要
目前用户每次新建 WebShell 会话或重启 daemon 后,都必须重新选择 Medium 或关闭思考。原有的全局 effort 枚举也无法表达
ultra、minimal、大小写不同的值或未来 Provider 新增值,并可能静默改变用户意图。Reviewer 测试计划
如何验证
model.reasoningEffort设置为ultra、minimal、自定义值和大小写混合值。每个值都应原样往返;只有精确的小写none会关闭思考。证据(修复前后)
Medium 在 daemon 重启及新会话后恢复
关闭思考在 daemon 重启及新会话后恢复
已测试平台
环境(可选)
在 macOS 上使用真实本地 daemon 和 WebShell,并为基线版本与修复版本分别使用临时独立配置目录。聚焦单元与集成测试、WebShell Playwright 思考场景、全仓构建、全仓类型检查和全仓 Lint 均在本地通过。
风险与范围
/effort命令的内置五档选择器有意保持不变。关联 Issue
无