feat(web-shell): visualize and manage dynamic workflow runs - #8950
feat(web-shell): visualize and manage dynamic workflow runs#8950qqqys wants to merge 11 commits into
Conversation
E2E validation reportValidated on macOS with isolated local daemon and Vite ports after the final review fixes.
Focused regression suites also passed after rebasing onto the current |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 17 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
Follow-up from Web Shell dogfooding: clicking a transcript Workflow tool row now expands the existing execution graph inline instead of showing the generic tool card.\n\nThe live path links the parent tool call to its workflow run before output is available, while older transcripts fall back to their recorded run ID. The same behavior is available in the main conversation and split panes. The collapsed row stays lightweight and inaccessible content is not pre-mounted.\n\nValidation:\n- Real foreground run: the expanded row showed the Inspect phase and running agent while execution was still active, then retained the completed multi-phase graph with dependency edges.\n- Web Shell: 571 focused/regression tests passed, including App and ChatPane.\n- Core/CLI workflow tests: 71 passed.\n- Core, ACP Bridge, SDK, CLI, and Web Shell typechecks passed.\n- Focused Prettier and ESLint checks passed.\n- Focused UI re-review found no remaining reproducible P0-P2 findings. |
E2E follow-up: inline transcript graph\n\nPASS on an isolated workflow-enabled daemon and fresh Web Shell.\n\n- While a foreground workflow was still running, clicking its transcript row changed the row to expanded and immediately rendered the workflow summary, current Inspect phase, and the running agent.\n- The task snapshot's parent tool identity exactly matched the transcript tool call.\n- After completion, reopening the same row retained the graph with 2/2 agents, Inspect and Verify phases, and the Verify agent displayed its dependency on the Inspect agent.\n- Reloading an earlier completed turn also restored its inline graph.\n- A fresh page reported no console errors.\n- Both probe histories, the temporary session, browser tabs, and isolated processes were removed after verification. |
|
已按最新 UI 反馈完成第二轮收敛:
验证证据:
提交:5c63e2f0b003029369abe2a2559695eb162b1923 |
|
继续按 UI 反馈优化执行图:
验证:
提交:f8316eb97d7bc31745e936c9f275f6c3240f92d4 |
|
Thanks for the PR! Template looks good ✓ Problem: Observed gap, not theoretical. The linked issue #8941 documents it: the Dynamic Workflow runtime already exists in core, but Web Shell only surfaces runs as terminal text — phase/agent progress, run controls, and history are invisible there. The issue is triaged P2 and sits on the roadmap/multi-agent and roadmap/background-automation tracks. It is self-reported by the PR author, but the maintainer-applied labels give the direction a real signal. Direction: Aligned. Dynamic Workflow is an active investment area — the reference product's CHANGELOG shows sustained work on workflow surfaces (agent grid, size guideline, status line), and this PR keeps the feature default-off, so users who haven't opted in see no behavior change. Size: Large. ~5,293 production logic lines (51 files) + ~4,304 test lines (30 files), spanning core, cli (serve + acp-integration), sdk-typescript, acp-bridge, webui, and web-shell. As a Approach: The core shape — advertising run state through the daemon task contract and rendering it in Web Shell — is the right one. The honest question is scope: if you cut 80%, would the remaining 20% (read-only live execution view + pause/stop) already cover most of the need? The history page with compare/export/delete, the saved-workflow listing, and the session-less capability advertisement each add substantial surface on top. Worth thinking about whether those are follow-ups rather than one PR — not a blocker, but a genuine question before deep review. Risk: High-risk paths from the revert-history analysis are touched: Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的缺口,不是理论问题。关联 issue #8941 说明了这一点:Dynamic Workflow 运行时已存在于 core,但 Web Shell 只能以终端文本展示运行——阶段/agent 进度、运行控制、历史记录都不可见。该 issue 已被定为 P2,属于 roadmap/multi-agent 和 roadmap/background-automation 方向。issue 由 PR 作者自报,但维护者打的标签说明方向本身是被认可的。 方向:对齐。Dynamic Workflow 是持续投入的方向——参考产品的 CHANGELOG 显示其 workflow 界面在持续演进(agent grid、size guideline、status line),且本 PR 保持功能默认关闭,未开启的用户行为不变。 规模:较大。约 5,293 行生产逻辑(51 个文件)+ 约 4,304 行测试(30 个文件),横跨 core、cli(serve + acp-integration)、sdk-typescript、acp-bridge、webui、web-shell。作为 方案:核心思路——通过 daemon task contract 暴露运行状态并在 Web Shell 渲染——是对的。真正的问题在范围:如果砍掉 80%,剩下的 20%(只读实时执行视图 + 暂停/停止)是否已覆盖大部分需求?带比较/导出/删除的历史页、已保存工作流列表、无 session 的能力声明,每一项都叠加了大量界面。值得考虑这些是否应该作为后续 PR,而不是合在一个 PR 里——不是拦截项,但值得在深入审查前认真想一下。 风险:触及 revert 历史分析得出的高风险路径: 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI formed a baseline before reading the diff: extend the existing background-task contract with an opt-in workflow payload, advertise enablement per workspace so the session-less welcome page can decide, one execution view with pause/stop controls, and minimal persisted history. The PR's shape matches that baseline — Blocker — CI is red on this commit, and every failure traces to this PR.
Non-blocking notes:
What's genuinely well done: history deletion is segment-validated and list-driven (no path escape), snapshot loading validates shape instead of trusting JSON, trace labels/prompts/errors are ANSI-stripped and length-capped, the UI guards every async handler against session-switch staleness ( 中文说明(代码审查)先独立构思了基线方案再读 diff:在现有后台任务契约上加可选的 workflow 负载、按 workspace 声明能力让无 session 欢迎页可判断、一个带暂停/停止控制的执行视图、以及最小化的历史持久化。PR 的结构与基线一致—— 拦截项——本 commit 的 CI 是红的,且所有失败都源于本 PR。
非拦截建议:
做得好的地方:历史删除做了段校验且基于列表驱动(无路径逃逸)、快照加载校验结构而非盲信 JSON、trace 的 label/prompt/error 都做了 ANSI 清理和长度上限、UI 每个异步处理器都防了切换 session 的过期响应(每次 await 后重查 Control flowsequenceDiagram
participant P1 as Web Shell UI
participant P2 as serve routes
participant P3 as HttpAcpBridge
participant P4 as AcpDispatcher
participant P5 as QwenAgent
participant P6 as WorkflowRunRegistry
participant P7 as WorkflowTool
P1->>P2: POST session tasks taskId workflow-action
P2->>P3: controlSessionWorkflowTask with trusted client context
P3->>P4: ext-method workflow-action over ACP
P4->>P5: withMutableOwned session guard
P5->>P6: pause or resume by runId
P5->>P7: retry rerun run-saved start background run
P7->>P6: register new run with lineage
P6-->>P1: status change flows into tasks snapshot
中文说明(流程图)控制流:Web Shell 发起 workflow-action 请求 → serve 路由校验所有权 → HttpAcpBridge 解析可信 clientId 并经 ACP ext-method 转发 → AcpDispatcher 在 mutable-owned 会话保护下交给 QwenAgent:pause/resume 直接作用于 WorkflowRunRegistry;retry/rerun/run-saved 则通过 WorkflowTool 后台启动新运行并登记 lineage;registry 的状态变更最终回流到任务快照驱动 UI 刷新。 Files changed (30 of 81 shown)
Testing evidenceThis is an unattended CI run — I do not build or execute PR code; the evidence below is the PR's own CI on the reviewed commit, read via the API. Log text quoted from the failing job is stdout from the PR's own run, cited as evidence of the failure, not as claims about its cause (I classified each failure from the diff). The failing excerpts: The other green lanes are real but narrower than they sound:
The central claim here is behavioural — live execution graph, run controls, history persistence across sessions — and a green suite would not settle it on its own; a red one settles it against. Sandboxed verification would settle the rest once CI is green: Not verified: interactive workflow behaviour (no lane run yet), Windows/Linux rendering, and the settings-vs-env capability mismatch flagged above. The PR's "Tested on" table is the author's claim, not evidence this pass re-ran. 中文说明(测试证据)这是无人值守 CI 运行——不会构建或执行 PR 代码;以上证据是通过 API 读取的该 commit 自身 CI 结果。失败日志引文仅作为失败证据,失败归因是根据 diff 判定的。 要点: 核心主张是行为性的——实时执行图、运行控制、跨 session 历史持久化——套件绿了也不能单独证明,红了则直接否定。CI 绿后可用沙箱验证收尾:由于作者是 fork 贡献者, 未验证:交互式 workflow 行为、Windows/Linux 渲染、上文指出的设置与环境变量能力不一致。PR 的"已测试平台"表是作者声明,不是本次审查重跑的证据。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the architecture is right and the craftsmanship is mostly good, but the suite is red on this commit with 18 PR-caused failures, so it cannot merge as-is. Stepping back: this is a well-motivated feature — the workflow runtime exists, the roadmap labels on the linked issue say the direction is wanted, and the PR's structure (opt-in task contract, per-workspace capability advertisement, session-owned controls) is what I would have proposed independently. Where it loses me is the landing. A change that wires itself into the Session constructor unconditionally and breaks three suites it never touched, registers a route without completing the telemetry audit that exists precisely to catch that, and misses one shape assertion in the very file it edited — that's a final integration pass that didn't happen against the full suite. None of the three root causes is architectural; all are mechanical fixes. That's why this is a request-changes, not a rethink-the-PR. The scope question from Stage 1 still stands, separately from the blockers: live visualization + controls is the core, and the history page with compare/export/delete plus the saved-workflow listing could plausibly be follow-ups. I'd rather see this merge in two reviewable pieces than one 5,300-production-line one — but that's a judgment for the maintainer and the author, not a gate I'll enforce. Standing flags for whoever picks this up next: this is a Stage 0 escalation (cross-package, ~5,293 production lines, touches the revert-correlated Requesting changes on the concrete items: (1) make the Session workflow hookup tolerate configs without 中文说明信心:2/5 —— 架构方向正确、工艺大体良好,但本 commit 的测试套件是红的(18 个失败全部由本 PR 引入),现状不可合并。 整体来看:这是一个动机充分的功能——workflow 运行时已存在,关联 issue 的 roadmap 标签说明方向被认可,PR 的结构(可选任务契约、按 workspace 能力声明、session 级控制)与我独立构思的方案一致。问题出在收尾:一个无条件接入 Session 构造函数、弄坏三个自己从未改动的套件的改动,一条注册了路由却没有更新专门用于捕捉此类遗漏的 telemetry 审计的改动,以及一处连自己编辑过的文件里都没更新的结构断言——说明最终集成没有在整个套件上跑过。三个根因都不是架构问题,都是机械性修复。所以这是 request-changes,而不是推翻重来。 Stage 1 的范围问题依然成立,与拦截项无关:实时可视化 + 控制是核心,带比较/导出/删除的历史页和已保存工作流列表完全可以作为后续 PR。我更希望它拆成两个可审查的部分合并,而不是一个 5,300 行生产代码的整体——但这是维护者和作者的判断,不是我会强制的门槛。 给后续接手者的长期标记:这是 Stage 0 升级项(跨包、约 5,293 行生产代码、触及与 revert 相关的 acp-integration 路径),即使 CI 变绿,也需要维护者审视新增可选字段和新路由的所有消费者——re-run 时不会自动批准。变绿之后,行为性主张仍需沙箱验证( 针对具体事项请求修改:(1)让 Session 的 workflow 接入容忍没有 getWorkflowRunRegistry 的 config(或在所有受支持处接入);(2)新增路由的同时更新 telemetry 路由目录审计;(3)修复过期的 workspace 结构断言;(4)让 ubuntu、macOS、Windows 三个单元腿全绿。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs some fixes before this can move forward — the unit suite is red on this commit, with 18 failures that all trace to this PR (details in my review comments above):
- 15 tests across three suites crash in the Session constructor —
this.config.getWorkflowRunRegistry is not a function(Session.ts:6556). The new workflow callbacks are wired unconditionally; configs without the accessor need to keep working. - The new
workflow-actionroute breaks the telemetry route-catalog audit (54 registered vs the audited 53 / 46-7 split) — the audit must be updated together with any new daemon route. - One stale workspace-shape assertion in
multi-workspace-sessions.test.ts(the newworkflowsEnabledfield).
The direction and the structure are sound — the opt-in contract, the capability advertisement, and the session-staleness guards are all in good shape. Once the three unit legs (ubuntu, macOS, Windows) are green, this is in a position to continue. 🙏 @qqqys
|
CI fix pushed in Root cause: Workflow support added new session/config dependencies, workspace capability data, and one daemon telemetry route without updating all full-suite fixtures and drift guards. The Workflow-only lazy detail mounting also unintentionally hid historical AskUserQuestion results. Validation:
The new CI run is in progress. |
已被后续 commit 取代,当前 head 需重新 review
yiliang114
left a comment
There was a problem hiding this comment.
Review Summary
Scope: 86 files (+9277/-352) across core, CLI, daemon, SDK, and web-shell packages. Adds first-class Dynamic Workflow visualization to Web Shell — live execution graph, phase lanes, dispatch dependency edges, pause/resume/stop/retry/rerun controls, a dedicated Workflow page with history management, and session-less capability advertisement. Closes #8941.
Pre-checks: tsc has a pre-existing vitest/globals error unrelated to this PR. CI is fully green on the latest commit.
Verdict: No blocking issues. The architecture is clean, the optional protocol fields preserve backward compatibility, the snapshot validation (isWorkflowSnapshot) and path safety (isSafeRunIdSegment) guards are thorough, and the i18n coverage is complete (EN + ZH). The previous bot review's three findings (Session constructor crash, telemetry route-catalog drift, stale workspace assertion) are all resolved in the latest commit.
P2 findings (2 inline comments):
-
Export includes full dispatch prompts — The
downloadWorkflowHistoryexport inWorkflowExecutionView.tsxserializesdispatcheswhich contain the fullprompttext. The linked issue criteria states "Export omits scripts, arguments, results, approvals, output paths, and other sensitive operational data." Consider redacting or truncating thepromptfield before export. -
run-savedaction uses bareAbortControllerwithout timeout — InacpAgent.ts, therun-savedcase creates anew AbortController().signaland passes it toworkflowTool.build().execute(). While the workflow has its own internal 30-minute cap, the caller has no timeout guard on theexecute()call itself.
Non-blocking: The direction and structure are sound. The opt-in contract, capability advertisement, session-staleness guards, and dispatch graph rendering are all in good shape.
| task: DaemonSessionWorkflowTaskStatus, | ||
| runs: readonly DaemonSessionWorkflowTaskStatus[], | ||
| ): void { | ||
| const content = JSON.stringify( |
There was a problem hiding this comment.
[P2] The downloadWorkflowHistory export serializes dispatches which include the full prompt field for each dispatch. The issue acceptance criteria states "Export omits scripts, arguments, results, approvals, output paths, and other sensitive operational data." The prompt text can contain sensitive operational details. Consider redacting or truncating the prompt field in the export output.
There was a problem hiding this comment.
已修复。验证证据:导出脱敏回归先 RED 后通过;WorkflowExecutionView 与 ToolGroup 测试 71/71、Web Shell typecheck 和 build、ESLint、Prettier 与 git diff --check 通过。
| if (!task) return { changed: false }; | ||
| if (action === 'retry' || action === 'rerun') { | ||
| const canStart = | ||
| action === 'retry' |
There was a problem hiding this comment.
[P2] The run-saved action creates a fresh new AbortController().signal and passes it to workflowTool.build().execute(). The abort controller is never cancelled, so the execute() call has no timeout from the caller's perspective. While the workflow has its own internal timeout, the execute() method could theoretically hang if the workflow tool doesn't properly propagate the abort signal. Consider adding a timeout guard or passing a cancellable signal.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): PR #8950 adds Dynamic Workflow visualization/management t...: did not execute the sdk-typescript/web-shell test suites to confirm the updated tests pass (relied on source-vs-test tracing instead).; PR #8950 adds Dynamic Workflow visualization/management t...: none — all checks above were completed within budget.; PR #8950 adds Dynamic Workflow visualization/management t...: none — all checks above were completed within budget.; PR #8950 adds Dynamic Workflow visualization/management t...: none — all checks above completed within budget.; PR #8950 adds Dynamic Workflow visualization/management t...: none — all checks above completed within budget., and 2 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):PR #8950 adds Dynamic Workflow visualization/management t...:did not execute the sdk-typescript/web-shell test suites to confirm the updated tests pass (relied on source-vs-test tracing instead).;PR #8950 adds Dynamic Workflow visualization/management t...:none — all checks above were completed within budget.;PR #8950 adds Dynamic Workflow visualization/management t...:none — all checks above were completed within budget.;PR #8950 adds Dynamic Workflow visualization/management t...:none — all checks above completed within budget.;PR #8950 adds Dynamic Workflow visualization/management t...:none — all checks above completed within budget.,另有 2 条。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const startParams: WorkflowParams = { | ||
| script: task.script, | ||
| args: task.args, | ||
| ...(action === 'retry' ? { resumeFromRunId: task.runId } : {}), |
There was a problem hiding this comment.
[Critical] R1-50: retry issued after delete-history silently loses the retried run's entire resume journal: deleteWorkflowSnapshot rms the run directory while jsonl-utils' process-global ensuredDirs mkdir cache still lists it, so every journal append of the retry (same runId via resumeFromRunId) skips mkdir and fails ENOENT behind the fire-and-forget catch. Probe-confirmed: unmodified code → retried journal never materializes; with the cache invalidated → probe flips. — Failure scenario: (1) Run X fails after dispatching ≥1 agent — the first journal.append cached /workflows/X/ in jsonl-utils' module-level ensuredDirs. (2) delete-history → fs.rm(/X), but the terminal registry entry for X survives (see the registry-retention finding), so retry still finds it, passes its gate (status === 'failed' && !getHandle), and reuses runId X. (3) Every journal.append reaches writeLine, sees ensuredDirs.has(dir) → skips mkdir → appendFile throws ENOENT swallowed by .catch(debugLogger.warn). The retried run completes normally with an empty journal: any later retry re-runs ALL dispatches live (resume-cache loss, doubled token spend), the new await journal?.drain() is a no-op, and nothing surfaces to the user. The delete-then-retry-same-runId sequence is newly reachable only through this PR's two new actions.
Suggested fix: After the fs.rm in deleteWorkflowSnapshot, invalidate that exact directory from the jsonl-utils cache (export an invalidateEnsuredDir(dir) next to _resetEnsuredDirsCacheForTest); alternatively make writeLine retry once with mkdir on ENOENT.
中文说明
delete-history 之后执行 retry 会静默丢失 retry run 的整个 resume journal:deleteWorkflowSnapshot rm 掉 run 目录,而 jsonl-utils 的进程级 ensuredDirs mkdir 缓存仍记录该目录,于是 retry(同 runId,经 resumeFromRunId)的每次 journal append 都跳过 mkdir,在 fire-and-forget 的 catch 背后 ENOENT 失败。探针确认:原代码下 retry 的 journal 从不落盘;使缓存失效后探针翻转。
故障场景:(1) run X 在派发 ≥1 个 agent 后失败——首次 journal.append 已把 /workflows/X/ 缓存进模块级 ensuredDirs。(2) delete-history → fs.rm(/X),但 X 的终态 registry 条目仍在(见 registry 保留相关发现),retry 仍能找到它、通过 gate(status === 'failed' && !getHandle)并复用 runId X。(3) 每次 journal.append 到 writeLine 时命中缓存 → 跳过 mkdir → appendFile 抛 ENOENT 被 .catch(debugLogger.warn) 吞掉。retry 正常完成但 journal 为空:之后再 retry 会全部重新实跑(resume 缓存失效、token 双倍花费),新增的 await journal?.drain() 成为空操作,用户毫无感知。"删除后以同 runId retry"的序列只有本 PR 的两个新动作才可达。
建议修复:deleteWorkflowSnapshot 的 fs.rm 之后使 jsonl-utils 缓存中该目录失效(在 _resetEnsuredDirsCacheForTest 旁导出 invalidateEnsuredDir(dir));或让 writeLine 在 ENOENT 时重试一次 mkdir。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const handle = this.config.getWorkflowRunRegistry().getHandle(runId); | ||
| if (handle) await handle.completion; |
There was a problem hiding this comment.
[Critical] R1-26: deleteWorkflowHistory treats any live handle for the runId as 'an owner still persisting', but retry restarts a run under the SAME runId (resumeFromRunId) — so the await latches onto the retry and the delete ultimately destroys the retry's artifacts while returning true. Probe-confirmed against the real Session/snapshot/registry code (delete resolved true and deleted the retry's just-written snapshot). — Failure scenario: Run wf_X fails → snapshot on disk, shown historical (delete button visible). User clicks retry: register() legally replaces the terminal entry with an active one under the same runId, attaching a handle. A delete-history for wf_X issued while the retry runs (stale tasks view, the poll window, or a direct SDK call — the route performs no active-run check) hits getHandle('wf_X') and blocks the ACP request for the entire retry (up to the ~30-min wall-clock cap; completion never rejects, and paused time doesn't count). When the retry settles, its finally overwrites wf_X.json before completion resolves; the delete then unlinks that snapshot and rms the retry's journal — silently destroying the just-completed retry's record and replay cache while reporting success.
Suggested fix: Distinguish 'active run' from 'settling owner': const entry = registry.get(runId); if (entry && isActiveWorkflowStatus(entry.status)) return false (or throw a busy error); keep the completion wait only when the entry is already terminal.
中文说明
deleteWorkflowHistory 把该 runId 的任何存活 handle 都当作"仍在持久化的 owner",但 retry 会用同一个 runId(resumeFromRunId)重启 run——await 会挂到 retry 上,删除最终销毁 retry 的产物却返回 true。已用真实 Session/snapshot/registry 代码探针确认(delete 解析为 true 并删除了 retry 刚写入的快照)。
故障场景:run wf_X 失败 → 快照在盘上、显示为历史(删除按钮可见)。用户点 retry:register() 合法地用同 runId 的活跃条目替换终态条目并挂上 handle。retry 运行期间对 wf_X 发起 delete-history(来自过期任务视图、轮询窗口或直接 SDK 调用——路由没有活跃 run 检查)会命中 getHandle('wf_X'),把 ACP 请求阻塞整个 retry 时长(最长 ~30 分钟墙钟上限;completion 永不 reject,暂停时间不计入)。retry 结束时其 finally 先覆写 wf_X.json,随后 delete 解除该快照链接并 rm retry 的 journal——静默销毁刚完成的 retry 记录与回放缓存并回报成功。
建议修复:区分"活跃 run"与"正在收尾的 owner":const entry = registry.get(runId); 若 entry && isActiveWorkflowStatus(entry.status) 则返回 false(或抛 busy 错误);仅当条目已终态才等待 completion。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| async deleteWorkflowHistory(runId: string): Promise<boolean> { | ||
| const handle = this.config.getWorkflowRunRegistry().getHandle(runId); | ||
| if (handle) await handle.completion; | ||
| await this.refreshWorkflowHistory(); |
There was a problem hiding this comment.
[Critical] R1-2: deleteWorkflowHistory unconditionally replaces the in-memory history cache with the disk listing (refreshWorkflowHistory) BEFORE its existence check, silently discarding terminal runs that are cached but not on disk — including the very run being deleted. The same wholesale replacement also fires on every includeWorkflows poll. — Failure scenario: A run reaches a terminal state: registry.complete() fires the status-change callback synchronously and the Session caches the snapshot, but writeWorkflowSnapshot happens later in the runner's finally and is explicitly best-effort (failures swallowed). If the write failed or is still in flight, the run exists only in this.workflowHistory. Deleting it: getHandle returns undefined, refreshWorkflowHistory() overwrites the cache with the disk list (run absent), some(...) returns false, the route reports { changed: false } ('The saved run is no longer available') while the row simultaneously vanishes from history and nothing re-adds it. Violates issue #8941's acceptance criterion 'failed deletion does not corrupt in-memory history'; the sibling test 'keeps cached history on disk failure' masks this because it mocks listWorkflowSnapshots to return the cache contents.
Suggested fix: Merge instead of wholesale-replace: in refreshWorkflowHistory, union the disk listing with remembered entries whose runId is absent on disk (or take the existence-check union of pre-refresh cache and disk list in deleteWorkflowHistory, dropping the cache entry and returning true when only the cache held it).
中文说明
deleteWorkflowHistory 在存在性检查之前就用磁盘列表无条件替换内存历史缓存(refreshWorkflowHistory),静默丢弃"已缓存但未落盘"的终态 run——包括正被删除的 run 本身;同样的整体替换也会在每次 includeWorkflows 轮询时发生。
故障场景:run 到达终态时 registry.complete() 同步触发回调缓存快照,但 writeWorkflowSnapshot 发生在 runner 的 finally 中且明确是尽力而为(失败被吞掉)。写入失败或仍在进行中时,该 run 仅存在于内存缓存;删除时 getHandle 返回 undefined,refreshWorkflowHistory() 用不含该 run 的磁盘列表覆盖缓存,some(...) 返回 false,路由返回 { changed: false }("该已保存的运行不再可用"),同时该行从历史消失且不会再被加回——违反 issue #8941 验收标准"删除失败不应破坏内存历史";姊妹测试"磁盘失败时保留缓存历史"因把 listWorkflowSnapshots mock 成缓存内容而掩盖了该问题。
建议修复:合并而非整体替换——refreshWorkflowHistory 中将磁盘列表与 runId 不在磁盘上的已缓存条目合并(或在 deleteWorkflowHistory 中用刷新前缓存与磁盘列表的并集做存在性检查,仅缓存持有时删除缓存条目并返回 true)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| debugLogger.warn('emitter.agentCompleted threw:', e); | ||
| } | ||
| try { | ||
| emitter?.dispatchSettled?.(dispatchId, message, Date.now()); |
There was a problem hiding this comment.
[Critical] R1-32: A dispatch that throws an empty-message Error is recorded as 'completed', not 'failed': the settle payload carries the raw message string and the new registry handler derives status via truthiness. Probe-confirmed: dispatchSettled(dispatchId, '') fires for throw new Error() and the registry persists status 'completed' with a dispatch-completed event (flip arm with error !== undefined passes). — Failure scenario: emitCompletion computes message = error instanceof Error ? error.message : String(error). Host-realm errors with no message (throw new Error(), new TypeError(), default-constructed DOMException — all message === '') produced anywhere in the production dispatch path yield dispatchSettled(dispatchId, '', Date.now()); in onDispatchSettled, '' is falsy, so the thrown dispatch is persisted as status 'completed' and a dispatch-completed event is appended. The persisted trace, the Web Shell dependency-graph visualization (green node), and the exported history all show that node as a success even though the dispatch rejected. The run itself still fails (blast radius is the per-dispatch record).
Suggested fix: Key the failed/completed branch off error !== undefined instead of truthiness (and/or pass an explicit failure flag / message || 'Dispatch failed.' into dispatchSettled).
中文说明
抛出空 message Error 的 dispatch 会被记录为 'completed' 而非 'failed':settle 载荷携带原始 message 字符串,而新的 registry handler 用真值判断推导状态。探针确认:throw new Error() 触发 dispatchSettled(dispatchId, ''),registry 持久化 status 'completed' 并追加 dispatch-completed 事件(把判断改为 error !== undefined 的翻转臂通过)。
故障场景:emitCompletion 计算 message = error instanceof Error ? error.message : String(error)。宿主域无 message 的错误(throw new Error()、new TypeError()、默认构造的 DOMException——message 均为 '')在 dispatch 生产路径任何位置抛出都会得到 dispatchSettled(dispatchId, '', Date.now());在 onDispatchSettled 中 '' 为假值,被抛弃的 dispatch 被持久化为 'completed' 并追加 dispatch-completed 事件。持久化轨迹、Web Shell 依赖图可视化(绿色节点)和导出历史都显示该节点成功,尽管 dispatch 实际 rejected。run 本身仍会失败(影响范围是单条 dispatch 记录)。
建议修复:以 error !== undefined 判定成败分支(和/或传显式失败标志 / message || 'Dispatch failed.' 给 dispatchSettled)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await journal?.drain(); | ||
| await writeWorkflowSnapshot(config, entry); |
There was a problem hiding this comment.
[Critical] R1-53: The new await journal?.drain() becomes the settlement finally's FIRST await, ahead of writeWorkflowSnapshot, breaking the settlement-time freeze invariant three comments explicitly document ('Capture the telemetry projection before the first await', 'Project BEFORE the first await', onBudgetUpdated's frozen-projection note). Probe: 30/30 runs persisted a snapshot with agentsCompleted higher than the settlement-time value under the PR ordering; 0/30 with the one-line revert. — Failure scenario: drain() waits for the journal's pending chain of sequential flushed fs appends (up to 2 per dispatch). During that window, in-flight dispatches deliberately keep mutating the live entry — onAgentCompleted/onBudgetUpdated have NO terminal-status gate by design ('dispatches in flight at settlement always drain after the terminal status is set') — and controller.abort() (the first statement of this same finally) asynchronously drives abort-settlements. Trigger: any run with a dispatch still in flight at settlement (the fire-and-forget pattern this PR itself adds a test for, or any abort mid-dispatch). Outcome: the persisted snapshot captures post-drain agentsCompleted/tokensSpent/perPhaseTokens while the telemetry event stays settlement-time — the exact disagreement the comment block was written to prevent. The existing freeze test exercises the writer in isolation, so it cannot catch this.
Suggested fix: Keep the projection ahead of every await: kick the write off before the drain (const snapshotWrite = writeWorkflowSnapshot(config, entry); await journal?.drain(); await snapshotWrite;) — the projection is synchronous at call time.
中文说明
新增的 await journal?.drain() 成为结算 finally 的第一个 await,位于 writeWorkflowSnapshot 之前,破坏了三处注释明确记载的"结算时冻结"不变量('Capture the telemetry projection before the first await'、'Project BEFORE the first await'、onBudgetUpdated 的冻结投影注释)。探针:PR 顺序下 30/30 次运行持久化的快照 agentsCompleted 高于结算时值;单行还原后 0/30。
故障场景:drain() 等待 journal 的待写链(每个 dispatch 最多 2 次刷盘 append)。窗口期内,在飞 dispatch 按设计继续修改存活条目(onAgentCompleted/onBudgetUpdated 无终态 gate——'dispatches in flight at settlement always drain after the terminal status is set'),且本 finally 首句 controller.abort() 异步驱动中止结算。触发:结算时仍有在飞 dispatch 的任何 run(本 PR 自己测试的 fire-and-forget 模式,或 dispatch 中途中止)。结果:持久化快照捕获 drain 之后的 agentsCompleted/tokensSpent/perPhaseTokens,而 telemetry 事件停留在结算时——正是注释块要防止的不一致;现有冻结测试只测 writer 本身,无法捕获。
建议修复:把投影放在所有 await 之前:const snapshotWrite = writeWorkflowSnapshot(config, entry); await journal?.drain(); await snapshotWrite;(投影在调用时同步完成)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(container.textContent).not.toContain( | ||
| 'Workflow could not be started', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-12: Pattern: four negative assertions reference strings/elements that no render path can produce — they can never fail, whatever the component renders. (occurrence 1 of 4)
Pattern: Pattern: four negative assertions reference strings/elements that no render path can produce — they can never fail, whatever the component renders.
Failure scenario: Each regression the assertions advertise (error banner leaking into the wrong session's tab; runtime metric re-enabled for embedded workflow rows; verbose intro copy returning; replay controls appearing on saved runs) ships undetected — the checks provide zero regression protection while readers believe they are covered.
Suggested fix: Assert against actually-rendered strings (e.g. not.toContain('could not be started'), /Time\s*5s/), or drop the assertions that reference nonexistent features (and land the guards together with a feature if one is planned).
中文说明
(模式发现,共 4 处;本评论标记第 1 处。)
模式:四个负向断言引用的字符串/元素是任何渲染路径都无法产生的——无论组件渲染什么都不会失败。分别:(1) 'Workflow could not be started'(大写 W)与实际产品文案 'The saved workflow could not be started…' 大小写不符(toContain 区分大小写);(2) 'Runtime 5s'——运行时长标签渲染为 'Time'(tasks.detail.runtime),从不是 'Runtime';(3) 'Dynamic workflow traces' / 'replay completed runs'——包内任何地方都不存在;(4) [data-run-replay] / input[type=range] / 'Run replay'——replay 功能任何地方都不存在(grep 仅匹配测试行;组件根本不渲染 )。
故障场景:各断言想要防住的回归(错误横幅泄漏到别的会话页签、嵌入式 workflow 行重新显示运行时长指标、冗长介绍文案回归、已保存 run 出现回放控件)都会在不被发现的情况下发布——检查提供零回归保护,读者却以为已被覆盖。
建议修复:断言实际渲染的字符串(如 not.toContain('could not be started')、/Time\s*5s/),或删除引用不存在功能的断言(若计划实现某功能,则与功能一起落地相应守卫)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const [nextSnapshot] = await Promise.all([ | ||
| actions.getTasks(), | ||
| actions.refreshCommands(), | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] R1-13: Promise.all couples the tasks fetch with the saved-workflow command refresh, so a failure of the non-essential refreshCommands() discards an already-successful getTasks() result and fails the entire page load. Probe-confirmed: PR code shows the loadError banner with a valid snapshot in hand; with refreshCommands().catch(() => undefined) the probe flips. — Failure scenario: Daemon supported-commands route transiently unavailable (e.g. daemon restart mid-view): getTasks() resolves with valid active/history runs, refreshCommands() rejects (it is an HTTP daemon route wrapped in a timeout), Promise.all rejects, setLoadError(true) runs, snapshot stays null. On initial load the user sees 'Failed to load workflow runs' and empty Active/History tabs even though the task data was fetched — data for two tabs blocked by a failure that only affects the third (Saved) tab.
Suggested fix: const [nextSnapshot] = await Promise.all([actions.getTasks(), actions.refreshCommands().catch((e) => { console.warn('[web-shell] failed to refresh commands:', e); })]); — or Promise.allSettled, keeping the tasks result when only the command refresh fails.
中文说明
Promise.all 把任务拉取与已保存 workflow 命令刷新耦合在一起:非关键的 refreshCommands() 失败会丢弃已成功的 getTasks() 结果并导致整页加载失败。探针确认:PR 代码下手握有效快照仍显示 loadError 横幅;改为 refreshCommands().catch(() => undefined) 后探针翻转。
故障场景:daemon supported-commands 路由瞬时不可用(如查看中途 daemon 重启):getTasks() 带着有效 active/history run 解析,refreshCommands()(带超时的 HTTP daemon 路由)拒绝,Promise.all 拒绝,setLoadError(true),snapshot 保持 null。初次加载时用户看到 "Failed to load workflow runs" 与空的 Active/History 页签,尽管任务数据已取到——两个页签的数据被一个只影响第三个(Saved)页签的失败阻塞。
建议修复:const [nextSnapshot] = await Promise.all([actions.getTasks(), actions.refreshCommands().catch((e) => { console.warn('[web-shell] failed to refresh commands:', e); })]); —— 或 Promise.allSettled,仅命令刷新失败时保留任务结果。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| manageActiveEvent={false} | ||
| syncSnapshot | ||
| taskView="workflow-active" |
There was a problem hiding this comment.
[Suggestion] R1-66: The embedded TasksStatusMessage instances poll getTasks every 3s (isOpen stays true — nothing on this page can close it) but manageActiveEvent={false} suppresses the ACTIVE_EVENT pause protocol (dispatchActive is skipped), so the App-level and ChatPane useBackgroundTasks pollers for the same session never pause while the Workflows page is open. No documented rationale for manageActiveEvent={false} exists in the diff. — Failure scenario: User opens the Workflows page while a workflow (or any background task) is active: the page's currently-selected panel polls getTasks every 3s AND the App-level poller (and the ChatPane instance) keep their own 3s polls for the same session — concurrent identical polls the ACTIVE_EVENT/tasksPanelActive machinery exists to prevent. Amplifier: every such poll now also costs the full-history disk re-read described in the per-poll cost finding. (Radix unmounts inactive tabs, so only the selected page panel polls — the duplication is page-panel vs App/ChatPane pollers.)
Suggested fix: Let the page's panels participate in the pause protocol (drop manageActiveEvent={false}, or dispatch an active event with the session id from WorkflowRunsPage while it is mounted) so the hook pollers yield to the page's own sync.
中文说明
嵌入式 TasksStatusMessage 实例每 3 秒轮询 getTasks(isOpen 保持 true——本页无任何路径可关闭它),但 manageActiveEvent={false} 抑制了 ACTIVE_EVENT 暂停协议(dispatchActive 被跳过),因此只要 Workflows 页打开,App 级与 ChatPane 的 useBackgroundTasks 轮询器对同一会话就永不暂停。diff 中无 manageActiveEvent={false} 的书面理由。
故障场景:workflow(或任何后台任务)活跃时打开 Workflows 页:页面当前选中面板每 3 秒轮询 getTasks,同时 App 级轮询器(与 ChatPane 实例)保持各自的 3 秒轮询——ACTIVE_EVENT/tasksPanelActive 机制本要防止的并发同会话轮询。放大器:每次此类轮询现在还叠加"每轮询全量历史重读"成本。(Radix 卸载非活动页签,所以只有选中的页面面板轮询——重复发生在页面面板与 App/ChatPane 轮询器之间。)
建议修复:让页面面板参与暂停协议(移除 manageActiveEvent={false},或由 WorkflowRunsPage 在挂载期间派发带会话 id 的 active 事件),使 hook 轮询器让位于页面自身同步。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| tokensSpent: 0, | ||
| recentLogs: [], | ||
| pendingApprovalCount: 0, |
There was a problem hiding this comment.
[Suggestion] R1-7: Pattern: four new workflow-task fixtures omit required DaemonSessionWorkflowTaskStatus fields (tokenBudgetTotal everywhere; currentPhase in one), violating their own type annotations. (occurrence 4 of 4)
Pattern: Pattern: four new workflow-task fixtures omit required DaemonSessionWorkflowTaskStatus fields (tokenBudgetTotal everywhere; currentPhase in one), violating their own type annotations.
Failure scenario: Any IDE or type-aware tooling resolving @qwen-code/sdk/daemon flags these with hard type errors today; the moment the tsconfig exclusion is lifted or a test tsconfig is added, the package typecheck breaks. The fixtures also model a state (undefined) the daemon never sends (null).
Suggested fix: Add tokenBudgetTotal: null, (and currentPhase: null, in composerTasks.test.ts) to each fixture, matching the sibling fixtures in this PR.
中文说明
(模式发现,共 4 处;本评论标记第 4 处。)
模式:四个新的 workflow-task 测试夹具遗漏了 DaemonSessionWorkflowTaskStatus 声明为必填(非可选)的字段:四处均缺 tokenBudgetTotal: number | null,composerTasks.test.ts 还缺 currentPhase: string | null。字面量违反自身类型标注(TS2739/TS2741)——当前不可见仅因 web-shell 的 typecheck 排除 client/**/*.test.{ts,tsx} 且 vitest 不做类型检查。同一 PR 新增的姊妹夹具(ToolGroup.test.tsx、WorkflowExecutionView.test.tsx、useBackgroundTasks.test.tsx、workflowTasks.test.ts)均包含这些字段。
故障场景:任何解析 @qwen-code/sdk/daemon 的 IDE 或类型感知工具今天就会报硬性类型错误;一旦取消 tsconfig 排除或新增测试 tsconfig,包级 typecheck 即失败;夹具还建模了 daemon 从不发送的状态(undefined 而非 null)。
建议修复:为各夹具补上 tokenBudgetTotal: null(composerTasks.test.ts 另补 currentPhase: null),与本 PR 的姊妹夹具一致。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| return typeof tool.args?.resumeFromRunId === 'string' | ||
| ? tool.args.resumeFromRunId | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] R1-45: The run-identity matching ladder's last two rungs — the args.resumeFromRunId fallback and the 'Workflow X started/—' text regex — are new behavior with zero test coverage, and the resumeFromRunId rung is load-bearing. Probe: removing both rungs keeps the shipped suite 4/4 green while a scenario probe (restored failed task, no toolUseId, failure text without runId) flips 2/2. — Failure scenario: A resumed run reuses the source run's id (resumeFromRunId ?? wf_), and snapshot-restored tasks persist sourceRunId/startMode but NOT toolUseId — so for historical/post-restart resumed runs the toolUseId rung always misses, and the terminal display payload of a failed run omits runId — leaving this fallback as the only rung that can link the tool call to its task. If a future change drops or breaks the fallback, the inline workflow view silently degrades to the static fallback card (workflow.inline.loading/unavailable) with every existing test green.
Suggested fix: Add two cases to workflowTasks.test.ts: (1) a tool with args: { resumeFromRunId: 'wf_expected' } and no identifying output text, expecting the wf_expected task; (2) a tool whose rawOutput is the background returnDisplay shape ('Workflow wf_expected started in the background (status: running)…'), expecting wf_expected.
中文说明
运行身份匹配阶梯的最后两级——args.resumeFromRunId 回退与 'Workflow X started/—' 文本正则——是零测试覆盖的新行为,且 resumeFromRunId 级是承重的。探针:移除两级后发布套件仍 4/4 全绿,而场景探针(恢复的失败任务、无 toolUseId、无 runId 的失败文本)2/2 翻转。
故障场景:恢复的 run 复用源 run 的 id(resumeFromRunId ?? wf_),快照恢复的任务持久化 sourceRunId/startMode 但不含 toolUseId——历史/重启后恢复任务的 toolUseId 级必然落空,失败 run 的终态显示载荷不含 runId——该回退成为唯一能把工具调用链接到任务的级。若未来改动删除或破坏该回退,内联 workflow 视图会静默降级为静态回退卡片(workflow.inline.loading/unavailable),而所有现有测试保持绿色。
建议修复:给 workflowTasks.test.ts 加两个用例:(1) 工具带 args: { resumeFromRunId: 'wf_expected' } 且无可识别输出文本,期望命中 wf_expected 任务;(2) rawOutput 为后台 returnDisplay 形状('Workflow wf_expected started in the background (status: running)…'),期望 wf_expected。
— qwen3.8-max via Qwen Code /review (v0.21.10)
已被后续 commit 取代,当前 head 需重新 review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget., and 5 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks above were completed within budget.,另有 5 条。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await this.refreshWorkflowHistory(); | ||
| if (!this.workflowHistory.some((item) => item.runId === runId)) { | ||
| return false; |
There was a problem hiding this comment.
[Critical] R1-2: deleteWorkflowHistory wholesale-replaces the in-memory history cache with the disk listing (refreshWorkflowHistory) BEFORE its existence check, silently discarding terminal runs that are cached but not yet on disk — including the run being deleted. The same wholesale replacement also fires on every includeWorkflows tasks poll. Probe-confirmed; violates issue #8941's acceptance criterion "failed deletion does not corrupt in-memory history". — Failure scenario: a run reaches a terminal state and is cached via the synchronous status-change callback, but writeWorkflowSnapshot runs later in the runner's finally and is best-effort (failures swallowed). Deleting it while the write failed/is in flight: getHandle returns undefined → refreshWorkflowHistory() overwrites the cache with the disk list (run absent) → some(...) returns false → the route reports { changed: false } while the row vanishes from history and nothing re-adds it. Probe: failed delete left in-memory history length 0; with a merge-based refresh the probe flips. The sibling test masks this by mocking listWorkflowSnapshots to return the cache contents.
| await this.refreshWorkflowHistory(); | |
| if (!this.workflowHistory.some((item) => item.runId === runId)) { | |
| return false; | |
| await this.refreshWorkflowHistory(); | |
| if ( | |
| !this.workflowHistory.some((item) => item.runId === runId) | |
| ) { | |
| return false; |
(建议修复为合并而非整体替换:refreshWorkflowHistory 中将磁盘列表与 runId 不在磁盘上的已缓存条目合并;或在 deleteWorkflowHistory 中用刷新前缓存与磁盘列表的并集做存在性检查。)
中文说明
R1-2:deleteWorkflowHistory 在存在性检查之前就用磁盘列表整体替换内存历史缓存(refreshWorkflowHistory),静默丢弃"已缓存但未落盘"的终态 run——包括正被删除的 run 本身;同样的整体替换也会在每次 includeWorkflows 轮询时发生。已用探针确认;违反 issue #8941 验收标准"删除失败不应破坏内存历史"。故障场景:run 到达终态时经同步状态回调被缓存,但 writeWorkflowSnapshot 在 runner 的 finally 中稍后执行且尽力而为(失败被吞掉)。在写入失败或仍在进行中时删除:getHandle 返回 undefined → refreshWorkflowHistory() 用不含该 run 的磁盘列表覆盖缓存 → some(...) 返回 false → 路由返回 { changed: false },同时该行从历史消失且不会再被加回。探针:失败的删除使内存历史长度变为 0;改为合并式刷新后探针翻转。姊妹测试因把 listWorkflowSnapshots mock 成缓存内容而掩盖了该问题。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await fs.rm(`${storage.getWorkflowRunsDir()}/${runId}`, { | ||
| recursive: true, | ||
| force: true, | ||
| }); |
There was a problem hiding this comment.
[Critical] R1-50: this fs.rm removes the run directory while jsonl-utils' process-global ensuredDirs mkdir cache still lists it, so every journal append of a later retry (same runId via resumeFromRunId) skips mkdir and fails ENOENT behind the fire-and-forget catch — the retried run completes with an empty resume journal. Probe-confirmed. — Failure scenario: (1) run X fails after dispatching ≥1 agent — the first journal.append cached <runsDir>/X/ in the module-level ensuredDirs; (2) delete-history rms the directory, but the terminal registry entry for X survives (delete-history has no registry-removal path), so retry still finds it, passes its gate (status === 'failed' && !getHandle), and reuses runId X; (3) every journal append of the retry sees ensuredDirs.has(dir) → skips mkdir → appendFile throws ENOENT swallowed by .catch(debugLogger.warn). The next retry re-runs ALL dispatches live (resume-cache loss, doubled token spend), the new await journal?.drain() is a no-op, and nothing surfaces. Probe: retry after delete → journal exists=false; with cache invalidation the probe flips.
| await fs.rm(`${storage.getWorkflowRunsDir()}/${runId}`, { | |
| recursive: true, | |
| force: true, | |
| }); | |
| await fs.rm(`${storage.getWorkflowRunsDir()}/${runId}`, { | |
| recursive: true, | |
| force: true, | |
| }); | |
| invalidateEnsuredDir(`${storage.getWorkflowRunsDir()}/${runId}`); |
(需先在 jsonl-utils 中导出 invalidateEnsuredDir(dir);或让 writeLine 在 ENOENT 时重试一次 mkdir。)
中文说明
R1-50:此 fs.rm 删除 run 目录后,jsonl-utils 的进程级 ensuredDirs mkdir 缓存仍记录该目录,于是之后 retry(经 resumeFromRunId 复用同 runId)的每次 journal append 都跳过 mkdir,在 fire-and-forget 的 catch 背后 ENOENT 失败——retry 完成时 resume journal 为空。已用探针确认。故障场景:(1) run X 在派发 ≥1 个 agent 后失败——首次 journal.append 已把 <runsDir>/X/ 缓存进模块级 ensuredDirs;(2) delete-history rm 目录,但 X 的终态 registry 条目仍在(delete-history 没有 registry 移除路径),retry 仍能找到它、通过 gate(status === 'failed' && !getHandle)并复用 runId X;(3) retry 的每次 journal append 命中 ensuredDirs.has(dir) → 跳过 mkdir → appendFile 抛 ENOENT 被 .catch(debugLogger.warn) 吞掉。之后再 retry 会全部重新实跑(resume 缓存失效、token 双倍花费),新增的 await journal?.drain() 成为空操作,用户毫无感知。探针:删除后 retry → journal exists=false;使缓存失效后探针翻转。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| dispatch.status = | ||
| entry.status === 'cancelled' | ||
| ? 'cancelled' | ||
| : error | ||
| ? 'failed' |
There was a problem hiding this comment.
[Critical] R1-32: onDispatchSettled derives failed/completed from the truthiness of the raw settle-message string; a dispatch that throws a message-less Error (throw new Error(), new TypeError(), default DOMException — message === '') is persisted as 'completed' with a dispatch-completed event. Probe-confirmed; violates issue #8941's "accurate phase and dispatch state". — Failure scenario: emitCompletion computes the settled message as error.message for Error instances, so host-realm errors with no message yield dispatchSettled(dispatchId, ''); '' is falsy here, so the thrown dispatch is persisted as status 'completed'. The persisted trace, the Web Shell dependency graph (green node), and the exported history all show that node as a success even though the dispatch rejected (the run itself still fails — blast radius is the per-dispatch record). Probe: observed dispatch.status='completed' + dispatch-completed event for a thrown dispatch; with error !== undefined the probe flips to dispatch-failed.
| dispatch.status = | |
| entry.status === 'cancelled' | |
| ? 'cancelled' | |
| : error | |
| ? 'failed' | |
| dispatch.status = | |
| entry.status === 'cancelled' | |
| ? 'cancelled' | |
| : error !== undefined | |
| ? 'failed' |
中文说明
R1-32:onDispatchSettled 用 settle 原始消息字符串的真值判断推导成败;抛出无 message Error 的 dispatch(throw new Error()、new TypeError()、默认构造的 DOMException——message === '')会被持久化为 'completed' 并追加 dispatch-completed 事件。已用探针确认;违反 issue #8941 "准确的阶段与调度状态"。故障场景:emitCompletion 对 Error 实例取 error.message 作为 settle 消息,宿主域无 message 的错误得到 dispatchSettled(dispatchId, '');'' 在此为假值,被丢弃的 dispatch 被持久化为 'completed'。持久化轨迹、Web Shell 依赖图(绿色节点)和导出历史都显示该节点成功,尽管 dispatch 实际 rejected(run 本身仍会失败——影响范围是单条 dispatch 记录)。探针:抛错 dispatch 观察到 dispatch.status='completed' 与 dispatch-completed 事件;改为 error !== undefined 后探针翻转为 dispatch-failed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| run_in_background: true, | ||
| } satisfies WorkflowParams) | ||
| .execute(new AbortController().signal)) as WorkflowToolResult; |
There was a problem hiding this comment.
[Critical] R1-68: runs started via this route (run-saved / retry / rerun) execute the tool directly in the handler, so no 'workflow' tool call ever appears in the session transcript, taskActivityKey never changes, and nothing bumps backgroundTasksRefreshTrigger (all five bump sites in App.tsx are non-workflow; <WorkflowRunsPage /> renders with no props) — the App/ChatPane pollers never wake for the run. — Failure scenario: on an idle session (taskActivityKey === '', refreshTrigger === 0) the useBackgroundTasks start gate returns early and polling never activates. The run executes on the daemon — DaemonClient.sessionTasks always requests includeWorkflows=true, so it WOULD be returned — but App/ChatPane sessionTasks stays []: status-bar task pills, the environment panel, and the inline transcript workflow graph never show the run for its whole lifetime, until an unrelated user action bumps the trigger. Leaving the Workflows page makes an active run invisible everywhere else. This re-creates issue #8941's problem statement for exactly the launch paths this PR adds.
| run_in_background: true, | |
| } satisfies WorkflowParams) | |
| .execute(new AbortController().signal)) as WorkflowToolResult; | |
| .execute(new AbortController().signal)) as WorkflowToolResult; | |
| onWorkflowRunStartedRef.current?.(); |
(建议:给 WorkflowRunsPage 一个来自 App 的 onWorkflowRunStarted prop,在 result.started 后 bump setBackgroundTasksRefreshTrigger,仿照现有 /fork 处理。)
中文说明
R1-68:经此路由启动的 run(run-saved / retry / rerun)在 handler 内直接执行工具,因此 session transcript 中不会出现任何 'workflow' 工具调用,taskActivityKey 永不变化,也没有任何地方 bump backgroundTasksRefreshTrigger(App.tsx 的五个 bump 点均与 workflow 无关;<WorkflowRunsPage /> 无 prop 渲染)——App/ChatPane 轮询器永远不会为该 run 唤醒。故障场景:空闲 session(taskActivityKey === ''、refreshTrigger === 0)下 useBackgroundTasks 启动 gate 提前返回,轮询永不激活。run 在 daemon 上运行——DaemonClient.sessionTasks 总是带 includeWorkflows=true,本可以返回该 run——但 App/ChatPane 的 sessionTasks 保持 []:状态栏任务胶囊、环境面板、transcript 内联 workflow 图在其整个生命周期都不显示该 run,直到某个无关用户操作 bump 触发器。离开 Workflows 页后活跃 run 在所有地方不可见。这在本 PR 新增的启动路径上重新制造了 issue #8941 要解决的问题。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| onKeyDown={(event) => { | ||
| if (event.key !== 'Enter' && event.key !== ' ') return; | ||
| event.preventDefault(); | ||
| activateTask(); | ||
| }} |
There was a problem hiding this comment.
[Critical] R1-3: DOM focus and arrow-key selection diverge on the new focusable rows — the global ArrowUp/ArrowDown handler (~line 659) only calls setSelectedTaskId and never moves DOM focus, every row renders tabIndex={0} (no roving tabindex), and this row keydown activates the row focus sits on. — Failure scenario: in any panel with keyboardShortcuts enabled (the default), a keyboard user Tabs into the list (focus lands on a row, onFocus syncs selection), then presses ArrowDown three times: ❯ moves to row 4 but DOM focus stays on row 1. Pressing Enter runs row 1's onKeyDown → activateTask() for row 1: it opens row 1's detail and snaps selection back while the pointer was visibly on row 4. Meanwhile 'x' still targets the arrow selection — two keys in the same panel act on two different 'current' rows. Pre-diff this was unreachable (rows were not focusable).
| onKeyDown={(event) => { | |
| if (event.key !== 'Enter' && event.key !== ' ') return; | |
| event.preventDefault(); | |
| activateTask(); | |
| }} | |
| onKeyDown={(event) => { | |
| if (event.key !== 'Enter' && event.key !== ' ') return; | |
| event.preventDefault(); | |
| activateTask(tasks[clampedSelectedIndex]); | |
| }} |
(最小修复:行 keydown 作用于 tasks[clampedSelectedIndex];更彻底的做法是 roving tabindex——仅选中行 tabIndex={0},方向键改变选择时同步移动 DOM focus。)
中文说明
R1-3:新的可聚焦行使 DOM focus 与方向键选择分离——全局 ArrowUp/ArrowDown handler(约 659 行)只调用 setSelectedTaskId 而不移动 DOM focus,每行都渲染 tabIndex={0}(无 roving tabindex),此行 keydown 激活 focus 所在行。故障场景:在 keyboardShortcuts 开启(默认)的面板中,键盘用户 Tab 进入列表(focus 落在某行、onFocus 同步选择)后按三次 ArrowDown:❯ 移到第 4 行但 DOM focus 仍在第 1 行;按 Enter 执行第 1 行的 onKeyDown → 对第 1 行 activateTask(),打开第 1 行详情并把选择拉回,而指针明明在第 4 行;同时 'x' 仍作用于方向键选择——同一面板两个键操作两个不同的"当前行"。改动前不可达(行不可聚焦)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| function isActiveStatus(status: DaemonSessionTaskStatus['status']): boolean { | ||
| return status === 'running' || status === 'pausing' || status === 'paused'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-16: this hand-duplicates the pre-existing isActive predicate from TasksStatusMessage.tsx — the same active/history partition is now defined in two places in the same package. The drift risk is concrete, not hypothetical: this very PR extended the active set with 'pausing' in one of the siblings. — Concrete cost: the 'Running'/'History' tab badges are computed by isActiveStatus (counts memo), while the lists those tabs render are filtered by TasksStatusMessage's isActive via tasksForView. If the workflow lifecycle set changes again and only one predicate is updated, the badge and the list silently disagree — a run in a newly-added non-terminal status is listed under the Running tab but counted into the History badge (or vice versa), with no error anywhere.
| function isActiveStatus(status: DaemonSessionTaskStatus['status']): boolean { | |
| return status === 'running' || status === 'pausing' || status === 'paused'; | |
| } | |
| function isActiveStatus(status: DaemonSessionTaskStatus['status']): boolean { | |
| return isTaskActive(status); | |
| } |
(建议导出 TasksStatusMessage.tsx 既有的 isActive(或移入共享模块)并在此复用,删除重复实现。)
中文说明
R2-16:此处手工复制了 TasksStatusMessage.tsx 既有的 isActive 谓词——同一 active/history 划分现在在同 package 的两处定义。漂移风险是具体的而非假设的:本 PR 刚在其中一个姊妹实现中把活跃集合扩展了 'pausing'。具体代价:'Running'/'History' 标签徽章由 isActiveStatus(counts memo)计算,而这些标签渲染的列表由 TasksStatusMessage 的 isActive 经 tasksForView 过滤。若 workflow 生命周期集合再次变化且只更新其中一个谓词,徽章与列表会静默不一致——处于新增非终态的 run 会列在 Running 标签下却被计入 History 徽章(或反之),任何地方都不报错。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| dispatchQueued: (event) => { | ||
| registry?.onDispatchQueued(runId, event); | ||
| emitUpdate(); | ||
| }, |
There was a problem hiding this comment.
[Suggestion] R1-51: each of the three new dispatch-lifecycle emitter hooks fires emitUpdate(), even though dispatchQueued fires in the same instant as the already-redrawing agentDispatched and dispatchSettled fires right before the already-redrawing budgetUpdated — against the redraw-minimization discipline documented on the adjacent agentCompleted hook ('No emitUpdate: budgetUpdated fires right after … avoids 2x TUI redraws per agent'). — Concrete cost: a foreground (non-run_in_background) workflow run — the interactive path where onUpdate drives updateOutput(buildLivePhaseTreeDisplay(entry)) — executes a fan-out near the 1000-agent cap with per-dispatch emitUpdate calls going from 2 to 5 (live) / 1 to 3 (cached resume); each synchronously rebuilds the live phase-tree display and pushes a TUI redraw — a ~2.5x redraw multiplier with quadratic total work, degrading exactly the runs the new tracing targets. Background runs (onUpdate undefined) are unaffected.
(建议:去掉 dispatchQueued 与 dispatchSettled 中的 emitUpdate()(其状态变化搭上紧随的 agentDispatched/budgetUpdated 重绘);仅当 slot-acquire 转变需要渲染时保留 dispatchStarted 的调用。)
中文说明
R1-51:三个新的 dispatch 生命周期 emitter hook 各自触发 emitUpdate(),尽管 dispatchQueued 与已经触发重绘的 agentDispatched 同时触发、dispatchSettled 紧挨在已经触发重绘的 budgetUpdated 之前——违背相邻 agentCompleted hook 上记载的重绘最小化纪律('No emitUpdate: budgetUpdated fires right after … avoids 2x TUI redraws per agent')。具体代价:前台(非 run_in_background)workflow run——onUpdate 驱动 updateOutput(buildLivePhaseTreeDisplay(entry)) 的交互路径——在接近 1000-agent 上限的扇出中,每 dispatch 的 emitUpdate 调用从 2 次升到 5 次(实跑)/ 1 次升到 3 次(缓存 resume);每次都同步重建实时阶段树显示并推送 TUI 重绘——约 2.5 倍重绘乘数、总工作量平方级,恰恰拖慢新追踪所针对的 run。后台 run(onUpdate 为 undefined)不受影响。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| prompt: stripAnsiAndControl(event.prompt).slice(0, 4_096), | ||
| status: event.cached ? 'cached' : 'queued', |
There was a problem hiding this comment.
[Suggestion] R1-52: the new cached-dispatch recording branch — status: event.cached ? 'cached' : 'queued', endedAt: queuedAt, the paired dispatch-queued + dispatch-cached events in onDispatchQueued, and the cached-preserving arm of onDispatchSettled — has zero test coverage anywhere (all seven onDispatchQueued test call sites omit cached; grep 'cached: true' matches only production code; the one 'cached' UI fixture is hand-built input). — Concrete cost: the orchestrator's P6 resume path reports cached: true dispatches to onDispatchQueued (workflow-orchestrator.ts:1389-1402). If a refactor drops or flips the event.cached ternaries (or the endedAt spread), replay-served dispatches are recorded as 'queued' with no endedAt: the persisted snapshot, exported trace, and dispatch graph show them pending forever, and no test fails.
(建议:补 registry 测试——以 cached: true 调 onDispatchQueued,断言 status: 'cached'、endedAt === queuedAt、事件序列 [dispatch-queued, dispatch-cached],且之后的 onDispatchSettled 保持 'cached' 而非翻转为 'completed'。)
中文说明
R1-52:新的缓存 dispatch 记录分支——status: event.cached ? 'cached' : 'queued'、endedAt: queuedAt、onDispatchQueued 中成对的 dispatch-queued + dispatch-cached 事件,以及 onDispatchSettled 保留 'cached' 的分支——在任何地方都没有测试覆盖(onDispatchQueued 的全部 7 个测试调用点都未传 cached;grep 'cached: true' 只命中生产代码;唯一的 'cached' UI fixture 是手工构造的输入)。具体代价:orchestrator 的 P6 resume 路径会向 onDispatchQueued 报告 cached: true 的 dispatch(workflow-orchestrator.ts:1389-1402)。若重构删除或翻转 event.cached 三元式(或 endedAt 展开),replay 供数的 dispatch 会被记录为无 endedAt 的 'queued':持久化快照、导出轨迹与 dispatch 图将永远显示它们处于挂起,且没有任何测试失败。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| pendingApprovals: [ | ||
| { | ||
| approvalId: 'wfap-1', | ||
| subagentId: 'correctness-agent-1', | ||
| name: 'write_file', | ||
| description: 'Update the implementation', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[Suggestion] R2-22: this test never asserts that callId and confirmationDetails are stripped from serialized workflow pending approvals — serializeWorkflowTask hand-picks the five fields of ServeWorkflowApprovalStatusEntry (approvalId/subagentId/name/description/at) while mapping phaseVisits/dispatches/events with spreads, and this deliberate picking has no regression guard. The fixture deliberately supplies callId: 'call-1' and confirmationDetails while toMatchObject subset-matches only the five expected keys. — Concrete cost: if a future edit aligns approvals with the sibling pattern — entry.pendingApprovals.map((approval) => ({ ...approval })) — tsc will NOT flag it (object spreads bypass excess-property checking) and this test still passes; the regression then ships callId plus the full confirmationDetails payload (file diffs, command details) in every tasks-status response, which web-shell polls every ~3 s — every connected client receives the process-local approval internals on every poll.
| pendingApprovals: [ | |
| { | |
| approvalId: 'wfap-1', | |
| subagentId: 'correctness-agent-1', | |
| name: 'write_file', | |
| description: 'Update the implementation', | |
| }, | |
| ], | |
| pendingApprovals: [ | |
| { | |
| approvalId: 'wfap-1', | |
| subagentId: 'correctness-agent-1', | |
| name: 'write_file', | |
| description: 'Update the implementation', | |
| }, | |
| ], |
(并补负向断言:expect(task.pendingApprovals?.[0]).not.toHaveProperty('callId'); 与 ...not.toHaveProperty('confirmationDetails');。)
中文说明
R2-22:此测试从未断言序列化的 workflow 待审批项剥离了 callId 与 confirmationDetails——serializeWorkflowTask 对 phaseVisits/dispatches/events 使用展开映射,却手工挑选 ServeWorkflowApprovalStatusEntry 的五个字段(approvalId/subagentId/name/description/at),而这一刻意挑选没有任何回归守卫。fixture 刻意提供 callId: 'call-1' 与 confirmationDetails,toMatchObject 却只子集匹配五个预期键。具体代价:若未来改动把审批项对齐姊妹模式——entry.pendingApprovals.map((approval) => ({ ...approval }))——tsc 不会报错(对象展开绕过多余属性检查),此测试仍通过;回归随即在每次 tasks-status 响应中带上 callId 与完整 confirmationDetails 载荷(文件 diff、命令详情),而 web-shell 约每 3 秒轮询一次——每个已连接客户端每次轮询都收到进程内部的审批细节。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect( | ||
| container.querySelector('button[aria-label="Workflows"]'), | ||
| ).toBeNull(); |
There was a problem hiding this comment.
[Suggestion] R2-26: this workflows-disabled test's sidebar assertion is vacuous — App.test.tsx vi.mocks WebShellSidebar (line 869) with a stub that renders only data-testid buttons (open-add-workspace, new-session, load-session, open-plugins, open-channels, open-daemon-status, open-sessions-overview, open-split-view) and never anything with aria-label="Workflows" (the mock's props don't even include onOpenWorkflows) — so the assertion passes for any gate state. — Concrete cost: a regression removing the workflowsEnabled gate from the sidebar's Workflows entry (WebShellSidebar.tsx:4099, aria-label={t('sidebar.workflows')}) would still show the button in the real UI, yet this App-level test stays green — zero gating signal while reading as if it verified 'no sidebar entry when disabled'. (The sidebar's own suite covers the gate on both sides, WebShellSidebar.workspace-removal.test.tsx:3061/3074, so no concrete regression escapes today — the cost is false confidence.)
(建议:删除此测试中的 button[aria-label="Workflows"] 断言——其余两个断言(workflow-runs-page 不存在、/workflows 转发给 sendPrompt)才是该 mock 设置真正能验证的;或改为断言 mock 暴露的 sidebar props。)
中文说明
R2-26:此 workflows-disabled 测试的侧边栏断言是空洞的——App.test.tsx 用 vi.mock 把 WebShellSidebar 换成只渲染 data-testid 按钮(open-add-workspace、new-session、load-session、open-plugins、open-channels、open-daemon-status、open-sessions-overview、open-split-view)的 stub,从不渲染任何带 aria-label="Workflows" 的元素(mock 的 props 甚至不含 onOpenWorkflows)——因此该断言在任何 gate 状态下都通过。具体代价:删除侧边栏 Workflows 入口上 workflowsEnabled gate 的回归(WebShellSidebar.tsx:4099,aria-label={t('sidebar.workflows')})在真实 UI 中仍会显示按钮,而此 App 级测试保持绿色——看似验证了"关闭时无侧边栏入口",实际毫无 gate 信号。(侧边栏自己的套件在两侧都覆盖了该 gate,WebShellSidebar.workspace-removal.test.tsx:3061/3074,因此今天没有具体回归能逃脱——代价是虚假的信心。)
— qwen3.8-max via Qwen Code /review (v0.21.10)
Superseded by subsequent commit be15bb0; current head requires re-review after scope split.
Note
This draft is the top layer of a three-PR stack: Core #9034 → daemon/API #9035 → Web Shell UI #8950. Its current
maindiff temporarily includes both prerequisites. After they merge, this branch will mergemainnormally so the review diff shrinks to the Web Shell layer without force-pushing. Please review #9034 first.What this PR does
This PR adds a first-class Dynamic Workflow experience to Web Shell. Workflow runs appear as live execution graphs with phase lanes, dependency edges, dispatch state, token usage, approval state, and pause, resume, stop, retry, and rerun controls. A dedicated Workflow page lists reusable saved workflows, active runs, and persisted history with filtering, comparison, export, rerun, and deletion actions.
This PR is now scoped to the Web Shell presentation and interaction layer. The Core execution state is owned by #9034, and the daemon/SDK contract is owned by #9035.
Why it's needed
Web Shell previously exposed Dynamic Workflow execution only through terminal text and generic task state. Users could not see how agents and phases were related, distinguish queued and running work, control a run safely, or revisit prior executions. The dedicated view makes the execution model visible and interactive while preserving the default-off product behavior.
Reviewer Test Plan
How to verify
maininto this branch and confirm only Web Shell files remain in the diff.Evidence (Before & After)
Before: Workflow execution was visible only through terminal/tool output and generic background-task state.
After: Live runs render as interactive execution traces and completed runs remain available for history management and reuse. Disabled workspaces continue to hide the feature.
The split itself changes commit ancestry only; the tree is identical to the prior #8950 head.
Tested on
Environment (optional)
Local TypeScript daemon and Vite Web Shell on macOS. Existing focused unit and browser evidence remains attached to this PR; the split introduced no source changes.
Risk & Scope
Linked Issues
Closes #8941
中文说明
本 PR 做了什么
本 PR 为 Web Shell 增加一等的 Dynamic Workflow 体验。Workflow 运行会展示为实时执行图,包含阶段泳道、依赖边、调度状态、Token 用量、审批状态,以及暂停、继续、停止、重试和重新运行控制。独立 Workflow 页面会列出可复用的已保存工作流、正在运行的任务和持久化历史,并提供筛选、比较、导出、重新运行和删除操作。
本 PR 现在仅负责 Web Shell 展示与交互层。Core 执行状态由 #9034 负责,daemon/SDK 契约由 #9035 负责。
为什么需要
此前 Web Shell 只能通过终端文本和通用任务状态间接呈现 Dynamic Workflow。用户无法看清 agent 与阶段的关系、区分排队和运行状态、安全控制某次运行,或回看之前的执行。独立视图在保持功能默认关闭的前提下,让执行模型变得可见且可交互。
Reviewer 测试计划
如何验证
mainmerge 到本分支,确认 diff 中只剩 Web Shell 文件。证据(改动前后)
改动前:Workflow 执行只能通过终端或 tool 输出及通用后台任务状态查看。
改动后:Live run 以可交互执行 trace 展示,完成的 run 可继续进行历史管理和复用;关闭能力的 workspace 仍隐藏该功能。
本次拆分只改变提交祖先关系,代码树与拆分前的 #8950 head 完全一致。
已测试平台
环境(可选)
macOS 本地 TypeScript daemon 和 Vite Web Shell。既有聚焦单测与浏览器证据仍保留在本 PR 中;拆分未引入源码变化。
风险与范围
关联 Issue
Closes #8941