feat(guardrails): block non-canonical git commit; drop --trailer conjunct - #736
Conversation
…unct The advisory covering `git commit` was overridden 11 times in a single session. An advisory that is always overridden trains the reader to filter it out, so the commit half becomes a blocking guard. It gates the MECHANIC, not the ritual. `git commit -m "<multi-line>"` flattens newlines unpredictably across shells; `-F -` is what prevents that. Gating on "did you invoke /commit" is not available anyway — anthropics/claude-code#22655 (expose skill_name to hooks) is closed as not planned — and shape is the better target regardless: it enforces an outcome a reviewer can verify in git log. Exempt, because no message-on-stdin form exists for them: --amend, -C/-c/--reuse-message/--reedit-message, --fixup/--squash, -F <path>, and any commit taken mid-merge/rebase/cherry-pick/revert. Gating those would strand a conflict resolution. The old condition also required --trailer, which was a latent false positive: /commit omits the trailer under trailer_policy `none`, so a repo forbidding co-author trailers saw its own conformant commits flagged every time. That had to be settled before blocking on the same condition — requiring --trailer to pass would have permanently blocked the skill in that configuration. gh pr create stays advisory. /pull-request create issues that exact command itself and no shape distinguishes them, so blocking it would deadlock the skill. Closes #731 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ogVV7z2Heg9ojJ88nqNxh
|
Claude finished @kyle-sexton's task in 4m 10s —— View job Code Review — PR #736
The overall design is sound. The separation of mechanic (stdin-form gate) from policy (trailer), the fail-closed posture on timeout, and the exemption logic for sequencer states are all well-reasoned. Four issues worth addressing before merge, ranging from a stale string to a minor coverage gap. Issues1. Stale
"description": "Advise when direct git commit / gh pr create bypasses the source-control skills (never blocks)"
Suggested fix: 2. Duplicate test case inflates the count by one
# line 37
run "git commit -F - (allowed)" "git commit -F - --cleanup=verbatim" 0
# line 44
run "git commit -F - without --trailer (allowed — trailer_policy none)" \
"git commit -F - --cleanup=verbatim" 0Both exercise the same command. The 3. Only
for f in MERGE_HEAD CHERRY_PICK_HEAD REVERT_HEAD rebase-merge rebase-applyThe test only creates and removes 4. Telemetry data schema diverges between the two hooks
'{tool:"Bash",subject:$subject,form:$form}'
'{tool:"Bash",subject:$subject,forms:$forms}'The blocking hook only ever emits a single form value ( Observations (no action needed)
Bundled short-option form
SummaryOne stale user-visible string in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94ccfd18c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
comment-hygiene flagged the bare upstream issue number as a repo-issue tracker ref. The comment keeps the constraint it was explaining — the hook payload has no originating-skill field — and the citation stays in the CHANGELOG, where a reader can follow it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ogVV7z2Heg9ojJ88nqNxh
|
Claude finished @kyle-sexton's task in 2m 34s —— View job Code Review — PR #736 (updated after
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf559b89d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`git -c alias.c=commit c -m bypass` resolved to subcommand `c`, not `commit`, so the guard returned early and allowed a noncanonical commit. Same for `alias.ci='commit -m x'` and the shell-alias form. Reproduced all three at exit 0 before the fix. The header already claimed aliases were resolved, which made this worse than a plain gap — the documented contract and the code disagreed. Ports the expansion block from block-dangerous-git verbatim, including the HOOK_NO_ALIAS one-level guard, and runs it BEFORE the `sub == commit` check rather than after. A canonical commit reached through an alias still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ogVV7z2Heg9ojJ88nqNxh
|
Claude finished @kyle-sexton's task in 3m 14s —— View job Code Review — PR #736 (updated after
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb214a4617
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…h guard --no-edit was exempt unconditionally, but git accepts it for an ordinary commit — `git commit --no-edit -m subject` created exactly the noncanonical commit the guard exists to stop. Amending is already covered by --amend and a merge's --no-edit by the sequencer branch, so the exemption is simply wrong and is removed. Aliases persisted in .git/config or a global config were invisible: the parser only captures inline `-c` values, so `git config alias.c commit` followed by `git c -m x` passed. The hook now asks git to resolve the alias when no inline one matched, letting git's own precedence apply. Sequencer probing ignored `git -C <path>`, so a conflict resolution driven at another repo read the session cwd's state instead of the target's — exempting the wrong repo in one direction and gating a real merge in the other. README advertised eight guards, five blocking, and named the advisory as the owner of direct `git commit`; all three are now stale. All four reproduced before fixing and covered by tests (44 assertions, including real git fixtures for the persisted alias and the -C target). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ogVV7z2Heg9ojJ88nqNxh
|
Claude finished @kyle-sexton's task in 4m 11s —— View job Code Review — PR #736 (updated after
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ca0266b6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
git uses the LAST -c value for a key, so `-c alias.c=status -c alias.c=commit c -m x` commits. Breaking on the first match let a decoy earlier value expanding to a harmless subcommand mask the real one — verified exit 0 before the fix, and the earlier persisted-alias test had masked it by blocking for an unrelated reason. Also stops the persisted-config lookup from re-running after an inline alias already matched. --config-env is left unresolved and documented as a residual rather than half-fixed: the shared parser records its value undifferentiated from -c, so the environment variable NAME arrives in place of the expansion. Separating them is a hook-utils change block-dangerous-git shares, filed as its own issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ogVV7z2Heg9ojJ88nqNxh
|
Claude finished @kyle-sexton's task in 3m 6s —— View job Code Review — PR #736 (updated after
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2552ee2a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`git --git-dir=X --work-tree=Y commit -m x` concludes a merge in X, but the probe read the session cwd instead, so a real in-progress merge was refused. A false block rather than a bypass — the safe direction — but still wrong, and it would strand exactly the conflict resolution the exemption exists to protect. Same family as the -C fix: the command names the repo, and the guard has to read that one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ogVV7z2Heg9ojJ88nqNxh
|
Claude finished @kyle-sexton's task in 2m 8s —— View job Code Review — PR #736 (updated after
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3efc8d572d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…git parser (#903) ## What The shared git-option parser `hook::git_resolve_subcommand` (`lib/hook-utils.sh`) collected `-c`, `--config`, and `--config-env` values into one `HOOK_GIT_CONFIG_VALUES` array with no marker distinguishing them. But `--config-env=<key>=<envvar>` supplies the **name of an environment variable** holding the value (git reads it at runtime), not the value itself. Both git guards read the env-var name as the literal alias expansion, so: - `block-noncanonical-commit` — `git --config-env=alias.z=AV z` with `AV=commit` was not recognized as a commit (verified exit 0 → **fail open**). - `block-dangerous-git` — the same shape for its alias-expansion path (`AV='reset --hard'`). Neither is a bug in the guards; the information they need was not in the array. Closes #740. ## Fix - **Parser tags origin** — `hook::git_resolve_subcommand` fills a parallel `HOOK_GIT_CONFIG_VALUE_KINDS` array (`"inline"` for `-c`/`--config`, `"env"` for `--config-env`), for both the two-word and `=`-attached forms. - **New resolver** — `hook::git_effective_config_values` projects each value to its EFFECTIVE assignment: inline values pass through; an `env` value `<key>=<envvar>` resolves to `<key>=${!envvar}` against the hook's inherited environment (the same environment git reads), gated on the env-var name being a valid shell identifier and using `${!envvar-}` for `set -u` safety. An unset or invalid-name variable projects to an empty value — git itself rejects an unset `--config-env` variable (fatal), so the assignment never takes effect and the empty projection yields no spurious alias match. - **Both guards** switch their alias-resolution loop from `HOOK_GIT_CONFIG_VALUES` to the resolved `HOOK_GIT_CONFIG_EFFECTIVE`. `block-no-verify.sh` is intentionally unchanged: it keys on the config **key** (`core.hooksPath=`), which survives resolution, so it is not affected by the env/value confusion. ## Review-driven hardening (two further fail-opens of the same guards) An independent security review of the initial fix found two more residual bypasses of these guards — both verified against real git behavior — now closed on this branch: - **Command-line env scope.** The resolver first read only the hook's *ambient* environment, but `AV=commit git --config-env=alias.c=AV c` (inline prefix) and `env AV=commit git …` set the variable only in *git's* environment — a self-contained one-liner that passed an ambient-only check. `hook::git_resolve_index` now collects the command-line assignments it already walks past (both the inline prefix and the `env` wrapper) into `HOOK_GIT_ENV_ASSIGNMENTS`, and the resolver prefers them over ambient (last wins), matching what git's process actually sees. The identifier gate on the indirect expansion is pinned by an injection-shaped-name test proving no evaluation occurs (no file created). - **Case-insensitive alias matching.** git config names are case-insensitive, so `git -c alias.RH='reset --hard' rh` and `git -c alias.rh=… RH` both run the alias, yet the guards' inline-alias re-check matched case-sensitively. Both guards now fold both sides of the alias-key match (the expansion value keeps its case). The `--config-env` inline-prefix case that the issue documented as out of scope is thus specifically covered here for config resolution; the parser still does not evaluate shell assignments generally. ## Blast radius `hook-utils.sh` is a shared library materialized into every carrying plugin via `scripts/sync-hook-utils.sh`, and the delivery gate requires every carrying plugin to bump its version (the plugin version is the consumer update-cache key). So this change syncs the lib to all 11 carrying plugins with a patch bump + changelog entry each (guardrails carries the real fix note; the other 10 note a no-behavior-change shared-lib sync). ## Tests - `lib/hook-utils.test.sh` — parser kind tagging (both forms); effective-value projection (env resolved, inline pass-through, unset → empty, invalid/injection-shaped identifier → empty with no evaluation); and command-line-assignment resolution (inline prefix, `env` wrapper, override-of-ambient) through the full `git_resolve_index` path. - Both guards' contract suites — env-sourced alias bypass blocked (`=` and two-word forms), inline-prefix and `env`-wrapper bypass blocked, case-folded alias (upper subcommand / upper key) blocked, safe env alias allowed, unset var allowed (git rejects it), injection-shaped env-var name allowed with an asserted no-file (identifier-gate pin). - All green locally: `lib/hook-utils.test.sh` (75), `block-noncanonical-commit` (59), `block-dangerous-git` (203); `shellcheck --rcfile=.shellcheckrc`; `sync-hook-utils.sh --check`/`--check-bump`; `check-changelog-parity.sh --check`/`--check-bump`; `validate-plugins.sh`; markdownlint — all pass. ## Related - Refs #731 / #736 — where the `--config-env` residual surfaced; #736 documented it in the new hook's header rather than half-fixing it in one consumer. This PR fixes it in the shared parser so both guards inherit the fix. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Closes #731
Summary
Turns the
git commithalf offlag-commit-pr-skill-bypassinto a blocking guard, and fixes a latent false positive that would have made blocking impossible.Fix
New
block-noncanonical-commit.sh(blocking). Deniesgit committhat does not pipe its message via-F -/--file -. That form prevents a real, silent failure:git commit -m "<multi-line>"flattens newlines unpredictably across shells, so a body that looked right in the tool call lands mangled in history.Exempt, because no message-on-stdin form exists for them and gating them would strand real work:
--amend/--no-edit,-C/-c/--reuse-message/--reedit-message,--fixup/--squash-F <path>— mechanic satisfied, just not via stdinKill switch
block_noncanonical_commit_enabled; allow-listblock_noncanonical_commit_allow(message-flagpermits a bare-m), following theblock_dangerous_git_allowidiom. Detection reuses the shared argv-grammar parser, sobash -lcwrappers and git aliases resolve, and a commit body mentioninggit commit -mnever fires.Dropped the
--trailerconjunct — this was a latent bug. The old condition required-F -and--trailer:But
/commitomits--trailerwhen the resolvedtrailer_policyisnone(plugins/source-control/skills/commit/SKILL.md:58,66-70,106). So a repo whose convention forbids a co-author trailer saw the skill's own conformant output flagged on every commit.This had to be settled before blocking on the same condition — requiring
--trailerto pass would have permanently blocked/commitin that configuration. The trailer is policy; only the stdin form is mechanic, and only the mechanic belongs in a gate.flag-commit-pr-skill-bypassis nowgh pr create-only, so the two never double-fire on one command.Why
gh pr createstays advisoryNot a preference — it cannot be otherwise.
/pull-request createissues that exact command itself, and anthropics/claude-code#22655 (exposeskill_nameto hook payloads) is closed as not planned. A hook cannot distinguish a skill-driven Bash call from an ad hoc one, so blockinggh pr createwould deadlock the skill it is advertising.Likewise, no hook can force skill invocation — hooks block tools, they do not direct the model. Gating on command shape is what is actually available, and is the better target anyway: it enforces an outcome verifiable in
git lograther than the ceremony that produced it.Verification
New hook — 34/34, covering both directions plus the config surface:
The sequencer cases run against a real
git initfixture withMERGE_HEADpresent and then removed, so the exemption is proven to key on actual repo state rather than on the flag.Advisory test rescoped to
gh pr create— 17/17, including a case assertinggit commit -mis now silent here, and one asserting-F -without--traileris silent (the regression this PR fixes).shellcheckon all changed scripts — clean.scripts/validate-plugin-contracts.mjs— 33 setup skills, 1825 plugin files checked, pass. Both new scripts committed100755. Version0.8.0→0.9.0with a matching## [0.9.0]heading.This PR's own commit was made with
git commit -F - --trailer ... --trailer ...and the advisory did not fire — which is also the direct evidence that the pre-existing advisory had been correct all along about the commits that did trip it.Related
skill_namein hook payload (closed, not planned).