feat: implement SDK canUseTool interrupt handling#2771
Open
Mingholy wants to merge 3 commits into
Open
Conversation
Mingholy
requested review from
DennisYu07,
DragonnZhang,
LaZzyMan,
gwinthis,
pomelo-nwu and
tanzhenxin
as code owners
Contributor
📋 Review SummaryThis PR implements interrupt handling for SDK 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
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. |
wenshao
reviewed
Apr 6, 2026
Comment on lines
313
to
+391
| @@ -365,11 +374,53 @@ export async function runNonInteractive( | |||
|
|
|||
| adapter.emitToolResult(finalRequestInfo, toolResponse); | |||
|
|
|||
| // Track response for interrupt detection | |||
| toolResponses.push({ | |||
| request: finalRequestInfo, | |||
| response: toolResponse, | |||
| }); | |||
|
|
|||
| if (toolResponse.responseParts) { | |||
| toolResponseParts.push(...toolResponse.responseParts); | |||
| } | |||
| } | |||
| currentMessages = [{ role: 'user', parts: toolResponseParts }]; | |||
|
|
|||
| // Check for interrupt - any tool with INTERRUPTED error type | |||
| const hasInterrupt = toolResponses.some( | |||
| ({ response }) => response.errorType === ToolErrorType.INTERRUPTED, | |||
Collaborator
There was a problem hiding this comment.
[Critical] Interrupt does not stop batch tool execution — subsequent tools still run after interrupt.
The hasInterrupt check runs outside the for (const requestInfo of toolCallRequests) loop (after line ~386). Additionally, cancelPendingTools() operates on this.toolCalls within a single CoreToolScheduler instance, but executeToolCall() creates a new isolated scheduler per tool. These two issues together mean:
- If the LLM returns 5 tool calls and tool pre-release: fix ci #1 triggers
interrupt: true, tools Where is the config saved? #2–TypeError in Authentication Selection Interface #5 still execute sequentially before the check fires. cancelPendingTools()cannot cancel tools Where is the config saved? #2–TypeError in Authentication Selection Interface #5 because they live in different scheduler instances.
Suggested fix: Move the interrupt check inside the loop and break early:
for (const requestInfo of toolCallRequests) {
// ... existing executeToolCall logic ...
const toolResponse = await executeToolCall(config, finalRequestInfo, abortController.signal, { ... });
adapter.emitToolResult(finalRequestInfo, toolResponse);
toolResponses.push({ request: finalRequestInfo, response: toolResponse });
// Check for interrupt immediately after each tool
if (toolResponse.errorType === ToolErrorType.INTERRUPTED) {
break;
}
if (toolResponse.responseParts) {
toolResponseParts.push(...toolResponse.responseParts);
}
}
wenshao
reviewed
Apr 6, 2026
Comment on lines
+1185
to
+1195
| // or a regular UI cancellation | ||
| const isPermissionDenial = payload && 'cancelMessage' in payload; | ||
|
|
||
| if (isPermissionDenial) { | ||
| // Permission denial from canUseTool - use error status | ||
| const errorType = payload?.interrupt | ||
| ? ToolErrorType.INTERRUPTED | ||
| : ToolErrorType.EXECUTION_DENIED; | ||
|
|
||
| const errorResponse = createErrorResponse( | ||
| toolCall.request, |
Collaborator
There was a problem hiding this comment.
[Critical] isPermissionDenial detection misclassifies infrastructure errors as permission denials.
The cancel handler uses payload && 'cancelMessage' in payload to distinguish SDK permission denial from regular UI cancellation. However, the catch block in permissionController.handleOutgoingPermissionRequest also sets cancelMessage for any error (timeouts, network failures, unexpected exceptions):
// permissionController.ts catch block - any error gets cancelMessage
await toolCall.confirmationDetails.onConfirm(
ToolConfirmationOutcome.Cancel,
{ cancelMessage: error.message } // No interrupt field
);This means infrastructure errors will have isPermissionDenial = true and get ToolErrorType.EXECUTION_DENIED instead of the previous 'cancelled' status. On main, these scenarios would result in status: 'cancelled'; this PR changes them to status: 'error' with errorType: 'execution_denied'. This is a behavioral regression for error reporting.
Suggested fix: Use an explicit discriminator field instead of inferring intent from cancelMessage presence:
// In permissionController.ts when SDK returns a denial:
const confirmPayload: {
cancelMessage?: string;
interrupt?: boolean;
isPermissionDenial?: boolean;
} = {};
if (cancelMessage) confirmPayload.cancelMessage = cancelMessage;
if (interrupt) confirmPayload.interrupt = true;
confirmPayload.isPermissionDenial = true;
// In coreToolScheduler.ts cancel handler:
const isPermissionDenial = payload?.isPermissionDenial === true;The catch block in permissionController should NOT set isPermissionDenial.
wenshao
reviewed
Apr 6, 2026
| // Cascade-cancelled tools use INTERRUPTED type with a unified message | ||
| const errorResponse = createErrorResponse( | ||
| pendingTool.request, | ||
| new Error("The user doesn't want to take this action right now."), |
Collaborator
There was a problem hiding this comment.
[Suggestion] Cascade-cancel message loses original denial context.
All cascade-cancelled tools receive the hardcoded message "The user doesn't want to take this action right now." regardless of why the original interrupt was triggered. If canUseTool denied tool A with "Write operations are not allowed in /etc", the cascade-cancelled tool B gets a generic message. SDK consumers inspecting permission_denials lose contextual information.
Suggested fix: Pass the original denial message to cancelPendingTools:
private async cancelPendingTools(
signal: AbortSignal,
triggeringCallId: string,
originalMessage: string,
): Promise<void> {
// ...
const errorResponse = createErrorResponse(
pendingTool.request,
new Error(`Cancelled: ${originalMessage}`),
ToolErrorType.INTERRUPTED,
);
}
wenshao
reviewed
Apr 6, 2026
Comment on lines
+1606
to
+1637
| debug: false, | ||
| }, | ||
| }); | ||
|
|
||
| const messages: SDKMessage[] = []; | ||
| let processExited = false; | ||
|
|
||
| try { | ||
| for await (const message of q) { | ||
| messages.push(message); | ||
| if (isSDKResultMessage(message)) { | ||
| resultMessages.push(message); | ||
| } | ||
| } | ||
| } catch { | ||
| // CLI may exit with code 130 (SIGINT) when interrupt is triggered | ||
| // This is expected behavior | ||
| processExited = true; | ||
| } finally { | ||
| await q.close(); | ||
| } | ||
|
|
||
| // Verify canUseTool was called for write_file | ||
| expect(canUseToolCalls).toContain('write_file'); | ||
|
|
||
| // Either we got a result message or the process exited (both are valid behaviors) | ||
| if (resultMessages.length > 0) { | ||
| const resultMsg = resultMessages[0]; | ||
| expect(isSDKResultMessage(resultMsg)).toBe(true); | ||
|
|
||
| if (isSDKResultMessage(resultMsg)) { | ||
| // Should be error_during_execution |
Collaborator
There was a problem hiding this comment.
[Suggestion] Test does not effectively verify cascade-cancel behavior.
The test 'should cascade-cancel pending tools when interrupt is triggered' allows file1.txt and file3.txt to be auto-approved, only denies file2.txt. It checks that file2.txt was not modified but has // file1.txt and file3.txt may or may not be modified depending on execution order. The test does not verify that canUseTool was NOT called for the cascade-cancelled tools, which would be the actual evidence that cascade cancellation worked.
Suggested fix: Track which tools canUseTool was actually called for. After the interrupt triggers for file2.txt, assert that canUseTool was NOT called for file3.txt:
const canUseToolCalls: {tool: string, input: unknown}[] = [];
// ... in canUseTool callback:
canUseToolCalls.push({tool: toolName, input: toolInput});
// ... after execution:
const indexOfInterrupt = canUseToolCalls.findIndex(
c => c.tool === 'write_file' && /* was the one denied with interrupt */
);
const callsAfterInterrupt = canUseToolCalls.filter((_, i) => i > indexOfInterrupt);
expect(callsAfterInterrupt.length).toBe(0);
wenshao
requested changes
Apr 6, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Code Review: PR #2771 - feat: implement SDK canUseTool interrupt handling
Summary
This PR adds interrupt handling for SDK canUseTool denials. The concept is sound and the type additions (ToolErrorType.INTERRUPTED, interrupt?: boolean) are clean. However, the core interrupt mechanism has a critical design flaw in batch tool call scenarios: the interrupt check runs after all tools in a batch have already executed, and cancelPendingTools() operates on an isolated per-tool scheduler, so it cannot cancel sibling tools. Additionally, the permission denial detection logic may misclassify infrastructure errors.
6 findings reported, 6 confirmed (6 high confidence) after independent verification.
Deterministic analysis: npm run lint and tsc --noEmit passed cleanly on all changed files.
Critical Findings
1. Interrupt does not stop batch tool execution — subsequent tools still run after interrupt
File: packages/cli/src/nonInteractiveCli.ts:319-391 and packages/core/src/core/coreToolScheduler.ts:1736-1759
The hasInterrupt check runs outside the for (const requestInfo of toolCallRequests) loop. Additionally, cancelPendingTools() operates on this.toolCalls within a single CoreToolScheduler instance, but executeToolCall() creates a new isolated scheduler per tool. These two issues together mean:
- If the LLM returns 5 tool calls and tool #1 triggers
interrupt: true, tools #2–#5 still execute sequentially before the check fires cancelPendingTools()cannot cancel tools #2–#5 because they live in different scheduler instances
The primary design goal — immediately terminating the request when interrupt: true — is not achieved for batch tool calls.
2. isPermissionDenial detection misclassifies infrastructure errors as permission denials
File: packages/core/src/core/coreToolScheduler.ts:1185-1195 and packages/cli/src/nonInteractive/control/controllers/permissionController.ts:449-475
The cancel handler uses payload && 'cancelMessage' in payload to distinguish SDK permission denial from regular UI cancellation. However, the catch block in permissionController.handleOutgoingPermissionRequest also sets cancelMessage for any error (timeouts, network failures, unexpected exceptions), causing infrastructure errors to be classified as EXECUTION_DENIED instead of 'cancelled'. This is a behavioral regression.
Suggestions
3. Cascade-cancel message loses original denial context
File: packages/core/src/core/coreToolScheduler.ts:1750
All cascade-cancelled tools receive a hardcoded generic message regardless of the original denial reason.
4. Test does not effectively verify cascade-cancel behavior
File: integration-tests/sdk-typescript/tool-control.test.ts:1606-1637
The test checks file modification but does not verify that canUseTool was NOT called for cascade-cancelled tools. The primary new behavior (cascade-cancel) has no effective test coverage.
Verdict
Request changes — Critical issues found with the core interrupt mechanism not functioning as designed for batch tool calls, and a behavioral regression in error classification for infrastructure failures.
Reviewed by qwen3.6-plus via Qwen Code /review
Add support for handling interrupt flag from SDK canUseTool callback:
- Add ToolErrorType.INTERRUPTED enum for tracking user-initiated interruptions
- Update ToolConfirmationPayload to include optional interrupt field
- Parse and pass interrupt flag from can_use_tool response in permissionController
- Handle interrupt in coreToolScheduler with proper error type and cascade cancellation
- Update nonInteractiveCli to detect INTERRUPTED errors and break the loop
- Track INTERRUPTED and EXECUTION_DENIED errors in permission_denials
- Fix: add tool responses to history before interrupt to preserve request/response pairs
This enables SDK users to interrupt the conversation flow by returning
{ behavior: 'deny', interrupt: true } from canUseTool callback.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Add INTERRUPTED error type for tool denial with interrupt flag - Update permission controller to parse and pass interrupt field from SDK - Implement cascade cancellation of pending tools on interrupt - Add permission_denials field to ResultOptions type - Add integration tests for canUseTool deny and interrupt behavior - Fix lint errors in tool-control.test.ts Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… messages - Add [PermissionDenied] prefix to permission denial messages for proper distinction - Propagate original denial message to cascade-cancelled tools in interrupt scenarios - Clean up test files by removing unnecessary debug options
Mingholy
force-pushed
the
fix-sdk-interrupt
branch
from
d7eea6e to
cd424d6
Compare
wenshao
reviewed
Apr 19, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
packages/cli/src/utils/errors.ts:137
[Suggestion] The new ToolErrorType.INTERRUPTED path is not treated as a permission-denial warning in non-interactive text mode. handleToolError() still special-cases only EXECUTION_DENIED, even though this PR classifies SDK interrupt denials as permission-style denials everywhere else.
Why it matters: users get inconsistent UX — a denied tool with interrupt: false shows the existing approval guidance, while the same denial with interrupt: true looks like a generic execution error and skips that guidance.
Suggested fix: treat ToolErrorType.INTERRUPTED the same as EXECUTION_DENIED in handleToolError(), and add/update tests in packages/cli/src/utils/errors.test.ts for the interrupt case.
— gpt-5.4 via Qwen Code /review
wenshao
requested changes
May 3, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Additional finding (multi-file): The entire interrupt handling path lacks debug logging. No debugLogger calls exist in: the if (toolResponse.errorType === ToolErrorType.INTERRUPTED) branch in nonInteractiveCli.ts, the cascade-cancellation loop, the hasInterrupt early-return block, or the cancelPendingTools() method in coreToolScheduler.ts. This makes production debugging impossible — the only trace is exit code 130, indistinguishable from a normal SIGINT. Please add debugLogger.info() calls at key junctions.
| const remainingTools = toolCallRequests.slice( | ||
| toolCallRequests.indexOf(requestInfo) + 1, | ||
| ); | ||
| for (const remainingTool of remainingTools) { |
Collaborator
There was a problem hiding this comment.
[Critical] The cascade-cancellation loop builds interruptedResponse for remaining tools and appends to toolResponseParts (model history), but never calls adapter.emitToolResult(remainingTool, interruptedResponse). Since BaseJsonOutputAdapter.emitToolResult() is the sole entry point that appends to this.permissionDenials, cascade-cancelled tools are entirely absent from the final permission_denials output. Only the triggering tool is recorded.
Also, all cascade-cancelled tools inherit the triggering tool's error message (toolResponse.error?.message) as their functionResponse.response.output — file3's response would say "User denied writing to file2.txt", which is misleading.
Suggested change
| for (const remainingTool of remainingTools) { | |
| for (const remainingTool of remainingTools) { | |
| const cascadeMessage = `Cascade-interrupted due to denial of ${finalRequestInfo.name}`; | |
| const interruptedResponse: ToolCallResponseInfo = { | |
| callId: remainingTool.callId, | |
| responseParts: [{ | |
| functionResponse: { | |
| id: remainingTool.callId, | |
| name: remainingTool.name, | |
| response: { error: cascadeMessage }, | |
| }, | |
| }], | |
| resultDisplay: cascadeMessage, | |
| error: new Error(cascadeMessage), | |
| errorType: ToolErrorType.INTERRUPTED, | |
| }; | |
| adapter.emitToolResult(remainingTool, interruptedResponse); | |
| toolResponses.push({ request: remainingTool, response: interruptedResponse }); | |
| if (interruptedResponse.responseParts) { | |
| toolResponseParts.push(...interruptedResponse.responseParts); | |
| } | |
| } |
— deepseek-v4-pro via Qwen Code /review
| durationMs: Date.now() - startTime, | ||
| apiDurationMs: totalApiDurationMs, | ||
| numTurns: turnCount, | ||
| errorMessage: 'Request interrupted by user for tool use', |
Collaborator
There was a problem hiding this comment.
[Suggestion] The errorMessage is hardcoded to 'Request interrupted by user for tool use', discarding whatever specific reason the SDK's canUseTool callback returned (e.g., 'User denied writing to /etc/passwd'). SDK consumers who need to differentiate denial reasons in result.error.message lose critical context.
Suggested change
| errorMessage: 'Request interrupted by user for tool use', | |
| errorMessage: toolResponse.error?.message || 'Request interrupted by user for tool use', |
— deepseek-v4-pro via Qwen Code /review
| // Determine if this is a permission denial from canUseTool | ||
| // Only cancelMessages with [PermissionDenied] prefix are treated as permission denials | ||
| // All other cases (infrastructure errors, regular UI cancellations) are treated as regular cancellations | ||
| const isPermissionDenial = |
Collaborator
There was a problem hiding this comment.
[Suggestion] The isPermissionDenial determination relies on a raw string prefix match: payload?.cancelMessage?.startsWith('[PermissionDenied] '). This creates implicit cross-package coupling between permissionController.ts (which constructs \[PermissionDenied] ${sdkMessage}`) and this file (which parses it). A format change in either location silently breaks the interrupt flow — permission denials degrade to regular cancellations and interrupt: true` is ignored.
Consider adding an explicit field to ToolConfirmationPayload (e.g., denialSource: 'permission_denied') or extracting '[PermissionDenied] ' as a shared constant.
— deepseek-v4-pro via Qwen Code /review
| * Cascade-cancelled tools are marked with INTERRUPTED error type. | ||
| * @param originalDenialMessage - The original denial reason to include in cascade-cancelled tool messages | ||
| */ | ||
| private async cancelPendingTools( |
Collaborator
There was a problem hiding this comment.
[Suggestion] cancelPendingTools is declared async but contains no await — causing unnecessary Promise allocation. It also accepts a signal: AbortSignal parameter that is never used in the method body, suggesting incomplete implementation or dead code.
Suggested change
| private async cancelPendingTools( | |
| private cancelPendingTools( | |
| triggeringCallId: string, | |
| originalDenialMessage?: string, | |
| ): void { |
And update the call site from await this.cancelPendingTools(signal, callId, actualMessage) to this.cancelPendingTools(callId, actualMessage).
— deepseek-v4-pro via Qwen Code /review
| // File should NOT be modified (write was denied) | ||
| const content = await helper.readFile('test.txt'); | ||
| expect(content).toBe('test content'); | ||
| }, |
Collaborator
There was a problem hiding this comment.
[Critical] The 'should return EXECUTION_DENIED when canUseTool returns deny without interrupt' test is missing key assertions that its sibling interrupt: true test includes. It verifies processExited === false, resultMessages.length > 0, and file content is unchanged — but never checks resultMsg.subtype (should be 'error_during_execution'), resultMsg.is_error (should be true), or that resultMsg.permission_denials contains an entry for 'write_file'. The test would pass even if the model simply chose not to call the tool (no denial at all).
Suggested change
| }, | |
| expect(resultMsg.subtype).toBe('error_during_execution'); | |
| expect(resultMsg.is_error).toBe(true); | |
| expect(resultMsg.permission_denials?.some(d => d.tool_name === 'write_file')).toBe(true); |
— deepseek-v4-pro via Qwen Code /review
| // At least one of file1 or file3 should be cascade-cancelled | ||
| // (the one that wasn't already executed when interrupt fired) | ||
| // Note: Depending on execution order, we may see 0, 1, or 2 cascade-cancelled tools | ||
| expect(cascadeCancelledDenials.length).toBeGreaterThanOrEqual(0); |
Collaborator
There was a problem hiding this comment.
[Critical] expect(cascadeCancelledDenials.length).toBeGreaterThanOrEqual(0) is a no-op — it's always true for any array. The test intends to verify that cascade-cancelled tools appear in permission_denials, but this assertion provides zero verification. Combined with the missing adapter.emitToolResult() call in the cascade loop, this test would silently pass even if cascade cancellation is completely broken.
Suggested change
| expect(cascadeCancelledDenials.length).toBeGreaterThanOrEqual(0); | |
| expect(cascadeCancelledDenials.length).toBeGreaterThan(0); |
— deepseek-v4-pro via Qwen Code /review
| // If we got messages, check for interrupt message | ||
| // The interrupt message may be sent before process exits | ||
| if (userMessages.length > 0) { | ||
| const interruptMessage = userMessages.find((msg) => |
Collaborator
There was a problem hiding this comment.
[Critical] The if (interruptMessage) guard makes the subsequent expect(interruptMessage).toBeDefined() tautological — it can never fail because reaching that line proves interruptMessage is truthy. If no interrupt message is emitted, the test silently passes (the if block is simply skipped).
Suggested change
| const interruptMessage = userMessages.find((msg) => | |
| const interruptMessage = userMessages.find((msg) => | |
| msg.includes('Request interrupted by user for tool use'), | |
| ); | |
| expect(interruptMessage).toBeDefined(); |
— deepseek-v4-pro via Qwen Code /review
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
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
2 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.
TLDR
Implements interrupt handling for SDK
canUseToolresponses. When the SDK denies a tool call withinterrupt: true, the request is terminated immediately and all pending tools are cascade-cancelled with anINTERRUPTEDerror type.Screenshots / Video Demo
N/A — no user-facing change. This is a backend feature for SDK integrations.
Dive Deeper
This PR adds support for the SDK
canUseToolinterrupt mechanism:New error type: Added
ToolErrorType.INTERRUPTEDto distinguish interrupted tool calls from regular permission denials (EXECUTION_DENIED).Core scheduler changes (
coreToolScheduler.ts):canUseToolwithcancelMessage, the status is now set to'error'withINTERRUPTEDorEXECUTION_DENIEDtype instead of'cancelled'.cancelPendingTools()method that cascade-cancels all pending tools (awaiting_approval or scheduled) when an interrupt is triggered. These tools are marked withINTERRUPTEDerror type.CLI changes:
permissionController.ts: Extractsinterruptflag from SDK response and passes it toonConfirm.BaseJsonOutputAdapter.ts: TreatsINTERRUPTEDerrors as permission denials for tracking purposes.nonInteractiveCli.ts: DetectsINTERRUPTEDresponses after tool execution and breaks the loop with an appropriate error message.Interface change: Added optional
interrupt?: booleanfield toToolConfirmationPayloadto support passing the interrupt flag from SDK responses.Reviewer Test Plan
canUseTooland returns{ cancelMessage: 'reason', interrupt: true }when denying a tool.interrupt: trueinterrupt: truestill work normally (tools can be re-allowed in subsequent turns).--output jsonand verifyINTERRUPTEDerrors are tracked as permission denials.Testing Matrix
Linked issues / bugs