Sharpen the safety-kit README fail-closed and scope wording - #454
Merged
Conversation
…losed Copilot review of the #452 promotion (#453): - The intro umbrella conflated branch-rule protection with local verification hooks; name both categories (bypasses a branch rule OR skips local verification) so it distinguishes rather than conflates. - The fail-closed description said only "when the rules cannot be read", but the hook also fails closed on a protected-default branch when the checkout's origin cannot be resolved to query them at all. Broaden both spots (What It Installs + Scope) to "when its rules cannot be determined". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Docs-only PR that refines the agent safety-kit README wording to more precisely describe (1) branch-rule bypass protection and (2) the fail-closed behavior when branch rules cannot be determined (including when origin cannot be resolved).
Changes:
- Sharpens the intro sentence to distinguish branch-rule bypass from verification-skipping behavior.
- Expands the fail-closed description to include cases where branch rules cannot be determined due to API reachability or inability to resolve
origin. - Updates the Scope and Limits bullet to use the broader "cannot be determined" wording.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/README.md:9
- This bullet describes the second class as "bypasses a repository protection", but the same paragraph also includes explicit bypass flags like
git commit/push --no-verifythat are not repository protections. Align the wording with the intro so it covers both branch-rule bypass and verification-skipping flags.
- **`hooks/gh-write-guard.py`** - a PreToolUse hook that denies two classes of dangerous action. First, the GitHub **write** footguns behind the cross-repo comment incident: a state-changing `gh` call whose output is discarded, a GraphQL mutation passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is outside the checkout's `origin`. Second, a **git operation that bypasses a repository protection**. The branch-rule cases - a direct push to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked - are judged against the branch's **live** rules, so a code-style `develop` is denied while a config-style `develop` is allowed with no per-repo configuration. A push to a protected-default branch fails closed when its rules cannot be determined - the API is unreachable, or the checkout's origin cannot be resolved to query them. The explicit-bypass flags (`gh pr merge --admin`, `git commit`/`git push --no-verify`) are denied **unconditionally**: the flag is the bypass, so no branch query is needed. Reads and everything else pass through. It fires even in autonomous / bypass-permissions sessions, which is how the incident happened.
Copilot review of #454: the intro grouped both override flags under "skips local verification", but the admin-merge flag overrides the server-side merge gate (required reviews/status checks) while the no-verify flag skips the local git hooks - different layers. Reword the intro to "bypasses a branch rule or overrides a required check" and describe each flag at its correct layer, in both the intro and the What It Installs bullet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
host-setup/agent-safety/README.md:3
- The intro calls the second category a "git operation" and says it "overrides a required check", but the examples include
gh pr merge --admin(not a git operation) and--no-verify(skips local hooks, not GitHub required checks). Rewording this sentence to avoid the GitHub-specific "required check" term and to cover both git and GitHub operations would make the scope unambiguous.
Per-machine, user-account-scoped guards against an agent making a mis-targeted GitHub **write** under the maintainer's identity, or a **git operation that bypasses a branch rule or overrides a required check** - a push, force-push, or delete that an active branch rule forbids, or an override flag (`--admin` past the server-side merge gate, `--no-verify` past the local git hooks). Deploy it as the **first thing on any new system** where Claude Code runs with the `gh` credentials logged in (WSL, Linux, macOS, Proxmox, Windows).
host-setup/agent-safety/README.md:9
- This bullet says the second class is a "git operation" but then treats
gh pr merge --adminas part of that class. Sincegh pr mergeis a GitHub CLI action (not a git command), consider broadening the wording here so the category matches the examples.
- **`hooks/gh-write-guard.py`** - a PreToolUse hook that denies two classes of dangerous action. First, the GitHub **write** footguns behind the cross-repo comment incident: a state-changing `gh` call whose output is discarded, a GraphQL mutation passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is outside the checkout's `origin`. Second, a **git operation that bypasses a repository protection**. The branch-rule cases - a direct push to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked - are judged against the branch's **live** rules, so a code-style `develop` is denied while a config-style `develop` is allowed with no per-repo configuration. A push to a protected-default branch fails closed when its rules cannot be determined - the API is unreachable, or the checkout's origin cannot be resolved to query them. The explicit-bypass flags are denied **unconditionally**, since the flag is itself the bypass and needs no branch query: `gh pr merge --admin` overrides the server-side merge gate, and `git commit`/`git push --no-verify` skips the local git hooks. Reads and everything else pass through. It fires even in autonomous / bypass-permissions sessions, which is how the incident happened.
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.
Two accuracy fixes Copilot raised on the #452 promotion (#453):
Docs only - no release.