feat(serve): expose Workflow tasks and controls - #9546
Conversation
github-actions
Bot
added
the
review/self-reported
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-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 12 scenario(s). — Qwen Code · serve A/B |
|
Re-run of the gate at the updated head — the verdict doesn't change, the footer does.
Moving on to code review. 🔍 中文说明在更新后的 head 上重跑门禁——结论不变，页脚更新。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review — re-run on
|
| File | What changed |
|---|---|
packages/cli/src/serve/workflow-session-gate.ts |
New: daemon-boundary redaction helper for supported-commands (the trust fix) |
packages/cli/src/serve/routes/session.ts |
Gates supported-commands, the tasks opt-in, cancel and the new workflow-action route on runtime.trusted |
packages/cli/src/serve/acp-http/dispatch.ts |
Same four gates for the ACP-HTTP methods; registers the two new vendor methods |
packages/cli/src/serve/routes/capabilities.ts |
Advertises per-workspace workflowsEnabled (enabled, trusted, kill-switch aware) |
packages/cli/src/serve/run-qwen-serve.ts |
Carries tools.workflowsEnabled into runtime env metadata, including settings reload |
packages/cli/src/serve/server.ts |
Wires daemon env into the capabilities route |
packages/cli/src/serve/types.ts |
workflowsEnabled on the capabilities envelope |
packages/cli/src/serve/server/telemetry.ts |
Registers the new route in the telemetry catalog (drift guard updated) |
packages/cli/src/serve/workspace-registry.ts |
workflowsEnabledBySettings metadata field |
packages/cli/src/acp-integration/acpAgent.ts |
Child handlers: control actions, run-saved, retry/rerun lineage, child gate, cross-session liveness |
packages/cli/src/acp-integration/session/Session.ts |
History cache, race-safe deletion, workflow holds and completion notifications |
packages/cli/src/acp-integration/session/tasksSnapshot.ts |
Projects live and persisted workflow runs only when opted in |
packages/core/src/agents/runtime/workflow-runner.ts |
Notifies the registry when a terminal snapshot is persisted |
packages/core/src/agents/workflow-run-registry.ts |
removeTerminal and the snapshot-persisted callback |
packages/core/src/agents/workflow-snapshot.ts |
writeWorkflowSnapshot reports whether it persisted |
packages/core/src/tools/workflow/workflow.ts |
buildSessionOwnedBackground for daemon-started runs |
packages/acp-bridge/src/bridge.ts |
includeWorkflows opt-in and the workflow-action bridge method, clientId checks |
packages/acp-bridge/src/bridgeTypes.ts |
Contract types; workflow joins the active-work hold categories |
packages/acp-bridge/src/status.ts |
Workflow status/event/approval types and the new control ext-method |
packages/sdk-typescript/src/daemon/DaemonClient.ts |
sessionWorkflowTasks and sessionWorkflowTaskAction; legacy methods unchanged |
packages/sdk-typescript/src/daemon/DaemonSessionClient.ts |
workflowTasks and controlWorkflowTask session helpers |
packages/sdk-typescript/src/daemon/acpRouteTable.ts |
Maps the cancel and workflow-action routes and the includeWorkflows query |
packages/sdk-typescript/src/daemon/types.ts |
Workflow types; the legacy DaemonSessionTaskStatus union is untouched |
packages/sdk-typescript/scripts/build.js |
Browser bundle budget +1 KiB for the added surface |
packages/webui/src/daemon/session/actions.ts |
Adapter actions: getWorkflowTasks, controlWorkflowTask, runSavedWorkflow |
packages/webui/src/daemon/session/types.ts |
Action contract and notice operations |
| …and 21 more files | Collocated tests covering every layer above |
Test evidence
This is an unattended CI run — I did not build or execute any PR code. The evidence below is the PR's own CI at the reviewed commit, fetched via the API, plus the bot signals that corroborate it.
| Check | Conclusion |
|---|---|
| Qwen Code CI · Test (ubuntu-latest, Node 22.x) | ✅ success |
| Qwen Code CI · Test (macos-latest / windows-latest, Node 22.x) | ⏭️ skipped |
| Qwen Code CI · Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
| Serve A/B (ubuntu-latest, Node 22.x) | ✅ success — 0 response changes vs PR base, 12 scenarios |
| Capture web-shell visuals (ubuntu-latest, Node 22.x) | ✅ success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ✅ success |
| Desktop Shell (ubuntu-22.04 / windows-2022) | ✅ success |
| Live Host (macos-latest) | ✅ success |
| Security Checks · Dependency CVE audit · Secret scan (TruffleHog) · precheck-pr | ✅ success |
| SDK Java · Java 11 and Java 21 legs (ubuntu / macos / windows) | ✅ success |
| SDK Java · ubuntu-latest / Java 17 | |
| Real daemon E2E / Java 11 | ✅ success |
The skips are not this PR's doing: the macOS/Windows unit legs and the no-sandbox integration leg are skipped identically on recently merged PRs (compared against #10059) — that is this repo's standard CI shape. The Java 17 cancellation died during runner setup ("A task was canceled" while downloading actions, before any checkout); its Java 11 and Java 21 siblings in the same workflow are green, so I classify it as runner-preemption noise rather than PR-caused.
Not verified: a live re-run of the untrusted-workspace arm against e80f785. The maintainer's stack verification proved every behavior at e139338 except the one that was broken there, and the fix commits since are pinned only by the new automated tests. Sandboxed verification would settle it: @qwen-code /verify — that the untrusted workspace now fails closed live (capability, supported-commands, all control actions, and the includeWorkflows read path), which the diff and the unit suite argue for but nobody has executed end-to-end on this head.
中文说明
代码审查 —— 在 e80f785 上重跑，聚焦上次审查之后落地的改动
本 PR 的形态与我第一轮独立提出的方案一致（在现有会话任务面上 opt-in、一条控制路由、独立的 Workflow 感知 SDK/adapter 方法、单一的"启用+非 bare+可信"谓词），因此本轮集中在上次审查之后、也是在 @wenshao 真实栈验证之后落地的两个提交：信任门禁修复（3497d0d）与删除竞态修复（c25a5f9）。
维护者的问题 1（不受信工作区仍宣告并执行 workflow）已按验证评论建议的方式修复。 daemon 边界现在用自己的信任判定强制管控每个 Workflow 面，两种传输都覆盖：
- REST（
routes/session.ts）：/supported-commands被脱敏（workflowsEnabled:false、savedWorkflows:[]、移除内建/workflows、其余命令不受影响）；tasks?includeWorkflows=true在到达 bridge 前被强制改为false；workflow-action返回{changed:false}；cancel kind:workflow返回{cancelled:false, reason:'disabled'}—— 全部基于runtime.trusted，且在任何 bridge 调用之前检查。 - ACP-HTTP（
acp-http/dispatch.ts）：同样四个方法以isWorkspaceTrusted()做门，该谓词在所有真实构造点（server.ts、run-qwen-serve.ts的三条 runtime 路径）都接到runtime.trusted—— 与/capabilities用的是同一谓词族，因此"宣告的能力"与"会话门"不会在信任上再分歧。 - 子进程侧
canUseWorkflowControls的信任子句保留为直连qwen --acp场景的纵深防御；它在生产中的盲点（packages/cli从不设置 folder-trust feature）经由 daemon 已不再重要，因为边界先行脱敏。 - 回归测试在两种传输上钉住了生产真实形态（
server.test.ts、acp-http/transport.test.ts）：不受信的 daemon runtime 得到上述失败关闭响应，并且证明变更请求与includeWorkflowsopt-in 根本没有到达子进程。这正是验证评论要求补的测试。
c25a5f9 关闭了跨会话删除竞态。 所有会话共享同一个快照存储但各自持有私有 registry，因此删除现在：(1) 任一兄弟 registry 仍持有活动或未落盘运行则拒绝（isWorkflowRunLiveOutsideSession）；(2) 删除前排空属主 handle；(3) 成功后移除兄弟终态条目——这同时解决了维护者的非阻塞观察 3；(4) 仅在 writeWorkflowSnapshot 报告成功后才清掉未持久化的历史缓存，兄弟删除不会再"复活"运行。Session.test.ts 里十几个新测试逐一钉住每个竞态。
上次只能静态阅读的部分现在有了实证。 @wenshao 在 e139338（这两个修复之前的那个提交）上驱动了真实 daemon 栈：44/44 控制生命周期检查、失败关闭输入、活动工作保持、重启/历史语义、SDK 对 base 的向后兼容、bare 模式、设置热加载。此后的提交只触及门禁与删除路径；仓库自己的 Serve A/B 检查在当前 head 上报告相对 PR base 零响应变化，即旧契约在 e80f785 依然成立。维护者其余非阻塞观察（run-saved 后约 100 ms 才填 label、X-Qwen-Client-Id 头可选、并发 rerun 语义、完成通知可见性）维持原议；均非泄漏或正确性问题。
阅读门禁时的一条遗留备注：子进程的 sessionTasks ext-method 不带自身门禁地接受 includeWorkflows。经 daemon 时没问题——边界已强制改写。直连 qwen --acp 时它只读取发起用户自己的快照存储，不跨越任何边界；写出来是为了防止以后有人"修"错层。
测试证据
这是无人值守的 CI 运行——我没有构建或执行任何 PR 代码。下方证据是审查提交上 PR 自己的 CI（经 API 获取），以及相互印证的 bot 信号。CI 表格见英文部分（机器可读区域，由 finalize 工作流在 CI 落定后就地更新）。
跳过的检查不是本 PR 造成的：macOS/Windows 单测腿与 no-sandbox 集成腿在最近已合并的 PR（对照 #10059）上同样被跳过——这是本仓库的标准 CI 形态。Java 17 的取消死于 runner 初始化阶段（下载 actions 时 "A task was canceled"，尚在任何 checkout 之前）；同一工作流的 Java 11 与 Java 21 腿为绿，故判定为 runner 抢占噪音而非 PR 所致。
未验证：在 e80f785 上实跑不受信工作区臂。 维护者的栈验证在 e139338 上证明了除当时坏掉的那一项之外的所有行为，而其后的修复提交目前只有新自动化测试钉住。沙箱验证可以定案：@qwen-code /verify —— 不受信工作区现在是否真的失败关闭（能力宣告、supported-commands、全部控制动作、includeWorkflows 读取路径），diff 与单测都支持这一结论，但还没有人在这个 head 上端到端执行过。
— Qwen Code · qwen3.8-max
Reviewed at e80f78518601e27eec4408c577ffa21ff9bf144a · re-run with @qwen-code /triage
|
Confidence: 3/5 — the review itself is clean (the maintainer's one blocking finding is fixed exactly as prescribed and pinned by tests on both transports), but a core-touching Stepping back over both runs: the approach is the one I would have chosen — opt-in on the existing task surface, one control route, separate SDK/adapter methods, one predicate at the daemon boundary. The maintainer's real-stack verification at Why defer instead of approve, then:
@wenshao — you verified this stack on real daemons and named the one thing that needed fixing; that fix and its regression tests are now in. Ready for your sign-off (or a 中文说明置信度：3/5 —— 审查本身是干净的（维护者唯一的阻塞性问题已按其建议的方式修复，并由两种传输上的测试钉住），但触及核心、约 1,500 行生产代码的 退后看两轮审查：方案正是我会选的那一个——在现有任务面上 opt-in、一条控制路由、独立的 SDK/adapter 方法、daemon 边界一个谓词。维护者在 那为什么是暂缓而不是批准：
@wenshao —— 你在真实 daemon 上验证了这个栈并点名了唯一需要修的问题；该修复及其回归测试现已就位。随时可以给出你的签核（或跑一次 — Qwen Code · qwen3.8-max Reviewed at |
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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| }); | ||
| }); | ||
|
|
||
| it('restores persisted workflow runs as read-only task history', () => { |
There was a problem hiding this comment.
[Suggestion] No test exercises the legacy persisted-snapshot shape (absent phaseVisits / dispatches / events) that serializeWorkflowSnapshot carries explicit ?? [] / ?.map guards for — every fixture in the new suite supplies all three fields (phaseVisits/dispatches guards are exercised by no test). Probe: removing the two ?? [] guards survives 14/14 tests. Shipped, one pre-graph-tracing <projectDir>/workflows/wf_*.json on disk passes isWorkflowSnapshot (which deliberately accepts the absent fields), reaches serializeWorkflowSnapshot, throws TypeError: Cannot read properties of undefined (reading 'map') inside the tasks array composition — and the entire session-tasks response fails for every includeWorkflows=true poller: agent, shell, and monitor tasks disappear with it. Part of a pattern: the workflow serializer/projection guards lack pinning tests.
Fix: in the new workflow graph block, pass a snapshot with the three keys deleted plus { includeWorkflows: true }, asserting it still serializes as a historical task with empty phaseVisits/dispatches and no events field.
中文说明
[Suggestion] 没有测试覆盖旧版持久化快照形态（缺少 phaseVisits / dispatches / events），而 serializeWorkflowSnapshot 明确带着 ?? [] / ?.map 守卫——新套件中的每个 fixture 都提供了全部三个字段（phaseVisits/dispatches 守卫没有任何测试执行到）。探针：移除两个 ?? [] 守卫后 14/14 个测试存活。若带病上线，磁盘上一个图形追踪之前的 <projectDir>/workflows/wf_*.json 会通过 isWorkflowSnapshot（它刻意接受缺失字段），进入 serializeWorkflowSnapshot，在 tasks 数组组装过程中抛出 TypeError: Cannot read properties of undefined (reading 'map')——整个 session-tasks 响应对每个 includeWorkflows=true 轮询者失败：agent、shell、monitor 任务一并消失。这是一个模式：workflow 序列化/投影守卫缺少锁定测试。
修复：在新的 workflow graph 块中，传入删除了这三个键的快照并附带 { includeWorkflows: true }，断言它仍能序列化为历史任务：phaseVisits/dispatches 为空且没有 events 字段。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(tasks).toMatchObject({ | ||
| sessionId, | ||
| tasks: [ | ||
| { | ||
| kind: 'workflow', | ||
| id: 'wf_saved', |
There was a problem hiding this comment.
[Suggestion] This assertion uses loose toMatchObject while the fixture deliberately carries script: 'return 1;' — it cannot catch a leak of the snapshot's script/scriptPath fields, which serializeWorkflowSnapshot intentionally does not project. Probe: adding script/scriptPath to that serializer passes all 14 workflow tests — the full workflow source plus an absolute filesystem path would flow to every daemon/SDK/WebUI client calling sessionTasks with includeWorkflows=true, and this test stays green (toMatchObject ignores extra properties). The sibling capabilities assertion in the same test is strict toEqual (it would catch a scriptPath leak in savedWorkflows), and the file already has the house pattern for absence checks (expect(JSON.stringify(tasks)).not.toContain('abortController')). Part of a pattern: the workflow serializer/projection guards lack pinning tests.
Fix: after the toMatchObject block, add expect(JSON.stringify(tasks)).not.toContain('return 1;'); and expect(JSON.stringify(tasks)).not.toContain('scriptPath');.
中文说明
[Suggestion] 该断言使用宽松的 toMatchObject，而 fixture 特意携带了 script: 'return 1;'——它无法捕获快照 script/scriptPath 字段的泄漏，而 serializeWorkflowSnapshot 是刻意不投影这两个字段的。探针：把 script/scriptPath 加入该序列化器后，全部 14 个 workflow 测试通过——完整 workflow 源码加绝对文件系统路径会流向每个以 includeWorkflows=true 调用 sessionTasks 的 daemon/SDK/WebUI 客户端，而该测试保持绿色（toMatchObject 忽略多余属性）。同一测试中兄弟的 capabilities 断言是严格 toEqual（能捕获 savedWorkflows 中的 scriptPath 泄漏），且本文件已有"缺失性检查"的惯用写法（expect(JSON.stringify(tasks)).not.toContain('abortController')）。这是一个模式：workflow 序列化/投影守卫缺少锁定测试。
修复：在 toMatchObject 块之后增加 expect(JSON.stringify(tasks)).not.toContain('return 1;'); 和 expect(JSON.stringify(tasks)).not.toContain('scriptPath');。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| }); | ||
|
|
||
| describe('buildSessionTasksStatus workflow graph', () => { | ||
| it('omits workflow tasks unless the caller opts in', () => { |
There was a problem hiding this comment.
[Suggestion] The opt-out test only exercises the persisted-history branch: configWith([]) leaves the workflow registry empty, so the test cannot detect a regression that drops the includeWorkflows gate from the live-registry mapping spread in buildSessionTasksStatus. Probe: ungating the live-registry read survives 10/10 tests; giving this test a non-empty registry fails under the mutant and passes on this PR's code. Shipped, default GET /session/<id>/tasks pollers that never pass includeWorkflows (pre-feature SDK/webui consumers) start receiving live kind: 'workflow' tasks — an unknown task kind and unrequested payload — with no test in the repo failing. Part of a pattern: the workflow serializer/projection guards lack pinning tests.
Fix: give the opt-out test a non-empty registry as well — reuse the graph-test's live-task fixture alongside [workflowSnapshot()], keeping expect(snapshot.tasks).toEqual([]).
中文说明
[Suggestion] opt-out 测试只覆盖了持久化历史分支：configWith([]) 让 workflow registry 为空，因此该测试无法检测到"从 buildSessionTasksStatus 的实时 registry 映射展开中移除 includeWorkflows 门禁"的回归。探针：解除实时 registry 读取的门禁后 10/10 个测试存活；给该测试一个非空 registry 后，突变体下失败、本 PR 代码下通过。若带病上线，从不传 includeWorkflows 的默认 GET /session/<id>/tasks 轮询者（功能上线前的 SDK/webui 消费者）会开始收到实时的 kind: 'workflow' 任务——未知任务类型、未被请求的载荷——而仓库中没有任何测试失败。这是一个模式：workflow 序列化/投影守卫缺少锁定测试。
修复：让 opt-out 测试也带上非空 registry——复用 graph 测试的实时任务 fixture，与 [workflowSnapshot()] 并存，保持 expect(snapshot.tasks).toEqual([])。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| workflowsEnabled: false, | ||
| kind: 'live', |
There was a problem hiding this comment.
[Suggestion] This new workflowsEnabled assertion resolves against the host's process.env: the harness injects no daemonEnv dep and the fake runtime uses env: { mode: 'parent-process' }, so workflowsEnabledForRuntime falls back to deps.daemonEnv ?? process.env (server.ts). A developer or CI host exporting QWEN_CODE_ENABLE_WORKFLOWS=1 (dogfooding workflows, or a workflow-exercising CI job) flips the advertisement to true and this exact-equality assertion fails spuriously. Probe: clean env — passes; QWEN_CODE_ENABLE_WORKFLOWS=1 — fails with - "workflowsEnabled": false / + "workflowsEnabled": true. The same class affects the envelope assertions this PR added in multi-workspace-sessions.test.ts (makeHarness is likewise unshielded); the author already applied the shield elsewhere in this diff (daemonEnv: {} in the dynamic-workspace test, explicit daemonEnv in the new capabilities test).
Fix: pass daemonEnv: {} in the createServeApp deps here and in makeHarness in multi-workspace-sessions.test.ts.
中文说明
[Suggestion] 这个新增的 workflowsEnabled 断言依赖宿主机的 process.env：测试 harness 未注入 daemonEnv 依赖，且假运行时使用 env: { mode: 'parent-process' }，因此 workflowsEnabledForRuntime 回退到 deps.daemonEnv ?? process.env（server.ts）。任何导出 QWEN_CODE_ENABLE_WORKFLOWS=1 的开发者或 CI 宿主（内测 workflows，或运行 workflow 相关套件的 CI 任务）都会把声明翻转为 true，使这个精确相等断言虚假失败。探针：干净环境——通过；QWEN_CODE_ENABLE_WORKFLOWS=1——失败，- "workflowsEnabled": false / + "workflowsEnabled": true。同类问题也影响本 PR 在 multi-workspace-sessions.test.ts 中新增的 envelope 断言（makeHarness 同样未屏蔽）；作者已在本 diff 的其他地方应用了屏蔽（动态工作区测试中的 daemonEnv: {}、新 capabilities 测试中的显式 daemonEnv）。
修复：在此处与 multi-workspace-sessions.test.ts 的 makeHarness 中，给 createServeApp 的 deps 传入 daemonEnv: {}。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| getWorkflowHistory(): readonly WorkflowSnapshot[] { | ||
| return this.workflowHistory; | ||
| } |
There was a problem hiding this comment.
[Suggestion] getWorkflowHistory() has zero production callers (grep across all packages: only the definition, Session.test.ts, and the acpAgent.test.ts session mock), and the live cache fed by #rememberWorkflowHistory is never observably consumed — every production read path re-derives from disk first (buildSessionTasksStatus → refreshWorkflowHistory(); deleteWorkflowHistory also refreshes before its membership check). Meanwhile every ACP session/new pays an unconditional listWorkflowSnapshots(config) preload — readdir plus sequential read + JSON.parse + schema validation of every retained snapshot — even when workflows are disabled; the new test pins that unconditionality with toHaveBeenCalledTimes(2) while running with isWorkflowsEnabled: false. The Session.test.ts cache test blesses live-cache behavior no production path can observe: a maintainer investigating "completed runs missing from history" sees green cache machinery and concludes live updates are wired, and future edits to the cache pass tests while changing nothing user-visible.
Fix: either route a production read through the cache (e.g. the tasks projection uses getWorkflowHistory() between refreshes) or drop the preload + constructor parameter + getWorkflowHistory() and derive purely from the disk listing; align the tests with whichever shape ships. If the accessor is intended for an imminent caller, name it in this PR.
中文说明
[Suggestion] getWorkflowHistory() 没有任何生产调用方（跨所有包 grep：只有定义、Session.test.ts 和 acpAgent.test.ts 的会话 mock），且 #rememberWorkflowHistory 维护的实时缓存从未被可观测地消费——每个生产读取路径都先从磁盘重新推导（buildSessionTasksStatus → refreshWorkflowHistory()；deleteWorkflowHistory 在成员检查前也会 refresh）。与此同时，每个 ACP session/new 都会支付一次无条件的 listWorkflowSnapshots(config) 预加载——readdir 加逐个 read + JSON.parse + 每个保留快照的 schema 校验——即使 workflows 未启用；新测试还在 isWorkflowsEnabled: false 下以 toHaveBeenCalledTimes(2) 锁定了这种无条件性。Session.test.ts 的缓存测试为一条生产路径不可观测的实时缓存行为背书：维护者排查"已完成运行从历史中消失"时看到绿色的缓存机制会认为实时更新已接通；未来对缓存的改动能通过测试，却不改变任何用户可见行为。
修复：或者让某个生产读取走缓存（例如任务投影在两次 refresh 之间使用 getWorkflowHistory()），或者移除预加载 + 构造函数参数 + getWorkflowHistory()、完全从磁盘列表推导；让测试与实际落地的形态对齐。如果该访问器是为即将到来的调用方准备的，请在本 PR 中指明。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Superseded by subsequent commit; current head requires re-review.
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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const task = registry.get(taskId); | ||
| if (!task) return { changed: false }; |
There was a problem hiding this comment.
[Suggestion] R2-3: retry/rerun resolve the target only from the in-memory registry, so snapshot-only runs (after a daemon restart or terminal eviction at 10 retained entries) are listed by includeWorkflows yet fail closed with a bare {changed:false} carrying no reason — indistinguishable from a disabled control or a precondition miss, while the sibling cancel path returns reason:'not_found'|'not_running' for the same kind. The refusal itself matches the PR's documented contract ("controls against historical-only states should fail closed") and WorkflowSnapshot has no args field, so the gap is only the missing reason code, not the refusal.
Concrete cost: after a daemon restart, a previously failed run appears in sessionTasksStatus(includeWorkflows=true) with status:'failed'; a client sending {action:'retry', taskId:'wf_x'} gets bare {changed:false} and cannot tell unknown/historical from precondition-not-met, while delete-history succeeds for the same run — an asymmetry visible inside this one endpoint. Fix: return {changed:false, reason:'not_found'} on the registry miss, mirroring the cancel path.
中文说明
retry/rerun 只从内存 registry 解析目标，因此仅有快照的运行（daemon 重启后或终态条目超过 10 条被逐出后）会被 includeWorkflows 列出，但控制请求只得到不带 reason 的 {changed:false}——与"功能被禁用"或"前置条件不满足"无法区分；而同为 workflow 的 cancel 分支会返回 reason:'not_found'|'not_running'。拒绝本身符合 PR 文档约定（"对仅历史状态的控制应 fail closed"），且 WorkflowSnapshot 没有 args 字段，因此缺口只在缺少 reason 码，而不在于拒绝行为。修复：对 registry 未命中返回 {changed:false, reason:'not_found'}，与 cancel 分支对齐。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (action === 'delete-history') { | ||
| return { changed: await session.deleteWorkflowHistory(taskId) }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-4: A successful delete-history never removes the run's live WorkflowRunRegistry entry — the registry has no single-entry removal and retains up to 10 terminal entries — so the deleted run stays listed by includeWorkflows (serializeWorkflowTask applies no status filter over registry.list()), the retry preconditions still pass for it, and retry re-registers the same runId: WorkflowJournal.load tolerates the deleted journal (empty replay), and settlement re-persists the snapshot — the deleted run is resurrected.
Observed by probe: after simulated delete-history with empty persisted history, the terminal entry still appears in the includeWorkflows listing ([{"id":"wf_ffff0000","status":"failed"}]); after retry settlement the snapshot is back on disk. The residue is in-process only (self-heals on restart) and resurrection needs an explicit client retry, but the endpoint reports {changed:true} while the same server keeps serving the run. Fix: give the registry a guarded terminal-entry removal (only non-active entries with no handle) and call it from Session.deleteWorkflowHistory after the disk delete succeeds.
中文说明
delete-history 成功后不会移除运行在 WorkflowRunRegistry 中的存活条目——registry 没有单条移除能力且最多保留 10 条终态条目——因此被删除的运行仍会出现在 includeWorkflows 列表中（serializeWorkflowTask 对 registry.list() 不做状态过滤），retry 前置条件对其仍然成立，retry 会用同一 runId 重新注册：WorkflowJournal.load 容忍已删除的 journal（空重放），结算时快照重新落盘——被删除的运行复活。
探针观察：模拟 delete-history（持久化历史为空）后，终态条目仍在 includeWorkflows 列表中；retry 结算后快照重新出现在磁盘上。残留仅在进程内（重启即自愈），复活也需要客户端显式 retry，但该端点在服务器仍在提供该运行的情况下返回了 {changed:true}。修复：为 registry 增加受保护的终态条目移除（仅限无 handle 的非活跃条目），并在磁盘删除成功后由 Session.deleteWorkflowHistory 调用。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const handle = registry.getHandle(taskId); | ||
| registry.cancel(taskId, Date.now()); | ||
| if (handle) await handle.completion; |
There was a problem hiding this comment.
[Suggestion] R2-5: The cancel test resolves the mock completion promise independently of the cancel call, so nothing pins that production must call registry.cancel() BEFORE await handle.completion here. In production the ordering is load-bearing: a paused run never settles naturally — WorkflowRunRegistry.cancel() is what terminates it (it aborts the run's controller/handle).
Probe witness: swapping the two statements leaves all 471 existing acpAgent tests green; an ordering probe whose completion settles only when cancel fires deadlocks under the swap (15s timeout) and passes on clean code. Shipped, the swap would hang cancellation of paused workflows until client timeout. Fix: make completion causally depend on cancel in the test — resolve inside the mock (const cancel = vi.fn(() => { task.status = 'cancelled'; resolveCompletion(); });) and delete the standalone resolveCompletion() call; under the swapped order nothing resolves completion and the test fails.
中文说明
cancel 测试中 mock 的 completion 与 cancel 调用相互独立地解决，因此没有任何测试钉住此处必须先调用 registry.cancel() 再 await handle.completion 的顺序。生产中该顺序是承重的：paused 运行不会自然结束——只有 WorkflowRunRegistry.cancel()（中止其 controller/handle）能使其结算。
探针证据：交换这两条语句后现有 471 个 acpAgent 测试全部通过；一个"只有 cancel 触发才结算 completion"的顺序探针在交换后死锁（15s 超时），在正确代码下通过。若交换被合入，暂停态 workflow 的取消会挂起直到客户端超时。修复：让测试中的 completion 因果依赖于 cancel——在 mock 内部解决（见上），并删除独立的 resolveCompletion() 调用。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| }); | ||
| }); | ||
|
|
||
| it('restores persisted workflow runs as read-only task history', () => { |
There was a problem hiding this comment.
[Suggestion] R1-21: Still stands at 135bf45 — tasksSnapshot.test.ts was not touched this round; no test exercises the legacy persisted-snapshot shape (absent phaseVisits / dispatches / events) that serializeWorkflowSnapshot carries explicit ?? [] / ?.map guards for — every fixture supplies the fields.
中文说明
R1-21 在 135bf45 仍然存在——本轮未改动 tasksSnapshot.test.ts；依旧没有测试覆盖旧版持久化快照形态（缺少 phaseVisits / dispatches / events），而 serializeWorkflowSnapshot 为此显式写了 ?? [] / ?.map 守卫——所有 fixture 都提供了这些字段。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| tasks: [ | ||
| { | ||
| kind: 'workflow', | ||
| id: 'wf_saved', |
There was a problem hiding this comment.
[Suggestion] R1-22: Still stands at 135bf45 — the persisted-snapshot assertion still uses loose toMatchObject while the fixture deliberately carries script: 'return 1;'; it cannot catch a leak of the snapshot's script/scriptPath fields, which serializeWorkflowSnapshot intentionally omits.
中文说明
R1-22 在 135bf45 仍然存在——持久化快照断言依旧使用宽松的 toMatchObject，而 fixture 刻意携带 script: 'return 1;'；它无法捕获快照 script/scriptPath 字段的泄漏，而 serializeWorkflowSnapshot 是刻意不输出它们的。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| }); | ||
|
|
||
| describe('buildSessionTasksStatus workflow graph', () => { | ||
| it('omits workflow tasks unless the caller opts in', () => { |
There was a problem hiding this comment.
[Suggestion] R1-23: Still stands at 135bf45 — the opt-out test still only exercises the persisted-history branch: configWith([]) leaves the workflow registry empty, so the test cannot detect a regression dropping the includeWorkflows gate from the live-registry mapping.
中文说明
R1-23 在 135bf45 仍然存在——opt-out 测试依旧只覆盖持久化历史分支：configWith([]) 使 workflow registry 为空，因此该测试无法发现从"实时 registry 映射"中移除 includeWorkflows 门禁的回归。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| primary: false, | ||
| trusted: true, | ||
| workflowsEnabled: false, | ||
| kind: 'live', |
There was a problem hiding this comment.
[Suggestion] R1-24: Still stands at 135bf45 — server.test.ts was not touched this round; the workflowsEnabled assertion still resolves against the host's process.env (no daemonEnv dep injected; the fake runtime uses env: { mode: 'parent-process' }), so its truth depends on the runner environment.
中文说明
R1-24 在 135bf45 仍然存在——本轮未改动 server.test.ts；workflowsEnabled 断言依旧解析到宿主机的 process.env（未注入 daemonEnv 依赖；假运行时使用 env: { mode: 'parent-process' }），其真假取决于运行环境。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| getWorkflowHistory(): readonly WorkflowSnapshot[] { | ||
| return this.workflowHistory; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-25: Still stands at 135bf45 — grep across all packages at HEAD confirms getWorkflowHistory() still has zero production callers (only the definition, Session.test.ts, and the acpAgent.test.ts session mock); the live cache fed by #rememberWorkflowHistory is never observed by the endpoint path (tasks status goes through refreshWorkflowHistory).
中文说明
R1-25 在 135bf45 仍然存在——在 HEAD 上跨包 grep 确认 getWorkflowHistory() 依旧没有生产调用方（只有定义、Session.test.ts 与 acpAgent.test.ts 的 session mock）；由 #rememberWorkflowHistory 维护的活跃缓存从未被端点路径观察（任务状态走的是 refreshWorkflowHistory）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Superseded by subsequent commit 4ab652a; the current head requires re-review.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-5 cancel-ordering pin — dropped at acpAgent.test.ts:10262, overlaps existing comment 3839813863 (R1-10 re-post thread)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/multi-workspace-sessions.test.ts:1149 — [probe] New workflowsEnabled capabilities assertion computed from ambient process.env (fails when QWEN_CODE_ENABLE_WORKFLOWS=1 is exported)packages/cli/src/serve/server.test.ts:9409 — [probe] Repurposed cancel test leaves legacy kinds (agent/monitor) without positive generic-route coverage; mutant ships greenpackages/webui/src/daemon/session/actions.ts:1962 — [review] getWorkflowTasks reports failures under getTasks' identity (message, operation 'load_tasks', shared once-key)packages/cli/src/acp-integration/session/Session.test.ts:1121 — [review] Workflow-hold test offers only 3 of 6 statuses; predicate-narrowing mutants pass greenpackages/cli/src/acp-integration/session/Session.test.ts:2488 — [review] removeTerminal call in deleteWorkflowHistory (the R2-4 fix) pinned by no test; deletion resurrection mutant ships greenpackages/cli/src/acp-integration/session/Session.ts:8345 — [review] todoStopGuard baseline keyed by runId; retry reuses the runId so retried attempts inherit baseline membershippackages/cli/src/serve/server.test.ts:4006 — [probe] Capabilities test pins no precedence combinations (settings+kill-switch, settings+untrusted); reorder mutants ship greenpackages/cli/src/acp-integration/session/Session.test.ts:34033 — [probe] Baseline-classification test confounds baseline membership with todoWorkChainId presence; chain-id mutant passespackages/cli/src/acp-integration/session/Session.test.ts:2297 — [probe] Failed workflow completions never exercised through the session queue; completed-only gating mutant passespackages/cli/src/acp-integration/session/tasksSnapshot.test.ts:378 — [probe] Live-vs-persisted runId dedup test uses only a terminal live entry; the retry-shaped active variant is unpinnedpackages/webui/src/daemon/session/actions.test.ts:1104 — [probe] runSavedWorkflow {changed:false} → started:false mapping unpinned; started:true mutant passes
Convergence: round 3 posted 35 inline comment(s), 1 of them reported for the first time; the previous round posted 38 (19 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.test.ts (findings in rounds 1, 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 35 条行内评论，其中 1 条是首次提出；上一轮发布了 38 条（其中 19 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/acpAgent.test.ts（第 1、2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await expect( | ||
| agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionWorkflowTaskAction, { | ||
| sessionId, | ||
| taskId: task.runId, | ||
| action: 'retry', | ||
| }), | ||
| ).resolves.toEqual({ changed: false, status: 'failed' }); | ||
| expect(execute).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] This new test pins the mutation-claim exclusion in only one direction (retry rejected while delete holds the claim). The delete-history branch's own re-entrancy guard (if (this.mutatingWorkflowTaskIds.has(mutationClaim)) return { changed: false };, acpAgent.ts:10363-10365) is exercised by no test with the claim already held by a retry/rerun. Mutant proof: removing that guard keeps all 13 workflow ext-method tests green (Tests 13 passed | 454 skipped), while a gated probe (retry holds the claim mid-execute, then delete-history for the same taskId) fails on the mutant with expected { changed: true } to deeply equal { changed: false } and passes with the guard restored. Without the guard, a delete-history arriving mid-retry descends into Session.deleteWorkflowHistory concurrently with the run restart — the same runId-reuse journal race from the delete side that the claim was added to close. Mirror this test with the roles swapped: block the retry execute mock on a gate, send delete-history, assert { changed: false } with deleteWorkflowHistory not called, then release and let the retry complete.
中文说明
这个新测试只锁定 mutation-claim 互斥的一个方向（delete 持有 claim 时 retry 被拒绝）。delete-history 分支自身的重入守卫（acpAgent.ts:10363-10365）没有任何测试在 retry/rerun 已持有 claim 时演练。变异证据：移除该守卫后 13 个 workflow ext-method 测试全绿（13 passed | 454 skipped），而门控探针（retry 在 execute 中持有 claim，随后对同一 taskId 发 delete-history）在变异体上失败（expected { changed: true } to deeply equal { changed: false }），恢复守卫后通过。没有该守卫时，retry 进行中的 delete-history 会与运行重启并发进入 Session.deleteWorkflowHistory——claim 本要堵住的 runId 复用 journal 竞态从 delete 侧重新打开。建议镜像本测试：门控 retry 的 execute，先发 delete-history 断言 { changed: false } 且 deleteWorkflowHistory 未被调用，再放行让 retry 完成。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const action = safeBody(req)['action']; | ||
| if ( | ||
| action !== 'pause' && | ||
| action !== 'resume' && |
There was a problem hiding this comment.
[Suggestion] R1-7: Still stands at e1393380 — the invalid-action rejection path still has zero negative tests at every layer (REST route, ACP-HTTP dispatch, acpAgent ext method), and the six-value allow-list remains hand-maintained in three separate entrances. An unrecognized action that slips past any loosened check reaches the pause/resume fallthrough and silently becomes a resume. Add one invalid-action negative case per layer.
中文说明
仍然成立：非法 action 拒绝路径在每一层（REST 路由、ACP-HTTP dispatch、acpAgent ext method）依然没有任何负向测试，六值白名单仍在三处手工维护。任何放宽检查后漏过的未知 action 会落入 pause/resume 兜底，静默变成 resume。建议每层补一个非法 action 负向用例。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it.each([ | ||
| ['workflows are disabled', false, false, false, true], | ||
| ['bare mode is active', true, true, false, true], | ||
| ['the workspace is untrusted', true, false, true, false], | ||
| ])( |
There was a problem hiding this comment.
[Suggestion] R1-8: Still stands at e1393380 — the gating it.each rows remain workflows-off, bare-mode, and untrusted (folderTrustFeature=true + folderTrust=false); no row enables getFolderTrustFeature with getFolderTrust=true, and none exercises feature-off + untrusted. Mutation (!getFolderTrustFeature() || getFolderTrust()) → (!getFolderTrustFeature() && getFolderTrust()) disables workflow controls for every user whose folder-trust feature is enabled in a trusted workspace, shipping green. Add rows for (feature=true, trust=true) expecting controls enabled, and (feature=false, trust=false).
中文说明
仍然成立：门禁 it.each 行仍只有功能关闭、bare 模式、不可信（folderTrustFeature=true + folderTrust=false）；没有 getFolderTrustFeature=true 且 getFolderTrust=true 的行，也没有功能关闭+不可信的行。变异 (!A || B) → (!A && B) 会令所有在可信 workspace 且启用 folder-trust 特性的用户失去 workflow 控制而测试全绿。建议补 (feature=true, trust=true) 与 (feature=false, trust=false) 两行。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await agentPromise; | ||
| }); | ||
|
|
||
| it('retries a failed workflow with its original script, args, and journal', async () => { |
There was a problem hiding this comment.
[Suggestion] R1-9: Still stands at e1393380 — retry (task.status === 'failed' && !registry.getHandle(taskId)) and rerun (terminal-status check) still have only happy-path and overlap-admission tests; no test drives a non-failed retry, a retry with a live handle, or a rerun of a running run. A mutation widening the retry precondition (e.g. dropping the !getHandle term) ships green while retries of active runs double-start. Add the three negative cases asserting {changed:false} with no execute call.
中文说明
仍然成立：retry（task.status === 'failed' && !registry.getHandle(taskId)）与 rerun（终态检查）仍只有快乐路径与重叠准入测试；没有测试演练非失败态的 retry、带活跃 handle 的 retry、或对 running 运行的 rerun。放宽 retry 前置条件的变异（如去掉 !getHandle）会让活跃运行被重复启动而测试全绿。建议补三个断言 {changed:false} 且不触发 execute 的负向用例。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await Promise.resolve(); | ||
| expect(settled).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R1-10: Still stands at e1393380 — re-verified this round: expect(settled).toBe(false) after one await Promise.resolve() still passes whether or not the cancel handler awaits handle.completion, because both async layers resolve in the same microtask window. Removing if (handle) await handle.completion; from the workflow cancel branch keeps this test green while cancel returns before the run actually settles. Gate resolveCompletion behind a second microtask/timer so dropping the await fails the test.
中文说明
仍然成立：单次 await Promise.resolve() 后的 expect(settled).toBe(false) 无论 cancel 处理器是否 await handle.completion 都会通过（两个异步层在同一微任务窗口内解决）。移除 cancel 分支中的 if (handle) await handle.completion; 测试仍绿，而 cancel 会在运行真正结束前返回。建议把 resolveCompletion 放到第二个微任务/定时器之后，使移除 await 时测试失败。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it('holds a queued workflow completion notification', async () => { | ||
| mockChat.sendMessageStream = vi | ||
| .fn() | ||
| .mockResolvedValue(createEmptyStream()); | ||
| createReportingSession(); | ||
| const releaseCloseGate = session.beginClose(); |
There was a problem hiding this comment.
[Suggestion] R2-15: Still stands at e1393380 — no test pins the in-flight continuation hold for workflow notifications (currentWorkflowNotificationTaskId). Once the queued item is dequeued for delivery, that field is the ONLY thing still holding active-work for the duration of the continuation turn; the queued-hold test stops at dequeue. Removing its set/clear in the drain path ships green while the session reports idle mid-delivery and can be reaped. Hold the drain mid-flight (gate sendMessageStream) and assert the hold persists until delivery settles.
中文说明
仍然成立：没有测试锁定 workflow 通知在途期间的持续 hold（currentWorkflowNotificationTaskId）。队列项出队投递后，该字段是续行 turn 期间唯一维持 active-work 的东西；排队 hold 测试在出队处就停了。在 drain 路径移除其 set/clear 会全绿，会话会在投递中途报告空闲并可能被回收。建议门控 sendMessageStream 使 drain 停在半路，断言投递完成前 hold 持续存在。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const deletion = session.deleteWorkflowHistory(snapshot.runId); | ||
| await Promise.resolve(); | ||
| expect(deleteWorkflowSnapshotSpy).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R2-16: Still stands at e1393380 — 'waits for an active run owner to finish persistence before deletion' stays green if if (handle) await handle.completion; is removed from Session.deleteWorkflowHistory: reaching deleteWorkflowSnapshot then needs the mocked finishPersistence anyway, so the wait itself is unverified. Dropping the wait ships green while delete-history races an unsettled run's snapshot write. Make deleteWorkflowSnapshot reachable without finishPersistence and assert it is not called until the completion resolves.
中文说明
仍然成立：若在 Session.deleteWorkflowHistory 中移除 if (handle) await handle.completion;，'waits for an active run owner to finish persistence before deletion' 仍绿：到达 deleteWorkflowSnapshot 反正需要 mock 的 finishPersistence，等待本身未被验证。移除等待会全绿，delete-history 会与未结算运行的快照写入竞态。建议让 deleteWorkflowSnapshot 不依赖 finishPersistence 也可达，并断言完成前不被调用。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| session.dispose(); | ||
| }); | ||
|
|
||
| it('holds each non-terminal workflow run', () => { |
There was a problem hiding this comment.
[Suggestion] R2-17: Still stands at e1393380 — the workflow status-change callback's #activeWorkChanged() push (Session.ts ~8330) is pinned by no test, unlike the task- and shell-registry siblings which each have an explicit 'status change notifies the owner' test; without it, hold changes on workflow transitions never reach the bridge. Add the workflow analogue of that sibling test.
中文说明
仍然成立：workflow 状态变更回调里的 #activeWorkChanged() 推送（Session.ts ~8330）没有任何测试锁定，而 task 与 shell registry 的兄弟各自都有显式的"状态变更通知属主"测试；缺了它，workflow 状态转换的 hold 变化永远到不了 bridge。建议补该兄弟测试的 workflow 对应项。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| callback('baseline result', '<baseline-workflow />', { | ||
| runId: 'baseline-workflow', | ||
| status: 'completed', | ||
| todoWorkChainId: 'stale-chain', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R2-18: Still stands at e1393380 — workflow completion todoWorkChainId propagation (enqueue → item.todoWorkChainId → startAutomaticActiveTodoWorkChain(promptId, item.todoWorkChainId)) is pinned by zero tests; the baseline-classification test asserts only the continuesTodoStopGuardWorkChain flag, not the chain-id handoff. Dropping todoWorkChainId from the queued item ships green while workflow completions never resume their work chain. Assert the queued item carries the chain id and delivery passes it through.
中文说明
仍然成立：workflow 完成的 todoWorkChainId 传播（入队 → item.todoWorkChainId → startAutomaticActiveTodoWorkChain(promptId, item.todoWorkChainId)）没有任何测试锁定；基线分类测试只断言 continuesTodoStopGuardWorkChain 标志，不断言链 id 交接。从队列项丢弃 todoWorkChainId 会全绿，workflow 完成将永远无法续接其工作链。建议断言队列项携带链 id 且投递时透传。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| primaryRuntimeEnv.workflowsEnabledBySettings = | ||
| fresh.merged.tools?.workflowsEnabled === true; |
There was a problem hiding this comment.
[Suggestion] R2-19: Still stands at e1393380 — the reload FALLBACK path (env rebuild throws) updating workflowsEnabledBySettings is pinned by zero tests. The assignment deliberately runs before the buildRuntimeEnvironment try/catch — so the capabilities flag flips even when the reload otherwise fails — and no test exercises that branch. Moving the assignment inside the try (or dropping it on fallback) ships green while a failed reload leaves stale capability advertisement. Add a reload test where buildRuntimeEnvironment throws and assert the flag still updates.
中文说明
仍然成立：reload 回退路径（环境重建抛错）更新 workflowsEnabledBySettings 没有任何测试。该赋值故意放在 buildRuntimeEnvironment 的 try/catch 之前——即使 reload 其他部分失败，能力标志也会翻转——但没有测试演练该分支。把赋值移进 try（或回退时丢弃）会全绿，失败的 reload 会留下过期的能力声明。建议补一个 buildRuntimeEnvironment 抛错的 reload 测试，断言标志仍被更新。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /takeover from 6 |
qwen-code-dev-bot
added
the
autofix/takeover
|
@qwen-code /takeover stop |
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.
14 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- dead getWorkflowHistory getter and preload — already reported (R1-25, comment 3839293583)
- cancel completion-wait test vacuity — already reported (R1-10, comment 3839293554 and reposts)
- SDK sessionTaskMutation wire-level coverage — already reported (R1-14, comment 3839293563)
- ACP dispatch validation branches untested — already reported (R1-7 family, comment 3839293546; round-5 deferred dispatch.ts:3640)
- retry getHandle guard unpinned — already reported (round-5/6 deferred: acpAgent.test.ts:10599, acpAgent.ts:10528)
- multi-workspace workflow-action ownership test — already reported (R1-20, comment 3839293575)
- cancel trust-gate kind-scoping pin — already reported (round-6 deferred server.test.ts:9591)
- mutation-claim release unpinned — already reported (R2-7, comment 3839813819)
- delete persistence-wait test vacuity — already reported (R2-16, comment 3839813833)
- cancelTask stale-notice suppression — already reported (round-6 deferred actions.ts:1995; R1-18 comment 3839293570)
- run-saved launch error collapses into bare {changed:false} — already reported (round-5 deferred acpAgent.ts:10476)
- JSON-RPC tasks opt-in default unpinned — already reported (round-6 deferred transport.test.ts:8436)
- stop-guard baseline keyed by runId misclassifies retried runs — already reported (round-6 deferred Session.ts:8389)
- webui new-action error-notice positive pin — already reported (R1-18, comment 3839293570; round-6 deferred actions.ts:2030)
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): chunk 14: execute packages/cli/src/serve/workflow-session-gate.test.ts (and the two server.test.ts SSE tests) under vitest — the review worktree has no node_modules , ….
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/workflow-snapshot.ts:140 — [probe] writeWorkflowSnapshot boolean contract unasserted; both mutants survivepackages/cli/src/serve/server.test.ts:939 — [probe] new fake's status?: string is a latent TS2322 behind the TODO(5691) excludepackages/cli/src/serve/workflow-session-gate.test.ts:102 — [probe] liveJournal-only redaction branch unpinned; guard mutant ships greenpackages/cli/src/acp-integration/acpAgent.test.ts:11637 — [review] sibling removeTerminal fan-out's if(changed) guard unpinned on the false pathpackages/cli/src/acp-integration/session/Session.test.ts:3203 — [review] post-completion-wait isDeletable() re-check unpinned by any testpackages/cli/src/serve/server.test.ts:13884 — [review] redaction tests pin the inner array only; envelope-level regressions ship green
Convergence: round 7 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.ts (findings in round 6; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (4 Critical(s)), the rate of first-time findings is not falling (this round 4, previous 3), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 14 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 14：execute packages/cli/src/serve/workflow-session-gate.test.ts (and the two server.test.ts SSE tests) under vitest — the review worktree has no node_modules , …。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 3 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/acpAgent.ts（第 6 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 4 条 Critical），首次发现的速率没有下降（本轮 4，上一轮 3），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…oles
Addresses R5-9, R7-4, R7-5 and R7-10 — all four Criticals open on this PR.
They are one family: a run's history can be deleted, retried, or listed
from any session, and each gate had a different idea of which runs exist.
R5-9 — the mutation claim is task-global, not session-scoped. Keyed
`sessionId\0taskId`, it serialized nothing that mattered: every session
shares one snapshot store. A sibling's retry passed canStart (`failed`,
no handle), took its own per-session claim, then awaited journal
load/compile before `register()`; a delete-history landing in that
structural window found the run terminal and handle-less in every
registry, removed the journal directory and snapshot, and answered
`{changed: true}` — after which the retry re-registered and its
settlement re-persisted the history the user was told was deleted. The
claim is now keyed by taskId alone and taken by delete-history, retry,
rerun and run-saved. `run-saved` keys off a saved workflow's NAME, so it
claims in its own `saved\0` namespace rather than colliding with runIds.
R7-4 — deletion tests membership against the uncapped merged set.
`buildSessionTasksStatus` serializes every registry entry
unconditionally while `refreshWorkflowHistory` truncates to
MAX_RETAINED_SNAPSHOTS by startTime, so a long run that settled after
~30 newer ones started stayed listed via the registry but fell out of
the window — terminal, handle-free, live in no sibling, and permanently
undeletable. `refreshWorkflowHistory` now records the merged id set
before the cap, and deletion gates on that, the registry, or the
unpersisted cache. `deleteWorkflowSnapshot` already tolerates an absent
target, so the wider gate cannot delete what is not there.
R7-5 — snapshot retirement is a latch. The registry's dispatch-drain
callbacks emit status changes on TERMINAL entries with no status gate,
and in-flight dispatches keep draining across the snapshot write, so a
terminal emission routinely landed after `notifySnapshotPersisted` had
retired the cache entry — re-inserting the run as "never persisted".
A sibling's deletion was then undone by the next refresh, which reads
"absent on disk, present in cache" as a pending write and republishes.
Persistence is now remembered per runId and `#rememberWorkflowHistory`
returns early for members; the latch releases when the runId goes active
again, so a genuine re-run is still cached.
R7-10 — the liveness gate sees runs whose session is gone. It iterated
`this.sessions` only, but close/kill/shutdown use force semantics and a
background run owns a detached controller, so after
`removeStoredSessionEntry` a still-settling run was invisible to the
gate and unreachable by the delete handler's sibling `removeTerminal`
loop: a sibling delete-history removed the LIVE run's journal and
snapshot, and the orphan's settlement write recreated it. Two halves —
`Session.dispose()` now aborts its workflow registry the way it already
aborts the agent registry (before the callbacks are torn down), and the
registry of a removed session is retained here until its runs drain, so
the gate still answers across the settlement window an abort cannot
compress to zero. Retention is bookkeeping: a Config that cannot answer
is logged, never turned into a shutdown failure.
Regressions, each mutation-checked against its own repair:
- acpAgent: a sibling's parked retry makes delete-history answer
`{changed: false}` without reaching the store, and the deletion goes
through once the claim releases
- acpAgent: a live run's registry stays visible to the gate across its
session's close, and is dropped once the handle is released
- Session: a status emission after `snapshotPersisted` no longer
resurrects a sibling-deleted run (reverting the latch reproduces the
reviewer's probe verbatim), and a re-registered runId is remembered
again
- Session: a run with the oldest startTime behind 30 newer snapshots is
deletable
- Session: dispose aborts the workflow registry before clearing its
callbacks
Verification: Session.test.ts 733 passed, acpAgent.test.ts 514 passed,
`tsc --noEmit -p packages/cli` clean, eslint and prettier clean. The 14
`packages/cli/src/serve` failures (fast-path import boundary,
capabilities-docs contract, workspace fs/agents/memory, conversation
runtime ownership) reproduce identically on the unmodified head — base
skew, untouched by this change.
Claude-Session: https://claude.ai/code/session_01M7z4PccYfDPyyfg3oGr8V1
|
On R7-10 specifically — both halves were needed, and your probe is why. A run cancelled via the abortAll shape still persists exactly one snapshot at settlement, so aborting alone cannot compress the window to zero; the gate has to keep answering while the handle lives. I used the retained-registry form rather than a parallel process-global runId set because the authoritative fact already lives in the registry ( One thing worth calling out from the repair: the first cut pushed a failed registry read in Regressions, each mutation-checked against its own repair:
Verification: Disclosed gap: The 14 Suggestion-level findings this round re-confirmed are all previously deferred items in this PR's convergence posture; they remain deferred, and this round did not add to them. 中文说明
关于 R7-10：两半缺一不可，依据正是您的探针——以 abortAll 形态取消的运行在结算时仍会写入一份快照，因此仅靠中止无法把窗口压缩为零。选择「保留 registry」而非并行的进程级 runId 集合，是因为权威事实本就在 registry 中（ 修复过程中的一个插曲值得记录：第一版把 五条回归测试均做了变异校验。验证： 已披露的缺口： |
Merging upstream/main (053f17b) clears the 9 `client.telemetrySwap` failures this branch had from predating QwenLM#10220, but main carries its own TS1117 on the same file: two commits added `getToolRegistry` to the same object literal independently and neither saw the other. 032b907 feat(serve): backfill session PR bindings ... (QwenLM#9729) 8241905 test(core): give the telemetry-swap client mock a getToolRegistry (QwenLM#10220) Checking that file out from upstream/main here and running `tsc --noEmit -p packages/core` reproduces `client.telemetrySwap.test.ts(103,5): error TS1117` verbatim, so taking the merge unmodified would have traded 9 test failures for a build that never reaches the tests at all. Removed as part of the merge rather than left for a follow-up: keeps QwenLM#10220's copy, which was added for this purpose and carries the explanation, and drops QwenLM#9729's incidental one. main still needs the same removal — this only keeps it out of the branch. Verified after the merge: `tsc --noEmit` clean for both packages/core and packages/cli; client.telemetrySwap 10 passed, Session.test.ts and acpAgent.test.ts 1260 passed together. Claude-Session: https://claude.ai/code/session_01M7z4PccYfDPyyfg3oGr8V1
|
@qwen-code /takeover stop |
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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.ts:3386 — [review] detachedWorkflowRegistries pruned only on the delete-history path (single caller); accumulates without deletionspackages/cli/src/acp-integration/acpAgent.ts:10996 — [review] run-saved overlap-admission guard exercised by no testpackages/cli/src/acp-integration/acpAgent.test.ts:11639 — [probe] R7-10 test's prune assertion vacuous — prune removable, mutant ships green (514/514)packages/cli/src/acp-integration/acpAgent.test.ts:11266 — [probe] claim release on error unpinned — a rejected execute strands the taskId claim until restartpackages/cli/src/serve/server.test.ts:9843 — [review] untrusted workflow-action gate tested for 1 of 6 actions (REST + ACP-HTTP twin)packages/cli/src/acp-integration/session/Session.test.ts:32106 — [probe] dispose test doesn't pin the setSnapshotPersistedCallback(undefined) clearpackages/cli/src/acp-integration/acpAgent.ts:10978 — [probe] sibling cleanup clears registries but not history caches — deleted run re-displays in sibling (display-only)packages/cli/src/acp-integration/session/Session.test.ts:3273 — [probe] R7-4 capped-window test's registry mock masks the fix — revert ships green (736/736)packages/cli/src/serve/server.test.ts:4066 — [review] kill-switch vs workflowsEnabledBySettings precedence unpinned in capabilities tests
Convergence: round 8 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.ts (findings in round 7; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 9 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 8 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/acpAgent.ts（第 7 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const registry = session.getConfig().getWorkflowRunRegistry?.(); | ||
| if (registry && QwenAgent.isWorkflowRegistryDraining(registry)) { | ||
| this.detachedWorkflowRegistries.add(registry); | ||
| } |
There was a problem hiding this comment.
[Critical] R7-4: (fix-induced) The round-7 fix retains a removed session's workflow registry through a one-shot draining check at removal time, and that one-shot shape opens a new resurrection window at the same site. A retry/rerun/run-saved that is already past canStart and holding its task claim, but whose WorkflowRunner.start() is still awaiting journal.load() before register(), is not yet draining when removeStoredSessionEntry runs — so its registry is not retained, the session leaves this.sessions, and the run then registers into a registry that no liveness gate consults (isWorkflowRunLiveOutsideSession iterates only this.sessions + detachedWorkflowRegistries). A sibling delete-history for that runId passes every check (terminal and handle-less in every visible registry), removes the journal directory and snapshot, and answers {changed:true}; the orphan's settlement then runs writeWorkflowSnapshot and the history the user was told was deleted resurrects in every session — with the journal rm'd under a live run. The claim set does not help: session close never takes a claim.
Witness — probe against the real Session.deleteWorkflowHistory + real registry + real WorkflowRunner, journal load parked pre-register:
PR CODE: deleteWorkflowHistory -> changed=true, snapshotExistsAfterDelete=false,
registryEntryAfterDelete=null; later registry.get(runId).status === 'completed'
and the snapshot file exists again
FLIP (registry retained across the window): deleteWorkflowHistory -> changed=false,
snapshot retained
Fix: fail closed at the start site — after execute(...) returns in the retry/rerun/run-saved handlers, check the session is still stored (this.sessions.get(sessionId) === session) and if not immediately registry.cancel(runId, Date.now()); alternatively serialize removeStoredSessionEntry against outstanding workflow-mutation claims for that session.
Please pin the fix with a test: park a retry inside its mocked execute (as the R5-9 test does), run sessionClose for that session, release the retry, and assert the started run is cancelled at once or a sibling delete-history for the runId keeps refusing {changed:false} while the handle lives — removing the fix must turn that test red.
中文说明
[Critical] R7-4：（修复引入）第 7 轮的修复通过在会话移除时刻做一次性的 draining 检查来保留被移除会话的 workflow registry，而这个一次性形态在同一位置打开了新的「复活」窗口。某个 retry/rerun/run-saved 已经通过 canStart 并持有任务申领，但其 WorkflowRunner.start() 仍停在 register() 之前的 journal.load() await 上——此时 removeStoredSessionEntry 执行时该 registry 还不算 draining，因此不会被保留；会话从 this.sessions 移除后，该运行随后注册进一个任何存活性门禁都不会查询的 registry（isWorkflowRunLiveOutsideSession 只遍历 this.sessions + detachedWorkflowRegistries）。兄弟会话对该 runId 的 delete-history 会通过所有检查（在所有可见 registry 中均为终态且无 handle），删除日志目录与快照并返回 {changed:true}；孤立的结算随后执行 writeWorkflowSnapshot，用户刚被告知已删除的历史在所有会话中复活——且日志是在运行仍存活时被 rm 的。申领集合帮不上忙：会话关闭从不申领。
证据——对真实 Session.deleteWorkflowHistory + 真实 registry + 真实 WorkflowRunner 的探针（日志加载停靠在注册前）：PR 代码返回 changed=true、快照被删、之后 registry.get(runId).status === 'completed' 且快照文件再次出现；翻转分支（跨窗口保留 registry）返回 changed=false、快照保留。
修复：在启动侧 fail closed——retry/rerun/run-saved 处理器在 execute(...) 返回后检查会话仍在存储中（this.sessions.get(sessionId) === session），否则立即 registry.cancel(runId, Date.now())；或者让 removeStoredSessionEntry 与该会话未完成的 workflow 变更申领串行化。
请为修复补充测试：让一个 retry 停在 mock 的 execute 内（仿照 R5-9 测试），对该会话执行 sessionClose，释放 retry，断言启动的运行被立即取消、或兄弟会话的 delete-history 在 handle 存活期间持续拒绝 {changed:false}——移除修复必须使该测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (action === 'run-saved') { | ||
| if (this.mutatingWorkflowTaskIds.has(mutationClaim)) { | ||
| return { changed: false }; | ||
| } |
There was a problem hiding this comment.
[Critical] The task-global mutation claim that closed the round-7 cross-session races is taken only by these control-plane branches (delete-history, retry/rerun, run-saved). The Workflow tool exposes a schema-documented resumeFromRunId parameter and is registered for daemon-session configs, so a model-invoked tool call re-registers an existing runId through WorkflowRunner.start() without ever consulting mutatingWorkflowTaskIds. During the resume's pre-register window (script compile + await journal.load() — nothing registered, no handle, no claim), a sibling delete-history for that runId passes every gate, rms the journal directory, unlinks the snapshot, and answers {changed:true}. WorkflowJournal.load() returns an empty replay on the missing file rather than throwing, so the resume proceeds, register() re-creates the entry, the R7-5 latch releases on the non-terminal emission, and settlement rewrites the snapshot — the deleted run resurrects through an entrance the claim does not cover. Additionally, Session.deleteWorkflowHistory ignores removeTerminal()'s false return (~line 3468), reporting success even when the entry could not be removed.
Witness — probe with the real Session + registry + runner + snapshot store, journal load parked pre-register:
PROBE[ungated] deleteWorkflowHistory -> changed=true, snapshotExistsAfterDelete=false,
registryEntryAfterDelete=null; post-settlement: run 'completed' and the
snapshot file exists again
PROBE[gated] (pending start visible to the gate) -> changed=false, snapshot retained
Fix: route re-registration of an existing runId (WorkflowRunner.start with resumeFromRunId, or register() itself) through the same task-global claim the control plane uses; defensively treat removeTerminal() returning false in Session.deleteWorkflowHistory as a failed deletion (return false) so a live re-registered run is never reported deleted.
Please pin the fix with a test: extend the R5-9 shape to drive the concurrent re-registration through the Workflow tool / resumeFromRunId instead of the retry ext-method and assert delete-history answers {changed:false} (or the run is not re-persisted), plus a Session.test.ts case asserting deleteWorkflowHistory returns false when registry.removeTerminal returns false — removing either guard must turn the respective test red.
中文说明
[Critical] 闭合第 7 轮跨会话竞态的任务级互斥申领只由这些控制面分支（delete-history、retry/rerun、run-saved）获取。Workflow 工具暴露了 schema 文档化的 resumeFromRunId 参数，并且已注册到 daemon 会话配置，因此模型发起的工具调用会通过 WorkflowRunner.start() 重新注册已有 runId，而完全不查询 mutatingWorkflowTaskIds。在 resume 的注册前窗口（脚本编译 + await journal.load()——未注册、无 handle、无申领），兄弟会话对该 runId 的 delete-history 会通过全部门禁：rm 日志目录、删除快照并返回 {changed:true}。WorkflowJournal.load() 在文件缺失时返回空回放而非抛错，因此 resume 继续执行，register() 重建条目，R7-5 锁存器在非终态发射时释放，结算重写快照——被删除的运行通过申领未覆盖的入口复活。此外，Session.deleteWorkflowHistory 忽略 removeTerminal() 的 false 返回（约 3468 行），即使条目未能移除也报告删除成功。
证据——真实 Session + registry + runner + 快照存储的探针（日志加载停靠在注册前）：未设防分支返回 changed=true、快照被删、结算后运行状态 completed 且快照文件再次出现；设防分支（待启动对门禁可见）返回 changed=false、快照保留。
修复：让已有 runId 的重新注册（带 resumeFromRunId 的 WorkflowRunner.start，或 register() 本身）走与控制面相同的任务级申领；防御性地在 Session.deleteWorkflowHistory 中把 removeTerminal() 返回 false 视为删除失败（返回 false），使存活的重新注册运行永远不会被报告为已删除。
请为修复补充测试：扩展 R5-9 形态，用 Workflow 工具 / resumeFromRunId（而非 retry ext 方法）驱动并发重新注册，断言 delete-history 返回 {changed:false}（或运行未被重新持久化）；另加一个 Session.test.ts 用例，断言当 registry.removeTerminal 返回 false 时 deleteWorkflowHistory 返回 false——移除任一防护必须使对应测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
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.
11 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R9-6 vacuous cancel-wait assertion — already reported (R1-10, comment 3839293554)
- R9-7 todoStopGuard true-direction test gap — already reported (R2-13, comment 3839813829)
- R9-8 run-saved overlap claim untested — already recorded (round-8 deferred list, acpAgent.ts:10996)
- R9-9 workflow cancel refusal branches untested — already reported (R2-8, comment 3839813821)
- R9-10 detachedWorkflowRegistries pruned only on the delete-history path — already recorded (round-8 deferred list, acpAgent.ts:3386)
- R9-11 vacuous handle-wait deletion test — already recorded (round-6 deferred list, Session.test.ts:2758)
- R9-12 capped-window test registry stub masks the R7-4 fix — already recorded (round-8 deferred list, Session.test.ts:3273)
- R9-13 retry canStart handle guard untested — already recorded (round-6 deferred list, acpAgent.ts:10528)
- R9-14 run-saved not-found branch untested — already reported (R2-6, comment 3839813817)
- R9-15 dispose setSnapshotPersistedCallback clear unpinned — already recorded (round-8 deferred list, Session.test.ts:32106)
- R9-16 run-saved launch error collapsed into bare changed:false — already recorded (round-5 deferred list, acpAgent.ts:10476)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 2: none — no check was cut short..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/session/Session.ts:3428 — [probe] stale in-flight refresh republishes a run deleted meanwhilepackages/core/src/agents/runtime/workflow-runner.test.ts:389 — [probe] loadSpy ordering witness vacuous (no storage in test config)packages/core/src/agents/runtime/workflow-runner.ts:181 — [probe] abort-during-start throws a plain Error the tool catch cannot mappackages/cli/src/acp-integration/acpAgent.ts:10884 — [probe] sessionTaskCancel is blind to the starting windowpackages/core/src/agents/workflow-run-registry.ts:594 — [review] register's JSDoc orphaned onto reserveStart
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 11 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 2：none — no check was cut short.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Four of the behavioural items the review deferred across rounds 8-9; the test-coverage-only entries stay deferred. - Cancel during the start window (R9 acpAgent.ts:10884 + workflow-runner.ts:181). Between `reserveStart` and `register` the runner loads the script and replays the journal — seconds, for a resume of a large one — and the registry has no entry yet. `sessionTaskCancel` answered `not_found` for a run the client could see starting, and `registry.cancel` could not reach the reserved controller either. New `cancelStarting` aborts it (the reservation stays the runner's to release, as after `abortAll`), and the cancel handler routes there when the liveness gate would say "starting". Doing that exposed the second half: the runner threw a bare `Error` for an abort during start, and the tool's catch only recognised the CALLER's signal — a registry-side abort surfaced as an unexplained failure. It is now a typed `WorkflowStartCancelledError`, mapped to the same "cancelled before it could start" result. - `detachedWorkflowRegistries` (R8 acpAgent.ts:3386) was pruned only inside the delete-history liveness check. A daemon that closes sessions mid-run and never deletes history retained every registry for its lifetime. Prune on session close as well. - Refresh/delete race (R9 Session.ts:3428). `refreshWorkflowHistory` reads the directory and merges without a claim; a delete landing between the read and the merge was overwritten by the stale listing and the run reappeared until the next refresh. Deletions are now sequenced, and a refresh drops any run deleted after its read began — keyed by runId and compared against the refresh's own mark, so a later retry that reuses the id is not suppressed. - The "Register a new run" JSDoc sat on `reserveStart` (R9 registry:594). Mutation-verified, all four at once against the full suites: disabling the starting-window branch, the prune-on-close, the deletion filter, and the typed-error mapping reddens exactly the four new tests and nothing else. Claude-Session: https://claude.ai/code/session_01VXsC4f71S6U6YkW82NRw7m
|
6c79490 picks up four of the behavioural items from the round-8/9 deferred lists. The coverage-only entries stay deferred, as the convergence note suggested.
Tests: 中文说明6c79490 处理了第 8/9 轮延后列表中的四条行为类条目；纯测试覆盖类条目按收敛提示继续延后。
测试：registry 的 |
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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/session/Session.ts:3496 — [review] removeTerminal() false return ignored in deleteWorkflowHistorypackages/cli/src/acp-integration/session/Session.ts:3526 — [review] Comment names a nonexistent method on a concurrency latchpackages/cli/src/acp-integration/session/Session.ts:3088 — [probe] Todo-stop-guard workflow branch has no testpackages/cli/src/acp-integration/acpAgent.ts:10901 — [review] Workflow not_running cancel branch untestedpackages/cli/src/acp-integration/acpAgent.ts:10943 — [review] Workflow action whitelist untestedpackages/cli/src/acp-integration/session/Session.test.ts:32215 — [probe] Dispose test misses the setSnapshotPersistedCallback teardownpackages/core/src/agents/workflow-run-registry.ts:1380 — [probe] Blocking-work message cannot name a starting workflowpackages/cli/src/acp-integration/acpAgent.test.ts:10947 — [probe] Cancel settlement wait is pinned by no mutation-sensitive testpackages/cli/src/acp-integration/session/Session.test.ts:3338 — [probe] R7-4 capped-window test never exercises the membership gatepackages/cli/src/acp-integration/acpAgent.test.ts:11173 — [probe] Refused pause/resume (changed: false) untestedpackages/cli/src/acp-integration/acpAgent.test.ts:11329 — [probe] run-saved overlap claim has no test
[Critical] R10-1 (inline comment withheld — an existing unresolved review thread already occupies Session.ts:3823): The workflow active-work hold skips the starting window — collectActiveWorkHolds() iterates registry.list(), which has no entry between reserveStart and register, so during a run-saved/retry start's pre-register window the session reports zero workflow holds and isIdle() is true. A daemon-initiated conditional close (onlyIfUnheld authorizes on collectActiveWorkHolds().length === 0) then proceeds, dispose() aborts the reserved starting controller, the start throws WorkflowStartCancelledError and the requested run never registers; the pending action resolves {changed: false}, indistinguishable from 'unknown saved workflow'. Meanwhile hasRunningEntries() (extended in this diff) and the delete/cancel liveness gates count isStarting as live. Probe-verified: PR code reports no hold in the starting window (AssertionError: expected [] to deeply equal [ 'wf-starting' ]); with the fix (expose reserved ids, push the hold) isIdle() is false. Fix: expose the reserved runIds on WorkflowRunRegistry (e.g. listStartingRunIds()) and push a workflow hold in collectActiveWorkHolds(); the hold self-releases (registration takes over with the per-entry running hold; failed/cancelled start removes it via releaseStart). Witness owed: Session.test.ts sibling of 'holds executing workflow runs but never paused ones' asserting a starting run yields a workflow hold — removing the guard must turn it red.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
[Critical] R10-1 (inline comment withheld — an existing unresolved review thread already occupies Session.ts:3823): The workflow active-work hold skips the starting window — collectActiveWorkHolds() iterates registry.list(), which has no entry between reserveStart and register, so during a run-saved/retry start's pre-register window the session reports zero workflow holds and isIdle() is true. A daemon-initiated conditional close (onlyIfUnheld authorizes on collectActiveWorkHolds().length === 0) then proceeds, dispose() aborts the reserved starting controller, the start throws WorkflowStartCancelledError and the requested run never registers; the pending action resolves {changed: false}, indistinguishable from 'unknown saved workflow'. Meanwhile hasRunningEntries() (extended in this diff) and the delete/cancel liveness gates count isStarting as live. Probe-verified: PR code reports no hold in the starting window (AssertionError: expected [] to deeply equal [ 'wf-starting' ]); with the fix (expose reserved ids, push the hold) isIdle() is false. Fix: expose the reserved runIds on WorkflowRunRegistry (e.g. listStartingRunIds()) and push a workflow hold in collectActiveWorkHolds(); the hold self-releases (registration takes over with the per-entry running hold; failed/cancelled start removes it via releaseStart). Witness owed: Session.test.ts sibling of 'holds executing workflow runs but never paused ones' asserting a starting run yields a workflow hold — removing the guard must turn it red.
— qwen3.8-max via Qwen Code /review (v0.22.2)
- Key the starting-window cancel on a live reservation rather than on the absence of an entry: a retry reuses its runId, so its terminal entry shadowed the reservation and cancel answered `not_running` about a run that was actively starting. - Answer `changed: false` from retry when `execute()` reports a start that never registered (no `workflowRunId`), mirroring rerun and run-saved. - Classify a registry-side abort of the reserved controller as a cancel in foreground starts too, not only background ones; the tool maps `WorkflowStartCancelledError` in either mode. - Report reserved-but-unregistered runs as workflow active-work holds (`WorkflowRunRegistry.listStartingRunIds`), so a daemon conditional close cannot dispose the session under a start it just accepted. - Propagate a successful history deletion into every sibling session's deletion marker and cached history, symmetric to the `removeTerminal` sweep, so a sibling refresh that had already read the directory cannot republish the deleted run. Claude-Session: https://claude.ai/code/session_018dYE4LwSMeMPFchXk5UBdM
…rkflow-daemon-api
|
Round-10 R10-5 (body-only, inline withheld — starting-window active-work hold) is closed in f119607: The other four round-10 Criticals are answered in their threads. c58b3fd merges upstream/main (one conflict, |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
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.
15 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- vacuous completion-wait pin on 'cancels active workflow tasks' — already reported (R1-10, comment 3839293554)
- workflow-action route has no cross-workspace dispatch test — already reported (R1-20, comment 3839293575)
- legacy persisted-snapshot shape never exercised — already reported (R1-21, comment 3839293576)
- opt-out test exercises only the persisted-history branch of the gate — already reported (R1-23, comment 3839293580)
- new fake's status?: string is a latent TS2322 behind the TODO(5691) exclude — already recorded (round-7 deferred list, server.test.ts:939)
- repurposed cancel test leaves legacy kinds without an untrusted-side success pin — already reported (R4-5, comment 3851196166; round-3/5/6 deferred lists)
- kill-switch vs workflowsEnabledBySettings precedence unpinned in capabilities tests — already recorded (round-6 deferred list, server.test.ts:4066)
- R7-4 capped-window test's registry mock masks the membership-gate fix — already recorded (round-8 deferred Session.test.ts:3273; round-10 deferred Session.test.ts:3338)
- comment names nonexistent #forgetPersistedWorkflowRun — already recorded (round-10 deferred list, Session.ts:3526)
- SDK wire-level tests missing for controlWorkflowTask/sessionWorkflowTaskAction — already reported (R1-14, comment 3839293563)
- todo-stop-guard workflow branch has no test — already reported (R2-13, comment 3839813829)
- invalid-action rejection path untested at every layer — already reported (R1-7, comment 3839293546)
- writeWorkflowSnapshot boolean contract unasserted — already recorded (round-7 deferred list, workflow-snapshot.ts:140)
- three new webui workflow actions have zero production call sites — already recorded (round-6 deferred list, actions.ts:2055)
- delete persistence-wait test vacuity — already reported (R2-16, comment 3839813833)
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): "agent 1c": whether standalone-session restore is actually reachable for untrusted workspaces (standalone-session-service trust posture) — I could not settle this within bu…; chunk 11: running packages/cli vitest suites (tasksSnapshot.test.ts, transport.test.ts) — the review worktree has no node_modules and a full install+build exceeded the …; "agent reverse-audit (round 1)": none — all planned checks completed; the one unresolved verification (whether beginDrain tears down old SSE subscriptions) is disclosed inside the finding's f…; chunk 3: execute packages/cli vitest run of acpAgent.test.ts to confirm the new tests pass (no node_modules in this review worktree; dependency install + workspace….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/session/Session.test.ts:3001 — [review] noteExternalWorkflowDeletion's clearing of the observer's unpersistedWorkflowHistory is pinned by no test
Convergence: round 11 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.ts (findings in round 10; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 15 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1c"：whether standalone-session restore is actually reachable for untrusted workspaces (standalone-session-service trust posture) — I could not settle this within bu…；chunk 11：running packages/cli vitest suites (tasksSnapshot.test.ts, transport.test.ts) — the review worktree has no node_modules and a full install+build exceeded the …；"agent reverse-audit (round 1)"：none — all planned checks completed; the one unresolved verification (whether beginDrain tears down old SSE subscriptions) is disclosed inside the finding's f…；chunk 3：execute packages/cli vitest run of acpAgent.test.ts to confirm the new tests pass (no node_modules in this review worktree; dependency install + workspace…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 11 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/acpAgent.ts（第 10 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
…ent across sessions Two cross-session gaps in the workflow control surface: - A retry consulted only the requesting session's registry. Every session shares one journal/snapshot store and the task-global claim is released as soon as the background start returns, so a sibling whose registry still showed the run `failed` started a second runner under the same runId. Retry now refuses while the runId is live in any session (or in its own starting window), checked synchronously beside canStart so the answer cannot go stale before the claim is taken. - Workspace reload updated `tools.workflowsEnabled` for `/capabilities` but never told existing sessions; `Config.workflowsEnabled` was set once at construction. The reload's `tools` branch now propagates the flag and pushes an available-commands update when it flips. Claude-Session: https://claude.ai/code/session_01NkW1J2aBKcsKS62dkPcWbT
|
Superseded by #10411 — same branch history plus the round-11 fixes, reopened on a fresh branch to reset the review thread history (~180 threads over 11 rounds). Closing this one. |
* feat(serve): expose workflow tasks and controls * fix(serve): align workflow capability gating * fix(cli): reject live workflow history deletion * fix(serve): start controlled workflows in background * fix(serve): address workflow control review blockers * fix(serve): make workflow history deletion race-safe * fix(serve): enforce workspace trust on the daemon Workflow surfaces (QwenLM#9546) * fix(serve): close cross-session workflow deletion races and untrusted task reads (QwenLM#9546) - Session history merge now treats a persisted snapshot as authoritative over a stale callback cache, and retires the cache once the runner confirms the snapshot write (new registry snapshot-persisted hook), so a sibling's deletion is not resurrected on refresh. - History deletion consults every sibling session's run registry (live entries and settling handles) before deleting from the shared store, and a successful deletion purges sibling terminal entries so retries cannot re-persist a deleted run. - Workflow holds mirror the registry's hasRunningEntries: paused runs no longer pin the session indefinitely. - The includeWorkflows opt-in is gated on workspace trust at the daemon boundary in both the ACP-HTTP dispatch and the REST tasks route, matching the fail-closed shape of the other workflow surfaces. * fix(cli): close the four cross-session workflow-history consistency holes Addresses R5-9, R7-4, R7-5 and R7-10 — all four Criticals open on this PR. They are one family: a run's history can be deleted, retried, or listed from any session, and each gate had a different idea of which runs exist. R5-9 — the mutation claim is task-global, not session-scoped. Keyed `sessionId\0taskId`, it serialized nothing that mattered: every session shares one snapshot store. A sibling's retry passed canStart (`failed`, no handle), took its own per-session claim, then awaited journal load/compile before `register()`; a delete-history landing in that structural window found the run terminal and handle-less in every registry, removed the journal directory and snapshot, and answered `{changed: true}` — after which the retry re-registered and its settlement re-persisted the history the user was told was deleted. The claim is now keyed by taskId alone and taken by delete-history, retry, rerun and run-saved. `run-saved` keys off a saved workflow's NAME, so it claims in its own `saved\0` namespace rather than colliding with runIds. R7-4 — deletion tests membership against the uncapped merged set. `buildSessionTasksStatus` serializes every registry entry unconditionally while `refreshWorkflowHistory` truncates to MAX_RETAINED_SNAPSHOTS by startTime, so a long run that settled after ~30 newer ones started stayed listed via the registry but fell out of the window — terminal, handle-free, live in no sibling, and permanently undeletable. `refreshWorkflowHistory` now records the merged id set before the cap, and deletion gates on that, the registry, or the unpersisted cache. `deleteWorkflowSnapshot` already tolerates an absent target, so the wider gate cannot delete what is not there. R7-5 — snapshot retirement is a latch. The registry's dispatch-drain callbacks emit status changes on TERMINAL entries with no status gate, and in-flight dispatches keep draining across the snapshot write, so a terminal emission routinely landed after `notifySnapshotPersisted` had retired the cache entry — re-inserting the run as "never persisted". A sibling's deletion was then undone by the next refresh, which reads "absent on disk, present in cache" as a pending write and republishes. Persistence is now remembered per runId and `#rememberWorkflowHistory` returns early for members; the latch releases when the runId goes active again, so a genuine re-run is still cached. R7-10 — the liveness gate sees runs whose session is gone. It iterated `this.sessions` only, but close/kill/shutdown use force semantics and a background run owns a detached controller, so after `removeStoredSessionEntry` a still-settling run was invisible to the gate and unreachable by the delete handler's sibling `removeTerminal` loop: a sibling delete-history removed the LIVE run's journal and snapshot, and the orphan's settlement write recreated it. Two halves — `Session.dispose()` now aborts its workflow registry the way it already aborts the agent registry (before the callbacks are torn down), and the registry of a removed session is retained here until its runs drain, so the gate still answers across the settlement window an abort cannot compress to zero. Retention is bookkeeping: a Config that cannot answer is logged, never turned into a shutdown failure. Regressions, each mutation-checked against its own repair: - acpAgent: a sibling's parked retry makes delete-history answer `{changed: false}` without reaching the store, and the deletion goes through once the claim releases - acpAgent: a live run's registry stays visible to the gate across its session's close, and is dropped once the handle is released - Session: a status emission after `snapshotPersisted` no longer resurrects a sibling-deleted run (reverting the latch reproduces the reviewer's probe verbatim), and a re-registered runId is remembered again - Session: a run with the oldest startTime behind 30 newer snapshots is deletable - Session: dispose aborts the workflow registry before clearing its callbacks Verification: Session.test.ts 733 passed, acpAgent.test.ts 514 passed, `tsc --noEmit -p packages/cli` clean, eslint and prettier clean. The 14 `packages/cli/src/serve` failures (fast-path import boundary, capabilities-docs contract, workspace fs/agents/memory, conversation runtime ownership) reproduce identically on the unmodified head — base skew, untouched by this change. Claude-Session: https://claude.ai/code/session_01M7z4PccYfDPyyfg3oGr8V1 * fix(core): drop the duplicated telemetry-swap mock property Merging upstream/main (053f17b) clears the 9 `client.telemetrySwap` failures this branch had from predating QwenLM#10220, but main carries its own TS1117 on the same file: two commits added `getToolRegistry` to the same object literal independently and neither saw the other. 032b907 feat(serve): backfill session PR bindings ... (QwenLM#9729) 8241905 test(core): give the telemetry-swap client mock a getToolRegistry (QwenLM#10220) Checking that file out from upstream/main here and running `tsc --noEmit -p packages/core` reproduces `client.telemetrySwap.test.ts(103,5): error TS1117` verbatim, so taking the merge unmodified would have traded 9 test failures for a build that never reaches the tests at all. Removed as part of the merge rather than left for a follow-up: keeps QwenLM#10220's copy, which was added for this purpose and carries the explanation, and drops QwenLM#9729's incidental one. main still needs the same removal — this only keeps it out of the branch. Verified after the merge: `tsc --noEmit` clean for both packages/core and packages/cli; client.telemetrySwap 10 passed, Session.test.ts and acpAgent.test.ts 1260 passed together. Claude-Session: https://claude.ai/code/session_01M7z4PccYfDPyyfg3oGr8V1 * fix(workflows): guard pending run lifecycle * fix(workflows): let a starting run be cancelled, and stop two slow leaks Four of the behavioural items the review deferred across rounds 8-9; the test-coverage-only entries stay deferred. - Cancel during the start window (R9 acpAgent.ts:10884 + workflow-runner.ts:181). Between `reserveStart` and `register` the runner loads the script and replays the journal — seconds, for a resume of a large one — and the registry has no entry yet. `sessionTaskCancel` answered `not_found` for a run the client could see starting, and `registry.cancel` could not reach the reserved controller either. New `cancelStarting` aborts it (the reservation stays the runner's to release, as after `abortAll`), and the cancel handler routes there when the liveness gate would say "starting". Doing that exposed the second half: the runner threw a bare `Error` for an abort during start, and the tool's catch only recognised the CALLER's signal — a registry-side abort surfaced as an unexplained failure. It is now a typed `WorkflowStartCancelledError`, mapped to the same "cancelled before it could start" result. - `detachedWorkflowRegistries` (R8 acpAgent.ts:3386) was pruned only inside the delete-history liveness check. A daemon that closes sessions mid-run and never deletes history retained every registry for its lifetime. Prune on session close as well. - Refresh/delete race (R9 Session.ts:3428). `refreshWorkflowHistory` reads the directory and merges without a claim; a delete landing between the read and the merge was overwritten by the stale listing and the run reappeared until the next refresh. Deletions are now sequenced, and a refresh drops any run deleted after its read began — keyed by runId and compared against the refresh's own mark, so a later retry that reuses the id is not suppressed. - The "Register a new run" JSDoc sat on `reserveStart` (R9 registry:594). Mutation-verified, all four at once against the full suites: disabling the starting-window branch, the prune-on-close, the deletion filter, and the typed-error mapping reddens exactly the four new tests and nothing else. Claude-Session: https://claude.ai/code/session_01VXsC4f71S6U6YkW82NRw7m * fix(core): close the round-5 review findings on workflow task controls - Key the starting-window cancel on a live reservation rather than on the absence of an entry: a retry reuses its runId, so its terminal entry shadowed the reservation and cancel answered `not_running` about a run that was actively starting. - Answer `changed: false` from retry when `execute()` reports a start that never registered (no `workflowRunId`), mirroring rerun and run-saved. - Classify a registry-side abort of the reserved controller as a cancel in foreground starts too, not only background ones; the tool maps `WorkflowStartCancelledError` in either mode. - Report reserved-but-unregistered runs as workflow active-work holds (`WorkflowRunRegistry.listStartingRunIds`), so a daemon conditional close cannot dispose the session under a start it just accepted. - Propagate a successful history deletion into every sibling session's deletion marker and cached history, symmetric to the `removeTerminal` sweep, so a sibling refresh that had already read the directory cannot republish the deleted run. Claude-Session: https://claude.ai/code/session_018dYE4LwSMeMPFchXk5UBdM * fix(serve): keep workflow retry and the workflowsEnabled flag consistent across sessions Two cross-session gaps in the workflow control surface: - A retry consulted only the requesting session's registry. Every session shares one journal/snapshot store and the task-global claim is released as soon as the background start returns, so a sibling whose registry still showed the run `failed` started a second runner under the same runId. Retry now refuses while the runId is live in any session (or in its own starting window), checked synchronously beside canStart so the answer cannot go stale before the claim is taken. - Workspace reload updated `tools.workflowsEnabled` for `/capabilities` but never told existing sessions; `Config.workflowsEnabled` was set once at construction. The reload's `tools` branch now propagates the flag and pushes an available-commands update when it flips. Claude-Session: https://claude.ai/code/session_01NkW1J2aBKcsKS62dkPcWbT * fix(serve): refuse to report a workflow history deletion whose registry entry survived `deleteWorkflowHistory` ignored `removeTerminal()`'s answer. The registry refuses to remove a live or handle-held entry — its own last word on whether the run is still active in this session — so a `false` for an entry that exists meant the run re-registered under the deletion and would re-persist the history the client was just told was gone. The entry is now retired before the store is touched, and a refusal fails the deletion; a persisted-only run has no entry and is unaffected. Claude-Session: https://claude.ai/code/session_01NkW1J2aBKcsKS62dkPcWbT * docs(serve): fix a stale method reference in the workflow history comment The persisted latch is released inline at the top of #rememberWorkflowHistory; no #forgetPersistedWorkflowRun exists. Claude-Session: https://claude.ai/code/session_01NkW1J2aBKcsKS62dkPcWbT * fix: report workflow lifecycle changes * fix(core): repair the workflow test doubles and converge every eviction The previous commit added a `listStartingRunIds()` call to `describeBlockingBackgroundWork` without teaching the registry test doubles about it, so seven tests across `clearCommand`, `useBranchCommand` and `useResumeCommand` threw `listStartingRunIds is not a function`. Add the reader to those stubs. Also move the eviction's status emission from `releaseHandle` into `evictTerminal` itself. Emitting only from `releaseHandle` left the same convergence gap at the four sweeping call sites that already emit: complete / fail / cancel / abortAll emit BEFORE they sweep, so a consumer that re-reads on the callback observes the pre-eviction list and keeps rendering a row that was just dropped. Emitting once after the sweep closes all five paths, and a future eviction site inherits the guarantee. New test pins the ordering; another pins that a release which evicts nothing stays silent. A starting reservation now reports `starting` rather than borrowing `running`, and its line no longer repeats the run id the bracket already carries. Claude-Session: https://claude.ai/code/session_01VENc5rZYmMJDdqvjwBEyZd * fix(cli): drop the duplicated mock keys and stop misdirecting a starting run R3-2: the previous commit's stub plumbing re-inserted a `list:` line into four `clearCommand.test.ts` registry mocks that already declared one. Last-wins made it behaviour-neutral, and neither eslint (`no-dupe-keys` is off here) nor tsc (the literal is not contextually typed, unlike the `useResumeCommand` mocks where TS1117 did fire) caught it. Remove the duplicates. R3-1: when the only blocker is a reserved-but-unregistered run, the blocked message ended with "Use /workflows to inspect them, then retry." — but `/workflows` renders `registry.list()`, which a reservation has not entered, so it named a surface that cannot show what is blocking. Track inspectability separately and fall back to a plain retry hint when nothing is listable; a registered run in the same set still points at `/workflows`. Claude-Session: https://claude.ai/code/session_01VENc5rZYmMJDdqvjwBEyZd * fix(cli): preserve starting workflow guidance * test(cli): pin background work hint guards * fix(serve): redact untrusted standalone workflow replay * test(web-shell): fix daemon SDK mock path --------- Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
4 participants
What this PR does
This PR exposes Workflow execution through the daemon as an explicit opt-in extension of the existing session task contract. Opted-in clients can inspect live and persisted runs with phase, dispatch, token, log, approval, lineage, and terminal-state data; control active runs; delete persisted history safely; and start saved definitions. The TypeScript SDK and shared WebUI daemon adapter expose separate Workflow-aware methods while their existing task methods retain the legacy agent, shell, and monitor contract.
It also advertises Workflow availability and saved definitions at the workspace/session boundary, and applies the same enabled, non-bare, trusted-session gate to capability reporting and every Workflow mutation. The existing visual task surfaces remain unchanged.
Why it's needed
The core runtime now has structured Workflow execution state, but daemon consumers cannot observe or control it without this transport and client layer. An opt-in boundary lets new clients use that state without widening legacy task unions or forcing incomplete Workflow semantics into existing task UIs.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — this PR adds daemon, SDK, and adapter contracts without a visual UI change.
Tested on
Environment (optional)
Local Node.js workspace with focused ACP, daemon, SDK, WebUI, and CLI tests, followed by the full repository build and typecheck.
Risk & Scope
Linked Issues
Closes #9033
Depends on #9034
中文说明
本 PR 做了什么
本 PR 通过显式 opt-in 的方式，在现有会话任务契约上扩展 daemon Workflow 执行能力。选择加入的客户端可以查看实时和持久化运行，包括阶段、调度、token、日志、审批、血缘和终态数据；可以控制活动运行、安全删除持久化历史并启动已保存的定义。TypeScript SDK 与共享 WebUI daemon adapter 提供独立的 Workflow 感知方法，现有任务方法继续保持 agent、shell、monitor 的旧契约。
同时，本 PR 在 workspace/session 边界声明 Workflow 可用性和已保存定义，并对能力声明及所有 Workflow 变更统一执行“功能启用、非 bare、可信会话”门禁。现有可视化任务界面保持不变。
为什么需要
Core runtime 现在已经具备结构化 Workflow 执行状态，但 daemon 消费端缺少观察和控制它的传输与客户端层。使用 opt-in 边界后，新客户端可以使用这些状态，同时不会扩大旧任务联合类型，也不会迫使现有任务 UI 提前承载不完整的 Workflow 语义。
Reviewer 测试计划
如何验证
证据（前后对比）
N/A — 本 PR 新增 daemon、SDK 和 adapter 契约，不包含可视化 UI 变化。
已测试平台
环境（可选）
本地 Node.js workspace，执行 ACP、daemon、SDK、WebUI 和 CLI 聚焦测试，随后执行完整仓库 build 与 typecheck。
风险与范围
关联 Issue
Closes #9033
Depends on #9034