|
| 1 | +# Blocking a merge from the PR itself |
| 2 | + |
| 3 | +`AGENTS.md` section 6, step 3 owns the rule: when firstmate's review of an already-open PR finds a merge-stopping defect, the verdict goes on the PR, not only into the chat. |
| 4 | +This doc owns the mechanics, the per-repo setup, and the verification record behind them. |
| 5 | + |
| 6 | +## The incident this exists for |
| 7 | + |
| 8 | +On 2026-07-30, PR #44 in this repo (`webjema/firstmate`) merged carrying seven confirmed defects while its crew was still fixing them. |
| 9 | +An independent review had found all seven and firstmate had told the user not to merge. |
| 10 | +The verdict existed only in that firstmate session's chat, so the person clicking merge acted without it. |
| 11 | +One of the seven bricks a worktree pool slot on every warm, on Linux as well as macOS. |
| 12 | + |
| 13 | +The generalizable failure is not "the review was late" - the review was on time and correct. |
| 14 | +It is that a review verdict living only in chat does not reach the person who merges, including a collaborator who never sees that session at all. |
| 15 | +Firstmate's normal review sits before the PR exists, which keeps this rare, but once a PR is open the chat is no longer where the merge decision happens. |
| 16 | + |
| 17 | +An advisory mark - a title prefix, or a comment on its own - is what already failed: it informs a reader who looks, and the merge button stays live for one who does not. |
| 18 | +Prefer the mechanism that blocks. |
| 19 | + |
| 20 | +## firstmate-authored PRs |
| 21 | + |
| 22 | +Convert the PR to draft, which disables merging outright - GitHub's own docs put it as "No one can merge the pull request until you mark the pull request as ready for review again" - and comment the verdict so the reason travels with the PR: |
| 23 | + |
| 24 | +```sh |
| 25 | +gh pr ready --undo <pr-url> # blocks the merge button |
| 26 | +gh pr comment <pr-url> --body-file verdict.md # carries the reason |
| 27 | +``` |
| 28 | + |
| 29 | +Restore it with `gh pr ready <pr-url>` once the fix lands and the re-review is clean. |
| 30 | + |
| 31 | +`--undo` is plan-gated (`gh pr ready --help`: "If supported by your plan"). |
| 32 | +It fails loudly - a GitHub API error and a non-zero exit - never silently, so treat a non-zero exit as the signal to fall back to the bot-authored path below on the same PR. |
| 33 | + |
| 34 | +Two things hold while a PR sits drafted: |
| 35 | + |
| 36 | +- PR CI keeps running on every push, so the fix loop keeps its gate (see the verification record). |
| 37 | +- `bin/fm-pr-merge.sh` fails closed, because GitHub refuses to merge a draft PR at the API. That is defense in depth, not a bug to route around: mark the PR ready first, then merge on the user's word. |
| 38 | + |
| 39 | +## Bot-authored PRs |
| 40 | + |
| 41 | +Never draft a bot's PR - Dependabot and similar bots act on their own PR's state, and drafting can interfere with that handling. |
| 42 | +Use a comment plus a blocking label: |
| 43 | + |
| 44 | +```sh |
| 45 | +gh pr comment <pr-url> --body-file verdict.md |
| 46 | +gh pr edit <pr-url> --add-label do-not-merge |
| 47 | +``` |
| 48 | + |
| 49 | +The label is advisory by itself; make it bite by requiring its absence in the repo's branch protection or merge-queue rules where that is configured. |
| 50 | +Remove it with `gh pr edit <pr-url> --remove-label do-not-merge`. |
| 51 | + |
| 52 | +`do-not-merge` is not a stock GitHub label and does not exist in most repos. |
| 53 | +Create it once per repo, on first use: |
| 54 | + |
| 55 | +```sh |
| 56 | +gh label create do-not-merge --color B60205 --description "Blocking review defect - do not merge" --force |
| 57 | +``` |
| 58 | + |
| 59 | +`--force` updates an existing label instead of failing, so the command is safe to re-run. |
| 60 | + |
| 61 | +Authorship and current draft state both come out of the `gh-axi pr view <pr>` firstmate already reads (`author:` and `draft:` lines); `gh pr view <pr-url> --json author --jq .author.is_bot` is the exact read when the distinction is not obvious from the login. |
| 62 | +Every mark above is a mutation, so it runs on plain `gh`, per section 6's read/mutate split. |
| 63 | + |
| 64 | +## Verification record |
| 65 | + |
| 66 | +Run 2026-07-30 in `webjema/firstmate`, `gh version 2.96.0 (2026-07-02)`, authenticated as `ignovak` with `push` on the repo. |
| 67 | + |
| 68 | +**1. Is `gh pr ready --undo` supported here, and how does it fail when it is not?** |
| 69 | + |
| 70 | +```console |
| 71 | +$ gh repo view webjema/firstmate --json visibility,isPrivate |
| 72 | +{"isPrivate":false,"visibility":"PUBLIC"} |
| 73 | +``` |
| 74 | + |
| 75 | +GitHub's current documentation for changing a PR's stage states no plan or visibility restriction on draft pull requests at all, so `gh`'s "If supported by your plan" caveat is at best conservative. |
| 76 | +This repo is public in any case - the visibility that has always carried draft support - and the underlying mutation is present in the API: |
| 77 | + |
| 78 | +```console |
| 79 | +$ gh api graphql -f query='{ __type(name:"Mutation"){ fields{ name } } }' --jq '.data.__type.fields[].name' | grep -i draft |
| 80 | +convertPullRequestToDraft |
| 81 | +... |
| 82 | +``` |
| 83 | + |
| 84 | +The failure mode is loud, not silent: |
| 85 | + |
| 86 | +```console |
| 87 | +$ gh pr ready --undo 99999 --repo webjema/firstmate |
| 88 | +GraphQL: Could not resolve to a PullRequest with the number of 99999. (repository.pullRequest) |
| 89 | +$ echo $? |
| 90 | +1 |
| 91 | +``` |
| 92 | + |
| 93 | +Not proven directly: a live draft conversion on this repo. |
| 94 | +Doing that needs an open PR, and a crewmate may not open one before firstmate approves its branch. |
| 95 | +The instruction is therefore written to key its fallback on the non-zero exit rather than on an assumption that the plan allows drafts, which makes it correct on either kind of plan. |
| 96 | + |
| 97 | +**2. Does drafting suppress this repo's PR CI?** |
| 98 | + |
| 99 | +No - which is what makes the draft path usable, and this was the check with the power to kill it. |
| 100 | + |
| 101 | +```console |
| 102 | +$ grep -n -A 5 "^on:" .github/workflows/ci.yml |
| 103 | +9:on: |
| 104 | +10- push: |
| 105 | +11- branches: [main] |
| 106 | +12- pull_request: |
| 107 | +13- branches: [main] |
| 108 | +$ grep -n "draft\|if:" .github/workflows/ci.yml |
| 109 | +$ echo $? |
| 110 | +1 |
| 111 | +``` |
| 112 | + |
| 113 | +The `pull_request` trigger carries no `types:` filter, so it uses the default activity types `opened`, `synchronize`, and `reopened`, and the workflow has no draft guard at all - the `if: github.event.pull_request.draft == false` idiom repos use to opt out of draft CI is absent. |
| 114 | + |
| 115 | +GitHub's own event reference does not spell out draft behavior, so this was checked against a live draft PR in a public repo whose workflow has the same trigger shape as ours: |
| 116 | + |
| 117 | +```console |
| 118 | +$ gh api repos/cli/cli/contents/.github/workflows/go.yml -H "Accept: application/vnd.github.raw" | head -6 |
| 119 | +name: Unit and Integration Tests |
| 120 | +on: |
| 121 | + push: |
| 122 | + branches: |
| 123 | + - trunk |
| 124 | + pull_request: |
| 125 | +$ gh pr view 14013 -R cli/cli --json isDraft,statusCheckRollup --jq '{isDraft, total:(.statusCheckRollup|length)}' |
| 126 | +{"isDraft":true,"total":15} |
| 127 | +``` |
| 128 | + |
| 129 | +That PR is a draft and carries 15 completed check runs, `build (ubuntu-latest)` among them, from a bare `pull_request:` trigger. |
| 130 | +So a draft PR does run this shape of workflow, and firstmate's Lint shell scripts, Behavior tests, and Repo invariants all keep running on every fix push while the PR is drafted. |
| 131 | + |
| 132 | +One nuance worth knowing: `converted_to_draft` and `ready_for_review` are not default activity types, so neither drafting the PR nor marking it ready spawns a run by itself. |
| 133 | +CI runs on the fix push, which is exactly when the gate matters. |
| 134 | + |
| 135 | +**3. Does a `do-not-merge` label exist?** |
| 136 | + |
| 137 | +No. |
| 138 | + |
| 139 | +```console |
| 140 | +$ gh label list --repo webjema/firstmate |
| 141 | +bug, documentation, duplicate, enhancement, good first issue, help wanted, invalid, question, wontfix |
| 142 | +``` |
| 143 | + |
| 144 | +Only the nine stock labels. |
| 145 | +The label must be created per repo on first use, with the `gh label create ... --force` command above. |
| 146 | + |
| 147 | +**4. Does the bot-versus-firstmate discriminator actually separate the two paths?** |
| 148 | + |
| 149 | +Yes, on live PRs of both kinds: |
| 150 | + |
| 151 | +```console |
| 152 | +$ gh pr view 44 --repo webjema/firstmate --json author --jq '.author | .login, .is_bot' |
| 153 | +ignovak |
| 154 | +false |
| 155 | +$ gh pr view 177523 --repo home-assistant/core --json author --jq '.author | .login, .is_bot' |
| 156 | +app/dependabot |
| 157 | +true |
| 158 | +``` |
| 159 | + |
| 160 | +`.author.is_bot` is the field to branch on; a bot's `login` also carries the `app/` prefix. |
| 161 | + |
| 162 | +The one step not exercised live is the draft conversion itself, for the reason given under verification 1. |
| 163 | +Any open firstmate PR closes that gap in two commands: `gh pr ready --undo <pr-url>` and then `gh pr ready <pr-url>`, checking that the merge button goes away and comes back. |
| 164 | +If that ever comes back refused, record it here and switch the firstmate path to the label mechanism too. |
0 commit comments