Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
54 changes: 48 additions & 6 deletions packages/cli/src/acp-integration/session/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
Storage,
ToolNames,
fireNotificationHook,
firePermissionDeniedHook,
firePermissionRequestHook,
firePreToolUseHook,
firePostToolUseHook,
Expand Down Expand Up @@ -1943,6 +1944,7 @@ export class Session implements SessionContext {
recordAllow(this.config.getAutoModeDenialState()),
);
}
let wasAutoModeDenialFallback = false;

// ── L5: AUTO mode three-layer filter (duplicated from
// coreToolScheduler.ts; ACP routes through this Session path).
Expand All @@ -1951,6 +1953,7 @@ export class Session implements SessionContext {
// existing manual-approval flow below.
if (!autoModeAllowed && shouldRunAutoModeForCall(approvalMode, fc.name)) {
const denialState = this.config.getAutoModeDenialState();
const fallback = shouldFallback(denialState);
// `buildClassifierContents` retains only the most recent
// MAX_TRANSCRIPT_MESSAGES messages; ask the chat client for
// exactly that tail rather than triggering a `structuredClone`
Expand All @@ -1967,7 +1970,9 @@ export class Session implements SessionContext {
messages,
config: this.config,
signal: abortSignal,
skipClassifier: shouldFallback(denialState).fallback,
skipClassifierReason: fallback.fallback
? fallback.reason
: undefined,
});

// Apply decision via shared helper — eliminates ~40 lines of
Expand All @@ -1985,9 +1990,42 @@ export class Session implements SessionContext {
autoModeAllowed = true;
break;
case 'blocked':
debugLogger.warn(
`Auto mode blocked (${outcome.reason}): tool=${fc.name}, ` +
`consecutiveBlock=${denialState.consecutiveBlock}, ` +
`consecutiveUnavailable=${denialState.consecutiveUnavailable}, ` +
`totalBlock=${denialState.totalBlock}, ` +
`totalUnavailable=${denialState.totalUnavailable}`,
);
if (!this.config.getDisableAllHooks?.()) {
const messageBus = this.config.getMessageBus?.();
if (messageBus) {
void firePermissionDeniedHook(
messageBus,
fc.name,
toolParams,
toolUseId,
outcome.reason,
abortSignal,
);
}
}
return earlyErrorResponse(new Error(outcome.errorMessage), fc.name);
case 'fallback':
// Drop through to the manual-approval flow below.
wasAutoModeDenialFallback =
outcome.reason === 'consecutive_block' ||
outcome.reason === 'consecutive_unavailable' ||
outcome.reason === 'total_denial';
if (wasAutoModeDenialFallback) {
debugLogger.warn(
`Auto mode fallback to manual approval (${outcome.reason}): ` +
`consecutiveBlock=${denialState.consecutiveBlock}, ` +
`consecutiveUnavailable=${denialState.consecutiveUnavailable}, ` +
`totalBlock=${denialState.totalBlock}, ` +
`totalUnavailable=${denialState.totalUnavailable}`,
);
}
break;
default: {
const _exhaustive: never = outcome;
Expand Down Expand Up @@ -2117,14 +2155,18 @@ export class Session implements SessionContext {
.nativeEnum(ToolConfirmationOutcome)
.parse(output.outcome.optionId);

// Reset the AUTO-mode fallback streak when the user manually
// Reset AUTO-mode fallback counters when the user manually
// approves a prompt that was raised because denialTracking forced
// fallback. Without this, a single block-streak permanently
// downgrades the rest of the session to manual approval until the
// mode is toggled. Parallels coreToolScheduler.ts:1705-1717.
// fallback. Without this, one denial cap hit would keep the
// session in manual approval until the mode is toggled.
// Parallels coreToolScheduler.ts.
// Cancel / abort do NOT reset — treating rejection as a signal
// the classifier was right to block.
if (approvalMode === ApprovalMode.AUTO && isApproveOutcome(outcome)) {
if (
approvalMode === ApprovalMode.AUTO &&
wasAutoModeDenialFallback &&
isApproveOutcome(outcome)
) {
this.config.setAutoModeDenialState(
recordFallbackApprove(this.config.getAutoModeDenialState()),
);
Expand Down
33 changes: 31 additions & 2 deletions packages/cli/src/ui/components/hooks/constants.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ describe('hooks constants', () => {
expect(exitCodes).toHaveLength(2);
});

it('should return exit codes for PermissionDenied event', () => {
const exitCodes = getHookExitCodes(HookEventName.PermissionDenied);
expect(exitCodes).toHaveLength(2);
expect(exitCodes[0].code).toBe(0);
expect(exitCodes[1].code).toBe('Other');
});

it('should return exit codes for SessionStart event', () => {
const exitCodes = getHookExitCodes(HookEventName.SessionStart);
expect(exitCodes).toHaveLength(2);
Expand Down Expand Up @@ -110,6 +117,11 @@ describe('hooks constants', () => {
expect(desc).toBe('When a new session is started');
});

it('should return description for PermissionDenied', () => {
const desc = getHookShortDescription(HookEventName.PermissionDenied);
expect(desc).toBe('When auto mode denies a tool call');
});

it('should return empty string for unknown event', () => {
const desc = getHookShortDescription('unknown_event' as HookEventName);
expect(desc).toBe('');
Expand All @@ -128,6 +140,12 @@ describe('hooks constants', () => {
expect(desc).toContain('response');
});

it('should return description for PermissionDenied', () => {
const desc = getHookDescription(HookEventName.PermissionDenied);
expect(desc).toContain('tool_name');
expect(desc).toContain('reason');
});

it('should return empty string for Stop event', () => {
const desc = getHookDescription(HookEventName.Stop);
expect(desc).toBe('');
Expand Down Expand Up @@ -176,12 +194,13 @@ describe('hooks constants', () => {
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.PreCompact);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.PostCompact);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.PermissionRequest);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.PermissionDenied);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.TodoCreated);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.TodoCompleted);
});

it('should have 16 events', () => {
expect(DISPLAY_HOOK_EVENTS).toHaveLength(16);
it('should have 17 events', () => {
expect(DISPLAY_HOOK_EVENTS).toHaveLength(17);
});
});

Expand Down Expand Up @@ -220,6 +239,16 @@ describe('hooks constants', () => {
expect(info.configs).toEqual([]);
});

it('should create empty info for PermissionDenied', () => {
const info = createEmptyHookEventInfo(HookEventName.PermissionDenied);

expect(info.event).toBe(HookEventName.PermissionDenied);
expect(info.shortDescription).toBe('When auto mode denies a tool call');
expect(info.description).toContain('tool_use_id');
expect(info.exitCodes).toHaveLength(2);
expect(info.configs).toEqual([]);
});

it('should create empty info for TodoCreated', () => {
const info = createEmptyHookEventInfo(HookEventName.TodoCreated);

Expand Down
8 changes: 8 additions & 0 deletions packages/cli/src/ui/components/hooks/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ export function getHookExitCodes(eventName: string): HookExitCode[] {
{ code: 0, description: t('use hook decision if provided') },
{ code: 'Other', description: t('show stderr to user only') },
],
[HookEventName.PermissionDenied]: [
{ code: 0, description: t('observe permission denial') },
{ code: 'Other', description: t('show stderr to user only') },
],
[HookEventName.TodoCreated]: [
{ code: 0, description: t('allow todo creation') },
{
Expand Down Expand Up @@ -137,6 +141,7 @@ export function getHookShortDescription(eventName: string): string {
[HookEventName.PermissionRequest]: t(
'When a permission dialog is displayed',
),
[HookEventName.PermissionDenied]: t('When auto mode denies a tool call'),
[HookEventName.TodoCreated]: t('When a new todo item is created'),
[HookEventName.TodoCompleted]: t('When a todo item is marked as completed'),
};
Expand Down Expand Up @@ -182,6 +187,9 @@ export function getHookDescription(eventName: string): string {
[HookEventName.PermissionRequest]: t(
'Input to command is JSON with tool_name, tool_input, and tool_use_id. Output JSON with hookSpecificOutput containing decision to allow or deny.',
),
[HookEventName.PermissionDenied]: t(
'Input to command is JSON with tool_name, tool_input, tool_use_id, and reason.',
),
[HookEventName.TodoCreated]: t(
'Input to command is JSON with todo_id, todo_content, todo_status, all_todos, and phase. In validation, output JSON with decision (allow/block/deny) and reason. In postWrite, block/deny is ignored.',
),
Expand Down
11 changes: 11 additions & 0 deletions packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ import {
createDenialState,
resetDenialState,
} from '../permissions/denialTracking.js';
import type { PermissionDeniedReason } from '../permissions/autoMode.js';
import { SubagentManager } from '../subagents/subagent-manager.js';
import type { SubagentConfig } from '../subagents/types.js';
import { BackgroundTaskRegistry } from '../agents/background-tasks.js';
Expand Down Expand Up @@ -1306,6 +1307,16 @@ export class Config {
signal,
);
break;
case 'PermissionDenied':
result = await hookSystem.firePermissionDeniedEvent(
(input['tool_name'] as string) || '',
(input['tool_input'] as Record<string, unknown>) || {},
(input['tool_use_id'] as string) || '',
(input['reason'] as PermissionDeniedReason) ||
'classifier_blocked',
signal,
);
break;
case 'SubagentStart':
result = await hookSystem.fireSubagentStartEvent(
(input['agent_id'] as string) || '',
Expand Down
120 changes: 120 additions & 0 deletions packages/core/src/core/coreToolScheduler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,126 @@ async function waitForStatus(
}

describe('CoreToolScheduler', () => {
type SchedulerDenialTrackingInternals = {
toolCalls: ToolCall[];
autoModeFallbackCallIds: Set<string>;
_handleConfirmationResponseInner: (
callId: string,
toolCall: ToolCall,
originalOnConfirm: (
outcome: ToolConfirmationOutcome,
payload?: ToolConfirmationPayload,
) => Promise<void>,
outcome: ToolConfirmationOutcome,
signal: AbortSignal,
payload?: ToolConfirmationPayload,
) => Promise<void>;
};

function createSchedulerForDenialTrackingApprovalTest() {
const denialState = {
consecutiveBlock: 0,
consecutiveUnavailable: 0,
totalBlock: 20,
totalUnavailable: 0,
};
const setAutoModeDenialState = vi.fn();
const scheduler = new CoreToolScheduler({
config: {
getSessionId: () => 'test-session-id',
getApprovalMode: () => ApprovalMode.AUTO,
getAutoModeDenialState: () => denialState,
setAutoModeDenialState,
getToolRegistry: () =>
({
getTool: () => undefined,
}) as unknown as ToolRegistry,
getUsageStatisticsEnabled: () => false,
getDebugMode: () => false,
getChatRecordingService: () => undefined,
} as unknown as Config,
onAllToolCallsComplete: vi.fn(),
onToolCallsUpdate: vi.fn(),
getPreferredEditor: () => 'vscode',
onEditorClose: vi.fn(),
});
const confirmationDetails: ToolCallConfirmationDetails = {
type: 'exec',
title: 'Run command',
command: 'python',
rootCommand: 'python',
onConfirm: vi.fn().mockResolvedValue(undefined),
};
const toolCall = {
status: 'awaiting_approval',
request: {
callId: 'call-1',
name: ToolNames.SHELL,
args: {},
isClientInitiated: false,
prompt_id: 'prompt-1',
},
tool: {},
confirmationDetails,
} as unknown as ToolCall;
const internals =
scheduler as unknown as SchedulerDenialTrackingInternals;
internals.toolCalls = [toolCall];
return { internals, toolCall, setAutoModeDenialState };
}

it('does not reset total denial counters for unrelated AUTO approvals', async () => {
const { internals, toolCall, setAutoModeDenialState } =
createSchedulerForDenialTrackingApprovalTest();

await internals._handleConfirmationResponseInner(
'call-1',
toolCall,
vi.fn().mockResolvedValue(undefined),
ToolConfirmationOutcome.ProceedOnce,
new AbortController().signal,
);

expect(setAutoModeDenialState).not.toHaveBeenCalled();
});

it('resets denial counters after approving a denialTracking fallback prompt', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The two new tests cover (1) no fallback callId + approve → no reset, and (2) fallback callId + approve → reset. But there's no negative test for (3) fallback callId + Cancel → no reset. The production code correctly guards this with isApproveOutcome(outcome), but without a test, a regression that removes this guard would silently allow cancel on a fallback prompt to reset denial counters — weakening denial-tracking protection for the rest of the session.

it('does not reset denial counters when user cancels a denialTracking fallback prompt', async () => {
  const { internals, toolCall, setAutoModeDenialState } =
    createSchedulerForDenialTrackingApprovalTest();
  internals.autoModeFallbackCallIds.add('call-1');

  await internals._handleConfirmationResponseInner(
    'call-1',
    toolCall,
    vi.fn().mockResolvedValue(undefined),
    ToolConfirmationOutcome.Cancel,
    new AbortController().signal,
  );

  expect(setAutoModeDenialState).not.toHaveBeenCalled();
});

— qwen3.7-max via Qwen Code /review

const { internals, toolCall, setAutoModeDenialState } =
createSchedulerForDenialTrackingApprovalTest();
internals.autoModeFallbackCallIds.add('call-1');

await internals._handleConfirmationResponseInner(
'call-1',
toolCall,
vi.fn().mockResolvedValue(undefined),
ToolConfirmationOutcome.ProceedOnce,
new AbortController().signal,
);

expect(setAutoModeDenialState).toHaveBeenCalledWith({
consecutiveBlock: 0,
consecutiveUnavailable: 0,
totalBlock: 0,
totalUnavailable: 0,
});
});

it('does not reset denial counters after cancelling a denialTracking fallback prompt', async () => {
const { internals, toolCall, setAutoModeDenialState } =
createSchedulerForDenialTrackingApprovalTest();
internals.autoModeFallbackCallIds.add('call-1');

await internals._handleConfirmationResponseInner(
'call-1',
toolCall,
vi.fn().mockResolvedValue(undefined),
ToolConfirmationOutcome.Cancel,
new AbortController().signal,
);

expect(setAutoModeDenialState).not.toHaveBeenCalled();
});

function createSchedulerForLegacyToolTests(options: {
toolsByName: Map<string, MockTool>;
approvalMode?: ApprovalMode;
Expand Down
Loading
Loading