Skip to content

fix(core): align session hook matcher targets #4354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pomelo-nwu merged 3 commits into from
May 21, 2026
+393 −67
Merged

fix(core): align session hook matcher targets #4354

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e895947
fix(core): align session hook matcher targets
qqqys May 20, 2026
339bcaf
fix(core): share hook matcher target mapping
qqqys May 20, 2026
bf809d4
fix(core): satisfy hook matcher exhaustiveness lint
qqqys May 20, 2026
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
233 changes: 233 additions & 0 deletions packages/core/src/hooks/hookEventHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import type {
HookAggregator,
AggregatedHookResult,
SessionHooksManager,
SessionHookEntry,
} from './index.js';
import type { HookConfig, HookOutput, PermissionSuggestion } from './types.js';
import type { HookExecutionResult } from './types.js';
Expand Down Expand Up @@ -108,6 +109,21 @@ describe('HookEventHandler', () => {
finalOutput,
});

const createSessionHookEntry = (
eventName: HookEventName,
matcher: string,
command: string = 'echo session-hook',
): SessionHookEntry => ({
hookId: `session-${eventName}-${matcher}`,
eventName,
matcher,
config: {
type: HookType.Command,
command,
source: HooksConfigSource.Session,
},
});

describe('fireUserPromptSubmitEvent', () => {
it('should execute hooks for UserPromptSubmit event', async () => {
const mockPlan = createMockExecutionPlan([]);
Expand Down Expand Up @@ -427,6 +443,223 @@ describe('HookEventHandler', () => {
});
});

describe('session hook matcher targets', () => {

@wenshao wenshao May 20, 2026

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 new tests cover 4 event types: SessionStart (trigger branch), StopFailure (error branch), Notification (notificationType branch), and UserPromptSubmit (no-matcher path). Two branches of getSessionHookMatcherTarget remain untested:

  1. toolName branch — PreToolUse, PostToolUse, PostToolUseFailure, PermissionRequest. This is the most commonly fired path (every tool call). A regression here would silently break session hook matching for all tool invocations.

  2. agentType branch — SubagentStart, SubagentStop. Uses a distinct context field (agentType) not covered by any other test.

Consider adding one test per untested branch:

it('matches PreToolUse session hooks against the tool name', async () => {
  const sessionHook = createSessionHookEntry(HookEventName.PreToolUse, 'shell');
  // ... setup mocks ...
  await hookEventHandler.firePreToolUseEvent('toolu_123', 'shell', { command: 'ls' });
  expect(mockSessionHooksManager.getMatchingHooks).toHaveBeenCalledWith(
    'test-session-id', HookEventName.PreToolUse, 'shell',
  );
});

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

it('matches SessionStart session hooks against the session source', async () => {
const sessionHook = createSessionHookEntry(
HookEventName.SessionStart,
SessionStartSource.Resume,
);

vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue(null);
vi.mocked(mockSessionHooksManager.getMatchingHooks).mockReturnValue([
sessionHook,
]);
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
createMockAggregatedResult(true),
);

await hookEventHandler.fireSessionStartEvent(
SessionStartSource.Resume,
'test-model',
);

expect(mockSessionHooksManager.getMatchingHooks).toHaveBeenCalledWith(
'test-session-id',
HookEventName.SessionStart,
SessionStartSource.Resume,
);
expect(mockHookRunner.executeHooksParallel).toHaveBeenCalledWith(
[sessionHook.config],
HookEventName.SessionStart,
expect.objectContaining({ source: SessionStartSource.Resume }),
expect.any(Function),
expect.any(Function),
undefined,
expect.any(Object),
);
});

it('matches PreToolUse session hooks against the tool name', async () => {
const sessionHook = createSessionHookEntry(
HookEventName.PreToolUse,
'shell',
);

vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue(null);
vi.mocked(mockSessionHooksManager.getMatchingHooks).mockReturnValue([
sessionHook,
]);
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
createMockAggregatedResult(true),
);

await hookEventHandler.firePreToolUseEvent(
'shell',
{ command: 'ls' },
'toolu_123',
PermissionMode.Default,
);

expect(mockSessionHooksManager.getMatchingHooks).toHaveBeenCalledWith(
'test-session-id',
HookEventName.PreToolUse,
'shell',
);
expect(mockHookRunner.executeHooksParallel).toHaveBeenCalledWith(
[sessionHook.config],
HookEventName.PreToolUse,
expect.objectContaining({ tool_name: 'shell' }),
expect.any(Function),
expect.any(Function),
undefined,
expect.any(Object),
);
});

it('matches SubagentStart session hooks against the agent type', async () => {
const sessionHook = createSessionHookEntry(
HookEventName.SubagentStart,
AgentType.Explorer,
);

vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue(null);
vi.mocked(mockSessionHooksManager.getMatchingHooks).mockReturnValue([
sessionHook,
]);
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
createMockAggregatedResult(true),
);

await hookEventHandler.fireSubagentStartEvent(
'agent_123',
AgentType.Explorer,
PermissionMode.Default,
);

expect(mockSessionHooksManager.getMatchingHooks).toHaveBeenCalledWith(
'test-session-id',
HookEventName.SubagentStart,
AgentType.Explorer,
);
expect(mockHookRunner.executeHooksParallel).toHaveBeenCalledWith(
[sessionHook.config],
HookEventName.SubagentStart,
expect.objectContaining({ agent_type: AgentType.Explorer }),
expect.any(Function),
expect.any(Function),
undefined,
expect.any(Object),
);
});

it('matches StopFailure session hooks against the error type', async () => {
const sessionHook = createSessionHookEntry(
HookEventName.StopFailure,
'rate_limit',
);

vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue(null);
vi.mocked(mockSessionHooksManager.getMatchingHooks).mockReturnValue([
sessionHook,
]);
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
createMockAggregatedResult(true),
);

await hookEventHandler.fireStopFailureEvent('rate_limit');

expect(mockSessionHooksManager.getMatchingHooks).toHaveBeenCalledWith(
'test-session-id',
HookEventName.StopFailure,
'rate_limit',
);
expect(mockHookRunner.executeHooksParallel).toHaveBeenCalledWith(
[sessionHook.config],
HookEventName.StopFailure,
expect.objectContaining({ error: 'rate_limit' }),
expect.any(Function),
expect.any(Function),
undefined,
expect.any(Object),
);
});

it('matches Notification session hooks against the notification type', async () => {
const sessionHook = createSessionHookEntry(
HookEventName.Notification,
NotificationType.PermissionPrompt,
);

vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue(null);
vi.mocked(mockSessionHooksManager.getMatchingHooks).mockReturnValue([
sessionHook,
]);
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
createMockAggregatedResult(true),
);

await hookEventHandler.fireNotificationEvent(
'permission needed',
NotificationType.PermissionPrompt,
);

expect(mockSessionHooksManager.getMatchingHooks).toHaveBeenCalledWith(
'test-session-id',
HookEventName.Notification,
NotificationType.PermissionPrompt,
);
expect(mockHookRunner.executeHooksParallel).toHaveBeenCalledWith(
[sessionHook.config],
HookEventName.Notification,
expect.objectContaining({
notification_type: NotificationType.PermissionPrompt,
}),
expect.any(Function),
expect.any(Function),
undefined,
expect.any(Object),
);
});

it('does not matcher-filter session hooks for events without matcher semantics', async () => {
const sessionHook = createSessionHookEntry(
HookEventName.UserPromptSubmit,
'ignored-matcher',
);

vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue(null);
vi.mocked(mockSessionHooksManager.getHooksForEvent).mockReturnValue([
sessionHook,
]);
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
createMockAggregatedResult(true),
);

await hookEventHandler.fireUserPromptSubmitEvent('hello');

expect(mockSessionHooksManager.getHooksForEvent).toHaveBeenCalledWith(
'test-session-id',
HookEventName.UserPromptSubmit,
);
expect(mockSessionHooksManager.getMatchingHooks).not.toHaveBeenCalled();
expect(mockHookRunner.executeHooksParallel).toHaveBeenCalledWith(
[sessionHook.config],
HookEventName.UserPromptSubmit,
expect.objectContaining({ prompt: 'hello' }),
expect.any(Function),
expect.any(Function),
undefined,
expect.any(Object),
);
});
});

describe('sequential vs parallel execution', () => {
it('should execute hooks sequentially when plan.sequential is true', async () => {
const mockPlan = createMockExecutionPlan(
Expand Down
20 changes: 12 additions & 8 deletions packages/core/src/hooks/hookEventHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import type { Config } from '../config/config.js';
import type { HookPlanner, HookEventContext } from './hookPlanner.js';
import { getHookMatcherTarget } from './hookPlanner.js';
import type { HookRunner } from './hookRunner.js';
import type { HookAggregator, AggregatedHookResult } from './hookAggregator.js';
import type { SessionHooksManager } from './sessionHooksManager.js';
Expand Down Expand Up @@ -568,14 +569,17 @@ export class HookEventHandler {

// Get session hooks and merge with registry hooks
const sessionId = input.session_id;
const targetName = context?.toolName || '';
const sessionHooks = sessionId
? this.sessionHooksManager.getMatchingHooks(
sessionId,
eventName,
targetName,
)
: [];
const matcherTarget = getHookMatcherTarget(eventName, context)?.target;
const sessionHooks =
sessionId !== undefined
? matcherTarget === undefined
? this.sessionHooksManager.getHooksForEvent(sessionId, eventName)
: this.sessionHooksManager.getMatchingHooks(
sessionId,
eventName,
matcherTarget,
)
: [];

// Merge hook configs from registry plan and session hooks
const registryHookConfigs = plan?.hookConfigs || [];
Expand Down
Loading
Loading