fix(core): align session hook matcher targets#4354
Conversation
📋 Review SummaryThis PR aligns session hook matcher targets with the event-specific context fields used by configured hooks, ensuring consistent behavior across both hook systems. The implementation is well-tested, type-safe, and mirrors the existing 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
| context?: HookEventContext, | ||
| ): string | undefined { | ||
| switch (eventName) { | ||
| case HookEventName.PreToolUse: |
There was a problem hiding this comment.
[Suggestion] getSessionHookMatcherTarget reimplements the same event-to-field mapping that HookPlanner.matchesContext() already encodes — two switch (eventName) blocks that must stay in lockstep. If a future PR adds a new HookEventName and updates one switch but not the other, session hooks and configured hooks will silently diverge with no compile-time error.
Consider extracting a shared helper (e.g., in hookPlanner.ts or a new matcherUtils.ts):
export function getEventMatcherField(eventName: HookEventName): keyof HookEventContext | undefined {
switch (eventName) {
case HookEventName.PreToolUse:
case HookEventName.PostToolUse:
case HookEventName.PostToolUseFailure:
case HookEventName.PermissionRequest:
return 'toolName';
case HookEventName.SubagentStart:
case HookEventName.SubagentStop:
return 'agentType';
// ... etc
default:
return undefined;
}
}Both matchesContext and this method consume the single source of truth, eliminating drift risk.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| case HookEventName.TodoCompleted: | ||
| default: | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The explicit default case prevents TypeScript from flagging unhandled HookEventName members. If someone adds a new event to the enum and forgets to add a case here, session hooks for that event will silently lose matcher filtering (running unconditionally) with no compile error.
Consider removing default and adding an exhaustiveness assertion:
case HookEventName.UserPromptSubmit:
case HookEventName.Stop:
case HookEventName.TodoCreated:
case HookEventName.TodoCompleted:
return undefined;
}
const _exhaustive: never = eventName;
return _exhaustive;Note: HookPlanner.matchesContext has the same pattern — both would benefit from this fix.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('session hook matcher targets', () => { |
There was a problem hiding this comment.
[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:
-
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. -
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
| eventName: HookEventName, | ||
| context?: HookEventContext, | ||
| ): HookMatcherTarget | undefined { | ||
| switch (eventName) { |
There was a problem hiding this comment.
[Critical] ESLint default-case: this switch (eventName) is missing a default case, causing the CI Lint check to fail. The project convention (see mermaidImageRenderer.ts:832) wraps the never exhaustiveness assertion inside a default block:
| switch (eventName) { | |
| case HookEventName.UserPromptSubmit: | |
| case HookEventName.Stop: | |
| case HookEventName.TodoCreated: | |
| case HookEventName.TodoCompleted: | |
| return undefined; | |
| default: { | |
| const exhaustive: never = eventName; | |
| return exhaustive; | |
| } | |
| } |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| return true; | ||
| } | ||
|
|
||
| switch (matcherTarget.kind) { |
There was a problem hiding this comment.
[Critical] Same ESLint default-case violation as above. Apply the same fix:
| switch (matcherTarget.kind) { | |
| case 'sessionTrigger': | |
| return this.matchesSessionTrigger(matcher, matcherTarget.target); | |
| default: { | |
| const exhaustive: never = matcherTarget.kind; | |
| return exhaustive; | |
| } | |
| } |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| target: string; | ||
| } | ||
|
|
||
| export function getHookMatcherTarget( |
There was a problem hiding this comment.
[Suggestion] getHookMatcherTarget is a new exported pure function with 7 branch groups but has zero direct unit tests — all coverage is indirect through integration tests. Consider adding a describe('getHookMatcherTarget', ...) block in hookPlanner.test.ts that directly asserts each branch's return value, including the ?? '' fallback for missing context fields and the undefined return for matcher-less events.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
All Round 2 findings are addressed in this revision:
- ESLint
default-case(Critical): Both switches inhookPlanner.tsnow wrap theneverexhaustiveness assertion insidedefaultblocks — CI Lint passes. getHookMatcherTargettests (Suggestion): 7 new unit tests covering all event categories added inhookPlanner.test.ts.- Session hook matcher tests (Suggestion): 6 new integration tests covering SessionStart, PreToolUse, SubagentStart, StopFailure, Notification, and the no-matcher-semantics path added in
hookEventHandler.test.ts.
Build passes, typecheck passes, all 174 tests pass. LGTM! ✅
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
pomelo-nwu
left a comment
There was a problem hiding this comment.
Review: Approve
Product direction — sound, and this is a real bug fix, not just a style unification.
Before this change, hookEventHandler always used context?.toolName || '' as the matcher target for session hooks. For non-tool events (SessionStart / Notification / StopFailure / Subagent*, etc.) the target was always '', so matchesPattern('resume', '') -> /^resume$/.test('') -> false. That means a session hook with a concrete matcher on a non-tool event would never fire, while config hooks (via HookPlanner) matched correctly against event-specific fields — a genuine behavior divergence. Extracting getHookMatcherTarget as the single source of truth for "event -> matched field", shared by both paths, is the right fix.
Code quality — good.
- Exhaustiveness check is solid: I verified all 16
HookEventNamevalues are covered (toolName x4 / agentType x2 / trigger x2 / sessionTrigger x2 / error x1 / notificationType x1 / undefined x4), soconst exhaustive: never = eventNamecompiles, and any future event added without handling will fail the build. - Behavior alignment is correct: events without matcher semantics (UserPromptSubmit / Stop / TodoCreated / TodoCompleted) go through
getHooksForEvent(no filtering), matching config-hook behavior. NotablyStopnow always runs, which is safer for/goal. - Test coverage is adequate.
Optional (non-blocking):
- For tool events with a missing
context,getHookMatcherTargetreturnstarget: ''(notundefined), so it goes throughgetMatchingHooks(…, '')rather than "match all". Harmless in practice (tool events always carry a toolName), but slightly inconsistent with the "empty target = match all" semantics — worth a comment. - Consider adding a reverse regression assertion — e.g. "a
resumematcher does NOT fire on astartupSessionStart" — to lock down exactly the bug being fixed. Current tests assert the correct target is passed, but not the negative filtering case.
中文说明
评审结论：通过（Approve）
产品方向 —— 合理，且是真实 bug 修复，不只是风格统一。
改动前，hookEventHandler 对 session hook 永远用 context?.toolName || '' 当匹配目标。对非工具事件（SessionStart / Notification / StopFailure / Subagent* 等）target 恒为 ''，于是 matchesPattern('resume', '') -> /^resume$/.test('') -> false，意味着在非工具事件上配了具体 matcher 的 session hook 永远不会触发；而 config hook（走 HookPlanner）用 event-specific 字段匹配是对的 —— 两条路径行为分歧。把"事件 -> 匹配字段"映射抽成单一函数 getHookMatcherTarget、两条路径共用，是正确的修法。
代码质量 —— 好。
- 穷尽性检查可靠：已核对
HookEventName全部 16 个值都被覆盖，const exhaustive: never = eventName能编译通过，将来新增事件漏处理会直接编译失败。 - 行为对齐正确：无 matcher 语义的事件走
getHooksForEvent（不过滤），与 config hook 一致；尤其Stop现在一定执行，对/goal更安全。 - 测试覆盖充分。
可选建议（不阻塞）：
- 工具事件在
context缺失时返回target: ''（非 undefined），会走getMatchingHooks(…, '')而非"全过"。实际无害（工具事件必带 toolName），但与"空 target = 全过"语义略不一致，值得一句注释。 - 可补一个反向回归断言（如"
startup触发的 SessionStart 上 matcher=resume的 hook 不执行"），直接锁住这次修的 bug。
Local maintainer validation — all PR-relevant gates green ✅Reviewed at head Environment
Results
Unrelated test failures (reproduced on base; NOT caused by this PR)
End-to-end proof (against built
|
Summary
toolName, so non-tool events such asSessionStart,Notification, andStopFailurecould not be filtered by their meaningful payload values.Validation
hookEventHandler.test.tspassed with 114 tests.npm run buildcompleted successfully with one unrelated existing lint warning inpackages/vscode-ide-companion/src/utils/editorGroupUtils.tsand a Browserslist data-age warning.npm run typecheckcompleted successfully.1 passed,114 passed; build and typecheck exited with code 0.Scope / Risk
Testing Matrix
Testing matrix notes:
npm run build,npm run typecheck, and the focused package-level Vitest command.Linked Issues / Bugs
Related to #4343.