feat(web-shell): Scope voice to composer workspace - #7754
Conversation
github-actions
Bot
added
the
review/self-reported
🖼️ 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 5 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 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Overall this is a solid, well-tested change — the ownership/trust/generation/invalidation model is careful and coverage is thorough. Two follow-ups after digging into the code:
#1 — loadVoiceSettings cwd validation: no change needed ✅
I initially flagged that loadVoiceSettings doesn't echo-validate workspaceCwd like loadVoiceStatus/loadVoiceProviders do. Retracting that: DaemonWorkspaceSettingsStatus (types.ts:2340) has no workspaceCwd field — only v / warnings / settings — so there's nothing to validate against. Staleness is already handled correctly by the ownerKey + requestRef generation guards in useVoiceWorkspaceSettings, and the request itself is workspace-qualified via workspaceById/ByCwd. All good here.
#2 — activeWorkspaceCwd fallback is redundant for Voice and broadens shared git state
App.tsx adds ?? workspace.capabilities?.workspaceCwd to activeWorkspaceCwd. But resolveVoiceWorkspaceTarget already performs this exact fallback internally (primaryCwd = registered?.find(primary)?.cwd ?? capabilities.workspaceCwd), so an old single-workspace daemon resolves the correct legacy-primary target with or without this line.
The only observable effect of the App-level change is on non-Voice shared state: activeWorkspaceCwd also drives the git-status effect (App.tsx:1488/1496), the Changes dialog (3240), and gitModeEligible (3243). For a legacy single-workspace daemon it now activates git-status fetching + a 30s poll before a session exists, which previously stayed off. That's benign (correct repo), but it contradicts the PR's "Voice-specific and small" framing and isn't covered by any test.
I verified locally on this branch:
- Baseline:
App+ChatPane+voice-workspace-target→ 261 passed. - With the fallback reverted: same suites +
VoiceButton→ 278 passed, zero failures.
So the revert is test-safe and Voice-neutral. Suggested change:
: (lockedWorkspaceCwd ??
selectedWorkspaceCwd ??
- workspaces.find((entry) => entry.primary)?.cwd ??
- workspace.capabilities?.workspaceCwd),
+ workspaces.find((entry) => entry.primary)?.cwd),
[
connection.sessionId,
connection.workspaceCwd,
lockedWorkspaceCwd,
selectedWorkspaceCwd,
- workspace.capabilities?.workspaceCwd,
workspaces,
],If the pre-session git-status behavior for legacy daemons is intentional, please keep it but call it out in the PR description and add a test — right now nothing exercises it (which is why reverting stays green).
中文
整体是高质量且测试充分的改动 —— 所有权/信任/世代/失效模型设计严谨,覆盖全面。深入代码后有两点后续:
#1 —— loadVoiceSettings 的 cwd 校验:无需改动 ✅
我最初提出它没像 loadVoiceStatus/loadVoiceProviders 那样回显校验 workspaceCwd。现撤回:DaemonWorkspaceSettingsStatus(types.ts:2340)根本没有 workspaceCwd 字段(只有 v / warnings / settings),无从校验。过期防护已由 useVoiceWorkspaceSettings 中的 ownerKey + requestRef 世代守卫正确处理,且请求本身经 workspaceById/ByCwd 定址。此处没问题。
#2 —— activeWorkspaceCwd 的 fallback 对 Voice 冗余,且扩大了共享的 git 状态
App.tsx 给 activeWorkspaceCwd 加了 ?? workspace.capabilities?.workspaceCwd。但 resolveVoiceWorkspaceTarget 内部已有完全相同的 fallback(primaryCwd = registered?.find(primary)?.cwd ?? capabilities.workspaceCwd),所以旧单 workspace daemon 无论加不加这行都能解析到正确的 legacy-primary target。
这行唯一可观察的影响落在非 Voice 的共享状态上:activeWorkspaceCwd 还驱动 git-status effect(App.tsx:1488/1496)、Changes 对话框(3240)、gitModeEligible(3243)。旧单 workspace daemon 现在会在 session 建立之前就启动 git-status 请求 + 30s 轮询,而这之前是关闭的。功能上良性(仓库正确),但与 PR "Voice-specific and small" 的表述冲突,且无测试覆盖。
本地在该分支已验证:
- Baseline:
App+ChatPane+voice-workspace-target→ 261 passed。 - 回退该 fallback 后: 上述 +
VoiceButton→ 278 passed,零失败。
即回退安全且对 Voice 中性,建议改动同上。若旧 daemon 的 pre-session git-status 行为是有意为之,请保留但在 PR 描述中说明并补测试 —— 目前完全没有测试触及它(所以回退才会全绿)。
d50a84b to
5846e58
Compare
|
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
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the full diff at 5846e58 and ran the suite locally: client/voice + vite-config + ChatPane + SplitView (153 passed), App.test.tsx (186 passed), and eslint over every changed file (clean). Typecheck was inconclusive in my checkout — the errors it reports are all in files this PR does not touch (GitHubPrsDialog, GitLogDialog, WebShellSidebar), from a stale SDK build, so I did not count it either way.
The architecture choice is right: a Voice-specific resolver returning an immutable descriptor, rather than retargeting DaemonWorkspaceProvider. The capture-generation/snapshot rework is the substantive win — snapshotting onFinal at start() instead of following a latest-ref, plus the three-way snapshotIsCurrent check (mounted / generation / snapshotRef identity), is exactly what closes the "transcript from the wrong runtime lands in this composer" hole. The resolver is pure and fails closed, and the e2e test asserting the legacy route is not called is the most valuable assertion in the PR.
One blocker below, plus a set of smaller items. Everything else reads clean.
中文说明
已完整审查 5846e58 的 diff，并在本地跑了测试：client/voice + vite-config + ChatPane + SplitView（153 通过）、App.test.tsx（186 通过），以及对所有改动文件的 eslint（干净）。typecheck 在我的工作区结论不可用——它报的错全在本 PR 未改动的文件里（GitHubPrsDialog、GitLogDialog、WebShellSidebar），来自过期的 SDK 构建产物，因此没有采信。
架构选择是对的：用 Voice 专属 resolver 返回不可变 descriptor，而不是去改 DaemonWorkspaceProvider。capture generation / snapshot 的改造是本 PR 的实质价值——在 start() 时快照 onFinal 而非沿用 latest-ref，加上 snapshotIsCurrent 的三重校验（mounted / generation / snapshotRef 同一性），正好堵死了「错误 runtime 的转录插进当前输入框」这个缺陷。resolver 是纯函数且失败关闭，而 e2e 里断言 legacy 路由没有被调用，是这个 PR 中最有价值的断言。
下面有一个阻塞项，以及若干较小的问题。其余部分读下来是干净的。
|
Addressed review feedback in
Validation: 300 focused Web Shell tests passed; Web Shell typecheck and production/library build passed; changed-file lint/format checks passed. Full-repository build remains unavailable in this checkout because the already-declared |
d3b3ee8 to
c831fed
Compare
|
Rebased onto Validation:
Review threads resolved: 10/10; remaining unresolved: 0. Rebased head: |
|
Thanks for the PR! Template looks good ✓ Problem: This is a real, observed architectural gap — not theoretical hardening. The daemon and SDK have supported workspace-qualified Voice since #6839, but the Web Shell client still hardcodes the primary workspace's legacy Voice routes. A secondary-workspace composer's microphone control, status, model picker, and settings all bind to the wrong runtime. The linked issue #6972 documents the gap with concrete acceptance criteria and implementation tracking. Direction: Aligned. The CHANGELOG records #6839 ("Voice input is now qualified per workspace for better multi-workspace support") and #7345 ("respect voice enabled setting") as prior steps. This PR is the natural client-side completion of that work. No new protocol, SDK, or UX surface — it routes existing Voice behavior through the correct workspace owner. Size: No core module paths touched — all changes are in Approach: The architecture is sound. A pure resolver ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 这是一个真实的架构缺口，而非理论性加固。daemon 和 SDK 自 #6839 起已支持工作区限定的 Voice，但 Web Shell 客户端仍然硬编码使用主工作区的旧 Voice 路由。次工作区输入框的麦克风控制、状态、模型选择器和设置都绑定到错误的运行时。关联的 issue #6972 记录了具体的验收标准和实现跟踪。 方向： 对齐。CHANGELOG 记录了 #6839（"Voice 输入按工作区限定"）和 #7345（"尊重 Voice 启用设置"）作为前置步骤。本 PR 是该工作的自然客户端完成。不引入新协议、SDK 或 UX 界面。 规模： 未触及核心模块路径——所有改动在 方案： 架构合理。纯函数解析器保持目标解析可测试且无副作用。录音快照模式正确防止过期回调跨越工作区所有者。diff 聚焦于既定目标，无无关改动。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: Given "scope Voice to the composer workspace", I would add a pure resolver mapping (capabilities, intendedCwd, sessionId, workspaces) to an immutable target descriptor with route kind and stream path; pass it from App through ChatEditor to VoiceButton; snapshot the target at capture start and reject stale callbacks via generation identity; resolve per-pane targets in ChatPane for split view; and add a narrow Vite proxy regex for qualified WebSocket upgrades. Comparison: The PR matches this approach and goes further in several areas: picker lifecycle management with pending-intent invalidation, revision-based status invalidation split by user vs workspace scope, structured close-code handling (1012 capability refresh, 1013 capacity preservation, 1006 revalidation), and layout-effect timing for owner changes to prevent races between React commit and browser events. The design doc's rejected alternatives (retargeting the global provider, resolving inside VoiceButton) are well-reasoned. No critical blockers found. No security issues — bearer subprotocol authentication is preserved, tokens never appear in URLs, and qualified failures never fall back to primary. The code follows project conventions (ESM, no Two observations, neither blocking:
sequenceDiagram
participant P1 as App or ChatPane
participant P2 as voice-workspace-target
participant P3 as VoiceButton
participant P4 as useVoiceCapture
participant P5 as Daemon
P1->>P2: resolveVoiceWorkspaceTarget(caps, cwd, session)
P2-->>P1: target (legacy-primary or workspace-qualified)
P1->>P3: target prop
P3->>P5: loadVoiceStatus(client, target)
P5-->>P3: enabled, workspaceCwd
Note over P3: gate open if enabled and cwd matches
P3->>P4: start() snapshots ownerKey and streamPath
P4->>P5: WebSocket to streamPath (qualified or legacy)
P5-->>P4: interim and final frames
Note over P4: deliver final only if snapshot is current
P4-->>P3: onFinal(text)
Files changed (20 of 20 shown)
TestingThis is an unattended CI run — PR code is not executed locally. Evidence comes from the PR's own CI checks on the reviewed commit. The web-shell E2E smoke test ( The sandboxed real-scenario lanes ( Not verified: physical microphone browser E2E (author noted Playwright Chromium is not installed locally), Windows and Linux behavior, provider credentials.
中文说明代码审查独立方案： 给定"将 Voice 限定到输入框工作区"的目标，我会添加一个纯函数解析器，将（capabilities、intendedCwd、sessionId、workspaces）映射为不可变的目标描述符；从 App 经 ChatEditor 传递到 VoiceButton；在录音启动时快照目标并通过代次身份拒绝过期回调；在 ChatPane 中为分屏解析每个面板的目标；并添加窄范围的 Vite 代理正则用于限定的 WebSocket 升级。 对比： PR 与此方案一致，并在多个方面更深入：选择器生命周期管理、按用户/工作区范围拆分的修订版本状态失效、结构化关闭码处理（1012 能力刷新、1013 容量保留、1006 重验证），以及布局效果时序防止 React 提交与浏览器事件之间的竞态。 未发现关键阻塞问题。无安全问题——bearer 子协议认证保留，令牌不出现在 URL 中，限定失败不回退到主工作区。代码遵循项目规范。 两个非阻塞观察：
测试这是无人值守的 CI 运行——不在本地执行 PR 代码。证据来自 PR 自身在审查提交上的 CI 检查。 web-shell E2E 冒烟测试通过，包括新的次工作区 Voice 状态测试。完整单元测试套件（Ubuntu）通过。Windows 和 macOS 测试被跳过（fork PR）。 沙箱真实场景测试通道不可用（作者为外部贡献者，无写入权限）。维护者可在一次性容器中检出 PR 并驱动多工作区 Web Shell 会话来验证行为声明。 未验证：物理麦克风浏览器 E2E、Windows 和 Linux 行为、提供商凭据。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — solid implementation that completes a well-scoped integration; only non-blocking nit is a minor 1012 refresh-failure behavior deviation from the design doc. This PR does one thing and does it well: it routes every existing Voice surface through the workspace that owns the composer. The pure resolver is clean and independently testable, the capture snapshot pattern correctly prevents stale callbacks from crossing owners, and the revision-based invalidation avoids unnecessary aborts across unrelated workspaces. The design doc is thorough without being bloated, and the rejected alternatives are well-reasoned. My independent proposal for this problem was essentially the same architecture — pure resolver, target propagation, capture snapshots, per-pane resolution. The PR exceeds it in edge case handling: picker lifecycle invalidation, structured close-code responses, and layout-effect timing for owner changes. I did not find a simpler path it missed. The scope is large (~1,446 production lines) but driven by the number of Voice surfaces that need workspace awareness. Every change in the diff serves the stated goal. The test coverage is comprehensive (~2,070 lines), and the E2E smoke test verifies the core routing claim. Non-blocking nit: the All CI checks passed on the reviewed commit. Approving. ✅ 中文说明置信度：4/5 —— 实现扎实，完成了一个范围明确的集成；唯一的非阻塞问题是 1012 刷新失败行为与设计文档的轻微偏差。 本 PR 只做一件事并做好：将每个现有 Voice 面路由到拥有输入框的工作区。纯函数解析器干净且可独立测试，录音快照模式正确防止过期回调跨越所有者，基于修订版本的失效避免了不相关工作区之间的不必要中断。 我的独立方案与此架构基本一致。PR 在边缘情况处理上超越了它。未发现更简路径。 范围较大（约 1,446 行生产代码），但由需要工作区感知的 Voice 面数量驱动。diff 中每项改动都服务于既定目标。测试覆盖全面（约 2,070 行），E2E 冒烟测试验证了核心路由声明。 非阻塞问题：1012 关闭码的 所有 CI 检查在审查提交上通过。批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Thanks — deferring this non-blocking documentation correction. The implementation intentionally clears the refresh latch on either outcome and then performs a same-owner status probe, which remains fail-closed; keeping the latch set permanently caused the recovery regression covered by the current tests. Since this PR has already exceeded roughly five review rounds, the stale design sentence is not worth another code-review cycle. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
Review —
|
|
Follow-up to the mutation audit in #7754 (comment):
Validation for the accepted fix: the focused Voice target suite passes (23/23), and the changed test file passes Prettier and ESLint. Resolved 0/0 review threads for this update: the audit was posted as a top-level comment rather than an inline review thread. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] App.tsx voiceTarget clearing on inline panels (activePanel !== null) — blocker comment 3654175742: approvals fixed but inline panels still clear; author's reply addressed only dialogs/approvals, intent for inline panels cannot be determined from the code
Not reviewed: reverse audit round 2 — auditors ran with hand-written prompts instead of CLI-built prompts.
Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
中文说明
未决，请确认：[Critical] App.tsx voiceTarget clearing on inline panels (activePanel !== null) — blocker comment 3654175742: approvals fixed but inline panels still clear; author's reply addressed only dialogs/approvals, intent for inline panels cannot be determined from the code
未审查：reverse audit round 2 — auditors ran with hand-written prompts instead of CLI-built prompts。
未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。
— qwen3.7-max via Qwen Code /review
| voiceTarget={ | ||
| activePanel !== null || mainView !== 'chat' | ||
| ? undefined | ||
| : mainVoiceTarget | ||
| } |
There was a problem hiding this comment.
[Critical] Inline panels (Settings, Tools, Memory, Sessions, etc.) still clear voiceTarget and abort an in-flight recording — Failure scenario: user presses mic → opens Settings panel → activePanel becomes non-null → voiceTarget becomes undefined → useVoiceCapture's targetIdentity layout effect calls abort() → recording is discarded with no user notification.
The approval-overlay fix correctly moved approvalOverlayActive to the disabled prop, but activePanel !== null remains in the voiceTarget condition. Modal dialogs (Shortcuts, Help, Theme) don't set activePanel, so they're also fine. But the inline panels listed in activePanel's type union ('settings' | 'status' | 'sessions' | 'extensions' | 'mcp' | 'skills' | 'plugins' | 'agents') still trigger the clearing. No test exercises opening an inline panel during active capture.
Note: the split-view ChatPane uses only hidden ? undefined : voiceTarget, which correctly keeps the target when the pane is merely covered (not hidden). The main composer's inline panels are analogous — the composer stays mounted and visible behind the panel.
中文说明
内联面板（设置、工具、记忆、会话等）仍然会清空 voiceTarget 并中止正在进行的录音。失败场景：用户点击麦克风 → 打开设置面板 → activePanel 变为非空 → voiceTarget 变为 undefined → useVoiceCapture 的 targetIdentity layout effect 调用 abort() → 录音被丢弃且无提示。
审批浮层的修复已正确将 approvalOverlayActive 移至 disabled prop，但 activePanel !== null 仍在 voiceTarget 条件中。模态对话框（快捷键、帮助、主题）不设置 activePanel，所以没问题。但 activePanel 类型联合中的内联面板仍会触发清空。分屏 ChatPane 仅使用 hidden ? undefined : voiceTarget，主输入框的内联面板场景类似——输入框仍然挂载且在面板后部可见。
| voiceTarget={ | |
| activePanel !== null || mainView !== 'chat' | |
| ? undefined | |
| : mainVoiceTarget | |
| } | |
| voiceTarget={ | |
| mainView !== 'chat' | |
| ? undefined | |
| : mainVoiceTarget | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks — won't take this one. activePanel replaces the chat surface: chatViewWrap receives chatViewHidden (display: none) plus aria-hidden, so the composer is mounted only to preserve state and is not visible or interactive behind the panel. Keeping capture alive would leave the microphone and transcript delivery running against an invisible composer. Clearing the target is the intentional fail-safe; normal dialogs and approvals preserve the target because they do not replace the owning chat surface. This also matches ChatPane, which clears the target when a pane is actually hidden.
| '/daemon/status': daemonProxy, | ||
| '/session': daemonProxy, | ||
| '/permission': daemonProxy, | ||
| [QUALIFIED_VOICE_STREAM_PROXY]: { ...daemonProxy, ws: true }, |
There was a problem hiding this comment.
[Suggestion] Vite dev proxy only covers the workspace-qualified WebSocket stream path — Concrete cost: the proxy entry [QUALIFIED_VOICE_STREAM_PROXY] matches only /workspaces/<id>/voice/stream. The workspace-qualified REST routes (GET /workspaces/<id>/voice, GET /workspaces/<id>/providers, GET/POST /workspaces/<id>/settings) have no proxy entry. When getDaemonBaseUrl() returns '' in dev (no ?daemon= param), these REST requests hit the vite dev server, get the SPA fallback index.html, and fail JSON parsing — silently disabling workspace-qualified Voice status, model picker, and settings in development. Primary workspaces are unaffected (they use the legacy /workspace proxy). The E2E smoke test doesn't catch this because the mock daemon intercepts at the network level.
中文说明
Vite 开发代理仅覆盖了工作区限定的 WebSocket 流路径。工作区限定的 REST 路由（voice、providers、settings）没有代理条目。当 getDaemonBaseUrl() 在开发环境中返回空字符串时，这些 REST 请求会命中 vite 开发服务器并获得 SPA 回退的 index.html，导致 JSON 解析失败——静默禁用工作区限定的 Voice 状态、模型选择器和设置。主工作区不受影响（使用旧的 /workspace 代理）。
| [QUALIFIED_VOICE_STREAM_PROXY]: { ...daemonProxy, ws: true }, | |
| '^/workspaces/[^/]+/(voice|providers|settings)/?$': daemonProxy, | |
| [QUALIFIED_VOICE_STREAM_PROXY]: { ...daemonProxy, ws: true }, |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks — won't take this one. Vite matches a non-regex proxy context with url.startsWith(context), so /workspaces/:id/voice, /providers, and /settings already match the existing /workspace entry. The dedicated qualified Voice rule is needed only for the WebSocket upgrade because the broad REST proxy does not enable ws: true; adding another REST rule would duplicate existing coverage.
|
Review follow-up for
The PR is already beyond roughly five review rounds, so only demonstrated correctness, security, data-loss, or regression fixes are being added. No code change was warranted by this batch. |
Route voice status, settings, model discovery, and streaming through the workspace that owns each main or split-view composer while preserving legacy primary behavior. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Prevent the Voice-only legacy workspace fallback from activating pre-session git polling, and cover both behaviors together. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
4d3bfa5 to
6286dc5
Compare
|
Rebased this PR onto
Validation:
Newly resolved review threads: 0. Existing pushed-back threads intentionally remain unresolved. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Round 2 — local verification of
|
| # | Round-1 finding | Status at 6286dc5085 |
Evidence |
|---|---|---|---|
| 1 | Untrusted-secondary fail-closed assertion is vacuous (one it() named two gates, pinned neither) |
Fixed — each gate now dies on its own test | A/B below |
| 2 | loadVoiceProviders cwd-echo guard unpinned |
Still unpinned (author deferred) — production guard present and correct | mutant F2 survives the full suite |
| 3 | Main composer panel/view Voice gate unpinned | Still unpinned (deferred) — gate present at App.tsx:8487 |
mutant F3 survives |
| 4 | VoiceButton settingsVersion owner guard unpinned |
Still unpinned (deferred) — guard present | mutant F4 survives |
| 5 | Picker owner-mismatch backstop unpinned | Still unpinned (deferred), low severity | mutant F5 survives |
Findings 2–5 are coverage gaps over correct code, not defects: I re-read each guard at this head and re-ran each mutation — the guards are all present and behave as round 1 established; only the tests that would pin them are missing. Nothing here blocks merge.
Rebase integrity — what actually changed since round 1
- Every production file this PR touches is byte-identical (sha256) at
c831feda1and6286dc5085, exceptApp.tsx. App.tsx's only delta is main's monitor-panel work: the diff between the two heads contains zero Voice lines, and the PR's own 136 added Voice lines in that file hash identically at both heads (43021b8c…).- The single authored change since round 1 is
voice-workspace-target.test.ts(+16/−6).
So all round-1 correctness evidence carries over verbatim, and every measured delta below is attributable to the test change alone.
1. Finding 1 is genuinely fixed — same production file, test file swapped
I ran the two gate mutations over the unmodified production file against the round-1 test file and against this head's test file. Old: both mutants survive (the assertion proved nothing). New: each mutant is killed, by the test whose name states that gate.
The failure detail is the useful part: with the trust gate removed, resolveVoiceWorkspaceTarget hands an untrusted workspace a live workspace-qualified target with a real streamPath. That is exactly the non-goal the test now pins.
2. Full-suite mutation matrix at this head (10 mutants × 2 325 tests)
- Controls
C1/C2(guards that round 1 found already covered) are still killed — the test-only commit traded no coverage away. - Each new test is killed by its own mutant and no other; no accidental cross-kill.
N1was newly probed this round and is an equivalent mutant, proven by running both variants:if (!cwd) return undefined;only changes behaviour for a capabilities payload whose workspace entry has nocwd, which the schema forbids. Not a gap — listing it so the matrix isn't read as five outstanding holes.
3. The pinned gate, exercised end-to-end in a browser
A reviewer-side spec driving the PR's own mockDaemon harness, flipping exactly one bit — the secondary workspace's trusted flag:
Trusted: mic renders, GET /workspaces/secondary/voice fires, /workspace/voice is never requested. Untrusted: same composer, no mic, and no /voice request of any kind — fail-closed in the running app, not just in the resolver unit test.
4. Gates re-run locally
Also checked: the PR trial-merges onto current main (6f8376ee1) with 0 conflicts, and the full web-shell suite passes on that merge result too. The ESLint pass was confirmed live with a planted defect (exit 1), so the clean run isn't a silently broken linter. FWIW the four files you reported failing locally for a missing localStorage passed here with no special flags (Node 22.23.1, plain vitest run).
Reproduce
git worktree add --detach /tmp/wt-7754 6286dc5085
cd /tmp/wt-7754 && npm ci # prepare also builds sdk/webui dist, which web-shell vitest needs
cd packages/web-shell
../../node_modules/.bin/vitest run --config vitest.config.ts # 141 files / 2325 tests
../../node_modules/.bin/tsc -p tsconfig.json --noEmit
../../node_modules/.bin/playwright test --config playwright.config.ts --project chromium --grep @smoke
# gate A/B: same production file, swap only the test file
git show c831feda1:packages/web-shell/client/voice/voice-workspace-target.test.ts > /tmp/old.test.ts
# then delete `if (!workspace.trusted) return undefined;` in voice-workspace-target.ts and run both variantsVerdict
The one item I asked to have fixed before merge is fixed, and fixed properly — the new tests are independently load-bearing, the previously covered gates stayed covered, and the rebase preserved the PR's Voice implementation byte-for-byte. Suite, typecheck, lint, format, build, and the E2E oracle are all green locally at this head, and it merges cleanly onto current main.
Ready to merge from my side. Findings 2–5 remain worthwhile as a small follow-up (four tests, all against harnesses that already exist in these files) — I'm happy for them to land after this PR rather than extending a sixth review round.
中文说明
第 2 轮 —— 对 6286dc5085 的本地验证
这是对我 第 1 轮评审（当时 head 为 c831feda1）的复验。此后 head 变为 6286dc5085：一次 rebase 到 8785216be5，外加 test(web-shell): Pin Voice trust and ambiguity gates。以下所有验证都在新 head 上、于全新的 detached worktree 中独立 npm ci 后重跑（未借用其它检出的 node_modules）；每一条沿用的问题都重新实测，而非照搬结论。
第 1 轮问题 → 当前 head 状态
| # | 第 1 轮问题 | 6286dc5085 上的状态 |
证据 |
|---|---|---|---|
| 1 | 不可信次工作区的失败关闭断言是空转的（一个 it() 声称覆盖两道门，实际一道都没钉住） |
已修复 —— 两道门各自被独立测试钉住 | 下方 A/B |
| 2 | loadVoiceProviders 的 cwd 回显校验未被钉住 |
仍未钉住（作者选择推迟）—— 生产代码中的校验存在且正确 | 变异体 F2 在全量套件下存活 |
| 3 | 主输入框面板/视图 Voice 门未被钉住 | 仍未钉住（推迟）—— 该门在 App.tsx:8487 存在 |
变异体 F3 存活 |
| 4 | VoiceButton 的 settingsVersion 归属守卫未被钉住 |
仍未钉住（推迟）—— 守卫存在 | 变异体 F4 存活 |
| 5 | 选择器归属不匹配兜底未被钉住 | 仍未钉住（推迟），严重度低 | 变异体 F5 存活 |
第 2–5 条都是针对正确代码的测试覆盖缺口，不是缺陷：我在当前 head 逐条重读了守卫并重跑了变异，生产行为均与设计文档一致。这些都不阻塞合并。
Rebase 完整性 —— 相比第 1 轮究竟变了什么
- 本 PR 涉及的所有生产文件在
c831feda1与6286dc5085上 sha256 完全一致，仅App.tsx除外。 App.tsx的差异全部来自 main 的 monitor 面板改动：两个 head 之间的 diff 中 没有任何一行 Voice 代码，且本 PR 在该文件中新增的 136 行 Voice 代码在两个 head 上哈希一致（43021b8c…）。- 第 1 轮以来唯一的作者改动是
voice-workspace-target.test.ts（+16/−6）。
因此第 1 轮的正确性证据可原样沿用，下面测得的所有差异都只能归因于这次测试改动。
1. 第 1 条确已修复 —— 生产文件不变，只换测试文件
我在未改动的生产文件上施加两个门的变异，分别用第 1 轮的测试文件与当前 head 的测试文件运行。旧版：两个变异体都存活（断言什么都没证明）。新版：各自被“名字正好说明该门”的测试杀死。
最有价值的是失败详情：删掉信任门后，resolveVoiceWorkspaceTarget 会把一个真实可用的 workspace-qualified 目标（含真实 streamPath）交给不可信工作区——这正是新测试所钉住的非目标。
2. 当前 head 的全量变异矩阵（10 个变异体 × 2325 个测试）
- 对照组
C1/C2（第 1 轮已被覆盖的守卫）依然被杀死——这次纯测试提交没有以牺牲其它覆盖为代价。 - 每个新测试只被自己对应的变异体杀死，没有误杀、没有交叉覆盖。
N1是本轮新增探测，属于等价变异体，并已实跑证明：if (!cwd) return undefined;只有在 capabilities 载荷中某个工作区条目缺少cwd（schema 不允许）时才会产生行为差异。它不是缺口——列出来是为了避免把矩阵读成“还有五个洞”。
3. 被钉住的这道门在真实浏览器中的端到端表现
一份评审侧 spec，驱动本 PR 自带的 mockDaemon 脚手架，只翻转一个比特——次工作区的 trusted 标志：
可信：麦克风渲染，发出 GET /workspaces/secondary/voice，从未请求 /workspace/voice。不可信：同一个输入框，无麦克风，且没有任何 /voice 请求——在运行中的应用里就是失败关闭，而不只是解析器单测里的行为。
4. 本地重跑的各道关卡
另外确认：本 PR 与当前 main（6f8376ee1）试合并 0 冲突，且在合并结果上 web-shell 全量套件同样通过。ESLint 的“干净”结论用植入缺陷做了正向对照（退出码 1），确保不是 linter 静默失效。另外，你提到在本地默认 Node 环境下因缺少 localStorage 而失败的那 4 个文件，在这里无需任何额外参数即可通过（Node 22.23.1，直接 vitest run）。
结论
我唯一要求合并前修复的问题已经修复，而且修得扎实：新测试各自独立生效，原有已覆盖的门没有退化，rebase 逐字节保留了本 PR 的 Voice 实现。当前 head 上套件、类型检查、lint、格式化、构建与 E2E 判据在本地全绿，且能干净合并到当前 main。
从我这边看可以合并。 第 2–5 条仍值得作为一个小的后续（四个测试，且都能复用这些文件里已有的脚手架）——我同意它们在本 PR 之后再补，而不是再开第六轮评审。
|
Thanks for the thorough fresh-head verification. I independently rechecked the four remaining guards at |
Reconcile the workspace-scoped voice refactor (#7754) with the hold-mode PCM buffering and deferred-stop work: port the buffer/overflow/finalize logic onto the snapshot-based capture lifecycle, keep the required target prop wired through the button tests, and assert the icon/pill button reuses one DOM node so pointer capture survives the recording transition.
|
Released in v0.21.1. |
3 participants
What this PR does
Routes the existing Web Shell Voice experience through the workspace that owns each composer. Main, locked, and split-view composers now resolve a fail-closed Voice target; trusted secondary workspaces use workspace-qualified status, provider, settings, model, and streaming routes, while primary and older single-workspace daemons retain the legacy routes. Each capture generation snapshots its owner and cleans up microphone, audio, timers, sockets, and stale callbacks when ownership changes. The development proxy now forwards only qualified Voice stream upgrades.
Why it's needed
Web Shell sessions can run in secondary workspaces, but Voice previously continued to use the primary workspace daemon. This could expose primary-workspace Voice configuration in a secondary composer and insert a transcript produced by the wrong runtime. Scoping Voice to the composer owner preserves workspace isolation and makes split-view behavior predictable.
Reviewer Test Plan
How to verify
Expected result: every Voice request and callback stays bound to the composer workspace that initiated it, with legacy compatibility limited to the primary single-workspace path.
Evidence (Before & After)
Before: Voice controls in a secondary-workspace composer queried and streamed through the primary runtime, so availability, settings, models, and transcripts could belong to the wrong workspace.
After: each composer resolves one exact Voice owner; trusted secondary workspaces use qualified routes, primary keeps legacy compatibility, and unresolved or unsafe secondary states disable Voice without primary fallback.
Tested on
Environment (optional)
macOS with the repository's Node.js 22+ toolchain. Verified with 359 focused Vitest tests, the Web Shell production and library builds, Web Shell typecheck, repository lint, and whitespace validation. Physical microphone browser E2E was not run because Playwright Chromium is not installed in the local environment.
Risk & Scope
Linked Issues
Closes #6972
中文说明
本 PR 做了什么
将现有 Web Shell Voice 体验路由到拥有各个输入框的工作区。主工作区、锁定工作区和分屏输入框现在都会以失败关闭方式解析 Voice 目标；可信的次工作区使用带工作区限定的状态、提供商、设置、模型和流式路由，而主工作区及旧版单工作区 daemon 继续使用兼容的旧路由。每一代录音都会快照其所有者，并在所有权变化时清理麦克风、音频、定时器、套接字和过期回调。开发代理现在只转发带工作区限定的 Voice 流式升级请求。
为什么需要
Web Shell 会话可以运行在次工作区中，但 Voice 之前仍然使用主工作区 daemon。这可能在次工作区输入框中暴露主工作区的 Voice 配置，并插入由错误运行时生成的转录。将 Voice 限定到输入框所有者可以保持工作区隔离，并使分屏行为可预测。
Reviewer 测试计划
如何验证
预期结果：每个 Voice 请求和回调都始终绑定到发起操作的输入框工作区，旧版兼容性仅限于主工作区的单工作区路径。
证据（之前与之后）
之前：次工作区输入框中的 Voice 控件通过主运行时查询和流式通信，因此可用性、设置、模型和转录可能属于错误的工作区。
之后：每个输入框解析到唯一且精确的 Voice 所有者；可信次工作区使用限定路由，主工作区保留旧版兼容性，无法解析或不安全的次工作区状态会禁用 Voice，且不会回退到主工作区。
测试平台
环境（可选）
macOS，使用仓库要求的 Node.js 22+ 工具链。已通过 359 个定向 Vitest 测试、Web Shell 生产构建和库构建、Web Shell 类型检查、仓库 lint 以及空白检查。由于本地环境未安装 Playwright Chromium，未执行真实麦克风浏览器 E2E。
风险与范围
关联 Issue
Closes #6972