From 5430a50059befd914a144a9a074b074715f06338 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Wed, 29 Apr 2026 22:50:30 +0100 Subject: [PATCH 1/2] fix: correct Claude settings.json hook emission (#1007) 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> --- CHANGELOG.md | 1 + docs/src/content/docs/guides/plugins.md | 18 + src/apm_cli/integration/hook_integrator.py | 104 +++- .../unit/integration/test_hook_integrator.py | 553 +++++++++++++++++- 4 files changed, 671 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c7dc47d2..f52e68400 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Hook integration no longer leaks cross-target variables (`${CURSOR_PLUGIN_ROOT}`, `${PLUGIN_ROOT}`) into Claude's `settings.json`, normalises event names to each target's convention (e.g. `postToolUse` -> `PostToolUse` for Claude), and deduplicates entries from multi-file hook packages. Target-specific hook files (e.g. `cursor-hooks.json`) are now routed only to their intended target. (#1007) - `shared/apm.md` gh-aw workflow no longer fails the "Validate downloaded bundles match matrix manifest" step with a spurious `missing APM bundles (group did not pack successfully): apm-default` error when the matrix has exactly one credential group. `actions/download-artifact@v5+` flattens contents directly into the destination path whenever a single artifact matches the pattern (overriding `merge-multiple: false`), which collapsed the per-group subdir layout the validator expects. A new "Normalise bundle layout" step re-creates the expected `apm-/` directory in the single-group case before validation runs. (#1051) - `apm install` and `apm compile` no longer exit 0 with success messages when `target:` in `apm.yml` is a CSV string -- the value now parses identically to the same input on `--target`, and zero-target resolution surfaces a warning instead of a silent no-op. (#820) - Remove redundant `seen` set from `_scan_patterns()` discovery walk (#918) diff --git a/docs/src/content/docs/guides/plugins.md b/docs/src/content/docs/guides/plugins.md index e5e1aaf54..35f83c6ee 100644 --- a/docs/src/content/docs/guides/plugins.md +++ b/docs/src/content/docs/guides/plugins.md @@ -209,6 +209,24 @@ By default APM looks for `agents/`, `skills/`, `commands/`, and `hooks/` directo - For **agents**, directory contents are flattened into `.apm/agents/` (agents are flat files, not named directories) - `hooks` also accepts an inline object: `"hooks": {"hooks": {"PreToolUse": [...]}}` +##### Target-specific hook files + +When a package ships hooks for multiple tools, use target-specific filenames so +each tool receives only its own hooks: + +| Filename pattern | Deployed to | +|---|---| +| `*-copilot-hooks.json` | GitHub Copilot only | +| `*-cursor-hooks.json` | Cursor only | +| `*-claude-hooks.json` | Claude Code only | +| `*-codex-hooks.json` | Codex CLI only | +| `*-gemini-hooks.json` | Gemini CLI only | +| `hooks.json` or other non-target `*-hooks.json` names | All targets | + +For example, a package with `copilot-hooks.json`, `cursor-hooks.json`, and +`hooks.json` routes the named files to their target while `hooks.json` deploys +everywhere. + #### MCP Server Definitions Plugins can ship MCP servers that are automatically deployed through APM's MCP pipeline. Define servers using `mcpServers` in `plugin.json`: diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index f8e12412c..14ad13d38 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -37,7 +37,8 @@ } Script path handling: - - ${CLAUDE_PLUGIN_ROOT}/path -> resolved relative to package root, rewritten for target + - ${CLAUDE_PLUGIN_ROOT}/path, ${CURSOR_PLUGIN_ROOT}/path, ${PLUGIN_ROOT}/path + -> resolved relative to package root, rewritten for target - ./path -> relative path, resolved from hook file's parent directory, rewritten for target - System commands (no path separators) -> passed through unchanged """ @@ -89,6 +90,11 @@ class _MergeHookConfig: # Copilot (camelCase) or Claude (PascalCase) names; targets that use # different conventions get their events renamed during merge. _HOOK_EVENT_MAP: dict[str, dict[str, str]] = { + "claude": { + # Copilot camelCase -> Claude PascalCase + "preToolUse": "PreToolUse", + "postToolUse": "PostToolUse", + }, "gemini": { # Copilot / Claude -> Gemini "PreToolUse": "BeforeTool", @@ -167,6 +173,53 @@ def _copilot_keys_to_gemini(hook: dict) -> None: } +# Mapping from hook-file stem suffix to the set of target keys that +# should receive the file. Files whose stem does not match any +# suffix are treated as universal and deployed to every target. +_HOOK_FILE_TARGET_SUFFIXES: dict[str, set[str]] = { + "copilot-hooks": {"copilot", "vscode"}, + "cursor-hooks": {"cursor"}, + "claude-hooks": {"claude"}, + "codex-hooks": {"codex"}, + "gemini-hooks": {"gemini"}, +} + + +def _filter_hook_files_for_target( + hook_files: List[Path], + target_key: str, +) -> List[Path]: + """Return only hook files intended for *target_key*. + + Routing is based on the file stem (case-insensitive): + - Stems ending with a known ``--hooks`` suffix are + restricted to matching targets. + - All other stems (e.g. ``hooks``, ``my-custom-hooks``) are + universal and pass through for every target. + + Args: + hook_files: All discovered hook JSON files. + target_key: Lowercase target name (e.g. ``"claude"``, ``"cursor"``). + + Returns: + Filtered list preserving original order. + """ + result: List[Path] = [] + for hf in hook_files: + stem_lower = hf.stem.lower() + matched_suffix: str | None = None + for suffix, allowed_targets in _HOOK_FILE_TARGET_SUFFIXES.items(): + if stem_lower == suffix or stem_lower.endswith(f"-{suffix}"): + matched_suffix = suffix + if target_key in allowed_targets: + result.append(hf) + break + if matched_suffix is None: + # Universal file -- deploy to all targets + result.append(hf) + return result + + class HookIntegrator(BaseIntegrator): """Handles integration of APM package hooks into target locations. @@ -295,10 +348,10 @@ def _rewrite_command_for_target( base_root = root_dir or ".claude" scripts_base = f"{base_root}/hooks/{package_name}" - # Handle ${CLAUDE_PLUGIN_ROOT} references (always relative to package root) + # Handle plugin root variable references (always relative to package root) # Match both forward-slash and backslash separators (Windows hook JSON # may use backslashes: ${CLAUDE_PLUGIN_ROOT}\scripts\scan.ps1) - plugin_root_pattern = r'\$\{CLAUDE_PLUGIN_ROOT\}([\\/][^\s]+)' + plugin_root_pattern = r'\$\{(?:CLAUDE_PLUGIN_ROOT|CURSOR_PLUGIN_ROOT|PLUGIN_ROOT)\}([\\/][^\s]+)' for match in re.finditer(plugin_root_pattern, command): full_var = match.group(0) # Normalize backslashes to forward slashes before Path construction @@ -452,6 +505,7 @@ def integrate_package_hooks(self, package_info, project_root: Path, HookIntegrationResult: Results of the integration operation """ hook_files = self.find_hook_files(package_info.install_path) + hook_files = _filter_hook_files_for_target(hook_files, "copilot") if not hook_files: return HookIntegrationResult( @@ -547,6 +601,7 @@ def _integrate_merged_hooks( return _empty hook_files = self.find_hook_files(package_info.install_path) + hook_files = _filter_hook_files_for_target(hook_files, config.target_key) if not hook_files: return _empty @@ -589,6 +644,12 @@ def _integrate_merged_hooks( # Merge hooks into config (additive) hooks = rewritten.get("hooks", {}) event_map = _HOOK_EVENT_MAP.get(config.target_key, {}) + + # Build reverse map: normalised name -> set of source aliases + reverse_map: dict[str, set[str]] = {} + for source_name, norm_name in event_map.items(): + reverse_map.setdefault(norm_name, set()).add(source_name) + for raw_event_name, entries in hooks.items(): if not isinstance(entries, list): continue @@ -609,18 +670,53 @@ def _integrate_merged_hooks( # package before appending fresh ones. Without this, every # `apm install` re-run duplicates the package's hooks # because `.extend()` is unconditional. See microsoft/apm#708. - # Only strip once per event per install run — a package + # Only strip once per event per install run -- a package # with multiple hook files targeting the same event # contributes each file's entries in turn, and stripping # on every iteration would erase earlier files' work. if event_name not in cleared_events: + # Clear from the normalised event json_config["hooks"][event_name] = [ e for e in json_config["hooks"][event_name] if not (isinstance(e, dict) and e.get("_apm_source") == package_name) ] + # Also clear from any alias events that map to + # this normalised name (handles migration from + # corrupted installs with mixed-case event keys). + for alias in reverse_map.get(event_name, set()): + if alias != event_name and alias in json_config["hooks"]: + json_config["hooks"][alias] = [ + e for e in json_config["hooks"][alias] + if not (isinstance(e, dict) and e.get("_apm_source") == package_name) + ] + # Remove the alias key entirely if now empty + if not json_config["hooks"][alias]: + del json_config["hooks"][alias] cleared_events.add(event_name) json_config["hooks"][event_name].extend(entries) + # Deduplicate same-package entries by content. + # Safety net for edge cases where multiple source files + # produce semantically identical entries. + 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}) + deduped.append(entry) + json_config["hooks"][event_name] = deduped + hooks_integrated += 1 # Copy referenced scripts diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 972b83ed5..b37bfda0c 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -17,7 +17,11 @@ import pytest -from apm_cli.integration.hook_integrator import HookIntegrator, HookIntegrationResult +from apm_cli.integration.hook_integrator import ( + HookIntegrator, + HookIntegrationResult, + _filter_hook_files_for_target, +) from apm_cli.models.apm_package import APMPackage, PackageInfo @@ -2599,3 +2603,550 @@ def test_rewrite_forward_slash_still_works(self, temp_project): assert ".github/hooks/scripts/my-pkg/scripts/scan.ps1" in cmd assert len(scripts) == 1 + + +# ─── Issue #1007: Claude settings.json hook emission fixes ─────────────────── + + +class TestIssue1007Fixes: + """Regression tests for the four bug-fixes shipped in issue #1007. + + Fix 1 – Target-aware hook file routing (_filter_hook_files_for_target) + Fix 2 – Variable pattern expansion (${PLUGIN_ROOT} / ${CURSOR_PLUGIN_ROOT}) + Fix 3 – Event name normalisation for Claude (camelCase → PascalCase) + Fix 4a – Alias-aware clearing during reinstall + Fix 4b – Content-based deduplication within a package + """ + + @pytest.fixture + def temp_project(self, tmp_path: Path) -> Path: + """Minimal project root; .claude/ is NOT pre-created (claude require_dir=False).""" + return tmp_path + + @pytest.fixture + def temp_project_with_cursor(self, tmp_path: Path) -> Path: + """Project root with .cursor/ pre-created (cursor require_dir=True).""" + (tmp_path / ".cursor").mkdir() + return tmp_path + + # ------------------------------------------------------------------ + # Helpers + # ------------------------------------------------------------------ + + def _make_pkg( + self, + project: Path, + pkg_name: str, + hook_files: dict, + ) -> PackageInfo: + """Create a package directory with the given hook JSON files. + + Args: + project: Project root path. + pkg_name: Package name used as directory name. + hook_files: Mapping of filename → hook dict to write under hooks/. + """ + pkg_dir = project / "apm_modules" / pkg_name + hooks_dir = pkg_dir / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + for filename, data in hook_files.items(): + (hooks_dir / filename).write_text(json.dumps(data), encoding="utf-8") + return _make_package_info(pkg_dir, pkg_name) + + def _read_claude_settings(self, project: Path) -> dict: + """Return parsed .claude/settings.json (or empty dict if absent).""" + path = project / ".claude" / "settings.json" + if not path.exists(): + return {} + return json.loads(path.read_text(encoding="utf-8")) + + def _read_cursor_hooks(self, project: Path) -> dict: + """Return parsed .cursor/hooks.json (or empty dict if absent).""" + path = project / ".cursor" / "hooks.json" + if not path.exists(): + return {} + return json.loads(path.read_text(encoding="utf-8")) + + # ------------------------------------------------------------------ + # Group A: Target-aware file routing + # ------------------------------------------------------------------ + + def test_filter_copilot_hooks_excluded_from_claude(self, tmp_path: Path) -> None: + """Files with *-copilot-hooks stem must be excluded from the claude target.""" + files = [ + tmp_path / "copilot-hooks.json", + tmp_path / "cursor-hooks.json", + tmp_path / "hooks.json", + ] + for f in files: + f.write_text("{}") + + result = _filter_hook_files_for_target(files, "claude") + + names = {f.name for f in result} + assert names == {"hooks.json"}, ( + "Only the generic hooks.json should reach the claude target; " + f"got {names}" + ) + + def test_filter_cursor_hooks_excluded_from_copilot(self, tmp_path: Path) -> None: + """Files with *-cursor-hooks stem must be excluded from the copilot target.""" + files = [ + tmp_path / "copilot-hooks.json", + tmp_path / "cursor-hooks.json", + tmp_path / "hooks.json", + ] + for f in files: + f.write_text("{}") + + result = _filter_hook_files_for_target(files, "copilot") + + names = {f.name for f in result} + assert "cursor-hooks.json" not in names, "cursor-hooks.json must not reach copilot" + assert "copilot-hooks.json" in names, "copilot-hooks.json must reach copilot" + assert "hooks.json" in names, "Generic hooks.json must reach copilot" + + def test_filter_generic_hooks_universal(self, tmp_path: Path) -> None: + """Generic stems (no *--hooks suffix) pass through for ALL targets.""" + generic_files = [ + tmp_path / "hooks.json", + tmp_path / "telemetry-hooks.json", + ] + for f in generic_files: + f.write_text("{}") + + for target in ("claude", "cursor", "copilot", "codex", "gemini"): + result = _filter_hook_files_for_target(generic_files, target) + assert set(result) == set(generic_files), ( + f"Generic hook files must be universal; target={target!r} got {result}" + ) + + def test_filter_prefixed_stem_routing(self, tmp_path: Path) -> None: + """Stems like azure-skills-cursor-hooks route only to cursor.""" + prefixed = tmp_path / "azure-skills-cursor-hooks.json" + prefixed.write_text("{}") + + assert _filter_hook_files_for_target([prefixed], "cursor") == [prefixed] + assert _filter_hook_files_for_target([prefixed], "claude") == [] + assert _filter_hook_files_for_target([prefixed], "copilot") == [] + assert _filter_hook_files_for_target([prefixed], "codex") == [] + assert _filter_hook_files_for_target([prefixed], "gemini") == [] + + def test_filter_case_insensitive(self, tmp_path: Path) -> None: + """Stem routing must be case-insensitive (Azure-Skills-Cursor-Hooks).""" + mixed = tmp_path / "Azure-Skills-Cursor-Hooks.json" + mixed.write_text("{}") + + assert _filter_hook_files_for_target([mixed], "cursor") == [mixed], ( + "Mixed-case cursor-hooks stem must route to cursor" + ) + assert _filter_hook_files_for_target([mixed], "claude") == [], ( + "Mixed-case cursor-hooks stem must NOT route to claude" + ) + + def test_claude_integration_skips_cursor_hook_files( + self, temp_project: Path + ) -> None: + """End-to-end: .claude/settings.json must not contain entries from cursor-hooks.json.""" + pkg_info = self._make_pkg( + temp_project, + "multi-hooks-pkg", + { + # Claude-specific hooks (PascalCase events, no cursor variable) + "hooks.json": { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo claude-only"} + ] + } + }, + # Cursor-specific hooks — must NOT appear in Claude output + "cursor-hooks.json": { + "hooks": { + "postToolUse": [ + { + "type": "command", + "command": "${CURSOR_PLUGIN_ROOT}/scripts/track.sh", + } + ] + } + }, + }, + ) + + integrator = HookIntegrator() + result = integrator.integrate_package_hooks_claude(pkg_info, temp_project) + + assert result.files_integrated == 1, "Exactly hooks.json should be integrated" + settings = self._read_claude_settings(temp_project) + hooks = settings.get("hooks", {}) + + # Must have the generic hook entry + assert "PostToolUse" in hooks, "PostToolUse from hooks.json must be present" + + # Must NOT have anything from cursor-hooks.json + all_commands = [ + entry.get("command", "") + for entries in hooks.values() + for entry in entries + if isinstance(entry, dict) + ] + assert not any("CURSOR_PLUGIN_ROOT" in cmd for cmd in all_commands), ( + "No ${CURSOR_PLUGIN_ROOT} reference should appear in Claude settings.json" + ) + + # ------------------------------------------------------------------ + # Group B: Variable pattern expansion + # ------------------------------------------------------------------ + + def test_rewrite_plugin_root_variable(self, tmp_path: Path) -> None: + """${PLUGIN_ROOT}/path must be rewritten to the installed script path.""" + pkg_dir = tmp_path / "pkg" + script = pkg_dir / "scripts" / "track.sh" + script.parent.mkdir(parents=True, exist_ok=True) + script.write_text("#!/bin/bash\necho track", encoding="utf-8") + + integrator = HookIntegrator() + cmd, scripts = integrator._rewrite_command_for_target( + "${PLUGIN_ROOT}/scripts/track.sh", + pkg_dir, + "my-pkg", + "claude", + ) + + assert "${PLUGIN_ROOT}" not in cmd, "Variable must be replaced" + assert len(scripts) == 1, "Script copy entry must be produced" + assert "scripts/track.sh" in cmd + + def test_rewrite_cursor_plugin_root_variable(self, tmp_path: Path) -> None: + """${CURSOR_PLUGIN_ROOT}/path must be rewritten to the installed script path.""" + pkg_dir = tmp_path / "pkg" + script = pkg_dir / "scripts" / "track.sh" + script.parent.mkdir(parents=True, exist_ok=True) + script.write_text("#!/bin/bash\necho track", encoding="utf-8") + + integrator = HookIntegrator() + cmd, scripts = integrator._rewrite_command_for_target( + "${CURSOR_PLUGIN_ROOT}/scripts/track.sh", + pkg_dir, + "my-pkg", + "claude", + ) + + assert "${CURSOR_PLUGIN_ROOT}" not in cmd, "Variable must be replaced" + assert len(scripts) == 1, "Script copy entry must be produced" + assert "scripts/track.sh" in cmd + + def test_rewrite_all_variable_forms_equivalent(self, tmp_path: Path) -> None: + """${PLUGIN_ROOT}, ${CURSOR_PLUGIN_ROOT}, ${CLAUDE_PLUGIN_ROOT} all produce the same output.""" + pkg_dir = tmp_path / "pkg" + script = pkg_dir / "x.sh" + pkg_dir.mkdir(parents=True, exist_ok=True) + script.write_text("#!/bin/bash\necho x", encoding="utf-8") + + integrator = HookIntegrator() + results = [] + for var in ("PLUGIN_ROOT", "CURSOR_PLUGIN_ROOT", "CLAUDE_PLUGIN_ROOT"): + cmd, scripts = integrator._rewrite_command_for_target( + f"${{{var}}}/x.sh", + pkg_dir, + "my-pkg", + "claude", + ) + results.append((cmd, len(scripts))) + + cmds = [r[0] for r in results] + script_counts = [r[1] for r in results] + + assert len(set(cmds)) == 1, ( + f"All three variable forms must produce identical commands; got {cmds}" + ) + assert script_counts == [1, 1, 1], "Each form must produce one script copy entry" + + def test_rewrite_partial_variable_no_match(self, tmp_path: Path) -> None: + """${MY_PLUGIN_ROOT} (unknown variable) must pass through unchanged.""" + pkg_dir = tmp_path / "pkg" + pkg_dir.mkdir(parents=True, exist_ok=True) + + integrator = HookIntegrator() + original = "${MY_PLUGIN_ROOT}/scripts/x.sh" + cmd, scripts = integrator._rewrite_command_for_target( + original, + pkg_dir, + "my-pkg", + "claude", + ) + + assert cmd == original, "Unknown variable must not be modified" + assert scripts == [], "No scripts should be scheduled for copy" + + # ------------------------------------------------------------------ + # Group C: Event normalisation for Claude + # ------------------------------------------------------------------ + + def test_claude_normalises_camelcase_events(self, temp_project: Path) -> None: + """postToolUse (camelCase) in hook files must be stored as PostToolUse.""" + pkg_info = self._make_pkg( + temp_project, + "camel-pkg", + { + "hooks.json": { + "hooks": { + "postToolUse": [ + {"type": "command", "command": "echo post"} + ] + } + } + }, + ) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + hooks = self._read_claude_settings(temp_project).get("hooks", {}) + assert "PostToolUse" in hooks, "Normalised PascalCase key must be present" + assert "postToolUse" not in hooks, "Original camelCase key must not remain" + + def test_claude_preserves_pascal_case_events(self, temp_project: Path) -> None: + """PostToolUse (already PascalCase) must be stored unchanged.""" + pkg_info = self._make_pkg( + temp_project, + "pascal-pkg", + { + "hooks.json": { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo post"} + ] + } + } + }, + ) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + hooks = self._read_claude_settings(temp_project).get("hooks", {}) + assert "PostToolUse" in hooks, "PascalCase key must be preserved" + assert "postToolUse" not in hooks, "No duplicate camelCase key should appear" + + def test_cursor_no_normalisation( + self, temp_project_with_cursor: Path + ) -> None: + """Cursor target has no event-name mapping; PostToolUse passes through as-is.""" + from apm_cli.integration.targets import KNOWN_TARGETS + + project = temp_project_with_cursor + pkg_info = self._make_pkg( + project, + "cursor-no-norm-pkg", + { + "cursor-hooks.json": { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo cursor-post"} + ] + } + } + }, + ) + + target = KNOWN_TARGETS["cursor"] + HookIntegrator().integrate_hooks_for_target(target, pkg_info, project) + + hooks = self._read_cursor_hooks(project).get("hooks", {}) + assert "PostToolUse" in hooks, "PostToolUse must survive cursor integration unchanged" + + # ------------------------------------------------------------------ + # Group D: Deduplication + # ------------------------------------------------------------------ + + def test_single_install_no_duplicates(self, temp_project: Path) -> None: + """End-to-end reproducer for issue #1007. + + A package with copilot-hooks.json, cursor-hooks.json, and hooks.json + must produce exactly ONE PostToolUse entry in .claude/settings.json + with no residual postToolUse key. + """ + pkg_info = self._make_pkg( + temp_project, + "multi-format-pkg", + { + # Generic (Claude) hooks — should be integrated + "hooks.json": { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo generic-post"} + ] + } + }, + # Copilot-specific — must be filtered out for Claude + "copilot-hooks.json": { + "hooks": { + "postToolUse": [ + {"type": "command", "command": "echo copilot-post"} + ] + } + }, + # Cursor-specific — must be filtered out for Claude + "cursor-hooks.json": { + "hooks": { + "postToolUse": [ + {"type": "command", "command": "echo cursor-post"} + ] + } + }, + }, + ) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + hooks = self._read_claude_settings(temp_project).get("hooks", {}) + assert "postToolUse" not in hooks, ( + "Residual camelCase key must not exist after Claude integration" + ) + assert "PostToolUse" in hooks, "PostToolUse key must be present" + assert len(hooks["PostToolUse"]) == 1, ( + f"Exactly 1 PostToolUse entry expected; got {len(hooks['PostToolUse'])}" + ) + + def test_content_dedup_same_package(self, temp_project: Path) -> None: + """Two hook files in the same package producing identical entries yield only 1.""" + identical_entry = { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo dedup-test"} + ] + } + } + pkg_info = self._make_pkg( + temp_project, + "dedup-pkg", + { + "a-hooks.json": identical_entry, + "b-hooks.json": identical_entry, + }, + ) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + hooks = self._read_claude_settings(temp_project).get("hooks", {}) + entries = hooks.get("PostToolUse", []) + assert len(entries) == 1, ( + f"Identical entries from same package must be deduplicated; got {len(entries)}" + ) + + def test_content_dedup_preserves_cross_package(self, temp_project: Path) -> None: + """Identical hook entries from DIFFERENT packages must both be kept.""" + identical_entry = { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo shared-command"} + ] + } + } + pkg_a = self._make_pkg(temp_project, "pkg-alpha", {"hooks.json": identical_entry}) + pkg_b = self._make_pkg(temp_project, "pkg-beta", {"hooks.json": identical_entry}) + + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(pkg_a, temp_project) + integrator.integrate_package_hooks_claude(pkg_b, temp_project) + + hooks = self._read_claude_settings(temp_project).get("hooks", {}) + entries = hooks.get("PostToolUse", []) + sources = {e.get("_apm_source") for e in entries if isinstance(e, dict)} + assert "pkg-alpha" in sources, "pkg-alpha entry must be retained" + assert "pkg-beta" in sources, "pkg-beta entry must be retained" + assert len(entries) == 2, ( + f"Cross-package identical entries must both be present; got {len(entries)}" + ) + + def test_reinstall_clears_aliased_events(self, temp_project: Path) -> None: + """Re-integration removes stale postToolUse (camelCase) aliases. + + Simulates a corrupted pre-fix state where the same package has entries + under both PostToolUse and postToolUse, then verifies that after a + fresh install only the PascalCase key survives. + """ + # Simulate corrupted pre-fix state + claude_dir = temp_project / ".claude" + claude_dir.mkdir(parents=True, exist_ok=True) + corrupted = { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo stale", "_apm_source": "alias-pkg"} + ], + "postToolUse": [ + {"type": "command", "command": "echo stale-alias", "_apm_source": "alias-pkg"} + ], + } + } + (claude_dir / "settings.json").write_text( + json.dumps(corrupted, indent=2), encoding="utf-8" + ) + + pkg_info = self._make_pkg( + temp_project, + "alias-pkg", + { + "hooks.json": { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo fresh"} + ] + } + } + }, + ) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + hooks = self._read_claude_settings(temp_project).get("hooks", {}) + assert "postToolUse" not in hooks, ( + "Stale camelCase alias key must be removed after reinstall" + ) + assert "PostToolUse" in hooks, "PascalCase key must remain after reinstall" + # Ensure the stale alias entry is gone — only the fresh entry survives + commands = [ + e.get("command", "") for e in hooks["PostToolUse"] if isinstance(e, dict) + ] + assert all("stale" not in cmd for cmd in commands), ( + f"Stale alias entries must be cleared; found: {commands}" + ) + + # ------------------------------------------------------------------ + # Group E: Regression + # ------------------------------------------------------------------ + + def test_reinstall_still_idempotent_with_routing(self, temp_project: Path) -> None: + """Running Claude integration 3 times with routing active must not grow entries.""" + pkg_info = self._make_pkg( + temp_project, + "idempotent-pkg", + { + # Only hooks.json passes the claude filter + "hooks.json": { + "hooks": { + "PostToolUse": [ + {"type": "command", "command": "echo idempotent"} + ] + } + }, + # This file is filtered out for claude — must not affect count + "cursor-hooks.json": { + "hooks": { + "postToolUse": [ + {"type": "command", "command": "echo cursor-only"} + ] + } + }, + }, + ) + + integrator = HookIntegrator() + for _ in range(3): + integrator.integrate_package_hooks_claude(pkg_info, temp_project) + + hooks = self._read_claude_settings(temp_project).get("hooks", {}) + entries = hooks.get("PostToolUse", []) + assert len(entries) == 1, ( + f"Entry count must remain constant across re-installs; got {len(entries)}" + ) From 714531ae289963e554a85b147725355a3907b53a Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Wed, 29 Apr 2026 23:01:37 +0100 Subject: [PATCH 2/2] fix: address panel findings -- add hook docs to apm-usage skill, fix 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> --- docs/src/content/docs/guides/plugins.md | 15 ++++++--- .../skills/apm-usage/package-authoring.md | 32 +++++++++++++++++++ src/apm_cli/integration/hook_integrator.py | 2 +- .../unit/integration/test_hook_integrator.py | 26 +++++++-------- 4 files changed, 57 insertions(+), 18 deletions(-) diff --git a/docs/src/content/docs/guides/plugins.md b/docs/src/content/docs/guides/plugins.md index 35f83c6ee..1bf54213a 100644 --- a/docs/src/content/docs/guides/plugins.md +++ b/docs/src/content/docs/guides/plugins.md @@ -221,11 +221,18 @@ each tool receives only its own hooks: | `*-claude-hooks.json` | Claude Code only | | `*-codex-hooks.json` | Codex CLI only | | `*-gemini-hooks.json` | Gemini CLI only | -| `hooks.json` or other non-target `*-hooks.json` names | All targets | +| Any other name (e.g. `hooks.json`, `telemetry-hooks.json`) | All targets | -For example, a package with `copilot-hooks.json`, `cursor-hooks.json`, and -`hooks.json` routes the named files to their target while `hooks.json` deploys -everywhere. +Example directory tree for a multi-target hook package: + +``` +my-hooks-pkg/ + hooks/ + hooks.json # deployed to all targets + copilot-hooks.json # Copilot only + cursor-hooks.json # Cursor only + claude-hooks.json # Claude Code only +``` #### MCP Server Definitions diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 3e50bd4b6..d20b19a3e 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -58,6 +58,38 @@ my-package/ resource2.md ``` +## Hook files + +Packages can ship hooks (pre/post tool-use scripts) by placing JSON +files under `hooks/` or `.apm/hooks/`. When a package targets multiple +tools, use target-specific filenames so each tool receives only its own +hooks: + +| Filename pattern | Deployed to | +|---|---| +| `*-copilot-hooks.json` | GitHub Copilot only | +| `*-cursor-hooks.json` | Cursor only | +| `*-claude-hooks.json` | Claude Code only | +| `*-codex-hooks.json` | Codex CLI only | +| `*-gemini-hooks.json` | Gemini CLI only | +| Any other name (e.g. `hooks.json`, `telemetry-hooks.json`) | All targets | + +Example directory tree for a multi-target hook package: + +``` +my-hooks-pkg/ + hooks/ + hooks.json # deployed to all targets + copilot-hooks.json # Copilot only + cursor-hooks.json # Cursor only + claude-hooks.json # Claude Code only +``` + +APM automatically normalises event names per target (e.g. `postToolUse` +becomes `PostToolUse` in Claude) and rewrites path variables +(`${PLUGIN_ROOT}`, `${CURSOR_PLUGIN_ROOT}`, `${CLAUDE_PLUGIN_ROOT}`) to +the correct target-specific form. + ## Manifest fields: `target:` validation contract The `target:` field in `apm.yml` controls which output runtimes the package diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 14ad13d38..8621c08c5 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -611,7 +611,7 @@ def _integrate_merged_hooks( target_paths: List[Path] = [] # Events whose prior-owned entries have already been cleared on # this install run. Packages can contribute to the same event - # from multiple hook files — we must only strip once so earlier + # from multiple hook files -- we must only strip once so earlier # files' fresh entries aren't wiped by later iterations. cleared_events: set = set() diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index b37bfda0c..e995ad20b 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -2605,17 +2605,17 @@ def test_rewrite_forward_slash_still_works(self, temp_project): assert len(scripts) == 1 -# ─── Issue #1007: Claude settings.json hook emission fixes ─────────────────── +# === Issue #1007: Claude settings.json hook emission fixes ==================== class TestIssue1007Fixes: """Regression tests for the four bug-fixes shipped in issue #1007. - Fix 1 – Target-aware hook file routing (_filter_hook_files_for_target) - Fix 2 – Variable pattern expansion (${PLUGIN_ROOT} / ${CURSOR_PLUGIN_ROOT}) - Fix 3 – Event name normalisation for Claude (camelCase → PascalCase) - Fix 4a – Alias-aware clearing during reinstall - Fix 4b – Content-based deduplication within a package + Fix 1 -- Target-aware hook file routing (_filter_hook_files_for_target) + Fix 2 -- Variable pattern expansion (${PLUGIN_ROOT} / ${CURSOR_PLUGIN_ROOT}) + Fix 3 -- Event name normalisation for Claude (camelCase -> PascalCase) + Fix 4a -- Alias-aware clearing during reinstall + Fix 4b -- Content-based deduplication within a package """ @pytest.fixture @@ -2644,7 +2644,7 @@ def _make_pkg( Args: project: Project root path. pkg_name: Package name used as directory name. - hook_files: Mapping of filename → hook dict to write under hooks/. + hook_files: Mapping of filename -> hook dict to write under hooks/. """ pkg_dir = project / "apm_modules" / pkg_name hooks_dir = pkg_dir / "hooks" @@ -2760,7 +2760,7 @@ def test_claude_integration_skips_cursor_hook_files( ] } }, - # Cursor-specific hooks — must NOT appear in Claude output + # Cursor-specific hooks -- must NOT appear in Claude output "cursor-hooks.json": { "hooks": { "postToolUse": [ @@ -2970,7 +2970,7 @@ def test_single_install_no_duplicates(self, temp_project: Path) -> None: temp_project, "multi-format-pkg", { - # Generic (Claude) hooks — should be integrated + # Generic (Claude) hooks -- should be integrated "hooks.json": { "hooks": { "PostToolUse": [ @@ -2978,7 +2978,7 @@ def test_single_install_no_duplicates(self, temp_project: Path) -> None: ] } }, - # Copilot-specific — must be filtered out for Claude + # Copilot-specific -- must be filtered out for Claude "copilot-hooks.json": { "hooks": { "postToolUse": [ @@ -2986,7 +2986,7 @@ def test_single_install_no_duplicates(self, temp_project: Path) -> None: ] } }, - # Cursor-specific — must be filtered out for Claude + # Cursor-specific -- must be filtered out for Claude "cursor-hooks.json": { "hooks": { "postToolUse": [ @@ -3104,7 +3104,7 @@ def test_reinstall_clears_aliased_events(self, temp_project: Path) -> None: "Stale camelCase alias key must be removed after reinstall" ) assert "PostToolUse" in hooks, "PascalCase key must remain after reinstall" - # Ensure the stale alias entry is gone — only the fresh entry survives + # Ensure the stale alias entry is gone -- only the fresh entry survives commands = [ e.get("command", "") for e in hooks["PostToolUse"] if isinstance(e, dict) ] @@ -3130,7 +3130,7 @@ def test_reinstall_still_idempotent_with_routing(self, temp_project: Path) -> No ] } }, - # This file is filtered out for claude — must not affect count + # This file is filtered out for claude -- must not affect count "cursor-hooks.json": { "hooks": { "postToolUse": [