Repository navigation
feat(hook-manager): add a lefthook adapter - #1895
Conversation
github-actions
Bot
added
family:setup
Kaap10
left a comment
Kaap10
left a comment
There was a problem hiding this comment.
The implementation covers all four requirements of Issue #1822 cleanly. The hide-global-config strategy effectively prevents lefthook from refusing installation when a global core.hooksPath is set, by isolating the git environment via GIT_CONFIG_GLOBAL=/dev/null. The registry additions cover all 20 configuration file variants, the agent guard correctly intercepts destructive flags, and the documentation details the npm package manager postinstall caveat.
Inline comments have been added directly on the respective diff lines for cross-platform portability considerations.
One point with no line in this diff:
PR #1892 (Issue #1891) introduced the Step P.3b eval harness for hook manager shims (step-hook-manager-shim in tools/skill-evals/). While this PR updates the prose documentation in step-p-sandbox-allowlists.md to reference the ~/.claude/bin/lefthook shim path, there is no corresponding eval case added to tools/skill-evals/cases/ yet. Once both PRs land, a follow-up test case should be added to that eval suite.
|
Agreed on the eval follow-up. Leaving the step-hook-manager-shim case for after #1892 lands. |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The lefthook adapter and its guard are well tested end to end, and Kaap10's three earlier points are addressed in eff4fb9; the shim should hide the global config only when a global core.hooksPath is actually set, the guard should catch the --flag=true spellings, and the branch needs a rebase onto main.
The branch currently conflicts with main and needs a rebase before this can merge.
Smaller observations
- See inline comments on
tools/agent-isolation/hook-manager-shim.sh:148,plugins/magpie-setup/skills/isolated-setup-install/guards/lefthook_hooks_path.py:44,docs/setup/sandbox-troubleshooting.md:1248.
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.
Why: lefthook refuses to install while the whole-user scope's global core.hooksPath is set. The shim hides that value from lefthook's scoped git config read, so lefthook's own target resolves to the repository's common hooks directory, linked worktrees included. The registry carries the adapter with no home because lefthook keeps nothing outside the repository, and the new lefthook-hooks-path agent guard blocks the two flags that would damage the setup: --force writes over the dispatcher and --reset-hooks-path unsets it. The contract records the npm postinstall path the shim cannot reach. Generated-by: DeepSeek V4.1 Flash
Why: review flagged three portability and consistency gaps in the lefthook adapter. guarded_head now recognizes backslash-delimited and .exe command paths, the hooks-path guard compares framework paths with os.path.normcase, and the shim's help passthrough uses the same case pattern as the other install strategy.
Why: the lefthook shim override ran on every host, so an install on a machine with no global core.hooksPath silently lost the operator's credential.helper, url.*.insteadOf and safe.directory even though it had nothing to hide; it now sets GIT_CONFIG_GLOBAL only when the global config actually carries a core.hooksPath, which makes the documented no-op true. The guard now reads the flag name before any =value, so lefthook's --force=true and --reset-hooks-path=true spellings are caught when the real binary runs outside the shim, while an explicit =false stays allowed. The new troubleshooting section and the secure-setup paragraph break one sentence per line. Also regenerates the isolated-setup fingerprint for the rebase onto main. Generated-by: DeepSeek V4.1 Flash
eff4fb9 to
440da68
Compare
…=false The lefthook guard read a false-ish `=value` before it looked at short switch bundles, so `lefthook install -fr=false`, which still sets --force, was allowed (the previous version denied it). The value now applies only to a single flag or to the last switch of a bundle; every switch before it counts as set. Tests deny `-fr=false` and `-rf=false` and keep `-f=false` allowed. Also reword the shim comment that called the hand-written install strategy branches "generated": the sync script generates only the adapter() block. The secure-setup fingerprint is restamped. Generated-by: Claude Opus 5
# Conflicts: # plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
440da68 addresses all three points from the earlier review (the global config is hidden only when a global core.hooksPath is set, the --flag=value spellings, semantic line breaks) and Kaap10's three, each with a test that fails on the previous commit. Two small things were left, so I pushed a fixup commit (772474e) rather than send it back:
guards/lefthook_hooks_path.py: the false-ish=valuewas read before the short-switch bundle check, solefthook install -fr=false, which still sets--force, was allowed. The value now applies only to a single flag or the last switch of a bundle. New deny cases-fr=falseand-rf=false, and-f=falsestays allowed (tools/agent-guard/tests/test_skill_guards.py).tools/agent-isolation/hook-manager-shim.sh: the comment no longer calls the install-strategy branches "generated";sync-hook-managers.pygenerates only theadapter()block. The secure-setup fingerprint is restamped.- 962092a merges
main, which had moved the fingerprint constant; it is regenerated for the merged tree.
For a follow-up rather than this PR: the guard reads the first non-option token as the subcommand, so a global option given its value as a separate argument before install (lefthook --colors off install --force) is not recognised, and package runners (npx lefthook install --force) are not unwrapped; and the basename match lets /usr/bin/git reach the bundled guards while git:* contributed guards still key on a literal argv[0] == "git". The shim still covers lefthook run from PATH.
Please pull before pushing again.
GitHub has not yet computed whether this branch merges cleanly with its base.
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.
main now carries the overcommit adapter (apache#1893). The shim dispatches on all three install strategies (git-dir-flag, hooks-path-env and hide-global-config), the registry validator accepts each, and the docs and spec list prek, overcommit and lefthook together. The secure-setup fingerprint is main's, restamped by the hook. Generated-by: Claude Opus 5
Summary
lefthookadapter to the hook-manager registry: no home, the 20 root config names (lefthook.*,.lefthook.*,-localvariants), and thecall_lefthookhook marker.hide-global-configinstall strategy in the shim and generator, solefthook installthrough~/.claude/bin/lefthookwrites to$(git rev-parse --git-common-dir)/hooksunder a globalcore.hooksPath, linked worktrees included. The open pre-commit adapter issue hook-manager: add a pre-commit adapter #1820 reuses the same strategy.lefthook-hooks-pathagent guard blocking--forceand--reset-hooks-pathwhile the global path is the frameworks, and document the npmpostinstallgap the shim cannot reach.Type of change
tools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)Test plan
prek run --all-files --skip workspace-pytestpassespytest,ruff check,ruff format --check, andmypy:tools/agent-guard,tools/agent-isolation,tools/dev,plugins,tools/sandbox-lint,tools/setup-preflighthide-global-configcases (plain install,--force,-r, linked worktree, help, passthrough), guard deny/allow/override cases, and registry/strategy validationcore.hooksPath: plain and--forceland in<repo>/.git/hooks/pre-commit, the shared dir stays empty, a linked worktree lands in the main checkouts.git/hooks/, and the unshimmed binary refuses as expectedagent_guard --check: exit 2 onlefthook install --force, exit 0 on plainlefthook installrender-preflight-fixtures/unittest (skill sibling scripts)passLinked issues
Closes #1822
Refs #1820
Notes for reviewers
The shim applies
GIT_CONFIG_GLOBAL=/dev/nullto everylefthook install, including one carrying--forceor--reset-hooks-path, so neither can reach the shared directory or unset the whole-user setup through the shim.MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1is the deliberate override, and the agent guard blocks an agent from trying.The npm
postinstallpath (node_modules/.bin/lefthook, ahead of~/.claude/bin) is documented but not fixed here; the contract proposes the session-start hook noticing hooks in the shared directory that are not the dispatcher.