Agent guardrails: make three standing rules mechanical#46
Merged
Conversation
The changelog rotted for six weeks because the rule lived only in prose. Other rules here have the same shape, so give them teeth. 1. Block irrecoverable git commands (.claude/hooks/block-dangerous-git.sh). CLAUDE.md forbids discarding uncommitted work, but nothing enforced it and a slip cannot be undone. Now blocked: worktree-discarding checkout/restore, reset --hard, clean -f, branch -D, force pushes. Plain push and commit are NOT blocked -- they are recoverable, and already gated by `ask` rules. A hook rather than a permissions.deny rule, because deny matches the command PREFIX: a compound `cd /repo && ...` sails straight past it. The hook greps the whole string. Two subtleties, both found by testing rather than by reasoning. A force flag can precede or follow the refspec, so it cannot be caught by one regex anchored after `push`. And a command that merely mentions a dangerous command is not an attempt to run it -- the first version blocked its own commit, because the commit message documented what it blocks. So heredoc bodies are stripped and a match must sit at a command position. 31 cases pinned in a `--test` self-check, including everything that must NOT block: checkout -b, switch -c, restore --staged, add -f, and prose. 2. PR checkpoint. The over-engineering review is already the routed reviewer but gets skipped exactly when it matters, while wrapping up a PR. A reminder now fires on `gh pr create` / `gh pr merge`. A nudge, not a block: pushing work-in-progress is untouched. 3. Warn when editing on master. Catches the mistake made earlier today, where four files were edited on master before anyone noticed. .claude/ was fully gitignored, so these would have been local-only -- the same durability problem as the rules they replace. The ignore is narrowed to re-include settings.json and hooks/, which must survive a fresh clone. It has to sit after the blanket `*.json` rule, since the last matching rule wins. settings.local.json, CLAUDE.md and plans/ stay local.
Both hooks grepped the raw command string, so a commit whose message merely mentioned `git reset --hard` or `gh pr create` tripped them. The git guard blocked its own commit; the PR checkpoint fired on it. A hook that cries wolf on every commit documenting what it does is a hook you learn to ignore. The fix is shared, so it lives in lib.sh: strip heredoc bodies, then require the match to sit at a command position (start of line, or after a shell separator) rather than mid-sentence. Prose sits mid-line and no longer matches. Two consumers of the same logic is a real seam, not a hypothetical one, which is why this is extracted rather than copied. Each hook keeps a `--test` self-check pinning the regression: 31 cases for the git guard, 9 for the PR checkpoint.
There was a problem hiding this comment.
Pull request overview
This PR makes several “standing rules” for the agent environment enforceable/mechanical by committing shared Claude settings + hooks into the repo and wiring them into .claude/settings.json, rather than leaving them as prose guidance.
Changes:
- Updates
.gitignoreto keep.claude/settings.jsonand.claude/hooks/versioned while leaving other.claude/content local. - Adds a Bash PreToolUse hook to block irrecoverable git commands (e.g.,
reset --hard, destructivecheckout/restore,clean -f,branch -D, force pushes). - Adds reminder/warning hooks for PR completion (
gh pr create/merge) and for editing directly onmaster/main.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Narrows .claude ignore to allow committing shared settings + hooks. |
.claude/settings.json |
Wires the new hook scripts into Claude’s PreToolUse hook pipeline. |
.claude/hooks/block-dangerous-git.sh |
Implements the safety blocklist + self-test cases for dangerous git operations. |
.claude/hooks/lib.sh |
Shared command-matching helpers (heredoc stripping + command-position anchoring). |
.claude/hooks/pr-checkpoint.sh |
Adds PR completion reminder for the over-engineering review step (+ self-test). |
.claude/hooks/warn-editing-master.sh |
Warns when editing on master/main to encourage branching first. |
Comment on lines
+28
to
+31
| # A command position: the start of the string, a new line, or just after a | ||
| # shell separator. Prose mentions ("run `git reset --hard`") sit mid-line and | ||
| # so do not match. | ||
| _AT_CMD='(^|[;&|]|[[:space:]]&&[[:space:]]|[[:space:]]\|\|[[:space:]])[[:space:]]*' |
Comment on lines
+59
to
+60
| check "$cmd" "git clean -f deletes untracked files, which exist in no commit." \ | ||
| 'git[[:space:]]+clean[[:space:]]+-[a-zA-Z]*f' && return 0 |
Comment on lines
+67
to
+72
| if check "$cmd" "" 'git[[:space:]]+push([[:space:]]|$)'; then | ||
| if has_flag "$cmd" '(^|[[:space:]])(--force-with-lease|--force|-f)([[:space:]]|=|$)'; then | ||
| REASON="A force push rewrites published history and can destroy others' commits." | ||
| return 0 | ||
| fi | ||
| fi |
| BRANCH=$(git -C "${CLAUDE_PROJECT_DIR:-.}" branch --show-current 2>/dev/null) | ||
|
|
||
| if [ "$BRANCH" = "master" ] || [ "$BRANCH" = "main" ]; then | ||
| printf '%s' '{"hookSpecificOutput":{"hookEventName":"PreToolUse","additionalContext":"You are editing on '"$BRANCH"'. Unless the user asked for a direct commit, branch first (git switch -c <type>/<name>) -- uncommitted edits carry over cleanly, but only if you branch before the work piles up. Mention it to the user rather than silently continuing."}}' |
- lib.sh: two of the three alternatives in the command-position regex were dead.
The [;&|] class already matches the second character of `&&` and `||`, so the
explicit alternatives never fired. Verified against both forms before cutting.
- block-dangerous-git.sh: two calls used check() as a bare predicate by passing
an empty reason string. runs() already is that predicate; call it directly.
- block-dangerous-git.sh: the matching rationale now lives in lib.sh, where the
logic is. The header points there instead of restating it.
- pr-checkpoint.sh: the reminder string was a variable used once. Inlined.
Net is only -2 lines: the header trim was largely spent documenting a limitation
worth knowing. A dangerous command quoted inside another ("echo 'git reset
--hard'") still sits at a command position and trips the block. Heredocs are
stripped, but general quote awareness means parsing shell in regex, which costs
more complexity than the bug does. The failure direction is safe -- it blocks,
and you run the command yourself.
Both self-checks still green: 31 cases and 9.
`codecov/project` is a required status check, but Codecov only posts a status when it receives an upload. On a PR that changes no package source, every test job is skipped, nothing is uploaded, and the check is ABSENT rather than failing -- so a docs-only or config-only PR can never satisfy it and can never merge. This PR was the first to hit it, and it would have hit every future one. No codecov.yml setting fixes this: that config is only read when Codecov processes an upload, and here Codecov is never invoked at all. An empty upload is Codecov's purpose-built answer -- it tells them there are no coverage-relevant changes and they emit the configured statuses as passing. `--force` skips their changed-file scan, which is safe because the job only runs when the same detect-changes gate that skipped the test jobs has already concluded no package source moved. Coverage enforcement on source PRs is untouched: the moment a package .py file changes, the strategy contains 'test', this job does not run, and real coverage is uploaded against the 90% target as before.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #46 +/- ##
=======================================
Coverage 92.58% 92.58%
=======================================
Files 40 40
Lines 5084 5084
=======================================
Hits 4707 4707
Misses 377 377
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The changelog rotted for six weeks because the rule lived only in prose. Several other rules here have the same shape, so this gives them teeth.
CLAUDE.mdgh pr create/gh pr mergemasterDesign notes
Why a hook and not
permissions.deny. A deny rule matches the command prefix, socd /repo && git reset --hardsails straight past it — and that is exactly how these commands get invoked in practice. The hook greps the whole string.What is deliberately not blocked. Plain
git pushandgit commitare recoverable and already gated byaskrules. Blocking them would only train us to bypass the hook. The block list is limited to what cannot be undone: worktree-discardingcheckout/restore,reset --hard,clean -f,branch -D, force pushes.Both hooks initially fired on their own commit. They grepped the raw command string, and the commit message documented the commands they trap — so the git guard blocked itself and the checkpoint nagged. That is the failure mode where a guardrail becomes noise and gets ignored. Fixed in a shared
lib.sh: heredoc bodies are stripped, and a match must sit at a command position rather than mid-sentence.Testing
Each hook carries a
--testself-check pinning the behaviour, including everything that must not trigger:The must-not-block set is the interesting half:
git checkout -b,git switch -c,git restore --staged,git add -f,git branch -d, plaingit push, and prose mentioning any of the above..gitignore
/.claudewas fully ignored, so these would have been local-only — the same durability problem as the rules they replace. The ignore is narrowed to re-includesettings.jsonandhooks/. It has to sit after the blanket*.jsonrule, since the last matching gitignore rule wins.settings.local.json(personal permissions),.claude/CLAUDE.mdandplans/stay local.Not included
Branch protection on
masterand theattributionsetting were considered and declined. Note thatmastertherefore still accepts direct pushes, which bypass the changelog gate and every test job.