ci: make reviewer merge-holds stick against auto-merge re-arming - #4454
ci: make reviewer merge-holds stick against auto-merge re-arming#4454dieterolson wants to merge 2 commits into
Conversation
Fleet automation re-arms auto-merge on PRs a reviewer disarmed (measured at 6-8s in Gasification_Model), so a manual disarm cannot hold a PR back. Adds a guard that revokes auto-merge on held PRs, refuses PRs deleting tracked files without acknowledgement, and converts to draft after repeated re-arms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 374531e051
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif printf '%s' "$BODY" | grep -qiE '^[[:space:]]*Deletions-Acknowledged:[[:space:]]*(yes|true)'; then | ||
| echo "PR #$PR deletes $REMOVED_COUNT file(s) — acknowledged in the PR body." |
There was a problem hiding this comment.
When a deletion PR is opened with Deletions-Acknowledged: yes already in its author-editable description, this branch suppresses the deletion hold and the guard never applies do-not-merge. An untrusted PR author can therefore opt their own destructive change out of the safeguard before the first run; require reviewer-controlled state such as the acknowledgment label or an approval from an authorized actor instead.
Useful? React with 👍 / 👎.
| gh api "repos/$REPO/pulls/$PR/files?per_page=100" --paginate \ | ||
| --jq '.[] | select(.status == "removed") | .filename' \ | ||
| > "$REMOVED_FILE" 2>/dev/null || : > "$REMOVED_FILE" |
There was a problem hiding this comment.
When the pull-files API returns a transient error, permission error, or rate-limit response, || : > "$REMOVED_FILE" converts that failure into an empty successful result. An armed deletion PR with no other hold signal is then reported as safe and left armed, potentially merging before the next 30-minute sweep; treat this API failure as an enforcement failure rather than as zero deleted files.
Useful? React with 👍 / 👎.
| LAST_DISARM="$(grep -v '^Bot ' "$TIMELINE" | cut -f2 | sort | tail -1)" | ||
| HEAD_DATE="$(gh api "repos/$REPO/commits/$HEAD_SHA" \ | ||
| --jq '.commit.committer.date' 2>/dev/null)" || HEAD_DATE="" | ||
| if [ -n "$LAST_DISARM" ] && [ -n "$HEAD_DATE" ] && [[ "$LAST_DISARM" > "$HEAD_DATE" ]]; then | ||
| add_reason "a reviewer disabled auto-merge at $LAST_DISARM, after the head commit ($HEAD_DATE) — no push has superseded that decision" |
There was a problem hiding this comment.
When a contributor pushes or force-pushes an existing commit whose committer date predates the reviewer's disarm, the head SHA changes but this cutoff remains older than the disarm. The workflow therefore concludes that no push superseded the decision and can also count revocations from before the new head, so the documented clearing behavior does not work and the refreshed PR may be converted to draft; track the synchronize/push ordering or associate the disarm with the observed head SHA instead.
Useful? React with 👍 / 👎.
`do-not-automate` is already the fleet-wide convention for "this work must not be automated" (shared_scripts/agent_identity.DO_NOT_AUTOMATE_LABEL). Honouring it here keeps one vocabulary instead of a parallel one, and collapses the per-label checks into a single list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Problem
Fleet automation re-arms GitHub auto-merge on pull requests that a reviewer has deliberately disarmed, which defeats the only lightweight mechanism available for holding a dangerous PR back.
Measured in
Gasification_Modelon 2026-08-14 (all events attributed todieterolson,type=User, i.e. the owner's credentials rather than a bot):PR #4709 is why this matters rather than merely annoying: it had auto-merge armed on a diff that deleted 13 files present on
mainand grewSPEC.mdfrom 5,084 to 60,863 lines.The re-armer is not a workflow in this repository. No workflow in
Gasification_Model,Tools,Tools_Private,Drake_Models,MuJoCo_Models,ControlsorMaxwell_Daemoncallsgh pr merge --autoorenablePullRequestAutoMerge. The arming comes from agent sessions running under the owner'sghcredentials, driven by thefleet-pr-queueautomation inRepository_Management/config/codex_fleet_automations.json, whose prompt instructs them to "enable auto-merge for the highest-confidence item". Because those sessions live outside any single repo, a repo-side guard is the only enforcement point that cannot be bypassed by editing an agent prompt.What this adds
.github/workflows/Merge-Hold-Guard.yml. It only ever removes auto-merge — it never merges, never arms, never pushes.Hold signals (any one is sufficient):
do-not-mergeorblockedlabeldeletions-acknowledgedlabel, or aDeletions-Acknowledged: yesline in the PR body)Signal 3 deliberately ignores bot actors so the guard's own revocations can never manufacture a hold, and a genuine push clears the hold naturally.
Triggers:
pull_request_targetincluding theauto_merge_enabledactivity type, so a held PR is disarmed seconds after any re-arm; plus a20,50 * * * *sweep over armed PRs as a backstop, offset off the hour so it does not collide with the fleet pass.Escalation. A plain revoke cannot win a 6-second re-arm race on its own. After 2 revocations against the same head commit, the guard converts the PR to a draft. This was verified empirically against
Tools#4453:GitHub refuses to arm auto-merge on a draft, so the hold becomes enforceable at the API level rather than advisory.
gh pr ready <n>reverses it deliberately.The guard also applies
do-not-mergeto deletion PRs, giving label-aware automation a signal it can honour before ever attempting to arm.Labels
do-not-mergeanddeletions-acknowledgeddid not exist in this repo (gh pr edit --add-label do-not-mergefailed with'do-not-merge' not found). Both have been created, and the workflow re-creates them if missing so it stays drop-in for new repos.Verification
The detection logic was dry-run against real PRs before this workflow was written. On #4709 it reports:
and on #4692/#4710/#4711 it correctly identifies all three as held with auto-merge currently armed. No false positives were found: every open PR in this repo is armed, and the only ones flagged are those the reviewer had disarmed.
YAML parses, all three
run:blocks passbash -n, and the file satisfies this repo's ownlint-workflow-files.ymlrules (concurrency,cancel-in-progress: true, per-jobtimeout-minutes, no hosted runners, no protected-branch push).Companion change
Repository_Managementneeds the arming side fixed too, so agents stop attempting the arm in the first place rather than relying on the guard to undo it.