fix(guardrails): block --force-with-lease forms that state no expected value

[kyle-sexton]

No linked issue

Summary

block-dangerous-git treated every --force-with-lease spelling as safe force. Two of them are not, by git's own account, and the guard let them through while blocking --force for the same underlying hazard.

Fix

--force-with-lease and --force-with-lease=<refname> state no expected value, so git leases against the remote-tracking ref. From git-push(1), "A general note on safety":

supplying this option without an expected value, i.e. as --force-with-lease or --force-with-lease=<refname> interacts very badly with anything that implicitly runs git fetch on the remote to be pushed to in the background, e.g. git fetch origin on your repository in a cronjob.

The protection it offers over --force is ensuring that subsequent changes your work wasn't based on aren't clobbered, but this is trivially defeated if some background process is updating refs in the background.

So the lease can be satisfied by a ref some other process fetched, and the push clobbers work the pusher never saw — the failure mode --force has, reached by a slower road. Git also marks every form other than =<refname>:<expect> experimental.

Those two no-expected-value forms are now blocked under a new push-lease-unsafe token, unless --force-if-includes (git 2.30+) is present — git's documented mitigation for exactly these forms, which it declares a no-op alongside an explicit :<expect>.

What still passes

• --force-with-lease=<refname>:<expect>, including an empty <expect> (asserts the ref must not exist — still explicit).
• Any lease form paired with --force-if-includes.
• A push dry-run, which disarms the whole check as before.

Detection detail

Unique-prefix abbreviations are handled. --force, --force-with-lease and --force-if-includes share the --force prefix, so --force-w and --force-i are the shortest spellings git accepts, and both match. A shorter --forc is ambiguous and git rejects it outright, which is why the exact --force arm needs no abbreviation handling. After --, words are operands rather than flags, so a literal --force-if-includes refspec does not disarm the check.

Why the hook, and not the permission deny-list

This started from the opposite direction: a --force-with-lease push was denied by the claude-permissions floor, and the obvious fix looked like removing that deny.

Research against the permissions docs killed that:

Rules are evaluated in order: deny, then ask, then allow. The first match in that order determines the outcome, and rule specificity doesn't change the order.

A broad deny rule like Bash(aws *) blocks every matching call, including calls that also match a narrower allow rule like Bash(aws s3 ls), so a deny rule can't carry allowlist exceptions.

Bash rules are whole-string globs with * as the only metacharacter — no negation. So "deny the unsafe lease forms, allow =<ref>:<expect>" is not expressible in the permission language, and the docs name a PreToolUse hook as the mechanism for what globs cannot express. This is that hook.

The blunt deny in melodic-software/standards is therefore doing a job the permission layer cannot do precisely. Removing it before this landed would have been a net widening — it would have exposed the unsafe bare form, which guardrails permitted. With this merged, that deny can be dropped and the policy becomes: safe form allowed, unsafe forms blocked, both enforced where the distinction is actually expressible.

Testing

block-dangerous-git.test.sh — 261 pass, 0 fail. 14 new cases: bare, =<refname>, =<refname>:<expect>, empty <expect>, both abbreviations, --force-if-includes alone and paired, dry-run, and the -- operand boundary. Three existing cases asserted the old permissive behavior and were updated to the new contract; one PowerShell case likewise, plus a new PowerShell case for the passing form.

shellcheck -x clean at the repo ruleset. markdownlint-cli2 clean. plugin.json validates.

Related

• melodic-software/standards#267 — in flight on the same claude-permissions component (it trims the allow floor; this affects deny policy). Its README states "Force/destructive spellings stay covered by deny, which always wins" — the follow-up that drops the lease deny will need to update that sentence.
• Follow-up, not in this PR: remove the four --force-with-lease deny patterns (Bash and PowerShell mirrors) from the claude-permissions component now that the precise check exists here.

[claude]

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

---

Code Review

• Gather context and read changed files
• Review hook logic (block-dangerous-git.sh)
• Review test coverage (block-dangerous-git.test.sh)
• Review manifest and documentation changes
• Post findings

---

The implementation is well-reasoned and the logic is correct. The pre-scan / main-scan dual-pass design correctly handles ordering (e.g. --force-if-includes appearing after --force-with-lease on the same command line), the -- operand boundary is respected in both passes, and the abbreviation floors are sound (--force-w and --force-i at length 7 are exactly the shortest unique prefixes).

Two minor findings and one nitpick below.

---

Finding 1 — if_includes not declared local (low severity)

block-dangerous-git.sh L232

local nseg gi k x rest ch sub sub_idx staged worktree dry excl pos opseen

if_includes is missing from this local declaration. In practice this isn't a runtime bug — the if_includes=0 reset at the top of the push) arm (line 327) always runs before any read, so state from a previous invocation or a nested recursive call cannot bleed in. But dry and every other per-push scan variable are declared local here, and the inconsistency would become a real bug if the reset were ever moved or the variable were read outside the push) arm. Adding it to the local line is a one-word fix.

Fix this →

---

Finding 2 — No allow-list contract test for push-lease-unsafe (low severity)

block-dangerous-git.test.sh L294–303

The existing allow-list section covers push-force and reset-hard but not the new push-lease-unsafe token. The allow-list path (allowed()) is generic and will work, but without a contract test there's nothing catching a future rename of the form token that silently breaks the escape hatch. The minimal addition would be:

run "allow-list push-lease-unsafe → bare lease allowed" "git push --force-with-lease" 0 \
  CLAUDE_PLUGIN_OPTION_BLOCK_DANGEROUS_GIT_ALLOW=push-lease-unsafe

Fix this →

---

Nitpick — block_dangerous_git_enabled description not updated

plugin.json L44

The block_dangerous_git_enabled toggle description still reads "push --force, reset --hard, clean -f, worktree-wide checkout/restore discards" — it no longer mentions the new unsafe lease forms. The block_dangerous_git_allow description (line 98) was correctly updated. The toggle description is a brief summary, so not updating it is defensible, but a new user reading just the toggle description won't know the scope includes lease forms.

---

Everything else looks correct

• The is_lease_opt() function stripping =... before abbreviation matching (L165) is the right approach.
• The [[ "$x" != *=*:* ]] predicate (L397) correctly passes =: (empty expect) and =refname:sha while blocking bare and =refname.
• The pre-scan correctly breaks at --, so --force-if-includes appearing as an operand does not disarm the lease check — the test at line 47 validates this.
• if_includes is scoped inside the push) arm and read only there; the --force-* fallthrough from the --force exact-match arm is correctly structured so --force never reaches the lease branch.
• Version bump 0.14.2 → 0.15.0 is appropriate for a behavior change.

[claude]

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

---

Security Review — PR #1275

• Load tools
• Fetch PR diff
• Read changed files
• Analyze for security vulnerabilities
• Post findings

---

Finding 1 — IMPORTANT · Confidence: PLAUSIBLE

--no-force-if-includes resets git's mitigation but not the hook's if_includes flag, creating a bypass

The pre-scan loop tracks whether --force-if-includes appeared by setting if_includes=1. The main scan then skips the push-lease-unsafe block when if_includes is 1. However, git's --force-if-includes is a boolean flag and git supports the --[no-]flag convention (via its parse-options framework, which enables the negation form unless PARSE_OPT_NONEG is set — not the case here). --no-force-if-includes would therefore be accepted by git ≥ 2.30 and would negate the mitigation at runtime.

The --no-* arm in the pre-scan only resets dry:

# block-dangerous-git.sh, pre-scan (around line 339)
--no-*)
  abbrev_match "dry-run" "--${x#--no-}" 2 && dry=0
  ;;

It never resets if_includes. An adversarial command of the form:

git push --force-with-lease --force-if-includes --no-force-if-includes origin main

traces through the pre-scan as:

• --force-if-includes → falls through --* ;;& to *) → if_includes=1
• --no-force-if-includes → matches --no-* → only dry-run check fires → ;; — if_includes remains 1

The main scan then sees --force-with-lease, finds !if_includes is false, and allows the push. Git, however, honours the last-wins rule for --[no-] pairs and executes the push without the mitigation — exactly the unsafe lease form the PR was written to block.

Relevant code: block-dangerous-git.sh L339-L353 (pre-scan --no-* arm) and L395-L401 (main scan branch decision).

Fix: add a symmetric reset in the --no-* pre-scan arm:

--no-*)
  abbrev_match "dry-run" "--${x#--no-}" 2 && dry=0
  abbrev_match "force-if-includes" "--${x#--no-}" 7 && if_includes=0
  ;;

The same pattern the script already uses for --no-dry-run / dry. Fix this →

---

No other findings

The remainder of the changes are clean from a security standpoint:

• Input handling: stdin is read through hook::buffer_stdin, the command is extracted with jq -r via printf '%s' "$INPUT" | jq -r ... (no shell-expansion surface), and all jq interpolations use --arg (no injection path).
• is_lease_opt extraction (${1%%=*}): strips the value portion before the abbrev_match check; the subsequent [[ "$x" != *=*:* ]] pattern correctly distinguishes the explicit-expectation form (=<refname>:<expect>) from the bare and =<refname>-only forms.
• -- operand boundary: the pre-scan breaks on --, so a literal --force-if-includes refspec after -- cannot disarm the check. The test case git push --force-with-lease -- --force-if-includes (blocked, exit 2) covers this.
• Allow-list injection: allowed() uses a comma-anchored substring match on ",${CLAUDE_PLUGIN_OPTION_BLOCK_DANGEROUS_GIT_ALLOW:-}," which prevents a value like push-lease-unsafeXXX from matching push-lease-unsafe. Safe.
• Length gate: the 16384-char hard limit and fail-closed behaviour on over-length commands are unchanged and correct.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1a75883e1

ℹ️ 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]

Classification of the code review findings, validated against the PR head f1a75883:

#	Finding	Classification	Evidence	Reacted
1	if_includes missing from check_segment's local declaration (L232)	VALID — fixing	Confirmed: if_includes is assigned at L327/L352 and read at L397 but never declared local, while every sibling per-push scan variable (dry, excl, pos, opseen) is. No runtime leak today — the if_includes=0 reset at the top of the push) arm precedes every read, and both recursive check_segment paths (the bash -c re-parse and the alias re-expansion) return or recurse before that arm — so this closes a latent leak rather than a live bug, exactly as the finding states.	👍
2	No allow-list contract test for the push-lease-unsafe token	VALID — fixing	Confirmed: grep -n "push-lease-unsafe" block-dangerous-git.test.sh returned zero matches; the allow-list section covered only push-force and reset-hard. allowed() is token-generic, so the escape hatch works but a rename would silently break it untested. Adding both the positive case (push-lease-unsafe → bare lease allowed) and a negative guard (push-force alone → bare lease still blocked), so a token rename or a cross-token leak both fail loudly.	👍
3	Nitpick — block_dangerous_git_enabled description omits the new lease forms	VALID — fixing	Confirmed: plugin.json L44 still read "push --force, reset --hard, clean -f, worktree-wide checkout/restore discards" while the sibling block_dangerous_git_allow description at L98 was updated. plugin.json is already touched by this commit, so the edit is in scope.	👍

Fixes are on the way in a follow-up commit; I will cite the SHA here once pushed.

[kyle-sexton]

Classification of the security review finding, validated against the PR head f1a75883:

#	Finding	Classification	Evidence	Reacted
1	IMPORTANT — --no-force-if-includes resets git's mitigation but not the hook's if_includes flag, creating a bypass	VALID — fixing	Confirmed on both axes. Git accepts the negation: git push -h on git 2.54.0 lists --[no-]force-if-includes verbatim in its own usage output, so the negation is a real, parse-options-generated spelling, not an inference. The hook does not track it: the pre-scan --no-* arm reset only dry, and it terminates with ;; (no ;;& fallthrough), so --no-force-if-includes reached neither the *) arm that sets if_includes=1 nor any reset — a preceding --force-if-includes left the flag latched at 1. The main scan's ((!if_includes)) guard then skipped the push-lease-unsafe block while git, applying last-wins, pushed without the mitigation. This is the same defect the P1 inline thread reports; both are fixed by one change.	👍

Fix applied is exactly the symmetric reset you proposed — abbrev_match "force-if-includes" "--${x#--no-}" 7 && if_includes=0 in the pre-scan --no-* arm, mirroring the existing --[no-]dry-run handling — plus regression tests covering the bypass command, the re-armed order (--no-force-if-includes --force-if-includes → allowed), the abbreviated negation (--no-force-i), and negative guards proving --no-force-w and --no-dry-run do not clear the mitigation. I will cite the commit SHA here once pushed.

[claude]

Claude finished @kyle-sexton's task in 1m 31s —— View job

---

Code Review — PR #1275

• Read changed files at HEAD (e0bfe61)
• Review the --no-force-if-includes fix
• Verify all prior findings were addressed
• Post findings

---

All four findings from the prior code review and security review have been addressed in e0bfe61.

Finding 1 — if_includes missing from local declaration ✅ Fixed

block-dangerous-git.sh L232 — if_includes is now declared alongside every sibling per-push scan variable. No live leak was present (the if_includes=0 reset at line 330 precedes every read), but the omission is closed.

Finding 2 — No allow-list contract test for push-lease-unsafe ✅ Fixed

block-dangerous-git.test.sh L308–311 — Two tests added: positive case (push-lease-unsafe token allows the bare form) and a cross-token negative guard (push-force alone does not unlock push-lease-unsafe). A token rename or cross-token leak will now fail loudly.

Security finding — --no-force-if-includes disarms the lease check ✅ Fixed

block-dangerous-git.sh L343 — The pre-scan --no-* arm now resets if_includes symmetrically with dry:

--no-*)
  abbrev_match "dry-run" "--${x#--no-}" 2 && dry=0
  abbrev_match "force-if-includes" "--${x#--no-}" 7 && if_includes=0
  ;;

Five regression tests cover the bypass command, the re-armed opposite order, the abbreviated negation --no-force-i, and two negative guards proving --no-force-w and --no-dry-run leave the mitigation intact. The -- boundary test (operand form does not disarm) was already present from the initial commit.

Nitpick — toggle description ✅ Fixed

plugin.json L44 — Description now reads "push --force, push --force-with-lease stating no expected value, reset --hard, clean -f, worktree-wide checkout/restore discards".

---

Overall

The implementation is correct and complete at HEAD. The dual-pass (pre-scan + main scan) design correctly handles argument ordering for both dry and if_includes, the -- operand boundary is respected in both passes, is_lease_opt strips =... before abbreviation matching, and the *=*:* predicate correctly admits the explicit-expectation form (including empty :<expect>). No further issues found.

[claude]

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

---

Security Review — PR #1275

• Read PR diff and changed files
• Analyze hook logic for security vulnerabilities
• Review argument-parsing bypass surfaces
• Post findings

---

No security issues found

The IMPORTANT finding from the previous security review (--no-force-if-includes resets git's mitigation but not the hook's flag, creating a bypass) has been correctly fixed in commit e0bfe61. The fix warrants a close read — here is the full verification.

---

Fix verification — --no-force-if-includes bypass (previously IMPORTANT)

block-dangerous-git.sh L341–344

--no-*)
  abbrev_match "dry-run" "--${x#--no-}" 2 && dry=0
  abbrev_match "force-if-includes" "--${x#--no-}" 7 && if_includes=0
  ;;

The --no-* arm ends with ;; (not ;;&), so a matching word exits the case immediately — it does not fall through to the *) arm that would re-set if_includes=1. Execution for --no-force-if-includes:

1. x = "--no-force-if-includes" → matches --no-*.
2. "--${x#--no-}" → "--force-if-includes". abbrev_match "dry-run" "--force-if-includes" 2 → "dry-run" == "force-if-includes"*? No.
3. abbrev_match "force-if-includes" "--force-if-includes" 7 → p="force-if-includes", length 17 ≥ 7, "force-if-includes" == "force-if-includes"*? Yes → if_includes=0. ✓
4. ;; breaks out. The *) arm (abbrev_match "force-if-includes" "${x%%=*}" 7 && if_includes=1) never runs.

Abbreviated negation (--no-force-i): "--force-i" → p="force-i", length 7 ≥ 7, "force-if-includes" == "force-i"*? Yes → correctly clears the flag. ✓

Negative guard (--no-force-w): "--force-w" → p="force-w", "force-if-includes" == "force-w"*? No. Does not clear if_includes. ✓

Negative guard (--no-dry-run): "--dry-run" → "force-if-includes" == "dry-run"*? No. Does not clear if_includes. ✓

Five regression tests on lines 48–52 lock in all four variants (negated-after-set, re-armed, abbreviated, wrong-suffix negative guards). The previously exploitable command git push --force-with-lease --force-if-includes --no-force-if-includes now correctly exits 2.

---

Other surfaces reviewed (all clean)

• is_lease_opt / [[ "$x" != *=*:* ]] predicate: --force-with-lease=main: (empty expect, means ref must not exist) — *=*:* matches on zero chars after : and the block is skipped. --force-with-lease= (no refname, no colon) — *=*:* does NOT match (no :) and the form is correctly blocked. --force-with-lease=main:abc — blocked correctly passes.
• ${x%%=*} in pre-scan if_includes tracking: strips longest =... suffix before abbrev_match, so --force-if-includes=foo (not a real git form) would still set the flag, not produce a false result. No injection surface.
• allowed() comma-anchor: ,push-lease-unsafeXXX, does not match ,push-lease-unsafe,. Prefix injection blocked by design.
• if_includes now local: declared on line 232 alongside every sibling per-push scan variable. Prevents state leak if the reset at line 330 were ever moved. Two check_segment recursion paths (shell alias re-parse and git alias splice) each reinitialize the variable in their own stack frame.
• -- operand boundary: pre-scan breaks on -- (line 335), so --force-if-includes appearing as a refspec after -- cannot set the flag and cannot disarm the lease check. The existing test at line 47 covers this; it was not changed and still expects exit 2.
• plugin.json description change: prose-only, no schema or permission change.

[kyle-sexton]

All four findings from the code review and the security review are fixed in e0bfe61d:

Finding	Source	Resolution
--no-force-if-includes bypass (IMPORTANT / P1)	security review + codex inline thread	--no-* pre-scan arm now clears if_includes, mirroring the --[no-]dry-run handling; 5 regression tests added
if_includes missing from local	code review #1	added to check_segment's local declaration
No allow-list contract test for push-lease-unsafe	code review #2	added both the positive case and a negative guard (push-force alone leaves the bare lease blocked)
Toggle description omits the lease forms	code review nitpick	block_dangerous_git_enabled description now names the no-expected-value lease form

Verification: full hook suite 268 passed, 0 failed; shellcheck clean on both shell files; plugin.json parses. No version bump — 0.15.0 in this PR already covers the behavior change.

Remaining blocker: the branch is behind main and GitHub reports BLOCKED. Leaving that, and the review threads, for the next cycle rather than resolving them here.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0bfe61d44

ℹ️ 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 encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 91e612517a

ℹ️ 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 15s —— View job

---

Code Review — PR #1275

• Gather context and identify what changed at HEAD (f345a1b)
• Read the hook and test files at the current HEAD
• Review the "decide lease family after scan" logic
• Evaluate Codex P2 findings against the new implementation
• Post findings

---

Reviewed the changes in f345a1b (the only unreviewed commit at HEAD). The prior code review and security review findings were all addressed in e0bfe61d and confirmed clean by two independent passes. This review covers only the structural change introduced by f345a1b.

---

What changed

The previous pre-scan acted on the lease the moment it matched a lease-family option — the decision was mid-scan rather than deferred. This meant --force-with-lease --no-force-with-lease was blocked even though git's last-wins rule for --[no-]force-with-lease makes the command a plain push. The fix introduces a tri-state lease variable:

• 0 — no lease option seen
• 1 — unsafe lease (bare or =<refname> only)
• 2 — safe lease (=<refname>:<expect>)

The block decision is now made once, after the full pre-scan, in the same place as the dry early return. The --force-* arm in the main scan is now a no-op (--force-*) ;;), since the lease is fully decided in the pre-scan.

---

Correctness — the logic traces correctly

Key flows through the new --no-* arm:

Input	"--${x#--no-}"	lease	if_includes
--no-force-with-lease	--force-with-lease	cleared	unchanged
--no-force-i (abbrev of --force-if-includes)	--force-i	unchanged	cleared
--no-force-w (abbrev of --force-with-lease)	--force-w	cleared	unchanged
--no-dry-run	--dry-run	unchanged	unchanged

The cross-family separation is correct: --no-force-w only matches abbrev_match "force-with-lease" 7 (since "force-with-lease" starts with "force-w"), not abbrev_match "force-if-includes" 7 (since "force-if-includes" does not start with "force-w"). ✓

The ((k++)); continue pattern in the --no-* arm skips the outer loop's ((k++)), so each word is consumed exactly once. The ;; after continue is unreachable dead code — it's syntactically required to close the case pattern but has no runtime effect. ✓

The four new test cases confirm the fixed behaviors: negated lease allowed, re-armed lease blocked, bare negation alone allowed, explicit-expectation negated allowed.

---

Finding — second Codex P2 still partially open (low severity)

block-dangerous-git.sh L369

The Codex P2 finding on e0bfe61d noted:

git push --force-with-lease --force-with-lease=refs/heads/main:<expected> origin HEAD:main — git uses the explicit per-ref lease for main; the bare fallback never applies to a covered ref.

In the new implementation, the last-wins model resolves this one way only:

• --force-with-lease then --force-with-lease=main:abc → lease=1 then lease=2 → not blocked ✓
• --force-with-lease=main:abc then --force-with-lease → lease=2 then lease=1 → blocked (false positive)

In git, per-ref explicit leases and the bare fallback are additive, not last-wins against each other — the explicit entry for main holds regardless of where the bare form appears. So the reversed-order case is a false positive.

This is a conservative miss, not a security bypass. The only fix would require collecting all pushed refspecs and matching them against per-ref leases to determine whether every ref is covered — substantially more complexity than this PR should carry. Worth a comment in the code or a test noting the known limitation, so a future reader understands why the =ref:sha --force-with-lease order is blocked:

# Note: `--force-with-lease=ref:sha --force-with-lease` is treated as lease=1
# (last-wins over the single state variable). Git treats them as additive, not
# last-wins, so this is a conservative false positive for that specific order.
# Resolving it would require tracking per-ref expectations against pushed
# refspecs, which is out of scope here.

Fix this →

---

Everything else looks correct

• The --no-* arm's ((k++)); continue correctly prevents --no-force-with-lease from falling through to the *) arm and being re-evaluated as a positive lease option. ✓
• is_lease_opt "$x" in the *) arm (reached via --* fall-through with ;;&) correctly assigns lease=1 or lease=2 based on [[ "$x" == *=*:* ]]. ✓
• The --force-*) ;; no-op in the main scan is correct — the lease decision is already made. ✓
• The -- break in both scans means a literal --force-with-lease refspec after -- does not affect any state variable. ✓
• lease is declared local alongside if_includes and all other per-push scan variables on the same line (L232). ✓
• Test count: 273 pass, 0 fail (commit message). Four new cases cover the core scenarios.

The fix for the first Codex P2 (--force-with-lease --no-force-with-lease blocked incorrectly) is complete and correct. The remaining partial gap is a known conservative limitation of the last-wins model.

[claude]

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

---

Security Review — PR #1275

• Load tools
• Fetch PR diff
• Read changed files at HEAD (f345a1b)
• Analyze for security vulnerabilities
• Post findings

---

The new commit (f345a1b) addresses the Codex P2 finding: --force-with-lease --no-force-with-lease was blocked when it should pass. Here is the full security analysis.

---

No new security issues found

The implementation is sound. Full verification below.

---

--no-force-with-lease now clears lease in the pre-scan

block-dangerous-git.sh L342–351

--no-*)
  abbrev_match "dry-run" "--${x#--no-}" 2 && dry=0
  abbrev_match "force-if-includes" "--${x#--no-}" 7 && if_includes=0
  abbrev_match "force-with-lease" "--${x#--no-}" 7 && lease=0
  ((k++))
  continue
  ;;

Trace for the previously-blocked command git push --force-with-lease --no-force-with-lease:

1. --force-with-lease → is_lease_opt true, no =*:* → lease=1
2. --no-force-with-lease → abbrev_match "force-with-lease" "--force-with-lease" 7 → true → lease=0; ((k++)); continue

Post-scan: lease=0, if_includes=0, dry=0. Decision: lease == 1 is false → no block. ✓

((k++)); continue vs ;; — no functional difference, correctly documented

The --no-* arm already terminated with ;; in prior commits, which prevents fall-through to --* or *). The ((k++)); continue form is equivalent (same net k increment) and makes the intent explicit to future maintainers, per the inline comment. No gap introduced.

Abbreviated negation --no-force-w

p = "force-w" (after stripping --no-), length 7 ≥ 7, [[ "force-with-lease" == "force-w"* ]] → true → lease=0. ✓

--no-force-with-lease does not clear if_includes

abbrev_match "force-if-includes" "--force-with-lease" 7 → p = "force-with-lease", [[ "force-if-includes" == "force-with-lease"* ]] → false. The mitigation flag is untouched. ✓ (Test at line 51 locks this in.)

--no-force-with-lease in the main scan is a no-op

The main (second) scan has no --no-* arm. --no-force-with-lease matches --* → is_push_value_opt false → ;;& falls through → not --force → not --force-* → not +* → not -[A-Za-z]* → * → abbrev_match "mirror" → false. Nothing fires. ✓

Post-scan decision is correctly placed

L378–382 — the push-lease-unsafe block fires once, after the pre-scan completes, consistent with dry and if_includes. An early-match (removed by this commit) would have allowed --force-with-lease=main:sha --force-with-lease to slip through when the explicit form came first. ✓

--force-* arm in the main scan is now a no-op ;;

L420 — the former per-occurrence lease check is gone. --force-with-lease in the main scan now falls into the empty --force-*) ;; arm. All lease-family decisions live in the pre-scan. No double-fire, no missed coverage. ✓

Operand boundary, allow-list, input handling — unchanged and still correct

• Pre-scan breaks at -- (line 336); --force-with-lease after -- is a refspec operand and does not set lease. Test at line 47 covers this.
• allowed() uses the comma-anchored pattern ",${tok}," — prefix injection into the allow-list is blocked by design.
• COMMAND is extracted via jq -r '.tool_input.command // empty' with printf '%s'; no shell-expansion surface.

---

Behavioral note (not a security issue)

git push --force-with-lease --force-with-lease=main:sha now resolves as lease=2 (explicit expectation wins, last-wins). The prior version blocked it because the per-occurrence main-scan check fired on the bare --force-with-lease regardless of what followed. The new behavior is consistent with the post-scan, last-wins model used for all three tracked flags, and git's own documentation confirms the explicit per-ref entry overrides the bare fallback for the named ref. This is a permissiveness increase for the mixed-form command. It was raised as a Codex P2 against e0bfe61 and is not a vulnerability introduced here — it is a known limitation of the last-wins tracking model, noted for completeness.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f345a1b0f7

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8fb92af7c1

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4dc425a39

ℹ️ 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 17s —— View job

---

Code Review — PR #1275

• Load tools
• Read changed files at HEAD (3b0cad8)
• Analyze Codex P1 and P2 findings from latest round
• Verify prior findings are still addressed
• Post review

---

Reviewed the changes at HEAD (3b0cad8). The commits 8fb92af and b4dc425 addressed the prior Codex P1 about --no-force-with-lease and --force-with-lease=<ref>:<mutable-name> respectively, and all four findings from the e0bfe61 round remain fixed. However, the two Codex findings raised against b4dc425a39 at 20:02 are both still open at HEAD.

---

Finding 1 — IMPORTANT · Confirmed open (Codex P1, b4dc425 review)

--force-if-includes must not bypass the block when the explicit expect is mutable

block-dangerous-git.sh L406
block-dangerous-git.test.sh L63

The hook allows git push --force-with-lease=main:origin/main --force-if-includes (exit 0). Test line 63 explicitly asserts this:

run "git push lease pinned to a movable name + --force-if-includes (mitigated, allowed)" \
  "git push --force-with-lease=main:origin/main --force-if-includes origin main" 0

This is wrong. git-push(1) explicitly states under --force-if-includes:

If passed along without --force-with-lease, or along with --force-with-lease=<refname>:<expect>, it is a "no-op".

The trace through the current code:

1. --force-with-lease=main:origin/main → is_lease_opt true; lease_pins_an_immutable_expect → expect="origin/main", not hex → false → lease_bare=1
2. --force-if-includes → if_includes=1
3. Decision: ((lease_bare)) && ((!if_includes)) → 1 && 0 → not blocked

origin/main is resolved at push time; a background fetch updates it; the hook misses it. This is the Codex-described failure mode.

Root cause: lease_bare conflates two distinct unsafe spellings that differ in whether --force-if-includes is a documented mitigation:

Spelling	--force-if-includes effect	Should bypass block?
--force-with-lease (bare)	documented mitigation	yes
--force-with-lease=<ref> (no colon)	documented mitigation	yes
--force-with-lease=<ref>:<mutable>	explicitly a no-op (git docs)	no

Fix — split into two state variables so the bypass is scoped correctly:

# In pre-scan reset:
lease_bare=0        # bare / =ref-only forms; --force-if-includes is a documented mitigation
lease_mutable=0     # =ref:<mutable-expect> forms; --force-if-includes is a git-documented no-op

# In --no-* arm (clears both):
if abbrev_match "force-with-lease" "--${x#--no-}" 7; then
  lease_bare=0
  lease_mutable=0
fi

# In * arm:
if is_lease_opt "$x"; then
  if [[ "$x" != *=*:* ]]; then
    lease_bare=1
  elif ! lease_pins_an_immutable_expect "$x"; then
    lease_mutable=1
  fi
fi

# Block decision:
if ((lease_bare && !if_includes)) || ((lease_mutable)); then
  block "push-lease-unsafe" ...
fi

This correctly handles all cases:

• --force-with-lease=main:origin/main --force-if-includes → lease_mutable=1 → blocked ✓
• --force-with-lease --force-if-includes → lease_bare=1, if_includes=1 → allowed ✓
• --force-with-lease=main:origin/main --no-force-with-lease → both cleared → allowed ✓

Fix this →

---

Finding 2 — Low · Design tradeoff still open (Codex P2, b4dc425 review)

{4,64} hex floor admits tag names that git resolves at push time

block-dangerous-git.sh L186

[[ "$expect" =~ ^[0-9a-fA-F]{4,64}$ ]]

A tag named dead (four hex chars) satisfies this pattern. Git resolves --force-with-lease=main:dead against a dead tag, not an object ID, so a background fetch that updates the tag lets the lease pass while clobbering unseen work. The code comment acknowledges that "a hex string cannot name a moving target" — but this is only true when the hex string is unambiguous as an object ID, which git cannot guarantee statically for short prefixes.

The Codex suggestion of requiring the full hash length (40 for SHA-1, 64 for SHA-256) would eliminate the gap entirely but would break the common workflow of copying a 7–12 char git log OID abbreviation. This is a genuine tradeoff. The minimum of 4 in the test (abc is blocked, abc123 is allowed) does not address the Codex P2 concern: abc1 through any 4–39 char hex string is accepted and could be a tag.

Options in order of safety:

1. Require ≥ 40 chars (or 64) — fully safe, breaks common abbreviated OID usage
2. Require ≥ 12 chars — matches GitHub's "safe abbreviation" floor, reduces but doesn't eliminate collision risk
3. Keep 4-char floor with a code comment acknowledging the limitation explicitly

Whatever is chosen, the comment currently says "a hex string cannot name a moving target" which is incorrect as stated — it should qualify this as "a sufficiently long hex string is unlikely to be a valid ref name."

Fix this →

---

Nitpick — Truncated comment at L388–390

block-dangerous-git.sh L388–390

Lines 388–390 have an incomplete sentence — a merge artifact from successive edits:

# The two spellings are INDEPENDENT, not one state. git scopes
# `=<refname>:<expect>` to that ref alone and leaves every other
# updated ref on the bare fallback, so an explicit entry never makes a
# Only a spelling that leases against something MUTABLE is tracked. An

"…so an explicit entry never makes a" is a sentence fragment. The comment should be repaired to a single coherent description of what lease_bare tracks. (After Finding 1 is fixed, this whole block will need to be rewritten anyway to cover lease_bare and lease_mutable.)

---

Nitpick — Block message doesn't match the mutable-explicit case

block-dangerous-git.sh L408

The block message reads "git push --force-with-lease without an expected value…" but lease_bare is now also set for --force-with-lease=refname:<mutable-name>, which does have an explicit value. After Finding 1 is fixed, the block decision will fire separately for bare forms and mutable explicit forms — each should get a message that accurately describes why it was blocked and what the fix is (stating an immutable SHA vs. using --force-if-includes are the two distinct remedies, and only the former applies to the mutable-explicit case).

---

What's clean

All prior findings remain fixed:

• if_includes and lease_bare declared local on L254 ✓
• --no-force-with-lease clears lease_bare in the pre-scan --no-* arm ✓
• --no-force-if-includes clears if_includes symmetrically ✓
• Allow-list contract test for push-lease-unsafe is present ✓
• plugin.json toggle description and block_dangerous_git_allow description updated ✓
• -- operand boundary respected in both scans ✓

[claude]

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

---

Security Review — PR #1275

• Load tools
• Fetch PR diff at HEAD (3b0cad85)
• Read hook file and analyze immutability check logic
• Evaluate open Codex findings (P1 + P2) against current HEAD
• Hunt for additional logic flaws and injection surfaces
• Post findings

---

The hook file at HEAD is identical to b4dc425 (the merge commit added no changes to the guardrails files). Two findings from the prior Codex review remain open and are confirmed below. No additional findings.

---

Finding 1 — IMPORTANT · Confidence: CONFIRMED

--force-if-includes is incorrectly accepted as a mitigation for the mutable-explicit-expect form

block-dangerous-git.sh L406

if ((lease_bare)) && ((!if_includes)); then

lease_bare is set to 1 for any lease form that does not pin an immutable expect — including --force-with-lease=<refname>:<mutable-name>. The post-scan decision then clears the block whenever if_includes=1. This is wrong.

From git-push(1) (--force-if-includes description):

This option is a no-op for requests that do not use the --force-with-lease option, or those that specify --force-with-lease=<refname>:<expect>.

So for the command:

git push --force-with-lease=refs/heads/main:refs/remotes/origin/main --force-if-includes origin HEAD:main

The pre-scan sets lease_bare=1 (mutable expect: refs/remotes/origin/main has non-hex chars → lease_pins_an_immutable_expect returns 1 → lease_bare=1) and if_includes=1. The decision at L406 therefore evaluates 1 && 0 = false and does not block. Git, however, runs --force-if-includes as a no-op and uses refs/remotes/origin/main as the lease expectation — which a background fetch can update before the push. The push clobbers work the pusher never saw.

The root issue is that lease_bare collapses two distinct cases into one state variable:

• No-expect forms (bare, =<refname>): --force-if-includes is git's documented mitigation → the bypass is correct here.
• Mutable-explicit-expect forms (=<refname>:<movable-name>): --force-if-includes is a documented no-op → the bypass is incorrect here.

Fix: Track these two sub-cases separately. Block the mutable-explicit-expect form unconditionally (without consulting if_includes); allow if_includes to suppress only the no-expect form. Concretely, replace lease_bare with two flags — lease_noexpect (bare and =<refname>) and lease_mutable_expect (=<refname>:<movable>) — and adjust the decision:

# in the pre-scan:
if is_lease_opt "$x"; then
  if [[ "$x" != *=*:* ]]; then
    lease_noexpect=1       # bare or =<refname>: no expect stated
  elif ! lease_pins_an_immutable_expect "$x"; then
    lease_mutable_expect=1 # =<refname>:<movable>: --force-if-includes is a no-op
  fi
fi
# (the --no-* arm clears both)

# post-scan decision:
if ((lease_mutable_expect)); then
  block "push-lease-unsafe" ...
fi
if ((lease_noexpect)) && ((!if_includes)); then
  block "push-lease-unsafe" ...
fi

Fix this →

---

Finding 2 — SUGGESTION · Confidence: PLAUSIBLE

4–39 hex character <expect> values are classified as immutable but could be movable refnames

block-dangerous-git.sh L186

[[ "$expect" =~ ^[0-9a-fA-F]{4,64}$ ]]

The 4-char floor is set on the basis that git accepts any unambiguous abbreviated SHA, and a hex string cannot name a moving target. But this conflates two things: a string that looks like a hex abbreviation and a string that git treats as one. Refnames may consist entirely of hex characters — a tag named dead or a branch named cafe1234 is legal git. For such a repo:

git push --force-with-lease=refs/heads/main:dead origin HEAD:main

The hook treats dead as an immutable 4-char object ID → lease_pins_an_immutable_expect returns 0 → lease_bare stays 0 → not blocked. Git, however, resolves dead using its disambiguation rules — and if dead is a refname in the repository, it may use the ref (which a background fetch can move). The concrete attack vector: a repo with a tag dead pointing at a remote commit; after a background fetch moves the tag to a newer commit; the lease passes while overwriting work the pusher never saw.

The correct floor is the full hash length for the repository's object format (40 for SHA-1, 64 for SHA-256). Only a full object ID is guaranteed unambiguous. The comment already acknowledges abbreviated hex (git resolves any unambiguous object-id prefix) but the boundary condition is that ambiguity depends on what refnames exist in the repo — a static check cannot determine that.

Fix this →

---

All prior findings remain fixed

• --no-force-if-includes bypass (e0bfe61) — correct: the --no-* arm clears if_includes symmetrically with dry.
• --no-force-with-lease bypass (f345a1b) — correct: lease_bare=0 is cleared in --no-*; the ((k++)); continue prevents re-match in *).
• Mixed bare + explicit lease (8fb92af) — correct: lease_bare tracks only the bare/unsafe form; explicit-only pushes don't set it.
• if_includes missing from local (e0bfe61) — fixed, declared on L254 alongside all other per-push scan variables.
• Allow-list contract test for push-lease-unsafe — two tests present (positive case + cross-token negative guard).
• Input handling, allowed(), -- boundary, is_lease_opt, jq extraction — all clean.

[claude]

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

---

Code Review — PR #1275

• Read changed files at HEAD (56e31c3)
• Evaluate open Codex P1 (--force-if-includes + mutable :<expect> bypass)
• Evaluate open Codex P2 (short hex ambiguity in lease_pins_an_immutable_expect)
• Check for any new issues introduced by the merge
• Post findings

---

The merge commit (56e31c3) only pulled in 2221a4e (source-control babysit guard fix), which has no contact with the guardrails hook. No issues from the merge itself.

Both open Codex findings are valid and confirmed by reading the current hook and test files.

---

Finding 1 — --force-if-includes incorrectly rescues mutable :<expect> leases (medium severity)

block-dangerous-git.sh L406 · block-dangerous-git.test.sh L63

The block decision is:

if ((lease_bare)) && ((!if_includes)); then

lease_bare is set to 1 by both the truly-bare forms (--force-with-lease, --force-with-lease=<refname>) and the mutable-expect forms (--force-with-lease=<refname>:<mutable>). The condition then allows any lease_bare=1 push to be rescued by if_includes=1. But git-push(1) is explicit:

This option is a no-op unless --force-with-lease is used without specifying the details of an expected value.

"Details of an expected value" means any :<expect> suffix — mutable or not. So --force-if-includes is a no-op alongside --force-with-lease=main:refs/remotes/origin/main. The hook treats that combination as safe; git does not apply the mitigation, so the push can clobber unseen work.

Trace for git push --force-with-lease=main:origin/main --force-if-includes:

1. is_lease_opt("--force-with-lease=main:origin/main") → TRUE
2. lease_pins_an_immutable_expect → expect="origin/main", fails hex check → FALSE
3. ! lease_pins_an_immutable_expect → TRUE → lease_bare=1
4. if_includes=1
5. Decision: ((1)) && ((!1)) = FALSE → not blocked (exit 0)

Git, however, treats the --force-if-includes as a no-op and pushes without the check — exactly the unsafe behavior the PR was written to block.

Confirmed by test line 63: the label reads "mitigated, allowed" with expected exit 0. This test encodes the current (incorrect) behavior; it should expect exit 2.

Fix — split lease_bare into two variables:

# truly bare (no :<expect>): --force-if-includes IS the mitigation
lease_bare_rescuable=0
# mutable-expect (=<ref>:<movable>): --force-if-includes is a no-op alongside :<expect>
lease_mutable_expect=0

In the *) arm (L398):

if is_lease_opt "$x"; then
  if [[ "$x" != *=*:* ]]; then
    lease_bare_rescuable=1           # no :<expect> at all
  elif ! lease_pins_an_immutable_expect "$x"; then
    lease_mutable_expect=1           # :<expect> present but movable
  fi
fi

In the --no-* arm (L368), clear both:

abbrev_match "force-with-lease" "--${x#--no-}" 7 && {
  lease_bare_rescuable=0
  lease_mutable_expect=0
}

Decision (replacing L406):

if { ((lease_bare_rescuable)) && ((!if_includes)); } || ((lease_mutable_expect)); then
  block "push-lease-unsafe" ...
fi

And flip test line 63 from 0 to 2:

run "git push lease pinned to a movable name + --force-if-includes (if_includes is no-op alongside :<expect>, blocked)" \
  "git push --force-with-lease=main:origin/main --force-if-includes origin main" 2

Fix this →

---

Finding 2 — lease_pins_an_immutable_expect misclassifies short hex ref names as object IDs (low severity)

block-dangerous-git.sh L186

[[ "$expect" =~ ^[0-9a-fA-F]{4,64}$ ]]

This accepts any 4–64 hex character string as "immutable." But gitrevisions(7) makes no such guarantee for abbreviated hashes: git's disambiguation order checks refs before abbreviated hashes. A tag or branch named dead, cafe, babe, or any 4–39 pure-hex string is therefore resolved as the ref, not as an abbreviated hash — and that ref can be updated by a background fetch.

Concrete scenario (Codex-verified on git 2.43.0):

1. Repo has a tag dead pointing to commit A
2. origin updates its main to B (commit not yet fetched)
3. A background fetch runs: dead tag is not updated, but origin/main is now at B
4. git push --force-with-lease=refs/heads/main:dead origin HEAD:main runs
5. Hook: expect="dead", matches ^[0-9a-fA-F]{4,64}$ → treated as pinned → exit 0
6. Git: resolves dead as the dead tag (pointing to A), which equals the current local refs/heads/main → lease passes, B overwritten

The comment at L178–179 ("Abbreviated hex is accepted because git resolves any unambiguous object-id prefix, and a hex string cannot name a moving target") is incorrect: a hex string can name a moving target when a ref with that name exists.

Fix options (tradeoffs):

Option	Safety	UX impact
Require full 40 (SHA-1) or 64 (SHA-256) char hex only	Closes the gap completely	Users with abbreviated hashes in their lease strings must switch to full hashes or add --force-if-includes
Accept 7–39 chars (above core.abbrev default)	Closes the most likely collision surface	Refs named exactly 7–39 hex chars remain ambiguous
Document the known limitation	No code change	Gap persists with a clear note

The most conservative safe fix is to require the full hash:

[[ "$expect" =~ ^[0-9a-fA-F]{40}$ ]] || [[ "$expect" =~ ^[0-9a-fA-F]{64}$ ]]

This would also require updating tests that use short hex expects (e.g. abc123 at line 38, abc1234 at line 44, 56, 57, 62) — those would become blocked unless paired with --force-if-includes, which is git's own documented recommendation for uncertain expectations.

Fix this →

---

Everything else looks correct

All prior round findings remain addressed at HEAD:

• The --no-force-with-lease reset (lease_bare=0, L368) is correctly placed inside --no-* arm with continue, preventing re-match in *).
• The --no-force-if-includes reset (if_includes=0, L366) is symmetric with the set at L386.
• The -- operand boundary breaks the pre-scan (L358) and the main scan (L415), so refspec operands cannot set or clear any flag.
• lease_bare and if_includes are both declared local at L254.
• The local declaration lists all per-push scan variables, consistent with prior round fixes.
• --force-with-lease=main: (empty expect, ref-must-not-exist assertion) correctly passes through [[ -z "$expect" ]] && return 0 at L185 — still allowed ✓

[claude]

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

---

Code Review — PR #1275

• Read changed files at HEAD (4f05933)
• Verify the Codex P1 finding (--force-if-includes + mutable :<expect> bypass) is resolved
• Verify the Codex P2 findings are resolved or documented
• Check for new issues introduced in the latest commit
• Post findings

---

The latest commit (4f05933) closes all four findings that were open at 56e31c3. Here is the full verification.

---

Codex P1 (IMPORTANT) — --force-if-includes + mutable :<expect> bypass ✅ Fixed

The lease_bare variable has been split into two: lease_tracking (bare / =<refname> — no explicit expect; --force-if-includes is git's documented mitigation) and lease_movable (=<refname>:<movable> — --force-if-includes is a git-documented no-op alongside an explicit :<expect>).

block-dangerous-git.sh L455–464

if ((lease_movable)); then
  block "push-lease-unsafe" \
    "BLOCKED: git push --force-with-lease=<refname>:<expect> whose <expect> is a name git resolves at push time..."
fi
if ((lease_tracking)) && ((!if_includes)); then
  block "push-lease-unsafe" \
    "BLOCKED: git push --force-with-lease without an expected value..."
fi

lease_movable is blocked unconditionally — if_includes=1 has no bearing on it. The previously-failing command git push --force-with-lease=main:origin/main --force-if-includes now correctly exits 2, covered by test line 72. The previously-correct case git push --force-with-lease --force-if-includes still exits 0 (test line 45).

---

Codex P2 — Short hex floor matches movable refnames ✅ Fixed

block-dangerous-git.sh L197

[[ "$expect" =~ ^([0-9a-fA-F]{40}|[0-9a-fA-F]{64})$ ]]

The old {4,64} floor that allowed dead (a valid 4-hex tag name) to be treated as immutable is replaced by an exact match for full-width object IDs only: 40 hex chars for SHA-1, 64 for SHA-256. The comment in lease_expect_is_immutable (L186–192) accurately explains that gitrevisions resolves a word as a ref before trying it as an abbreviated OID, and that only a full-width ID is unambiguously an object ID. Tests line 69–71 cover abbreviated (7-char), 4-hex tag-shape, and 39-char (one short of full-width) expectations, all correctly blocked.

---

Codex P2 — Honor first explicit lease for a repeated ref ✅ Fixed

block-dangerous-git.sh L209–212

The new lease_ref_claimed function tracks which refnames have already been processed. When a per-ref lease entry is encountered, its refname is checked against lease_seen; if already present, the entry is skipped entirely — mirroring git's apply_cas() first-match behavior. Four tests cover the key cases:

Command	Expected	Test line
pinned first, movable second (git uses first)	allowed	78
movable first, pinned second (git uses first — movable)	blocked	79
no-expect first, pinned second (first is tracking-based)	blocked	80
no-expect first, pinned second, --force-if-includes	allowed	81
different refs, one pinned one movable	blocked	82

The --no-force-with-lease reset clears lease_seen along with both flags, so a restated lease after negation starts a fresh first-match window — test line 62 covers this.

---

Codex P2 — CHANGELOG and plugin.json documentation ✅ Fixed

CHANGELOG.md lines 22–28 now explicitly calls out the movable =<refname>:<expect> form as blocked, explains why --force-if-includes is a no-op there (citing git-push(1)), and names the passing forms (full-width OID or empty expect). The prior text that implied every =<refname>:<expect> passed is replaced.

plugin.json L44 reads: "push --force-with-lease leasing against a value git resolves at push time — either no expected value, or an expectation that is not a full-length object id". This accurately describes both blocked sub-cases. The version bump to 0.16.0 is appropriate.

---

Logic correctness on the new lease_ref_claimed sentinel

The sentinel [[ $'\n'"$seen" == *$'\n'"$ref"$'\n'* ]] is correct for the append-with-newline accumulation pattern (lease_seen="$lease_seen$lease_ref"$'\n'): the leading $'\n' anchors the first entry, and each subsequent entry is surrounded by newlines. Refnames cannot contain newlines (gitcli forbids it), so the delimiter is collision-safe.

---

All prior findings remain fixed

• --no-force-if-includes resets the mitigation flag (e0bfe61) ✓
• --no-force-with-lease clears lease_tracking, lease_movable, AND lease_seen (f345a1b) ✓
• if_includes and lease_tracking/lease_movable are all local on L280 ✓
• Allow-list contract test for push-lease-unsafe (positive + cross-token negative guard) ✓
• -- operand boundary respected in both scans ✓
• Input handling, allowed() comma-anchor, is_lease_opt abbreviation matching — all clean ✓

---

No further issues found

The implementation is complete and correct. The dual-flag design (lease_tracking + lease_movable), the lease_ref_claimed first-match guard, the full-width-only OID floor, and the --no-force-with-lease full-reset together close every known bypass surface in the lease family.

[claude]

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

---

Security Review — PR #1275

• Load tools
• Fetch PR diff and read changed files at HEAD (4f05933)
• Analyze hook logic — the "fail closed / first-match" changes in the new commit
• Check for open Codex P1/P2 findings
• Post findings

---

No new security issues found

4f05933 closes all three open findings from the prior security reviews. Full verification below.

---

Finding (P1 from b4dc425 round) — --force-if-includes rescues mutable :<expect> — ✅ Fixed

block-dangerous-git.sh L379–L464

lease_bare (which collapsed both unsafe spellings into one state) is replaced by two independent flags:

• lease_tracking — set for bare --force-with-lease and --force-with-lease=<refname> (no : suffix); --force-if-includes is git's documented mitigation for this form → only blocked when if_includes=0
• lease_movable — set for --force-with-lease=<refname>:<expect> where <expect> fails lease_expect_is_immutable; git-push(1) explicitly declares --force-if-includes a no-op alongside :<expect> → blocked unconditionally, if_includes is never consulted

The bypass command from the P1 finding (git push --force-with-lease=main:origin/main --force-if-includes) now correctly sets lease_movable=1 and if_includes=1 and fires the unconditional block. Test line 72 locks this in (exit 2). ✓

---

Finding (P2 from b4dc425 round) — short hex classified as immutable — ✅ Fixed

block-dangerous-git.sh L194–L198

The renamed lease_expect_is_immutable now requires exactly 40 or 64 hex characters:

[[ "$expect" =~ ^([0-9a-fA-F]{40}|[0-9a-fA-F]{64})$ ]]

The prior {4,64} floor admitted tags and branches whose names happen to be pure hex (e.g. a tag named dead). The new floor is the full SHA-1 or SHA-256 width, where git's disambiguation rules cannot prefer a refname over an object ID — the only unambiguous case. Tests L69–71 cover dead (4 hex), an arbitrary abbreviation, and a 39-char (one short of full width), all exit 2. ✓

---

Finding (P2 from 56e31c3 round) — first-match for repeated ref — ✅ Fixed

block-dangerous-git.sh L200–L212 and L431–L445

The new lease_ref_claimed function tracks which literal refnames have already been encountered in lease_seen (newline-delimited). When the same ref appears again, the later entry is skipped — matching git's apply_cas() first-match behavior. The newline delimiter is safe for this purpose: git's check-ref-format forbids control characters (ASCII ≤ 0x1f, including \n) in refnames, so no valid refname can break the boundary.

Key flows traced against the new code:

Command	Verdict	Trace
--force-with-lease=main:$SHA1_OID --force-with-lease=main:origin/main	allowed (exit 0)	First entry: immutable → lease_movable stays 0, main claimed. Second entry: main already claimed → skipped entirely
--force-with-lease=main:origin/main --force-with-lease=main:$SHA1_OID	blocked (exit 2)	First entry: movable → lease_movable=1, main claimed. Second entry: skipped. Decision fires on lease_movable=1
--force-with-lease=main:$SHA1_OID --force-with-lease=other:origin/other	blocked (exit 2)	main is pinned (immutable); other is distinct — not claimed → lease_movable=1

Tests L78–82 cover all four cases including the cross-ref scenario. ✓

---

Prior rounds all clean at HEAD

• --no-force-with-lease pre-scan arm clears lease_tracking, lease_movable, and lease_seen atomically — preserves if_includes; the ((k++)); continue prevents re-match as the positive option. ✓
• --no-force-if-includes pre-scan arm clears if_includes — does not clear lease state. ✓
• lease_tracking, lease_movable, if_includes all declared local on L279–280. ✓
• -- operand boundary breaks the pre-scan (L386) — a literal --force-if-includes refspec after -- cannot set if_includes. ✓
• Allow-list contract test for push-lease-unsafe is present (positive + cross-token negative guard). ✓
• Input handling, allowed() comma-anchor, jq -r extraction — unchanged and clean. ✓
• CHANGELOG, README, and plugin.json description updated to the narrowed contract (full-width-only object IDs, first-match semantics documented). ✓

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f05933cc9

ℹ️ 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 36s —— View job

---

Code Review — PR #1275

• Read the diff and changed files at HEAD (62a8ded)
• Review the hash-width probe (08a17a5 — accept only the repo's own hash width)
• Review the full hook logic for correctness and security
• Verify prior findings remain addressed
• Post findings

---

The implementation is correct and all prior findings (code review rounds through 4f05933, security rounds through the same, and the Codex P2 from the 56e31c3 round about per-ref first-match) remain addressed. The 08a17a5 commit correctly closes the final open Codex finding about wrong-width hash names resolving as movable refs.

One low-severity finding on the new code; everything else is clean.

---

Finding — _repo_oid_width cache is never populated (low severity)

block-dangerous-git.sh L201–224

The comment at L201–203 says the width probe is "resolved at most once per hook run." In practice it runs once per lease entry with a non-empty, all-hex <expect> value.

# lease_expect_is_immutable:
local width
width=$(repo_oid_width)      # ← command substitution: forks a subshell

$(repo_oid_width) runs repo_oid_width in a subshell. Inside that subshell, _repo_oid_width is still "" (inherited from the parent), so the if [[ -z "$_repo_oid_width" ]] guard fires and git rev-parse --show-object-format runs — then sets _repo_oid_width=40 in the subshell copy of the variable. When the subshell exits the assignment is discarded and the parent's _repo_oid_width remains "". The next call to lease_expect_is_immutable repeats the cycle.

This is not a correctness bug — git rev-parse --show-object-format is a pure local query (reads .git/config) that returns the same answer every time, completes in a few milliseconds, and fails closed on error. For a typical push with one or two lease entries this runs at most twice. The only real defect is the misleading comment.

Fix — call repo_oid_width in the current shell so the global actually caches, then read _repo_oid_width directly:

lease_expect_is_immutable() {
  local expect="$1"
  [[ -z "$expect" ]] && return 0
  [[ "$expect" =~ ^[0-9a-fA-F]+$ ]] || return 1
  repo_oid_width > /dev/null   # runs in the current shell, populates _repo_oid_width
  ((_repo_oid_width)) && ((${#expect} == _repo_oid_width))
}

Redirecting stdout discards the printf output since _repo_oid_width is now read directly. The correctness of the two conditions is identical to the current code.

Fix this →

---

Nitpick — CHANGELOG sentence fragment

CHANGELOG.md L38

  an ordinary ref git resolves at push time, so it moves like any other name.
  git scopes
  a pin to its own ref, so a bare fallback alongside a pinned entry still governs

"git scopes" at L38 is an isolated line ending — a carry-over from the prior version of this paragraph where it was grammatically attached. The sentence reads fine in rendered markdown (single newlines collapse to spaces), but looks like a fragment in raw text view. Worth reflowing to …like any other name. git scopes a pin to its own ref,… on one paragraph-line.

---

All prior findings remain addressed at HEAD

• --no-force-if-includes bypass (e0bfe61) — pre-scan --no-* arm clears if_includes symmetrically with dry. ✓
• --no-force-with-lease bypass (f345a1b) — --no-* arm clears lease_tracking, lease_movable, lease_seen atomically; ((k++)); continue prevents re-match in *). ✓
• lease_bare / --force-if-includes on mutable explicit expect (4f05933) — split into lease_tracking (no-op mitigation for bare/=ref; --force-if-includes rescues) and lease_movable (blocked unconditionally; git docs say --force-if-includes is a no-op alongside :<expect>). ✓
• Short hex classified as immutable (4f05933) — lease_expect_is_immutable now requires the full hash width. ✓
• Wrong-width hash classified as immutable (08a17a5) — repo_oid_width reads the local repo's own format and accepts only its width; 64-hex in a SHA-1 repo and 40-hex in a SHA-256 repo are correctly blocked. ✓
• Per-ref first-match (4f05933) — lease_ref_claimed tracks claimed refnames and skips later entries for already-claimed refs. ✓
• All variables (if_includes, lease_tracking, lease_movable, lease_seen, lease_ref, lease_expect) declared local on L307–308. ✓
• Allow-list contract test for push-lease-unsafe (positive + cross-token negative guard) present. ✓
• -- operand boundary breaks both scans. ✓
• SHA-1 / SHA-256 / non-repo fixture tests all present and differentiated correctly. ✓

[claude]

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

---

Security Review — PR #1275

• Fetch PR diff and read changed files
• Analyze hook logic for security vulnerabilities
• Check for injection surfaces and argument-parsing bypasses
• Post findings

---

Finding — SUGGESTION · Confidence: PLAUSIBLE

repo_oid_width() reads the hook's CWD, not the repository the push will run in — creates a cross-format bypass

block-dangerous-git.sh L201–L215

The code comment states: "Width of the repository the push will run in." The implementation reads:

case "$(git rev-parse --show-object-format 2>/dev/null)" in
sha1) _repo_oid_width=40 ;;
sha256) _repo_oid_width=64 ;;

git rev-parse runs in the hook's own process, querying the repository at the hook's CWD. The hook's CWD is the Claude Code project root — but the push command may execute in a different repository via GIT_DIR, env -C, or a compound cd && git push. In all three cases the hook reads the wrong width:

Attack form	Effect
GIT_DIR=/sha256-repo git push --force-with-lease=main:<40hex>	hook::git_resolve_index skips GIT_DIR=/sha256-repo as an env-prefix assignment; hook reads its own (SHA-1) CWD; 40-char expect passes as a full OID
env -C /sha256-repo git push --force-with-lease=main:<40hex>	-C is recognized and skipped by the resolver (line 694), so git push is identified; but width probe still uses the hook's CWD, not the -C target
cd /sha256-repo && git push --force-with-lease=main:<40hex>	Each shell segment is analyzed separately; check_segment for the push uses the hook's CWD

Exploit scenario (requires preconditions):

1. Hook CWD is a SHA-1 repository (width = 40)
2. A SHA-256 repository exists on the filesystem with a 40-hex tag (e.g. deadbeef... × 10 chars) that points to an old remote commit
3. A push command is issued via one of the three forms above, targeting that SHA-256 repo, with --force-with-lease=refs/heads/main:<40hex-tag-name>
4. lease_expect_is_immutable returns true (40 chars = SHA-1 width); lease_movable stays 0; push is allowed
5. git resolves <40hex-tag-name> as a ref in the SHA-256 repo, the lease passes against the tag's pointee, and unseen remote work is clobbered

This is a targeted, non-trivial bypass that requires write access to the local filesystem and knowledge of both repository formats. For opportunistic use it is unrealistic; as a prompt-injection attack vector against an agent it is plausible if the attacker can pre-stage the SHA-256 fixture.

Fix: In lease_expect_is_immutable, query the width from the SAME directory git will use rather than the hook's ambient CWD. The hook already resolves env-prefix GIT_DIR values and env -C chdir targets during argv parsing — surface those to repo_oid_width() so it reads the correct repository. Alternatively, document and accept the limitation explicitly in the comment (replacing "the repository the push will run in" with the accurate scope).

Fix this →

---

Everything else reviewed and clean

The new 08a17a5 commit (repo_oid_width width gating) is the only substantive security surface added since the prior review. All previously-confirmed findings remain fixed:

• --no-force-if-includes bypass (e0bfe61) — --no-* arm resets if_includes symmetrically; ((k++)); continue prevents re-match in *). ✓
• --no-force-with-lease bypass (f345a1b) — --no-* arm clears lease_tracking, lease_movable, and lease_seen; continue prevents the word from re-matching as the positive option. ✓
• --force-if-includes as no-op alongside :<expect> (4f05933) — lease_movable is blocked unconditionally; if_includes only disarms lease_tracking. ✓
• First-match per-ref tracking (4f05933) — lease_ref_claimed correctly uses a $'\n'-delimited sentinel; git refnames cannot contain newlines, so the delimiter is collision-safe. ✓
• repo_oid_width union-of-widths (08a17a5) — width is now queried from the local repository rather than accepting either 40 or 64 unconditionally; no user-controlled input reaches the git rev-parse call. ✓
• -- operand boundary — pre-scan breaks at --; a literal --force-if-includes refspec after -- neither sets if_includes nor disarms the lease check. ✓
• Allow-list injection — allowed() comma-anchors the token on both sides: ,push-lease-unsafeXXX, cannot match ,push-lease-unsafe,. ✓
• Input handling — command extracted with printf '%s' "$INPUT" | jq -r '…'; no shell-expansion surface. ✓

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 62a8ded13b

ℹ️ 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 2m 39s —— View job

---

Code Review — PR #1275

• Read changed files at HEAD (6265943)
• Identify what changed since the last review (62a8ded)
• Review the docs commit and remaining open findings
• Post findings

---

6265943 is a documentation-only commit: it corrects the comment above repo_oid_width() in block-dangerous-git.sh. No logic changed; the analysis is brief.

---

What changed

block-dangerous-git.sh L201–212

The prior text claimed the probe returns "Width of the repository the push will run in." That was inaccurate: git rev-parse --show-object-format runs in the hook's own process, querying the hook's CWD — not the directory the push command will execute in. The corrected text reads "Width of the repository at the HOOK'S OWN working directory" and adds a Known gap paragraph:

# Known gap: a compound `cd <elsewhere> && git push …` pushes from a directory
# this probe never sees, so a hex expectation is judged against the wrong
# repository when the two hash formats differ. Resolving the cd target would
# mean evaluating arbitrary shell word expansion, which this guard deliberately
# does not do (static matching over the literal command string only). The
# residual case needs a SHA-256 repository, a lease pinned to a full-width hex
# word that is also a ref name there, and a compound cd into it.

This is the correct resolution of the SUGGESTION raised in the previous security review and the Codex P2 from the 62a8ded round. The gap is genuine but narrow (SHA-256 repo + hex-named ref + cd && compound command), and closing it would require arbitrary shell word expansion — which the guard explicitly avoids. Documenting it is the right call.

---

Two items from the 62a8ded round that remain open

Nitpick — CHANGELOG sentence fragment at L38

CHANGELOG.md L38

  …so it moves like any other name.
  git scopes
  a pin to its own ref, so a bare fallback alongside a pinned entry…

"git scopes" sits on its own line as a sentence fragment, a carry-over from earlier edits to this paragraph. Single newlines collapse in rendered markdown, so this is cosmetic, but it looks wrong in raw text view. A one-line reflow would close it.

Fix this →

Low severity — _repo_oid_width cache never populates across calls

block-dangerous-git.sh L231–233

local width
width=$(repo_oid_width)
((width)) && ((${#expect} == width))

$(repo_oid_width) runs repo_oid_width in a subshell. The _repo_oid_width=40 assignment inside the function is local to that subshell and discarded when it exits; the parent's _repo_oid_width stays "". Each subsequent call to lease_expect_is_immutable with a hex expect re-forks the subshell and re-runs git rev-parse --show-object-format. Not a correctness bug — the query is idempotent — but repo_oid_width was written to cache and the cache never fires.

Fix: call in the current shell and read _repo_oid_width directly:

repo_oid_width > /dev/null   # populates _repo_oid_width in this shell
((_repo_oid_width)) && ((${#expect} == _repo_oid_width))

Both this finding and the CHANGELOG fragment were raised against 62a8ded; neither was addressed in 6265943. They are minor enough to leave for a follow-up, but calling them out here so they are not lost.

Fix this →

---

Everything else at HEAD is clean

All findings from prior rounds remain addressed:

• --no-force-if-includes bypass (e0bfe61) ✓
• --no-force-with-lease bypass (f345a1b) ✓
• --force-if-includes as no-op alongside mutable :<expect> (4f05933) ✓
• Short/wrong-width hex classified as immutable (4f05933, 08a17a5) ✓
• Per-ref first-match semantics (4f05933) ✓
• All scoped local variables, -- operand boundary, allow-list comma-anchor, input handling — all unchanged and clean ✓

The docs commit is correct and complete. The repo_oid_width CWD scope is now accurately described, and the known gap is documented with enough context for a future reader to understand the deliberate design tradeoff.

[claude]

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

---

Security Review — PR #1275

• Load tools
• Fetch PR diff and read changed files at HEAD (6265943)
• Analyze for open security findings from prior rounds
• Hunt for new injection surfaces and logic flaws
• Post findings

---

No new security issues found

The latest commit (6265943) is a documentation-only change — it rewrites the comment block above repo_oid_width() to accurately name the hook's own working directory as the probe target and adds a "Known gap" paragraph for the cd <elsewhere> && git push cross-format case. No functional code changed.

---

Status of prior open finding (SUGGESTION from 62a8ded round)

block-dangerous-git.sh L201–212

The prior SUGGESTION finding — repo_oid_width() reads the hook's CWD, not the repository the push will run in, creating a cross-format misclassification via cd <sha256-repo> && git push --force-with-lease=main:<40-hex-ref> — is now documented as a known limitation rather than fixed:

# Known gap: a compound `cd <elsewhere> && git push …` pushes from a directory
# this probe never sees, so a hex expectation is judged against the wrong
# repository when the two hash formats differ. Resolving the cd target would
# mean evaluating arbitrary shell word expansion, which this guard deliberately
# does not do (static matching over the literal command string only). The
# residual case needs a SHA-256 repository, a lease pinned to a full-width hex
# word that is also a ref name there, and a compound cd into it.

This is the correct disposition: fixing it would require evaluating shell word expansion (cd <path> extraction), which the guard explicitly avoids. The gap requires a SHA-256 repository with a 40-hex refname and a compound cd form — a very specific and contrived precondition. The comment accurately describes the scope and residual case; the finding remains at SUGGESTION level and is now self-documenting.

---

All prior IMPORTANT/CONFIRMED findings remain fixed

• --no-force-if-includes bypass (e0bfe61) — --no-* arm clears if_includes symmetrically with dry. ✓
• --no-force-with-lease bypass (f345a1b) — --no-* arm clears all three lease state variables atomically; ((k++)); continue prevents re-match. ✓
• --force-if-includes treated as mitigation for =<ref>:<movable> (4f05933) — lease_movable is blocked unconditionally, without consulting if_includes. ✓
• Short hex classified as immutable (4f05933) — lease_expect_is_immutable now requires exactly 40 or 64 hex chars. ✓
• Wrong-width hash accepted (08a17a5) — width is read from the local repository; a 64-hex name in a SHA-1 repo (((64 == 40)) → false) and a 40-hex name in a SHA-256 repo are both blocked. ✓
• First-match per-ref (4f05933) — lease_ref_claimed prevents a later entry for an already-claimed ref from overriding the first. ✓
• -- operand boundary — pre-scan breaks at --; post--- words cannot set any flag. ✓
• Allow-list injection — comma-anchored token match prevents prefix injection. ✓
• Input handling — printf '%s' "$INPUT" | jq -r '…' with no shell-expansion surface. ✓

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6265943e15

ℹ️ 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 encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30178756065
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":1789,"total_cost_usd":0}

Re-running the job, or pushing a new commit, will retry the review.

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30178756074
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":2097,"total_cost_usd":0}

Re-running the job, or pushing a new commit, will retry the review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20ec3bebbb

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

[chatgpt-codex-connector]

Replay wrapper directory changes in the hash probe

When Git is invoked through env -C <repo>, this collects only options following the git word, so repo_oid_width probes the hook's working directory rather than <repo>. I checked env --help, which defines -C, --chdir=DIR as changing the command's working directory; from a SHA-1 checkout, env -C <sha256-repo> git push --force-with-lease=main:<40-hex> ... made this hook return 0 even though Git runs in the SHA-256 repository, where that 40-hex spelling can resolve as a movable ref. Preserve recognized wrapper working-directory changes when constructing the probe so the unsafe lease cannot bypass the guard.

Useful? React with 👍 / 👎.

[kyle-sexton]

DISCHARGED on main by PR #2100 (merge commit 1cbb4bc9). Verified independently — I did not write that fix, and I built my own reproduction rather than accept the PR body's wrapper matrix.

Every case run against BOTH trees: PRE = 1cbb4bc9^, POST = origin/main (50e7a33b). Exit 2 = BLOCKED, 0 = ALLOWED. Real SHA-1 and SHA-256 fixture repositories (41- and 65-character OIDs confirmed).

The filed form:

env -C <sha256-repo> git push --force-with-lease=main:<40-hex>
      PRE=ALLOWED   POST=BLOCKED

Controls — unchanged by the fix, which is what makes the rest worth anything:

no wrapper, 40-hex in sha1 repo   (legit OID)   PRE=ALLOWED  POST=ALLOWED
no wrapper, 40-hex in sha256 repo (wrong width) PRE=BLOCKED  POST=BLOCKED
no wrapper, 64-hex in sha256 repo (legit OID)   PRE=ALLOWED  POST=ALLOWED
plain --force                                   PRE=BLOCKED  POST=BLOCKED

Wrapper forms beyond those the PR reported — all fixed:

env --chdir=<dir>              PRE=ALLOWED  POST=BLOCKED
env --chdir <dir>              PRE=ALLOWED  POST=BLOCKED
env -C<dir>   (bundled)        PRE=ALLOWED  POST=BLOCKED
nice env -C <dir>  (nested)    PRE=ALLOWED  POST=BLOCKED
env -C <dir> env -C <dir>      PRE=ALLOWED  POST=BLOCKED
env -u FOO -C <dir>            PRE=ALLOWED  POST=BLOCKED
git -C <dir>  (git's own)      PRE=BLOCKED  POST=BLOCKED
env -C <dir> git -C <same>     PRE=BLOCKED  POST=BLOCKED

Reverse direction — the fix relocates the probe rather than just blocking more:

env -C <sha1> from sha256 cwd, 64-hex   PRE=ALLOWED  POST=BLOCKED
env -C <sha1> from sha256 cwd, 40-hex   PRE=BLOCKED  POST=ALLOWED   (a genuine OID there)

That last row is the one I would have missed if I had only tried to break the guard. A lease that really is an object id in the repository git actually runs in is correctly allowed again.

One case looked like a surviving bypass and was not. env FOO=1 -C <dir> git push … is ALLOWED on both trees. Before reporting it, I checked what GNU env does with an option that follows a NAME=VALUE assignment:

$ env FOO=1 -C target pwd
env: '-C': No such file or directory     (rc=127)

env (GNU coreutils) 8.32 stops option parsing at the first assignment, so -C becomes the command name and nothing executes — git never runs and no push happens. Allowing an inert command is correct, not a hole. Recording it because the form looks live, and the next person to try it deserves the answer without re-deriving it.

[kyle-sexton]

Correcting my own comment above: I resolved this thread and should not have. Reopened, and tracked at #2124.

My verification ran 16 cases and all 16 behaved. That was not enough. The session that wrote the fix then handed me a list of forms neither of us had tried, and several of them bypass the guard on origin/main today.

Every "ALLOWED" below was additionally confirmed to actually execute git in the SHA-256 repository, by substituting rev-parse --show-toplevel for the push. That check is what separates a hole from an inert command, and it is what I used above to correctly discard env FOO=1 -C.

1. The payload's cwd is never read — and this one needs no wrapper at all.

payload .cwd = sha256, hook process cwd = sha1     ->  ALLOWED   <-- bypass
payload .cwd = sha256, hook process cwd = sha256   ->  BLOCKED   (control)
payload .cwd = sha1,   hook process cwd = sha1     ->  ALLOWED   (control, correct)

repo_oid_width probes the hook process's working directory. The sibling guard in this same plugin does not:

block-noncanonical-commit.sh:118  HOOK_CWD=$(... jq -r '.cwd // empty' ...)
block-noncanonical-commit.sh:883  HOOK_EFFECTIVE_BASE="${HOOK_CWD:-${CLAUDE_PROJECT_DIR:-.}}"
block-dangerous-git.sh            (no match for cwd)

2. env -S / --split-string splices the chdir past the parser — squarely inside this finding's scope, since it is env performing a -C:

env -S '-C <sha256> git push --force-with-lease=main:<40-hex> origin main'   ALLOWED (executes)
env --split-string='-C <sha256> git push --force-with-lease=... '            ALLOWED (executes)

3. A shell cd reaches the same harm by another route — arguably outside the wrapper-replay design, but recorded because #1 already means the no-cd case is unsound:

sh -c "cd <sha256> && git push --force-with-lease=main:<40-hex> origin main"  ALLOWED (executes)
(cd <sha256> && git push --force-with-lease=main:<40-hex> origin main)        ALLOWED (executes)

What the fix genuinely did buy, and why it should not be reverted: timeout 60 env -C, nohup env -C, command env -C, bash -c 'env -C …' all BLOCK, as do all sixteen round-one cases. xargs -I{} env -C reads ALLOWED but did not execute in the harness, so I am not counting it.

On my own error. Sixteen passing cases and four controls felt like enough, and the wrong lesson to draw is "run more cases." What actually caught this was someone who knew the code handing over the forms they had not tried — the negative space of their own testing. My round one inherited the blind spot of the fix it was checking, which is precisely what an independent verifier is supposed to not do. Resolving on it was the mistake; a finding this class should have stayed open until an attack round found nothing new, not until my first round found nothing.

[kyle-sexton]

DO NOT RESOLVE THIS THREAD. The bypass is live on origin/main today — shipped code, no wrapper needed, fails open. Tracked at #2124; it should be reopened again if anything clears it before that lands.

Flagging this explicitly because two PRs on this sweep (#2100 and #2115) have merged with their review threads resolved by an actor that neither session working them can account for. resolvedBy is a single shared account so it cannot discriminate, and GraphQL exposes no resolvedAt. Until that is understood, an open thread is not a reliable hold, so the hold is stated here in the text as well.