fix(hooks): keep project-scope hook commands repo-relative (closes #1394)#1396
Open
srid wants to merge 2 commits into
Open
fix(hooks): keep project-scope hook commands repo-relative (closes #1394)#1396srid wants to merge 2 commits into
srid wants to merge 2 commits into
Conversation
…crosoft#1394) microsoft#1354 unconditionally threaded ``deploy_root=project_root`` through ``_integrate_merged_hooks`` to absolutize hook ``command`` paths. That fix was correct for user-scope (``~/.claude/settings.json`` is read without a fixed cwd, and ``${CLAUDE_PLUGIN_ROOT}`` is not expanded inside ``settings.json`` -- the bug microsoft#1310 was about), but it also rewrote project-scope ``<repo>/.claude/settings.json`` / ``<repo>/.codex/hooks.json`` / sidecar ``apm-hooks.json`` -- files that are typically checked into the repo. Every clone / contributor / CI runner then saw the installer's machine-local absolute prefix baked into committed config. Restrict the absolute-path rewrite to user-scope deploys by gating on ``project_root.resolve() == Path.home().resolve()``. The ``InstallScope.USER`` branch lands ``project_root`` at ``Path.home()`` (see ``apm_cli.core.scope.get_deploy_root``), so the home-equality check is a faithful, side-effect-free proxy without threading a scope kwarg through the integrator stack. ``CLAUDE_CONFIG_DIR`` remains honored upstream of this check -- it redirects the deploy *directory* (``target.root_dir``), not ``project_root`` itself. The behaviour applies to all targets that merge into a single JSON config (Claude / Codex / Cursor / Gemini / Windsurf) since they all flow through ``_integrate_merged_hooks``. Tests: - ``test_project_scope_writes_relative_hook_paths`` (regression for microsoft#1394): asserts the rewritten command is repo-relative and contains no absolute prefix. - ``test_user_scope_still_writes_absolute_hook_paths`` (preserves microsoft#1310 / microsoft#1354): patches ``Path.home`` to the deploy root and asserts the rewritten command is absolute, matching the original ``deploy_root`` resolution. - ``test_codex_project_scope_keeps_relative_hook_paths`` mirrors the Claude regression test through the Codex public entry point so the shared scope check is asserted from a second target. - ``test_reinstall_preserves_multiple_hook_files_same_event`` updated to assert relative commands (its prior absolute-form assertion was documenting the regression).
srid
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Restricts the absolute-path rewriting of hook commands introduced in #1354 to user-scope deploys only, preserving repo-relative paths in project-scope hook configs that are typically checked into version control.
Changes:
- Gate
deploy_rootthreading in_integrate_merged_hookson aproject_root == Path.home()check to detect user-scope deploys. - Add regression tests for both project-scope (relative paths) and user-scope (absolute paths) behavior across Claude and Codex targets.
- Update an existing test that was documenting the regression to assert the corrected relative-path behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/apm_cli/integration/hook_integrator.py | Adds user-scope detection via home-directory equality check; only passes deploy_root when user-scope. |
| tests/unit/integration/test_hook_integrator.py | Adds regression tests for project-scope (relative) and user-scope (absolute) command rewriting; updates prior assertion. |
| CHANGELOG.md | Documents the fix under Unreleased / Fixed. |
Comment on lines
+737
to
+746
| # Absolutize hook commands only for user-scope deploys. Claude | ||
| # Code (and the Codex/Cursor/Gemini equivalents) reads | ||
| # ``~/.claude/settings.json`` without a fixed cwd and does not | ||
| # expand ``${CLAUDE_PLUGIN_ROOT}`` in that file (see #1310 / #1354), | ||
| # so user-scope deploys must write absolute paths. Project-scope | ||
| # ``<repo>/.claude/settings.json`` is typically checked in and runs | ||
| # with cwd at the repo root, where repo-relative paths resolve | ||
| # correctly -- baking absolute machine paths into checked-in config | ||
| # breaks portability across clones, contributors, and CI (#1394). | ||
| _is_user_scope = project_root.resolve() == Path.home().resolve() |
Contributor
Author
There was a problem hiding this comment.
Addressed in b576258 -- replaced the project_root == Path.home() heuristic with an explicit user_scope: bool kwarg threaded from services.integrate_package_primitives (where the InstallScope is already in scope).
Concrete answers to the three concerns:
$HOME-as-project misclassification: gone. User-scope is now driven byscope is InstallScope.USER, not deploy-root shape.- Implicit coupling to
get_deploy_root: gone. If user-scope deploys later move to~/.config/..., the gate still holds because it doesn't inspectproject_rootat all. - Inference vs intent: now explicit at the call site. Sibling integrators (prompt/agent/command/instruction) keep their existing signatures -- the
user_scopekwarg is added only tointegrate_hooks_for_target, the three deprecated per-target wrappers, and_integrate_merged_hooks, and the dispatch inservices.pyincludes it only when_prim_name == "hooks".
The user-scope regression test (test_user_scope_still_writes_absolute_hook_paths) now sets user_scope=True directly instead of monkey-patching Path.home, so it exercises the same explicit signal production code uses.
Replace the ``project_root.resolve() == Path.home().resolve()`` heuristic in ``_integrate_merged_hooks`` with an explicit ``user_scope: bool`` kwarg threaded from the caller's ``InstallScope``. Surfaces three improvements raised in the Copilot review of microsoft#1396: * The gate no longer silently misclassifies the (admittedly rare) case where ``apm install`` runs with the repo at ``$HOME``. * ``_integrate_merged_hooks`` no longer couples to an implicit invariant of ``apm_cli.core.scope.get_deploy_root`` -- if user scope later moves to ``~/.config/...`` the gate keeps holding. * Intent is now explicit at the call site rather than inferred from deploy-root shape. Plumbing: * ``_integrate_merged_hooks``, ``integrate_hooks_for_target``, and the three deprecated per-target wrappers (``integrate_package_hooks_claude/cursor/codex``) accept ``user_scope: bool = False``. * ``services.integrate_package_primitives`` computes ``user_scope = scope is InstallScope.USER`` and passes it only on the hooks dispatch so sibling integrators (prompt/agent/command/ instruction) keep their existing signatures. ``InstallScope`` is imported at runtime inside the function, matching the local-import pattern already used in ``integrate_local_bundle``. The user-scope regression test now sets ``user_scope=True`` directly instead of monkey-patching ``Path.home``, so it asserts on the same explicit signal production code uses. ``unittest.mock.patch`` import removed (no longer needed).
srid
added a commit
to juspay/kolu
that referenced
this pull request
May 19, 2026
srid
added a commit
to juspay/kolu
that referenced
this pull request
May 19, 2026
The runner now ships an MCP server via process-compose's built-in JSON-RPC
surface ('ci run --mcp', juspay/ci commit 87f48d9f); the same PR also
publishes ci as an APM package whose 'apm.yml' exposes the MCP entry for
downstream consumers (6d42a62c). Consume it:
- 'juspay/ci#feat-platform-fanout-and-ssh' added under 'dependencies.apm'
in 'apm.yml'. Pinned to the PR branch because the MCP surface is not
on main yet; drop the '#…' suffix once the PR lands.
- '.apm/skills/ci/bin/serve' is the launcher (a four-line bash 'nix
run' wrapper) committed locally because our existing '.apm/skills/ci/
SKILL.md' (the /ci slash command) collides with upstream's package name,
and APM's directory-merge story drops the upstream package's bin/ subtree
on collision. Sourced verbatim from upstream with a header comment
explaining the divergence.
- 'agents/ai.just' switches 'apm_cmd' to srid/apm#fix/1394-project-scope-hook-paths
until microsoft/apm#1396 lands. 0.14.0 absolutized project-scope hook
paths to /home/<user>/<worktree>/… that only resolve on the author's
machine; the PR keeps them repo-relative.
- 'just ai::apm' regenerated the runtime tree against the patched apm-cli;
'.mcp.json' / 'opencode.json' / '.codex/config.toml' all carry the new
'ci' server entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#1354 unconditionally threaded
deploy_root=project_rootthrough_integrate_merged_hooksto absolutize hookcommandpaths. That fixwas correct for user-scope (
~/.claude/settings.jsonis read without afixed cwd, and
\${CLAUDE_PLUGIN_ROOT}is not expanded insidesettings.json-- the bug #1310 was about), but it also rewroteproject-scope
<repo>/.claude/settings.json,<repo>/.codex/hooks.json, and the sidecar<repo>/.claude/apm-hooks.json-- files that are typically checkedinto the repo. Every clone / contributor / CI runner then saw the
installer's machine-local absolute prefix baked into committed config.
This PR restricts the absolute-path rewrite to user-scope deploys by
gating on
project_root.resolve() == Path.home().resolve(). TheInstallScope.USERbranch landsproject_rootatPath.home()(seeapm_cli.core.scope.get_deploy_root), so the home-equality check is afaithful, side-effect-free proxy without threading a scope kwarg
through the integrator stack.
CLAUDE_CONFIG_DIRremains honoredupstream of this check -- it redirects the deploy directory
(
target.root_dir), notproject_rootitself.The behaviour applies to all targets that merge into a single JSON
config (Claude / Codex / Cursor / Gemini / Windsurf) since they all
flow through
_integrate_merged_hooks.Fixes #1394
Type of change
Testing
New / updated tests
test_project_scope_writes_relative_hook_paths(regression for [BUG] 0.14.0: project-scope hook paths rewritten to absolute, breaking portability of checked-in .claude/settings.json and .codex/hooks.json #1394): asserts the rewritten command is repo-relative and contains no absolute prefix.test_user_scope_still_writes_absolute_hook_paths(preserves [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): patchesPath.hometo the deploy root and asserts the rewritten command is absolute, matching the originaldeploy_rootresolution.test_codex_project_scope_keeps_relative_hook_pathsmirrors the Claude regression test through the Codex public entry point so the shared scope check is asserted from a second target.test_reinstall_preserves_multiple_hook_files_same_eventupdated to assert relative commands (its prior absolute-form assertion was documenting the regression).Local validation
End-to-end repro
The original bug repro from #1394 uses
srid/kolu(an APM consumer withsrid/agencypinned as a dependency that ships aStophook, and.claude/settings.json+.codex/hooks.jsonchecked in). Running the patched build against kolu produces a clean diff:After the fix, the patched
apm installproduces byte-identical output to the pre-0.14.0 (relative-path) state in kolu's committed history.