Skip to content

fix(agent-guard): fail closed on lefthook install behind global options and package runners - #1948

Merged
potiuk merged 3 commits into
apache:mainfrom
mickwd:fix/1918-lefthook-guard-fail-closed
Nov 2, 2026
Merged

potiuk merged 3 commits into
apache:mainfrom
mickwd:fix/1918-lefthook-guard-fail-closed

Conversation

@mickwd

@mickwd mickwd commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The lefthook guard took the first bare word as the subcommand, so lefthook --colors off install --force read off and went through. Any install word now makes it an install and every argument is checked, the same fail closed shape you suggested in the issue
  • npx, pnpx, bunx, pnpm exec/dlx, yarn exec/dlx and npm exec are looked through when they launch lefthook, including lefthook@x.y and @evilmartians/lefthook
  • argv[0] is normalised once in Segment with the same command_name helper guarded_head uses, so git.exe or /mingw64/bin/git.exe now reaches the git:* contributed guards too

Type of change

  • Python package (tools/*/ with pyproject.toml)

Test plan

  • New cases in test_skill_guards.py for every shape in the issue plus runner forms that must stay allowed (npm install --save-dev lefthook --force, npx lefthook run pre-commit --force, npx prettier --write install), and test_no_verify_guard_denied_for_a_path_qualified_git in test_guards.py
  • pytest tests -k "lefthook or path_qualified" gives 77 passed, against the old code the same selection gives 28 failed
  • Full tools/agent-guard suite has the same failure set before and after this change. I ran it on Windows where a few existing tests fail for platform reasons (pwd, path separators), none of them touch this code
  • ruff check and ruff format --check pass, and the plugin copy of the engine is in sync
  • Checked against real lefthook 2.2.2, which rejects options before the subcommand, so --colors off install is a 1.x shape. Matching any install word covers both

Linked issues

Closes #1918

Notes for reviewers

The runner unwrap takes the first word naming the lefthook package and does not parse each runner's own options. It can over match something like npm run lefthook, but that only matters when the rest of the line also says install with a destructive flag, which felt like the right side to err on for a defence in depth guard.

@Kaap10 Kaap10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @mickwd! The implementation is clean, well-tested, and addresses both requirements from #1918 (the lefthook global option and package runner bypasses, and the command_name normalization).

The logic and test suite look solid. There is just one line-ending issue to fix before CI can turn green:

Line Endings (CRLF to LF)

The 6 modified files were committed with CRLF line endings from Windows. In Apache Magpie, the mixed-line-ending pre-commit hook enforces LF line endings.

You can renormalize the line endings locally and amend your commit with:

git add --renormalize .
git commit --amend --no-edit
git push --force-with-lease

Alternatively, running prek run mixed-line-ending --all-files will automatically convert them to LF.

Once line endings are normalized, this is ready to go!

@mickwd

mickwd commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @Kaap10, glad the logic and tests look good!

I checked the line endings, and the 6 files in this PR are already LF in the pushed commit (c1467489). They have 0 CR bytes, both in the blobs on GitHub and in the diff against main. I also ran your suggested fixes:

  • git add --renormalize . → nothing staged
  • prek run mixed-line-ending --all-files → no changes to commit (it only touched my local Windows checkout, where core.autocrlf adds CRLF; the committed content is unchanged)

So I think the CRLF you saw was a local checkout artifact, not something in the commit. CI hasn't actually failed yet. The workflows show action_required because this is a first-time fork PR waiting for approval to run.

Could a maintainer approve the workflow run? If prek or the tests flag anything once they run, I'll fix it right away.

@github-actions github-actions Bot added family:setup setup-* skills family:tools tools/* family:docs Docs, MISSION.md, READMEs capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) substrate:action-guard Tool substrate: deterministic pre-tool-use command guards labels Nov 1, 2026
@potiuk
potiuk force-pushed the fix/1918-lefthook-guard-fail-closed branch from c146748 to 6c14942 Compare November 1, 2026 23:20

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extends the lefthook-hooks-path guard to lefthook install behind global options and package runners, and normalises path-qualified and .exe command heads for every guard. I rebased the branch onto current main and pushed one commit restamping the isolated-setup fingerprint, which the prek check was failing on, so please pull before you push again (git pull --rebase). One gap is left, inline: a few runner spellings still get through.

Smaller observations

  • See inline comments on tools/agent-guard/src/agent_guard/__init__.py:2620.

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.

mickwd and others added 3 commits November 2, 2026 00:59
…ns and package runners

The lefthook hooks-path guard took the first bare word as the subcommand,
so `lefthook --colors off install --force` read `off` and was allowed, and
`npx lefthook install --force` never reached the guard at all. Any
`install` word now makes the segment an install, and npx, pnpm, yarn, bun
and npm exec are looked through when they launch lefthook.

argv[0] is normalised once with the helper guarded_head uses, so git.exe
or a path-qualified git also reaches the git:* contributed guards.

Closes apache#1918
@potiuk
potiuk force-pushed the fix/1918-lefthook-guard-fail-closed branch from a66683c to 52571f0 Compare November 2, 2026 00:00

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, a66683c6 covers every spelling from the last review: a runner's -c / --call / --shell-mode value and a shell it launches are now split like any other -c string, corepack is looked through, and .cmd / .bat are stripped like .exe, each with a test. I rebased the branch onto main again after #1899 merged (the only conflict was the isolated-setup fingerprint, restamped in the last commit), so please reset to the branch before you push again. Approving.


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.

@potiuk
potiuk merged commit fe8f1cc into apache:main Nov 2, 2026
12 checks passed
@mickwd
mickwd deleted the fix/1918-lefthook-guard-fail-closed branch November 2, 2026 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) family:docs Docs, MISSION.md, READMEs family:setup setup-* skills family:tools tools/* substrate:action-guard Tool substrate: deterministic pre-tool-use command guards

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lefthook guard: recognise install after valued global options, and package runners

3 participants