Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/core/src/scheduler/confirmation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ export interface ConfirmationResult {
export interface ResolutionResult {
outcome: ToolConfirmationOutcome;
lastDetails?: SerializableConfirmationDetails;
/**
* Final payload from the user's confirmation response. May carry feedback
* (when outcome is Cancel) or other outcome-specific data.
*/
payload?: ToolConfirmationPayload;
}

/**
Expand Down Expand Up @@ -125,6 +130,7 @@ export async function resolveConfirmation(
const callId = toolCall.request.callId;
let outcome = ToolConfirmationOutcome.ModifyWithEditor;
let lastDetails: SerializableConfirmationDetails | undefined;
let lastPayload: ToolConfirmationPayload | undefined;

// Loop exists to allow the user to modify the parameters and see the new
// diff.
Expand Down Expand Up @@ -173,6 +179,7 @@ export async function resolveConfirmation(
);
onWaitingForConfirmation?.(false);
outcome = response.outcome;
lastPayload = response.payload;

if ('onConfirm' in details && typeof details.onConfirm === 'function') {
await details.onConfirm(outcome, response.payload);
Expand All @@ -195,7 +202,7 @@ export async function resolveConfirmation(
}
}

return { outcome, lastDetails };
return { outcome, lastDetails, payload: lastPayload };
}

/**
Expand Down
45 changes: 45 additions & 0 deletions packages/core/src/scheduler/scheduler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -978,6 +978,51 @@ describe('Scheduler (Orchestrator)', () => {
);
});

it('should include feedback in cancel reason when payload carries feedback', async () => {
vi.mocked(checkPolicy).mockResolvedValue({
decision: PolicyDecision.ASK_USER,
rule: undefined,
});

const resolution = {
outcome: ToolConfirmationOutcome.Cancel,
lastDetails: undefined,
payload: { feedback: "don't overwrite, append instead" },
};
vi.mocked(resolveConfirmation).mockResolvedValue(resolution);

await scheduler.schedule(req1, signal);

expect(mockStateManager.updateStatus).toHaveBeenCalledWith(
'call-1',
CoreToolCallStatus.Cancelled,
"User denied execution. Feedback: don't overwrite, append instead",
);
expect(mockExecutor.execute).not.toHaveBeenCalled();
});

it('should fall back to plain reason when feedback is empty string', async () => {
vi.mocked(checkPolicy).mockResolvedValue({
decision: PolicyDecision.ASK_USER,
rule: undefined,
});

const resolution = {
outcome: ToolConfirmationOutcome.Cancel,
lastDetails: undefined,
payload: { feedback: '' },
};
vi.mocked(resolveConfirmation).mockResolvedValue(resolution);

await scheduler.schedule(req1, signal);

expect(mockStateManager.updateStatus).toHaveBeenCalledWith(
'call-1',
CoreToolCallStatus.Cancelled,
'User denied execution.',
);
});

it('should preserve confirmation details (e.g. diff) in cancelled state', async () => {
vi.mocked(checkPolicy).mockResolvedValue({
decision: PolicyDecision.ASK_USER,
Expand Down
19 changes: 14 additions & 5 deletions packages/core/src/scheduler/scheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,7 @@ export class Scheduler {
// User Confirmation Loop
let outcome = ToolConfirmationOutcome.ProceedOnce;
let lastDetails: SerializableConfirmationDetails | undefined;
let cancelFeedback: string | undefined;

if (decision === PolicyDecision.ASK_USER) {
const result = await resolveConfirmation(toolCall, signal, {
Expand All @@ -661,6 +662,15 @@ export class Scheduler {
});
outcome = result.outcome;
lastDetails = result.lastDetails;
if (
outcome === ToolConfirmationOutcome.Cancel &&
result.payload &&
'feedback' in result.payload &&
typeof result.payload.feedback === 'string' &&
result.payload.feedback.length > 0
) {
cancelFeedback = result.payload.feedback;
}
}

this.state.setOutcome(callId, outcome);
Expand All @@ -679,11 +689,10 @@ export class Scheduler {

// Handle cancellation (cascades to entire batch)
if (outcome === ToolConfirmationOutcome.Cancel) {
this.state.updateStatus(
callId,
CoreToolCallStatus.Cancelled,
'User denied execution.',
);
const reason = cancelFeedback
? `User denied execution. Feedback: ${cancelFeedback}`
: 'User denied execution.';
Comment on lines +692 to +694
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The cancelFeedback variable contains untrusted user input interpolated into the reason string sent to the LLM, posing a prompt injection risk. To mitigate this, avoid including user-provided input in content passed to the LLM (llmContent); use returnDisplay if the input is for display purposes. If the input must be passed to the LLM, ensure it is sanitized at the tool or subagent level (not in the executor) by trimming and removing newlines and context-breaking characters like ].

References
  1. To prevent prompt injection, avoid including user-provided input in content passed to the LLM (llmContent). If the input is needed for display purposes, use returnDisplay instead.
  2. Prompt injection sanitization should be handled at the tool or subagent level, not on a per-tool basis within the agent executor.
  3. Sanitize data from LLM-driven tools before injecting it into a system prompt to prevent prompt injection. At a minimum, remove newlines and context-breaking characters (e.g., ']').
  4. When validating string parameters from tools, trim the string first and then check for emptiness to prevent whitespace-only values from being accepted.

this.state.updateStatus(callId, CoreToolCallStatus.Cancelled, reason);
this.state.cancelAllQueued('User cancelled operation');
return; // Skip execution
}
Expand Down
13 changes: 12 additions & 1 deletion packages/core/src/tools/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1005,10 +1005,21 @@ export interface ToolExitPlanModeConfirmationPayload {
feedback?: string;
}

export interface ToolCancelWithFeedbackPayload {
/**
* User-authored reason for cancelling the tool call. When present on a
* response whose outcome is Cancel, the scheduler appends this string to
* the cancellation message delivered back to the model, so the model can
* see why the call was denied without waiting for the next user turn.
*/
feedback: string;
}

export type ToolConfirmationPayload =
| ToolEditConfirmationPayload
| ToolAskUserConfirmationPayload
| ToolExitPlanModeConfirmationPayload;
| ToolExitPlanModeConfirmationPayload
| ToolCancelWithFeedbackPayload;

export interface ToolSandboxExpansionConfirmationDetails {
type: 'sandbox_expansion';
Expand Down
Loading