From 506f9b73fa20b0d3bb45958638aa18374a72909c Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 16 Oct 2026 19:47:41 +0000 Subject: [PATCH 1/3] Detect review-required policy refusals before Self-merge (#707) --- CONTEXT.md | 2 +- README.md | 7 +- ...rge-by-thirdshift-not-github-auto-merge.md | 1 + src/delivery.rs | 2 +- src/delivery/repair_loop.rs | 2 +- src/failed_run.rs | 26 ++- src/github.rs | 13 ++ src/pull_request.rs | 22 ++- src/run_ending.rs | 9 +- tests/fakes/gh.rs | 6 +- tests/merge_run.rs | 185 ++++++++++++++++++ tests/spec_run.rs | 48 +++++ 12 files changed, 306 insertions(+), 17 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index e9264eab9..ef1428b4b 100644 --- a/CONTEXT.md +++ b/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 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. diff --git a/README.md b/README.md index 1656be4f7..6025a8443 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ From a clone of the issue's repository, `thirdshift `: 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-` 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 --failed`, once per workflow run), says so on stderr as `re-running the failed checks on : [, …]`, 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 `), 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 `). 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 #, merged into 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 `), 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 `). 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 #, merged into 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 ` 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 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 needs an approving review before it can merge; it is ready for review` on stderr and in the Run notification. 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 ()`, 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 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`. diff --git a/docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md b/docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md index 3c89bc8af..d562e7e53 100644 --- a/docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md +++ b/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. diff --git a/src/delivery.rs b/src/delivery.rs index 534ad960e..a43c7dbd9 100644 --- a/src/delivery.rs +++ b/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) } diff --git a/src/delivery/repair_loop.rs b/src/delivery/repair_loop.rs index 279665f51..d71a75697 100644 --- a/src/delivery/repair_loop.rs +++ b/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)); } } } diff --git a/src/failed_run.rs b/src/failed_run.rs index 2e0b2acc7..76d985ecd 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -22,17 +22,29 @@ pub struct FailedRun { pub ticket_lines: Vec, } -/// 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" + ), + } } } diff --git a/src/github.rs b/src/github.rs index 55f2b2323..8fb75a0b8 100644 --- a/src/github.rs +++ b/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<()> { diff --git a/src/pull_request.rs b/src/pull_request.rs index 61d1d570f..24a5d695e 100644 --- a/src/pull_request.rs +++ b/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 PullRequest { } } - /// 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 { @@ -269,6 +271,17 @@ impl PullRequest { .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; + 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())); diff --git a/src/run_ending.rs b/src/run_ending.rs index 7acfba777..1bc1f7961 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -13,7 +13,7 @@ use std::process::ExitCode; use crate::architect::Reviewed; use crate::base_fix::Advice; use crate::command::{Ending, Skip}; -use crate::failed_run::FailedRun; +use crate::failed_run::{FailedRun, PolicyRefusal}; use crate::logs; use crate::progress::{self, ChildLine}; use crate::run::Ended; @@ -285,10 +285,15 @@ impl<'a> Account<'a> { } } -/// `interrupted`, or `failure` for a failure that was not an interrupt. +/// A named interruption or review requirement, otherwise `failure`. fn failure_outcome(failed: &FailedRun, failure: &'static str) -> &'static str { if failed.interrupted { "interrupted" + } else if matches!( + failed.error.downcast_ref::(), + Some(PolicyRefusal::ReviewRequired { .. }) + ) { + "review required" } else { failure } diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 6210c6dce..4d61179bf 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -12,7 +12,7 @@ //! "bodies"?: {"": ""}, //! "comments"?: {"": ["", ...]}, //! "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body", -//! "mergeable"?, "unknown_polls"?}], +//! "mergeable"?, "unknown_polls"?, "reviewDecision"?}], //! "checks": {"": [{"id", "name", "conclusion", "url", "pending_polls"?, //! "rerun"?: {"conclusion", "stale_polls"?, "pending_polls"?}, //! "stale"?: {"id", "conclusion", "polls"}}]}, @@ -422,6 +422,10 @@ fn pr_fields(pr: &Json, wanted: &[String]) -> Json { "mergeable", pr.get("mergeable").cloned().unwrap_or(string("MERGEABLE")), ), + ( + "reviewDecision", + pr.get("reviewDecision").cloned().unwrap_or(string("")), + ), ("body", pr.get("body").cloned().unwrap_or(string(""))), ( "mergeCommit", diff --git a/tests/merge_run.rs b/tests/merge_run.rs index ed34e8654..c7a3f88b3 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -5,6 +5,7 @@ mod support; +use support::resend::ResendStandIn; use support::{Scenario, leaves_running}; /// The agent commits its work and opens a PR that closes issue #7, into @@ -736,3 +737,187 @@ fn an_issue_github_closed_after_the_merge_has_its_claim_removed_too() { assert!(scenario.gh_calls_of("issue", "close").is_empty()); assert_eq!(scenario.issue_labels(7), Vec::::new()); } + +#[test] +fn a_required_review_refuses_the_merge_without_another_round_and_names_the_outcome() { + for decision in ["REVIEW_REQUIRED", "CHANGES_REQUESTED"] { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["ready-for-agent"]); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 reviewDecision '\"{decision}\"'\n{}", + checks_on_head(r#"[{"name":"test","conclusion":"success"}]"#), + )); + let resend = ResendStandIn::replying(200, r#"{"id":"1"}"#); + + let result = scenario.run_with_env( + &[ + "merge", + &scenario.issue_url(7), + "email", + "owner@example.com", + ], + &[ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", "test-key"), + ], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + let reason = format!( + "PR {PR_URL} needs an approving review before it can merge; it is ready for review" + ); + assert!(result.stderr.contains(&reason), "{}", result.stderr); + assert!(scenario.gh_calls_of("pr", "merge").is_empty()); + assert_eq!( + result.stderr.matches("waiting up to 0s for CI on").count(), + 1, + "{}", + result.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1); + let gh = scenario.gh_state(); + assert_eq!(gh["prs"][0]["state"], "OPEN"); + assert_eq!(gh["prs"][0]["isDraft"], false); + assert_eq!(gh["issues"]["7"], "OPEN"); + assert_eq!(scenario.issue_labels(7), ["in-progress"]); + assert_eq!( + scenario.origin_log("issue-7").unwrap(), + ["Add feature", "Initial commit"] + ); + scenario.assert_cleaned_up("issue-7"); + let requests = resend.requests(); + assert_eq!(requests.len(), 1); + assert!( + requests[0].body["subject"] + .as_str() + .unwrap() + .contains("review required"), + "{:?}", + requests[0].body + ); + assert!( + requests[0].body["text"].as_str().unwrap().contains(&reason), + "{:?}", + requests[0].body + ); + assert!( + requests[0].body["text"] + .as_str() + .unwrap() + .contains("Result: review required") + ); + } +} + +#[test] +fn unblocked_or_unknown_review_decisions_allow_the_guarded_merge() { + for decision in [r#""""#, r#""APPROVED""#, r#""FUTURE_VALUE""#, "null"] { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 reviewDecision '{decision}'\n" + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "{decision}: {}", result.stderr); + assert_issue_7_merged_into(&scenario, "main"); + assert!(scenario.gh_calls().iter().any(|call| call + == &[ + "pr", + "view", + "1", + "--repo", + "acme/widgets", + "--json", + "reviewDecision" + ])); + } +} + +#[test] +fn an_unreadable_review_decision_keeps_existing_merge_and_policy_refusal_behavior() { + for refused in [false, true] { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 reviewDecision '\"REVIEW_REQUIRED\"'\n\ + gh fake fails 'pr view 1 --repo acme/widgets --json reviewDecision'\n{}", + if refused { + refuse_merges(1, "Merge commits are not allowed.") + } else { + String::new() + }, + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!( + result.code, + Some(if refused { 1 } else { 0 }), + "{}", + result.stderr + ); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 1); + assert!(!result.stderr.contains("needs an approving review")); + if refused { + assert!( + result.stderr.contains("Merge commits are not allowed."), + "{}", + result.stderr + ); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], false); + } else { + assert_issue_7_merged_into(&scenario, "main"); + } + } +} + +#[test] +fn the_review_decision_is_read_again_after_a_repair_before_retrying_the_merge() { + let scenario = Scenario::new(); + let hook = format!( + "{}\ngh fake pr issue-7 reviewDecision '\"REVIEW_REQUIRED\"'\n", + someone_pushes_to_issue_7("late.txt", "late", "Late commit"), + ); + scenario.agent_does_in_session( + 1, + &format!( + "{AGENT_OPENS_PR}gh fake on-merge 1 '{}'\n{}", + hook.replace('\'', r"'\''"), + refuse_merges(1, "Head branch was modified."), + ), + ); + scenario.agent_does_in_session(2, ""); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert!( + result.stderr.contains("needs an approving review"), + "{}", + result.stderr + ); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 1); + assert_eq!(scenario.claude_calls().len(), 2); + assert_eq!( + result.stderr.matches("waiting up to 0s for CI on").count(), + 2, + "{}", + result.stderr + ); + assert_eq!( + scenario + .gh_calls() + .iter() + .filter(|call| call.last().is_some_and(|arg| arg == "reviewDecision")) + .count(), + 2 + ); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], false); + assert_eq!(scenario.origin_log("main").unwrap(), ["Initial commit"]); + assert_eq!( + scenario.origin_file("issue-7", "late.txt").as_deref(), + Some("late\n") + ); +} diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 32843752e..7346cd492 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -2565,3 +2565,51 @@ fn a_spec_run_checks_its_model_once_and_every_ticket_and_the_spec_review_runs_on ); } } + +#[test] +fn a_review_required_spec_pr_stays_ready_without_an_attempt_or_another_round() { + for decision in ["REVIEW_REQUIRED", "CHANGES_REQUESTED"] { + let scenario = linear_spec(); + scenario.agent_does_for( + SPEC, + &format!( + "gh fake pr issue-20 reviewDecision '\"{decision}\"'\n{}", + checks_on_head(r#"[{"name":"test","conclusion":"success"}]"#), + ), + ); + let resend = ResendStandIn::replying(200, r#"{"id":"1"}"#); + + let result = run_emailing( + &scenario, + &resend, + &["merge", &spec_url(&scenario), "email", "owner@example.com"], + ); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/2\n"); + let reason = "PR https://github.com/acme/widgets/pull/2 needs an approving review before it can merge; it is ready for review"; + assert_contains(&result.stderr, reason); + let spec = spec_pr(&scenario); + assert_eq!(spec["state"], "OPEN"); + assert_eq!(spec["isDraft"], false); + assert_eq!(scenario.issue_labels(SPEC), ["in-progress"]); + assert_eq!(sessions_by_issue(&scenario), ["21", "22", "20"]); + assert!( + scenario + .gh_calls_of("pr", "merge") + .iter() + .all(|call| call[2] != "2") + ); + assert_eq!( + result.stderr.matches("waiting up to 0s for CI on").count(), + 3, + "{}", + result.stderr + ); + scenario.assert_cleaned_up("issue-20"); + let (subject, text) = the_one_notification(&resend); + assert_contains(&subject, "review required"); + assert_contains(&text, reason); + assert_contains(&text, "Result: review required"); + } +} From 9b3819fa2e952a49e3aa582217be028469895691 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 16 Oct 2026 15:49:17 -0400 Subject: [PATCH 2/3] Report review-required policy refusals before Self-merge (#707) --- README.md | 7 +- ...rge-by-thirdshift-not-github-auto-merge.md | 1 + src/delivery.rs | 2 +- src/delivery/repair_loop.rs | 2 +- src/failed_run.rs | 26 +++- src/github.rs | 10 ++ src/pull_request.rs | 23 +++ src/run_ending.rs | 20 ++- tests/fakes/gh.rs | 7 +- tests/merge_run.rs | 145 ++++++++++++++++++ tests/run_notification.rs | 43 ++++++ tests/spec_run.rs | 81 ++++++++++ 12 files changed, 348 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 1656be4f7..446272358 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ From a clone of the issue's repository, `thirdshift `: 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-` 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 --failed`, once per workflow run), says so on stderr as `re-running the failed checks on : [, …]`, 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 `), 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 `). 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 #, merged into 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 `), 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 `). Before each attempt, it reads the PR's `reviewDecision`; `REVIEW_REQUIRED` or `CHANGES_REQUESTED` ends the Run as **Review required** without attempting the merge or taking a Repair round. 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 #, merged into 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 ` 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 is merged` as stderr's last line. +- **Review required**: just before each Self-merge attempt, `reviewDecision` is `REVIEW_REQUIRED` or `CHANGES_REQUESTED`. thirdshift skips the merge attempt and the Repair round, exits `1` with the pull request's URL on stdout, and says `PR needs an approving review before it can merge; it is ready for review` on stderr. The Run notification and summary name `review required`. This is a policy refusal: the PR stays open and ready, its Claim stays, and cleanup runs as usual. An empty or `APPROVED` decision, or a read that fails, keeps the usual merge behavior. This applies to a Spec run's Spec PR too. - **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 ()`, 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, including **Review required**, detected from `reviewDecision` before attempting the Self-merge: 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. 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`. diff --git a/docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md b/docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md index 3c89bc8af..fc19920c9 100644 --- a/docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md +++ b/docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md @@ -13,5 +13,6 @@ A **Merge run** ends with thirdshift itself running `gh pr merge --merge --match ## Consequences +- Before each Self-merge attempt, thirdshift detects a blocking review up front from the structured `reviewDecision` field (`REVIEW_REQUIRED` or `CHANGES_REQUESTED`) and ends as **Review required**, a policy refusal, without a merge attempt or Repair round. An empty or `APPROVED` decision, or a failed read, leaves the existing merge behavior in place. - 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. - 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. diff --git a/src/delivery.rs b/src/delivery.rs index 534ad960e..41a46eda3 100644 --- a/src/delivery.rs +++ b/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::MergeRefused)); } self.check(Fails::Goal) } diff --git a/src/delivery/repair_loop.rs b/src/delivery/repair_loop.rs index 279665f51..eb2a49604 100644 --- a/src/delivery/repair_loop.rs +++ b/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::MergeRefused)); } } } diff --git a/src/failed_run.rs b/src/failed_run.rs index 2e0b2acc7..03bdeca02 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -22,17 +22,29 @@ pub struct FailedRun { pub ticket_lines: Vec, } -/// 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 Self-merge blocked by policy. The Run fails but leaves the PR ready +/// for review, with no failure commit pushed onto the watched head. #[derive(Debug)] -pub struct PolicyRefusal; +pub enum PolicyRefusal { + /// A refused merge whose Repair round found nothing to fix. + MergeRefused, + /// A structured review decision blocks the merge before any attempt. + ReviewRequired { 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::MergeRefused => f.write_str( + "the merge was refused with nothing left to fix, so the PR stays ready for review", + ), + Self::ReviewRequired { url } => write!( + f, + "PR {url} needs an approving review before it can merge; it is ready for review" + ), + } } } diff --git a/src/github.rs b/src/github.rs index 55f2b2323..a8d0089d2 100644 --- a/src/github.rs +++ b/src/github.rs @@ -597,6 +597,16 @@ impl GitHub { ]) } + /// Whether the PR's structured review decision currently blocks a merge. + /// Callers may treat a failed read as unknown and let GitHub decide. + pub fn review_blocks_merge(&self, issue: &IssueUrl, number: u64) -> Result { + let json = self.pr_view(issue, &number.to_string(), "reviewDecision")?; + Ok(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<()> { diff --git a/src/pull_request.rs b/src/pull_request.rs index 61d1d570f..0532b1765 100644 --- a/src/pull_request.rs +++ b/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; @@ -269,6 +270,21 @@ impl PullRequest { .as_ref() .expect("gate captured identity") .number; + if self + .adapter + .review_blocks_merge(&self.issue, number) + .unwrap_or(false) + { + return Err(PolicyRefusal::ReviewRequired { + 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 +429,7 @@ pub(crate) trait Adapter { title: &str, body: &str, ) -> Result; + fn review_blocks_merge(&mut self, issue: &IssueUrl, number: u64) -> Result; fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()>; fn pause(&mut self) -> Result<()> { poll::pause() @@ -480,6 +497,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) -> Result { + 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 +734,9 @@ mod tests { )); world.creation.clone().map_err(anyhow::Error::msg) } + fn review_blocks_merge(&mut self, _: &IssueUrl, _: u64) -> Result { + Ok(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())); diff --git a/src/run_ending.rs b/src/run_ending.rs index 7acfba777..fa029a7b8 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -13,7 +13,7 @@ use std::process::ExitCode; use crate::architect::Reviewed; use crate::base_fix::Advice; use crate::command::{Ending, Skip}; -use crate::failed_run::FailedRun; +use crate::failed_run::{FailedRun, PolicyRefusal}; use crate::logs; use crate::progress::{self, ChildLine}; use crate::run::Ended; @@ -88,8 +88,8 @@ pub fn show_skip(skip: &Skip) -> ExitCode { #[derive(Debug, PartialEq, Eq)] pub struct Account<'a> { /// In a word or a few, as a Run notification's subject gives it: its - /// pull request's outcome, `failed` or `interrupted`; or, for an - /// Architect run that dispatched nothing, how its review ended, or + /// pull request's outcome, `review required`, `failed` or `interrupted`; + /// or, for an Architect run that dispatched nothing, how its review ended, or /// `review failed`. pub outcome: &'static str, /// The line that says how it ended, once it succeeded: its pull @@ -271,11 +271,14 @@ impl<'a> Account<'a> { /// How the command ended, in short, as its Activity log end line gives /// it: the line that says how it ended once it succeeded, or `failed: ` - /// and the first line of its cause; after the plan it dispatched, in an - /// Architect run that dispatched one. + /// (or `review required: `) and the first line of its cause; after the + /// plan it dispatched, in an Architect run that dispatched one. pub fn summary(&self) -> String { let ended = match &self.ended { Ok(line) => line.clone(), + Err(cause) if self.outcome == "review required" => { + format!("review required: {}", cause.first_line()) + } Err(cause) => format!("failed: {}", cause.first_line()), }; match self.review.as_ref().and_then(|review| review.dispatched) { @@ -285,10 +288,15 @@ impl<'a> Account<'a> { } } -/// `interrupted`, or `failure` for a failure that was not an interrupt. +/// `interrupted`, `review required` for a blocking review, or `failure`. fn failure_outcome(failed: &FailedRun, failure: &'static str) -> &'static str { if failed.interrupted { "interrupted" + } else if matches!( + failed.error.downcast_ref::(), + Some(PolicyRefusal::ReviewRequired { .. }) + ) { + "review required" } else { failure } diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 6210c6dce..95ad00c83 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -12,7 +12,7 @@ //! "bodies"?: {"": ""}, //! "comments"?: {"": ["", ...]}, //! "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body", -//! "mergeable"?, "unknown_polls"?}], +//! "mergeable"?, "unknown_polls"?, "reviewDecision"?}], //! "checks": {"": [{"id", "name", "conclusion", "url", "pending_polls"?, //! "rerun"?: {"conclusion", "stale_polls"?, "pending_polls"?}, //! "stale"?: {"id", "conclusion", "polls"}}]}, @@ -119,6 +119,7 @@ //! labels, and `gh label create ` adds one, failing if the repository //! has it already. //! +//! A PR's reviewDecision defaults to an empty string when no review rule is set. //! A PR's mergeable state reports as UNKNOWN for its first `unknown_polls` //! reads. A check run reports as in progress for its first `pending_polls` //! reads, then as completed with its `conclusion`. @@ -423,6 +424,10 @@ fn pr_fields(pr: &Json, wanted: &[String]) -> Json { pr.get("mergeable").cloned().unwrap_or(string("MERGEABLE")), ), ("body", pr.get("body").cloned().unwrap_or(string(""))), + ( + "reviewDecision", + pr.get("reviewDecision").cloned().unwrap_or(string("")), + ), ( "mergeCommit", match pr.get("mergeCommit") { diff --git a/tests/merge_run.rs b/tests/merge_run.rs index ed34e8654..a18659e66 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -50,6 +50,151 @@ fn assert_issue_7_merged_into(scenario: &Scenario, base: &str) -> String { head } +#[test] +fn a_review_required_merge_run_stops_without_a_merge_or_repair() { + assert_review_blocks_merge("REVIEW_REQUIRED"); +} + +#[test] +fn changes_requested_stops_without_a_merge_or_repair() { + assert_review_blocks_merge("CHANGES_REQUESTED"); +} + +fn assert_review_blocks_merge(decision: &str) { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 reviewDecision '\"{decision}\"'\n{}", + checks_on_head(r#"[{"name":"test","conclusion":"success"}]"#) + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert!(scenario.gh_calls_of("pr", "merge").is_empty()); + assert_eq!(scenario.claude_calls().len(), 1); + assert_eq!( + result + .stderr + .matches("merging origin/main into issue-7") + .count(), + 1, + "{}", + result.stderr + ); + assert!( + result.stderr.contains(&format!( + "PR {PR_URL} needs an approving review before it can merge; it is ready for review" + )), + "{}", + result.stderr + ); + let state = scenario.gh_state(); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert_eq!(state["prs"][0]["isDraft"], false); + assert_eq!(state["issues"]["7"], "OPEN"); + assert!( + scenario + .issue_labels(7) + .contains(&"in-progress".to_string()) + ); + assert_eq!( + scenario.origin_log("issue-7").unwrap(), + ["Add feature", "Initial commit"] + ); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn empty_approved_and_unknown_review_decisions_allow_the_merge() { + for decision in ["", "APPROVED", "FUTURE_DECISION"] { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 reviewDecision '\"{decision}\"'\n" + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "{decision}: {}", result.stderr); + assert_issue_7_merged_into(&scenario, "main"); + let calls = scenario.gh_calls(); + let merge = calls + .iter() + .position(|call| call.starts_with(&["pr".to_string(), "merge".to_string()])) + .unwrap(); + assert_eq!( + calls[merge - 1], + [ + "pr", + "view", + "1", + "--repo", + "acme/widgets", + "--json", + "reviewDecision" + ] + ); + } +} + +#[test] +fn a_failed_review_decision_read_still_attempts_the_merge() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 reviewDecision '\"REVIEW_REQUIRED\"'\n\ + gh fake fails 'pr view 1 --repo acme/widgets --json reviewDecision'\n" + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_issue_7_merged_into(&scenario, "main"); +} + +#[test] +fn a_review_required_after_repair_stops_the_next_merge_attempt() { + let scenario = Scenario::new(); + let foreign = someone_pushes_to_issue_7("late.txt", "late", "Late commit"); + scenario.agent_does_in_session( + 1, + &format!( + "{AGENT_OPENS_PR}gh fake on-merge 1 '{}'\n", + foreign.replace('\'', r"'\''") + ), + ); + scenario.agent_does_in_session( + 2, + "gh fake pr issue-7 reviewDecision '\"REVIEW_REQUIRED\"'\n", + ); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 1); + assert_eq!(scenario.claude_calls().len(), 2); + assert_review_repair_from( + &scenario.claude_calls()[1]["prompt"], + &sha_of(&scenario, "issue-7", "Add feature"), + ); + assert_eq!( + result + .stderr + .matches("merging origin/main into issue-7") + .count(), + 2, + "{}", + result.stderr + ); + assert!( + result.stderr.contains("needs an approving review"), + "{}", + result.stderr + ); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], false); + scenario.assert_cleaned_up("issue-7"); +} + #[test] fn a_clean_merge_run_merges_the_pr_and_says_so() { let scenario = Scenario::new(); diff --git a/tests/run_notification.rs b/tests/run_notification.rs index cf015ac86..3a3e58d33 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -74,6 +74,49 @@ fn assert_contains(text: &str, part: &str) { assert!(text.contains(part), "expected {part:?} in: {text}"); } +#[test] +fn a_review_required_notification_and_summary_name_the_refusal() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 reviewDecision '\"REVIEW_REQUIRED\"'\n" + )); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &["merge", &scenario.issue_url(7), "email", "me@example.com"], + Some(KEY), + ); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + let request = the_one_request(&resend); + assert!( + subject(&request).ends_with(": review required"), + "{}", + subject(&request) + ); + assert!( + text(&request).starts_with("Result: review required\n"), + "{}", + text(&request) + ); + assert_contains( + text(&request), + &format!( + "Cause: PR {PR_URL} needs an approving review before it can merge; it is ready for review" + ), + ); + let activity = + std::fs::read_to_string(scenario.path("home/.thirdshift/logs/acme/widgets/activity.log")) + .unwrap(); + assert_contains( + &activity, + &format!("review required: PR {PR_URL} needs an approving review"), + ); +} + #[test] fn a_notification_keeps_current_models_when_an_unrelated_record_is_corrupt() { let scenario = Scenario::new(); diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 32843752e..add3ef9e6 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1149,6 +1149,87 @@ fn a_shared_base_fix_that_fails_fails_the_ticket_that_started_it_and_the_one_wai ); } +#[test] +fn a_blocking_review_on_the_spec_pr_stops_without_a_merge_or_repair() { + for decision in ["REVIEW_REQUIRED", "CHANGES_REQUESTED"] { + let scenario = linear_spec(); + scenario.agent_does_for( + SPEC, + &format!( + "gh fake pr issue-20 reviewDecision '\"{decision}\"'\n{}", + checks_on_head(GREEN) + ), + ); + let resend = ResendStandIn::replying(200, r#"{"id":"test"}"#); + + let result = scenario.run_with_env( + &["merge", &spec_url(&scenario), "email", "me@example.com"], + &[ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", "re_test_123"), + ], + ); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + let spec = spec_pr(&scenario); + let url = spec["url"].as_str().unwrap(); + assert_eq!(result.stdout, format!("{url}\n")); + assert_eq!(spec["state"], "OPEN"); + assert_eq!(spec["isDraft"], false); + assert_eq!(scenario.gh_state()["issues"]["20"], "OPEN"); + assert!( + scenario + .issue_labels(SPEC) + .contains(&"in-progress".to_string()) + ); + assert_contains( + &result.stderr, + &format!( + "PR {url} needs an approving review before it can merge; it is ready for review" + ), + ); + assert_eq!(spec_prompts(&scenario).len(), 1); + assert_eq!( + result + .stderr + .matches("merging origin/main into issue-20") + .count(), + 1, + "{}", + result.stderr + ); + let number = spec["number"].as_u64().unwrap().to_string(); + assert!( + scenario + .gh_calls_of("pr", "merge") + .iter() + .all(|call| call[2] != number) + ); + for ticket in ["issue-21", "issue-22"] { + assert_eq!(pr_from(&scenario, ticket).unwrap()["state"], "MERGED"); + } + assert_eq!(scenario.origin_log("main").unwrap(), ["Initial commit"]); + assert!( + !scenario + .origin_log("issue-20") + .unwrap() + .iter() + .any(|subject| subject.contains("failed run")) + ); + scenario.assert_cleaned_up("issue-20"); + let requests = resend.requests(); + assert_eq!(requests.len(), 1); + assert_contains( + requests[0].body["subject"].as_str().unwrap(), + ": review required", + ); + assert_contains( + requests[0].body["text"].as_str().unwrap(), + "Result: review required\n", + ); + } +} + #[test] fn a_policy_refusal_on_the_spec_pr_leaves_it_ready_and_exits_1() { let scenario = linear_spec(); From a4184a6457f7c91860a691d44434c7cb07a5c5fe Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 16 Oct 2026 20:30:32 +0000 Subject: [PATCH 3/3] Record review of imported issue-707 commits Review since 506f9b73fa20b0d3bb45958638aa18374a72909c with independent Standards and Spec reviewers. All three changed files read on both axes. Standards: one low-severity Primitive Obsession judgement declined with locality evidence in PR #708. Spec: no findings. No source fixes warranted. Review-only empty commit; reports retained in .thirdshift-review-ClfBLi/. Validation: formatting, Clippy, 23 ending tests, 26 Merge run tests, 31 notification tests and 18 Spec review tests passed.