fix: resolve hook paths to absolute in settings.json for --target claude#1354
fix: resolve hook paths to absolute in settings.json for --target claude#1354sergio-sisternes-epam wants to merge 2 commits into
Conversation
When apm install --target claude deploys hooks, the emit path wrote
${CLAUDE_PLUGIN_ROOT} or relative paths into ~/.claude/settings.json.
Claude Code only expands that variable inside a plugin's hooks/hooks.json,
not in settings.json, leaving every APM-integrated hook as dead config.
Thread deploy_root (= project_root) through _rewrite_hooks_data and
_rewrite_command_for_target so merged-hook commands are resolved to
absolute paths at install time. The Copilot path (integrate_package_hooks)
is unchanged -- it does not pass deploy_root and keeps relative paths.
Fixes microsoft#1310
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
This PR updates hook command rewriting so merged hook configurations can emit absolute script paths, addressing Claude settings not expanding plugin-root variables.
Changes:
- Adds
deploy_rootthreading through hook rewrite helpers. - Uses
deploy_rootin merged hook integration to resolve deployed script paths. - Updates and adds hook integrator unit tests for absolute-path behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/apm_cli/integration/hook_integrator.py |
Adds deploy-root-aware command rewriting and passes project root through merged hook integration. |
tests/unit/integration/test_hook_integrator.py |
Updates existing expectations and adds tests for deploy-root command rewriting behavior. |
Comments suppressed due to low confidence (4)
tests/unit/integration/test_hook_integrator.py:3003
- This test assumes absolute paths start with
/, which is false for normal Windows paths such asC:\.... Since the project runs tests on Windows, this should usePath(cmd).is_absolute()or another platform-neutral check.
assert cmd.startswith("/"), "Command must be absolute path"
assert "run.sh" in cmd, "Command must contain the script name"
tests/unit/integration/test_hook_integrator.py:3051
- This assertion is POSIX-specific and will fail on Windows for the same reason as the earlier deploy-root test: the resolved path is not guaranteed to stringify with
/fake/home/separators. Assert viaPathcomponents or normalize the path before comparing.
assert cmd.startswith("/fake/home/"), (
f"Command must be absolute path under deploy_root; got {cmd}"
)
tests/unit/integration/test_hook_integrator.py:3076
- This is another POSIX-only absolute-path check. On Windows, an absolute source path normally starts with a drive letter, so this assertion can fail even though the command is absolute.
assert "missing.sh" in cmd, "Command must contain the script name"
assert cmd.startswith("/"), "Command must be an absolute path (the source file)"
src/apm_cli/integration/hook_integrator.py:441
- The new
deploy_rootparameter is not documented in this method's Args section. Since this method now controls whether merged hook commands are emitted as absolute paths, the docstring should describe the parameter's effect.
deploy_root: Path | None = None,
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Minimal, correct parameter threading; one DRY opportunity in the duplicate absent-file branch; overall shape is sound for this scope. |
| CLI Logging Expert | 0 | 1 | 1 | Fix is silent on the absent-script fallback path -- a hook command pointing at a missing file is written into settings.json with no warning emitted to the user. |
| DevX UX Expert | 0 | 1 | 1 | Fix converts silent dead-hook installs into working absolute-path config for --target claude; UX promise restored. One UX gap: absent scripts silently write a non-existent path instead of warning the user. |
| Supply Chain Security Expert | 0 | 1 | 1 | No blocking issues; one recommended guard: use ensure_path_within() instead of raw .resolve() to maintain the architectural chokepoint contract. |
| OSS Growth Hacker | 0 | 2 | 1 | Silent hook failure for Claude Code users is a high-impact adoption blocker fixed here; deserves a prominent CHANGELOG entry and Claude Code integration story. |
| Doc Writer | 0 | 2 | 1 | CHANGELOG missing a fix entry; hooks-and-commands.md needs a note that claude target writes absolute paths at install time -- current pitfall wording conflicts with the new behavior. |
| Test Coverage Expert | 0 | 1 | 1 | 5 unit tests cover _rewrite_command_for_target; absolute-path assertion in unit fixtures confirms new logic. No integration-with-fixtures test exercises apm install --target claude end-to-end. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add integration-with-fixtures test asserting hook commands are absolute after
apm install --target claude-- The critical user promise (absolute paths in settings.json) is only covered at unit tier. A silent threading regression -- deploy_root not reaching the rewrite path -- would pass all 8503 existing tests. outcome: missing on a critical hook surface ranks above all recommended opinion findings. - [CLI Logging Expert] Emit
_rich_warningand skip entry when referenced script is absent at install time -- Three independent panelists converged on this: absent-script fallback silently writes a guaranteed-broken install-time path into settings.json. User gets a dead hook with zero signal at install time and a cryptic error at runtime. One-line fix in the branch this PR already touches. - [Supply Chain Security Expert] Replace raw
.resolve()withensure_path_within(deploy_root / target_rel, deploy_root)-- Maintains the architectural chokepoint invariant at negligible cost. Actual risk is low (target_rel already gated), but the invariant exists precisely to guard against future code paths that skip the upstream gate. - [Doc Writer] Add CHANGELOG
[Unreleased]entry and updatehooks-and-commands.mdPitfalls section -- Users with pre-fix installs need to know to re-runapm install --target claude; the current docs Pitfalls section describes the pre-fix (broken) behavior. Both oss-growth-hacker and doc-writer flagged this independently -- must land before release cut. - [Python Architect] Log warning and omit the entry (instead of writing source_file) when script is absent -- Complements the cli-logging-expert warning finding: surviving the raw
${CLAUDE_PLUGIN_ROOT}/...token is safer than writing a path guaranteed to not exist at runtime.
Architecture
classDiagram
direction LR
class BaseIntegrator {
<<AbstractBase>>
+check_collision()
+is_content_identical_to_source()
}
class HookIntegrator {
<<ConcreteIntegrator>>
+find_hook_files()
+integrate_package_hooks()
-_integrate_merged_hooks()
-_rewrite_hooks_data()
-_rewrite_command_for_target()
-_parse_hook_json()
}
class _MergeHookConfig {
<<ValueObject>>
+target_key str
+config_filename str
+require_dir bool
}
class HookIntegrationResult {
<<ValueObject>>
+files_integrated int
+scripts_copied int
+target_paths list
}
class PackageInfo {
<<ValueObject>>
+install_path Path
}
BaseIntegrator <|-- HookIntegrator
HookIntegrator ..> _MergeHookConfig : reads
HookIntegrator ..> HookIntegrationResult : returns
HookIntegrator ..> PackageInfo : reads
class HookIntegrator:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install --target claude"] --> B["_integrate_merged_hooks"]
B --> C["find_hook_files\n[FS] scan install_path"]
B --> D["_parse_hook_json\n[I/O] read hooks/*.json"]
D --> E["_rewrite_hooks_data\ndeploy_root=project_root NEW"]
E --> F["_rewrite_command_for_target\ndeploy_root: Path|None NEW"]
F --> G{"source_file exists?"}
G -- "yes + deploy_root" --> H["deploy_root / target_rel .resolve()\nabsolute path into settings.json"]
G -- "yes, no deploy_root" --> I["target_rel relative -- Copilot path unchanged"]
G -- "no + deploy_root" --> J["str(source_file) fallback\n[see followup #2]"]
H --> K["new_command with absolute path"]
I --> K
J --> K
K --> L["merge into ~/.claude/settings.json"]
E --> M["scripts_to_copy list"]
M --> N["shutil.copy2 script files"]
Recommendation
Ship this PR. The core fix is correct, minimal, and proven. No panelist raised a blocking finding. The four followups above are real improvements -- particularly the missing integration test and the absent-script warning -- but none of them are regressions introduced by this PR, and holding the fix for them would extend the window in which Claude Code users have silently broken hooks. Land the CHANGELOG entry and docs update in this PR or as an immediate follow-on before the release cut. Track the integration test and ensure_path_within guard as dedicated followup issues.
Full per-persona findings
Python Architect
- [nit] Duplicate absent-file fallback branches can be extracted into a helper at
src/apm_cli/integration/hook_integrator.py:395
The same 3-line pattern appears twice inside_rewrite_command_for_target. A tiny inline helper would make future changes to the fallback policy a single-site edit instead of two.
Suggested: Extract a_resolve_cmd(deploy_root, target_rel, source_file, fallback_to_source=True) -> strhelper. - [nit]
deploy_rootmissing from_rewrite_command_for_targetdocstring Args block atsrc/apm_cli/integration/hook_integrator.py:331
The new parameter is undocumented in the docstring; the_rewrite_hooks_datadocstring also omits it.
Suggested: Adddeploy_root: Optional base path for absolute resolution; when set, target paths are resolved to absolute paths (used by Claude target). - [recommended] Absent-file fallback emits
source_file(install-time path) into settings.json -- semantically wrong for a deployed config atsrc/apm_cli/integration/hook_integrator.py:395
When a script file does not exist at install time, the fallback writes the install-timesource_fileabsolute path into~/.claude/settings.json. That path may be inside a package cache or temp dir. Claude Code will attempt to execute a stale path.
Suggested: Log a_log.warning(...)andcontinueso the raw${CLAUDE_PLUGIN_ROOT}/...token survives -- Claude Code error is then clearly attributed to an unresolved variable, not a stale path.
CLI Logging Expert
- [recommended] Absent-script fallback emits no warning to the user at
src/apm_cli/integration/hook_integrator.py
When a referenced script file does not exist, the code silently rewrites the hook command to an absolute source path and continues. The user gets a dead hook insettings.jsonwith zero signal at install time.
Suggested: After the elif block, add_log.warning('Hook script not found at install time: %s -- hook command written with absolute source path; hook may fail at runtime', source_file). - [nit] Inline comment tense is misleading at
src/apm_cli/integration/hook_integrator.py
The comment says 'gets a clear file not found' but that only happens at Claude Code runtime, not at install time.
Suggested: Reword to:# File absent at install time: write absolute source path so runtime error is deterministic rather than an unexpanded variable or relative ref.
DevX UX Expert
- [recommended] Absent-script fallback writes a non-existent absolute path silently -- user gets no feedback at install time at
src/apm_cli/integration/hook_integrator.py
When a referenced script does not exist at install time, code writesstr(source_file)intosettings.json. Claude Code will later fail with a cryptic 'file not found'. Violates the 'failure mode is the product' principle.
Suggested: Emit a_rich_warningat install time and consider skipping the entry entirely rather than writing a guaranteed-broken path. - [nit] The Copilot path divergence (relative vs absolute) is undocumented in CLI help or user-facing output
A user who runsapm install --target copilotafter reading about the claude fix may be confused why hooks still use relative paths. This asymmetry is intentional but invisible.
Suggested: Add a note to the--targetflag description in cli-commands.md distinguishing path resolution behavior betweenclaudeandcopilottargets.
Supply Chain Security Expert
- [recommended] Raw
.resolve()on(deploy_root / target_rel)bypasses theensure_path_withinchokepoint atsrc/apm_cli/integration/hook_integrator.py
ensure_path_within()is the single sanctioned containment predicate. The new code calls(deploy_root / target_rel).resolve()directly. Ifdeploy_rootcontains a symlink pointing outside the tree,.resolve()will silently follow it. Actual risk is low becausetarget_relalready passed theis_relative_to(package_path)gate, but the architectural invariant is violated.
Suggested: Replacestr((deploy_root / target_rel).resolve())withstr(ensure_path_within(deploy_root / target_rel, deploy_root))(import fromapm_cli.utils.path_security). - [nit] Absent-script fallback writes without a warning
Same signal as cli-logging-expert and devx-ux-expert: user gets dead config with no install-time signal.
Suggested: Addlogger.warning('Hook script not found at install time, path written to settings.json may be unusable: %s', source_file)in the elif branch.
OSS Growth Hacker
- [recommended] CHANGELOG entry is missing
Every APM-integrated hook was silently dead for--target claudeusers. Fixing it is a high-signal trust moment that must surface in release notes so upgrading users know to re-runapm install --target claude.
Suggested: Add under### Fixedin[Unreleased]: "apm install --target claudenow resolves hook script paths to absolute paths in.claude/settings.json-- previously relative paths were written, causing Claude Code to silently ignore all installed hooks. Re-runapm install --target claudeto activate. (fix: resolve hook paths to absolute in settings.json for --target claude #1354)" - [recommended] No user-facing callout that existing installs need
apm install --target claudere-run
Users who installed before this fix have a brokensettings.jsonright now. They won't know to re-run.
Suggested: At minimum, add a bold migration note in the release post. Ideally, emit a warning atapm installorapm runtime when stale relative paths are detected insettings.json. - [nit] PR body buries user impact -- lead with 'hooks were silently dead' not the internal variable name
For CHANGELOG and release notes, the user-facing story is far more compelling and searchable.
Auth Expert -- inactive
The PR only modifies hook_integrator.py and test_hook_integrator.py to resolve hook script paths to absolute paths at install time -- no auth behavior, token management, credential resolution, host classification, or remote-host fallback semantics are touched.
Doc Writer
- [recommended] CHANGELOG
[Unreleased]has no entry for this bug fix atCHANGELOG.md
Every other fix in[Unreleased]is documented in detail. Silent dead-config hooks forapm install --target claudeis a user-visible bug that users may have been working around; they deserve to know it is fixed and when.
Suggested: Add under### Fixedin[Unreleased]: "apm install --target claudenow resolves hook script paths to absolute paths in.claude/settings.json; previously relative paths were written, causing Claude Code to silently ignore all installed hooks. (fix: resolve hook paths to absolute in settings.json for --target claude #1354)" - [recommended]
hooks-and-commands.mdPitfalls section conflicts with new absolute-path behavior atdocs/src/content/docs/producer/author-primitives/hooks-and-commands.md
The doc currently implies relative paths land insettings.json(the pre-fix broken behavior). After this fix, the claude target always writes absolute paths. A reader debugging cross-machine behavior needs to know this.
Suggested: Add: 'For theclaudetarget, APM resolves${PLUGIN_ROOT}and${CLAUDE_PLUGIN_ROOT}to absolute paths at install time so Claude Code can locate scripts regardless of working directory.' In the Pitfalls section, tighten to: 'Do not write absolute paths in your hook source files -- they break on other machines. Use${PLUGIN_ROOT}instead; APM resolves it to an absolute path for claude and a target-relative path for other harnesses.' - [nit]
targets-matrix.mdclaude hooks row omits path-format detail atdocs/src/content/docs/reference/targets-matrix.md
Low urgency sincehooks-and-commands.mdis the authoritative page.
Suggested: Append '(absolute paths)' to the claude hooks row note.
Test Coverage Expert
- [recommended] No integration-with-fixtures test asserts absolute paths survive
apm install --target claudeend-to-end attests/integration/test_local_install.py
The critical user promise -- hooks deployed viaapm install --target claudecontain absolute paths insettings.json-- is only exercised at unit tier. No test invokes the apm install CLI and readssettings.jsonto assert every command starts with/. A silent threading regression (deploy_root not reaching the rewrite path) would pass all 8503 existing tests. Probed: grep'dtests/integration/forCLAUDE_PLUGIN_ROOT,absolute, anddeploy_root-- no hit asserting command path shape in a CLI-invocation test.
Proof (missing):tests/integration/test_local_install.py::test_hook_commands_are_absolute_after_install-- proves: apm install --target claude writes absolute command paths into settings.json so Claude Code can execute hooks without unexpanded variables [multi-harness-support]
assert all(cmd.startswith('/') for cmd in extract_hook_commands(settings_json)), 'hook commands must be absolute paths' - [nit] Unit test could also assert
${CLAUDE_PLUGIN_ROOT}absence independently attests/unit/integration/test_hook_integrator.py
test_rewrite_command_no_deploy_root_stays_relativeassertscmd.startswith('.claude/hooks/my-pkg/')but a separate assertion on${CLAUDE_PLUGIN_ROOT}absence is a stronger regression trap.
Proof (passed):tests/unit/integration/test_hook_integrator.py::test_rewrite_command_no_deploy_root_stays_relative-- proves: without deploy_root, relative paths are preserved for backward compatibility [multi-harness-support]
assert cmd.startswith('.claude/hooks/my-pkg/'), f'Command must not be absolute without deploy_root'
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1354 · ● 2M · ◷
github-actions
Bot
removed
the
panel-review
- Regex: exclude trailing quote chars from plugin-root and rel-path capture groups ([^\s]+ → [^\s"']+) so os.path.exists() receives clean paths (B1+B2) - Path security: replace raw Path.resolve()+is_relative_to() with ensure_path_within() in _rewrite_command_for_target for traversal protection (Panel R2) - Warning: emit _rich_warning() when a hook script is absent at install time so broken paths surface immediately (Panel R1) - Docstrings: document deploy_root parameter in _rewrite_command_for_target and _rewrite_hooks_data (N1) - Tests: use str(deploy_root.resolve()) and Path(cmd).is_absolute() instead of hardcoded /fake/home/ prefix so assertions are Windows-portable (Copilot R1) - CHANGELOG: add Fixed entry for microsoft#1310 (Panel R3) - Docs: add Claude target path-resolution pitfall note to hooks-and-commands.md (Panel R4)
Panel feedback addressed in
|
| Finding | Status | Fix |
|---|---|---|
| [B] Regex trailing quote bug in quoted hook commands | Fixed | Updated capture group boundary to exclude trailing quotes |
[B] Path security: ensure_path_within guard missing |
Fixed | Added ensure_path_within() validation for deploy_root paths |
| [R] Windows-portable test assertions | Fixed | Replaced POSIX string prefix checks with pathlib parts/is_absolute |
| [R] Absent-script warning not raised | Fixed | Added warning when hook script not found at source path |
| [R] Missing CHANGELOG entry | Fixed | Added entry under [Unreleased] ### Fixed |
| [R] Docs update for hooks pitfalls | Fixed | Updated hooks-and-commands.md with absolute-path pitfall note |
[N] deploy_root docstring params undocumented |
Fixed | Added Args section entries for both methods |
All Copilot review threads resolved. CI green, lint clean.
Description
When
apm install --target claudedeploys hooks, the emit path wrote${CLAUDE_PLUGIN_ROOT}or relative paths into~/.claude/settings.json. Claude Code only expands that variable inside a plugin's ownhooks/hooks.json, not insettings.json, so every APM-integrated hook was dead config.Thread
deploy_root(=project_root) through_rewrite_hooks_dataand_rewrite_command_for_targetinhook_integrator.pyso merged-hook commands are resolved to absolute paths at install time._rewrite_command_for_targetdeploy_root: Path | None = Noneparam; resolves(deploy_root / target_rel).resolve()when set_rewrite_hooks_datadeploy_rootto both inner call sites_integrate_merged_hooksdeploy_root=project_rootThe Copilot path (
integrate_package_hooks) is unchanged -- it does not passdeploy_rootand keeps relative paths.Fixes #1310
Type of change
Testing
5 new unit tests covering: absolute path resolution, absent-script fallback, backward-compat (no deploy_root), relative-path handler, and deploy_root vs no-deploy_root distinction. All 129 hook integrator tests pass. Full suite: 8503 passed. Lint clean (
ruff check+ruff format --check).