diff --git a/CHANGELOG.md b/CHANGELOG.md index a5cc53e42..baffe8c77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - MCP server installation now respects the `targets:` whitelist exactly like `apm install`: drop a non-listed runtime even when its `.cursor/`, `.codex/`, or other on-disk signal exists. Previously the MCP install path called `active_targets()` reading the singular `target:` key only, so projects whitelisting `targets: [copilot]` could still receive `~/.codex/config.toml` and `.cursor/mcp.json` writes from foreign signals. The fix audits both paths: (a) the call site at `local_bundle_handler.py` now forwards the canonical plural list; (b) the gate now delegates to the same `resolve_targets` resolver that backs `apm install` skills, so a malformed `targets:` field (conflicting `target:` + `targets:`, `targets: []`, or unknown target name) fails closed with the same `[x]` red error voice + remediation block. The same delegation closes a related asymmetry: a greenfield project (no `targets:`, no `--target` flag, no detected signals) used to silently fall back to `[copilot]` for MCP-only invocations, while `apm install` raised `NoHarnessError` on the same input -- both surfaces now error consistently. Drop lines now use the `[i] Skipped MCP config for X (active targets: Y)` format mirroring the canonical `Targets: X (source: Y)` provenance line. The `-g`/`--global` carve-out is unchanged: `apm install -g --mcp NAME` writes to user-scope (`~/.config/...`, `~/.codex/`, etc.) bypassing the project-scope gate by design. (#1335) - Gemini CLI: `apm install -g --mcp NAME` now correctly writes to `~/.gemini/settings.json` (user scope) and `apm install` from outside the target project writes to `/.gemini/settings.json` instead of `cwd`. Previously `--global` had no effect on Gemini and project-scope writes silently landed in the wrong directory. The matching opt-in gate and cleanup paths in `MCPIntegrator` are aligned in the same change. (#1299) +- Root `.apm/hooks/*.json` installs now use a stable local hook source id and heal stale same-content merged hook entries left behind by older checkout-derived `_apm_source` values, keeping Claude/Codex/Cursor/Gemini/Windsurf hook configs idempotent. (#1329) - `apm install --target claude` now preserves self-defined stdio MCP `env` values from `apm.yml` and writes non-string values such as `PORT: 3000` and `DEBUG: false` as MCP-compatible strings. (#1222) - Non-skill integrators (agent, instruction, prompt, command, hook script-copy) silently adopt byte-identical pre-existing files so a degraded `deployed_files=[]` lockfile no longer permanently blocks installs gated by `required-packages-deployed`. (#1313) - `apm audit` drift check now returns skip-with-info (`passed=True`) when the install cache is cold, instead of failing the audit; bare `apm audit` surfaces the skip reason on stderr so CI pipelines that have not yet run `apm install` are not incorrectly red-marked. (#1289) diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 70b84cd6c..9682754bf 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -51,8 +51,14 @@ from pathlib import Path from typing import Dict, List, Optional, Tuple # noqa: F401, UP035 +import yaml + from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult -from apm_cli.utils.path_security import ensure_path_within +from apm_cli.utils.path_security import ( + PathTraversalError, + ensure_path_within, + validate_path_segments, +) from apm_cli.utils.paths import portable_relpath _log = logging.getLogger(__name__) @@ -506,17 +512,271 @@ def _rewrite_hooks_data( return rewritten, unique_scripts - def _get_package_name(self, package_info) -> str: + @staticmethod + def _is_root_local_package(package_info, project_root: Path | None) -> bool: + """Return True when *package_info* represents the project's own .apm content.""" + if project_root is None: + return False + try: + return Path(package_info.install_path).resolve() == Path(project_root).resolve() + except (OSError, RuntimeError): + return False + + @staticmethod + def _safe_source_name(value: str | None, fallback: str = "_local") -> str: + """Return a stable source marker that is also safe for hook script paths.""" + if not isinstance(value, str) or not value: + return fallback + safe = re.sub(r"[^A-Za-z0-9._-]+", "-", value.strip()).strip(".-_") + if not safe or safe in {".", ".."}: + return fallback + return safe + + def _get_root_local_package_name(self, package_info, project_root: Path) -> str: + """Get the stable source marker for root .apm content.""" + apm_yml = Path(project_root) / "apm.yml" + if apm_yml.exists(): + try: + from apm_cli.utils.yaml_io import load_yaml + + data = load_yaml(apm_yml) + if isinstance(data, dict): + manifest_name = self._safe_source_name(data.get("name")) + if manifest_name != "_local": + return manifest_name + except (OSError, ValueError, yaml.YAMLError): + pass + + package = getattr(package_info, "package", None) + package_name = self._safe_source_name(getattr(package, "name", None)) + if package_name != "_local": + return package_name + return "_local" + + def _get_package_name(self, package_info, project_root: Path | None = None) -> str: """Get a short package name for use in file/directory naming. Args: package_info: PackageInfo object Returns: - str: Package name derived from install path + str: Package name used as hook source marker and script namespace """ + if self._is_root_local_package(package_info, project_root): + return self._get_root_local_package_name(package_info, Path(project_root)) return package_info.install_path.name + def _get_hook_source_marker( + self, + package_info, + project_root: Path, + package_name: str, + ) -> str: + """Get the marker stored in merged hook JSON for ownership cleanup.""" + if self._is_root_local_package(package_info, project_root): + if package_name == "_local": + return "_local" + return f"_local/{package_name}" + return package_name + + @staticmethod + def _hook_entry_content_key(entry: dict) -> str: + """Build a stable comparison key excluding APM ownership metadata.""" + comparable = {k: v for k, v in sorted(entry.items()) if k != "_apm_source"} + return json.dumps(comparable, sort_keys=True, separators=(",", ":")) + + @staticmethod + def _dependency_hook_sources(project_root: Path) -> set[str]: + """Return source markers that correspond to installed dependency dirs.""" + apm_modules = project_root / "apm_modules" + if not apm_modules.is_dir(): + return set() + + lockfile_paths, lockfile_readable = HookIntegrator._lockfile_dependency_paths(project_root) + if lockfile_readable: + sources: set[str] = set() + for rel_path in lockfile_paths: + package_path = HookIntegrator._safe_dependency_path(apm_modules, rel_path) + if package_path is None: + continue + HookIntegrator._add_dependency_source(sources, package_path) + return sources + + return HookIntegrator._bounded_dependency_hook_sources(apm_modules) + + @staticmethod + def _lockfile_dependency_paths(project_root: Path) -> tuple[list[str], bool]: + """Return installed dependency paths from a readable lockfile, if present.""" + try: + from apm_cli.deps.lockfile import LEGACY_LOCKFILE_NAME, LockFile, get_lockfile_path + + lockfile_path = get_lockfile_path(project_root) + if not lockfile_path.exists(): + legacy_path = project_root / LEGACY_LOCKFILE_NAME + if legacy_path.exists(): + lockfile_path = legacy_path + if not lockfile_path.exists(): + return [], False + lockfile = LockFile.read(lockfile_path) + if lockfile is None: + return [], False + return lockfile.get_installed_paths(project_root / "apm_modules"), True + except (AttributeError, OSError, TypeError, ValueError, KeyError): + return [], False + + @staticmethod + def _safe_dependency_path(apm_modules: Path, rel_path: str) -> Path | None: + """Return a lockfile dependency path without escaping apm_modules.""" + try: + validate_path_segments( + rel_path, + context="lockfile dependency path", + reject_empty=True, + ) + package_path = apm_modules / Path(rel_path) + ensure_path_within(package_path, apm_modules) + if HookIntegrator._has_symlink_component(apm_modules, package_path): + return None + return package_path + except (OSError, PathTraversalError, RuntimeError, TypeError): + return None + + @staticmethod + def _has_symlink_component(apm_modules: Path, package_path: Path) -> bool: + """Return True when any component below apm_modules is a symlink.""" + try: + relative = package_path.relative_to(apm_modules) + current = apm_modules + for part in relative.parts: + current = current / part + if current.is_symlink(): + return True + return False + except (OSError, ValueError): + return True + + @staticmethod + def _is_dependency_package_dir(path: Path) -> bool: + """Return True when *path* looks like an installed package root.""" + try: + hooks = path / "hooks" + apm_hooks = path / ".apm" / "hooks" + apm_yml = path / "apm.yml" + skill_md = path / "SKILL.md" + return ( + (hooks.is_dir() and not hooks.is_symlink()) + or (apm_hooks.is_dir() and not apm_hooks.is_symlink()) + or (apm_yml.is_file() and not apm_yml.is_symlink()) + or (skill_md.is_file() and not skill_md.is_symlink()) + ) + except OSError: + return False + + @staticmethod + def _add_dependency_source(sources: set[str], package_path: Path) -> bool: + """Add package_path.name to sources when package_path is a package root.""" + try: + if ( + not package_path.is_dir() + or package_path.is_symlink() + or not HookIntegrator._is_dependency_package_dir(package_path) + ): + return False + except OSError: + return False + sources.add(package_path.name) + return True + + @staticmethod + def _child_dependency_dirs(path: Path) -> list[Path]: + """Return direct non-hidden child dirs without following symlink roots.""" + try: + if path.is_symlink() or not path.is_dir(): + return [] + return sorted( + [ + child + for child in path.iterdir() + if not child.is_symlink() and child.is_dir() and not child.name.startswith(".") + ], + key=lambda child: child.name, + ) + except OSError: + return [] + + @staticmethod + def _collect_known_subdirectory_sources(sources: set[str], repo_root: Path) -> None: + """Collect dependency sources from known virtual subdirectory layouts.""" + for namespace in ("collections", "skills"): + for package_path in HookIntegrator._child_dependency_dirs(repo_root / namespace): + HookIntegrator._add_dependency_source(sources, package_path) + + apm_dir = repo_root / ".apm" + try: + if apm_dir.is_symlink() or not apm_dir.is_dir(): + return + except OSError: + return + for primitive in ("agents", "commands", "hooks", "instructions", "prompts", "skills"): + for package_path in HookIntegrator._child_dependency_dirs(apm_dir / primitive): + HookIntegrator._add_dependency_source(sources, package_path) + + @staticmethod + def _collect_remote_dependency_sources(sources: set[str], namespace: Path) -> None: + """Collect fallback sources from explicit remote install layouts.""" + if HookIntegrator._add_dependency_source(sources, namespace): + return + + for repo_or_project in HookIntegrator._child_dependency_dirs(namespace): + if HookIntegrator._add_dependency_source(sources, repo_or_project): + continue + + HookIntegrator._collect_known_subdirectory_sources(sources, repo_or_project) + + for ado_repo in HookIntegrator._child_dependency_dirs(repo_or_project): + if HookIntegrator._add_dependency_source(sources, ado_repo): + continue + HookIntegrator._collect_known_subdirectory_sources(sources, ado_repo) + + @staticmethod + def _collect_local_dependency_sources(sources: set[str], local_namespace: Path) -> None: + """Collect apm_modules/_local/ package roots only.""" + for local_package in HookIntegrator._child_dependency_dirs(local_namespace): + HookIntegrator._add_dependency_source(sources, local_package) + + @staticmethod + def _bounded_dependency_hook_sources(apm_modules: Path) -> set[str]: + """Fallback source scan limited to known apm_modules package layouts.""" + sources: set[str] = set() + + for package_root in HookIntegrator._child_dependency_dirs(apm_modules): + if package_root.name == "_local": + HookIntegrator._collect_local_dependency_sources(sources, package_root) + continue + + HookIntegrator._collect_remote_dependency_sources(sources, package_root) + return sources + + def _should_remove_prior_merged_entry( + self, + entry, + *, + source_marker: str, + fresh_content_keys: set[str], + heal_stale_root_source: bool, + dependency_sources: set[str], + remove_current_source: bool, + ) -> bool: + """Return True when an existing merged-hook entry should be replaced.""" + if not isinstance(entry, dict): + return False + source = entry.get("_apm_source") + if remove_current_source and source == source_marker: + return True + if not heal_stale_root_source or not source or source in dependency_sources: + return False + return self._hook_entry_content_key(entry) in fresh_content_keys + def integrate_package_hooks( self, package_info, @@ -556,7 +816,7 @@ def integrate_package_hooks( hooks_dir = project_root / root_dir / "hooks" hooks_dir.mkdir(parents=True, exist_ok=True) - package_name = self._get_package_name(package_info) + package_name = self._get_package_name(package_info, project_root) hooks_integrated = 0 scripts_copied = 0 scripts_adopted = 0 @@ -662,7 +922,12 @@ def _integrate_merged_hooks( if not hook_files: return _empty - package_name = self._get_package_name(package_info) + package_name = self._get_package_name(package_info, project_root) + source_marker = self._get_hook_source_marker(package_info, project_root, package_name) + heal_stale_root_source = self._is_root_local_package(package_info, project_root) + dependency_sources = ( + self._dependency_hook_sources(project_root) if heal_stale_root_source else set() + ) hooks_integrated = 0 scripts_copied = 0 scripts_adopted = 0 @@ -724,7 +989,12 @@ def _integrate_merged_hooks( # Mark each entry with APM source for sync/cleanup for entry in entries: if isinstance(entry, dict): - entry["_apm_source"] = package_name + entry["_apm_source"] = source_marker + fresh_content_keys = { + self._hook_entry_content_key(entry) + for entry in entries + if isinstance(entry, dict) + } # Idempotent upsert: drop any prior entries owned by this # package before appending fresh ones. Without this, every @@ -734,12 +1004,20 @@ def _integrate_merged_hooks( # 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: + remove_current_source = event_name not in cleared_events + if remove_current_source or heal_stale_root_source: # 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) + if not self._should_remove_prior_merged_entry( + e, + source_marker=source_marker, + fresh_content_keys=fresh_content_keys, + heal_stale_root_source=heal_stale_root_source, + dependency_sources=dependency_sources, + remove_current_source=remove_current_source, + ) ] # Also clear from any alias events that map to # this normalised name (handles migration from @@ -749,8 +1027,13 @@ def _integrate_merged_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 + if not self._should_remove_prior_merged_entry( + e, + source_marker=source_marker, + fresh_content_keys=fresh_content_keys, + heal_stale_root_source=heal_stale_root_source, + dependency_sources=dependency_sources, + remove_current_source=remove_current_source, ) ] # Remove the alias key entirely if now empty diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index c6be6b4da..f50f9a72d 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -2730,6 +2730,21 @@ def _make_pkg( (hooks_dir / filename).write_text(json.dumps(data), encoding="utf-8") return _make_package_info(pkg_dir, pkg_name) + def _make_pkg_at( + self, + project: Path, + relative_path: str, + pkg_name: str, + hook_files: dict, + ) -> PackageInfo: + """Create a package below apm_modules at a specific relative path.""" + pkg_dir = project / "apm_modules" / Path(relative_path) + 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" @@ -2744,6 +2759,49 @@ def _read_cursor_hooks(self, project: Path) -> dict: return {} return json.loads(path.read_text(encoding="utf-8")) + def _read_codex_hooks(self, project: Path) -> dict: + """Return parsed .codex/hooks.json (or empty dict if absent).""" + path = project / ".codex" / "hooks.json" + if not path.exists(): + return {} + return json.loads(path.read_text(encoding="utf-8")) + + def _make_root_local_pkg( + self, + project: Path, + *, + manifest_name: str = "sample-project", + hook_data: dict | None = None, + ) -> PackageInfo: + """Create root .apm hook content plus apm.yml metadata.""" + if hook_data is None: + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + } + ] + } + } + (project / "apm.yml").write_text( + f"name: {manifest_name}\nversion: 0.0.0\n", + encoding="utf-8", + ) + hooks_dir = project / ".apm" / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + (hooks_dir / "pre-push-review.json").write_text( + json.dumps(hook_data), + encoding="utf-8", + ) + return _make_package_info(project, project.name) + # ------------------------------------------------------------------ # Group A: Target-aware file routing # ------------------------------------------------------------------ @@ -3095,6 +3153,469 @@ def test_content_dedup_preserves_cross_package(self, temp_project: Path) -> None f"Cross-package identical entries must both be present; got {len(entries)}" ) + def test_root_local_source_uses_manifest_name(self, temp_project: Path) -> None: + """Root .apm hooks use stable apm.yml metadata, not checkout basename.""" + manifest_name = f"{temp_project.name}-manifest" + pkg_info = self._make_root_local_pkg(temp_project, manifest_name=manifest_name) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + assert entries[0]["_apm_source"] == f"_local/{manifest_name}" + + def test_root_local_heals_stale_source_in_claude_settings( + self, + temp_project: Path, + ) -> None: + """Root .apm reinstall removes same-content entries from old checkout sources.""" + pkg_info = self._make_root_local_pkg(temp_project, manifest_name="sample-project") + settings_path = temp_project / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "suspicious-bardeen-e50cf8", + }, + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo user-owned"}], + }, + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + managed = [e for e in entries if isinstance(e, dict) and "_apm_source" in e] + user_owned = [e for e in entries if isinstance(e, dict) and "_apm_source" not in e] + assert [e["_apm_source"] for e in managed] == ["_local/sample-project"] + assert len(user_owned) == 1 + assert user_owned[0]["hooks"][0]["command"] == "echo user-owned" + + def test_root_local_heals_stale_source_in_codex_hooks( + self, + temp_project: Path, + ) -> None: + """Codex merged hooks get the same stale root-source healing.""" + (temp_project / ".codex").mkdir() + pkg_info = self._make_root_local_pkg(temp_project, manifest_name="sample-project") + hooks_path = temp_project / ".codex" / "hooks.json" + hooks_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "suspicious-bardeen-e50cf8", + } + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_codex(pkg_info, temp_project) + + entries = self._read_codex_hooks(temp_project)["hooks"]["PreToolUse"] + assert len(entries) == 1 + assert entries[0]["_apm_source"] == "_local/sample-project" + + def test_root_local_healer_preserves_dependency_source_entries( + self, + temp_project: Path, + ) -> None: + """Same-content dependency hooks are not mistaken for stale root hooks.""" + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="sample-project", + hook_data=hook_data, + ) + dep_info = self._make_pkg(temp_project, "dep-hooks", {"hooks.json": hook_data}) + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(dep_info, temp_project) + + settings_path = temp_project / ".claude" / "settings.json" + settings = json.loads(settings_path.read_text(encoding="utf-8")) + settings["hooks"]["PreToolUse"].append( + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "suspicious-bardeen-e50cf8", + } + ) + settings_path.write_text(json.dumps(settings), encoding="utf-8") + + integrator.integrate_package_hooks_claude(root_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + sources = [e["_apm_source"] for e in entries if isinstance(e, dict)] + assert sources == ["dep-hooks", "_local/sample-project"] + + @pytest.mark.parametrize( + "relative_path", + [ + "owner/dep-hooks", + "org/project/dep-hooks", + "owner/repo/collections/dep-hooks", + "org/project/repo/collections/dep-hooks", + "owner/repo/.apm/skills/dep-hooks", + "_local/dep-hooks", + ], + ) + def test_root_local_healer_preserves_bounded_dependency_layouts( + self, + temp_project: Path, + relative_path: str, + ) -> None: + """Bounded dependency scans preserve known package root layouts.""" + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="sample-project", + hook_data=hook_data, + ) + dep_info = self._make_pkg_at( + temp_project, + relative_path, + "dep-hooks", + {"hooks.json": hook_data}, + ) + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(dep_info, temp_project) + + settings_path = temp_project / ".claude" / "settings.json" + settings = json.loads(settings_path.read_text(encoding="utf-8")) + settings["hooks"]["PreToolUse"].append( + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "stale-root-name", + } + ) + settings_path.write_text(json.dumps(settings), encoding="utf-8") + + integrator.integrate_package_hooks_claude(root_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + sources = [e["_apm_source"] for e in entries if isinstance(e, dict)] + assert sources == ["dep-hooks", "_local/sample-project"] + + def test_dependency_hook_sources_uses_lockfile_paths( + self, + temp_project: Path, + ) -> None: + """Readable lockfiles provide exact dependency roots without broad scans.""" + pkg_dir = temp_project / "apm_modules" / "owner" / "repo" / "collections" / "dep-hooks" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: dep-hooks\n", encoding="utf-8") + (temp_project / "apm.lock.yaml").write_text( + "\n".join( + [ + 'lockfile_version: "1"', + "dependencies:", + " - repo_url: owner/repo", + " virtual_path: collections/dep-hooks", + " is_virtual: true", + "", + ] + ), + encoding="utf-8", + ) + + assert HookIntegrator._dependency_hook_sources(temp_project) == {"dep-hooks"} + + def test_dependency_hook_sources_rejects_lockfile_symlink_root( + self, + temp_project: Path, + ) -> None: + """Lockfile dependency roots do not follow symlink package roots.""" + real_pkg = temp_project / "apm_modules" / "owner" / "real-dep" + real_pkg.mkdir(parents=True) + (real_pkg / "apm.yml").write_text("name: real-dep\n", encoding="utf-8") + link_pkg = temp_project / "apm_modules" / "owner" / "link-dep" + try: + link_pkg.symlink_to(real_pkg, target_is_directory=True) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlink unavailable: {exc}") + (temp_project / "apm.lock.yaml").write_text( + "\n".join( + [ + 'lockfile_version: "1"', + "dependencies:", + " - repo_url: owner/link-dep", + "", + ] + ), + encoding="utf-8", + ) + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_dependency_hook_sources_falls_back_when_lockfile_paths_are_invalid( + self, + temp_project: Path, + ) -> None: + """Invalid lockfile paths do not disable bounded fallback discovery.""" + pkg_dir = temp_project / "apm_modules" / "owner" / "dep-hooks" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: dep-hooks\n", encoding="utf-8") + (temp_project / "apm.lock.yaml").write_text( + "\n".join( + [ + 'lockfile_version: "1"', + "dependencies:", + " - repo_url: owner/repo", + " virtual_path: ../bad", + " is_virtual: true", + "", + ] + ), + encoding="utf-8", + ) + + assert HookIntegrator._dependency_hook_sources(temp_project) == {"dep-hooks"} + + def test_bounded_dependency_scan_stops_at_package_root( + self, + temp_project: Path, + ) -> None: + """Nested package content below a package root is not a dependency source.""" + root_info = self._make_root_local_pkg(temp_project, manifest_name="sample-project") + package_root = temp_project / "apm_modules" / "owner" / "repo" + package_root.mkdir(parents=True) + (package_root / "apm.yml").write_text("name: repo\n", encoding="utf-8") + nested_skill = package_root / "tools" / "deep-skill" + nested_skill.mkdir(parents=True) + (nested_skill / "SKILL.md").write_text("# Deep Skill\n", encoding="utf-8") + + settings_path = temp_project / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "bash .codex/hooks/pre-push-review.sh", + } + ], + "_apm_source": "deep-skill", + } + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_claude(root_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + sources = [e["_apm_source"] for e in entries if isinstance(e, dict)] + assert sources == ["_local/sample-project"] + + def test_bounded_dependency_scan_ignores_unrecognized_nested_markers( + self, + temp_project: Path, + ) -> None: + """Fallback discovery does not scan arbitrary package internals.""" + nested = ( + temp_project / "apm_modules" / "owner" / "repo" / "tests" / "fixtures" / "dep-hooks" + ) + nested.mkdir(parents=True) + (nested / "SKILL.md").write_text("# Fixture Skill\n", encoding="utf-8") + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_bounded_dependency_scan_rejects_symlinked_namespace( + self, + temp_project: Path, + ) -> None: + """Fallback discovery does not follow namespace symlinks.""" + repo_root = temp_project / "apm_modules" / "owner" / "repo" + repo_root.mkdir(parents=True) + outside = temp_project / "outside" / "dep-hooks" + outside.mkdir(parents=True) + (outside / "SKILL.md").write_text("# External Skill\n", encoding="utf-8") + try: + (repo_root / "collections").symlink_to(outside.parent, target_is_directory=True) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlink unavailable: {exc}") + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_bounded_dependency_scan_rejects_symlinked_marker( + self, + temp_project: Path, + ) -> None: + """Fallback discovery does not accept symlinked package markers.""" + package_root = temp_project / "apm_modules" / "owner" / "repo" / "collections" / "dep-hooks" + package_root.mkdir(parents=True) + outside_hooks = temp_project / "outside" / "hooks" + outside_hooks.mkdir(parents=True) + try: + (package_root / "hooks").symlink_to(outside_hooks, target_is_directory=True) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlink unavailable: {exc}") + + assert HookIntegrator._dependency_hook_sources(temp_project) == set() + + def test_root_local_source_marker_does_not_collide_with_dependency_name( + self, + temp_project: Path, + ) -> None: + """Root source markers are namespaced even when apm.yml matches a dependency.""" + hook_data = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo shared"}], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="matching-dep", + hook_data=hook_data, + ) + dep_info = self._make_pkg(temp_project, "matching-dep", {"hooks.json": hook_data}) + + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(dep_info, temp_project) + integrator.integrate_package_hooks_claude(root_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + sources = [e["_apm_source"] for e in entries if isinstance(e, dict)] + assert sources == ["matching-dep", "_local/matching-dep"] + + def test_root_local_heals_stale_source_for_multiple_hook_files_same_event( + self, + temp_project: Path, + ) -> None: + """Stale-source healing applies to every root hook file for an event.""" + first = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo first"}], + } + ] + } + } + second = { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo second"}], + } + ] + } + } + root_info = self._make_root_local_pkg( + temp_project, + manifest_name="sample-project", + hook_data=first, + ) + (temp_project / ".apm" / "hooks" / "z-second.json").write_text( + json.dumps(second), + encoding="utf-8", + ) + settings_path = temp_project / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo second"}], + "_apm_source": "old-root-name", + } + ] + } + } + ), + encoding="utf-8", + ) + + HookIntegrator().integrate_package_hooks_claude(root_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + commands = [e["hooks"][0]["command"] for e in entries if isinstance(e, dict)] + sources = [e["_apm_source"] for e in entries if isinstance(e, dict)] + assert commands == ["echo first", "echo second"] + assert sources == ["_local/sample-project", "_local/sample-project"] + def test_reinstall_clears_aliased_events(self, temp_project: Path) -> None: """Re-integration removes stale postToolUse (camelCase) aliases.