fix(web-shell): scope mock sessions by workspace - #10273
Conversation
Keep the visual mock daemon aligned with workspace-scoped session routes so multi-workspace sidebar screenshots do not duplicate session rows across sections. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @AaronZ345! The change itself looks plausible, but the PR body doesn't follow the repository's PR template — none of the required sections are present: What this PR does, Why it's needed, Reviewer Test Plan (with How to verify / Evidence (Before & After) / Tested on), Risk & Scope, and Linked Issues.
Could you rework the body to follow the template? The commands you already list under "Evidence" are a good start for the Reviewer Test Plan — please add what a reviewer should expect (e.g. the workspace sidebar visual scenario rendering once per workspace instead of failing Playwright strict mode), which OS you tested on, and the risk/scope section.
中文说明
感谢贡献，@AaronZ345！改动本身看起来合理，但 PR 描述没有遵循仓库的 PR 模板——缺少全部必填章节：What this PR does、Why it's needed、Reviewer Test Plan（含 How to verify / Evidence (Before & After) / Tested on）、Risk & Scope、Linked Issues。
请按模板重写 PR 描述。你在 "Evidence" 里列出的命令可以作为 Reviewer Test Plan 的起点——请补充审阅者应看到的结果（例如 workspace sidebar 可视化场景每个工作区只渲染一次、不再触发 Playwright strict mode 失败）、测试所用的操作系统，以及风险/范围章节。
— Qwen Code · qwen3.8-max
🖼️ 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 |
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.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| : sourceSessions; | ||
| } | ||
|
|
||
| function workspaceCwdFromSessionsPath(path: string): string | undefined { |
There was a problem hiding this comment.
[Suggestion] Workspace scoping fails open on guard/extractor drift. workspaceCwdFromSessionsPath returns undefined on a non-matching path, and both consumers — the live-state handler's !workspaceCwd || fallback and filterScenarioSessions' workspaceCwd ? … : scenario.sessions branch — then silently serve the unscoped cross-workspace catalog, the exact behavior this PR exists to remove. Both fallbacks are dead today (every route guard regex is a subset of the helper's patterns), but this file already carries three parallel copies of the route shapes (isDaemonPath, isDaemonRoute, handleDaemonRoute), so guard/extractor drift is its known failure mode: if a new sessions path shape is added to a route guard without extending this helper, the handler silently returns sessions from every workspace again, and the symptom surfaces as Playwright strict-mode duplicate-name failures in visual specs — a recurrence of the bug this PR fixes — many files away from the cause. Making the helper total turns that silent re-leak into a loud failure at the cause:
function workspaceCwdFromSessionsPath(path: string): string {
const workspaceMatch = path.match(
/^\/workspaces\/([^/]+)\/sessions(?:\/live-state)?\/?$/,
);
if (workspaceMatch) return decodeURIComponent(workspaceMatch[1]);
const legacyMatch = path.match(/^\/workspace\/(.+)\/sessions\/?$/);
if (legacyMatch) return decodeURIComponent(legacyMatch[1]);
throw new Error(`Unrecognized sessions path: ${path}`);
}…plus deleting the !workspaceCwd || fallback in the live-state handler and the unscoped branch in filterScenarioSessions. Every call site sits behind a guard that guarantees a cwd segment, so the throw fires precisely when guard and extractor disagree.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| .filter( | ||
| (session) => !workspaceCwd || session.workspaceCwd === workspaceCwd, | ||
| ) |
There was a problem hiding this comment.
[Suggestion] The newly added workspace scoping on the live-state route has no test that can tell scoped from unscoped or empty responses. The only spec enabling workspace_session_live_state (web-shell.session-live-state.spec.ts) is single-workspace, where this filter is a no-op, and it asserts only request counters plus a tab click — deleting or inverting the filter leaves every assertion green. The multi-workspace workspace sidebar visual scenario does not enable the live-state capability, so its new toHaveCount(1) assertions pin only the sessions-listing route, not this one. A future regression in workspaceCwdFromSessionsPath on this path would therefore ship green: the mock silently diverges from the real daemon's per-workspace live-state contract, and recordLiveSessions (session-catalog-store.ts) stores the response per workspace without re-filtering, so live-state rows get attributed to the wrong workspace with no E2E failing. Consider adding multi-workspace live-state coverage — either extend web-shell.session-live-state.spec.ts to two workspaces with one live session (clientCount: 1) each and assert scoping on content, or enable workspace_session_live_state in the workspace sidebar visual scenario so its existing toHaveCount(1) assertions pin this path too. If you add that coverage, please confirm it bites: removing or inverting this .filter(...) must make the new assertion fail.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const scenario = createWebShellDaemonScenario({ | ||
| workspaceCwd: primaryCwd, | ||
| displayName: primarySessionName, | ||
| sessions, |
There was a problem hiding this comment.
[Suggestion] The scenario overrides workspaceCwd, displayName and sessions but not sessionId, so the loaded session (builder default web-shell-e2e-session) is present in no seeded workspace listing — the row the test waits for (workspace-primary-session) merely shares its display name. App.tsx's loaded-session reconciliation therefore misses on page.sessions.find(...) and silently takes the summary.displayName fallback, so the capture exercises the fallback path instead of the normal listing-hit path and depicts a state a real daemon cannot produce (a client connected to a session no workspace lists). The sidebar also marks the current row by session identity (isCurrentSession compares the connection's sessionId), so no row in this capture is ever flagged as current, and any regression in the current-session treatment (highlight, aria-current, action gating) stays invisible in the only multi-workspace capture. Aligning the loaded session with the seeded primary session fixes both:
| const scenario = createWebShellDaemonScenario({ | |
| workspaceCwd: primaryCwd, | |
| displayName: primarySessionName, | |
| sessions, | |
| const scenario = createWebShellDaemonScenario({ | |
| workspaceCwd: primaryCwd, | |
| displayName: primarySessionName, | |
| sessions, | |
| sessionId: 'workspace-primary-session', |
goto/SSE keying all derive from scenario.sessionId, so this stays self-consistent — and the adjacent "Wait for the loaded session's row" comment then matches reality too.
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /triage |
Require mock session routes to resolve an explicit workspace and cover live-state scoping so visual fixtures cannot leak sessions across workspaces. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const legacyMatch = path.match(/^\/workspace\/(.+)\/sessions\/?$/); | ||
| if (legacyMatch) return decodeURIComponent(legacyMatch[1]); |
There was a problem hiding this comment.
[Suggestion] R1-1: (fix-induced) The round-2 fix closed the round-1 input — the extractor now throws instead of returning undefined, and both consumers filter unconditionally — but it opened a new mechanism at the same site. The throw couples the two route guards to the two patterns inside workspaceCwdFromSessionsPath: four independent regexes, ~370 lines apart, that must silently stay in agreement, and the route callback in installMockDaemon has no try/catch around handleDaemonRoute. The hardcoded legacy capture also diverges from the real daemon's route semantics: the legacy guard admits .+ (spanning /), while the real daemon's Express :id param matches a single segment.
If a future edit widens or adds a sessions route guard without touching this helper, workspaceCwdFromSessionsPath throws Unrecognized sessions path inside the Playwright route callback; nothing catches it, so the catalog/live-state fetch never resolves and unrelated e2e tests fail with a hang or an error pointing at the mock rather than the route. And a request like /workspace/a/b/sessions gets a 200 with sessions: [] from the mock where the real daemon would 404, so an empty-state assertion can pass against behavior the real daemon never serves. Verified by running the mock's literal regexes and the repo's own express 5.2.1 side by side:
MOCK path /workspace/a/b/sessions | legacy capture: "a/b" (→ 200, sessions filtered by cwd "a/b" → [])
EXPRESS 404 /workspace/a/b/sessions -> {"matched":false}
EXPRESS 200 /workspace/%2Ftmp%2Fa/sessions -> {"matched":"GET /workspace/:id/sessions","id":"/tmp/a"}
Match-and-capture in one place: use the capture groups of the guard regexes themselves (hoist the match into the handler and drop the separate helper), and align the legacy pattern to [^/]+ to mirror Express :id single-segment semantics. That leaves exactly two patterns, both living where the route is dispatched, and the guard itself guarantees the capture succeeded.
中文说明
[Suggestion] R1-1：（由上轮修复引入）第 2 轮的修复关闭了第 1 轮报告的输入——提取函数现在抛出异常而不是返回 undefined，两个消费方也都无条件过滤——但它在同一位置引入了新的机制。这个 throw 把两个路由守卫和 workspaceCwdFromSessionsPath 内部的两个正则耦合在了一起：四个独立的正则相距约 370 行，必须悄悄保持一致，而 installMockDaemon 的路由回调在 handleDaemonRoute 外没有 try/catch。硬编码的 legacy 捕获还与真实 daemon 的路由语义存在偏差：legacy 守卫接受 .+（可跨 /），而真实 daemon 的 Express :id 参数只匹配单段。
如果未来某个改动拓宽或新增了 sessions 路由守卫却没有同步修改这个辅助函数，workspaceCwdFromSessionsPath 会在 Playwright 路由回调里抛出 Unrecognized sessions path；由于没有捕获，catalog/live-state 请求永远不会 resolve，不相关的 e2e 测试会以挂起失败，或报出指向 mock 而非路由的错误。另外，像 /workspace/a/b/sessions 这样的请求会从 mock 拿到 200 和空 sessions: []，而真实 daemon 会返回 404，于是空状态断言可能在真实 daemon 根本不会提供的行为上通过。上面的 probe 输出是用 mock 的原始正则和仓库自带的 express 5.2.1 实际运行对比得出的。
建议在一处完成匹配和捕获：使用守卫正则自身的捕获组（把匹配提升到 handler 里，删掉独立的辅助函数），并把 legacy 模式对齐为 [^/]+，与 Express :id 的单段语义一致。这样只留下两个模式，且都在路由分发处，守卫本身即可保证捕获成功。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Parse the daemon URL port before asserting that EADDRINUSE retry skipped the occupied port, avoiding false failures on ports like 41705. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
🩺 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 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 mock workspace extractor throw coupling and legacy route divergence — still stands at the reviewed commit (mockDaemon.ts unchanged since round 2); already reported, open thread (comment 3872634895)
中文说明
已审查。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Keep mock session route capture in the dispatch path so workspace scoping cannot drift from the route guard. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Keep the mock daemon route guards and handlers on the same workspace-session parser so review-thread fixes cannot drift between matching and extraction. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — the Test (macos-latest/windows-latest, Node 22.x) unit jobs never ran at this commit (skipped in CI), and the local unit suite could not run: a pre-existing tsc error in packages/web-shell/client/App.tsx (untouched by this PR) stopped the harness before its test phase.
Not explored to full depth (tool budget reached): "agent 3b": none — no checks were cut short..
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — the Test (macos-latest/windows-latest, Node 22.x) unit jobs never ran at this commit (skipped in CI), and the local unit suite could not run: a pre-existing tsc error in packages/web-shell/client/App.tsx (untouched by this PR) stopped the harness before its test phase。
未探索到全部深度（达到工具调用预算）："agent 3b"：none — no checks were cut short.。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const workspaceSessions = scenario.sessions.filter( | ||
| (session) => session.workspaceCwd === workspaceCwd, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-1: The workspace scoping added here for the full-catalog sessions routes has no direct test, and its interaction with the group === 'pinned' branch is exercised by no multi-workspace scenario. Live-state scoping has a dedicated direct-fetch test (scopes live-state sessions to the requested workspace), but no spec fetches /workspaces/:cwd/sessions or .../sessions?group=pinned or asserts their response content: the only isPinned fixture (web-shell.channels.spec.ts) is single-workspace, and the two-workspace workspace sidebar visual test seeds no pinned sessions, so the pinned bucket is never fetched across workspaces. If a future edit of filterScenarioSessions bypassed the workspace filter on the pinned branch (filtering scenario.sessions instead of workspaceSessions when group === 'pinned'), nothing would go red — the mock would silently return other workspaces' pinned sessions for a workspace-scoped request, re-introducing exactly the cross-workspace duplication this PR exists to eliminate.
Consider adding a sibling to scopes live-state sessions to the requested workspace in web-shell.session-live-state.spec.ts: a two-workspace scenario where the non-requested workspace also has an isPinned session, then direct page.evaluate fetches of /workspaces/${encodeURIComponent(cwd)}/sessions and .../sessions?group=pinned asserting the exact returned sessionId lists contain only the requested workspace's sessions. The new test must go red if the workspace filter is skipped on the group === 'pinned' or sourceType branches — prove it with the mutant that filters unscoped scenario.sessions on the pinned branch.
中文说明
[建议] R4-1：这里为全量 catalog sessions 路由新增的 workspace 过滤没有直接测试，它与 group === 'pinned' 分支的组合也没有任何多 workspace 场景覆盖。live-state 过滤有专门的直接请求测试（scopes live-state sessions to the requested workspace），但没有任何 spec 直接请求 /workspaces/:cwd/sessions 或 .../sessions?group=pinned 并断言响应内容：唯一的 isPinned fixture（web-shell.channels.spec.ts）是单 workspace 的，双 workspace 的 workspace sidebar 可视化测试也没有准备 pinned session，因此 pinned bucket 从未在跨 workspace 场景下被请求。如果未来对 filterScenarioSessions 的修改绕过了 pinned 分支上的 workspace 过滤（比如在 group === 'pinned' 时过滤 scenario.sessions 而不是 workspaceSessions），不会有任何测试变红——mock 会悄悄为按 workspace 的请求返回其他 workspace 的 pinned sessions，重新引入这个 PR 要消除的跨 workspace 重复。
建议在 web-shell.session-live-state.spec.ts 中增加一个 scopes live-state sessions to the requested workspace 的同族测试：双 workspace 场景，其中未被请求的 workspace 还有一个 isPinned session，然后通过 page.evaluate 直接请求 /workspaces/${encodeURIComponent(cwd)}/sessions 和 .../sessions?group=pinned，断言返回的 sessionId 列表只包含被请求 workspace 的 session。如果在 group === 'pinned' 或 sourceType 分支上跳过 workspace 过滤，新测试必须变红——可以用「在 pinned 分支上过滤未过滤的 scenario.sessions」这个变异体来证明。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| ); | ||
| if (liveStateMatch) { | ||
| return { | ||
| workspaceCwd: decodeURIComponent(liveStateMatch[1]), |
There was a problem hiding this comment.
[Suggestion] R1-1: (fix-induced) The round-4 consolidation closed the reported input — the throwing workspaceCwdFromSessionsPath helper is gone, matchWorkspaceSessionsRoute returns undefined on a non-matching path, and the legacy pattern now mirrors Express :id single-segment semantics — but it opened a new mechanism at the same site. The matcher now decodes the captured segment eagerly and is invoked from isDaemonPath, a predicate run for EVERY same-origin request intercepted by installMockDaemon, where previously only non-throwing regexes ran and there is no try/catch around the call. decodeURIComponent throws URIError on a malformed percent sequence (a bare % survives WHATWG URL parsing), so a request such as /workspaces/%/sessions/live-state throws inside the Playwright route callback before any route.fulfill/abort/fallback — the request never resolves and the awaiting code gets no response, with the only diagnostic pointing into mock internals. Probe against this commit: fetching /workspaces/%/sessions/live-state fails with URIError: URI malformed at mockDaemon.ts:667 via isDaemonPath (mockDaemon.ts:709); with a guarded decode the same probe falls through cleanly via route.fallback() and the well-formed control (/workspaces/%2Ftmp%2Fqwen-web-shell-e2e/sessions) still returns 200. No current producer emits malformed sequences (the SDK and specs all encode via encodeURIComponent), so today this is a latent landmine in test infrastructure rather than a live failure.
Guard the decode so the matcher stays total — wrap decodeURIComponent in try/catch at both decode sites (here and the sessions match below) returning undefined on URIError, so malformed paths fall through to route.fallback() like any other non-daemon path. A guard spec that fetches ${baseURL}/workspaces/%/sessions/live-state through the installed mock and asserts the request settles should go red if the unguarded decode is reintroduced.
中文说明
[建议] R1-1：（由上轮修复引入）第 4 轮的整合关闭了此前报告的输入——会抛异常的 workspaceCwdFromSessionsPath 辅助函数已移除，matchWorkspaceSessionsRoute 对不匹配的路径返回 undefined，legacy 模式也已对齐 Express :id 的单段语义——但它在同一位置引入了新的机制。matcher 现在会提前解码捕获的片段，并被 isDaemonPath 调用——这个谓词会在 installMockDaemon 拦截的每一个同源请求上运行，而这里之前只有不抛异常的正则，调用外层也没有 try/catch。decodeURIComponent 遇到畸形的百分号序列会抛 URIError（单独的 % 能通过 WHATWG URL 解析），因此像 /workspaces/%/sessions/live-state 这样的请求会在 Playwright 路由回调里、在任何 route.fulfill/abort/fallback 之前抛出——请求永远不会 resolve，等待方拿不到任何响应，唯一的报错指向 mock 内部。已在本提交上用 probe 验证：请求 /workspaces/%/sessions/live-state 得到 URIError: URI malformed（mockDaemon.ts:667，经 isDaemonPath (mockDaemon.ts:709)）；对解码加保护后，同样的请求干净地落到 route.fallback()，正常格式的请求（/workspaces/%2Ftmp%2Fqwen-web-shell-e2e/sessions）仍返回 200。当前没有任何调用方会发出畸形序列（SDK 和 spec 都用 encodeURIComponent 编码），所以目前这是测试设施里一个潜在的雷，而不是现实中的失败。
建议给解码加保护，让 matcher 保持全函数定义——在两处解码点（这里和下面的 sessions 匹配）用 try/catch 包住 decodeURIComponent，遇到 URIError 返回 undefined，让畸形路径像其他非 daemon 路径一样落到 route.fallback()。可以增加一个守卫 spec：通过已安装的 mock 请求 ${baseURL}/workspaces/%/sessions/live-state 并断言请求有确定结果；如果重新引入无保护的解码，该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — the Test (macos-latest/windows-latest, Node 22.x) unit jobs never ran at this commit (skipped in CI), and the local unit suite could not run: a pre-existing tsc error in packages/web-shell/client/App.tsx (untouched by this PR, byte-identical to the merge base) stopped the harness before its test phase; the changed e2e files are excluded from both the unit suite and the failing compilation set regardless.
Not explored to full depth (tool budget reached): "agent 3b": none.**.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/e2e/utils/mockDaemon.ts:1051 — [review] workspace-scoping predicate duplicated between the live-state handler and filterScenarioSessionspackages/web-shell/client/e2e/utils/mockDaemon.ts:639 — [probe] mock resolves the :workspace selector as cwd only; production resolves workspace ids first, so id-form selectors return silently-empty session lists
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — the Test (macos-latest/windows-latest, Node 22.x) unit jobs never ran at this commit (skipped in CI), and the local unit suite could not run: a pre-existing tsc error in packages/web-shell/client/App.tsx (untouched by this PR, byte-identical to the merge base) stopped the harness before its test phase; the changed e2e files are excluded from both the unit suite and the failing compilation set regardless。
未探索到全部深度（达到工具调用预算）："agent 3b"：none.**。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const secondaryPinned = await page.evaluate( | ||
| async ({ baseURL, cwd }) => { | ||
| const response = await fetch( | ||
| `${baseURL}/workspaces/${encodeURIComponent(cwd)}/sessions?group=pinned`, |
There was a problem hiding this comment.
[Suggestion] This test fetches the pinned bucket as ?group=pinned without view=organized, but the production daemon rejects that shape with 400 (invalid_session_group_filter) — listWorkspaceSessionsHandler requires view=organized whenever group is present (packages/cli/src/serve/routes/session.ts), and the real client always sends the two together (WebShellSidebar.tsx sends view: 'organized', group: 'pinned' in both the primary and the secondary-workspace pinned queries). The test therefore validates a request production refuses and never exercises the wire shape the app actually sends, misdocumenting the daemon's query contract inside a mock whose own comment says it must "Mirror production query modes"; the day the mock grows production's view validation, this test breaks. The mock ignores view, so adding the parameter keeps every assertion green while the test adopts the production shape.
| `${baseURL}/workspaces/${encodeURIComponent(cwd)}/sessions?group=pinned`, | |
| `${baseURL}/workspaces/${encodeURIComponent(cwd)}/sessions?view=organized&group=pinned`, |
If the fix lands, scopes full and pinned sessions to the requested workspace must still pass, and if the mock ever rejects group without view=organized, only the fixed URL keeps passing for the right reason — prove it by temporarily reverting the URL once that validation exists and confirming the test goes red.
中文说明
[建议] 该测试请求 pinned bucket 时只带了 ?group=pinned 而没有 view=organized，但生产环境的 daemon 会以 400（invalid_session_group_filter）拒绝这种请求形态——只要带 group 参数，listWorkspaceSessionsHandler 就要求 view=organized（packages/cli/src/serve/routes/session.ts）——而且真实客户端总是同时发送这两个参数（WebShellSidebar.tsx 在主工作区和次工作区的 pinned 查询中都发送 view: 'organized', group: 'pinned'）。因此这个测试验证的是一个生产环境会拒绝的请求，从未覆盖应用实际发出的请求形态，在一个注释里声明必须 "Mirror production query modes"（镜像生产查询模式）的 mock 中，这会误导 daemon 的查询契约；一旦 mock 增加生产环境的 view 校验，该测试就会失败。由于 mock 会忽略 view，加上该参数后所有断言依然成立，同时测试也采用了生产形态。
应用该修复后，scopes full and pinned sessions to the requested workspace 必须仍然通过；如果未来 mock 拒绝不带 view=organized 的 group，只有修复后的 URL 才会因正确的原因继续通过——届时可以临时还原 URL，验证测试会变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- workspace-scoping predicate duplicated between the live-state handler and filterScenarioSessions (mockDaemon.ts:1051) — already reported (round 5 deferred list, review 5045401216)
Not explored to full depth (tool budget reached): "agent 6c": none — the Playwright e2e specs were verified statically only (not executed), but no finding candidate depended on runtime behavior I couldn't establish from so….
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/e2e/web-shell.session-live-state.spec.ts:126 — [review] page.evaluate fetch-and-response.json() block pasted four times in the new spec
中文说明
已审查。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 6c"：none — the Playwright e2e specs were verified statically only (not executed), but no finding candidate depended on runtime behavior I couldn't establish from so…。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Cover full, source-filtered, and pinned catalogs across multiple workspaces to prevent cross-workspace leakage. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- U7-1 live-state fetch block pasted twice while fetchSessions already covers it (web-shell.session-live-state.spec.ts:126) — already reported (round 6 deferred list, review 5048462618)
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/e2e/web-shell.session-live-state.spec.ts:233 — [review] workspace scoping is pinned only at the mock layer, never through the apppackages/web-shell/client/e2e/web-shell.session-live-state.spec.ts:235 — [probe] legacy /workspace/:cwd/sessions arm has no scoping testpackages/web-shell/client/e2e/web-shell.session-live-state.spec.ts:71 — [probe] liveness filter is an identity — every fixture session is live
中文说明
已审查。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…ebar-strict-mode
Resolve packages/web-shell/client/e2e/visuals/screenshots.spec.ts: main
switched the workspace-sidebar assertion to an exact getByRole('button')
locator (the session name is also rendered outside the row after the
WebShell cutover). Keep that locator and apply this branch's
per-workspace uniqueness check to both the primary and the secondary
session.
qwen-code-ci-bot
left a comment
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:
- R8-2 live-state fetch block pasted twice while the fetchSessions helper pattern already exists in the same file (web-shell.session-live-state.spec.ts:117) — already reported (round 6 deferred list, review 5048462618)
Not reviewed: build-and-test — zero test suites ran in this review: npm ci + packages/core build consumed the per-call budget before the test phase; the packages/acp-bridge build timed out (infrastructure); the test-efficacy probe had no production source to target (test-only diff). CI at this commit: Test (ubuntu-latest, Node 22.x) failed; Test (macos-latest/windows-latest) and Integration Tests (CLI, No Sandbox) were skipped.
Not explored to full depth (tool budget reached): "agent 6a": none — I completed every check I started; nothing was cut short at the tool ceiling.; "agent 1a": runtime execution of the changed CLI test ( run-qwen-serve.test.ts , "retries the next port on EADDRINUSE") — the fresh-worktree vitest guard requires workspace…; "agent 1a": runtime execution of the new Playwright specs ( web-shell.session-live-state.spec.ts , visuals workspace-sidebar ) — requires the built web-shell dev server an….
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — zero test suites ran in this review: npm ci + packages/core build consumed the per-call budget before the test phase; the packages/acp-bridge build timed out (infrastructure); the test-efficacy probe had no production source to target (test-only diff). CI at this commit: Test (ubuntu-latest, Node 22.x) failed; Test (macos-latest/windows-latest) and Integration Tests (CLI, No Sandbox) were skipped。
未探索到全部深度（达到工具调用预算）："agent 6a"：none — I completed every check I started; nothing was cut short at the tool ceiling.；"agent 1a"：runtime execution of the changed CLI test ( run-qwen-serve.test.ts , "retries the next port on EADDRINUSE") — the fresh-worktree vitest guard requires workspace…；"agent 1a"：runtime execution of the new Playwright specs ( web-shell.session-live-state.spec.ts , visuals workspace-sidebar ) — requires the built web-shell dev server an…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| ).toHaveCount(1); | ||
| await expect( | ||
| sidebar.getByRole('button', { | ||
| name: secondarySessionName, |
There was a problem hiding this comment.
[Critical] R8-1: [fails-closed] [regression] This PR does not fix #10250 — at this HEAD the workspace sidebar visual scenario still fails deterministically in both themes, and the failure is now on the very toHaveCount(1) assertion this PR adds. The primary session row never renders: WebShellSidebar.tsx:5427 sets renderSessions={!ws.primary}, so primary-workspace session rows are sourced from the top-level catalog body (fed by the legacy /workspace/:cwd/sessions catalog query with sourceType=default), not from the per-workspace fetch the PR's mock scoping targets — under the scoped mock + fixture, that path no longer yields the primary row. Merge this PR as-is and the web-shell visuals workflow reproduces #10250's user-visible outcome: at exactly this commit, CI run 33279625656 (job 99172457102) fails workspace sidebar in dark and light on all 3 attempts each, toHaveCount(1) for getByRole('button', { name: 'Run auth migration', exact: true }) resolves to 0 elements for the full timeout, the job ends 2 failed, and the preview bot posts "No preview: one or more scenarios failed to render". The same run's base capture (main without this PR) passed 32/32, so the red state is caused by this diff rather than inherited from main.
Witness:
PR arm (run 33279625656, job 99172457102, head fa9786d):
Error: expect(locator).toHaveCount(expected) failed
Locator: getByRole('complementary').getByRole('button', { name: 'Run auth migration', exact: true })
Expected: 1 Received: 0
> 855 | ).toHaveCount(1);
2 failed … 35 passed (3.3m) (dark and light, retries identical)
BASE arm (merge-base 413b6d15, same job):
32 passed (1.8m) — includes workspace sidebar (dark) and (light)
Preview comment on this head (updated 2026-08-29T23:10:09Z):
⚠️ No preview: one or more scenarios failed to render on this head
Make the scenario pass against merged main before merging: the fixture/mock must satisfy the top-level catalog path the sidebar actually consumes (legacy /workspace/:cwd/sessions with sourceType=default). Keep the workspace scoping and the toHaveCount(1) assertions — do not fix by deleting the secondary workspace, re-allowing cross-workspace leakage, or weakening the counts.
The fix must not re-introduce a duplicated session row across workspace groups (per issue #10250), and cannot be premised on the primary section's per-workspace fetch: packages/web-shell/client/components/sidebar/WebShellSidebar.tsx:5427 (renderSessions={!ws.primary}) means primary rows no longer render from that path. The acceptance test is the workspace sidebar scenario itself (this file, dark and light): it is red at this HEAD, must go green with the fix, and should go red again if the fix is removed — please confirm by reverting the fix and watching it fail.
中文说明
[Critical] R8-1：[fails-closed] [regression] 这个 PR 并没有修复 #10250 —— 在当前 HEAD 上，workspace sidebar 可视化场景仍然在深色和浅色两个主题下确定性失败，而且失败的正是本 PR 自己新增的 toHaveCount(1) 断言。主工作区的 session 行根本没有渲染出来：WebShellSidebar.tsx:5427 是 renderSessions={!ws.primary}，主工作区的 session 行来自顶层 catalog body（由带 sourceType=default 的旧版 /workspace/:cwd/sessions catalog 查询提供），而不再是本 PR mock 过滤所针对的按工作区 fetch —— 在按工作区过滤的 mock + fixture 下，这条路径拿不到主工作区的 session 行。按现状合并这个 PR 并运行 web-shell visuals workflow，#10250 的用户可见结果会原样复现：在恰好这个 commit 上，CI 运行 33279625656（job 99172457102）中 workspace sidebar 深色和浅色各 3 次尝试全部失败，getByRole('button', { name: 'Run auth migration', exact: true }) 的 toHaveCount(1) 在整个超时期间只解析到 0 个元素，job 以 2 failed 结束，preview 机器人发布 "No preview: one or more scenarios failed to render"。同一次运行的 base 采集（不含本 PR 的 main）32/32 全部通过，因此红灯是这个 diff 造成的，而不是从 main 继承的。
证据输出（CI 日志逐字引用）见上方英文部分的 Witness 代码块。
请在合并前让该场景在已合并 main 的状态下通过：fixture/mock 必须满足 sidebar 实际消费的顶层 catalog 路径（带 sourceType=default 的旧版 /workspace/:cwd/sessions）。请保留按工作区过滤和 toHaveCount(1) 断言 —— 不要通过删除第二个工作区、重新允许跨工作区泄漏或放宽计数来"修复"。
修复不得重新引入跨工作区重复的 session 行（依据 issue #10250），也不能建立在主工作区 section 的按工作区 fetch 之上：packages/web-shell/client/components/sidebar/WebShellSidebar.tsx:5427（renderSessions={!ws.primary}）意味着主工作区的行不再从该路径渲染。验收测试就是 workspace sidebar 场景本身（本文件，深色和浅色）：它在当前 HEAD 上是红的，修复后必须变绿，且移除修复后应再次变红 —— 请通过撤销修复并观察其失败来确认。
— qwen3.8-max via Qwen Code /review (v0.22.3)
3 participants
What this PR does
This PR scopes the web-shell mock daemon session catalog by workspace when visual tests request
/workspaces/:cwd/sessionsand live-state data. The workspace-sidebar visual scenario now seeds one explicit session per workspace and asserts that each rendered session name appears only once.Why it's needed
Issue #10250 tracks a deferred review finding from #10230: the mock daemon returned the same sessions for every workspace, so the workspace sidebar visual scenario rendered duplicate
Run auth migrationentries. Playwright strict mode then matched two elements and failed before the preview screenshots could be produced. The fix keeps the visual fixture aligned with the multi-workspace behavior it is meant to test.Reviewer Test Plan
How to verify
Run the focused workspace-sidebar visual scenario and confirm it no longer fails strict mode from duplicate session names. Reviewers should see one session rendered for the default workspace and one for the secondary workspace, with no cross-workspace leakage in the mock session list or live-state responses.
Evidence (Before & After)
Before: the workspace sidebar mock data was shared across workspace requests, so both workspace sections could render the same loaded session name and
getByText('Run auth migration')matched duplicates.After:
/workspaces/:cwd/sessionsand live-state responses are filtered by the requested workspace cwd; the visual scenario pins separate sessions per workspace and asserts each expected name appears exactly once.Local verification:
Tested on
Environment (optional)
Node.js local workspace on macOS. The change is limited to the web-shell mock daemon and visual fixture; cross-platform coverage is expected from repository CI.
Risk & Scope
Linked Issues
Fixes #10250
中文说明
What this PR does
这个 PR 让 web-shell mock daemon 在处理
/workspaces/:cwd/sessions和 live-state 数据时按 workspace 过滤 session catalog。workspace-sidebar 可视化场景现在为每个 workspace 明确准备一个 session，并断言每个 session 名称只渲染一次。Why it's needed
#10250 记录了 #10230 的延迟审查问题：mock daemon 对所有 workspace 返回同一批 sessions，导致 workspace sidebar visual 场景里出现重复的
Run auth migration。Playwright strict mode 因为匹配到两个元素而失败，预览截图也无法生成。这个修复让 visual fixture 和它要覆盖的多 workspace 行为保持一致。Reviewer Test Plan
How to verify
运行聚焦的 workspace-sidebar visual 场景，确认它不再因为重复 session 名称触发 strict mode 失败。Reviewer 应能看到默认 workspace 和第二个 workspace 各自只渲染自己的 session，mock session list 和 live-state response 都不会跨 workspace 泄漏。
Evidence (Before & After)
Before：workspace sidebar mock 数据在所有 workspace 请求之间共享，因此两个 workspace section 可能渲染同一个 loaded session 名称，
getByText('Run auth migration')会匹配到重复元素。After：
/workspaces/:cwd/sessions和 live-state responses 会按请求里的 workspace cwd 过滤；visual 场景为每个 workspace 固定独立 session，并断言每个期望名称只出现一次。本地验证：
Tested on
Environment (optional)
macOS 本地 Node.js 工作区。改动只涉及 web-shell mock daemon 和 visual fixture；跨平台覆盖交给仓库 CI。
Risk & Scope
Linked Issues
Fixes #10250