From d7885f9578a2352219b7c10de4beb4945977d54e Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 6 Apr 2026 23:22:42 +0200 Subject: [PATCH] fix: scope-aware hook deployment, dynamic sync prefixes, auto_create guard Rework of PR #566 on top of post-#562 architecture: - integrate_package_hooks: accept target=, use target.root_dir instead of hardcoded .github for Copilot hook deployment path - _integrate_merged_hooks: use target.root_dir instead of f'.{config.target_key}' for Claude/Cursor/Codex JSON merge path - _rewrite_command_for_target / _rewrite_hooks_data: accept root_dir= override so script paths match the scope-resolved directory - sync_integration: derive hook prefixes dynamically from KNOWN_TARGETS instead of hardcoded .github/.claude/.cursor/.codex paths; accept targets= parameter - install.py: gate directory creation on auto_create=True to prevent creating dirs for targets that don't want them (e.g. Claude, Cursor at project scope) - 6 new tests covering scope-resolved deploy, script rewrite, sync cleanup, and auto_create guard Fixes #565 Addresses #576 (P1-1 sync, P1-4 auto_create) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 3 + src/apm_cli/commands/install.py | 2 + src/apm_cli/integration/hook_integrator.py | 159 ++++++++++-------- .../unit/integration/test_hook_integrator.py | 136 +++++++++++++++ 4 files changed, 232 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc9d4516f..14c02ed9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `apm install -g` now deploys hooks to the scope-resolved target directory (e.g. `~/.copilot/hooks/`) instead of hardcoding `.github/hooks/` (#565) +- Hook sync/cleanup now derives prefixes dynamically from `KNOWN_TARGETS` instead of hardcoded paths (#565) +- `auto_create=False` targets no longer get directories unconditionally created during install (#576) - `apm deps update -g` now correctly passes scope, preventing user-scope updates from silently using project-scope paths (#562) ## [0.8.10] - 2026-04-03 diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 9e8b0119b..09f57c6eb 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1431,6 +1431,8 @@ def _collect_descendants(node, visited=None): ) for _t in _targets: + if not _t.auto_create: + continue _root = _t.root_dir _target_dir = project_root / _root if not _target_dir.exists(): diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index c174eddf2..658e63886 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -192,6 +192,7 @@ def _rewrite_command_for_target( package_name: str, target: str, hook_file_dir: Optional[Path] = None, + root_dir: Optional[str] = None, ) -> Tuple[str, List[Tuple[Path, str]]]: """Rewrite a hook command to use installed script paths. @@ -205,6 +206,7 @@ def _rewrite_command_for_target( package_name: Name used for the scripts subdirectory target: "vscode" or "claude" hook_file_dir: Directory containing the hook JSON file (for ./path resolution) + root_dir: Override root directory (e.g. ".copilot" for user scope) Returns: Tuple of (rewritten_command, list of (source_file, relative_target_path)) @@ -213,13 +215,17 @@ def _rewrite_command_for_target( new_command = command if target == "vscode": - scripts_base = f".github/hooks/scripts/{package_name}" + base_root = root_dir or ".github" + scripts_base = f"{base_root}/hooks/scripts/{package_name}" elif target == "cursor": - scripts_base = f".cursor/hooks/{package_name}" + base_root = root_dir or ".cursor" + scripts_base = f"{base_root}/hooks/{package_name}" elif target == "codex": - scripts_base = f".codex/hooks/{package_name}" + base_root = root_dir or ".codex" + scripts_base = f"{base_root}/hooks/{package_name}" else: - scripts_base = f".claude/hooks/{package_name}" + base_root = root_dir or ".claude" + scripts_base = f"{base_root}/hooks/{package_name}" # Handle ${CLAUDE_PLUGIN_ROOT} references (always relative to package root) plugin_root_pattern = r'\$\{CLAUDE_PLUGIN_ROOT\}(/[^\s]+)' @@ -263,6 +269,7 @@ def _rewrite_hooks_data( package_name: str, target: str, hook_file_dir: Optional[Path] = None, + root_dir: Optional[str] = None, ) -> Tuple[Dict, List[Tuple[Path, str]]]: """Rewrite all command paths in a hooks JSON structure. @@ -274,6 +281,7 @@ def _rewrite_hooks_data( package_name: Name for scripts subdirectory target: "vscode" or "claude" hook_file_dir: Directory containing the hook JSON file (for ./path resolution) + root_dir: Override root directory (e.g. ".copilot" for user scope) Returns: Tuple of (rewritten_data_copy, list of (source_file, target_rel_path)) @@ -296,6 +304,7 @@ def _rewrite_hooks_data( new_cmd, scripts = self._rewrite_command_for_target( matcher[key], package_path, package_name, target, hook_file_dir=hook_file_dir, + root_dir=root_dir, ) if scripts: _log.debug( @@ -315,6 +324,7 @@ def _rewrite_hooks_data( new_cmd, scripts = self._rewrite_command_for_target( hook[key], package_path, package_name, target, hook_file_dir=hook_file_dir, + root_dir=root_dir, ) if scripts: _log.debug( @@ -348,8 +358,9 @@ def _get_package_name(self, package_info) -> str: def integrate_package_hooks(self, package_info, project_root: Path, force: bool = False, managed_files: set = None, - diagnostics=None) -> HookIntegrationResult: - """Integrate hooks from a package into .github/hooks/ (VSCode target). + diagnostics=None, + target=None) -> HookIntegrationResult: + """Integrate hooks from a package into hooks dir (Copilot target). Deploys hook JSON files with clean filenames and copies referenced script files. Skips user-authored files unless force=True. @@ -359,6 +370,7 @@ def integrate_package_hooks(self, package_info, project_root: Path, project_root: Root directory of the project force: If True, overwrite user-authored files on collision managed_files: Set of relative paths known to be APM-managed + target: Optional TargetProfile for scope-resolved root_dir Returns: HookIntegrationResult: Results of the integration operation @@ -371,7 +383,8 @@ def integrate_package_hooks(self, package_info, project_root: Path, files_skipped=0, target_paths=[], ) - hooks_dir = project_root / ".github" / "hooks" + root_dir = target.root_dir if target else ".github" + hooks_dir = project_root / root_dir / "hooks" hooks_dir.mkdir(parents=True, exist_ok=True) package_name = self._get_package_name(package_info) @@ -388,6 +401,7 @@ def integrate_package_hooks(self, package_info, project_root: Path, rewritten, scripts = self._rewrite_hooks_data( data, package_info.install_path, package_name, "vscode", hook_file_dir=hook_file.parent, + root_dir=root_dir, ) # Generate target filename (clean, no -apm suffix) @@ -436,6 +450,7 @@ def _integrate_merged_hooks( force: bool = False, managed_files: set = None, diagnostics=None, + target=None, ) -> HookIntegrationResult: """Integrate hooks by merging into a target-specific JSON config. @@ -448,7 +463,8 @@ def _integrate_merged_hooks( files_skipped=0, target_paths=[], ) - target_dir = project_root / f".{config.target_key}" + root_dir = target.root_dir if target else f".{config.target_key}" + target_dir = project_root / root_dir # Opt-in check: some targets only deploy when their dir exists if config.require_dir and not target_dir.exists(): @@ -486,6 +502,7 @@ def _integrate_merged_hooks( data, package_info.install_path, package_name, config.target_key, hook_file_dir=hook_file.parent, + root_dir=root_dir, ) # Merge hooks into config (additive) @@ -603,6 +620,7 @@ def integrate_hooks_for_target( package_info, project_root, force=force, managed_files=managed_files, diagnostics=diagnostics, + target=target, ) config = _MERGE_HOOK_TARGETS.get(target.name) @@ -611,6 +629,7 @@ def integrate_hooks_for_target( config, package_info, project_root, force=force, managed_files=managed_files, diagnostics=diagnostics, + target=target, ) return HookIntegrationResult( @@ -619,7 +638,8 @@ def integrate_hooks_for_target( ) def sync_integration(self, apm_package, project_root: Path, - managed_files: set = None) -> Dict: + managed_files: set = None, + targets=None) -> Dict: """Remove APM-managed hook files. Uses *managed_files* (relative paths) to surgically remove only @@ -628,35 +648,41 @@ def sync_integration(self, apm_package, project_root: Path, **Never** calls ``shutil.rmtree``. - Also cleans APM entries from ``.claude/settings.json`` and - ``.cursor/hooks.json`` via the ``_apm_source`` marker. + Also cleans APM entries from merged-hook JSON files via the + ``_apm_source`` marker. """ + from .targets import KNOWN_TARGETS + stats: Dict[str, int] = {'files_removed': 0, 'errors': 0} + # Derive hook prefixes dynamically from targets + source = targets if targets is not None else list(KNOWN_TARGETS.values()) + hook_prefixes = [] + for t in source: + if t.supports("hooks"): + sm = t.primitives["hooks"] + effective_root = sm.deploy_root or t.root_dir + hook_prefixes.append(f"{effective_root}/hooks/") + hook_prefix_tuple = tuple(hook_prefixes) + if managed_files is not None: - # Manifest-based removal -- only remove tracked files + # Manifest-based removal -- only remove tracked files deleted: list = [] for rel_path in managed_files: - # Normalize path separators for cross-platform compatibility normalized = rel_path.replace("\\", "/") - # Only handle hook-related paths - is_hook = ( - normalized.startswith(".github/hooks/") - or normalized.startswith(".claude/hooks/") - or normalized.startswith(".cursor/hooks/") - or normalized.startswith(".codex/hooks/") - ) - if not is_hook or ".." in rel_path: + if not normalized.startswith(hook_prefix_tuple): + continue + if ".." in rel_path: continue - target = project_root / rel_path - if target.exists() and target.is_file(): + target_file = project_root / rel_path + if target_file.exists() and target_file.is_file(): try: - target.unlink() + target_file.unlink() stats['files_removed'] += 1 - deleted.append(target) + deleted.append(target_file) except Exception: stats['errors'] += 1 - # Batch parent cleanup -- single bottom-up pass + # Batch parent cleanup -- single bottom-up pass self.cleanup_empty_parents(deleted, stop_at=project_root) else: # Legacy fallback -- glob for old -apm suffix files @@ -669,48 +695,45 @@ def sync_integration(self, apm_package, project_root: Path, except Exception: stats['errors'] += 1 - # Clean APM entries from .claude/settings.json (safe -- uses _apm_source marker) - settings_path = project_root / ".claude" / "settings.json" - if settings_path.exists(): - try: - with open(settings_path, 'r', encoding='utf-8') as f: - settings = json.load(f) - - if "hooks" in settings: - modified = False - for event_name in list(settings["hooks"].keys()): - matchers = settings["hooks"][event_name] - if isinstance(matchers, list): - filtered = [ - m for m in matchers - if not (isinstance(m, dict) and "_apm_source" in m) - ] - if len(filtered) != len(matchers): - modified = True - settings["hooks"][event_name] = filtered - if not filtered: - del settings["hooks"][event_name] - - if not settings["hooks"]: - del settings["hooks"] - - if modified: - with open(settings_path, 'w', encoding='utf-8') as f: - json.dump(settings, f, indent=2) - f.write('\n') - stats['files_removed'] += 1 - except (json.JSONDecodeError, OSError): - stats['errors'] += 1 - - # Clean APM entries from .cursor/hooks.json (safe -- uses _apm_source marker) - self._clean_apm_entries_from_json( - project_root / ".cursor" / "hooks.json", stats, - ) - - # Clean APM entries from .codex/hooks.json (safe -- uses _apm_source marker) - self._clean_apm_entries_from_json( - project_root / ".codex" / "hooks.json", stats, - ) + # Clean APM entries from merged-hook JSON configs (uses _apm_source marker) + for t in source: + config = _MERGE_HOOK_TARGETS.get(t.name) + if config is not None: + json_path = project_root / t.root_dir / config.config_filename + if t.name == "claude": + # Claude uses settings.json with special structure + if json_path.exists(): + try: + with open(json_path, 'r', encoding='utf-8') as f: + settings = json.load(f) + + if "hooks" in settings: + modified = False + for event_name in list(settings["hooks"].keys()): + matchers = settings["hooks"][event_name] + if isinstance(matchers, list): + filtered = [ + m for m in matchers + if not (isinstance(m, dict) and "_apm_source" in m) + ] + if len(filtered) != len(matchers): + modified = True + settings["hooks"][event_name] = filtered + if not filtered: + del settings["hooks"][event_name] + + if not settings["hooks"]: + del settings["hooks"] + + if modified: + with open(json_path, 'w', encoding='utf-8') as f: + json.dump(settings, f, indent=2) + f.write('\n') + stats['files_removed'] += 1 + except (json.JSONDecodeError, OSError): + stats['errors'] += 1 + else: + self._clean_apm_entries_from_json(json_path, stats) return stats diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 97dc1b439..c6dced6c7 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -1829,3 +1829,139 @@ def test_codex_hooks_not_deployed_without_codex_dir(self): result = integrator.integrate_package_hooks_codex(pi, self.root) assert result.files_integrated == 0 + + +# ─── Scope-resolved target tests (PR #566 rework) ──────────────────────────── + + +class TestScopeResolvedHookDeployment: + """Tests for scope-aware hook deployment using target.root_dir.""" + + def setup_method(self): + self.tmpdir = tempfile.mkdtemp() + self.root = Path(self.tmpdir) + # Create package with hooks + self.pkg_dir = self.root / "apm_modules" / "scope-pkg" + hooks_dir = self.pkg_dir / ".apm" / "hooks" + hooks_dir.mkdir(parents=True) + hooks_dir.joinpath("hooks.json").write_text(json.dumps({ + "hooks": { + "SessionStart": [{"type": "command", "command": "echo hello"}] + } + }), encoding="utf-8") + + def teardown_method(self): + shutil.rmtree(self.tmpdir, ignore_errors=True) + + def _make_target(self, name, root_dir, primitives=None): + """Create a minimal mock TargetProfile.""" + from unittest.mock import MagicMock + t = MagicMock() + t.name = name + t.root_dir = root_dir + t.supports = lambda prim: prim in (primitives or {"hooks"}) + if primitives is None: + primitives = {"hooks"} + t.primitives = {} + for p in primitives: + mapping = MagicMock() + mapping.deploy_root = None + t.primitives[p] = mapping + return t + + def test_copilot_hooks_deploy_to_scope_resolved_dir(self): + """Copilot hooks at user scope deploy to .copilot/hooks/ not .github/hooks/.""" + copilot_target = self._make_target("copilot", ".copilot") + pi = _make_package_info(self.pkg_dir, "scope-pkg") + integrator = HookIntegrator() + + result = integrator.integrate_package_hooks( + pi, self.root, target=copilot_target, + ) + + assert result.files_integrated > 0 + # Hook file should be under .copilot/hooks/, not .github/hooks/ + hooks_dir = self.root / ".copilot" / "hooks" + assert hooks_dir.exists() + assert not (self.root / ".github" / "hooks").exists() + + def test_copilot_hooks_default_to_github(self): + """Without target, hooks deploy to .github/hooks/ (backward compat).""" + pi = _make_package_info(self.pkg_dir, "scope-pkg") + integrator = HookIntegrator() + + result = integrator.integrate_package_hooks(pi, self.root) + + assert result.files_integrated > 0 + assert (self.root / ".github" / "hooks").exists() + + def test_merged_hooks_use_target_root_dir(self): + """Claude hooks at user scope use target.root_dir for JSON path.""" + claude_target = self._make_target("claude", ".claude") + (self.root / ".claude").mkdir() + pi = _make_package_info(self.pkg_dir, "scope-pkg") + integrator = HookIntegrator() + + result = integrator.integrate_hooks_for_target( + claude_target, pi, self.root, + ) + + assert result.files_integrated > 0 + assert (self.root / ".claude" / "settings.json").exists() + + def test_script_paths_rewritten_with_scope_root(self): + """Script paths in hook commands use the scope-resolved root_dir.""" + # Create a hook with a script reference + hooks_dir = self.pkg_dir / ".apm" / "hooks" + script = hooks_dir / "run.sh" + script.write_text("#!/bin/bash\necho test", encoding="utf-8") + hooks_dir.joinpath("hooks.json").write_text(json.dumps({ + "hooks": { + "SessionStart": [{"type": "command", "command": "./run.sh"}] + } + }), encoding="utf-8") + + copilot_target = self._make_target("copilot", ".copilot") + pi = _make_package_info(self.pkg_dir, "scope-pkg") + integrator = HookIntegrator() + + result = integrator.integrate_package_hooks( + pi, self.root, target=copilot_target, + ) + + # Script should be copied to .copilot/hooks/scripts/scope-pkg/ + scripts_dir = self.root / ".copilot" / "hooks" / "scripts" / "scope-pkg" + assert scripts_dir.exists() + assert (scripts_dir / "run.sh").exists() + + def test_sync_with_copilot_scope_prefix(self): + """sync_integration removes .copilot/hooks/ files when target is present.""" + # Deploy first + copilot_target = self._make_target("copilot", ".copilot") + pi = _make_package_info(self.pkg_dir, "scope-pkg") + integrator = HookIntegrator() + result = integrator.integrate_package_hooks( + pi, self.root, target=copilot_target, + ) + + # Collect deployed paths + managed = set() + for p in result.target_paths: + try: + managed.add(str(p.relative_to(self.root)).replace("\\", "/")) + except ValueError: + pass + + # Sync should clean them up + stats = integrator.sync_integration( + None, self.root, managed_files=managed, targets=[copilot_target], + ) + assert stats['files_removed'] > 0 + + def test_auto_create_guard(self): + """Targets with auto_create=False should not get directories created.""" + from apm_cli.integration.targets import KNOWN_TARGETS + # All targets except copilot have auto_create=False + for name, profile in KNOWN_TARGETS.items(): + if not profile.auto_create: + assert name != "copilot", "copilot should have auto_create=True"