Repository navigation
feat(hook-manager): add a lefthook adapter - #1895
Anurag-S1ngh wants to merge 5 commits into
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.
| head = argv[0] | ||
| if head in GUARDED_HEADS: | ||
| return head | ||
| base = head.rsplit("/", 1)[-1] |
There was a problem hiding this comment.
Non-blocking / Portability: guarded_head only splits on POSIX forward slashes (/). On Windows or environments where callers pass backslash-delimited paths like node_modules\.bin\lefthook, rsplit('/', 1) retains the backslash-delimited string, so base fails to match 'lefthook'. Furthermore, when the binary has an .exe suffix on Windows (lefthook.exe), base in GUARDED_HEADS evaluates to false. Normalizing slashes via head.replace('\\', '/').rsplit('/', 1)[-1] or os.path.basename, and stripping an optional .exe suffix, ensures consistent guard interception across platforms.
There was a problem hiding this comment.
Fixed in eff4fb9: guarded_head now normalizes backslash separators and strips an optional .exe suffix. Added unit coverage for both.
|
|
||
| def _is_framework_path(value): | ||
| actual = os.path.realpath(os.path.expanduser(value)) | ||
| return any(actual == os.path.realpath(os.path.expanduser(name)) for name in FRAMEWORK_HOOKS_PATHS) |
There was a problem hiding this comment.
Non-blocking / Portability: _is_framework_path uses exact string equality on os.path.realpath. On case-insensitive file systems (such as Windows NTFS or macOS APFS), differences in casing (for instance, uppercase vs lowercase drive letters or user home directory capitalization) can cause direct string equality to return False even when resolving to the exact same directory. Applying os.path.normcase(...) to both paths makes the check reliable across platforms.
There was a problem hiding this comment.
Fixed in eff4fb9: both paths now go through os.path.normcase, with a regression test for a differently cased framework path.
| hide-global-config) | ||
| # Never interfere with help output. | ||
| for _a in "$@"; do | ||
| if [ "$_a" = "-h" ] || [ "$_a" = "--help" ]; then |
There was a problem hiding this comment.
Non-blocking / Consistency: In the git-dir-flag block above (lines 121 to 127), argument parsing uses a shell case "$_a" in statement to match -h and help flags. In hide-global-config, it switches to an if [ ... ] comparison. Both are functionally correct and properly pass help requests through without setting GIT_CONFIG_GLOBAL, but matching the case pattern style keeps the shim implementation uniform.
There was a problem hiding this comment.
Fixed in eff4fb9: the hide-global-config help check now uses the same case pattern as git-dir-flag.
|
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.
| done | ||
|
|
||
| if git rev-parse --git-common-dir >/dev/null 2>&1; then | ||
| GIT_CONFIG_GLOBAL=/dev/null exec "$real_tool" "$install_subcommand" "$@" |
There was a problem hiding this comment.
hide-global-config applies on every host
It is a no-op on hosts with no global
core.hooksPath.
Nothing checks for a global core.hooksPath before this line, so every in-repo lefthook install through the shim runs with the whole global config hidden, which contradicts the sentence this PR adds to Step P (and the same claim in tools/agent-isolation/README.md). It also drops credential.helper, url.*.insteadOf, safe.directory and the like for the install, and lefthook can clone remote configs during it. Could the override apply only when git config --global --get core.hooksPath returns a value, with a shim test asserting GIT_CONFIG_GLOBAL stays unset on a host without one?
Flagged by both the primary and adversarial reviewers.
There was a problem hiding this comment.
Fixed in 440da68: GIT_CONFIG_GLOBAL is set only when git config --global --get core.hooksPath returns a value, with one test for the host that has none (GIT_CONFIG_GLOBAL stays unset) and one for a custom, non-framework path. Step P and both READMEs now match.
|
|
||
|
|
||
| def _destructive_flag(argument): | ||
| if argument in ("-f", "--force"): |
There was a problem hiding this comment.
--flag=value spellings pass the guard
--force=true and --reset-hooks-path=true, which lefthook accepts for its boolean flags, match neither branch and are not short clusters, so the guard returns no denial. The shim neutralises them, but the guard is what covers a direct run of the real binary (./node_modules/.bin/lefthook install --force=true), which can then overwrite the shared dispatcher.
Flagged by both the primary and adversarial reviewers.
There was a problem hiding this comment.
Fixed in 440da68: the flag name is matched before any =value, so --force=true and --reset-hooks-path=true are denied (as are =1), while an explicit --force=false stays allowed. Parametrized deny/allow cases added.
|
|
||
| ### Symptom | ||
|
|
||
| A repository that uses lefthook has no working hooks, and `lefthook install` |
There was a problem hiding this comment.
semantic line breaks
Format prose across the repository (skills, docs, canned responses) using semantic line breaks (SemBr).
Break lines at natural linguistic boundaries — specifically one sentence per line (or at clause boundaries when helpful).
This section, and the new paragraph in docs/setup/secure-agent-setup.md, wraps at a column, so lines break mid-clause ("and lefthook install / fails:"). One sentence or clause per line would match the rest of the docs.
There was a problem hiding this comment.
Fixed in 440da68: this section and the new secure-agent-setup.md paragraph are one sentence per line now.
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
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.