feat(workflows): bubble workflow agent approvals - #8240
Conversation
Real terminal E2E reportResult: PASS for the foreground Ink/TUI approval path on the final production bundle.
The driver used the final bundled CLI through a real Boundary: the model side used the repository's local fake OpenAI server with an isolated temporary home; no standard provider API key was present. This evidence covers the production bundle, PTY layout/focus/key handling, Workflow registry and approval routing, real Shell confirmation/execution/denial, and cleanup. It does not cover live-provider authentication, network, or latency; ACP and stream-json host behavior is covered by focused protocol regressions. 中文说明结果：最终 production bundle 的前台 Ink/TUI 审批链真实终端 E2E 全部通过。
Driver 通过真实 边界：模型侧使用仓库本地 fake OpenAI server，并隔离到临时 Home；没有配置标准 Provider API Key。本证据覆盖 production bundle、PTY 布局/焦点/按键、Workflow Registry 与审批路由、真实 Shell 确认/执行/拒绝及清理；不覆盖真实 Provider 的认证、网络和延迟。ACP 与 stream-json Host 行为由聚焦协议回归测试覆盖。 |
qqqys
added
the
autofix/takeover
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Thanks for the PR — this is the foreground permission path for Dynamic Workflows, and it reads well. Template looks good ✓ — all sections present, bilingual body, a real reviewer test plan. Problem: real and observed, not theoretical. A foreground Workflow child agent that hits a permission-requiring tool call currently parks with no route for the parent session to answer it, so the run can block indefinitely. This is phase PR1A of the Dynamic Workflows roadmap (#8105, a Direction: aligned. Establishing an explicit, fail-closed response chain before background execution and run-detach land is exactly the right order — auto-approving to close the gap would widen permissions, and deferring the chain would make the later phases worse. CHANGELOG has no entry for this exact feature yet, but the area is an active, shipping roadmap item (workflow abort #8107, channel pairing approvals #8081, background-agent roster work), so this is core territory rather than a tangent. Size: this touches core infrastructure ( Approach: the scope feels right rather than bloated. The five transport surfaces (TUI dialog/pill, ACP, stream-json control channel, IDE host, webui) look like a lot, but an approval that can't reach a user through whichever surface is driving the session is an undeliverable feature — cutting to just the registry would leave the chain broken. I didn't spot drive-by refactors or unrelated churn; the Risk: Stage 1e flagged a high-risk path — this PR modifies Flagging the core-size escalation and the acp-integration risk for discussion; moving on to a full code review. 🔍 中文说明感谢贡献——这是 Dynamic Workflows 的前台权限路径，整体读起来很扎实。 模板 完整 ✓——各节齐全，双语正文，有真实的 reviewer 测试计划。 问题： 真实且已观测，不是理论性的。前台 Workflow 子 Agent 一旦遇到需要审批的工具调用，目前会停住，而父会话没有回答入口，因此 run 可能无限阻塞。这是 Dynamic Workflows 路线图（#8105， 方向： 对齐。在后台执行和 run-detach 落地之前先建立显式、失败关闭的响应链，顺序完全正确——用自动批准来堵这个缺口会扩大权限，推迟响应链则会让后续阶段更糟。CHANGELOG 还没有这个具体功能的条目，但该领域是正在持续交付的路线图项（workflow abort #8107、channel pairing approvals #8081、background-agent roster 等），属于核心地带而非旁支。 规模： 触及核心基础设施（ 方案： 范围合理，不臃肿。五个传输面（TUI dialog/pill、ACP、stream-json 控制通道、IDE host、webui）看起来很多，但一个无法通过驱动会话的任一界面到达用户的审批，等于无法交付的功能——只保留 registry 会让响应链断裂。没有发现顺手重构或无关改动； 风险： Stage 1e 命中高风险路径——本 PR 修改了 先标出核心规模升级与 acp-integration 风险供讨论；进入完整代码审查。🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewMy independent take before reading the diff: park the child's permission request on the owning run with a unique id, expose a restricted display DTO to the parent surface, route the one-shot decision back through a responder, and fail closed on every terminal transition. The PR matches this and goes further than I would have bothered to on a first pass — bounded pending list (32), a 64 KB display cap, an exhaustive type restriction with a No critical blockers and no AGENTS.md violations found. The security posture is the standout:
The UI layer reuses the existing background-agent approval pattern rather than forking it ( Nothing to fix before merge from a correctness standpoint. The only thing I'd want a maintainer to confirm is the shared ACP queue noted in Stage 1 — it's intentional, but it's the one piece of genuinely new shared state. sequenceDiagram
participant P1 as Workflow Agent
participant P2 as WorkflowRunRegistry
participant P3 as Host Channel
participant P4 as User
participant P5 as Child Tool Call
P1->>P2: emit TOOL_WAITING_APPROVAL
P2->>P2: park, restrict DTO, bound and dedup
P2->>P3: approval request callback
P3->>P4: surface one-shot prompt
P4->>P3: allow once or deny
P3->>P2: resolvePendingApproval by approvalId
P2->>P5: respond, resume or cancel
Note over P2: terminal transition drains pending, fail closed
Files changed (16 of 28 shown)
Test evidence (the PR's own CI)I did not run any PR code — this is a static review, and the signal below is the PR's own CI on the reviewed commit. The ubuntu unit suite is green; macOS, Windows, and the CLI integration suite are
The author reports focused suites passing locally (Core 217, CLI 733, WebUI 3, VS Code companion 51) plus real-PTY approve/deny scenarios — that's the author's claim, posted as a separate PR comment, not something I re-ran. Sandboxed verification would settle the remaining gap: 中文说明代码审查读 diff 之前我的独立方案：把子 Agent 的权限请求停放在所属 run 上并赋予唯一 id，向父级界面暴露一个受限的展示 DTO，通过 responder 回传单次决定，并在所有终态转换上失败关闭。PR 与之一致，而且比我第一遍会做的更彻底——有界待审批列表（32）、64 KB 展示上限、带 未发现关键阻断项，也未发现 AGENTS.md 违规。安全姿态是亮点：
UI 层复用了既有的 background-agent 审批模式而非另起炉灶（ 从正确性角度没有合并前必须修的东西。我唯一想让维护者确认的是 Stage 1 提到的共享 ACP 队列——它是有意为之，但确实是唯一一处真正新增的共享状态。 （时序图与文件清单见英文部分。） 测试证据（PR 自身 CI）我没有运行任何 PR 代码——这是静态审查，下面是 PR 自身在被审 commit 上的 CI 信号。ubuntu 单测套件为绿；macOS、Windows 和 CLI 集成套件为 作者报告本地聚焦套件通过（Core 217、CLI 733、WebUI 3、VS Code companion 51）以及真实 PTY 的批准/拒绝场景——这是作者的陈述，作为独立 PR 评论发布，不是我重新跑的结果。 沙箱验证可以补齐剩余缺口： — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review across every stage, but the Stage 0 core-module escalation (a Stepping back: this is genuinely good work. The motivation is real (foreground Workflow children block indefinitely on a permission request with no answer route), the fix is the right foundational move before background/detach phases land, and the implementation is more rigorous than the feature strictly demanded — exactly-once settlement, fail-closed on every terminal path, a restricted DTO that strips raw args and edit contents, persistent outcomes normalized to cancel, and identity isolation so shared If I had to maintain this in six months I'd thank the author, not curse them. I'm not approving it myself for one reason only: it's a cross-package ⏸️ Deferring to @wenshao — the code is ready as far as I can tell; this needs a human maintainer call on two things the gate can't settle: (1) sign-off on a core-touching 中文说明置信度：3/5——每个阶段都是干净的审查，但 Stage 0 核心模块升级（一个触及核心、达 800 生产行的 退一步看：这是真正出色的工作。动机真实（前台 Workflow 子 Agent 会在权限请求上无限阻塞，因为没有回答入口），修复方向正确——在后台/detach 阶段落地之前先打好这个基础，而且实现比功能本身要求的更严谨——恰好一次结算、所有终态路径失败关闭、剥离原始参数与编辑内容的受限 DTO、把持久结果归一为 cancel、以及身份隔离使共享 如果六个月后由我来维护，我会感谢作者而不是骂他。我不自己批准，只有一个原因：这是一个跨 package 的 ⏸️ 转交 @wenshao——就我所能判断，代码已经就绪；这需要人类维护者在两件门禁无法裁定的事上拍板：（1）按 Stage 0 升级，对一个这种规模、触及核心的 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
🤝 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. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action requiredNo code change was made this round. The feedback newer than the last evaluation contains no actionable findings:
With no defects, no change requests, and no failing checks, there is nothing to fix this round. 中文说明Autofix 审查轮次 —— 无需处理本轮未做任何代码改动。上次评估之后的新反馈中没有可处理的发现：
由于没有缺陷、没有改动请求、也没有失败的检查，本轮没有任何需要修复的内容。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
✅ Independent local build + real-PTY verification of PR #8240Verdict: PASS — looks ready to merge. Maintainer-side local reproduction complementing the author's own PTY report. Each terminal state below was captured with the repo's Environment. Verified in an isolated git worktree pinned to PR head Build & unit tests
Real PTY — foreground Workflow approval bubbling (headline feature)The mock makes the parent call the Approve path.
Deny path. Selecting Methodology notes
中文说明（维护者独立本地构建 + 真实 PTY 验证）结论：通过，可合并。 维护者侧本地复现，补充作者 PTY 报告。下列每个终端态均用仓库 环境。 在隔离 git worktree 中验证，固定 PR head 构建与单测
真实 PTY——前台 Workflow 审批冒泡（核心特性）mock 使父会话调用 批准路径。 ① 停放请求在底部 pill 显示 拒绝路径。 在同一界面选 方法说明
— Qwen Code · maintainer local verification |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action requiredThe only new feedback since the last evaluation is a maintainer's independent local build + real-PTY verification report (@wenshao) with a PASS verdict: "looks ready to merge." The report confirms both the approve and deny paths of the workflow approval bubbling feature work correctly end-to-end. There are no review comments, no inline findings, no failed checks, and no still-red checks to address. No code changes are needed. 中文说明无需操作自上次评估以来，唯一的新反馈是维护者（@wenshao）的独立本地构建 + 真实 PTY 验证报告，结论为 通过："可合并"。该报告确认 workflow 审批冒泡功能的批准和拒绝两条路径均端到端正常工作。 没有审查评论、没有行内发现、没有失败的检查、也没有持续红色的检查需要处理。无需代码变更。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
| this.fail( | ||
| runId, | ||
| `Failed to resolve workflow approval: ${approvalId}`, | ||
| Date.now(), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The resolvePendingApproval error path — where runtime.respond() throws, triggering this.fail() and handle abort — has no test coverage. — Concrete cost: if a future change breaks this multi-step recovery (fail + sibling drain + abort), no test would catch the regression.
| this.fail( | |
| runId, | |
| `Failed to resolve workflow approval: ${approvalId}`, | |
| Date.now(), | |
| ); | |
| // Add a test in workflow-run-registry.test.ts: | |
| // it('fails the run and drains siblings when respond throws', async () => { | |
| // const r = new WorkflowRunRegistry(); | |
| // r.register({ runId: 'r1', ... }); | |
| // const respond = vi.fn(async () => { throw new Error('boom'); }); | |
| // // park approval with respond, then resolve → assert run failed, siblings rejected | |
| // }); |
中文说明
resolvePendingApproval 的异常路径（runtime.respond() 抛出异常时触发 this.fail() 和 handle abort）目前没有测试覆盖。如果未来的修改破坏了这套多步恢复逻辑（fail + 排空兄弟审批 + abort），没有测试能捕获回归。
— qwen3.8-max-preview via Qwen Code /review
| const confirmationPayload = allowed | ||
| ? this.buildAllowConfirmationPayload( | ||
| approval.name, | ||
| payload['updatedInput'], | ||
| ) | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] handleWorkflowApproval discards the host's deny message (payload['message']), unlike the adjacent handleTeammateApproval which forwards it as { cancelMessage }. — Concrete cost: the workflow subagent's model sees only the generic 'User did not allow tool call' instead of the host's specific denial reason, making it more likely to retry the same denied command.
| const confirmationPayload = allowed | |
| ? this.buildAllowConfirmationPayload( | |
| approval.name, | |
| payload['updatedInput'], | |
| ) | |
| : undefined; | |
| const confirmationPayload = allowed | |
| ? this.buildAllowConfirmationPayload( | |
| approval.name, | |
| payload['updatedInput'], | |
| ) | |
| : typeof payload['message'] === 'string' | |
| ? ({ cancelMessage: payload['message'] } as ToolConfirmationPayload) | |
| : undefined; |
中文说明
handleWorkflowApproval 在拒绝时丢弃了 Host 的拒绝消息（payload['message']），而相邻的 handleTeammateApproval 会将其作为 { cancelMessage } 转发。这导致 Workflow 子 Agent 的模型只能看到通用的 'User did not allow tool call'，而非 Host 给出的具体拒绝原因，增加了模型重试同一被拒命令的可能性。
— qwen3.8-max-preview via Qwen Code /review
| expect(workflowReg.setApprovalChangeCallback.mock.calls).toEqual([ | ||
| [expect.any(Function)], | ||
| [undefined], | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] The unmount test asserts workflowReg.setApprovalChangeCallback cleanup but omits the matching workflowReg.setStatusChangeCallback assertion. — Concrete cost: if a future refactor drops the setStatusChangeCallback(undefined) cleanup, neither test would fail, leaking a stale callback into an unmounted component.
| expect(workflowReg.setApprovalChangeCallback.mock.calls).toEqual([ | |
| [expect.any(Function)], | |
| [undefined], | |
| ]); | |
| expect(workflowReg.setApprovalChangeCallback.mock.calls).toEqual([ | |
| [expect.any(Function)], | |
| [undefined], | |
| ]); | |
| expect(workflowReg.setStatusChangeCallback.mock.calls).toEqual([ | |
| [expect.any(Function)], | |
| [undefined], | |
| ]); |
中文说明
卸载测试断言了 workflowReg.setApprovalChangeCallback 的清理，但遗漏了对应的 workflowReg.setStatusChangeCallback 断言。如果未来的重构意外删除了 setStatusChangeCallback(undefined) 清理调用，两个测试都不会失败，从而泄漏一个过时的回调到已卸载的组件中。
— qwen3.8-max-preview via Qwen Code /review
| r.setApprovalRequestCallback(async () => { | ||
| throw new Error('host disconnected'); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The synchronous-throw path in parkPendingApproval (source lines 532–541) has no test coverage; this test only exercises the async-rejection path (async () => { throw ... } always produces a rejected promise). — Concrete cost: if a future change breaks the sync-throw cleanup (runtime deletion, controller abort, direct filter), no test would catch it.
| r.setApprovalRequestCallback(async () => { | |
| throw new Error('host disconnected'); | |
| }); | |
| r.setApprovalRequestCallback(async () => { | |
| throw new Error('host disconnected'); | |
| }); | |
| // Also add a sync-throw variant: | |
| // r.setApprovalRequestCallback(() => { throw new Error('sync failure'); }); |
中文说明
parkPendingApproval 的同步抛出路径（源码 532–541 行）没有测试覆盖；当前测试仅验证了异步拒绝路径（async () => { throw ... } 总是产生 rejected promise）。如果未来的修改破坏了同步抛出的清理逻辑（runtime 删除、controller abort、直接 filter），没有测试能捕获。
— qwen3.8-max-preview via Qwen Code /review
|
🤖 AutoFix ran out of time before finishing (timeout (3000000ms)) (attempt 1/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/30660674462 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： SummaryAddressed all four automated-reviewer suggestions on PR #8240. No base-conflict merge was required ( Feedback points
Failed "Test" checkInvestigated the red The only failure reproducible in this sandbox is the integration test No code change was made for the Test check; per the workflow design its independent CI remains the final verification gate. Verification
中文说明概要已处理 PR #8240 上自动审查器提出的全部四条建议。本次无需解决与 base 的冲突（ 反馈点
失败的 “Test” 检查已排查红色的 在本沙箱中唯一可复现的失败是集成测试 未针对 Test 检查做任何代码修改；按照工作流设计，其独立的 CI 仍是最终验证关口。 验证
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review triage — no action this roundNo actionable feedback was found on this PR, so no code change was made. What was triaged
Why no action The single review comment is informational about the reviewer's own coverage, not a finding about the code. Verifying against the raw check data confirms every check on the PR is either If a future round surfaces a concrete integration-test failure or a specific finding, it will be addressed then. 中文说明Autofix 审查分诊 —— 本轮无需处理本 PR 未发现可处理的反馈，因此未做任何代码改动。 分诊内容
为何不处理 这条唯一的审查评论只是关于审查者自身覆盖范围的信息性说明，并非针对代码的发现。对照原始检查数据核实后确认：本 PR 上的每一项检查要么是 如果后续某一轮出现具体的集成测试失败或明确的发现，届时再予以处理。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
OverviewCompletes the foreground Dynamic Workflow permission chain. A workflow subagent's I checked out The security posture is the strongest part of this PR, and it holds up under tracing:
Findings below, most severe first. 1. (High) The new connection-wide permission queue can be held open by a request nobody will answer
The problem is what happens on abort. The caller's promise is raced against the signal, but the tail deliberately waits for the raw transport promise: const transportRequest = prior.then(() =>
signal.aborted
? requestPermissionWithAbort(this.client, params, signal)
: this.client.requestPermission(params),
);
const tail = transportRequest.then(() => undefined, () => undefined);Nothing cancels the outstanding How bad it gets depends entirely on the host answering an orphaned prompt:
Suggest bounding the tail rather than relying on host behavior: race 2. (Medium) Fail-closed denials are completely silent
The 64 KB cap is the one users will actually hit: a workflow agent editing a large generated file produces a 3. (Medium) The synthetic parent tool call reports approval outcome as execution outcome
4. (Low) Per-agent MCP tools degrade in the ACP card
5. Nits
Test coverageGenuinely good, and unusually specific — cross-session serialization, abort-before-settle queue ordering, host timeout, session dispose, core-side clear, the restricted edit diff rendering as text rather than a The one gap that matters is finding 1: there's a test proving the queue waits for an abandoned request, but none for what happens when nobody ever settles it. If you bound the tail, that's the test to add. VerdictThe core state machine and the permission-narrowing logic are sound, and I couldn't find a path that auto-approves or leaks a persistent grant. Finding 1 is the one I'd want addressed before merge — not because the workflow path is wrong, but because it hands a connection-wide failure mode to the primary tool path, which previously had none. |
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedFinding 1 (High): Permission queue wedge on aborted RPC — FixedThe connection-wide permission queue tail now advances when the caller's signal aborts, rather than waiting indefinitely for the orphaned RPC to settle. Implemented via Changed: Finding 2 (Medium): Silent fail-closed denials — FixedAdded Changed: Finding 3 (Medium): Approval outcome reported as execution outcome — FixedAdded Changed: Finding 4 (Low): Per-agent MCP tools degrade in ACP card — NotedAdded a comment at the Changed: Nit 5a: Synthetic client in
|
Review:
|
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 25 passed · 0 failed · 25 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：25 通过 · 0 失败 · 25 总计 Verification report (report.md)Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
✅ Maintainer local verification of PR #8240 — re-run at head
|
| Case | Expected | Observed |
|---|---|---|
| plain headless, no control channel | deny, don't wait forever | child got …permission was declined (non-interactive mode cannot prompt…), exit 0 in ~1s |
| stream-json, host allows once | only the approved op runs | can_use_tool emitted with a run-scoped tool_use_id (wfap_1); after behavior: allow the child received the real file content |
| stream-json, host denies | deny, with the host's own reason | child got [Operation Cancelled] Reason: denied by SDK host — i.e. 7a2ffab behaves as advertised |
One methodology note worth recording for reviewers: in headless default mode the CLI already drops Shell/edit tools from the registry entirely (pre-existing policy, unrelated to this PR), so a shell request can't even be formed there. To exercise the approval path for real I forced a confirmation on a tool that does stay registered (permissions.ask: ["read_file"]). The stream-json control channel also only comes up when the first stdin message is an initialize control request — a plain user message first puts the session in direct mode with no control plane, which is easy to mistake for a routing bug.
3. Build and tests
- Production bundle built clean from PR source; root
npm run typecheckpasses. - Suites covering every file this PR touches: core 185/185 ✓ · cli 697 passed / 1 skipped ✓ · webui 3/3 ✓ · vscode-ide-companion 51/51 ✓.
- The two
Sessionfailures I reported in my previous round are gone at this head — they were environmental/pre-existing and no longer reproduce.
Scope of this verification
Verified end-to-end in a real environment: the Ink/TUI approval path (approve + deny, with real side effects), plain headless, and stream-json allow/deny. Not exercised live here — the ACP host path, the bundled VS Code companion, the WebUI drawer, and concurrent multi-session prompt serialization; those rest on the PR's unit tests, which pass. Background execution, durable resume, and persistent grants remain out of scope for this phase by the author's own framing.
中文版
✅ 维护者本地验证 PR #8240 —— 在最新 head 5355391 重跑
结论：通过。未发现阻塞性问题，从验证角度看可以合并。
本条替代我之前那次验证（当时在 acc6694）。此后又落了三个提交（ba531d3 合并 base、7a2ffab 转发 host 拒绝原因、5355391 abort 时推进权限队列 + 审批可观测性），因此我重新构建、重新验证。以下所有结论均来自本地全新构建的 production bundle，通过真实 macOS PTY 和真实 stream-json 控制通道驱动，模型端使用确定性本地 mock，不涉及任何线上 provider 或网络。
环境。 隔离 git worktree 固定在 5355391 · npm run build && npm run bundle → dist/cli.js（--version = 0.21.2）· macOS（Darwin 25.6）· Node 22 · QWEN_CODE_ENABLE_WORKFLOWS=1 · --approval-mode default · 用工作区级 settings 预放行父会话自己的 workflow 调用，使只有子 agent 的请求需要确认。
截图见上方英文部分，编号与下文的"图 N"一一对应。
1. 前台 TUI —— 核心路径
mock 让父会话调用 workflow；脚本派发一个子 agent，该子 agent 请求需要确认的 Shell 命令（echo approve > proof.txt）。在 default 模式下，这个请求必须冒泡到父 TUI，既不能挂死，也不能自动批准。
- 停放的请求出现在底部：
1 workflow ⚠ needs approval，同时 workflow 工具的流式结果仍显示 run 处于agentsDispatched: 1, agentsCompleted: 0（图 1）。 - 从输入框按 ↓ ↓ 沿焦点链进入任务列表，该行同样带有待审批标记（图 2）。
- 回车进入详情页，用与普通工具完全一致的一次性确认界面渲染受限命令；如设计所述，没有"始终允许"选项（图 3）。
- 单次允许后只有该操作执行：
proof.txt被创建且内容为approve，run 结算为1 task done，workflow 返回PROOF_DONE，输入框恢复（图 4）。 - 拒绝（选
2. No）为 fail-closed：Shell 命令不执行（磁盘上无proof.txt），父会话仍正常结束而非挂死，run 结算后无残留待审批（图 5）。
两条 PTY 链路都是断言驱动而非肉眼判断——既校验屏幕字符串，也校验真实的文件系统副作用；approve 与 deny 均报告 PASS（0 failures）。
2. Headless 传输通道
测试计划中三条非 TUI 的父级传输，全部基于同一个 bundle 实测（图 6）：
| 场景 | 期望 | 实测 |
|---|---|---|
| 普通 headless，无控制通道 | 拒绝，且不能永久等待 | 子 agent 收到 …permission was declined (non-interactive mode cannot prompt…)，约 1 秒内 exit 0 |
| stream-json，host 单次允许 | 仅被允许的操作执行 | 发出 can_use_tool，tool_use_id 为 run 级标识（wfap_1）；回 behavior: allow 后子 agent 拿到真实文件内容 |
| stream-json，host 拒绝 | 拒绝，并带上 host 自己的原因 | 子 agent 收到 [Operation Cancelled] Reason: denied by SDK host，即 7a2ffab 行为符合描述 |
有两点方法学说明值得留给评审者：其一，headless default 模式下 CLI 本来就会把 Shell/编辑类工具整体移出注册表（既有策略，与本 PR 无关），因此那里根本构造不出 shell 请求；为了真正走通审批路径，我改用一个在 headless 下仍然注册的工具并强制其需要确认（permissions.ask: ["read_file"]）。其二，stream-json 的控制通道只有在 stdin 第一条消息是 initialize 控制请求时才会建立；先发普通 user 消息会让会话进入 direct 模式、完全没有控制面，这一点很容易被误判成路由 bug。
3. 构建与测试
- production bundle 从 PR 源码干净构建成功；根目录
npm run typecheck通过。 - 覆盖本 PR 所有改动文件的单测：core 185/185 ✓ · cli 697 通过 / 1 跳过 ✓ · webui 3/3 ✓ · vscode-ide-companion 51/51 ✓。
- 我上一轮报告的 2 个
Session失败在当前 head 已消失——属于环境性/既有问题，现已不复现。
本次验证的边界
在真实环境端到端验证的部分：Ink/TUI 审批路径（批准与拒绝，含真实副作用）、普通 headless、stream-json 允许/拒绝。未在真实环境实跑：ACP host 路径、内置 VS Code companion、WebUI 抽屉、以及并发多 Session 的审批串行化——这些依赖本 PR 的单测（均通过）。后台执行、持久恢复、持久授权按作者的定义本就不在本阶段范围内。
— maintainer local verification · production bundle @ 5355391 · real PTY + real stream-json control channel
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: You are verifying review findings for PR #8240 in the qwe... — the agent made no tool call: it read nothing.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查：You are verifying review findings for PR #8240 in the qwe...——该 agent 未发起任何工具调用：它什么都没读。
— qwen3.8-max-preview via Qwen Code /review
| updateToolCallId === pendingToolCallId && | ||
| (updateStatus === 'completed' || updateStatus === 'failed') | ||
| ) { | ||
| this.pendingPermissionResolve?.('cancel'); |
There was a problem hiding this comment.
[Suggestion] The new auto-resolve path (lines 487–496) settles the pending permission when a tool call goes terminal, but the pre-existing permissionResponse message handler (~line 556) has no stale-invocation guard. After auto-resolve clears pendingPermissionResolve, a late permissionResponse — the user clicking "No" before the webview processes the permissionResolved message and closes the drawer — still runs that handler: pendingPermissionResolve?.(optionId) is a no-op, but the handler then proceeds to isCancel → cancelCurrentPrompt(), which cancels whatever the agent is doing next (not the already-settled tool call), and sends streamEnd. — Concrete cost: a terminal update arriving while the drawer is open, followed by a user "No" before the drawer closes, cancels the agent's next action and emits a spurious streamEnd; the isWorkflowApproval guard only suppresses this for workflow approvals, so ordinary (non-workflow) permission prompts are the exposed case.
Guard the handler against stale invocation once the permission is already settled:
const handler = (message: PermissionResponseMessage) => {
if (message.type !== 'permissionResponse') return;
if (!this.pendingPermissionResolve) return; // already settled (e.g. by tool-call auto-resolve)
// ...existing logic
};中文说明
新增的自动结算路径（487–496 行）会在工具调用进入终态时结算待审批，但既有的 permissionResponse 消息处理器（约 556 行）没有针对过期调用的防护。自动结算清空 pendingPermissionResolve 之后，如果用户在 webview 处理 permissionResolved 消息并关闭抽屉之前点击「No」，这个迟到的 permissionResponse 仍会执行该处理器：pendingPermissionResolve?.(optionId) 是空操作，但处理器随后会继续走到 isCancel → cancelCurrentPrompt()，从而取消 agent 接下来要做的事（而不是已经结算的工具调用），并发送 streamEnd。具体代价：抽屉打开时到达终态更新、且用户在抽屉关闭前点击「No」，就会取消 agent 的下一个动作并发出一个多余的 streamEnd；isWorkflowApproval 防护只对 workflow 审批抑制此行为，因此普通（非 workflow）权限请求是暴露的场景。
建议在权限已结算后让处理器提前返回：
const handler = (message: PermissionResponseMessage) => {
if (message.type !== 'permissionResponse') return;
if (!this.pendingPermissionResolve) return; // 已结算（例如被工具调用自动结算）
// ...原有逻辑
};— qwen3.8-max-preview via Qwen Code /review
- Remove abort listener leak on shared turn signal in Session.ts by holding the listener reference and cleaning up via .finally() - Guard stale permissionResponse in WebViewProvider after auto-resolve - Add stream-json defensive guard to handleWorkflowApproval - Add regression test for normal diff-based edit prompts in PermissionDrawer - Document seenSources dedup safety in workflow-run-registry
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressedInline comments[rc:3694451843] Stale Added Issue-level review (@wenshao)1. Abort-listener leak on shared turn signal (medium) — Fixed Held the abort listener in a variable and added 2. Cross-session head-of-line blocking (medium, design) — Deferred to maintainer The reviewer asks whether per-session scope would remove the starvation risk without losing anything. This is a deliberate design tradeoff — the current connection-level serialization matches the ACP protocol's one-prompt-at-a-time model. Changing to per-session tails is an architecture decision that needs maintainer input on whether hosts actually render one prompt per session. 3. Oversized approvals denied rather than truncated (medium) — Declined The deny-on-oversized behavior is intentional fail-closed design. The 64KB cap ( 4. Programmatic denials indistinguishable from user denials (medium) — Declined Threading 5. Missing stream-json guard (low) — Fixed Added the same 6. No run identity in stream-json request (low) — Declined Changing 7. Synthetic tool call reports
8. WebUI change broader than PR describes (low) — Fixed Added a regression test asserting that a normal Minor: Added a comment explaining why the Minor: ACP diff as content text — Declined Noted for follow-up. Structured diffs for workflow approvals would require carrying Verification
中文说明已处理的评审反馈行内评论[rc:3694451843] 过期 在 Issue 级评审（@wenshao）1. 共享 turn 信号上的 abort 监听器泄漏（中等） — 已修复 在 2. 跨 Session 队头阻塞（中等，设计） — 延迟至维护者决定 评审者询问按 Session 划分是否能消除饥饿风险。这是一个刻意的设计权衡——当前连接级串行化与 ACP 协议的一次一个审批模型匹配。改为按 Session 的 tail 是一个架构决策，需要维护者确认 host 是否确实按 Session 渲染审批。 3. 超大审批被拒绝而非截断（中等） — 已拒绝 超大即拒绝是刻意的 fail-closed 设计。64KB 上限（ 4. 程序化拒绝与用户拒绝不可区分（中等） — 已拒绝 为 5. 缺少 stream-json 防护（低） — 已修复 添加了与 6. stream-json 请求中无 run 标识（低） — 已拒绝 将 7. 合成工具调用在审批时报告
8. WebUI 变更比 PR 描述的更广（低） — 已修复 添加了回归测试，断言普通 次要： 添加了注释，解释 次要：ACP diff 作为 content 文本 — 已拒绝 已记录待跟进。Workflow 审批的结构化 diff 需要通过受限 DTO 携带 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Solid permission bubbling: allow-once only (hideAlwaysAllow), approvalId-scoped settlement, ACP single-flight serialization, session-scoped transport identity, fail-closed cleanup on all terminal paths. Raw args stay process-local. Comprehensive test coverage across all surfaces (TUI/ACP/stream-json/VS Code). ✅
|
Released in v0.21.3. |
5 participants
What this PR does
This PR completes the foreground Dynamic Workflow permission path. When a Workflow agent reaches a Shell, edit, MCP, or information request that needs confirmation, the request is parked on its owning run and surfaced through the parent TUI, ACP host, or stream-json control channel. The parent can allow the operation once or deny it; persistent approval choices are not offered or accepted.
Pending requests are bounded, identified independently across runs and agents, and settled exactly once. Terminal transitions, cancellation, retry cleanup, session disposal, and host failure all clear the request and fail closed. Public run state retains only the restricted review details needed by the approval UI; raw arguments and runtime responders remain process-local and approval state is omitted from snapshots.
ACP permission prompts are serialized at the shared connection boundary, including prompts from concurrent sessions, primary tools, nested agents, and Workflow agents. Workflow approval cards use a session-and-run-scoped transport identity so a late terminal update from one session cannot close another session's prompt. The bundled IDE host also keeps a denied Workflow child request isolated from the parent prompt and renders restricted edit diffs for review.
Why it's needed
Foreground Workflows currently stop indefinitely when a child agent requests permission because the parent session has no route to answer it. That gap would become more severe once runs can detach in a later phase, and silently auto-approving would widen permissions. This phase establishes the explicit, fail-closed response chain first while preserving the existing foreground execution and output contract.
Reviewer Test Plan
How to verify
can_use_toolresponse and plain headless mode without a control channel. Confirm the former applies only an explicit one-shot allow (including sanitized updated input) and the latter denies rather than waiting forever.Focused verification passed: Core 217 tests; CLI 733 tests with one pre-existing skip; WebUI 3 tests; VS Code companion 51 tests; root build, typecheck, lint, formatting, and diff checks. Real PTY scenarios cover approve at 100 columns, deny at 48 columns, and a 13-row roster scrolling in a 60x18 terminal, including live approval refresh and actual allow/deny side effects.
Evidence (Before & After)
Before: the Workflow agent entered an approval wait, but the parent Workflow surface had no pending request to render or resolve, so execution could remain blocked indefinitely.
After: the real terminal shows
needs approval, routes focus from the footer into the Workflow row and shared confirmation view, updates the live agent count when the decision settles, restores the composer, and executes only the approved side effect. Detailed PTY evidence is posted as a separate PR comment.Tested on
Environment (optional)
Production bundle in a real macOS PTY with deterministic local model responses; package-local Vitest suites for runtime, Ink, ACP, stream-json, WebUI, and the bundled IDE host. Windows and Linux are left to CI.
Risk & Scope
Linked Issues
Part of #8105
中文说明
本 PR 做了什么
本 PR 补齐前台 Dynamic Workflow 的权限响应链。当 Workflow Agent 遇到需要确认的 Shell、编辑、MCP 或信息请求时，请求会停放在所属 run 上，并通过父会话的 TUI、ACP Host 或 stream-json 控制通道展示。父会话只能单次允许或拒绝该操作，不提供也不接受持久授权选项。
待审批请求有数量上限，在不同 run 和 Agent 之间具有独立身份，并且只会结算一次。终态转换、取消、重试清理、Session 销毁和 Host 失败都会清理请求并按拒绝处理。公开 run 状态只保留审批 UI 所需的受限审阅信息；原始参数和运行时响应器仅存在于进程内，Snapshot 不保存审批状态。
ACP 权限请求在共享 Connection 边界串行化，覆盖并发 Session、主工具、嵌套 Agent 和 Workflow Agent。Workflow 审批卡使用包含 Session 与 run 的传输身份，因此一个 Session 的晚到终态更新不会关闭另一个 Session 的审批。内置 IDE Host 还会把 Workflow 子请求的拒绝限制在该子请求内，不取消父 Prompt，并能展示受限的编辑 diff 供用户审阅。
为什么需要
目前前台 Workflow 的子 Agent 一旦请求权限就可能无限停住，因为父会话没有回答入口。后续阶段引入后台运行后，这个缺口会更严重，而静默自动批准又会扩大权限。本阶段先建立显式、失败关闭的响应链，同时保持现有前台执行与输出契约不变。
Reviewer 测试计划
如何验证
can_use_tool响应的 stream-json 和没有控制通道的普通 headless 模式。确认前者仅接受显式单次允许并应用安全更新后的输入，后者直接拒绝而不会永久等待。聚焦验证已通过：Core 217 个测试；CLI 733 个测试，另有 1 个既有 skip；WebUI 3 个测试；VS Code companion 51 个测试；根目录 build、typecheck、lint、格式和 diff 检查均通过。真实 PTY 场景覆盖 100 列终端批准、48 列窄终端拒绝，以及 60x18 终端中 13 行任务列表滚动，并验证审批 live refresh 和真实允许/拒绝副作用。
前后对比证据
之前：Workflow Agent 进入审批等待，但父级 Workflow 界面没有可展示或响应的待审批请求，因此执行可能永久阻塞。
之后：真实终端显示
needs approval，焦点可以从底部状态进入 Workflow 行和共享确认视图，决定结算后实时更新 Agent 计数并恢复输入框，而且只有被允许的副作用会执行。详细 PTY 证据会作为独立 PR 评论发布。已测试平台
环境（可选）
在真实 macOS PTY 中运行 production bundle，并使用确定性的本地模型响应；同时运行 Runtime、Ink、ACP、stream-json、WebUI 和内置 IDE Host 的 package-local Vitest。Windows 和 Linux 交给 CI 验证。
风险与范围
关联 Issue
Part of #8105