Read a Closed Read-Back as Unmerged in pr_review.py merge

scripts/README.md

@@ -285,7 +285,7 @@ uv run --no-project --script scripts/pr_review.py merge 123 --repo ptr727/Projec
 
 Claude Code's auto-mode classifier refuses a hand-typed `gh pr merge --squash` on a pull request whose gate is met. It refuses most often inside a subagent, which sees neither the review nor the maintainer's go-ahead. A command matching the allow rule is settled before that classifier runs, so every check the merge needs lives here. Whether the merge is authorized stays with the skill that runs it. What the code checks is the Merge Gate's mechanically readable items, and that the merge is current, in the owner, and into `develop` only. The items no code reads, such as a finding the pull request's body lists, stay the driver's per `pr-review-conduct`'s Merge Gate.
 
-Before any read it runs the write-scope check `cleanup` runs, so a `--cleanup` after a landed merge in a registered repository cannot refuse on scope. It reads the pull request live and refuses unless it is open, not a draft, based on `develop`, and headed in its own repository. A `main` base is refused by name, since a promotion merge belongs to `merge-and-release` with the maintainer's go-ahead. A permanent head branch, `develop`, `main`, or the default branch, is refused, and those are the only branches the fleet's rulesets cover. So a `main -> develop` pull request is never squashed into `develop`. It records the head, then reads the gate through `wait`'s own decision, so the two cannot drift apart. That read runs on a short bound and sends no review request, since `wait` is the step that waits and requests. A gate not met prints `wait`'s status line and returns its code unchanged. A `wait` exit of `0` does not decide every item, so a fresh read follows. It refuses unless coverage reads `full`, `table`, `carried:table`, or `local`, and no thread is unresolved, every page walked. Each count the digest builds of a finding no thread carries has to read zero. Those are `suppressed`, `previously_missed`, `cr_outside_diff`, `qodo_open`, and `FINDINGS WITH NO THREAD`. The code reads the fields the digest builds rather than its printed line. A count field the digest did not build refuses as `COUNT_UNREAD` rather than reading as zero. So does one holding no number, such as an `overview` count of `?`, and one a cut reviews window leaves a floor, such as `0+`. A count built as no value reads zero only where its source is absent. That is a `previously_missed`, `cr_outside_diff`, or `qodo_open` with nothing to count, or an `overview` where no round covers the head. A `shapes=HELD` refuses as `UNPLACED_PARTS_UNACKNOWLEDGED` until its acknowledgment posts. A head round with no overview marker still refuses after it, as `COUNT_UNREAD` on `overview`, since item 3 reads no count there. A part under `REVIEWER OUTPUT NOTED` refuses as `OUTPUT_NOTED`, since that block is read by hand and nothing here reads the answer. A Copilot review request still pending refuses as `REVIEW_PENDING`. A request to a human or team reviewer is the driver's per `pr-review-conduct`. A `--checkout` that is not the primary clone of the target is refused before any merge. `cleanup`'s worktree and tip checks still run after it, so they can refuse once the merge has landed. The merge state has to read `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`, the states `wait` treats as finished. Each unmet item prints its own `status=` line. Some of those counts span every round, such as `suppressed`, and nothing here reads an answer posted to one. So a pull request carrying any such finding, answered or not, is refused here and reported. `FINDINGS WITH NO THREAD` reads the head's round alone, so an earlier round's no-thread finding stays the driver's. The head is read again after the gate, and a head that moved refuses as `HEAD_MOVED` before any unmet item prints. A merge `gh` refused on a head that moved prints `HEAD_MOVED` too. The merge runs as `gh pr merge <number> --squash --match-head-commit <head>`, never with `--admin`, `--auto`, or `--delete-branch`. Its output is printed whole. Reading the pull request back decides the outcome. A pull request read back merged at a head other than the gated one prints `status=PEER_MERGED` at exit `1`. This run merged nothing, so it runs no cleanup itself. A merge `gh` refused that reads back open is a refusal. A call that timed out or never ran, or a read back that stays ambiguous, may still have landed on the server. `--cleanup` runs `cleanup` on the same pull request once the merge is confirmed, with `--checkout` passed through to it, and its exit is the command's.
+Before any read it runs the write-scope check `cleanup` runs, so a `--cleanup` after a landed merge in a registered repository cannot refuse on scope. It reads the pull request live and refuses unless it is open, not a draft, based on `develop`, and headed in its own repository. A `main` base is refused by name, since a promotion merge belongs to `merge-and-release` with the maintainer's go-ahead. A permanent head branch, `develop`, `main`, or the default branch, is refused, and those are the only branches the fleet's rulesets cover. So a `main -> develop` pull request is never squashed into `develop`. It records the head, then reads the gate through `wait`'s own decision, so the two cannot drift apart. That read runs on a short bound and sends no review request, since `wait` is the step that waits and requests. A gate not met prints `wait`'s status line and returns its code unchanged. A `wait` exit of `0` does not decide every item, so a fresh read follows. It refuses unless coverage reads `full`, `table`, `carried:table`, or `local`, and no thread is unresolved, every page walked. Each count the digest builds of a finding no thread carries has to read zero. Those are `suppressed`, `previously_missed`, `cr_outside_diff`, `qodo_open`, and `FINDINGS WITH NO THREAD`. The code reads the fields the digest builds rather than its printed line. A count field the digest did not build refuses as `COUNT_UNREAD` rather than reading as zero. So does one holding no number, such as an `overview` count of `?`, and one a cut reviews window leaves a floor, such as `0+`. A count built as no value reads zero only where its source is absent. That is a `previously_missed`, `cr_outside_diff`, or `qodo_open` with nothing to count, or an `overview` where no round covers the head. A `shapes=HELD` refuses as `UNPLACED_PARTS_UNACKNOWLEDGED` until its acknowledgment posts. A head round with no overview marker still refuses after it, as `COUNT_UNREAD` on `overview`, since item 3 reads no count there. A part under `REVIEWER OUTPUT NOTED` refuses as `OUTPUT_NOTED`, since that block is read by hand and nothing here reads the answer. A Copilot review request still pending refuses as `REVIEW_PENDING`. A request to a human or team reviewer is the driver's per `pr-review-conduct`. A `--checkout` that is not the primary clone of the target is refused before any merge. `cleanup`'s worktree and tip checks still run after it, so they can refuse once the merge has landed. The merge state has to read `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`, the states `wait` treats as finished. Each unmet item prints its own `status=` line. Some of those counts span every round, such as `suppressed`, and nothing here reads an answer posted to one. So a pull request carrying any such finding, answered or not, is refused here and reported. `FINDINGS WITH NO THREAD` reads the head's round alone, so an earlier round's no-thread finding stays the driver's. The head is read again after the gate, and a head that moved refuses as `HEAD_MOVED` before any unmet item prints. A merge `gh` refused on a head that moved prints `HEAD_MOVED` too. The merge runs as `gh pr merge <number> --squash --match-head-commit <head>`, never with `--admin`, `--auto`, or `--delete-branch`. Its output is printed whole. Reading the pull request back decides the outcome. A pull request read back merged at a head other than the gated one prints `status=PEER_MERGED` at exit `1`. This run merged nothing, so it runs no cleanup itself. A merge `gh` refused that reads back open is a refusal. A pull request that reads back closed is one too, whatever `gh` answered, since a closed pull request cannot merge later. A call that timed out or never ran, or a read back that stays ambiguous, may still have landed on the server. `--cleanup` runs `cleanup` on the same pull request once the merge is confirmed, with `--checkout` passed through to it, and its exit is the command's.
 
 With `--cleanup`, a `status=MERGED` line means the merge landed, and the exit is then `cleanup`'s own. Otherwise exit `0` is merged and confirmed. Exit `1` is a run that merged nothing. That is a refusal, a moved head, an unmet item, a registry refusal, or a merge `gh` refused. It is also a pull request another hand already merged, which still owes cleanup. Exit `2` is a read that did not answer or a usage error. It is also a merge call the read back could not settle, which may have happened on the server. Exit `64` is the owner check. Any other code is the gate's, from the `pr_review.py` exit codes above, and nothing was merged.
 

scripts/pr_merge.py

@@ -214,11 +214,12 @@ def merge(owner: str, repo: str, num: int, cleanup: bool, checkout: str | None)
             return 1
         print(f"status=MERGED pr={num} head={head} merge_commit={commit}, confirmed on read back")
         return pr_cleanup.cleanup(owner, repo, num, checkout) if cleanup else 0
-    if answered not in (None, 0) and state == "OPEN":
+    if state == "CLOSED" or (answered not in (None, 0) and state == "OPEN"):
         status = "MERGE_REFUSED" if (now := landed.get("headRefOid")) == head else "HEAD_MOVED"
+        status = "REFUSED" if state == "CLOSED" else status
         print(
-            f"status={status} gh refused the merge of #{num} on {target} with exit "
-            f"{answered}, and it reads OPEN at {now}, so nothing was merged"
+            f"status={status} #{num} on {target} reads {state} at {now} after gh pr merge "
+            f"answered {'nothing' if answered is None else answered}, so nothing was merged"
         )
         return 1
     print(

tests/test_pr_merge.py

@@ -306,7 +306,10 @@ def test_a_failed_merge_call_read_back_merged_is_done(self) -> None:
     def test_a_merge_gh_refused_that_reads_back_open_is_refused(self) -> None:
         self.merge_rc = 1
         self.assertEqual(1, self.run_merge(target(), target(), target()))
-        self.assertIn("status=MERGE_REFUSED gh refused the merge of #123", self.out.getvalue())
+        self.assertIn(
+            f"status=MERGE_REFUSED #123 on example/repo reads OPEN at {HEAD} after gh pr merge answered 1",
+            self.out.getvalue(),
+        )
         self.assertIn("so nothing was merged", self.out.getvalue())
         self.cleanup.assert_not_called()
 
@@ -323,6 +326,22 @@ def test_a_merge_call_that_never_finished_is_unconfirmed_rather_than_refused(sel
         self.assertIn("status=MERGE_NOT_CONFIRMED", self.out.getvalue())
         self.cleanup.assert_not_called()
 
+    def test_a_read_back_closed_is_a_refusal_rather_than_unconfirmed(self) -> None:
+        for rc, err, said in ((0, "", "0"), (1, "", "1"), (1, pr_merge.UNRUN, "nothing")):
+            with self.subTest(rc=rc, err=err):
+                self.calls.clear()
+                self.out.seek(0)
+                self.out.truncate()
+                self.merge_rc, self.merge_err = rc, err
+                self.assertEqual(1, self.run_merge(target(), target(), target(state="CLOSED")))
+                self.assertIn(
+                    f"status=REFUSED #123 on example/repo reads CLOSED at {HEAD} after gh pr "
+                    f"merge answered {said}, so nothing was merged",
+                    self.out.getvalue(),
+                )
+                self.assertNotIn("MERGE_NOT_CONFIRMED", self.out.getvalue())
+                self.cleanup.assert_not_called()
+
     def test_a_read_back_that_does_not_answer_says_the_merge_may_have_happened(self) -> None:
         unread = pr_merge.pr_cleanup.shared.GraphQLError("gh graphql failed rc=1")
         self.assertEqual(2, self.run_merge(target(), target(), unread))
@@ -567,7 +586,7 @@ def test_a_head_moved_is_reported_before_any_unmet_item(self) -> None:
     def test_a_merge_gh_refused_on_a_moved_head_reads_head_moved(self) -> None:
         self.merge_rc = 1
         self.assertEqual(1, self.run_merge(target(), target(), target(headRefOid=MOVED)))
-        self.assertIn("status=HEAD_MOVED gh refused the merge of #123", self.out.getvalue())
+        self.assertIn("status=HEAD_MOVED #123 on example/repo reads OPEN", self.out.getvalue())
         self.assertIn(f"reads OPEN at {MOVED}", self.out.getvalue())
         self.cleanup.assert_not_called()