fix(claude-permissions): drop the --force-with-lease deny, enforce it in the hook - #272
Conversation
… in the hook The floor denied every `--force-with-lease` spelling, including the forms that are actually safe. It cannot do otherwise: Claude Code's Bash rules are whole-string globs with `*` as the only metacharacter, and precedence is fixed at deny then ask then allow, so a deny entry cannot carry an allowlist exception. That matters because the forms are not equivalent. git-push(1) warns that `--force-with-lease` and `--force-with-lease=<refname>` lease against the remote-tracking ref and are "trivially defeated if some background process is updating refs in the background", while `--force-with-lease=<refname>:<expect>` states the expectation and is the one form git does not mark experimental. No single glob denies the unsafe spellings and permits the safe one. So the four patterns (two Bash, two PowerShell) come out, and the precise check lives where it can actually be expressed: the `guardrails` plugin's `block-dangerous-git` PreToolUse hook, which parses the argv, blocks any lease that resolves against something movable at push time, honors `--force-if-includes`, and tracks the last-wins negations. Claude Code's own docs name a PreToolUse hook as the mechanism for what globs cannot express. Ordering note: this lands AFTER the hook, not before. Removing the deny on its own would be a net widening — the hook permitted every lease form until melodic-software/claude-code-plugins#1275, so dropping the blunt rule first would have exposed the unsafe spellings with nothing catching them. The README's "Force/destructive spellings stay covered by `deny`, which always wins" is corrected rather than deleted: it still holds for every other force spelling, and the carve-out is recorded beneath it with the reason and both upstream citations.
|
Claude finished @kyle-sexton's task in 1m 59s —— View job Code Review
The reasoning here is technically sound and the documentation is thorough. Two observations worth considering: 🟡 Nit — Consumer prerequisite is implicit, not stated The carve-out section explains why the deny rules were removed and what the hook does, but it doesn't state that the Suggest one sentence along the lines of: "Consumers of this component must have the 🟡 Nit — The Everything else looks correct. The core argument — glob precedence is fixed at deny→ask→allow with no exception mechanism, so the per-form distinction The merge-ordering dependency on plugins#1275 is correctly identified and noted in the PR body; the one documentation gap is that this repository's README is the long-lived artifact, and the ordering rationale lives only in the ephemeral PR description. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27c52c034a
ℹ️ 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".
An explicit `--force-with-lease=<refname>:<expect>` is not categorically
safe. A movable name in the `<expect>` slot (`origin/main`, `HEAD`,
`@{u}`) is resolved when the push runs, so a background fetch advances it
first and the lease passes while clobbering unseen work -- the same hole
the bare and `=<refname>` forms have.
Narrow the claim to what git actually guarantees: an object id, or the
empty string asserting the ref must not already exist. This also matches
the hook that now carries the enforcement, which rejects named
expectations.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nZuFpcEQqj9kpVew4imts
|
Claude finished @kyle-sexton's task in 1m 56s —— View job Code Review
Reviewing HEAD Codex P2 — Addressed ✅Commit
This is accurate, matches the hook's enforcement logic ("requires an immutable Prior nit #2 — Addressed ✅The Prior nit #1 — Still open 🟡The hook is described as where the enforcement lives, but it's still framed as an implementation fact rather than a consumer requirement. A consumer who syncs this component without having deployed Suggested addition after the current last sentence of the carve-out section:
JSON — Correct ✅All four Everything is technically accurate. The one remaining item is the nit above — a consumer reading only this README still can't tell that the hook is a hard dependency, not an optional enhancement. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7573687245
ℹ️ 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".
Summary
The permission floor denied every
--force-with-leasespelling — including the forms that are actually safe. This removes those four patterns and moves enforcement to the one place the distinction can be expressed.Why
denycannot do this jobClaude Code's Bash rules are whole-string globs with
*as the only metacharacter, and precedence is fixed (permissions):No negation, no exceptions. So a
denyhere is all-or-nothing.Why that matters — the forms are not equivalent
git-push(1), "A general note on safety":
Only
--force-with-lease=<refname>:<expect>states the expectation, and it is the one form git does not mark experimental. A single glob cannot deny the unsafe spellings and permit the safe one — so the floor denied all of them, which is why a correctly-formed safe push was blocked in practice.What replaces it
The
guardrailsplugin'sblock-dangerous-gitPreToolUse hook, which parses the argv and can therefore make the distinction:=<refname>, or an=<refname>:<expect>whose<expect>is a name likeorigin/main,HEAD, orrefs/remotes/....--force-if-includes(git 2.30+) as git's documented mitigation for the unpinned forms.--[no-]force-with-leaseand--[no-]force-if-includes.Claude Code's own docs name a PreToolUse hook as the mechanism for exactly what globs cannot express.
Ordering — this lands second, deliberately
Removing the deny on its own would have been a net widening. The hook permitted every lease form until melodic-software/claude-code-plugins#1275, so dropping the blunt rule first would have exposed the unsafe spellings with nothing catching them. The hook hardens first; the blunt rule comes out after.
README
"Force/destructive spellings stay covered by
deny, which always wins" is corrected rather than deleted — it still holds for every other force spelling. The carve-out is recorded beneath it with the reason and both upstream citations, so the next reader does not re-add the patterns.Verification
claude-permissions.jsonvalidates; the four removed entries are the only change to it (2 Bash, 2 PowerShell mirrors).Related
No linked issue: this is the second half of the melodic-software/claude-code-plugins#1275 change, not a separately tracked defect.