fix(core): Match hook display-name matchers to tool ids - #6373
Merged
Conversation
LaZzyMan
force-pushed
the
lazzy/hook-matcher-tool-alias
branch
from
July 6, 2026 07:23
4539c8b to
7b61537
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
LaZzyMan
marked this pull request as ready for review
July 6, 2026 07:26
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
requested changes
Jul 6, 2026
qwen-code-ci-bot
requested changes
Jul 7, 2026
| HookConfig, | ||
| HookExecutionResult, | ||
| } from './types.js'; | ||
| HookEventName} from './types.js'; |
Collaborator
There was a problem hiding this comment.
[Suggestion] Malformed import formatting: HookEventName} is on the same line with wrong indentation. Prettier check fails on this file.
Suggested change
| HookEventName} from './types.js'; | |
| HookEventName, | |
| } from './types.js'; |
— qwen3.7-max via Qwen Code /review
|
|
||
| const debugLogger = createDebugLogger('SESSION_HOOKS_MANAGER'); | ||
|
|
||
| function isToolMatcherEvent(event: HookEventName): boolean { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The new isToolMatcherEvent routing adds a branch that dispatches tool events through matchesToolPattern and non-tool events through matchesPattern. All getMatchingHooks tests use PreToolUse only — there is no test verifying that non-tool events (e.g., SubagentStart, SessionStart) still match correctly through the non-tool branch after this dispatch was added.
Consider adding at least one test that registers a hook for a non-tool event and verifies getMatchingHooks returns correct results.
— qwen3.7-max via Qwen Code /review
| private matchesToolName(matcher: string, toolName: string): boolean { | ||
| const targets = getToolMatcherTargets(toolName); | ||
|
|
||
| if ( |
Collaborator
There was a problem hiding this comment.
[Suggestion] The pipe-split in matchesToolName only checks aliases per alternative then falls through to regex on the full unsplit string. In contrast, sessionHooksManager.matchesToolPattern recurses per alternative, applying alias check + anchored regex to each independently. The two implementations of the same matching logic diverge — consider extracting a shared helper to prevent further drift.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No new findings beyond the 12 existing inline comments. All identified concerns (pipe-split divergence, alias resolution duplication, test coverage gaps, regex anchoring inconsistency, import formatting) are already captured. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /triage |
Collaborator
|
Thanks for the PR! Template looks good. Problem: Observed and documented. Issue #6321's side note explicitly reports that a matcher of Direction: Aligned. The docs' examples used display names ( Size: 68 production logic lines across 3 core files ( Approach: Minimal and focused. Reuses the existing Moving on to code review. Chinese感谢贡献！ 模板完整。 问题： 已观测到并有记录。#6321 的 side note 明确报告了 方向： 对齐。文档示例使用展示名，但运行时传递的是 snake_case id。让 matcher 同时接受两者是正确的做法——向后兼容且低风险。 规模： 3 个核心文件共 68 行生产逻辑。194 行测试。70 行文档。远低于任何阈值。 方案： 最小化且聚焦。复用了已有的 进入代码审查。 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal: I would have exported Findings: The implementation is correct and well-tested. The alias expansion is exact-match only (no substring matching through aliases), which is the right call — it prevents One observation: the pipe-split heuristic ( No correctness bugs, no security concerns, no regressions. The change is backward-compatible: existing runtime-id matchers ( TestingUnit tests: 107/107 pass ( Behavioral verification (ran on PR branch via The fix works as advertised. Tmux scenario test: Not practical for hook matcher logic — would require full API-key setup, hook config file, and triggering a specific tool call interactively. The unit tests + behavioral verification above provide equivalent coverage of the matching semantics. Chinese代码审查独立方案： 我会导出 发现： 实现正确且测试充分。别名展开仅限精确匹配，防止 管道分隔的启发式逻辑在两处重复，可以接受但将来可作为小的 DRY 改进。不阻塞。 无正确性 bug、无安全问题、无回归。 测试单元测试： 107/107 通过。新测试覆盖所有关键边界情况。 行为验证： 见上方英文部分的终端输出。修复前 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, well-scoped fix for a real user-facing inconsistency. The docs said The approach is exactly what I would have done: reuse the existing The before/after verification is unambiguous — 107 unit tests pass, including 8 new tests that cover the key edge cases (display names, pipe-separated patterns, legacy aliases, regex isolation, negative lookahead exclusion, unknown-tool passthrough). 68 production lines, 194 test lines, 70 doc lines. Nothing extraneous. Approving. Chinese这是一个干净、范围合理的修复，解决了一个真实存在的用户痛点。文档写的是 方案和我独立想到的一致：复用已有的 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 7, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
🔬 Local E2E Verification Report / 本地 E2E 验证报告Verified by: Maintainer local build + tmux-based behavioral test Unit Tests / 单元测试Behavioral E2E Test: Before vs After / 行为对比测试Built the project locally on both 在 ❌ BEFORE (main branch)→ Display-name matchers silently fail — configs following the old docs examples are broken. ✅ AFTER (PR 6373 branch)→ All display-name matchers now correctly resolve to runtime tool IDs. Code Review Summary / 代码审查总结
Verdict / 结论✅ LGTM — Ready to merge. The fix is minimal, well-tested, and backward-compatible. It correctly addresses the gap where display-name matchers (following old doc examples) silently failed to trigger hooks. 修复最小化、测试充分、向后兼容。正确解决了按旧文档示例使用展示名 matcher 时 hook 静默不触发的问题。 📸 Screenshots saved locally at E2E verified on macOS (Apple Silicon) with Node 22.x, built from source on both branches. |
wenshao
approved these changes
Jul 7, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — verified locally with unit tests (107/107), E2E behavioral test (20/20), and before/after comparison on main. See detailed report in comment above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Allows built-in hook matcher patterns written with display names, such as
WriteFile, to match the runtime tool id used during hook execution, such aswrite_file. The hook input still reports the raw runtime id, and the user docs now recommend runtime ids while documenting display-name compatibility.Why it's needed
The PreToolUse hook documentation and examples implied that a matcher like
WriteFilewould catch write-file operations. At runtime, the hook matcher only sawwrite_file, so configurations that followed the display-name example did not run for write-file tool calls.Reviewer Test Plan
How to verify
Reviewers can confirm that a PreToolUse matcher written as
WriteFilenow matches a write-file tool call whose runtime id iswrite_file, and that configs using the rawwrite_fileid remain the recommended path.Evidence (Before & After)
N/A - non-UI hook matcher and documentation change.
Tested on
Environment (optional)
Local validation:
cd packages/core && npx vitest run src/hooks/hookPlanner.test.ts src/hooks/sessionHooksManager.test.tscd packages/core && npx prettier --check src/hooks/hookPlanner.ts src/hooks/hookPlanner.test.ts src/hooks/sessionHooksManager.ts src/hooks/sessionHooksManager.test.ts ../../docs/users/features/hooks.mdnpm run build && npm run typecheckRisk & Scope
tool_namevalue passed to hooks.permissionDecision: "ask"runtime issue from PR feat(core): surface PreToolUse hook 'ask' as a TUI confirmation #5629 is intentionally not changed here.Linked Issues
Refs #6321.
中文说明
这个 PR 做了什么
让使用内置工具展示名编写的 hook matcher（例如
WriteFile）可以匹配 hook 执行时使用的运行时工具 id（例如write_file）。hook 输入仍然报告原始运行时 id，用户文档现在推荐使用运行时 id，并说明展示名 matcher 兼容行为。为什么需要
PreToolUse hook 文档和示例暗示
WriteFile这样的 matcher 会拦截写文件操作。但运行时 hook matcher 只看到write_file，所以只按展示名示例配置的 hook 不会在写文件工具调用时运行。Reviewer Test Plan
如何验证
Reviewer 可以确认写成
WriteFile的 PreToolUse matcher 现在会匹配运行时 id 为write_file的写文件工具调用，同时使用原始write_fileid 的配置仍然是推荐路径。Evidence（Before & After）
N/A - 非 UI 的 hook matcher 和文档改动。
Tested on
Environment（可选）
本地验证：
cd packages/core && npx vitest run src/hooks/hookPlanner.test.ts src/hooks/sessionHooksManager.test.tscd packages/core && npx prettier --check src/hooks/hookPlanner.ts src/hooks/hookPlanner.test.ts src/hooks/sessionHooksManager.ts src/hooks/sessionHooksManager.test.ts ../../docs/users/features/hooks.mdnpm run build && npm run typecheckRisk & Scope
tool_name不变。permissionDecision: "ask"运行时问题，这里刻意不处理。Linked Issues
Refs #6321.