fix(guardrails): distinguish --config-env from -c/--config in shared git parser

[kyle-sexton]

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 feat(guardrails): block non-canonical git commit; fix trailer-policy false positive #731 / feat(guardrails): block non-canonical git commit; drop --trailer conjunct #736 — where the --config-env residual surfaced; feat(guardrails): block non-canonical git commit; drop --trailer conjunct #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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a03ed406cb

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 5m 35s —— View job

---

Security Review

• Read PR diff and changed files (lib/hook-utils.sh, guard scripts, tests)
• Analyze hook::git_effective_config_values resolver logic
• Trace control flow through both guards for the non-identifier env var case
• Verify Codex P1 finding against guard logic and test coverage
• Check for additional injection surfaces and fail-open conditions

---

Finding 1 — Fail-open bypass via non-identifier env-var name in --config-env

Severity: IMPORTANT | Confidence: HIGH

The Codex P1 comment is correct and warrants deeper explanation.

Root cause. hook::git_effective_config_values resolves --config-env entries only when the env-var name passes ^[A-Za-z_][A-Za-z0-9_]*$. When the name fails that gate (e.g. bad-name, which contains a hyphen), the code falls to the else branch and projects an empty value — alias.c= — rather than blocking:

claude-code-plugins/lib/hook-utils.sh

Lines 895 to 896 in ba3af53

	else
	HOOK_GIT_CONFIG_EFFECTIVE+=("${key}=")

Why git DOES resolve it. git's --config-env implementation calls the C standard getenv(env_name) on whatever string appears after the =. On Linux, getenv("bad-name") performs a linear scan of environ for a string starting with bad-name= — it does not validate identifier syntax. The env(1) command populates environ directly via execve, so env 'bad-name=commit' git --config-env=alias.c=bad-name c -m x is a working git invocation; git finds bad-name=commit in its environment and uses commit as the alias expansion.

Control-flow trace through the guards. For the same command:

1. The segment tokeniser sees env, skips into the env wrapper handler, then encounters bad-name=commit. That token contains =, so it reaches the assignment branch in hook::git_resolve_index. It fails the identifier check, so it is not added to HOOK_GIT_ENV_ASSIGNMENTS — but it is still skipped (the continue runs unconditionally). The resolver then finds git and continues normally.

2. hook::git_resolve_subcommand records alias.c=bad-name with kind "env" and sets sub="c".

3. hook::git_effective_config_values sees envvar="bad-name", fails the identifier gate, and pushes alias.c= (empty value) into HOOK_GIT_CONFIG_EFFECTIVE.

4. block-noncanonical-commit: the alias loop matches cv="alias.c=" against alias.${sub}=* — TRUE — and sets exp="". Then if [[ -n "$exp" ]]; then is FALSE, so inline_alias_handled stays 0. The git config --get "alias.c" fallback returns nothing (the alias is command-line-only). The guard reaches [[ "$sub" == "commit" ]] || return 0 — sub is "c", not "commit" → return 0 (allow). The guard fails open.

5. block-dangerous-git: the alias loop matches and breaks with exp="". The else branch calls hook::env_s_split "" (empty), then check_segment git with no subcommand — harmless → return 0 (allow). For a dangerous variant like env 'bad-name=reset --hard' git --config-env=alias.rh=bad-name rh, the guard similarly allows it.

The test gap. The new test confirms that injection-shaped names (e.g. $(touch ...)) neither evaluate nor match. The test comment reads "allowed (git rejects it)", which is correct for names containing characters git's getenv call would never find in a normal environment — but the comment implies that ALL non-identifier names are safe for git, which is not true for names that env(1) can place in the process environment (hyphens, dots, etc.).

claude-code-plugins/plugins/guardrails/hooks/block-noncanonical-commit.test.sh

Lines 111 to 114 in ba3af53

	"AV=commit git --config-env=alias.c=AV c" 2
	run "config-env alias with an env-wrapper env var (blocked)" \
	"env AV=commit git --config-env=alias.c=AV c" 2
	# The env-var NAME is resolved through bash indirect expansion, gated to valid shell

Suggested fix. When a --config-env env-var name is not a valid shell identifier, the correct fail-safe is to block (fail closed) rather than project an empty value and allow. The empty projection is safe against evaluation, but it silently discards a config assignment that git will actually act on. The simplest change is to replace the else branch in hook::git_effective_config_values with a sentinel that causes the guards to treat the entry as unresolvable-and-dangerous:

else
  # Non-identifier name: cannot resolve safely in bash; fail closed by
  # treating this as a non-empty, unrecognised alias value so the guard
  # blocks rather than allowing through.
  HOOK_GIT_CONFIG_EFFECTIVE+=("${key}=<unresolvable>")
fi

Any non-empty, non-matching expansion string causes block-noncanonical-commit to set inline_alias_handled=1 (then recurse via hook::env_s_split "<unresolvable>" which yields the word <unresolvable> as a subcommand — harmless but at least closed) and causes block-dangerous-git to break with a non-empty exp which is re-parsed. A cleaner approach is to add an explicit guard in each consumer: if the resolved value signals "unresolvable", block outright. The exact mechanism is less important than the direction: non-identifier names should not silently allow.

Fix this →

---

No other security findings

The remaining changes are structurally sound:

• Identifier-gated indirect expansion (${!envvar-}) prevents command injection: the =~ regex gate runs before any indirect lookup, and the test that pins it (the $(touch pwned) case in both guard test suites) confirms no evaluation occurs for injection-shaped names.
• Last-wins semantics for HOOK_GIT_ENV_ASSIGNMENTS correctly mirrors what git's process sees.
• Case-fold on alias key matching (${cv,,} / ${sub,,}) is applied only to the key side; the expansion value keeps its original case, so no correctness issue there.
• The HOOK_GIT_CONFIG_VALUE_KINDS parallel array is maintained 1:1 with HOOK_GIT_CONFIG_VALUES across all four parse branches (two-word and attached forms for both --config and --config-env), which is correct.

[claude]

Claude finished @kyle-sexton's task in 7m 23s —— View job

---

PR Review

• Gather context from PR body and changed files list
• Read core changed files: lib/hook-utils.sh, lib/hook-utils.test.sh
• Read guard scripts: block-noncanonical-commit.sh, block-dangerous-git.sh
• Read guard test files
• Analyze security properties of the fix
• Assess the Codex review comment on non-identifier env var bypass
• Check test coverage breadth
• Review changelog/version bump pattern
• Post final review

---

Summary

The core design is correct and well-tested. The parser tagging, effective-value resolver, case-folding, and "last wins" semantics are all sound. However the Codex P1 comment identifies a real gap that the PR description incorrectly reasons away. One confirmed bypass remains. Details below.

---

Finding 1 — CONFIRMED bypass: non-identifier env var names fail open

Severity: Security / block before merge

The Codex reviewer is correct. The guard's comment at lib/hook-utils.sh:868 says:

A named variable that is unset, or a name that is not a valid shell identifier, resolves to an empty value: git rejects an unset --config-env variable (fatal), so the assignment never takes effect…

This reasoning conflates two distinct cases:

• Unset variable — git DOES reject it (fatal). Empty projection is safe.
• Non-identifier name that IS set — git uses getenv() (C library), which can read any OS-level env var regardless of whether the name is a valid shell identifier. The empty projection is not safe here.

Exploitable path:

env 'bad-name=commit' git --config-env=alias.c=bad-name c -m x

Flow through the guard:

1. hook::git_resolve_index sees bad-name=commit. It has =, fires the *=* branch, fails the identifier regex ^[A-Za-z_][A-Za-z0-9_]*= (hyphen), and is not added to HOOK_GIT_ENV_ASSIGNMENTS. Execution advances past it to git.
2. hook::git_effective_config_values resolves --config-env=alias.c=bad-name: bad-name fails ^[A-Za-z_][A-Za-z0-9_]*$, projects to alias.c= (empty).
3. Alias match for c fails → guard exits 0 (allowed).
4. But git calls getenv("bad-name") = "commit" and runs the alias.

The same shape works for block-dangerous-git (env 'AV=reset --hard' git --config-env=alias.rh=AV rh).

Note the inline-prefix form (bad-name=commit git …) is not exploitable — the shell treats a non-identifier name=val as a command name, not an env assignment, so git never sees it. Only the env '…' wrapper is vulnerable.

Minimal fix:

The identifier gate on COLLECTION (hook::git_resolve_index, line 654) should not filter out tokens that appear after an env command — env uses execve() and CAN set non-identifier names. One path: also capture non-identifier name=value tokens into HOOK_GIT_ENV_ASSIGNMENTS (or a parallel HOOK_GIT_ENV_ASSIGNMENTS_RAW), and in hook::git_effective_config_values, for non-identifier $envvar, do a string-prefix search against that collection instead of the forbidden ${!envvar} indirect expansion. The identifier gate stays as the guard against indirect-expansion injection specifically.

There is also a missing test for this scenario — the existing injection-shaped test ($(touch ...)) pins the RCE gate but a test like:

run "env-wrapper non-identifier env var (blocked)" \
  "env 'bad-name=commit' git --config-env=alias.c=bad-name c" 2

would have caught this gap.

Fix this →

---

Finding 2 — Dead code in env handler

Severity: Cosmetic

lib/hook-utils.sh env handler:

while ((i < n)) && [[ "${w[i]}" == -* ]]; do
  case "${w[i]}" in
  ...
  -*) ((i++)) ;;
  *) ((i++)) ;;   # ← unreachable: while condition requires token to start with -
  esac
done

The while condition requires ${w[i]} to start with -. Any token inside the loop that reaches the case either matches a specific -… pattern or falls to -). The bare *) is never reached. Not a correctness or security issue, but it will confuse a future reader.

---

What the PR gets right

The rest of the fix is well-executed:

• Parallel array initialization — HOOK_GIT_CONFIG_VALUE_KINDS=() is reset alongside HOOK_GIT_CONFIG_VALUES=() in hook::git_resolve_subcommand and the kind="${…:-inline}" fallback is defensive.
• Case folding — [[ "${cv,,}" == "alias.${sub,,}="* ]] with exp="${cv#*=}" (original case) is correct: key match is case-insensitive but the expansion value is preserved.
• "Last wins" semantics — the HOOK_GIT_ENV_ASSIGNMENTS loop overwrites val on each match, matching git's behavior for repeated -c assignments.
• Injection gate pins — assert_file_absent tests verify that command-substitution injection shapes ($(touch …)) are never evaluated.
• block-no-verify.sh rationale — the guard keys on config KEY (core.hooksPath=), which survives --config-env resolution unchanged. Correctly left alone.
• HOOK_GIT_ENV_ASSIGNMENTS+x guard — protects hook::git_effective_config_values against being called before hook::git_resolve_index sets the array.
• Blast-radius handling — syncing the shared lib to all 11 carrying plugins with a patch version bump and individual changelog entries is the right approach.
• Test breadth — kind tagging, inline pass-through, unset-env empty projection, injection gate pin, inline-prefix/env-wrapper command-line override, ambient override, case-folded alias variants — all covered. Only the non-identifier-but-set path is missing.

---

The one change needed before merge is the non-identifier env '…' bypass (Finding 1). Everything else is solid.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba3af533d0

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 4m 30s —— View job

---

PR Review

• Read core changed files (lib/hook-utils.sh, guard scripts, tests)
• Verify prior reviewer findings against current code
• Check for additional issues
• Post complete review

---

Both prior security findings are confirmed against the current code. Two P1 issues must be fixed before merge. The rest of the implementation is sound.

---

Finding 1 — CONFIRMED: Non-identifier env-var name bypasses both guards

Severity: P1 / Security

The prior reviews correctly identify this. Tracing through the current code:

Attack vector:

env 'bad-name=commit' git --config-env=alias.c=bad-name c -m x

Trace through block-noncanonical-commit (same applies to block-dangerous-git):

1. The outer tokenizer in hook::git_resolve_index sees bad-name=commit (after the env handler exhausts options and continues back). It has =, hits the *=* branch, but fails the identifier regex ^[A-Za-z_][A-Za-z0-9_]*= (hyphen). Not added to HOOK_GIT_ENV_ASSIGNMENTS. Silently skipped.

2. hook::git_effective_config_values sees --config-env=alias.c=bad-name. bad-name fails ^[A-Za-z_][A-Za-z0-9_]*$, falls to the else branch, and projects alias.c= (empty value) into HOOK_GIT_CONFIG_EFFECTIVE.

3. In block-noncanonical-commit, the alias loop matches alias.c= for subcommand c, gets exp="", if [[ -n "$exp" ]]; then is false, inline_alias_handled stays 0. The git config --get fallback finds nothing (command-line-only alias). The guard checks sub == "commit" — it's "c" — and returns 0. Fail open.

4. In block-dangerous-git, the loop matches alias.c=, gets exp="", calls hook::env_s_split "" (yields no words), check_segment git with no subcommand — harmless — returns 0. Fail open.

Why the comment at lib/hook-utils.sh:868–871 is wrong for this case:

"A named variable that is unset, or a name that is not a valid shell identifier, resolves to an empty value: git rejects an unset --config-env variable (fatal), so the assignment never takes effect…"

This is correct for unset variables. It is wrong for non-identifier names supplied via env '…': git calls C's getenv("bad-name") which does a linear scan of the process environ array and finds bad-name=commit regardless of POSIX identifier rules. The env(1) command populates environ via execve, so the value is there when git looks.

The inline-prefix form (bad-name=commit git …) is NOT exploitable — the shell treats a non-identifier name=val as a command name, not an env assignment. Only the env '…' wrapper is vulnerable.

Missing test: The existing injection test at line 118–120 of block-noncanonical-commit.test.sh and line 222–225 of block-dangerous-git.test.sh cover command-substitution injection shapes ($(touch ...)), but not the env 'bad-name=…' exploit above. The label "allowed — not an identifier" is misleading since such a command is NOT blocked by git.

Fix direction (two options):

Option A — Fail closed with a sentinel: Replace the else branch with a non-empty sentinel so both guards treat the entry as unresolvable-and-dangerous:

else
  # Cannot resolve non-identifier name via ${!…}; env(1) can set it for
  # git via execve. Fail closed so the guards block rather than allow.
  HOOK_GIT_CONFIG_EFFECTIVE+=("${key}=<unresolvable>")
fi

This is conservative (may block valid, harmless uses of non-identifier env-var names) but is the simplest safe path.

Option B — Collect and linearly resolve: Collect ALL name=value tokens in the env handler (including non-identifier ones) into a separate raw array (HOOK_GIT_ENV_ASSIGNMENTS_RAW). In hook::git_effective_config_values, for non-identifier $envvar, do a string-prefix linear scan of that raw array instead of ${!envvar}. This matches git's actual behavior more precisely.

Fix this →

---

Finding 2 — CONFIRMED: block-dangerous-git alias loop breaks on first match, not last

Severity: P1 / Security

The Codex P1 inline comment on block-dangerous-git.sh:236 is correct.

The discrepancy:

• block-noncanonical-commit.sh (line 223–228): iterates all config values, updates exp on each match (no break). Last matching value wins — correct git behavior.
• block-dangerous-git.sh (line 236–257): iterates, finds first match, acts immediately and breaks. First value wins — wrong.

Attack vector:

AV='reset --hard' git -c alias.rh=status --config-env=alias.rh=AV rh

After hook::git_effective_config_values, cfgv = ["alias.rh=status", "alias.rh=reset --hard"].

The for loop at line 232: first iteration matches alias.rh=status, sets exp="status", calls check_segment git status (harmless), then break. The dangerous alias.rh=reset --hard is never processed. Guard exits 0. Fail open.

Note: git's actual behavior runs reset --hard because the last -c/--config-env assignment for a key wins. The guard has it backwards.

Missing test: block-dangerous-git.test.sh has no "last alias wins" test. block-noncanonical-commit.test.sh correctly has lines 86–91 covering both orderings. block-dangerous-git needs the same.

Fix: Restructure the alias loop to accumulate the last-matching expansion before acting, mirroring block-noncanonical-commit.sh:

# block-dangerous-git.sh — replace break-on-first with last-wins accumulation
local last_exp="" last_is_shell=0
for cv in ${cfgv[@]+"${cfgv[@]}"}; do
  [[ "${cv,,}" == "alias.${sub,,}="* ]] || continue
  last_exp="${cv#*=}"
  [[ "$last_exp" == '!'* ]] && last_is_shell=1 || last_is_shell=0
done
if [[ -n "$last_exp" ]]; then
  if ((last_is_shell)); then
    reparse="${last_exp#!}"
    for a in "${w[@]:sub_idx+1}"; do reparse+=" $(printf '%q' "$a")"; done
    hook::bash_parse_segments "$reparse" check_segment
  else
    hook::env_s_split "$last_exp"
    expw=(${HOOK_ENV_S_WORDS[@]+"${HOOK_ENV_S_WORDS[@]}"})
    HOOK_NO_ALIAS=1
    check_segment "${w[@]:0:gi+1}" ${expw[@]+"${expw[@]}"} "${w[@]:sub_idx+1}"
    HOOK_NO_ALIAS=0
  fi
fi

Also add the missing "last alias wins" test cases analogous to block-noncanonical-commit.test.sh:88–91.

Fix this →

---

Finding 3 — Cosmetic: Dead code in env handler

lib/hook-utils.sh:688:

while ((i < n)) && [[ "${w[i]}" == -* ]]; do
  case "${w[i]}" in
  ...
  -*) ((i++)) ;;
  *) ((i++)) ;;   # ← unreachable: while condition requires token starts with -
  esac
done

Any token reaching the case inside this loop must start with - (loop guard). The *) ((i++)) arm is unreachable. Not a correctness or security issue, but dead code that may confuse a reader. (The sudo handler at lines 711–717 has the same pattern; both should have the dead arm removed.)

---

What the PR gets right

The core design and most of the implementation are solid:

• Parallel kind array (HOOK_GIT_CONFIG_VALUE_KINDS) is maintained 1:1 with HOOK_GIT_CONFIG_VALUES across all four parse branches. The "env" vs "inline" tagging is correct for both two-word and attached forms.
• ${!envvar-} indirect expansion is correctly gated on the identifier regex before use, preventing command-injection through crafted env-var names.
• Last-wins semantics for HOOK_GIT_ENV_ASSIGNMENTS (lines 889–892) correctly mirrors what git's process sees.
• HOOK_GIT_ENV_ASSIGNMENTS+x guard (line 889) correctly protects against calling the effective-values function before hook::git_resolve_index has run.
• Case-fold on alias key matching (${cv,,}/${sub,,}) is applied only to the key; the expansion value retains its case. Correct.
• block-noncanonical-commit.sh alias loop already uses last-wins correctly (no break). The test suite covers both orderings (lines 88–91).
• block-no-verify.sh left unchanged — it keys on config key (core.hooksPath=) which is kind-agnostic, so the env/inline distinction is irrelevant. Correct reasoning.
• Blast radius handling — syncing hook-utils.sh to all 11 carrying plugins with a patch bump and individual changelog entries is correct.
• Injection gate pins (assert_file_absent for $(touch ...) shapes) in both guard test suites confirm the identifier gate prevents evaluation.

---

Summary

Two security issues need to be fixed before this can merge:

1. Non-identifier env-var name via env '…' wrapper — both guards fail open. The else branch at hook-utils.sh:895-896 should fail closed (sentinel value or raw-assignment collection), and the misleading comment at line 868-871 needs correction. New tests needed in both guard suites.

2. First-alias-wins in block-dangerous-git — the break at line 256 must be removed and the loop restructured to last-wins, matching block-noncanonical-commit.sh. A "last alias wins" test suite (both orderings) is missing from block-dangerous-git.test.sh.

Once these two items are addressed, the branch should be in a mergeable state.

[claude]

Claude finished @kyle-sexton's task in 4m 47s —— View job

---

Security Review

• Load tools and set up todo list
• Read lib/hook-utils.sh (core parser changes)
• Read plugins/guardrails/hooks/block-dangerous-git.sh
• Read plugins/guardrails/hooks/block-noncanonical-commit.sh
• Read test files for gap analysis
• Analyze for: injection surfaces, fail-open conditions, logic flaws, unsafe expansion
• Cross-check prior review findings vs current code
• Post security findings

---

Two confirmed security findings remain in this branch. Both were identified by prior automated reviews (Codex P1 and the prior Claude security reviews) and have not been addressed.

---

Finding 1 — IMPORTANT | Confidence: HIGH

Non-identifier --config-env env-var names fail open in both guards

Location: lib/hook-utils.sh:895–896

The comment at lines 868–871 states:

A named variable that is unset, or a name that is not a valid shell identifier, resolves to an empty value: git rejects an unset --config-env variable (fatal), so the assignment never takes effect…

This reasoning conflates two distinct cases. The "fatal" claim is correct for unset variables, but not for non-identifier names that are set. git's --config-env implementation calls the C library getenv(env_name) on whatever string follows the =. On Linux, getenv("bad-name") does a linear scan of environ for an entry starting with bad-name= — it does not require the name to be a POSIX identifier. The env(1) command populates environ directly via execve(), so the following is a live bypass:

env 'bad-name=commit' git --config-env=alias.c=bad-name c -m x

Trace through block-noncanonical-commit:

1. hook::git_resolve_index sees bad-name=commit — it fails ^[A-Za-z_][A-Za-z0-9_]*= (hyphen), so it is not added to HOOK_GIT_ENV_ASSIGNMENTS. Execution advances to git.
2. hook::git_effective_config_values resolves --config-env=alias.c=bad-name: bad-name fails ^[A-Za-z_][A-Za-z0-9_]*$, falls to the else branch, and pushes alias.c= (empty) into HOOK_GIT_CONFIG_EFFECTIVE.
3. The alias loop sets exp="" → [[ -n "$exp" ]] is false → inline_alias_handled stays 0 → git config alias.c fallback returns nothing (command-line-only) → [[ "$sub" == "commit" ]] is false (sub is "c") → return 0 (allowed).
4. git calls getenv("bad-name") = "commit", expands the alias, and runs git commit -m x.

The same shape applies to block-dangerous-git with env 'bad-name=reset --hard' git --config-env=alias.rh=bad-name rh.

Why this is distinct from the injection-shaped test that already exists in the test suite: the existing test pins that $(touch ...) inside an env-var name is never evaluated (correct). But the comment in both test files labels non-identifier names as "allowed — git rejects it", which is only true when the variable is unset, not when it is injected via env '...'. There is no test for env 'bad-name=commit' git --config-env=alias.c=bad-name c expecting exit 2.

Suggested fix: In hook::git_effective_config_values, replace the empty projection in the else branch with a sentinel that causes consumers to fail closed:

else
  # Non-identifier name cannot be resolved safely via bash indirect
  # expansion. Fail closed: treat as an unresolvable alias value so
  # guards block rather than allow.
  HOOK_GIT_CONFIG_EFFECTIVE+=("${key}=__unresolvable__")
fi

A non-empty, non-matching expansion causes block-noncanonical-commit to set inline_alias_handled=1 (it then re-parses the sentinel word, which is harmless), and causes block-dangerous-git to break with a non-empty exp that is re-parsed as __unresolvable__ (also harmless). Alternatively, collect non-identifier NAME=VALUE tokens from env '...' invocations into a raw-assignment parallel array and do a linear string-prefix scan (safe, no ${!...}) for the non-identifier lookup.

Fix this →

---

Finding 2 — IMPORTANT | Confidence: HIGH

block-dangerous-git.sh uses first-match alias semantics; git uses last-match

Location: plugins/guardrails/hooks/block-dangerous-git.sh:256

block-noncanonical-commit.sh was correctly updated in this PR to accumulate exp across the entire alias loop and act on the last match (mirroring git's "last -c value wins" semantics). block-dangerous-git.sh was not. Its loop still breaks on the first matching alias value (line 256), which means a decoy harmless alias placed before the dangerous one masks the real expansion:

AV='reset --hard' git -c alias.rh=status --config-env=alias.rh=AV rh

Trace:

1. After effective resolution, cfgv = ["alias.rh=status", "alias.rh=reset --hard"].
2. First match: alias.rh=status → check_segment git status → allowed → break.
3. alias.rh=reset --hard is never evaluated by the guard.
4. git applies the last config value: alias.rh=reset --hard → runs git reset --hard → bypass.

This is structurally identical to the block-noncanonical-commit fix that was applied in this PR (see the LAST value wins comment at block-noncanonical-commit.sh:219–222). There is no corresponding "last alias value wins" test in block-dangerous-git.test.sh.

Suggested fix: Mirror the noncanonical-commit pattern: accumulate exp through the full loop, then act outside it:

exp=""
for cv in ${cfgv[@]+"${cfgv[@]}"}; do
  [[ "${cv,,}" == "alias.${sub,,}="* ]] && exp="${cv#*=}"
done
if [[ -n "$exp" ]]; then
  if [[ "$exp" == '!'* ]]; then
    ...hook::bash_parse_segments...
  else
    ...check_segment...
  fi
fi

Fix this →

---

What is sound

The rest of the implementation is correctly executed:

• ${!envvar-} indirect expansion is gated by ^[A-Za-z_][A-Za-z0-9_]*$ before any lookup; the injection pin test ($(touch ...)) confirms no evaluation occurs for non-identifier names. The gate itself is correct.
• "Last wins" semantics in HOOK_GIT_ENV_ASSIGNMENTS lookup — the inner for a in ... loop overwrites val on each match, correctly mirroring what git's process sees.
• Case folding (${cv,,} / ${sub,,}) is applied only to the key side of the alias match; exp="${cv#*=}" preserves original case on the expansion value. Correct.
• Parallel HOOK_GIT_CONFIG_VALUE_KINDS array is maintained 1:1 with HOOK_GIT_CONFIG_VALUES across all four parse branches (two-word and attached forms for both --config and --config-env). Correct.
• HOOK_GIT_ENV_ASSIGNMENTS+x guard handles the case where hook::git_effective_config_values is called without first calling hook::git_resolve_index; falls back safely to ambient env. Correct.
• block-no-verify.sh is correctly left unchanged (it keys on config key core.hooksPath=, not value, which survives resolution intact).

Both unaddressed findings are bypasses of the same two guards this PR is trying to harden. The PR should not merge until Finding 1 and Finding 2 are resolved.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba3af533d0

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 7m 40s —— View job

---

PR Review — Commit 2f8bf01

• Load tools
• Read lib/hook-utils.sh — check Finding 1 fix (non-identifier env-var bypass)
• Read block-dangerous-git.sh — check Finding 2 fix (first-vs-last alias semantics)
• Read guard test files — verify new test coverage
• Assess Codex Finding 3 (shell-alias recursion env scope)
• Post final review

---

Result: 1 of 3 prior P1 findings is now fully closed. 2 remain open.

---

Finding 1 — PARTIALLY addressed (new gap remains)

Prior finding: Non-identifier env-var names (env 'bad-name=commit' git --config-env=alias.c=bad-name c) fail open in both guards.

What the new commit does: Replaces ${!envvar} with printenv "$envvar" in hook::git_effective_config_values. This is the correct primitive — printenv wraps getenv() and can resolve non-POSIX-identifier names that ${!...} bash indirect expansion would reject. For the ambient-environment case (where bad-name is already in the hook process's environment), this now works correctly. The new test at block-noncanonical-commit.test.sh:118–119 validates this path:

run "config-env alias with a non-identifier env name (blocked)" \
  "git --config-env=alias.c=bad-name c" 2 bad-name=commit

The run harness sets bad-name=commit in the hook subprocess's ambient environment via env "$@" bash "$HOOK" (line 28). The hook's printenv "bad-name" then finds it. This case is now correctly blocked. ✓

Remaining gap — command-line env '...' wrapper:

The attack vector from the prior reviews is still live:

env 'bad-name=commit' git --config-env=alias.c=bad-name c -m x

Trace through the current code:

1. hook::git_resolve_index encounters env, enters the handler, increments i. The inner while exits immediately because bad-name=commit doesn't start with -. continue returns to the outer loop.

2. The outer loop at lib/hook-utils.sh:649–656 sees bad-name=commit. It matches *=*, but the identifier gate ^[A-Za-z_][A-Za-z0-9_]*= rejects the hyphen, so bad-name=commit is not added to HOOK_GIT_ENV_ASSIGNMENTS. It is silently skipped.

3. In hook::git_effective_config_values (line 896): printenv "bad-name" reads from the hook process's ambient environment. The hook hasn't executed env 'bad-name=commit'; it only received the command string to analyze. bad-name is not in the hook's ambient environment → val="".

4. The HOOK_GIT_ENV_ASSIGNMENTS override loop at lines 900–902 finds nothing for bad-name (step 2 dropped it) → val stays empty.

5. HOOK_GIT_CONFIG_EFFECTIVE = ["alias.c="]. Both guards fail open.

The collection gate at line 654 is the root cause. Its comment correctly explains it for inline-prefix (AV=commit git … — the shell only treats identifier-named tokens as assignments, so non-identifier ones are command words). But the same gate runs over tokens that appear after env flags, where env CAN accept any name via execve(). The printenv resolver is correct; the collector is not.

Missing test: Neither guard test suite has a case for the env '...' wrapper form with a non-identifier name:

run "env-wrapper non-identifier env var (blocked)" \
  "env 'bad-name=commit' git --config-env=alias.c=bad-name c" 2

Adding this would expose the gap (it would fail).

Suggested fix: Have the env handler in hook::git_resolve_index collect NAME=VALUE tokens (including non-identifier ones) before the outer loop sees them, since env is explicitly able to set any name:

env)
  ((i++))
  while ((i < n)) && [[ "${w[i]}" == -* ]]; do
    case "${w[i]}" in
    # … existing flag handling …
    esac
  done
  # env can set non-identifier names (via execve); collect ALL NAME=VALUE
  # tokens here so the resolver can find them via the string-comparison
  # override loop (not ${!...}), before the outer loop's identifier gate
  # drops them.
  while ((i < n)) && [[ "${w[i]}" == *=* ]]; do
    HOOK_GIT_ENV_ASSIGNMENTS+=("${w[i]}")
    ((i++))
  done
  continue
  ;;

The HOOK_GIT_ENV_ASSIGNMENTS consumer at line 901 uses [[ "$a" == "$envvar="* ]] — pure string comparison, no ${!...} — so it safely handles non-identifier names already. The only change is collection.

Fix this →

---

Finding 2 — NOT addressed

block-dangerous-git.sh breaks on first alias match; git uses last-wins semantics.

plugins/guardrails/hooks/block-dangerous-git.sh:256 still has break after the first alias match. Compare with block-noncanonical-commit.sh (lines 222–228), which was correctly updated to use last-wins (exp="" before the loop, update without break, act after):

# block-noncanonical-commit.sh (correct) — accumulates last match
exp=""
for cv in ${cfgv[@]+"${cfgv[@]}"}; do
  [[ "${cv,,}" == "alias.${sub,,}="* ]] && exp="${cv#*=}"
done

# block-dangerous-git.sh (still incorrect) — acts on first match and breaks
for cv in ${cfgv[@]+"${cfgv[@]}"}; do
  [[ "${cv,,}" == "alias.${sub,,}="* ]] || continue
  exp="${cv#*=}"
  if [[ "$exp" == '!'* ]]; then ...
  else ...
  fi
  break    # ← wrong
done

Working bypass:

AV='reset --hard' git -c alias.rh=status --config-env=alias.rh=AV rh

After hook::git_effective_config_values, cfgv = ["alias.rh=status", "alias.rh=reset --hard"]. The loop hits alias.rh=status first, calls check_segment git status (allowed), then break. The dangerous second entry is never reached. Guard exits 0. git applies the last config value (alias.rh=reset --hard) and runs git reset --hard.

Also missing from block-dangerous-git.test.sh: a "last alias wins" pair analogous to block-noncanonical-commit.test.sh. This test would expose the bug.

Fix this →

---

Finding 3 — NOT addressed (Codex)

Shell-alias recursion resets env assignments before the inner git is analyzed.

hook::git_resolve_index:642 unconditionally resets HOOK_GIT_ENV_ASSIGNMENTS=(). When a !-shell alias triggers reparse (both guards call hook::bash_parse_segments "$reparse" check_segment), the inner check_segment → hook::git_resolve_index call discards the outer command's env assignments. So:

AV=commit git -c alias.sh='!git --config-env=alias.c=AV c --allow-empty -m x' sh

1. Outer: AV=commit is collected into HOOK_GIT_ENV_ASSIGNMENTS (valid identifier). The outer alias alias.sh='!git …' triggers shell-alias reparse.
2. Inner: hook::git_resolve_index starts fresh. HOOK_GIT_ENV_ASSIGNMENTS=() (reset). The inner command git --config-env=alias.c=AV c sees no AV in HOOK_GIT_ENV_ASSIGNMENTS.
3. printenv "AV" reads from the hook process's ambient environment — empty if AV wasn't pre-exported. val="". Guard fails open.
4. git runs the shell alias with AV=commit inherited in git's actual environment, expands alias.c=commit, commits.

This is lower-severity than Findings 1 and 2 (requires combining a shell alias with --config-env in a non-obvious way), but is structurally the same class of bypass. The prior Codex reviewer and the latest Claude review both flagged it. It was marked as out of scope in the PR description but that reasoning wasn't explained.

---

What the new commit gets right

• printenv "$envvar" as the resolver is the correct primitive. It matches git's C-library getenv(), handles any name shape, and passes the name as a single quoted argument (no re-parsing, no injection). The injection-gate pin tests (assert_file_absent) are preserved and still correctly verify no evaluation for $(touch …) shapes. ✓
• New test for non-identifier ambient env — bad-name=commit set in the hook's process environment — now verifies this path is blocked. ✓
• No -- portability guard on printenv — the comment correctly explains why (BSD printenv may read -- as the literal name). ✓
• Everything else from the prior positive assessment holds: parallel kind array, last-wins semantics in HOOK_GIT_ENV_ASSIGNMENTS, case-fold alias match, block-no-verify.sh left unchanged, blast-radius sync.

---

Summary

The new commit closes one important gap (non-identifier names in the ambient environment) but two P1 bypasses from the prior reviews remain unaddressed:

#	Issue	Status
1	Non-identifier env-var via env '...' wrapper fails open	Partially fixed — ambient-env case blocked; env 'bad-name=...' git vector still open
2	block-dangerous-git.sh breaks on first alias match	Not fixed — break at line 256 unchanged
3	Shell-alias recursion loses outer env assignments	Not fixed

[claude]

Claude finished @kyle-sexton's task in 8m 3s —— View job

---

Security Review

• Read PR diff and changed security-relevant files
• Analyze hook::git_effective_config_values resolver — latest commit (2f8bf01) replaces ${!envvar} with printenv "$envvar"
• Trace non-identifier env-var bypass paths
• Verify block-dangerous-git alias loop first-match vs last-match
• Verify shell-alias-recursion env-assignment loss
• Post findings

---

The latest commit (2f8bf01) closes the ambient-environment non-identifier case (the original fix target), but three security bypasses remain open before merge.

---

Finding 1 — IMPORTANT | Confidence: HIGH

env 'bad-name=…' git wrapper with non-identifier env-var name still bypasses both guards

Location: lib/hook-utils.sh:642 and lib/hook-utils.sh:896

The printenv fix correctly closes the case where a non-identifier name is already in the hook's own ambient environment. It does NOT close the env '…' wrapper case, because the non-identifier NAME=VALUE token that follows env is never collected into HOOK_GIT_ENV_ASSIGNMENTS.

Trace for block-dangerous-git:

env 'bad-rh=reset --hard' git --config-env=alias.rh=bad-rh rh

1. The env handler at line 660 increments i past env, then the inner while loop exits immediately (next token doesn't start with -). continue returns to the outer loop.
2. Outer loop at line 649: tok = "bad-rh=reset --hard" — contains =, so the *=* branch fires. Fails the identifier regex ^[A-Za-z_][A-Za-z0-9_]*= (hyphen). Not added to HOOK_GIT_ENV_ASSIGNMENTS. i++, continue.
3. At line 896: printenv "bad-rh" executes in the hook's process environment. bad-rh is not there (it's only in git's subprocess environment). Returns empty.
4. HOOK_GIT_ENV_ASSIGNMENTS override loop: empty, no match.
5. HOOK_GIT_CONFIG_EFFECTIVE = ["alias.rh="] — empty expansion.
6. Guard alias loop: matches, gets exp="", [[ -n "$exp" ]] is false, acts as if no alias → exit 0 (allowed).
7. git calls getenv("bad-rh") = "reset --hard" → alias expands → git reset --hard runs.

The same shape applies to block-noncanonical-commit:

env 'bad-c=commit' git --config-env=alias.x=bad-c x -m msg

The existing test at block-dangerous-git.test.sh:224 sets bad-rh=reset --hard as the hook's AMBIENT environment (via env "bad-rh=…" bash "$HOOK" …). This passes. The env '…' git … wrapper form is not tested.

Fix: The env handler (or the surrounding outer loop) must also collect non-identifier NAME=VALUE tokens into a separate raw-assignment array. Then hook::git_effective_config_values can do a string-prefix scan of that raw array for non-identifier names instead of relying on printenv (which only sees the hook's own ambient environment).

Fix this →

---

Finding 2 — IMPORTANT | Confidence: HIGH

block-dangerous-git.sh alias loop uses first-match semantics; git uses last-match

Location: plugins/guardrails/hooks/block-dangerous-git.sh:256

This was identified by prior reviews and has not been addressed. The break at line 256 causes the guard to act on the first matching alias value in cfgv, but git applies the last value for a repeated config key. A decoy harmless alias set before the dangerous one masks the real one:

AV='reset --hard' git -c alias.rh=status --config-env=alias.rh=AV rh

After hook::git_effective_config_values, cfgv = ["alias.rh=status", "alias.rh=reset --hard"].

The alias loop at line 232 matches alias.rh=status first, calls check_segment git status (harmless), then breaks at line 256. The second, dangerous alias is never evaluated. Guard exits 0 (allowed); git uses reset --hard.

block-noncanonical-commit.sh already has the correct last-wins pattern (lines 222–228 with no break). block-dangerous-git.sh needs the same treatment, and needs the "last alias value wins" test cases that block-noncanonical-commit.test.sh has at lines 88–91.

Fix this →

---

Finding 3 — IMPORTANT | Confidence: HIGH

Shell alias recursion resets HOOK_GIT_ENV_ASSIGNMENTS, losing outer git's env-var assignments

Location: lib/hook-utils.sh:642

This is the Codex P1 comment on the HOOK_GIT_ENV_ASSIGNMENTS reset. When a !-shell alias expansion causes a recursive check_segment call, that recursive call invokes hook::git_resolve_index, which unconditionally resets HOOK_GIT_ENV_ASSIGNMENTS=() at line 642. Any env-var assignments the outer git saw (inline prefix or env wrapper) are gone before the inner git's --config-env is resolved.

Verified attack for block-noncanonical-commit:

AV=commit git -c alias.sh='!git --config-env=alias.c=AV c --allow-empty -m x' sh

1. Outer hook::git_resolve_index runs: AV=commit → valid identifier → HOOK_GIT_ENV_ASSIGNMENTS = ["AV=commit"].
2. alias.sh is inline (passes through). Expansion is a shell alias (!): reparse = "git --config-env=alias.c=AV c --allow-empty -m x".
3. Recursive check_segment called on the expansion → recursive hook::git_resolve_index → HOOK_GIT_ENV_ASSIGNMENTS = [].
4. Inner hook::git_effective_config_values: envvar = "AV", printenv "AV" → empty (not in hook's ambient env — it was an inline prefix for the outer git only); HOOK_GIT_ENV_ASSIGNMENTS = []. val = "".
5. Guard: empty expansion for c, no git config fallback match, c ≠ commit → return 0 (allowed).
6. Actual execution: outer git's inline AV=commit is in git's process environment; git runs the shell alias via sh -c, which inherits that environment; inner git calls getenv("AV") = "commit" → alias.c expands to commit → git commit runs.

The analogous attack on block-dangerous-git: AV='reset --hard' git -c alias.rh='!git --config-env=alias.x=AV x' rh follows the same path and also exits 0 while executing reset --hard.

Fix: Before the hook::bash_parse_segments recursive call for a ! shell alias, snapshot HOOK_GIT_ENV_ASSIGNMENTS and restore it after — or pass it as a parameter into the recursion so the inner hook::git_resolve_index can merge rather than reset.

Fix this →

---

What the latest commit (2f8bf01) closes

The printenv "$envvar" replacement correctly closes the ambient-environment non-identifier case: if bad-name is somehow present in the hook's own process environment (e.g., set by some outer process and exported), the prior ${!envvar} would have silently dropped it (bash rejects indirect expansion of non-identifier names), while printenv correctly finds it. The new tests at block-dangerous-git.test.sh:224 and block-noncanonical-commit.test.sh:118 cover this case. The injection safety argument (name passed as a single quoted argument to printenv, no re-parsing) is sound.

---

Summary

Three security bypasses remain open. All three are of the same fail-open class that this PR set out to close. None are introduced by the latest commit; all were present before it and remain unaddressed.

#	Finding	Severity	Confidence	Status
1	env 'bad-name=…' git — non-identifier name in env wrapper, not in HOOK_GIT_ENV_ASSIGNMENTS, printenv returns empty	IMPORTANT	HIGH	Open
2	block-dangerous-git.sh:256 break — first-match alias wins; git uses last-match	IMPORTANT	HIGH	Open
3	hook::git_resolve_index resets HOOK_GIT_ENV_ASSIGNMENTS on recursive shell-alias call	IMPORTANT	HIGH	Open

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f8bf01b89

ℹ️ 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".

[kyle-sexton]

Operator ruling (ratification interview, 2026-07-22 ~03:05Z): security review CLEARED — merge approved. Reviewed via tower security brief: fail-open in hook::git_resolve_subcommand (--config-env alias bypass of block-noncanonical-commit / block-dangerous-git) fixed; both residual bypasses (case-fold key comparison, command-line env assignments) confirmed present in final diff; injection-shaped-name tests assert no evaluation. Remaining risk assessed low. Tower removing do-not-merge and merging under this ruling.

[kyle-sexton]

Tower correction (2026-07-22 ~03:10Z): merge HELD — ruling premise broken. The clear-and-merge ruling was based on a review snapshot predating four NEW unresolved Codex P1 threads (23:29Z–00:46Z), each a verified fail-open bypass of the guards this PR fixes: (1) non-identifier --config-env names allowed instead of failing closed, (2) first-match alias expansion lets a later dangerous --config-env value mask behind an earlier harmless -c, (3) command-line env assignments lost across '!' shell-alias recursion, (4) leading-dash env names dropped by printenv option-parsing. Defer-forbidden classes 2/3 — these get fixed on this branch before merge. do-not-merge restored; fixer dispatched; re-merge after fixes + clean re-review under the operator's overnight Opus-confirm protocol.

[claude]

Claude finished @kyle-sexton's task in 6m 42s —— View job

---

Security Review

• Load tools
• Read current lib/hook-utils.sh — verify all four prior P1 bypasses
• Read block-dangerous-git.sh — verify alias loop last-wins fix
• Read block-noncanonical-commit.sh
• Read test files — verify new coverage
• Hunt for new injection surfaces or logic gaps
• Post findings

---

Result: All four prior P1 bypasses are now closed. No new security findings.

The four bypasses identified by prior automated reviews and tracked through the do-not-merge hold have been addressed in commit fa132eb.

---

Prior Finding 1 — CLOSED: env 'bad-name=…' git non-identifier env-var bypass

Was: The env) handler exited with continue after consuming flags, letting non-identifier NAME=VALUE tokens (e.g. bad-name=reset --hard) fall to the outer loop's identifier gate (^[A-Za-z_][A-Za-z0-9_]*=), which dropped them silently. The resolver never saw the assignment and projected an empty alias expansion → fail open.

Now: The env) handler runs a second inner loop after flag consumption (line 716–718) that collects all NAME=VALUE operands regardless of name shape (elif [[ "${w[i]}" == *=* ]]; then HOOK_GIT_ENV_ASSIGNMENTS+=("${w[i]}")). The HOOK_GIT_ENV_ASSIGNMENTS override loop in hook::git_effective_config_values uses a quoted-literal bash string comparison ([[ "$a" == "$envvar="* ]]), not a glob-expanded one — $envvar is inside double quotes on the pattern side, so it is matched literally, not as a glob. The test at block-dangerous-git.test.sh:234–235 and block-noncanonical-commit.test.sh:142–143 prove the env-wrapper operand is collected from the command line, not the ambient env (no extra-env arg is passed). ✓

---

Prior Finding 2 — CLOSED: env -- '-AV=…' git leading-dash env-var bypass

Was: printenv "$envvar" parsed a leading-dash name as an option flag and returned empty → fail open. Additionally, the prior env handler had no -- awareness so a post--- leading-dash token would exit the inner loop and be mishandled.

Now: Two independent fixes compose:

1. Resolver: replaced printenv with awk 'BEGIN { n = ENVIRON["__HOOK_CE_NAME"]; if (n in ENVIRON) print ENVIRON[n] }' where __HOOK_CE_NAME is set as an env var (not an awk -v argument). awk's ENVIRON is an exact-name-keyed map equivalent to C's getenv() — it does not option-parse the name. A leading-dash name is looked up by key, not as an option. The name is passed through a fixed env-var slot, never as a command argument, so no argument-injection surface exists.

2. Collector: the new env_past_optmark flag (line 682–688) correctly implements env -- semantics: -- sets the flag; subsequent tokens with = are collected as assignments regardless of a leading -. Tested at block-dangerous-git.test.sh:239–240. ✓

The __HOOK_CE_NAME carrier variable was verified to be safe against collision: the assignment __HOOK_CE_NAME="$envvar" in the command substitution subshell wins over any ambient __HOOK_CE_NAME, and the awk script is a fixed literal with no part of $envvar interpolated into it.

---

Prior Finding 3 — CLOSED: block-dangerous-git.sh first-match alias loop

Was: The alias loop at the old line 256 breaked on the first match; git uses last-value-wins for repeated config keys. A decoy harmless -c alias.rh=status placed before the dangerous --config-env=alias.rh=AV rh would shadow it.

Now: The loop initialises exp="" before iterating (line 236) and accumulates with [[ ... ]] && exp="${cv#*=}" without breaking (line 241–242), matching block-noncanonical-commit.sh's correct last-wins pattern. The if [[ -n "$exp" ]]; then ... fi block acts on the final accumulated value. Tests cover both orderings at block-dangerous-git.test.sh:243–246. ✓

---

Prior Finding 4 — CLOSED: Shell-alias recursion loses enclosing git env

Was: hook::git_resolve_index unconditionally reset HOOK_GIT_ENV_ASSIGNMENTS=(). When a ! shell alias triggered a recursive check_segment → hook::git_resolve_index call, the outer invocation's command-line assignments were gone before the inner git's --config-env was resolved → fail open.

Now: The new hook::git_reparse_shell_alias wrapper saves HOOK_GIT_ENV_INHERITED, sets it to the current HOOK_GIT_ENV_ASSIGNMENTS, calls hook::bash_parse_segments, then restores it. hook::git_resolve_index now seeds HOOK_GIT_ENV_ASSIGNMENTS from HOOK_GIT_ENV_INHERITED (line 656) rather than starting empty. Both guards now call hook::git_reparse_shell_alias instead of hook::bash_parse_segments directly for ! shell aliases. The save/restore pattern is stack-safe for nested aliases. Tests at block-dangerous-git.test.sh:250–251 and block-noncanonical-commit.test.sh:148–149. ✓

---

No new security findings

All surfaces examined:

• awk injection gate: $envvar is never interpolated into the awk script; it is set as env var __HOOK_CE_NAME and the script reads it via ENVIRON["__HOOK_CE_NAME"]. Injection-shaped names (containing $(…), ;, etc.) are looked up by key — not parsed — and return empty if not in environment. The existing assert_file_absent pin tests confirm no execution. ✓
• Quoted-literal bash == comparison: [[ "$a" == "$envvar="* ]] — "$envvar=" is double-quoted in the pattern, so $envvar is literal (not glob-expanded). No glob bypass via crafted envvar names. ✓
• HOOK_GIT_ENV_INHERITED global state: initialised safely via the +x guard pattern (${arr[@]+"${arr[@]}"}) on unset arrays; the save/restore in hook::git_reparse_shell_alias is correct for nested recursion. ✓
• env --unset=VAR (attached form): the -u | --unset branch handles only space-separated form; --unset=VAR falls to -) and is skipped as one token. This means an env -u unset is not fully modeled, but the resulting behaviour is conservative (the guard may over-block what git would reject), not a fail-open. Not a security issue. ✓
• env_past_optmark local re-initialisation: local env_past_optmark=0 is function-scoped in bash but re-assigns to 0 each time the env) case is entered (re-entering via the outer while), correctly resetting state for each env token. ✓

---

Cosmetic (pre-existing, not blocking)

The *) ((i++)) ;; arm inside the env) handler (line 714) remains dead code: the enclosing if condition requires [[ "${w[i]}" == -* ]], so any token reaching the case must start with -, making *) unreachable. Same pattern in the sudo) handler. Neither affects correctness or security.

---

This PR is clear for merge from a security standpoint. The four fail-open bypasses that triggered the do-not-merge hold are now closed; the injection gate is intact; test coverage for each bypass shape is in place.

[claude]

Claude finished @kyle-sexton's task in 3m 52s —— View job

---

PR Review — Commit fa132eb

• Read diff for new commit fa132eb vs prior state
• Read lib/hook-utils.sh — verify all four bypass fixes
• Read block-dangerous-git.sh — verify last-match alias loop
• Read block-noncanonical-commit.sh — check for regressions
• Read test files — verify coverage for all four scenarios
• Verify Finding 1: env 'bad-name=…' git wrapper (non-identifier env-var)
• Verify Finding 2: block-dangerous-git first-match alias break
• Verify Finding 3: shell-alias recursion resets HOOK_GIT_ENV_ASSIGNMENTS
• Verify Finding 4: leading-dash env-var names dropped by printenv
• Post final review

---

All four prior P1 findings from the review thread are closed in this commit. Details below.

---

Finding 1 — CLOSED: env 'bad-name=…' git wrapper with non-identifier env-var name

Location: lib/hook-utils.sh:682–723

The env handler's inner while loop was restructured to handle three cases:

1. Before --, token starts with -: enters the option-processing case as before.
2. -- encountered: sets env_past_optmark=1 and advances. After this point, option-parsing is suppressed.
3. After all options (or after --), *=* token: collected into HOOK_GIT_ENV_ASSIGNMENTS regardless of name shape (no identifier gate).
4. First non-*=* operand: break — this is git or the command.

Previously, the inner while only ran while ${w[i]} == -*, and non-identifier assignments were then dropped by the outer loop's identifier gate. Now the env handler consumes them before the outer loop gets a chance to drop them.

Verification: The new test case env-wrapper non-identifier operand (blocked) in both guard test suites (env 'bad-rh=reset --hard' git --config-env=alias.rh=bad-rh rh → exit 2) targets exactly this path without setting anything in the ambient environment, proving the operand is collected from the one-liner rather than read from the hook's own env. ✓

---

Finding 2 — CLOSED: block-dangerous-git.sh breaks on first alias match

Location: plugins/guardrails/hooks/block-dangerous-git.sh:232–264

The loop is restructured from break-on-first to last-wins:

exp=""
for cv in ${cfgv[@]+"${cfgv[@]}"}; do
  [[ "${cv,,}" == "alias.${sub,,}="* ]] && exp="${cv#*=}"
done
if [[ -n "$exp" ]]; then
  ...
fi

No break. Last match overwrites exp; action is taken outside the loop on the final value. This mirrors block-noncanonical-commit.sh (which was already correct from prior work on this branch).

Verification: Two new tests cover both orderings:

• later --config-env value wins over decoy -c alias (blocked) → AV='reset --hard' git -c alias.rh=status --config-env=alias.rh=AV rh exits 2. ✓
• later harmless --config-env value wins (allowed) → AV=status git -c alias.rh='reset --hard' --config-env=alias.rh=AV rh exits 0. ✓

---

Finding 3 — CLOSED: Shell-alias recursion loses outer git's env assignments

Location: lib/hook-utils.sh:955–961

New function hook::git_reparse_shell_alias wraps the shell-alias reparse path:

hook::git_reparse_shell_alias() {
  local cb="$1" reparse="$2"
  local -a saved_inherited=(${HOOK_GIT_ENV_INHERITED[@]+"${HOOK_GIT_ENV_INHERITED[@]}"})
  HOOK_GIT_ENV_INHERITED=(${HOOK_GIT_ENV_ASSIGNMENTS[@]+"${HOOK_GIT_ENV_ASSIGNMENTS[@]}"})
  hook::bash_parse_segments "$reparse" "$cb"
  HOOK_GIT_ENV_INHERITED=(${saved_inherited[@]+"${saved_inherited[@]}"})
}

And hook::git_resolve_index now seeds from HOOK_GIT_ENV_INHERITED instead of starting empty:

HOOK_GIT_ENV_ASSIGNMENTS=(${HOOK_GIT_ENV_INHERITED[@]+"${HOOK_GIT_ENV_INHERITED[@]}"})

Both guards and the git config fallback path in block-noncanonical-commit.sh (line 259) use hook::git_reparse_shell_alias for shell aliases. The save/restore of HOOK_GIT_ENV_INHERITED means nested shell aliases compose correctly — each recursive reparse carries the immediately enclosing git's environment.

Verification: Both guard test suites have a new test: shell alias carries enclosing git env into nested --config-env (blocked) → AV='reset --hard' git -c alias.sh='!git --config-env=alias.rh=AV rh' sh exits 2. ✓

---

Finding 4 — CLOSED: Leading-dash env-var names dropped by printenv option-parsing

Two changes address this:

a) Resolver: printenv → awk/ENVIRON

lib/hook-utils.sh:928:

val="$(__HOOK_CE_NAME="$envvar" awk 'BEGIN { n = ENVIRON["__HOOK_CE_NAME"]; if (n in ENVIRON) print ENVIRON[n] }' 2>/dev/null || true)"

The env-var name is passed through a fixed __HOOK_CE_NAME environment variable, not as an argument to a command that option-parses its arguments. awk's ENVIRON is an exact-name-keyed map equivalent to C's getenv(). A leading-dash name like -AV is not option-parsed by awk's ENVIRON lookup. An injection-shaped name (e.g. $(touch ...)) reaches awk only as the value of __HOOK_CE_NAME, never as a command substitution within the awk program, which is a fixed single-quoted literal.

b) Collection: env -- handling

lib/hook-utils.sh:686–688: the new -- case in the env handler sets env_past_optmark=1. After that, the elif *=* branch collects any operand containing = — including leading-dash ones like -AV=val — into HOOK_GIT_ENV_ASSIGNMENTS.

Verification:

• env -- leading-dash operand (blocked) → env -- '-AV=reset --hard' git --config-env=alias.rh=-AV rh exits 2 in both guard suites. ✓
• hook-utils.test.sh has a subshell test for leading-dash ambient env: env -- '-AV=commit' bash -c '...' validates the awk/ENVIRON path resolves a leading-dash name from the hook's own ambient environment. ✓

---

Remaining cosmetic issue (not introduced by this PR)

lib/hook-utils.sh:714:

-*) ((i++)) ;;
*) ((i++)) ;;   # ← unreachable: outer `if` requires token to start with -

The *) arm inside the option-parsing branch of the env handler is still dead code (the outer if ((env_past_optmark == 0)) && [[ "${w[i]}" == -* ]] guarantees any token reaching the case starts with -). This is cosmetic and was flagged in prior reviews. Not a correctness or security concern.

---

What the implementation gets right

• awk injection safety: The env-var name is passed only through __HOOK_CE_NAME and never interpolated into the awk program string. The n in ENVIRON guard prevents printing unset vars. __HOOK_CE_NAME="$envvar" overrides any inherited value of that key. ✓
• env_past_optmark scoping: local env_past_optmark=0 inside the case arm is local to the function call; no leak between env tokens in a single segment. ✓
• Save/restore in hook::git_reparse_shell_alias: Nested shell aliases (!alias that calls another !alias) compose safely because the function saves and restores HOOK_GIT_ENV_INHERITED. ✓
• Identifier gate for inline prefix preserved: The outer loop's ^[A-Za-z_][A-Za-z0-9_]*= gate at line 668 is correct for the shell-prefix case (the shell itself rejects non-identifier NAME=val as an assignment) and untouched by this fix. ✓
• HOOK_GIT_ENV_INHERITED default: ${HOOK_GIT_ENV_INHERITED[@]+"..."} correctly expands to empty when the var is unset (top-level parse), matching the prior HOOK_GIT_ENV_ASSIGNMENTS=() behavior. ✓
• git config fallback in block-noncanonical-commit.sh: The fallback path at line 259 also uses hook::git_reparse_shell_alias, so a shell alias found via git config --get rather than inline also carries the env. ✓
• Blast-radius sync: All 11 carrying plugins sync the updated hook-utils.sh. ✓

---

Summary

Commit fa132eb closes all four previously confirmed P1 bypasses. The implementation approach is sound — the env handler fix, the awk/ENVIRON resolver, the last-wins alias loop, and the hook::git_reparse_shell_alias propagation are all correctly designed and tested. The one remaining cosmetic issue (dead *) arm in the env handler) is minor and predates this commit.

This branch is ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa132ebd1d

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 6m 13s —— View job

---

PR Review — HEAD 24bbc1b

• Read PR diff and identify changed files
• Read core changed files (lib/hook-utils.sh, guard scripts, test files)
• Assess current security posture after structural reposture
• Verify open Codex findings against current HEAD
• Post final review

---

Overall Assessment

The structural reposture introduced in 9eb9c0e and hardened in 7ea3b9d is architecturally correct and cleanly executed. All the value-resolution fail-opens that generated 5+ rounds of iterative findings are closed by construction — the resolver machinery is gone, not backstopped. The implementation is sound, test coverage is comprehensive, and the acceptance cases are correctly identified. Two items from the latest Codex review remain open; their status is assessed below.

---

What this PR gets right

Structural design. hook::git_alias_expansion (lib/hook-utils.sh:891–906) returns 2 for env-kind aliases and never reads the environment variable. This closes the entire class of bypasses that fed through --config-env's indirect expansion: ambient vars, env '…' wrappers, inline prefixes, export, set -a, declare -x, export-before-assign, assignment-prefixed export, bash -c wrappers, GIT_CONFIG_PARAMETERS propagation — all irrelevant once the value is never read. The threat-model note in the CHANGELOG is accurate.

Depth-2 ungating (7ea3b9d). The structural shape refusal now fires before the HOOK_NO_ALIAS gate in both guards, so git -c alias.rh='--config-env=alias.foo=AV foo' rh is blocked even at recursion depth 2. The comment at block-dangerous-git.sh:227–231 correctly explains why only the inline-alias re-expansion is depth-bounded by HOOK_NO_ALIAS, while the shape refusal is not.

Parser correctness. hook::git_resolve_subcommand maintains HOOK_GIT_CONFIG_VALUE_KINDS 1:1 with HOOK_GIT_CONFIG_VALUES across all four parse branches (two-word -c/--config, attached --config=, two-word --config-env, attached --config-env=). Last-match wins in hook::git_alias_expansion, matching git's own precedence for repeated config keys.

hook::git_resolve_index simplification. The function now walks past env NAME=value operands without collecting their values (lib/hook-utils.sh:698–701). The comment is correct: a --config-env alias for the invoked subcommand is refused by shape, so the resolver never needs the value. This is a significant simplification vs. prior rounds.

Acceptance cases correctly identified. These remain allowed without reading any value: --config-env setting a non-alias key, aliases for subcommands that are not the invoked one, and an inline -c value that last-wins over an earlier --config-env for the same key. All three have test coverage.

Test breadth. The test suites cover: direct shape (both forms), case-folded keys, case-folded subcommands, non-identifier env names, leading-dash names, env -- wrapper, last-wins in both orderings, depth-2 shape refusal, shell-alias wrapper, bash -c wrapper, export-after-assign, then/do keyword exports, export-before-assign, assignment-prefixed export, set -a allexport, internal-global name collision, and the $(touch …) injection pin. All acceptance scenarios have matching tests.

Blast radius handled correctly. All 12 carrying plugins synced and bumped. block-no-verify.sh correctly unchanged (keys on core.hooksPath=, a config key, not an alias value).

---

Open Findings from Latest Codex Review (0b6cb50913)

Finding 1 — Codex P1: Option-prefixed inline alias chains (H1 / #964)

The attack: git -c alias.rh='-c alias.foo="--config-env=alias.bar=AV bar" foo' rh with AV='reset --hard'.

Trace through the guard:

1. sub="rh", hook::git_alias_expansion("rh") → kind=inline, HOOK_GIT_ALIAS_EXP="-c alias.foo=… foo". alias_rc=0.
2. HOOK_NO_ALIAS=0 → enter inline expansion. hook::env_s_split splits to ["-c", "alias.foo=--config-env=alias.bar=AV bar", "foo"].
3. HOOK_NO_ALIAS=1, recursive check_segment [git] ["-c", "alias.foo=--config-env=alias.bar=AV bar", "foo"].
4. In the recursive call: hook::git_resolve_subcommand sees -c alias.foo=--config-env=alias.bar=AV bar as kind=inline (it came from a -c token, not --config-env). Sub=foo.
5. hook::git_alias_expansion("foo") → kind=inline (not env), alias_rc=0. Structural fail-closed (alias_rc==2) NOT reached.
6. HOOK_NO_ALIAS=1 → inline expansion suppressed.
7. Sub=foo is not push/reset/etc. → return 0. Guard allows.

Real git behavior: alias.foo=--config-env=alias.bar=AV bar is a non-! git alias that expands to git --config-env=alias.bar=AV bar, where bar then reads AV='reset --hard' and runs git reset --hard.

Status: The tower acknowledges this as H1, describes it as the same root cause as #964 ("one-level re-expansion drops command-line -c/--config-env"), calls it pre-existing and not a regression, and has explicitly scoped it as out-of-scope for this PR. The shape required (3-level alias nesting with a --config-env embedded inside an inline alias value) is baroque; the threat model for these guards is accidental-destruction friction, not adversarial-proof sandbox. The #964 ticket captures the broader fix. Not blocking this PR.

Finding 2 — Codex P1: alias.*.command syntax

The Codex reviewer claims alias.rh.command=reset --hard is equivalent to alias.rh=reset --hard per the git docs. After review: the standard git alias format is alias.<name>=<command>. I cannot find alias.<name>.command=<value> as a documented equivalent in stable git releases. The git-config man page and git source use only alias.<name>. This finding is likely a false positive by Codex. It should be investigated against a real git installation before merge to confirm, but is unlikely to be blocking.

---

Cosmetic (pre-existing, not blocking)

lib/hook-utils.sh:696: the *) ((i++)) ;; arm inside the env handler's option-processing branch is dead code — the enclosing if ((env_past_optmark == 0)) && [[ "${w[i]}" == -* ]] guard guarantees any token entering the case starts with -. The sudo/nice/nohup handlers have the same pattern. All three were flagged in prior review rounds; none affect correctness.

---

State

The do-not-merge hold is correctly set pending operator re-ratification of the structural reposture (the change from value-resolution to shape-refusal). The security engineering within this PR's stated scope is sound. The one confirmed open bypass (H1 / #964) is pre-existing, acknowledged, and filed separately; Codex's alias.*.command finding is likely a false positive.

[claude]

Claude finished @kyle-sexton's task in 11m 0s —— View job

---

Security Review

• Read PR diff to identify changed security-relevant files
• Read lib/hook-utils.sh — parser and alias expansion logic
• Read block-dangerous-git.sh and block-noncanonical-commit.sh
• Assess latest Codex P1 findings (alias.*.command, outer config carry-through, option-prefixed alias bodies)
• Hunt for new injection surfaces, fail-open conditions, logic flaws
• Post security findings

---

Head commit reviewed: 24bbc1b (version restack; no security changes vs. 7ea3b9d)
Security-critical commits: 9eb9c0e (structural fail-closed) + 7ea3b9d (depth-invariant shape refusal)

---

Design summary

The core security bet of this PR is the structural fail-closed reposture introduced in 9eb9c0e: instead of resolving a --config-env alias's value at hook time, the guard refuses by shape alone. Any --config-env=alias.<sub>=<envvar> (or two-word form) whose key matches the invoked subcommand is blocked unconditionally — the environment variable's value is never read.

hook::git_alias_expansion (lib/hook-utils.sh:891) iterates HOOK_GIT_CONFIG_VALUES, applies last-wins semantics, and returns 2 (env kind), 0 (inline kind), or 1 (no alias). The shape refusal on return 2 is not gated by HOOK_NO_ALIAS, so it fires at every alias recursion depth — the fix in 7ea3b9d.

---

No new security findings at this HEAD

The structural approach eliminates the entire class of value-propagation bypasses that drove the prior ~10 review rounds. Every bypass pattern from those rounds (ambient env, env '...' wrapper, export, declare -x, set -a, assignment-prefix export, compound-keyword export, awk pivot-key collision, leading-dash name, entry-time snapshot collision) is now closed by construction: the guard never reads the value, so none of those propagation paths matter.

Verified working correctly:

• Direct shape: git --config-env=alias.rh=AV rh → exit 2 (value never read, shape alone)
• Last-wins: git --config-env=alias.rh=AV -c alias.rh=status rh (last is inline) → exit 0; reverse ordering → exit 2. Correct per git semantics.
• Depth-2 inline-wrapping: git -c alias.rh='--config-env=alias.foo=AV foo' rh — outer inline alias rh is re-expanded, inner parse sees --config-env=alias.foo=AV with kind env → hook::git_alias_expansion "foo" returns 2 → shape refusal fires even with HOOK_NO_ALIAS=1. Test at block-dangerous-git.test.sh:229-230. ✓
• Through shell alias body: git -c "alias.sh=!git --config-env=alias.rh=AV rh" sh — outer inline alias is re-parsed as a shell command, inner segment triggers shape refusal. Test at line 225. ✓
• Env wrapper, non-identifier names, leading-dash: env -- '-AV=x' git --config-env=alias.rh=-AV rh → shape refusal (no value needed). Tests at lines 214-215. ✓
• set -a, export, compound-keyword export patterns: All blocked because the FINAL git --config-env=alias.rh=AV rh segment always triggers the shape refusal regardless of what set AV. Tests at lines 232-237. ✓
• Non-alias key passes through: git --config-env=core.pager=PAGERVAR status → exit 0. Test at line 240. ✓
• Inline last-wins over earlier env: git --config-env=alias.rh=AV -c alias.rh=status rh → inline wins, re-checked as git status → exit 0. Test at line 242. ✓

---

Acknowledged residuals — out of scope for this PR, referred to #964

Two bypass classes are explicitly deferred; they pre-date and survive this PR's scope:

H1 — Outer command's --config-env alias not visible in shell-alias sub-parse

AV='reset --hard' git --config-env=alias.rh=AV -c alias.sh='!git rh' sh

Trace: the outer command's alias.rh (env kind) is collected into HOOK_GIT_CONFIG_VALUES for the outer hook::git_resolve_subcommand call. But hook::git_resolve_subcommand resets HOOK_GIT_CONFIG_VALUES=() at entry. When the shell alias body !git rh is re-parsed via hook::bash_parse_segments, the inner check_segment "git" "rh" gets a fresh empty HOOK_GIT_CONFIG_VALUES — alias.rh is gone. The inner hook::git_alias_expansion "rh" returns 1 (no entry), falls to gitconfig lookup (which also finds nothing), sub = "rh" not in dangerous set → guard exits 0.

Real git: passes alias.rh=<AV value> via GIT_CONFIG_PARAMETERS to the ! subprocess → inner git rh expands → executes reset --hard. Bypass is real.

This is the H1 class. Kyle-sexton's comment at 07:46Z explicitly acknowledges it and defers it to #964 as "pre-existing, NOT a regression, correctly untouched here."

H2 — Three-level alias chain through option-prefixed expansion (Codex P1 at 07:36Z)

git -c alias.rh='-c alias.foo="--config-env=alias.bar=AV bar" foo' rh

Trace: outer rh (inline) → expansion -c alias.foo=... foo → inner parse with HOOK_NO_ALIAS=1. Inner: sub = "foo", alias.foo is inline (defined via -c), so hook::git_alias_expansion "foo" returns 0 (not 2). Shape refusal doesn't fire. HOOK_NO_ALIAS=1 prevents re-expansion of foo. foo is not in dangerous set → guard exits 0.

Real git: foo expands to --config-env=alias.bar=AV bar; inner git sees alias.bar as env alias; with AV='reset --hard', runs reset --hard. Bypass is real.

The distinction from the depth-2 case fixed by 7ea3b9d: in that fix, the inline alias value directly contains --config-env=alias.<sub>=<envvar> (the VALUE becomes a config-env entry at depth 2). In H2, the inline alias value introduces a NEW -c alias.foo=... (inline) whose VALUE then contains --config-env. The shape refusal fires at the config-env entry level, but here alias.foo is inline (not env), so alias_rc == 0 not 2.

This is the H2/Codex P1 class. Also acknowledged at 07:46Z and filed to #964.

---

Codex P1 from 06:50Z — alias.*.command — likely false positive

Verdict: SUGGESTION | Confidence: MEDIUM

The Codex P1 from the 0f5e93d011 review claims git supports alias.<name>.command as equivalent to alias.<name>, and that lib/hook-utils.sh:897 would miss it:

[[ "${cv,,}" == "alias.${sub,,}="* ]] || continue

Standard git config does not document alias.<name>.command. Git aliases are defined as alias.<name> only — no .command sub-key. The Codex comment cites the git-config manpage but alias.*.command does not appear in that documentation. No evidence of this format was found in the guardrails test files, git source, or standard git documentation. This finding should be verified against actual git behavior before acting on it; it is most likely a Codex hallucination.

---

Over-blocks (P2, fail-safe direction — confirmed acceptable)

• env -u VAR git --config-env=alias.rh=VAR rh with VAR=reset --hard in ambient env: guard blocks (shape refusal), but git would fatal (VAR removed from subprocess env). Over-block confirmed at 05:55Z and accepted.
• env -i ... similarly.

These are false positives (guard blocks a command that git would reject), which is the correct failure mode for a friction guard.

---

What the implementation gets right

• HOOK_GIT_CONFIG_VALUE_KINDS parallel array — maintained 1:1 with HOOK_GIT_CONFIG_VALUES across all four parse branches (two-word and attached forms for both --config and --config-env). Correct.
• hook::git_alias_expansion last-wins iteration — iterates all entries, overwrites on each match; acts on final found_kind. Correct per git semantics.
• Case-fold on alias key match — ${cv,,} / ${sub,,} applied only to the key side; expansion value in HOOK_GIT_ALIAS_EXP retains original case. Correct.
• Shape refusal ungated from HOOK_NO_ALIAS — hook::git_alias_expansion "$sub" is called unconditionally before the HOOK_NO_ALIAS gate; only alias_rc == 2 exits early, while alias_rc == 0 re-expansion stays gated. Correct design.
• Inline aliases still resolved — alias_rc == 0 path (with HOOK_NO_ALIAS protection) preserves re-checking of -c/--config aliases whose expansions are literally present and bounded.
• Acceptance cases correct — --config-env setting a non-alias key, or an alias for a non-invoked subcommand, both allowed. Inline last-wins over earlier env is allowed (and re-checked). All these have regression tests.
• Blast-radius sync — all 11 carrying plugins sync hook-utils.sh with a patch bump and individual changelog entries. Correct.

---

Summary

The structural fail-closed reposture is a sound design decision that eliminates an unbounded attack surface (env-value propagation through shell constructs). No new security findings were identified at the current HEAD.

The two acknowledged residuals (H1 and H2 — three-level alias chains) are real but explicitly deferred to #964 and represent a pre-existing limitation of one-level alias re-expansion. The Codex alias.*.command P1 is most likely a false positive.

This branch is clear for merge from a security standpoint subject to the H1/H2 deferral and operator ratification of the structural reposture, both of which are tracked externally.

[claude]

Claude finished @kyle-sexton's task in 4m 0s —— View job

---

PR Review — Commit df23907

• Read the new commit's diff (df23907)
• Read lib/hook-utils.sh — hook::git_alias_expansion, alias subkey detection
• Read guard scripts and test files
• Assess the alias.<name>.command change against prior Codex P1 and git documentation
• Check for new injection surfaces or logic gaps introduced by the change
• Post final review

---

Summary

The new commit closes the Codex P1 finding about alias.<name>.command by extending hook::git_alias_expansion to match both the plain alias.<sub>= and the alias.<sub>.command= spellings. The implementation is correct and the tests are comprehensive.

---

What the commit does

lib/hook-utils.sh:900 — the single changed line in the security-critical path:

# before
[[ "${cv,,}" == "alias.${sub,,}="* ]] || continue

# after
[[ "${cv,,}" == "alias.${sub,,}="* || "${cv,,}" == "alias.${sub,,}.command="* ]] || continue

This is a minimal, targeted change. No new globals, no new state, no structural changes to the function.

---

Correctness analysis

Matching logic. The key match "${cv,,}" == "alias.${sub,,}.command="* correctly case-folds both the collected config value and the subcommand, matching git's case-insensitive config key lookup. The expansion is stripped from the ORIGINAL (unfolded) cv via "${cv#*=}", preserving the case of the expansion value — correct for both inline re-checking and env-kind shape-refusal.

Last-wins across both spellings. The loop accumulates the last matching entry across BOTH alias.<sub> and alias.<sub>.command spellings, updating HOOK_GIT_ALIAS_EXP and found_kind on each match without breaking. This means:

• git -c alias.rh='reset --hard' -c alias.rh.command=status rh → last match is .command=status (inline), guard allows ✓
• git -c alias.rh.command=status -c alias.rh='reset --hard' rh → last match is plain alias.rh=reset --hard (inline), guard blocks ✓

Shape-refusal for --config-env. git --config-env=alias.rh.command=AV rh → cv = "alias.rh.command=AV", kind = env, matched by new second condition → found_kind = "env" → returns 2 → both guards exit 2 before the HOOK_NO_ALIAS gate. The env-var value AV is never read. ✓

Non-command subkey control. alias.rh.nope=reset --hard — "alias.rh.nope=" matches neither alias.rh= nor alias.rh.command= → found_kind="" → returns 1 → guard treats rh as a direct subcommand (not in dangerous set) → allowed. ✓

Injection surface. No new surface: the .command literal is a fixed string in the pattern, never derived from input. The condition is a purely structural bash == pattern match.

---

Test coverage

The new tests in lib/hook-utils.test.sh (lines 731–780) cover the classification layer; the guard end-to-end tests in both suites cover the hook decision:

Scenario	block-dangerous-git	block-noncanonical-commit
Inline .command alias blocked	✓	✓
--config-env .command shape-blocked	✓	✓
Case-folded .command key blocked	✓	✓
Non-command subkey NOT treated as alias	✓	✓
.command last-wins over earlier plain (allowed)	✓	✓
Plain last-wins over earlier .command (blocked)	✓	✓

---

Open question: is alias.<name>.command a real git feature?

The prior review assessed the Codex P1 as "likely a false positive" (MEDIUM confidence) because alias.rh.command would be parsed as section=alias, subsection=rh, variable=command — a different config key from alias.rh. The commit author's test comment states the behavior is verified against real git. If it IS a real git feature, the fix is exactly right. If it is NOT, the fix adds matching logic for a config key git never reads, which is harmless: alias.rh.nope=reset --hard (any other subkey) still correctly returns 1 and doesn't over-block, because only .command is matched.

In either case the fix is strictly an improvement: if the feature is real it closes a bypass; if it is not, it adds a harmless additional pattern.

---

One modeling subtlety (pre-existing, not introduced by this commit)

Git's config system treats alias.rh and alias.rh.command as independent config keys, each with their own last-wins stack. When both are set, git presumably picks one over the other by some precedence rule (e.g., always prefers .command if present, or uses whichever appears later across all config sources). The guard's cross-form last-wins model treats both as interchangeable in a single linear ordering — which matches the -c command-line ordering but may diverge from git's precedence if both keys appear in gitconfig. This is a pre-existing modeling approximation that applies to the entire --config-env shape-refusal design and is not introduced by this commit. The guard's model is conservative in the security direction for the relevant attack shapes.

---

Dead code (pre-existing, not introduced by this commit)

lib/hook-utils.sh:696: *) ((i++)) ;; inside the env handler is unreachable (outer condition requires token to start with -). Flagged in prior reviews; not a correctness or security concern.

---

Assessment

The implementation is correct, the tests are comprehensive for all relevant cases, and no new security surfaces are introduced. This commit closes the Codex P1 finding about the .command subkey and is consistent with the structural shape-refusal design established in 9eb9c0e.

This branch remains clear for merge from a security standpoint. The two explicitly deferred items (H1 and H2 from #964) are pre-existing and unchanged by this commit.

[claude]

Claude finished @kyle-sexton's task in 4m 22s —— View job

---

Security Review

• Read PR diff and identify security-critical changed files
• Read lib/hook-utils.sh — parser, alias expansion, hook::git_alias_expansion
• Read block-dangerous-git.sh and block-noncanonical-commit.sh
• Inspect the alias.<name>.command fix (df23907)
• Verify prior security posture: shape-refusal and depth-invariant blocking
• Hunt for new injection surfaces, fail-open conditions, logic flaws
• Post findings

---

HEAD reviewed: df23907 (fix(guardrails): detect the alias.<name>.command subkey as an alias)

The df23907 commit is the only security-relevant change since the prior clean review (24bbc1b). All four P1 bypasses from the prior do-not-merge hold, the structural fail-closed reposture, and the depth-invariant shape refusal are intact and unchanged.

---

Finding — IMPORTANT | Confidence: HIGH

block-noncanonical-commit gitconfig fallback does not check the alias.<sub>.command form

Location: plugins/guardrails/hooks/block-noncanonical-commit.sh:261

This PR establishes that git honors alias.<sub>.command as a first-class alias definition, equivalent to alias.<sub>, and correctly classifies both forms in hook::git_alias_expansion. The command-line cases are now handled. However, the gitconfig alias fallback queries only the plain form:

pexp=$(git -C "$(effective_dir "${w[@]}")" config --get "alias.$sub" 2>/dev/null)

git config --get "alias.c" returns empty when the gitconfig contains [alias] c.command = commit, because these are different config keys. The fallback produces an empty pexp, takes no action, and the guard falls through to [[ "$sub" == "commit" ]] — false for "c" — and returns 0.

Attack path:

[alias]
    c.command = commit

git c -m "non-canonical message"

1. No command-line -c flag → HOOK_GIT_CONFIG_VALUES is empty → hook::git_alias_expansion "c" returns 1 (no entry).
2. inline_alias_handled = 0; fallback: git config --get "alias.c" → empty string (the key is alias.c.command).
3. sub = "c" ≠ "commit" → guard returns 0 (allows).
4. git reads alias.c.command = commit, expands, runs git commit -m "non-canonical message".

block-dangerous-git has no gitconfig fallback at all, so the analogous gap there is pre-existing and consistent with its design. The gap here is specific to block-noncanonical-commit, which added the fallback precisely to catch gitconfig aliases — and that protection is now incomplete for the .command form.

Suggested fix: Add a second --get call when the first returns empty:

pexp=$(git -C "$(effective_dir "${w[@]}")" config --get "alias.$sub" 2>/dev/null)
[[ -z "$pexp" ]] && pexp=$(git -C "$(effective_dir "${w[@]}")" config --get "alias.$sub.command" 2>/dev/null)

Add a test case to block-noncanonical-commit.test.sh exercising a gitconfig alias.c.command = commit alias (the test already uses the live git binary for the fallback path, so a .git/config write in the test teardown cycle would cover this).

Fix this →

---

alias.<name>.command fix — CORRECT

The core change in df23907 is:

[[ "${cv,,}" == "alias.${sub,,}="* || "${cv,,}" == "alias.${sub,,}.command="* ]] || continue

Correctness analysis:

• Value extraction — HOOK_GIT_ALIAS_EXP="${cv#*=}" uses the first = as the split point, which is correct for both forms: alias.rh=status → status, alias.rh.command=reset --hard → reset --hard. ✓
• Last-wins — the loop has no break; later entries overwrite found_kind and HOOK_GIT_ALIAS_EXP, so git's last-wins semantics across the two spellings are modeled correctly. Cross-form test coverage (plain wins over .command decoy, and vice versa) confirms both orderings. ✓
• Case folding — both sides use ${…,,} so alias.RH.COMMAND=… matches sub = "rh". ✓
• Kind determination — kind comes from HOOK_GIT_CONFIG_VALUE_KINDS[i], which was set at parse time to "env" or "inline" based on whether the config value came from --config-env or -c/--config. The .command fix does not change this determination. A --config-env=alias.rh.command=AV entry carries kind "env", returns 2 (shape-refused). ✓
• No over-matching — alias.rh.commandx=… does not match alias.rh.command=* (the = is required immediately after .command). The .nope control test confirms non-command subkeys are not detected. ✓
• No injection surface — sub is the git subcommand word (alphanumeric), expanded into a [[ == ]] pattern via ${sub,,}. No special glob characters expected; the pattern is not interpolated into a shell eval or command substitution. ✓

---

Prior posture — CONFIRMED INTACT

• Structural shape refusal (hook::git_alias_expansion → exit 2) for env-kind aliases, ungated from HOOK_NO_ALIAS, is unchanged. ✓
• Depth-invariant blocking (shape refusal before the HOOK_NO_ALIAS gate in both guards) is unchanged. ✓
• Last-wins for env override (a later inline -c wins over an earlier --config-env) works unchanged — the .command form participates in the same last-wins loop. ✓
• H1 / H2 acknowledged gaps (outer --config-env alias via GIT_CONFIG_PARAMETERS, option-prefixed 3-level chains) are pre-existing, unchanged, and tracked in CRITICAL: git guards fail open on chained inline aliases — one-level re-expansion drops command-line -c/--config-env (case C + config-env H1/H2) #964. ✓

---

Summary

One new security gap is introduced by this commit: the block-noncanonical-commit gitconfig fallback does not check alias.<sub>.command, creating an inconsistency between what the command-line parser now recognizes and what the gitconfig probe covers. The gap is exploitable when a gitconfig alias uses the .command form and no -c flag appears on the command line.

Everything else in the commit is correct and the prior security posture is intact.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df23907aa1

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 5m 27s —— View job

---

PR Review — HEAD aeffecb (merge of main atop cf08116)

• Read core changed files (lib/hook-utils.sh, guard scripts, tests)
• Verify cf08116 cross-spelling union fix correctness
• Check block-noncanonical-commit gitconfig fallback for .command form
• Verify H1/H2 deferred findings are unchanged (no new regressions)
• Assess test coverage for new commit fixes
• Post final review

---

Result: All prior findings closed except one residual gap in the gitconfig fallback

---

The cf08116 cross-spelling union fix — CORRECT

What it does. hook::git_alias_expansion (lib/hook-utils.sh:899–924) now:

• Accumulates the last value WITHIN alias.<sub> (plain) and WITHIN alias.<sub>.command (.command) separately, never collapsing across spellings.
• Returns 2 (shape-refuse) if either spelling has kind == "env" — one spelling's env-kind cannot be masked by the other's inline.
• Returns 0 with HOOK_GIT_ALIAS_EXPS populated with up to two entries (one per present spelling), so both guards re-check every expansion independently with no short-circuit.

Why this is correct. The Codex P1 finding at 16:33Z noted that the previous cross-spelling last-wins model allowed git -c alias.rh='reset --hard' -c alias.rh.command=status rh to be allowed (the .command=status overwrote the dangerous plain value). The max-danger union avoids all git-version-dependent precedence assumptions: on any git, a dangerous expansion in either spelling is always caught.

Test coverage is comprehensive. Both guard suites cover:

• All four union quadrants (dangerous-plain×benign-.command, benign-plain×dangerous-.command, both orderings)
• Env-kind in either spelling refuses despite a benign inline sibling (both orderings)
• Within-spelling last-wins across both spellings independently
• non-command control (.nope subkey must not match)

---

Finding — gitconfig fallback in block-noncanonical-commit does not probe alias.<sub>.command

Severity: MODERATE | Confidence: HIGH

Location: plugins/guardrails/hooks/block-noncanonical-commit.sh:263

df23907 confirmed that git honors alias.<sub>.command as a first-class alias definition and added detection of that spelling to hook::git_alias_expansion for the command-line path. The gitconfig fallback probe — which handles aliases defined in .git/config, ~/.gitconfig, or system config rather than on the command line — was not updated to match:

pexp=$(git -C "$(effective_dir "${w[@]}")" config --get "alias.$sub" 2>/dev/null)

git config --get alias.c returns empty when the gitconfig contains [alias]\nc.command = commit, because these are different config keys. The fallback finds nothing, inline_alias_handled stays 0, the guard checks sub == "commit" (false for "c") and returns 0.

Attack path:

[alias]
    c.command = commit

git c -m "non-canonical bypass message"

1. No command-line -c/--config flag → HOOK_GIT_CONFIG_VALUES = [] → hook::git_alias_expansion "c" returns 1.
2. inline_alias_handled = 0; fallback: git config --get "alias.c" → empty (key is alias.c.command).
3. Guard checks sub == "commit" → false → return 0 (allowed).
4. git reads alias.c.command = commit, runs git commit -m "non-canonical bypass message".

Note: block-dangerous-git has no gitconfig fallback, so the analogous gap there is consistent with its design and not exploitable.

Missing test. No test writes c.command = commit into the test repo's .git/config and asserts that git c -m bypass is blocked (exit 2). The existing tests only exercise the command-line path (-c alias.c.command=commit).

Suggested fix. Add a second probe when the first returns empty:

pexp=$(git -C "$(effective_dir "${w[@]}")" config --get "alias.$sub" 2>/dev/null)
[[ -z "$pexp" ]] && pexp=$(git -C "$(effective_dir "${w[@]}")" config --get "alias.$sub.command" 2>/dev/null)

Add a regression test that writes [alias]\nc.command = commit to the test tmpdir's .git/config (the test harness already runs in a git repo context) and asserts exit 2 for git c -m bypass.

Fix this →

---

H1 and H2 (deferred to #964) — NO REGRESSION

The H1 and H2 alias-chain bypasses are unchanged by cf08116. Confirmed traces:

H1: AV='reset --hard' git --config-env=alias.rh=AV -c alias.sh='!git rh' sh — the outer command's alias.rh (env kind) is in the outer HOOK_GIT_CONFIG_VALUES, but the inner !git rh shell-alias reparse calls hook::git_resolve_subcommand fresh, resetting HOOK_GIT_CONFIG_VALUES = []. hook::git_alias_expansion "rh" returns 1 → inner guard exits 0 while git propagates the alias via GIT_CONFIG_PARAMETERS. Pre-existing, tracked as #964.

H2: git -c alias.rh='-c alias.foo="--config-env=alias.bar=AV bar" foo' rh — at depth 2, the inline alias expansion -c alias.foo=... is a new inline entry (kind = "inline"), not env. hook::git_alias_expansion "foo" returns 0 (not 2); with HOOK_NO_ALIAS=1 the inline re-expansion is suppressed; foo is not dangerous → exit 0 while git runs the chain. Pre-existing, tracked as #964.

The cf08116 changes (max-danger union, HOOK_GIT_ALIAS_EXPS array, iterating both guard consumers) have no structural effect on H1 or H2 — those bypasses operate at a different level (recursive check_segment scope and alias chain depth, not cross-spelling classification). No regression.

---

What the PR gets right

• hook::git_alias_expansion function design — max-danger union correctly eliminates git-version-dependent precedence assumptions; the parallel plain_*/cmd_* variable pairs cleanly separate the two spellings; ${HOOK_GIT_CONFIG_VALUE_KINDS[i]:-inline} defensive fallback is correct.
• Guard consumers — both block-dangerous-git.sh and block-noncanonical-commit.sh correctly iterate HOOK_GIT_ALIAS_EXPS with the ${arr[@]+"${arr[@]}"} idiom for set -u safety; neither breaks on the first expansion.
• Shape refusal ungated from HOOK_NO_ALIAS — hook::git_alias_expansion is called unconditionally before the HOOK_NO_ALIAS gate in both guards; only inline re-expansion is depth-bounded. Correct.
• Case folding — ${key,,} / ${sub,,} on both spelling match arms; cv#*= extracts the expansion from the original (unfolded) value. Correct.
• Env-kind union — [[ "$plain_kind" == "env" || "$cmd_kind" == "env" ]] && return 2 — correctly treats an env spelling in either position as shape-refused, regardless of the sibling's kind.
• HOOK_GIT_CONFIG_VALUE_KINDS parallel array — maintained 1:1 with HOOK_GIT_CONFIG_VALUES across all four parse branches; kind="${HOOK_GIT_CONFIG_VALUE_KINDS[i]:-inline}" is safe on any index.
• Blast-radius sync — all 12 plugin copies re-synced byte-identical. Version bumps correct.

---

Cosmetic (pre-existing, not blocking)

lib/hook-utils.sh:714: *) ((i++)) ;; inside the env handler's if ((env_past_optmark == 0)) && [[ ... -* ]] branch is dead code — the if condition guarantees any token entering the case starts with -. Same pattern in the sudo handler. Not a correctness or security issue; flagged across multiple prior review rounds.

---

Summary

The cf08116 max-danger union fix is architecturally sound and closes the Codex P1 about cross-spelling masking. One concrete gap remains: the block-noncanonical-commit gitconfig fallback probes only alias.$sub, not alias.$sub.command, creating an exploitable inconsistency with the command-line parser. The fix is a single extra git config --get call with a regression test. The two H1/H2 pre-existing bypasses are tracked in #964 and have not regressed.

[claude]

Claude finished @kyle-sexton's task in 5m 52s —— View job

---

Security Review — HEAD aeffecb

• Read PR diff to identify changed files at current HEAD
• Read lib/hook-utils.sh — hook::git_alias_expansion and HOOK_GIT_ALIAS_EXPS union design (cf08116)
• Read guard scripts — block-dangerous-git.sh, block-noncanonical-commit.sh
• Read test files — verify coverage for cross-spelling masking
• Verify cf08116 fix: alias spelling union, no cross-spelling last-wins
• Check for new injection surfaces or fail-open conditions
• Post findings

---

Head commit reviewed: aeffecb (merge restack over main)
Security-critical commits in this review scope: cf08116 (fail-closed on alias spelling union, not cross-spelling last-wins) and predecessors

---

No new P1 findings. All prior security issues confirmed closed.

The cf08116 fix correctly implements the max-danger union approach. Analysis of every security-relevant surface in the current HEAD follows.

---

cf08116 union design — CORRECT

lib/hook-utils.sh:899–924

The prior round-1 fix used cross-spelling last-wins (hook::git_alias_expansion kept a single accumulator and cmd_exp/cmd_kind could overwrite plain_exp/plain_kind). That allowed a benign .command value arriving after a dangerous plain value to mask it — a fail-open on any git version where alias.<sub> wins over .command. cf08116 replaces this with independent accumulators (plain_exp/plain_kind and cmd_exp/cmd_kind) and a union rule:

[[ "$plain_kind" == "env" || "$cmd_kind" == "env" ]] && return 2
[[ -n "$plain_kind" ]] && HOOK_GIT_ALIAS_EXPS+=("$plain_exp")
[[ -n "$cmd_kind" ]]   && HOOK_GIT_ALIAS_EXPS+=("$cmd_exp")
((${#HOOK_GIT_ALIAS_EXPS[@]})) && return 0
return 1

All four cases are correct:

plain_kind	cmd_kind	result
env	any	return 2 (shape-refuse — correct)
any	env	return 2 (shape-refuse — correct)
inline	""	rc=0, EXPS=(plain_exp)
""	inline	rc=0, EXPS=(cmd_exp)
inline	inline	rc=0, EXPS=(plain_exp, cmd_exp)

In the last three rows, both guards iterate every entry in HOOK_GIT_ALIAS_EXPS and re-check each expansion independently — a dangerous entry in one spelling can no longer be suppressed by a benign entry in the other.

Masking case (the Codex P1 from df23907): git -c alias.rh='reset --hard' -c alias.rh.command=status rh → plain_exp="reset--hard" (inline), cmd_exp="status" (inline) → EXPS=("reset --hard", "status") → reset --hard is re-checked and blocks. Correct ✓

Reverse masking: git -c alias.rh=status -c alias.rh.command='reset --hard' rh → EXPS=("status", "reset --hard") → reset --hard is re-checked and blocks. Correct ✓

Env masking across spellings (both orders): git --config-env=alias.rh=AV -c alias.rh.command=status rh → plain_kind="env" → return 2 unconditionally. git --config-env=alias.rh.command=AV -c alias.rh=status rh → cmd_kind="env" → return 2. Both correct ✓

---

Structural shape refusal — CONFIRMED INTACT

block-dangerous-git.sh:235–236

hook::git_alias_expansion is called BEFORE the HOOK_NO_ALIAS gate. The alias_rc == 2 path (exit 2) fires at every recursion depth — a wrapping inline alias that expands to --config-env=alias.<sub>=… is refused at depth 2, as required. The depth-invariant block from 7ea3b9d is preserved. ✓

---

No injection surface in the new code

• key="${cv%%=*}" strips at the first =; no evaluation occurs.
• [[ "${key,,}" == "alias.${sub,,}" ]] and [[ "${key,,}" == "alias.${sub,,}.command" ]] are pure bash string comparisons; sub is not glob-expanded.
• HOOK_GIT_ALIAS_EXPS entries are consumed with ${arr[@]+"${arr[@]}"} (set -u safe), then passed to the existing alias-expansion consumers (shell-alias reparse via hook::bash_parse_segments, or git-alias splice via hook::env_s_split). These paths were already present and their injection safety was verified in prior rounds.

---

Gitconfig fallback gap — PRE-EXISTING, filed as #1022 (not blocking)

block-noncanonical-commit.sh:263:

pexp=$(git -C "$(effective_dir "${w[@]}")" config --get "alias.$sub" 2>/dev/null)

git config --get "alias.c" returns empty when the gitconfig contains [alias "c"] command = commit (a different config key than [alias] c = commit). The fallback therefore misses gitconfig-resident alias.<sub>.command aliases. Attack: a user with [alias "c"] command = commit in ~/.gitconfig can run git c -m "non-canonical message" without triggering block-noncanonical-commit.

This gap is pre-existing — it was not introduced by this PR. It was present before df23907 (at that point, neither gitconfig nor command-line .command aliases were detected). This PR closed the command-line case; the gitconfig case remains open. This was identified in the 16:30:51Z review round and split to issue #1022 ("config-write threat model, orthogonal"). block-dangerous-git has no gitconfig fallback at all — consistent with its design.

Not a regression; not blocking per operator tracking of #1022.

---

Deferred items — unchanged, not blocking

• H1/H2 from CRITICAL: git guards fail open on chained inline aliases — one-level re-expansion drops command-line -c/--config-env (case C + config-env H1/H2) #964 (AV='reset --hard' git --config-env=alias.rh=AV -c alias.sh='!git rh' sh; outer -c/--config-env entries not visible to inner shell-alias reparse) — pre-existing, explicitly filed, scoped as a follow-on defer-forbidden CRITICAL.
• env -u/env -i over-blocking (P2) — guard blocks a command git would also reject (fatal on missing var). Conservative direction; tracked in guardrails: env -u / env -i prefixes false-positive under value-blind config-env shape guard (over-block, fail-safe) #1013.

---

Cosmetic (pre-existing, not blocking)

lib/hook-utils.sh:696: *) ((i++)) ;; inside the env handler option-processing branch is dead code — the outer if ((env_past_optmark == 0)) && [[ "${w[i]}" == -* ]] guarantees any token reaching the case starts with -. Same pattern in the sudo handler. Not a correctness or security issue.

---

Summary

The cf08116 union fix is correct and complete for its stated scope. No new P1 or P2 security issues are present in the current HEAD. The one confirmed gap (gitconfig fallback for .command form) is pre-existing and tracked as #1022.

This branch is clear for merge from a security standpoint, subject to the pre-existing H1/H2 (#964) and #1022 deferrals and pending operator re-ratification of the structural reposture.