feat(web-shell): make Session Workflow dependencies navigable and quiet its chrome - #10938
Conversation
The `auto` size floors the panel at `min-w-[min(100%,560px)]` so a small plan graph does not collapse to a narrow column. tailwind-merge keeps that class alongside the base ceiling `max-w-[calc(100%-2rem)]` — different modifier groups — and below the `sm:` breakpoint min-width wins over max-width. On a 390px viewport the Plan & tasks dialog therefore rendered its rounded, ringed panel flush to both screen edges, while every fixed size (sm/md/lg/xl) kept the 1rem gutter at that same width. Floor the panel at the gutter the ceiling already reserves.
…aces lanes Lane spacing was stated twice in two languages: `EDGE_LANE_HEIGHT` in TS, which places each layer-spanning return lane at `lane * EDGE_LANE_HEIGHT`, and a literal `9px` in `.dagCanvas`'s bottom padding, which reserves room for them. The two are coupled only through the `--plan-edge-lanes` inline style, so raising the constant alone leaves the reservation short; with enough spanning edges the clamp pins the outer lanes together and layer-spanning edges overlap again — the exact defect the lanes exist to fix. Nothing would have caught it: the lane test mocks `offsetHeight` directly. TS now publishes the pitch as `--plan-edge-lane-height` beside the lane count, and the stylesheet reads it. Also give the graph a phone: three 240px lanes plus two 64px gutters are ~850px, so the canvas scrolled in both axes at once and the horizontal scroll hid the layer the vertical scroll was looking for. Below 720px the lane and gutter narrow. Layer semantics and the rect-measured edge routing are untouched, so this changes what fits, not what is drawn. The new stylesheet guard pins both, in the shape the other `.css.test.ts` files use, since jsdom does not compute the cascade. One test-only addition on the same component: the spanning-edge test fixed the edge's arrival column (`H 876`) but not its arrival row, and nothing else in the file asserted a spanning edge's `endY`. Substituting the return lane's `routeY` for `endY` in the path tail draws the edge ~170px below the target node while every existing assertion still passes. Pinned corner-agnostically. Refs #10866.
`compactTaskExecutionOutput` keeps an allowlist of `task_execution` fields when it drops the bulky ones, and every neighbouring entry — `executionSummary`, `tokenCount` — is already pinned by the reducer tests. `skills` was not: removing it from the keep-set left the whole suite green, and a session restored from a persisted transcript would silently lose the skill list the live run recorded. Mutation-checked: deleting `'skills'` from the keep-set fails exactly this assertion (1 failed | 327 passed); restored, 328/328.
…rface state each thing once
Two related passes over the Session Workflow inspector and cockpit.
## Dependencies become navigation
A step's dependencies rendered as `todo.blockedBy!.join(', ')` — a run of raw
Todo ids, in both the graph node and the inspector detail. An id is an
address, not a label, so following an edge meant reading `step-3, step-7` and
then finding those rows by eye, in a panel whose entire subject is which step
waits on which.
Each reference is now the step it points at — the step number the list and
the graph both already show, plus its title — as a control that selects that
step. The dependency list becomes the graph's navigation.
Recent activity was sliced to 6 while the count beside it reported the true
total, so the panel contradicted itself and the remainder was unreachable.
The cap stays as a preview and gains a control that opens the rest.
## The surface states each thing once
The panel had accumulated chrome that competed with its content:
- Section labels were tracked-out uppercase at 10-11px, so every fixed label
shouted over the step title and status it introduced. They are labels, not
headlines: sentence case at a muted weight.
- The cockpit spent the accent colour on a fixed "Dependency graph" eyebrow
above the session title — making the one blue thing on the page the one
thing that never changes, while the graph below uses that same accent to
mean "this node is selected". The label moves to the subtitle line and the
accent is left to mean selection.
- Completed-over-total sat in a tinted pill directly above the progress bar
that draws the same ratio, beside the status word that names the same
state — three surfaces for one fact. The count keeps the tone as ink and
drops the pill.
- Runtime, tool calls, tokens and artifact kind/status were glued into single
strings with middle dots, which wrapped as a unit and read out as one run of
text. They are separate facts, so they are separate elements.
The identity line keeps the mono face only on the short session id, which is
an address the user copies or matches against a daemon log.
No token or theme values change; this is the existing system applied more
sparingly.
Both behaviour changes are mutation-checked: reverting the dependency list to
the joined id string, and restoring the silent cap, each fail exactly their
new test.
Refs #10866.
8ab5c5e to
f2c818f
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）为单个提交。 |
…flag `todo_write`'s omitted-`blockedBy` preservation, and the `blockedBy` schema text that invites the omission, both reach sessions with Session Workflow disabled. That was a deliberate choice when the behaviour landed, but nothing in the file said so, so it reads as a missed gate — the `hasActivePlan` derivation sat bare and the only nearby comment explains the dangling-edge filter, not the gating. Record the decision and its reasoning: these are plan data-model semantics rather than presentation, and gating them on a visualization switch would give the tool two dialects, storing different plans for the same call depending on whether anyone is looking at the graph. Record the blast radius in the same place, because "reaches every session" overstates it. The merge changes an update only when the previous plan still has an unfinished item, the same id carried a non-empty `blockedBy`, and this call omits it. Plans that never used dependencies are untouched (`undefined?.filter()` is `undefined`), callers that always send the full list are unaffected, and `[]` stays the explicit clear. What does reach every session is the schema text, which is why it is named as widening this branch's exposure rather than as an independent change. Comment-only; no behaviour, no signatures.
|
Re-run against head Template looks good ✓ — every required heading is there, plus the Chinese Problem: observed, not theoretical. Each item is something you can point at in the pre-diff source: dependencies rendered as Direction: aligned. It closes behaviour/shape items left open by the merged cockpit (#8583) plus the still-open #10866, entirely inside Size: no core paths and no cross-package changes, so the two-tier core gate does not apply. Of the 2351 changed lines: 837 production (431 TSX, 406 CSS, plus 5 in i18n), 1298 test, 216 docs. Counting the design doc as neither test nor generated puts the non-test total at 1053, so the non-blocking 1000+ advisory technically trips — it is a big PR. Informational only; nothing here is blocked on size. Approach: six distinct concerns ride together — node-face redesign, dependency navigation, the activity cap, the lane-pitch binding, the narrow viewport tiers, and the inspector chrome. In the abstract I'd have asked for these as separate PRs. Two things make me not ask now: several are genuinely entangled (dropping the id off the node face is what makes the step number necessary, which is what makes the chips navigable), and three rounds of @wenshao's runtime verification plus the autofix rounds have accumulated against this exact tree — splitting it would throw that away. Worth noting for the next round of this surface rather than acting on here. Risk: no elevated risk signals — none of the 13 files match the revert-correlated path list, and the change is confined to one package's rendering layer. Moving on to code review. 🔍 中文说明本次是针对 head 模板完整 ✓ —— 所有必需小标题都在，中文 问题： 已观测到的，不是理论性加固。每一项都能改前源码里直接指出来：依赖渲染成 方向： 对齐。它补上已合并的 cockpit（#8583）遗留的行为/形态条目，加上仍未关闭的 #10866，改动全部落在 规模： 未触及核心路径，也没有跨包改动，所以两级核心 gate 不适用。2351 行改动中：生产代码 837 行（TSX 431、CSS 406、i18n 5 行）、测试 1298 行、文档 216 行。把设计文档算作非测试非生成的话，非测试总计 1053 行，因此 1000+ 的大 PR 建议在字面上成立——这确实是个大 PR。仅供参考，不会因为规模而阻断。 方案： 六个独立议题被放在一起——节点面重设计、依赖导航、活动条数上限、泳道间距绑定、窄视口档位，以及 inspector 界面收敛。抽象地说我会要求拆成多个 PR。现在不提这个要求有两点原因：其中几项确实互相纠缠（把 id 从节点面拿掉，才使步骤号成为必需，才使 chip 可导航）；而且 @wenshao 的三轮真实环境验证加上 autofix 的多轮迭代都是针对这棵确切的代码树积累的——拆开等于把这些验证作废。这一点更适合作为这个界面下一轮改动的提醒，而不是在这里执行。 风险： 无升级风险信号——13 个文件都没有命中与 revert 相关的路径列表，改动局限在单个包的渲染层。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI wrote my own proposal before opening the diff. For "make dependencies navigable" I'd have built an id → (index, title) map and rendered each reference as a control calling the existing selection handler, keeping the node-face chips read-only because they live inside the node's own The PR does all five, and beats my proposal in four places I would not have thought of:
What I verified against
Two things I looked hard at and am not reporting as findings, because both survive contact with the code:
Non-blocking findingsNothing here blocks merge. All five are real, and four are already known to @wenshao's rounds — I'm recording them so they don't get lost when the branch is deleted.
Files changed (13)
TestingThis is an unattended run, so I did not build or execute anything from this branch. The evidence below is the PR's own CI on the reviewed commit, read through the API, plus one thing I checked myself: the author says head
No red checks, so there is no failing log to quote. The Not verified by this pass, and why it doesn't leave a hole: the ≤720px / ≤480px tiers, the inspector chrome, and the light theme have no rendered CI evidence — 中文说明代码审查我在看 diff 之前先写了自己的方案。要让依赖可导航，我会建一个 id → (序号, 标题) 的映射，把每个引用渲染成调用现有选中处理函数的控件，并保持节点面上的 chip 只读——因为它位于节点自己的 这五点 PR 都做了，并且在四个我自己想不到的地方更好：
以下是我对着
有两点我认真查过，但不作为问题上报，因为它们在代码面前站不住：
非阻断问题以下都不阻断合并。五条都是真实存在的，其中四条 @wenshao 的历轮验证已经知道——我记录下来是为了分支删除后它们不会丢失。
测试这是无人值守的一轮，所以我没有构建或执行本分支的任何代码。下面的证据是通过 API 读到的、本 PR 自己在被审提交上的 CI 结果，外加我亲自核对的一件事：作者说 head 没有红色 check，因此没有失败日志可引用。 本轮未验证、以及为什么这不构成缺口：≤720px / ≤480px 档位、inspector 界面和浅色主题没有渲染类 CI 证据—— — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — solid, and unusually well-evidenced for a rendering change; the reservations are documentation and deferred follow-ups, not the code. Stepping back: this started life as a design-ledger cleanup on a merged feature and turned into something better than that. The part that convinced me isn't the visual work — I can read a stylesheet, and so can anyone — it's that the author kept finding the second channel. Status became a left rule, so status also had to stay in the accessibility tree as words. Attention folded into the same rule, which meant a paused node needing attention became pixel-identical to a healthy one, so it needed a shape mark. Edges are My independent proposal was materially simpler in exactly one place — I'd have left the chip row gated and accepted the accessibility hole — and the diff is right and I was wrong. The lane-pitch binding is the cleanest single change here: a literal On the results matching the promise: I could not run this branch, and did not try. What I could do is check that the evidence attached to it is real rather than asserted — and it is. Two candidates I chased down and dropped, because reporting them would have been wrong: the narrow tiers use viewport What I'd still want, and am not blocking on:
Per this repo's own rule about not letting review rounds balloon a PR, this one is far past five rounds — so those four belong in a follow-up issue, not in another round here. Only the description edit is worth doing now. Housekeeping for whoever owns the autofix loop, not something I'll touch: the Verdict: approve. Both 中文说明Confidence: 4/5 —— 扎实，而且对一个渲染类改动来说证据异常充分；我的保留意见都在文档和延期的后续项上，不在代码本身。 退一步看：这个 PR 起初是对一个已合并功能的设计台账清理，最后做成的东西比那更好。真正说服我的不是视觉部分——样式表谁都能读——而是作者一直在把第二条通道找出来。状态改成左侧竖条，于是状态也必须以文字留在无障碍树里。attention 并入同一根竖条，于是一个需要关注的 paused 节点与一个健康节点在像素上变得完全相同，所以它需要一个形状标记。边是 我自己的方案只在一个地方明显更简单——我会让 chip 行保持 gated，并接受那个无障碍缺口——而 diff 是对的，我是错的。泳道间距绑定是这里最干净的一处改动：样式表里一个字面量 关于结果是否兑现承诺：我不能运行这个分支，也没有尝试。我能做的是核实附在它上面的证据是真实的而不是声称的——而它是真实的。 有两个候选问题我追下去之后放弃了，因为上报它们会是错的：窄屏档位在别处使用 我仍然希望、但不据此阻断的：
按本仓库自己关于不要让评审轮次把 PR 撑大的规则，这个 PR 已经远超五轮——所以那四项应该进一个后续 issue，而不是在这里再开一轮。只有描述那一处值得现在改。 一件交给 autofix loop 负责人的杂务，我不会去动： 结论：批准。 该 SHA 上两条 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs some rethinking before this can merge — two blockers, both detailed in my Stage 2 note above.
.showAllActivityis out-specified by the existing.activityList > buttonrule (0,1,1 vs 0,1,0), so the new "Show all N runs" control becomes a four-column grid and its label lands in the 52px first column. jsdom cannot see this and no visual spec renders the inspector, so it ships green. Same collision you already solved for.metrics—.activityList > button.showAllActivityfixes it.- The body says the raw-id dependency rendering was fixed "in both the graph node and the inspector detail", but
PlanExecutionView.tsx:1311-1315and:1352-1361stilljoin(', '). The node case is genuinely harder (that line sits inside the node's own button), so deferring is fine — the description just has to say so.
Also worth a look: 588px of DAG content still scrolls horizontally on a 390px phone, and the core/SDK test riders are not mentioned in the description.
中文说明
合入前需要再考虑一下 —— 两个拦截项，细节都写在上面的 Stage 2 评论里。
.showAllActivity被既有的.activityList > button规则压过（特异性 0,1,1 对 0,1,0），于是新的 "Show all N runs" 控件变成四列网格，标签落在 52px 的第一列里。jsdom 看不到这一点，也没有任何 visual spec 渲染 inspector，所以它会一路绿灯合进来。这正是你在.metrics上已经解决过的同一类冲突 —— 改成.activityList > button.showAllActivity即可。- 正文说裸 id 的依赖渲染"在图节点和 inspector 详情两处"都修了，但
PlanExecutionView.tsx:1311-1315和:1352-1361仍然是join(', ')。图节点那一处确实更难（那行位于节点自己的 button 内部），所以延后没问题 —— 只是描述里需要写明。
另外建议看一下：588px 的 DAG 内容在 390px 手机上仍然要横向滚动；以及 core / SDK 的两处搭载测试改动在描述里没有提到。
— Qwen Code · qwen3.8-max-2026-09-02
Read against the three questions a plan graph is opened with — what is running, what is blocked on what, what needs me — the node answered none of them first. **Reading order was inverted.** The node led with a status glyph, then the raw Todo id, then the status word; `nodeContent`, the only element that says what the step *is*, came third. At graph scale a reader scanned `● step-3 Blocked` before "Compare findings". An id is an address and the least useful thing at that scale, so it leaves the node face entirely — it stays in the detail panel, where it is actually copied. Content leads, at 13px/500, behind the step number the inspector list and dependency chips already use, so the three surfaces address a step identically instead of three different ways. **Status was stated three or four times per node** — glyph, status word, border tint, plus an attention badge. It is now stated once, as a 3px left rule, and only when it is worth saying: waiting is the resting state of most nodes in a real plan and carries no rule. The stylesheet already made this argument for `blocked`; extending it consistently is what stops the canvas reading as a wall of tinted boxes and gives the one running node something to stand out against. Attention folds into the same rule and outranks the status tone, because it is the state the reader is being asked to act on. Status is colour, which assistive tech cannot read, so it stays in the accessibility tree as words. **Time was invisible** in an execution trace. The node face now carries the agent count and the summed agent runtime for that step — the "is this alive or hung" signal, which is the first thing asked of a live run. Two facts with real spacing, not one middle-dot-joined string that wraps as a unit and reads out as a single run of text. **Dependencies were stated twice**, as a drawn edge and again as `Depends on step-3, step-7`. The row is now rendered only when no edge states it — above `MAX_RENDERED_PLAN_EDGES` the graph draws none, and there the row is the dependency's only statement, so deleting it outright would have lost information in exactly the case that needs it most. It renders as the same number-and-title chips the inspector uses. The input port dot goes with them: the left edge now carries the status rule, and an incoming edge already terminates in an arrowhead at the node — that arrowhead is the input marker. The output port stays, since outgoing edges leave their source unmarked. Both port dots also drop `--agent-blue-500`; every node with an edge had one, so a permanently blue dot across most of the graph left selection no colour of its own. Edge routing, the layering algorithm, the return lanes and the measure pass are untouched. This is the node face and the colour rule. Three geometry tests identified a node by its first `<span>` — the Todo id. That is now the accessible status word, so they bind to `data-plan-node-id` instead, which is the stable hook and was always the better one. Every behavioural change is mutation-checked: ungating the dependency row, deleting the accessible status word, and dropping the elapsed each fail exactly their own assertion. Design plan, including the two revisions the review pass forced, is in docs/plans/2026-09-04-session-workflow-dag-design.md. Refs #10866.
🖼️ 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 |
ESLint flags the graph destructure binding as never used; the memo still computes and returns the map for other consumers. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtlr9x6ugb
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.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): test/unit/daemonUi.test.ts — no such file or directory.
[Critical] R1-3: [certifies-falsely] [regression] The description claims raw-id dependency rendering was fixed "in both the graph node and the inspector detail", but PlanExecutionView's own step-detail panel still renders selectedTodo.blockedBy!.join(', ') (~line 1402) and selectedDependents.join(', ') (~line 1407) — raw ids with no navigation — and the description does not name the deferral. The panel is reachable via TasksStatusMessage and ToolApproval, which render PlanExecutionView with showStepDetails defaulting to true: a user opening step details reads Depends on: research with zero clickable controls while the node face now shows "Completed 1 Research" (the id was removed from the face by this diff), so the panel's reference resolves to nothing visible — the exact gap this PR's "Dependencies become navigation" section claims to close persists on this surface. This is the exact blocker the prior CHANGES_REQUESTED review reported; verified still present at this HEAD. Witness: jsdom probe rendering PlanExecutionView (default showStepDetails) with a selected todo — panel text "Depends on: research" with 0 clickable controls in the row; the referenced node's face reads "Completed1Research". Fix: extend the same navigable chip/link treatment to the step-detail panel's Depends on / Unblocks rows, or state the deferral explicitly in the PR description as the prior review requested.
— qwen3.8-max via Qwen Code /review (v0.23.0)
…ontrol Round-1 review findings. **Blocker.** `.showAllActivity` was a single class (0,1,0), but the button is a direct child of `.activityList`, whose base rule `.activityList > button` (0,1,1) declares the four-column activity grid and wins. The label — a bare text run that Grid wraps into an anonymous item — auto-placed into the 52px first column and wrapped to three or four lines with three empty columns beside it, and the intended padding lost too. Hover only looked right by coincidence, because the row's own hover set the same background. This is the collision handled two rules earlier for `.metrics`, missed here. Both are now pinned by the stylesheet guard so neither can regress to a bare class, and the fix resets `display` and `grid-template-columns` explicitly rather than relying on source order. **The graph's step-details panel still joined raw ids.** The node face's chips cannot be controls — that row lives inside the node's own `<button>` and a nested button is invalid HTML — but this panel sits outside it, so its references become real controls that select the step they name, which is what makes a dependency list navigation rather than text. `Depends on` and `Unblocks` both. **The narrow-viewport claim was wider than the fix.** Three 168px lanes plus two 32px gutters is ~588px, which removes horizontal scrolling between there and the 720px breakpoint but still scrolls on a 390px phone. Added a second step below 480px that gets a three-layer plan under a phone viewport, and said plainly in the comment that deeper plans still scroll — reusing the flat layout stays with #10866. The guard test is renamed to what it pins: jsdom computes no layout, so it can assert the two steps exist and narrow, not an outcome. **The lane pitch had a second copy.** The `var()` fallback restated `9px` next to the `.dagCanvas` default, so "TS owns the pitch" held only when the graph has dependencies. The default is now `0px` — the zero-lane case collapses the reservation regardless — and the guard rejects the fallback copy too. **Comment antecedent.** "the prompt change" in todoWrite named nothing a later reader could find; it now quotes the schema description it means and says why a tool schema reaches every session. Refs #10866.
…lipping its port Two defects from my own review of the DAG pass, both in the class the round-1 review already caught once: invisible to jsdom, invisible to a passing unit run, and only findable by reading the stylesheet. **The status glyph was an accessibility channel, not decoration.** Making the left rule the single carrier of status dropped the glyph — but the constant it came from says exactly why it exists: "Status is carried by a glyph as well as a colour so the graph survives colour-blindness, high-contrast mode, and a greyscale screenshot." Colour plus a screen-reader-only word leaves a sighted colour-blind reader, high-contrast mode, and any greyscale capture unable to tell running from completed from needs-attention. "One status, one place" was the wrong rule; the right one is one carrier of the status *colour*, plus a redundant non-colour channel. The glyph returns for every status, in the meta line so the step's content still leads, and muted so the rule stays the only thing carrying the tone. Pinned per status, and pinned as distinct from each other, so the shape channel cannot silently collapse. **The status rule clipped the outgoing port.** Drawing it as `.node::before` needed `overflow: hidden` for the node's 8px radius to apply to it — which also clips the port dot at `right: -4px`, which is deliberately outside the box. It is a `border-left` instead: border-radius clips it natively, no overflow, and a transparent rule paints the node's own background so a resting node reads as 3px of extra left padding with no width difference between states. The stylesheet guard now rejects the overflow, the pseudo-element and a missing port offset together, since the three only fail as a set. Also drops `.nodeGlyph`'s dead sibling state: the rule is back in use, so nothing here is unused CSS. The design plan records both as corrections rather than folding them in, so a later reader sees which of its rules were wrong and why. Refs #10866.
Round-2 review findings that survive at this head; the round's three blockers (the node's clipped port, the show-all specificity collision, and the step-details panel's raw ids) were already fixed in the two commits after the head it reviewed. **The chips' `text-overflow: ellipsis` was dead.** It was declared on the `inline-flex` chip itself, but `text-overflow` applies to a block container's own inline content — the title sat in an anonymous flex item, so a long dependency title was clipped hard with no ellipsis. The title now has its own span carrying `overflow`, `min-width: 0` and the ellipsis, in all three places that render a reference (the node face's read-only chips and both directions of the step-details panel's links), and the stylesheet guard pins that it lives on the title rather than the box. **The comment above `.node` still described the old status behaviour** — it said status drives the accent that drives the border, which this branch stopped doing two commits ago. Rewritten to describe the border/rule split, keeping the hazard it originally recorded: with status off the border, a selected completed or blocked node can no longer pick up a second border in a clashing hue. **The compaction test's comment overclaimed.** It said every other keep-set entry is pinned by that test; it pins `skills`. Narrowed to what it does. Two coverage gaps the same round named: - The inspector test exercised only the upstream direction. Upstream and downstream render through one helper but read from different sources (`blockedBy` versus the projection's `dependentsByTodo`), so covering one did not cover the other; the downstream link is now rendered, asserted and clicked. - `planExecution.agentCount`'s singular and plural branches are separate formatter paths, and only the singular one was exercised — `1 agents` would have shipped green. Both are now pinned. Refs #10866.
|
@qwen-code /review Re-review requested against
This head additionally closes R1-8 (the chips' One finding I believe is incorrect, flagged rather than silently skipped: R1-9 says Also disclosed in the body: this environment has no browser, so the visual work was read from the stylesheet rather than rendered, and the |
|
Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 inspector upstream chip dedup — already reported (comment 3928733340)
- R1-17 design doc status line — already reported (comment 3928733447)
Not reviewed: reverse audit — ran to the 5-round cap without converging (round 5 still surfaced 2 findings, both verified before compose).
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/messages/PlanExecutionView.tsx:1317 — [probe] D2-1 the either/or SR-only status word drops the status word for attention nodespackages/web-shell/client/components/messages/PlanExecutionView.css.test.ts:36 — [probe] D2-2 the lane-pitch test pins TS publication of --plan-edge-lane-height but not of --plan-edge-lanespackages/web-shell/client/components/messages/PlanExecutionView.css.test.ts:28 — [probe] D2-3 the lane-pitch guard never pins that the reservation multiplies pitch by lane count
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：reverse audit — ran to the 5-round cap without converging (round 5 still surfaced 2 findings, both verified before compose).
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
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.
15 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 inspector upstream blockedBy dedup — already reported (comment 3928733340)
- R1-4 node-face chip row blockedBy dedup — already reported (comment 3932891691)
- R1-6 dead dependencyIdsByTodo — already reported (comment 3932891710)
- R1-8 (fix-induced) > span rules mute dependency titles — already reported (comment 3932891635)
- R1-9 attention rule precedes blocked/ready — already reported (comment 3932891713)
- R1-10 agentCount plural branch untested — already reported (comment 3932891722)
- R1-12 attention render surfaces untested — already reported (comment 3932891733)
- R1-13 metrics guard pins selectors only — already reported (comment 3932891739)
- R1-17 design doc status line — already reported (comment 3928733447)
- R2-1 step-details buttons raw blockedBy dedup — already reported (comment 3932891642)
- R2-2 dangling blockedBy click dismisses panel — already reported (comment 3932891616)
- R2-3 dependency navigation drops focus — already reported (comment 3932891674)
- R2-5 'Unblocks:' row label unpinned — already reported (comment 3932891621)
- R2-7 truncation guard misses overflow:hidden — already reported (comment 3932891652)
- R2-10 the 480px tier still overflows 390px phones — already reported (comment 3932891684)
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/sdk-typescript/test/unit/daemonUi.test.ts:8558 — [test] the added skills keep-set assertion is inert with respect to this diff — it pins pre-existing SDK compaction behaviour and gates nothing this PR ships (deferred by the code-ag…
中文说明
已审查。 建议见行内评论。
本轮确认的 15 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Local runtime verification (round 3) — PR #10938 @
|
| Arm | Port | Bundle |
|---|---|---|
| base | 4939 | Sources from the merge base bc7a186 |
| previous head | 4943 | 5f70a13 |
| lanefix | 4941 | Round-2 candidate patch. Not part of this PR |
| head | 4944 | d91a0f7e, from a fresh vite build |
Both sessions were created through the real Plan & Review flow: enter_plan_mode → todo_write with blockedBy → exit_plan_mode → Approve and execute · Full Access.
- DAG (from round 2): 5 steps in 3 layers. The edge
3 → 5skips a layer, and the layer it passes holds a single step. - SKIP (new this round): 7 steps in 4 layers, with 3 steps in layer 1 and 2 steps in layer 2. Three edges skip layers:
1 → 5and3 → 7skip one layer each, and1 → 7skips two. Every drop and rise gutter borders a layer with several steps. The DAG session cannot show this: round 2 noted that its drop segment missed a node only because the layer it passes has one step.
1. The round-2 repro (DAG session)
| Viewport · gutter | Previous head 5f70a13 |
Head d91a0f7e |
|---|---|---|
| 1440 · 64px | 0 crossings | All 4 paths byte-identical to the previous head |
| 700 · 32px | Verticals at x=250 / 426 (gutters 222–254 / 422–454) | x=238 / 438, the gutter centres. End tangent (6, 0) |
| 430 · 18px | The rise at x=251 passes through step 4 (106px of overlap) | x=136 / 270, centred. 0 crossings, end tangent (2.5, 0) |
| 390 · 18px | The rise at x=246 passes through step 4 (106px of overlap) | x=131 / 265, centred. 0 crossings, end tangent (2.5, 0) |
At 700, 430 and 390px, all 4 paths are also byte-identical to the round-2 candidate. The adjacent-layer arrowheads keep the tangents the round-2 fix gave them: (28, 0), (12, 0) and (5, 0).
2. A harder plan: stacked layers and a two-layer skip (SKIP session)
| Viewport · gutter | base | Previous head 5f70a13 |
Head d91a0f7e |
|---|---|---|---|
| 1440 · 64px | 0 crossings | 0 crossings | 0 crossings. All 11 paths byte-identical to the previous head and to the candidate |
| 700 · 32px | 0 crossings (the gutter is 64px at every width) | 0 crossings, but all 6 verticals sit 12px off-centre, 4px from the next step | All 6 verticals centred, end tangent (6, 0) |
| 430 · 18px | 0 crossings | 12 lane/step overlaps. 1→5 drops and rises through steps 3 and 4, 3→7 through steps 5 and 6, and 1→7 through steps 3/4 and 5/6. Each vertical lands 10px or 106px inside a step, with up to 142px of overlap |
0 overlaps. All verticals centred, end tangent (2.5, 0) |
| 390 · 18px | 0 crossings | The same 12 overlaps | 0 overlaps. Centred, end tangent (2.5, 0) |
Live resize. I resized one page on the head from 1440 → 700 → 390 → 1440px. At every step, all 11 paths are byte-identical to a fresh load at that width, with 0 crossings.
Harness notes:
- I discarded my first 1440px pass. The cockpit's graph viewport loads at different widths depending on inspector state (1132px or 628px), which moves every path by a constant offset. Each byte comparison above was made at the same viewport width. The narrow tiers are not affected.
- Screenshots at ≤700px were taken with the inspector drawer closed. The auto-selected step opens the drawer over the graph, so I dismissed it with Escape and checked with
elementFromPointthat the graph was on top. The geometry is read from the DOM either way.
3. Tests, mutation checks and static checks at d91a0f7e
| Check | Result |
|---|---|
vitest run client/components/messages/PlanExecutionView client/components/workflow |
✅ 9 files, 90 tests (88 at 5f70a13, plus the 2 new ones) |
Full web-shell vitest run |
✅ 311 files, 7694 tests |
tsc -p packages/web-shell/tsconfig.json --noEmit |
✅ 0 errors |
eslint --max-warnings 0 on the 2 changed files · prettier --check on all 13 PR files |
✅ · ✅ |
Each mutant below is a small, deliberate break in the new router (or, for M20, the agent count), run against PlanExecutionView.test.tsx; "killed" means the tests caught it. The source was restored byte-for-byte after each run.
| Mutant | Result |
|---|---|
| M22: both shoulders back to a fixed 24px (the round-2 defect) | Killed: 18px drop column: expected 196 to be less than 186 |
| M28: only the drop keeps 24px · M29: only the rise keeps 24px | Killed · killed |
| M25: drop measured against its own layer · M26: rise measured against its own layer | Killed · killed |
| M23: corner not halved · M27: corner back to the 6px constant | Killed (end tangent x: expected 0 …) · killed (… -1 …) |
M20: toolUseId dedup dropped (it survived in round 2) |
Killed by counts an agent seen as both a live task and a transcript sub-tool once |
M24: run not halved (min(24, run)) |
Survives (43/43 pass) |
M24 is minor but real. Without the halving, at 18px the vertical lands at nextLayer.left − 4, right on the tips of the adjacent edges' arrowheads. At 32px it lands 4px from the next step, which is the previous head's 700px geometry. The test only asserts "strictly inside the gutter", so nothing checks the centring the code comment claims. To kill M24, assert on the 32px and 18px tiers that dropX is (startX + leftOf(1) - 4) / 2, and mirror that for riseX. This is optional.
4. CI and review state
- CI has not run on
d91a0f7e. Qwen Code CI, Web-shell Visuals and the PR review workflow were created at 08:50 UTC and are stillqueuedwith 0 jobs. At the time of writing, 59 runs are queued across the repository whilemain's CI keeps completing, so this looks like an Actions backlog rather than anything in the PR. The previous head5f70a13finished all green: Test (ubuntu), Lint & Static, web-shell E2E Smoke, web-shell visuals, Integration (no-AK) and Desktop Shell (ubuntu and windows).d91a0f7echanges onlyPlanExecutionView.tsxand its test, and the local runs in section 3 cover both. - It merges cleanly with current
main.mainis ateb780062, 13 commits past the merge base, and none of those commits touch the PR's 13 files. On a local merge commit of the two (d91a0f7e+eb780062), the focused suites pass 90/90, full web-shellvitestpasses 311 files and 7895 tests (the extra tests come frommain), andtscreports 0 errors. - 11 review threads are unresolved (9 not outdated). All of them are
[Suggestion]test-guard hardening items that the author deferred on purpose: R2-5, R2-6, R4-1, R5-1 to R5-6, and the skills keep-set. - The PR is still marked
CHANGES_REQUESTED. That status comes fromqwen-code-ci-botreviews on earlier heads. Rounds 1–3 fixed their Criticals and verified the fixes at runtime, so merging needs a fresh review or a dismissal.
Still open from earlier rounds
None of these block merge, and d91a0f7e does not touch any of them.
PlanExecutionView.module.css:247-250still says a 390px phone "scrolls the last ~6px". The measured overflow is 30px.- The chip row for more than 500 dependencies has no cap. Not re-measured this round.
- Focus falls to
<body>after you activate a dependency link or "Show all" (D9-3). Not re-measured this round. - Mutant M7 still survives. Not re-run this round.
Not covered
- macOS and Windows.
- The light theme, for this round's captures.
- The path through the Plan & tasks dialog.
Figures, JSON facts, logs and the probe scripts are in asserts/pr-10938/r3.
中文版
本地真实环境验证（第三轮）—— PR #10938 @ d91a0f7e
建议：等 CI 在这个 head 上真正跑绿后合并。 d91a0f7e74 满足了第二轮提出的两个合并条件，本轮没有发现新问题：
- 泳道穿越已消除。 我检查了第二轮的复现会话，以及本轮专门构造的一个更难的计划：泳道经过的层里叠放了多个步骤，另有一条边跨两层。在 700、430、390px 下，每一段竖直泳道现在都正好落在所在间隙的中线上，与步骤的穿越为 0。上一个 head 在这个更难的计划上有 12 处。桌面端路径与上一个 head 逐字节相同。
- M20 测试缺口已补上。 去掉
toolUseId去重后，新测试会失败。
还剩一处可选的收紧。泳道测试只检查每条竖线落在间隙内，没有检查它是否居中，所以变异 M24 能存活（见第 3 节）。这个 head 上的 CI 还没开始跑（见第 4 节），目前没有 CI 信号。
与第二轮相比的变化
d91a0f7e74 改了 2 个文件：
PlanExecutionView.tsx（+62/−10）。 跨层边的路由现在按每一侧实际经过的间隙推导肩宽min(24, run / 2)，并把转角半径减半，让曲线仍然放得下：min(6, dropShoulder / 2, riseShoulder / 2)。这就是第二轮候选补丁的公式，只有一处差别：相邻层没有测量到时，回退为完整的 24px 肩宽。PlanExecutionView.test.tsx（+170）。 新增两个测试。一个检查 64/32/18px 三档间隙下的泳道几何，并把桌面端路径钉成 golden 字符串。另一个检查：同时以实时任务和subTools条目出现的 agent 只计一次。
环境
本轮沿用前两轮的验证环境：Linux 上的 Node 22.22.2，浏览器是 Playwright 1.58.2 驱动的无头 Chromium。所有分支共用一个由 5c4f1de 构建的真实 qwen serve daemon。这个构建仍然有效，因为 5c4f1de 之后的每个提交都只改 web-shell 客户端文件：git diff --stat 5c4f1de..d91a0f7e 只列出 2 个文件，都在 packages/web-shell/client 下。每个分支是一个静态代理：提供自己的 bundle，把 REST 和 SSE 请求转发给这个 daemon，所以所有分支渲染的是同一批会话。
| 分支 | 端口 | bundle |
|---|---|---|
| base | 4939 | merge base bc7a186 的源文件 |
| 上一个 head | 4943 | 5f70a13 |
| lanefix | 4941 | 第二轮的候选补丁，不属于本 PR |
| head | 4944 | d91a0f7e，重新 vite build 所得 |
两个会话都通过真实的 Plan & Review 流程创建：enter_plan_mode → 带 blockedBy 的 todo_write → exit_plan_mode → Approve and execute · Full Access。
- DAG（沿用第二轮）：5 个步骤分 3 层。边
3 → 5跨过一层，它经过的那一层只有一个步骤。 - SKIP（本轮新建）：7 个步骤分 4 层，第 1 层 3 个步骤，第 2 层 2 个步骤。有三条跨层边：
1 → 5和3 → 7各跨一层，1 → 7跨两层。每个下降间隙和上升间隙都紧挨着一个有多个步骤的层。DAG 会话展示不了这种情况：第二轮就指出，它的下降段没有碰到节点，只是因为经过的那一层恰好只有一个步骤。
1. 第二轮的复现（DAG 会话）
| 视口 · 间隙 | 上一个 head 5f70a13 |
head d91a0f7e |
|---|---|---|
| 1440 · 64px | 0 处穿越 | 4 条路径与上一个 head 逐字节相同 |
| 700 · 32px | 竖线在 x=250 / 426（间隙 222–254 / 422–454） | x=238 / 438，正好是间隙中线。 末端切线 (6, 0) |
| 430 · 18px | x=251 处的上升段穿过步骤 4（重叠 106px） | x=136 / 270，居中。 0 处穿越，末端切线 (2.5, 0) |
| 390 · 18px | x=246 处的上升段穿过步骤 4（重叠 106px） | x=131 / 265，居中。 0 处穿越，末端切线 (2.5, 0) |
在 700、430、390px 下，这 4 条路径也与第二轮的候选补丁逐字节相同。相邻层边的箭头保持第二轮修复后的切线：(28, 0)、(12, 0)、(5, 0)。
2. 更难的计划：多步骤层与跨两层的边（SKIP 会话）
| 视口 · 间隙 | base | 上一个 head 5f70a13 |
head d91a0f7e |
|---|---|---|---|
| 1440 · 64px | 0 处穿越 | 0 处穿越 | 0 处穿越。11 条路径与上一个 head、候选补丁都逐字节相同 |
| 700 · 32px | 0 处穿越（任何宽度下间隙都是 64px） | 0 处穿越，但 6 条竖线都偏离中线 12px，离下一个步骤只有 4px | 6 条竖线全部居中，末端切线 (6, 0) |
| 430 · 18px | 0 处穿越 | 泳道与步骤重叠 12 处。 1→5 的下降段和上升段穿过步骤 3 和 4，3→7 穿过步骤 5 和 6，1→7 穿过步骤 3/4 和 5/6。每条竖线落在步骤内部 10px 或 106px 处，重叠最高 142px |
0 处重叠。竖线全部居中，末端切线 (2.5, 0) |
| 390 · 18px | 0 处穿越 | 同样的 12 处重叠 | 0 处重叠。居中，末端切线 (2.5, 0) |
实时缩放。 我在 head 上把同一个页面从 1440 → 700 → 390 → 1440px 依次缩放。每一步的 11 条路径都与该宽度下刷新加载的结果逐字节相同，且都没有穿越。
验证环境说明：
- 第一遍 1440px 的测量结果已作废。 cockpit 图区域的宽度取决于 inspector 的状态，每次加载可能是 1132px，也可能是 628px，这会让所有路径整体平移一个固定偏移。上面每次逐字节比对都是在相同视口宽度下做的，窄屏档位不受影响。
- ≤700px 的截图是在关闭 inspector 抽屉后拍的。 自动选中的步骤会弹出抽屉盖住图，所以我先按 Escape 关掉，再用
elementFromPoint确认图在最上层。几何数据是从 DOM 读取的，不受遮挡影响。
3. d91a0f7e 上的测试、变异与静态检查
| 检查 | 结果 |
|---|---|
vitest run client/components/messages/PlanExecutionView client/components/workflow |
✅ 9 个文件、90 个用例（5f70a13 上是 88 个，加上 2 个新用例） |
web-shell 全量 vitest run |
✅ 311 个文件、7694 个用例 |
tsc -p packages/web-shell/tsconfig.json --noEmit |
✅ 0 个错误 |
对 2 个改动文件跑 eslint --max-warnings 0 · 对 PR 全部 13 个文件跑 prettier --check |
✅ · ✅ |
下表每个变异体都是对新路由（M20 则是 agent 计数）做的一处小而刻意的破坏，再跑 PlanExecutionView.test.tsx；“被抓到”表示测试能发现这处破坏。每次跑完都把源文件逐字节还原。
| 变异 | 结果 |
|---|---|
| M22：两侧肩宽都退回固定 24px（即第二轮的缺陷） | 被抓到：18px drop column: expected 196 to be less than 186 |
| M28：只有下降段保留 24px · M29：只有上升段保留 24px | 被抓到 · 被抓到 |
| M25：下降段按自身所在层测量 · M26：上升段按自身所在层测量 | 被抓到 · 被抓到 |
| M23：转角不减半 · M27：转角退回 6px 常量 | 被抓到（end tangent x: expected 0 …）· 被抓到（… -1 …） |
M20：去掉 toolUseId 去重（第二轮存活） |
被新测试 counts an agent seen as both a live task and a transcript sub-tool once 抓到 |
M24：跨度不减半（min(24, run)） |
存活（43/43 通过） |
M24 影响很小，但确实存在。不减半时，18px 间隙下竖线落在 nextLayer.left − 4，正好压在相邻边箭头的尖端上；32px 间隙下竖线离下一个步骤只有 4px，也就是上一个 head 在 700px 下的几何。测试只断言“严格位于间隙内”，所以代码注释声称的居中没有被任何测试检查。要抓到 M24，可以在 32px 和 18px 两档上断言 dropX 等于 (startX + leftOf(1) - 4) / 2，riseX 同理。这一项可选。
4. CI 与评审状态
d91a0f7e上的 CI 还没跑。 Qwen Code CI、Web-shell Visuals 和 PR review 工作流都在 08:50 UTC 创建，至今仍是queued，一个 job 都没有。撰写本报告时，整个仓库有 59 个 run 在排队，而main的 CI 一直在正常完成，所以这看起来是 Actions 积压，与 PR 本身无关。上一个 head5f70a13的 CI 全绿：Test (ubuntu)、Lint & Static、web-shell E2E Smoke、web-shell visuals、Integration (no-AK)、Desktop Shell（ubuntu 和 windows）。d91a0f7e只改了PlanExecutionView.tsx及其测试，第 3 节的本地运行覆盖了这两个文件。- 与当前
main可以无冲突合并。main现在是eb780062，比 merge base 多 13 个提交，这些提交都没有改动本 PR 的 13 个文件。在本地把两者（d91a0f7e+eb780062）合成一个合并提交后：聚焦测试 90/90 通过，web-shell 全量vitest311 个文件、7895 个用例通过（多出的用例来自main），tsc0 个错误。 - 还有 11 个评审 thread 未解决（其中 9 个未过期）。全部是作者有意推迟的
[Suggestion]类测试守卫加固：R2-5、R2-6、R4-1、R5-1 至 R5-6，以及 skills keep-set。 - PR 仍标记为
CHANGES_REQUESTED。 这个状态来自qwen-code-ci-bot在更早 head 上的评审。那些 Critical 已在第 1–3 轮修复，并在运行时验证过，所以合并前需要一次新的评审，或者 dismiss 旧评审。
前几轮遗留项
这些都不阻断合并，d91a0f7e 也没有改动其中任何一项。
PlanExecutionView.module.css:247-250的注释仍写着 390px 手机“最后约 6px 需要滚动”，实测溢出是 30px。- 依赖超过 500 条时，chip 行没有上限。本轮未重测。
- 激活依赖链接或 “Show all” 后，焦点掉到
<body>（D9-3）。本轮未重测。 - 变异 M7 仍然存活。本轮未重跑。
未覆盖
- macOS 和 Windows。
- 浅色主题（本轮的截图）。
- 经由 Plan & tasks 对话框的路径。
图、JSON 数据、日志和探针脚本都在 asserts/pr-10938/r3。
The Qwen Code CI, Web-shell Visuals and review-lane runs created for d91a0f7 at 2026-09-13T08:50:29Z never scheduled a single job: a repo-wide Actions queue jam in that window left 59 runs stuck at status=queued with zero jobs, and both cancel and rerun are refused on them (cancel reports 'completed', rerun reports 'already running'). Runs created after ~09:30Z schedule normally, so these are dead rather than slow. No content change. This empty commit only re-fires pull_request:synchronize so the lanes actually run on the exact tree wenshao validated in round 3 ('merge once CI has actually run green on this head'). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtzq8wyy28
|
CI 补跑说明：head 从 @wenshao 你第三轮给的合并条件是「等 CI 在这个 head 上真的跑绿」。 因此推了一个空提交重新触发 当前状态（head
11 条 unresolved 全是 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (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: 94 passed · 0 failed · 94 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：94 通过 · 0 失败 · 94 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10938 — deep verificationVerdict: Verified head: 中文摘要结论： A/B 结论：把同一份 harness（两棵树中 sha256 完全一致）分别跑在 head 与 边几何单独做了一组 A/B（ 变异矩阵（ 发现：均为 Suggestion 级，无阻塞项。最主要的一条是归因问题——节点面 chip 的去重与自引用过滤这两个守卫，只被一个为别的目的写的测试（ 未覆盖范围：32 个提交中本地只可达 1 个（shallow checkout），无法做逐提交归因；容器内没有 Playwright 浏览器，改动过的 visuals spec 未能真实执行；所有几何数字来自按断点合成的 rect 喂给真实 router，不是浏览器排版结果，因此状态竖条的对比度（作者自己说"合并前不会省的验证"）本轮未测；未跑仓库级 lint/build 与其他包的测试。 Central claim and A/BCentral claim. A step's dependency references stop being raw comma-joined Todo ids and become navigable controls labelled Secondary claims. (A) The node face leads with number + title, carries agent count and summed runtime, keeps status in the accessibility tree as words, and states the dependency exactly once through one of two mutually exclusive channels. (B) The lane reservation is bound to the constant that places lanes, published from TS with no duplicated literal and no One harness, byte-identical in both trees (
head 19/19 passed · base 0/19 passed · load-bearing 19/19 · 38 scored assertions, 0 fail. The base-arm reds are behavioural mismatches at the intended assertions, not import or compile failures — the harness rendered on both arms. Representative messages: Edge geometry A/BThe shoulder changes are the only computational change in the diff, and the PR body rests a safety argument on them ("the desktop curve and its pinned path … are unchanged"). The router reads real measured rects, so the harness feeds it the rects each breakpoint's CSS produces (lane 240/gutter 64, 168/32, 116/18, canvas padding 10 → 6px) and dumps the emitted
32/32 assertions, 0 fail. The measured numbers reproduce the PR's own arithmetic exactly (a 24px run at ≤720px, a 10px run at ≤480px, Mutation matrix — are the PR's new tests load-bearing?Each mutant reverts one guard in a scratch worktree at the merge commit and runs the PR's own changed suites (both
9/9 killed, 0 survivors. The M0 control is landed in the same collection as every mutant and turns tests red, so "no survivors" is a measurement, not an unexercised suite. Every run restored its file byte-for-byte ( Lane pitch — single source of truthScored as scripted checks over the shipped files: the reservation reads Step numbers agree across surfacesThe graph numbers steps from its own CorrectionsThese are corrections to the PR description's factual statements, not requests to change code.
FindingsNo blocking findings. Two Suggestion-level observations, both about completeness of the test net rather than behaviour. S1 — the node-face dedup and self-drop are pinned only incidentally
The contrast is visible in the same matrix: the inspector's equivalent guards have dedicated tests ( S2 — the lane-pitch binding is asserted from source text, and the consumed padding is not asserted at allM4 was killed by a stylesheet guard that matches the TSX source against Scarier consequences tested, which do NOT holdReporting these because each was a plausible blocker and each is bounded evidence rather than reassurance.
Not covered
MethodologyCI verify job, Control validity was asserted, not assumed. The base arm is a Drivers Gates, reported separately and not double-counted in
Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
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. ✅
|
Released in v0.23.4. |
6 participants
What this PR does
Closes the navigation, shape and documentation gaps left in the Session Workflow surface after #8583, and takes a design pass over the plan DAG and the inspector chrome.
The plan DAG leads with the step, not its status. Read against the three questions a plan graph is opened with — what is running, what is blocked on what, what needs me — the node answered none of them first.
Content leads. The node led with a glyph, the raw Todo id, then the status word; the only element saying what the step is came third. The id leaves the node face entirely — it is an address, so it stays in the detail panel where it is copied — and the step number takes its place, because the inspector list and the dependency chips already address steps by number and the three surfaces have to agree.
Status is stated once, as a left rule, and only when it is worth saying. Waiting is the resting state of most nodes in a real plan and carries no rule; the stylesheet already made this argument for
blocked, and extending it consistently is what stops the canvas reading as a wall of tinted boxes and gives the one running node something to stand out against. Attention folds into the same rule and outranks the status tone, since it is the state the reader is asked to act on. Status is colour, which assistive tech cannot read, so it stays in the accessibility tree as words.Time becomes visible. The face carries the agent count and the summed agent runtime for that step — the "is this alive or hung" signal, which is the first thing asked of a live run.
Dependencies are stated once. The
Depends on …row renders only when no edge states it: aboveMAX_RENDERED_PLAN_EDGESthe graph draws no edges at all, and there the row is the dependency's only statement, so deleting it outright would have lost information in exactly the case that needs it most. The input port dot goes with it — the left edge now carries the status rule, and an incoming edge already terminates in an arrowhead, which is the input marker. The output port stays. Both ports also drop--agent-blue-500: every node with an edge had one, so a permanently blue dot across most of the graph left selection no colour of its own.Edge routing, the layering algorithm, the return lanes and the measure pass are untouched. This is the node face and the colour rule. The design plan, including the two revisions the review pass forced, is in
docs/plans/2026-09-04-session-workflow-dag-design.md.Dependencies become navigation. A step's dependencies rendered as
todo.blockedBy!.join(', ')— raw ids, in the inspector detail and in the graph's own step-details panel. An id is an address, so following an edge meant readingstep-3, step-7and then finding those rows by eye, in a panel whose entire subject is which step waits on which. Both places now render each reference as the step it points at — number and title — as a control that selects it. The chips on the node face are the one exception and stay read-only: that row lives inside the node's own<button>, and a nested button is invalid HTML.Recent activity stops contradicting itself. The list sliced to 6 while the count beside it reported the true total, so the remainder was unreachable. The cap stays as a preview and gains a control that opens the rest.
The lane reservation is bound to the constant that places lanes.
.dagCanvasreserved bottom padding for layer-spanning return lanes with a literal9pxwhile TS placed them atlane * EDGE_LANE_HEIGHT. Raising the constant alone left the reservation short; the clamp then pinned the outer lanes together and spanning edges overlapped again — the defect the lanes exist to fix — with no test signal, because the lane test mocksoffsetHeight. TS now publishes the pitch and the stylesheet reads it, with no second copy as avar()fallback.The graph narrows at two viewport steps. Three 240px lanes and two 64px gutters are ~850px, so the canvas scrolled in both axes at once and the horizontal scroll hid the layer the vertical scroll was looking for. Below 720px the lane and gutter narrow, and below 480px they narrow again, which gets a three-layer plan under a phone viewport. Deeper plans still scroll horizontally — that is the honest limit of narrowing alone, and reusing the flat layout stays with #10866. Layer semantics and the rect-measured edge routing are untouched.
The auto-sized dialog keeps its gutter.No longer in this diff. TheDialogShellgutter fix reachedmainseparately via #11077, so the current 13-file diff touches no dialog code. Struck rather than deleted because the review rounds below still refer to it.The inspector chrome states each thing once. Section labels were tracked-out uppercase that shouted over the content they introduced; the cockpit spent the selection accent on a fixed eyebrow, making the one blue thing on the page the one thing that never changes; completed-over-total sat in a tinted pill directly above the progress bar drawing the same ratio and beside the status word naming the same state; and four meta strings were glued with middle dots, wrapping as a unit and reading out as one run of text. No token or theme values change — this is the existing system applied more sparingly.
todo_write's ungated dependency semantics are documented.todo_writecommit (ff4b4907) is in neither this branch's diff normain, so nothing here documents those semantics. The rationale survives in the review history; the diff no longer carries it.Round-1 review fixes are folded in: the show-all control's specificity collision, the step-details panel's raw ids, the overstated narrow-viewport claim and its test name, the duplicated lane-pitch fallback, and a comment antecedent. Details under Risk & Scope.
Why it's needed
These are the still-open items from #8583's review that are behaviour, shape or documentation rather than contract decisions, plus #10866.
Verified against
mainfirst, and deliberately not re-litigated here: R1-11, R1-21, R1-22, R1-25, R1-26, R1-28, R1-36, R5-5 and the sibling-fan-out ordering are already fixed there. R1-41 is moot — the skills chip it covered no longer exists on this surface.Reviewer Test Plan
How to verify
blockedBydependencies. Expect each graph node to read<number> <title>with the agent count and elapsed beneath, and a left rule only when running/completed/attention. The visibleDepends onrow is off while edges are drawn, with two corrections: the cockpit (showStepDetails={false}) does render the chips by design, and in the interactive graph the dependency is still stated to assistive tech by an sr-only summary on the node button (4860e0a7e6), because the drawn edges arearia-hidden.Depends onandUnblocksto render each reference as<number> <title>, and clicking one to select that step. Expect the same in the inspector detail.MAX_RENDERED_PLAN_EDGES(500 dependencies). Expect no edges, the existing "Too many dependencies to draw" notice, and theDepends onchips back on each node — that is the case where the row is the only statement of the dependency.mainvia fix(web-shell): keep the auto-sized dialog's gutter on a phone #11077, not in this diff.Evidence (Before & After)
The ASCII before/after under "What this PR does" is the node-face change. CI's
Capture web-shell visualsjob rendersworkflow-page-*in both themes on every push and posts the composites to this PR; note that suite does not cover the inspector panel, so the inspector changes have no rendered evidence here.Local runs are focused suites only, Node 22, linux:
client/components/messages/PlanExecutionView(+.css.test.ts) — 45/45client/components/workflow(+client/components/workflows/WorkflowRunsPage) — 39/39; the combinedvitest run client/components/messages/PlanExecutionView client/components/workflowis 9 files, 84 testsDialogShellandpackages/sdk-typescript/test/unit/daemonUi.test.tsare no longer run for this PR: neither is in the diff (the gutter fix is onmainvia fix(web-shell): keep the auto-sized dialog's gutter on a phone #11077; thedaemonUi.test.tskeep-set commit3863bb68is in neither the diff normain)mainmerge in5c4f1de9e8; CI on the merged ref is the authorityEvery behavioural change is mutation-checked (red on revert, green restored): the joined-id dependency lists, the silent activity cap, ungating the node's dependency row, deleting the node's accessible status word, dropping the elapsed, removing the step-details link handler, the
endY→routeYpath-tail substitution, and removing'skills'from the compactor keep-set each fail exactly their own assertion. One probe initially survived —Runningalso appears in an execution row inside the node — and the assertion was tightened to a node with no linked agent before it killed the mutant.Three geometry tests identified a node by its first
<span>, which was the Todo id and is now the accessible status word; they bind todata-plan-node-idinstead, which is the stable hook and was always the better one.Tested on
Environment (optional)
Unit tests only; no local build or whole-package typecheck was run, and no browser is available in that environment — CI owns both.
Risk & Scope
.showAllActivitywas a single class (0,1,0) losing to.activityList > button(0,1,1), which auto-placed its label into the 52px first column of the activity grid. It is fixed with the.activityList >prefix plus explicitdisplay/grid-template-columnsresets, and both it and the earlier.metricscollision are now pinned by the stylesheet guard so neither can regress to a bare class. A real render before merge is still the verification I would not skip, particularly the status rule's contrast in dark mode.showStepDetailsand keyboard-traversal items, which are not in this diff — henceRefsrather thanCloses; and the deferred test-coverage items R1-13, R1-24, R1-29 and R1-43.DialogShellgutter fix is no longer part of this diff — it is onmainvia fix(web-shell): keep the auto-sized dialog's gutter on a phone #11077. The node face changes what is displayed, not what is measured. Edge geometry is changed now, by the R6-3 fix in4860e0a7e6: the adjacent-layer router's shoulder went fromMath.max(24, run / 2)torun / 2, which is identical for every gutter wide enough to afford the 24px floor (so the desktop curve and its pinned pathM 214 50 C 255 50, 255 50, 296 50are unchanged) and otherwise shortens a shoulder the narrow tiers could not afford. Thetodo_writecommit is no longer in this diff either.Linked Issues
Refs #8583, #10866.
中文说明
这个 PR 做了什么
补上 #8583 之后 Session Workflow 面上遗留的导航、形态和文档缺口，并对 plan DAG 与 inspector 的界面做一次设计收敛。
plan DAG 改成以步骤开头，而不是以状态开头。 对着打开一张 plan 图时要回答的三个问题——什么在跑、谁卡在谁上面、哪个需要我处理——节点一个都没有优先回答。
内容优先。原来节点先出状态字形、再出裸 Todo id、再出状态词，唯一说明这一步是什么的元素排在第三。id 完全离开节点面——它是地址，所以留在你真正会去复制它的详情面板里——由步骤号取而代之，因为 inspector 列表和依赖 chip 本来就按号寻址，三个面必须一致。
状态只说一次，用左侧竖条承载，而且只在值得说的时候说。等待是真实 plan 中大多数节点的静息态，不画竖条；样式表本来就为
blocked写过这个论证，把它一致地推广开，正是让画布不再像一堵染色方块墙、让唯一在跑的那个节点有东西可以反衬的前提。attention 并进同一根竖条并压过状态色，因为它才是要读者动手的那个状态。状态是颜色，读屏读不到，所以状态词保留在无障碍树里。时间变得可见。节点面上现在带该步骤的 agent 数和 agent 运行时长合计——也就是"这东西还活着还是挂了"的信号，那是看实时运行时的第一个问题。
依赖只说一次。
Depends on …那一行只在没有边表述它时才渲染：依赖数超过MAX_RENDERED_PLAN_EDGES时图根本不画边，那时这一行是依赖的唯一表述，直接删掉恰恰会在最需要它的场景丢失信息。输入端口圆点随之去掉——左边现在是状态竖条，而入边本来就以箭头收尾，那个箭头就是输入标记；输出端口保留。两个端口点也都不再用--agent-blue-500：之前每个有边的节点都挂一个，于是图上大部分地方常驻一个蓝点，选中色等于没有自己的颜色。边路由、分层算法、回程泳道和测量 pass 都没有改动。本次改的是节点面和颜色规则。design plan（含评审那一遍强制改掉的两处）在
docs/plans/2026-09-04-session-workflow-dag-design.md。依赖变成导航。 一个步骤的依赖原本渲染为
todo.blockedBy!.join(', ')——裸 id，出现在 inspector 详情和图自己的 step-details 面板里。id 是地址，所以顺着一条边走意味着先读step-3, step-7，再用眼睛去找那些行，而这个面板的全部主题恰恰就是谁在等谁。这两处现在都把每个引用渲染成它指向的那个步骤——号码加标题——并作为点击即选中的控件。节点面上的 chip 是唯一例外，保持只读：那一行位于节点自己的<button>内部，嵌套 button 是非法 HTML。最近活动不再自相矛盾。 列表截断到 6 条，而旁边的计数报的是真实总数，于是其余部分无法触达。上限保留为预览，并新增一个展开其余部分的控件。
泳道预留绑定到放置泳道的那个常量。
.dagCanvas用字面量9px为跨层回程泳道预留底部内边距，而 TS 按lane * EDGE_LANE_HEIGHT放置它们。只调高常量会让预留不足，随后 clamp 把外侧泳道压在一起、跨层边再次重叠——正是泳道存在要解决的那个缺陷——而且没有任何测试信号，因为泳道测试直接 mock 了offsetHeight。现在由 TS 下发间距、样式表读取，且不再以var()兜底的形式留第二份拷贝。图在两级视口断点收窄。 三条 240px 泳道加两个 64px 间隙约 850px，于是画布同时在两个轴上滚动，横向滚动把纵向滚动正在找的那一层挡住了。720px 以下泳道和间隙收窄，480px 以下再收窄一次，使三层的 plan 能放进手机视口。更深的 plan 仍会横向滚动——这是单靠收窄能达到的诚实上限，改用平铺布局仍归 #10866。分层语义和基于实测矩形的边路由都未改动。
自适应尺寸的对话框保住了它的边距。已不在本 diff 中。DialogShell的边距修复已经通过 #11077 单独进入main，所以当前 13 个文件的 diff 不涉及任何对话框代码。之所以划掉而不是直接删除，是因为下面的评审轮次仍然引用它。inspector 的界面元素每件事只说一次。 区块标签原本是加了字距的全大写，盖过它所引出的内容；cockpit 把选中强调色花在一个固定 eyebrow 上，使页面上唯一的蓝色成了唯一永不变化的东西；完成数/总数装在一枚染色药丸里，而正下方的进度条画的是同一个比例、左边的状态词说的是同一个状态；还有四处 meta 串用中点粘成一体，会作为整体折行、并被读屏当成一串连续文本读出。没有改动任何 token 或主题值——这是把现有系统用得更克制。
记录已不在本 diff 中。 那次纯注释的todo_write依赖语义不受开关控制的原因。todo_write提交（ff4b4907）既不在本分支的 diff 里，也不在main上，所以这里没有任何内容在记录那些语义。论证过程保留在评审历史里，diff 不再携带它。第一轮评审的修复已并入：show-all 控件的特异度冲突、step-details 面板的裸 id、说过头的窄视口结论及其测试名、重复的泳道间距兜底，以及一处注释指代。细节见 Risk & Scope。
为什么需要
这些是 #8583 评审中仍然开着、且属于行为、形态或文档而非契约决策的条目，加上 #10866。
先对着
main核对过、并刻意不在此重做的：R1-11、R1-21、R1-22、R1-25、R1-26、R1-28、R1-36、R5-5 以及 sibling 扇出的顺序问题，均已在 main 上修复。R1-41 已失效——它覆盖的 skills chip 在这个面上已不存在。评审者验证计划
如何验证
blockedBy依赖、已批准 plan 的 session。每个图节点应读作<号码> <标题>，下方是 agent 数与耗时；只有 running/completed/attention 才有左侧竖条。画了边时可见的Depends on行是关掉的，但有两处更正：cockpit（showStepDetails={false}）按设计确实会渲染 chip；而在交互图里，依赖仍然通过节点按钮上的 sr-only 摘要表述给辅助技术（4860e0a7e6），因为画出来的边是aria-hidden。Depends on和Unblocks的每个引用应渲染为<号码> <标题>，点击应选中该步骤。inspector 详情中同理。MAX_RENDERED_PLAN_EDGES（500 条依赖）的 plan。应看不到边、看到既有的 "Too many dependencies to draw" 提示，并且每个节点上的Depends onchip 回来了——那正是这一行是依赖唯一表述的场景。main上（fix(web-shell): keep the auto-sized dialog's gutter on a phone #11077），不在本 diff 中。证据（改前 / 改后）
"这个 PR 做了什么"下方的 ASCII 对比即节点面的改动。CI 的
Capture web-shell visualsjob 每次推送都会在深浅两个主题下渲染workflow-page-*并把对比图贴到本 PR；注意该套件不覆盖 inspector 面板，所以 inspector 的改动在此没有渲染证据。本地只跑了聚焦套件，Node 22，linux：
client/components/messages/PlanExecutionView（含.css.test.ts）—— 45/45client/components/workflow（含client/components/workflows/WorkflowRunsPage）—— 39/39；合并跑vitest run client/components/messages/PlanExecutionView client/components/workflow为 9 个文件、84 个用例DialogShell和packages/sdk-typescript/test/unit/daemonUi.test.ts：两者都不在 diff 里（边距修复已在main，见 fix(web-shell): keep the auto-sized dialog's gutter on a phone #11077；daemonUi.test.ts的 keep-set 提交3863bb68既不在 diff 也不在main）5c4f1de9e8合并main之前测的；合并后的状态以 CI 为准每一处行为改动都做了变异验证（回滚即红、恢复即绿）：拼接式 id 依赖列表、静默的活动上限、解除节点依赖行的 gate、删除节点的无障碍状态词、去掉耗时、移除 step-details 的点击处理、把路径尾部的
endY换成routeY、以及从 compactor keep-set 中移除'skills'，各自精确打红对应断言。有一个探针最初存活——Running在节点内的执行行里也会出现——把断言收紧到一个没有关联 agent 的节点之后才杀死该变异体。三个几何测试原本靠节点里第一个
<span>来识别节点，那时是 Todo id，现在是无障碍状态词；它们改绑到data-plan-node-id，那本来就是更稳的钩子。测试环境
运行环境（可选）
仅单元测试；没有在本地跑构建或整包 typecheck，该环境也没有浏览器——这两项由 CI 负责。
风险与范围
.showAllActivity是单类 (0,1,0)，输给.activityList > button(0,1,1)，导致它的标签被自动放进活动网格 52px 的第一列。现已通过加.activityList >前缀并显式重置display/grid-template-columns修复，并且它和先前的.metrics冲突都已被样式表守卫钉住，两者都不能退回裸类。合并前做一次真实渲染仍是我不会省的验证，尤其是状态竖条在深色模式下的对比度。showStepDetails与键盘遍历两项不在本 diff 内——因此用Refs而非Closes；以及延期的测试覆盖项 R1-13、R1-24、R1-29、R1-43。DialogShell的边距修复已不属于本 diff —— 它在main上（fix(web-shell): keep the auto-sized dialog's gutter on a phone #11077）。节点面改变的是显示内容而非测量内容。边的几何现在确实改了，来自4860e0a7e6里的 R6-3 修复：相邻层路由的肩宽从Math.max(24, run / 2)改为run / 2；对任何容得下 24px 下限的间隙两者完全等价（所以桌面端曲线及其钉住的路径M 214 50 C 255 50, 255 50, 296 50未变），其余情况只是把窄断点容不下的肩宽缩短。todo_write那次提交也已不在本 diff 中。关联 Issue
Refs #8583, #10866.