Report review-required policy refusals before Self-merge

CONTEXT.md

@@ -174,7 +174,7 @@ _Avoid_: init, onboarding, configure
 The step at the end of a **Merge run** in which thirdshift itself merges Delivery's captured pull request by number in its repository, with a merge commit at the exact head whose CI it watched. A fresh observation validates its expected **Issue branch** and **Base branch**, open state, readiness and mergeability immediately before the request. GitHub guards the requested head, but offers no atomic expected-base guard against a retarget between that observation and the server merge. No human reviews it first. A failed or interrupted request counts as merged only when a completion observation of the same number confirms the requested head and expected branches. Completion then continues even after Command interruption. It ends with the **Issue branch** deleted, the issue closed by thirdshift, unless it is already closed, and the issue's **Claim** removed.
 
 **Policy refusal**:
-A merge the **Self-merge** tried that failed, where the round of the Repair loop that followed found nothing to fix: the **Base branch** unchanged, no conflict, CI green or absent, the pull request mergeable. The cause is a repository setting or rule, such as merge commits disallowed or a review required. thirdshift never reads GitHub's error text to decide it. The **Merge run** is a **Failed run** that leaves the pull request ready for review.
+A **Self-merge** blocked by a required review, or a merge it tried that failed where the round of the Repair loop that followed found nothing to fix: the **Base branch** unchanged, no conflict, CI green or absent, the pull request mergeable. The cause is a repository setting or rule, such as merge commits disallowed or a review required. thirdshift never reads GitHub's error text to decide it. A blocking review ends the Run as **review required** without a merge attempt or another Repair round. The **Merge run** is a **Failed run** that leaves the pull request ready for review.
 
 **Declined CI fix**:
 A CI-fix **Repair** that found nothing on the branch to fix, for example because the failure is not the branch's but was not recognised as an **Inherited failure**, since the **Base branch** commit had no finished result for that check: once the Repair ends and the **Base branch** is merged in again, the head of the **Issue branch** is still the commit whose CI just failed. That head gets its one **Check re-run** for the **Delivery**; if its own checks are red again, or they cannot be re-run, the **Run** is a **Failed run** with the cause `CI red on <short sha> and the Repair found nothing to fix on the branch`, rather than spending its remaining Repairs. Own checks found red on that head in any later round, including after a refused **Self-merge**, fail with the same cause without another CI-fix Repair or Check re-run. Inherited-only failures keep their usual handling, and a different head may receive its own CI-fix Repair and Check re-run.

README.md

@@ -36,7 +36,7 @@ From a clone of the issue's repository, `thirdshift <Issue URL>`:
 4. Runs a headless agent session in that worktree on the **Harness**, Claude Code unless you [choose another](#harness-model-and-effort), with the **Factory skills** in [`skills/`](skills/) linked into its `.claude/skills/`, or `.agents/skills/` on the other Harnesses, as `thirdshift-<skill>` and kept out of git by a `.git/info/exclude` entry ([ADR-0012](docs/adr/0012-factory-skills-linked-into-the-worktree-codex-unsandboxed.md)), started with one of the **Session prompts** in [`prompts/`](prompts/). The agent implements the issue, reviews its work against the Base branch, addresses the **Standards findings** and **Spec findings** it agrees with, and opens a ready-for-review pull request that lists every **Unaddressed finding** with a reason and closes the issue. With a [Security review](#security-review) asked for, a second session then reviews the change for vulnerabilities.
 5. Takes over deterministically: pushes the Issue branch, skipping your repo's git hooks since the session runs the tests and CI gates the pull request, checks through `gh` that the pull request exists, is open and targets the Base branch, and marks it ready for review (`gh pr ready`) if the agent left it as a draft.
 6. Keeps the pull request mergeable and green: merges the Base branch in and watches CI, starting a **Repair** session for a merge conflict or failing checks, at most 5 per Run. A failed check that also failed, under the same name, on the Base branch commit the head last merged in is an **Inherited failure**, not the branch's to fix: the CI-fix Repair is given only the branch's own failures, with the Inherited failures listed as not to fix. If every failed check is an Inherited failure, no Repair starts: the Run merges the Base branch again if it has moved since, and otherwise ends as a [Failed run](#failed-runs) that says to fix the Base branch first, unless it was given `base-fix` or the [User config](#user-config) sets `base.fix`, when it starts a [Base fix](#base-fix) first. A check that is pending, passed or missing on that Base branch commit is the branch's own, and thirdshift never triggers or waits for the Base branch's CI. If a CI-fix Repair leaves the head unchanged, with no commit of its own and no Base branch move to merge, thirdshift does a **Check re-run**, once for that head: it asks GitHub to re-run the branch's own failed checks (`gh run rerun <run-id> --failed`, once per workflow run), says so on stderr as `re-running the failed checks on <short sha>: <check>[, <check>…]`, and watches CI on that head again, from the new attempt on. A check that failed for a reason that does not repeat, such as a flaky test or a runner fault, then passes, and the Run goes on as for any green head. If CI is red again, the Run ends as a Failed run, with no second Repair or re-run for that head. It ends the same way, with nothing re-run, if one of those checks can't be re-run (a commit status, or a check run that is not a GitHub Actions job), since the head could not go green. When GitHub refuses a re-run, stderr shows the refusal and thirdshift reads all checks on that head again: a Repair may already have re-run them. Green continues the Run, pending checks are watched, and the branch's own checks still red fail as before, without another Repair or request for that head. Checks that cannot be read or disappear after known failures cannot establish green CI. Any earlier successful workflow request still waits for its new attempt before the whole head is assessed. What the Repair concluded is never read, Inherited failures are never re-run on their own account, and a Check re-run is no Repair, so it counts against no budget ([ADR-0010](docs/adr/0010-one-check-re-run-before-a-declined-ci-fix.md)). If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. In a Merge run, each round also takes in **Foreign commits** first: see [Foreign commits in a Merge run](#foreign-commits-in-a-merge-run).
-7. In a **Merge run** (`thirdshift merge <Issue URL>`), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit <sha>`). It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails goes back round step 6, within the same budgets, and is tried again on the new green head. If that round finds nothing to fix, the refusal is a **policy refusal**, such as merge commits being disallowed or a review being required. After the merge, thirdshift deletes the Issue branch on `origin`, and closes the issue if it is still open, with the comment `Closed by #<pr>, merged into <base> by a thirdshift Merge run.` GitHub closes it on its own only for a merge into the repository's default branch, and then thirdshift leaves it alone. Once the issue is closed, either way, thirdshift takes `in-progress` off it, [removing the Claim](#when-the-claim-ends).
+7. In a **Merge run** (`thirdshift merge <Issue URL>`), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit <sha>`). Just before each attempt, it reads the PR's `reviewDecision`: `REVIEW_REQUIRED` or `CHANGES_REQUESTED` ends the Run as **review required**, a policy refusal, without attempting the merge or taking another Repair round. An empty or `APPROVED` decision, or a failed read, keeps the existing merge behavior. It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails goes back round step 6, within the same budgets, and is tried again on the new green head. If that round finds nothing to fix, the refusal is a **policy refusal**, such as merge commits being disallowed or a review being required. After the merge, thirdshift deletes the Issue branch on `origin`, and closes the issue if it is still open, with the comment `Closed by #<pr>, merged into <base> by a thirdshift Merge run.` GitHub closes it on its own only for a merge into the repository's default branch, and then thirdshift leaves it alone. Once the issue is closed, either way, thirdshift takes `in-progress` off it, [removing the Claim](#when-the-claim-ends).
 8. Cleans up: removes the worktree, the local Issue branch and the temporary directory the Factory skills were written out to, whether the Run succeeded or not. The one exception is a **Failed run** whose work could not be pushed: see below.
 
 ### The Claim
@@ -149,9 +149,10 @@ thirdshift merge https://github.com/acme/widgets/issues/7
 
 `thirdshift --merge <Issue URL>` does the same, and either goes before or after the URL. The agent gets the same implement prompt either way; the differences are the Self-merge at the end and the review of [Foreign commits](#foreign-commits-in-a-merge-run). To make every Run on a machine a Merge run, set `merge.always` in the [User config](#user-config); `no-merge` (or `--no-merge`), before or after the URL, then makes one Run end ready for review instead. Giving a flag twice, or `merge` together with `no-merge`, is an argument error.
 
-A Merge run ends in one of three ways:
+A Merge run ends in one of four ways:
 
 - **Merged**: exit `0`, the pull request's URL on stdout, and `PR <url> is merged` as stderr's last line.
+- **Review required**: the PR's `reviewDecision` is `REVIEW_REQUIRED` or `CHANGES_REQUESTED` just before a Self-merge attempt. Exit `1`, the PR URL on stdout, and `PR <url> needs an approving review before it can merge; it is ready for review` on stderr and in the Run notification. The Run notification and Activity log summary name `review required`. The PR stays ready, the Claim is kept, and no merge attempt or further Repair round runs. An empty or `APPROVED` decision, or a failed read, leaves the existing merge behavior in place.
 - **Policy refusal**: GitHub refused the merge and a round of merging the Base branch and watching CI found nothing left to fix. Exit `1`, the pull request's URL on stdout, and GitHub's error on stderr. The pull request stays open and ready for review, not a draft, for you to merge by hand or to change the repository's settings. thirdshift never reads GitHub's error text to decide this.
 - **Any other failure**: a [Failed run](#failed-runs), as for a Run without `merge`.
 
@@ -546,7 +547,7 @@ A **Failed run** is one that ends, including by Ctrl-C or a closed terminal, wit
 A Failed run:
 
 1. Commits any uncommitted work as `thirdshift: failed run (<reason>)`, with a timestamp and the hostname, and pushes the Issue branch, so nothing is lost. If the branch has no changes against the Base branch, nothing is pushed.
-2. Converts its open pull request, if any, back to a draft, so a pull request only claims to be ready when the factory stands behind it. The next successful Continuation marks it ready again. The exception is a Merge run's policy refusal: the pull request is ready, mergeable and green and only the Self-merge could not happen, so it stays ready for review, and no failure commit is pushed onto the head whose CI was watched.
+2. Converts its open pull request, if any, back to a draft, so a pull request only claims to be ready when the factory stands behind it. The next successful Continuation marks it ready again. The exception is a Merge run's policy refusal: the pull request is ready, mergeable and green and only the Self-merge could not happen, so it stays ready for review, and no failure commit is pushed onto the head whose CI was watched. This includes **review required**: a review blocks the merge, so thirdshift skips the merge attempt and another Repair round. stderr and the Run notification say `PR <url> needs an approving review before it can merge; it is ready for review`, with `review required` as the notification outcome. The Run exits `1`, prints the PR URL on stdout, keeps its Claim and cleans up as usual. The same applies to a Spec run's Spec PR.
 3. Cleans up as usual, prints the reason to stderr and exits non-zero. If the push failed, the worktree and local Issue branch are kept instead, and stderr names the branch, its head commit and the worktree path, so you can recover the work or push it by hand.
 4. Releases its [Claim](#when-the-claim-ends), unless it is a failed Security fix, if it left nothing on `origin`: no Issue branch and no pull request. Otherwise the issue stays `in-progress`.
 

docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md

@@ -14,4 +14,5 @@ A **Merge run** ends with thirdshift itself running `gh pr merge --merge --match
 ## Consequences
 
 - A merge that fails with nothing left to fix (no new commits, Base branch unchanged, mergeable, green) is a policy refusal, such as a disallowed merge method or a required review. The Run fails, but it leaves the pull request ready for review, not a draft. thirdshift never parses GitHub's error text to tell policy refusals apart from other failures.
+- Before each Self-merge attempt, thirdshift detects a blocking review up front from the PR's structured `reviewDecision` (`REVIEW_REQUIRED` or `CHANGES_REQUESTED`). It ends as a `review required` policy refusal without a merge attempt or another Repair round; an empty or approved decision, or a failed read, leaves the existing behavior in place.
 - A repository that requires reviews can't be used with a Merge run; that is the repository owner's choice to have a human look first.

src/delivery.rs

@@ -664,7 +664,7 @@ mod tests {
         fn take_to_goal(&mut self, _: &str, goal: Goal) -> Result<()> {
             self.calls.push(Call::Goal(goal));
             if self.refused {
-                return Err(anyhow!("merge refused").context(PolicyRefusal));
+                return Err(anyhow!("merge refused").context(PolicyRefusal::NothingToFix));
             }
             self.check(Fails::Goal)
         }

src/delivery/repair_loop.rs

@@ -131,7 +131,7 @@ pub(super) fn take_to_goal(outside: &mut impl Outside, base: &str, goal: Goal) -
             Round::NothingToFix => {
                 // Only a PR still ready and mergeable is left ready.
                 repair_loop.outside.ensure_pr_ready_and_mergeable()?;
-                return Err(error.context(PolicyRefusal));
+                return Err(error.context(PolicyRefusal::NothingToFix));
             }
         }
     }

src/failed_run.rs

@@ -22,17 +22,29 @@ pub struct FailedRun {
     pub ticket_lines: Vec<String>,
 }
 
-/// A merge GitHub refused when a round of the Repair loop found nothing to
-/// fix. The Run fails, but, as it can do no more, leaves the PR ready for review
-/// rather than a draft. It is the context of the merge error.
+/// A repository policy that prevents Self-merge of a ready PR. The Run
+/// fails without pushing a failure commit or converting the PR to a draft.
 #[derive(Debug)]
-pub struct PolicyRefusal;
+pub enum PolicyRefusal {
+    /// A refused merge whose next Repair round found nothing to fix.
+    NothingToFix,
+    /// A structured review decision that blocks the merge before a request.
+    ReviewRequired { pr_url: String },
+}
+
+impl std::error::Error for PolicyRefusal {}
 
 impl fmt::Display for PolicyRefusal {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.write_str(
-            "the merge was refused with nothing left to fix, so the PR stays ready for review",
-        )
+        match self {
+            Self::NothingToFix => f.write_str(
+                "the merge was refused with nothing left to fix, so the PR stays ready for review",
+            ),
+            Self::ReviewRequired { pr_url } => write!(
+                f,
+                "PR {pr_url} needs an approving review before it can merge; it is ready for review"
+            ),
+        }
     }
 }
 

src/github.rs

@@ -597,6 +597,19 @@ impl GitHub {
         ])
     }
 
+    /// Whether a structured review decision blocks this numbered PR's merge.
+    /// Failed or unrecognized reads are unknown and leave the merge to GitHub.
+    pub fn review_blocks_merge(&self, issue: &IssueUrl, number: u64) -> bool {
+        self.pr_view(issue, &number.to_string(), "reviewDecision")
+            .ok()
+            .is_some_and(|json| {
+                matches!(
+                    json["reviewDecision"].as_str(),
+                    Some("REVIEW_REQUIRED" | "CHANGES_REQUESTED")
+                )
+            })
+    }
+
     /// One guarded merge request, with a merge commit and without auto-merge
     /// or local branch deletion (ADR 0004). Delivery owns reconciliation.
     pub fn merge(&self, issue: &IssueUrl, number: u64, head: &str) -> Result<()> {

src/pull_request.rs

@@ -4,6 +4,7 @@
 
 use anyhow::{Context, Result, bail};
 
+use crate::failed_run::PolicyRefusal;
 use crate::github::{GitHub, Mergeable, PrSnapshot, PrState};
 use crate::harness::Choice;
 use crate::issue::IssueUrl;
@@ -259,7 +260,8 @@ impl<A: Adapter> PullRequest<A> {
         }
     }
 
-    /// A fresh gate immediately precedes the numbered request. GitHub guards
+    /// A fresh gate and best-effort review decision read precede each numbered
+    /// request. A blocking review is a terminal policy refusal. GitHub guards
     /// its head; a retarget between the gate and server merge remains a remote
     /// race because the request has no expected-base precondition.
     pub fn merge(&mut self, watched: &str) -> Result<MergeAttempt> {
@@ -269,6 +271,17 @@ impl<A: Adapter> PullRequest<A> {
             .as_ref()
             .expect("gate captured identity")
             .number;
+        if self.adapter.review_blocks_merge(&self.issue, number) {
+            return Err(PolicyRefusal::ReviewRequired {
+                pr_url: self
+                    .identified
+                    .as_ref()
+                    .expect("gate captured identity")
+                    .url
+                    .clone(),
+            }
+            .into());
+        }
         let Err(error) = self.adapter.merge(&self.issue, number, watched) else {
             return Ok(MergeAttempt::Merged);
         };
@@ -413,6 +426,7 @@ pub(crate) trait Adapter {
         title: &str,
         body: &str,
     ) -> Result<String>;
+    fn review_blocks_merge(&mut self, issue: &IssueUrl, number: u64) -> bool;
     fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()>;
     fn pause(&mut self) -> Result<()> {
         poll::pause()
@@ -480,6 +494,9 @@ impl Adapter for GitHub {
         self.completion()
             .create_draft_pr(issue, branch, base, title, body)
     }
+    fn review_blocks_merge(&mut self, issue: &IssueUrl, number: u64) -> bool {
+        GitHub::review_blocks_merge(self, issue, number)
+    }
     fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()> {
         GitHub::merge(self, issue, number, head)
     }
@@ -714,6 +731,9 @@ mod tests {
             ));
             world.creation.clone().map_err(anyhow::Error::msg)
         }
+        fn review_blocks_merge(&mut self, _: &IssueUrl, _: u64) -> bool {
+            false
+        }
         fn merge(&mut self, _: &IssueUrl, number: u64, head: &str) -> Result<()> {
             let mut world = self.0.borrow_mut();
             world.merges.push((number, head.to_string()));