Qwen Code review did not complete successfully: Qwen review timed out after 85 minutes. See workflow logs.

@qwen-code /triage

Thanks for the PR @doudouOUC!

Template looks good ✓

On direction: this fixes a real user-facing bug — after the 5-minute ask_user_question timeout, the model keeps executing without the user's answer, which is a broken experience. Claude Code's CHANGELOG has several related cancellation/subagent-stop fixes, confirming this area is squarely within scope. Clear direction alignment.

On approach: the scope feels right for what it's solving. The cancellation signal needs to propagate through four distinct layers (top-level prompt loop → sequential tool batches → concurrent Agent siblings → nested sub-agent questions), and the diff addresses each with a dedicated mechanism (abort controllers, RunToolResult wrapper, onAskUserQuestionCancel callback, #preserveCancelledAskUserQuestionToolRun). The test count is high (~1000 lines) but each test covers a distinct propagation path — that's justified for a fix that touches this many concurrency boundaries. No unrelated changes or drive-by refactors spotted.

One question worth thinking about: the onAskUserQuestionCancel callback is threaded through SubAgentTracker constructor and then into runTool/runBounded as an optional parameter. This works, but it adds a new optional parameter at three levels. An alternative would be an AbortController with a sentinel reason (like the existing USER_CANCEL_ABORT_REASON) shared between the tracker and the batch runner, checked at the same points. That might reduce the parameter surface. Not a blocker — just a thought for the author to consider.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Reviewed the diff against my independent proposal. The problem requires cancelling the turn at four distinct concurrency boundaries (top-level prompt loop, sequential batches, concurrent Agent batches, nested sub-agent questions), and the PR handles each with a dedicated mechanism. The RunToolResult wrapper is the cleanest way to thread the stop signal through the existing return types without refactoring runToolCalls into a generator or event emitter.

No critical blockers found. Specific observations:

• Abort controller layering is correct. The per-batch batchAbortController wraps the parent abortSignal, and the per-Agent agentToolAbortController wraps the tool-level signal. Cleanup happens in finally blocks and removeEventListener calls — no leaked listeners.
• recordSkippedToolCall is resilient. It catches client notification failures (e.g. disconnected ACP client) but still records the skipped response in history. The test preserves skipped tool responses when skipped tool updates fail covers this explicitly.
• Hook semantics are consistent. PostToolUse success hooks are suppressed for cancelled/aborted tools, PostToolUseFailure fires with isInterrupt: true — matches the core path in coreToolScheduler.ts.
• SubAgentTracker onAskUserQuestionCancel ordering is correct. The callback fires before event.respond() in the success path (parent learns about cancel first) and is wrapped in try/catch in the error path. The new test verifies ordering via mock.invocationCallOrder.
• Sequential batch path doesn't pass onStopAfterUserQuestionCancel — intentionally, because the outer for loop in runToolCalls checks r.stopAfterUserQuestionCancel after each tool and skips the rest. Correct.

One minor note: the onAskUserQuestionCancel callback threads through three levels (SubAgentTracker constructor → runTool parameter → runBounded parameter). It works, but a shared AbortController with a sentinel reason might be slightly cleaner. Already flagged in Stage 1 — not a blocker.

Tests

Unit tests: 173/173 passed (Session.test.ts: 145, SubAgentTracker.test.ts: 28). The new tests cover all cancellation propagation paths: direct cancel, sequential skip, concurrent batch abort, nested agent cancel, sibling abort, unstarted skip, hook suppression, interrupt labeling, and rewrite-wait-before-end.

Build: ✅ exit 0 (lint warnings are pre-existing in vscode-ide-companion, unrelated to this PR)

Typecheck: ✅ exit 0 across all packages

Real-Scenario Testing

Basic smoke test — npm run dev -- -p 'Say hello in one word' before (main) and after (PR branch). Both produce identical output, confirming no regression in the normal execution path.

Before (installed build — main branch)

runner@runnervm1li68:~/work/qwen-code/qwen-code$ npm run dev -- -p 'Say hello in one word' 2>&1 | tail -20

> @qwen-code/[email protected] dev
> node scripts/dev.js -p Say hello in one word

DEV is set to true, but the React DevTools server is not running. Start it with:

$ npx react-devtools

Hello!
runner@runnervm1li68:~/work/qwen-code/qwen-code$

After (this PR)

runner@runnervm1li68:~/work/qwen-code/qwen-code/.qwen/worktrees/triage$ npm run dev -- -p 'Say hello in one word' 2>&1 | tail -20

> @qwen-code/[email protected] dev
> node scripts/dev.js -p Say hello in one word

DEV is set to true, but the React DevTools server is not running. Start it with:

$ npx react-devtools

Hello!
runner@runnervm1li68:~/work/qwen-code/qwen-code/.qwen/worktrees/triage$

Limitation: the specific bug (ask_user_question 5-minute timeout → model continues executing) requires an ACP client to cancel the permission request, which isn't reproducible in a simple tmux prompt. The 173 unit tests thoroughly cover this cancellation path at every concurrency boundary.

— Qwen Code · qwen3.7-max

Reflection

This PR fixes a real, user-reported bug: after the 5-minute ask_user_question timeout, the model keeps running without the user's answer. The fix is well-scoped — every change traces back to propagating a cancellation signal through the ACP session's concurrency layers.

Going back to my independent proposal: I would have used a shared AbortController with a sentinel reason, checked at the same boundaries. The PR takes a slightly different path — a RunToolResult wrapper plus an onAskUserQuestionCancel callback — which is more explicit about why the turn is stopping (not just "aborted" but specifically "user question was cancelled"). That explicitness is a net positive for maintainability. The #preserveCancelledAskUserQuestionToolRun helper, which I wouldn't have thought to add, ensures replay correctness — a detail that matters for daemon sessions.

The test suite (173 tests, ~1000 lines of additions) is large, but each test isolates a distinct propagation path: direct cancel, sequential skip, concurrent batch abort, nested agent, sibling abort, unstarted skip, hook suppression, interrupt labeling, rewrite-wait, cron stop. For a fix that touches this many concurrency boundaries, the coverage is insurance, not bloat.

Build, typecheck, and smoke test all pass. No critical blockers in code review.

The one reservation from Stage 1 stands — the callback threading through three constructor/parameter levels is slightly more surface area than a pure abort-controller approach would need. But it's a stylistic choice, not a correctness issue, and the author clearly thought about it (the callback name is descriptive, the parameter is optional at every level).

Verdict: this is a solid, well-tested fix for a real bug. Ready to ship. ✅

— Qwen Code · qwen3.7-max

Qwen Code review did not complete successfully: Qwen review timed out after 85 minutes. See workflow logs.