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。 |
|
Re-run at the new head — round-13 review fixes have landed since the last full pass (2026-08-01), and a maintainer approval arrived at this exact head. Re-checking the gate from the top. Template ✓ — all required sections present, bilingual body, and a concrete reviewer test plan with before/after expectations. Problem: this is roadmap feature work, not a bugfix — the Dynamic Workflows staged roadmap (#8105) carries the repo's Direction: aligned — a direct next stage of a roadmap the maintainers are actively shipping. No direction concerns. Size: core paths are touched ( Approach: the scope feels right. Every piece ties to the stated goal — the pause-aware scheduler replaces the concurrency limiter (whose only consumer was the orchestrator), the result gate hangs off the existing settlement paths, and the UI changes are the same lifecycle surfaced on existing controls. No unrelated changes or drive-by refactors spotted. Risk: no high-risk-path signal from the revert-history check. Moving on to code review. 🔍 中文说明在新 head 上重跑——自上次完整审查(2026-08-01)以来第 13 轮 review 修复已合入,且 maintainer 已在当前 head 上批准。从头重新过 gate。 模板 ✓ —— 必填章节齐全,正文双语,reviewer 测试计划包含具体的 before/after 预期。 **问题:**这是路线图功能开发,不是 bugfix —— Dynamic Workflows 分阶段路线图(#8105)带有仓库的 **方向:**对齐 —— 是维护者正在推进的路线图的直接下一阶段。无方向性顾虑。 **规模:**触及核心路径( **方案:**范围合理。每一部分都服务于既定目标——可感知暂停的调度器替换并发限制器(其唯一消费者是 orchestrator),结果门挂在既有结算路径上,UI 改动是同一生命周期在既有控件上的呈现。未发现无关改动或顺手重构。 **风险:**revert 历史高风险路径检查无命中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed the full diff at My independent proposal for cooperative pause would be: a pause-aware state machine at the dispatch layer (where concurrency is already enforced), a result gate so in-flight results aren't consumed by the script while paused, lifecycle widening in the registry, and consistent surfacing on the existing controls. This PR matches that shape almost exactly:
No critical blockers at this head. One honest reservation: the unconsumed-rejection mirror machinery in The standing The pause/resume control flow, since it spans five components: sequenceDiagram
participant P1 as User
participant P2 as Dialog and slash command
participant P3 as WorkflowRunRegistry
participant P4 as Run handle
participant P5 as Dispatch scheduler
participant P6 as Result gate
P1->>P2: press p or run p subcommand
P2->>P3: pause or resume the runId
P3->>P4: delegate when the state allows
P4->>P5: pause or resume
P5->>P5: running to pausing, paused when drained
P5->>P6: hold results while not running
P6-->>P5: wake waiters on resume, reject on abort
Files changed (20 of 42 shown)
Testing evidenceUnattended run — no local tmux drive on the CI path; the evidence below is the PR's own CI, fetched via the API for the reviewed commit.
The three skipped jobs are skipped by repository design, for every PR: macOS/Windows test jobs and the CLI integration suite are gated on The author reports 251 focused core tests and 128 CLI tests passing locally plus real-terminal pause/resume runs — that is the author's claim, not independently re-run here; the CI suite above is the evidence this review carries. Sandboxed verification would settle the remaining gap: 中文说明代码审查在 我对"协作式暂停"的独立方案是:在派发层(并发控制已有之处)做可感知暂停的状态机、加一道结果门使在飞结果在暂停期间不被脚本消费、注册表生命周期扩展、既有控件统一呈现。本 PR 与该形状几乎完全一致:
当前 head 无关键阻断项。 一个诚实的保留意见: 本 PR 上现存的 测试证据无人值守运行——CI 路径不做本地 tmux 实测;以上证据为该 PR 自身 CI,经 API 在受审 commit 上取得。 三个 skipped 作业是仓库设计使然、对所有 PR 一致:macOS/Windows 测试与 CLI 集成套件在 作者自述本地通过 251 个 core focused tests、128 个 CLI tests 以及真实终端 pause/resume 场景——这是作者的陈述,本审查未独立复跑;本审查携带的证据是上述 CI 套件。 沙箱验证可以补齐剩余缺口: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review at this head, including the round-13 fixes, and @wenshao has approved at exactly this commit; the 3 is the Stage-0 policy cap (a Stepping back: the implementation matches my independent proposal almost line for line — pause-aware scheduler at the dispatch layer, a result gate chained onto every settlement path, lifecycle widening in the registry, and the same lifecycle surfaced consistently across the dialog, the pill, sorting, snapshots, shutdown, and session switch. Nothing in the diff reads like it is trying too hard; the scheduler is 159 lines and does exactly one thing, and the old limiter was removed rather than left parallel. The round-13 delta is the kind of fix round that raises trust rather than patience — every change names the finding it answers, and each lands with a test that would go red if the fix regressed. Two reservations, both stated plainly above and neither a blocker: the sandbox's unconsumed-rejection mirror machinery is genuinely intricate (thirteen rounds and an A/B verification have been at it, and I still would not claim to have traced every edge), and macOS/Windows plus integration coverage only lands in the merge queue by repo design — true for every PR, but worth naming so nobody reads the green table as full-platform evidence. On the verdict: all three stages came out clean at this head, and the standing 中文说明置信:3/5 —— 当前 head(含第 13 轮修复)审查干净,且 @wenshao 已在同一 commit 上批准;3 分是 Stage-0 政策上限(触及核心路径且生产逻辑 ≥500 行的 退一步看:实现与我的独立方案几乎逐条吻合——派发层的可感知暂停调度器、挂在每条结算路径上的结果门、注册表生命周期扩展,以及对话框、pill、排序、snapshot、shutdown、session switch 各面统一呈现同一生命周期。diff 中没有过度设计的痕迹:调度器 159 行、只做一件事,旧 limiter 被移除而非并行保留。第 13 轮修复是提升信任而非消耗耐心的那种——每处改动都点名其回应的问题,且各自带着"修复回归即变红"的测试。 两点保留意见,上文已明说,均非阻断:sandbox 的未消费 rejection 镜像机制确实复杂(十三轮 review 加一次 A/B 验证反复打磨,我仍不敢声称穷尽每条边界);macOS/Windows 与集成覆盖按仓库设计只在合并队列落地——对每个 PR 都如此,但值得点名,避免把为绿的表格误读为全平台证据。 关于结论:三个阶段在当前 head 均为干净,现存的 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. 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-preview via Qwen Code /review
|
已修复:前台 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
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。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 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-preview via Qwen Code /review (v0.21.3)
| const idx = windowStart + visibleIdx; | ||
| const isSelected = idx === selectedIndex; | ||
| const terminal = terminalStatusPresentation(entry.status); | ||
| const terminal = statusPresentation(entry.status); |
There was a problem hiding this comment.
[Suggestion] The local variable is still named terminal after terminalStatusPresentation was renamed to statusPresentation. That function now also returns a presentation for the new active states (pausing/paused), so const terminal = statusPresentation(entry.status) is a misnomer — the value is no longer terminal-only. Concrete cost: a future contributor adding a presentation for another active state reads const terminal and concludes the result is only consumed for terminal states, potentially adding a redundant isTerminal guard that silently drops the new styling. Rename the local (e.g. presentation) at all six call sites (lines 403, 523, 696, 1001, 1082, 1173).
| const terminal = statusPresentation(entry.status); | |
| const presentation = statusPresentation(entry.status); |
中文说明
函数 terminalStatusPresentation 已重命名为 statusPresentation(现在也会为新的 active 状态 pausing/paused 返回值),但六个调用处仍把结果绑定到名为 terminal 的局部变量。这个名字现在具有误导性——该值不再只针对 terminal 状态。具体代价:未来若为其它 active 状态新增 presentation,贡献者读到 const terminal 可能误以为该结果只用于 terminal 状态,进而加上多余的 isTerminal 判断,悄悄丢掉新状态的样式。建议在全部六个调用处(第 403、523、696、1001、1082、1173 行)将局部变量改名为 presentation。
— qwen3.8-max-preview via Qwen Code /review
| if (target.status === 'running') { | ||
| return registry.pause(runId) |
There was a problem hiding this comment.
[Suggestion] The registry.pause() / registry.resume() false-return branches (the "…could not be paused/resumed because its state changed" errors) have no test coverage. Every test in workflowsCommand.test.ts stubs pauseMock / resumeMock with .mockReturnValue(true), so the false arm is never exercised. Concrete cost: a regression that makes registry.pause return false for a valid running backgrounded run — or a mutation flipping this ternary — would surface the wrong or missing message, and no test goes red. Add cases with pauseMock.mockReturnValue(false) / resumeMock.mockReturnValue(false) asserting the messageType: 'error' "state changed" content for a running and a paused target respectively.
中文说明
registry.pause() / registry.resume() 返回 false 的分支(即 "…could not be paused/resumed because its state changed" 错误)没有任何测试覆盖。workflowsCommand.test.ts 中所有测试都用 .mockReturnValue(true) 打桩 pauseMock / resumeMock,因此 false 分支从未被执行。具体代价:如果某个回归使 registry.pause 对一个合法的 running 后台 run 返回 false,或有人误翻转了这个三元表达式,将输出错误或缺失的消息,而没有任何测试会变红。建议新增 pauseMock.mockReturnValue(false) / resumeMock.mockReturnValue(false) 的用例,分别对 running 和 paused 目标断言 messageType: 'error' 的 "state changed" 文案。
— qwen3.8-max-preview via Qwen Code /review
| if ( | ||
| !entry || | ||
| (!isActiveWorkflowStatus(entry.status) && entry.status !== 'cancelled') || | ||
| entry.agentsCompleted >= entry.agentsDispatched |
There was a problem hiding this comment.
[Suggestion] The new over-count cap entry.agentsCompleted >= entry.agentsDispatched is mutation-vacuous: no test drives onAgentCompleted past the dispatched count, so deleting this cap keeps the whole suite green. The only test touching this path lands exactly at agentsCompleted: 2, agentsDispatched: 2, where the cap never fires. Concrete cost: this cap is the registry-level backstop for the orchestrator's exactly-once emitCompletion latch; if that latch ever regressed (double-emitting on the abort/error arm), the pill/dialog completion count could exceed dispatched and nothing would catch it. Add a registry unit test that calls onAgentDispatched once then onAgentCompleted twice (in both an active and a cancelled state) and asserts agentsCompleted stays at 1. The cap itself is correct and defensive — this finding is about the missing test, not the cap.
中文说明
新增的超额上限 entry.agentsCompleted >= entry.agentsDispatched 是「变异真空」的:没有任何测试把 onAgentCompleted 驱动到超过 dispatched 数量,因此删掉这个上限整个测试套件仍然全绿。唯一触及该路径的测试恰好停在 agentsCompleted: 2, agentsDispatched: 2,此时上限从不触发。具体代价:该上限是 orchestrator 的 exactly-once emitCompletion 锁存在 registry 层的兜底;一旦那个锁发生回归(在 abort/error 分支重复触发),pill/dialog 的完成计数可能超过 dispatched 而无人发现。建议新增一个 registry 单测:调用一次 onAgentDispatched 再调用两次 onAgentCompleted(分别在 active 状态和 cancelled 状态下),断言 agentsCompleted 保持为 1。上限本身正确且是防御性的——本条指的是缺失的测试,而非上限本身。
— qwen3.8-max-preview via Qwen Code /review
| : { | ||
| 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] registry.pause() returns false for three distinct reasons — a foreground run (!isBackgrounded), a status that is no longer running, or a missing handle — but this branch always reports "could not be paused because its state changed". Foreground runs are the default and ARE listed by /workflows, and the target.status === 'running' pre-check above does not filter them out, so /workflows p <foreground-runId> on a healthy running foreground run prints a misleading "state changed" message when the real reason is that foreground runs are intentionally not pausable. The pause is correctly refused — only the diagnostic is wrong. Concrete cost: a user is told a race occurred when nothing did, and may retry or doubt the pause. Pre-check target.isBackgrounded and return a distinct message (e.g. "Foreground workflow runs cannot be paused; only background runs support cooperative pause"), reserving "state changed" for a genuine status race. The resume arm below has the same shape.
中文说明
registry.pause() 返回 false 有三种不同原因——前台 run(!isBackgrounded)、状态已不是 running、或缺少 handle——但这个分支一律报告 "could not be paused because its state changed"。前台 run 是默认形态,且确实会被 /workflows 列出;上方的 target.status === 'running' 预检查并不会把它们过滤掉。因此对一个正常的 running 前台 run 执行 /workflows p <foreground-runId> 时,会输出一条误导性的 "state changed" 消息,而真正的原因是前台 run 本就不允许暂停。暂停确实被正确拒绝了——只是诊断信息错了。具体代价:用户被告知发生了竞态(其实并没有),可能重试或怀疑暂停是否生效。建议预先检查 target.isBackgrounded 并返回一条独立消息(例如 "Foreground workflow runs cannot be paused; only background runs support cooperative pause"),把 "state changed" 保留给真正的状态竞态。下方的 resume 分支同理。
— qwen3.8-max-preview via Qwen Code /review
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 均通过。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 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-preview via Qwen Code /review
| onDispatchStateChange(runId: string, state: WorkflowDispatchState): void { | ||
| const entry = this.entries.get(runId); | ||
| if (!entry || isTerminalWorkflowStatus(entry.status)) return; |
There was a problem hiding this comment.
[Suggestion] The terminal-state guard in onDispatchStateChange is the only thing that stops a late scheduler callback from resurrecting a settled run, yet no test covers it. — Failure scenario: cancel a background run while its scheduler is pausing with one dispatch in flight (inFlight=1). abortPending() rejects queued jobs and gate waiters but leaves the scheduler state at pausing; when the in-flight thunk then rejects on the abort signal, pump()'s finally sees state === 'pausing' && inFlight === 0 and calls setState('paused'), firing onDispatchStateChange('paused') against an entry that is now cancelled. This guard discards that callback. If it regressed, the cancelled run would flip back to paused: counted active in hasRunningEntries() (blocking /clear and session shutdown), escaping terminal eviction, and rendering as paused in the Background Tasks dialog. No existing test drives onDispatchStateChange after a terminal transition.
// Suggested registry unit test
const r = createRegistry();
r.register({ runId: 'wf_x', isBackgrounded: true, /* ... */ });
r.onDispatchStateChange('wf_x', 'pausing');
r.cancel('wf_x', Date.now());
r.onDispatchStateChange('wf_x', 'paused'); // late callback
r.onDispatchStateChange('wf_x', 'running'); // late callback
expect(r.get('wf_x')!.status).toBe('cancelled');中文说明
onDispatchStateChange 中的终态守卫是阻止调度器迟到回调把已结束的运行「复活」的唯一防线,但目前没有测试覆盖它。失败场景:在一个后台运行的调度器处于 pausing 且有一个在飞调度(inFlight=1)时取消该运行。abortPending() 会拒绝队列中的任务与结果门等待者,但不改变调度器状态(仍为 pausing);当在飞 thunk 随后因 abort 信号 reject 时,pump() 的 finally 看到 state === 'pausing' && inFlight === 0,调用 setState('paused'),对此时已是 cancelled 的 entry 触发 onDispatchStateChange('paused')。该守卫丢弃这个迟到回调。若发生回归,被取消的运行会被翻回 paused:在 hasRunningEntries() 中被计为活跃(阻塞 /clear 与会话关闭)、逃过终态驱逐,并在 Background Tasks 对话框中显示为已暂停。现有测试没有任何一个在终态转换之后再调用 onDispatchStateChange。建议补充上方代码块中的 registry 单测。
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
已修复。验证:cd packages/core && npx vitest run src/agents/workflow-run-registry.test.ts(63/63)与 git diff --check 均通过。
| import { toModelVisibleSubagentResult } from '../subagent-result.js'; | ||
| import { SUBAGENT_PLAN_LIFECYCLE_TOOLS } from './subagent-plan-tool-policy.js'; | ||
| import { runWithAgentContext } from './agent-context.js'; | ||
| import { WorkflowDispatchScheduler } from './workflow-dispatch-scheduler.js'; |
There was a problem hiding this comment.
[Suggestion] This import swap removes the only production consumer of createConcurrencyLimiter, leaving packages/core/src/utils/concurrencyLimiter.ts and its ~115-line test file as dead code. — Concrete cost: the module is not exported from the core index.ts and no other production file imports it, yet CI keeps running its test suite on every change, and future maintainers may preserve its API contract assuming it is still in use. WorkflowDispatchScheduler fully subsumes it (concurrency limiting and abort draining, plus the new pause/resume gate).
git rm packages/core/src/utils/concurrencyLimiter.ts packages/core/src/utils/concurrencyLimiter.test.ts中文说明
这次 import 替换移除了 createConcurrencyLimiter 唯一的生产消费者,使 packages/core/src/utils/concurrencyLimiter.ts 及其约 115 行测试文件成为死代码。具体代价:该模块没有从 core 的 index.ts 导出,也没有任何其他生产文件 import 它,但 CI 仍会在每次变更时运行它的测试套件,未来的维护者可能会以为它仍在使用而刻意维护其 API 契约。WorkflowDispatchScheduler 已完全覆盖它的能力(并发限制与 abort 排空,外加新的暂停/恢复门)。建议在本 PR 或后续 PR 中删除这两个文件(见上方命令)。
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. 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-preview via Qwen Code /review (v0.21.3)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 458 passed · 0 failed · 458 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:458 通过 · 0 失败 · 458 总计 Verification reportPR #8320 Deep Verification —
|
| cell | environment | observable oracle | result |
|---|---|---|---|
| HEAD (gate active) | real compiled scheduler, limit=1, deferred in-flight A, queued B/C | result held while paused; FIFO resume; exactly-once; abort drains queue | 32/32 pass |
| CONTROL (gate removed) | same scheduler, waitUntilRunning() short-circuited |
same 32 assertions | 31/32 — exactly one flip |
The single flip is the load-bearing property: S1 A result HELD at gate while paused goes PASS → FAIL (state=fulfilled value=A) — with the gate gone, the in-flight result is delivered immediately even though the run is paused. Every other assertion (queued B/C never start while paused, FIFO A,B,C resume, exactly-once settlement, abort draining a 50-job backlog in <50 ms, concurrency window) is unchanged across the two cells, proving those properties live in the scheduler's pump/abort logic while "hold results until resume" is contributed specifically by the waitUntilRunning() chaining — the orchestrator's load-bearing hunk. Witness: 01-ab-scheduler-head-vs-gateless.png.
Vacuity check (the gate hunk pins the central test)
Reverted only the live-dispatch success-path gate in workflow-orchestrator.ts ((result) => scheduler.waitUntilRunning().then(() => result) → (result) => result, interface-preserving) and ran the central test keeps a nested agent result behind the shared pause gate:
- Unmutated: green (
Tests 1 passed). - Mutated: red on the intended assertion —
AssertionError: expected true to be falseat line 836expect(settled).toBe(false)(the run settles while paused because the result is no longer held). Source restored afterward;git statusclean.
The revert breaks the behavior the test exists to catch (not the import/compile/fixture), so the test is non-vacuous and is pinned by exactly this hunk. Witness: 02-vacuity-gate-hunk.png.
End-to-end wire-oracle (orchestrator + scheduler + vm sandbox)
Drove the real WorkflowOrchestrator with a counting, controllable fake provider (the dispatch seam the orchestrator is designed to accept; real vm sandbox, real scheduler) through the Reviewer Test Plan scenario (concurrency 1, parallel([agent('A'),agent('B'),agent('C')])):
- E1 pause/resume: only
Areaches the provider initially; afterpause()andAsettling, state ispaused, the provider still saw onlyA(B/C held out), and the run is not settled (A's result held at the gate). Afterresume(), the provider is calledA,B,Cin FIFO order and the run settles exactly once with['A','B','C']. - E2 stop a paused run: with B/C queued and A held,
controller.abort()→ B/C never reach the provider and the run rejects (no success settlement).
Both pass (2/2). Witness: 04-e2e-pause-resume-cancel.png.
Harness-fidelity note (recorded so the next reader does not repeat it): my first E2 run showed the run resolving on abort. That was my harness omitting
abortOnTimeout, which leftparallel's internal signalundefinedsosettleToNullArray'sif (signal?.aborted) thrownever fired. Production threads the same controller throughabortOnTimeout(workflow-runner.tsline ~187); mirroring that, E2 rejects as designed. This is a fake-peer-semantics bug in my harness, not a defect in the PR.
Registry state-machine probe (secondary claim 1)
Drove the real compiled WorkflowRunRegistry (no mocks): 21/21 pass. Witness: 03-registry-state-machine.png.
- Transitions are strict:
running→pausedandpausing→runningare rejected; onlyrunning→pausing→paused→runningis admitted. - Cancel from
paused→cancelled, and the entry is then terminal: subsequentonDispatchStateChange,complete(), andfail()are all no-ops (this is what suppresses a success notification on stop — the contract behind Reviewer step 3). - Completion counter is exactly-once: 5
onAgentCompletedcalls against 2 dispatched capagentsCompletedat 2. pause()/resume()gating:pause()needs backgrounded +running+ attached handle;resume()needspaused+ handle (false whilerunningorpausing); foreground runs cannot be paused; apausedrun still counts as active for re-registration.
Targeted gates (affected workspaces)
| workspace | files | result |
|---|---|---|
| core | workflow-dispatch-scheduler, workflow-orchestrator, workflow-runner, workflow-run-registry, workflow-snapshot, tools/workflow/workflow |
6 files, 254/254 pass |
| cli | workflowsCommand, BackgroundTasksDialog, BackgroundTasksPill, useBackgroundTaskView |
4 files, 132/132 pass |
The gate is live: the vacuity check above is the positive control (the central test goes red when the gate hunk is reverted), so the green suite is shown to actually pin the behavior. The CLI p handler was read directly: it rejects non-interactive use, malformed usage, unknown runId, and foreground runs with clear messages, and returns a warning (not a silent no-op) when p is pressed during pausing.
Findings
No blocking findings. Two non-blocking observations, both judged to be intended behavior:
- (nit) Scheduler state stays
pausedafter abort.abortPending()rejects queued jobs and gate waiters but does not transitionstateout ofpaused. This is harmless: terminal status is tracked by the registry (proven to lock out further transitions in R2), and the per-run scheduler is discarded with the run. No assertion depends on a post-abort scheduler state. - (nit, already handled)
pduringpausing. Neitherpause()(needsrunning) norresume()(needspaused) acts while a run ispausing; the CLI surfaces this as an explicit warning ("still pausing; wait until it reaches paused"), so it is a deliberate cooperative-pause boundary with clear feedback, not a dead control.
Not covered
- Durable cross-process resume, journal durability, real provider auth/network, per-agent controls — explicitly out of scope per the PR description; not probed.
- Per-commit attribution — the CI checkout is depth 2 (merge commit + base tip + PR head only); the five commits in the metadata snapshot are not individually reachable, so claims were verified against the aggregate
HEAD^1..HEADdiff.git rev-parse --is-shallow-repository= true. - Repo-wide lint / typecheck / bundle — covered by the PR's own CI; not re-run (my A/B used the pre-built
distand vitest-on-source, neither of which required a rebuild). - Standalone
nodeexecution of the orchestrator harness — importing the orchestrator as an entry module trips a pre-existing circular-init TDZ betweenworktreeCleanup.tsandgitWorktreeService.ts. Proven environmental (A/A): both files are byte-identical on base and head and untouched by this PR. The same orchestrator loads cleanly under the vitest load order, which is how the e2e wire-oracle was run. - Windows / Linux real-terminal pause/resume — the PR marks these
⚠️ (untested by the author); this Linux container ran the unit/integration layers, not a TUI session.
Methodology
Environment: CI verify container (node:22-bookworm, Node v22.23.2), working tree at refs/pull/8320/merge, npm ci + npm run build pre-completed at HEAD. All harnesses are mock-free with respect to the unit under test — they import the real compiled dist modules (scheduler, registry) or drive the real orchestrator + vm sandbox through the dispatch seam (the only fake is the provider, the seam the orchestrator is designed to accept). The A/B control differs from HEAD by nothing else than the gate hunk (short-circuited waitUntilRunning() in the harness; reverted hunk in the source for the vacuity check). Raw per-cell logs live in logs/ (01-scheduler-head.txt, 02-scheduler-gateless.txt, 03-vacuity-green.txt, 04-vacuity-mutated.txt, 05-core-gate.txt, 06-cli-gate.txt, 07-registry-probe.txt, 08-e2e-orchestrator.txt); rerunnable harnesses are scheduler-harness.mjs, registry-harness.mjs, e2e-orchestrator.mjs, and e2e-orchestrator.harness.test.ts (the last must run under the vitest load order — see Not covered). Assertion tally: scheduler 32 + control differential 1 + registry 21 + e2e 16 + vacuity 2 + targeted gates 386 = 458, all met expectation; the single intermediate e2e red was a diagnosed harness-fidelity bug (corrected), so unexpected fail = 0.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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 |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 736 passed · 0 failed · 736 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:736 通过 · 0 失败 · 736 总计 Verification reportPR #8320 Deep Verification (follow-up round) —
|
| # | Prior finding | Severity | Status at this head | How re-measured |
|---|---|---|---|---|
| V | Verdict merge-ready, 649/649 |
— | stands (re-measured) | Central A/B, vacuity, registry, nit1, watchdog, e2e, and both gates re-run fresh → 736/736 (count differs by harness granularity; the load-bearing isolation is the same shape: control flips exactly the gate-dependent assertions). |
| 1 | Scheduler state stays in the pause band after abortPending() (never terminal) |
nit | stands (re-measured) | Fresh nit1-probe.mjs (11/11) on the real compiled scheduler: pause with in-flight → pausing; abort → no transition; late resolve lands in paused; pause/resume/run all refuse after abort. Harmless by construction: terminal status is the registry's job (re-confirmed in T12/T13: cancel locks cancelled, late complete/fail/onDispatchStateChange are no-ops). |
| 2 | p pressed during pausing returns an explicit warning |
nit (already handled) | stands (re-measured) | CLI gate 162/162 includes the parameterized ['pausing', 'still pausing', 'warning'] case (workflowsCommand.test.ts L278) and the foreground-rejection case; handler shape unchanged. |
No prior finding worsened; none was declined.
Scope selection
Central claim: cooperative pause holds fulfilled/rejected dispatch results at a gate (waitUntilRunning() chained onto all result paths — cache-hit, live-dispatch success, error arm, and, new in the R12 delta, entry-gate rejections via rejectThroughPauseGate) until resume; the pump stops new dispatches while paused; cancellation rejects queued work and wakes gated waiters while preserving exactly-once settlement.
Secondary claims: (1) the registry lifecycle distinguishes running/pausing/paused, allows cancel from any active state, locks terminal state, keeps counters exactly-once, and (R12) hasRunningEntries() excludes paused; (2) /workflows p <runId> and Background Tasks p provide the control and reject foreground / non-interactive / terminal / still-pausing cases clearly.
Budget went to: fresh A/B load-bearing proof with scripted differential (~half), fresh vacuity check, fresh e2e wire-oracle with R12 delta cells, fresh registry/nit1/watchdog probes, fresh targeted gates.
Central claim — A/B load-bearing proof (re-measured fresh)
The gate is WorkflowDispatchScheduler.waitUntilRunning(), chained by the orchestrator onto four result paths: entry-gate rejections (rejectThroughPauseGate, workflow-orchestrator.ts L1360, new in the R12 delta), cache-hit (L1433), live-dispatch success (L1603), and the error arm (L1612) — each resolving/rejecting with the original value whether the gate resolves or rejects (so a cancelled run never surfaces an unhandled rejection). Base has zero occurrences of waitUntilRunning (git show HEAD^1:…workflow-orchestrator.ts | grep -c = 0), so the gate is entirely this PR's. I drove the real compiled scheduler (packages/core/dist/…/workflow-dispatch-scheduler.js) through pause/resume/cancel/abort scenarios via a harness mirroring the orchestrator's exact gate-chain pattern, then ran the identical harness with the gate short-circuited (control = the orchestrator gate hunks reverted, nothing else), and scripted the differential.
| cell | environment | observable oracle | result |
|---|---|---|---|
| HEAD (gate active) | real compiled scheduler, limit=1, deferred in-flight A, queued B/C, error shapes, 50-job backlog | result/error held while paused; FIFO resume; exactly-once; abort drains queue + wakes gate waiter | 25/25 pass |
| CONTROL (gate removed) | same scheduler, gate chain short-circuited in the harness | same 25 assertions | 21/25 — exactly four flips, all in the "held while paused" family |
| differential (scripted) | ab-differential.mjs parses both cells |
HEAD all green; control flips exactly {S1-held-at-gate, S2-zero-deliveries-while-paused, S18-held-before-abort, S19-error-held-while-paused}; nothing else moves |
11/11 pass |
The four flips are the load-bearing property: with the gate gone, a fulfilled result is delivered while the run is paused (S1/S2), a held result settles before abort instead of being woken by it (S18), and a rejected dispatch settles during pause instead of being held (S19). Every other assertion — queued B/C never start while paused, FIFO A,B,C resume, exactly-once delivery, abort draining a 50-job backlog with AbortError in <500 ms, pause/resume guard rails, pre-aborted-signal refusal, invalid-limit throw, double-transition listener semantics (pausing then paused) — is unchanged across the two cells: those live in the scheduler's pump/abort logic; "hold results until resume" is contributed specifically by the waitUntilRunning() chaining. Witness: 01-ab-scheduler-head-vs-gateless.png (image keeps the tail — control cell + differential; the head cell's PASS lines are in the raw log); logs 01-scheduler-head.txt, 02-scheduler-gateless.txt, 09-ab-differential.txt.
Vacuity check (the gate hunk pins the central test) — re-measured fresh
Reverted only the live-dispatch success-path gate in workflow-orchestrator.ts ((result) => scheduler.waitUntilRunning().then(() => result, () => result) → (result) => result, interface-preserving) and ran the central test keeps a nested agent result behind the shared pause gate:
- Unmutated: green (
1 passed | 132 skipped), log03-vacuity-green.txt. - Mutated: red on the intended assertion —
AssertionError: expected true to be falseat L868expect(settled).toBe(false)(the run settles while paused because the result is no longer held), log04-vacuity-mutated.txt. Source restored afterward;git status --porcelainempty (byte-identicalcmp).
Witness: 02-vacuity-gate-hunk.png. The revert breaks the behavior the test exists to catch (not the import/compile/fixture), so the test is non-vacuous and pinned by exactly this hunk.
End-to-end wire-oracle (real orchestrator + vm sandbox + counting dispatch) — re-measured fresh
Drove the real WorkflowOrchestrator (real vm sandbox, real injected scheduler) with a counting, controllable fake dispatch — the seam the orchestrator's constructor is designed to accept — through the Reviewer Test Plan scenario (parallel([() => agent('A'), () => agent('B'), () => agent('C')]) at concurrency 1, abortOnTimeout threaded exactly as the runner does). 5 tests / 51 scripted assertions, all green:
- E1 pause/resume (18): only
Areaches the dispatch initially; afterpause()+ A settling, state ispaused, the dispatch still saw onlyA, queued=2, and the run is not settled (result held at the gate). Afterresume(), dispatches arriveA,B,Cin FIFO order, the transition band is exactlypausing,paused,running, the run settles exactly once with['resA','resB','resC']. - E2 stop a paused run (11): with B/C queued and A held,
controller.abort()→ B/C never reach the dispatch, the run rejects (message carries "abort"), settlement count is exactly 1 and rejection-shaped. - E3 (R12 delta, 8): a budget-gate rejection issued while
pausedis held (20 ms tick, no settlement, zero dispatch calls) and delivered on resume withexceeded the token budget. - E4 (R12 delta, 9): with
QWEN_CODE_MAX_WORKFLOW_AGENTS=1, the cap rejection for the secondagent()is held while paused; on resume the first (cap-passing) dispatch fires and the script observes both the cap error and its result. - E5 (R12 delta, 5): aborting a held entry-gate rejection preserves the original error through the gate's abort arm (script catches
exceeded the token budget), settling exactly once.
Witness: 04-e2e-pause-resume-cancel.png; log 08-e2e-orchestrator.txt.
Registry state-machine probe (secondary claim 1) — re-measured fresh
Drove the real compiled WorkflowRunRegistry (no mocks; handle stubs mirror WorkflowRunHandle's pause/resume/abort shape): 33/33 pass. Witness: 03-registry-state-machine.png; log 07-registry-probe.txt.
- Transitions are strict:
running→pausing→paused→runningadmitted; therunning→pausedskip,pausing→running, andpaused→pausingare all rejected (T4–T7). - Cancel from
paused→cancelled, aborting through the attached handle; the entry is then terminal: latecomplete(),fail(),onDispatchStateChange()are no-ops, and no completion<task-notification>or bell notification fires for cancel (the contract behind Reviewer step 3). - Completion counter is exactly-once: 5
onAgentCompletedcalls against 2 dispatched capagentsCompletedat 2 (T16). pause()needs backgrounded +running+ attached handle;resume()needspaused; foreground runs cannot be paused; duplicate active register throws;attachHandleis ignored for terminal entries;abortAll()cancels running and paused entries, leaves terminal ones, and fires statusChange exactly once;reset()clears.- R12 delta (T18):
hasRunningEntries()isfalsefor a lonepausedentry,trueforrunningandpausing— the property that keeps a paused-and-forgotten run from blocking/clearand session switch, with the CLI'suseBranchCommand/backgroundWorkUtilstests (R12) pinning the consumer side inside the gates.
nit1 probe (prior finding 1) + pause-aware wall-clock watchdog — re-measured fresh
nit1-probe.mjs on the real compiled scheduler (11/11): after abortPending() the scheduler never self-terminates — state rests in the pause band (pausing when in-flight never drained, paused when it had), the state listener sees no post-abort transition, and a late in-flight resolve keeps the state in the pause band; pause()/resume()/run() all refuse after abort and waitUntilRunning() rejects with AbortError. Harmless: terminal status is the registry's job (T12/T13 above).
watchdog-probe.mjs on the real compiled createWorkflowSandbox (3/3): with a 150 ms maxWallClockMs cap, a run that parks 400 ms in paused completes with done:ok (W1 — paused time neither burns budget nor kills the run), while the identical shape without pausing is killed by the cap with exceeded … active time (paused time is not counted) (W2 — the positive control proving W1's green is the suspension, not a disabled watchdog). Logs 10-nit1-probe.txt, 11-watchdog-probe.txt; witness 06-nit1-watchdog-probes.png.
Targeted gates (affected workspaces) — re-measured fresh
| workspace | files | result |
|---|---|---|
| core | workflow-dispatch-scheduler, workflow-orchestrator, workflow-runner, workflow-sandbox, workflow-run-registry, workflow-snapshot, tools/workflow/workflow |
7 files, 438/438 pass |
| cli | workflowsCommand, BackgroundTasksDialog, BackgroundTasksPill, useBackgroundTaskView, backgroundWorkUtils |
5 files, 162/162 pass |
Witness: 05-targeted-gates-core-cli.png; logs 05-core-gate.txt, 06-cli-gate.txt. Counts rose vs the prior round (402 → 438 core, 143 → 162 cli) — the delta is the R11–R13 test additions (e.g. the sandbox R11 promise-adoption suite, the R12 entry-gate and hasRunningEntries tests), all green inside these gates. The gate is live: the vacuity check above is the positive control (the central test goes red when the gate hunk is reverted). The deleted concurrencyLimiter module leaves no remaining references in packages/, scripts/, or integration-tests/.
Corrections
None. The prior report's mechanism description (gate = waitUntilRunning() chained on the result paths; terminal lockout in the registry suppresses the stop notification) was re-verified against the new head's source and stands; the R12 delta adds a fourth gated path (rejectThroughPauseGate) and the hasRunningEntries exclusion, both now covered above.
Findings
No blocking findings. The two carried-forward nits (status table rows 1–2) remain the only PR-attributable observations, both non-blocking and judged intended behavior.
One pre-existing, non-PR observation (not a finding): the terminal-bell notification slot (emitNotification) fires for foreground runs too — emitNotification is ungated at HEAD^1 as well (only the model-facing emitCompletion checks isBackgrounded), so this predates the PR; my initial probe assertion encoding the opposite was a harness assumption error, corrected in the final 33/33 registry run.
Not covered
- Durable cross-process resume, journal durability, real provider auth/network, per-agent controls — explicitly out of scope per the PR description; not probed.
- Per-commit attribution — the CI checkout is depth 2 (merge commit + base tip + PR head only);
git rev-parse --is-shallow-repository= true,git cat-file -t fd8a6ed77…(prior head) fails, and the metadata snapshot lists 31 commits while only 1 is locally reachable. Claims were verified against the aggregateHEAD^1..HEADdiff; the delta since the prior round (rounds 11–13) is scoped via the round markers in source (R11 sandbox, R12 orchestrator/registry) and covered by the fresh gates + probes, not attributed per commit. - Repo-wide lint / typecheck / bundle — covered by the PR's own CI; not re-run. My A/B used the pre-built
dist(timestamps precede this round) and vitest-on-source. - Standalone
nodeexecution of the orchestrator harness — importing the orchestrator as an entry module trips a pre-existing circular-init TDZ (carried forward as environmental from the prior round); the e2e wire-oracle therefore ran under the vitest load order via a temporary in-package test file, removed afterward (git statusclean). - Windows / macOS real-terminal pause/resume and interactive TUI sessions — the PR marks these
⚠️ (untested by the author); this Linux container ran the unit/integration layers, not a TUI session.
Methodology
Environment: CI verify container (node:22-bookworm, Node v22.23.2), working tree at refs/pull/8320/merge, npm ci + npm run build pre-completed at HEAD. Follow-up round with a changed head and base: every measurement re-run fresh; the prior head is unreachable locally so the delta is the aggregate diff. All harnesses are mock-free with respect to the unit under test — they import the real compiled dist modules (scheduler, registry, sandbox) or drive the real orchestrator + vm sandbox through the dispatch seam (the only fake is the dispatch, the seam the orchestrator is designed to accept). The A/B control differs from HEAD by nothing else than the gate chain (short-circuited in the harness; reverted hunk in the source for the vacuity check). Raw per-cell logs live in logs/ (01-scheduler-head.txt, 02-scheduler-gateless.txt, 03-vacuity-green.txt, 04-vacuity-mutated.txt, 05-core-gate.txt, 06-cli-gate.txt, 07-registry-probe.txt, 08-e2e-orchestrator.txt, 09-ab-differential.txt, 10-nit1-probe.txt, 11-watchdog-probe.txt); rerunnable harnesses are scheduler-harness.mjs, ab-differential.mjs, registry-harness.mjs, nit1-probe.mjs, watchdog-probe.mjs. Assertion tally: scheduler HEAD 25 + differential 11 + vacuity 2 + e2e 51 + registry 33 + nit1 11 + watchdog 3 + gates 600 (438 core + 162 cli) = 736, all met expectation; the control cell's four reds are the predicted base-arm failures, scripted as passes via ab-differential.mjs, so unexpected fail = 0.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
🤖 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
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
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).
|
Released in v0.21.8. |



































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