diff --git a/CHANGELOG.md b/CHANGELOG.md index 95f94bcd2..22c086933 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fixed hook commands using relative paths that break for Claude target (#1310) - Fixed target not propagating to intermediate CompilationConfig during compilation (#765) - Fixed direct GitHub API and ADO/GHES `git ls-remote` calls not respecting `PROXY_REGISTRY_ONLY` mode; all four validation code paths (virtual package, GitHub.com API, ADO/GHES git, and parse-failure fallback) now skip outbound network probes and return `True` when proxy-only mode is active. (#615) - Fixed Claude hook ownership metadata lost due to `additionalProperties` schema restriction: ownership is now stored in a `.claude/apm-hooks.json` sidecar file and re-injected on read, so APM can track which hooks it owns without violating the Claude settings schema. (#1279) diff --git a/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md b/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md index 93f670d95..a2a415a15 100644 --- a/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md +++ b/docs/src/content/docs/producer/author-primitives/hooks-and-commands.md @@ -151,6 +151,13 @@ agent a procedure" fits a skill -- and reaches every harness. - **Script paths.** Use `${PLUGIN_ROOT}` (or the harness-specific alias) for scripts that ship inside the package. Plain absolute paths break on consumers' machines. +- **Claude target path resolution.** When installing for the Claude + target, `apm install` rewrites `${PLUGIN_ROOT}` and relative `./` + references to absolute paths so Claude Code can execute scripts + regardless of the working directory. Scripts that are missing at + install time are replaced with their expected absolute path and a + warning is emitted — the hook is written, but the script will fail + at runtime until the file is present. - **Same `.prompt.md` is two primitives.** A single `.apm/prompts/foo.prompt.md` becomes Copilot's prompt and Claude's `/foo` command in the same install. Name files with both surfaces diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 6bbe9946c..4c8272d88 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -52,7 +52,8 @@ from typing import Dict, List, Optional, Tuple # noqa: F401, UP035 from apm_cli.integration.base_integrator import BaseIntegrator, IntegrationResult -from apm_cli.utils.path_security import ensure_path_within +from apm_cli.utils.console import _rich_warning +from apm_cli.utils.path_security import PathTraversalError, ensure_path_within from apm_cli.utils.paths import portable_relpath _log = logging.getLogger(__name__) @@ -371,6 +372,7 @@ def _rewrite_command_for_target( target: str, hook_file_dir: Path | None = None, root_dir: str | None = None, + deploy_root: Path | None = None, ) -> tuple[str, list[tuple[Path, str]]]: """Rewrite a hook command to use installed script paths. @@ -386,6 +388,11 @@ def _rewrite_command_for_target( target: "vscode" or "claude" hook_file_dir: Directory containing the hook JSON file (for ./path resolution) root_dir: Override root directory (e.g. ".copilot" for user scope) + deploy_root: Absolute root of the deployment directory. When provided, + rewritten script paths are resolved to absolute paths under this + root so the target (e.g. Claude Code) can execute them regardless + of the working directory. When *None*, rewritten paths stay + relative (backward-compatible behaviour). Returns: Tuple of (rewritten_command, list of (source_file, relative_target_path)) @@ -413,7 +420,7 @@ def _rewrite_command_for_target( # Match both forward-slash and backslash separators (Windows hook JSON # may use backslashes: ${CLAUDE_PLUGIN_ROOT}\scripts\scan.ps1) plugin_root_pattern = ( - r"\$\{(?:CLAUDE_PLUGIN_ROOT|CURSOR_PLUGIN_ROOT|PLUGIN_ROOT)\}([\\/][^\s]+)" + r"\$\{(?:CLAUDE_PLUGIN_ROOT|CURSOR_PLUGIN_ROOT|PLUGIN_ROOT)\}([\\/][^\s\"']+)" ) for match in re.finditer(plugin_root_pattern, command): full_var = match.group(0) @@ -421,14 +428,24 @@ def _rewrite_command_for_target( # (on Unix, Path treats backslashes as literal filename chars) rel_path = match.group(1).replace("\\", "/").lstrip("/") - source_file = (package_path / rel_path).resolve() - # Reject path traversal outside the package directory - if not source_file.is_relative_to(package_path.resolve()): + try: + source_file = ensure_path_within(package_path / rel_path, package_path) + except PathTraversalError: continue if source_file.exists() and source_file.is_file(): target_rel = f"{scripts_base}/{rel_path}" scripts_to_copy.append((source_file, target_rel)) - new_command = new_command.replace(full_var, target_rel) + resolved_cmd = ( + str((deploy_root / target_rel).resolve()) + if deploy_root is not None + else target_rel + ) + new_command = new_command.replace(full_var, resolved_cmd) + elif deploy_root is not None: + # File absent: resolve to absolute source path so Claude Code + # gets a clear "file not found" rather than an unexpanded variable. + _rich_warning(f"Hook script not found: {source_file}") + new_command = new_command.replace(full_var, str(source_file)) # Handle relative ./path and .\path references (safe to run after # ${CLAUDE_PLUGIN_ROOT} substitution since replacements produce paths @@ -437,20 +454,30 @@ def _rewrite_command_for_target( # may use backslashes: .\scripts\scan.ps1) # Resolve from hook file's directory if available, else fall back to package root resolve_base = hook_file_dir if hook_file_dir else package_path - rel_pattern = r"(\.[\\/][^\s]+)" + rel_pattern = r"(\.[\\/][^\s\"']+)" for match in re.finditer(rel_pattern, new_command): rel_ref = match.group(1) # Normalize to forward slashes for path resolution rel_path = rel_ref[2:].replace("\\", "/") - source_file = (resolve_base / rel_path).resolve() - # Reject path traversal outside the package directory - if not source_file.is_relative_to(package_path.resolve()): + try: + source_file = ensure_path_within(resolve_base / rel_path, package_path) + except PathTraversalError: continue if source_file.exists() and source_file.is_file(): target_rel = f"{scripts_base}/{rel_path}" scripts_to_copy.append((source_file, target_rel)) - new_command = new_command.replace(rel_ref, target_rel) + resolved_cmd = ( + str((deploy_root / target_rel).resolve()) + if deploy_root is not None + else target_rel + ) + new_command = new_command.replace(rel_ref, resolved_cmd) + elif deploy_root is not None: + # File absent: resolve to absolute source path so the target + # gets a clear "file not found" rather than a bare relative ref. + _rich_warning(f"Hook script not found: {source_file}") + new_command = new_command.replace(rel_ref, str(source_file)) return new_command, scripts_to_copy @@ -462,6 +489,7 @@ def _rewrite_hooks_data( target: str, hook_file_dir: Path | None = None, root_dir: str | None = None, + deploy_root: Path | None = None, ) -> tuple[dict, list[tuple[Path, str]]]: """Rewrite all command paths in a hooks JSON structure. @@ -474,6 +502,10 @@ def _rewrite_hooks_data( target: "vscode" or "claude" hook_file_dir: Directory containing the hook JSON file (for ./path resolution) root_dir: Override root directory (e.g. ".copilot" for user scope) + deploy_root: Absolute root of the deployment directory. When provided, + all rewritten script paths are resolved to absolute paths so the + target can locate scripts regardless of the working directory. + When *None*, paths remain relative (backward-compatible behaviour). Returns: Tuple of (rewritten_data_copy, list of (source_file, target_rel_path)) @@ -501,6 +533,7 @@ def _rewrite_hooks_data( target, hook_file_dir=hook_file_dir, root_dir=root_dir, + deploy_root=deploy_root, ) if scripts: _log.debug( @@ -527,6 +560,7 @@ def _rewrite_hooks_data( target, hook_file_dir=hook_file_dir, root_dir=root_dir, + deploy_root=deploy_root, ) if scripts: _log.debug( @@ -765,6 +799,7 @@ def _integrate_merged_hooks( config.target_key, hook_file_dir=hook_file.parent, root_dir=root_dir, + deploy_root=project_root, ) # Merge hooks into config (additive) diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 793dfe7b3..538468103 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -763,9 +763,12 @@ def extract_commands(text: str) -> set: assert all("_apm_source" not in e for e in stop) return {h["command"] for entry in stop for h in entry["hooks"]} + # Commands are now absolute paths (deploy_root=project_root is threaded + # through _integrate_merged_hooks so Claude Code never sees an unexpanded + # ${CLAUDE_PLUGIN_ROOT} variable). Assert on the absolute form. assert extract_commands(first) == { - ".claude/hooks/multi-stop-pkg/hooks/stop-a.sh", - ".claude/hooks/multi-stop-pkg/hooks/stop-b.sh", + str((temp_project / ".claude/hooks/multi-stop-pkg/hooks/stop-a.sh").resolve()), + str((temp_project / ".claude/hooks/multi-stop-pkg/hooks/stop-b.sh").resolve()), } # Verify sidecar has the ownership info @@ -3268,6 +3271,128 @@ def test_rewrite_partial_variable_no_match(self, tmp_path: Path) -> None: assert cmd == original, "Unknown variable must not be modified" assert scripts == [], "No scripts should be scheduled for copy" + def test_rewrite_command_deploy_root_produces_absolute_path(self, tmp_path: Path) -> None: + """deploy_root parameter makes _rewrite_command_for_target produce absolute paths.""" + pkg_dir = tmp_path / "pkg" + script = pkg_dir / "hooks" / "run.sh" + script.parent.mkdir(parents=True, exist_ok=True) + script.write_text("#!/bin/bash\necho run", encoding="utf-8") + + integrator = HookIntegrator() + deploy_root = Path("/fake/home") + cmd, scripts = integrator._rewrite_command_for_target( + "${CLAUDE_PLUGIN_ROOT}/hooks/run.sh", + pkg_dir, + "my-pkg", + "claude", + deploy_root=deploy_root, + ) + + assert "${CLAUDE_PLUGIN_ROOT}" not in cmd, "Variable must be replaced" + assert len(scripts) == 1, "Script copy entry must be produced" + assert cmd.startswith(str(deploy_root.resolve())), ( + f"Command must be absolute path under deploy_root; got {cmd}" + ) + assert cmd.endswith(".claude/hooks/my-pkg/hooks/run.sh"), ( + f"Command must end with .claude/hooks/my-pkg/hooks/run.sh; got {cmd}" + ) + + def test_rewrite_command_deploy_root_absent_script_resolves_to_source( + self, tmp_path: Path + ) -> None: + """When script is absent and deploy_root is set, use source file absolute path.""" + pkg_dir = tmp_path / "pkg" + pkg_dir.mkdir(parents=True, exist_ok=True) + # Note: NOT creating the script file + + integrator = HookIntegrator() + deploy_root = Path("/fake/home") + cmd, scripts = integrator._rewrite_command_for_target( + "${CLAUDE_PLUGIN_ROOT}/hooks/run.sh", + pkg_dir, + "my-pkg", + "claude", + deploy_root=deploy_root, + ) + + assert "${CLAUDE_PLUGIN_ROOT}" not in cmd, "Variable must be replaced" + assert len(scripts) == 0, "No script copy entry when source is absent" + assert Path(cmd).is_absolute(), "Command must be absolute path" + assert "run.sh" in cmd, "Command must contain the script name" + # Command should resolve to the source path (even if file doesn't exist) + + def test_rewrite_command_no_deploy_root_stays_relative(self, tmp_path: Path) -> None: + """Without deploy_root, command must stay relative (backward compatibility).""" + pkg_dir = tmp_path / "pkg" + script = pkg_dir / "hooks" / "run.sh" + script.parent.mkdir(parents=True, exist_ok=True) + script.write_text("#!/bin/bash\necho run", encoding="utf-8") + + integrator = HookIntegrator() + cmd, scripts = integrator._rewrite_command_for_target( + "${CLAUDE_PLUGIN_ROOT}/hooks/run.sh", + pkg_dir, + "my-pkg", + "claude", + deploy_root=None, # Explicit None for clarity + ) + + assert "${CLAUDE_PLUGIN_ROOT}" not in cmd, "Variable must be replaced" + assert len(scripts) == 1, "Script copy entry must be produced" + assert cmd.startswith(".claude/hooks/my-pkg/"), ( + f"Command must be relative (start with .claude/); got {cmd}" + ) + assert not cmd.startswith("/"), "Command must not be absolute without deploy_root" + + def test_rewrite_command_deploy_root_relative_path_handler(self, tmp_path: Path) -> None: + """deploy_root makes ./path references produce absolute paths too.""" + pkg_dir = tmp_path / "pkg" + script = pkg_dir / "hooks" / "run.sh" + script.parent.mkdir(parents=True, exist_ok=True) + script.write_text("#!/bin/bash\necho run", encoding="utf-8") + + integrator = HookIntegrator() + deploy_root = Path("/fake/home") + cmd, scripts = integrator._rewrite_command_for_target( + "./hooks/run.sh", + pkg_dir, + "my-pkg", + "claude", + hook_file_dir=pkg_dir, # ./hooks resolves relative to pkg_dir + deploy_root=deploy_root, + ) + + assert "./" not in cmd, "Relative ./ reference must be replaced" + assert len(scripts) == 1, "Script copy entry must be produced" + assert cmd.startswith(str(deploy_root.resolve())), ( + f"Command must be absolute path under deploy_root; got {cmd}" + ) + assert not cmd.startswith("./"), "Command must not be relative" + + def test_rewrite_command_nonexistent_script_with_deploy_root(self, tmp_path: Path) -> None: + """When deploy_root is set and script is absent, variable is still resolved (not left expanded).""" + pkg_dir = tmp_path / "pkg" + pkg_dir.mkdir(parents=True, exist_ok=True) + # NOT creating the script file + + integrator = HookIntegrator() + deploy_root = Path("/some/root") + cmd, _ = integrator._rewrite_command_for_target( + "${CLAUDE_PLUGIN_ROOT}/hooks/missing.sh", + pkg_dir, + "my-pkg", + "claude", + deploy_root=deploy_root, + ) + + # With deploy_root, the variable IS resolved to the source file path + # (even though file doesn't exist), so the caller sees a clear "not found" + assert "${CLAUDE_PLUGIN_ROOT}" not in cmd, ( + "Variable must be resolved to source path when deploy_root is set" + ) + assert "missing.sh" in cmd, "Command must contain the script name" + assert Path(cmd).is_absolute(), "Command must be an absolute path (the source file)" + # ------------------------------------------------------------------ # Group C: Event normalisation for Claude # ------------------------------------------------------------------