diff --git a/packages/core/src/confirmation-bus/message-bus.ts b/packages/core/src/confirmation-bus/message-bus.ts index 11dab9ca23..722cb37344 100644 --- a/packages/core/src/confirmation-bus/message-bus.ts +++ b/packages/core/src/confirmation-bus/message-bus.ts @@ -7,12 +7,8 @@ import { randomUUID } from 'node:crypto'; import { EventEmitter } from 'node:events'; import type { PolicyEngine } from '../policy/policy-engine.js'; -import { PolicyDecision, getHookSource } from '../policy/types.js'; -import { - MessageBusType, - type Message, - type HookPolicyDecision, -} from './types.js'; +import { PolicyDecision } from '../policy/types.js'; +import { MessageBusType, type Message } from './types.js'; import { safeJsonStringify } from '../utils/safeJsonStringify.js'; import { debugLogger } from '../utils/debugLogger.js'; @@ -89,39 +85,6 @@ export class MessageBus extends EventEmitter { default: throw new Error(`Unknown policy decision: ${decision}`); } - } else if (message.type === MessageBusType.HOOK_EXECUTION_REQUEST) { - // Handle hook execution requests through policy evaluation - const hookRequest = message; - const decision = await this.policyEngine.checkHook(hookRequest); - - // Map decision to allow/deny for observability (ASK_USER treated as deny for hooks) - const effectiveDecision = - decision === PolicyDecision.ALLOW ? 'allow' : 'deny'; - - // Emit policy decision for observability - this.emitMessage({ - type: MessageBusType.HOOK_POLICY_DECISION, - eventName: hookRequest.eventName, - hookSource: getHookSource(hookRequest.input), - decision: effectiveDecision, - reason: - decision !== PolicyDecision.ALLOW - ? 'Hook execution denied by policy' - : undefined, - } as HookPolicyDecision); - - // If allowed, emit the request for hook system to handle - if (decision === PolicyDecision.ALLOW) { - this.emitMessage(message); - } else { - // If denied or ASK_USER, emit error response (hooks don't support interactive confirmation) - this.emitMessage({ - type: MessageBusType.HOOK_EXECUTION_RESPONSE, - correlationId: hookRequest.correlationId, - success: false, - error: new Error('Hook execution denied by policy'), - }); - } } else { // For all other message types, just emit them this.emitMessage(message); diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index 786894a972..aeecf73b3e 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -18,9 +18,6 @@ export enum MessageBusType { TOOL_EXECUTION_SUCCESS = 'tool-execution-success', TOOL_EXECUTION_FAILURE = 'tool-execution-failure', UPDATE_POLICY = 'update-policy', - HOOK_EXECUTION_REQUEST = 'hook-execution-request', - HOOK_EXECUTION_RESPONSE = 'hook-execution-response', - HOOK_POLICY_DECISION = 'hook-policy-decision', TOOL_CALLS_UPDATE = 'tool-calls-update', ASK_USER_REQUEST = 'ask-user-request', ASK_USER_RESPONSE = 'ask-user-response', @@ -120,29 +117,6 @@ export interface ToolExecutionFailure { error: E; } -export interface HookExecutionRequest { - type: MessageBusType.HOOK_EXECUTION_REQUEST; - eventName: string; - input: Record; - correlationId: string; -} - -export interface HookExecutionResponse { - type: MessageBusType.HOOK_EXECUTION_RESPONSE; - correlationId: string; - success: boolean; - output?: Record; - error?: Error; -} - -export interface HookPolicyDecision { - type: MessageBusType.HOOK_POLICY_DECISION; - eventName: string; - hookSource: 'project' | 'user' | 'system' | 'extension'; - decision: 'allow' | 'deny'; - reason?: string; -} - export interface QuestionOption { label: string; description: string; @@ -186,9 +160,6 @@ export type Message = | ToolExecutionSuccess | ToolExecutionFailure | UpdatePolicy - | HookExecutionRequest - | HookExecutionResponse - | HookPolicyDecision | AskUserRequest | AskUserResponse | ToolCallsUpdateMessage; diff --git a/packages/core/src/hooks/hookEventHandler.test.ts b/packages/core/src/hooks/hookEventHandler.test.ts index af7a6be37a..3dbc4ae881 100644 --- a/packages/core/src/hooks/hookEventHandler.test.ts +++ b/packages/core/src/hooks/hookEventHandler.test.ts @@ -8,7 +8,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HookEventHandler } from './hookEventHandler.js'; import type { Config } from '../config/config.js'; import type { HookConfig } from './types.js'; -import type { Logger } from '@opentelemetry/api-logs'; import type { HookPlanner } from './hookPlanner.js'; import type { HookRunner } from './hookRunner.js'; import type { HookAggregator } from './hookAggregator.js'; @@ -18,7 +17,6 @@ import { SessionStartSource, type HookExecutionResult, } from './types.js'; -import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; // Mock debugLogger const mockDebugLogger = vi.hoisted(() => ({ @@ -54,7 +52,6 @@ vi.mock('../telemetry/clearcut-logger/clearcut-logger.js', () => ({ describe('HookEventHandler', () => { let hookEventHandler: HookEventHandler; let mockConfig: Config; - let mockLogger: Logger; let mockHookPlanner: HookPlanner; let mockHookRunner: HookRunner; let mockHookAggregator: HookAggregator; @@ -74,8 +71,6 @@ describe('HookEventHandler', () => { }), } as unknown as Config; - mockLogger = {} as Logger; - mockHookPlanner = { createExecutionPlan: vi.fn(), } as unknown as HookPlanner; @@ -91,11 +86,9 @@ describe('HookEventHandler', () => { hookEventHandler = new HookEventHandler( mockConfig, - mockLogger, mockHookPlanner, mockHookRunner, mockHookAggregator, - createMockMessageBus(), ); }); diff --git a/packages/core/src/hooks/hookEventHandler.ts b/packages/core/src/hooks/hookEventHandler.ts index e208dd1ed4..cae3c61625 100644 --- a/packages/core/src/hooks/hookEventHandler.ts +++ b/packages/core/src/hooks/hookEventHandler.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { Logger } from '@opentelemetry/api-logs'; import type { Config } from '../config/config.js'; import type { HookPlanner, HookEventContext } from './hookPlanner.js'; import type { HookRunner } from './hookRunner.js'; @@ -38,265 +37,9 @@ import type { } from '@google/genai'; import { logHookCall } from '../telemetry/loggers.js'; import { HookCallEvent } from '../telemetry/types.js'; -import type { MessageBus } from '../confirmation-bus/message-bus.js'; -import { - MessageBusType, - type HookExecutionRequest, -} from '../confirmation-bus/types.js'; import { debugLogger } from '../utils/debugLogger.js'; import { coreEvents } from '../utils/events.js'; -/** - * Validates that a value is a non-null object - */ -function isObject(value: unknown): value is Record { - return typeof value === 'object' && value !== null; -} - -/** - * Validates BeforeTool input fields - */ -function validateBeforeToolInput(input: Record): { - toolName: string; - toolInput: Record; - mcpContext?: McpToolContext; -} { - const toolName = input['tool_name']; - const toolInput = input['tool_input']; - const mcpContext = input['mcp_context']; - if (typeof toolName !== 'string') { - throw new Error( - 'Invalid input for BeforeTool hook event: tool_name must be a string', - ); - } - if (!isObject(toolInput)) { - throw new Error( - 'Invalid input for BeforeTool hook event: tool_input must be an object', - ); - } - if (mcpContext !== undefined && !isObject(mcpContext)) { - throw new Error( - 'Invalid input for BeforeTool hook event: mcp_context must be an object', - ); - } - return { - toolName, - toolInput, - mcpContext: mcpContext as McpToolContext | undefined, - }; -} - -/** - * Validates AfterTool input fields - */ -function validateAfterToolInput(input: Record): { - toolName: string; - toolInput: Record; - toolResponse: Record; - mcpContext?: McpToolContext; -} { - const toolName = input['tool_name']; - const toolInput = input['tool_input']; - const toolResponse = input['tool_response']; - const mcpContext = input['mcp_context']; - if (typeof toolName !== 'string') { - throw new Error( - 'Invalid input for AfterTool hook event: tool_name must be a string', - ); - } - if (!isObject(toolInput)) { - throw new Error( - 'Invalid input for AfterTool hook event: tool_input must be an object', - ); - } - if (!isObject(toolResponse)) { - throw new Error( - 'Invalid input for AfterTool hook event: tool_response must be an object', - ); - } - if (mcpContext !== undefined && !isObject(mcpContext)) { - throw new Error( - 'Invalid input for AfterTool hook event: mcp_context must be an object', - ); - } - return { - toolName, - toolInput, - toolResponse, - mcpContext: mcpContext as McpToolContext | undefined, - }; -} - -/** - * Validates BeforeAgent input fields - */ -function validateBeforeAgentInput(input: Record): { - prompt: string; -} { - const prompt = input['prompt']; - if (typeof prompt !== 'string') { - throw new Error( - 'Invalid input for BeforeAgent hook event: prompt must be a string', - ); - } - return { prompt }; -} - -/** - * Validates AfterAgent input fields - */ -function validateAfterAgentInput(input: Record): { - prompt: string; - promptResponse: string; - stopHookActive: boolean; -} { - const prompt = input['prompt']; - const promptResponse = input['prompt_response']; - const stopHookActive = input['stop_hook_active']; - if (typeof prompt !== 'string') { - throw new Error( - 'Invalid input for AfterAgent hook event: prompt must be a string', - ); - } - if (typeof promptResponse !== 'string') { - throw new Error( - 'Invalid input for AfterAgent hook event: prompt_response must be a string', - ); - } - // stopHookActive defaults to false if not a boolean - return { - prompt, - promptResponse, - stopHookActive: - typeof stopHookActive === 'boolean' ? stopHookActive : false, - }; -} - -/** - * Validates model-related input fields (llm_request) - */ -function validateModelInput( - input: Record, - eventName: string, -): { llmRequest: GenerateContentParameters } { - const llmRequest = input['llm_request']; - if (!isObject(llmRequest)) { - throw new Error( - `Invalid input for ${eventName} hook event: llm_request must be an object`, - ); - } - return { llmRequest: llmRequest as unknown as GenerateContentParameters }; -} - -/** - * Validates AfterModel input fields - */ -function validateAfterModelInput(input: Record): { - llmRequest: GenerateContentParameters; - llmResponse: GenerateContentResponse; -} { - const llmRequest = input['llm_request']; - const llmResponse = input['llm_response']; - if (!isObject(llmRequest)) { - throw new Error( - 'Invalid input for AfterModel hook event: llm_request must be an object', - ); - } - if (!isObject(llmResponse)) { - throw new Error( - 'Invalid input for AfterModel hook event: llm_response must be an object', - ); - } - return { - llmRequest: llmRequest as unknown as GenerateContentParameters, - llmResponse: llmResponse as unknown as GenerateContentResponse, - }; -} - -/** - * Validates Notification input fields - */ -function validateNotificationInput(input: Record): { - notificationType: NotificationType; - message: string; - details: Record; -} { - const notificationType = input['notification_type']; - const message = input['message']; - const details = input['details']; - if (typeof notificationType !== 'string') { - throw new Error( - 'Invalid input for Notification hook event: notification_type must be a string', - ); - } - if (typeof message !== 'string') { - throw new Error( - 'Invalid input for Notification hook event: message must be a string', - ); - } - if (!isObject(details)) { - throw new Error( - 'Invalid input for Notification hook event: details must be an object', - ); - } - return { - notificationType: notificationType as NotificationType, - message, - details, - }; -} - -/** - * Validates SessionStart input fields - */ -function validateSessionStartInput(input: Record): { - source: SessionStartSource; -} { - const source = input['source']; - if (typeof source !== 'string') { - throw new Error( - 'Invalid input for SessionStart hook event: source must be a string', - ); - } - return { - source: source as SessionStartSource, - }; -} - -/** - * Validates SessionEnd input fields - */ -function validateSessionEndInput(input: Record): { - reason: SessionEndReason; -} { - const reason = input['reason']; - if (typeof reason !== 'string') { - throw new Error( - 'Invalid input for SessionEnd hook event: reason must be a string', - ); - } - return { - reason: reason as SessionEndReason, - }; -} - -/** - * Validates PreCompress input fields - */ -function validatePreCompressInput(input: Record): { - trigger: PreCompressTrigger; -} { - const trigger = input['trigger']; - if (typeof trigger !== 'string') { - throw new Error( - 'Invalid input for PreCompress hook event: trigger must be a string', - ); - } - return { - trigger: trigger as PreCompressTrigger, - }; -} - /** * Hook event bus that coordinates hook execution across the system */ @@ -305,29 +48,17 @@ export class HookEventHandler { private readonly hookPlanner: HookPlanner; private readonly hookRunner: HookRunner; private readonly hookAggregator: HookAggregator; - private readonly messageBus: MessageBus; constructor( config: Config, - logger: Logger, hookPlanner: HookPlanner, hookRunner: HookRunner, hookAggregator: HookAggregator, - messageBus: MessageBus, ) { this.config = config; this.hookPlanner = hookPlanner; this.hookRunner = hookRunner; this.hookAggregator = hookAggregator; - this.messageBus = messageBus; - - // Subscribe to hook execution requests from MessageBus - if (this.messageBus) { - this.messageBus.subscribe( - MessageBusType.HOOK_EXECUTION_REQUEST, - (request) => this.handleHookExecutionRequest(request), - ); - } } /** @@ -729,152 +460,4 @@ export class HookEventHandler { private getHookTypeFromResult(result: HookExecutionResult): 'command' { return result.hookConfig.type; } - - /** - * Handle hook execution requests from MessageBus - * This method routes the request to the appropriate fire*Event method - * and publishes the response back through MessageBus - * - * The request input only contains event-specific fields. This method adds - * the common base fields (session_id, cwd, etc.) before routing. - */ - private async handleHookExecutionRequest( - request: HookExecutionRequest, - ): Promise { - try { - // Add base fields to the input - const enrichedInput = { - ...this.createBaseInput(request.eventName as HookEventName), - ...request.input, - } as Record; - - let result: AggregatedHookResult; - - // Route to appropriate event handler based on eventName - switch (request.eventName) { - case HookEventName.BeforeTool: { - const { toolName, toolInput, mcpContext } = - validateBeforeToolInput(enrichedInput); - result = await this.fireBeforeToolEvent( - toolName, - toolInput, - mcpContext, - ); - break; - } - case HookEventName.AfterTool: { - const { toolName, toolInput, toolResponse, mcpContext } = - validateAfterToolInput(enrichedInput); - result = await this.fireAfterToolEvent( - toolName, - toolInput, - toolResponse, - mcpContext, - ); - break; - } - case HookEventName.BeforeAgent: { - const { prompt } = validateBeforeAgentInput(enrichedInput); - result = await this.fireBeforeAgentEvent(prompt); - break; - } - case HookEventName.AfterAgent: { - const { prompt, promptResponse, stopHookActive } = - validateAfterAgentInput(enrichedInput); - result = await this.fireAfterAgentEvent( - prompt, - promptResponse, - stopHookActive, - ); - break; - } - case HookEventName.BeforeModel: { - const { llmRequest } = validateModelInput( - enrichedInput, - 'BeforeModel', - ); - const translatedRequest = - defaultHookTranslator.toHookLLMRequest(llmRequest); - // Update the enrichedInput with translated request - enrichedInput['llm_request'] = translatedRequest; - result = await this.fireBeforeModelEvent(llmRequest); - break; - } - case HookEventName.AfterModel: { - const { llmRequest, llmResponse } = - validateAfterModelInput(enrichedInput); - const translatedRequest = - defaultHookTranslator.toHookLLMRequest(llmRequest); - const translatedResponse = - defaultHookTranslator.toHookLLMResponse(llmResponse); - // Update the enrichedInput with translated versions - enrichedInput['llm_request'] = translatedRequest; - enrichedInput['llm_response'] = translatedResponse; - result = await this.fireAfterModelEvent(llmRequest, llmResponse); - break; - } - case HookEventName.BeforeToolSelection: { - const { llmRequest } = validateModelInput( - enrichedInput, - 'BeforeToolSelection', - ); - const translatedRequest = - defaultHookTranslator.toHookLLMRequest(llmRequest); - // Update the enrichedInput with translated request - enrichedInput['llm_request'] = translatedRequest; - result = await this.fireBeforeToolSelectionEvent(llmRequest); - break; - } - case HookEventName.Notification: { - const { notificationType, message, details } = - validateNotificationInput(enrichedInput); - result = await this.fireNotificationEvent( - notificationType, - message, - details, - ); - break; - } - case HookEventName.SessionStart: { - const { source } = validateSessionStartInput(enrichedInput); - result = await this.fireSessionStartEvent(source); - break; - } - case HookEventName.SessionEnd: { - const { reason } = validateSessionEndInput(enrichedInput); - result = await this.fireSessionEndEvent(reason); - break; - } - case HookEventName.PreCompress: { - const { trigger } = validatePreCompressInput(enrichedInput); - result = await this.firePreCompressEvent(trigger); - break; - } - default: - throw new Error(`Unsupported hook event: ${request.eventName}`); - } - - // Publish response through MessageBus - if (this.messageBus) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.messageBus.publish({ - type: MessageBusType.HOOK_EXECUTION_RESPONSE, - correlationId: request.correlationId, - success: result.success, - output: result.finalOutput as unknown as Record, - }); - } - } catch (error) { - // Publish error response - if (this.messageBus) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.messageBus.publish({ - type: MessageBusType.HOOK_EXECUTION_RESPONSE, - correlationId: request.correlationId, - success: false, - error: error instanceof Error ? error : new Error(String(error)), - }); - } - } - } } diff --git a/packages/core/src/hooks/hookSystem.ts b/packages/core/src/hooks/hookSystem.ts index bfb855c5d5..e3d14b4a62 100644 --- a/packages/core/src/hooks/hookSystem.ts +++ b/packages/core/src/hooks/hookSystem.ts @@ -11,8 +11,6 @@ import { HookAggregator } from './hookAggregator.js'; import { HookPlanner } from './hookPlanner.js'; import { HookEventHandler } from './hookEventHandler.js'; import type { HookRegistryEntry } from './hookRegistry.js'; -import { logs, type Logger } from '@opentelemetry/api-logs'; -import { SERVICE_NAME } from '../telemetry/constants.js'; import { debugLogger } from '../utils/debugLogger.js'; import type { SessionStartSource, @@ -155,9 +153,6 @@ export class HookSystem { private readonly hookEventHandler: HookEventHandler; constructor(config: Config) { - const logger: Logger = logs.getLogger(SERVICE_NAME); - const messageBus = config.getMessageBus(); - // Initialize components this.hookRegistry = new HookRegistry(config); this.hookRunner = new HookRunner(config); @@ -165,11 +160,9 @@ export class HookSystem { this.hookPlanner = new HookPlanner(this.hookRegistry); this.hookEventHandler = new HookEventHandler( config, - logger, this.hookPlanner, this.hookRunner, this.hookAggregator, - messageBus, // Pass MessageBus to enable mediated hook execution ); } diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index a5df8e8167..782123cfb3 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -1821,291 +1821,4 @@ describe('PolicyEngine', () => { expect(result.decision).toBe(PolicyDecision.DENY); }); }); - - describe('checkHook', () => { - it('should allow hooks by default', async () => { - engine = new PolicyEngine({}, mockCheckerRunner); - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - expect(decision).toBe(PolicyDecision.ALLOW); - }); - - it('should deny all hooks when allowHooks is false', async () => { - engine = new PolicyEngine({ allowHooks: false }, mockCheckerRunner); - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - expect(decision).toBe(PolicyDecision.DENY); - }); - - it('should deny project hooks in untrusted folders', async () => { - engine = new PolicyEngine({}, mockCheckerRunner); - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'project', - trustedFolder: false, - }); - expect(decision).toBe(PolicyDecision.DENY); - }); - - it('should allow project hooks in trusted folders', async () => { - engine = new PolicyEngine({}, mockCheckerRunner); - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'project', - trustedFolder: true, - }); - expect(decision).toBe(PolicyDecision.ALLOW); - }); - - it('should allow user hooks in untrusted folders', async () => { - engine = new PolicyEngine({}, mockCheckerRunner); - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - trustedFolder: false, - }); - expect(decision).toBe(PolicyDecision.ALLOW); - }); - - it('should run hook checkers and deny on DENY decision', async () => { - const hookCheckers = [ - { - eventName: 'BeforeTool', - checker: { type: 'external' as const, name: 'test-hook-checker' }, - }, - ]; - engine = new PolicyEngine({ hookCheckers }, mockCheckerRunner); - - vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({ - decision: SafetyCheckDecision.DENY, - reason: 'Hook checker denied', - }); - - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - expect(decision).toBe(PolicyDecision.DENY); - expect(mockCheckerRunner.runChecker).toHaveBeenCalledWith( - expect.objectContaining({ name: 'hook:BeforeTool' }), - expect.objectContaining({ name: 'test-hook-checker' }), - ); - }); - - it('should run hook checkers and allow on ALLOW decision', async () => { - const hookCheckers = [ - { - eventName: 'BeforeTool', - checker: { type: 'external' as const, name: 'test-hook-checker' }, - }, - ]; - engine = new PolicyEngine({ hookCheckers }, mockCheckerRunner); - - vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({ - decision: SafetyCheckDecision.ALLOW, - }); - - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - expect(decision).toBe(PolicyDecision.ALLOW); - }); - - it('should return ASK_USER when checker requests it', async () => { - const hookCheckers = [ - { - checker: { type: 'external' as const, name: 'test-hook-checker' }, - }, - ]; - engine = new PolicyEngine({ hookCheckers }, mockCheckerRunner); - - vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({ - decision: SafetyCheckDecision.ASK_USER, - reason: 'Needs confirmation', - }); - - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - expect(decision).toBe(PolicyDecision.ASK_USER); - }); - - it('should return DENY for ASK_USER in non-interactive mode', async () => { - const hookCheckers = [ - { - checker: { type: 'external' as const, name: 'test-hook-checker' }, - }, - ]; - engine = new PolicyEngine( - { hookCheckers, nonInteractive: true }, - mockCheckerRunner, - ); - - vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({ - decision: SafetyCheckDecision.ASK_USER, - reason: 'Needs confirmation', - }); - - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - expect(decision).toBe(PolicyDecision.DENY); - }); - - it('should match hook checkers by eventName', async () => { - const hookCheckers = [ - { - eventName: 'AfterTool', - checker: { type: 'external' as const, name: 'after-tool-checker' }, - }, - { - eventName: 'BeforeTool', - checker: { type: 'external' as const, name: 'before-tool-checker' }, - }, - ]; - engine = new PolicyEngine({ hookCheckers }, mockCheckerRunner); - - vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({ - decision: SafetyCheckDecision.ALLOW, - }); - - await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - expect(mockCheckerRunner.runChecker).toHaveBeenCalledWith( - expect.anything(), - expect.objectContaining({ name: 'before-tool-checker' }), - ); - expect(mockCheckerRunner.runChecker).not.toHaveBeenCalledWith( - expect.anything(), - expect.objectContaining({ name: 'after-tool-checker' }), - ); - }); - - it('should match hook checkers by hookSource', async () => { - const hookCheckers = [ - { - hookSource: 'project' as const, - checker: { type: 'external' as const, name: 'project-checker' }, - }, - { - hookSource: 'user' as const, - checker: { type: 'external' as const, name: 'user-checker' }, - }, - ]; - engine = new PolicyEngine({ hookCheckers }, mockCheckerRunner); - - vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({ - decision: SafetyCheckDecision.ALLOW, - }); - - await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - expect(mockCheckerRunner.runChecker).toHaveBeenCalledWith( - expect.anything(), - expect.objectContaining({ name: 'user-checker' }), - ); - expect(mockCheckerRunner.runChecker).not.toHaveBeenCalledWith( - expect.anything(), - expect.objectContaining({ name: 'project-checker' }), - ); - }); - - it('should deny when hook checker throws an error', async () => { - const hookCheckers = [ - { - checker: { type: 'external' as const, name: 'failing-checker' }, - }, - ]; - engine = new PolicyEngine({ hookCheckers }, mockCheckerRunner); - - vi.mocked(mockCheckerRunner.runChecker).mockRejectedValue( - new Error('Checker failed'), - ); - - const decision = await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - expect(decision).toBe(PolicyDecision.DENY); - }); - - it('should run hook checkers in priority order', async () => { - const hookCheckers = [ - { - priority: 5, - checker: { type: 'external' as const, name: 'low-priority' }, - }, - { - priority: 20, - checker: { type: 'external' as const, name: 'high-priority' }, - }, - { - priority: 10, - checker: { type: 'external' as const, name: 'medium-priority' }, - }, - ]; - engine = new PolicyEngine({ hookCheckers }, mockCheckerRunner); - - vi.mocked(mockCheckerRunner.runChecker).mockImplementation( - async (_call, config) => { - if (config.name === 'high-priority') { - return { decision: SafetyCheckDecision.DENY, reason: 'denied' }; - } - return { decision: SafetyCheckDecision.ALLOW }; - }, - ); - - await engine.checkHook({ - eventName: 'BeforeTool', - hookSource: 'user', - }); - - // Should only call the high-priority checker (first in sorted order) - expect(mockCheckerRunner.runChecker).toHaveBeenCalledTimes(1); - expect(mockCheckerRunner.runChecker).toHaveBeenCalledWith( - expect.anything(), - expect.objectContaining({ name: 'high-priority' }), - ); - }); - }); - - describe('addHookChecker', () => { - it('should add a new hook checker and maintain priority order', () => { - engine = new PolicyEngine({}, mockCheckerRunner); - - engine.addHookChecker({ - priority: 5, - checker: { type: 'external', name: 'checker1' }, - }); - engine.addHookChecker({ - priority: 10, - checker: { type: 'external', name: 'checker2' }, - }); - - const checkers = engine.getHookCheckers(); - expect(checkers).toHaveLength(2); - expect(checkers[0].priority).toBe(10); - expect(checkers[0].checker.name).toBe('checker2'); - expect(checkers[1].priority).toBe(5); - expect(checkers[1].checker.name).toBe('checker1'); - }); - }); }); diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index 48feb537e6..be5536a9df 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -11,8 +11,6 @@ import { type PolicyRule, type SafetyCheckerRule, type HookCheckerRule, - type HookExecutionContext, - getHookSource, ApprovalMode, type CheckResult, } from './types.js'; @@ -20,7 +18,6 @@ import { stableStringify } from './stable-stringify.js'; import { debugLogger } from '../utils/debugLogger.js'; import type { CheckerRunner } from '../safety/checker-runner.js'; import { SafetyCheckDecision } from '../safety/protocol.js'; -import type { HookExecutionRequest } from '../confirmation-bus/types.js'; import { SHELL_TOOL_NAMES, initializeShellParsers, @@ -81,26 +78,6 @@ function ruleMatches( return true; } -/** - * Check if a hook checker rule matches a hook execution context. - */ -function hookCheckerMatches( - rule: HookCheckerRule, - context: HookExecutionContext, -): boolean { - // Check event name if specified - if (rule.eventName && rule.eventName !== context.eventName) { - return false; - } - - // Check hook source if specified - if (rule.hookSource && rule.hookSource !== context.hookSource) { - return false; - } - - return true; -} - export class PolicyEngine { private rules: PolicyRule[]; private checkers: SafetyCheckerRule[]; @@ -108,7 +85,6 @@ export class PolicyEngine { private readonly defaultDecision: PolicyDecision; private readonly nonInteractive: boolean; private readonly checkerRunner?: CheckerRunner; - private readonly allowHooks: boolean; private approvalMode: ApprovalMode; constructor(config: PolicyEngineConfig = {}, checkerRunner?: CheckerRunner) { @@ -124,7 +100,6 @@ export class PolicyEngine { this.defaultDecision = config.defaultDecision ?? PolicyDecision.ASK_USER; this.nonInteractive = config.nonInteractive ?? false; this.checkerRunner = checkerRunner; - this.allowHooks = config.allowHooks ?? true; this.approvalMode = config.approvalMode ?? ApprovalMode.DEFAULT; } @@ -495,84 +470,6 @@ export class PolicyEngine { return this.hookCheckers; } - /** - * Check if a hook execution is allowed based on the configured policies. - * Runs hook-specific safety checkers if configured. - */ - async checkHook( - request: HookExecutionRequest | HookExecutionContext, - ): Promise { - // If hooks are globally disabled, deny all hook executions - if (!this.allowHooks) { - return PolicyDecision.DENY; - } - - const context: HookExecutionContext = - 'input' in request - ? { - eventName: request.eventName, - hookSource: getHookSource(request.input), - trustedFolder: - typeof request.input['trusted_folder'] === 'boolean' - ? request.input['trusted_folder'] - : undefined, - } - : request; - - // In untrusted folders, deny project-level hooks - if (context.trustedFolder === false && context.hookSource === 'project') { - return PolicyDecision.DENY; - } - - // Run hook-specific safety checkers if configured - if (this.checkerRunner && this.hookCheckers.length > 0) { - for (const checkerRule of this.hookCheckers) { - if (hookCheckerMatches(checkerRule, context)) { - debugLogger.debug( - `[PolicyEngine.checkHook] Running hook checker: ${checkerRule.checker.name} for event: ${context.eventName}`, - ); - try { - // Create a synthetic function call for the checker runner - // This allows reusing the existing checker infrastructure - const syntheticCall = { - name: `hook:${context.eventName}`, - args: { - hookSource: context.hookSource, - trustedFolder: context.trustedFolder, - }, - }; - - const result = await this.checkerRunner.runChecker( - syntheticCall, - checkerRule.checker, - ); - - if (result.decision === SafetyCheckDecision.DENY) { - debugLogger.debug( - `[PolicyEngine.checkHook] Hook checker denied: ${result.reason}`, - ); - return PolicyDecision.DENY; - } else if (result.decision === SafetyCheckDecision.ASK_USER) { - debugLogger.debug( - `[PolicyEngine.checkHook] Hook checker requested ASK_USER: ${result.reason}`, - ); - // For hooks, ASK_USER is treated as DENY in non-interactive mode - return this.applyNonInteractiveMode(PolicyDecision.ASK_USER); - } - } catch (error) { - debugLogger.debug( - `[PolicyEngine.checkHook] Hook checker failed: ${error}`, - ); - return PolicyDecision.DENY; - } - } - } - } - - // Default: Allow hooks - return PolicyDecision.ALLOW; - } - private applyNonInteractiveMode(decision: PolicyDecision): PolicyDecision { // In non-interactive mode, ASK_USER becomes DENY if (this.nonInteractive && decision === PolicyDecision.ASK_USER) { diff --git a/packages/core/src/test-utils/mock-message-bus.ts b/packages/core/src/test-utils/mock-message-bus.ts index 1bd18c2f55..c28f077bf2 100644 --- a/packages/core/src/test-utils/mock-message-bus.ts +++ b/packages/core/src/test-utils/mock-message-bus.ts @@ -6,12 +6,7 @@ import { vi } from 'vitest'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; -import { - MessageBusType, - type Message, - type HookExecutionRequest, - type HookExecutionResponse, -} from '../confirmation-bus/types.js'; +import { MessageBusType, type Message } from '../confirmation-bus/types.js'; /** * Mock MessageBus for testing hook execution through MessageBus @@ -22,8 +17,6 @@ export class MockMessageBus { Set<(message: Message) => void> >(); publishedMessages: Message[] = []; - hookRequests: HookExecutionRequest[] = []; - hookResponses: HookExecutionResponse[] = []; defaultToolDecision: 'allow' | 'deny' | 'ask_user' = 'allow'; /** @@ -32,26 +25,6 @@ export class MockMessageBus { publish = vi.fn((message: Message) => { this.publishedMessages.push(message); - // Capture hook-specific messages - if (message.type === MessageBusType.HOOK_EXECUTION_REQUEST) { - this.hookRequests.push(message); - - // Auto-respond with success for testing - const response: HookExecutionResponse = { - type: MessageBusType.HOOK_EXECUTION_RESPONSE, - correlationId: message.correlationId, - success: true, - output: { - decision: 'allow', - reason: 'Mock hook execution successful', - }, - }; - this.hookResponses.push(response); - - // Emit response to subscribers - this.emit(MessageBusType.HOOK_EXECUTION_RESPONSE, response); - } - // Handle tool confirmation requests if (message.type === MessageBusType.TOOL_CONFIRMATION_REQUEST) { if (this.defaultToolDecision === 'allow') { @@ -115,78 +88,13 @@ export class MockMessageBus { } } - /** - * Manually trigger a hook response (for testing custom scenarios) - */ - triggerHookResponse( - correlationId: string, - success: boolean, - output?: Record, - error?: Error, - ) { - const response: HookExecutionResponse = { - type: MessageBusType.HOOK_EXECUTION_RESPONSE, - correlationId, - success, - output, - error, - }; - this.hookResponses.push(response); - this.emit(MessageBusType.HOOK_EXECUTION_RESPONSE, response); - } - - /** - * Get the last hook request published - */ - getLastHookRequest(): HookExecutionRequest | undefined { - return this.hookRequests[this.hookRequests.length - 1]; - } - - /** - * Get all hook requests for a specific event - */ - getHookRequestsForEvent(eventName: string): HookExecutionRequest[] { - return this.hookRequests.filter((req) => req.eventName === eventName); - } - /** * Clear all captured messages (for test isolation) */ clear() { this.publishedMessages = []; - this.hookRequests = []; - this.hookResponses = []; this.subscriptions.clear(); } - - /** - * Verify that a hook execution request was published - */ - expectHookRequest( - eventName: string, - input?: Partial>, - ) { - const request = this.hookRequests.find( - (req) => req.eventName === eventName, - ); - if (!request) { - throw new Error( - `Expected hook request for event "${eventName}" but none was found`, - ); - } - - if (input) { - Object.entries(input).forEach(([key, value]) => { - if (request.input[key] !== value) { - throw new Error( - `Expected hook input.${key} to be ${JSON.stringify(value)} but got ${JSON.stringify(request.input[key])}`, - ); - } - }); - } - - return request; - } } /**