From b673c4b334aa9a7fdb7dea98d08fc85bbfe90bfc Mon Sep 17 00:00:00 2001 From: Koichi Takahashi Date: Fri, 15 May 2026 06:10:06 +0900 Subject: [PATCH 1/8] fix: stabilize root hook source ids --- CHANGELOG.md | 1 + src/apm_cli/integration/hook_integrator.py | 144 ++++++++- .../unit/integration/test_hook_integrator.py | 274 ++++++++++++++++++ 3 files changed, 410 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eba73a6d..bacda7485 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - `apm install` honors the SSH user portion of dependency URLs (`ssh://user@host/...` and scp shorthand `user@host:org/repo`) instead of hardcoding `git@`; unblocks EMU accounts and other non-`git` SSH identities. User values are validated against a strict allowlist before composing the clone URL. (#1385, closes #1383) +- Root `.apm` hooks no longer duplicate after renaming the project directory or using git worktrees; Claude/Codex/Cursor/Gemini/Windsurf hook configs stay idempotent across checkouts. (#1329) ## [0.14.0] - 2026-05-18 diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 4c8272d88..fd9f8d2f7 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -51,6 +51,8 @@ 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.console import _rich_warning from apm_cli.utils.path_security import PathTraversalError, ensure_path_within @@ -583,17 +585,118 @@ 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() + sources: set[str] = set() + for path in apm_modules.rglob("*"): + if not path.is_dir(): + continue + if ( + (path / "hooks").is_dir() + or (path / ".apm" / "hooks").is_dir() + or (path / "apm.yml").is_file() + or (path / "SKILL.md").is_file() + ): + sources.add(path.name) + 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, @@ -633,7 +736,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 @@ -739,7 +842,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 @@ -825,7 +933,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 @@ -835,12 +948,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 @@ -850,8 +971,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 9ee3e2d00..56dc5d542 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -3065,6 +3065,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 = "hibi", + 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 # ------------------------------------------------------------------ @@ -3545,6 +3588,237 @@ 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.""" + pkg_info = self._make_root_local_pkg(temp_project, manifest_name="hibi") + + HookIntegrator().integrate_package_hooks_claude(pkg_info, temp_project) + + entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] + assert temp_project.name != "hibi" + assert entries[0]["_apm_source"] == "_local/hibi" + + 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="hibi") + 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/hibi"] + 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="hibi") + 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/hibi" + + 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="hibi", + 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/hibi"] + + 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="hibi", + hook_data=hook_data, + ) + dep_info = self._make_pkg(temp_project, "hibi", {"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 == ["hibi", "_local/hibi"] + + 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="hibi", + 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/hibi", "_local/hibi"] + def test_reinstall_clears_aliased_events(self, temp_project: Path) -> None: """Re-integration removes stale postToolUse (camelCase) aliases. From 05bfda90bf4970df4597e446f5c141b3d7381639 Mon Sep 17 00:00:00 2001 From: Koichi Takahashi Date: Fri, 15 May 2026 07:44:40 +0900 Subject: [PATCH 2/8] test: remove project-specific hook fixture names --- .../unit/integration/test_hook_integrator.py | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 56dc5d542..74ffad775 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -3076,7 +3076,7 @@ def _make_root_local_pkg( self, project: Path, *, - manifest_name: str = "hibi", + manifest_name: str = "sample-project", hook_data: dict | None = None, ) -> PackageInfo: """Create root .apm hook content plus apm.yml metadata.""" @@ -3590,20 +3590,20 @@ def test_content_dedup_preserves_cross_package(self, temp_project: Path) -> None def test_root_local_source_uses_manifest_name(self, temp_project: Path) -> None: """Root .apm hooks use stable apm.yml metadata, not checkout basename.""" - pkg_info = self._make_root_local_pkg(temp_project, manifest_name="hibi") + 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 temp_project.name != "hibi" - assert entries[0]["_apm_source"] == "_local/hibi" + 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="hibi") + 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( @@ -3637,7 +3637,7 @@ def test_root_local_heals_stale_source_in_claude_settings( 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/hibi"] + 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" @@ -3647,7 +3647,7 @@ def test_root_local_heals_stale_source_in_codex_hooks( ) -> 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="hibi") + 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( @@ -3675,7 +3675,7 @@ def test_root_local_heals_stale_source_in_codex_hooks( entries = self._read_codex_hooks(temp_project)["hooks"]["PreToolUse"] assert len(entries) == 1 - assert entries[0]["_apm_source"] == "_local/hibi" + assert entries[0]["_apm_source"] == "_local/sample-project" def test_root_local_healer_preserves_dependency_source_entries( self, @@ -3699,7 +3699,7 @@ def test_root_local_healer_preserves_dependency_source_entries( } root_info = self._make_root_local_pkg( temp_project, - manifest_name="hibi", + manifest_name="sample-project", hook_data=hook_data, ) dep_info = self._make_pkg(temp_project, "dep-hooks", {"hooks.json": hook_data}) @@ -3726,7 +3726,7 @@ def test_root_local_healer_preserves_dependency_source_entries( 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/hibi"] + assert sources == ["dep-hooks", "_local/sample-project"] def test_root_local_source_marker_does_not_collide_with_dependency_name( self, @@ -3745,10 +3745,10 @@ def test_root_local_source_marker_does_not_collide_with_dependency_name( } root_info = self._make_root_local_pkg( temp_project, - manifest_name="hibi", + manifest_name="matching-dep", hook_data=hook_data, ) - dep_info = self._make_pkg(temp_project, "hibi", {"hooks.json": 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) @@ -3756,7 +3756,7 @@ def test_root_local_source_marker_does_not_collide_with_dependency_name( entries = self._read_claude_settings(temp_project)["hooks"]["PreToolUse"] sources = [e["_apm_source"] for e in entries if isinstance(e, dict)] - assert sources == ["hibi", "_local/hibi"] + assert sources == ["matching-dep", "_local/matching-dep"] def test_root_local_heals_stale_source_for_multiple_hook_files_same_event( self, @@ -3785,7 +3785,7 @@ def test_root_local_heals_stale_source_for_multiple_hook_files_same_event( } root_info = self._make_root_local_pkg( temp_project, - manifest_name="hibi", + manifest_name="sample-project", hook_data=first, ) (temp_project / ".apm" / "hooks" / "z-second.json").write_text( @@ -3817,7 +3817,7 @@ def test_root_local_heals_stale_source_for_multiple_hook_files_same_event( 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/hibi", "_local/hibi"] + 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. From c55cb62e4933bcbb7a2f9c8fa329681114280d40 Mon Sep 17 00:00:00 2001 From: Koichi Takahashi Date: Sun, 17 May 2026 22:41:19 +0900 Subject: [PATCH 3/8] fix(hooks): bound dependency hook source discovery --- src/apm_cli/integration/hook_integrator.py | 167 ++++++++++++- .../unit/integration/test_hook_integrator.py | 231 ++++++++++++++++++ 2 files changed, 391 insertions(+), 7 deletions(-) diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index fd9f8d2f7..c112ae7d1 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -55,7 +55,11 @@ from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult from apm_cli.utils.console import _rich_warning -from apm_cli.utils.path_security import PathTraversalError, 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__) @@ -664,17 +668,166 @@ def _dependency_hook_sources(project_root: Path) -> set[str]: apm_modules = project_root / "apm_modules" if not apm_modules.is_dir(): return set() - sources: set[str] = set() - for path in apm_modules.rglob("*"): - if not path.is_dir(): - continue - if ( + + 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: + return ( (path / "hooks").is_dir() or (path / ".apm" / "hooks").is_dir() or (path / "apm.yml").is_file() or (path / "SKILL.md").is_file() + ) + 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) ): - sources.add(path.name) + 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( diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 74ffad775..e42cf9817 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -3051,6 +3051,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" @@ -3728,6 +3743,222 @@ def test_root_local_healer_preserves_dependency_source_entries( 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_root_local_source_marker_does_not_collide_with_dependency_name( self, temp_project: Path, From 3bb95210b15db99897926ec353dd1fbebd980185 Mon Sep 17 00:00:00 2001 From: Koichi Takahashi Date: Mon, 18 May 2026 06:58:37 +0900 Subject: [PATCH 4/8] fix(hooks): reject symlinked dependency markers --- src/apm_cli/integration/hook_integrator.py | 12 ++++++++---- tests/unit/integration/test_hook_integrator.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index c112ae7d1..586daf3b5 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -736,11 +736,15 @@ def _has_symlink_component(apm_modules: Path, package_path: Path) -> bool: 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 ( - (path / "hooks").is_dir() - or (path / ".apm" / "hooks").is_dir() - or (path / "apm.yml").is_file() - or (path / "SKILL.md").is_file() + (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 diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index e42cf9817..4f0516397 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -3959,6 +3959,22 @@ def test_bounded_dependency_scan_rejects_symlinked_namespace( 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, From 17dcb853843029c0231f3b514156bb0086e39d79 Mon Sep 17 00:00:00 2001 From: Koichi Takahashi Date: Tue, 19 May 2026 15:13:22 +0200 Subject: [PATCH 5/8] test(hooks): subprocess-tier coverage + debug logs for #1329 - Add tests/integration/test_hook_root_source_drift_e2e.py: subprocess test that seeds stale .claude/settings.json + apm-hooks.json sidecar with old source-id, runs apm install, asserts entries heal to _local/ and user-owned hook entry survives. - Add two _log.debug lines in hook_integrator.py: - manifest-parse fallback in _get_root_local_package_name - heal count when stale same-content merged entries are removed - Update affected unit tests to read _apm_source from Claude sidecar (post-#1359 schema-strict layout). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/integration/hook_integrator.py | 33 +++++++- .../test_hook_root_source_drift_e2e.py | 82 +++++++++++++++++++ .../unit/integration/test_hook_integrator.py | 60 ++++++++++---- 3 files changed, 156 insertions(+), 19 deletions(-) create mode 100644 tests/integration/test_hook_root_source_drift_e2e.py diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 586daf3b5..1bf36130a 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -621,8 +621,13 @@ def _get_root_local_package_name(self, package_info, project_root: Path) -> str: manifest_name = self._safe_source_name(data.get("name")) if manifest_name != "_local": return manifest_name - except (OSError, ValueError, yaml.YAMLError): - pass + except (OSError, ValueError, yaml.YAMLError) as exc: + _log.debug( + "Hook integrator: apm.yml manifest unreadable for %s (%s), " + "falling back to install_path basename", + project_root, + exc.__class__.__name__, + ) package = getattr(package_info, "package", None) package_name = self._safe_source_name(getattr(package, "name", None)) @@ -1108,9 +1113,10 @@ def _integrate_merged_hooks( 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] = [ + prior_entries = json_config["hooks"][event_name] + kept_entries = [ e - for e in json_config["hooks"][event_name] + for e in prior_entries if not self._should_remove_prior_merged_entry( e, source_marker=source_marker, @@ -1120,6 +1126,25 @@ def _integrate_merged_hooks( remove_current_source=remove_current_source, ) ] + if heal_stale_root_source: + healed = sum( + 1 + for e in prior_entries + if isinstance(e, dict) + and e.get("_apm_source") + and e.get("_apm_source") != source_marker + and e.get("_apm_source") not in dependency_sources + and e not in kept_entries + ) + if healed: + _log.debug( + "Hook integrator: healed %d stale same-content " + "merged hook entries for source %s in event %s", + healed, + source_marker, + event_name, + ) + json_config["hooks"][event_name] = kept_entries # Also clear from any alias events that map to # this normalised name (handles migration from # corrupted installs with mixed-case event keys). diff --git a/tests/integration/test_hook_root_source_drift_e2e.py b/tests/integration/test_hook_root_source_drift_e2e.py new file mode 100644 index 000000000..513eb7064 --- /dev/null +++ b/tests/integration/test_hook_root_source_drift_e2e.py @@ -0,0 +1,82 @@ +"""End-to-end regression for #1329: stale root hook _apm_source heals on reinstall.""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +CLI = [sys.executable, "-m", "apm_cli.cli"] + + +def _run(cwd: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(CLI + list(args), cwd=str(cwd), capture_output=True, text=True) + + +def test_root_hook_source_drift_heals_on_reinstall(tmp_path: Path) -> None: + project = tmp_path / "myapp" + project.mkdir() + (project / "apm.yml").write_text( + "name: myapp\nversion: 0.0.0\ntargets:\n - claude\n", + encoding="utf-8", + ) + hooks_dir = project / ".apm" / "hooks" + hooks_dir.mkdir(parents=True) + (hooks_dir / "pre.json").write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "bash .codex/hooks/pre.sh"}], + } + ] + } + } + ), + encoding="utf-8", + ) + + # Simulate a legacy install whose _apm_source came from an old checkout basename. + settings = project / ".claude" / "settings.json" + settings.parent.mkdir(parents=True, exist_ok=True) + settings.write_text( + json.dumps( + { + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "bash .codex/hooks/pre.sh"}], + "_apm_source": "old-checkout-name", + }, + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo user-owned"}], + }, + ] + } + } + ), + encoding="utf-8", + ) + + result = _run(project, "install") + assert result.returncode == 0, result.stderr or result.stdout + + entries = json.loads(settings.read_text())["hooks"]["PreToolUse"] + sidecar_path = project / ".claude" / "apm-hooks.json" + sidecar = json.loads(sidecar_path.read_text()) + sidecar_sources = [ + e.get("_apm_source") for e in sidecar.get("PreToolUse", []) if isinstance(e, dict) + ] + user_owned = [ + e for e in entries if isinstance(e, dict) and e["hooks"][0]["command"] == "echo user-owned" + ] + + assert sidecar_sources == ["_local/myapp"], ( + f"Expected single _local/myapp entry in sidecar, got {sidecar_sources}" + ) + assert len(user_owned) == 1, "User-owned hook entry must survive healing" diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 4f0516397..0029c2962 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -3073,6 +3073,39 @@ def _read_claude_settings(self, project: Path) -> dict: return {} return json.loads(path.read_text(encoding="utf-8")) + def _read_claude_sidecar(self, project: Path) -> dict: + """Return parsed .claude/apm-hooks.json sidecar (or empty dict if absent).""" + path = project / ".claude" / "apm-hooks.json" + if not path.exists(): + return {} + return json.loads(path.read_text(encoding="utf-8")) + + def _claude_sources(self, project: Path, event: str) -> list[str]: + """Return _apm_source markers for an event, ordered by settings.json entries. + + Schema-strict Claude stores ownership in apm-hooks.json sidecar; match each + settings.json entry by content to its sidecar twin so order is preserved. + """ + entries = self._read_claude_settings(project).get("hooks", {}).get(event, []) + sidecar_entries = self._read_claude_sidecar(project).get(event, []) + pool = list(sidecar_entries) + sources: list[str] = [] + for entry in entries: + if not isinstance(entry, dict): + continue + cmp = {k: v for k, v in entry.items() if k != "_apm_source"} + match_idx = None + for idx, sc in enumerate(pool): + if not isinstance(sc, dict): + continue + sc_cmp = {k: v for k, v in sc.items() if k != "_apm_source"} + if sc_cmp == cmp: + match_idx = idx + break + if match_idx is not None: + sources.append(pool.pop(match_idx).get("_apm_source")) + return sources + def _read_cursor_hooks(self, project: Path) -> dict: """Return parsed .cursor/hooks.json (or empty dict if absent).""" path = project / ".cursor" / "hooks.json" @@ -3610,8 +3643,8 @@ def test_root_local_source_uses_manifest_name(self, temp_project: Path) -> None: 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}" + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == [f"_local/{manifest_name}"] def test_root_local_heals_stale_source_in_claude_settings( self, @@ -3650,11 +3683,12 @@ def test_root_local_heals_stale_source_in_claude_settings( 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"] + sources = self._claude_sources(temp_project, "PreToolUse") + assert sources == ["_local/sample-project"] + # settings.json must retain both the managed entry and the user-owned hook + assert len(entries) == 2 + user_owned = [e for e in entries if e["hooks"][0]["command"] == "echo user-owned"] 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, @@ -3739,8 +3773,7 @@ def test_root_local_healer_preserves_dependency_source_entries( 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)] + sources = self._claude_sources(temp_project, "PreToolUse") assert sources == ["dep-hooks", "_local/sample-project"] @pytest.mark.parametrize( @@ -3807,8 +3840,7 @@ def test_root_local_healer_preserves_bounded_dependency_layouts( 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)] + sources = self._claude_sources(temp_project, "PreToolUse") assert sources == ["dep-hooks", "_local/sample-project"] def test_dependency_hook_sources_uses_lockfile_paths( @@ -3925,8 +3957,7 @@ def test_bounded_dependency_scan_stops_at_package_root( 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)] + sources = self._claude_sources(temp_project, "PreToolUse") assert sources == ["_local/sample-project"] def test_bounded_dependency_scan_ignores_unrecognized_nested_markers( @@ -4001,8 +4032,7 @@ def test_root_local_source_marker_does_not_collide_with_dependency_name( 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)] + sources = self._claude_sources(temp_project, "PreToolUse") assert sources == ["matching-dep", "_local/matching-dep"] def test_root_local_heals_stale_source_for_multiple_hook_files_same_event( @@ -4062,7 +4092,7 @@ def test_root_local_heals_stale_source_for_multiple_hook_files_same_event( 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)] + sources = self._claude_sources(temp_project, "PreToolUse") assert commands == ["echo first", "echo second"] assert sources == ["_local/sample-project", "_local/sample-project"] From 9474bd52f01a0b1e3c3eab6a79e8b5c60368878d Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Thu, 21 May 2026 15:21:42 +0200 Subject: [PATCH 6/8] fix(hooks): act on review-panel recommendations for #1329 follow-up Addresses the top recommended-tier follow-ups from the APM review panel on #1392 (https://github.com/microsoft/apm/pull/1392#issuecomment-4507830595). In-PR: * CHANGELOG: add [Unreleased] > Fixed entry with user-outcome framing, enumerating all five affected harnesses (Claude / Codex / Cursor / Gemini / Windsurf) and the explicit parenthetical that Copilot is unaffected because its hooks live in per-file namespaces rather than a shared merged config. (doc-writer + oss-growth-hacker convergence) * Integration coverage: parametrize the e2e heal regression test across all five merged-hook harnesses instead of Claude only. Uses an install-then-corrupt-then-reinstall pattern so the seeded entries match each target's on-disk shape (sidecar for Claude; in-file _apm_source for Codex / Cursor / Gemini / Windsurf), closing the silent-drift gap the test-coverage-expert flagged as highest-signal. * _safe_source_name hardening: collapse runs of 2+ dots to a single dot before stripping edges. No traversal risk today (the marker is JSON metadata), but guards any future caller that passes the name into a Path join. (supply-chain-security recommendation.) * Nits: include exception message (not just class name) in the manifest-unreadable debug log; replace O(n*m) list membership in the heal counter with an id-set; document the project_root parameter on _get_package_name; promote _get_root_local_package_name, _get_hook_source_marker, and _should_remove_prior_merged_entry to @staticmethod (they touch no instance state). The DependencySourceScanner extraction the python-architect proposed is explicitly out of scope for this PR per the panel's own guidance ("does not need to land in this PR but should not slip past the next meaningful touch to hook_integrator.py"). Validation: * uv run --extra dev ruff check src/ tests/ -- clean * uv run --extra dev ruff format --check src/ tests/ -- clean * uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py -- 156 passed * uv run --extra dev pytest tests/integration/test_hook_root_source_drift_e2e.py -- 5 passed (claude, codex, cursor, gemini, windsurf) * uv run --extra dev pytest tests/unit/integration/ tests/integration/test_hook_root_source_drift_e2e.py -- 1364 passed Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 + src/apm_cli/integration/hook_integrator.py | 33 ++- .../test_hook_root_source_drift_e2e.py | 203 ++++++++++++++---- 3 files changed, 191 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6743eda6..1d80b4771 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Unit test coverage raised to 88% (gate: `fail_under = 80`); integration test coverage raised to 71% with first CI gate at 55%. (#1402) +### Fixed + +- Root `.apm` hooks no longer duplicate after renaming the project directory or using git worktrees; Claude, Codex, Cursor, Gemini, and Windsurf hook configs stay idempotent across checkouts. The hook source-id is now derived from `apm.yml`'s `name` field instead of `install_path.name`, and `apm install` silently heals stale same-content entries from prior checkout basenames. Copilot is unaffected (its hooks live in per-file namespaces under `.github/hooks/`, not a shared merged config). (#1392, closes #1329) + ## [0.14.1] - 2026-05-20 ### Added diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 1bf36130a..c20342c01 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -604,12 +604,17 @@ 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(".-_") + safe = re.sub(r"[^A-Za-z0-9._-]+", "-", value.strip()) + # Collapse any run of 2+ dots to a single dot before stripping edges. + # Embedded sequences like "foo..bar" would otherwise pass through the + # earlier guard and reach downstream Path joins as a parent-dir hop. + safe = re.sub(r"\.{2,}", ".", safe).strip(".-_") if not safe or safe in {".", ".."}: return fallback return safe - def _get_root_local_package_name(self, package_info, project_root: Path) -> str: + @staticmethod + def _get_root_local_package_name(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(): @@ -618,19 +623,20 @@ def _get_root_local_package_name(self, package_info, project_root: Path) -> str: data = load_yaml(apm_yml) if isinstance(data, dict): - manifest_name = self._safe_source_name(data.get("name")) + manifest_name = HookIntegrator._safe_source_name(data.get("name")) if manifest_name != "_local": return manifest_name except (OSError, ValueError, yaml.YAMLError) as exc: _log.debug( - "Hook integrator: apm.yml manifest unreadable for %s (%s), " + "Hook integrator: apm.yml manifest unreadable for %s (%s: %s), " "falling back to install_path basename", project_root, exc.__class__.__name__, + exc, ) package = getattr(package_info, "package", None) - package_name = self._safe_source_name(getattr(package, "name", None)) + package_name = HookIntegrator._safe_source_name(getattr(package, "name", None)) if package_name != "_local": return package_name return "_local" @@ -640,22 +646,26 @@ def _get_package_name(self, package_info, project_root: Path | None = None) -> s Args: package_info: PackageInfo object + project_root: When provided and the package is the project root, + reads ``apm.yml`` ``name`` for a stable source marker instead + of falling back to ``install_path.name`` (which drifts on + directory renames and worktrees). See #1329. Returns: 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 HookIntegrator._get_root_local_package_name(package_info, Path(project_root)) return package_info.install_path.name + @staticmethod 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 HookIntegrator._is_root_local_package(package_info, project_root): if package_name == "_local": return "_local" return f"_local/{package_name}" @@ -839,8 +849,8 @@ def _bounded_dependency_hook_sources(apm_modules: Path) -> set[str]: HookIntegrator._collect_remote_dependency_sources(sources, package_root) return sources + @staticmethod def _should_remove_prior_merged_entry( - self, entry, *, source_marker: str, @@ -857,7 +867,7 @@ def _should_remove_prior_merged_entry( 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 + return HookIntegrator._hook_entry_content_key(entry) in fresh_content_keys def integrate_package_hooks( self, @@ -1127,6 +1137,7 @@ def _integrate_merged_hooks( ) ] if heal_stale_root_source: + kept_ids = {id(e) for e in kept_entries} healed = sum( 1 for e in prior_entries @@ -1134,7 +1145,7 @@ def _integrate_merged_hooks( and e.get("_apm_source") and e.get("_apm_source") != source_marker and e.get("_apm_source") not in dependency_sources - and e not in kept_entries + and id(e) not in kept_ids ) if healed: _log.debug( diff --git a/tests/integration/test_hook_root_source_drift_e2e.py b/tests/integration/test_hook_root_source_drift_e2e.py index 513eb7064..b29543333 100644 --- a/tests/integration/test_hook_root_source_drift_e2e.py +++ b/tests/integration/test_hook_root_source_drift_e2e.py @@ -1,4 +1,10 @@ -"""End-to-end regression for #1329: stale root hook _apm_source heals on reinstall.""" +"""End-to-end regression for #1329: stale root hook _apm_source heals on reinstall. + +Parametrized across every harness whose hooks live in a merged config file +(Claude, Codex, Cursor, Gemini, Windsurf). Copilot is intentionally excluded: +its hooks live in per-file namespaces under ``.github/hooks/`` and are not +subject to the merged-config drift this fix targets. +""" from __future__ import annotations @@ -7,6 +13,8 @@ import sys from pathlib import Path +import pytest + CLI = [sys.executable, "-m", "apm_cli.cli"] @@ -14,11 +22,119 @@ def _run(cwd: Path, *args: str) -> subprocess.CompletedProcess: return subprocess.run(CLI + list(args), cwd=str(cwd), capture_output=True, text=True) -def test_root_hook_source_drift_heals_on_reinstall(tmp_path: Path) -> None: +# Per-target on-disk layout descriptors: +# - settings_rel: path (relative to project root) of the merged hook config +# - sidecar_rel: path of the apm-hooks.json sidecar (schema-strict targets only) +# - event_key: event key under which PreToolUse entries land for the target +# +# Sidecar-bearing targets (currently Claude) keep _apm_source in the sidecar +# rather than in settings.json; the heal assertion reads from there. +_HARNESS_CASES = [ + pytest.param( + "claude", + ".claude/settings.json", + ".claude/apm-hooks.json", + "PreToolUse", + id="claude", + ), + pytest.param( + "codex", + ".codex/hooks.json", + None, + "PreToolUse", + id="codex", + ), + pytest.param( + "cursor", + ".cursor/hooks.json", + None, + "PreToolUse", + id="cursor", + ), + pytest.param( + "gemini", + ".gemini/settings.json", + None, + "BeforeTool", + id="gemini", + ), + pytest.param( + "windsurf", + ".windsurf/hooks.json", + None, + "PreToolUse", + id="windsurf", + ), +] + + +def _load_sources( + project: Path, + settings_rel: str, + sidecar_rel: str | None, + event_key: str, +) -> list[str]: + """Return the list of _apm_source markers for the target's PreToolUse-equivalent. + + Entries lacking ``_apm_source`` (user-owned hooks) are excluded so the + heal assertion can compare just the APM-owned slice. + """ + if sidecar_rel: + sidecar = json.loads((project / sidecar_rel).read_text(encoding="utf-8")) + return [ + e["_apm_source"] + for e in sidecar.get(event_key, []) + if isinstance(e, dict) and e.get("_apm_source") + ] + settings = json.loads((project / settings_rel).read_text(encoding="utf-8")) + return [ + e["_apm_source"] + for e in settings.get("hooks", {}).get(event_key, []) + if isinstance(e, dict) and e.get("_apm_source") + ] + + +def _rewrite_source( + project: Path, + settings_rel: str, + sidecar_rel: str | None, + event_key: str, + *, + old: str, + new: str, +) -> None: + """Mutate _apm_source markers in-place to simulate a stale checkout basename.""" + target_rel = sidecar_rel or settings_rel + target = project / target_rel + data = json.loads(target.read_text(encoding="utf-8")) + container = data if sidecar_rel else data.get("hooks", {}) + entries = container.get(event_key, []) + for entry in entries: + if isinstance(entry, dict) and entry.get("_apm_source") == old: + entry["_apm_source"] = new + target.write_text(json.dumps(data), encoding="utf-8") + + +@pytest.mark.parametrize("target, settings_rel, sidecar_rel, event_key", _HARNESS_CASES) +def test_root_hook_source_drift_heals_on_reinstall( + tmp_path: Path, + target: str, + settings_rel: str, + sidecar_rel: str | None, + event_key: str, +) -> None: + """After a checkout rename, a second `apm install` heals stale source markers. + + Strategy: install once cleanly so the integrator writes target-shaped entries + with `_local/myapp`; then rewrite the marker to simulate an old checkout + basename and append a user-owned hook; then install again and assert the + stale marker is gone, the user-owned hook survives, and exactly one APM + entry with `_local/myapp` remains. + """ project = tmp_path / "myapp" project.mkdir() (project / "apm.yml").write_text( - "name: myapp\nversion: 0.0.0\ntargets:\n - claude\n", + f"name: myapp\nversion: 0.0.0\ntargets:\n - {target}\n", encoding="utf-8", ) hooks_dir = project / ".apm" / "hooks" @@ -30,7 +146,7 @@ def test_root_hook_source_drift_heals_on_reinstall(tmp_path: Path) -> None: "PreToolUse": [ { "matcher": "Bash", - "hooks": [{"type": "command", "command": "bash .codex/hooks/pre.sh"}], + "hooks": [{"type": "command", "command": "echo apm-managed"}], } ] } @@ -39,44 +155,55 @@ def test_root_hook_source_drift_heals_on_reinstall(tmp_path: Path) -> None: encoding="utf-8", ) + # First install: produces target-shaped entries marked `_local/myapp`. + first = _run(project, "install") + assert first.returncode == 0, first.stderr or first.stdout + assert _load_sources(project, settings_rel, sidecar_rel, event_key) == ["_local/myapp"], ( + "First install must produce a single _local/myapp entry; " + f"got {_load_sources(project, settings_rel, sidecar_rel, event_key)}" + ) + # Simulate a legacy install whose _apm_source came from an old checkout basename. - settings = project / ".claude" / "settings.json" - settings.parent.mkdir(parents=True, exist_ok=True) - settings.write_text( - json.dumps( - { - "hooks": { - "PreToolUse": [ - { - "matcher": "Bash", - "hooks": [{"type": "command", "command": "bash .codex/hooks/pre.sh"}], - "_apm_source": "old-checkout-name", - }, - { - "matcher": "Bash", - "hooks": [{"type": "command", "command": "echo user-owned"}], - }, - ] - } - } - ), - encoding="utf-8", + _rewrite_source( + project, + settings_rel, + sidecar_rel, + event_key, + old="_local/myapp", + new="old-checkout-name", ) - result = _run(project, "install") - assert result.returncode == 0, result.stderr or result.stdout + # Append a user-owned entry directly to the settings file (never in the sidecar). + settings_path = project / settings_rel + settings_data = json.loads(settings_path.read_text(encoding="utf-8")) + settings_data.setdefault("hooks", {}).setdefault(event_key, []).append( + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo user-owned"}], + } + ) + settings_path.write_text(json.dumps(settings_data), encoding="utf-8") - entries = json.loads(settings.read_text())["hooks"]["PreToolUse"] - sidecar_path = project / ".claude" / "apm-hooks.json" - sidecar = json.loads(sidecar_path.read_text()) - sidecar_sources = [ - e.get("_apm_source") for e in sidecar.get("PreToolUse", []) if isinstance(e, dict) - ] + # Second install: must heal the stale marker without touching the user-owned entry. + second = _run(project, "install") + assert second.returncode == 0, second.stderr or second.stdout + + sources = _load_sources(project, settings_rel, sidecar_rel, event_key) + assert sources == ["_local/myapp"], ( + f"Expected single _local/myapp entry after heal for {target}, got {sources}" + ) + + settings_data = json.loads(settings_path.read_text(encoding="utf-8")) + entries = settings_data.get("hooks", {}).get(event_key, []) user_owned = [ - e for e in entries if isinstance(e, dict) and e["hooks"][0]["command"] == "echo user-owned" + e + for e in entries + if isinstance(e, dict) + and isinstance(e.get("hooks"), list) + and e["hooks"] + and isinstance(e["hooks"][0], dict) + and e["hooks"][0].get("command") == "echo user-owned" ] - - assert sidecar_sources == ["_local/myapp"], ( - f"Expected single _local/myapp entry in sidecar, got {sidecar_sources}" + assert len(user_owned) == 1, ( + f"User-owned hook entry must survive healing for {target}; entries={entries}" ) - assert len(user_owned) == 1, "User-owned hook entry must survive healing" From 1cec34db730edbe37d9465d1f102c1580320b952 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Thu, 21 May 2026 15:47:21 +0200 Subject: [PATCH 7/8] fix(ci): resolve R0801 duplication; expand lint contract to all 7 CI gates CI lint was failing the PR with R0801 (pylint duplicate-code). The duplication was introduced when main was merged into this branch: the recent MCP-client work on main left an identical `_select_best_package` method in both CodexClientAdapter and CopilotClientAdapter, each ~22 lines long. pylint --fail-on=R0801 catches this at the merge commit CI tests, so the PR cannot pass until the dup is resolved. Fix: hoist `_select_best_package` to the `MCPClientAdapter` base class (alongside the existing `_infer_registry_name` it already calls) and delete the duplicates from codex.py and copilot.py. Both call sites use `self._select_best_package(packages)`, so inheritance resolves the lookup transparently. Mirrors the in-flight refactor on `daf99e0f refactor(adapters): hoist _select_remote_with_url and _select_best_package to base` (not yet on main). Also: expand the canonical lint contract at `.apm/instructions/linting.instructions.md` to cover all seven CI Lint steps (ruff check, ruff format, YAML I/O guard, file-length guard, no raw `str(relative_to)`, pylint R0801, auth-signal lint) instead of only ruff. Adds the full `uv run ... && ...` mirror chain and calls out the merge-commit semantics (CI tests HEAD merged with main, so duplication on main can fail a clean PR diff). Apm compile flows the change into the generated AGENTS.md (gitignored at root) and into `.github/copilot-instructions.md`. Validation: * uv run --extra dev ruff check src/ tests/ -- clean * uv run --extra dev ruff format --check src/ tests/ -- clean * uv run --extra dev python -m pylint --disable=all --enable=R0801 \ --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ -- 10.00/10, exit 0 * bash scripts/lint-auth-signals.sh -- clean * uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py \ tests/integration/test_hook_root_source_drift_e2e.py -- 161 passed Pre-existing main-side test failures in `tests/unit/test_codex_adapter_compatibility.py` (introduced by #1262 / #1277 on main and tracked by the in-flight `32991fb6 fix(adapters): restore _resolve_env_placeholders shim and reject SSE remotes in tests` branch) are out of scope for this PR and will resolve when that follow-up lands on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .apm/instructions/linting.instructions.md | 39 ++++++++++++++++----- .github/copilot-instructions.md | 41 ++++++++++++++++++----- src/apm_cli/adapters/client/base.py | 24 +++++++++++++ src/apm_cli/adapters/client/codex.py | 23 ------------- src/apm_cli/adapters/client/copilot.py | 23 ------------- 5 files changed, 87 insertions(+), 63 deletions(-) diff --git a/.apm/instructions/linting.instructions.md b/.apm/instructions/linting.instructions.md index 9654360c5..32a63a575 100644 --- a/.apm/instructions/linting.instructions.md +++ b/.apm/instructions/linting.instructions.md @@ -10,20 +10,41 @@ report) that claims CI is green. ## CI-mirror commands -The `Lint` job runs: +The `Lint` job runs (see `.github/workflows/ci.yml`): -- `uv run --extra dev ruff check src/ tests/` -- `uv run --extra dev ruff format --check src/ tests/` +1. `uv run --extra dev ruff check src/ tests/` +2. `uv run --extra dev ruff format --check src/ tests/` +3. YAML I/O safety guard (rejects raw `yaml.dump(..., handle)` outside + `utils/yaml_io.py`; mark approved exceptions with `# yaml-io-exempt`). +4. File length guardrail (no `src/**/*.py` may exceed **2450 lines**). +5. No raw `str(path.relative_to(...))` patterns -- use + `portable_relpath()` from `apm_cli.utils.paths`. +6. **Code duplication guardrail (pylint R0801):** + `uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/` +7. Auth-protocol boundary check: `bash scripts/lint-auth-signals.sh` -Both must be silent. +All seven must succeed. CI evaluates these on the **PR merge commit** +(HEAD merged with current `main`), so duplication introduced by a +recent main commit can fail your PR even if your own diff is clean. +Always merge `main` locally before running the mirror. ## Local workflow - **Auto-fix style+imports:** `uv run --extra dev ruff check src/ tests/ --fix` - **Apply formatter:** `uv run --extra dev ruff format src/ tests/` -- **Verify (must be silent):** `uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/` +- **Verify the full Lint job (must all be silent / exit 0):** + ```bash + uv run --extra dev ruff check src/ tests/ \ + && uv run --extra dev ruff format --check src/ tests/ \ + && uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ \ + && bash scripts/lint-auth-signals.sh + ``` + (The YAML, file-length, and `relative_to` guards are pure-grep one-liners + from `ci.yml`; run them directly if you have touched those surfaces.) -Always run the verify pair before `git push` -- the CI Lint job +Always run the verify chain before `git push` -- the CI Lint job fails on any remaining diagnostic. ## Common surprises @@ -36,11 +57,13 @@ fails on any remaining diagnostic. - `F401` / `F841` -- remove unused imports / unused locals. - `SIM103` -- inline negated returns where the body is one line. - `I001` -- import sort order (auto-fixable). +- `R0801` -- 10+ identical lines across two files. Extract the shared + block into a base class / helper module instead of disabling. ## Lifecycle binding This is the canonical lint contract for the repo. Skills that produce artifacts asserting green CI -- notably `pr-description-skill` (whose "Validation evidence" row covers CI checks) -- inherit this -gate transitively. Do NOT redefine ruff commands inside individual -skills; honor this instruction before invoking them. +gate transitively. Do NOT redefine ruff or pylint commands inside +individual skills; honor this instruction before invoking them. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 28118a193..1e6af7f2d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,5 +1,5 @@ - + @@ -11,20 +11,41 @@ report) that claims CI is green. ## CI-mirror commands -The `Lint` job runs: +The `Lint` job runs (see `.github/workflows/ci.yml`): -- `uv run --extra dev ruff check src/ tests/` -- `uv run --extra dev ruff format --check src/ tests/` +1. `uv run --extra dev ruff check src/ tests/` +2. `uv run --extra dev ruff format --check src/ tests/` +3. YAML I/O safety guard (rejects raw `yaml.dump(..., handle)` outside + `utils/yaml_io.py`; mark approved exceptions with `# yaml-io-exempt`). +4. File length guardrail (no `src/**/*.py` may exceed **2450 lines**). +5. No raw `str(path.relative_to(...))` patterns -- use + `portable_relpath()` from `apm_cli.utils.paths`. +6. **Code duplication guardrail (pylint R0801):** + `uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/` +7. Auth-protocol boundary check: `bash scripts/lint-auth-signals.sh` -Both must be silent. +All seven must succeed. CI evaluates these on the **PR merge commit** +(HEAD merged with current `main`), so duplication introduced by a +recent main commit can fail your PR even if your own diff is clean. +Always merge `main` locally before running the mirror. ## Local workflow - **Auto-fix style+imports:** `uv run --extra dev ruff check src/ tests/ --fix` - **Apply formatter:** `uv run --extra dev ruff format src/ tests/` -- **Verify (must be silent):** `uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/` +- **Verify the full Lint job (must all be silent / exit 0):** + ```bash + uv run --extra dev ruff check src/ tests/ \ + && uv run --extra dev ruff format --check src/ tests/ \ + && uv run --extra dev python -m pylint --disable=all --enable=R0801 \ + --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ \ + && bash scripts/lint-auth-signals.sh + ``` + (The YAML, file-length, and `relative_to` guards are pure-grep one-liners + from `ci.yml`; run them directly if you have touched those surfaces.) -Always run the verify pair before `git push` -- the CI Lint job +Always run the verify chain before `git push` -- the CI Lint job fails on any remaining diagnostic. ## Common surprises @@ -37,14 +58,16 @@ fails on any remaining diagnostic. - `F401` / `F841` -- remove unused imports / unused locals. - `SIM103` -- inline negated returns where the body is one line. - `I001` -- import sort order (auto-fixable). +- `R0801` -- 10+ identical lines across two files. Extract the shared + block into a base class / helper module instead of disabling. ## Lifecycle binding This is the canonical lint contract for the repo. Skills that produce artifacts asserting green CI -- notably `pr-description-skill` (whose "Validation evidence" row covers CI checks) -- inherit this -gate transitively. Do NOT redefine ruff commands inside individual -skills; honor this instruction before invoking them. +gate transitively. Do NOT redefine ruff or pylint commands inside +individual skills; honor this instruction before invoking them. --- diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index d5e865539..0ba405c6f 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -248,6 +248,30 @@ def _infer_registry_name(package): return "" + @classmethod + def _select_best_package(cls, packages): + """Select the best package for installation from available packages. + + Prioritizes packages in order: npm, docker, pypi, homebrew, others. + Uses ``_infer_registry_name`` so selection works even when the + registry API returns empty ``registry_name``. + + Args: + packages (list): List of package dictionaries. + + Returns: + dict: Best package to use, or None if no suitable package found. + """ + priority_order = ["npm", "docker", "pypi", "homebrew"] + + for target in priority_order: + for package in packages: + if cls._infer_registry_name(package) == target: + return package + + # If no priority package found, return the first one + return packages[0] if packages else None + @staticmethod def _warn_input_variables(mapping, server_name, runtime_label): """Emit a warning for each ``${input:...}`` reference found in *mapping*. diff --git a/src/apm_cli/adapters/client/codex.py b/src/apm_cli/adapters/client/codex.py index 43794a873..17615db3a 100644 --- a/src/apm_cli/adapters/client/codex.py +++ b/src/apm_cli/adapters/client/codex.py @@ -548,26 +548,3 @@ def _select_remote_with_url(remotes): if url: return remote return None - - def _select_best_package(self, packages): - """Select the best package for installation from available packages. - - Prioritizes packages in order: npm, docker, pypi, homebrew, others. - Uses ``_infer_registry_name`` so selection works even when the - registry API returns empty ``registry_name``. - - Args: - packages (list): List of package dictionaries. - - Returns: - dict: Best package to use, or None if no suitable package found. - """ - priority_order = ["npm", "docker", "pypi", "homebrew"] - - for target in priority_order: - for package in packages: - if self._infer_registry_name(package) == target: - return package - - # If no priority package found, return the first one - return packages[0] if packages else None diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 1769b98ed..778038849 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -1007,29 +1007,6 @@ def _select_remote_with_url(remotes): return remote return None - def _select_best_package(self, packages): - """Select the best package for installation from available packages. - - Prioritizes packages in order: npm, docker, pypi, homebrew, others. - Uses ``_infer_registry_name`` so selection works even when the - registry API returns empty ``registry_name``. - - Args: - packages (list): List of package dictionaries. - - Returns: - dict: Best package to use, or None if no suitable package found. - """ - priority_order = ["npm", "docker", "pypi", "homebrew"] - - for target in priority_order: - for package in packages: - if self._infer_registry_name(package) == target: - return package - - # If no priority package found, return the first one - return packages[0] if packages else None - def _is_github_server(self, server_name, url): """Securely determine if a server is a GitHub MCP server. From 98375e34e57acb463380211aa6d5989c4a44f6de Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Thu, 21 May 2026 13:38:25 +0200 Subject: [PATCH 8/8] fix(adapters): restore _resolve_env_placeholders shim and reject SSE remotes in tests PR #1277 removed the _resolve_env_placeholders legacy wrapper from adapters and softened Codex remote-server rejection (https URLs are now accepted as streamable-http), but did not update the phase-3 and compatibility test suites. The result was 6 pre-existing test failures on Build & Test that this PR's Windows-targeted changes inherited. - Restore _resolve_env_placeholders on MCPClientAdapter as a thin delegate to _resolve_variable_placeholders (empty runtime_vars). External callers and 4 test cases rely on the legacy name. - Update test_returns_false_for_remote_only_server in both codex test files to use an SSE-transport remote, which is still the rejection contract post-#1277 (streamable-http is now accepted). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/adapters/client/base.py | 10 ++++++++++ tests/unit/test_codex_adapter_compatibility.py | 6 +++++- tests/unit/test_codex_adapter_phase3.py | 6 +++++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/apm_cli/adapters/client/base.py b/src/apm_cli/adapters/client/base.py index 0ba405c6f..a4869f27c 100644 --- a/src/apm_cli/adapters/client/base.py +++ b/src/apm_cli/adapters/client/base.py @@ -566,6 +566,16 @@ def _replace_runtime(match): return processed + def _resolve_env_placeholders(self, value, resolved_env): + """Legacy thin wrapper for backward compatibility. + + Kept because external callers and the phase-3 test suite invoke + the pre-#1277 name. Delegates to ``_resolve_variable_placeholders`` + with an empty ``runtime_vars`` map. New code should call + ``_resolve_variable_placeholders`` directly. + """ + return self._resolve_variable_placeholders(value, resolved_env, {}) + # ------------------------------------------------------------------ # Shared server-info helpers (used by all adapter subclasses) # ------------------------------------------------------------------ diff --git a/tests/unit/test_codex_adapter_compatibility.py b/tests/unit/test_codex_adapter_compatibility.py index 844eeaa33..82bea4cb5 100644 --- a/tests/unit/test_codex_adapter_compatibility.py +++ b/tests/unit/test_codex_adapter_compatibility.py @@ -208,7 +208,11 @@ def test_returns_false_when_server_not_found(self, tmp_path: Path) -> None: def test_returns_false_for_remote_only_server(self, tmp_path: Path) -> None: adapter = _make_adapter(project_root=tmp_path) - server_info = {"remotes": [{"url": "https://example.com/sse"}], "packages": []} + # SSE transport is rejected by Codex (streamable-http only). + server_info = { + "remotes": [{"url": "https://example.com/sse", "transport_type": "sse"}], + "packages": [], + } with patch.object(adapter, "_fetch_server_info", return_value=server_info): result = adapter.configure_mcp_server("owner/remote-server") assert result is False diff --git a/tests/unit/test_codex_adapter_phase3.py b/tests/unit/test_codex_adapter_phase3.py index d243eb723..8c851cb9b 100644 --- a/tests/unit/test_codex_adapter_phase3.py +++ b/tests/unit/test_codex_adapter_phase3.py @@ -208,7 +208,11 @@ def test_returns_false_when_server_not_found(self, tmp_path: Path) -> None: def test_returns_false_for_remote_only_server(self, tmp_path: Path) -> None: adapter = _make_adapter(project_root=tmp_path) - server_info = {"remotes": [{"url": "https://example.com/sse"}], "packages": []} + # SSE transport is rejected by Codex (streamable-http only). + server_info = { + "remotes": [{"url": "https://example.com/sse", "transport_type": "sse"}], + "packages": [], + } with patch.object(adapter, "_fetch_server_info", return_value=server_info): result = adapter.configure_mcp_server("owner/remote-server") assert result is False