fix(hook): catch two spellings that took the guards away unnoticed - #33
Merged
Conversation
The inline `--config-env=core.hooksPath=...` form set the bypass flag but never the hooks-path override flag, unlike the three sibling branches beside it. The gap predates the module split. A commit carrying that spelling passed the agent guard, and the advisory it printed named `--no-verify` as the cause, which was not present. The class test claimed in its own comment to assert the whole set and listed six forms; it now lists ten. The hooks directory also did not have to be spelled in full to be removed: `commandMayTouchHooks` tested for the literal `.git/hooks`, so `rm -rf .git/h*` read as an ordinary prefix. The test now matches the path segment, with a lookahead that keeps `cat .git/HEAD && git commit` allowed, and covers `--git-path hooks`. A deletion routed through `find` or a hook installer still escapes a text test; that belongs to the enforcement layer, not the parser.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two ways a command could remove the managed guards without the agent layer
reading it as a bypass.
--config-env=did not set the hooks-path flagparseGithandles four spellings of a config override:-c key=value,-ckey=value,--config-env key=value, and--config-env=key=value. The firstthree run all three predicates. The fourth ran
hookAffectingConfigandaliasAffectingConfigbut nothooksPathOverrideConfig, socommandHooksPathOverridestayed false.A commit carrying
git --config-env=core.hooksPath=EVIL commit -m xreachedhistory with the agent guard allowing it, and the advisory printed alongside
named
--no-verifyas the cause, which was not present.The gap predates the module split —
git show 013da61^:src/hook.mjshas thesame asymmetry — so it has never been covered. The fix is the one missing line,
matching the shape of the three branches beside it. Collapsing the four branches
into a shared helper was considered and dropped: they differ in slice offset and
in how far they advance, so merging them touches the hottest path of the parser
for no behaviour change.
The class test at
tests/hook.test.mjs:3016claims in its own comment to assertthe whole class and listed six forms. It now lists ten: both
--config-envspellings against
core.hooksPathand againstinclude.path. Only the inlinecore.hooksPathform was leaking; the other three already denied, and pinningthem stops the set drifting apart again.
The hooks directory did not have to be spelled in full
commandMayTouchHookstested for the literal string.git/hooks, sorm -rf .git/h*read as an ordinary prefix and one commit on that line wentthrough with only the advisory path firing.
The test now matches a
.git/hpath segment. A bare segment match would denycat .git/HEAD && git commit, since the test is case-insensitive, so alookahead steps around
HEAD.--git-path hooksis covered too, for therm -rf $(git rev-parse --git-path hooks)shape.A deletion routed through
find -delete, or a hook installer that replaces thedirectory, still escapes a text test — no string on the command line names the
hooks. That is a limit of reading commands rather than watching the filesystem,
and the answer belongs at the enforcement layer, not here. The next invocation
already denies with "run 'aimhooman init' and retry", so exactly one commit
slips and the breakage is reported rather than silent.
The allow-by-default for unmodelled prefixes stays as it is. Refusing them
taught agents to drop the
&&gate rather than run the command separately,which is why it was relaxed, and inverting it to an allowlist breaks
./scripts/check.sh && git commitand turns off the ref-mutation deny for everybuild runner.
Tests
Two new denials and two new guardrails, added to the existing prefix tests:
.git/h*and the--git-path hooksform must deny;cat .git/HEADandgit rev-parse HEAD~1before a commit must stay allowed.Full suite green.