Repository navigation
feat(workflows): add cooperative pause and resume - #8320
Conversation
E2E evidence / E2E 证据Validated the production bundle in a real terminal harness using node-pty, headless xterm, and Ink on macOS with Node.js v24.14.1. The model transport was the repository fake OpenAI server.
限制：该 E2E 使用仓库 fake OpenAI server，没有覆盖真实 provider 的认证、网络和延迟；暂停是 cooperative，不冻结普通 JavaScript 或任意外部 Promise。 |
Review: feat(workflows): add cooperative pause and resumeOverviewThis PR introduces Particularly good:
Findings1. Foreground workflows are pausable from the Background Tasks dialog, silently blocking the parent turn (medium)
2. A pause request cannot be withdrawn while
3. A throwing listener propagates out of 4. On cancel of a paused run, queued jobs that never ran their thunk are counted via the abort-rejection 5. Docs not updated (low) The new Minor notes
VerdictCore scheduler and lifecycle plumbing look correct; tests are strong and target the actual race windows. Finding 1 (foreground runs pausable from the dialog) is the one thing I'd want resolved or explicitly decided before merge; the rest are polish. |
|
已修复：前台 workflow 现在在 registry 与 Background Tasks UI 两层拒绝 pause，仅后台 run 显示并响应 验证证据：
|
|
已修复：前台 workflow 现在在 registry 与 Background Tasks UI 两层拒绝 pause，仅后台 run 显示并响应 。\n\n验证证据：\n- ✓ src/agents/workflow-run-registry.test.ts (62 tests) 68ms Test Files 2 passed (2) JUNIT report written to /Users/qqqys/Desktop/qys/qwen-code-worktrees/dynamic-workflows-pr3/packages/core/junit.xml ✓ src/ui/components/background-view/BackgroundTasksDialog.test.tsx (66 tests) 8614ms Test Files 1 passed (1) JUNIT report written to /Users/qqqys/Desktop/qys/qwen-code-worktrees/dynamic-workflows-pr3/packages/cli/junit.xml
Successfully copied files.
Building web-templates...
Successfully copied files.
Compiling input files...
Successfully copied files.
vite v5.4.21 building for production... [vite:dts] Start generate declaration files... dist/styles.css 83.30 kB │ gzip: 15.52 kB
vite v5.4.21 building for production...
Static assets synced -> dist/extension
|
Maintainer verification — real-environment run of the reviewer test plan ✅Verified this PR locally against the production Environment: macOS (Darwin 25.6.0), Node v24.18.1, PR head Static checks: Scenario 1 — pause while agent A in flight, then resumeWorkflow: one
Pausing (in-flight A may finish) → Paused (queued work held):
Scenario 2 — pause, then stop instead of resuming
Scenario 3 — foreground unchanged + non-interactive rejection
More screenshots (dialog list with pause hint, foreground run)
Observations (non-blocking)
Conclusion: all four items of the reviewer test plan reproduce on macOS with the production bundle; state transitions, queue holding, FIFO resume, notification exactly-once, stop-while-paused, and the non-interactive guard all behave as described. LGTM from the real-environment verification standpoint. 中文版本（Chinese version）维护者验证 — 真实环境跑通 Reviewer 测试计划 ✅在真实终端（tmux 120×36 pty）中运行生产 环境：macOS（Darwin 25.6.0）、Node v24.18.1、PR head 静态检查： 场景一 — A 在飞时暂停，然后恢复Workflow：
场景二 — 暂停后停止
场景三 — 前台行为不变 + 非交互拒绝
观察项（不阻塞合并）
结论：Reviewer 测试计划四项在 macOS 生产 bundle 上全部复现；状态迁移、队列保持、FIFO 恢复、通知 exactly-once、暂停后停止、非交互拒绝均符合描述。从真实环境验证角度 LGTM。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
— qwen3.7-max via Qwen Code /review
| const toggleSelectedWorkflowPause = useCallback(() => { | ||
| if (!config) return; | ||
| const target = entries[selectedIndex]; | ||
| if (!target || target.kind !== 'workflow' || !target.isBackgrounded) return; | ||
| const registry = config.getWorkflowRunRegistry(); | ||
| if (target.status === 'running') { | ||
| registry.pause(target.runId); | ||
| } else if (target.status === 'paused') { | ||
| registry.resume(target.runId); | ||
| } | ||
| }, [config, entries, selectedIndex]); |
There was a problem hiding this comment.
[Suggestion] No unit test covers toggleSelectedWorkflowPause, the primary new user-facing action in this PR. The Background Tasks dialog integration tests exercise this indirectly through keypress simulation, but a direct unit test would catch future regressions in the callback itself (e.g., swapped status checks, missing isBackgrounded guard, dependency array changes). — Failure scenario: a refactor that changes the callback's early-return conditions or status dispatch would only be caught by the heavier dialog integration tests, not at the unit level.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Declined — the callback is a thin delegation to registry.pause()/registry.resume() gated by status checks. It is already exercised end-to-end by the BackgroundTasksDialog integration tests (66 tests simulating the p keypress), and the registry-level gating is independently tested in workflow-run-registry.test.ts (65 tests, including the new pausing-state approval and over-count cap tests from this round). A direct unit test would require mocking the entire context provider infrastructure for minimal additional coverage beyond what the integration and registry tests already provide.
中文说明
拒绝 — 该回调是对 registry.pause()/registry.resume() 的薄委托，由状态检查门控。BackgroundTasksDialog 集成测试（66 个测试，模拟 p 按键）已端到端覆盖该路径；registry 层门控由 workflow-run-registry.test.ts（65 个测试，包括本轮新增的 pausing 状态 approval 和超额上限测试）独立测试。直接单元测试需要 mock 整个 context provider 基础设施，额外覆盖有限。
| if (target.status === 'running') { | ||
| return registry.pause(runId) | ||
| ? { | ||
| type: 'message' as const, | ||
| messageType: 'info' as const, | ||
| content: `Cooperative pause requested for workflow ${runId}.`, | ||
| } | ||
| : { | ||
| type: 'message' as const, | ||
| messageType: 'error' as const, | ||
| content: `Workflow ${runId} could not be paused because its state changed.`, | ||
| }; |
There was a problem hiding this comment.
[Suggestion] The error message "could not be paused because its state changed" is misleading when registry.pause() returns false because the workflow is not backgrounded. The command checks target.status === 'running' before calling pause(), but the registry also gates on entry.isBackgrounded — so a foreground workflow with status === 'running' passes the command's check but fails the registry's, and the user sees a state-change error that doesn't apply. — Failure scenario: user types /workflows p <runId> on a foreground workflow and receives "could not be paused because its state changed" when the actual reason is that the workflow isn't running in the background.
| if (target.status === 'running') { | |
| return registry.pause(runId) | |
| ? { | |
| type: 'message' as const, | |
| messageType: 'info' as const, | |
| content: `Cooperative pause requested for workflow ${runId}.`, | |
| } | |
| : { | |
| type: 'message' as const, | |
| messageType: 'error' as const, | |
| content: `Workflow ${runId} could not be paused because its state changed.`, | |
| }; | |
| if (target.status === 'running') { | |
| if (!target.isBackgrounded) { | |
| return { | |
| type: 'message' as const, | |
| messageType: 'error' as const, | |
| content: `Workflow ${runId} is not running in the background and cannot be paused.`, | |
| }; | |
| } | |
| return registry.pause(runId) | |
| ? { | |
| type: 'message' as const, | |
| messageType: 'info' as const, | |
| content: `Cooperative pause requested for workflow ${runId}.`, | |
| } | |
| : { | |
| type: 'message' as const, | |
| messageType: 'error' as const, | |
| content: `Workflow ${runId} could not be paused because its state changed.`, | |
| }; |
— qwen3.7-max via Qwen Code /review
| return scheduler.waitUntilRunning().then(() => { | ||
| throw error; | ||
| }); |
There was a problem hiding this comment.
[Suggestion] waitUntilRunning() in the error path can reject with AbortError, replacing the original dispatch error that the handler intended to re-throw. If the scheduler is paused when a dispatch fails, and the user then cancels (triggering abortPending() which rejects gate waiters), the AbortError from the gate replaces the real dispatch error (e.g., rate limit, budget exceeded) in the caller's rejection chain. — Failure scenario: dispatch fails with a diagnostic error → waitUntilRunning() creates a gate waiter → user cancels → abortPending() rejects the waiter with AbortError → .then(() => { throw error }) never executes → the workflow run outcome carries AbortError instead of the dispatch-level diagnostic.
| return scheduler.waitUntilRunning().then(() => { | |
| throw error; | |
| }); | |
| return scheduler.waitUntilRunning().then( | |
| () => { | |
| throw error; | |
| }, | |
| () => { | |
| throw error; | |
| }, | |
| ); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
已修复。验证：cd packages/core && npx vitest run src/agents/runtime/workflow-orchestrator.test.ts（124/124），根目录 npm run build && npm run typecheck、修改文件 ESLint 与 git diff --check 均通过。
Maintainer re-verification at latest head
|
| Step | Expected | Observed |
|---|---|---|
p in Background Tasks while A in flight |
Running → Pausing |
✅ detail shows … Pausing + "Pause is cooperative; in-flight work may finish before the workflow is paused."; hint flips to cooperative pause pending |
| A's request released | Pausing → Paused, B/C stay queued |
✅ ⏸ Paused · 1/3 agents, hint p resume (cooperative) |
| While paused | no new provider traffic | ✅ provider request log frozen for the entire paused window (> 3 min) — zero requests for B or C |
/workflows after closing the dialog |
paused run stays in Active bucket | ✅ Active: wf_fed71499c61f2912 paused |
/workflows p <runId> |
FIFO resume, one completion notification | ✅ B dispatched at 02:08:50.461, C at 02:08:50.471 (A→B→C order), exactly one task-notification model turn at 02:08:50.496, "Background workflow "pause-verify" completed." |
Provider request log (the hard evidence — every model request the CLI actually issued):
02:03:16 launch-turn (workflow tool call returned)
02:03:16 agent-alpha (dispatch #1 in flight, held open by the test gate)
02:04:16 agent-alpha (60s client retry of the held request — harness artifact, see note)
02:05:16 agent-alpha (retry; gate released → ALPHA_DONE; run converges to Paused)
--- paused window: no bravo / no charlie / no notification ---
02:08:50.461 agent-bravo (resume requested via /workflows p)
02:08:50.471 agent-charlie (FIFO after bravo)
02:08:50.496 notification-turn (exactly one completion notification)
Note: the duplicate agent-alpha lines are the provider client's 60s streaming-timeout retries against my deliberately stalling fake server — in Scenario 2, where the gate opens within 60s, agent A issues exactly one request. Not PR behavior.
| Pausing (A in flight) | Paused (1/3, B/C queued) |
|---|---|
 |
 |
Full sequence — /workflows Active bucket while paused, resume via /workflows p, single completion notification:
Scenario 2 — pause, then stop instead of resume (test-plan step 3)
| Step | Expected | Observed |
|---|---|---|
pause → release A → Paused |
as scenario 1 | ✅ (A issued exactly one provider request this time) |
x, x to confirm stop |
queued agents never start | ✅ request log ends at A — zero B/C requests, ever |
| after stop | no success notification | ✅ zero task-notification turns; detail shows ✖ Stopped |
| next prompt | parent session usable | ✅ follow-up prompt round-trips normally (PONG_OK) |
| Stopped from paused | Session still usable |
|---|---|
 |
 |
Scenario 3 — foreground runs reject pause (verifies the b84c144 fix; test-plan step 4)
Started the same workflow without run_in_background and opened the Background Tasks dialog mid-turn while agent A was in flight:
- The foreground workflow row shows no
p pausehint (onlyx stop), and pressingpis a no-op — status staysrunning, noPausingtransition. This is the review finding from the first round, now fixed and verified in the real TUI. - After releasing the gates the run completed synchronously through the normal tool-result channel with zero background notifications — foreground semantics unchanged.
Guard rails (real TUI)
/workflows p wf_deadbeef00000000 → Unknown live workflow runId; /workflows p <completed runId> → Workflow … is completed and cannot be paused or resumed. Non-interactive/ACP rejection and the foreground /workflows p error path are covered by the passing unit tests (workflowsCommand.test.ts).
More screenshots (running dialog with p hint · /workflows paused listing · guard errors)
Observations (non-blocking)
- A stopped-from-paused run displays
3/3 agentsalthough only agent A ever executed — queued dispatches settled as rejected by cancellation are counted intoagentsCompleted(deliberate perbeba054's exactly-once settlement; review finding 4). Fine as-is, but a UI distinction between "ran" and "settled by cancellation" would avoid misreading. - User docs (
docs/users/reference/keyboard-shortcuts.md, commands doc) still don't mention thepkey //workflows p(review finding 5). Suggest a small follow-up.
Verdict
All four reviewer-test-plan scenarios pass against the production bundle at the latest head, the pause gate provably keeps queued dispatches out of the provider, resume preserves FIFO order with exactly one completion notification, stop-from-paused leaks nothing, and the first-round review findings 1 (foreground pause) has been fixed and re-verified. LGTM from the verification standpoint — merge-ready, with the docs follow-up tracked as a nice-to-have.
中文版本（Chinese version）
维护者复验 — 最新 head 552cb77,后续修复已确认 ✅
上一轮验证针对的是首个提交 b88fdacbf。此后作者推送了 4 个针对评审意见的修复提交(b84c144 限制仅后台运行可暂停、2337d58 澄清前台暂停错误提示、beba054 取消时保留 dispatch 错误、552cb77 补充迟到状态回调测试),因此本轮在当前 head 552cb7707 上重跑了全部验证:生产 dist/cli.js bundle、真实终端(tmux 120×42 pty)、仓库自带 fake OpenAI server 作为模型后端(agent 请求阻塞在释放文件上,可精确复现"第一个 agent 在飞时暂停")、隔离的 $HOME 与 fixture 工作区。
环境:macOS (Darwin 25.6.0)、Node v24.18.1、QWEN_CODE_ENABLE_WORKFLOWS=1、QWEN_CODE_MAX_WORKFLOW_CONCURRENCY=1、--yolo。
静态检查(均在 552cb77):lint ✅ · typecheck ✅ · build + bundle ✅。Focused 测试:core 254/254(6 个被改动的 core 文件)、CLI 132/132(4 个被改动的 CLI 文件);比首个 head 的 251/128 多,是因为修复提交新增了测试。GitHub 显示分支可合并到 main。
场景 1 — agent A 在飞时暂停,随后恢复(测试计划步骤 1–2)
Workflow:parallel([A, B, C]),并发度 1,run_in_background: true,A 的 provider 请求被测试门挡住。
- Background Tasks 中按
p(A 在飞)→ ✅ 详情页显示… Pausing与"Pause is cooperative…"提示,快捷键提示变为cooperative pause pending - 放行 A 的请求 → ✅
⏸ Paused · 1/3 agents,提示p resume (cooperative) - 暂停期间 → ✅ provider 请求日志在整个暂停窗口(> 3 分钟)完全静止,零条 B/C 请求
- 关闭对话框后
/workflows→ ✅ 暂停中的运行保留在 Active 分组(paused) /workflows p <runId>恢复 → ✅ B 于02:08:50.461下发、C 于.471下发(A→B→C FIFO),恰好一条完成通知(02:08:50.496),提示 "Background workflow "pause-verify" completed."
(请求日志摘录见英文部分;重复的 agent-alpha 行是 provider 客户端对被故意挂起请求的 60 秒流式超时重试——场景 2 中 60 秒内放行时 A 只发出一条请求,与 PR 行为无关。)
场景 2 — 暂停后停止而非恢复(测试计划步骤 3)
- 暂停 → 放行 A →
Paused(本次 A 恰好只有一条请求)→ 按x、x确认停止 - ✅ 请求日志止于 A——B/C 从未下发;✅ 零条完成通知;详情显示
✖ Stopped;✅ 后续消息正常往返(PONG_OK),父会话可用
场景 3 — 前台运行拒绝暂停(验证 b84c144 修复;测试计划步骤 4)
不带 run_in_background 启动同一 workflow,turn 进行中打开 Background Tasks:前台 workflow 行没有 p pause 提示(仅 x stop),按 p 为无操作——状态保持 running。这正是第一轮评审的发现 1,现已修复并在真实 TUI 中复验。放行后运行通过正常工具结果通道同步完成,零条后台通知——前台语义未变。
护栏(真实 TUI)
/workflows p wf_deadbeef00000000 → Unknown live workflow runId;对已完成运行执行 → is completed and cannot be paused or resumed.。非交互 / ACP 拒绝与前台 /workflows p 错误路径由通过的单测覆盖。
观察(不阻塞合并)
- 从暂停停止的运行显示
3/3 agents,但实际只有 A 执行过——被取消拒绝的排队 dispatch 也计入agentsCompleted(beba054的 exactly-once 结算语义,评审发现 4)。现状可接受,但 UI 上区分"执行过"与"被取消结算"更不易误读。 - 用户文档(快捷键、命令文档)仍未提及
p键 //workflows p(评审发现 5),建议小型后续 PR 补上。
结论
四个评审测试计划场景在最新 head 的生产 bundle 上全部通过:暂停门可证明地把排队 dispatch 挡在 provider 之外;恢复保持 FIFO 且恰好一条完成通知;暂停后停止无泄漏;第一轮评审发现 1(前台可被暂停)已修复并复验。从验证角度 LGTM,可合并;文档补充作为 nice-to-have 跟进。
|
@qwen-code /triage |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/30729968501)._ |
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at 552cb77. Two of my three earlier findings are fixed; the third stands. More importantly, I ran mutation probes rather than take the outstanding vacuity claims on trust, and all three I tested survived — so the guards this PR adds to the registry are not pinned by any test in the suite.
My earlier findings, at this HEAD
- Fixed — the misleading pause error.
workflowsCommand.tsnow has an explicitif (!target.isBackgrounded)branch before the status branches, with its own accurate message ("Foreground workflow runs cannot be paused or resumed…"), so "could not be paused because its state changed" is now reachable only for the genuine race. This also closes the ci-bot's near-duplicate at :232. - Fixed — the dispatch error being replaced by
AbortError.workflow-orchestrator.ts:1580-1587now re-throws the originalerrorfrom both arms of thewaitUntilRunning()settlement, so a gate rejection can no longer overwrite the cause the handler meant to surface. - Stands —
toggleSelectedWorkflowPausestill has no direct unit test.git grepfinds it only inBackgroundTasksDialog.tsxandBackgroundTaskViewContext.tsx; no.test.file references it, so the primary new user-facing action is covered only indirectly through dialog keypress simulation.
Mutation probes — three guards, three survivors
Baseline first: the six workflow suites are 254/254 green at this HEAD (workflow-run-registry, workflow-orchestrator, workflow-runner, workflow-dispatch-scheduler, workflow-snapshot, workflow). Then, one mutation at a time, restoring in between and confirming the file byte-identical to HEAD afterwards:
| mutation | result |
|---|---|
delete the over-count cap || entry.agentsCompleted >= entry.agentsDispatched (workflow-run-registry.ts:711) |
survived — suite still green |
revert the approval guard !isActiveWorkflowStatus(entry.status) → entry.status !== 'running' (:588) |
survived — suite still green |
delete the terminal-state guard || isTerminalWorkflowStatus(entry.status) from onDispatchStateChange (:464) |
survived — suite still green |
So the three ci-bot vacuity findings on this file are confirmed by probe, not merely asserted. That matters for what the guards are for: :464 is the only thing stopping a late scheduler callback from moving a settled run back to pausing/paused, :588 is what admits pausing to the approval path (the widening this PR exists to make), and :711 is what keeps agentsCompleted from passing agentsDispatched once cancellation and gate-release race. Each is a new invariant introduced by cooperative pause, and none of them would notice being removed.
I did not probe the remaining claims, but two are checkable by inspection and hold: workflowsCommand.test.ts:135 asserts toContain('p') — a single character that the fixture runIds wf_pausing/wf_paused and the status column supply on their own, so it cannot fail while those strings are present; and the createConcurrencyLimiter import swap in workflow-orchestrator.ts:49 does leave utils/concurrencyLimiter.ts with no production consumer.
Assessment
The implementation reads coherent and the three-state lifecycle is threaded through the consumers consistently — I am not disputing the design, and the two fixes above were the right ones. But this is a concurrency feature in packages/core/src/agents/** whose new invariants are, measurably, untested: three separate guards can each be deleted without a single test noticing. Per AGENTS.md a missing test is a Suggestion rather than a Critical, and I am not blocking — a maintainer has already approved at this exact commit. I am recording the probe results because "the suite is green" is currently weak evidence for this diff specifically, and the cheapest way to make it strong is one test per guard: drive onAgentCompleted past the dispatched count, transition an entry to pausing before an approval, and deliver a scheduler state change after a run has settled.
Process note for the record, not a request: this is a feat (not refactor) from a non-maintainer touching core, at roughly 335 production lines in packages/core — under the 500-line hard block and under the 1000-line advisory, so the two-tier gate is satisfied by wenshao's approval rather than needing escalation. Flagging it only because AGENTS.md asks for awareness on core-touching external PRs.
All local runs at this HEAD; the registry file was restored and verified byte-identical after probing. CI green.
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
- Rename misleading `terminal` local to `presentation` in BackgroundTasksDialog
- Fix vacuous `toContain('p')` assertion to `toContain('Background tasks + p')`
- Fix vacuous gate assertion with macrotask yield in scheduler test
- Add over-count cap test for `onAgentCompleted` past dispatched count
- Add pausing-state approval parking test
- Remove dead `concurrencyLimiter` module (no production consumers)
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Address review summary — PR #8320Feedback points and dispositionsImplemented this round
Verified as already fixed (earlier commits)
Declined with evidence
Conflict notesNo conflicts ( Verification
中文说明评审意见处理总结 — PR #8320反馈要点与处理本轮实施
已验证为先前提交已修复
附证据拒绝
冲突说明无冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round-12 review feedback — summaryAll findings were verified against the live head Fixed1. Budget gate and agent-cap rejections bypass the pause gate (Medium, standing) In 2. A paused run blocked Implemented the reviewer's predicate-split option:
3. Pressing
4. Stale Removed from Verified already fixed (round 11) — no action needed
Declined
For the maintainer (no code action)
Verification
中文说明第 12 轮评审反馈 — 总结所有发现均已在最新的 head 已修复1. 预算门禁与 agent 上限的拒绝绕过了暂停门（Medium，仍然成立） 在 2. 暂停的运行无限期阻塞 采用评审者给出的"拆分谓词"方案：
3. 在
4. 过期的 已从 已核实第 11 轮已修复 — 无需处理
已拒绝
交由维护者决定（不做代码改动）
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round 13 — review feedback addressedAll 34 inline findings (3 Critical, 31 Suggestion) were verified against the current code and addressed in one batch ( Critical findingsR11-1 — settlement guard only special-cased R11-2 — watchdog pause-banking used R11-3 — adoption escapes disarmed the mirror (forgotten-await failure class). Design decision, implemented as suggested (consumption ≠ handling) plus a host-side escape hook, because the adopting promise (typically an async wrapper's implicit promise) is a plain vm-realm Promise the mirror cannot observe, and nothing inside the sandbox can reach it:
Sandbox mirror Suggestions
Pause/resume surface Suggestions
Registry / persistence Suggestions
Docs Suggestions
Review-level concern (integration suite not run)The round-12 review's CHANGES_REQUESTED ("Integration Tests (CLI, No Sandbox) were skipped in CI, and the suite was not run locally") is addressed by actually running the suite locally this round — see Verification. Result: 182 passed / 1 failed / 18 skipped. The single failure is environmental, not code:
VerificationAll commands actually run, in the final state (commit
中文说明第 13 轮 —— 评审反馈处理情况全部 34 条行内发现（3 条 Critical、31 条 Suggestion）均已对照当前代码核实，并在一个提交批次（ Critical 发现R11-1 —— 结算守卫只特判 R11-2 —— 看门狗暂停记账使用 R11-3 —— 收养（adoption）逃逸使镜像失效（忘写 await 的失败类别）。 按建议方向（消费 ≠ 处理）实现，并辅以宿主侧逃逸钩子 —— 因为收养方 promise（通常是 async 包装器的隐式 promise）是镜像无法观察的普通 vm-realm Promise，且沙箱内部无法触及它：
沙箱镜像相关 Suggestion
暂停/恢复界面相关 Suggestion
注册表 / 持久化相关 Suggestion
文档相关 Suggestion
评审级关注点（集成测试套件未运行）第 12 轮评审的 CHANGES_REQUESTED（"Integration Tests (CLI, No Sandbox) 在 CI 中被跳过，且该套件未在本地运行"）已通过本轮实际本地运行该套件来处理 —— 见下方验证清单。结果：182 通过 / 1 失败 / 18 跳过。唯一失败为环境问题，与代码无关：
验证（Verification）最终状态（提交
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Maintainer re-verification at head
|
| # | Test-plan item | Result |
|---|---|---|
| 1 | Pause while agent A is in flight → Pausing → Paused, B and C still queued |
✅ |
| 2 | Paused run stays in the /workflows Active bucket; /workflows p <runId> resumes in FIFO order; exactly one completion notification |
✅ |
| 3 | Stop a paused run instead of resuming → queued agents never start, no success notification, session stays usable | ✅ |
| 4 | Foreground keeps synchronous semantics; the dialog refuses pause on a foreground run | ✅ (see note C) |
| 5 | Pause-aware wall-clock watchdog: paused time is not charged, backstop still armed while running | ✅ re-confirmed |
1 · Running → Pausing → Paused
p was pressed while agent A was still gated at the provider. The run went to … Pausing · 0/3 agents with the honest banner "Pause is cooperative; in-flight work may finish before the workflow is paused", and only reached ⏸ Paused · 1/3 agents after A settled. The footer hint tracks the state exactly: p pause while running, no p at all while pausing, p resume once paused — you cannot ask for a transition the state does not accept.
2 · Paused stays Active, and resumes from the slash command
/workflows listed the run under Active as paused (not dropped into the terminal bucket), and /workflows p <runId> — the process-local control path, not the dialog — resumed it.
The provider-side timeline is the load-bearing evidence:
77.2 seconds of pause with zero requests for B and C, then B and C in script order after resume, then exactly one <kind>workflow</kind> completion carrying {"out":["marker-A-done","marker-B-done","marker-C-done"]} — the position-aligned array survives the pause. The first run of this scenario produced the same shape with a 77.0 s window.
3 · Pause → stop
Stopping the paused run recorded it as cancelled, B and C never reached the provider (1 agent request total, for A), zero completion notifications were emitted, and the parent session answered the very next prompt. Process stderr stayed clean across every scenario in this report — no unhandledRejection from cancelled gate waiters.
5 · Wall-clock watchdog (re-checked because round 13 rewrote the sandbox)
- Held paused for 52 s on a 25 s budget → the run was not killed mid-pause, resumed cleanly, and completed. Paused time genuinely is not charged.
- Control, same 25 s budget, never paused → aborted at exactly 25 s with
Workflow execution exceeded 25000 ms of active time (paused time is not counted).The backstop is suspended, not removed.
New since my last pass — rounds 11–13
R12 · a paused run no longer blocks /clear
This is the behaviour change with the most operator-visible consequence, so I ran it as an A/B.
- Running →
/clearstill refused:Stop the current session's running background tasks before starting a new session. - Paused →
/clearwent through. - The paused run was genuinely aborted, not leaked: over 20 s of quiet afterwards, zero further provider requests, zero completion notifications, and an on-disk snapshot recording
status: "cancelled". TheabortAll()-before-reset()ordering does what its comment claims.
I agree with the call — a paused-and-forgotten run has its watchdog suspended, so if it blocked the switch there would be no backstop to release it. But it does mean a session switch silently cancels a paused run, and that is not mentioned in the PR description's Risk & Scope section. Worth one line there.
R12 · p during Pausing now answers instead of looking stuck
A second p while the run is still pausing lights Pause/resume was rejected; the workflow state changed. Try again. and the flash clears itself ~3 s later. Since pausing can last a whole subagent dispatch, this is a real improvement over the previous silent no-op.
R11 / R13 · /workflows p on a run the registry no longer holds
After a session switch emptied the registry, /workflows p <runId> for a run that is still listed from its snapshot answers Workflow <id> is cancelled and cannot be paused or resumed. — not the contradictory "Unknown live workflow runId" it used to give. An id present in neither source still gets Unknown live workflow runId. A completed run gets the same terminal wording. Both halves behave as described.
Tests and hygiene at this head
packages/corefocused: 438 passed / 7 files (workflow-dispatch-scheduler,workflow-orchestrator,workflow-runner,workflow-sandbox,workflow-run-registry,workflow-snapshot,tools/workflow) — up from 402 at my last pass.packages/clifocused: 188 passed / 6 files (workflowsCommand,BackgroundTasksDialog,BackgroundTasksPill,useBackgroundTaskView,backgroundWorkUtils,useBranchCommand).npm run build,npm run bundle,npm run typecheck(0 TS errors),npm run lint— all clean. (Lint reported 26 problems, all of them in my own harness files under an untrackedharness/directory, none in the PR's files.)- The deleted
utils/concurrencyLimiter.tsstill has no remaining references anywhere underpackages/.
Three non-blocking notes
A. Round 11's stated hazard for Promise.all does not reproduce — the fix improves log wording, not crash-safety
Round 11 (R10-9) justified wrapping Promise.all / race / any by saying that without it, a fire-and-forget aggregate holding a failed dispatch "fires a process-level unhandledRejection (the interactive CRITICAL banner, or Node's default --unhandled-rejections=throw termination in headless hosts)". I A/B'd that claim: I disabled just the three static wrappers, rebundled, and re-ran two fire-and-forget shapes (a bare rejecting aggregate, a .then() derived off one, and a nested aggregate), using the per-run agent cap to produce a deterministic dispatch rejection.
Neither build crashed. No CRITICAL banner, no unhandledRejection on stderr, process alive in both. What actually changed is the mirrored log line:
| script | with the wrap (this PR) | wrap disabled |
|---|---|---|
fire-and-forget Promise.all + .finally() on a rejected dispatch |
result not consumed + rejection not handled |
rejection not handled ×2 |
.then() derived off a rejecting aggregate + a nested aggregate |
rejection not handled + result not consumed |
rejection not handled ×2 |
(Both lines are prefixed dispatch failed (…): Workflow exceeded the maximum of N agent() calls per run.; I am not claiming which line maps to which shape, only the classification each build produced.)
Both builds emit the same number of lines. The host-side adoption-escape hook — which the R11-16 comment itself mentions — is the real backstop; the aggregate wrap refines the classification. That is a legitimate improvement and I am not asking for it to be reverted. But the round-11 summary overstates the risk it retired, and nobody should treat "would otherwise terminate the process" as verified.
B. The persisted agent counter for a terminated paused run depends on how it was terminated
Same script, same outcome — only agent A ever reached the provider — but:
| how the paused run ended | live /workflows |
on-disk snapshot | /workflows after a restart |
|---|---|---|---|
x from Background Tasks |
3/3 agents |
dispatched=3 completed=3 |
3/3 agents |
/clear while paused |
— | dispatched=3 completed=1 |
1/3 agents |
The /clear path calls abortAll() then reset(), so the draining dispatches' onAgentCompleted() calls find no entry and stop counting; the dialog path keeps the entry and the drain lands before the snapshot write. Both numbers are defensible under "settled" versus "executed", and R11-10 (widening the drain past cancelled to completed/failed) and R13 (freezing the snapshot projection before the first await) each make sense on their own — they just pull in opposite directions. The visible result is that a run where one agent executed persists as either 1/3 or 3/3 depending on how it was killed. I also hit the failed face of this in the watchdog control run: a run aborted at 25 s with a single dispatch to the provider reports failed · 3/3 agents.
This is the concrete second face of the cosmetic note I raised last round. Still cosmetic, still non-blocking — flagging it because it is now reachable on completed and failed runs, not just cancelled ones.
C. The live-foreground /workflows p wording is effectively unreachable from the TUI
A foreground workflow blocks the composer for its entire lifetime, so /workflows p <runId> typed during one is queued and only submits after the run has already settled — at which point round 13's new terminal check answers first with the terminal wording. I confirmed the terminal wording at this head; I could not reach the foreground wording through the TUI this round (I did observe it at fd8a6ed77). The branch is still correct defensive code and the dialog-level foreground gate is re-confirmed here — a foreground run offers x stop but no p pause, and pressing p is a no-op with no flash. Just noting that its user-facing string is close to theoretical.
Not covered by this run
Real provider authentication and network latency, durable cross-process resume, journal durability, per-agent controls, macOS and Windows. Gated agent requests would hit the provider client's stream timeout if a pause were held for minutes; every pause window here stayed well under it, so no retry noise contaminated the timelines.
中文版本
维护者在最新 head 750133f4 上的复验 —— 真实终端 + 生产 bundle，运行于 Linux ✅
我此前三轮验证分别跑在 b88fdacbf、552cb77 和 fd8a6ed77 上。之后 round 11–13 陆续合入，其中包括对 workflow-sandbox.ts 的又一次大改（仅 round 13 就是 +446/−147）、对「什么会阻塞 session 切换」的调整，以及 /workflows p 的新文案。我在当前 head 上从零重建 harness，重跑了完整的 reviewer 测试计划，并新增五个专门针对上次之后改动的场景。
结论：可以合并。 reviewer 测试计划每一项都复现。下面三条不阻塞的说明——其中一条推翻了 round 11 总结中的一个说法，在把该说法当作「已验证」之前值得一读。
PR 的「Tested on」表格中 🐧 Linux 标为
环境
在隔离的 git worktree 中检出 750133f4007ce1a5e5e8534e4debbbe298186849，完整执行 npm run build + npm run bundle；以下全部操作驱动的都是生产 dist/cli.js，不是 dev 入口，也不是单测 harness。
- 模型侧：仓库自带的
integration-tests/fake-openai-server.ts，外面包一层，使每个 agent 请求阻塞在释放文件上。正因如此，「agent A 在飞、B 与 C 排队」是确定状态，而非竞态。 - UI：
tmux new-session -x 130 -y 42的真实 pty，隔离QWEN_HOME，QWEN_CODE_ENABLE_WORKFLOWS=1，QWEN_CODE_MAX_WORKFLOW_CONCURRENCY=1。 - 被测脚本：
parallel([agent A, agent B, agent C])，并发窗口为 1。 - 每个 provider 请求都带时间戳写入
requests.jsonl，所以**「没有到达 provider」是观测结果，不是推断**。 - 场景 1–3、
/clearA/B 与 pausing 反馈检查各完整跑了两遍，时间线与结论均可复现。
给想重建此 harness 的人两个坑：fake server 必须只按最后一条 user 消息路由；工具要用注册名
workflow调用（不是显示名Workflow）。主对话历史把脚本正文嵌在 assistant 的 tool-call 参数里，全文匹配会把主 turn 误判成 agent 请求并卡死在门上。
Reviewer 测试计划
| # | 测试项 | 结果 |
|---|---|---|
| 1 | agent A 在飞时暂停 → Pausing → Paused，B、C 仍在队列 |
✅ |
| 2 | 暂停中的 run 保留在 /workflows 的 Active 分组；/workflows p <runId> 按 FIFO 恢复；恰好一次完成通知 |
✅ |
| 3 | 暂停后选择停止而非恢复 → 队列 agent 永不启动、无成功通知、父会话仍可用 | ✅ |
| 4 | foreground 保持同步语义；对话框拒绝对 foreground run 暂停 | ✅（见说明 C） |
| 5 | 可感知暂停的 wall-clock watchdog：暂停时间不计费，运行期间 backstop 仍武装 | ✅ 再次确认 |
1 · Running → Pausing → Paused
在 agent A 仍被 provider 侧门挡住时按下 p：状态进入 … Pausing · 0/3 agents，并给出与事实一致的提示「Pause is cooperative; in-flight work may finish before the workflow is paused」；只有在 A 结算之后才到达 ⏸ Paused · 1/3 agents。底部提示与状态严格对应：running 时是 p pause，pausing 时完全不出现 p，paused 后变成 p resume —— 不会让用户请求一个当前状态不接受的转换。
2 · 暂停中的 run 留在 Active，并可从 slash command 恢复
/workflows 把该 run 列在 Active 分组、状态 paused（没有被丢进终态分组）；/workflows p <runId>（进程内控制路径，而非对话框按键）成功恢复。
provider 侧时间线是关键证据：暂停期间 77.2 秒内 B、C 的请求数为 0；恢复后 B、C 按脚本顺序依次派发；随后恰好一条 <kind>workflow</kind> 完成通知，携带 {"out":["marker-A-done","marker-B-done","marker-C-done"]} —— 位置对齐的数组在暂停后依然正确。该场景的第一次运行得到同样形状，窗口为 77.0 秒。
3 · 暂停 → 停止
停止暂停中的 run 后记录为 cancelled，B、C 从未到达 provider（全程只有 A 一次 agent 请求），没有任何完成通知，父会话随即正常回答了下一条消息。本报告涉及的所有场景，进程 stderr 均干净——没有来自被取消的 gate waiter 的 unhandledRejection。
5 · wall-clock watchdog（因 round 13 重写 sandbox 而重测）
- 在 25 秒预算下暂停了 52 秒 → run 没有在暂停期间被杀掉，恢复后正常完成。暂停时间确实没有计费。
- 对照组，同样 25 秒预算、不暂停 → 恰好在 25 秒中止：
Workflow execution exceeded 25000 ms of active time (paused time is not counted).backstop 是被挂起，不是被移除。
上次之后的新增内容 —— round 11–13
R12 · 暂停中的 run 不再阻塞 /clear
这是本轮对操作者可见影响最大的行为变化，所以我做了 A/B。
- running →
/clear仍被拒绝：Stop the current session's running background tasks before starting a new session. - paused →
/clear放行。 - 暂停中的 run 确实是被中止而非泄漏：之后 20 秒安静期内零 provider 请求、零完成通知，磁盘快照记录
status: "cancelled"。abortAll()先于reset()的顺序确实达成了注释所述效果。
我认同这个取舍——暂停中的 run watchdog 被挂起，若它还能阻塞切换，就没有任何 backstop 能把它释放。但这也意味着一次 session 切换会静默取消暂停中的 run，而 PR 描述的 Risk & Scope 一节并未提及。建议在那里补一行。
R12 · Pausing 期间再按 p 会给出反馈，而不是看起来卡住
在 run 仍处于 pausing 时再按一次 p，会亮起 Pause/resume was rejected; the workflow state changed. Try again.，约 3 秒后自动消失。由于 pausing 可能持续一整个子代理调度，这比之前的静默无响应是实质改进。
R11 / R13 · 对注册表已不再持有的 run 执行 /workflows p
session 切换清空注册表后，对一个仍能从快照列出的 run 执行 /workflows p <runId>，返回 Workflow <id> is cancelled and cannot be paused or resumed. —— 而不是过去那句自相矛盾的「Unknown live workflow runId」。两个来源都查不到的 id 仍然返回 Unknown live workflow runId。completed 的 run 得到同样的终态文案。两侧行为都与描述一致。
该 head 上的测试与卫生检查
packages/core定向用例：438 通过 / 7 个文件（workflow-dispatch-scheduler、workflow-orchestrator、workflow-runner、workflow-sandbox、workflow-run-registry、workflow-snapshot、tools/workflow）—— 上次是 402。packages/cli定向用例：188 通过 / 6 个文件（workflowsCommand、BackgroundTasksDialog、BackgroundTasksPill、useBackgroundTaskView、backgroundWorkUtils、useBranchCommand）。npm run build、npm run bundle、npm run typecheck（0 个 TS 错误）、npm run lint全部通过。（lint 报了 26 条问题，全部位于我自己未纳入版本控制的harness/目录，PR 自身文件零问题。）- 被删除的
utils/concurrencyLimiter.ts在packages/下仍无任何引用。
三条不阻塞的说明
A. round 11 为 Promise.all 声称的风险无法复现——该修复改善的是日志分类，而非崩溃安全性
round 11（R10-9）为包装 Promise.all / race / any 给出的理由是：不包装的话，持有失败 dispatch 的即发即弃聚合会「触发进程级 unhandledRejection（交互式下的 CRITICAL 横幅，或 headless 宿主中 Node 默认 --unhandled-rejections=throw 导致的进程终止）」。我对这个说法做了 A/B：只禁用那三个静态方法的包装，重新打包，再跑两类即发即弃形状（裸的拒绝聚合、由聚合派生的 .then()、以及嵌套聚合），用每次运行的 agent 上限来制造确定性的 dispatch 拒绝。
两个构建都没有崩溃。 没有 CRITICAL 横幅，stderr 没有 unhandledRejection，进程都存活。真正变化的是镜像日志行：
| 脚本 | 有包装（本 PR） | 禁用包装 |
|---|---|---|
即发即弃 Promise.all + 对被拒绝 dispatch 调用 .finally() |
result not consumed + rejection not handled |
rejection not handled ×2 |
由拒绝聚合派生的 .then() + 嵌套聚合 |
rejection not handled + result not consumed |
rejection not handled ×2 |
（两种行的完整前缀都是 dispatch failed (…): Workflow exceeded the maximum of N agent() calls per run.；我不声称哪一行对应哪种形状，只陈述各构建产生的分类组合。）
两个构建产生的行数相同。真正的兜底是宿主侧的 adoption-escape hook —— R11-16 的注释本身也提到了它；聚合包装细化的是分类。这本身是合理的改进，我不建议回退。但 round 11 总结夸大了它所消除的风险，任何人都不应把「否则会终止进程」当作已验证的结论。
B. 被终止的暂停 run，其持久化 agent 计数取决于以何种方式终止
同一个脚本、同样的结果——只有 agent A 到达过 provider——但：
| 暂停 run 的终止方式 | 实时 /workflows |
磁盘快照 | 重启后 /workflows |
|---|---|---|---|
Background Tasks 中按 x |
3/3 agents |
dispatched=3 completed=3 |
3/3 agents |
暂停期间 /clear |
— | dispatched=3 completed=1 |
1/3 agents |
/clear 路径先 abortAll() 再 reset()，于是正在收敛的 dispatch 调用 onAgentCompleted() 时已找不到 entry，计数停止；对话框路径保留 entry，收敛发生在快照写入之前。在「已结算」与「已执行」两种定义下两个数字都说得通，R11-10（把收敛从 cancelled 扩展到 completed/failed）与 R13（把快照投影冻结在第一个 await 之前）各自也都合理——只是方向相反。可见结果是：只有一个 agent 执行过的 run，会因终止方式不同而持久化为 1/3 或 3/3。watchdog 对照组还让我撞上了它的 failed 形态：一个只向 provider 发出过一次 dispatch、在 25 秒被中止的 run，显示为 failed · 3/3 agents。
这是我上轮那条「外观性」说明的具体第二副面孔。仍属外观问题、不阻塞——之所以提出，是因为它现在在 completed 和 failed 的 run 上也能出现，不再只限于 cancelled。
C. 「live foreground」下 /workflows p 的文案在 TUI 中几乎不可达
foreground workflow 会在其整个生命周期内占用输入框，因此期间键入的 /workflows p <runId> 会被排队，只有在该 run 已经落为终态之后才提交——而此时 round 13 新增的终态检查会先一步作答，返回终态文案。我在当前 head 确认了终态文案；本轮无法通过 TUI 触达 foreground 文案（我在 fd8a6ed77 上曾观测到它）。该分支仍是正确的防御性代码，且对话框层的 foreground 护栏在本 head 再次确认：foreground run 只提供 x stop、没有 p pause，按 p 无任何效果也不出现闪烁。这里只是说明：它面向用户的那句文案基本停留在理论层面。
本轮未覆盖
真实 provider 认证与网络延迟、跨进程持久恢复、journal durability、逐 agent 控制、macOS 与 Windows。被门挡住的 agent 请求如果暂停达到分钟级会触发 provider 客户端的流式超时；本轮所有暂停窗口都远低于该阈值，因此时间线中没有重试噪声。
|
@qwen-code /triage |
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Round 14 — response to the maintainer re-verification at
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Not approving — escalating to a maintainer. Reviewed at 444b52bb49. This is a review comment, not a block: I found no Critical regression, but this PR is outside what I can sign off on under this repo's own gate, and the reason is scale, not a specific defect.
Why I am not casting an approval
AGENTS.md puts core modules (packages/core/src/**, cross-package changes) behind a two-tier gate for non-maintainer PRs. Small-scope changes may be gated, but only at 100% confidence — any doubt escalates. This PR is +5087/−525 across 42 files, roughly 1200 lines of production logic in packages/core (excluding tests), spanning workflow-sandbox, workflow-orchestrator, workflow-runner, workflow-dispatch-scheduler, workflow-run-registry, workflow-snapshot, plus CLI UI and web-shell. It is feat-type, so not hard-blocked on size — but it crosses the 1000-line advisory threshold and is on review round 14 with 9 Critical and 91 Suggestion threads still open. I sampled and verified the 9 Criticals and a handful of Suggestions; I did not audit 5000 lines line-by-line, so I cannot honestly claim the 100% bar. Handing this to the maintainer is the correct outcome, and per AGENTS.md a large core feat escalates for awareness regardless.
What I did verify — 8 of the 9 Criticals are genuinely fixed
None of the 9 has an author reply, so I checked each against the code at this head rather than the thread flags:
| Thread | Verdict at 444b52bb49 |
|---|---|
workflow-runner.ts:209 (R11-1) — settlement guard only special-cased 'cancelled', so an externally-failed entry could settle ok: true |
fixed — now isTerminalWorkflowStatus(entry.status) → ok: false with the entry's message, exactly the suggested widening |
workflow-sandbox.ts:627 (R11-2) — pause-banking mixed Date.now() with a monotonic setTimeout deadline |
fixed — both armedAt and the banking subtraction use performance.now(), with a comment naming the divergence |
workflow-orchestrator.ts:1352 — pause did not suspend the wall-clock watchdog, so a paused run died at the cap and could never resume |
fixed — WallClockWatchdog now clears the timer and banks the remainder on pause, re-arms on resume |
workflow-sandbox.ts:1256 — cancelling a paused run whose script hangs in ungated code orphaned it permanently |
addressed — an abort listener re-arms the suspended watchdog, so the Promise.race settles via the banked remainder. Now bounded rather than permanent; see the note below |
workflow-sandbox.ts:914 (R8-7) — derived .then() chains got no rejection observer |
fixed — probe: 0 process-level escapes, failure mirrored as dispatch failed (rejection not handled) |
workflow-sandbox.ts:978 (R10) — teardown suppression keyed only on AbortError, but in-flight cancel rejects with a plain Error |
fixed — suppression is now `readFlag(err,'__wfAbort') |
workflow-orchestrator.ts:1607 / :1619 — pause-gate success/cached/error arms turned a cancelled run into an unobserved rejection |
fixed — probe: the teardown AbortError shape now yields 0 escapes and logs nothing, which is the intended contract |
The 9th (R11-3) partially stands — but it is not a regression
await / Promise.resolve adoption of an ObservedPromise still lets the rejection reach Node as a process-level unhandledRejection. Probed at this head with a failing dispatch:
| Script shape | this PR | main (merge-base 650e085f) |
|---|---|---|
async function step(){ await agent('x'); } step(); |
unhandled=1, logged | unhandled=1, no log |
[1,2].map(async i => { await agent('x'+i); }) |
unhandled=2, logged ×2 | unhandled=2, no log |
Promise.resolve(agent('x')) |
unhandled=1, logged | unhandled=1, no log |
bare agent('x') (control) |
unhandled=0, logged | unhandled=1, no log |
agent('x').then(v => …) |
unhandled=0, logged | unhandled=1, no log |
plain-Error CANCELLED |
unhandled=0 | unhandled=1, no log |
teardown AbortError |
unhandled=0, silent | unhandled=1, no log |
The A/B is what matters here: main escapes on all seven shapes and logs nothing on any of them. This PR takes four shapes to zero escapes and, critically, gives every shape a run-log entry. The three await-adoption shapes still escape — so R11-3's "the rejection reaches Node" half is real — but its "leaves no log, alarm, or telemetry" half is fixed, and the escape itself is pre-existing behaviour that this PR narrows rather than introduces. Treating it as a blocker on this PR would be wrong; it belongs in a follow-up. (These shapes are also a genuine forgotten-await bug in the user's workflow script, which makes the product-level "CRITICAL … file a bug report" banner the more objectionable part than the rejection itself.)
Other evidence
- Tests: all 7 core workflow suites pass at this head —
workflow-sandbox148,workflow-orchestrator133,workflow-run-registry76,workflow.test39,workflow-runner15,workflow-snapshot14,workflow-dispatch-scheduler13 = 438 passed, 0 failed. CI's ubuntu leg is green on444b52bb49. - i18n: the 9 new keys are present in all 9 locale files.
concurrencyLimiter.tsand its test are deleted (−111/−117), replaced byworkflow-dispatch-scheduler.ts(+159) — a real consumer swap, not dead code left behind.
Suggestion for how to land this
The blocker is process, not correctness: 91 open Suggestions at round 14 is well past the ~5-round guidance in AGENTS.md, which says to land only Critical fixes at this point and defer the rest. I would ask the maintainer to (a) take the sign-off on the core surface, and (b) triage the 91 Suggestions into "must-fix" versus a follow-up issue, so the diff stops widening. Every further round has been adding production lines to a change that already crossed the advisory threshold.
中文说明
不予批准 —— 上交维护者决策。 审查提交 444b52bb49。这是一条评审意见而非阻断：我没有发现 Critical 回归，但本 PR 超出了我在本仓库自身门禁下可以签署的范围，原因是规模而非某个具体缺陷。
为何不投批准票：AGENTS.md 对非维护者的 core 改动设有两级门禁，小范围改动「必须 100% 确信，任何疑虑即上交」。本 PR 为 +5087/−525、42 文件，packages/core 中约 1200 行生产逻辑（不含测试），横跨 sandbox/orchestrator/runner/dispatch-scheduler/run-registry/snapshot 以及 CLI UI 与 web-shell；类型为 feat 故不因体积硬阻断，但已越过 1000 行提示线，且处于第 14 轮评审、仍有 9 个 Critical 与 91 个 Suggestion 未解决。我抽样核验了 9 个 Critical 与部分 Suggestion，但未逐行审计 5000 行，因此无法诚实地宣称达到 100% 标准。按 AGENTS.md，大型 core feat 本身也应上交维护者。
已核验：9 个 Critical 中 8 个确已修复（全部无作者回复，故按代码而非线程标记判定）：R11-1 结算守卫已改为 isTerminalWorkflowStatus；R11-2 已全部改用 performance.now()；暂停未挂起 wall-clock 看门狗已修（暂停清零并寄存余量、恢复时重新装载）；「取消已暂停且脚本悬挂的运行会永久孤立」已改为在 abort 时重新装载看门狗，从永久变为有界；R8-7 派生 .then 链已修（探针 0 次逃逸并写入日志）；R10 已加入 isRunAborted() 析取覆盖纯 Error 取消路径；暂停闩锁的成功/缓存/错误三臂已修（teardown AbortError 探针 0 逃逸且不写日志）。
第 9 个（R11-3）部分成立，但不是回归：await / Promise.resolve 采纳 ObservedPromise 时，拒绝仍会以进程级 unhandledRejection 逃逸。关键是 A/B 对照：main 上全部 7 种形态都逃逸且都不写日志；本 PR 将其中 4 种降为 0 逃逸，并让全部 7 种都写入运行日志。因此「无日志、无告警、无遥测」这一半已修复，「拒绝到达 Node」这一半是本 PR 收窄而非引入的既有行为——把它当作本 PR 的阻断项并不恰当，应转为后续跟进。
其他证据：本 head 上 7 个 core workflow 套件 438 全部通过；CI ubuntu 绿；9 个新 i18n key 在 9 个语言包中齐全；concurrencyLimiter 及其测试被 workflow-dispatch-scheduler 真实替换，未留死代码。
落地建议：瓶颈在流程而非正确性。第 14 轮仍有 91 条未解决 Suggestion，已远超 AGENTS.md 的约 5 轮指引（此后只应合入 Critical 修复）。建议由维护者（a）承接 core 面的签署，（b）把 91 条 Suggestion 分为「必修」与「后续 issue」，以止住 diff 继续扩张——每一轮都在给一个已越过提示线的改动继续添加生产代码。
|
Review: the cooperative pause/resume state machine is well-constructed and well-tested — scheduler states running->pausing->paused->running with strict one-step guards, registry mirrors transitions with explicit validation (running->paused skip rejected, terminal entries ignore late events), pause() gated to background+running, resume() gated to paused, both refuse after abort. Pause never interrupts mid-tool-call (dispatch-boundary pause; in-flight dispatches run to completion under 'pausing', queued ones never start, completed results held behind waitUntilRunning gates until resume), and resume pumps the untouched queue without re-running completed work (journal started-ids assigned before dequeue, results appended before the gate opens). All race pairs are test-pinned (double-pause, resume-before-paused, pause-then-natural-finish, cancel-during-pause), all 9 locales carry the 5 new keys, docs match. No P0/P1. Holding approval for two P2s worth addressing before merge: P2 — paused runs are silently cancelled by /clear, /branch, and session switch. paused is deliberately excluded from hasRunningEntries() (so it doesn't block switching), and session switch calls abortAll() before reset() — so a user who pauses a long workflow then clears/switches sessions loses the run, with only a cancelled snapshot as trace and nothing in the normal /clear confirm mentioning it. Suggest naming paused runs in the blocking-gate confirmation, or adding a line to the dialog's Paused explainer ('session switch cancels paused runs'). P2 — pause parked on a pending tool approval can burn the budget and die. The watchdog suspends only on 'paused'. If pause lands while an in-flight dispatch is parked waiting on tool approval, state stays 'pausing', the wall-clock budget keeps burning (default 30 min), and resume() returns false from 'pausing' — the run dies with a timeout while the UI shows 'Pausing', and the user can't rescue it except by cancelling. Acknowledged in source comments, but it's a real trap: consider suspending on 'pausing' when the entry has pendingApprovals (or surfacing remaining budget), and at minimum note it in the Pausing explainer, not just code comments. P3s: 'p' on a 'pausing' row routes to pause() which always refuses and flashes 'state changed — Try again', which can never succeed until paused (distinct wording suggested); the ~10 new dynamic p-branch messages are raw English not t()-wrapped (matches the file's existing dynamic-runId pattern, but the 5 new static keys ARE localized — optionally wrap static portions); the PR bundles three features (pause/resume, a watchdog wall-clock->active-time rewrite with a user-visible timeout-message change, and an unconsumed-rejection mirror + nested-log merge) — splitting would ease review/revert and the active-time semantics change deserves a release note; the 3s rejection flash can overwrite the two-step 'x again to confirm stop' hint if both fire on one row. |
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Round 15 summary — PR #8320Addressed the two P2 findings from @yiliang114's review (approval held on @yiliang114 — P2: paused runs are silently cancelled by /clear, /branch, and session switch — FIXEDAdded one sentence to the dialog's Paused explainer, per the reviewer's @yiliang114 — P2: pause parked on a pending tool approval burns the budget and dies — FIXED (reviewer's "at minimum")The Pausing explainer now surfaces the trap at the moment the user sees
@yiliang114 — P3 items — DEFERRED (recorded, not dropped)Past the ~5-round guidance in
@doudouOUC — COMMENTED review (escalation to maintainer) — NO CODE CHANGEThe review verified 8 of 9 Criticals fixed at the previous head and explicitly Files changed
No conflict ( VerificationCommands actually run on the final tree (commit
中文说明第 15 轮总结 — PR #8320已处理 @yiliang114 评审中的两个 P2 问题（其批准以这两项为前提）。@doudouOUC 的评审属于流程性上交，未要求代码改动。提交： @yiliang114 — P2：已暂停（paused）的运行会被 /clear、/branch 和会话切换静默取消 — 已修复按评审者给出的方案 (b)，在对话框的 Paused 说明文案中新增一句："/clear、/branch 以及切换会话会取消已暂停的运行。"该表述准确无误： @yiliang114 — P2：暂停时卡在待处理工具审批上会耗尽时长预算并导致运行死亡 — 已修复（采用评审者的"至少"方案）Pausing 说明文案现在会在用户看到 Pausing 状态时直接揭示该陷阱："等待工具审批的 agent 调用会让运行保持在此状态，且在审批得到响应前仍会计入活跃时间上限。"以下两个更强的方案经评估后拒绝，附证据：
@yiliang114 — P3 各项 — 延后处理（记录在案，不会丢弃）已超过
@doudouOUC — COMMENTED 评审（上交维护者） — 无代码改动该评审核验了上一个 head 上 9 个 Critical 中的 8 个已修复，并明确建议第 9 个（R11-3， 变更文件
无冲突（ 验证在最终代码树（提交
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving per maintainer — the P2s below are accepted as non-blocking follow-ups. The cooperative pause/resume core is solid: running->pausing->paused->running with strict one-step guards, registry transition validation, pause gated to background+running / resume gated to paused / both refuse after abort, pause never interrupts mid-tool-call (in-flight dispatches finish under 'pausing', queued never start, completed results held behind gates until resume), and resume doesn't re-run completed work. Race pairs all test-pinned, all 9 locales carry the new keys. Follow-ups (non-blocking): (1) paused runs are silently cancelled by /clear//branch/session switch — worth naming them in the blocking-gate confirm or the Paused explainer; (2) pause parked on a pending tool approval stays 'pausing', budget burns, resume() refuses — consider suspending on 'pausing' when there are pendingApprovals or noting it in the Pausing explainer. Minor: distinct flash wording for 'pausing' (not 'Try again'), optionally t()-wrap the dynamic messages, and release-note the watchdog active-time semantics change.
Resolves the conflict in packages/core/src/tools/workflow/workflow.ts. main's QwenLM#8320 edited the inline constructor description this branch is replacing, so the two sides touched the same argument: - Kept this branch's `WORKFLOW_TOOL_DESCRIPTION` constant — extracting that description is the whole point of the PR. - Ported QwenLM#8320's fact into the constant. The extracted text still listed the `/workflows` dialog controls as "live phase tree, token usage, cancel"; taking our side verbatim would have dropped cooperative pause/resume from the description the model actually reads. - Pinned that capability list in the existing description test. Nothing else asserted it, so the same silent drop could recur on the next base merge. Verified: npm run build, npm run typecheck, eslint on both changed files, and packages/core src/tools/workflow/workflow.test.ts (40 passed).
5 participants
What this PR does
This PR adds whole-run cooperative pause and resume to Dynamic Workflows. A pause-aware per-run scheduler stops dequeuing new agent dispatches, lets already in-flight work converge, and holds fulfilled or rejected results at a gate until the run resumes. Cancellation rejects queued work and wakes gated waiters while preserving exactly-once dispatch settlement.
The workflow lifecycle now distinguishes
running,pausing, andpaused. Background Tasks exposespto pause or resume and keeps all active states stoppable, while/workflows p <runId>provides the same process-local control in the interactive TUI. Text, pill, sorting, snapshot, shutdown, and session-switch consumers now treat all three states consistently.Why it's needed
Opt-in background workflows can continue after the parent turn, but before this change operators could only observe or stop an entire run. Cooperative pause provides a truthful control boundary: no new agents start, queued work remains ordered, in-flight agents may finish, and the workflow script does not consume their results until resume.
This is the next stage of the Dynamic Workflows roadmap. It does not add durable cross-process resume, change the journal format, freeze arbitrary JavaScript or external promises, or introduce per-agent controls.
Reviewer Test Plan
How to verify
PausingtoPaused, with the second and third agents still queued./workflows. Expect the paused run to remain in the Active bucket. Run/workflows p <runId>and expect the queued agents to continue in FIFO order and the workflow to emit exactly one completion notification.Evidence (Before & After)
Before: a background workflow could be observed or stopped, but there was no pause state, pause gate, or resume control.
After: real terminal testing against the production bundle observed
Running → Pausing → Paused → Running, kept queued dispatches out of the provider while paused, resumed them inA, B, Corder through/workflows p <runId>, emitted one completion notification after resume, and emitted none after stopping a paused run.Local verification: 251 focused core tests and 128 focused CLI tests passed; full lint, build, typecheck, and bundle passed; the real terminal pause/resume and pause/cancel scenarios passed.
Tested on
Environment (optional)
macOS with Node.js v24.14.1, the production
dist/cli.jsbundle, node-pty, headless xterm, Ink, and the repository fake OpenAI server.Risk & Scope
Paused.Linked Issues
Part of #8105
中文说明
本 PR 做了什么
本 PR 为 Dynamic Workflows 增加整次运行级别的协作式暂停与恢复。每次运行拥有一个可感知暂停状态的调度器：暂停时停止取出新的 Agent 调度，允许已经在飞的任务自然收敛，并把成功或失败结果阻塞在结果门之后，直到运行恢复。取消会拒绝队列中的工作、唤醒等待结果门的调用，同时保持每次调度只结算一次。
Workflow 生命周期现在明确区分
running、pausing和paused。Background Tasks 使用p暂停或恢复，并允许停止所有 active 状态；交互式 TUI 中的/workflows p <runId>提供同等的进程内控制。文本输出、footer pill、排序、snapshot、shutdown 和 session switch 等消费者也统一处理这三个状态。为什么需要
Opt-in 后台 Workflow 可以在父 turn 返回后继续执行，但本次改动前，操作者只能观察或停止整次运行。协作式暂停提供了与事实一致的控制边界：不再启动新 Agent、队列顺序保持稳定、在飞 Agent 可以完成，而 Workflow 脚本在恢复前不会消费其结果。
这是 Dynamic Workflows 路线图的下一阶段。本 PR 不增加跨进程持久恢复，不改变 journal 格式，不冻结任意 JavaScript 或外部 Promise，也不引入逐 Agent 控制。
Reviewer 测试计划
如何验证
Pausing进入Paused，第二、第三个 Agent 仍保留在队列中。/workflows。预期暂停中的运行仍位于 Active 分组。执行/workflows p <runId>，预期队列中的 Agent 按 FIFO 顺序继续，并且 Workflow 只产生一次完成通知。证据（Before & After）
Before：后台 Workflow 可以被观察或停止，但没有暂停状态、暂停 gate 或恢复控制。
After：基于生产 bundle 的真实终端测试观察到
Running → Pausing → Paused → Running；暂停期间队列任务没有进入 provider；通过/workflows p <runId>恢复后，调度顺序为A, B, C；恢复完成后只产生一次通知，停止暂停中的运行则不产生完成通知。本地验证：251 个 core focused tests 与 128 个 CLI focused tests 通过；全量 lint、build、typecheck 和 bundle 通过；真实终端 pause/resume 与 pause/cancel 场景均通过。
测试系统
环境（可选）
macOS、Node.js v24.14.1、生产
dist/cli.jsbundle、node-pty、headless xterm、Ink，以及仓库 fake OpenAI server。风险与范围
Paused前完成。关联 Issue
Part of #8105