feat(core): expose workflow execution state - #9034
Conversation
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR! Template ✓ — all sections present, bilingual body included. Problem: real and already verified, not theoretical. The linked issue #9032 was triaged and accepted for exploration: the live workflow task state only carries aggregate counters ( Direction: aligned. Dynamic Workflows is an actively shipped capability (pause/resume in #8320, orchestration-policy guidance in #8694), and the sibling slices (#9033 daemon task contract, the parent split #8950) are already in flight. Issue triage verified the gap in code before accepting. Size: the PR touches core paths ( Approach: the slice matches the issue's acceptance criteria — additive JSON-safe state, snapshot round-trip that keeps legacy snapshots readable, runtime-only handles kept out of persistence, no daemon routes/SDK/UI. One scope question before deeper review: the diff also removes the "interactive TUI only" gate on Risk: no elevated-risk paths matched the revert-history signal. Moving on to code review. 🔍 中文说明感谢贡献！ 模板 ✓ —— 各节齐全，含双语正文。 问题：真实存在且已被核实，不是理论性问题。关联 issue #9032 已经过分拣并接受探索：当前运行时任务状态只有聚合计数（ 方向：对齐。Dynamic Workflows 是持续交付中的能力（#8320 暂停/恢复、#8694 编排策略指引），姊妹切片（#9033 daemon 任务契约、母拆分 #8950）也在推进中。issue 分拣时已在代码中核实过该缺口。 规模：本 PR 触及核心路径（ 方案：切片与 issue 验收标准一致 —— 增量 JSON-safe 状态、保持旧快照可读的快照往返、运行时句柄不落盘、不含 daemon 路由/SDK/UI。深入审查前有一个范围问题：diff 还移除了 风险：未命中回滚历史高风险路径信号。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI wrote my baseline first (registry-centric structured state, an ordered JSON-safe event ledger, additive snapshot round-trip with legacy defaults) and then compared the diff against it. The PR matches that shape; the async-local "dependency tails" mechanism is a genuinely nicer way to obtain real What I verified and found solid:
No critical blockers. Non-blocking notes:
The scope question from Stage 1 stands and belongs to this review too: the sequenceDiagram
participant S as Workflow script (vm sandbox)
participant O as Orchestrator
participant R as Run registry
participant J as Resume journal
participant F as Snapshot file
S->>O: agent(prompt) call
O->>O: mint dispatch id, read and replace dependency tails
O->>R: dispatchQueued (id, dependsOn)
Note over O,R: scheduler slot acquired
O->>R: dispatchStarted (id)
O->>R: dispatchSettled (id, error or none)
R->>R: append ordered JSON-safe event, bound and strip text
Note over S,F: terminal transition (complete, fail, cancel)
R->>R: close current phase, cancel live dispatches
O->>J: drain pending appends
R->>F: write validated snapshot (new fields optional on read)
Files changed (18 total)
Test evidenceThis is an unattended CI run — PR code is never built or executed here. The evidence below is the PR's own CI fetched via API at the reviewed commit; the focused-suite numbers in the PR body (7 core files / 466 tests, 31 CLI compat tests) are the author's claim, not independently re-run. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The Linux unit suite is still running at review time — per 中文说明代码审查先独立写下基线方案（以 registry 为中心的结构化状态、有序 JSON-safe 事件账本、增量快照往返且兼容旧快照），再与 diff 对比。PR 与基线一致；用 async-local "dependency tails" 从 已核实且可靠的部分：
无阻塞性问题。非阻塞意见：
Stage 1 的范围问题在代码审查中依然成立： 测试证据本次为无人值守 CI 运行——此处不构建、不执行 PR 代码。以下证据来自被审提交上 PR 自身 CI 的 API 查询；PR 正文中的聚焦测试数字（7 个 core 文件 / 466 测试、31 个 CLI 兼容测试）为作者自述，未独立复跑。CI 表格见上方英文版；审查时 Linux 单测仍在运行——按 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — a carefully engineered slice with genuinely good tests, capped by the core-size maintainer-awareness rule and one scope question that needs a human answer. Stepping back: the approach matches — and in places beats — my independent baseline. The async-local dependency-tail tracking extracts real Why I'm not approving:
The four non-blocking review notes (run-id guard consistency, uncalled primitives, untested nested-workflow tracing, closed-keyed event validation) stand as recorded in Stage 2 — none of them block. CI on this commit: the Linux PR gate was still running at review time; the finalize job updates the Stage 2 table once it settles. Note there is no deferred-approval marker on this PR — approval here requires a maintainer decision first, not just green checks. 中文说明置信度：3/5 —— 工程上精细、测试扎实的切片；因核心规模需维护者关注的规则封顶，且有一个范围问题需要人类回答。 整体看：方案与我的独立基线一致，部分更优。async-local 依赖尾追踪在不改动脚本接口的前提下从扇出中提取真实的 不批准的原因：
Stage 2 记录的四条非阻塞意见（run-id 守卫一致性、无调用方原语、嵌套 workflow 追踪无测试、事件封闭键校验）均不构成阻塞。 本提交的 CI：审查时 Linux PR 门禁仍在运行；落定后 finalize 任务会更新 Stage 2 表格。注意本 PR 没有延迟批准标记——此处批准需要先有维护者决定，而不仅是绿灯。 — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @yiliang114 @wenshao — needs a human call on this one. Two reasons, detailed in the stage comments above:
The review itself found no critical blockers; the code quality and test coverage are strong. CI was still running on the reviewed commit at review time — the finalize job will update the test table when it settles, but approval should wait for the maintainer decision either way. 中文说明⏸️ 转交 @yiliang114 @wenshao —— 需要人类判断。 两个原因（详见上方各阶段评论）：
审查本身未发现阻塞性问题；代码质量与测试覆盖都很扎实。审查时该提交的 CI 仍在运行——落定后 finalize 任务会更新测试表格，但无论如何批准都应等待维护者决定。 |
|
已修复： |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none significant — I did not verify the single setLineage call site for misuse (low relevance; #8 covers readback), and I did not trace TUI dialog consumers out…; "You are review agent reverse-audit — Reverse audit agent…": did not trace every setLineage call site for misuse (its read-back absence is already finding #8); TUI component consumers were cleared by grep (no phaseVisi…, and 7 more.
Test Plan (not a blocker): 466 tests passed — this review observed 19354, 19828, 1285, 1511, 494, 3393, 555 passed.
中文说明
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none significant — I did not verify the single setLineage call site for misuse (low relevance; #8 covers readback), and I did not trace TUI dialog consumers out…；"You are review agent reverse-audit — Reverse audit agent…"：did not trace every setLineage call site for misuse (its read-back absence is already finding #8); TUI component consumers were cleared by grep (no phaseVisi…，另有 7 条。
Test Plan（非阻断）：466 tests passed — this review observed 19354, 19828, 1285, 1511, 494, 3393, 555 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| label: | ||
| stripAnsiAndControl(event.label ?? '').slice(0, 200) || fallbackLabel, | ||
| prompt: stripAnsiAndControl(event.prompt).slice(0, 4_096), | ||
| status: event.cached ? 'cached' : 'queued', |
There was a problem hiding this comment.
[Suggestion] The cached dispatch branch (status: 'cached', endedAt = queuedAt, dispatch-queued + dispatch-cached events, and the endedAt guard absorbing the follow-up dispatchSettled) plus the orchestrator's cache-hit issueDispatchTrace(prompt, opts, true) have zero test coverage anywhere, though issue #9032 AC1/AC2/AC6 name the cached path explicitly. — Failure scenario: mutation "delete the cached branch" stays green: journal-resumed dispatches would record as queued, then the follow-up dispatchSettled flips them to completed with a spurious dispatch-completed event per cache hit (the endedAt guard that prevents that is set only in the cached branch) — the resume-vs-live distinction in the persisted graph silently breaks with the whole suite green.
Add a registry test for onDispatchQueued with cached: true (status, endedAt === queuedAt, both events, and that a subsequent onDispatchSettled is a no-op), or extend a P6 resume test with a dispatchQueued collector asserting cached: true.
中文说明
[Suggestion] cached dispatch 分支（status: 'cached'、endedAt = queuedAt、dispatch-queued + dispatch-cached 事件、以及吸收后续 dispatchSettled 的 endedAt 守卫）与 orchestrator 缓存命中路径的 issueDispatchTrace(prompt, opts, true) 完全没有测试覆盖，而 issue #9032 的 AC1/AC2/AC6 明确点名了 cached 路径。失败场景：变异"删除 cached 分支"后整套测试仍为绿：journal 恢复的 dispatch 会记为 queued，随后 dispatchSettled 把它们翻成 completed 并为每个缓存命中追加多余的 dispatch-completed 事件（阻止这一行为的 endedAt 守卫只在 cached 分支设置）——持久化依赖图中 resume 与 live 的区分被静默破坏。建议为 onDispatchQueued 增加 cached: true 的 registry 测试，或扩展 P6 resume 测试断言 cached: true 与 dispatch-cached 事件。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Deferred to the next round (this round was capped at 8 findings, Criticals first — all Criticals were already resolved). Still planned: a registry test for onDispatchQueued with cached: true asserting status cached, endedAt === queuedAt, the dispatch-queued + dispatch-cached events, and that a follow-up onDispatchSettled is a no-op.
中文说明
延后至下一轮（本轮按 8 条上限处理，Critical 优先——目前所有 Critical 均已解决）。仍计划：为 onDispatchQueued 增加 cached: true 的 registry 测试，断言状态 cached、endedAt === queuedAt、dispatch-queued + dispatch-cached 事件，且后续 onDispatchSettled 为空操作。
| const canary = path.join(projectDir, 'CANARY.txt'); | ||
| await fs.writeFile(canary, 'keep', 'utf8'); |
There was a problem hiding this comment.
[Suggestion] The canary in this traversal test is never on any unguarded deletion path: for input '../CANARY' the recursive fs.rm target resolves to <projectDir>/CANARY and the unlink target to <projectDir>/CANARY.json — neither matches the planted <projectDir>/CANARY.txt. — Failure scenario: probe-verified — with the isSafeRunIdSegment guard bypassed the test still fails only at the boolean assertion, never at the canary; with a second mutation (unlink no longer tolerating ENOENT — the future scenario this test is meant to guard) all 19 tests pass while fs.rm(..., {recursive: true, force: true}) deletes any traversal target that exists. The file-damage assertion its name promises cannot fail for any runId-shaped input.
| const canary = path.join(projectDir, 'CANARY.txt'); | |
| await fs.writeFile(canary, 'keep', 'utf8'); | |
| const canary = path.join(projectDir, 'CANARY'); | |
| await fs.writeFile(canary, 'keep', 'utf8'); |
(then pass '../CANARY' as the traversal run id — an unguarded recursive rm actually deletes the canary and the read-back fails)
中文说明
[Suggestion] 该穿越测试中的金丝雀文件从不位于任何未设防的删除路径上：输入 '../CANARY' 时递归 fs.rm 目标解析为 <projectDir>/CANARY、unlink 目标为 <projectDir>/CANARY.json——两者都与植入的 <projectDir>/CANARY.txt 不匹配。失败场景（探针验证）：绕过 isSafeRunIdSegment 守卫时测试只在布尔断言处失败，金丝雀断言永远不触发；再做第二个变异（unlink 不再容忍 ENOENT——正是该测试要防的未来场景）时全部 19 个测试通过，而 fs.rm 会删除任何真实存在的穿越目标。建议将金丝雀改名为不带扩展名的 CANARY，未设防的递归删除就会真正删掉它，使读回断言可以失败。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const parsed = JSON.parse( | ||
| await fs.readFile(snapshotPath, 'utf8'), | ||
| ) as Record<string, unknown>; | ||
| delete parsed['events']; |
There was a problem hiding this comment.
[Suggestion] This legacy-load test strips only events, but the validator promises legacy tolerance for five more new optional fields — description, phaseVisits, dispatches are never exercised absent (probe-verified; sourceRunId/startMode happen to be absent because JSON.stringify drops undefined keys). — Failure scenario: probe — removing the phaseVisits === undefined || branch from isWorkflowSnapshot keeps all 19 snapshot tests green, because the rewritten "legacy" file still carries phaseVisits: [], dispatches: [], description. A genuine pre-tracing snapshot lacks all six keys, so such a future tightening silently skips every snapshot written by an earlier release (debug-log-only) — it vanishes from /workflows history while this test stays green.
| delete parsed['events']; | |
| delete parsed['events']; | |
| delete parsed['phaseVisits']; | |
| delete parsed['dispatches']; | |
| delete parsed['description']; |
中文说明
[Suggestion] 该旧快照加载测试只删除了 events，但校验器承诺对另外五个新增可选字段兼容旧格式——其中 description、phaseVisits、dispatches 从未在缺失状态下被验证（探针确认；sourceRunId/startMode 只是碰巧因 JSON.stringify 丢弃 undefined 键而缺失）。失败场景：探针显示，移除 isWorkflowSnapshot 中的 phaseVisits === undefined || 分支后全部 19 个快照测试仍为绿，因为被改写的"旧"文件仍带有 phaseVisits: []、dispatches: []、description。真正的历史快照六个键全部缺失，未来若收紧校验会静默跳过所有早期版本写入的快照（仅 debug 日志），它们从 /workflows 历史中消失而本测试依然为绿。建议同时删除这三个字段。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| dispatch.status = | ||
| entry.status === 'cancelled' | ||
| ? 'cancelled' | ||
| : error | ||
| ? 'failed' |
There was a problem hiding this comment.
[Suggestion] The failed-dispatch settle path has no test: no test anywhere calls onDispatchSettled with an error, and no orchestrator test collects dispatchSettled events for a rejecting dispatch — the error ? 'failed' arm, dispatch.error population, the dispatch-failed ledger event, and the orchestrator catch-arm emission are all uncovered. — Failure scenario: mutation "ignore the error argument" stays green: status falls through to completed, no error is stored, a dispatch-completed event is appended instead — a failed dispatch is persisted/replayed as completed with no error text.
Add a registry test: onDispatchQueued → onDispatchStarted → onDispatchSettled(runId, id, 'boom', at) asserting status: 'failed', truncated dispatch.error, and a dispatch-failed event; plus an orchestrator test with a rejecting dispatch asserting the settled event carries the message.
中文说明
[Suggestion] 失败 dispatch 的落定路径没有测试：没有任何测试用带 error 的参数调用 onDispatchSettled，也没有 orchestrator 测试收集被拒绝 dispatch 的 dispatchSettled 事件——error ? 'failed' 分支、dispatch.error 填充、dispatch-failed 账本事件、orchestrator catch 分支的上报全部未覆盖。失败场景：变异"忽略 error 参数"后测试仍为绿：状态落入 completed、不记录错误、追加 dispatch-completed 事件——失败的 dispatch 被持久化/回放为成功且无错误文本。建议补充 registry 与 orchestrator 两层测试。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Deferred to the next round. Still planned: a registry test driving onDispatchQueued → onDispatchStarted → onDispatchSettled with an error (asserting status: 'failed', truncated dispatch.error, dispatch-failed event), plus an orchestrator test asserting the settled event for a rejecting dispatch.
中文说明
延后至下一轮。仍计划：registry 测试依次执行 onDispatchQueued → onDispatchStarted → 带 error 的 onDispatchSettled（断言 status: 'failed'、截断后的 dispatch.error、dispatch-failed 事件），并补充 orchestrator 层对被拒绝 dispatch 落定事件的断言。
| getPhases: () => [...phases], | ||
| getLogs: () => [...logs], | ||
| appendLog: (line: string) => safeLog(line), | ||
| appendLog: (line: string) => safeLog(line, false), |
There was a problem hiding this comment.
[Suggestion] The nested-workflow settlement merge goes through appendLog → safeLog(line, notify=false), which silently drops lines once the parent buffer is at MAX_LOG_LINES — while the same nested lines were already live-recorded into recentLogs/events via the shared emitter. The merge comment's own intent ("retains the same lines exactly once") fails at the cap: the outcome retains them zero times while the event ledger retains them. — Failure scenario: probe (unmodified PR): parent logs 10,001 lines, nested logs 5 → live record has all 5; outcome/recentLogs has zero of them while events lists all 5 — the persisted projections disagree. Boundary probe: parent at 9,998 merges exactly 2 of 5 nested lines, adds a truncation marker, silently drops the rest. Pre-PR flip arm (logAppended: () => {}) agrees — this PR's live wiring creates the divergence.
Make the merge loss observable — e.g. have appendLog report whether the line was accepted and push a single [nested logs truncated…] marker into the parent buffer on first drop.
中文说明
[Suggestion] 嵌套 workflow 落定时的合并经由 appendLog → safeLog(line, notify=false)，当父缓冲已达 MAX_LOG_LINES 时会静默丢弃日志行——而同样的嵌套行此前已通过共享 emitter 实时记录进 recentLogs/events。合并注释自身的意图（"完全一次地保留相同行"）在上限处失效：outcome 一次都没保留，事件账本却全部保留。失败场景（未改动 PR 上的探针）：父级记录 10,001 行、嵌套记录 5 行 → 实时记录有全部 5 行；outcome/recentLogs 为 0 行而 events 列出全部 5 行——持久化的两份投影不一致。边界探针：父级 9,998 行时恰好合并 5 行中的 2 行，追加截断标记，其余静默丢失。PR 之前的翻转臂（logAppended: () => {}）两份投影一致——是本 PR 的实时接线引入了分歧。建议让合并丢失可观测：例如 appendLog 返回是否接受，并在首次丢弃时向父缓冲推入一条 [nested logs truncated…] 标记。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Deferred to the next round. Agreed: the nested-settlement merge silently drops lines once the parent buffer is at MAX_LOG_LINES while the event ledger retains them. Planned: make appendLog report whether the line was accepted and push a single [nested logs truncated…] marker into the parent buffer on first drop.
中文说明
延后至下一轮。认同：嵌套落定合并在父缓冲达到 MAX_LOG_LINES 时会静默丢行，而事件账本仍保留这些行。计划：让 appendLog 返回是否接受该行，并在首次丢弃时向父缓冲推入一条 [nested logs truncated…] 标记。
| await fs.rm(`${storage.getWorkflowRunsDir()}/${runId}`, { | ||
| recursive: true, | ||
| force: true, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Three probe-observed issues in deleteWorkflowSnapshot/listWorkflowSnapshots: (a) IDENTITY — the list keys entries by filename but never reconciles the content runId, while delete targets <runId>.json + <runId>/: a planted workflows/x.json claiming runId: "wf_deadbeef" lists as a phantom whose delete destroys the REAL wf_deadbeef journal + snapshot, returns true, and leaves x.json permanently undeletable (probe-observed). (b) ORDERING — this irreversible recursive journal-dir delete runs before the snapshot unlink, so a non-ENOENT unlink failure (EBUSY/EACCES — locked file, indexer/AV handle, NFS) returns false with the run still listed but its resume journal destroyed; a later resume loads a missing journal (returns empty maps) and silently re-runs every agent, re-spending tokens (probe-observed). The existing test codifies only the opposite failure direction. (c) GUARD — pruneSnapshots gates the identical recursive delete on /^wf_[0-9a-f]+$/ against exactly this threat (its own comment) while delete uses the looser isSafeRunIdSegment (the guard mismatch itself was noted in triage stage 2; identity/ordering are new). No production caller exists yet, which is why this is a Suggestion today — escalate to Critical if a caller is wired before these are fixed. — Concrete cost: collateral destruction of another run's resume data + a permanently undeletable phantom entry, demonstrated by probe on the reviewed commit.
Swap the order (unlink the snapshot first, recursive-delete the journal dir last) and make the filename the single identity: skip (warn) in listWorkflowSnapshots when parsed.runId !== file.replace(/\.json$/, ''); reusing the wf_<hex> gate keeps both deletion paths behind one guard. Both directions were patch-verified by probe.
中文说明
[Suggestion] deleteWorkflowSnapshot/listWorkflowSnapshots 的三个探针实测问题：(a) 身份不一致——列表按文件名建立条目但从不校验内容中的 runId，而删除按 <runId>.json + <runId>/ 定位：植入一个声明 runId: "wf_deadbeef" 的 workflows/x.json 会列出幽灵条目，删除它会摧毁真实 wf_deadbeef 的 journal + 快照、返回 true、且 x.json 永远无法删除（探针实测）。(b) 顺序——这个不可逆的递归 journal 目录删除先于快照 unlink 执行，若 unlink 遇到非 ENOENT 失败（EBUSY/EACCES——锁定文件、索引器/杀软句柄、NFS），函数返回 false 但运行仍在列表、resume journal 已被摧毁；之后 resume 加载缺失的 journal（返回空映射）并静默重跑全部 agent、重复消耗 token（探针实测）。现有测试只钉住了相反方向的失败。(c) 守卫——pruneSnapshots 对同一递归删除使用 /^wf_[0-9a-f]+$/ 严格门控以应对完全相同的威胁（见其注释），而 delete 用的是更宽松的 isSafeRunIdSegment（守卫不一致在 triage stage 2 已被提及；身份与顺序问题为新发现）。目前尚无生产调用方，因此暂为 Suggestion——若在修复前接入调用方应升级为 Critical。建议：交换顺序（先 unlink 快照、最后递归删除 journal 目录），并让文件名成为唯一身份：listWorkflowSnapshots 中当 parsed.runId !== file.replace(/\.json$/, '') 时跳过（并告警）；复用 wf_<hex> 门控使两条删除路径共用同一守卫。两种修复均已通过探针验证。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Partially addressed: the guard half (c) was fixed in 15895b8 — deleteWorkflowSnapshot now gates on the same /^wf_[0-9a-f]+$/ shape as pruneSnapshots (round-5 thread has the verification). The remaining halves — (a) identity reconciliation in listWorkflowSnapshots and (b) unlink-before-recursive-rm ordering — are deferred to the next round.
中文说明
已部分处理：守卫部分 (c) 已在 15895b8 修复——deleteWorkflowSnapshot 现与 pruneSnapshots 使用同一 /^wf_[0-9a-f]+$/ 门控（验证见第 5 轮线程）。剩余两半——(a) listWorkflowSnapshots 的身份核对与 (b) 先 unlink 快照再递归删 journal 的顺序——延后至下一轮。
| const operation = this.pending.then(() => writeLine(this.path, entry)); | ||
| this.pending = operation.catch(() => undefined); |
There was a problem hiding this comment.
[Suggestion] The chain-poisoning guard that isolates a failed append from all subsequent appends (this.pending = operation.catch(() => undefined)) has zero test coverage — no test makes an append reject. Verified: the mutation this.pending = operation keeps 214/214 workflow tests and the full packages/core suite green. — Failure scenario: probe flips — with the mutation, one transient writeLine failure (EMFILE / disk full / AV lock) leaves this.pending rejected, and every later append short-circuits through .then without ever calling writeLine — the journal silently loses every remaining started/result entry, and the next Workflow({resumeFromRunId}) re-runs the whole suffix live: duplicated token spend, potentially different results, only a debug-level warning as signal.
Add one test: make the first append reject (e.g. mockRejectedValueOnce on fs.promises.appendFile), then assert the failing append rejects for its caller while the second append still writes and drain() resolves.
中文说明
[Suggestion] 隔离失败追加、保护后续所有追加的防中毒守卫（this.pending = operation.catch(() => undefined)）零测试覆盖——没有任何测试让 append 被拒绝。已验证：变异 this.pending = operation 后 214/214 workflow 测试与整个 packages/core 套件仍为绿。失败场景（探针翻转）：变异之后，一次瞬时 writeLine 失败（EMFILE/磁盘满/杀软锁）使 this.pending 保持 rejected，此后每次 append 都经 .then 短路、永不调用 writeLine ——journal 静默丢失剩余全部 started/result 条目，下一次 Workflow({resumeFromRunId}) 会完整 live 重跑整个后缀：重复消耗 token、结果可能不同，唯一的信号是一条 debug 级警告。建议补一个测试：令第一次 append 被拒绝，断言失败的 append 对调用方拒绝、第二次 append 仍写入、drain() 正常 resolve。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Deferred to the next round. Still planned: one journal test making the first append reject (mockRejectedValueOnce on fs.promises.appendFile), asserting the failing append rejects for its caller while the second append still writes and drain() resolves.
中文说明
延后至下一轮。仍计划：新增一个 journal 测试令首次 append 被拒绝（对 fs.promises.appendFile 使用 mockRejectedValueOnce），断言失败的 append 对调用方拒绝、第二次 append 仍写入、drain() 正常 resolve。
| // The shared emitter already publishes nested logs live. Merge | ||
| // them into the parent buffer without re-emitting so the final | ||
| // outcome retains the same lines exactly once. |
There was a problem hiding this comment.
[Suggestion] Nested-workflow log lines reach the registry live (occurrence order) but enter the parent sandbox buffer only at nested settlement (merge order), so the persisted events log order and recentLogs order can disagree even when nothing is dropped — distinct from the buffer-full drop at the appendLog site. — Failure scenario: probe (real runner end-to-end): parallel([() => workflow('child' logging N1), () => { await agent('wait'); log('B'); }]) persists recentLogs: ["B", "N1"] beside events ordered ["N1", "B"] on the same snapshot, fired with an empty parent buffer. Flip arm (removing the settlement setRecentLogs overwrite) makes them agree, locating the divergence at the overwrite. Pre-PR the emitter was a no-op — one projection, no disagreement; this diff's live wiring creates the second. The UI log display also visibly reorders at settlement.
Pick one source of truth per projection: merge nested logs into the parent buffer as they are emitted (live), or drop the merge entirely now that the shared emitter publishes nested lines live and setRecentLogs is the terminal projection.
中文说明
[Suggestion] 嵌套 workflow 的日志行实时进入 registry（发生顺序），但只在嵌套落定时才进入父 sandbox 缓冲（合并顺序），因此持久化的 events 日志顺序与 recentLogs 顺序可能不一致——即使一行都没丢；与 appendLog 处上限丢弃是不同问题。失败场景（真实 runner 端到端探针）：parallel([() => workflow('child' 记录 N1), () => { await agent('wait'); log('B'); }]) 在同一份快照中持久化 recentLogs: ["B", "N1"] 与顺序为 ["N1", "B"] 的 events，且父缓冲为空。翻转臂（移除落定时的 setRecentLogs 覆写）使两者一致，将分歧定位在覆写处。PR 之前 emitter 是空操作——只有一份投影、无分歧；是本 diff 的实时接线制造了第二份。UI 日志展示也会在落定时可见地重排。建议每个投影选定唯一事实来源：要么在嵌套行发出时即合并进父缓冲，要么在共享 emitter 已实时发布嵌套行、setRecentLogs 即为终态投影的现在彻底移除合并。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Deferred to the next round. Both fix directions (merge nested logs into the parent buffer live, or drop the settlement-time merge now that the shared emitter publishes nested lines live) need a probe of the UI log-display impact first; the divergence between events order and recentLogs order is confirmed real.
中文说明
延后至下一轮。两个修复方向（嵌套行发出时即合并进父缓冲，或在共享 emitter 已实时发布嵌套行的现在移除落定合并）都需要先探针评估 UI 日志展示的影响；events 顺序与 recentLogs 顺序的分歧已确认真实存在。
| const dispatchId = entry.dispatches.find( | ||
| (dispatch) => dispatch.subagentId === approval.subagentId, | ||
| )?.id; |
There was a problem hiding this comment.
[Suggestion] The dispatch↔approval correlation is only half-tested: the binding write (dispatch.subagentId) is asserted, but this consumer lookup — which stamps dispatchId onto approval-settled ledger events — has zero coverage. The only approval-event assertions run on an entry with no dispatches, so dispatchId is structurally absent there and passes regardless. — Failure scenario: probe — replacing the lookup + spread with a plain { type, at, name } append keeps all five workflow suites green (302/302). The PR's stated purpose for threading dispatchId through bridgeApprovalEvents is attributing approvals to dispatches in the replay ledger (issue #9032 AC3) — a regression deleting the attribution ships silently, and replay/audit consumers can no longer tell which dispatch an approval blocked.
Extend the binding test to resolve the parked approval and assert the ledger linkage, e.g. expect(entry.events).toEqual(expect.arrayContaining([expect.objectContaining({ type: 'approval-settled', dispatchId: 'dispatch-1' })])).
中文说明
[Suggestion] dispatch↔approval 关联只测了一半：绑定写入（dispatch.subagentId）有断言，但此消费方查找——把 dispatchId 盖到 approval-settled 账本事件上——零覆盖。仅有的 approval 事件断言运行在没有任何 dispatch 的条目上，dispatchId 在那里结构性缺失也能通过。失败场景（探针）：把查找 + 展开替换为普通的 { type, at, name } 追加，全部五个 workflow 套件 302/302 仍为绿。本 PR 把 dispatchId 穿进 bridgeApprovalEvents 的声明目的正是在回放账本中把审批归属到 dispatch（issue #9032 AC3）——删除归属的回归会静默通过，回放/审计消费方将无法分辨审批阻塞的是哪个 dispatch。建议扩展绑定测试：resolve 被停泊的审批并断言账本关联。
— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Deferred to the next round. Still planned: extend the binding test to resolve the parked approval and assert the ledger linkage, e.g. expect(entry.events).toEqual(expect.arrayContaining([expect.objectContaining({ type: 'approval-settled', dispatchId: 'dispatch-1' })])).
中文说明
延后至下一轮。仍计划：扩展绑定测试，resolve 被停泊的审批并断言账本关联，例如 expect(entry.events).toEqual(expect.arrayContaining([expect.objectContaining({ type: 'approval-settled', dispatchId: 'dispatch-1' })]))。
已被后续 commit 取代，当前 head 需重新 review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": did not execute a runtime reproduction of the phase-collapse scenario (code-traced only; no test run).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — completed the full walk within the tool budget; no check left unfinished., and 7 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): 466 tests passed — this review observed 19357, 19829, 1285, 1511, 494, 3393, 555 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：did not execute a runtime reproduction of the phase-collapse scenario (code-traced only; no test run).；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — completed the full walk within the tool budget; no check left unfinished.，另有 7 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：466 tests passed — this review observed 19357, 19829, 1285, 1511, 494, 3393, 555 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| try { | ||
| emitter?.dispatchSettled?.(dispatchId, message, Date.now()); | ||
| } catch (e) { |
There was a problem hiding this comment.
[Suggestion] R2-21: External abort settles dispatches as failed on a run that settles cancelled. This new dispatchSettled emission fires from emitCompletion's rejection arms while entry.status is still active — the runner's registry.cancel() runs downstream of the same rejection chain — so onDispatchSettled records error ? 'failed' and sets endedAt; cancel()'s cancelLiveDispatches then skips the already-settled dispatches and none ever becomes cancelled. — Failure scenario: probe-reproduced end-to-end on the real runner: a foreground run aborted via options.signal mid-dispatch settles with entry.status: 'cancelled' but every dispatch status: 'failed' (errors "This operation was aborted" / "Workflow dispatch scheduler aborted.") and events [dispatch-failed…, workflow-cancelled]; removing only this emission flips both arms to dispatch-cancelled. Trigger: any Ctrl+C / caller-abort during a foreground workflow with queued/in-flight dispatches (the dialog-cancel path calls registry.cancel() first, background runs and the timeout path are insulated). The persisted snapshot/ledger/dialog then shows a cancelled run whose dispatches all failed — dispatch status drives no scheduling or resume, hence Suggestion rather than Critical. — Suggested fix: do not classify an abort as a generic failure — when the error is abort-shaped (isAbortError exists in utils/errors.ts), skip the dispatchSettled emission and let cancelLiveDispatches record cancelled; or cancel the registry entry synchronously on options.signal abort before settlement propagates.
中文说明
[Suggestion] 外部中止会把落定在 cancelled 状态运行中的 dispatch 记为 failed。这个新增的 dispatchSettled 上报在 emitCompletion 的拒绝分支中触发，此时 entry.status 仍是活跃态——runner 的 registry.cancel() 在同一条拒绝链的更下游才执行——因此 onDispatchSettled 走 error ? 'failed' 分支并设置 endedAt；随后 cancel() 的 cancelLiveDispatches 会跳过这些已落定的 dispatch，没有任何 dispatch 变为 cancelled。失败场景（真实 runner 探针复现）：前台运行在 dispatch 途中被 options.signal 中止后，entry.status: 'cancelled' 但每个 dispatch 都是 status: 'failed'（错误为 "This operation was aborted" / "Workflow dispatch scheduler aborted."），事件序列为 [dispatch-failed…, workflow-cancelled]；仅移除此处上报后两侧均翻转为 dispatch-cancelled。触发条件：前台 workflow 有排队/在途 dispatch 时按 Ctrl+C / 调用方中止（对话框取消路径会先调 registry.cancel()，后台运行与超时路径不受影响）。持久化的快照/账本/对话框会显示一个被取消的运行里所有 dispatch 都失败——dispatch 状态不驱动调度或恢复，故为 Suggestion 而非 Critical。建议修复：不要把中止当作普通失败——错误为中止形态时（utils/errors.ts 已有 isAbortError）跳过 dispatchSettled 上报，交给 cancelLiveDispatches 记录 cancelled；或在 options.signal 中止时同步取消 registry 条目，再让落定传播。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (last !== title) { | ||
| entry.phases.push(title); | ||
| const priorVisit = entry.phaseVisits[entry.phaseVisits.length - 1]; |
There was a problem hiding this comment.
[Suggestion] R2-20: Registry phase-collapse and sandbox phase-collapse run on different arrays. The registry collapses consecutive duplicates against entry.phases, which nested-child phases pollute (the child sandbox shares the parent emitter and fires phaseStarted into the same run entry), while the sandbox collapses against its own parent-only phases array — so the new phaseVisits/dispatch-attribution machinery silently skips visits across the nesting boundary. — Failure scenario: probe-reproduced end-to-end: parent phase('A'); await workflow('child' calling phase('B') + agent()); phase('B'); agent('after-nested') — the child's phase('B') opens visit phase-2; the parent's own post-nested phase('B') (new for its sandbox) is collapsed by the registry: no new visit, no phase-started event, and the parent's post-nested dispatch gets phaseVisitId: 'phase-2' — the child-opened visit. The persisted timeline/dispatch attribution conflates parent work with the child's visit. (The mirror variant does not reproduce — a child's duplicate phase is itself collapsed, so no child visit exists to steal attribution.) — Suggested fix: make one layer the sole owner of collapse — either pass an explicit new/repeat signal from the sandbox (which owns the per-script phase sequence), or give the nested sandbox a child-scoped emitter shim that namespaces/suppresses phaseStarted (mirroring the appendLog no-re-emit log merge) and merge child visits deliberately at settlement.
中文说明
[Suggestion] registry 侧的 phase 折叠与 sandbox 侧的折叠运行在不同的数组上。registry 基于 entry.phases 折叠连续重复项，而嵌套子 workflow 的 phase 会污染该数组（子 sandbox 共享父级 emitter，phaseStarted 直接打进同一个运行条目）；sandbox 则基于自己仅有的 phases 数组折叠——因此新增的 phaseVisits/dispatch 归属机制会在跨嵌套边界时静默跳过 visit。失败场景（端到端探针复现）：父脚本 phase('A'); await workflow('child'（其中调用 phase('B') + agent()）); phase('B'); agent('after-nested') —— 子级的 phase('B') 打开 visit phase-2；父级嵌套返回后自己的 phase('B')（对其 sandbox 而言是新 phase）被 registry 折叠：没有新 visit、没有 phase-started 事件，父级嵌套后的 dispatch 被标记 phaseVisitId: 'phase-2'——即子级打开的 visit。持久化的时间线/dispatch 归属把父级工作与子级 visit 混为一谈。（镜像变体不复现——子级的重复 phase 自身会被折叠，不存在可抢占归属的子 visit。）建议修复：让其中一层独占折叠决策——要么由 sandbox（拥有每个脚本自己的 phase 序列）传递明确的 新/重复 信号，要么为嵌套 sandbox 提供子级作用域的 emitter 适配层，对 phaseStarted 做命名空间隔离/抑制（类比 appendLog 的不重发合并），并在落定时有意识地合并子级 visit。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round. The registry/sandbox phase-collapse divergence across the nesting boundary needs either an explicit new/repeat signal from the sandbox or a child-scoped emitter adapter for phaseStarted — a design change larger than this round's budget.
中文说明
延后至下一轮。registry 与 sandbox 在嵌套边界上的 phase 折叠分歧，需要 sandbox 传递明确的新/重复信号，或为 phaseStarted 提供子级作用域的 emitter 适配层——属于超出本轮预算的设计改动。
| const dependsOn = Array.from(new Set(store?.tails ?? [])); | ||
| if (store) store.tails = [id]; |
There was a problem hiding this comment.
[Suggestion] R2-18: A parallel()/pipeline() branch that completes without issuing any dispatch keeps the inherited tails its store was seeded with, and mergeFanoutTails's !inherited.has(tail) filter guards only currentParentTails — branchTails enters the union unfiltered, re-injecting ancestor ids into parent.tails and producing redundant transitive dependsOn edges in the recorded graph. — Failure scenario: probe-observed through the real orchestrator: after agent('a'), parallel([() => agent('b'), () => 42]) plus an agent('m') issued before the await gives z deps ['b','a','m'] — 'a' is a redundant transitive edge (reachable via 'b'); the minimal-edge semantics the PR's own new dependency tests pin would give ['b','m']. The pipeline variant leaks identically when an item's chain drops before its first dispatch. Reachability is preserved (edges only point at true ancestors), so nothing schedules wrongly — the cost is fidelity of the persisted execution graph (the issue #9032 AC2 artifact). The filter-flip fix was probe-tested and keeps all 139 orchestrator tests green. — Suggested fix: filter branch tails in mergeFanoutTails (branchTails.filter((tail) => !inherited.has(tail)) — safe because a dispatching branch always has tails = [newId]), or track a per-branch dispatched flag and merge only dispatched branches' tails.
中文说明
[Suggestion] 未发出任何 dispatch 就结束的 parallel()/pipeline() 分支会保留其 store 初始化时继承的 tails，而 mergeFanoutTails 的 !inherited.has(tail) 过滤只作用于 currentParentTails——branchTails 未经过滤进入并集，把祖先 id 重新注入 parent.tails，使记录的依赖图产生冗余的传递性 dependsOn 边。失败场景（真实 orchestrator 探针观测）：agent('a') 之后执行 parallel([() => agent('b'), () => 42])，且在 await 前发出 agent('m')，则 z 的依赖为 ['b','a','m']——'a' 是冗余传递边（经由 'b' 可达）；本 PR 新增依赖测试所钉住的最小边语义应给出 ['b','m']。pipeline 变体在条目链在首个 dispatch 前中断时以同样方式泄漏。可达性不受影响（边只指向真正的祖先），调度不会出错——代价是持久化执行图（issue #9032 AC2 的产物）的保真度。过滤翻转修复已经过探针验证，139 个 orchestrator 测试全部保持绿色。建议修复：在 mergeFanoutTails 中过滤分支 tails（branchTails.filter((tail) => !inherited.has(tail))——发出过 dispatch 的分支恒有 tails = [newId]，因此安全），或为每个分支记录 dispatched 标记，只合并发出过 dispatch 的分支 tails。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| clearStatusChangeCallback(cb: WorkflowRunStatusChangeCallback): void { | ||
| if (this.statusChangeCallback === cb) this.statusChangeCallback = undefined; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-6 (1 of 4 — dead public surface pattern, AGENTS.md grep-the-read-sites rule): clearStatusChangeCallback has zero production callers and no test. The only clearStatusChangeCallback call site in production code is Session.ts:3106 (dispose), which targets config.getBackgroundTaskRegistry() — the sibling registry with its own method — not the workflow registry; the TUI disposes the workflow registry's callback via setStatusChangeCallback(undefined) (useBackgroundTaskView.ts:338). The identity-preserving clear this method offers is unreachable API surface. Sibling comments cover the pattern's other instances (setLineage, args, workflowRunId). — Suggested fix: wire it into the session dispose path next to the background-task registry clear, or delete it until a caller exists.
中文说明
[Suggestion] R2-6（共 4 处之 1——死的公开表面模式，AGENTS.md 的 grep 读取方规则）：clearStatusChangeCallback 没有任何生产调用方，也没有测试。生产代码中唯一的 clearStatusChangeCallback 调用点是 Session.ts:3106（dispose），其目标是 config.getBackgroundTaskRegistry()——拥有自己同名方法的兄弟 registry——而非 workflow registry；TUI 通过 setStatusChangeCallback(undefined) 清理 workflow registry 的回调（useBackgroundTaskView.ts:338）。该方法提供的保身份清理是不可达的 API 表面。相邻评论覆盖该模式的其余实例（setLineage、args、workflowRunId）。建议修复：在 session dispose 路径中与 background-task registry 的清理并列接入，或在调用方存在之前删除。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Declined: the maintainer's local verification report (F8) explicitly records the non-test-callable surfaces as expected for this split slice ('Expected for a split slice — noting it so the follow-up doesn't drift'). Removing clearStatusChangeCallback now would churn the follow-up slice that wires it; tracking it there is the recorded intent.
中文说明
拒绝：维护者的本地验证报告（F8）明确把无非测试调用方的表面记为拆分切片的预期状态（'Expected for a split slice — noting it so the follow-up doesn't drift'）。现在移除 clearStatusChangeCallback 只会给后续接入它的切片制造反复；按记录交给后续切片处理。
| setLineage( | ||
| runId: string, | ||
| sourceRunId: string, |
There was a problem hiding this comment.
[Suggestion] R2-6 (2 of 4 — dead public surface pattern): setLineage — the ONLY writer of startMode: 'rerun' — has zero production callers; the entire 'rerun' lineage mode is unreachable. A repo-wide grep matches only the definition and the two assertions in the unit test this PR adds; the runner stamps only 'retry', inline at register time — so no persisted or live run can ever carry startMode: 'rerun', even though the snapshot validator was extended to accept it and WorkflowRunStartMode advertises it. Cost: shipped dead public API + enum member + validator tolerance, and the passing 'records rerun lineage' test gives the illusion the rerun flow is wired end-to-end. — Suggested fix: wire the rerun path that calls setLineage in the slice that needs it, or drop the method, the 'rerun' member, and the validator arm (and the test) until the rerun flow lands.
中文说明
[Suggestion] R2-6（共 4 处之 2——死的公开表面模式）：setLineage——startMode: 'rerun' 的唯一写入方——没有任何生产调用方；整个 'rerun' 血缘模式不可达。全仓库 grep 只命中定义与本 PR 新增单测中的两处断言；runner 只在 register 时内联写入 'retry'——因此没有任何持久化或运行中的条目会带 startMode: 'rerun'，尽管快照校验器已扩展为接受它、WorkflowRunStartMode 也声明了它。代价： shipped 的死公开 API + 枚举成员 + 校验器容忍分支，且通过的 'records rerun lineage' 测试造成 rerun 流程已端到端接通的假象。建议修复：在需要 rerun 的切片中接入调用 setLineage 的路径，或在 rerun 流程落地前移除该方法、'rerun' 成员与校验器分支（及测试）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Declined: same rationale as the companion thread — the maintainer's F8 accepts the staged lineage surface for this split slice. setLineage and the 'rerun' mode are staged for the rerun flow; removing them (method + enum member + validator arm + test) now would be undone by that slice.
中文说明
拒绝：与相邻线程同一理由——维护者 F8 接受本拆分切片预留的血缘表面。setLineage 与 'rerun' 模式为 rerun 流程预留；现在移除（方法 + 枚举成员 + 校验分支 + 测试）会被该切片重新加回。
| /** Original structured arguments, retained so a failed run can resume the same journal prefix. */ | ||
| args?: unknown; |
There was a problem hiding this comment.
[Suggestion] R2-6 (3 of 4 — dead public surface pattern): the new WorkflowTask.args field is written at registration (workflow-runner.ts:137), has zero production read sites, is not persisted in toSnapshot — and the purpose its doc comment claims is implemented by a different mechanism. — Failure scenario: resume never consults entry.args: retry passes args: this.params.args — the NEW call's args — with resumeFromRunId (workflow.ts:221), and the journal prefix is seeded from exactly that (deriveArgsSeed(req.args), workflow-orchestrator.ts:1419, whose own comment says the seed exists "so a resume with different args produces a disjoint key space"). The only reader anywhere is the retention test. Cost: arbitrary-size user JSON retained per entry until eviction with no consumer, and the comment would mislead a future rerun implementation into trusting a field whose stated mechanism is false — a maintainer who omits re-passing args gets a silent 100% cache-miss live re-run. — Suggested fix: drop the field (retry already threads args explicitly), or fix the doc comment to say what is true (informational only; resume re-seeds from the caller's new args).
中文说明
[Suggestion] R2-6（共 4 处之 3——死的公开表面模式）：新增的 WorkflowTask.args 字段在 register 时写入（workflow-runner.ts:137），没有任何生产读取方，也不被 toSnapshot 持久化——其文档注释声称的用途实际由另一机制实现。失败场景：恢复从不读取 entry.args：retry 携带 resumeFromRunId 时传入的是新调用的 args（args: this.params.args，workflow.ts:221），journal 前缀正是用它播种的（deriveArgsSeed(req.args)，workflow-orchestrator.ts:1419，其注释写明播种是为了「不同 args 的恢复产生不相交的键空间」）。全仓库唯一读取方是保留性测试。代价：任意大小的用户 JSON 被每个条目持有直到驱逐而无消费方；注释会误导未来的 rerun 实现去信任一个所述机制为假的字段——维护者若不再重新传入 args，会得到一次静默的 100% 缓存未命中 live 重跑。建议修复：移除该字段（retry 已显式传递 args），或修正文档注释说明事实（仅信息用途；恢复由调用方的新 args 重新播种）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round (not covered by the maintainer's F8 acceptance). Decision then: fix the doc comment to state what is true (informational only; resume re-seeds the journal prefix from the caller's new args via deriveArgsSeed) or drop WorkflowTask.args, since retry already threads args explicitly.
中文说明
延后至下一轮（不在维护者 F8 认可范围之列）。届时决策：修正文档注释说明事实（仅信息用途；恢复由调用方的新 args 经 deriveArgsSeed 重新播种 journal 前缀），或移除 WorkflowTask.args——retry 已显式传递 args。
| export interface WorkflowToolResult extends ToolResult { | ||
| /** Exact run started by a successfully admitted background invocation. */ | ||
| workflowRunId?: string; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-6 (4 of 4 — dead public surface pattern): workflowRunId is populated on every admitted background invocation (workflow.ts:244) and exported from core's public API, but has zero production read sites — a repo-wide grep finds exactly three hits: the declaration, the set site, and the unit test. The result→run association the field advertises (tracking or cancelling the backgrounded run from its tool call, as the agent/monitor correlation plumbing does) is wired nowhere, and the test blesses the dead behavior so its absence will never surface. Distinct from the open toolUseId thread (a different field). — Suggested fix: wire the field to a real consumer (e.g. the background-task view / notification correlation) or drop it from the result and the public export until a consumer exists.
中文说明
[Suggestion] R2-6（共 4 处之 4——死的公开表面模式）：workflowRunId 在每个被接受的后台调用上填充（workflow.ts:244）并从 core 公开 API 导出，但没有任何生产读取方——全仓库 grep 恰好三处命中：声明、写入点与单测。该字段宣称的 结果→运行 关联能力（像 agent/monitor 关联接线那样，从工具调用追踪或取消后台运行）没有任何接线，测试还为这一死行为背书，其缺失永远不会暴露。与既有的 toolUseId 评论是不同字段。建议修复：把字段接到真实消费方（如后台任务视图/通知关联），或在消费方存在之前从结果与公开导出中移除。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Declined: the maintainer's F8 explicitly notes WorkflowToolResult.workflowRunId is populated but not consumed as expected for a split slice. The result→run correlation consumer belongs to the follow-up slice (background-task view / notification correlation); dropping the field now would be undone there.
中文说明
拒绝：维护者 F8 明确指出 WorkflowToolResult.workflowRunId 只写不读是拆分切片的预期状态。结果→运行关联的消费方属于后续切片（后台任务视图/通知关联）；现在移除该字段会在后续切片被重新加回。
| const parent = dependencyContext.getStore(); | ||
| const inheritedTails = parent?.tails ?? []; | ||
| const branches = thunks.map((thunk) => { |
There was a problem hiding this comment.
[Suggestion] R2-2: The fan-out store/merge block (~20 lines: inherited-tails capture, per-branch store construction with dependencyContext.run, settleToNullArray(...).then(merge)) is duplicated verbatim between makeParallelImpl (~line 1880) and makePipelineImpl (~line 1944), differing only in the thunk body and the 'pipeline' kind argument — mergeFanoutTails was extracted, but its entire call-site scaffolding was copy-pasted. — Failure scenario: any future change to fan-out dependency semantics (the abort path where the .then merge is currently skipped, or the branches.length > 0 guard) must be applied in two places; fixing one and missing the other makes parallel() and pipeline() produce different dependsOn graphs for identical shapes — the existing it.each cross-checks only the already-tested shapes, so a one-sided edit to an uncovered shape ships. — Suggested fix: extract a shared helper, e.g. runFanout(dependencyContext, signal, kind, items, makeThunk), owning store creation and the post-settle mergeFanoutTails write-back; each impl passes only its per-item thunk builder.
中文说明
[Suggestion] fan-out 的 store/merge 代码块（约 20 行：继承 tails 捕获、用 dependencyContext.run 构造每个分支的 store、settleToNullArray(...).then(merge)）在 makeParallelImpl（约 1880 行）与 makePipelineImpl（约 1944 行）之间逐字重复，仅 thunk 主体与 'pipeline' kind 参数不同——mergeFanoutTails 被抽取了，但其整个调用点脚手架被复制粘贴。失败场景：未来任何对 fan-out 依赖语义的修改（当前跳过 .then merge 的 abort 路径、或 branches.length > 0 守卫）都必须两处同步；改一处漏一处会使 parallel() 与 pipeline() 对相同形态产生不同的 dependsOn 图——现有 it.each 只交叉检查已测形态，对未覆盖形态的单边修改会直接 shipped。建议修复：抽取共享辅助函数，如 runFanout(dependencyContext, signal, kind, items, makeThunk)，统一持有 store 创建与落定后的 mergeFanoutTails 回写；每个实现只传入按条目构造 thunk 的函数。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round: a shared runFanout(dependencyContext, signal, kind, items, makeThunk) extraction is a pure refactor of the duplicated store/merge scaffolding; scheduled after the fan-out tails semantics settle (the R2-18 fix landed this round).
中文说明
延后至下一轮：抽取共享的 runFanout(dependencyContext, signal, kind, items, makeThunk) 是对重复 store/merge 脚手架的纯重构；安排在 fan-out tails 语义稳定之后（R2-18 修复已在本轮落地）。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at this commit and its suite did not run locally in this review (the macOS/Windows unit-test matrix jobs were also skipped; those suites ran on Linux, in CI and locally).
Not explored to full depth (tool budget reached): "This PR (QwenLM/qwen-code #9034) adds a structured,…": could not run unit tests/typecheck (worktree has no node_modules; skipped npm install to stay within budget).; "This PR (QwenLM/qwen-code #9034) adds a structured,…": could not run npm run typecheck / the workflow unit tests — the review worktree has no node_modules and installing was out of budget; compilation-level enfo…; "You are review agent reverse-audit — Reverse audit agent…": did not execute the vitest suite — verification was static (reads/greps against the reviewed commit) only.; "You are review agent reverse-audit — Reverse audit agent…": did not re-walk deriveArgsSeed / deriveAgentKey journal-key internals (adjacent to R3-6's chunk) beyond confirming the prefix-chain call-order argument in com…; "You are review agent reverse-audit — Reverse audit agent…": did not re-walk WorkflowDispatchScheduler internals for paused→resumed thunk timing beyond confirming dispatchStarted fires at slot-acquire., and 5 more.
Test Plan (not a blocker): 466 tests passed — this review observed 19354, 19829, 1285, 1511, 494, 3393, 555 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at this commit and its suite did not run locally in this review (the macOS/Windows unit-test matrix jobs were also skipped; those suites ran on Linux, in CI and locally)。
未探索到全部深度（达到工具调用预算）："This PR (QwenLM/qwen-code #9034) adds a structured,…"：could not run unit tests/typecheck (worktree has no node_modules; skipped npm install to stay within budget).；"This PR (QwenLM/qwen-code #9034) adds a structured,…"：could not run npm run typecheck / the workflow unit tests — the review worktree has no node_modules and installing was out of budget; compilation-level enfo…；"You are review agent reverse-audit — Reverse audit agent…"：did not execute the vitest suite — verification was static (reads/greps against the reviewed commit) only.；"You are review agent reverse-audit — Reverse audit agent…"：did not re-walk deriveArgsSeed / deriveAgentKey journal-key internals (adjacent to R3-6's chunk) beyond confirming the prefix-chain call-order argument in com…；"You are review agent reverse-audit — Reverse audit agent…"：did not re-walk WorkflowDispatchScheduler internals for paused→resumed thunk timing beyond confirming dispatchStarted fires at slot-acquire.，另有 5 条。
Test Plan（非阻断）：466 tests passed — this review observed 19354, 19829, 1285, 1511, 494, 3393, 555 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| onLogAppended(runId: string, line: string, at = Date.now()): void { | ||
| const entry = this.entries.get(runId); | ||
| if (!entry || !isActiveWorkflowStatus(entry.status)) return; | ||
| const message = stripAnsiAndControl(line); |
There was a problem hiding this comment.
[Suggestion] R3-2: Journal-bound log lines are sanitized but never length-truncated — unlike every sibling script-derived string this diff persists (label 200 / prompt 4,096 / dispatch error 4,096). The sandbox caps line count (10,000) but not line length, and toSnapshot persists events verbatim. — Failure scenario: a script logging long lines (log(JSON.stringify(hugeObject)) in a loop) leaves up to ~100 copies of unbounded length in recentLogs and the persisted events ledger — N MB lines produce an N×100 MB snapshot write and matching heap pressure, where the same content through dispatch.error would be capped at 4 KB. Issue #9032 AC3 requires bounded persisted logs. Probe-confirmed with a 1,000,000-char line; the cap flips the probe.
| const message = stripAnsiAndControl(line); | |
| const message = stripAnsiAndControl(line).slice(0, 4_096); |
中文说明
[Suggestion] R3-2：进入 journal 的日志行做了清洗，但没有长度截断——与本 diff 持久化的所有姊妹脚本派生字符串不同（label 200 / prompt 4,096 / dispatch error 4,096）。sandbox 只限制行数（10,000），不限制单行长度，而 toSnapshot 会逐字持久化 events。— 失败场景：脚本循环打印长行（如 log(JSON.stringify(hugeObject))）会在 recentLogs 与持久化 events 账本中留下最多约 100 条不限长度的副本——N MB 的行会产生 N×100 MB 的快照写入与相应堆压力，而同样内容经 dispatch.error 会被限制在 4 KB。Issue #9032 AC3 要求持久化日志有界。已用 1,000,000 字符的行探针确认；加上限后探针翻转。
建议修复：与姊妹字段一致地截断，例如 .slice(0, 4_096)（或引入共享的命名常量）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| entry.currentPhase = title; | ||
| const last = entry.phases[entry.phases.length - 1]; |
There was a problem hiding this comment.
[Suggestion] R3-5: Script-derived phase titles enter entry.currentPhase, entry.phases, entry.phaseVisits[].title, and the persisted phase-started events with no stripAnsiAndControl and no length cap — safePhase caps entry count (10,000) only, and toSnapshot persists each title three times (phases, phaseVisits, events). — Failure scenario: a script alternating two large titles (phase(JSON.stringify(hugeObj)) in an A/B loop defeats the consecutive-duplicate collapse) up to 10,000 entries produces multi-GB snapshot writes from MB-scale titles; raw ANSI/control sequences in titles also reach the TUI pill/dialog phase rendering unstripped (BackgroundTasksDialog and workflowsCommand render entry.currentPhase raw). Probe-confirmed with alternating 50 KB ANSI-bearing titles; normalizing at the top of onPhaseStarted flips the probe.
Fix:
onPhaseStarted(runId: string, rawTitle: string, at = Date.now()): void {
const entry = this.entries.get(runId);
if (!entry || !isActiveWorkflowStatus(entry.status)) return;
const title = stripAnsiAndControl(rawTitle).slice(0, 200) || 'phase';
// ...use `title` for currentPhase / phases / the visit / the event中文说明
[Suggestion] R3-5：脚本派生的 phase 标题在写入 entry.currentPhase、entry.phases、entry.phaseVisits[].title 以及持久化的 phase-started 事件时，既没有 stripAnsiAndControl 也没有长度上限——safePhase 只限制条目数量（10,000），而 toSnapshot 会把每个标题持久化三份（phases、phaseVisits、events）。— 失败场景：脚本以两个大标题交替（phase(JSON.stringify(hugeObj)) 的 A/B 循环可绕过相邻去重）最多 10,000 条时，MB 级标题会产生数 GB 的快照写入；标题中的原始 ANSI/控制序列还会未经清洗直达 TUI 胶囊/对话框的 phase 渲染（BackgroundTasksDialog 与 workflowsCommand 原样渲染 entry.currentPhase）。已用 50 KB 含 ANSI 的交替标题探针确认；在 onPhaseStarted 顶部归一化后探针翻转。
建议修复：在 onPhaseStarted 顶部统一归一化，例如 const title = stripAnsiAndControl(rawTitle).slice(0, 200) || 'phase';，并用于 currentPhase/phases/visit/事件。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ).then((result) => { | ||
| if (parent && branches.length > 0) { | ||
| parent.tails = mergeFanoutTails( |
There was a problem hiding this comment.
[Suggestion] R3-8: Dependency edges are silently dropped when a tails write lands in a branch store after the fan-out's settle-time merge has already run — this settle-time parent.tails = mergeFanoutTails(...) is the only branch→parent propagation, and nothing forwards later writes to the detached store. Two traced/probed triggers: (1) a nested fan-out created inside a branch whose promise is awaited only after the branch settled merges its tails into the detached store; (2) an escaped continuation (late = gate.then(() => agent('late')) registered in a branch, firing after settle) whose store.tails = [id] write (issueDispatchTrace, ~line 1394) lands in the detached store. The identical merge shape exists in makePipelineImpl (~line 1960). — Failure scenario: script let inner; await parallel([async () => { inner = parallel([() => agent('inner', {label:'inner'})]); }]); await inner; await agent('after', {label:'after'}); — probe against the real orchestrator: 'after' receives dependsOn: [] even though the script explicitly awaited inner first; the persisted snapshot graph and dispatch-queued events omit a real data dependency. No functional consumer of dependsOn exists today, so impact is confined to the persisted trace. The fix-direction probe flipped the result and kept all 139 orchestrator tests green.
Suggested fix: give branch stores a parent link when merged and forward late writes/merges up the chain; at minimum, add a test pinning the intended semantics for these shapes.
中文说明
[Suggestion] R3-8：当 tails 写入发生在 fan-out 的落定合并已经执行之后，依赖边会被静默丢弃——落定时的 parent.tails = mergeFanoutTails(...) 是唯一的分支→父级传播途径，之后对已分离 store 的写入没有任何转发。两个经追踪/探针确认的触发形态：(1) 在分支内创建、且其 promise 在分支落定之后才被 await 的嵌套 fan-out，会把 tails 合并进已分离的 store；(2) 逃逸续体（在分支内注册、落定后才触发的 late = gate.then(() => agent('late'))），其 store.tails = [id] 写入（issueDispatchTrace，约 1394 行）落在已分离的 store 上。makePipelineImpl（约 1960 行）存在完全相同的合并形状。— 失败场景：脚本 let inner; await parallel([async () => { inner = parallel([() => agent('inner', {label:'inner'})]); }]); await inner; await agent('after', {label:'after'}); —— 对真实 orchestrator 探针：尽管脚本显式先 await 了 inner，'after' 的 dependsOn 仍为 []；持久化快照图与 dispatch-queued 事件丢失了真实数据依赖。目前 dependsOn 没有功能性消费者，影响限于持久化 trace。修复方向的探针使结果翻转，且 139 个 orchestrator 测试全部保持绿色。
建议修复：在分支 store 被合并时赋予父级链接，并把后续写入/合并沿链转发；至少补充测试钉住这些形态的预期语义。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round. Forwarding late writes/merges up from detached branch stores (parent-link on merge) is a design change to the dependency-store lifecycle; planned together with the fan-out merge corner tests (R6-6 thread).
中文说明
延后至下一轮。把迟到写入/合并从已分离的分支 store 沿父级链接向上传递，属于依赖 store 生命周期的设计改动；计划与 fan-out 合并角落测试（R6-6 线程）一起处理。
| const id = `dispatch-${(dispatchTraceCount += 1)}`; | ||
| const store = dependencyContext.getStore(); | ||
| const dependsOn = Array.from(new Set(store?.tails ?? [])); |
There was a problem hiding this comment.
[Suggestion] R3-9: Raw promise combinators over agent() (Promise.all/race/allSettled) silently drop dependency edges: every issueDispatchTrace overwrites the shared store's tails with ONLY the just-issued id, so concurrency expressed without parallel()/pipeline() (which get per-branch stores) records only the last-issued dispatch as the tail — and even records a spurious serial edge between the concurrent dispatches. — Failure scenario: await Promise.all([agent('a', {label:'a'}), agent('b', {label:'b'})]); await agent('c', {label:'c'}); — probe-observed: c persists with dependsOn: ['b'] only (the real a edge absent) plus a spurious b→a edge; the identical script via parallel([...]) correctly gives c: ['a','b']. Impact is confined to the persisted trace (no functional consumer of dependsOn today).
Suggested fix: document the limitation where dependsOn is produced (only parallel()/pipeline() preserve full graph edges), or pin the intended semantics with a test.
中文说明
[Suggestion] R3-9：对 agent() 使用原生 promise 组合器（Promise.all/race/allSettled）会静默丢失依赖边：每次 issueDispatchTrace 都会把共享 store 的 tails 覆写为刚发出的那一个 id，因此不走 parallel()/pipeline()（它们有每分支独立 store）的并发，只会把最后发出的 dispatch 记为 tail——甚至会在并发 dispatch 之间记录一条虚假的串行边。— 失败场景：await Promise.all([agent('a', {label:'a'}), agent('b', {label:'b'})]); await agent('c', {label:'c'}); —— 探针观测：c 持久化的 dependsOn 只有 ['b']（真实的 a 边缺失），还多出一条虚假的 b→a 边；同样的脚本经 parallel([...]) 则正确得到 c: ['a','b']。影响限于持久化 trace（目前 dependsOn 无功能性消费者）。
建议修复：在 dependsOn 的产生处记录该限制（只有 parallel()/pipeline() 保留完整图边），或用测试钉住预期语义。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round. Planned: document at the dependsOn production site (issueDispatchTrace) that only parallel()/pipeline() preserve full graph edges — raw Promise.all/race/allSettled record only the last-issued tail — and pin the intended semantics with a test.
中文说明
延后至下一轮。计划：在 dependsOn 产生处（issueDispatchTrace）注明只有 parallel()/pipeline() 保留完整图边——原生 Promise.all/race/allSettled 只记录最后发出的 tail——并用测试钉住预期语义。
| await writeWorkflowSnapshot(config, entry); | ||
| await journal?.drain(); |
There was a problem hiding this comment.
[Suggestion] R3-10: releaseHandle(runId, handle) runs only after await writeWorkflowSnapshot(...) and this await journal?.drain(), while registry.complete()/fail() already fired the status change and the background completion notification at settlement — so a same-runId resume issued during the snapshot+drain window fails spuriously with Workflow run <runId> is already active. (register() checks this.handles.has(runId) even for terminal entries). — Failure scenario: a backgrounded run completes; the model receives the completion notification and calls Workflow({resumeFromRunId}) → register() throws 'already active'. Probe-confirmed: at the moment status reads completed the handle is still attached, and the same resume succeeds once the journal writes unblock. The window is milliseconds on local disk but unbounded when journal writes stall — the PR's own freeze test models exactly that. (Distinct from the round-1 drain-ordering fix: the snapshot/telemetry projections remain frozen before these awaits.)
Fix (probe-verified direction):
registry?.releaseHandle(runId, handle);
await writeWorkflowSnapshot(config, entry);
await journal?.drain();or gate register()'s duplicate check on entry status instead of handle presence.
中文说明
[Suggestion] R3-10：releaseHandle(runId, handle) 只在 await writeWorkflowSnapshot(...) 与这个 await journal?.drain() 之后执行，而 registry.complete()/fail() 在落定时已经触发了状态变更与后台完成通知——因此在 snapshot+drain 窗口内发起的同 runId 续跑会误报 Workflow run <runId> is already active.（register() 对已终态条目仍检查 this.handles.has(runId)）。— 失败场景：后台运行完成；模型收到完成通知后调用 Workflow({resumeFromRunId}) → register() 抛出 'already active'。探针确认：状态读数为 completed 的时刻 handle 仍然挂载，journal 写入解除阻塞后同一个续跑即可成功。该窗口在本地磁盘上只有几毫秒，但 journal 写入停滞时无上限——本 PR 自己的 freeze 测试恰好模拟了这种情况。（与第 1 轮的 drain 顺序修复不同：快照/遥测投影在这些 await 之前仍是冻结的。）
建议修复（探针验证过的方向）：把 releaseHandle 移到 snapshot/drain 的 await 之前；或让 register() 的重复检查以条目状态而非 handle 存在性为准。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Declined THIS fix direction with evidence: moving releaseHandle before await writeWorkflowSnapshot / await journal?.drain() lets a same-runId retry call journal.load() while the old run's drain is still flushing the SAME journal file — the retry resumes from a partial prefix and re-dispatches already-completed agents, the exact hazard await journal?.drain() was added to prevent (see the R2-5 thread). The current spurious 'already active' rejection is transient and retryable; a safe fix needs a drain-state-aware register() gate, which is larger than this round. Leaving the thread open for that redesign.
中文说明
以证据拒绝此修复方向：把 releaseHandle 移到 await writeWorkflowSnapshot / await journal?.drain() 之前，会让同 runId 的重试在旧运行 drain 仍在写同一 journal 文件时调用 journal.load()——重试从不完整前缀恢复并重跑已完成的 agent，恰是 await journal?.drain() 存在所要防止的危害（见 R2-5 线程）。当前误报的 'already active' 拒绝是瞬时且可重试的；安全修复需要 register() 感知 drain 状态的门控，超出本轮范围。线程保持打开，等待该重设计。
| t.phases.push('Mutated'); | ||
| t.events[0]!.at = 0; | ||
| expect(s.phases).toEqual(['Plan', 'Build']); |
There was a problem hiding this comment.
[Suggestion] R3-12: The toSnapshot decoupling test covers only phases and events; the defensive copies this PR adds for dispatches (including the nested dependsOn: [...dispatch.dependsOn] spread) and phaseVisits have zero mutation coverage — the factory seeds both as [] and no test populates them. — Failure scenario: a future edit degrading toSnapshot to alias the live arrays (dispatches: task.dispatches, or dropping the dependsOn spread) keeps the entire suite green; because writeWorkflowSnapshot serializes after await fs.mkdir, a late dispatch mutation across that yield then lands in the persisted file — the same late-mutation hazard the sibling freeze test documents (and late post-settlement dispatch settlement is a real shape — see the freeze test's own construction).
Suggested fix: seed task() with one dispatch (including a dependsOn entry) and one phase visit; after toSnapshot, mutate t.dispatches[0]!.status, push into t.dispatches[0]!.dependsOn, and mutate t.phaseVisits[0]!.title, asserting the snapshot is unchanged.
中文说明
[Suggestion] R3-12：toSnapshot 的解耦测试只覆盖了 phases 与 events；本 PR 为 dispatches（包括嵌套的 dependsOn: [...dispatch.dependsOn] 展开）和 phaseVisits 新增的防御性拷贝没有任何变异覆盖——工厂函数把两者都初始化为 []，且没有测试填充它们。— 失败场景：未来某次修改把 toSnapshot 退化为直接引用活数组（dispatches: task.dispatches，或丢掉 dependsOn 展开），整个测试套件仍为绿色；由于 writeWorkflowSnapshot 在 await fs.mkdir 之后才序列化，跨越该让出点的迟到 dispatch 变异会落入持久化文件——这正是姊妹 freeze 测试所记录的那种迟到变异风险（落定后迟到的 dispatch 落定是真实存在的形态，见 freeze 测试自身的构造）。
建议修复：给 task() 预置一个 dispatch（含 dependsOn 条目）和一个 phase visit；在 toSnapshot 之后修改 t.dispatches[0]!.status、向 t.dispatches[0]!.dependsOn push、修改 t.phaseVisits[0]!.title，断言快照不受影响。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round. Still planned: seed task() with one dispatch (including a dependsOn entry) and one phase visit; after toSnapshot, mutate t.dispatches[0]!.status, push into t.dispatches[0]!.dependsOn, and mutate t.phaseVisits[0]!.title, asserting the snapshot is unchanged.
中文说明
延后至下一轮。仍计划：给 task() 预置一个 dispatch（含 dependsOn 条目）与一个 phase visit；在 toSnapshot 之后修改 t.dispatches[0]!.status、向 t.dispatches[0]!.dependsOn push、修改 t.phaseVisits[0]!.title，断言快照不受影响。
| const index = entry.phaseVisits.length; | ||
| const visit: WorkflowPhaseVisit = { | ||
| id: `phase-${index + 1}`, |
There was a problem hiding this comment.
[Suggestion] R3-3: Phase revisit — the stated reason phaseVisits exists alongside phases (each revisit gets a fresh stable id) — is untested; all 13 onPhaseStarted calls in the suite are forward-only. — Failure scenario: a script re-entering an earlier phase (phase('Plan'); phase('Build'); phase('Plan')) must open a fresh visit with a new id and close the prior one. Mutation-tested: a mutant that reuses the first visit whose title matches (instead of always appending a new visit) survives the full 104-test suite, and an A→B→A probe fails against it (the revisit silently re-points currentPhaseVisitId at phase-1) — dispatches in the revisited phase would then attach to the wrong phaseVisitId in the persisted replay graph.
Suggested fix: add a registry test running A → B → A asserting two distinct phase-N visits for A with correct startedAt/endedAt and matching phase-started/phase-completed events.
中文说明
[Suggestion] R3-3：phase 重访——phaseVisits 与 phases 并存的设计理由（每次重访都有新的稳定 id）——没有测试覆盖；套件中全部 13 次 onPhaseStarted 调用都是单向递进的。— 失败场景：脚本重入早前的 phase（phase('Plan'); phase('Build'); phase('Plan')）时，应开启带新 id 的新 visit 并关闭前一个。变异测试：一个「复用首个标题匹配的 visit」（而非总是追加新 visit）的变异体在整个 104 项测试套件中存活，A→B→A 探针对该变异体失败（重访会静默把 currentPhaseVisitId 指回 phase-1）——重访 phase 中的 dispatch 会在持久化回放图中挂到错误的 phaseVisitId 上。
建议修复：新增 registry 测试执行 A → B → A，断言 A 有两个不同的 phase-N visit，startedAt/endedAt 正确，且配套 phase-started/phase-completed 事件。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round. Still planned: a registry test running A → B → A asserting two distinct phase-N visits for A with correct startedAt/endedAt and matching phase-started/phase-completed events.
中文说明
延后至下一轮。仍计划：registry 测试执行 A → B → A，断言 A 有两个不同的 phase-N visit，startedAt/endedAt 正确，且配套 phase-started/phase-completed 事件。
已被后续 commit 9778f9e 取代，当前 head 需重新 review。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at this commit and its suite did not run locally in this review.
Not explored to full depth (tool budget reached): "This PR (QwenLM/qwen-code #9034) adds a structured…": didn't re-verify whether onBudgetUpdated also overwrites tokenBudgetTotal on the stale path (per the read at ~lines 1040-1042 it sets entry.tokenBudgetTota…; "This PR (QwenLM/qwen-code #9034) adds a structured…": did not run an empirical probe for the Finding 1 race (code-trace evidence only); did not separately re-verify onBudgetUpdated 's tokenBudgetTotal overwrite …; "This PR (QwenLM/qwen-code #9034) adds a structured…": I did not execute the two new runner tests under vitest — I verified their assertions against the implementation source line-by-line instead (mock lifecycle inc…; "This PR (QwenLM/qwen-code #9034) adds a structured…": none — all checks above completed within budget.; "This PR (QwenLM/qwen-code #9034) adds a structured…": none — all checks I started were completed within budget., and 8 more.
Test Plan (not a blocker): 466 tests passed — this review observed 19831, 1285, 1511, 494, 3393, 555 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI at this commit and its suite did not run locally in this review。
未探索到全部深度（达到工具调用预算）："This PR (QwenLM/qwen-code #9034) adds a structured…"：didn't re-verify whether onBudgetUpdated also overwrites tokenBudgetTotal on the stale path (per the read at ~lines 1040-1042 it sets entry.tokenBudgetTota…；"This PR (QwenLM/qwen-code #9034) adds a structured…"：did not run an empirical probe for the Finding 1 race (code-trace evidence only); did not separately re-verify onBudgetUpdated 's tokenBudgetTotal overwrite …；"This PR (QwenLM/qwen-code #9034) adds a structured…"：I did not execute the two new runner tests under vitest — I verified their assertions against the implementation source line-by-line instead (mock lifecycle inc…；"This PR (QwenLM/qwen-code #9034) adds a structured…"：none — all checks above completed within budget.；"This PR (QwenLM/qwen-code #9034) adds a structured…"：none — all checks I started were completed within budget.，另有 8 条。
Test Plan（非阻断）：466 tests passed — this review observed 19831, 1285, 1511, 494, 3393, 555 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| phaseVisitId: entry.currentPhaseVisitId, | ||
| label: | ||
| stripAnsiAndControl(event.label ?? '').slice(0, 200) || fallbackLabel, | ||
| prompt: stripAnsiAndControl(event.prompt).slice(0, 4_096), |
There was a problem hiding this comment.
[Suggestion] The new input-clamping branches have zero test coverage: label truncation to 200, prompt truncation to 4,096, the fallback label for empty labels, and the dependsOn filter dropping ids not present in entry.dispatches (the matching dispatch-error truncation in onDispatchSettled too). All existing registry tests feed short, valid inputs. Probe-verified: a mutation removing the prompt cap and the dependsOn filter ships green (86/86), while a probe asserting the clamped lengths flips. — Failure scenario: workflow scripts are user-authored and agent(prompt) accepts arbitrary strings → if the .slice(0, 4_096) cap or the dependsOn filter regresses, multi-megabyte prompts are stored verbatim and serialized into every snapshot, or the replay graph persists edges to nonexistent dispatch ids — shipping green with the current suite.
Add one registry test next to the other lifecycle tests:
it("clamps queued dispatch label/prompt and filters unknown dependsOn ids", () => {
registry.onDispatchQueued(runId, {
id: "dispatch-9",
prompt: "p".repeat(5_000),
label: "l".repeat(250),
dependsOn: ["missing"],
queuedAt: 1_000,
});
const trace = registry.get(runId)!.dispatches.at(-1)!;
expect(trace.prompt).toHaveLength(4_096);
expect(trace.label).toHaveLength(200);
expect(trace.dependsOn).toEqual([]);
});中文说明
[Suggestion] 新增的输入钳制分支零测试覆盖：label 截断至 200、prompt 截断至 4,096、空 label 的回退名称，以及丢弃不存在于 entry.dispatches 的 id 的 dependsOn 过滤器（onDispatchSettled 中对应的 dispatch error 截断同样如此）。现有 registry 测试只喂入短小、合法的输入。探针验证：移除 prompt 上限与 dependsOn 过滤器的变异可以保持整套测试绿色（86/86），而断言钳制后长度的探针会翻转。— 失败场景：workflow 脚本由用户编写且 agent(prompt) 接受任意字符串 → 若 .slice(0, 4_096) 上限或 dependsOn 过滤器回归，数 MB 的 prompt 会被逐字存入并序列化进每份快照，或回放图持久化指向不存在 dispatch id 的边——在当前测试套件下全部为绿。建议按上方示例补充一个 registry 测试。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round. Still planned: one registry test covering label truncation to 200, prompt truncation to 4,096, the empty-label fallback, the dependsOn filter dropping unknown ids, and the sibling dispatch-error truncation in onDispatchSettled.
中文说明
延后至下一轮。仍计划：一个 registry 测试覆盖 label 截断至 200、prompt 截断至 4,096、空 label 回退、丢弃未知 id 的 dependsOn 过滤器，以及 onDispatchSettled 中对应的 dispatch error 截断。
| /** Dispatch-level execution graph for live UI consumers. */ | ||
| dispatches: WorkflowDispatchTrace[]; |
There was a problem hiding this comment.
[Suggestion] dispatches and events have no retention cap — unlike every sibling structure (recentLogs 100, sandbox logs/phases 10k, pending approvals 32, prompt 4KB, terminal entries 10, snapshots 30) — and the orchestrator's cache-hit path re-materializes the prior run's full graph outside the agent cap (it returns before agentCount += 1). Probe with 20,000 dispatches through the real registry: dispatches=20000, events=60001; toSnapshot + JSON.stringify(…, null, 2) = 22.87 MB at 512-char prompts (~4× more at the 4KB prompt cap) paid in one synchronous burst at settlement; building the array also cost 8.2–10.9 s of synchronous CPU in the registry's O(n) dedup some(...) / find(...) scans over the uncapped array. listWorkflowSnapshots (production-called from workflowsCommand list/rerun/retry) then parses and deep-validates every retained snapshot (≤30). With HARD_MAX_AGENTS_PER_RUN_CEILING (10k) a resumed run can hold ~20k traces + ~60k events; defaults bound this to ~2k traces, so this is a scaling gap, not a default-config bug. — Failure scenario: a resumed run under the 10k agent ceiling replays the full prior graph as cached traces on top of up to a full cap of live dispatches → tens of MB per entry deep-copied into the settlement snapshot, with seconds of synchronous event-loop blocking during construction and again on every listWorkflowSnapshots validation pass.
Cap dispatches (and the non-log portion of events) the way phases/logs are capped — e.g. drop oldest settled traces while keeping event ids valid — or at minimum stop persisting the full prompt per trace beyond a retained tail; an id→dispatch Map alongside the array would also remove the O(n) scans measured above.
中文说明
[Suggestion] dispatches 与 events 没有保留上限——不同于所有同级结构（recentLogs 100、sandbox 日志/阶段 10k、待处理审批 32、prompt 4KB、终态条目 10、快照 30）——且 orchestrator 的缓存命中路径会在 agent 上限之外重建上一次运行的完整图（在 agentCount += 1 之前返回）。对真实 registry 注入 20,000 个 dispatch 的探针：dispatches=20000、events=60001；toSnapshot + JSON.stringify(…, null, 2) 在 512 字符 prompt 下为 22.87 MB（4KB prompt 上限下约 4 倍），在落定时以一次同步突发支付；构建数组还在 registry 对未封顶数组的 O(n) 去重 some(...)/find(...) 扫描上花费 8.2–10.9 秒同步 CPU。listWorkflowSnapshots（由 workflowsCommand 的 list/rerun/retry 调用）随后会解析并深度校验全部保留快照（≤30）。在 10k agent 上限下，一次续跑可持有约 2 万条 trace + 6 万个事件；默认配置约 2k 条 trace，因此这是扩展性缺口而非默认配置缺陷。— 失败场景：10k agent 上限下的续跑把先前完整图以 cached trace 重建、叠加满上限的 live dispatch → 每个条目数十 MB 被深拷贝进落定快照，构建期与每次 listWorkflowSnapshots 校验都有秒级同步事件循环阻塞。建议按上文为 dispatches（及 events 非日志部分）设置上限，或至少不要逐条持久化完整 prompt；并用 id→dispatch 的 Map 消除上述 O(n) 扫描。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to a dedicated round: capping dispatches/events retention (drop oldest settled traces while keeping event ids valid, or an id→dispatch Map removing the O(n) scans) is a scaling change that needs its own budget and mutation coverage, not a tail-end addition here.
中文说明
延后至专门一轮：为 dispatches/events 设置保留上限（在保持事件 id 有效的前提下驱逐最旧已落定 trace，或用 id→dispatch Map 消除 O(n) 扫描）是扩展性改动，需要独立预算与变异覆盖，不适合作为本轮收尾附加项。
| const firstLog = entry.events.findIndex((event) => event.type === 'log'); | ||
| if (firstLog >= 0) entry.events.splice(firstLog, 1); |
There was a problem hiding this comment.
[Suggestion] Once recentLogs reaches its 100 cap, every appended log line runs this findIndex from index 0 plus a mid-array splice — O(ledger size) per line, on the hot path whose doc comment promises no per-line cost. Phase/dispatch events at the head are never removed, and safePhase allows 10k distinct phases (~20k events ahead of the first log). Probe on unmodified code (10k log lines after P phases): P=500 → 112 ms; P=1000 → 175 ms; P=2000 → 308 ms; P=4000 → 628 ms — linear in the pre-log event count, extrapolating to ≈1.5 s of synchronous event-loop blocking at the full caps. A tracked-index fix flips this to ~8 ms flat (77× at P=4000), semantics-preserving with all 108 tests green. — Failure scenario: a phase-heavy workflow (10k phase() calls) that then logs to the 10k cap pays ~10k full-ledger scans plus splices — roughly a second and a half of main-thread blocking spread across the run, on the event loop that drives the TUI.
Track the first-log position incrementally instead of scanning from 0 — e.g. keep a count of the non-log events preceding the first log event (updated on append/splice), or hold the ledger's log lines in a dedicated capped array with a head pointer.
中文说明
[Suggestion] 一旦 recentLogs 达到 100 上限，之后每追加一行日志都会执行这个从下标 0 开始的 findIndex 加数组中部 splice——每行 O(账本规模)，恰好落在注释承诺"无逐行开销"的热路径上。头部的 phase/dispatch 事件永不移除，而 safePhase 允许 10k 个不同 phase（首个 log 之前可达约 2 万个事件）。对未修改代码的探针（P 个 phase 之后写 10k 行日志）：P=500 → 112 ms；P=1000 → 175 ms；P=2000 → 308 ms；P=4000 → 628 ms——与首个 log 前的事件数线性相关，外推到满上限约为 1.5 秒同步事件循环阻塞。改为跟踪首个 log 位置的修复可将其降至约 8 ms 平稳（P=4000 时 77 倍），语义不变且 108 个测试全部保持绿色。— 失败场景：phase 密集（10k 次 phase() 调用）随后又写满 10k 行日志的 workflow，要支付约 1 万次全账本扫描 + splice——运行期间累计约 1.5 秒主线程阻塞，发生在驱动 TUI 的事件循环上。建议增量跟踪首个 log 事件位置（如维护其前导非 log 事件计数，append/splice 时更新），或将账本日志行放入独立的带首指针的封顶数组。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Deferred to the next round. Planned: track the first-log ledger position incrementally (a counter of non-log events preceding the first log event, updated on append/splice) instead of scanning findIndex from 0 per line at the cap.
中文说明
延后至下一轮。计划：增量跟踪账本中首个 log 事件的位置（维护其前导非 log 事件计数，append/splice 时更新），替代达到上限后每行从 0 开始的 findIndex 扫描。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "This PR (QwenLM/qwen-code #9034) adds a structured,…": did not trace snapshot-load consumers ( /workflows dialog) to confirm the uncapped message is rendered/re-parsed per open (cost stated at the persistence level…; "This PR (QwenLM/qwen-code #9034) adds a structured,…": none — all checks I started were completed within budget (~9 of ~54 calls).; "This PR (QwenLM/qwen-code #9034) adds a structured,…": none — all checks I started were completed within budget.; "This PR (QwenLM/qwen-code #9034) adds a structured,…": none (≈15/49 calls used).; "This PR (QwenLM/qwen-code #9034) adds a structured,…": none — all checks I identified were completed within budget., and 4 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): 466 tests passed — this review observed 19572, 19929, 1364, 1539, 494, 3502, 559 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："This PR (QwenLM/qwen-code #9034) adds a structured,…"：did not trace snapshot-load consumers ( /workflows dialog) to confirm the uncapped message is rendered/re-parsed per open (cost stated at the persistence level…；"This PR (QwenLM/qwen-code #9034) adds a structured,…"：none — all checks I started were completed within budget (~9 of ~54 calls).；"This PR (QwenLM/qwen-code #9034) adds a structured,…"：none — all checks I started were completed within budget.；"This PR (QwenLM/qwen-code #9034) adds a structured,…"：none (≈15/49 calls used).；"This PR (QwenLM/qwen-code #9034) adds a structured,…"：none — all checks I identified were completed within budget.，另有 4 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：466 tests passed — this review observed 19572, 19929, 1364, 1539, 494, 3502, 559 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — packages/webui unit suite did not run (whole-call budget exhausted before it).
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): 466 tests passed — this review observed 20956, 20233, 1471, 1571, 494, 3655 passed.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/workflow-run-registry.ts:886 — [probe] cached-dispatch path (cached: true → dispatch-cached) has zero test coverage anywhere in the repopackages/core/src/agents/runtime/workflow-journal.ts:188 — [probe] serialized-append on-disk order is never asserted; the drain test only checks entry presence
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — packages/webui unit suite did not run (whole-call budget exhausted before it)。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
Test Plan（非阻断）：466 tests passed — this review observed 20956, 20233, 1471, 1571, 494, 3655 passed。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
已被后续 commit 取代，当前 head 需重新 review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 466 tests passed — this review observed 20956, 20236, 1471, 1571, 494, 3655, 504 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：466 tests passed — this review observed 20956, 20236, 1471, 1571, 494, 3655, 504 passed。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
@qwen-code /review |
|
@qwen-code /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，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the reverse-audit round cap of 5 without two consecutive dry rounds.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally in this review.
Test Plan (not a blocker): 466 tests passed — this review observed 20956, 20236, 1471 passed.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/runtime/workflow-sandbox.test.ts:708 — [probe] SEC-I2 test pins only the truncation marker; multi-line log-emission completeness/ordering unpinnedpackages/core/src/agents/runtime/workflow-sandbox.ts:762 — [probe] truncation marker never reaches the events ledger when overflow arrives via appendLog (notify=false)packages/core/src/agents/runtime/workflow-orchestrator.ts:2042 — [probe] aborted fan-out never merges branch dependency tails back into the parent storepackages/core/src/agents/runtime/workflow-orchestrator.ts:1721 — [probe] budget-refused dispatches are recorded in the execution graph as started+failedpackages/core/src/agents/workflow-run-registry.test.ts:104 — [probe] approval-requested dispatchId injection has zero test coverage (mutation survives)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the reverse-audit round cap of 5 without two consecutive dry rounds。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally in this review。
Test Plan（非阻断）：466 tests passed — this review observed 20956, 20236, 1471 passed。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…M#9034) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9034Commit: Feedback triageAll 56 inline threads were classified. The five Criticals (R1 drain ordering, R3-1 empty error, R3-4 settlement un-sanitize, R8-1 cancel asymmetry, R8-2 cross-run leak) were already fixed by earlier commits on this branch; each was re-verified against HEAD this round (code + covering tests present) and is listed in Implemented this round (8 findings, batch cap)
Declined (3 findings)
Deferred to the next round (30 threads)Replies with the concrete plan for each are in Deferred to the follow-up queue (
|
| 发现 | 修复 | 证据 |
|---|---|---|
[rc:3774951168] R2-18 —— 不发 dispatch 的 fan-out 分支把继承的 tails 重新注入，产生冗余传递性 dependsOn 边 |
mergeFanoutTails 现以继承集过滤分支 tails；当没有分支发过 dispatch 时保留透传（维持空 fan-out 契约） |
RED→GREEN：新泄漏测试在修复前失败；变异探针——审查者建议的无条件过滤会使新的透传测试失败 |
[rc:3773243564] —— 被取消运行的迟到日志行经 setRecentLogs 的 R7 许可进入 recentLogs，却进不了 events 账本 |
onLogAppended 与 setRecentLogs 对齐，同样接受 'cancelled'，两份投影保持一致 |
RED→GREEN；说明：迟到行按设计追加在 workflow-cancelled 之后（它们确实发生在取消之后） |
| [rc:3777184192] R3-2 —— 进入 journal 的日志行做了清洗但未按长度截断 | 在 recentLogs 的两个入口（onLogAppended 与 setRecentLogs）统一 .slice(0, 4_096)，与 label/prompt/error 上限一致 |
两个入口均 RED→GREEN |
| [rc:3777184216] R3-5 —— 脚本派生的 phase 标题未清洗、无上限地进入四处持久化表面 | onPhaseStarted 在 registry 边界一次性归一化：`stripAnsiAndControl(rawTitle).slice(0, 200) |
|
[rc:3773243555] —— snapshotToTask 在每次重启后重建时丢弃已持久化的 sourceRunId/startMode |
读回这两个字段（函数导出以便直接测试） | RED→GREEN |
[rc:3773243545] F7（维护者变异测试）—— recentLogs↔log 事件淘汰联动无测试 |
新 registry 测试：写入 150 行 → 两份投影恰好保留最后 100 行；非 log 事件幸存 | 钉住既有行为（修复前本就为绿） |
| [rc:3773243517] —— 穿越测试的金丝雀从不在删除路径上 | 金丝雀改为无扩展名的 CANARY，未设防的递归 rm 会真正删掉它 |
测试加固 |
[rc:3773243536] —— 旧快照加载测试仍保留 phaseVisits/dispatches/description |
测试现删除全部六个新字段键，真正验证旧格式容忍 | 测试加固 |
拒绝（3 条）
- [rc:3774951238] / [rc:3774951244] / [rc:3774951263]（R2-6 死公开表面：
clearStatusChangeCallback、setLineage/'rerun'、workflowRunId）：维护者本地验证报告（F8）明确把未接线的预留表面记为拆分切片的预期状态。现在移除只会被后续接入它们的切片重新加回。 - [rc:3777184238]（R3-10，仅拒绝该修复方向）：把
releaseHandle移到 snapshot/drain 的 await 之前，会让同 runId 的重试在旧运行 drain 仍在写同一 journal 时调用journal.load()——从不完整前缀恢复会重跑已完成的 agent，恰是await journal?.drain()所要防止的危害（见 R2-5 线程）。误报的 "already active" 窗口是瞬时且可重试的；安全修复需要感知 drain 状态的register()门控。线程保持打开并附此分析。
延后至下一轮（30 个线程）
每条的具体计划见 comment-replies.json（cached 路径覆盖、失败 dispatch 落定测试、abortAll() 账本测试、toolUseId 接入完成元数据、嵌套合并截断标记、delete 身份/顺序、journal 防中毒测试、嵌套日志顺序、approval 查找测试、R2-20 嵌套折叠、R2-3 三元清理、校验器往返、cancel 顺序断言、依赖推断钉住、拒绝路径记账、嵌套执行覆盖、args 注释/移除、runFanout 抽取、R3-8/R3-9 tails 语义、R3-12/R3-3 快照测试、R4-1/R4-2/R4-3 钳制/上限/性能、R6-5/R6-6/R6-7 语义与角落场景）。
延后至后续队列（deferred-findings.json）
维护者认可的后续项：F3（未知事件键导致整个运行被丢弃）、F4（续跑覆盖真实执行记录、血缘指向自身）、F5（快照字节预算），以及第 8 轮审查延后的三项（未清洗的 entry.error、runner emitter 接线覆盖、pruneSnapshots 把形状非法文件计入保留上限）。
验证
npm run build—— 通过（exit 0）npm run typecheck—— 通过（exit 0）npm run lint—— 通过（exit 0）- 对全部 7 个改动文件执行
npx prettier --write—— 已应用；git diff --check干净 - 聚焦 Vitest，
packages/core（9 个相关/相邻文件：workflow-run-registry、workflow-snapshot、workflow-runner、workflow-orchestrator、workflow-journal、workflow-sandbox、workflow-dispatch-scheduler、workflow-budget、workflow 工具）—— 提交后 527 通过 - 聚焦 Vitest，
packages/cli（workflowsCommand.test.ts）—— 提交后 32 通过 - RED 优先：五个行为修复在修复前源码上均失败（5 个失败测试），修复后通过；变异探针确认透传测试能拒绝无条件过滤变体
- 未改动 settings 源 → 无需
generate:settings-schema；未触及仅集成层覆盖的行为 → 无需打包后的集成测试
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/模型 qwen3.8-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the reverse-audit round cap of 5 without two consecutive dry rounds.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 466 tests passed — this review observed 20957, 20243, 1471, 1571, 494 passed.
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/commands/workflowsCommand.test.ts:698 — [probe] vacuous snapshotToTask graph-mapping test — the three new ?? [] mappings survive deletion (mutation probe D11-1)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the reverse-audit round cap of 5 without two consecutive dry rounds。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：466 tests passed — this review observed 20957, 20243, 1471, 1571, 494 passed。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| entry.recentLogs = tail.map((line) => | ||
| stripAnsiAndControl(line).slice(0, 4_096), | ||
| ); |
There was a problem hiding this comment.
[Critical] Settlement-time setRecentLogs(runId, outcome.logs) overwrites the live-mirrored recentLogs projection with the sandbox buffer tail while the 'log' event ledger keeps live-emitted content — post-settlement the two persisted log projections of the same run disagree in membership AND order, against the invariant the adjacent comment states ("the two persisted log projections must keep agreeing"). Probe-confirmed on the real orchestrator + registry through two entrances: (1) a nested-workflow settlement merge at the parent's MAX_LOG_LINES — appendLog is safeLog(line, false), so the truncation sentinel is pushed WITHOUT being emitted and the remaining nested lines are dropped from getLogs() even though the mirror recorded them live; (2) any parent-level safeLog while a nested workflow() is in flight lands in the buffer before the merged nested block but interleaved in the event ledger. — Failure scenario: a workflow logs 10,000 lines then invokes a nested workflow → the persisted snapshot's recentLogs holds the parent tail + sentinel while the log-event window holds the nested lines (and any parent line logged during a nested run sits at different positions in the two projections; past 100 lines the windows can hold different line sets) → post-restart /workflows replay shows a different log account than the live Logs panel for the same run.
Witness (probe through the real orchestrator/registry; a one-spot setRecentLogs resync fix flipped both entrances to agree = true, then reverted):
cap entrance: post recentLogs nested count = 0 | sentinel present = true
post logEvents nested count = 100 | sentinel present = false
POST-SETTLEMENT agree = false
ordering: post recentLogs has P-late = false | has N1 = true
post logEvents has P-late = true | has N1 = false
POST-SETTLEMENT agree = false
Suggested fix: make one source of truth feed both projections — e.g. in setRecentLogs, rebuild the 'log' event window from the same tail it writes into recentLogs (the probe's one-spot fix); alternatively drop the settlement overwrite and rely on the live mirror, or make the cap/merge path emit consistently.
中文说明
[Critical] 落定时的 setRecentLogs(runId, outcome.logs) 用 sandbox 缓冲区尾部整体覆盖实时镜像建立的 recentLogs 投影，而 'log' 事件账本保留的是实时发出的内容——落定后同一运行的两份持久化日志投影在成员与顺序上都会分歧，恰好违背相邻注释声明的不变量（"两份持久化日志投影必须保持一致"）。已在真实 orchestrator + registry 上用探针经两个入口确认：(1) 嵌套 workflow 落定合并恰逢父缓冲区达到 MAX_LOG_LINES——appendLog 即 safeLog(line, false)，截断哨兵被推入缓冲区却不发出事件，其余嵌套行从 getLogs() 中丢弃，尽管镜像早已实时记录；(2) 嵌套 workflow() 在途时的任何父级 safeLog 行，在缓冲区中位于合并的嵌套块之前，而在事件账本中却是交错记录。— 失败场景：workflow 记录 10,000 行后调用嵌套 workflow → 持久化快照的 recentLogs 是父尾部+哨兵，而 log 事件窗口是嵌套行（顺序入口下，嵌套运行期间的父级日志行在两份投影中位置不同；超过 100 行时两个窗口甚至可以包含不同的行集合）→ 重启后 /workflows 重放与实时 Logs 面板对同一运行呈现两套不同的日志。
证据（真实 orchestrator/registry 探针；对 setRecentLogs 的单点重同步修复使两个入口均翻转为 agree = true，随后已还原）：见英文部分。
建议修复：让单一事实来源同时驱动两份投影——例如在 setRecentLogs 中用它写入 recentLogs 的同一尾部重建 'log' 事件窗口（探针的单点修复即此方案）；或移除落定覆盖、完全依赖实时镜像，或让上限/合并路径一致地发出事件。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (!entry || !isActiveWorkflowStatus(entry.status)) return; | ||
| // Script-derived titles reach persisted snapshots and TUI rendering: | ||
| // normalize at this registry boundary like every sibling string. | ||
| const title = stripAnsiAndControl(rawTitle).slice(0, 200) || 'phase'; |
There was a problem hiding this comment.
[Critical] This registry-boundary normalization collapses consecutive phase titles on the NORMALIZED value, but the sandbox's safePhase collapses on RAW title identity and emits raw titles (the agent({phase}) wrapper compares raw too) — so two surfaces of the same run report different phase histories: the terminal tool-result returnDisplay renders outcome.phases (the raw sandbox list, ANSI escapes serialized into the JSON) while the live output block, the /workflows dialog, the persisted snapshot, and telemetry (phase_count) all report the registry's collapsed list. currentPhaseVisitId also never advances for the collapsed repeat, so dispatches queued while the sandbox believes it is in the second phase persist under the first phase's visit in the new graph. Found independently by two reverse-audit agents this round. — Failure scenario: a script calls phase('\x1b[1mBuild\x1b[0m') then phase('Build') (any pair colliding after normalization or after char 200) → the terminal JSON shows 2 phases with raw control bytes while every persisted surface shows 1, and the second phase's dispatch is attributed to phase-1 in the persisted dispatch graph.
Witness (probe through the real orchestrator + sandbox + registry; normalizing inside safePhase flipped to parity on all surfaces, then reverted):
SANDBOX outcome.phases: ["\u001b[1mBuild\u001b[0m","Build"] ← terminal returnDisplay: 2 phases
REGISTRY entry.phases: ["Build"] ← live UI / snapshot / telemetry: 1
REGISTRY dispatch attribution: dispatch-1 → phase-1, dispatch-2 → phase-1 (visit never advanced)
Suggested fix: normalize once at the sandbox boundary — apply stripAnsiAndControl(t).slice(0, 200) || 'phase' inside safePhase (and the agent({phase}) comparison) before collapse/push/emit, so both projections compare the same value; add a parity test driving sandbox + registry together with normalization-colliding titles.
中文说明
[Critical] 该 registry 边界归一化按归一化后的值折叠相邻阶段标题，而 sandbox 的 safePhase 按原始标题相等折叠并发出原始标题（agent({phase}) 包装层同样按原始值比较）——同一运行的两个表面因此报告不同的阶段历史：终端工具结果 returnDisplay 渲染 outcome.phases（sandbox 原始列表，ANSI 转义被原样序列化进 JSON），而实时输出块、/workflows 对话框、持久化快照与遥测（phase_count）都报告 registry 折叠后的列表。被折叠的重复标题也不会推进 currentPhaseVisitId，导致 sandbox 认为处于第二阶段时入队的 dispatch 在新图中被持久化到第一个阶段的 visit 之下。本轮由两个反向审计 agent 独立发现。— 失败场景：脚本先 phase('\x1b[1mBuild\x1b[0m') 再 phase('Build')（任何归一化后或 200 字符后相同的标题对）→ 终端 JSON 显示 2 个阶段并携带原始控制字符，而所有持久化表面显示 1 个，且第二个阶段的 dispatch 在持久化 dispatch 图中归属 phase-1。
证据（真实 orchestrator + sandbox + registry 探针；在 safePhase 内归一化后所有表面翻转为一致，随后已还原）：见英文部分。
建议修复：在 sandbox 边界一次性归一化——在 safePhase（以及 agent({phase}) 的比较）折叠/推入/发出之前应用 stripAnsiAndControl(t).slice(0, 200) || 'phase'，使两份投影比较同一个值；并补充以归一化冲突标题驱动 sandbox + registry 的一致性测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| entry.error = message; | ||
| this.appendEvent(entry, { | ||
| type: 'workflow-failed', | ||
| at: endTime, | ||
| error: stripAnsiAndControl(message).slice(0, 4_096), | ||
| }); |
There was a problem hiding this comment.
[Critical] fail() sanitizes and caps the failure message only in the new workflow-failed event; entry.error keeps the un-normalized, uncapped string, which toSnapshot persists into <runId>.json and /workflows renders — the listing shows entry.error.slice(0, 80) and the detail view the full string, both unsanitized — and the same payload rides raw into the background completion notification XML (escapeXml does not strip control sequences). The message is script-derived (a sandbox vm throw via extractErrorMessage), so it can carry ESC/control sequences and exceed 4 KB — the exact surface class this PR's sanitization work targets, and the divergence falsifies this diff's own registry-boundary rule ("normalize at this registry boundary like every sibling string"). Round 8 recorded this as the deferred "AC3 half-met" suggestion on the assumption it was persisted-bytes-only; the read-site sweep below disproves that premise. — Failure scenario: a workflow script throws an error containing \x1b[2J\x1b[H plus a 5 KB payload → the snapshot persists the raw 5023-char string under error while the sibling event in the same file holds the clean capped copy → /workflows renders the screen-clear/home sequence into the user's terminal, and the payload survives restart via the snapshot merge.
Witness (probe through the real runner + registry + snapshot writer; the one-line fix flipped it, then reverted):
PR: entry.error contains \u001b[2J: true | NUL: true | length = 5023
persisted snapshot.error === raw payload: true | listWorkflowSnapshots accepted: true
(sibling workflow-failed event copy: length = 4096, no ESC)
FIXED: entry.error length = 4096 | contains \u001b[2J: false | persisted snapshot.error sanitized
Render read-sites verified at this commit: workflowsCommand.ts listing ` — ${entry.error.slice(0, 80)}` and detail ` error : ${entry.error}` (full, uncapped), plus the registry completion notification <result>Error: ${escapeXml(entry.error ?? '')}</result>.
| entry.error = message; | |
| this.appendEvent(entry, { | |
| type: 'workflow-failed', | |
| at: endTime, | |
| error: stripAnsiAndControl(message).slice(0, 4_096), | |
| }); | |
| entry.error = stripAnsiAndControl(message).slice(0, 4_096); | |
| this.appendEvent(entry, { | |
| type: 'workflow-failed', | |
| at: endTime, | |
| error: entry.error, | |
| }); |
中文说明
[Critical] fail() 只在新的 workflow-failed 事件中清洗并截断失败消息；entry.error 保留未归一化、无上限的原始字符串，由 toSnapshot 持久化进 <runId>.json 并被 /workflows 渲染——列表渲染 entry.error.slice(0, 80)，详情渲染完整字符串，两者均未清洗——同一负载还会原样进入后台完成通知 XML（escapeXml 不移除控制序列）。消息来源于脚本（sandbox vm 抛出的错误经 extractErrorMessage 提取），完全可能携带 ESC/控制序列并超过 4 KB——正是本 PR 清洗工作针对的表面类别，也使本 diff 自己的 registry 边界规则（"像每个姊妹字符串一样在此边界归一化"）自我证伪。第 8 轮曾以 "AC3 half-met" 将其记录为延后 Suggestion，前提是它只影响持久化字节；下方的读点排查推翻了该前提。— 失败场景：workflow 脚本抛出含 \x1b[2J\x1b[H 与 5 KB 负载的错误 → 快照在 error 字段持久化 5023 字符原始字符串，而同一文件中的姊妹事件持有清洗后的 4096 上限副本 → /workflows 把清屏/光标归位序列渲染进用户终端，且负载随快照合并在重启后依然有效。
证据（真实 runner + registry + snapshot writer 探针；一行修复后翻转，随后已还原）：见英文部分。已在本提交核实渲染读点：workflowsCommand.ts 列表与详情两处，以及 registry 完成通知的 <result>Error: …</result>。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #9034 (round: 3 Critical inline findings)All three inline findings were resolved in code. Each claim was first Findings and dispositionsrc:3806896360 — [Critical] Settlement-time
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none — full chunk read (diff lines 1520-1909), full implementation file, runner call sites, and test run all completed within budget.
Test Plan (not a blocker): 466 tests passed — this review observed 20960, 20246, 1471, 1571, 494, 3655, 504 passed.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/workflow-run-registry.ts:1091 — [probe] setRecentLogs resync re-appends rebuilt log events after the terminal event (Date.now() timestamps) on dialog-cancelled runs
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：none — full chunk read (diff lines 1520-1909), full implementation file, runner call sites, and test run all completed within budget。
Test Plan（非阻断）：466 tests passed — this review observed 20960, 20246, 1471, 1571, 494, 3655, 504 passed。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
@qwen-code /takeover stop |
qqqys
removed
the
autofix/takeover
|
Released in v0.21.15. |
5 participants
What this PR does
This PR adds a structured, observable runtime model for Workflow execution. It records run and step lifecycle events, persists journal state, reconstructs snapshots, and exposes cancellation and retention primitives to downstream consumers.
This is the first layer split from #8950. Two existing CLI snapshot construction paths initialize the newly required runtime fields so the Core contract remains independently buildable; the PR intentionally contains no daemon routes, SDK surface, Web Shell UI, or client-eligibility change. Existing interactive-TUI and Zed gating for background workflows remains unchanged and is deferred to the slice that wires the new completion channel.
Why it's needed
Workflow execution currently has no durable source of truth that other surfaces can safely observe or control. A core-owned execution journal and snapshot model gives the CLI, daemon, and UI a consistent view while keeping orchestration semantics in the runtime that owns them.
Reviewer Test Plan
How to verify
Run the focused Workflow runtime tests and confirm lifecycle events, persisted snapshots, cancellation, retention, and sandbox behavior remain deterministic. Confirm historical snapshots receive empty defaults in the CLI compatibility path, then build and type-check the complete workspace.
Evidence (Before & After)
N/A — non-UI runtime change.
Tested on
Environment (optional)
Node.js 25.9.0 on macOS arm64. Focused result: 7 Core files and 466 tests passed; the CLI compatibility suite passed 31 tests; complete workspace build and type-check passed.
Risk & Scope
Linked Issues
Closes #9032
中文说明
本 PR 做了什么
本 PR 为 Workflow 执行增加结构化、可观察的运行时模型。它记录运行和步骤生命周期事件，持久化 journal 状态，重建快照，并向下游消费者提供取消与保留能力。
这是从 #8950 拆出的第一层。两个既有 CLI 快照构造路径会初始化新增的必填运行时字段，使 Core 契约可以独立构建；本 PR 刻意不包含 daemon 路由、SDK 接口、Web Shell UI 或客户端启用行为变更。后台 Workflow 既有的交互式 TUI 与 Zed 限制保持不变，留给真正接入新完成通道的后续切片处理。
为什么需要
Workflow 执行目前没有可供其他界面安全观察或控制的持久事实来源。由 Core 所有的执行 journal 与快照模型可以让 CLI、daemon 和 UI 获得一致视图，同时把编排语义保留在真正拥有它们的运行时中。
Reviewer 测试计划
如何验证
运行聚焦的 Workflow 运行时测试，确认生命周期事件、持久化快照、取消、保留和 sandbox 行为保持确定性。确认 CLI 兼容路径会为历史快照提供空默认值；随后构建完整 workspace 并运行类型检查。
证据（前后对比）
不适用——这是非 UI 的运行时改动。
测试平台
环境（可选）
macOS arm64，Node.js 25.9.0。Core 聚焦测试结果为 7 个文件、466 个测试通过；CLI 兼容测试 31 个通过；完整 workspace 构建和类型检查通过。
风险与范围
关联 Issue
Closes #9032