mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
fix(core): report a cancelled shell as interrupted, not completed (#15836)
* fix(core): report a cancelled shell as interrupted, not completed The shell tool kills the process group on a user cancel but returns an ordinary result, so the scheduler's post-execute cancellation branch labelled the call "already completed" and told the model not to retry work that never finished. Let producers mark that outcome on the result; the shell tool sets it on a non-timeout, non-promote-refused abort. Closes #15833 * fix(core): keep a cancelled shell's partial output with the notice The scheduler replaced the whole model-visible payload with the cancellation notice, so the output a killed command had already produced disappeared even though the shell tool reports it. Append the producer's own interrupted text when it sets interruptedBeforeCompletion, and pin the marker's negative cases that the existing suites did not distinguish. * fix(core): flag returned cancellations as interrupted The monitor, create_sub_session, ask_user_question and workflow tools report a cancelled call by returning a result instead of throwing, so the scheduler's returned-means-finished assumption told the model the work had completed. They now set interruptedBeforeCompletion, which the scheduler already honours for the shell tool. * fix(core): mark only aborted unanswered questions as interrupted A plain decline is a completed decision. Setting the marker on it is inert today, but would report a finished interaction as "do not retry" if a future read site consumed it. * fix(cli): report an interrupted shell as interrupted in ACP sessions --------- Co-authored-by: 2SinceJuly <[email protected]> Co-authored-by: Shaojin Wen <[email protected]>
This commit is contained in:
15 files changed
+185
-9
No files matched your search
@@ -52778,6 +52778,34 @@ describe('Session', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('reports a returned interruption as interrupted and keeps its output', async () => {
|
||||
const abortController = new AbortController();
|
||||
mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(true);
|
||||
mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO);
|
||||
const execute = vi.fn().mockImplementation(async () => {
|
||||
abortController.abort();
|
||||
return {
|
||||
llmContent: 'Below is the output before it was cancelled.\npartial',
|
||||
returnDisplay: 'partial',
|
||||
interruptedBeforeCompletion: true,
|
||||
};
|
||||
});
|
||||
mockToolRegistry.getTool.mockReturnValue(
|
||||
mockAllowedTool('cancel_tool', execute),
|
||||
);
|
||||
|
||||
const result = await (
|
||||
session as unknown as ToolCallInternals
|
||||
).runToolCalls(abortController.signal, 'prompt-interrupted-cancel', [
|
||||
{ id: 'interrupted_cancel_call', name: 'cancel_tool', args: {} },
|
||||
]);
|
||||
|
||||
expect(result.parts[0].functionResponse?.response).toEqual({
|
||||
error:
|
||||
'User intentionally cancelled this tool call. Stop and await further instructions; do not retry or work around it.\n\nBelow is the output before it was cancelled.\npartial',
|
||||
});
|
||||
});
|
||||
|
||||
it('does not execute after cancellation settles during PreToolUse', async () => {
|
||||
const logToolCallSpy = vi
|
||||
.spyOn(core, 'logToolCall')
|
||||
|
||||
@@ -1043,6 +1043,10 @@ export const LOOP_DETECTED_TURN_ERROR_MESSAGE =
|
||||
const TOOL_EXECUTION_CANCELLED_MESSAGE = 'Tool execution was cancelled.';
|
||||
const TOOL_POST_EXECUTION_CANCELLED_MESSAGE =
|
||||
'The tool had already completed; its output was discarded.';
|
||||
// Mirrors the scheduler's wording for a producer that reports interruption by
|
||||
// returning (the shell tool on user cancel) instead of throwing.
|
||||
const TOOL_CANCELLED_BEFORE_COMPLETION_MESSAGE =
|
||||
'User intentionally cancelled this tool call. Stop and await further instructions; do not retry or work around it.';
|
||||
const USER_PROMPT_BLOCKED_TOOL_RESULT =
|
||||
'A UserPromptSubmit hook blocked this tool result from reaching the model. Do not rerun the tool to get it.';
|
||||
|
||||
@@ -19647,16 +19651,29 @@ export class Session implements SessionContext {
|
||||
}
|
||||
}
|
||||
|
||||
const interruptedBeforeCompletion =
|
||||
toolResult.interruptedBeforeCompletion === true;
|
||||
const cancelReason = interruptedBeforeCompletion
|
||||
? TOOL_CANCELLED_BEFORE_COMPLETION_MESSAGE
|
||||
: TOOL_POST_EXECUTION_CANCELLED_MESSAGE;
|
||||
if (
|
||||
activeToolAbortSignal.aborted &&
|
||||
!(isExecutionTimeout && parentAbortedAtExecutionSettle)
|
||||
) {
|
||||
status = 'cancelled';
|
||||
// Keep the producer's own partial output as the detail, the way
|
||||
// the scheduler does, instead of discarding it with the notice.
|
||||
const cancelDetail =
|
||||
interruptedBeforeCompletion &&
|
||||
typeof toolResult.llmContent === 'string' &&
|
||||
toolResult.llmContent.trim()
|
||||
? `\n\n${toolResult.llmContent}`
|
||||
: '';
|
||||
responseParts = convertToFunctionErrorResponse(
|
||||
modelFacingToolName,
|
||||
callId,
|
||||
TOOL_POST_EXECUTION_CANCELLED_MESSAGE,
|
||||
TOOL_POST_EXECUTION_CANCELLED_MESSAGE,
|
||||
`${cancelReason}${cancelDetail}`,
|
||||
cancelReason,
|
||||
);
|
||||
}
|
||||
terminalStatus = status;
|
||||
@@ -19666,7 +19683,7 @@ export class Session implements SessionContext {
|
||||
status === 'error' && toolResult.error
|
||||
? new Error(toolResult.error.message)
|
||||
: status === 'cancelled'
|
||||
? new Error(TOOL_POST_EXECUTION_CANCELLED_MESSAGE)
|
||||
? new Error(cancelReason)
|
||||
: undefined;
|
||||
if (isTrustedTodoWriteTool && status === 'cancelled') {
|
||||
this.todoStopGuard.suspend();
|
||||
|
||||
@@ -11225,6 +11225,17 @@ describe('CoreToolScheduler telemetry spans', () => {
|
||||
'The tool had already completed',
|
||||
'User intentionally cancelled this tool call. Stop',
|
||||
],
|
||||
[
|
||||
'tells the model a returned interruption result never completed',
|
||||
{
|
||||
llmContent:
|
||||
'Command was cancelled by user before it could complete. There was no output before it was cancelled.',
|
||||
returnDisplay: 'Command cancelled by user.',
|
||||
interruptedBeforeCompletion: true,
|
||||
},
|
||||
'User intentionally cancelled this tool call.',
|
||||
'had already completed',
|
||||
],
|
||||
])('%s', async (_title, outcome, notice, wrongNotice) => {
|
||||
const { call } = await runAbortedDuringExecution({}, outcome);
|
||||
|
||||
@@ -11234,6 +11245,31 @@ describe('CoreToolScheduler telemetry spans', () => {
|
||||
expect(responseText).not.toContain(wrongNotice);
|
||||
});
|
||||
|
||||
// The notice says the call did not finish; the producer's own text says how
|
||||
// far it got. Replacing the whole payload with the notice loses the partial
|
||||
// output the model needs to decide what remains.
|
||||
it('keeps the partial output a cancelled producer reported', async () => {
|
||||
const { call } = await runAbortedDuringExecution(
|
||||
{},
|
||||
{
|
||||
llmContent:
|
||||
'Command was cancelled by user before it could complete. Below is the output before it was cancelled:\nstart',
|
||||
returnDisplay: 'Command cancelled by user.',
|
||||
interruptedBeforeCompletion: true,
|
||||
},
|
||||
);
|
||||
|
||||
const completedCall = expectSettled(call, 'cancelled', 'cancelled');
|
||||
const responseText = JSON.stringify(completedCall.response.responseParts);
|
||||
expect(responseText).toContain(
|
||||
'User intentionally cancelled this tool call.',
|
||||
);
|
||||
expect(responseText).toContain(
|
||||
'Below is the output before it was cancelled',
|
||||
);
|
||||
expect(responseText).toContain('start');
|
||||
});
|
||||
|
||||
// A post-execution cancellation drops the model-visible output, but the
|
||||
// references to files the tool already spilled to disk must survive —
|
||||
// otherwise nothing points at them and they are orphaned (#8180 review).
|
||||
|
||||
@@ -1275,13 +1275,19 @@ const createCancelledResponse = (
|
||||
// model-visible output must not orphan files already persisted to disk.
|
||||
persistedOutputFiles?: string[],
|
||||
visionBridgeNotice?: string,
|
||||
// A producer that reported its own interrupted work by returning (the shell
|
||||
// tool on user cancel) already described what ran before the kill. Keep that
|
||||
// text: the cancellation notice says the call never finished, the detail says
|
||||
// how far it got.
|
||||
detail?: string,
|
||||
): CoreToolCallResponseInfo => {
|
||||
const cancellationPrefix =
|
||||
getModelFacingToolName(request) === ToolNames.TOOL_CALL &&
|
||||
executionStatus === 'not_started'
|
||||
? DEFERRED_TOOL_CALL_CANCELLATION_PREFIX
|
||||
: '';
|
||||
const errorMessage = `${cancellationPrefix}[Operation Cancelled] Reason: ${reason}`;
|
||||
const errorDetail = detail?.trim() ? `\n\n${detail}` : '';
|
||||
const errorMessage = `${cancellationPrefix}[Operation Cancelled] Reason: ${reason}${errorDetail}`;
|
||||
return {
|
||||
callId: request.callId,
|
||||
responseParts: [
|
||||
@@ -6597,8 +6603,12 @@ export class CoreToolScheduler {
|
||||
}
|
||||
if (aborted) {
|
||||
// PostToolUseFailure Hook
|
||||
// `execute()` returned a result here, so the tool's work did finish.
|
||||
let cancelMessage = TOOL_CANCELLED_AFTER_COMPLETION_MESSAGE;
|
||||
// `execute()` returned a result, but that alone does not mean the work
|
||||
// finished: producers that signal interruption by returning (the shell
|
||||
// tool on user cancel) set `interruptedBeforeCompletion`.
|
||||
let cancelMessage = toolResult.interruptedBeforeCompletion
|
||||
? TOOL_CANCELLED_BEFORE_COMPLETION_MESSAGE
|
||||
: TOOL_CANCELLED_AFTER_COMPLETION_MESSAGE;
|
||||
let failureHookArtifacts: ToolArtifact[] | undefined;
|
||||
if (hooksEnabled && messageBus) {
|
||||
const failureHookResult = await this.withHookSpan(
|
||||
@@ -6642,6 +6652,11 @@ export class CoreToolScheduler {
|
||||
executionStatus,
|
||||
failureHookArtifacts,
|
||||
toolResult.persistedOutputFiles,
|
||||
undefined,
|
||||
typeof toolResult.llmContent === 'string' &&
|
||||
toolResult.interruptedBeforeCompletion
|
||||
? toolResult.llmContent
|
||||
: undefined,
|
||||
);
|
||||
observeProducerOutput(cancelledResponse, true);
|
||||
this.setStatusInternal(callId, 'cancelled', cancelledResponse);
|
||||
|
||||
@@ -59,9 +59,9 @@ describe('AskUserQuestionTool', () => {
|
||||
questions: Question[],
|
||||
outcome: ToolConfirmationOutcome,
|
||||
answers?: Record<string, string>,
|
||||
signal: AbortSignal = new AbortController().signal,
|
||||
) => {
|
||||
const invocation = tool.build({ questions });
|
||||
const signal = new AbortController().signal;
|
||||
const confirmation = await invocation.getConfirmationDetails(signal);
|
||||
await confirmation.onConfirm(outcome, answers && { answers });
|
||||
return invocation.execute(signal);
|
||||
@@ -302,6 +302,22 @@ describe('AskUserQuestionTool', () => {
|
||||
ToolConfirmationOutcome.Cancel,
|
||||
);
|
||||
expect(result.llmContent).toContain('declined to answer');
|
||||
// A decline is a completed decision, not interrupted work.
|
||||
expect(result.interruptedBeforeCompletion).toBeUndefined();
|
||||
});
|
||||
|
||||
it('marks the unanswered questions as interrupted when the turn is aborted', async () => {
|
||||
const controller = new AbortController();
|
||||
controller.abort();
|
||||
|
||||
const result = await answerAndRun(
|
||||
[testQuestion({ multiSelect: false })],
|
||||
ToolConfirmationOutcome.Cancel,
|
||||
undefined,
|
||||
controller.signal,
|
||||
);
|
||||
expect(result.llmContent).toContain('declined to answer');
|
||||
expect(result.interruptedBeforeCompletion).toBe(true);
|
||||
});
|
||||
|
||||
it('should return formatted answers when user provides them', async () => {
|
||||
|
||||
@@ -250,7 +250,7 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation<
|
||||
return details;
|
||||
}
|
||||
|
||||
async execute(_signal: AbortSignal): Promise<ToolResult> {
|
||||
async execute(signal: AbortSignal): Promise<ToolResult> {
|
||||
try {
|
||||
// In non-interactive mode without ACP support, we cannot collect user input
|
||||
if (!this.canCollectAnswers()) {
|
||||
@@ -267,6 +267,9 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation<
|
||||
return {
|
||||
llmContent: cancellationMessage,
|
||||
returnDisplay: cancellationMessage,
|
||||
// Only a cancelled turn leaves the questions unanswered; a plain
|
||||
// decline is a completed decision, not interrupted work.
|
||||
...(signal.aborted ? { interruptedBeforeCompletion: true } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -186,6 +186,8 @@ describe('CreateSubSessionTool', () => {
|
||||
|
||||
expect(res.returnDisplay).toBe('Cancelled');
|
||||
expect(res.llmContent).toMatch(/cancelled/i);
|
||||
// Returned, not thrown: the scheduler must be told the call did not finish.
|
||||
expect(res.interruptedBeforeCompletion).toBe(true);
|
||||
// The sub-session is NOT cancelled — it has no abort seam — so the tool
|
||||
// must say so rather than implying the work was undone.
|
||||
expect(res.llmContent).toMatch(/runs independently|not cancelled/i);
|
||||
|
||||
@@ -167,7 +167,13 @@ class CreateSubSessionInvocation extends BaseToolInvocation<
|
||||
'been created; it runs independently and is not cancelled.'
|
||||
: 'create_sub_session was cancelled before it started. No ' +
|
||||
'sub-session was created.';
|
||||
return { llmContent: message, returnDisplay: 'Cancelled' };
|
||||
return {
|
||||
llmContent: message,
|
||||
returnDisplay: 'Cancelled',
|
||||
// The turn was cancelled mid-flight, so the call never reported a
|
||||
// spawn result; the scheduler must not call it completed.
|
||||
interruptedBeforeCompletion: true,
|
||||
};
|
||||
}
|
||||
|
||||
// Embed a clickable session link in the display output so the web shell
|
||||
|
||||
@@ -869,6 +869,9 @@ describe('MonitorTool', () => {
|
||||
expect(result.llmContent).toContain(
|
||||
'Monitor was cancelled before it could start.',
|
||||
);
|
||||
// The tool returns on abort rather than throwing, so the scheduler needs
|
||||
// this to report an interruption instead of a completed call.
|
||||
expect(result.interruptedBeforeCompletion).toBe(true);
|
||||
expect(result.error).toBeUndefined();
|
||||
});
|
||||
|
||||
|
||||
@@ -327,6 +327,9 @@ class MonitorToolInvocation extends BaseToolInvocation<
|
||||
return {
|
||||
llmContent: 'Monitor was cancelled before it could start.',
|
||||
returnDisplay: 'Monitor cancelled.',
|
||||
// Returned rather than thrown, so the scheduler needs this to avoid
|
||||
// reporting a monitor that never spawned as already completed.
|
||||
interruptedBeforeCompletion: true,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -2195,6 +2195,33 @@ describe('ShellTool', () => {
|
||||
expectSpawned('npm test');
|
||||
});
|
||||
|
||||
it('marks a user-cancelled command as interrupted before completion', async () => {
|
||||
const result = await runFg('npm test', {
|
||||
output: 'start',
|
||||
exitCode: null,
|
||||
signal: 15,
|
||||
aborted: true,
|
||||
});
|
||||
|
||||
// The tool returns (rather than throws), so the scheduler needs this
|
||||
// signal to avoid reporting a killed command as already completed.
|
||||
expect(result.interruptedBeforeCompletion).toBe(true);
|
||||
expect(String(result.llmContent)).toContain(
|
||||
'Command was cancelled by user before it could complete.',
|
||||
);
|
||||
// What already ran is part of the interrupted account, not a detail the
|
||||
// cancellation may drop.
|
||||
expect(String(result.llmContent)).toContain(
|
||||
'Below is the output before it was cancelled:\nstart',
|
||||
);
|
||||
});
|
||||
|
||||
it('does not mark a completed command as interrupted', async () => {
|
||||
const result = await runFg('npm test', { output: 'done', exitCode: 0 });
|
||||
|
||||
expect(result.interruptedBeforeCompletion).toBeUndefined();
|
||||
});
|
||||
|
||||
it('preserves shell wrapper environment and flags during foreground execution', async () => {
|
||||
const command = `FOO=bar bash -e -c 'echo "$FOO"; false; echo bad'`;
|
||||
await runFg(command);
|
||||
@@ -4477,6 +4504,8 @@ describe('ShellTool', () => {
|
||||
['timed out'],
|
||||
);
|
||||
expect(result.returnDisplay).toMatchObject({ outcome: 'completed' });
|
||||
// Neither a timeout nor a refused promote is a returned interruption.
|
||||
expect(result.interruptedBeforeCompletion).toBeUndefined();
|
||||
});
|
||||
|
||||
it('rethrows + kills child when registry.register throws — no orphan zombie', async () => {
|
||||
@@ -5369,6 +5398,8 @@ describe('ShellTool', () => {
|
||||
message: TIMED_OUT,
|
||||
type: ToolErrorType.EXECUTION_TIMEOUT,
|
||||
});
|
||||
// A timeout is reported as a tool error, never as a returned interruption.
|
||||
expect(result.interruptedBeforeCompletion).toBeUndefined();
|
||||
});
|
||||
|
||||
it('returns a structured timeout when the command produced no output', async () => {
|
||||
|
||||
@@ -3459,6 +3459,11 @@ export class ShellToolInvocation extends BaseToolInvocation<
|
||||
},
|
||||
...(persistedOutputFiles !== undefined ? { persistedOutputFiles } : {}),
|
||||
...(outputBudgetApplied ? { outputBudgetApplied } : {}),
|
||||
// A user cancel kills the process group but returns a normal result, so
|
||||
// tell the scheduler its work did not finish.
|
||||
...(result.aborted && !wasTimeout && !wasPromoteRefused
|
||||
? { interruptedBeforeCompletion: true }
|
||||
: {}),
|
||||
...executionError,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -629,6 +629,13 @@ export interface ToolResult {
|
||||
*/
|
||||
outputBudgetApplied?: boolean;
|
||||
|
||||
/**
|
||||
* Set by a producer that knows its own work was cut short but reports that
|
||||
* outcome as an ordinary result instead of throwing. The scheduler uses it to
|
||||
* tell the model the call was interrupted rather than completed.
|
||||
*/
|
||||
interruptedBeforeCompletion?: boolean;
|
||||
|
||||
/**
|
||||
* Markdown string for user display.
|
||||
* This provides a user-friendly summary or visualization of the result.
|
||||
|
||||
@@ -71,6 +71,7 @@ const live = () => new AbortController().signal;
|
||||
const CANCELLED_BEFORE_START = {
|
||||
llmContent: 'Workflow was cancelled before it could start.',
|
||||
returnDisplay: 'Workflow cancelled.',
|
||||
interruptedBeforeCompletion: true,
|
||||
};
|
||||
const runIdOf = (scriptPath: string) =>
|
||||
scriptPath.match(/(wf_[0-9a-f]+)\.js$/)![1];
|
||||
|
||||
@@ -967,6 +967,9 @@ function startCancelledResult(): WorkflowToolResult {
|
||||
return {
|
||||
llmContent: 'Workflow was cancelled before it could start.',
|
||||
returnDisplay: 'Workflow cancelled.',
|
||||
// Same returned-interruption shape as the monitor: nothing ran, so the
|
||||
// scheduler must not tell the model the call completed.
|
||||
interruptedBeforeCompletion: true,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user