From 4537d021b374ebdc70a9b2bcb34475924d348a1d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 13 Oct 2026 19:58:38 -0700 Subject: [PATCH 1/3] Delete a Merged Feature Branch Through a pr_review.py cleanup Subcommand (#2970) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Adds a `cleanup` subcommand to `scripts/pr_review.py` that runs the merged-feature-branch verify-then-delete in code, so the delete runs under the host-safety installer's existing `Bash(python3 scripts/pr_review.py:*)` allow rule instead of as a hand-typed delete the write guard or the auto-mode classifier refuses. `drive-pr` step 4 and `merge-and-release` step 8 now call it, and the hand form stays as the fallback for a host without that rule, with its commands reported paste-ready where the seat cannot run them. `python3 scripts/pr_review.py cleanup --repo OWNER/NAME`: - Reads the head branch from the pull request. It takes no branch name. - Refuses (exit 1) unless the pull request is merged, its base is `develop`, its `headRepository.nameWithOwner` equals `--repo`, and the head branch is not `develop`, `main`, or the default branch read live. - Checks the same origin owner `comment` and `reply` check (exit 64). Then it binds the owner to `registry/repos.json` and admits only a registered repository or a fork, using `handoff.py`'s `require_write_scope`, which it imports rather than copies. - Refuses (exit 1) a branch that any ruleset (`rules/branches/{branch}`) or classic protection (`branches/{branch}`, `.protected`) covers. A read that fails or comes back in the wrong shape exits 2, and it never counts as "no rules". - Reads the ref by exact qualified name through GraphQL. A branch that is already gone exits 0. A tip that differs from `headRefOid` exits 1 and names both commits. Otherwise it sends `deleteRef` with the node id that same read returned. - Confirms the delete by reading the ref back. If the delete reported a failure but the ref is gone, it reports done. If the ref is still there, or the read-back does not answer, it exits 2. The delete's output is printed rather than suppressed. - Has no `--dry-run`, because no other subcommand of this script has one. `deleteRef` is the script's fifth mutation document, and the whole-source write test now pins five. The delete carries no expected tip, so this is verify-then-delete, the same gate the hand form uses, rather than a compare-and-swap. ## Tests `tests/test_pr_review.py` has new `CleanupCase` cases. They mock GraphQL and REST and use constructed names only. They cover: - each refusal - an already-gone branch - a moved tip - a read-back that still shows the ref - a read-back that does not answer - a delete that reports a failure, decided by the read-back either way - unread rule, protection, pull request, and tip reads - a branch name that would change a REST path - out-of-scope and registry refusals, an admitted fork, and an unreadable registry `cleanup` is also added to the cross-owner write table, which now also asserts that neither of the new transports is reached. The new subcommand was not run against any live branch. ## Local Review A strict local pass ran for three rounds, and the receipt was recorded at this head. Filed rather than fixed: - #2968: the installer's description of the allow rule now understates what it approves. The host-setup files are outside this lane. - #2969: deleting a branch another open pull request uses closes that pull request. The hand form already had this gap. Declined with evidence: - The concern that a private repository on a plan without rulesets would always exit 2. A read of a private fleet repository's `rules/branches` endpoint returned a JSON list. Left open after the edit budget was spent, class `introduced`: - No test asserts that the failed REST read's stderr and rc reach the output (`cleanup_rest`). Only the diagnostic text is uncovered. The exit 2 itself is covered. Closes #2967 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .agents/skills/drive-pr/SKILL.md | 17 +- .agents/skills/merge-and-release/SKILL.md | 13 +- .../fleet-skills/.source-digests/drive-pr | 2 +- .../.source-digests/merge-and-release | 2 +- .../fleet-skills/skills/drive-pr/SKILL.md | 17 +- .../skills/merge-and-release/SKILL.md | 13 +- .github/skills/drive-pr/SKILL.md | 17 +- .github/skills/merge-and-release/SKILL.md | 13 +- scripts/README.md | 7 +- scripts/pr_review.py | 245 +++++++++++++++- tests/test_pr_review.py | 263 +++++++++++++++++- 11 files changed, 582 insertions(+), 27 deletions(-) diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index be6f4f542..6fbe9062f 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -107,7 +107,22 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. to delete it, which fails when `develop` is already checked out somewhere else, the ordinary case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone, remove the worktree and delete the now-merged local task branch, then verify before deleting the remote - one, which is this skill's own step rather than that one's. Both remote commands resolve `origin`, + one, which is this skill's own step rather than that one's. Delete the remote one with + `python3 scripts/pr_review.py cleanup --repo /`, run from a hub checkout. + It takes the pull request rather than a branch name and does the verify-then-delete below in + code. It refuses unless the pull request merged into `develop` from a head in that same + repository, and refuses a permanent branch or one any ruleset or branch protection covers. It + checks the tip is still at the pull request's `headRefOid` before deleting, and reads the ref + back to confirm. + Exit `0` is done, the branch deleted or already gone. Any other exit stops and reports rather + than deleting by hand: `1` is a refusal, a moved tip among them, `2` a read that did not answer + or a delete it could not confirm, and `64` a target under an owner other than the hub + checkout's own `origin`, or that `origin` unreadable. A registry refusal is a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the hand form below is the fallback, and where this seat cannot + run that either, report its commands to the maintainer, paste-ready with the values substituted. + Both remote commands of the hand form resolve `origin`, so they hold only where the pull request's head branch lives in this repository, which step 1 guarantees by branching here. A pull request opened from a fork follows `upstream-contribution-workflow` instead and neither command applies to it, since `origin` would diff --git a/.agents/skills/merge-and-release/SKILL.md b/.agents/skills/merge-and-release/SKILL.md index ab771a2f8..cd721748a 100644 --- a/.agents/skills/merge-and-release/SKILL.md +++ b/.agents/skills/merge-and-release/SKILL.md @@ -227,7 +227,18 @@ skill covers all of it, scoped down by what the maintainer actually asks for. use `git branch -D -- ""` here, safe only because the GitHub-state check just proved that exact branch finished, the narrow post-squash exception git-commit-conventions describes, never applied to an unverified branch. Then, only when the remote branch still - exists, delete it the same way, `git push origin --delete -- ""`. + exists, delete it with `python3 scripts/pr_review.py cleanup --repo owner/repo`, run + from a hub checkout, `` being the merged pull request the check above returned. It + re-reads that pull request and repeats the merged, base, head-repository, and tip checks in + code, refuses a permanent or protected branch, and confirms the delete by reading the ref + back. Exit `0` is done, and any other exit stops and reports: `1` is a refusal, `2` an + unanswered read or an unconfirmed delete, and `64` a target under an owner other than the hub + checkout's own `origin` or that `origin` unreadable, a registry refusal being a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the fallback is `git push origin --delete -- ""` once the + tip checks above pass, and where this seat cannot run it, report it to the maintainer, + paste-ready with the values substituted. Never `--force-with-lease` here, git-commit-conventions forbids it unconditionally, the GitHub-state check just completed is the verification gate, not a compare-and-swap at delete time. Never apply this sweep to `develop` or `main` diff --git a/.claude-plugin/fleet-skills/.source-digests/drive-pr b/.claude-plugin/fleet-skills/.source-digests/drive-pr index 932f8d99a..f510ab023 100644 --- a/.claude-plugin/fleet-skills/.source-digests/drive-pr +++ b/.claude-plugin/fleet-skills/.source-digests/drive-pr @@ -1 +1 @@ -aeb0b23f1960e8f7 +227f27065490536c diff --git a/.claude-plugin/fleet-skills/.source-digests/merge-and-release b/.claude-plugin/fleet-skills/.source-digests/merge-and-release index cfde66251..8dd862964 100644 --- a/.claude-plugin/fleet-skills/.source-digests/merge-and-release +++ b/.claude-plugin/fleet-skills/.source-digests/merge-and-release @@ -1 +1 @@ -0d6d711570caaa67 +f94d03359a89a1ea diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index be6f4f542..6fbe9062f 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -107,7 +107,22 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. to delete it, which fails when `develop` is already checked out somewhere else, the ordinary case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone, remove the worktree and delete the now-merged local task branch, then verify before deleting the remote - one, which is this skill's own step rather than that one's. Both remote commands resolve `origin`, + one, which is this skill's own step rather than that one's. Delete the remote one with + `python3 scripts/pr_review.py cleanup --repo /`, run from a hub checkout. + It takes the pull request rather than a branch name and does the verify-then-delete below in + code. It refuses unless the pull request merged into `develop` from a head in that same + repository, and refuses a permanent branch or one any ruleset or branch protection covers. It + checks the tip is still at the pull request's `headRefOid` before deleting, and reads the ref + back to confirm. + Exit `0` is done, the branch deleted or already gone. Any other exit stops and reports rather + than deleting by hand: `1` is a refusal, a moved tip among them, `2` a read that did not answer + or a delete it could not confirm, and `64` a target under an owner other than the hub + checkout's own `origin`, or that `origin` unreadable. A registry refusal is a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the hand form below is the fallback, and where this seat cannot + run that either, report its commands to the maintainer, paste-ready with the values substituted. + Both remote commands of the hand form resolve `origin`, so they hold only where the pull request's head branch lives in this repository, which step 1 guarantees by branching here. A pull request opened from a fork follows `upstream-contribution-workflow` instead and neither command applies to it, since `origin` would diff --git a/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md b/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md index ab771a2f8..cd721748a 100644 --- a/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md @@ -227,7 +227,18 @@ skill covers all of it, scoped down by what the maintainer actually asks for. use `git branch -D -- ""` here, safe only because the GitHub-state check just proved that exact branch finished, the narrow post-squash exception git-commit-conventions describes, never applied to an unverified branch. Then, only when the remote branch still - exists, delete it the same way, `git push origin --delete -- ""`. + exists, delete it with `python3 scripts/pr_review.py cleanup --repo owner/repo`, run + from a hub checkout, `` being the merged pull request the check above returned. It + re-reads that pull request and repeats the merged, base, head-repository, and tip checks in + code, refuses a permanent or protected branch, and confirms the delete by reading the ref + back. Exit `0` is done, and any other exit stops and reports: `1` is a refusal, `2` an + unanswered read or an unconfirmed delete, and `64` a target under an owner other than the hub + checkout's own `origin` or that `origin` unreadable, a registry refusal being a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the fallback is `git push origin --delete -- ""` once the + tip checks above pass, and where this seat cannot run it, report it to the maintainer, + paste-ready with the values substituted. Never `--force-with-lease` here, git-commit-conventions forbids it unconditionally, the GitHub-state check just completed is the verification gate, not a compare-and-swap at delete time. Never apply this sweep to `develop` or `main` diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index be6f4f542..6fbe9062f 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -107,7 +107,22 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. to delete it, which fails when `develop` is already checked out somewhere else, the ordinary case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone, remove the worktree and delete the now-merged local task branch, then verify before deleting the remote - one, which is this skill's own step rather than that one's. Both remote commands resolve `origin`, + one, which is this skill's own step rather than that one's. Delete the remote one with + `python3 scripts/pr_review.py cleanup --repo /`, run from a hub checkout. + It takes the pull request rather than a branch name and does the verify-then-delete below in + code. It refuses unless the pull request merged into `develop` from a head in that same + repository, and refuses a permanent branch or one any ruleset or branch protection covers. It + checks the tip is still at the pull request's `headRefOid` before deleting, and reads the ref + back to confirm. + Exit `0` is done, the branch deleted or already gone. Any other exit stops and reports rather + than deleting by hand: `1` is a refusal, a moved tip among them, `2` a read that did not answer + or a delete it could not confirm, and `64` a target under an owner other than the hub + checkout's own `origin`, or that `origin` unreadable. A registry refusal is a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the hand form below is the fallback, and where this seat cannot + run that either, report its commands to the maintainer, paste-ready with the values substituted. + Both remote commands of the hand form resolve `origin`, so they hold only where the pull request's head branch lives in this repository, which step 1 guarantees by branching here. A pull request opened from a fork follows `upstream-contribution-workflow` instead and neither command applies to it, since `origin` would diff --git a/.github/skills/merge-and-release/SKILL.md b/.github/skills/merge-and-release/SKILL.md index ab771a2f8..cd721748a 100644 --- a/.github/skills/merge-and-release/SKILL.md +++ b/.github/skills/merge-and-release/SKILL.md @@ -227,7 +227,18 @@ skill covers all of it, scoped down by what the maintainer actually asks for. use `git branch -D -- ""` here, safe only because the GitHub-state check just proved that exact branch finished, the narrow post-squash exception git-commit-conventions describes, never applied to an unverified branch. Then, only when the remote branch still - exists, delete it the same way, `git push origin --delete -- ""`. + exists, delete it with `python3 scripts/pr_review.py cleanup --repo owner/repo`, run + from a hub checkout, `` being the merged pull request the check above returned. It + re-reads that pull request and repeats the merged, base, head-repository, and tip checks in + code, refuses a permanent or protected branch, and confirms the delete by reading the ref + back. Exit `0` is done, and any other exit stops and reports: `1` is a refusal, `2` an + unanswered read or an unconfirmed delete, and `64` a target under an owner other than the hub + checkout's own `origin` or that `origin` unreadable, a registry refusal being a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the fallback is `git push origin --delete -- ""` once the + tip checks above pass, and where this seat cannot run it, report it to the maintainer, + paste-ready with the values substituted. Never `--force-with-lease` here, git-commit-conventions forbids it unconditionally, the GitHub-state check just completed is the verification gate, not a compare-and-swap at delete time. Never apply this sweep to `develop` or `main` diff --git a/scripts/README.md b/scripts/README.md index dcabc6393..5b3594605 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -164,7 +164,7 @@ A missing required tool prints its platform source and runnable remedy. An `inst ## `pr_review.py` -One compact interface to a pull request's Copilot review loop. `status` prints the digest, and `wait` runs the backoff in-process. `comment` posts a PR-conversation answer, while `reply` answers one thread, or each copy of a finding posted twice, and optionally resolves it. `claims` reads the description against the branch it describes. The runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. +One compact interface to a pull request's Copilot review loop. `status` prints the digest, and `wait` runs the backoff in-process. `comment` posts a PR-conversation answer, while `reply` answers one thread, or each copy of a finding posted twice, and optionally resolves it. `claims` reads the description against the branch it describes. `cleanup` deletes a merged pull request's head branch once it is verified. The runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. ```sh python3 scripts/pr_review.py status 452 --repo ptr727/ProjectTemplate @@ -175,6 +175,7 @@ python3 scripts/pr_review.py comment 452 --repo ptr727/ProjectTemplate \ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ --match "retry count is off by one" --body "Fixed in abc1234: the loop now stops at n." --resolve python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree +python3 scripts/pr_review.py cleanup 452 --repo ptr727/ProjectTemplate ``` **A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round is covered by the recorded local pass the push already owes. That pass lives in the checkout's git directory, where nothing on GitHub can read it, so `attest` publishes it as a comment carrying ``, and only after confirming the checkout is the pushed head, holds no other change, and has a covering pass in `local_review.py status`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from an owner, member, or collaborator, stands as a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head. Where it is attested, `wait` polls the head's checks instead. The poll runs only while the checks can still move the merge, as the `30` paragraph below lists. Once they settle, or the pull request closes, the head ends as covered. Where it is not attested, `wait` exits `49`, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. A default branch the payload does not name reads as a promotion, so an unreadable field costs a Copilot request rather than passing a head on a local pass alone. The attestation conditions above also let one cover a head whose Copilot round states no coverage, has none carried to it, and carries no table. `status` and `wait` then read `review_on_head=yes` with `coverage=local` rather than exiting `45`. Neither reading applies to the head the first round itself landed on, since that head is no fix push after it. The review loop attests every push a recorded pass covers, the opening push included, and the comment outlives the receipt that removing the worktree deletes. A promotion reads the comment only where `pr-review-conduct` Merge Gate item 2 falls back to per-file evidence. @@ -183,7 +184,7 @@ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout **Do not infer one field's spelling from another's.** Several states print upper-case, `merge=` reproduces GitHub's own enum verbatim, and a field can arrive behind a prefix, so a pattern built by analogy with a neighboring field silently never fires. `review_on_head` is the one that catches a watcher, rendering `yes` against `NO`, so a pattern written for `YES` waits out its whole bound over a review that landed. A watcher that cannot fire reads exactly like a review that has not landed, so run the command once, read a real line, and write the pattern against that. Anchor the pattern on what is unique to the field being read, meaning that field's own name with its value, and for a pair nested in a parenthesized group the group's name ahead of it, since values repeat across fields and the groups carry the same inner names as each other. A watcher testing for a bare negative matches a line a covered review wrote, so it reports no review over a review that landed and waits out its whole bound saying so. That one matches on the wrong evidence rather than failing to match, which is worse than the casing trap above, since its answer is the digest's own inverted rather than absent. Give the watcher a branch that exits non-zero where the digest cannot be read at all, or a pattern that never fires and a command that never ran are equally silent. -`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. On an attested held head it exits `30` with `status=CHECKS_PENDING` at the timeout. That is a required check still settling, one running long included, or any check while no required one has posted. It is also a check the base branch's rules require that the head has not registered yet, on a `BLOCKED` merge whose rollup was read whole. It is also no check posted on a `BLOCKED`, `BEHIND`, or `DRAFT` merge, or the merge still `UNKNOWN`. A merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll ahead of all of these. On a `BLOCKED` merge three shapes of required check end the poll at once even while another required check still runs, and the exit is then `44` unless a review verdict outranks it. A failed check ends it on the first read. A status expected and never posted, and a check queued with no runner, end it only once each is past `--check-grace`. A required check merely running long ends nothing. A round requested while that poll runs ends it with `status=PENDING`, so the next `wait` polls for that round. A quota reading outranks that line with its own `46` or `47`, and an answer outside a review with `40`. `--min-rounds N` counts what `status` prints as `rounds=`, every round the reviewer posted on the pull request. That count spans every head and includes refusals, and the wait holds out for a round past the Nth. N is therefore the `rounds=` a `status` read before a re-request. One above it waits for the round after the one the re-request was for, so the round that lands leaves the wait at `30`. Where that round lands during the wait, the `status=PENDING` line names the position it still waits for. A round already there when `wait` began leaves the bare line, since nothing then tells it apart from a correct count. `wait`'s failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one. +`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. On an attested held head it exits `30` with `status=CHECKS_PENDING` at the timeout. That is a required check still settling, one running long included, or any check while no required one has posted. It is also a check the base branch's rules require that the head has not registered yet, on a `BLOCKED` merge whose rollup was read whole. It is also no check posted on a `BLOCKED`, `BEHIND`, or `DRAFT` merge, or the merge still `UNKNOWN`. A merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll ahead of all of these. On a `BLOCKED` merge three shapes of required check end the poll at once even while another required check still runs, and the exit is then `44` unless a review verdict outranks it. A failed check ends it on the first read. A status expected and never posted, and a check queued with no runner, end it only once each is past `--check-grace`. A required check merely running long ends nothing. A round requested while that poll runs ends it with `status=PENDING`, so the next `wait` polls for that round. A quota reading outranks that line with its own `46` or `47`, and an answer outside a review with `40`. `--min-rounds N` counts what `status` prints as `rounds=`, every round the reviewer posted on the pull request. That count spans every head and includes refusals, and the wait holds out for a round past the Nth. N is therefore the `rounds=` a `status` read before a re-request. One above it waits for the round after the one the re-request was for, so the round that lands leaves the wait at `30`. Where that round lands during the wait, the `status=PENDING` line names the position it still waits for. A round already there when `wait` began leaves the bare line, since nothing then tells it apart from a correct count. `wait`'s failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the five mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, `resolveReviewThread`, and `deleteRef`, so a sixth arriving is a write nobody reviewed as one. `wait` exits `64` when the target is out of scope, the same refusal `comment` and `reply` raise, checked before either the auto-request or the backoff loop reaches GitHub. The `reply` paragraph below states what that refusal covers. @@ -238,6 +239,8 @@ Every failure is a stop rather than a fallback, because each alternative closes What this trades away is stated rather than glossed. A mutation spelled as a `gh` command in a shell is read by the `gh-write-guard` hook and one this script performs is not. The script removes that failure at its source instead: every node ID comes from a live query in the same run. The owner check is also enforced in-process. The whole-source test guards against every other state-changing call and counts each reviewed mutation document. +`cleanup` deletes the head branch of a merged pull request, the verify-then-delete `drive-pr` and `merge-and-release` prescribe after a merge into `develop`. It exists because both hand forms fail from an agent seat in auto mode. The `gh-write-guard` hook denies `git push origin --delete` from a primary checkout, and Claude Code's auto-mode classifier refuses the ref API delete as a destructive git operation on some days and allows it on others. A command matching a narrow `permissions.allow` rule is settled before that classifier runs, and the host-safety installer already registers `Bash(python3 scripts/pr_review.py:*)`, so the delete runs here under that rule with no settings change. That rule approves any arguments, so every check lives in the subcommand. It takes the pull request number and `--repo`, never a branch name, and reads the branch from the pull request. It refuses unless the pull request is merged, its base is `develop`, its head repository is `--repo` itself, and the head branch is neither `develop`, `main`, nor the default branch read live. Past the same owner check `comment` and `reply` make, `handoff.py`'s own write-scope check, imported rather than copied, binds the owner to `registry/repos.json` and admits a repository only where it is registered or a fork. A branch any ruleset or branch protection covers is refused, since the hook's per-branch rule check never sees a delete made from inside this script. The ref is read by its exact qualified name, and the delete is sent only where that read's tip is the pull request's `headRefOid`, by the node id the same read returned. The delete carries no expected tip of its own, so this is verify-then-delete rather than a compare-and-swap, the same gate the hand form is. The delete is confirmed by reading the ref back rather than by the mutation's own answer, so a delete that reports a failure and landed still reads as done, and one that reports success and left the ref reads as unconfirmed. Exit `0` is the branch deleted and confirmed gone, or already gone. Exit `1` is a refusal, the registry check's among them, and so is a tip that moved after the merge, which names both commits. Exit `2` is a read that did not answer, the rule read included, which never reads as no rules, or a delete the read back could not confirm. A usage error also exits `2`, argparse's own code. Exit `64` is the owner check `comment` and `reply` make against this checkout's `origin`, which also exits `64` where that `origin` cannot be read, run before the registry and before any read. No `--dry-run` exists, since no other subcommand here has one. + `claims` checks that a description does not contradict its own branch, and it exists for the same reason `sha-pin` now resolves: a reference that points at nothing is a silent failure caught by a reviewer or not at all. Three stale descriptions in one session generated six review findings between them, each a reviewer noticing that the body named a commit or a behavior the branch no longer carried. It reads the commits a body **claims** the branch carries and the `uses:` refs it quotes, confirms the head tree still carries each, and exits `70` where one does not. Prose claims stay **out of scope**, since judging those needs a similarity heuristic, which [`spec/section-model.md`][section-model] rejects for exactly the reason it would fail here. **Scanning for bare SHAs was built first and the corpus rejected it.** Over the 25 most recent merged pull requests it raised four findings, and every one was correct prose: a `develop` commit named as history, a SHA inside a pasted digest, and two commits in `ptr727/Blog` written without a URL. Nothing in the *form* of a bare SHA separates those from a real claim, and separating them by meaning is the heuristic already ruled out above. A path arm was measured on the same corpus and is worse, flagging 54 of 215 backticked candidates, nearly all of them bare basenames, `origin/develop`, and other repositories. What survives is the verb: a commit counts only where the body says it was fixed, landed, shipped, added, introduced, corrected, resolved, carried or amended in it. That alternation raises exactly one reference over the same 25, and that one is true. It is an **inclusion** list, so a phrasing nobody thought of costs a detection rather than producing a finding, which is the direction to be incomplete in, and a claimed SHA still has to carry a digit as a backstop on the list growing later, since `accede` and `defaced` inflect into all-hex English words. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 5110606f2..775c795a0 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -31,6 +31,25 @@ reviewer or not at all, and three stale descriptions in one session generated six review findings between them. Read-only. Exit 0 = every reference resolves, 70 = one does not, 71 = there were references and none could be read, so nothing was decided. + cleanup Delete a merged pull request's head branch, the verify-then-delete `drive-pr` and + `merge-and-release` prescribe, done here so it runs under the one allow rule the + host-safety installer registers for this script rather than as a hand-typed delete. + That rule approves any arguments, so every check lives here. It takes the pull request + number, never a branch name, and reads the branch from the pull request. It refuses + unless the pull request is merged into `develop` from a head in `--repo` itself, and + the head branch is none of `develop`, `main`, or the default branch read live. The + owner is bound to `registry/repos.json`, and the repository must be registered or a + fork, by `handoff.py`'s own write-scope check. It refuses a branch any ruleset or + branch protection covers, since the write guard's per-branch check never sees a delete + made in here. The ref is read exactly, its tip is checked against the pull request's + `headRefOid` before the delete is sent, and the delete is confirmed by reading the ref + back. The delete itself carries no expected tip, so this is verify-then-delete rather + than a compare-and-swap. No `--dry-run` exists, since no other subcommand here has + one. Exit 0 = deleted and confirmed gone, or already gone, 1 = refused, a + moved tip naming both commits among them, 2 = a read did not answer, a partial one + included, or the delete could not be confirmed, which argparse's own usage error + shares, 64 = the write scope could not be established or excludes the target, checked + before the registry and before any read. status One digest line, any unresolved threads, and any suppressed findings. Read-only. Exit 0 = no Copilot review covers the head yet, or every output shape is recognized and full diff coverage is stated for this head, by the round covering it or by the @@ -351,6 +370,7 @@ from typing import NamedTuple import gh_graphql as shared +import handoff # The directory this script sits in, and the anchor every scope answer is read from. # One value serves the owner probe and the refusal naming it, so the two cannot disagree. @@ -999,6 +1019,22 @@ def strip_fences( resolveReviewThread(input:{threadId:$threadId}){ thread{ id isResolved } }} """ +Q_CLEANUP_TARGET = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ + defaultBranchRef{ name } + pullRequest(number:$n){ + merged baseRefName headRefName headRefOid headRepository{ nameWithOwner } } }} +""" +Q_CLEANUP_REF = """ +query($o:String!,$r:String!,$q:String!){ + repository(owner:$o,name:$r){ ref(qualifiedName:$q){ id target{ oid } } }} +""" +M_DELETE_REF = """ +mutation($refId:ID!){ + deleteRef(input:{refId:$refId}){ clientMutationId }} +""" + WORKFLOW_KEYS_UNAVAILABLE = "workflowKeysUnavailable" @@ -2069,23 +2105,28 @@ def reread_json(path: str, jq: str) -> object: None wherever nothing parseable came back, an absence GitHub answered included, so the caller has one value to refuse on. """ + return parsed_json(reread(path, jq)) - def parsed(proc: subprocess.CompletedProcess) -> object: - if proc.returncode != 0: - return None - try: - return json.loads(proc.stdout.strip()) - except json.JSONDecodeError: - return None +def reread(path: str, jq: str) -> subprocess.CompletedProcess: + """The last of `reread_json`'s reads, whole, so a caller can say why nothing parsed.""" read = functools.partial(gh_rest, path, jq) - proc = backoff( + return backoff( read(), read, - lambda p: not answered_absent(p) and parsed(p) is None, + lambda p: not answered_absent(p) and parsed_json(p) is None, delays=READ_RETRY_DELAYS, ) - return parsed(proc) + + +def parsed_json(proc: subprocess.CompletedProcess) -> object: + """The read's JSON, or None where it failed or printed nothing parseable.""" + if proc.returncode != 0: + return None + try: + return json.loads(proc.stdout.strip()) + except json.JSONDecodeError: + return None def tree_at(owner: str, repo: str, commit: str) -> dict[str, tuple[str, str]] | None: @@ -5571,6 +5612,183 @@ def check_claims(owner: str, repo: str, num: int) -> int: return 0 +class CleanupUnread(Exception): + """A read `cleanup` needed did not produce an answer, reported at exit 2.""" + + +def cleanup_read(query: str, **variables: object) -> dict: + """One GraphQL read for `cleanup`, raising rather than ending the run at exit 1. + + `gh_graphql` exits 1 on a failed read, which is `cleanup`'s refusal code, so a read that could + not answer would be reported as a refusal the caller can act on. + """ + try: + return shared.request(query, variables).data + except shared.GraphQLError as exc: + sys.stderr.write(exc.detail) + raise CleanupUnread(str(exc)) from exc + + +def cleanup_rest(path: str, what: str) -> object: + """One REST read for `cleanup`, retried, raising with its stderr where nothing parsed.""" + proc = reread(path, ".") + data = parsed_json(proc) + if data is None: + sys.stderr.write(proc.stderr) + raise CleanupUnread(f"the {what} read failed rc={proc.returncode} or printed no JSON") + return data + + +def cleanup_ref(owner: str, repo: str, branch: str) -> dict | None: + """The branch's ref node, or None where the branch does not exist. + + The read is by exact qualified name, so a branch sharing a suffix never answers for it. The + node id it returns goes straight to the delete, so no caller ever holds one to mistype. + """ + data = cleanup_read(Q_CLEANUP_REF, o=owner, r=repo, q=f"refs/heads/{branch}") + repository = data.get("repository") + if not isinstance(repository, dict): + raise CleanupUnread(f"{owner}/{repo} returned no repository for the ref read") + ref = repository.get("ref") + if ref is None: + return None + oid = (ref.get("target") or {}).get("oid") if isinstance(ref, dict) else None + if not isinstance(ref, dict) or not ref.get("id") or not oid: + raise CleanupUnread(f"the ref read for {branch!r} returned no id and tip: {ref!r}") + return ref + + +def branch_rules(owner: str, repo: str, branch: str) -> list[str]: + """What protects the branch, rulesets and classic protection alike, empty where nothing does. + + The branch travels inside a REST path, so a name that would change the path is not read, and + an unread rule set is never reported as no rules. + """ + if UNSAFE_REF.search(branch) or DOT_SEGMENT.search(branch): + raise CleanupUnread( + f"the head branch {branch!r} holds a character that changes a REST path, so the " + "rules applying to it cannot be read" + ) + rules = cleanup_rest(f"repos/{owner}/{repo}/rules/branches/{branch}", "branch rules") + if not isinstance(rules, list) or not all(isinstance(r, dict) for r in rules): + raise CleanupUnread(f"the branch rules read returned no list of rules: {rules!r}"[:400]) + found = [f"ruleset rule {r.get('type') or 'unnamed'}" for r in rules] + detail = cleanup_rest(f"repos/{owner}/{repo}/branches/{branch}", "branch protection") + protected = detail.get("protected") if isinstance(detail, dict) else None + if not isinstance(protected, bool): + raise CleanupUnread(f"the branch read returned no protected flag: {detail!r}"[:400]) + if protected: + found.append("branch protection") + return found + + +def cleanup(owner: str, repo: str, num: int) -> int: + """Delete the head branch of merged pull request `num` once every check passes. + + Returns 0 where the branch is deleted and the read back confirms it, or is already gone, 1 on + a refusal, 2 where a read did not answer or the delete could not be confirmed, and 64 where + the write scope `comment` and `reply` check excludes the target. Past that check, the owner is + bound to the fleet registry and the repository must be registered or a fork, by `handoff.py`'s + own write-scope check, reused rather than copied so the two cannot drift apart. Its refusal + text is printed as `handoff.py` words it. + """ + ok, why = in_scope(owner) + if not ok: + print(f"status=OUT_OF_SCOPE nothing was deleted: {why}") + return 64 + target = f"{owner}/{repo}" + try: + handoff.require_write_scope(argparse.Namespace(cmd="cleanup", repo=target)) + except handoff.Refusal as exc: + print(f"status=REFUSED nothing was deleted, refused by handoff.py's write scope: {exc}") + return 1 + except handoff.Execution as exc: + print(f"status=UNREAD nothing was deleted: {exc}") + return 2 + try: + return _cleanup(owner, repo, num) + except CleanupUnread as exc: + print(f"status=UNREAD nothing was deleted: {exc}") + return 2 + + +def _cleanup(owner: str, repo: str, num: int) -> int: + """The checks, the delete, and the read back, raising `CleanupUnread` on any unanswered read. + + A branch anything protects is refused whatever that protection would allow, since the write + guard's per-branch rule check never sees a delete made from inside this script. A delete that + reports a failure may still have landed, so the read back decides the outcome either way. + """ + target = f"{owner}/{repo}" + data = cleanup_read(Q_CLEANUP_TARGET, o=owner, r=repo, n=num) + repository = data.get("repository") or {} + pr = repository.get("pullRequest") + default = (repository.get("defaultBranchRef") or {}).get("name") + if not isinstance(pr, dict) or not default: + raise CleanupUnread(f"{target} #{num} returned no pull request or no default branch") + branch = pr.get("headRefName") + head = pr.get("headRefOid") + if not branch or not head: + raise CleanupUnread(f"{target} #{num} returned no head branch or head commit") + head_repo = (pr.get("headRepository") or {}).get("nameWithOwner") or "" + refusals = [] + if pr.get("merged") is not True: + refusals.append("the pull request is not merged") + if pr.get("baseRefName") != "develop": + refusals.append(f"its base is {pr.get('baseRefName')!r}, not 'develop'") + if head_repo.lower() != target.lower(): + refusals.append(f"its head lives in {head_repo or 'no repository'}, not {target}") + if branch in ("develop", "main", default): + refusals.append(f"its head branch {branch!r} is a permanent branch") + if refusals: + print(f"status=REFUSED nothing was deleted from {target} #{num}: {', and '.join(refusals)}") + return 1 + + ref = cleanup_ref(owner, repo, branch) + if ref is None: + print(f"status=ALREADY_GONE branch={branch} is not on {target}, so nothing was deleted") + return 0 + rules = branch_rules(owner, repo, branch) + if rules: + print( + f"status=REFUSED nothing was deleted: {branch!r} on {target} is covered by " + f"{', '.join(rules)}, so its delete is the maintainer's call" + ) + return 1 + tip = ref["target"]["oid"] + if tip != head: + print( + f"status=TIP_MOVED nothing was deleted: {branch!r} on {target} is at {tip}, and " + f"#{num} merged {head}, so something reached the branch after the merge or the name " + "was reused" + ) + return 1 + + print(f"deleting: {branch} on {target} at {tip}") + try: + response = shared.request(M_DELETE_REF, {"refId": ref["id"]}).data + print(f" response: {json.dumps(response)[:400]}") + except shared.GraphQLError as exc: + print(f" the delete reported a failure: {exc}") + sys.stderr.write(exc.detail) + try: + after = cleanup_ref(owner, repo, branch) + except CleanupUnread as exc: + print( + f"status=DELETE_NOT_CONFIRMED the delete of {branch!r} on {target} was sent, and " + f"reading the ref back did not answer: {exc}" + ) + return 2 + if after is not None: + print( + f"status=DELETE_NOT_CONFIRMED {branch!r} on {target} still reads at " + f"{after['target']['oid']} after the delete" + ) + return 2 + print(f"status=DELETED branch={branch} oid={tip}, confirmed by reading the ref back") + return 0 + + def utf8_console() -> None: """Write the console streams as UTF-8, whatever the host code page is. @@ -5586,7 +5804,9 @@ def utf8_console() -> None: def main(argv: list[str] | None = None) -> int: utf8_console() ap = argparse.ArgumentParser() - ap.add_argument("cmd", choices=["attest", "claims", "comment", "status", "reply", "wait"]) + ap.add_argument( + "cmd", choices=["attest", "claims", "cleanup", "comment", "status", "reply", "wait"] + ) ap.add_argument("number", type=int) # No default, because the wrong repository is the failure this argument has actually had. # A default names one repository, and every run from elsewhere silently reads that one. @@ -5738,6 +5958,9 @@ def main(argv: list[str] | None = None) -> int: if a.cmd == "claims": return check_claims(owner, repo, a.number) + if a.cmd == "cleanup": + return cleanup(owner, repo, a.number) + if a.cmd == "attest": return attest(owner, repo, a.number, a.checkout or ".") diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index df3dba642..f5512b451 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -9665,17 +9665,19 @@ def test_the_runbook_names_partial_coverage_as_a_state_that_blocks_a_merge(self) self.assertIn("A round reporting partial coverage of the diff blocks the merge", text) self.assertIn("a coverage statement that does not reach this head", text) - def test_the_only_writes_are_the_four_named_here(self) -> None: - """Every write this script makes is one of four, and each arrived as a reviewed change. + def test_the_only_writes_are_the_five_named_here(self) -> None: + """Every write this script makes is one of five, and each arrived as a reviewed change. The read subcommands are the bulk of it and a mutation reaching them is a digest that writes, so the whole-source guard stays and is narrowed to the documents the script actually needs rather than dropped when the first of them arrived. `requestReviews` closes the gap where `wait` only polled and never asked. `addComment` owns body-only - finding responses. The reply and resolve pair owns inline threads. `union:true` only + finding responses. The reply and resolve pair owns inline threads. `deleteRef` is + `cleanup`'s, by an id read in the same run, so no REST verb enters for it. `union:true` only adds to the request set, so it cannot drop a requested human reviewer. `docs/pr-reviewer-reference.md` keeps the `union:false` clear-and-recover form as a - manual operation, the runbook carrying no mutation of its own. + manual operation, the runbook carrying no mutation of its own. A sixth document arriving + is a write nobody reviewed as one rather than a style drift. """ source = (REPO / "scripts" / "pr_review.py").read_text(encoding="utf-8") for verb in ( @@ -9702,8 +9704,8 @@ def test_the_only_writes_are_the_four_named_here(self) -> None: "reviewer is pending, which no script can decide", ) # `mutation(` opens a document, so the count is the number of documents. - # A fifth arriving is a write nobody reviewed as one rather than a style drift. - self.assertEqual(4, source.count("mutation(")) + self.assertEqual(5, source.count("mutation(")) + self.assertIn("deleteRef", source) self.assertIn("addComment", source) self.assertIn("addPullRequestReviewThreadReply", source) self.assertIn("resolveReviewThread", source) @@ -10236,6 +10238,7 @@ def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anythin ], "wait": ["wait", "7", "--repo", "someone-else/r"], "attest": ["attest", "7", "--repo", "someone-else/r"], + "cleanup": ["cleanup", "7", "--repo", "someone-else/r"], } # The parser's remaining `cmd` choices, none of which write. @@ -10264,12 +10267,16 @@ def test_each_write_command_refuses_before_reaching_either_transport(self) -> No mock.patch.object(pr_review, "origin_owner", return_value="acme"), mock.patch.object(pr_review, "gql", side_effect=boom) as gql, mock.patch.object(pr_review, "gh_graphql", side_effect=boom) as gh_graphql, + mock.patch.object(pr_review.shared, "request", side_effect=boom) as request, + mock.patch.object(pr_review, "gh_rest", side_effect=boom) as rest, ): code = pr_review.main(argv) self.assertEqual(64, code) self.assertIn("status=OUT_OF_SCOPE", out.getvalue()) gql.assert_not_called() gh_graphql.assert_not_called() + request.assert_not_called() + rest.assert_not_called() class TestRequiredContexts(unittest.TestCase): @@ -10806,5 +10813,249 @@ def test_the_dedupe_falls_back_to_the_app_slug_for_a_null_workflow_run(self) -> self.assertEqual([("lint", "FAILURE"), ("lint", "SUCCESS")], kept) +CLEANUP_HEAD = "a" * 40 +CLEANUP_BRANCH = "feature/example-topic" + + +def cleanup_pr(**overrides: object) -> dict: + """A merged pull request into `develop` from a head in o/r, every field overridable.""" + pr: dict[str, object] = { + "merged": True, + "baseRefName": "develop", + "headRefName": CLEANUP_BRANCH, + "headRefOid": CLEANUP_HEAD, + "headRepository": {"nameWithOwner": "o/r"}, + } + pr.update(overrides) + return pr + + +class CleanupCase(unittest.TestCase): + """Drive `cleanup` against crafted reads, so no case reaches GitHub or deletes anything.""" + + def setUp(self) -> None: + self.out = self.enterContext(contextlib.redirect_stdout(io.StringIO())) + self.enterContext(contextlib.redirect_stderr(io.StringIO())) + self.enterContext(mock.patch.object(pr_review, "origin_owner", return_value="o")) + self.enterContext(mock.patch.object(pr_review, "READ_RETRY_DELAYS", ())) + self.enterContext( + mock.patch.object(pr_review.handoff, "registry", return_value=("o", {"r"})) + ) + self.docs: list[tuple[str, dict]] = [] + self.rest: list[str] = [] + + def wire( + self, + pr: dict | None = None, + default: str = "main", + refs: list[object] | None = None, + rules: object = (), + protected: object = False, + delete_fails: bool = False, + ) -> None: + """Answer each document in turn, `refs` holding one answer per ref read in order. + + A ref answer is a tip, None for no such branch, or an exception the read raises. + """ + pending = list(refs if refs is not None else [CLEANUP_HEAD, None]) + + def fake(query: str, variables: dict, **_: object) -> pr_review.shared.Response: + self.docs.append((query, variables)) + if "pullRequest(number" in query: + repository = {"defaultBranchRef": {"name": default}, "pullRequest": pr} + return pr_review.shared.Response({"repository": repository}, []) + if "deleteRef" in query: + if delete_fails: + raise pr_review.shared.GraphQLError("gh graphql reported errors") + return pr_review.shared.Response({"deleteRef": {"clientMutationId": None}}, []) + if "qualifiedName" in query: + answer = pending.pop(0) + if isinstance(answer, Exception): + raise answer + ref = None if answer is None else {"id": "REF_read", "target": {"oid": answer}} + return pr_review.shared.Response({"repository": {"ref": ref}}, []) + raise AssertionError(f"unexpected document: {query[:60]}") + + def rest( + path: str, jq: str | None = None, raw: bool = False + ) -> subprocess.CompletedProcess: + self.rest.append(path) + if "/rules/branches/" in path: + body = rules + else: + body = {"name": CLEANUP_BRANCH, "protected": protected} + if isinstance(body, Exception): + return subprocess.CompletedProcess([], 1, "", "gh: Not Found (HTTP 404)") + text = json.dumps(list(body) if isinstance(body, tuple) else body) + return subprocess.CompletedProcess([], 0, text, "") + + self.enterContext(mock.patch.object(pr_review.shared, "request", side_effect=fake)) + self.enterContext(mock.patch.object(pr_review, "gh_rest", side_effect=rest)) + + def run_cleanup(self, repo: str = "o/r") -> int: + return pr_review.main(["cleanup", "7", "--repo", repo]) + + def deleted(self) -> bool: + return any("deleteRef" in q for q, _ in self.docs) + + +class TestCleanupDeletesOnlyAVerifiedMergedBranch(CleanupCase): + def test_a_matching_tip_is_deleted_by_the_id_just_read_and_confirmed_gone(self) -> None: + """Two exact reads by qualified name, the tip before the delete and the read back after.""" + self.wire(cleanup_pr()) + self.assertEqual(0, self.run_cleanup()) + delete = next(v for q, v in self.docs if "deleteRef" in q) + self.assertEqual({"refId": "REF_read"}, delete) + reads = [v for q, v in self.docs if "qualifiedName" in q] + self.assertEqual([f"refs/heads/{CLEANUP_BRANCH}"] * 2, [v["q"] for v in reads]) + self.assertIn("status=DELETED", self.out.getvalue()) + + def test_a_branch_already_gone_is_done_and_deletes_nothing(self) -> None: + self.wire(cleanup_pr(), refs=[None]) + self.assertEqual(0, self.run_cleanup()) + self.assertFalse(self.deleted()) + self.assertIn("status=ALREADY_GONE", self.out.getvalue()) + + def test_a_moved_tip_refuses_and_names_both_commits(self) -> None: + moved = "b" * 40 + self.wire(cleanup_pr(), refs=[moved]) + self.assertEqual(1, self.run_cleanup()) + self.assertFalse(self.deleted()) + out = self.out.getvalue() + self.assertIn("status=TIP_MOVED", out) + self.assertIn(moved, out) + self.assertIn(CLEANUP_HEAD, out) + + def test_a_ref_still_there_after_the_delete_is_not_reported_deleted(self) -> None: + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, CLEANUP_HEAD]) + self.assertEqual(2, self.run_cleanup()) + self.assertTrue(self.deleted()) + self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + self.assertNotIn("status=DELETED", self.out.getvalue()) + + def test_a_read_back_that_does_not_answer_is_not_reported_deleted(self) -> None: + unread = pr_review.shared.GraphQLError("gh graphql failed rc=1") + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, unread]) + self.assertEqual(2, self.run_cleanup()) + self.assertTrue(self.deleted()) + self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + + def test_a_delete_reporting_failure_is_decided_by_the_read_back(self) -> None: + """A failed-looking delete may have landed, so the ref read decides rather than the error.""" + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, None], delete_fails=True) + self.assertEqual(0, self.run_cleanup()) + out = self.out.getvalue() + self.assertIn("the delete reported a failure", out) + self.assertIn("status=DELETED", out) + + def test_a_delete_reporting_failure_with_the_ref_still_there_exits_2(self) -> None: + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, CLEANUP_HEAD], delete_fails=True) + self.assertEqual(2, self.run_cleanup()) + self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + + +class TestCleanupRefusesBeforeAnyDelete(CleanupCase): + def assert_refused(self, code: int = 1, status: str = "status=REFUSED") -> None: + self.assertEqual(code, self.run_cleanup()) + self.assertFalse(self.deleted()) + self.assertIn(status, self.out.getvalue()) + + def test_an_unmerged_pull_request_is_refused(self) -> None: + self.wire(cleanup_pr(merged=False)) + self.assert_refused() + + def test_a_base_other_than_develop_is_refused(self) -> None: + self.wire(cleanup_pr(baseRefName="main")) + self.assert_refused() + + def test_a_head_in_another_repository_is_refused(self) -> None: + self.wire(cleanup_pr(headRepository={"nameWithOwner": "o/r-fork"})) + self.assert_refused() + + def test_a_head_repository_gone_is_refused(self) -> None: + self.wire(cleanup_pr(headRepository=None)) + self.assert_refused() + + def test_a_develop_head_is_refused(self) -> None: + self.wire(cleanup_pr(headRefName="develop")) + self.assert_refused() + + def test_a_main_head_is_refused_where_main_is_not_the_default(self) -> None: + self.wire(cleanup_pr(headRefName="main"), default="trunk") + self.assert_refused() + + def test_the_default_branch_read_live_is_refused(self) -> None: + self.wire(cleanup_pr(headRefName="trunk"), default="trunk") + self.assert_refused() + + def test_a_ruleset_on_the_branch_is_refused(self) -> None: + self.wire(cleanup_pr(), rules=[{"type": "deletion"}]) + self.assert_refused() + self.assertIn("deletion", self.out.getvalue()) + + def test_classic_branch_protection_is_refused(self) -> None: + self.wire(cleanup_pr(), protected=True) + self.assert_refused() + + def test_an_unread_rule_set_stops_at_2_rather_than_reading_as_none(self) -> None: + self.wire(cleanup_pr(), rules=RuntimeError("unread")) + self.assert_refused(2, "status=UNREAD") + + def test_a_rule_read_that_is_not_a_list_stops_at_2(self) -> None: + self.wire(cleanup_pr(), rules={"message": "Upgrade to GitHub Pro"}) + self.assert_refused(2, "status=UNREAD") + + def test_an_unread_protection_flag_stops_at_2(self) -> None: + self.wire(cleanup_pr(), protected=None) + self.assert_refused(2, "status=UNREAD") + + def test_a_branch_name_that_changes_the_rest_path_stops_at_2(self) -> None: + self.wire(cleanup_pr(headRefName="feature/a?b"), refs=[CLEANUP_HEAD]) + self.assert_refused(2, "status=UNREAD") + self.assertEqual([], self.rest) + + def test_an_unread_pull_request_stops_at_2(self) -> None: + self.wire(None) + self.assert_refused(2, "status=UNREAD") + + def test_an_unread_tip_stops_at_2(self) -> None: + self.wire(cleanup_pr(), refs=[pr_review.shared.GraphQLError("gh graphql failed rc=1")]) + self.assert_refused(2, "status=UNREAD") + + def test_an_owner_other_than_the_checkout_is_out_of_scope_before_any_read(self) -> None: + self.wire(cleanup_pr()) + self.assertEqual(64, self.run_cleanup(repo="x/r")) + self.assertEqual([], self.docs) + self.assertIn("status=OUT_OF_SCOPE", self.out.getvalue()) + + def test_an_owner_other_than_the_registry_is_refused_before_any_read(self) -> None: + self.wire(cleanup_pr()) + with mock.patch.object(pr_review.handoff, "registry", return_value=("p", {"r"})): + self.assertEqual(1, self.run_cleanup()) + self.assertEqual([], self.docs) + self.assertIn("status=REFUSED", self.out.getvalue()) + + def test_an_unregistered_repository_that_is_not_a_fork_is_refused(self) -> None: + self.wire(cleanup_pr()) + flags = {"hasIssuesEnabled": True, "isFork": False} + with mock.patch.object(pr_review.handoff, "gh_json", return_value=flags): + self.assertEqual(1, self.run_cleanup(repo="o/unlisted")) + self.assertEqual([], self.docs) + + def test_an_unregistered_fork_under_the_owner_is_admitted(self) -> None: + self.wire(cleanup_pr(headRepository={"nameWithOwner": "o/forked"})) + flags = {"hasIssuesEnabled": True, "isFork": True} + with mock.patch.object(pr_review.handoff, "gh_json", return_value=flags): + self.assertEqual(0, self.run_cleanup(repo="o/forked")) + self.assertTrue(self.deleted()) + + def test_an_unreadable_registry_stops_at_2(self) -> None: + self.wire(cleanup_pr()) + failure = pr_review.handoff.Execution("could not read the fleet registry") + with mock.patch.object(pr_review.handoff, "registry", side_effect=failure): + self.assertEqual(2, self.run_cleanup()) + self.assertEqual([], self.docs) + + if __name__ == "__main__": unittest.main(verbosity=2) From e98def1f3f62f240abc47b6ff2f8f5dec3a4962b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 13 Oct 2026 20:44:43 -0700 Subject: [PATCH 2/3] Deny Include Overrides on Commits, Command-Running Subcommands, and Non-Builtins, and Read a Local Branch Name Whole in the Write Guard (#2974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This closes three allow gaps in `host-setup/agent-safety/claude/gh-write-guard.py`. Each fix adds a reading or a deny beside develop's and replaces none of develop's readings. - **An include override on a commit or push (#2958).** An `include.path`/`includeIf` override on `git commit` or `git push` now denies as a hooks-path override. The file it pulls in may set `core.hooksPath` where the guard cannot read it. `-c core.hooksPath=` was already denied the same way. - **An include override on a command that runs other git commands (#2960).** The override reaches every child git command, so on `rebase`, `submodule`, `bisect`, `filter-branch`, `difftool`, or `mergetool` it is unreadable whatever the arguments say. An `eval` argument holding an include key is now read as the command it runs, for the push check. The reading uses the quote mask and skips to each payload's end. It is bounded by the eval budget the wait rule already uses, and an include key met after that budget is spent fails closed. - **A local branch name and a non-builtin command (#2962).** - `_read_branch` keeps a local branch name whole, such as `release/1.0`, and also checks the stripped name develop read. The remote-tracking `@{push}` read is unchanged. - Under an include override, only a command git builds in is read as one no alias can shadow. An `--exec-path`, a `GIT_EXEC_PATH`, or a PATH change can hand any other name to an alias the file defines. That makes a separate `--exec-path` check redundant, so none is kept. - **Same-class reads found in local review.** An include key in a wrapper argument the guard does not read (`bash -c '$1' _ '...'`), or in a wrapper body past the nesting cap, now fails closed. The self-test carries discriminating cases for each one: develop allows them and this branch denies them. A `release/1.0` worktree in the real-repository push cases exercises the branch read. The agent-safety README's requirement 4 and the Claude kit README state the new denies. ## Differential Against Develop's Guard Develop's guard (the merge base, d2ed08c4) and this branch's guard were loaded side by side and run over the same calls. - **Real-repository mode:** 7,214 constructed commands. They cross 15 global-option sets (include, includeIf, `--config-env`, mixed case, `--exec-path` before and after an include, `-C`, `core.hooksPath`, an inline alias) with 35 subcommands. Those cover push heads, commit, listed and unlisted names, non-builtins, every command-running subcommand, and fetch and merge. Each sits in 14 wrappers: plain, `eval` single-quoted, double-quoted, `--`, chained, `bash -c`, `sh -c`, eval inside a wrapper, a wrapper inside eval, `&&`, `;`, a leading `cd`, and `timeout`. The extras are PATH and `GIT_EXEC_PATH` prefixes, rows of 3 to 300 evals, an unread wrapper argument, and wrapper bodies 3 to 7 levels deep. Each command ran in a constructed repository's develop checkout and in worktrees on `feature/example`, `release/1.0`, `x/develop`, and `release/2.0` with a real upstream. They ran under six rule sets, one of which protects only the stripped names `1.0` and `example`. That is 216,420 calls per guard with no current-branch seam. - **Seam mode:** the same corpus under 7 current-branch seams, 5 rule seams, and the real and a pinned command list (504,980 calls). - **Self-test replay:** every `classify` call the branch self-test makes (845). - **Rule 6:** the corpus with a primary-checkout lookup (7,214 calls). **Allow flips (develop denies, this branch allows): zero in every mode.** Every deny flip, by the reason the branch gives, from real-repository mode's 1,354 distinct flipped commands: - 568: an include override on a command git does not build in (`instaweb`, `request-pull`, `mergetool`, `filter-branch`, and `submodule`, a script in git 2.47), plain or inside a now-read `eval`. - 448: an include override on a builtin command-running subcommand (`rebase`, `bisect`, `difftool`), `rebase --continue` included. - 209: an include override on a name git does not list, inside an `eval` this branch now reads. - 65: a bare or `HEAD` push from the `release/1.0` worktree, only under the rule set protecting `release/1.0`. These are the #2962 fix. - 58: an include override on a commit. - 4: an include key in a wrapper body past the nesting cap. - 1: an include key in an unread wrapper argument. - 1: an include key after a spent eval budget (a row of 300 evals). Seam mode adds one more class. An `eval` payload is now read under the pinned command list, which lacks `log` and `fetch`, so it denies there as an unlisted name. Rule 6 shows the same classes. A row of 20,000 `eval x;` before a push classifies in 1.46s on this branch against 1.27s on develop, both deny. ## Known New Over-Denials - Any include override on a commit or push. - An include override on any command git does not build in, even where git would run the real command rather than an alias. - An include override on `rebase`, `submodule`, `bisect`, `filter-branch`, `difftool`, or `mergetool`, whatever its arguments. - An include key in a wrapper's unread argument, or in a body past the nesting cap, even where it sits on a listed command. - After about 256 evals spend the reading budget, any include key later in that text. - An `eval` that is only an argument, as in `echo eval 'git -c include.path=/example/config p'`, is read as one that runs. Narrowing it is filed as #2975. ## Left Open Deliberately - An include or `core.hooksPath` override on a commit inside a quoted `eval` argument still passes the hook-bypass check, which does not read one. That is the #2943 class, and both READMEs now say so rather than claiming every such commit is denied. - A quote-split or positional-argument-split include key, as in `bash -c '$1 $2' _ git '-c include.path=...'`, is the #2959 word-rewrite class. - A config key that runs a command (`core.fsmonitor`, `diff.external`, `core.sshCommand`, `credential.helper`) set inline or by an included file, on any git command, is filed as #2972. It is the wider form #2958's comment raised. - A remote-tracking push target whose remote holds a slash, or whose abbreviation carries `remotes/`, is filed as #2973. ## Local Review Three local strict-review passes ran at the `frontier` tier. None found an allow flip against develop. Rounds one and two found same-class bypasses, an eval-chain false deny, a quadratic eval path after the budget, consolidation points, and doc claims, each fixed in the second and third commits. The deferred findings are the issues above. The third pass raised one sentence-length note on a docstring line, which is declined, since that check is in no gate and the sentence is a defensible form. Two more passes covered the README qualification Copilot asked for. After merge, the host-safety installer payload changes. The installer is not run here, and the maintainer runs it after promotion. Closes #2958 Closes #2960 Closes #2962 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- host-setup/agent-safety/README.md | 24 +- host-setup/agent-safety/claude/README.md | 2 +- .../agent-safety/claude/gh-write-guard.py | 359 ++++++++++++++++-- 3 files changed, 355 insertions(+), 30 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 2cf776784..c9b5fb61d 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -55,7 +55,10 @@ text says, because the harm it covers was never in the text. 4. **A git operation that would only succeed by bypassing an active branch rule is denied**: a direct push to a branch whose rules require a pull request, a force-push where history is protected, a delete where deletion is blocked, or an explicit-bypass flag (`--admin` on a merge, - `--no-verify` or a `-c core.hooksPath=` override on a commit/push). Judge branch-rule cases against that branch's *live* rules, so a + `--no-verify` or a `-c core.hooksPath=` override on a commit/push). An `include.path`/`includeIf` + override on a commit or push is denied as one too. The file it pulls in may set + `core.hooksPath` where the hook cannot read it. These flags are read in a command the hook + parses, a wrapper shell's body among them, and not inside a quoted `eval` argument. Judge branch-rule cases against that branch's *live* rules, so a code-style `develop` denies and a config-style `develop` allows with no per-repo configuration. Each refspec's operation is judged on its own, so a `+src` or a `:dst` refspec forces or deletes only its own branch. A force-push and a delete each write the branch too, so a branch @@ -68,8 +71,16 @@ text says, because the harm it covers was never in the text. that case, a push whose target the hook cannot read at all is denied whatever branch it names. That covers a push in text the hook does not parse and a push carrying a `-c`/`--config-env` override. It covers a git command under an `include.path`/`includeIf` override too, where git - lists no such subcommand, since the included file may define it as a push alias. It also covers - a bare push or a `HEAD`/`@` refspec whose directory or branch may change before it runs. The + lists no such subcommand, since the included file may define it as a push alias. The same holds + where git does not build the subcommand in. An `--exec-path`, a `GIT_EXEC_PATH`, or a PATH + change can leave such a name to an alias. The override is also unreadable on a git command that + runs other git commands (`rebase`, `submodule`, `bisect`, `filter-branch`, `difftool`, + `mergetool`), whatever its arguments say. It reaches every git command that one runs. An `eval` + argument holding an include key is read as the command it runs. It also covers a bare push or a + `HEAD`/`@` refspec whose directory or branch may change before it runs. Such a push's local + branch is checked under its whole name, such as `release/1.0`. It is also checked with its first + segment dropped. A remote-tracking push target such as `origin/feature/x` is checked with its + remote dropped. The workaround is an explicit refspec run as its own command. Every other requirement here favors precision over recall throughout, denying only a positively-identified dangerous shape, since a hook that fails closed on an unrelated resolution failure blocks @@ -464,9 +475,10 @@ opt-in per clone, visible to anyone reading the repo, and bypassable on purpose, rule whose harm is a quality miss rather than a destruction, which the host layer's bar excludes. The hub's own `.husky/pre-push` is the worked example there, refusing a branch push that no recorded local review pass covers, per [`GOVERNANCE.md`][governance] "Verification Discipline". -The two layers meet at requirement 4, which denies `--no-verify` and a `-c core.hooksPath=` -override unconditionally, so a Claude Code session meets a committed hook it -cannot wave through with either while a human keeps the escape hatch. They do not compose into a +The two layers meet at requirement 4, which denies `--no-verify`, a `-c core.hooksPath=` +override, and an include override on a commit or push unconditionally, wherever it reads them. So +a Claude Code session meets a committed hook it +cannot wave through with any of them while a human keeps the escape hatch. They do not compose into a seal, and saying so would be the more comfortable claim rather than the true one. A committed hook is bypassable by construction, since it cannot police its own invocation, and `--no-verify` is the documented route rather than the only one. This requirement diff --git a/host-setup/agent-safety/claude/README.md b/host-setup/agent-safety/claude/README.md index a8069da8e..bd8464d61 100644 --- a/host-setup/agent-safety/claude/README.md +++ b/host-setup/agent-safety/claude/README.md @@ -12,7 +12,7 @@ installation and operational detail. Codex and opencode have no equivalent hook Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): -- **`hooks/gh-write-guard.py`**: a PreToolUse hook that denies four 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 under an owner other than the checkout origin's. Sibling repositories under the same owner are allowed, since the harm this guards is reaching a stranger's repository rather than working across one maintainer's own fleet, and a different owner is allowed only when the maintainer names it in `GH_WRITE_GUARD_ALLOW` (an `owner/repo` list, where `owner/*` grants a whole owner). That variable is read from the environment the session was launched with, which is the one channel an agent cannot set for itself: a hook runs as its own process, so an inline `VAR=x cmd` prefix or an `export` inside a Bash call never reaches it. Second, a **git operation that bypasses a repository protection**. The branch-rule cases (a direct push, a force-push, or a branch delete to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked) are judged per refspec 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, either because the API is unreachable or because the checkout's origin cannot be resolved to query them. A push whose target the hook cannot read, such as one in text it does not parse, one carrying a `-c` config override, or a git command an `include.path`/`includeIf` override may define as a push alias, is denied whatever branch it names. 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, as does a `git -c core.hooksPath=` override on either, which points git at another hooks directory. Third, a **mutating git operation run directly against a primary checkout** (`checkout`/`switch`/`pull`/`reset`/`rebase`/`merge`/`cherry-pick`/`revert`/`restore`/`add`/`commit`/`rm`/`mv`/`apply`/`am`/`push`/`stash` (anything but `list`/`show`)/`clean -f`/`worktree remove -f`), the harm behind a second incident where an agent reused the maintainer's own primary checkout instead of a worktree despite having read the prose rule against it. "Primary" is decided by comparing `git rev-parse --git-dir` against `--git-common-dir`, not a `.git`-is-a-directory guess. `worktree add`/`list`/`prune`, an unforced `worktree remove`, `merge --ff-only`/`pull --ff-only`, and a `checkout `/`switch ` carrying no force flag (bundled or attached forms like `-qf`/`-Bname` recognized, not only an exact token) stay exempt even in a primary checkout, matching the documented base-clone cleanup step the `repo-worktree` skill already carries. A subcommand this rule does not recognize is resolved through a chain of git aliases, inline and persisted, before falling through, so a custom alias cannot hide a denied builtin behind an unrecognized name. A `!`-prefixed shell alias is denied outright rather than interpreted. A directory named with a `..` component is not resolved, so a mutating command naming one is denied. That holds for `-C`, any `cd`, `pushd`, `popd`, or `chdir` word in the command, `--work-tree`, `--git-dir`, and a `GIT_DIR=`/`GIT_WORK_TREE=` prefix. The target directory follows real git's own `-C`/`--work-tree`/`--git-dir` priority, not a last-option-wins scan. A different owner-style grant, `GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT`, follows the same channel as `GH_WRITE_GUARD_ALLOW`. Reads and everything else pass through. Fourth, an **unbounded shell wait**, which the root spec's requirement 7 defines in full rather than this bullet redefining it. In outline: a loop that sleeps, carrying no bound the rule recognizes. It is the shape an agent writes when a brief tells it to poll. It is also the harm behind a third incident, where seven such loops outlived the subagents that wrote them and the run that dispatched those subagents. A shell started by a tool call runs in a session of its own, so the agent could not have reaped it even had it tried. A nested loop is judged on its own terms, and a command that forks anything out of a `timeout`'s reach, by backgrounding it or by `setsid`, is bounded by nothing. A heredoc body is data rather than a command line unless a shell is what it is fed to, so a document quoting the forbidden shape is written rather than denied. It fires even in autonomous / bypass-permissions sessions, which is how the first incident happened. See the root spec's requirements 6 and 7 for the exact rules. +- **`hooks/gh-write-guard.py`**: a PreToolUse hook that denies four 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 under an owner other than the checkout origin's. Sibling repositories under the same owner are allowed, since the harm this guards is reaching a stranger's repository rather than working across one maintainer's own fleet, and a different owner is allowed only when the maintainer names it in `GH_WRITE_GUARD_ALLOW` (an `owner/repo` list, where `owner/*` grants a whole owner). That variable is read from the environment the session was launched with, which is the one channel an agent cannot set for itself: a hook runs as its own process, so an inline `VAR=x cmd` prefix or an `export` inside a Bash call never reaches it. Second, a **git operation that bypasses a repository protection**. The branch-rule cases (a direct push, a force-push, or a branch delete to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked) are judged per refspec 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, either because the API is unreachable or because the checkout's origin cannot be resolved to query them. A push whose target the hook cannot read, such as one in text it does not parse, one carrying a `-c` config override, a git command an `include.path`/`includeIf` override may define as a push alias, or a git command that runs other git commands under one, is denied whatever branch it names. 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, as does a `git -c core.hooksPath=` override on either, which points git at another hooks directory, and an `include.path`/`includeIf` override on either, whose file may do the same. These are read in a command the hook parses, a wrapper shell's body among them, and not inside a quoted `eval` argument. Third, a **mutating git operation run directly against a primary checkout** (`checkout`/`switch`/`pull`/`reset`/`rebase`/`merge`/`cherry-pick`/`revert`/`restore`/`add`/`commit`/`rm`/`mv`/`apply`/`am`/`push`/`stash` (anything but `list`/`show`)/`clean -f`/`worktree remove -f`), the harm behind a second incident where an agent reused the maintainer's own primary checkout instead of a worktree despite having read the prose rule against it. "Primary" is decided by comparing `git rev-parse --git-dir` against `--git-common-dir`, not a `.git`-is-a-directory guess. `worktree add`/`list`/`prune`, an unforced `worktree remove`, `merge --ff-only`/`pull --ff-only`, and a `checkout `/`switch ` carrying no force flag (bundled or attached forms like `-qf`/`-Bname` recognized, not only an exact token) stay exempt even in a primary checkout, matching the documented base-clone cleanup step the `repo-worktree` skill already carries. A subcommand this rule does not recognize is resolved through a chain of git aliases, inline and persisted, before falling through, so a custom alias cannot hide a denied builtin behind an unrecognized name. A `!`-prefixed shell alias is denied outright rather than interpreted. A directory named with a `..` component is not resolved, so a mutating command naming one is denied. That holds for `-C`, any `cd`, `pushd`, `popd`, or `chdir` word in the command, `--work-tree`, `--git-dir`, and a `GIT_DIR=`/`GIT_WORK_TREE=` prefix. The target directory follows real git's own `-C`/`--work-tree`/`--git-dir` priority, not a last-option-wins scan. A different owner-style grant, `GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT`, follows the same channel as `GH_WRITE_GUARD_ALLOW`. Reads and everything else pass through. Fourth, an **unbounded shell wait**, which the root spec's requirement 7 defines in full rather than this bullet redefining it. In outline: a loop that sleeps, carrying no bound the rule recognizes. It is the shape an agent writes when a brief tells it to poll. It is also the harm behind a third incident, where seven such loops outlived the subagents that wrote them and the run that dispatched those subagents. A shell started by a tool call runs in a session of its own, so the agent could not have reaped it even had it tried. A nested loop is judged on its own terms, and a command that forks anything out of a `timeout`'s reach, by backgrounding it or by `setsid`, is bounded by nothing. A heredoc body is data rather than a command line unless a shell is what it is fed to, so a document quoting the forbidden shape is written rather than denied. It fires even in autonomous / bypass-permissions sessions, which is how the first incident happened. See the root spec's requirements 6 and 7 for the exact rules. - **`hooks/tool-containment.py`**: a shell prefix, registered as `CLAUDE_CODE_SHELL_PREFIX` in the `env` block of `settings.json`, that runs every Bash tool call inside a transient systemd user scope carrying `TasksMax`, `MemoryMax`, and `MemorySwapMax=0`, the last so a runaway at the memory ceiling is killed rather than spilled into the host's swap. A runaway fan-out then fails at the ceiling with `fork: Resource temporarily unavailable` instead of growing until the host has to be reset, the harm behind the probe-shim incident in the root spec's requirement 9. The guard above reads the command text, and that incident's recursion lived in a script file the command only named. The ceilings default to 8192 tasks and 25% of RAM. A `claude` session started from a tool call is nested in the session that ran it. Every scope of such a chain also joins one slice, `claude-tree-.slice`. That slice carries a total ceiling for the whole tree, 32768 tasks and 50% of RAM. The root's id reaches nested sessions as `AGENT_CONTAINMENT_ROOT`, which every contained command inherits. The tree's ceiling is a runtime drop-in put on when its slice is first created, and the sweep below reverts it. The maintainer raises any ceiling in the same `env` block. An inline `VAR=x` in a command cannot change one, since the prefix reads it before the command runs. The variables are `AGENT_CONTAINMENT_TASKS_MAX` and `AGENT_CONTAINMENT_MEMORY_MAX` for a scope, and `AGENT_CONTAINMENT_TREE_TASKS_MAX` and `AGENT_CONTAINMENT_TREE_MEMORY_MAX` for the tree. A tree ceiling set below a scope's binds first. Each scope is named `claude-tool---`, so the sweep below can stop one session's tool calls and nothing else. Its description records the pid of the agent that ran it. That record is how the sweep tells a scope whose session has ended from one whose session still runs. The installer sets the prefix only where it can contain, per the Linux bullet under "Scope and Limits", and never over a `CLAUDE_CODE_SHELL_PREFIX` value that names anything else. - **`hooks/stray-process-sweep.py`**: a SessionEnd hook in two phases. First, it stops every `claude-tool--*` scope this session left loaded and reports what each held, since the scope's name proves which session started it. It then stops every scope in this session's tree whose recorded agent is no longer running, and names each one's session. That is how a nested session's scopes end, since stopping the parent's scopes kills the nested `claude` before its own sweep runs. Each stop can end an agent nested deeper, so it judges again, up to four rounds. The root session then stops its tree's slice and reverts the ceiling put on it, once nothing is left in it. Second, it reports, and never ends, the process trees outside those scopes that outlive the session. The guard above stops a leak from being written. This finds the ones already running: the ones started before the guard was installed, and the shapes the guard deliberately does not reach. It reports every descendant of the agent process running outside the agent's own session. It names each subtree root's command and age, and hands over the `kill` line. This phase kills nothing itself. A long build backgrounded on purpose reads identically to a leaked wait from there. What is still wanted on the machine is the maintainer's call. A SessionEnd hook cannot block, and its JSON output is discarded, so stderr on exit 2 is the one channel that reaches the maintainer. The sweep uses that for a report rather than for a refusal. It registers under a group carrying no matcher, which fires on every exit reason. It also carries an explicit timeout, since the event's own budget is 1.5 seconds and only a declared per-hook timeout raises it. See the root spec's requirement 8. - **The whole of `CLAUDE.md`, rendered on every run**, loaded into every session on the machine (including ad-hoc work outside any project). It holds a header, two marker-delimited blocks of host-wide content, and the host-local file. Nothing else survives an install. Content outside the blocks was never written or checked, and it restated fleet rules in wording the fleet had since changed. diff --git a/host-setup/agent-safety/claude/gh-write-guard.py b/host-setup/agent-safety/claude/gh-write-guard.py index ca9c783d3..75b499b07 100755 --- a/host-setup/agent-safety/claude/gh-write-guard.py +++ b/host-setup/agent-safety/claude/gh-write-guard.py @@ -314,24 +314,35 @@ def _live_branch_rules(owner, repo, branch): def _current_push_branch(cwd): - """Resolve the destination branch of a bare `git push` from the branch's configured push target.""" + """Resolve the destination branches of a bare `git push` from the branch's configured push target.""" return _read_branch( cwd, ( - ["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{push}"], - ["rev-parse", "--abbrev-ref", "HEAD"], + (["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{push}"], True), + (["rev-parse", "--abbrev-ref", "HEAD"], False), ), ) def _head_branch(cwd): """Resolve the branch `HEAD` names, which a `git push HEAD` updates under the same name.""" - return _read_branch(cwd, (["rev-parse", "--abbrev-ref", "HEAD"],)) + return _read_branch(cwd, ((["rev-parse", "--abbrev-ref", "HEAD"], False),)) + + +def _branch_names(branch): + """The branch names a bare push's resolved branch stands for, a seam's single string included.""" + return [branch] if isinstance(branch, str) else list(branch) def _read_branch(cwd, reads): - """The branch the first of `reads` that succeeds in `cwd` names, or None.""" - for args in reads: + """The branch names the first of `reads` that succeeds in `cwd` gives, or None. + + Each read is `(args, remote)`. A remote read gives a remote-tracking name such as + `origin/feature/x`, and its first segment is stripped as the remote name. A local read gives a + branch name such as `release/1.0`, which is kept whole. That name is also read with its first + segment stripped, as this function read every name before, so both names are checked. + """ + for args, remote in reads: try: r = subprocess.run( ["git", "-C", cwd or ".", *args], @@ -348,7 +359,8 @@ def _read_branch(cwd, reads): return None ref = r.stdout.strip() if r.returncode == 0 and ref and ref != "HEAD": - return ref.split("/", 1)[1] if "/" in ref else ref + stripped = ref.split("/", 1)[1] if "/" in ref else ref + return [stripped] if remote else list(dict.fromkeys((ref, stripped))) return None @@ -983,6 +995,15 @@ def _overrides_hooks_path(global_opts): return any(_config_key(v) == "core.hookspath" for v in _git_config_overrides(global_opts)) +def _overrides_include(global_opts): + """True when the global options pull in a config file by `include.path` or `includeIf`. + + That file may set `core.hooksPath` where the guard cannot read it, so a commit or push under + one is read as a hooks-path override. + """ + return any(_includes_config_file(v) for v in _git_config_overrides(global_opts)) + + # --- Rule 6: a mutating git op against a primary checkout --------------------------------------------- # `-C `, `--work-tree `/`--work-tree=`, and a `GIT_WORK_TREE=` command-text prefix are the options this rule resolves the mutation target directory from, following real git's own priority rather than a last-one-wins scan across all three. # `--work-tree`/`GIT_WORK_TREE` name the working tree a mutating command like `reset --hard`/`clean -f` actually writes into, and win regardless of where `-C` points or how many `-C` options preceded it. @@ -1634,6 +1655,7 @@ def _cd_dot_dot(cmd): _GIT_COMMAND_NAMES: list[set[str]] = [] +_GIT_BUILTIN_NAMES: list[set[str]] = [] def _git_command_names(): @@ -1643,10 +1665,23 @@ def _git_command_names(): possible alias, and the caller fails closed. The self-test pins the set, so its cases do not depend on the git that runs them. """ - if not _GIT_COMMAND_NAMES: + return _listed_git_commands("builtins,main,others", _GIT_COMMAND_NAMES) + + +def _git_builtin_names(): + """The commands built into git, which no `--exec-path` or PATH change can hand to an alias. + + It is read and pinned the way `_git_command_names` is. + """ + return _listed_git_commands("builtins", _GIT_BUILTIN_NAMES) + + +def _listed_git_commands(kinds, cache): + """The names `git --list-cmds=` prints, read once into `cache`, or an empty set on failure.""" + if not cache: try: r = subprocess.run( - ["git", "--list-cmds=builtins,main,others"], + ["git", f"--list-cmds={kinds}"], capture_output=True, text=True, encoding="utf-8", @@ -1657,8 +1692,8 @@ def _git_command_names(): names = set(r.stdout.split()) if r.returncode == 0 else set() except Exception: # noqa: BLE001 names = set() - _GIT_COMMAND_NAMES.append(names) - return _GIT_COMMAND_NAMES[0] + cache.append(names) + return cache[0] def _check_primary_checkout_mutation( @@ -2180,6 +2215,7 @@ def _is_short_u(arg): _INLINE_PUSH_ALIAS = re.compile(r"alias\..*\bpush\b", re.DOTALL | re.IGNORECASE) _GIT_INCLUDE = re.compile(r"git\b.*?include", re.DOTALL | re.IGNORECASE) +_GIT_INCLUDE_KEY = re.compile(r"git\b.*?include(?:\.path|if\.)", re.DOTALL | re.IGNORECASE) def _includes_config_file(value): @@ -2211,6 +2247,17 @@ def _unreadable_push(cmd): - a `-c` config value, an inline alias expanding to a push among them - a config file an `include.path`/`includeIf` override pulls in, read when the git command it overrides names no command git lists, so it may be an alias that file defines + - the same file on a command git does not build in, which an `--exec-path`, a + `GIT_EXEC_PATH`, or a PATH change can leave to an alias that file defines + - the same file on one of `_COMMAND_RUNNING_GIT_SUBS`, whose child git commands it reaches, so + an alias it defines may push whatever the arguments name + - an include key in a wrapper shell argument other than the `-c` body, or in a wrapper body + nested past the reading cap + + An `eval` argument holding an include key is read as the command it runs, for this check + alone. It is bounded by `_EVAL_READ_BUDGET` and `_EVAL_READ_COST`, as the wait rule's is. + Once the budget is spent, the text from the next `eval` on is searched once for an include + key, which fails closed, and no later `eval` in that text is read. Any `-c`/`--config-env` on a push, or on a git command that runs one, counts too, an attached `-c` among them, which git rejects (see `_git_config_overrides`). Config such @@ -2220,12 +2267,28 @@ def _unreadable_push(cmd): """ overflow = [] texts = [cmd, *_embedded_wrapper_commands(cmd, overflow=overflow)] - if any(_GIT_PUSH.search(t) for t in overflow): + if any(_GIT_PUSH.search(t) or _GIT_INCLUDE_KEY.search(t) for t in overflow): return "wrapper shells nested deeper than the guard reads" + budget = _EVAL_READ_BUDGET for text in texts: toks = _shell_tokens(text) n = len(toks) + quoted = _quoted_mask(text, toks) if "eval" in toks else None + eval_end = 0 for k, t in enumerate(toks): + if t == "eval" and k >= eval_end: + if budget <= 0: + if _GIT_INCLUDE_KEY.search(" ".join(toks[k:])): + return "an include override inside more `eval` text than the guard reads" + eval_end = n + else: + payload, eval_end = _eval_payload(toks, k, quoted) + budget -= len(payload) + _EVAL_READ_COST + if _GIT_INCLUDE.search(payload): + texts.append(payload) + texts.extend(_embedded_wrapper_commands(payload, overflow=overflow)) + if any(_GIT_PUSH.search(o) or _GIT_INCLUDE_KEY.search(o) for o in overflow): + return "wrapper shells nested deeper than the guard reads" if t in ("eval", "source", ".") or _is_shell_wrapper_exe(t): args, _ = _collect_arglist(toks, k + 1) if _is_shell_wrapper_exe(t): @@ -2234,6 +2297,11 @@ def _unreadable_push(cmd): return ( f"a push inside an argument of `{t}` the guard does not read as a command" ) + if _is_shell_wrapper_exe(t) and any(_GIT_INCLUDE_KEY.search(a) for a in args): + return ( + f"an include override inside an argument of `{t}` the guard does not read " + "as a command" + ) if not _is_git_exe(t): continue j = _git_subcommand_index(toks, k) @@ -2246,15 +2314,23 @@ def _unreadable_push(cmd): continue if toks[j] == "push" and config: return "a push carrying a `-c`/`--config-env` override, which can change what it updates" - if ( - any(_includes_config_file(v) for v in values) - and not _is_shell_op(toks[j]) - and toks[j] not in _git_command_names() - ): + includes = _overrides_include(opts) + if includes and not _is_shell_op(toks[j]) and toks[j] not in _git_command_names(): return ( f"`git {toks[j]}` under an `include.path`/`includeIf` override, whose file " "may define it as a push alias" ) + if includes and not _is_shell_op(toks[j]) and toks[j] not in _git_builtin_names(): + return ( + f"`git {toks[j]}` under an `include.path`/`includeIf` override, a command git " + "does not build in, which an `--exec-path` or a PATH change may leave to an " + "alias that file defines" + ) + if includes and toks[j] in _COMMAND_RUNNING_GIT_SUBS: + return ( + f"`git {toks[j]}` under an `include.path`/`includeIf` override, whose file " + "may define a push alias the git commands it runs expand" + ) if toks[j] in _COMMAND_RUNNING_GIT_SUBS: args, _ = _collect_arglist(toks, j + 1) if config and any(a == "push" or _GIT_PUSH.search(a) for a in args): @@ -2326,7 +2402,7 @@ def _push_invocations(cmd): def _bare_push_branch(cwd, c_dirs, work_tree, leading_cd, cd_unreliable, head=False): - """The branch a bare push in the directory it runs in targets, or `_UNRESOLVED`. + """The branch names a bare push in the directory it runs in targets, or `_UNRESOLVED`. A directory the command moves to is read, never the session's own, and one that does not resolve to a directory, or that a later `cd` may have changed, is unresolved. The shell runs the @@ -2428,7 +2504,7 @@ def _push_targets(cmd, cwd=None, current_branch=None): if b is _UNRESOLVED: results.append(("unresolved", leading_cd or " ".join(c_dirs))) elif b: - targets.append((ops, b)) + targets.extend((ops, name) for name in _branch_names(b)) continue if dst.startswith("refs/heads/"): dst = dst[len("refs/heads/") :] @@ -2453,7 +2529,7 @@ def _push_targets(cmd, cwd=None, current_branch=None): results.append(("unresolved", leading_cd or " ".join(c_dirs))) continue if b: - targets = [(push_ops or {"update"}, b)] + targets = [(push_ops or {"update"}, name) for name in _branch_names(b)] results.extend((op, br) for ops, br in targets for op in sorted(ops)) if matched_ops: matched = [*_PROTECTED_DEFAULT_ORDER, *(br for _ops, br in targets)] @@ -2519,6 +2595,17 @@ def _check_bypass_flags(cmd): "holds instead of the repository's, and none where it holds none. That is a hook bypass; run " "the command without the override." + _handoff(cmd) ) + if any( + _overrides_include(opts) + for sub in ("commit", "push") + for t in texts + for opts, _ in _git_subcommand_invocations(t, sub) + ): + return "deny", ( + "This pulls a config file into a commit or push by an include.path or includeIf override. " + "The guard does not read that file, which may set core.hooksPath and so bypass the " + "repository's hooks. Run the command without the override." + _handoff(cmd) + ) return "allow", "" @@ -5441,6 +5528,174 @@ def classify( "allow", "control: an alias cannot shadow a command git lists, so an include on status defines no push alias", ), + ( + "git -c include.path=/example/config commit -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "an included file may set core.hooksPath for a commit, read as the hooks-path override it may be", + ), + ( + "git -c includeIf.gitdir:/example/.path=/example/config commit -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "an includeIf file on a commit is read the same way", + ), + ( + "git --config-env=include.path=EXAMPLE_CONFIG commit -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "an include set through --config-env on a commit is read the same way", + ), + ( + "bash -c 'git -c include.path=/example/config commit -m x'", + "feature/x", + {"feature/x": set()}, + "deny", + "an include on a commit inside a wrapper body is read too", + ), + ( + "git -c user.name=Example commit -m x", + "feature/x", + {"feature/x": set()}, + "allow", + "control: an override that pulls in no file leaves a commit's hooks readable", + ), + ( + "git -c include.path=/example/config rebase --exec 'git p origin develop' HEAD~1", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "an include reaches the git commands rebase runs, so an alias it defines may push", + ), + ( + "git -c include.path=/example/config submodule foreach 'git p origin develop'", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "an include reaches the git commands submodule foreach runs too", + ), + ( + "git -c include.path=/example/config rebase --continue", + "feature/x", + {"feature/x": set()}, + "deny", + "an include on a command-running subcommand is unreadable whatever its arguments say", + ), + ( + "git -c user.name=Example rebase --exec 'make test' HEAD~1", + "feature/x", + {"feature/x": set()}, + "allow", + "control: a plain override on rebase with no push word stays allowed", + ), + ( + "eval 'git -c include.path=/example/config p'", + None, + {"develop": _CODE_RULES}, + "deny", + "an eval argument holding an include key is read as the command it runs", + ), + ( + "eval 'git -c include.path=/example/config rebase --exec true HEAD~1'", + "feature/x", + {"feature/x": set()}, + "deny", + "an include on rebase inside an eval argument is read too", + ), + ( + "eval \"bash -c 'git -c include.path=/example/config p'\"", + None, + {"develop": _CODE_RULES}, + "deny", + "a wrapper body inside an eval argument is read too", + ), + ( + "eval 'git -c include.path=/example/config status'", + "feature/x", + {"feature/x": set()}, + "allow", + "control: an eval argument read as a command holds no push alias where the command is listed", + ), + ( + "eval " + "eval " * 9 + "'git -c include.path=/example/config status'", + "feature/x", + {"feature/x": set()}, + "allow", + "control: a chain of evals is read once per payload, so a listed command at its end passes", + ), + ( + "; ".join(["eval 'git -c include.path=/example/config status'"] * 300), + "feature/x", + {"feature/x": set()}, + "deny", + "an include inside more eval text than the reading budget covers fails closed", + ), + ( + "; ".join(["eval 'git -c include.path=/example/config status'"] * 6), + "feature/x", + {"feature/x": set()}, + "allow", + "control: evals side by side are each read once", + ), + ( + "bash -c '$1' _ 'git -c include.path=/example/config p'", + None, + {"develop": _CODE_RULES}, + "deny", + "an include key in a wrapper argument the guard does not read fails closed", + ), + ( + "git --exec-path=/example/libexec -c include.path=/example/config instaweb", + "feature/x", + {"feature/x": set()}, + "deny", + "an --exec-path does not make a command git does not build in readable", + ), + ( + "git -c include.path=/example/config --exec-path=/example/libexec status", + "feature/x", + {"feature/x": set()}, + "allow", + "control: a builtin runs before any alias, so an --exec-path cannot hand it to one", + ), + ( + "PATH=/example/bin git -c include.path=/example/config lfs", + "feature/x", + {"feature/x": set()}, + "deny", + "a PATH prefix does not make a command git does not build in readable", + ), + ( + "GIT_EXEC_PATH=/example/libexec git -c include.path=/example/config instaweb", + "feature/x", + {"feature/x": set()}, + "deny", + "a GIT_EXEC_PATH prefix does not make a command git does not build in readable", + ), + ( + "export GIT_EXEC_PATH=/example/libexec; bash -c 'git -c include.path=/example/config instaweb'", + "feature/x", + {"feature/x": set()}, + "deny", + "nor does an exported GIT_EXEC_PATH before a wrapper body", + ), + ( + "git -c include.path=/example/config instaweb", + "feature/x", + {"feature/x": set()}, + "deny", + "a command git does not build in is read as one an exec-path or PATH change may hand to an alias", + ), + ( + "git --exec-path=/example/libexec status", + "feature/x", + {"feature/x": set()}, + "allow", + "control: an --exec-path with no include pulls in no alias", + ), ] # (command, cwd, {dir: is_primary_or_None}, {(dir, ref): resolves_as_ref} or None, expected_decision, label) -- is_primary is True (a primary checkout), False (a linked worktree), or None (unresolved, for example when no repository exists or the git query itself fails, such as on a pre-2.31 git lacking `rev-parse --path-format`). @@ -8231,6 +8486,36 @@ def _nested_wrapper(body, depth): _PUSH_DIR_CASES = [ + ( + "@wt " + _nested_wrapper("git -c include.path=/example/config p", 6), + "deny", + "an include key in a wrapper body past the nesting cap fails closed", + ), + ( + "@wt " + _nested_wrapper("git -c user.name=Example status", 6), + "allow", + "control: a body past the nesting cap holding no push and no include key passes", + ), + ( + "@rel git push", + "deny", + "a local branch with no push target is read whole, so release/1.0 is checked as itself", + ), + ( + "@rel git push origin HEAD", + "deny", + "the branch HEAD names is read whole too", + ), + ( + "cd {rel} && git push", + "deny", + "a leading cd into the release worktree reads its branch whole", + ), + ( + "@wt git push origin HEAD", + "allow", + "control: a feature branch read whole and stripped matches no protected rule", + ), ( "@wt cd -P {base} && git push", "deny", @@ -8689,6 +8974,7 @@ def _push_dir_selftest(origin): try: base = os.path.join(root, "base") wt = os.path.join(root, "wt") + rel = os.path.join(root, "rel") missing = os.path.join(root, "missing") git = [ "git", @@ -8703,6 +8989,7 @@ def _push_dir_selftest(origin): [*git, "init", "-q", "-b", "develop", base], [*git, "-C", base, "commit", "-q", "--allow-empty", "--no-verify", "-m", "init"], [*git, "-C", base, "worktree", "add", "-q", "-b", "feature/example", wt], + [*git, "-C", base, "worktree", "add", "-q", "-b", "release/1.0", rel], ) for step in steps: try: @@ -8717,13 +9004,22 @@ def _push_dir_selftest(origin): os.makedirs(os.path.join(base, "sub2")) os.symlink(os.path.join(base, "sub2"), os.path.join(wt, "t")) rules = lambda br: ( - {"pull_request", "non_fast_forward"} if br in ("develop", "main") else set() + {"pull_request", "non_fast_forward"} + if br in ("develop", "main", "release/1.0") + else set() ) for template, want, label in _PUSH_DIR_CASES: session = base if template.startswith("@wt "): template, session = template[4:], wt - cmd = template.replace("{wt}", wt).replace("{base}", base).replace("{missing}", missing) + elif template.startswith("@rel "): + template, session = template[5:], rel + cmd = ( + template.replace("{wt}", wt) + .replace("{rel}", rel) + .replace("{base}", base) + .replace("{missing}", missing) + ) got, _ = classify( cmd, cwd=session, @@ -8801,7 +9097,23 @@ def _selftest(): ok = False print(f" {mark} [{got:5}] want={want:5} {label}") saved_names = list(_GIT_COMMAND_NAMES) - _GIT_COMMAND_NAMES[:] = [{"checkout", "commit", "reset", "status", "worktree"}] + saved_builtins = list(_GIT_BUILTIN_NAMES) + _GIT_COMMAND_NAMES[:] = [ + { + "checkout", + "commit", + "instaweb", + "lfs", + "rebase", + "reset", + "status", + "submodule", + "worktree", + } + ] + _GIT_BUILTIN_NAMES[:] = [ + {"checkout", "commit", "rebase", "reset", "status", "submodule", "worktree"} + ] for cmd, cur, rmap, want, label in _GIT_CASES: got, _ = classify( cmd, @@ -8850,6 +9162,7 @@ def _selftest(): primary_checkout_lookup=lambda _d: False, ) _GIT_COMMAND_NAMES[:] = saved_names + _GIT_BUILTIN_NAMES[:] = saved_builtins mark = "ok " if got == "deny" else "FAIL" if got != "deny": ok = False From 4a0370f920adb7997f59bac21263ea7c7bd415df Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 13 Oct 2026 22:54:52 -0700 Subject: [PATCH 3/3] Fail Closed on Words the Shell Rewrites Before Git Reads Them in the Write Guard (#2979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This closes the write guard's gaps where the shell rewrites or drops a word before git reads it, in `host-setup/agent-safety/claude/gh-write-guard.py`, and names `pr_review.py cleanup` in the managed permission rule's description. Each guard fix adds a reading or a deny beside develop's and replaces none of develop's readings. - **Quote-split words and uppercase `GIT` (#2959).** The push and include pre-filters also read each command with its quotes and backslashes removed, with `git` matched without case. So `git p''ush origin develop`, `git "pu"sh ...`, `git p\ush ...`, `git -c 'inc''lude.path=/example/config' p`, and `GIT push ...` reach the existing push and include checks, which already read the joined tokens. The same widened match is used inside `_unreadable_push` for `eval`, wrapper, `-c`, and command-running arguments. - **Run-time words (#2959, #2961).** A new deny, `_rewritten_git_words`, fails closed rather than modeling the expansion. A run-time word is a `$`, a backtick, a brace expansion, or a lone parenthesis run from `$(`. It denies: - a run-time word where any git command's subcommand goes, which covers a backtick word ending the global options, as in ``git -c core.hooksPath=/dev/null `true` commit`` - a run-time global option word that word splitting may split, on any git command, or before no subcommand word at all - any run-time word among a push's or a commit's global options other than a directory option's value - a run-time word among a push's arguments, as in `git push origin $(echo develop)` or `"$B"` - a run-time word among a commit's arguments, other than a commit option's value whose expansions sit inside double quotes, or a path after `--` - a redirection where the subcommand goes or among the global options, unless git's first word is a terminal option such as `--version`, and a redirection target spanning several words on a push or commit, since the push and bypass readings stop at its parenthesis - **Comments (#2961).** An unquoted comment among a push's words denies, so `git push # feature/y` on a develop checkout is no longer read as an explicit push of `feature/y`. A comment on a commit only adds words the guard reads, which can only add a deny, so it is left alone. - **The managed permission rule (#2968).** The comment above `MANAGED_PERMISSIONS` and the Claude kit README name `cleanup` beside `reply`, and say every check for the delete lives inside the subcommand because the `:*` rule approves any arguments. No behavior changes in `install.py`. Only a git word in a command's position counts, so `grep -rn git "$W"` is no git command, and a git word after an unquoted `(` counts, so `<(git ...)` is read. The common agent shapes still allow: a heredoc `-m "$(cat <<'EOF' ...)"` message (with apostrophes, backticks, braces, or one unbalanced double quote in its body), `-m "$MSG"`, `--author="$A"`, `git -C "$W" push origin feature/x`, unquoted `git -C $W commit`/`push`, and `git -C "$W" log`/`status`/`diff`. Requirement 4 in `host-setup/agent-safety/README.md` and the guard bullet in the Claude kit README state the new denies. ## Differential Against Develop's Guard Develop's guard (the merge base, e98def1f) and this branch's guard were loaded side by side and run over the same calls. - **Corpus:** 18,393 constructed commands. They cross 35 heads with 8 mutation families and 12 wrappers. The heads are 18 push forms, 9 commit forms including two heredoc messages, and 8 other git commands. The mutation families are quote splits of `push`/`commit`/`include`/`git`/`develop` (uppercase and mixed-case `git` among them), run-time refspecs (`$(...)`, backtick, `$B`, `"$B"`, `${B}`, `$'...'`, an empty expansion, a substitution holding a `;`), run-time subcommands, run-time global options (`-c "$X"`, `` `true` ``, `$(true)`, `-C "$W"`, `-C $W`, `-C $(pwd)`, `--git-dir=`, `--work-tree=`, `$OPTS`), comments, run-time commit arguments, and redirections. The wrappers are plain, `bash -c`, `sh -c ... _ develop`, `eval` quoted and unquoted, a leading `cd`, `&&`, `;`, `timeout`, a subshell, `x=$(...)`, and `grep -rn`. Sixteen shapes the issues name are added whole. - **Real-repository mode:** each command in a constructed repository's develop checkout and in worktrees on `feature/example`, `release/1.0`, and `x/develop`, each with a real upstream, under six rule sets, one protecting only the stripped names `1.0` and `example`. That is 441,432 calls per guard with no current-branch seam. - **Seam mode:** the same corpus under 6 current-branch seams and 4 rule seams (441,432 calls). - **Self-test replay:** every `classify` call the branch self-test makes (894), with each pinned command list mirrored into develop's module. - **Rule 6:** the corpus with a primary-checkout lookup (18,393 calls). **Allow flips (develop denies, this branch allows): zero in every mode. Crashes: zero.** Every deny flip, by the reason the branch gives, from real-repository mode's 7,303 distinct flipped commands: - 2,721: a run-time word where the subcommand goes (`git $S`, `git $(echo push)`, `` git `true` commit ``, `git -C $(pwd) status`). - 1,485: a quote-split or uppercase push now read by the existing push check, denied as a pull-request, force, delete, or unresolved-branch push. - 1,424: a run-time push argument. - 764: a comment among a push's words. - 320: a run-time commit argument (`"$F"` before `--`, `-m $M`, `$(echo --no-verify)`, `--author=$A`). - 237: a quote-split push or include key now read inside an `eval` or wrapper argument, or under an include override. - 180: a redirection target spanning several words (`2>$(mktemp)`). - 170: a run-time global option word on a push or commit (`-c "$X"`, `-c user.email=$E`). - 2: a quote-split push inside an inline alias value or a `rebase --exec` argument. Seam mode gives the same classes over 7,561 distinct commands. Rule 6 gives 3,009, the same classes less those rule 6 already denies on develop. The self-test replay flips are exactly the 37 new discriminating cases. Each new deny case fails against develop's guard, which `_GIT_CASES` run against develop's module confirms. On long commands the branch costs about 1.4s more on a 3,000-line heredoc commit (5.5s against 4.0s) and about 1.2s more on 2,000 chained `git -C "$W" status` commands. ## Known New Over-Denials - Any run-time push refspec or remote, such as `git push origin "$B"` or `"$(git branch --show-current)"`. - Any run-time word where a git command's subcommand goes, on any command, such as `git $S`, `git "$@"`, and `git -C $(pwd) status`. - An unquoted run-time global option value that may split, such as `git -C $W` with no subcommand word, or `git --namespace $X log`. Any run-time global option on a push or commit other than a directory value, such as `git -c user.email="$E" commit`. - An unquoted run-time commit argument, such as `-m $MSG`, `--author=$A`, or `-m $(cat file)`, and a quoted one before `--` that no option takes, such as `git commit -m x "$F"`. - Any comment among a push's words, such as `git push origin feature/x # note`. - A redirection target spanning several words on a push or commit, such as `2>$(mktemp)`. - A redirection before the subcommand, such as `git 2>/dev/null status`, unless git's first word is a terminal option such as `--version`. - A `-m` value whose quote-keeping token cannot be read, other than one closed `"$(...)"` word, when some other part of the command breaks that token reading. ## Left Open Deliberately - An unquoted directory value that word splitting splits into more options, as in `X='. -c core.hooksPath=/dev/null'; git -C $X commit`, stays read as it stands, since an unquoted `git -C $W commit` is a common agent spelling. Filed as #2977. A brace expansion in a directory value denies. - A git word the guard never sees as git, a backtick-opened `` `git push ...` `` or a run-time command word `G=git; $G push`, is filed as #2976. - An abbreviated `--no-verify`, as in `git commit --no-veri`, is outside this class and filed as #2978. ## Local Review Eight local strict-review passes ran at the `frontier` tier, each recorded. None found an allow flip against develop. Each of the first five found gaps in this change's own fix, fixed in the commits after the first: - a quoted part before an unquoted expansion in a commit value - a run-time option value supplying the subcommand, on any git command - abbreviated value options taking `--` as their value - brace expansion, quoted parts inside a brace included - a multi-token redirection target, and a redirection where the subcommand goes or among the global options, numbered and `{name}` forms included - a terminal option such as `--version` exempting a redirection, now only as git's first word, which also clears the `git --version >/dev/null 2>&1` over-deny an earlier commit introduced The sixth pass raised one deny-reason wording mismatch, fixed in a commit the seventh pass confirmed changes that string alone. The eighth pass covered the fix for Copilot's quoted-operator finding and found no open issue. Declined: - Sharing `_opaque_cd`'s run-time predicate would replace a develop reading, since its exclusions differ. - `git -C "$W" --version >/dev/null` denies, since the terminal-option exception holds only as git's first word. Agents write `git --version` first, so it is a stated over-deny. - Sentence-length findings are declined as style, since that check is in no gate. No local finding is open. ## Verification - `gh-write-guard.py --selftest`: SELFTEST PASS, 37 new discriminating cases and 12 new controls. - `ruff check`, `ruff format --check`, `mypy`, `python3 -m unittest discover -s tests`, `spec/audit.py --selftest`, `test_install.py`, `build_dist.py --check`, `repo_gate.py` (default, `eol`, `eol-coverage`), `prose_lint.py` on the changed files and `--diff` against the merge base, `spec/validate.py`, and `docker_lint.py` all pass. The only prose finding is `comment-added`, for the `install.py` comment #2968 asks for and one `classify` comment, so this pull request carries the `comments` label. After merge, the host-safety installer payload changes. The installer is not run here, and the maintainer runs it after promotion. Closes #2959 Closes #2961 Closes #2968 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- host-setup/agent-safety/README.md | 13 +- host-setup/agent-safety/claude/README.md | 4 +- .../agent-safety/claude/gh-write-guard.py | 711 +++++++++++++++++- host-setup/agent-safety/claude/install.py | 5 +- 4 files changed, 717 insertions(+), 16 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index c9b5fb61d..4a594b047 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -80,7 +80,18 @@ text says, because the harm it covers was never in the text. `HEAD`/`@` refspec whose directory or branch may change before it runs. Such a push's local branch is checked under its whole name, such as `release/1.0`. It is also checked with its first segment dropped. A remote-tracking push target such as `origin/feature/x` is checked with its - remote dropped. The + remote dropped. A word the shell rewrites or drops before git reads it is denied where it can + change what git runs. Such a word is a `$`, backtick, or brace-expansion word. It is denied + where any git command's subcommand goes, and among a push's words or a commit's. On any git + command it is denied among the global options where word splitting may split it. A + redirection where the subcommand goes or among the global options is denied too, a numbered + one such as `2>` included. The exception is a git whose first word is an option such as + `--version` that ends git there. A redirection target spanning several words on a push or a + commit is denied, and so is a comment among a push's words. Some words are read as they + stand. They are a directory option's value short of a brace expansion, a commit option's + double-quoted value, a commit path after `--`, and a redirection target held in one word. The + push and include checks also read each command with its quotes and backslashes removed and + `git` in any case, so a `p''ush` or an uppercase `GIT` reaches them. The workaround is an explicit refspec run as its own command. Every other requirement here favors precision over recall throughout, denying only a positively-identified dangerous shape, since a hook that fails closed on an unrelated resolution failure blocks diff --git a/host-setup/agent-safety/claude/README.md b/host-setup/agent-safety/claude/README.md index bd8464d61..94dd84409 100644 --- a/host-setup/agent-safety/claude/README.md +++ b/host-setup/agent-safety/claude/README.md @@ -12,7 +12,7 @@ installation and operational detail. Codex and opencode have no equivalent hook Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): -- **`hooks/gh-write-guard.py`**: a PreToolUse hook that denies four 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 under an owner other than the checkout origin's. Sibling repositories under the same owner are allowed, since the harm this guards is reaching a stranger's repository rather than working across one maintainer's own fleet, and a different owner is allowed only when the maintainer names it in `GH_WRITE_GUARD_ALLOW` (an `owner/repo` list, where `owner/*` grants a whole owner). That variable is read from the environment the session was launched with, which is the one channel an agent cannot set for itself: a hook runs as its own process, so an inline `VAR=x cmd` prefix or an `export` inside a Bash call never reaches it. Second, a **git operation that bypasses a repository protection**. The branch-rule cases (a direct push, a force-push, or a branch delete to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked) are judged per refspec 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, either because the API is unreachable or because the checkout's origin cannot be resolved to query them. A push whose target the hook cannot read, such as one in text it does not parse, one carrying a `-c` config override, a git command an `include.path`/`includeIf` override may define as a push alias, or a git command that runs other git commands under one, is denied whatever branch it names. 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, as does a `git -c core.hooksPath=` override on either, which points git at another hooks directory, and an `include.path`/`includeIf` override on either, whose file may do the same. These are read in a command the hook parses, a wrapper shell's body among them, and not inside a quoted `eval` argument. Third, a **mutating git operation run directly against a primary checkout** (`checkout`/`switch`/`pull`/`reset`/`rebase`/`merge`/`cherry-pick`/`revert`/`restore`/`add`/`commit`/`rm`/`mv`/`apply`/`am`/`push`/`stash` (anything but `list`/`show`)/`clean -f`/`worktree remove -f`), the harm behind a second incident where an agent reused the maintainer's own primary checkout instead of a worktree despite having read the prose rule against it. "Primary" is decided by comparing `git rev-parse --git-dir` against `--git-common-dir`, not a `.git`-is-a-directory guess. `worktree add`/`list`/`prune`, an unforced `worktree remove`, `merge --ff-only`/`pull --ff-only`, and a `checkout `/`switch ` carrying no force flag (bundled or attached forms like `-qf`/`-Bname` recognized, not only an exact token) stay exempt even in a primary checkout, matching the documented base-clone cleanup step the `repo-worktree` skill already carries. A subcommand this rule does not recognize is resolved through a chain of git aliases, inline and persisted, before falling through, so a custom alias cannot hide a denied builtin behind an unrecognized name. A `!`-prefixed shell alias is denied outright rather than interpreted. A directory named with a `..` component is not resolved, so a mutating command naming one is denied. That holds for `-C`, any `cd`, `pushd`, `popd`, or `chdir` word in the command, `--work-tree`, `--git-dir`, and a `GIT_DIR=`/`GIT_WORK_TREE=` prefix. The target directory follows real git's own `-C`/`--work-tree`/`--git-dir` priority, not a last-option-wins scan. A different owner-style grant, `GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT`, follows the same channel as `GH_WRITE_GUARD_ALLOW`. Reads and everything else pass through. Fourth, an **unbounded shell wait**, which the root spec's requirement 7 defines in full rather than this bullet redefining it. In outline: a loop that sleeps, carrying no bound the rule recognizes. It is the shape an agent writes when a brief tells it to poll. It is also the harm behind a third incident, where seven such loops outlived the subagents that wrote them and the run that dispatched those subagents. A shell started by a tool call runs in a session of its own, so the agent could not have reaped it even had it tried. A nested loop is judged on its own terms, and a command that forks anything out of a `timeout`'s reach, by backgrounding it or by `setsid`, is bounded by nothing. A heredoc body is data rather than a command line unless a shell is what it is fed to, so a document quoting the forbidden shape is written rather than denied. It fires even in autonomous / bypass-permissions sessions, which is how the first incident happened. See the root spec's requirements 6 and 7 for the exact rules. +- **`hooks/gh-write-guard.py`**: a PreToolUse hook that denies four 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 under an owner other than the checkout origin's. Sibling repositories under the same owner are allowed, since the harm this guards is reaching a stranger's repository rather than working across one maintainer's own fleet, and a different owner is allowed only when the maintainer names it in `GH_WRITE_GUARD_ALLOW` (an `owner/repo` list, where `owner/*` grants a whole owner). That variable is read from the environment the session was launched with, which is the one channel an agent cannot set for itself: a hook runs as its own process, so an inline `VAR=x cmd` prefix or an `export` inside a Bash call never reaches it. Second, a **git operation that bypasses a repository protection**. The branch-rule cases (a direct push, a force-push, or a branch delete to a branch that requires a pull request, a force-push where history is protected, a branch delete where deletion is blocked) are judged per refspec 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, either because the API is unreachable or because the checkout's origin cannot be resolved to query them. A push whose target the hook cannot read, such as one in text it does not parse, one carrying a `-c` config override, a git command an `include.path`/`includeIf` override may define as a push alias, or a git command that runs other git commands under one, is denied whatever branch it names. So is a word the shell rewrites or drops before git reads it, where it can change what git runs. Such a word is a `$`, backtick, or brace-expansion refspec, subcommand, or commit argument, or a comment among a push's words. 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, as does a `git -c core.hooksPath=` override on either, which points git at another hooks directory, and an `include.path`/`includeIf` override on either, whose file may do the same. These are read in a command the hook parses, a wrapper shell's body among them, and not inside a quoted `eval` argument. Third, a **mutating git operation run directly against a primary checkout** (`checkout`/`switch`/`pull`/`reset`/`rebase`/`merge`/`cherry-pick`/`revert`/`restore`/`add`/`commit`/`rm`/`mv`/`apply`/`am`/`push`/`stash` (anything but `list`/`show`)/`clean -f`/`worktree remove -f`), the harm behind a second incident where an agent reused the maintainer's own primary checkout instead of a worktree despite having read the prose rule against it. "Primary" is decided by comparing `git rev-parse --git-dir` against `--git-common-dir`, not a `.git`-is-a-directory guess. `worktree add`/`list`/`prune`, an unforced `worktree remove`, `merge --ff-only`/`pull --ff-only`, and a `checkout `/`switch ` carrying no force flag (bundled or attached forms like `-qf`/`-Bname` recognized, not only an exact token) stay exempt even in a primary checkout, matching the documented base-clone cleanup step the `repo-worktree` skill already carries. A subcommand this rule does not recognize is resolved through a chain of git aliases, inline and persisted, before falling through, so a custom alias cannot hide a denied builtin behind an unrecognized name. A `!`-prefixed shell alias is denied outright rather than interpreted. A directory named with a `..` component is not resolved, so a mutating command naming one is denied. That holds for `-C`, any `cd`, `pushd`, `popd`, or `chdir` word in the command, `--work-tree`, `--git-dir`, and a `GIT_DIR=`/`GIT_WORK_TREE=` prefix. The target directory follows real git's own `-C`/`--work-tree`/`--git-dir` priority, not a last-option-wins scan. A different owner-style grant, `GH_WRITE_GUARD_ALLOW_PRIMARY_CHECKOUT`, follows the same channel as `GH_WRITE_GUARD_ALLOW`. Reads and everything else pass through. Fourth, an **unbounded shell wait**, which the root spec's requirement 7 defines in full rather than this bullet redefining it. In outline: a loop that sleeps, carrying no bound the rule recognizes. It is the shape an agent writes when a brief tells it to poll. It is also the harm behind a third incident, where seven such loops outlived the subagents that wrote them and the run that dispatched those subagents. A shell started by a tool call runs in a session of its own, so the agent could not have reaped it even had it tried. A nested loop is judged on its own terms, and a command that forks anything out of a `timeout`'s reach, by backgrounding it or by `setsid`, is bounded by nothing. A heredoc body is data rather than a command line unless a shell is what it is fed to, so a document quoting the forbidden shape is written rather than denied. It fires even in autonomous / bypass-permissions sessions, which is how the first incident happened. See the root spec's requirements 6 and 7 for the exact rules. - **`hooks/tool-containment.py`**: a shell prefix, registered as `CLAUDE_CODE_SHELL_PREFIX` in the `env` block of `settings.json`, that runs every Bash tool call inside a transient systemd user scope carrying `TasksMax`, `MemoryMax`, and `MemorySwapMax=0`, the last so a runaway at the memory ceiling is killed rather than spilled into the host's swap. A runaway fan-out then fails at the ceiling with `fork: Resource temporarily unavailable` instead of growing until the host has to be reset, the harm behind the probe-shim incident in the root spec's requirement 9. The guard above reads the command text, and that incident's recursion lived in a script file the command only named. The ceilings default to 8192 tasks and 25% of RAM. A `claude` session started from a tool call is nested in the session that ran it. Every scope of such a chain also joins one slice, `claude-tree-.slice`. That slice carries a total ceiling for the whole tree, 32768 tasks and 50% of RAM. The root's id reaches nested sessions as `AGENT_CONTAINMENT_ROOT`, which every contained command inherits. The tree's ceiling is a runtime drop-in put on when its slice is first created, and the sweep below reverts it. The maintainer raises any ceiling in the same `env` block. An inline `VAR=x` in a command cannot change one, since the prefix reads it before the command runs. The variables are `AGENT_CONTAINMENT_TASKS_MAX` and `AGENT_CONTAINMENT_MEMORY_MAX` for a scope, and `AGENT_CONTAINMENT_TREE_TASKS_MAX` and `AGENT_CONTAINMENT_TREE_MEMORY_MAX` for the tree. A tree ceiling set below a scope's binds first. Each scope is named `claude-tool---`, so the sweep below can stop one session's tool calls and nothing else. Its description records the pid of the agent that ran it. That record is how the sweep tells a scope whose session has ended from one whose session still runs. The installer sets the prefix only where it can contain, per the Linux bullet under "Scope and Limits", and never over a `CLAUDE_CODE_SHELL_PREFIX` value that names anything else. - **`hooks/stray-process-sweep.py`**: a SessionEnd hook in two phases. First, it stops every `claude-tool--*` scope this session left loaded and reports what each held, since the scope's name proves which session started it. It then stops every scope in this session's tree whose recorded agent is no longer running, and names each one's session. That is how a nested session's scopes end, since stopping the parent's scopes kills the nested `claude` before its own sweep runs. Each stop can end an agent nested deeper, so it judges again, up to four rounds. The root session then stops its tree's slice and reverts the ceiling put on it, once nothing is left in it. Second, it reports, and never ends, the process trees outside those scopes that outlive the session. The guard above stops a leak from being written. This finds the ones already running: the ones started before the guard was installed, and the shapes the guard deliberately does not reach. It reports every descendant of the agent process running outside the agent's own session. It names each subtree root's command and age, and hands over the `kill` line. This phase kills nothing itself. A long build backgrounded on purpose reads identically to a leaked wait from there. What is still wanted on the machine is the maintainer's call. A SessionEnd hook cannot block, and its JSON output is discarded, so stderr on exit 2 is the one channel that reaches the maintainer. The sweep uses that for a report rather than for a refusal. It registers under a group carrying no matcher, which fires on every exit reason. It also carries an explicit timeout, since the event's own budget is 1.5 seconds and only a declared per-hook timeout raises it. See the root spec's requirement 8. - **The whole of `CLAUDE.md`, rendered on every run**, loaded into every session on the machine (including ad-hoc work outside any project). It holds a header, two marker-delimited blocks of host-wide content, and the host-local file. Nothing else survives an install. Content outside the blocks was never written or checked, and it restated fleet rules in wording the fleet had since changed. @@ -21,7 +21,7 @@ Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): - A local file that cannot be read, is not a regular file, or is not UTF-8 stops the install before anything changes. So does one carrying this kit's own markers, since appending it would duplicate a block. - The `agent-safety` block carries restrictions alone, so nothing in it can widen a permission. `Repository Boundaries and Write Safety` mirrors the committed `GOVERNANCE.md` section of the same name, carrying as behavioral guidance its write rules and its rules that a task works in its own checkout and isolates into its own worktree before its first file edit. `Data in Agent-Authored Text` mirrors `GOVERNANCE.md` "Representative Data in Agent-Authored Text", so an agent illustrates with constructed data rather than data observed on the machine. `Authorization Scope and Memory Hygiene` bounds how a granted permission is recorded and read. Each mirrors a rule that otherwise reaches fleet repos only. - The `fleet-bootstrap` block carries `Fleet Governance Entry Point`, which names the template repository and routes by the state a repository is actually in. It is separate precisely because it enables rather than restricts, and the safety block's own text says nothing in it widens a permission, so merging the two would contradict that. It is host-wide rather than per repository because the repositories that most need it are the ones carrying no instruction set to point the way, and it mirrors the byte-locked `AGENTS.md` "Fleet Bootstrap" section that a conformant repository carries. -- **The permission rules this kit owns, merged into `settings.json`** beside the hook registration. Each is declared as a prefix and a rule, and a re-run drops every rule the prefix owns before adding the current one, so a rule whose spelling changes updates in place rather than accumulating beside the version it replaced. Ownership requires a rule-syntax delimiter after the prefix, since the prefix ends at the script name and a bare prefix test would also claim a longer path such as `pr_review.py-custom`, so a rule written by hand for a different script is never touched. These widen rather than restrict, which is why they are their own component for the same reason the `fleet-bootstrap` block is separate from the `agent-safety` one. Today the list holds one rule, for `scripts/pr_review.py`, the review loop's reply and resolve. Driving that loop by hand needs a raw GraphQL mutation carrying a node id, which is the shape that reached a stranger's repository, where the script queries the id itself and takes no argument an id fits in. What the rule decides is which command runs without a prompt, and it matches the command text rather than the directory the command runs in, so it reaches a `scripts/pr_review.py` in any checkout that carries one. An absolute path would not narrow that, since the hub is reached as a checkout of the caller's own and its location differs per task, so pinning one path would name a checkout the next task does not use. What bounds it is the rule that an agent reaches the hub as a checkout of its own, fetched immediately before it is read, rather than a copy it happens to find on disk, which the `fleet-bootstrap` block beside this carries and [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" states in full. +- **The permission rules this kit owns, merged into `settings.json`** beside the hook registration. Each is declared as a prefix and a rule, and a re-run drops every rule the prefix owns before adding the current one, so a rule whose spelling changes updates in place rather than accumulating beside the version it replaced. Ownership requires a rule-syntax delimiter after the prefix, since the prefix ends at the script name and a bare prefix test would also claim a longer path such as `pr_review.py-custom`, so a rule written by hand for a different script is never touched. These widen rather than restrict, which is why they are their own component for the same reason the `fleet-bootstrap` block is separate from the `agent-safety` one. Today the list holds one rule, for `scripts/pr_review.py`, which covers the review loop's `reply` and resolve and the `cleanup` delete of a merged pull request's head branch. Driving that loop by hand needs a raw GraphQL mutation carrying a node id, which is the shape that reached a stranger's repository, where the script queries the id itself and takes no argument an id fits in. The `:*` rule approves any arguments, so it approves any `cleanup` call. Every check the delete makes therefore lives inside the subcommand rather than in the rule. What the rule decides is which command runs without a prompt, and it matches the command text rather than the directory the command runs in, so it reaches a `scripts/pr_review.py` in any checkout that carries one. An absolute path would not narrow that, since the hub is reached as a checkout of the caller's own and its location differs per task, so pinning one path would name a checkout the next task does not use. What bounds it is the rule that an agent reaches the hub as a checkout of its own, fetched immediately before it is read, rather than a copy it happens to find on disk, which the `fleet-bootstrap` block beside this carries and [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" states in full. See [`../README.md`][spec] "Requirements" for which of these each rule implements, and "Auditing an Implementation Against This Spec" for how to check this hook still satisfies them after a change. diff --git a/host-setup/agent-safety/claude/gh-write-guard.py b/host-setup/agent-safety/claude/gh-write-guard.py index 75b499b07..e0804701c 100755 --- a/host-setup/agent-safety/claude/gh-write-guard.py +++ b/host-setup/agent-safety/claude/gh-write-guard.py @@ -2216,6 +2216,27 @@ def _is_short_u(arg): _INLINE_PUSH_ALIAS = re.compile(r"alias\..*\bpush\b", re.DOTALL | re.IGNORECASE) _GIT_INCLUDE = re.compile(r"git\b.*?include", re.DOTALL | re.IGNORECASE) _GIT_INCLUDE_KEY = re.compile(r"git\b.*?include(?:\.path|if\.)", re.DOTALL | re.IGNORECASE) +_GIT_PUSH_UNQUOTED = re.compile(r"(?i:git)\b.*?\bpush\b", re.DOTALL) +_QUOTING_CHARS = re.compile(r"[\"'\\]") + + +def _unquoted(text): + """`text` with every quote and backslash removed, which joins the words the shell joins.""" + return _QUOTING_CHARS.sub("", text) + + +def _names_push(text): + """True when `text` spells a git push as written, or once its quoting is removed. + + The shell joins `p''ush` into `push` before git reads it. The second reading matches `git` + without case, as `_is_git_exe` does, since a case-insensitive filesystem resolves `GIT` to git. + """ + return bool(_GIT_PUSH.search(text) or _GIT_PUSH_UNQUOTED.search(_unquoted(text))) + + +def _names_include(text, pattern=_GIT_INCLUDE): + """True when `pattern` matches `text` as written, or once its quoting is removed.""" + return bool(pattern.search(text) or pattern.search(_unquoted(text))) def _includes_config_file(value): @@ -2267,7 +2288,7 @@ def _unreadable_push(cmd): """ overflow = [] texts = [cmd, *_embedded_wrapper_commands(cmd, overflow=overflow)] - if any(_GIT_PUSH.search(t) or _GIT_INCLUDE_KEY.search(t) for t in overflow): + if any(_names_push(t) or _names_include(t, _GIT_INCLUDE_KEY) for t in overflow): return "wrapper shells nested deeper than the guard reads" budget = _EVAL_READ_BUDGET for text in texts: @@ -2278,26 +2299,30 @@ def _unreadable_push(cmd): for k, t in enumerate(toks): if t == "eval" and k >= eval_end: if budget <= 0: - if _GIT_INCLUDE_KEY.search(" ".join(toks[k:])): + if _names_include(" ".join(toks[k:]), _GIT_INCLUDE_KEY): return "an include override inside more `eval` text than the guard reads" eval_end = n else: payload, eval_end = _eval_payload(toks, k, quoted) budget -= len(payload) + _EVAL_READ_COST - if _GIT_INCLUDE.search(payload): + if _names_include(payload): texts.append(payload) texts.extend(_embedded_wrapper_commands(payload, overflow=overflow)) - if any(_GIT_PUSH.search(o) or _GIT_INCLUDE_KEY.search(o) for o in overflow): + if any( + _names_push(o) or _names_include(o, _GIT_INCLUDE_KEY) for o in overflow + ): return "wrapper shells nested deeper than the guard reads" if t in ("eval", "source", ".") or _is_shell_wrapper_exe(t): args, _ = _collect_arglist(toks, k + 1) if _is_shell_wrapper_exe(t): args = _wrapper_unread_args(args) - if any(_GIT_PUSH.search(a) for a in args): + if any(_names_push(a) for a in args): return ( f"a push inside an argument of `{t}` the guard does not read as a command" ) - if _is_shell_wrapper_exe(t) and any(_GIT_INCLUDE_KEY.search(a) for a in args): + if _is_shell_wrapper_exe(t) and any( + _names_include(a, _GIT_INCLUDE_KEY) for a in args + ): return ( f"an include override inside an argument of `{t}` the guard does not read " "as a command" @@ -2307,7 +2332,12 @@ def _unreadable_push(cmd): j = _git_subcommand_index(toks, k) opts = toks[k + 1 : j] values = _git_config_overrides(opts) - if any(_GIT_PUSH.search(v) or _INLINE_PUSH_ALIAS.match(v) for v in values): + if any( + _names_push(v) + or _INLINE_PUSH_ALIAS.match(v) + or _INLINE_PUSH_ALIAS.match(_unquoted(v)) + for v in values + ): return "a push inside a git `-c` config value" config = bool(values) if j >= n: @@ -2333,13 +2363,320 @@ def _unreadable_push(cmd): ) if toks[j] in _COMMAND_RUNNING_GIT_SUBS: args, _ = _collect_arglist(toks, j + 1) - if config and any(a == "push" or _GIT_PUSH.search(a) for a in args): + if config and any(a == "push" or _names_push(a) for a in args): return f"a push run by `git {toks[j]}` under a `-c`/`--config-env` override" - if any(_GIT_PUSH.search(a) for a in args): + if any(_names_push(a) for a in args): return f"a push inside one quoted argument of `git {toks[j]}`" return None +_GIT_DIR_VALUE_OPTS = frozenset({"-C", "--git-dir", "--work-tree"}) +_COMMIT_LONG_VALUE_OPTS = frozenset( + {"--message", "--file", "--reuse-message", "--reedit-message", "--author", "--date"} + | {"--template", "--cleanup", "--fixup", "--squash", "--trailer", "--pathspec-from-file"} +) +_COMMIT_VALUE_OPTS = _COMMIT_LONG_VALUE_OPTS | {"-m", "-F", "-C", "-c", "-t"} +_COMMIT_VALUE_LETTERS = "mFCct" +_GIT_TERMINAL_OPTS = frozenset({"--version", "-v", "--help", "-h", "--html-path", "--man-path"}) +_BRACE_EXPANSION = re.compile(r"\{[^{}'\"\s]*(?:,|\.\.)[^{}'\"\s]*\}") + + +def _is_run_time_word(tok): + """True when the shell may rewrite a token at run time. + + That is a `$` or a backtick in it, a brace expansion such as `{a,b}`, or a lone parenthesis run. + """ + return ( + "$" in tok + or "`" in tok + or (tok != "" and set(tok) <= {"(", ")"}) + or bool(_BRACE_EXPANSION.search(tok)) + ) + + +def _region_end(toks, k, quoted=None): + """The index of the separator after `toks[k]` that ends its command. + + A parenthesis the command opens, as a `$(...)` or a `<(...)` in one of its words does, is read + through to the one closing it, separators inside included. A closing one it did not open ends + the command, as a subshell's does. A token `quoted` marks as quoted is a word rather than an + operator, and with no mask every operator-shaped token is read as one. + """ + n = len(toks) + depth = 0 + e = k + 1 + while e < n: + t = toks[e] + if _is_shell_op(t) and not (quoted and quoted[e]): + sep = _is_separator(t) + if sep and depth == 0 and t[0] not in "()": + break + depth += t.count("(") - t.count(")") + if depth < 0 or (sep and depth == 0 and not set(t) <= {"(", ")"}): + break + e += 1 + return e + + +def _span_end(toks, i, end): + """The index after a run-time word starting at `toks[i]` that spans several tokens, or `i + 1`. + + A `$(...)` and a backtick substitution holding a space each lex as several tokens. The token at + `end` is read too, since a fused `))` closes the span and the group holding the command at once. + None says the span does not close by then. + """ + t = toks[i] + if t.endswith("$") and i + 1 < end and toks[i + 1].startswith("("): + depth = 0 + for x in range(i + 1, min(end + 1, len(toks))): + if _is_shell_op(toks[x]): + depth += toks[x].count("(") - toks[x].count(")") + if depth <= 0: + return min(x + 1, end) + return None + if t.count("`") % 2: + for x in range(i + 1, end): + if toks[x].count("`") % 2: + return x + 1 + return None + return i + 1 + + +def _is_commit_value_option(tok): + """True when a commit argument takes the next word as its value. + + That is `-m`, a cluster such as `-am`, or a long value option or an abbreviation of one, since + git reads `--mess` as `--message` and takes a following `--` as its value. + """ + if tok in _COMMIT_VALUE_OPTS: + return True + if tok.startswith("--") and len(tok) > 2 and "=" not in tok: + return any(o.startswith(tok) for o in _COMMIT_LONG_VALUE_OPTS) + return ( + len(tok) > 1 + and tok[0] == "-" + and tok[1] != "-" + and tok[1:].isalpha() + and tok[-1] in _COMMIT_VALUE_LETTERS + ) + + +def _holds_attached_value(tok): + """True when a run-time word sits after an option name that takes it as an attached value. + + That is `--name=`, or a short commit value option such as `-m`. + """ + cut = min((tok.index(c) for c in "$`" if c in tok), default=len(tok)) + literal = tok[:cut] + if literal.startswith("--") and "=" in literal: + return True + return len(literal) >= 2 and literal[0] == "-" and literal[1] in _COMMIT_VALUE_LETTERS + + +def _expands_unquoted(raw, braces_only=False): + """True when a token as written holds a `$`, a backtick, or a brace expansion outside double quotes. + + Word splitting may split what such an expansion gives into more arguments, and a brace + expansion gives several words itself. One inside a `$(...)` that double quotes hold is still + inside those quotes. A brace expands where its `{`, its `,` or `..`, and its `}` are unquoted, + whatever quoted parts sit between them. With `braces_only`, only a brace expansion counts. + """ + stack = [] + i = 0 + while i < len(raw): + ch = raw[i] + top = stack[-1] if stack else None + unquoted = '"' not in stack + if top == "'": + if ch == "'": + stack.pop() + elif ch == "\\": + i += 1 + elif top == '"': + if ch == '"': + stack.pop() + elif raw.startswith("$(", i): + stack.append("(") + i += 1 + elif ch == "`": + stack.append("`") + elif ch in "'\"": + stack.append(ch) + elif (top == "(" and ch == ")") or (top == "`" and ch == "`"): + stack.pop() + elif top in ("{", "{,") and ch == "}": + if stack.pop() == "{," and unquoted: + return True + elif top == "{" and (ch == "," or raw.startswith("..", i)): + stack[-1] = "{," + elif ch == "{" and raw[i - 1 : i] != "$": + stack.append("{") + elif unquoted and not braces_only and ch in "$`": + return True + elif raw.startswith("$(", i): + stack.append("(") + i += 1 + elif ch == "`": + stack.append("`") + i += 1 + return False + + +def _split_free(x, toks, mask, raw): + """True when word splitting cannot split `toks[x]`, read from the quote-keeping token where known. + + Where that token is unknown, a token holding an expansion is split-free only where it opens + with `$(` and ends with the `)` closing it. The operator lex and `_context_lex` each split an + unquoted `$(` at its parenthesis, and the token union `_shell_tokens` returns then holds a + reading that denies it. Any other spelling, such as `"a"$Y`, is read as one that may split. + """ + if raw is not None: + return not _expands_unquoted(raw[x]) + tok = toks[x] + return not _expands_unquoted(tok) or ( + len(tok) > 3 and tok.startswith("$(") and tok.endswith(")") + ) + + +def _rewritten_git_words(cmd): + """Name the form when a git command holds a rewritten word where it can change what git runs, else None. + + The guard reads each word as written, while the shell expands a `$`, a backtick, or a brace + word and drops a comment before git reads any of them. Modeling either is open-ended, so the + command fails closed. + + - a run-time word where the subcommand goes, on any git command, since it may expand to `push` or + to an option that moves the subcommand, as a backtick word ending the global options does + - a run-time word that word splitting may split among any git command's global options, or + before no subcommand word at all, since `git -c $X README.md` may expand to a commit under an + override, and a brace expansion in a directory option's value + - a redirection where the subcommand goes or among the global options, a file-descriptor number + or a `{name}` before it included, since the readings below then see no subcommand or pair an + option with the wrong value, unless git's first word is an option such as `--version` that + ends git there + - a run-time word among a push's or a commit's global options, since `-c "$X"` may set + `core.hooksPath` + - a run-time word among a push's arguments, since it may name any refspec or remote + - a run-time word among a commit's arguments, since it may expand to `--no-verify` + - a redirection target spanning several tokens on a push or a commit, since the push and + bypass readings stop at its parenthesis and every later word goes unread + - a comment among a push's words, since the shell drops what the guard reads as a refspec or a + flag there, which can turn a bare push into one naming another branch + + A comment on a commit only adds words the guard reads, which can only add a deny, so it is left + alone. Four kinds of run-time word are left alone too. One is the value of a global option + naming a directory before a subcommand word, unquoted too, since `-C $dir` is the common + spelling. What word splitting makes of an unquoted one is not read, and a brace expansion in + one is denied. One is a commit + option's value whose expansions sit inside double quotes, since word splitting cannot then make + it an option, as in `-m "$(cat <<'EOF' ...)"`. One is a commit path after `--`. One is a + redirection target held in one token, such as `2>"$LOG"`. + Only a git word in a command's position counts, so a `grep git "$dir"` is no git command, and + that test is `_runs_as_command`'s. A git word right after an unquoted `(` counts too, since a + process substitution such as `<(git ...)` runs it whatever command reads the result. + """ + for text in [cmd, *_embedded_wrapper_commands(cmd)]: + toks = _shell_tokens(text) + mask = raw = None + masked = False + for k, t in enumerate(toks): + if not _is_git_exe(t): + continue + if not masked: + mask, raw, masked = _quoted_mask(text, toks), _quote_kept_tokens(text, toks), True + end = _region_end(toks, k, mask) + j = min(_git_subcommand_index(toks, k), end) + if j == k + 1 and j >= end: + continue + opened = k and toks[k - 1].endswith("(") and not (mask and mask[k - 1]) + if not opened and not _runs_as_command(toks, k, mask): + continue + if j >= end: + if any( + _is_run_time_word(toks[x]) and not _split_free(x, toks, mask, raw) + for x in range(k + 1, end) + ): + return "a run-time word among the options of a `git` with no subcommand word" + continue + sub = toks[j] + if _is_run_time_word(sub): + return f"a run-time word, `{sub}`, where the subcommand of `git` goes" + redirected = _is_redir_op(sub) or ( + (sub.isdigit() or _NAMED_FD_RE.match(sub)) + and j + 1 < end + and _is_redir_op(toks[j + 1]) + ) + redirected = redirected or any(_is_redir_op(toks[x]) for x in range(k + 1, j)) + if redirected and toks[k + 1] not in _GIT_TERMINAL_OPTS: + return ( + "a redirection where the subcommand of `git` goes or among its global options" + ) + for x in range(k + 1, j): + word = toks[x] + if not _is_run_time_word(word): + continue + if toks[x - 1] in _GIT_DIR_VALUE_OPTS or word.startswith( + ("--git-dir=", "--work-tree=") + ): + if ( + _expands_unquoted(raw[x], braces_only=True) + if raw is not None + else _BRACE_EXPANSION.search(word) + ): + return f"a brace expansion, `{word}`, among the global options of `git`" + continue + if not _split_free(x, toks, mask, raw): + return f"a run-time word, `{word}`, that may split among the global options of `git`" + if sub not in ("push", "commit"): + continue + if sub == "push" and any( + toks[x].startswith("#") and not (mask and mask[x]) for x in range(k + 1, end) + ): + return "a comment among the words of `git push`" + x = k + 1 + while x < j: + opt = toks[x] + if opt in _GIT_DIR_VALUE_OPTS and x + 1 < j and _span_end(toks, x + 1, j) == x + 2: + x += 2 + continue + if not opt.startswith(("--git-dir=", "--work-tree=")) and _is_run_time_word(opt): + return f"a run-time word, `{opt}`, among the global options of `git {sub}`" + x += 1 + x = j + 1 + after_dashes = False + while x < end: + arg = toks[x] + if _is_redir_op(arg): + x += 1 + if x < end and not _is_shell_op(toks[x]): + if _span_end(toks, x, end) != x + 1: + return ( + f"a redirection target spanning several words among the words of " + f"`git {sub}`" + ) + x += 1 + continue + if sub == "commit": + after_dashes = after_dashes or arg == "--" + if after_dashes: + x += 1 + continue + if ( + _is_commit_value_option(arg) + and x + 1 < end + and _split_free(x + 1, toks, mask, raw) + ): + x += 2 + continue + if _holds_attached_value(arg) and _split_free(x, toks, mask, raw): + x += 1 + continue + if _is_run_time_word(arg): + return f"a run-time word, `{arg}`, among the arguments of `git {sub}`" + x += 1 + return None + + def _push_invocations(cmd): """Every `git push` as (c_dirs, work_tree, leading_cd, cd_unreliable, args), wrappers included. @@ -4193,10 +4530,19 @@ def classify( dec, reason = _check_bypass_flags(cmd) if dec == "deny": return dec, reason + # A word the shell rewrites before git reads it can hide a push target or a commit bypass, so it fails closed. + rewritten = _rewritten_git_words(cmd) + if rewritten: + return "deny", ( + f"This command holds {rewritten}, which the shell rewrites or drops before git reads " + "it, so the guard cannot tell what the command runs. Failing closed. Run the push or " + "commit as its own command with every word written out, and a push with an explicit " + "refspec (git push origin :refs/heads/)." + _handoff(cmd) + ) # The `_push_targets` helper tokenizes with shlex and keys off a real `git push` argv adjacency, so a push named only inside a quoted argument yields no target. # The raw substring is just a cheap pre-filter. - if _GIT_PUSH.search(cmd) or _GIT_INCLUDE.search(cmd): - if split_word and _GIT_PUSH.search(cmd): + if _names_push(cmd) or _names_include(cmd): + if split_word and _names_push(cmd): return "deny", ( "This push sits in a command with a word split by a backslash-newline, which the shell joins " "into one word the guard cannot read, so a hidden cd could redirect the push. Write each " @@ -5696,6 +6042,349 @@ def classify( "allow", "control: an --exec-path with no include pulls in no alias", ), + ( + "git p''ush origin develop", + None, + {"develop": _CODE_RULES}, + "deny", + "a push word split by quotes is read as the push the shell joins it into", + ), + ( + 'git "pu"sh origin develop', + None, + {"develop": _CODE_RULES}, + "deny", + "a push word split by a double-quoted part is read the same way", + ), + ( + "GIT push origin develop", + None, + {"develop": _CODE_RULES}, + "deny", + "an uppercase GIT reaches the push check, as a case-insensitive filesystem resolves it to git", + ), + ( + "git -c 'inc''lude.path=/example/config' p", + "feature/x", + {"feature/x": set()}, + "deny", + "an include key split by quotes reaches the include check", + ), + ( + "eval \"git p''ush origin develop\"", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a quote-split push inside an eval argument is still a push the guard does not read", + ), + ( + "git push origin $(echo develop)", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a substitution as a refspec is a run-time word the guard cannot read", + ), + ( + "git push origin `echo develop`", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a backtick refspec is a run-time word too", + ), + ( + "B=develop; git push origin $B", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a variable refspec is a run-time word too", + ), + ( + 'git push origin "$B"', + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a quoted variable refspec still expands at run time", + ), + ( + "git push origin dev${X}elop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "an empty expansion inside a refspec is a run-time word", + ), + ( + "git push origin $'develop'", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "an ANSI-C quoted refspec is a run-time word", + ), + ( + "bash -c 'git push origin \"$1\"' _ develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a positional parameter as a refspec in a wrapper body is a run-time word", + ), + ( + "git $(echo push) origin develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a substitution where the subcommand goes may expand to push", + ), + ( + "git push # feature/y", + "develop", + {"develop": _CODE_RULES, "feature/y": set()}, + "deny", + "a comment's words are not read as the refspecs of a bare push of develop", + ), + ( + "git -c core.hooksPath=/dev/null `true` commit -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "a backtick word ending the global options does not hide a hooks-path override on a commit", + ), + ( + "git commit $(echo --no-verify) -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "a substitution among a commit's arguments may expand to --no-verify", + ), + ( + "git commit -m $MSG", + "feature/x", + {"feature/x": set()}, + "deny", + "an unquoted commit message variable may split into more options", + ), + ( + 'git -c "$X" commit -m x', + "feature/x", + {"feature/x": set()}, + "deny", + "a run-time config override on a commit may set core.hooksPath", + ), + ( + 'git -C "$W" push origin feature/x', + None, + {"feature/x": set()}, + "allow", + "control: a run-time directory value leaves an explicit push readable", + ), + ( + "git commit -m \"$(cat <<'EOF'\nFix the example's edge\n\nCo-Authored-By: Example \nEOF\n)\"", + "feature/x", + {"feature/x": set()}, + "allow", + "control: a double-quoted heredoc commit message cannot split into options", + ), + ( + 'git commit -m "$(cat <<\'EOF\'\nQuote " once\nEOF\n)"', + "feature/x", + {"feature/x": set()}, + "allow", + "control: a heredoc commit message holding one double quote is still read as quoted", + ), + ( + 'git commit -m "$MSG" -- "$F"', + "feature/x", + {"feature/x": set()}, + "allow", + "control: a quoted message variable and a path after -- leave a commit readable", + ), + ( + 'git commit -m "fix" # note', + "feature/x", + {"feature/x": set()}, + "allow", + "control: a comment on a commit only adds words the guard reads", + ), + ( + 'grep -rn git "$W"', + "feature/x", + {"feature/x": set()}, + "allow", + "control: git as an argument of grep is no git command", + ), + ( + 'git push origin feature/x 2>&1 | tee "$LOG"', + None, + {"feature/x": set()}, + "allow", + "control: a run-time word in a command the push pipes into is not a refspec", + ), + ( + "Y=' --no-veri'fy; git commit -m \"a\"$Y", + "feature/x", + {"feature/x": set()}, + "deny", + "an unquoted expansion after a quoted part of a commit message may split into more options", + ), + ( + "X='core.hooksPath=/dev/null commit -m x'; git -c $X", + "feature/x", + {"feature/x": set()}, + "deny", + "an unquoted option value on a git with no subcommand word may split into a commit", + ), + ( + "git commit --mess -- $(printf -- --no-veri%s fy)", + "feature/x", + {"feature/x": set()}, + "deny", + "an abbreviated value option takes a following -- as its value, so later words are options", + ), + ( + "git {push,origin} develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a brace expansion where the subcommand goes may expand to push", + ), + ( + "git commit -m x {--no-verify,--quiet}", + "feature/x", + {"feature/x": set()}, + "deny", + "a brace expansion among a commit's arguments may expand to --no-verify", + ), + ( + "git push origin feature/x > $(printf a) develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a redirection target spanning several words hides the push's later refspecs", + ), + ( + "git push origin feature/x < <(printf a) develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a process substitution redirection hides the push's later refspecs too", + ), + ( + 'git push origin feature/x 2>"$LOG"', + None, + {"feature/x": set()}, + "allow", + "control: a run-time redirection target held in one word is not a refspec", + ), + ( + "git commit -m {x,'--no-veri'fy}", + "feature/x", + {"feature/x": set()}, + "deny", + "a brace expansion holding a quoted part in a commit value still gives more words", + ), + ( + "X='core.hooksPath=/dev/null commit -m x --'; git -c $X README.md", + "feature/x", + {"feature/x": set()}, + "deny", + "an unquoted config value may split into the subcommand, whatever literal word follows", + ), + ( + "git -c {core.hooksPath=/dev/null,commit} -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "a brace expansion in a config value may give the subcommand", + ), + ( + "git -C {.,push} origin develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a brace expansion in a directory value may give the subcommand", + ), + ( + "git -c core.hooksPath=/dev/null > a commit -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "a redirection where the subcommand goes hides a commit under an override", + ), + ( + 'git -C "$W" -c user.name="$N" status', + "feature/x", + {"feature/x": set()}, + "allow", + "control: quoted run-time values before a read-only subcommand cannot split", + ), + ( + "git 2>/dev/null push origin develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a numbered redirection where the subcommand goes hides a push", + ), + ( + "git -c core.hooksPath=/dev/null 2>/dev/null commit -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "a numbered redirection where the subcommand goes hides a commit under an override", + ), + ( + "git --version >/dev/null 2>&1", + "feature/x", + {"feature/x": set()}, + "allow", + "control: a redirection after --version, which ends git before any subcommand", + ), + ( + "git --namespace -h > a commit --no-verify -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "a terminal option taken as another option's value does not end git", + ), + ( + "git {fd}>/dev/null push origin develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a named file descriptor redirection where the subcommand goes hides a push", + ), + ( + "git -c > a core.hooksPath=/dev/null commit -m x", + "feature/x", + {"feature/x": set()}, + "deny", + "a redirection among the global options pairs an option with the word after its target", + ), + ( + "git -C > a . push origin develop", + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a redirection after -C leaves the push readable only to the shell", + ), + ( + 'git push origin feature/x ")" $(echo develop)', + "feature/x", + {"feature/x": set(), "develop": _CODE_RULES}, + "deny", + "a quoted parenthesis is a word, so the scan reaches the substitution after it", + ), + ( + 'git commit -m ")" -m ";"', + "feature/x", + {"feature/x": set()}, + "allow", + "control: quoted operator characters as commit message values are words", + ), + ( + "(git push origin feature/x)", + None, + {"feature/x": set()}, + "allow", + "control: a subshell's closing parenthesis ends the push", + ), ] # (command, cwd, {dir: is_primary_or_None}, {(dir, ref): resolves_as_ref} or None, expected_decision, label) -- is_primary is True (a primary checkout), False (a linked worktree), or None (unresolved, for example when no repository exists or the git query itself fails, such as on a pre-2.31 git lacking `rev-parse --path-format`). diff --git a/host-setup/agent-safety/claude/install.py b/host-setup/agent-safety/claude/install.py index 2802cf118..7299bc2de 100755 --- a/host-setup/agent-safety/claude/install.py +++ b/host-setup/agent-safety/claude/install.py @@ -212,8 +212,9 @@ def at(data, path): # Ownership needs a delimiter after the prefix, so a longer path such as `pr_review.py-custom` is not claimed. # These widen rather than restrict, so they stay their own step for the reason the two CLAUDE.md blocks stay separate. MANAGED_PERMISSIONS = [ - # The review loop's reply and resolve, the one write in that loop an agent performs. - # Driving it by hand needs a raw GraphQL mutation carrying a node id, which is the shape to avoid. + # The review loop's reply and resolve, and the `cleanup` delete of a merged pull request's head branch. + # Driving either by hand needs a raw mutation carrying an id, which is the shape to avoid. + # The `:*` rule approves any arguments, so every check the delete makes lives inside the `cleanup` subcommand. # The rule decides which command skips a prompt, and it bounds no checkout, since it matches the text. ("Bash(python3 scripts/pr_review.py", "Bash(python3 scripts/pr_review.py:*)"), ]