[TejasS1233]

Summary

• Adds the overcommit adapter to the hook-manager contract (hook-manager: add an overcommit adapter #1821), so a repository that uses overcommit keeps working under the whole-user dispatcher instead of replacing it.
• overcommit reads core.hooksPath unscoped and installs into whatever it names, then re-checks that install on every hook run. A bare overcommit --install under whole-user scope writes into ~/.claude/git-hooks/ and preserve_old_hooks moves the dispatcher's symlinks aside; and even a correct install fails behind the dispatcher, because the run-time self-check looks for its hook at the shared path and gets ENOENT. So it needs both a new hooks-path-env install strategy and a run-time override that prek's --git-dir flag does not cover.
• The adapter is one entry in tools/hook-manager/registry.json; the shim and dispatcher blocks are generated from it by tools/dev/sync-hook-managers.py, which gains the hooks-path-env strategy, a subcommand that may be a flag (--install), and the install.runtime_env field. The run against the real manager is recorded in the contract's new Verified runs subsection.

Issue coverage

All four items of the issue are done, and each acceptance criterion was exercised against the real manager (see Test plan).

• 1. hooks-path-env in hook-manager-shim.sh and in STRATEGIES; install.subcommand may be a flag, and the prek-only install.flag is now read safely under set -u
• 2. Dispatcher exports the same core.hooksPath override before chaining into a hook whose marker matches an adapter with install.runtime_env
• 3. overcommit adapter in registry.json, blocks regenerated, shim + dispatcher tests added
• 4. Self-update behaviour verified by running it against overcommit v0.73.0, result recorded in tools/hook-manager/README.md

Type of change

• Tool / bridge contract (tools/<system>/*.md)
• Python package (tools/*/ with pyproject.toml)
• Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
• Documentation (docs/, README.md, CONTRIBUTING.md)
• CI / dev loop (prek, workflows, validators)

Test plan

Run inside a Linux environment (Ubuntu 24.04, Ruby 3.2.3, overcommit 0.73.0 — the version the issue analysed), against a simulated whole-user setup: global core.hooksPath at the shared dir with the dispatcher symlinked into it, the shim installed as ~/.claude/bin/overcommit.

• overcommit --install through the shim reports Installing hooks into <repo>; the hook lands in the repository's own .git/hooks/ carrying the overcommit marker; ~/.claude/git-hooks/ is byte-identical before and after (no overcommit-hook, no old-hooks/, global core.hooksPath unchanged, no repository-level core.hooksPath).
• A commit behind the dispatcher runs the repository's own .overcommit.yml with no self-update failure: a clean commit succeeds, and a trailing-whitespace commit is rejected by overcommit's PreCommit TrailingWhitespace hook — so the manager really ran rather than the commit merely passing.
• Both controls fail, which is why the adapter is needed: a bare overcommit --install with no shim installs into the shared dir and moves the dispatcher's symlinks aside (Hook '…/pre-commit' already exists and was not installed by Overcommit. Moving to '…/.claude/git-hooks/old-hooks'), and a correct install under the dispatcher from origin/main dies in overcommit's own check (No such file or directory @ rb_file_s_size - ~/.claude/git-hooks/overcommit-hook, installer.rb:59).
• tools/agent-isolation suite runs on Linux: the two files this change touches pass (26 tests), and the full suite's failures in test_sandbox_error_hint.py are identical on origin/main, so they pre-date this change.
• tools/dev: ruff check, ruff format --check, mypy clean and uv run pytest passes, including the two new registry cases (a --flag subcommand is accepted; a non-boolean runtime_env is refused). tools/sandbox-lint: uv run pytest passes. tools/dev/sync-hook-managers.py --check → exit 0, so the generated blocks match registry.json.
• prek run --all-files --skip workspace-pytest — running; CI runs it on this PR regardless.

RFC-AI-0004 compliance

• Sandbox — no new unrestricted host access: the manager's install is redirected into the repository's own hooks dir, and the chained hook gets only a GIT_CONFIG_* override.
• Vendor neutrality — a second hook manager is supported through the shared registry rather than a per-manager special case.

Linked issues

Closes #1821.

Notes for reviewers (optional)

• No troubleshooting-page entry. The contract's Adding an adapter step 6 says to update the troubleshooting and verify pages when a manager brings a failure mode of its own. overcommit brings none that those pages cover: docs/setup/sandbox-troubleshooting.md is shaped around sandbox write/read and network blocks with a settings.json widening as the fix, and overcommit's residual failure modes are not blocks — its "shim not on PATH" consequence is already stated once, for every adapter, in isolated-setup-install Step P.3b-whole-user, and check 19d covers homes, which overcommit does not have. Happy to add an entry anyway if you would rather have it there.
• An isolated-setup-doctor check that flags a foreign hook sitting in the shared ~/.claude/git-hooks/ (the leftover of an install made before the adapter existed) would catch the reverse of this change. Out of scope here; say the word and I will file it.
• Found while building the control above, and out of scope for this PR: two copies of the shim on one PATH recurse. The shim skips itself by canonical path, so a second copy (for example ~/.claude/bin/overcommit plus a ~/.claude-config/bin/overcommit symlink, which Step P offers) is executed as if it were the real binary and bounces back. Worth a guard and an issue of its own; I have not touched it here.
• install.runtime_env is validated to be a boolean and to be settable only with hooks-path-env, so a later adapter cannot ask for the run-time override from a strategy that does not produce one.

---

Generative AI disclosure: prepared with Buffy (DeepSeek V4.1 Flash); reviewed by the submitter.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[Kaap10]

Thanks @TejasS1233 for working on this. The implementation cleanly addresses the acceptance criteria in #1821 and adheres to the hook manager contract.

Code generation is consistent, documentation updates are comprehensive, and including the empirical reproduction baseline under the Verified runs section in tools/hook-manager/README.md is appreciated.

I have gone through the diff and verified the changes locally (sync-hook-managers.py with check flag passes with 0 drift, test_sync_hook_managers.py passes 28/28, licence headers and markdownlint checks are clean).

A few observations and notes for discussion:

1. Short option bypass (-i)

In tools/agent-isolation/hook-manager-shim.sh:

if [ "${1:-}" != "$install_subcommand" ]; then
  exec "$real_tool" "$@"
fi

Overcommit supports -i as the short alias for the install action flag. Because registry.json declares the long install flag, invoking overcommit -i will bypass the shim and execute the real binary directly against the shared hooks directory. We should either note in the documentation that the long flag form is required, or consider whether the shim should handle alias flags.

2. Option ordering

The check assumes the install command is always $1. With tools where the action is a flag rather than a positional subcommand verb, options can sometimes precede the action (for example overcommit -f followed by the install option). Any leading modifier will cause the invocation to bypass the shim.

3. Overwriting GIT_CONFIG_* variables

In both hook-manager-shim.sh and git-hook-dispatcher.sh:

export GIT_CONFIG_COUNT=1
export GIT_CONFIG_KEY_0=core.hooksPath
export GIT_CONFIG_VALUE_0=...

Setting GIT_CONFIG_COUNT=1 overwrites any pre-existing git configuration overrides that a caller environment may have defined. This matches the design in #1821 and is standard for hook isolation, but is worth keeping in mind.

4. Defensive guard in hook-manager-shim.sh

if [ -n "${install_flag:-}" ]; then
  case "$_a" in
    "$install_flag"|"$install_flag"=*) inject=0; break ;;
  esac
fi

Good catch guarding ${install_flag:-} to prevent unbound variable errors under set -u when adapters have no long flag defined.

5. Cross-PR Coordination with Step P.3b Evals

PR #1892 introduces behavioral eval coverage for Step P.3b in setup-isolated-setup-install. Once both PRs land on main, we should add an eval case covering Overcommit detection (.overcommit.yml triggering installation of ~/.claude/bin/overcommit).

[potiuk]

The hooks-path-env strategy and the dispatcher's run-time override fit the contract well and are checked against real overcommit; four points on how far the rewrite and the exported override reach, and the spec, before it lands.

GitHub has not yet computed whether this branch merges cleanly with its base.

Smaller observations

• tools/agent-isolation/hook-manager-shim.sh:104 — overcommit's install is an option, not a positional subcommand: it also takes -i, and accepts it after other options (overcommit --force --install). Both skip the rewrite, reach the real binary unchanged, and install into the shared ~/.claude/git-hooks/, which is the host-wide failure this adapter exists to prevent. For a flag-style install, scan all of argv for the flag and its aliases (e.g. an install.aliases list in the registry), or at least say in the adapter row that only a leading --install is rewritten.
• See inline comments on tools/agent-isolation/git-hook-dispatcher.sh:165, tools/hook-manager/registry.json:69, tools/agent-isolation/git-hook-dispatcher.sh:167.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

More on how Apache Magpie handles maintainer review:
Contributing guide.

[TejasS1233]

Thanks for the review. Addressed all four points in eaea149:

• The shim now scans all of argv for overcommit's flag-style install action and its -i alias, using install.aliases in the registry. overcommit --force --install and overcommit -i now take the rewrite.
• Both the shim and dispatcher append core.hooksPath at the next free GIT_CONFIG_* index and increment GIT_CONFIG_COUNT, so caller-provided entries survive. Added tests for both.
• The exported override is documented as a known limitation in the hooks-path-env strategy row, the dispatcher comment, and the spec. I did not scope it because only overcommit's self-check needs it.
• Updated agent-isolation-sandbox.md line 385 for overcommit, hooks-path-env, install.runtime_env, aliases, and the limitation.

Verification: sync-hook-managers.py --check clean, 34 dev tests pass, 32 agent-isolation shim/dispatcher tests pass, and ruff check/format are clean.

The #1892 eval coverage remains a post-merge follow-up once both PRs land.

[potiuk]

eaea149 addresses all four points from the earlier review: the argv/alias scan, appending core.hooksPath at the next free GIT_CONFIG_* index (with tests), the documented reach of the exported override, and the spec. One gap was left, so I pushed it as fixup commits rather than send it back:

• tools/agent-isolation/hook-manager-shim.sh (7685671, ef134d6): overcommit parses its options with Ruby's OptionParser, which also takes a single-letter switch inside a bundle (overcommit -fi) and any unambiguous prefix of a long one (overcommit --inst); both reached the real binary unchanged and installed into the shared hooks directory. The shim now matches those too, failing closed on any prefix of --install, since a wrongly rewritten run only points core.hooksPath at the repository's own hooks. New cases in tests/test_hook_manager_shim.py cover -fi, --i, --inst and --force --insta, and pin that --run, --uninstall and -- still pass through.
• step-p-sandbox-allowlists.md, tools/hook-manager/README.md and the agent-isolation-sandbox spec now describe the same matching, and the secure-setup fingerprint is restamped.
• b1fb779 merges main, which had moved the fingerprint constant; it is regenerated for the merged tree.

Please pull before pushing again.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Apache Magpie handles maintainer review:
Contributing guide.