Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
22 changes: 18 additions & 4 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 @@ -1985,6 +1986,19 @@ export class Session implements SessionContext {
autoModeAllowed = true;
break;
case 'blocked':
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.
Expand Down Expand Up @@ -2117,11 +2131,11 @@ 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)) {
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
27 changes: 21 additions & 6 deletions packages/core/src/core/coreToolScheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
firePostToolUseHook,
firePostToolUseFailureHook,
fireNotificationHook,
firePermissionDeniedHook,
firePermissionRequestHook,
appendAdditionalContext,
} from './toolHookTriggers.js';
Expand Down Expand Up @@ -1713,6 +1714,21 @@ export class CoreToolScheduler {
this.setStatusInternal(reqInfo.callId, 'scheduled');
continue;
case 'blocked':
if (!this.config.getDisableAllHooks()) {
const messageBus = this.config.getMessageBus() as
| MessageBus
| undefined;
if (messageBus) {
void firePermissionDeniedHook(

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 blocked path has no debug log, while the sibling fallback path (line ~1749) logs debugLogger.warn(...) with full counter state (consecutiveBlock, consecutiveUnavailable, totalBlock, totalUnavailable). The same asymmetry exists in Session.ts.

An operator debugging "AUTO mode is denying all tool calls" has zero server-side signal on the blocked path: no tool name, no reason (classifier_blocked vs classifier_unavailable), no counter state. Adding a symmetric debugLogger.warn here would close the observability gap.

Suggested change
void firePermissionDeniedHook(
case 'blocked':
debugLogger.warn(
`Auto mode blocked (${outcome.reason}): tool=${canonicalName}, ` +
`consecutiveBlock=${denialState.consecutiveBlock}, ` +
`consecutiveUnavailable=${denialState.consecutiveUnavailable}, ` +
`totalBlock=${denialState.totalBlock}, totalUnavailable=${denialState.totalUnavailable}`,
);

— qwen3.7-max via Qwen Code /review

messageBus,
canonicalName,
toolParams,
reqInfo.callId,
outcome.reason,
signal,

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] No test in coreToolScheduler.test.ts verifies that firePermissionDeniedHook is actually called when AUTO mode blocks a tool call via the scheduler's case 'blocked' branch. The hook function has unit tests in toolHookTriggers.test.ts (3 cases), and the applyAutoModeDecision blocked-branch mapping is tested in autoMode.test.ts, but the scheduler integration path — verifying that messageBus.request is called with eventName: 'PermissionDenied' and the correct tool_name, tool_input, tool_use_id, and reason — is untested.

A regression in the wiring (wrong argument order, missing reason, or getDisableAllHooks guard removed) would go undetected.

— qwen3.7-max via Qwen Code /review

);
}
}
this.setStatusInternal(
reqInfo.callId,
'error',
Expand All @@ -1731,7 +1747,7 @@ export class CoreToolScheduler {
// a pmForcedAsk fallback isn't an audit-worthy event).
if (fallback.fallback) {
debugLogger.warn(
`Auto mode fallback to manual approval (${fallback.reason}): consecutiveBlock=${denialState.consecutiveBlock}, consecutiveUnavailable=${denialState.consecutiveUnavailable}`,
`Auto mode fallback to manual approval (${fallback.reason}): consecutiveBlock=${denialState.consecutiveBlock}, consecutiveUnavailable=${denialState.consecutiveUnavailable}, totalBlock=${denialState.totalBlock}, totalUnavailable=${denialState.totalUnavailable}`,
);
}
break;
Expand Down Expand Up @@ -2188,11 +2204,10 @@ export class CoreToolScheduler {
this.setToolCallOutcome(callId, outcome);

// AUTO-mode denialTracking recovery: when the user manually approves a
// call that fell back from AUTO (either by streak threshold or by an
// explicit ask rule), reset the consecutiveBlock counter so subsequent
// calls return to classifier flow. Without this, a session that hit
// the denial threshold once would stay in fallback for the rest of
// the session even after the user explicitly approves the next call.
// call that fell back from AUTO (by denial cap or by an explicit ask
// rule), clear the armed counters so subsequent calls return to
// classifier flow. Without this, a session that hit a denial threshold
// once would stay in fallback even after explicit user approval.
// Cancel / abort do NOT reset — spec §9.1.4 treats rejection as a
// signal that the classifier was correct to block.
if (
Expand Down
62 changes: 62 additions & 0 deletions packages/core/src/core/toolHookTriggers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
fireNotificationHook,
appendAdditionalContext,
firePermissionRequestHook,
firePermissionDeniedHook,
} from './toolHookTriggers.js';
import type { MessageBus } from '../confirmation-bus/message-bus.js';
import { NotificationType } from '../hooks/types.js';
Expand Down Expand Up @@ -1071,4 +1072,65 @@ describe('toolHookTriggers', () => {
expect(result2.hasDecision).toBe(true);
});
});

describe('firePermissionDeniedHook', () => {
it('should return empty result when no messageBus is provided', async () => {
const result = await firePermissionDeniedHook(
undefined,
'run_shell_command',
{ command: 'rm -rf /' },
'toolu_123',
'classifier_blocked',
);

expect(result).toEqual({});
});

it('should send PermissionDenied payload to MessageBus', async () => {
const mockMessageBus = createMockMessageBus();
(mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
success: true,
output: {},
});

await firePermissionDeniedHook(
mockMessageBus,
'run_shell_command',
{ command: 'rm -rf /' },
'toolu_123',
'classifier_blocked',
);

expect(mockMessageBus.request).toHaveBeenCalledWith(
{
type: MessageBusType.HOOK_EXECUTION_REQUEST,
eventName: 'PermissionDenied',
input: {
tool_name: 'run_shell_command',
tool_input: { command: 'rm -rf /' },
tool_use_id: 'toolu_123',
reason: 'classifier_blocked',
},
},
MessageBusType.HOOK_EXECUTION_RESPONSE,
);
});

it('should swallow hook execution errors', async () => {
const mockMessageBus = createMockMessageBus();
(mockMessageBus.request as ReturnType<typeof vi.fn>).mockRejectedValue(
new Error('Network error'),
);

const result = await firePermissionDeniedHook(
mockMessageBus,
'test-tool',
{},
'toolu_123',
'classifier_unavailable',
);

expect(result).toEqual({});
});
});
});
51 changes: 51 additions & 0 deletions packages/core/src/core/toolHookTriggers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type {
HookExecutionRequest,
HookExecutionResponse,
} from '../confirmation-bus/types.js';
import type { PermissionDeniedReason } from '../permissions/autoMode.js';
import {
createHookOutput,
type PreToolUseHookOutput,
Expand Down Expand Up @@ -512,6 +513,56 @@ export async function firePermissionRequestHook(
}
}

/**
* Result of PermissionDenied hook execution
*/
export interface PermissionDeniedHookResult {
/** Reserved for future non-blocking hook diagnostics */
hookError?: string;
}

/**
* Fire PermissionDenied hook via MessageBus.
* Called after AUTO mode blocks a tool call without asking the user.
* This hook is observational only; it cannot approve or modify the call.
*/
export async function firePermissionDeniedHook(
messageBus: MessageBus | undefined,
toolName: string,
toolInput: Record<string, unknown>,
toolUseId: string,
reason: PermissionDeniedReason,
signal?: AbortSignal,
): Promise<PermissionDeniedHookResult> {
if (!messageBus) {
return {};
}

try {
await messageBus.request<HookExecutionRequest, HookExecutionResponse>(
{
type: MessageBusType.HOOK_EXECUTION_REQUEST,

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] Unlike every other fire*Hook function in this file (firePreToolUseHook, firePostToolUseHook, firePostToolUseFailureHook), this function:

  1. Does not check response.success from messageBus.request — a hook runner failure (success: false with an error message) is silently treated as success.
  2. Returns {} in the catch block instead of { hookError: message } — the PermissionDeniedHookResult interface declares hookError?: string but it is never populated (dead code).

Since callers use void (fire-and-forget), there is no immediate bug. But the inconsistency means the next person adding a hook will copy this pattern and miss the response check. Consider matching the established pattern:

const response = await messageBus.request<...>(...);
if (!response.success || !response.output) {
  const message = response.error?.message || 'hook runner returned no output';
  debugLogger.warn(`PermissionDenied hook response failure for ${toolName}: ${message}`);
  return { hookError: message };
}
return {};

And in the catch block: return { hookError: error instanceof Error ? error.message : String(error) };

— qwen3.7-max via Qwen Code /review

eventName: 'PermissionDenied',
input: {
tool_name: toolName,
tool_input: toolInput,
tool_use_id: toolUseId,
reason,
},
signal,
},
MessageBusType.HOOK_EXECUTION_RESPONSE,
);

return {};
} catch (error) {
debugLogger.warn(
`PermissionDenied hook error: ${error instanceof Error ? error.message : String(error)}`,
);
return {};
}
}

/**
* Append additional context to tool response content
*
Expand Down
Loading
Loading