fix: correct Claude settings.json hook emission (#1007)#1054
fix: correct Claude settings.json hook emission (#1007)#1054danielmeppiel merged 4 commits intomainfrom
Conversation
Fix four bugs in .claude/settings.json hook emission when installing
multi-target hook packages:
1. Target-aware hook file routing -- files named *-cursor-hooks.json,
*-claude-hooks.json etc. are now routed only to their intended target
instead of leaking into every target's config.
2. Expanded variable pattern recognition -- bare ${PLUGIN_ROOT} and
${CURSOR_PLUGIN_ROOT} are now correctly rewritten alongside
${CLAUDE_PLUGIN_ROOT} during target-specific command rewriting.
3. Event name normalisation for Claude -- camelCase event names
(postToolUse, preToolUse) from Copilot/Cursor hook files are
normalised to PascalCase (PostToolUse, PreToolUse) for Claude.
4. Content-based deduplication -- duplicate hook entries from multiple
source files targeting the same event are collapsed (scoped to
same-package entries to preserve cross-package ownership).
Includes alias-aware clearing so reinstalls correctly remove entries
written under previously leaked event name variants.
Closes #1007
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (1 item)
Nits (16 items, skip if you want)
CEO arbitrationThis is a well-scoped, well-tested bug fix that closes a real user-reported issue (#1007). The four fixes are surgical, the 18 new tests provide strong regression coverage, and the docs/guides/plugins.md update documents the new convention. The panelists converged on the quality of the implementation -- no REQUIRED findings on the code itself. The sole REQUIRED finding comes from DevX: the apm-usage skill resource (package-authoring.md) was not updated with the new target-specific hook filename convention. I uphold this as REQUIRED. Rule 4 is an explicit written instruction in this repository, the convention is genuinely new (not a fix to existing behaviour), and the apm-usage skill is the primary surface AI agents consult when authoring packages. Without the update, agents will never discover that naming a file On nits: the non-ASCII characters in the test file (box-drawing, en dashes, arrows in docstrings and section banners) violate the project encoding rule which scopes to all source files, not just CLI output. These should be cleaned up in this PR since the test file is new code being added. The O(n^2) dedup scan noted by three panelists is fine at current hook-count scale. The CHANGELOG single-line entry follows Keep a Changelog convention (one line per PR) but could benefit from the Growth Hacker's bulleted sub-list suggestion given that four distinct fixes are packed into one sentence -- that is an author-discretion nit, not a blocker. Dissent resolved: DevX classified the missing apm-usage/package-authoring.md update as REQUIRED; OSS Growth Hacker echoed the same gap but classified it as a nit. I side with DevX. Rule 4 is not discretionary guidance -- it is a mandatory instruction checked by the project's own conventions. The convention is new, not pre-existing, so the omission is attributable to this PR. Growth/positioning note: The Growth Hacker's observation about a runnable directory-tree example in the docs is worth amplifying. Package authors seeing a concrete tree (e.g. my-package/hooks/copilot-hooks.json, cursor-hooks.json, hooks.json) will grok the convention faster than a table alone. Consider adding one to either plugins.md or package-authoring.md. Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class BaseIntegrator {
check_collision()
cleanup_empty_parents()
should_integrate()
}
class HookIntegrator {
+HOOK_COMMAND_KEYS Tuple
+find_hook_files(package_path) List~Path~
+integrate_package_hooks()
+integrate_hooks_for_target()
-_integrate_merged_hooks()
-_rewrite_hooks_data()
-_rewrite_command_for_target()
-_parse_hook_json()
+sync_integration()
}
class _MergeHookConfig {
+config_filename str
+target_key str
+require_dir bool
}
class _filter_hook_files_for_target {
+__call__(hook_files, target_key) List~Path~
}
class _HOOK_EVENT_MAP {
+claude dict
+gemini dict
}
class _HOOK_FILE_TARGET_SUFFIXES {
+copilot-hooks set
+cursor-hooks set
+claude-hooks set
+codex-hooks set
+gemini-hooks set
}
BaseIntegrator <|-- HookIntegrator
HookIntegrator *-- _MergeHookConfig : uses via _MERGE_HOOK_TARGETS
HookIntegrator ..> _filter_hook_files_for_target : calls at 2 sites
HookIntegrator ..> _HOOK_EVENT_MAP : reads during merge
_filter_hook_files_for_target ..> _HOOK_FILE_TARGET_SUFFIXES : reads
CLI Logging Expert
DevX UX Expert
Supply Chain Security Expert
Auth ExpertInactive -- PR touches only hook_integrator.py, test_hook_integrator.py, CHANGELOG.md, and plugins.md -- no auth, token, credential, or host classification files changed. OSS Growth Hacker
Verdict computed deterministically: 1 required finding across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. |
…ASCII encoding - Add target-specific hook file naming section to packages/apm-guide/.apm/skills/apm-usage/package-authoring.md (addresses REQUIRED panel finding from DevX UX Expert re Rule 4) - Replace non-ASCII characters (box-drawing, en dashes, em dashes, arrows) with ASCII equivalents in test docstrings and comments - Fix pre-existing em-dash in hook_integrator.py comment (line 614) - Improve docs table clarity and add directory tree example Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Panel findings addressed (714531a)All findings from the review panel have been addressed: REQUIRED (resolved):
Nits adopted:
Nits deferred (follow-up):
|
sergio-sisternes-epam
added
the
panel-review
APM Review Panel Verdict: APPROVE
All REQUIRED findings are resolved. No new REQUIRED findings identified. Prior REQUIRED finding — resolved ✅
Nits addressed since prior review ✅
Deferred nits (follow-up PRs, not blocking)
CEO arbitrationThe prior REJECT was on a single, low-effort gap: the apm-usage skill resource lacked the new target-specific hook filename convention. That gap is closed. The four bug-fixes remain well-scoped, well-tested (18 new regression tests covering all fix dimensions), and the documentation is now coherent across The PR is a draft and has a dirty merge state — those are operational matters for the author to resolve before merge, not panel concerns. Panel approves. Clear merge conflicts and mark ready for review to proceed. Per-persona findings (full)Python ArchitectNo new REQUIRED findings. Architecture is correct: CLI Logging ExpertNo new REQUIRED findings. Em-dash cleaned. New test class is ASCII-clean. Pre-existing non-ASCII section banners (box-drawing chars in lines 34–2495) are out-of-scope for this PR. Deferred: debug logging for filtered/deduped hooks. DevX UX ExpertNo new REQUIRED findings. Rule 4 satisfied: Supply Chain Security ExpertNo new REQUIRED findings. Auth ExpertInactive — no auth, token, credential, or host classification files changed. OSS Growth HackerNo new REQUIRED findings. Directory tree examples present in both docs. CHANGELOG single-line follows Keep a Changelog convention (author discretion to expand). Deferred: bulleted sub-list. Verdict computed deterministically: 0 required findings across 5 active panelists. APPROVE iff N == 0. ✅
|
github-actions
Bot
added
panel-approved
github-actions
Bot
removed
the
panel-approved
There was a problem hiding this comment.
Pull request overview
This PR fixes .claude/settings.json hook emission when installing multi-target hook packages by ensuring only target-appropriate hook files are merged, normalizing event names for Claude, expanding additional plugin-root variable forms, and preventing duplicate hook entries.
Changes:
- Add target-aware routing for hook JSON files via filename stem suffixes (e.g.,
*-cursor-hooks.jsononly to Cursor; generichooks.jsonto all). - Expand plugin-root variable rewriting to handle
${PLUGIN_ROOT}and${CURSOR_PLUGIN_ROOT}in addition to${CLAUDE_PLUGIN_ROOT}, and normalize Claude event keys (camelCase -> PascalCase). - Add regression tests for issue #1007 and document target-specific hook filename conventions.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/integration/hook_integrator.py |
Adds hook-file routing by target, Claude event mapping, expanded plugin-root variable matching, alias-aware clearing, and content-based dedup logic. |
tests/unit/integration/test_hook_integrator.py |
Adds a new regression test class covering routing, variable rewriting, event normalization, deduplication, and reinstall behavior. |
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md |
Documents target-specific hook filenames and notes event/variable normalization behavior. |
docs/src/content/docs/guides/plugins.md |
Documents target-specific hook filenames in the plugin authoring guide. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
| seen_content: list[dict] = [] | ||
| deduped: list = [] | ||
| for entry in json_config["hooks"][event_name]: | ||
| if not isinstance(entry, dict): | ||
| deduped.append(entry) | ||
| continue | ||
| # Build comparison key (all fields except _apm_source) | ||
| cmp = {k: v for k, v in sorted(entry.items()) if k != "_apm_source"} | ||
| source = entry.get("_apm_source") | ||
| is_dup = False | ||
| for seen in seen_content: | ||
| if seen.get("_source") == source and seen.get("_cmp") == cmp: | ||
| is_dup = True | ||
| break | ||
| if not is_dup: | ||
| seen_content.append({"_source": source, "_cmp": cmp}) |
There was a problem hiding this comment.
The new content-based deduplication runs over the entire event list and dedups entries for any _apm_source value (including other packages) and also user-authored entries that have no _apm_source (treated as None). This can unintentionally remove duplicates that are not owned by the package currently being installed. Dedup should be scoped to the current package only (e.g., only consider/remove duplicates among entries where _apm_source == package_name, leaving other entries untouched).
| seen_content: list[dict] = [] | |
| deduped: list = [] | |
| for entry in json_config["hooks"][event_name]: | |
| if not isinstance(entry, dict): | |
| deduped.append(entry) | |
| continue | |
| # Build comparison key (all fields except _apm_source) | |
| cmp = {k: v for k, v in sorted(entry.items()) if k != "_apm_source"} | |
| source = entry.get("_apm_source") | |
| is_dup = False | |
| for seen in seen_content: | |
| if seen.get("_source") == source and seen.get("_cmp") == cmp: | |
| is_dup = True | |
| break | |
| if not is_dup: | |
| seen_content.append({"_source": source, "_cmp": cmp}) | |
| # | |
| # Scope deduplication to entries owned by the package being | |
| # installed. Leave entries from other packages and | |
| # user-authored entries without _apm_source untouched. | |
| seen_content: list[dict] = [] | |
| deduped: list = [] | |
| for entry in json_config["hooks"][event_name]: | |
| if not isinstance(entry, dict): | |
| deduped.append(entry) | |
| continue | |
| if entry.get("_apm_source") != package_name: | |
| deduped.append(entry) | |
| continue | |
| # Build comparison key for package-owned entries | |
| # only (all fields except _apm_source). | |
| cmp = { | |
| k: v for k, v in sorted(entry.items()) if k != "_apm_source" | |
| } | |
| is_dup = False | |
| for seen in seen_content: | |
| if seen == cmp: | |
| is_dup = True | |
| break | |
| if not is_dup: | |
| seen_content.append(cmp) |
Description
Fixes four bugs in
.claude/settings.jsonhook emission when installing multi-target hook packages (e.g.microsoft/azure-skills):${CURSOR_PLUGIN_ROOT}emitted into Claude's config (Claude does not expand it)${PLUGIN_ROOT}-- unrecognised variable passes through unchangedPostToolUseandpostToolUseappear; Claude only reads PascalCaseFixes #1007
Type of change
Approach
Four surgical fixes in
hook_integrator.py:_filter_hook_files_for_target()routes*-cursor-hooks.jsononly to Cursor,*-claude-hooks.jsononly to Claude, etc. Generichooks.jsondeploys everywhere.${PLUGIN_ROOT}and${CURSOR_PLUGIN_ROOT}pass throughpostToolUseandPostToolUsekeys in Claude config_HOOK_EVENT_MAPmapping camelCase to PascalCaseAdditionally, alias-aware clearing ensures reinstalls correctly remove entries written under previously leaked event name variants.
Testing
TestIssue1007Fixesclass covering: