test(web-shell): render the Session Workflow cockpit in the visuals preview - #11014
Conversation
…review
The Session Workflow surfaces — the plan DAG, the cockpit header and the
inspector — have never been rendered by the visuals suite. `grep -rn
"sessionWorkflow" client/e2e/` returned nothing before this commit, so every
change to them has shipped on a stylesheet reading plus a jsdom unit run,
neither of which computes layout or the cascade. Three defects on the current
follow-up branch were of exactly that class: a rule drawn as a pseudo-element
that needed `overflow: hidden` and so clipped the port dot positioned outside
the node, a status glyph removed without noticing it was the graph's only
non-colour status channel, and a `text-overflow: ellipsis` declared on a flex
box where it can never fire. None of the three is visible to jsdom.
Adds a scenario that renders the dependency canvas in both themes. The plan is
a diamond — two independent steps, one that waits on both, one that waits on
that — so the capture exercises what the graph exists to draw rather than a
single chain: more than one layer, a node with two upstreams, and a step that
unblocks two others. One step is completed and one running, each with a linked
Agent task carrying real runtime and token counts, so the node's status
treatments and the inspector's agent rows are both in frame.
Two things the fixture has to get right, both recorded in comments because
each fails by rendering something plausible rather than by erroring:
- The mock daemon must advertise `experimental.sessionWorkflow` as effective.
Without it `App` passes `planTodos={[]}` and the graph never mounts.
- Stable ids and `blockedBy` travel in `_meta.qwenTodo`, not as top-level
fields. A fixture that puts them at the top level renders four nodes, zero
edges and the flat layout — which looks like a working graph until you count
the lines. The edge assertion is a count for that reason, and because an SVG
path with a stroke and no fill is not what Playwright calls visible.
`gotoSession` grows an optional search-parameter argument so the spec can open
`?view=cockpit`, the canvas's addressable entry, without re-implementing the
theme priming and the theme assertion.
Verified locally against a real Chromium: both themes pass, the canvas reports
one graph and three routed edges, and the captures show the surface.
|
|
|
Thanks for the PR! This is a re-run at Template looks good ✓ — every required section filled in, a real reviewer test plan, and a full Chinese translation. Problem: observed, and checkable rather than asserted. At the merge base the visuals directory holds Direction: aligned. A dependency canvas's whole job is layout and edge routing, which is exactly what jsdom does not compute, and the visuals suite is the right home for it; landing the scenario ahead of #10938 is the right order. Nothing here touches auth, sandboxing, model selection, telemetry, release, or a public contract. One thing worth keeping in view: the third file is CI infrastructure, and its blast radius is every future web-shell PR's preview rather than this scenario. It is the part that deserves the most attention — and the part the description does not mention yet. Size: not core, so no threshold applies. 206 lines across 3 files — 186 in the new spec (test), 10 in Approach: the scope feels right and I could not find a materially simpler path. Extending the shared
Risk: no elevated risk signals — no file in the diff matches the revert-correlated path list. Moving on to code review. 🔍 中文说明感谢贡献！本次是在 模板完整 ✓ —— 所有必填章节都写到位了，包含一份真正的评审验证计划和完整的中文翻译。 问题： 已观测到，而且是可核实的、不是靠断言。在 merge base 上，visuals 目录下有 方向： 对齐。依赖画布的全部职责就是布局与边的布线，而这恰恰是 jsdom 不计算的；visuals 套件是它自然的归属，赶在 #10938 之前落地这个场景也是对的顺序。这里没有触及鉴权、沙箱、模型选择、遥测、发布或任何公开契约。有一点值得留意：第三个文件是 CI 基础设施，它的影响面是此后每个 web-shell PR 的 preview，而不只是本场景。它是最该被认真看的部分——也是描述里目前还没提到的部分。 规模： 不涉及核心路径，因此没有阈值适用。3 个文件共 206 行——186 行在新 spec（测试），10 行在 方案： 范围合理，我没有找到明显更简的路径。给共享的
风险： 无升级风险信号——diff 中没有任何文件命中与回滚相关的路径清单。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI wrote my own plan before opening the diff: reuse the existing visuals harness, seed a This head adds three things since
I skipped the sequence diagram and the changed-files table: three files, test and CI only, and both would be noise here. Testing evidenceI did not run the spec — on an unattended CI run the review is static and PR-derived code is never executed. The evidence below is this PR's own CI on the reviewed commit, fetched once, no polling. The decisive check is Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified by me: the pixel content of the two cockpit composites. I tried to pull the published images down to look at them and the fetch is blocked in this environment, so I did not see the capture. A green visuals job proves the spec ran and its assertions held; it cannot prove the frame is worth anything, because @wenshao measured exactly that locally on this commit, and I am citing his numbers rather than reproducing them: the DAG viewport is 452px against an 868px laid-out graph, so No sandboxed lane applies, and I want to be explicit about why rather than leave the gap hanging: this PR ships no product behaviour, so 中文说明代码审查 打开 diff 之前我先写了自己的方案：复用既有 visuals harness、通过 mock daemon 注入 相比
我跳过了时序图与改动文件表：三个文件，只涉及测试与 CI，两者在这里都是噪音。 测试证据 我没有运行这个 spec——在无人值守的 CI 运行中，审查是静态的，绝不执行 PR 带来的代码。下面的证据来自本 PR 自己在被审查 commit 上的 CI，一次性获取，没有轮询。 决定性的检查是 我没有核实的部分：两张 cockpit 合成图的像素内容。 我尝试把已发布的图片拉下来看，但这个环境里该请求被拦截了，所以我没有看到截图。visuals job 变绿只能证明 spec 跑通了、断言成立；它无法证明这张画面有价值，因为 @wenshao 在这个 commit 上本地测量了正是这一点，我引用他的数字而不是复述成自己的：DAG 视口宽 452px，而图的布局宽度为 868px，因此 没有适用的沙箱通道，我想把原因说清楚，而不是让这个缺口悬着：本 PR 不上线任何产品行为，所以 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — solid, and the delta since my last pass answered the one thing I raised; what keeps it off 5 is a set of non-blocking nits plus a capture whose pixel value I could not check myself. Stepping back, the sequencing argument is what makes this worth merging rather than merely harmless. Landing the scenario ahead of #10938 means the branch that actually touches this surface gets a rendered before/after instead of an argument, and the author shipped the harness first rather than using it as leverage later. The third commit also repairs something that pre-dated the PR: views from any spec outside What convinces me on the spec itself is that its numbers are derivable rather than reported. I re-derived the edge and node counts at this base (a merge from main sits between the two commits, so I did not carry the last pass's conclusion forward), traced The comments earn their place, which is why I would thank rather than curse whoever maintains this in six months. The note that a stroke-only SVG path is not Playwright-visible, and the note that mis-nested dependencies render four nodes and zero edges in a flat layout that looks plausible until you count the lines, are exactly what stops someone later from "simplifying" the fixture and quietly turning the capture into decoration. My reservations, all non-blocking and all named above: The honest limit of what I can attest: node and edge counts pin the graph's structure, not its pixels, and I could not fetch the published composite in this environment, so I have not looked at the capture. @wenshao ran it locally on this commit and reports that the frame covers roughly half the laid-out graph — the convergence step is entirely off-frame — and that the dock width is not render-stable, which will make this view noisy against the 0.02% threshold once it has a baseline. Those are his measurements, not mine, and neither is a defect in the spec; together they are the case for a follow-up that pins the dock width and, ideally, frames the whole graph. Worth filing rather than fixing here. CI on this commit is not finished — the ubuntu unit suite is still running — so I am posting no approval in this run. Approval is deferred until CI lands green on 中文说明Confidence: 4/5 —— 扎实，而且相比上一轮的增量正好回应了我提出的那一点；没给到 5 分，是因为一组非阻塞的小意见，外加一张我本人无法核对像素价值的截图。 退一步看，真正让这个 PR 值得合入、而不只是无害的，是它的顺序论证。赶在 #10938 之前落地这个场景，意味着那个真正触碰此面的分支拿到的是渲染出来的 before/after，而不是一段论证；作者先把 harness 发出去，而不是留作日后的筹码。第三个 commit 还修复了一个早于本 PR 就存在的问题：除 spec 本身说服我的地方在于它的数值是可推导的、而不是被汇报的。我在这个 base 上重新推导了边数与节点数（两个 commit 之间夹了一次来自 main 的合并，所以我没有沿用上一轮的结论），顺着 这些注释是有价值的，这也是为什么六个月后维护它的人我会感谢而不是埋怨。关于「只有 stroke 的 SVG path 在 Playwright 眼里并不 visible」的那条，以及关于「依赖嵌套错了会渲染出四个节点、零条边，退化成一个在数线条之前看起来很合理的平铺布局」的那条，恰恰能阻止后来有人去「简化」这个 fixture，从而悄悄把截图降级成装饰品。 我的保留意见全部是非阻塞的，也都在上文点名了：base 允许列表里的 我能背书的边界也要说清楚：节点数与边数钉住的是图的结构，不是像素，而这个环境里我无法拉取已发布的合成图，所以我没有看过这张截图。@wenshao 在这个 commit 上本地跑过，他报告说画面只覆盖了布局后图形的约一半——那个收敛步骤完全在框外——并且面板宽度不是渲染稳定的，一旦这个视图有了基线，它会在 0.02% 阈值上变得嘈杂。这些是他的测量，不是我的，而且两者都不是 spec 的缺陷；它们合起来构成了一个跟进的理由：把面板宽度固定下来，最好也把整张图框进画面。这值得单开 issue，而不是在这里修。 这个 commit 上的 CI 还没跑完——ubuntu 单测套件仍在运行——所以本次不发布任何 approve。批准已延后到 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 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 |
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.23.0)
The base (merge-base) capture step filtered the Playwright run to screenshots.spec.ts only, so views produced by any other spec never got a baseline and the composer kept stamping them 'new scenario' on every PR instead of diffing them. Widen the filter to an explicit allowlist of the screenshot-producing specs; the video-flow and harness specs stay out because the base arm discards recordings.
…ntent
- Annotate the agentTasks fixture as DaemonSessionAgentTaskStatus[] so an
SDK reshape of the agent-task payload fails this spec at compile time
instead of silently degrading the capture (the node/edge counts only
guard the todo graph).
- Assert the running task's rendered runtime ('1m 14s') before capturing,
so a broken tool-call <-> task linkage fails the spec instead of
shipping a degraded screenshot.
Local verification of
|
| Step from the description | Result |
|---|---|
npx playwright test --config playwright.visuals.config.ts session-workflow |
2 passed (7.6s); session-workflow-cockpit-{light,dark}.png written |
move blockedBy from _meta.qwenTodo to the top level |
fails, toHaveCount(3) → received 0 at session-workflow.spec.ts:177, both themes, flat layout |
drop the experimental.sessionWorkflow setting |
fails, toHaveCount(4) → received 0 at session-workflow.spec.ts:171, both themes |
whole visuals suite unaffected by the gotoSession signature change |
43 passed; and in a same-commit base-vs-head compose of all 64 composited views, 61 came out at 0.00% — the only differences were the two cockpit views and code-review-artifact-dark, see N2 |
| lint / format on the three changed files | eslint --max-warnings 0 clean, prettier --check clean |
The three points from round 1 — re-verified, each fixed
R1-1 (base-pass baseline). Confirmed the premise and the fix. With the old filter, playwright test --list screenshots.spec.ts selects 34 tests in 1 file against a full suite of 43 tests in 6 files. With the allowlist this PR adds, the base arm runs 39 tests / 4 files and emits 67 screenshots — the same 67 filenames the head arm emits, so every composited view now has a baseline. I ran the compose script over those two directories: the cockpit is diffed as a real before/after instead of being stamped NEW.
Cost: 81.0s → 82.0s wall locally (+1%), and the CI job went 17m26s → 18m7s against a 30-minute limit. The effect is already on this PR — the new preview comment dropped the permanently-NEW workflow-page-* composites and no longer carries the "one or more scenarios failed to render" warning.
R1-2 (fixture typing). The pin is valid and it discriminates: an isolated tsc over the spec (against a freshly built SDK dist) exits 0 as shipped, and renaming recentActivities → recentActivitiesRENAMED produces TS2561 on both fixture entries. Without the pin the same drift compiles clean, exactly as the round-1 comment said. See N4 for the caveat.
R1-3 (task-derived gate). Earns its place. Mutating the running task's toolUseId to a value no toolCallEvent matches: before the gate the spec passed green with a degraded capture ("In progress" instead of "Running", agent row Completed · 0s, "Active agents 0"); with the shipped gate it fails at session-workflow.spec.ts:183 on getByText('1m 14s'). Intact fixture still passes. I also checked the strict-mode risk: exactly one leaf element contains 1m 14s (the node metrics strip only renders metrics when expanded), so the locator is unambiguous today.
Non-blocking observations
N1 — the capture frames about half the graph. The DAG viewport is 452 px wide and the laid-out graph is 868 px, so 48% of the canvas is never painted. Per-element intersection with the clip rect, both themes: inspect-readme 100%, inspect-package 100%, compare-findings 58%, write-summary 0%, and 2 of the 3 edges. The count assertions don't see this — toHaveCount counts DOM nodes, and the off-frame node and edge are in the DOM. The practical effect is that the diamond's payoff (the step that the two-upstream node unblocks) is not in the evidence, and the description's "Open either capture. Expect … four step nodes, three routed edges" can't be followed as written.
N2 — the view is not render-stable, and after merge that becomes visible. Rendering the same commit twice, the cockpit lands on one of two layouts: the cockpit column measures 500 px in 7 of 10 renders and 502 px in 3 (inspector left edge 777.00 vs 778.63). Scored with this repo's own compose script, every cross-group pair differs by exactly 12 596 px = 1.230%, against CHANGED_PCT_THRESHOLD = 0.02% — 61× the cutoff; same-group pairs are 0.000%. Because the base and head arms render in separate jobs, with p≈0.3 they land in different groups ≈42% of the time, so once this scenario has a baseline it will post a "cockpit changed" composite on roughly two of every five web-shell PRs that never touched it.
This is not something the PR introduces: of the 65 views I rendered twice from the same commit, the only other one that differed was code-review-artifact-dark (3.14%) — the other scenario that opens this same right dock. But this PR adds a second instance and, through its own base-pass fix, is what makes both visible. Worth a follow-up that pins the dock width for the capture (the harness already seeds theme through localStorage, and the panel width is persisted under qwen-code-web-shell-right-panel-state).
N3 — git-branch-picker.spec.ts in the base allowlist is dead weight. It writes 01-branch-picker.png, 02-commit-dialog.png, 03-create-pr-form.png; parseShot() in web-shell-visuals-compose.mjs only accepts <view>-<light|dark>.png, so those three are dropped from both sides and can never be composited. Rendering that spec in the base arm buys nothing. Either drop it from the filter or rename its shots to the -light/-dark shape so they actually enter the preview.
N4 — nothing in CI type-checks the pinned fixture. client/e2e/** is excluded from both packages/web-shell/tsconfig.json and tsconfig.lib.json (tsc -p tsconfig.json --listFiles lists 0 files under e2e/visuals), and no ESLint block in the repo is type-aware. Playwright transpiles without checking. So the pin protects the editor, not the pipeline — the same is true of workflow-page.spec.ts's existing pin. A small tsconfig.e2e.json added to the typecheck script would make R1-2's fix actually bite.
N5 — the description no longer matches the head. It still describes a two-file change; the current head also edits .github/workflows/web-shell-visuals.yml (the base-pass allowlist) and adds the type pin and the runtime gate. Worth folding into "What this PR does" / "Risk & Scope" before merge, since the workflow edit is the part with blast radius beyond this scenario.
Two things that are not this PR
- The
web-shell E2E Smokefailure on the previous headfceef01was the job hitting its 20-minute limit, not a broken test.playwright.config.tshastestIgnore: '**/visuals/**', so neither changed file is in that suite; the smoke suite runs 50 passed in 1.6m locally at this head. - The
terminal-turn-error-copy-narrow-{dark,light}composites on this PR's own preview differ only in a wall-clock timestamp (16:59:20vs16:50:41) — an unfrozen clock inscreenshots.spec.ts. Pre-existing, and part of the same preview-noise story as N2.
Verdict
Merge-ready. The scenario does what it claims, the assertions discriminate, and the workflow fix repairs a real, measured gap that pre-dated this PR. N1/N2 are about how much the capture is worth once it lands, not about whether it should land; N5 is a description edit.
中文说明
对 19881d68 的本地验证（真实 Chromium，完整 visuals 套件）
我在本地重建环境、用真实浏览器跑了这个场景，而不是只读 diff。Reviewer Test Plan 中的每一步都能复现，上一轮的三条意见都已真正修复，而且 base-pass 的改动在本 PR 自己的 preview 评论里已经能看到效果。没有阻塞项，我认为可以合入。 下面是五条非阻塞观察，其中一条我认为值得单开跟进。
环境。 packages/web-shell 位于 19881d68，merge base 为 9bb2f853；Playwright 1.61.1 / chromium-headless-shell 1228，Linux，使用 playwright.visuals.config.ts 自带的 dev server；无 daemon。
Reviewer Test Plan —— 逐条复现
| 描述中的步骤 | 结果 |
|---|---|
npx playwright test --config playwright.visuals.config.ts session-workflow |
2 passed (7.6s)，两张 session-workflow-cockpit-{light,dark}.png 均已生成 |
把 blockedBy 从 _meta.qwenTodo 移到顶层 |
失败，toHaveCount(3) 实际为 0，位置 session-workflow.spec.ts:177，深浅两个主题都失败，退化为平铺布局 |
去掉 experimental.sessionWorkflow 设置 |
失败，toHaveCount(4) 实际为 0，位置 session-workflow.spec.ts:171，两个主题都失败 |
gotoSession 签名改动不影响整套 visuals |
43 passed；并且在同一 commit 的 base/head 对比中，全部 64 个合成视图里有 61 个差异为 0.00% —— 唯一有差异的是两张 cockpit 与 code-review-artifact-dark，见 N2 |
| 三个改动文件的 lint / format | eslint --max-warnings 0 通过，prettier --check 通过 |
上一轮的三条意见 —— 逐条复验，均已修复
R1-1（base 侧基线）。 前提与修复都确认成立。用旧过滤器时，playwright test --list screenshots.spec.ts 选出 34 tests in 1 file，而完整套件为 43 tests in 6 files。换成本 PR 的允许列表后，base 侧运行 39 tests / 4 files，产出 67 张截图 —— 与 head 侧产出的 67 个文件名完全一致，因此每个进入合成的视图都有了基线。我用 compose 脚本跑了这两个目录：cockpit 现在是真正的 before/after 差分，而不再被打上 NEW。
代价：本地 81.0s → 82.0s（+1%），CI 任务从 17m26s → 18m7s，上限为 30 分钟。效果在本 PR 上已经可见 —— 新的 preview 评论里，长期处于 NEW 状态的 workflow-page-* 合成图消失了，"one or more scenarios failed to render" 的告警也没有了。
R1-2（fixture 类型）。 这个 pin 是有效的，而且确有判别力：对该 spec 做隔离 tsc（针对新构建的 SDK dist）按原样退出码为 0；把 recentActivities 改名为 recentActivitiesRENAMED 后，两个 fixture 条目各报一个 TS2561。没有 pin 时同样的漂移可以编译通过 —— 与上一轮评论所说完全一致。注意事项见 N4。
R1-3（任务派生的门）。 这个门确有价值。把运行中任务的 toolUseId 改成任何 toolCallEvent 都匹配不上的值：加门之前，spec 绿灯通过，而截图已经降级（徽章显示 "In progress" 而非 "Running"，agent 行显示 Completed · 0s，"Active agents" 为 0）；加上本次提交的门之后，它在 session-workflow.spec.ts:183 的 getByText('1m 14s') 处失败。fixture 完好时仍然通过。我还检查了 strict mode 的风险：包含 1m 14s 的叶子元素恰好只有一个（节点指标条只在 expanded 时才渲染 metrics），所以目前该定位器不存在歧义。
非阻塞观察
N1 —— 截图只框住了大约一半的图。 DAG 视口宽 452 px，而图的布局宽度为 868 px，因此 48% 的画布从未被绘制。逐元素与裁剪矩形求交（两个主题一致）：inspect-readme 100%、inspect-package 100%、compare-findings 58%、write-summary 0%，三条边中有两条在框内。计数断言看不见这一点 —— toHaveCount 数的是 DOM 节点，而框外的那个节点和那条边都在 DOM 里。实际后果是：菱形结构最关键的收敛点（被双上游节点解锁的那一步）并不在证据里，而描述中"打开任一截图，应看到四个步骤节点、三条已布线的边"这条验证步骤按字面无法执行。
N2 —— 该视图的渲染不稳定，而合入后这一点会显现出来。 同一个 commit 渲染两次，cockpit 会落到两种布局之一：10 次渲染中 cockpit 列宽 7 次为 500 px、3 次为 502 px（inspector 左边缘 777.00 vs 778.63）。用本仓库自己的 compose 脚本打分，每一对跨组的截图差异都恰好是 12 596 px = 1.230%，而 CHANGED_PCT_THRESHOLD = 0.02% —— 是阈值的 61 倍；同组之间则为 0.000%。由于 base 与 head 在不同的 job 中渲染，按 p≈0.3 计算，两侧落到不同组的概率约 42%，因此这个场景一旦有了基线，大约每五个未触碰 cockpit 的 web-shell PR 中就会有两个被贴出 "cockpit 变了" 的合成图。
这并非本 PR 引入：在我用同一 commit 渲染两遍的 65 个视图中，唯一另一个出现差异的是 code-review-artifact-dark（3.14%）—— 也正是打开同一个右侧面板的那个场景。但本 PR 增加了第二个实例，并且通过它自己的 base-pass 修复，使这两个实例都变得可见。建议单开一个跟进，把捕获时的面板宽度固定下来（harness 已经通过 localStorage 预置主题，而面板宽度持久化在 qwen-code-web-shell-right-panel-state 下）。
N3 —— 允许列表里的 git-branch-picker.spec.ts 是无效开销。 它产出 01-branch-picker.png、02-commit-dialog.png、03-create-pr-form.png；而 web-shell-visuals-compose.mjs 里的 parseShot() 只接受 <view>-<light|dark>.png，因此这三张在两侧都被丢弃，永远不可能进入合成。在 base 侧渲染这个 spec 没有任何收益。要么把它从过滤器里去掉，要么把它的截图改成 -light/-dark 的命名形态，让它们真正进入 preview。
N4 —— CI 里没有任何环节对这个被 pin 的 fixture 做类型检查。 client/e2e/** 同时被 packages/web-shell/tsconfig.json 与 tsconfig.lib.json 排除（tsc -p tsconfig.json --listFiles 在 e2e/visuals 下列出 0 个文件），并且仓库里没有任何一段 ESLint 配置是 type-aware 的。Playwright 只做转译、不做类型检查。所以这个 pin 保护的是编辑器，而不是流水线 —— workflow-page.spec.ts 已有的 pin 同样如此。给 typecheck 脚本加一个小小的 tsconfig.e2e.json，才能让 R1-2 的修复真正生效。
N5 —— 描述与当前 head 已经不一致。 描述里仍写作两个文件的改动；而当前 head 还改了 .github/workflows/web-shell-visuals.yml（base-pass 允许列表），并新增了类型 pin 与运行时门。建议合入前把这些补进 "这个 PR 做了什么" / "风险与范围" —— 工作流那处改动的影响面超出了本场景。
两件与本 PR 无关的事
- 上一版 head
fceef01上web-shell E2E Smoke的失败，是任务撞上了 20 分钟上限，而不是测试挂了。playwright.config.ts里有testIgnore: '**/visuals/**'，因此两个改动文件都不在该套件内；在当前 head 上，smoke 套件本地跑出 50 passed in 1.6m。 - 本 PR preview 里的
terminal-turn-error-copy-narrow-{dark,light}合成图，唯一差异是消息上的挂钟时间（16:59:20vs16:50:41）——screenshots.spec.ts里有一处未冻结的时钟。属于既有问题，和 N2 同属 preview 噪声这条线。
结论
可以合入。该场景确实做到了它声称的事，断言具有判别力，工作流那处修复填上的是一个早于本 PR 就存在、且可被测量的缺口。N1/N2 关系到这张截图落地后的价值大小，而不是它该不该落地；N5 只是描述层面的修订。
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 30 passed · 0 failed · 30 total Flakiness gate: not applicable — no runnable changed test files (1 out-of-scope file(s) noted in the log) 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：30 通过 · 0 失败 · 30 总计 抖动门：不适用 — no runnable changed test files (1 out-of-scope file(s) noted in the log) Verification reportPR #11014 — deep verificationVerdict: 中文摘要
Central claim and A/BCentral claim: the new visuals scenario renders the Session Workflow dependency canvas in both themes, produces the two captures, and its three assertions (4 nodes / 3 routed edges / Secondary claims: (a) the The A/B here is a mutation A/B (the diff is test-only): each cell reverts/breaks one point of the fixture or of the production code the spec claims to hold down, runs the light cell, and records whether the spec goes red. Expected reds are passes.
8/8 killed, 0 survived, unmutated control green; both positive controls caught, so the harness demonstrably can fail. Witness: The capture itself, read as a reviewer would ( Secondary claim (a):
|
| plan step | outcome |
|---|---|
| 1. run the spec, expect 2 passes + both PNGs | performed — 2 passed, both PNGs >85 KB |
| 2. open a capture, expect 4 nodes / 3 edges / status treatments / inspector beside canvas | performed, partially satisfied — DOM assertions hold (4/3), inspector is in frame with the agent row; but in the image only 2 of 4 nodes are fully legible and compare-findings is clipped at the inspector boundary (Finding 1) |
3a. move blockedBy to top level, expect edge count to fail at 0 |
performed — M1, failed exactly there |
| 3b. drop the gating setting, expect node count to fail at 0 | performed — M2, failed exactly there |
Findings
F1 — the new capture exposes a pre-existing cockpit header/canvas layout collapse (product code, not this diff) — Suggestion
At the capture viewport the cockpit's middle column is 500 px (260 px sidebar + ~520 px inspector flank it). .header is display:flex with .backButton and .headerStatus both flex: none (~87 px + ~312 px), so .identity — which carries min-width: 0 — absorbs all the shrink: measured title column 33 px at 1280 and 48 px at 1440. The <span> view label has no overflow rule (only the h1 does), so its 86 px of text paints 32 px past its own box, under the status chips; the session title ellipsizes (scrollWidth 311 vs clientWidth 33, renders as "E.."); the workspace path wraps one word per line in a 40 px column. The canvas is clipped the same way: of the fixture's four nodes, two are fully in frame and compare-findings is cut at the inspector edge.
Measured, not eyeballed (logs/02-header-probe.txt, logs/07-probe-widths.txt, assertions C6–C9):
| viewport | cockpit column | title column | label↔chip overlap |
|---|---|---|---|
| 1280 (capture viewport) | 500 px | 33 px | +32 px |
| 1440 | 515 px | 48 px | +17 px |
| 1680 | 621 px | 154 px | −40 px (clears) |
Reproduce: cd packages/web-shell && npx playwright test --config playwright.visuals.config.ts --retries=0 session-workflow, then open session-workflow-cockpit-light.png; crop in 04-cockpit-header-collapse-crop.png.
Why it is a finding and not a blocker for this PR: the diff touches zero product files (git diff --name-status HEAD^1..HEAD = 2 modified non-product files + 1 new spec), so base renders identically; the defect lives in SessionWorkflowCockpit.module.css's header flex layout today. The PR's stated purpose was to make this surface visible to something other than a stylesheet reading — the capture did that on day one. Note the corollary for the spec: its three assertions pass while the header is illegible, so the assertions guard the graph and the agent row, not header legibility; the PNG is the only guard for that, which is inherent to a capture-only scenario but worth knowing when reading future composites. No fix was attempted this round (a product-CSS change is out of scope for verifying a test-only PR); the mechanism above names where it lives (.identity / .identity > div need a width floor or the status chips need to yield, and the <span> label needs the same overflow treatment as the h1).
F2 — the spec's claimed compile-time pinning is enforced by no gate — Suggestion
The fixture comment ("Pinned to the SDK type so a reshape of the agent-task payload fails this spec at compile time") and commit 19881d68's title both promise a compile-time guard. All three web-shell tsconfigs (tsconfig.json, tsconfig.lib.json, and the build config) exclude client/e2e/**, CI's npm run typecheck runs the workspace scripts, and Playwright transpiles without typechecking — so nothing compiles this file.
Proven by planting a cast-free type error (runtimeMs: "seventy-four",) and observing both sides (tmp/typecheck-blindness.mjs, logs/10-typecheck-blindness.txt, assertions T1–T3):
| run | result |
|---|---|
workspace npm run typecheck with the planted error |
exit 0 — gate blind |
| same file forced into a tsc project | exit 2, session-workflow.spec.ts(107,5): error TS2322: Type 'string' is not assignable to type 'number' — the plant is a real type error |
| after restore | spec byte-identical to HEAD, typecheck exit 0 |
So T1's green is blindness, not health. The behavioural guards are real (M3/M6 kill the runtime-text assertion), so the payload shape is not unguarded — but the stated mechanism is dead, and a future SDK reshape that keeps runtimeMs numeric while dropping a rendered field would degrade the capture silently. Fix direction (unmeasured this round): add client/e2e to a typecheck project with node types and wire it into CI.
Not covered
- The full visuals suite was not run end to end at head (
screenshots.spec.tsalone is ~26 views × 2 themes); coverage of the delta is the new spec run plus--listcollection of the whole allowlist in both trees. - The
web-shell-visuals.ymljob was not executed as Actions (no token, no runner). The changedrun:line was executed verbatim in both trees viaplaywright --list, and the composer's NEW-stamping logic was read, but the compose step's pixel-diff/threshold behaviour was not replayed against a real emitted before/after pair — the replay is uncalibrated; calibrating it would need a real artifact from a production run of the compose step. - Per-commit attribution: the snapshot lists 4 commits but the checkout is depth 2 (merge commit, base tip, PR head only), so only the aggregate
HEAD^1..HEADdiff was verified; thefix(ci)commit's claim was verified through the aggregate (base filter list + composer logic), not by checking oute166a9b6. - The header defect (F1) was measured at head only. Base renders identically by construction (zero product files in the diff); no separate base render of the cockpit was performed because base has no spec for it.
npm run lint/prettier --checkon the changed files were not run (the PR's own CI covers them); web-shellnpm run typecheckwas run and passes, with the caveat in F2 that it does not seeclient/e2e.- Environment note, not a PR issue: this lane provisions its evidence browser for the repo's
playwright@1.58.2tree (chromium 1208, resolved fromintegration-tests/terminal-capture), while the web-shell visuals specs run under@playwright/test@1.61.1whose nestedplaywright-corewants chromium 1228. The first run died onExecutable doesn't exist at …/chromium_headless_shell-1228/…; I installed it withnpx playwright install chromium(logs/00-pw-install.txt). The real visuals job is unaffected — itsnpx playwright install --with-deps chromiumresolves the 1.61.1 CLI from the repo root.
Methodology
CI merge-ref checkout (HEAD = merge, HEAD^1 = base tip 419e8d57, HEAD^2 = verified head 19881d68), node:22-bookworm container, node v22.23.2, Playwright 1.61.1 with chromium 1228 installed into PLAYWRIGHT_BROWSERS_PATH as above. Every harness drives the real compiled/dev-served app through real Chromium via the repo's own playwright.visuals.config.ts and its mock daemon — no stubs of the code under test; mutations are single-point string replacements applied to a scratch state of one file, run, then restored and verified by sha256 (the matrix harness asserts each restore). The base-arm measurement ran in a scratch git worktree at HEAD^1 (removed afterwards). Raw per-cell logs, the four evidence PNGs, and the six rerunnable harnesses (mutation-matrix.mjs, url-identity.mjs, checks.mjs, typecheck-blindness.mjs, print-matrix.mjs, print-ab.mjs) are in this directory; assertion totals come only from those harnesses' executed checks (9 + 9 + 9 + 3 = 30).
Flakiness gate log
e2e suite, out of gate scope: packages/web-shell/client/e2e/visuals/session-workflow.spec.ts
verdict: n/a
summary: no runnable changed test files (1 out-of-scope file(s) noted in the log)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
APPROVE (verified at head 19881d6)
Historical items
Round 1's three findings were all Suggestions, and all three were adopted in this head — each verified in the code at 19881d6, not just from thread status: R1-1 via the explicit base-pass allowlist (screenshots.spec.ts workflow-page.spec.ts git-branch-picker.spec.ts session-workflow.spec.ts, with the "append new screenshot specs here" contract documented inline), R1-2 via const agentTasks: DaemonSessionAgentTaskStatus[] pinned to the SDK type, R1-3 via the expect(page.getByText('1m 14s')).toBeVisible() task-derived capture gate. No Critical or Request-Changes has ever existed on this PR; the human reviewer independently rebuilt head and ran the real-browser suite (43 visuals tests, the two negative mutations failing as designed, lint/prettier clean) and approved.
Critical-only scan
harness.ts:gotoSessiongains a 5th parameter with a{}default, so every existing caller is unaffected;URLSearchParams({ theme, ...search })keepsencodeURIComponent(sessionId)and cannot collide because the only current caller passes{ view: 'cockpit' }.- Workflow: the base arm keeps
continue-on-error: trueand itsif:guards; Playwright ORs positional filters, so a filter that matches no base file (e.g. the new spec against an older merge base) contributes nothing rather than erroring, and the after arm's full-suite run is untouched. - New spec: fixed clock (
T0), fixture counts are derivable (4 nodes, 3 diamond edges), title carries no@smoketag so the smoke job's--grep @smokecannot select it, and thetest:e2e:smoke/test:e2escripts use a different Playwright config entirely — the file is inert outsideplaywright.visuals.config.ts. - No new Critical found.
CI attribution for the two red checks
Test (ubuntu-latest, Node 22.x) failed with a Terminated + memory/disk dump after all package suites it had reached passed (runner-side kill; 80%-used disk on runner-31), and web-shell E2E Smoke failed on two pre-existing web-shell.smoke.spec.ts scenarios untouched by this PR. Both signatures are reproduced on the head of a completely unrelated PR merged from the same base around the same window (web-shell E2E Smoke failure + Test cancelled at eff408c on #10999), so they are environmental/fleet conditions on the current main baseline, not introduced here. Web-shell Visuals itself passed on head, and the preview comment shows the allowlist repair working (workflow-page composites no longer permanently NEW). Per policy these non-attributable failures do not gate.
4 participants
What this PR does
Adds a visuals scenario that renders the Session Workflow dependency canvas in both themes, so the
Capture web-shell visualsjob posts a before/after composite for that surface on every push.The plan in the fixture is a diamond — two independent steps, one that waits on both, one that waits on that — so the capture exercises what the graph exists to draw rather than a single chain: more than one layer, a node with two upstreams, and a step that unblocks two others. One step is completed and one running, each with a linked Agent task carrying real runtime and token counts, so the node status treatments and the inspector's agent rows are both in frame.
gotoSessiongrows an optional search-parameter argument so the spec can open?view=cockpit— the canvas's addressable entry — without re-implementing the theme priming and the theme assertion that helper already owns.Why it's needed
These surfaces have never been rendered by the visuals suite. Before this commit
grep -rn "sessionWorkflow" packages/web-shell/client/e2e/returned nothing, so every change to the plan DAG, the cockpit header and the inspector has shipped on a stylesheet reading plus a jsdom unit run — neither of which computes layout or the cascade.That gap has a measured cost. Three defects on the in-flight follow-up branch (#10938) were exactly this class, and all three were found by reading CSS rather than by any test:
overflow: hiddento pick up the node's radius, which also clipped the port dot deliberately positioned atright: -4pxoutside the box;text-overflow: ellipsiswas declared on aninline-flexchip, where it can never fire because the title sits in an anonymous flex item.A fourth,
.showAllActivitylosing to.activityList > buttonand rendering its label inside a 52px grid column, was caught by review rather than by a test. None of the four is visible to jsdom, and the visuals suite could not have caught them either, because it does not render this surface.Landing the scenario first means #10938 — and anything after it that touches this surface — gets a rendered before/after instead of an argument.
Reviewer Test Plan
How to verify
cd packages/web-shell && npx playwright test --config playwright.visuals.config.ts session-workflow. Expect two passing tests andsession-workflow-cockpit-{light,dark}.pngunder the visuals output directory.blockedByfrom_meta.qwenTodoto the top level and expect the edge count to fail at 0 with the flat layout; drop theexperimental.sessionWorkflowsetting and expect the node count to fail at 0.Evidence (Before & After)
N/A as a product change — this adds coverage and ships no behaviour. The captures it produces are the evidence, and they will appear on this PR from the
Capture web-shell visualsjob as head-only views, since the base has no such scenario.Verified locally against a real Chromium (Playwright 1.61.1, chromium-headless-shell 1228): both themes pass, the canvas reports one graph and three routed edges.
Tested on
Environment (optional)
Playwright visuals config with its own dev server on the default port; no daemon, the scenario is served by the existing mock.
Risk & Scope
gotoSessionsignature change is additive and defaulted, and every existing call site keeps its current URL shape.Linked Issues
Refs #8583, #10866, #10938.
中文说明
这个 PR 做了什么
新增一个 visuals 场景，在深浅两个主题下渲染 Session Workflow 的依赖画布，使
Capture web-shell visualsjob 每次推送都会为这个面贴出 before/after 对比图。fixture 里的 plan 是一个菱形——两个互相独立的步骤、一个同时等待这两者的步骤、再一个等待它的步骤——所以捕获的是这张图存在的意义所在，而不是一条单链：多于一层、一个有两个上游的节点、一个解锁两个下游的步骤。其中一步已完成、一步正在运行，各自关联一个带真实运行时长与 token 计数的 Agent 任务，因此节点的状态表现和 inspector 的 agent 行同时在画面内。
gotoSession增加一个可选的查询参数，使 spec 能打开?view=cockpit——画布的可寻址入口——而无需重复实现该 helper 已经负责的主题预置与主题断言。为什么需要
这些面从未被 visuals 套件渲染过。本次提交之前，
grep -rn "sessionWorkflow" packages/web-shell/client/e2e/没有任何结果，因此对 plan DAG、cockpit 头部和 inspector 的每一次改动，都是靠读样式表加上一次 jsdom 单测发出去的——而这两者都不计算布局，也不计算层叠。这个缺口有可量化的代价。在途的跟进分支 #10938 上有三个缺陷正属于这一类，而且三个都是靠读 CSS 发现的，没有任何测试抓到：
overflow: hidden才能吃到节点圆角，而这同时裁掉了刻意定位在right: -4px、位于盒子外面的端口圆点；text-overflow: ellipsis声明在inline-flex的 chip 上，而它在那里永远不会生效，因为标题位于匿名 flex item 中。第四个——
.showAllActivity输给.activityList > button、把标签渲染进 52px 的网格列——是评审发现的，同样不是测试发现的。这四个都不为 jsdom 所见，visuals 套件也同样抓不到，因为它不渲染这个面。先落地这个场景，意味着 #10938 以及此后任何触碰这个面的改动，拿到的是渲染结果而不是一段论证。
评审者验证计划
如何验证
cd packages/web-shell && npx playwright test --config playwright.visuals.config.ts session-workflow。应有两个通过的用例，并在 visuals 输出目录下生成session-workflow-cockpit-{light,dark}.png。blockedBy从_meta.qwenTodo移到顶层，边数断言应在 0 处失败并退化为平铺布局；去掉experimental.sessionWorkflow设置，节点数断言应在 0 处失败。证据（改前 / 改后）
作为产品改动而言不适用——本 PR 只增加覆盖，不改变任何行为。它产出的截图本身就是证据，并会由
Capture web-shell visualsjob 以 head-only 视图的形式出现在本 PR 上，因为 base 上没有这个场景。已在真实 Chromium 上本地验证（Playwright 1.61.1，chromium-headless-shell 1228）：两个主题均通过，画布报告一张图与三条已布线的边。
测试环境
运行环境（可选）
Playwright visuals 配置及其自带的 dev server，使用默认端口；无需 daemon，场景由既有的 mock 提供。
风险与范围
gotoSession的签名改动是增量且有默认值的，所有既有调用点的 URL 形态保持不变。关联 Issue
Refs #8583, #10866, #10938.