fix(web-shell): support model & approval-mode changes for non-primary workspace sessions - #6737
Merged
Merged
Conversation
qwen-code-ci-bot
requested changes
Jul 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Hey @wenshao — thanks for the PR and the detailed write-up. The description is thorough, but it doesn't follow our PR template. The current headings (## Summary, ## Root cause, ## Changes, ## Testing, ## Out of scope / follow-ups) don't match the required sections:
## What this PR does— what the change does in prose## Why it's needed— motivation / problem being solved## Reviewer Test Plan— with### How to verify,### Evidence (Before & After), and### Tested onsubsections## Risk & Scope— main risk, what's out of scope, breaking changes## Linked Issues— linked issue or "none"
Could you reformat the body to match the template? The content is all there — it just needs to be mapped to the right headings so reviewers can find what they need quickly. The ### Evidence (Before & After) and ### Tested on sections are especially important for this kind of behavioral change.
中文说明
@wenshao 你好——感谢这个 PR 和详尽的描述。不过 PR 正文没有按照我们的 PR 模板 来写。当前的标题（## Summary、## Root cause、## Changes、## Testing、## Out of scope / follow-ups）和模板要求的章节不一致：
## What this PR does— 用文字描述变更内容## Why it's needed— 动机 / 要解决的问题## Reviewer Test Plan— 包含### How to verify、### Evidence (Before & After)和### Tested on三个子节## Risk & Scope— 主要风险、不在范围内的内容、破坏性变更## Linked Issues— 关联的 issue 或写 "none"
能否按模板重新组织一下正文？内容都在，只是需要映射到正确的标题下，方便 reviewer 快速找到所需信息。### Evidence (Before & After) 和 ### Tested on 对于这类行为变更尤其重要。
— Qwen Code · qwen3.7-max
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. |
… workspace sessions POST /session/:id/model and /session/:id/approval-mode used withMutableSession, which rejects non-primary workspace sessions with a Phase 2a "primary-only" 400. Creating a session in a newly registered (non-primary) workspace in Web Shell surfaced "Set model failed" / "Set approval mode failed" error toasts. Switch both routes to withOwnerMutableSession + runtime.bridge, resolving the session's owning workspace runtime — the same pattern POST /session/:id/prompt already uses. Each WorkspaceRuntime owns a full AcpSessionBridge, so the mutation lands on the correct workspace (and approval-mode persist targets that workspace's own settings). Primary sessions are unchanged: the primary runtime's bridge is the same object as the closed-over primary bridge. Also fold the initial approval mode into the Web Shell create request (POST /session already applies it via spawnOrAttach) so a new session applies its mode atomically at spawn: one fewer round-trip, and fail-closed on the approval setting — a mode that can't be applied aborts creation instead of silently running in a different mode than requested. The model stays a best-effort follow-up because creation only accepts a modelServiceId, not the composer's plain modelId (that follow-up now works on non-primary workspaces too, via the route change above).
wenshao
force-pushed
the
fix/non-primary-session-model-approval
branch
from
July 11, 2026 16:14
b402690 to
0491f91
Compare
wenshao
changed the title
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
doudouOUC
approved these changes
Jul 11, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
APPROVE — verified the fix against the code at 0491f9141. The daemon route change, the type chain, the fail-closed narrative, and the tests all hold up.
What I checked
- Owner-scoped routing is correct.
POST /session/:id/modelandPOST /session/:id/approval-modenow usewithOwnerMutableSession+runtime.bridge.*, identical to the pattern/promptalready uses. The trust gate is preserved —requireSessionRuntimestill returns403 untrusted_workspacefor a non-primary, non-trusted runtime before the bridge runs (confirmed by the new test). - Primary sessions are unchanged. The closed-over
bridgeisprimaryRuntime.bridge, so for a primary sessionruntime.bridgeresolves to the very same object. No leftover primarybridge.references remain in either handler. - The fail-closed narrative matches the daemon. At spawn, model application is fail-open (
.catch) while approval-mode is fail-closed (closes the freshly-spawned session and rethrows onapproval_mode_initialization_failed). Folding approval intoPOST /sessionand keeping the model as a best-effort follow-up mirrors that split exactly. - The type chain is sound.
CreateSessionRequest.approvalModealready exists in the SDK, soPick<…, 'approvalMode'>and the spreads type-check. TheApp.tsxcall site passes a cast variable (not a fresh object literal), so droppingsetApprovalModefrom the actions type is safe — no excess-property error. - No client/daemon mode divergence.
DAEMON_APPROVAL_MODESequals the daemon'sAPPROVAL_MODES(plan / default / auto-edit / auto / yolo), so folding a picked mode into creation never spuriously 400s — the fail-closed path only triggers on a genuine agent-side application failure. - No dangling state on failure.
createSessionassignssessionRef.currentonly after a successful create, so a fail-closed rejection leaves no half-created client session; the daemon tears its own down.
Tests
Solid. The multi-workspace tests drive a real WorkspaceRegistry + real route registration (only the bridge is faked), covering owner-scoped routing for both routes, creation-time approvalMode on the non-primary runtime, and the untrusted-rejection gate. Primary-path coverage for both routes already lives in server.test.ts, so the wrapper swap can't silently regress the primary path.
One thing to confirm (non-blocking)
The fail-open → fail-closed shift for approval mode applies to all Web Shell prompt-session creations via the shared createAndAttachSessionForPrompt path, not only non-primary ones — so a primary-workspace session that previously degraded-to-default + warned will now abort creation on an approval-mode failure. This is disclosed in the
(Heads-up: the CHANGES_REQUESTED on this PR is the CI PR-template check, not a code issue — filling in the PR template will clear that gate.)
ytahdn
reviewed
Jul 11, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: review-pr. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/cli/src/serve/multi-workspace-sessions.test.ts (approval-mode route test) |
clientId context forwarding is tested for the model route (X-Qwen-Client-Id header + context assertion) but not for the approval-mode route. Both routes gained the same runtime parameter — a copy-paste gap could drop context on one path. |
Add .set('X-Qwen-Client-Id', 'client-1') to the approval-mode test and assert secondaryBridge.setApprovalModeCalls[0]?.context equals { clientId: 'client-1' }. |
packages/web-shell/.../sessionPreparation.ts:38 + packages/webui/.../actions.ts:595 |
The "fold approval mode into creation" logic is independently implemented in two packages with subtly different shapes. If one is updated and the other is not, web-shell and webui will silently diverge. | Extract a shared helper (e.g. buildApprovalModeOverride(modeId)) into a common package, or add cross-reference comments pointing to the twin implementation. |
— qwen3.7-max via Qwen Code /review
ytahdn
approved these changes
Jul 11, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM ✅
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
approved these changes
Jul 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
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
4 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.
Summary
Creating a session in a newly registered (non-primary) workspace in Web Shell surfaced
Set model failed/Set approval mode failederror toasts:POST /session/:id/modelandPOST /session/:id/approval-modewere wrapped inwithMutableSession, which hard-rejects any non-primary workspace session with a Phase 2a primary-only400— even thoughPOST /session/:id/promptalready serves non-primary sessions viawithOwnerMutableSession. These two routes simply had not been migrated.Root cause
withMutableSessiongates onruntime.primary:Web Shell creates a session and then pushes the picked model + approval mode with follow-up calls; on a non-primary workspace those two calls hit the primary-only gate and fail.
Changes
1. Daemon — enable both routes for non-primary owned sessions (
packages/cli)POST /session/:id/modelandPOST /session/:id/approval-modefromwithMutableSessiontowithOwnerMutableSession, and callruntime.bridge.*instead of the closed-over primarybridge.*. This resolves the session's owningWorkspaceRuntime— the exact pattern/promptalready uses.WorkspaceRuntimeowns a fullAcpSessionBridge, so the mutation lands on the correct workspace, andpersist: truetargets that workspace's own settings. Primary sessions are unchanged: the primary runtime's bridge is the closed-overbridgeobject.requireSessionRuntimestill returns403 untrusted_workspacefor a non-primary, non-trusted runtime before the bridge runs.This fixes the reported error and also makes the manual model dropdown / approval-mode toggle work on non-primary sessions.
2. Web Shell — apply approval mode at session creation (
packages/web-shell,packages/webui)approvalModeonPOST /session(→spawnOrAttach({ approvalMode })), per-runtime. Web Shell now folds the initial approval mode into the create request instead of a follow-upsetApprovalMode— one fewer round-trip, mode applied atomically at spawn.createSession(options)gains an optionalapprovalMode, threaded into both the active (createOrAttachSession) and detached create branches.modelServiceId, not the composer's plainmodelId. That follow-up now works on non-primary workspaces via change pre-release: fix ci #1.The daemon treats spawn-time approval-mode application as fail-closed: if the requested mode can't be applied it closes the freshly-spawned session and rethrows, so
POST /sessionfails. Folding approval mode into creation therefore changes Web Shell from degrade-to-default + warn to abort creation on failure. This is intentional — it prevents silently running a session in a different (possibly less-safe) mode than requested (e.g. asking forplanand silently gettingdefault). The model path stays fail-open (non-fatal), matching the daemon's own model-vs-approval distinction.Testing
Real
vitest runoutput (rows marked NEW are added by this PR):packages/cli·multi-workspace-sessions.test.ts— 42 passed: owner-scoped routing for/model&/approval-mode, creation-timeapprovalModeon the non-primary runtime, and the untrusted-rejection gate.packages/web-shell·sessionPreparation.test.ts— 8 passed: approval-into-create fold + fail-closed propagation.packages/webui·actions.test.ts— 18 passed:approvalModethreaded into both create branches.cliserver.test.ts670, webuidaemon/session202.tsc --noEmit, eslint, prettier clean across all three packages.Out of scope / follow-ups
Other session-mutation routes stay primary-only (
withMutableSession):recap,branch(deliberately primary-only — has a Phase 2a test),fork,rewind,goal/clear,language. Some are reachable from Web Shell quick-actions on a non-primary active session and would hit the same class of error; they can be migrated the same way, but each warrants its own review, so they are kept out of this focused fix.中文版（点击展开）
概述
在 Web Shell 中，于新注册的（非主）工作区创建会话时会弹出
Set model failed/Set approval mode failed报错：POST /session/:id/model和POST /session/:id/approval-mode用的是withMutableSession包装器，会对非主工作区会话直接返回 Phase 2a 的 primary-only400——而POST /session/:id/prompt早已通过withOwnerMutableSession支持非主会话。这两个路由只是没跟上迁移。根因
withMutableSession以runtime.primary为闸门：Web Shell 先建会话、再用后续调用下发所选模型与审批模式；在非主工作区，这两个后续调用命中 primary-only 闸门而失败。
改动
1. 守护进程——为非主的“归属”会话放开这两个路由（
packages/cli）POST /session/:id/model与POST /session/:id/approval-mode从withMutableSession换成withOwnerMutableSession，并调用runtime.bridge.*而非闭包里的主bridge.*，从而解析出会话所属的WorkspaceRuntime——与/prompt完全一致的模式。WorkspaceRuntime自带完整的AcpSessionBridge，因此变更会精确落到对应工作区，persist: true也写入该工作区自己的 settings。主会话行为不变：主 runtime 的 bridge 就是闭包里的bridge对象。requireSessionRuntime对“非主且不受信任”的 runtime 仍在触达 bridge 前返回403 untrusted_workspace。这既修复了报错，也让非主会话里手动切模型 / 切审批模式生效。
2. Web Shell——在建会话时直接下发审批模式（
packages/web-shell、packages/webui）POST /session上接受approvalMode（→spawnOrAttach({ approvalMode })），且按 runtime 生效。现在 Web Shell 把初始审批模式随建会话请求一起下发，省掉一次setApprovalMode往返，并在 spawn 时原子应用。createSession(options)新增可选approvalMode，贯通到 active（createOrAttachSession）与 detached 两条创建分支。modelServiceId，而 composer 给的是modelId；该后续调用现在也能在非主工作区生效（得益于改动 pre-release: fix ci #1）。守护进程对 spawn 阶段的审批模式应用采用 fail-closed：若请求的模式无法应用，它会关闭刚创建的会话并重新抛出，于是
POST /session失败。因此把审批模式折入创建，会把 Web Shell 从“降级为默认模式并告警”改为“失败即中止创建”。这是有意为之——避免会话悄悄以与请求不同（可能更不安全）的模式运行（例如请求plan却静默变成default）。模型路径仍是 fail-open（非致命），与守护进程对“模型 vs 审批”的区分一致。测试
真实
vitest run输出（标注 NEW 的为本 PR 新增）见上方英文版截图。packages/cli·multi-workspace-sessions.test.ts— 42 通过。packages/web-shell·sessionPreparation.test.ts— 8 通过。packages/webui·actions.test.ts— 18 通过。cliserver.test.ts670、webuidaemon/session202；三个包的tsc --noEmit、eslint、prettier 均干净。范围之外 / 后续
其余会话变更路由仍为 primary-only（
withMutableSession）：recap、branch（有意 primary-only，且有 Phase 2a 测试）、fork、rewind、goal/clear、language。其中部分可从 Web Shell 快捷操作在非主的活动会话上触达，会遇到同类报错；可按同样方式迁移，但每个都需单独评估，故不纳入本次聚焦修复。