Repository navigation
feat(hook-manager): add a CaptainHook adapter - #1899
Anurag-S1ngh wants to merge 3 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.
43a7290 to
d41af3f
Compare
|
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.
adc0cb4 to
70681c3
Compare
Why: CaptainHook is a common PHP hook manager, and under whole-user scope its install writes into the shared core.hooksPath directory the same way overcommit's does, so it needs the same install fix. What: the registry entry detects captainhook.json and the CaptainHook hook marker and reuses the hooks-path-env strategy. Two new registry keys carry the parts of its CLI the shim needs. install.match: prefix finds the install action at any non-option argument, because its Symfony Console CLI takes global options before the command and resolves abbreviations (captainhook -n install, captainhook inst). Matching a run rewrites only core.hooksPath, which is harmless. install.vendor_bin lets the shim run a Composer project's vendor/bin/captainhook when PATH holds no real binary. The candidate gets the same canonical-path self-check as the PATH walk, so a symlink back to the shim is refused with exit 127 instead of exec'ing itself forever, and canonicalisation no longer depends on readlink -f being present. The fallback is registry-gated, so prek and overcommit still exit 127 rather than run whatever a repository ships as vendor/bin/prek. The shim also warns on a CaptainHook install when composer.json requires captainhook/hook-installer or captainhook/plugin-composer and neither extra.captainhook.disable-plugin nor config.allow-plugins turns it off, since that plugin installs behind the shim on every dependency update. The keys are read with python3 rather than matched as text, so a disable-plugin under another package's extra cannot silence the warning; without python3 the check is skipped. Generated-by: DeepSeek V4.1 Flash
Why: the contract page is where an adapter's strategy, detection and verified run are recorded, and CaptainHook has all three. What: add the adapter row with its verified run, the source-derived notes on how it reads core.hooksPath and how its CLI spells the install, the two new registry fields, and the vendor/bin fallback with the rule that a global binary on PATH wins over the project's pinned one. The install rewrite row, the shim row in tools/agent-isolation and the capability map now name CaptainHook. Generated-by: DeepSeek V4.1 Flash
Why: an operator who installs the shim for a Composer project needs to know that the binary comes from vendor/bin, that its Composer plugin installs outside the shim, and what to do when that plugin has already replaced the dispatcher. What: install Step P.3b lists captainhook.json as the detection file, names the install spellings the shim covers and the vendor/bin fallback, and asks the operator to disable the plugin before installing the shim. Verify check 8 reports a repository with either plugin enabled, the troubleshooting page gains the symptom, root cause and fix, and the whole-user section and its result table name the CaptainHook hook behind the dispatcher. Generated-by: DeepSeek V4.1 Flash
70681c3 to
ecebd60
Compare
Summary
captainhookadapter to the hook-manager registry:captainhook.jsondetection, theCaptainHookhook marker, andinstallthrough thehooks-path-envstrategy.captainhook -n install,captainhook --ansi install,captainhook inst), via a newinstall.match: prefixregistry field, so a Symfony Console invocation cannot bypass the rewrite.<repository root>/vendor/bin/<tool>only for adapters withinstall.vendor_bin, so a Composer project's local CaptainHook works without wideningPATH; avendor/binsymlink back to the shim exits 127 instead of exec'ing itself, and canonicalisation no longer depends onreadlink -f.composer.jsonrequirescaptainhook/hook-installerorcaptainhook/plugin-composerand neitherextra.captainhook.disable-pluginnorconfig.allow-pluginsturns it off. The keys are read withpython3rather than matched as text, so adisable-pluginunder another package'sextracannot silence it.The branch is rebased onto
mainand carries only the CaptainHook delta; overcommit and lefthook are unchanged frommain.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, plusprek run lychee --files <changed .md>: 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.