@qwen-code /triage

✅ Local runtime verification — fix confirmed

I verified this PR locally with real test runs under tmux, driving the actual production Session code (runToolCalls turn-orchestration) via a git worktree at the PR head with an A/B against the base. Verdict: the fix is correct and the new tests genuinely catch the regression — safe to merge.

The behavior change

Before: only ask_user_question cancellation set the turn-stop flag. For any other permissioned tool (shell/file/MCP), a cancelled — or daemon-timed-out → cancelled — permission returned stopAfterUserQuestionCancel: false, so later tool calls in the same model response still ran. After: any Cancel outcome (including reject→Cancel, a thrown requestPermission, or a failed cancellation onConfirm) calls onStopAfterPermissionCancel and returns stopAfterPermissionCancel: true, so later tools are skipped with the permission-cancel skip message; nested subagent permission cancellation is generalized the same way (onPermissionCancel fires for any tool, fail-closed).

How I verified

Worktree at PR head 3d1dafcca (its base 5c87b5649 postdates my local checkout, so the surrounding tree — e.g. midTurnUserMessage.ts — only exists there); node_modules symlinked from the main repo. For each A/B I swap only the two source files (Session.ts, SubAgentTracker.ts) between base and PR and keep the PR's tests.

1) PR shipped state — green

Session.test.ts + SubAgentTracker.test.ts on PR source: 187 passed (159 + 28), 0 failed.

2) The new tests are genuine regression tests (base ✗)

PR test files against base source: 25 failed / 162 passed. Those 25 are the 10 new permission-cancellation tests (non-question cancel / request-failure / reject-option / confirm-cleanup-failure / nested-subagent variants, + the 3 SubAgentTracker ones) plus 15 pre-existing tests the PR updated to the renamed field/message (stopAfterUserQuestionCancel→stopAfterPermissionCancel, new skip text). All 10 new behavioral tests fail on base.

3) Rename-agnostic behavioral isolation (the load-bearing proof)

Because the renamed-field assertion trips first on base, I added one probe that asserts only the tool-execution side effects (never the renamed field/message) and ran it on both sources — driving the real session.runToolCalls([shell(cancelled), read_file]):

This is the exact fix, isolated from the rename: on base a non-question permission cancel lets the next tool run; on the PR it is skipped.

4) Typecheck — no new type errors

After npm run generate + building core/acp-bridge in the worktree, base and PR source produce the identical error set (diff empty). The residual errors are pre-existing worktree acp-bridge dist-staleness in files the PR doesn't touch (e.g. the Session.ts:102 MID_TURN_QUEUE_DRAIN_METHOD import is outside the PR's diff). PR 5258 adds zero type errors.

Notes for the maintainer (non-blocking)

• Verification level: I exercised the real Session.runToolCalls turn loop with a faithful ACP-client + tool-registry mock (the project's own integration harness), plus the behavioral probe above — this directly runs the changed orchestration. I did not re-run the full daemon HTTP/SSE E2E from the test plan; the PR author's macOS daemon evidence (one permission_request, first tool cancelled, second skipped, end_turn, sentinels absent) is consistent with what the Session-level A/B shows.
• base ✗ count nuance: 25 (not 10) tests fail on base because the PR also renames the result field/skip message that ~15 pre-existing tests assert on. The behavioral change itself is isolated by the probe in (3).
• The fail-closed hardening is sound: requestPermission and the cancellation onConfirm are now wrapped so a transport failure stops the turn instead of silently continuing, and cleanupAgentToolResources() now also runs on the early-error path (covered by cleans up Agent sub-agent listeners when permission request fails before execution).

Recommendation: correct, well-tested, prioritizes the user's cancellation as intended. 👍 to merge.

_{Verified by running the project's ACP Session/SubAgentTracker test suites under tmux in a PR-head worktree, base↔PR source A/B, + a rename-agnostic behavioral probe on the real runToolCalls. Source diff is the ask_user_question→generalized-permission cancel change in Session.ts + SubAgentTracker.ts.}

@qwen-code /triage

Thanks for the PR, @doudouOUC!

Template looks good ✓

On direction: this is a clear safety/correctness fix and a natural follow-up to #5218. When a user cancels a permission prompt (or it times out), the model should not silently continue executing subsequent tools — that violates the user's intent and creates a confusing experience where the UI shows "cancelled" but tools still run. This is squarely within the project's core mission around safe tool permissions. CHANGELOG has #5218 as the direct predecessor — this PR completes the work by removing the ask_user_question-only guard.

On approach: the scope feels right. The core change is removing a toolName === ToolNames.ASK_USER_QUESTION guard and generalizing the rename from stopAfterUserQuestionCancel to stopAfterPermissionCancel. The added error handling for requestPermission failures and onConfirm failures are part of the same "fail closed" principle. The cleanupAgentToolResources() extraction is a genuine dedup — the same cleanup was copy-pasted in error and success paths. No unrelated changes or scope creep.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

The change is straightforward and well-contained. Four files, all in acp-integration/session/ — the permission handling layer.

What it does: Removes the toolName === ToolNames.ASK_USER_QUESTION guard so that cancelling ANY permission (shell, file, MCP, etc.) stops the turn and skips subsequent tool calls. Previously only ask_user_question cancellations triggered this behavior.

Independent proposal vs PR approach: My independent proposal matched the PR almost exactly — remove the tool name guard, rename stopAfterUserQuestionCancel → stopAfterPermissionCancel, apply in SubAgentTracker, add try/catch for requestPermission failures. The PR goes slightly further with cleanupAgentToolResources() (genuine dedup of cleanup code that was copy-pasted in error and success paths) and error handling for onConfirm failures (correct fail-closed behavior).

No critical issues found. The rename is complete (zero stale references to stopAfterUserQuestionCancel or ASK_USER_QUESTION_CANCEL_SKIP_MESSAGE in the codebase). Error handling in the new requestPermission catch block is correct — it calls onConfirm(Cancel) to notify the tool, then stops the turn. The onConfirm catch for cancellation failures is also correct — logs and returns stopAfterPermissionCancel() rather than letting it bubble.

One minor note: the onConfirm error in the cancel path is swallowed (logged but not re-thrown). This is intentional — if the user cancelled, we want to stop the turn regardless of whether onConfirm succeeds. Good judgment.

Tests

Unit tests (PR code):

• Session.test.ts: 159 passed (0 failed) — includes new tests for non-question permission cancel, reject option cancel, onConfirm cleanup failure, permission request failure, and nested non-question permission cancel
• SubAgentTracker.test.ts: 28 passed (0 failed) — includes updated tests verifying all tool types (not just ask_user_question) trigger the cancel callback

Typecheck: Clean across all packages (cli, core, sdk, webui).

Tmux Smoke Test

$ npm run dev -- -p 'echo hello from PR 5258'

> @qwen-code/[email protected] dev
> node scripts/dev.js -p echo hello from PR 5258

DEV is set to true, but the React DevTools server is not running.
Start it with:
$ npx react-devtools

Hello! How can I help you with PR #5258?

CLI starts and responds normally. The specific permission cancellation behavior requires the daemon/WebShell setup described in the PR's test plan (QWEN_SERVER_TOKEN=codex-local-verify npm run dev:daemon -- --port 4170), which is not available in this CI environment. The behavior is thoroughly covered by the 187 passing unit tests including dedicated tests for each cancellation scenario (non-question cancel, reject option, onConfirm failure, permission request failure, nested permissions).

$ npm run dev -- -p 'echo hello from PR 5258'

> @qwen-code/[email protected] dev
> node scripts/dev.js -p echo hello from PR 5258

DEV is set to true, but the React DevTools server is not running.
Start it with:
$ npx react-devtools

Hello! How can I help you with PR #5258?

— Qwen Code · qwen3.7-max

Reflection

This is a clean, well-scoped behavior fix that closes a real safety gap. The predecessor PR (#5218) stopped the turn only for ask_user_question cancellations — this PR generalizes it to all permissioned tools, which is the correct and expected behavior. If a user cancels a shell command permission, subsequent tools in the same turn should not silently continue executing.

The implementation is minimal and focused: remove a tool name guard, rename for generality, add fail-closed error handling for requestPermission and onConfirm failures. The cleanupAgentToolResources() extraction is a genuine improvement that eliminates duplicated cleanup code. No scope creep, no drive-by refactors.

The test coverage is thorough — 187 tests pass, with dedicated tests for each cancellation scenario (non-question cancel, reject option, onConfirm failure, permission request failure, nested permissions). Typecheck is clean. The CLI smoke test confirms it builds and runs.

I'd approve this. It solves a real user problem, the approach matches what I'd independently propose, and the code is maintainable.

Verdict: Approve ✅

— Qwen Code · qwen3.7-max