fix(hooks): anchor POSIX user-scope hook commands to $HOME - #2944
fix(hooks): anchor POSIX user-scope hook commands to $HOME#2944lzx (Bronyal-lzx) wants to merge 2 commits into
Conversation
User-scope installs expanded the installing host's home directory into the merged hook `command` entries, so a `~/.claude/settings.json` kept in a dotfiles repo differed on every machine it was synced to -- and a manually normalized file was rewritten back on the next `apm install -g`. On POSIX hosts, anchor the rewritten path to `$HOME` when the deployed script lives under the deploy root. Hooks run through a shell, which expands `$HOME` at invocation time, so the microsoft#1310 / microsoft#1354 cwd-independence is preserved while the merged file becomes host-independent. Windows keeps the absolute form, and a dynamic target root outside the home directory (for example CLAUDE_CONFIG_DIR) stays absolute too. The idempotent upsert already strips prior entries through their `_apm_source` ownership marker rather than by comparing command strings, so a reinstall converges on the anchored form without duplicating hooks. Fixes microsoft#2821
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate implementation issues remain, along with documentation and changelog follow-ups.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR anchors POSIX user-scope hook commands to $HOME, preserving absolute paths for Windows and external roots.
Changes:
- Updates hook rewriting and scope propagation.
- Adds unit and lifecycle coverage.
- Updates documentation and changelog entries.
- Outstanding fixes remain for quoted
$HOMEexpansion and external-root bundle copying.
File summaries
| File | Summary |
|---|---|
tests/unit/integration/test_hook_integrator.py |
Tests path rewriting across platforms and roots. |
tests/integration/test_hook_js_sidecar_lifecycle_contract.py |
Covers $HOME expansion during lifecycle execution. |
tests/integration/test_hook_integrator_copilot_casing_e2e.py |
Tests scoped Copilot and Kiro paths. |
src/apm_cli/integration/hook_integrator.py |
Implements $HOME-anchored paths; critical (2 votes) single-quote expansion and moderate (1 vote) external-root bundle-copy issues remain. |
src/apm_cli/install/services.py |
Updates user-scope hook handling. |
docs/src/content/docs/reference/targets-matrix.md |
Documents target path behavior. |
docs/src/content/docs/producer/author-primitives/hooks-and-commands.md |
Documents portable hook paths; usage skill documentation also needs updating (nit, 3 votes). |
CHANGELOG.md |
Records the fix; PR number should be appended (nit, 3 votes). |
Review details
Suppressed comments (1)
src/apm_cli/integration/hook_integrator.py:605
- When
CLAUDE_CONFIG_DIRis an absolute path outsideHOME,root_dirand eachtarget_relare absolute. This branch returns an absolute command, butcopy_deployed_hook_bundlelater callsensure_path_within(target_file, project_root)for that same target (hook_bundle.py:201-203), which rejects the script outsideHOME; a real global install containing a hook therefore fails. Support the absolute root through bundle copying and manifest bookkeeping as well, or do not advertise this case as supported.
# A dynamic target root such as CLAUDE_CONFIG_DIR may live
# outside the deploy root. Keep that explicit absolute path
# rather than emitting a misleading $HOME-relative command.
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| rewrites `${PLUGIN_ROOT}` and relative `./` references so Claude Code | ||
| and Copilot CLI can execute scripts regardless of the working | ||
| directory. On POSIX hosts the rewritten path is anchored to `$HOME` | ||
| (for example `$HOME/.claude/hooks/<pkg>/run.sh`), which the shell | ||
| expands at invocation time, so a user-scope config kept in a dotfiles |
lzx (Bronyal-lzx)
changed the title
|
@microsoft-github-policy-service agree |
Review feedback on microsoft#2944: a shell expands $HOME outside single quotes only, so anchoring a single-quoted reference turned `node '${CLAUDE_PLUGIN_ROOT}/hooks/run.mjs'` into a literal '$HOME/.claude/hooks/...' path the target would never expand. Thread the quote character through instead of a boolean and anchor only for unquoted or double-quoted references. Also sync the shipped apm-guide package-authoring resource and the CHANGELOG PR reference.
|
Thanks for the review — all three items are addressed in Single-quoted references (critical). Confirmed, and it was a regression I introduced rather than pre-existing behavior: the old absolute form worked under any quoting, while CHANGELOG PR reference. Fixed — the entry now ends with apm-guide resource. Fixed — Absolute root outside Local re-run after the fix: 195 passed (hook unit suite plus the in-process install-dispatch suite), 5 passed (real-CLI lifecycle contract against a rebuilt binary), ruff clean. The Mode B substantive-line count for this diff is now 22, above the 20-line threshold, so the PR body carries the documented |
There was a problem hiding this comment.
🔵 Needs a closer look
Three unresolved moderate findings remain involving repeated mixed-quote replacements and lifecycle shell-expansion coverage.
Review details
Suppressed comments (3)
src/apm_cli/integration/hook_integrator.py:699
- The quote policy is computed for the current match, but this global replacement rewrites every identical reference in the command. For example,
bash "${CLAUDE_PLUGIN_ROOT}/hooks/run.sh" && bash '${CLAUDE_PLUGIN_ROOT}/hooks/run.sh'makes the second occurrence$HOME/...inside single quotes, where it is literal and the hook fails. Replace only the current occurrence (for example withcount=1, while processing matches in order) so each reference keeps its own quote semantics.
_wrapping_quote(command, match),
)
new_command = new_command.replace(full_var, resolved_cmd)
src/apm_cli/integration/hook_integrator.py:749
- The same global replacement issue applies to repeated relative references with mixed quoting. If an unquoted/double-quoted
./hooks/run.shappears before a single-quoted copy, this replaces both with$HOME/..., leaving the latter as a literal path and breaking that hook. Limit the replacement to the current match so the single-quoted occurrence remains absolute.
_wrapping_quote(command, match),
)
new_command = new_command.replace(rel_ref, resolved_cmd)
tests/integration/test_hook_js_sidecar_lifecycle_contract.py:731
- This lifecycle test replaces
$HOMEin the parsed argv before invoking the hook, so it never exercises the shell expansion that makes the new representation executable. A regression that writes a literal$HOMEpath or invokes this command without a shell would still pass here; run the generated command through the same POSIX shell with the isolatedHOME(and keep a platform-appropriate direct invocation for Windows) so this contract is actually covered.
hook_argv = [arg.replace("$HOME", str(scenario.isolated.home)) for arg in command[1:]]
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
fix(hooks): anchor POSIX user-scope hook commands to $HOME
TL;DR
User-scope installs (
apm install -g) expanded the installing machine's homedirectory into merged hook commands, so a
~/.claude/settings.jsonkept in adotfiles repo differed on every host and hand-normalizing it was undone by the
next install. On POSIX hosts the rewritten path is now anchored to
$HOME,which the invoking shell expands, so cwd-independence is unchanged. Windows
keeps absolute paths, and so do single-quoted references and a dynamic target
root outside the home directory.
Note
Fixes #2821. Only the form of the user-scope path changes; the merge,
ownership, and cleanup behavior is untouched.
Problem (WHY)
apm install -gembedded the installing host's home prefix in merged hook commands:"command": "node \"/Users/alice/.claude/hooks/context-mode/hooks/pretooluse.mjs\"".~/.claude/settings.jsonis commonly tracked in a dotfiles repo, so the same hooks produced a host-specific diff on every machine — the install is reproducible, the tracked file is not.Why these matter: the absolute rewrite itself is required — hook commands
resolve against the working directory, not against the settings file:
The repo's own authoring guidance says the same thing about tracked files:
Approach (WHAT)
deploy_rootbranch (populated for user scope only), emit$HOME/<path-relative-to-deploy-root>instead ofstr((deploy_root / target_rel).resolve()). The deploy root isPath.home()for user scope, so the substitution is exact rather than heuristic.relative_to(deploy_root)succeeds. A target root that resolves outside the deploy root —CLAUDE_CONFIG_DIRpointing elsewhere — keeps the previous absolute path instead of gaining a$HOMEprefix that would point at the wrong location._POSIX_USER_HOOK_PATHS = os.name != "nt") so Windows keeps the current form and tests can drive both branches deterministically.path_is_quotedbefore, and it still does not, so no command string changes shape beyond its prefix.$HOMEoutside single quotes only, so a single-quoted reference keeps the previous absolute form instead of handing the target a literal'$HOME/…'path. The existing quote detection now reports the quote character rather than a boolean (_wrapping_quote), which is the whole cost of this guard.Implementation (HOW)
Diff for line-level evidence:
https://github.com/microsoft/apm/pull/2944/files
src/apm_cli/integration/hook_integrator.py— the anchoring itself, in_project_scoped_command_path(L606-L625), the quote-character helper_wrapping_quote(L123) that replaced the inline boolean, plus two docstring lines that described the old absolute contract. Nothing else in the module changed: the missing-script warning path, the ownership markers, and the script-copy plan are deliberately untouched.src/apm_cli/install/services.py— comment only. Records that the user-scope signal now produces cwd-independent,$HOME-anchored paths.src/apm_cli/integration/…tests —tests/unit/integration/test_hook_integrator.pyre-expresses the user-scope expectations as the$HOMEform and adds two boundary cases (Windows keeps absolute, dynamic root outside home stays absolute).tests/integration/test_hook_integrator_copilot_casing_e2e.py— Copilot and Kiro user-scope branches, since both consume the same rewrite decision.tests/integration/test_hook_js_sidecar_lifecycle_contract.py— the global Copilot lifecycle contract asserted an absolute path and executed it directly. The real harness launches hooks through a shell, so the test now expands$HOMEfrom the isolated home before asserting and executing.docs/.../hooks-and-commands.md,docs/.../targets-matrix.md— describe the anchoring instead of "absolute paths", including the late-resolution caveat.CHANGELOG.md— entry underFixed, matching how [BUG] apm install --target claude writes ${CLAUDE_PLUGIN_ROOT} into ~/.claude/settings.json where Claude Code refuses to expand it #1310 / fix: resolve hook paths to absolute in settings.json for --target claude #1354 and [BUG] 0.14.0: project-scope hook paths rewritten to absolute, breaking portability of checked-in .claude/settings.json and .codex/hooks.json #1394 were recorded.Trade-offs
$HOMEover leaving the absolute path and documenting a dotfiles workaround. Rejected: telling users to post-process a file APM owns — the rewrite would still lose on the next install, as [FEATURE] Anchor user-scope hook commands to $HOME on POSIX targets so ~/.claude/settings.json stays portable across machines #2821 reports.%USERPROFILE%) in this PR, because its cmd/PowerShell semantics differ and Project-scope hook path rewriting produces cwd-dependent commands on Windows (Claude Code target) #2408 shows cwd handling is already special there.$HOME/...there, which would resolve to a path that does not contain the deployed script._project_scoped_command_pathis shared by every merge-based target. Rejected: restricting toclaude/codexwithout maintainer input — it is a one-line condition if you prefer that.mmdc, which is not available in this environment, so an unvalidated diagram would be worse than none.Benefits
apm install -gon such a repo produces no diff instead of rewriting every hook entry.Validation
pytest tests/unit/integration/test_hook_integrator.py -q -n0pytest tests/integration/test_hook_js_sidecar_lifecycle_contract.py -q -n0— real CLI, locally builtapmbinary, isolatedHOME:ruff check src/ tests/andruff format --check src/ tests/(both must be silent in CI):Baseline comparison, because this machine cannot run the full CI matrix:
Full unit suite and lifecycle-smoke selection, branch vs. an untouched baseline checkout using the same binary
Every failure on both sides is a pre-existing environment limitation here:
no
ghCLI, nopwsh, sandboxedptyallocation, and network-dependentcontracts.
Scenario Evidence
apm install -gon a POSIX host — the tracked settings file holds a$HOME-anchored command instead of the installing host's prefix (regression trap for #2821)tests/unit/integration/test_hook_integrator.py::TestClaudeIntegration::test_user_scope_writes_portable_home_hook_pathstests/integration/test_hook_js_sidecar_lifecycle_contract.py::test_required_global_copilot_sidecar_lifecycleapm uninstall --globalremoves exactly the package-owned filestests/integration/test_hook_js_sidecar_lifecycle_contract.py::test_required_global_copilot_sidecar_lifecycletests/integration/test_hook_integrator_copilot_casing_e2e.py::test_copilot_install_scope_controls_script_paths[True],::test_kiro_install_scope_controls_script_paths[True]tests/unit/integration/test_hook_integrator.py::TestIssue1007Fixes::test_rewrite_command_deploy_root_keeps_windows_absolute_pathCLAUDE_CONFIG_DIRoutside the home directory — the command is never rewritten into a wrong$HOMEpathtests/unit/integration/test_hook_integrator.py::TestIssue1007Fixes::test_rewrite_command_dynamic_root_outside_home_stays_absolute$HOMEtests/unit/integration/test_hook_integrator.py::TestIssue1007Fixes::test_rewrite_command_single_quoted_reference_stays_absoluteHow to test
uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py -q -n0→188 passed, including the three boundary cases (Windows, single-quoted reference, root outside home).uv run --extra dev pytest tests/integration/test_hook_integrator_copilot_casing_e2e.py -q -n0→ passes for bothuser_scopeparametrizations of Copilot and Kiro.scripts/build-binary.sh) and runAPM_BINARY_PATH=… APM_E2E_TESTS=1 uv run --extra dev pytest tests/integration/test_hook_js_sidecar_lifecycle_contract.py -q -n0→5 passed.uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/→ both silent.HOME=/tmp/scratch apm install --global --target claude <pkg>thengrep -o '\$HOME[^"]*' /tmp/scratch/.claude/settings.json→ prints$HOME/.claude/hooks/…and never/tmp/scratch/....Review feedback addressed
Responding to the Copilot review on
6331f22:hook_integrator.py:613) — correct, and this was a regression I introduced, not a pre-existing one: the previous absolute form worked inside any quoting.node '${CLAUDE_PLUGIN_ROOT}/hooks/run.mjs'producednode '$HOME/.claude/hooks/…', where the shell expands nothing. Anchoring now requires an unquoted or double-quoted reference; the quote character is threaded through instead of a boolean, and a regression test asserts the single-quoted form stays absolute and executable.(#2944).packages/apm-guide/.apm/skills/apm-usage/package-authoring.mdstill documents absolute paths — fixed: that resource now describes the$HOMEanchor and its three exceptions.hook_integrator.py:605) — verified as pre-existing rather than introduced here. On an untouchedmaincheckout, the same input produces the same absolute command and the same absolute copy target from this branch, so theensure_path_withinrejection incopy_deployed_hook_bundleis independent of this diff. This PR neither toucheshook_bundle.pynor changes that path's behavior; happy to file or take a follow-up that supports an absolute root end to end.Type of change
Testing
Spec conformance (OpenAPM v0.1)
The Mode B detector counts substantive added lines under
src/apm_cli/integration/;this diff is at 22, above the 20-line threshold, so it carries the documented
waiver rather than a spec citation. No existing
req-XXXcovers user-scopehook anchoring, and inventing one for a path-spelling change would put
implementation detail into the spec:
apm-spec-waiver: user-scope hook anchoring changes path spelling only, adding no new OpenAPM requirement
If you would rather make user-scope anchoring normative, I am happy to add the
anchor + manifest row + conformance test instead and drop the waiver.