Repository navigation
feat(hook-manager): add a CaptainHook adapter - #1899
Anurag-S1ngh wants to merge 6 commits into
Conversation
github-actions
Bot
added
family:setup
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The registry entry and the generated shim and dispatcher blocks are consistent and tested, but the install path doesn't hold up yet: options before the command bypass the rewrite, the shim can't find a vendor/bin CaptainHook, and the Composer plugin path is left to prose while the adapter is marked shipping without a real run. This branch also carries an older copy of #1893's overcommit commit, so once #1893 lands please rebase onto main so this PR is only the CaptainHook delta (that also clears the current conflict). The real CI hasn't run on this branch, so these come from reading the code.
The branch currently conflicts with main and needs a rebase before this can merge.
options before the command, or an abbreviated command, bypass the rewrite (tools/agent-isolation/hook-manager-shim.sh:103)
A manager's install must put its hook in the repository's own
$(git rev-parse --git-common-dir)/hooks/, never in the sharedcore.hooksPathdirectory
if [ "${1:-}" != "$install_subcommand" ]; then
The rewrite fires only when the first argument is literally install. CaptainHook's CLI is a Symfony Console application, which accepts global options before the command name (captainhook -n install, captainhook --ansi install) and resolves unambiguous abbreviations (captainhook inst); each of those reaches the real binary unchanged and installs into the shared ~/.claude/git-hooks/. This is the same bypass class raised on the overcommit adapter (#1893). For hooks-path-env the simplest robust fix is to export the GIT_CONFIG_* override on every invocation, which is harmless for hook:* runs and needs no argv parsing; otherwise match the first non-option argument by prefix. Please add shim tests for -n install and an abbreviated command.
the documented install exits 127 for a vendor/bin CaptainHook (docs/setup/secure-agent-setup.md:1443)
Composer plugin run outside the shim, so its install is one explicit
`~/.claude/bin/captainhook install` with the plugin disabled in
The shim locates the real binary only by walking PATH for the next captainhook that is not itself (the real_tool loop in hook-manager-shim.sh). CaptainHook is usually a per-project Composer dev dependency in vendor/bin/, which is not on PATH, so ~/.claude/bin/captainhook install exits 127 with "could not locate the real 'captainhook' binary on PATH". Either document PATH="$HOME/.claude/bin:$PWD/vendor/bin:$PATH" captainhook install, or teach the shim a vendor/bin/<tool> fallback for this adapter, with a test. The same text is in tools/hook-manager/README.md.
the Composer plugin path is left to prose, and the adapter has no verified run (tools/hook-manager/README.md:185)
- Exercise the adapter against the real manager — the install through the shim and a commit behind the dispatcher, plus the control that fails without it — and record the run under Verified runs.
| `captainhook` | PHP | `hooks-path-env` (`install`); ... | shipping (source-derived; ...
The captainhook/hook-installer Composer plugin installs on every composer install / composer update and, as this row says, does not pass through the shim; under the contract such a manager needs a strategy outside the shim, but here it is covered only by asking the operator to set extra.captainhook.disable-plugin, with nothing detecting the plugin in composer.json or warning at setup or verify time. The PR body also says CaptainHook was never actually run, which step 7 requires before an adapter ships. Please do a real run (a php:8 container is enough) and record it under Verified runs, which would also settle the two points above; and either detect the plugin and warn, or mark the row planned rather than shipping.
Smaller observations
- See inline comments on
plugins/magpie-setup/skills/isolated-setup-install/step-p-sandbox-allowlists.md:337.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| CaptainHook's `vendor/bin` command and its `captainhook/hook-installer` | ||
| Composer plugin run outside the shim, so its install is one explicit | ||
| `~/.claude/bin/captainhook install` with the plugin disabled in | ||
| `composer.json`. |
There was a problem hiding this comment.
the documented install exits 127 for a vendor/bin CaptainHook
The shim locates the real binary only by walking PATH for the next captainhook that is not itself (the real_tool loop in hook-manager-shim.sh). CaptainHook is usually a per-project Composer dev dependency in vendor/bin/, which is not on PATH, so ~/.claude/bin/captainhook install exits 127 with "could not locate the real 'captainhook' binary on PATH". Either document PATH="$HOME/.claude/bin:$PWD/vendor/bin:$PATH" captainhook install, or teach the shim a vendor/bin/<tool> fallback for this adapter, with a test. The same text is in tools/hook-manager/README.md.
| @@ -172,10 +183,12 @@ What a repository or an operator configures is where each manager's environments | |||
|
|
|||
| | Adapter | Language | Install strategy | Home | Status | | |||
| |---|---|---|---|---| | |||
There was a problem hiding this comment.
the Composer plugin path is left to prose, and the adapter has no verified run
- Exercise the adapter against the real manager — the install through the shim and a commit behind the dispatcher, plus the control that fails without it — and record the run under Verified runs.
The captainhook/hook-installer Composer plugin installs on every composer install / composer update and, as this row says, does not pass through the shim; under the contract such a manager needs a strategy outside the shim, but here it is covered only by asking the operator to set extra.captainhook.disable-plugin, with nothing detecting the plugin in composer.json or warning at setup or verify time. The PR body also says CaptainHook was never actually run, which step 7 requires before an adapter ships. Please do a real run (a php:8 container is enough) and record it under Verified runs, which would also settle the two points above; and either detect the plugin and warn, or mark the row planned rather than shipping.
| that a repository the operator works in uses — `~/.claude/bin/prek` today. | ||
| A manager whose install strategy is `none` needs no install fix | ||
| and gets no shim. | ||
| that a repository the operator works in uses — `~/.claude/bin/prek` and |
There was a problem hiding this comment.
the install skill does not know about captainhook
- Add a row above, and update the troubleshooting and verify pages if the manager brings a failure mode of its own.
Step P.3b names only prek and overcommit for the shims to install and the files that detect them, so setup-isolated-setup-install never installs ~/.claude/bin/captainhook or tells the operator about the Composer plugin and vendor/bin. Add captainhook to the list and the detection sentence (captainhook.json), and the plugin note to the troubleshooting and verify pages.
Why: overcommit reads core.hooksPath unscoped and installs into whatever it names, then re-checks its install on every hook run. Under the whole-user setup that would replace the dispatcher for every repository on the host and then fail behind it, so it needs both an install strategy and a run-time override that prek's --git-dir flag does not cover. The new hooks-path-env strategy runs the manager's install with GIT_CONFIG_* pointing core.hooksPath at the repository's own <common dir>/hooks, and the dispatcher re-exports the same override before chaining into a hook that carries the adapter's marker (registry install.runtime_env). Refs apache#1821. Generated-by: Buffy (DeepSeek V4.1 Flash)
Why: CaptainHook reads core.hooksPath and installs into whatever it names, so under the whole-user dispatcher a bare install replaces the shared hook directory. The hooks-path-env strategy already covers that case; this adds the adapter, its detection rules, and the documented operator path for the vendor/bin command and the Composer plugin that the PATH shim cannot reach.
Why: CaptainHook installs unchanged when global options precede the command or the command is abbreviated, and the shim found the binary only on PATH, so a Composer project's vendor/bin install exited 127. Apply the hooks-path-env override to every invocation, fall back to vendor/bin, and warn when captainhook/hook-installer would install behind the shim. Generated-by: DeepSeek V4.1 Flash
Why: the vendor/bin fallback and the Composer plugin warning change the operator path, and a real CaptainHook run now backs the adapter's shipping status. Generated-by: DeepSeek V4.1 Flash
Why: Step P.3b named only prek and overcommit, and neither the troubleshooting page nor verify check 8 covered the Composer plugin that installs behind the shim. Generated-by: DeepSeek V4.1 Flash
43a7290 to
d41af3f
Compare
Why: the warning now has its own argv scan, so prove it fires for an install behind a global option and for an abbreviation as well. Generated-by: DeepSeek V4.1 Flash
|
Addressed the review:
The branch is rebased onto |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks, the three points from the last review are addressed: hooks-path-env now applies the override on every invocation so options and abbreviations before install can no longer slip past, the shim finds a Composer project's vendor/bin/captainhook, and the plugin is covered in Step P.3b, verify and the troubleshooting page, with a real run recorded under Verified runs.
Two things block a merge now:
- The branch needs a rebase onto
main. #1893 (overcommit) and #1895 (lefthook) have both merged, and this branch still carries an older overcommit commit that replaces #1893's reviewed argv matching (bundled-fi,--instprefixes, the-ialias) with the every-invocation override. Please rebase so this PR carries only the CaptainHook delta, keep overcommit as it is onmain(if the always-on override is the better design for overcommit too, that is its own PR), and keep the newhide-global-configstrategy working alongsidehooks-path-env. - The
vendor/binfallback can call the shim itself (inline below), which hangs everycaptainhookcall.
The other two inline comments are smaller and fine in the same push. One more, optional: the shim tests fail when the caller's environment already sets GIT_CONFIG_COUNT, since they expect index 0; stripping GIT_CONFIG_* in _setup would make them independent of the environment they run in.
The branch currently conflicts with main and needs a rebase before this can merge.
the vendor/bin fallback can exec the shim itself (tools/agent-isolation/hook-manager-shim.sh:107)
locate the REAL binary: the next
$toolon PATH that is not us
if _root="$(git rev-parse --show-toplevel 2>/dev/null)" && [ -x "$_root/vendor/bin/$tool" ]; then
real_tool="$_root/vendor/bin/$tool"
The PATH walk skips the shim through the self_canon comparison; the fallback does not. With vendor/bin/captainhook symlinked to ~/.claude/bin/captainhook (an easy workaround for the old exit 127) and no real captainhook on PATH, every captainhook call execs itself forever and hangs with no message. Please run the same readlink -f self check on the candidate, exit 127 with the "must sit ahead of" message when it is the shim, and add a test for it.
Smaller observations
- See inline comments on
tools/agent-isolation/hook-manager-shim.sh:104,tools/agent-isolation/hook-manager-shim.sh:137.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| if [ -z "$real_tool" ]; then | ||
| # A Composer project keeps the binary in vendor/bin, which is not on | ||
| # PATH; resolve it from the repository root the command runs in. | ||
| if _root="$(git rev-parse --show-toplevel 2>/dev/null)" && [ -x "$_root/vendor/bin/$tool" ]; then |
There was a problem hiding this comment.
the vendor/bin fallback can exec the shim itself
locate the REAL binary: the next
$toolon PATH that is not us
The PATH walk skips the shim through the self_canon comparison; the fallback does not. With vendor/bin/captainhook symlinked to ~/.claude/bin/captainhook (an easy workaround for the old exit 127) and no real captainhook on PATH, every captainhook call execs itself forever and hangs with no message. Please run the same readlink -f self check on the candidate, exit 127 with the "must sit ahead of" message when it is the shim, and add a test for it.
| break | ||
| done | ||
|
|
||
| if [ -z "$real_tool" ]; then |
There was a problem hiding this comment.
the vendor/bin fallback applies to every adapter
When
PATHholds no real binary, the shim falls back to<repository root>/vendor/bin/<tool>, so a Composer project's local binary works without wideningPATH.
Only Composer keeps a binary in vendor/bin, but the fallback runs for every adapter, so prek --version or overcommit in a cloned repository with no real binary on PATH would run whatever that repository ships as vendor/bin/prek instead of exiting 127. Please limit it to the captainhook adapter (a registry flag, or [ "$tool" = captainhook ]), and say in the README that a global captainhook on PATH wins over the project's pinned one.
| _root="$(git rev-parse --show-toplevel 2>/dev/null)" || return 0 | ||
| [ -f "$_root/composer.json" ] || return 0 | ||
| grep -q '"captainhook/hook-installer"' "$_root/composer.json" || return 0 | ||
| if grep -Eq '"disable-plugin"[[:space:]]*:[[:space:]]*true' "$_root/composer.json"; then |
There was a problem hiding this comment.
the Composer plugin check matches the wrong keys
When
composer.jsonrequires the plugin and does not setdisable-plugin, an install through the shim warns on stderr that the next dependency update will undo it.
disable-plugin: true anywhere in the file silences the warning, including under another package's extra key, and the older auto-installing captainhook/plugin-composer is not checked at all. Please anchor the match on the captainhook block of extra (or parse with php -r / jq when present) and check captainhook/plugin-composer as well.
Summary
captainhookadapter to the hook-manager registry:captainhook.jsondetection, theCaptainHookhook marker, andinstallthrough the existinghooks-path-envstrategy.hooks-path-envoverride on every invocation, so an install behind global options (captainhook -n install) or an abbreviation (captainhook inst) still lands in the repository's own hooks.<repository root>/vendor/bin/<tool>whenPATHhas no real binary, so~/.claude/bin/captainhook installworks in a Composer project.captainhook/hook-installeris enabled withoutextra.captainhook.disable-plugin, and teach install Step P.3b, the troubleshooting page and verify check 8 about it.Test plan
uv run --project tools/agent-isolation --group dev pytest tools/agent-isolation/tests: passuv run --project tools/dev --group dev pytest tools/dev/tests: passuv run --project tools/sandbox-lint --group dev pytest tools/sandbox-lint/tests: passuv run --project tools/setup-preflight --group dev pytest tools/setup-preflight/tests: passprek run --files <changed files> --skip workspace-pytest: passcomposer:2container: CaptainHook 5.29.2, PHP 8.5.12, Composer 2.11.1, git 2.54.0. The install through the shim used thevendor/binfallback and wrote the repository's own.git/hooks/; the shared dispatcher dir and globalcore.hooksPathwere unchanged. A commit behind the dispatcher ran the repository'scaptainhook.jsonand itsExistsaction rejected it. The control, a barevendor/bin/captainhook installwithout the shim, wrote through the shared dispatcher symlinks and replaced the dispatcher.Depends on #1893 for
hooks-path-env. The final rebase ontomain, dropping the overcommit copy, still waits for #1893 to land.