feat(core): surface PreToolUse hook 'ask' as a TUI confirmation - #5629
Merged
Conversation
A PreToolUse hook returning permissionDecision:'ask' was treated the same as 'deny'. The hook fires in the execution phase (_executeToolCallBody), after the confirmation flow in _schedule has finished, so an 'ask' could only block the tool as EXECUTION_DENIED instead of prompting the user. Bounce the tool from the execution phase back to awaiting_approval when a hook asks: build a synthetic 'info' confirmation whose onConfirm routes through handleConfirmationResponse (ProceedOnce re-executes, Cancel cancels). PreToolUse keeps its "before execution" timing — only the 'ask' branch is new; 'denied'/'stop' keep deny-as-error. A non-interactive CLI or background agent cannot prompt, so 'ask' falls back to deny there. The re-execution after approval skips both the hook re-fire (no infinite re-ask loop) and the non-idempotent path-unescape prelude. A walk-away abort sets a terminal status so the turn cannot hang, and the tool span survives the bounce so it is finalized exactly once. Tests: add coverage for ask->awaiting_approval, approve->execute-once (no re-ask loop), decline->cancelled, non-interactive/background deny, walk-away abort, single span finalize, and no double path-unescape.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
✅ Verification report — real end-to-end test of PR #5629Verdict: behavior matches the PR description exactly. Recommend merge. I built the real Test harness
Results
Key evidencePR build — interactive Merge-base build (A/B control) — same scenario, same hook, no prompt; auto-denied: The A/B swapped a single compiled file ( Non-interactive Coverage notes (honest scope)
No regressions or unexpected behavior observed. The change is well-scoped (one source file; deny/stop paths untouched) and the edge cases called out in the description hold up under a real run. 🇨🇳 中文版验证报告（点击展开）✅ 验证报告 —— 对 PR #5629 的真实端到端测试结论：实际行为与 PR 描述完全一致，建议合并。 我从本 PR 分支构建了真实的 测试装置
结果
关键证据PR 构建 —— 交互式 Merge-base 构建（A/B 对照）—— 同场景同钩子，无弹窗、被自动拒绝： A/B 仅替换单个编译产物（ 非交互式 覆盖范围说明（如实标注）
未发现回归或异常行为。改动范围清晰（单一源文件；deny/stop 路径未改），描述中列出的边界情况在真实运行下均成立。 Verified by building the PR branch and driving the real CLI in tmux against a deterministic mock model; A/B against merge-base |
wenshao
requested changes
Jun 22, 2026
qwen-code-ci-bot
requested changes
Jun 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Hi @LaZzyMan — thanks for the PR and the detailed description of the behavior and edge cases. The concept here is solid.
However, the PR body doesn't follow the pull request template. The template requires specific headings that help reviewers quickly assess scope, risk, and how to verify:
- What this PR does / Why it's needed — the PR uses "Summary" and "Behavior" instead
- Reviewer Test Plan with "How to verify", "Evidence (Before & After)", and "Tested on" (OS matrix) — the PR has "Tests" but no reproduction steps, no before/after evidence, and no OS test matrix
- Risk & Scope — missing entirely (main risk/tradeoff, out-of-scope items, breaking changes)
- Linked Issues — missing (is there an issue this closes?)
- Chinese translation (
<details>block) — missing
Could you update the PR description to follow the template? It makes review significantly easier, especially the "How to verify" steps and before/after evidence — a hook bouncing a tool back to confirmation is a user-visible TUI change that reviewers need to see working.
Note: @wenshao has already left a substantive code review with important concerns (multi-tool batch hang, orphaned hook events, missing test coverage) that should also be addressed.
中文说明
感谢 PR 以及对行为和边界情况的详细描述，方向是好的。
但 PR 正文没有遵循 PR 模板。模板要求的几个关键部分缺失：
- What this PR does / Why it's needed — 目前用的是 "Summary" 和 "Behavior"
- Reviewer Test Plan 含 "How to verify"、"Evidence (Before & After)"、"Tested on"（操作系统矩阵）— 目前只有 "Tests"，缺少复现步骤、前后对比证据和测试平台信息
- Risk & Scope — 完全缺失（主要风险/权衡、不在范围内的内容、破坏性变更）
- Linked Issues — 缺失（是否有关联的 issue？）
- 中文翻译（
<details>块）— 缺失
请按模板更新 PR 描述。特别是 "How to verify" 步骤和前后对比证据——hook 将工具弹回确认是用户可见的 TUI 变更，reviewer 需要看到实际运行效果。
另外，@wenshao 已经留下了实质性的代码审查意见（多工具并发挂起、孤立 hook 事件、测试覆盖缺失），也请一并处理。
— Qwen Code · qwen3.7-max
qwen-code-ci-bot
requested changes
Jun 22, 2026
Round 1 review of #5629 surfaced edge cases in the bounce mechanism: - Multi-tool batch hang: a bounced tool approved while a sibling was still executing stayed stuck in 'scheduled'. attemptExecutionOfScheduledCalls now loops, re-checking for newly-scheduled bounce-approved calls after each batch drains. - Orphaned hook events: the post-approval re-execution generated a fresh tool_use_id, leaving PreToolUse(old)/PostToolUse(new) unpaired. Preserve and reuse the original id across the bounce. - ModifyWithEditor double-unescape: request.args is unescaped in place before the hook fires, so the ModifyWithEditor branch must skip its own unescape for a bounced tool (it would double-strip escaped metacharacters). - Missing signal.aborted re-check before bouncing: mirror the confirmation-phase guard so an aborted signal falls through to deny instead of flashing a confirmation nobody can answer. Tests: multi-tool-hang regression (RED before the loop fix), non-interactive STREAM_JSON and Zed bounce paths, and span-finalize assertions on the walk-away abort test.
Collaborator
Author
Round 1 review addressed — 302f12fThanks for the thorough pass. Code fixes (
Declined as out of scope (this PR makes
PR description updated to follow the template (What / Why, Reviewer Test Plan with @wenshao's A/B evidence, Risk & Scope, Linked Issues #5424, 中文 block). |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Collaborator
✅ Local tmux verification —
|
| Scenario | Before (a233394e5) |
After (this PR) |
|---|---|---|
| Interactive | ask → auto-denied, no prompt (x ListFiles) |
confirmation box → approve runs once / decline cancels |
Non-interactive -p |
denied | falls back to deny, exit 0, no hang |
1) Interactive — ask now renders a confirmation; approve runs the tool exactly once
╭─────────────────────────────────────────────────────────────────╮
│ ? ListFiles . ← │
│ │
│ Hook gate: please confirm running this tool before it executes. │
│ │
│ Do you want to proceed? │
│ │
│ › 1. Yes, allow once │
│ 2. No, suggest changes (esc) │
╰─────────────────────────────────────────────────────────────────╯
⠏ Waiting for user confirmation...
- The hook
permissionDecisionReasonis surfaced as the prompt body. - Only Yes, allow once / No, suggest changes — no “always allow” (
hideAlwaysAllowhonored, since the hook re-evaluates every call).
After Yes, allow once → tool executes once and the model answers:
✓ ListFiles .
Listed 2 item(s)
✦ ALL_DONE_SENTINEL
The mock received the directory listing as a tool result exactly once — the re-execution skips the PreToolUse hook, so there is no re-ask loop.
2) Interactive — decline cancels the tool
Selecting No, suggest changes on the same box:
- ListFiles .
- is the Canceled glyph (vs ✓ Success / x Error). The tool never ran and the TUI returned to the prompt with no hang.
3) Non-interactive (-p) — ask falls back to deny (control isolates the hook)
-p -y run |
tool result the model received | tool ran? | exit |
|---|---|---|---|
with ask hook |
Hook gate: please confirm running this tool… (deny) |
No | 0, no hang |
| control, no hook | Listed 2 item(s) … [DIR] .qwen, hook-ask.sh |
Yes | 0 |
stderr (hooked run): Tool "list_directory" requires user approval but cannot execute in non-interactive mode. The control proves the deny is caused by the hook ask, not by the tool itself.
4) Before/after (merge-base) — confirms the behavior change
Same hook + same mock on a233394e5:
x ListFiles {"path":"…/project"}
Hook gate: please confirm running this tool before it executes.
✦ ALL_DONE_SENTINEL
x = Error: ask was silently auto-denied with no prompt (the hook reason became the failure message; the tool never ran). This PR turns that into the interactive confirmation above.
Verdict
Every behavior in the Reviewer Test Plan reproduces on a real built CLI: interactive ask → native confirmation (approve runs once, no loop; decline cancels), and non-interactive ask → graceful deny with no hang. The merge-base A/B confirms the change is exactly the intended “ask was deny → now prompts”. LGTM for merge from a behavioral standpoint.
中文版（合并参考）
✅ 本地 tmux 真实验证 —— PreToolUse 的 ask → TUI 确认框
在真实构建的 CLI（npm ci && npm run build，head 302f12f0c，独立 worktree，Linux）上通过 tmux 驱动验证，配合确定性 mock OpenAI 服务器和一个真实的、返回 permissionDecision: "ask" 的 PreToolUse command hook，并与 merge-base a233394e5（无 ask-bounce 代码）做了 A/B 对比。
环境
- Hook —— 项目
.qwen/settings.json，matcher*，已预信任，输出：
{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"Hook gate: please confirm running this tool before it executes."}} - 模型 —— mock OpenAI：第一轮用户消息返回一个
list_directorytool call，随后返回最终文本（ALL_DONE_SENTINEL）。 - 使用
-y运行，使工具自身的确认被自动通过，让 hook 的ask成为唯一闸门（正常交互会话中getShouldAvoidPermissionPrompts()为false，所以-y不会抑制 bounce）。
结论一览
| 场景 | 改动前（a233394e5） |
改动后（本 PR） |
|---|---|---|
| 交互模式 | ask → 静默拒绝、无弹窗（x ListFiles） |
弹出确认框 → 批准执行一次 / 拒绝则取消 |
非交互 -p |
拒绝 | 回退为 deny，exit 0，不挂起 |
1) 交互模式 —— ask 现在弹出确认框，批准后工具恰好执行一次
确认框显示 hook 的 permissionDecisionReason，选项仅有 Yes, allow once / No, suggest changes，没有 “always allow”（hideAlwaysAllow 生效，因为 hook 每次都会重新评估）。选择 Yes, allow once 后工具执行一次（✓ ListFiles . / Listed 2 item(s)），mock 仅收到一次目录列表结果 —— 重执行会跳过 PreToolUse hook，没有重复询问循环。
2) 交互模式 —— 拒绝则取消
在同一确认框选择 No, suggest changes 后，工具显示 -（Canceled）—— 未执行，TUI 正常回到输入框，不挂起。
3) 非交互（-p）—— ask 回退为 deny
带 hook 时工具被拒绝（模型收到的 tool result 即 hook 文案，工具未执行），exit 0、不挂起；无 hook 的对照组工具正常执行（返回目录列表）。对照组证明该拒绝来自 hook ask，而非工具本身。
4) 改动前后对比（merge-base）
在 a233394e5 上用相同 hook + mock：工具显示 x（Error），ask 被静默自动拒绝、无任何弹窗（hook 文案变成失败信息，工具从未执行）。本 PR 将其改为上面的交互式确认。
结论
Reviewer Test Plan 中的所有行为都在真实构建的 CLI 上复现：交互式 ask → 原生确认（批准只执行一次、无循环；拒绝则取消），非交互 ask → 优雅 deny 且不挂起。merge-base A/B 印证了改动正是预期的“ask 原本=deny → 现在弹确认”。从行为角度 LGTM，可以合并。
Verified by @wenshao on Linux via a real built CLI + mock OpenAI server driven over tmux (head 302f12f0c vs merge-base a233394e5).
wenshao
previously approved these changes
Jun 23, 2026
Collaborator
|
@qwen-code /triage |
Round 2 review: autoApproveCompatiblePendingTools auto-approved every awaiting_approval tool when a sibling was approved with ProceedAlways — including tools bounced by a PreToolUse 'ask'. The bounced tool would be auto-approved and re-executed with the hook skipped (isPostAskReexecution), silently defeating the hook's confirmation gate. Exclude bounced callIds from the auto-approve filter so a hook 'ask' always requires explicit confirmation. Test: a sibling's ProceedAlways no longer auto-approves a bounced ask (RED before the filter guard).
Collaborator
Author
Round 2 review addressed — 4535a91
Declined as out of scope (this PR makes
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). — qwen3.7-max via Qwen Code /review
wenshao
previously approved these changes
Jun 24, 2026
Collaborator
|
@qwen-code /triage |
Collaborator
|
Re-running triage on request (2026-07-06). Three new commits since the last review — all edge-case fixes for the ask bounce feature. Template, direction, and scope assessment unchanged from the prior triage. Template ✓ — all required headings still present and filled in properly. On direction: still clearly aligned. On approach: scope remains tight. The three new commits ( Moving on to code review. 🔍 中文说明按请求重跑 triage（2026-07-06）。自上次审查以来新增 3 个提交——均为 ask bounce 功能的边界情况修复。模板、方向和范围评估与上次 triage 一致。 模板 ✓ —— 所有必填部分仍然完整。 方向：仍然明确对齐。 方案：范围仍然紧凑。三个新提交（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
|
Re-running triage on request (2026-07-06). Nine new commits since the original triage (2026-06-24), three of which are feature work; the rest are merges from Code reviewMy independent proposal (before reading the diff): since Comparison with the diff: the PR's approach matches my proposal almost exactly — it's the natural solution. The new commits since the last triage address edge cases I'd flagged as "would bite if missed":
No correctness bugs, no security issues, no AGENTS.md violations found. On the other TestingUnit tests (worktree, fresh Total: 477 passing tests across the three test files (up from 289 at the original triage — the 188 new tests cover the bounce happy path, cancel path, non-interactive fallbacks, abort drain, path-unescape idempotency, sibling tool interactions, catch-path cleanup, ModifyWithEditor guard, batch pause, STREAM_JSON/Zed bounce, autoApproveCompatiblePendingTools exclusion, and the hideAlwaysAllow approval-mode filter). CI (current head Tmux real-CLI test: I ran the PR branch build (via Non-interactive mode with @wenshao's independent tmux A/B run on a real built CLI (Linux, isolated worktree, deterministic mock OpenAI server, earlier commit 中文说明按请求重跑 triage（2026-07-06）。自上次 triage（2026-06-24）以来新增 9 个提交，其中 3 个是功能提交，其余为 代码审查我的独立方案（读 diff 前）：既然 与 diff 的对比： PR 的方案几乎与我的独立方案完全一致。自上次 triage 以来的新提交处理了我标记为"遗漏会出问题"的边界情况：
无正确性 bug、无安全问题、无 AGENTS.md 违规。 关于另一个 测试单元测试（worktree，全新 CI（当前 head Tmux 真实 CLI 测试： 我用 PR 分支构建运行了 PreToolUse hook 返回 — Qwen Code · qwen3.7-max |
Collaborator
|
Re-running triage on request (2026-07-06). The PR has matured significantly since the original approval on 2026-06-24 — three new commits addressing edge cases identified through code review and Stepping back: this PR does exactly what it says — makes
The test coverage is thorough — 477 passing tests across three test files, up from 289 at the original triage. CI is green on the current head ( My non-interactive tmux test confirms the CLI runs from the PR branch and the I have no reservations. The PR has been through multiple rounds of review, each round found real edge cases, and each was addressed with focused fixes and regression tests. Still approving. Ready to ship. ✅ 中文说明按请求重跑 triage（2026-07-06）。自 2026-06-24 原始批准以来，PR 经过多次迭代——三个新提交处理了代码审查和 再次退后一步看：这个 PR 完全做了它承诺的事——让
测试覆盖充分——477 个测试通过。CI 全绿。非交互 tmux 测试确认 CLI 正常运行且 无疑虑。仍批准合并。可以发布。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| expect(completed[0].status).toBe('success'); | ||
| expect(execute).toHaveBeenCalledTimes(1); | ||
| // The re-execution skips the hook → PreToolUse fired exactly once. | ||
| expect(preToolUseCallCount(messageBus)).toBe(1); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The bouncedToolUseId map (added to keep PreToolUse/PostToolUse events correlated by the same tool_use_id across a bounce) is exercised here but never asserted. This test verifies preToolUseCallCount === 1 but does not assert that PostToolUse fires, or that the tool_use_id passed to PostToolUse matches the one from the bounced PreToolUse call. If the bouncedToolUseId lookup were broken (e.g., returning a fresh id instead of the captured one), audit-trail correlation would silently break with no test catching it.
Consider adding a PostToolUse assertion:
const preCall = messageBus.request.mock.calls.find(
(c) => (c[0] as { eventName?: string })?.eventName === 'PreToolUse',
);
const preToolUseId = (preCall?.[0] as { tool_use_id?: string })?.tool_use_id;
const postCall = messageBus.request.mock.calls.find(
(c) => (c[0] as { eventName?: string })?.eventName === 'PostToolUse',
);
expect(postCall).toBeDefined();
expect(
(postCall?.[0] as { tool_use_id?: string })?.tool_use_id,
).toBe(preToolUseId);— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
requested changes
Jun 24, 2026
| if (hooksEnabled && messageBus) { | ||
| // PreToolUse Hook — skipped on a post-'ask' re-execution (the hook | ||
| // already ran and the user already confirmed; re-firing would loop). | ||
| if (hooksEnabled && messageBus && !isPostAskReexecution) { |
Collaborator
There was a problem hiding this comment.
[Critical] ModifyWithEditor on a bounced tool bypasses the PreToolUse hook
The isPostAskReexecution flag skips the PreToolUse hook on re-execution (this guard), which is correct for the normal approve-after-bounce path. However, a programmatic client (STREAM_JSON, IDE integration) can send ToolConfirmationOutcome.ModifyWithEditor for a bounced tool — _handleConfirmationResponseInner (line ~2635) only checks isModifiableDeclarativeTool(waitingToolCall.tool), not confirmationDetails.type. After the editor modifies the tool arguments and the user approves via ProceedOnce, isPostAskReexecution is consumed here and the PreToolUse hook is skipped entirely — the modified arguments bypass the hook's security gate.
The TUI is safe (info-type confirmations don't expose ModifyWithEditor), but the backend lacks a matching guard.
Suggested fix: In _handleConfirmationResponseInner's ModifyWithEditor branch, also check that the confirmation type supports modification:
} else if (outcome === ToolConfirmationOutcome.ModifyWithEditor) {
const waitingToolCall = toolCall as WaitingToolCall;
if (
waitingToolCall.confirmationDetails.type !== 'info' &&
isModifiableDeclarativeTool(waitingToolCall.tool)
) {Or alternatively, clear bouncedAwaitingApproval before re-execution when args were modified via editor, so the PreToolUse hook re-fires with the modified args.
— qwen3.7-max via Qwen Code /review
wenshao
reviewed
Jun 24, 2026
| // (before the span guard) so a bounced call is cleared even on the | ||
| // defensive no-span path. | ||
| this.bouncedAwaitingApproval.delete(callId); | ||
| this.bouncedToolUseId.delete(callId); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Cancel path orphans PreToolUse event — no PostToolUseFailure fires when user cancels a bounced tool
When the user cancels a bounced tool at the TUI confirmation prompt, the cancel path calls finalizeToolSpan which deletes the stored bouncedToolUseId without firing a PostToolUseFailure hook event. The PreToolUse event was already fired on the first (bounced) attempt, so consumers that correlate Pre/Post pairs by tool_use_id (audit trails, metrics dashboards) see an unclosed entry.
This is unique to bounced tools: for non-bounced tools cancelled at confirmation, PreToolUse hasn't fired yet, so there is no orphan. The bounce makes Pre fire before the user decision, creating a new gap.
Consider reading this.bouncedToolUseId.get(callId) before finalizeToolSpan clears it, and firing safelyFirePostToolUseFailureHook with the stored toolUseId and a cancel reason (mirroring the abort path's invocation in _executeToolCallBody).
— qwen3.7-max via Qwen Code /review
| // Each iteration either drains ≥1 'scheduled' call or returns, so this | ||
| // cannot spin: a re-bounce lands back in awaiting_approval (guard fails → | ||
| // return), and a clean run leaves nothing 'scheduled' (length 0 → return). | ||
| while (true) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Sibling throw can strand a bounce-approved tool in 'scheduled'
The while(true) loop has no error recovery around batch execution. Both executeSingleToolCall and runConcurrently re-throw on prelude failures (line ~3017: throw error). If a sibling tool's execution throws and escapes the loop, any bounce-approved tool that was re-scheduled (status: 'scheduled') during that iteration is stranded — nothing else calls attemptExecutionOfScheduledCalls, so it never executes and never reaches a terminal state. checkAndNotifyCompletion is never reached because the throw skips it.
Before this PR, all tools in callsToExecute were past 'scheduled' when any sibling threw, so the re-throw only affected error propagation. Now the bounce mechanism creates a path where a tool re-enters 'scheduled' while a sibling is still executing (via handleConfirmationResponse → nested attemptExecutionOfScheduledCalls which returns because the sibling is executing). If that sibling subsequently throws, the re-scheduled tool is abandoned.
Consider wrapping batch execution inside the while loop with a try/catch that absorbs the re-throw and continues, since executeSingleToolCall already sets terminal error status internally:
for (const batch of batches) {
try {
if (batch.concurrent && batch.calls.length > 1) {
await this.runConcurrently(batch.calls, signal);
} else {
for (const call of batch.calls) {
await this.executeSingleToolCall(call, signal);
}
}
} catch {
// executeSingleToolCall already set terminal error status.
// Continue the while loop so any newly-scheduled bounce-approved
// tool is not stranded.
}
}— qwen3.7-max via Qwen Code /review
| // the original deny-as-error behavior. | ||
| if ( | ||
| preHookResult.blockType === 'ask' && | ||
| !signal.aborted && |
Collaborator
There was a problem hiding this comment.
[Suggestion] The !signal.aborted guard before bounce is untested
The !signal.aborted check (added in fix commit 302f12f) ensures the code falls through to deny-as-error when the signal is already aborted. The existing "cancels a pending ask" test aborts after the bounce completes — no test covers the case where the signal is already aborted at the moment the hook returns 'ask' (e.g., the user cancels during the hook's async processing).
If this guard were accidentally removed, the code would bounce to awaiting_approval on a dead signal and the turn could hang until the next drain cycle.
Consider adding a test that aborts the signal during hook processing:
it('denies a PreToolUse ask when signal is already aborted at hook-return time', async () => {
const ac = new AbortController();
const messageBus = {
request: vi.fn().mockImplementation(async (req) => {
if (req.eventName === 'PreToolUse') {
ac.abort();
return { type: MessageBusType.HOOK_EXECUTION_RESPONSE, correlationId: 'pre-hook', success: true, output: { decision: 'ask', reason: 'confirm' } };
}
return { type: MessageBusType.HOOK_EXECUTION_RESPONSE, correlationId: 'pre-hook', success: true, output: {} };
}),
};
// ... schedule with ac.signal, assert status is 'error' (deny path), not 'awaiting_approval'
});— qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /resolve |
…k bounce feature Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
# Conflicts: # packages/core/src/core/coreToolScheduler.test.ts
wenshao
previously approved these changes
Jul 3, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
— qwen3.7-max via Qwen Code /review
AmariahAK
pushed a commit
to AmariahAK/qwen-code
that referenced
this pull request
Jul 3, 2026
QwenLM#6260) These jobs transitively depend on precheck-pr via authorize. precheck-pr is intentionally skipped for same-repo PRs (only runs for forks). Without always(), GitHub Actions' default behavior propagates the skipped upstream job, causing delay-automatic-review to be skipped even when authorize succeeds. This breaks the entire review chain for same-repo PRs on opened/synchronize events. Fixes PR QwenLM#5629 review not triggering.
wenshao
reviewed
Jul 6, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No high-confidence Critical issues found. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/core/src/core/coreToolScheduler.ts:3309 |
executeSingleToolCall catch path skips finalizeToolSpan for bounced tools — if runInToolSpanContext throws before invoking the callback, bouncedAwaitingApproval marker is never consumed and the finally block skips finalize, leaking bouncedToolUseId and the tool span |
Add this.bouncedAwaitingApproval.delete(callId) and this.bouncedToolUseId.delete(callId) in the catch block before existing error handling |
packages/shared/src/automations/history-store.ts:181 |
recoverHistoryObjectsFromLine drops valid objects after mid-line garbage — when a glued line contains {"valid":1}GARBAGE{"valid":2}, only the first object is recovered; the function returns early on non-whitespace at depth 0 instead of scanning past the garbage |
Advance cursor past the garbage and continue scanning for the next { instead of returning early |
— qwen3.7-max via Qwen Code /review
| return false; | ||
| } | ||
| const { confirmationDetails } = call; | ||
| return !( |
Collaborator
There was a problem hiding this comment.
[Suggestion] hideAlwaysAllow filter is broader than the bounce use case — it also catches pmForcedAsk tools
The hideAlwaysAllow property is set by two independent producers:
- The new bounce path (
coreToolScheduler.ts:3369) — intended target of this filter. - The pre-existing
pmForcedAskpath (coreToolScheduler.ts:2657) — permission-manager explicit 'ask' rules.
Before this PR, the filter was simply call.status === 'awaiting_approval', so switching to YOLO or AUTO_EDIT auto-approved all pending tools including pmForcedAsk ones. After this PR, pmForcedAsk tools are silently excluded from batch auto-approval with no indication to the user.
This is a behavioral regression: a user who switches to YOLO to blast through pending approvals will find that tools governed by PM forced-ask rules stay stuck in awaiting_approval with no error or log explaining why.
Consider scoping the filter to bounce confirmations specifically, e.g. checking confirmationDetails.type === 'info' (set only by the bounce path) rather than the shared hideAlwaysAllow property.
— qwen3.7-max via Qwen Code /review
| // PreToolUse Hook — skipped on a post-'ask' re-execution (the hook | ||
| // already ran and the user already confirmed; re-firing would loop). | ||
| if (hooksEnabled && messageBus && !isPostAskReexecution) { | ||
| // Convert ApprovalMode to permission_mode string for hooks |
Collaborator
There was a problem hiding this comment.
[Suggestion] PostToolUse hook audit trail inconsistency on re-execution
On re-execution after a bounce, PreToolUse is skipped here (!isPostAskReexecution) but PostToolUse fires with the reused tool_use_id. The audit trail shows PreToolUse(ask, id=X) → PostToolUse(success, id=X) with no corresponding PreToolUse(allow). Monitoring hooks or compliance systems that correlate Pre/Post pairs by tool_use_id would see a tool that apparently executed successfully despite the hook requesting it be blocked — producing false positives and potential alert fatigue.
Consider either: (a) firing a synthetic PreToolUse event with permissionDecision:'allow' and a distinct tool_use_id before re-execution, or (b) adding an isPostAskReexecution: true flag to the PostToolUse payload so consumers can distinguish re-executions.
— qwen3.7-max via Qwen Code /review
| const call = this.toolCalls.find((c) => c.request.callId === callId); | ||
| if (call?.status !== 'awaiting_approval') { | ||
| continue; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] Abort strands scheduled siblings when a bounced tool is in awaiting_approval
drainSpansForBatch only cancels awaiting_approval calls here (if (call?.status !== 'awaiting_approval') continue). If tool A bounced to awaiting_approval and tool B remains scheduled (waiting in the while(true) loop), aborting only cancels A. Tool B stays scheduled indefinitely — checkAndNotifyCompletion doesn't consider scheduled as terminal, so batch-level hooks (PostToolBatch) never fire and spans remain open.
Consider also cancelling scheduled calls in this drain, or calling attemptExecutionOfScheduledCalls after the drain so the while loop re-enters and picks up scheduled siblings (which would then observe the abort signal and cancel themselves).
— qwen3.7-max via Qwen Code /review
| for (const batch of batches) { | ||
| if (batch.concurrent && batch.calls.length > 1) { | ||
| await this.runConcurrently(batch.calls, signal); | ||
| if (this.hasExecutingOrAwaitingApprovalCall()) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The hasExecutingOrAwaitingApprovalCall() guard after runConcurrently is never tested returning true
The existing concurrent test ("approving a bounced ask runs the tool even while a sibling is still executing") holds toolB pending via a manual promise, so runConcurrently doesn't return until toolB resolves — by which time toolA has already been re-scheduled and re-executed via handleConfirmationResponse. The guard returns through the allCallsFinalOrScheduled check, not through this post-runConcurrently check.
To cover this path: use two concurrent (Read) tools where toolA bounces on PreToolUse 'ask' and toolB completes immediately (no held promise), plus a third tool (Edit) in a later sequential batch. Assert toolC does NOT execute while toolA is awaiting approval.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 6, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
approved these changes
Jul 6, 2026
Collaborator
📸 Visual verification —
|
| Scenario | Expected | Observed |
|---|---|---|
Interactive ask |
native confirmation, not deny | box with hook reason; 2 options, no “always allow” |
| Approve | runs, turn continues | 1 execution, hook fired 1×, no loop |
| Decline (Esc) | cancelled, not executed | - Cancelled, 0 executions, no hang |
Non-interactive -p |
deny fallback, no hang | is_error + reason, 0 executions, exit 0 |
| Before (merge-base) | old code denied, no prompt | x auto-deny, no box |
| Unit tests | green | 258 + 157 + 62 = 477 |
No regressions or surprises: the change is well-scoped (deny/stop paths and the separate PermissionRequest hook are untouched) and every behavior in the Reviewer Test Plan reproduces on a real built CLI.
🇨🇳 中文版验证报告（点击展开）
📸 可视化验证 —— PreToolUse 的 ask → TUI 确认框（真实 tmux A/B，附截图）
这是对前两份文字报告（#issuecomment-4767476001、#issuecomment-4778198880）的“带截图”收尾。我重新构建了已合并的提交，并在真实构建的 CLI 上通过 tmux 对其父提交做了 A/B，这次把实际的 TUI 画面截了图。
结论：真实运行下的行为与 PR 描述完全一致 —— 批准后工具只执行一次（无重复询问循环），拒绝则取消，非交互回退为 deny，merge-base 则是旧的静默拒绝。 ✅
测试装置（确定性，无 LLM 随机性）
- 改动后（After） = 合并的 squash 提交
7281eb58f；改动前（Before） = 其父提交42921028f（即本 PR 合并前的 main）。两者仅相差本 PR 的 4 个文件，且package-lock.json完全相同，是可能范围内最紧的 A/B。 - 两侧各自构建真实
qwenCLI（scripts/build.js），通过tmux驱动，对接一个零依赖 mock OpenAI 服务器（第 1 轮返回一个run_shell_command工具调用，其 shell 向文件追加一行哨兵，使执行次数可计数；第 2 轮返回 stop），并配一个真实的PreToolUsecommand hook（matcher: "*"）返回permissionDecision:"ask"。 - 特意用
--approval-mode yolo：工具自身的确认被自动通过，因此唯一可能弹出的就是 hookask触发的确认 —— 从而隔离被测特性。（PR 里useGeminiStream的hideAlwaysAllow过滤，正是防止 YOLO 把这个 bounce 一并自动批准的关键。）
1 · 交互式 ask → 原生确认（After，YOLO 下）
见上方截图 1。确认框把 hook 的 permissionDecisionReason 作为正文，选项仅有 Yes, allow once / No, suggest changes，没有“always allow”（hideAlwaysAllow 生效，因为 hook 每次都会重新评估）。此时哨兵仍为空 —— 尚未执行任何东西。
2 · 批准 Yes, allow once → 工具恰好执行一次
见上方截图 2。✓ Shell 执行，模型继续输出 MOCK_TURN_DONE。整个“批准→重执行”周期中，哨兵文件被写入 1 次、hook 触发 1 次 —— 重执行会跳过 PreToolUse hook，因此没有重复询问循环。
3 · 拒绝（Esc）→ 取消，0 次执行
见上方截图 3。显示 -（Cancelled）字形；工具从未运行（哨兵为空），TUI 正常回到输入框，不挂起。
4 · 改动前（merge-base 42921028f）—— 同一 hook 被自动拒绝、无弹窗
见上方截图 4。显示 x（Error）：同样的 ask 被静默自动拒绝 —— hook 文案变成失败信息，工具从未运行，模型立即继续。本 PR 把它变成了截图 1 的样子。
另外验证（未截图）
- 非交互
qwen -p（同 hook，YOLO）：ask回退为 deny —— 模型收到的 tool result 即 hook 文案且is_error，0 次执行，exit 0（不挂起）。stderr：Tool "run_shell_command" requires user approval but cannot execute in non-interactive mode. - PR 单元测试（Linux，vitest）：
coreToolScheduler.test.ts258/258 ·useGeminiStream.test.tsx157/157 ·toolHookTriggers.test.ts62/62。
| 场景 | 预期 | 实测 |
|---|---|---|
交互式 ask |
原生确认（而非拒绝） | 弹出含 hook 文案的确认框；2 个选项，无“always allow” |
| 批准 | 执行，回合继续 | 执行 1 次，hook 触发 1 次，无循环 |
| 拒绝（Esc） | 取消，不执行 | - Cancelled，0 次执行，不挂起 |
非交互 -p |
回退为 deny，不挂起 | is_error + 文案，0 次执行，exit 0 |
| 改动前（merge-base） | 旧代码拒绝、无弹窗 | x 自动拒绝，无弹窗 |
| 单元测试 | 全绿 | 258 + 157 + 62 = 477 |
未见回归或意外：改动范围清晰（deny/stop 路径与独立的 PermissionRequest hook 均未改动），Reviewer Test Plan 中的每一条行为都在真实构建的 CLI 上复现。
Verified by @wenshao on Linux: real built CLI + zero-dep mock OpenAI server driven over tmux; After 7281eb58f vs merge-base 42921028f; screenshots rendered from live tmux capture-pane output. 🤖 Assisted by Claude Code — Claude Opus 4.8 (1M context).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
A
PreToolUsehook returningpermissionDecision: "ask"now surfaces a native TUI confirmation before the tool runs, instead of being treated like"deny". The hook fires in the execution phase (_executeToolCallBody), after the confirmation flow has already finished, so previously an"ask"could only block the tool as anEXECUTION_DENIEDerror. This change bounces the tool from the execution phase back into the existingawaiting_approvalflow: a syntheticinfoconfirmation whoseonConfirmroutes throughhandleConfirmationResponse(approve → re-execute, decline → cancelled).PreToolUsekeeps its "before execution" timing — only the"ask"branch is new;"denied"/"stop"keep deny-as-error, and the separatePermissionRequesthook is untouched. Non-interactive CLI and background agents cannot prompt, so"ask"falls back to deny there.Why it's needed
permissionDecision: "ask"is already part of the hook schema (PreToolUseHookOutput.isAsk()) and mirrors Claude Code, where anasklets the user confirm a tool in the TUI. In qwen-code it was silently equivalent todeny— the confirmation never appeared. This is the human-in-the-loop gate that external-injection / CI-driven workflows asked for (see #5424): push a proposed action and have the human approve it in the TUI before the agent acts.Reviewer Test Plan
How to verify
Configure a
PreToolUsecommand hook (matcher*) returning{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"confirm running this tool"}}, then drive the CLI so the model calls any tool. Interactive: a confirmation box appears showing the hook reason withYes, allow once/No, suggest changes(no "always allow", since the hook re-evaluates every call); approving runs the tool once, declining cancels it. Non-interactive (-p/ background agent): theaskfalls back to deny (tool_resultis_error), no hang.Evidence (Before & After)
@wenshao independently built the PR branch and ran a real tmux A/B against the merge-base with a deterministic mock model: #5629 (comment) — Before (merge-base): the same hook produced no prompt and the tool was auto-denied; After (this PR): the interactive
askrenders a confirmation, approve runs the tool exactly once (no re-ask loop), and non-interactive falls back to deny with exit 0.Tested on
macOS: unit tests (
coreToolScheduler.test.ts,toolHookTriggers.test.ts) plus @wenshao's real-CLI tmux run. Windows/Linux: covered by CI (cross-platform unit tests green).Environment (optional)
Unit tests only locally; @wenshao's run used a real built CLI + mock OpenAI server over tmux.
Risk & Scope
coreToolScheduler.ts); the bounce reuses the existingawaiting_approvalmachinery rather than adding a parallel path. Deny/stop and thePermissionRequesthook are unchanged.reasoncarries the message) — a separable enhancement.askpreviously denied; it now prompts (interactive) or still denies (non-interactive).Linked Issues
Relates to #5424.
Relates to #6321.
中文说明
这个 PR 做了什么
PreToolUsehook 返回permissionDecision: "ask"时，现在会在工具执行前弹出原生 TUI 确认，而不再等同于"deny"。hook 在执行阶段（_executeToolCallBody）触发，此时确认流程已结束，所以之前"ask"只能把工具当成EXECUTION_DENIED拦掉。本改动把工具从执行阶段打回现有的awaiting_approval流程：构造一个info确认，其onConfirm走handleConfirmationResponse（批准→重执行，拒绝→取消）。PreToolUse保持"执行前"的触发时机——只有"ask"分支是新增的，"denied"/"stop"仍是 deny-as-error，独立的PermissionRequesthook 不受影响。非交互 CLI 和后台 agent 无法弹确认，"ask"在那里回退为 deny。为什么需要
permissionDecision: "ask"本就是 hook schema 的一部分（PreToolUseHookOutput.isAsk()），对齐 Claude Code——ask让用户在 TUI 里确认工具。但在 qwen-code 里它静默等同于deny，确认框从不出现。这正是外部注入 / CI 驱动工作流所需的人在环路闸门（见 #5424）：推送一个待执行动作，让人在 TUI 批准后 agent 才执行。Reviewer Test Plan
如何验证
配置一个
PreToolUsecommand hook（matcher*）返回{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"confirm running this tool"}}，然后驱动 CLI 让模型调用任意工具。交互模式：弹出确认框显示 hook reason，选项Yes, allow once/No, suggest changes（无"始终允许"，因为 hook 每次重新评估）；批准后工具执行一次，拒绝则取消。非交互（-p/ 后台 agent）：ask回退为 deny（tool_resultis_error），不挂起。证据（前后对比）
@wenshao 独立构建本 PR 分支，用真实 tmux + 确定性 mock 模型对 merge-base 做了 A/B：#5629 (comment) —— 前（merge-base）：同样的 hook 没有弹窗，工具被自动拒绝；后（本 PR）：交互式
ask弹出确认，批准后工具恰好执行一次（无重复询问循环），非交互回退为 deny 且 exit 0。测试平台
macOS：单元测试（
coreToolScheduler.test.ts、toolHookTriggers.test.ts）加 @wenshao 的真实 CLI tmux 运行。Windows/Linux：由 CI 覆盖（跨平台单测绿）。环境（可选）
本地仅单元测试；@wenshao 的运行用了真实构建的 CLI + mock OpenAI 服务器，通过 tmux 驱动。
风险与范围
coreToolScheduler.ts）；bounce 复用现有awaiting_approval机制而非新增并行路径。Deny/stop 与PermissionRequesthook 不变。reason已承载文案）——属于可单独提的增强。ask之前是拒绝；现在交互模式弹确认、非交互仍拒绝。关联 Issue
Relates to #5424.
Relates to #6321.