diff --git a/docs/code-navigation.md b/docs/code-navigation.md index 1cd834ac..a5e99e87 100644 --- a/docs/code-navigation.md +++ b/docs/code-navigation.md @@ -10,11 +10,11 @@ Start with the issue's terms in `GLOSSARY.md`, then use this index to find their | Command limit, Pass skip at its start | `src/command_limit.rs` — `hold_pass` (count and take a slot under the short lock, the lock of a `PassKind` for `Sharing::OneOfAKind`, the pre-Command-limit lock) and `hold_slot` (a Command on an Issue URL); `src/launch.rs` — `start`, which a Pickup run, Architect run and Security run call; `src/reservation.rs` — `Reservation::of_published`, an Architect plan or Security fix reserved before it is marked ready; `src/run.rs` and `src/processes.rs` take a slot for a hand-started Command | `tests/pickup_run.rs`, `tests/architect_run.rs`, `tests/process.rs`, `tests/user_config.rs` | | Ticket run, Continuation, Issue branch | `src/run.rs`; `src/branch.rs` | `tests/fresh_run.rs`, `tests/continuation.rs`, `tests/numbered_branches.rs` | | Worktree | `src/worktree.rs` and `src/worktree/` | `src/worktree/acquisition_tests.rs`, `src/worktree/cleanup_tests.rs` | -| Review run, pull-request Process input | `src/review.rs` — parsed Pull request URL; `src/review/reservation.rs` — origin reservation and `in-progress` lifetime; `src/processes/review.rs` — author Pass admission, Built-in and Custom pull-request Processes, captured-head sessions and recommendation delivery; `src/config/review.rs` — per-repository extra validation commands; `src/prompt.rs` — worktree validation and reporting instructions; `src/github/review.rs` — author/marker/base-head eligibility and filtered review input; `src/worktree/skill.rs` — head-branch checkout | `tests/review_run.rs`, `tests/review_pass.rs`, `tests/review_security.rs`, `tests/review_validation_after_security.rs` | +| Review run, pull-request Process input | `src/review.rs` — parsed Pull request URL; `src/review/reservation.rs` — origin reservation and `in-progress` lifetime; `src/processes/review.rs` — author Pass admission, Built-in and Custom pull-request Processes, captured-head sessions and recommendation delivery; `src/config/review.rs` — per-repository extra validation commands; `src/prompt.rs` — worktree validation and reporting instructions; `src/github/review.rs` — author/marker/base-head eligibility and filtered review input; `src/processes/review/delivery.rs` — push, CI watch and Repairs, delegating its mergeability gate to `src/pull_request.rs` — `ensure_review_ready`; `src/worktree/skill.rs` — head-branch checkout | `tests/review_run.rs`, `src/pull_request.rs` inline tests (Review readiness through `ensure_review_ready`), `tests/review_pass.rs`, `tests/review_security.rs`, `tests/review_validation_after_security.rs` | | Fix run, unattended Fix Pass | `src/fix.rs` — Command-limit admission, local occupancy pass-over, candidate reservation and Delivery; `src/fix/base_ci.rs` — unscoped Pass's Base branch repair; `src/github/base_ci.rs` — tip CI, re-run and repair deduplication; `src/github/review.rs` — `fix_candidates`, `fix_candidate_branch`, current-head eligibility; `src/worktree.rs` — `occupying_checkout`; `src/ci.rs` — `needs_repair`; `src/processes/fix.rs` — Built-in sequence | `tests/fix_run.rs`, `tests/fix_pass.rs`, `tests/fix_base_ci.rs` | | Spec run, Spec branch, Spec PR | `src/processes/spec.rs` — validated Built-in Spec sequence from `processes/spec-run.toml`; `src/spec_run.rs` — Ticket scheduling and accounting; Spec PR accounting and the Self-merge check of a Spec PR changed under its Spec run: `src/pull_request.rs` — `SpecPrChanged`, `ensure_spec_pr_unchanged`, `gate_self_merge_on`; `src/child_run.rs` — `SpecPr` | `tests/spec_run.rs`, `tests/process/spec_run.rs` | | Ticket board, Tickets checklist, Unready Ticket, Startable Ticket | `src/spec_run/ticket_board.rs` — `TicketBoard`, `next_to_start`; `src/spec_run.rs` — `unstartable`, shared with `src/ready.rs`; `src/labels.rs` | Inline tests; `tests/spec_run.rs` | -| Delivery, Repair loop, Foreign commit | `src/delivery.rs` — `Delivery`; `src/delivery/repair_loop.rs`; Delivery stage: `src/delivery/stage.rs` — `Stage`, `Tracker`, its table to the `Stopped at:` and Command record stages | Inline tests; `tests/merge_run.rs` | +| Delivery, Repair loop, Foreign commit | `src/delivery.rs` — `Delivery`; `src/delivery/repair_loop.rs`; Delivery stage: `src/delivery/stage.rs` — `Stage`, `Tracker`, its table to the `Stopped at:` and Command record stages; pull-request readiness for Delivery, Fix and Review runs: `src/pull_request.rs` — `ensure_ready_and_mergeable`, `is_mergeable`, `ensure_review_ready`, one snapshot validation and mergeability poll | Inline tests, including `src/pull_request.rs` readiness tests through those operations; `tests/merge_run.rs` | | Failed ticket run | `src/failed_run.rs` | `tests/failed_run.rs` | | Recovery | `src/recovery.rs` — `Recovery`, its add-a-fact methods and `lines`; the Recovery comment, `Recovery::comment`, `comment` and `cause` | Inline tests in `src/recovery.rs`; `tests/failed_run.rs`, `tests/spec_run.rs` | | CI watch, Check re-run, Abandoned job, Declined CI fix | `src/ci.rs` — `Watch`, `assess`, `after_repair`; Delivery constructs one watch in `src/delivery.rs` — `RunOutside`; `src/delivery/repair_loop.rs` requests assessments and reports completed CI-fix Repairs | Inline CI assessment and Repair loop tests; `src/github/execution_tests.rs`; `tests/ci.rs` | diff --git a/src/github.rs b/src/github.rs index 1d86be35..11afb8ca 100644 --- a/src/github.rs +++ b/src/github.rs @@ -96,6 +96,7 @@ pub(crate) fn operational_cause(error: &anyhow::Error) -> Option<&'static str> { /// GitHub CLI operations bound to an immutable interruption policy. /// Ordinary views stop for Command interruption; completion views finish /// independently without changing the recorded request or other views. +#[derive(Clone)] pub struct GitHub { interruption: Interruption, wait_for_quota: bool, diff --git a/src/processes/review/delivery.rs b/src/processes/review/delivery.rs index 3bdf6519..9c77e869 100644 --- a/src/processes/review/delivery.rs +++ b/src/processes/review/delivery.rs @@ -3,6 +3,7 @@ use super::*; use crate::ci::{self, Ci, FailedChecks, Watch}; use crate::github::review::ReviewInput; +use crate::pull_request::PullRequest; pub(super) struct Delivery<'a> { pub git: &'a Git, @@ -221,37 +222,14 @@ impl Delivery<'_> { Ok(()) } + /// Delegate to the pull-request owner's Review readiness for `head`. fn require_mergeable(&self, head: &str) -> Result<()> { - let mut backoff = crate::poll::Backoff::new(); - loop { - self.unchanged_head(head)?; - let observed = self - .github - .pr_snapshot(&self.pr.issue, &self.pr.url)? - .context("captured pull request disappeared during Review run")?; - ensure!( - observed.head_commit == head - && observed.pr.number == self.pr.issue.number - && observed.pr.url == self.pr.url - && observed.pr.head == self.snapshot.branch - && observed.pr.base == self.snapshot.base - && observed.pr.is_open() - && !observed.pr.is_draft - && !observed.from_fork, - "pull request changed during Review run readiness" - ); - self.unchanged_head(head)?; - match observed.mergeable { - crate::github::Mergeable::Yes => return Ok(()), - crate::github::Mergeable::No => bail!( - "pull request needs updating before merge; the author must resolve the conflict with {}", - self.snapshot.base - ), - crate::github::Mergeable::Unknown => { - let state = (observed.head_commit, observed.mergeable); - crate::poll::sleep(backoff.observe(state, std::time::Instant::now()))?; - } - } - } + PullRequest::on_pull_request( + self.pr, + &self.snapshot.branch, + &self.snapshot.base, + self.github.clone(), + ) + .ensure_review_ready(head) } } diff --git a/src/pull_request.rs b/src/pull_request.rs index 9cfe82af..c00ff915 100644 --- a/src/pull_request.rs +++ b/src/pull_request.rs @@ -1,11 +1,12 @@ -//! Pull request ownership for Spec Ticket accounting and Delivery. Numbered -//! observations, checklist edits, capture transitions, readiness and merge -//! reconciliation stay behind this interface. +//! Pull request ownership for Spec Ticket accounting, Delivery and Review +//! readiness. Numbered observations, checklist edits, capture transitions, +//! readiness and merge reconciliation stay behind this interface. use anyhow::{Context, Result, bail}; use crate::child_run::SpecPr; use crate::failed_run::PolicyRefusal; +use crate::github::review::ReviewPullRequest; use crate::github::{GitHub, Mergeable, PrSnapshot, PrState, PullRequest as Record}; use crate::harness::Choice; use crate::harness::built_with::MARKER as BUILT_WITH_MARKER; @@ -109,6 +110,16 @@ pub(crate) struct PullRequest { adapter: A, } +/// How a readiness check differs by caller. Ordinary Delivery and Fix may +/// see a new head and wait for mergeability only within the grace period. +/// Review stands behind one exact head, rereads Review eligibility around +/// each numbered observation and waits until GitHub answers. +#[derive(Clone, Copy)] +enum Readiness<'h> { + Delivery, + Review { head: &'h str }, +} + #[derive(PartialEq, Eq)] enum CaptureScope { TicketAccounting, @@ -508,7 +519,7 @@ impl PullRequest { /// coherent numbered observation. Waiting remains interruptible. pub fn ensure_ready_and_mergeable(&mut self) -> Result<()> { progress::step("checking the PR is open, ready and mergeable"); - let snapshot = self.settled_ready_snapshot()?; + let snapshot = self.settled_ready_snapshot(Readiness::Delivery)?; let url = &snapshot.pr.url; match snapshot.mergeable { Mergeable::Yes => Ok(()), @@ -523,33 +534,117 @@ impl PullRequest { /// [`Self::ensure_ready_and_mergeable`] polls it. Mergeability GitHub has /// not worked out within the grace period is not mergeable. pub fn is_mergeable(&mut self) -> Result { - Ok(self.settled_ready_snapshot()?.mergeable == Mergeable::Yes) + Ok(self.settled_ready_snapshot(Readiness::Delivery)?.mergeable == Mergeable::Yes) + } + + /// Review readiness for the exact `head` a Review run reviewed or pushed. + /// Each poll reads Review eligibility, observes and validates this + /// numbered PR, then reads eligibility again before deciding. Branch + /// names stay fixed; the Base branch tip may advance. Unknown + /// mergeability waits, interruptibly, until GitHub answers. This never + /// marks ready, merges, pushes or comments, and a merged target is a + /// failed check, never [`AlreadyMerged`]. + pub fn ensure_review_ready(&mut self, head: &str) -> Result<()> { + let snapshot = self.settled_ready_snapshot(Readiness::Review { head })?; + match snapshot.mergeable { + Mergeable::Yes => Ok(()), + Mergeable::No => bail!( + "pull request needs updating before merge; the author must resolve the conflict with {}", + self.base + ), + // Review waits out Unknown; it never reaches this decision. + Mergeable::Unknown => bail!( + "GitHub has not worked out whether PR {} is mergeable", + snapshot.pr.url + ), + } } /// An observation of this open, ready PR, polled while its mergeability - /// is unknown until the grace period ends. - fn settled_ready_snapshot(&mut self) -> Result { + /// is unknown: until the grace period ends for ordinary Delivery, and + /// until GitHub answers or the wait is interrupted for Review. + fn settled_ready_snapshot(&mut self, readiness: Readiness) -> Result { let deadline = std::time::Instant::now() + self.adapter.grace_period(); let mut waiting_line = poll::WaitingLine::new(); let mut backoff = poll::Backoff::new(); loop { - let snapshot = self.observe(false)?.context("no PR found")?; - self.validate(&snapshot, true)?; - match snapshot.mergeable { - Mergeable::Unknown if std::time::Instant::now() < deadline => { + self.ensure_review_eligible(readiness)?; + let snapshot = self.ready_snapshot(readiness)?; + self.ensure_review_eligible(readiness)?; + match (snapshot.mergeable, readiness) { + (Mergeable::Unknown, Readiness::Review { .. }) => {} + (Mergeable::Unknown, Readiness::Delivery) + if std::time::Instant::now() < deadline => + { if waiting_line.should_report(()) { progress::step(format!( "waiting for GitHub to calculate mergeability of PR {}", snapshot.pr.url )); } - let state = (snapshot.head_commit, snapshot.mergeable); - self.adapter - .pause(backoff.observe(state, std::time::Instant::now()))?; } - Mergeable::Yes | Mergeable::No | Mergeable::Unknown => return Ok(snapshot), + (Mergeable::Yes | Mergeable::No | Mergeable::Unknown, _) => return Ok(snapshot), } + let state = (snapshot.head_commit, snapshot.mergeable); + self.adapter + .pause(backoff.observe(state, std::time::Instant::now()))?; + } + } + + /// One numbered observation, validated as an open, ready PR of this + /// owner. Review also requires the exact captured URL and `head`, and + /// reports a merged target as a failed check. + fn ready_snapshot(&mut self, readiness: Readiness) -> Result { + let Readiness::Review { head } = readiness else { + let snapshot = self.observe(false)?.context("no PR found")?; + self.validate(&snapshot, true)?; + return Ok(snapshot); + }; + let snapshot = self + .adapter + .observe(&self.issue, &self.selector(), false)? + .context("captured pull request disappeared during Review run")?; + let pr = &snapshot.pr; + let refused = self.capture(&snapshot).is_err() + // Checked before `validate`, which reports a merged target as + // ordinary Delivery's `AlreadyMerged` completion. + || !pr.is_open() + || self.validate(&snapshot, true).is_err() + || self.identified.as_ref().is_none_or(|captured| captured.url != pr.url) + || snapshot.head_commit != head; + // Review's diagnostic is this one line, whatever changed. + if refused { + bail!("pull request changed during Review run readiness"); + } + Ok(snapshot) + } + + /// For Review, a fresh read of the existing Review eligibility, with its + /// Trusted account checks, at the captured branch names and exact `head`. + fn ensure_review_eligible(&mut self, readiness: Readiness) -> Result<()> { + let Readiness::Review { head } = readiness else { + return Ok(()); + }; + let target = self.review_target()?; + let current = self.adapter.review_eligibility(&target)?; + if current.branch != self.branch || current.base != self.base { + bail!("pull request changed during Review run"); } + if current.head != head { + bail!("pull request changed during Review run CI watch"); + } + Ok(()) + } + + fn review_target(&self) -> Result { + let pr = self + .identified + .as_ref() + .context("Review readiness needs a captured pull request")?; + Ok(crate::review::PullRequestUrl { + url: pr.url.clone(), + issue: self.issue.clone(), + }) } /// A fresh gate and best-effort review decision read precede each numbered @@ -618,12 +713,16 @@ impl PullRequest { )) } - fn observe(&mut self, completion: bool) -> Result> { - let selector = self - .identified + /// The captured number, or this Issue branch before capture. + fn selector(&self) -> String { + self.identified .as_ref() .map(|pr| pr.number.to_string()) - .unwrap_or_else(|| self.branch.clone()); + .unwrap_or_else(|| self.branch.clone()) + } + + fn observe(&mut self, completion: bool) -> Result> { + let selector = self.selector(); let snapshot = self.adapter.observe(&self.issue, &selector, completion)?; if let Some(snapshot) = &snapshot { self.capture(snapshot)?; @@ -750,6 +849,12 @@ pub(crate) trait Adapter { ) -> Result; fn review_blocks_merge(&mut self, issue: &IssueUrl, number: u64) -> bool; fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()>; + /// The existing Review eligibility read, with its Trusted account and + /// repository checks. + fn review_eligibility( + &mut self, + pr: &crate::review::PullRequestUrl, + ) -> Result; fn pause(&mut self, delay: std::time::Duration) -> Result<()> { poll::sleep(delay) } @@ -822,6 +927,12 @@ impl Adapter for GitHub { fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()> { GitHub::merge(self, issue, number, head) } + fn review_eligibility( + &mut self, + pr: &crate::review::PullRequestUrl, + ) -> Result { + self.review_pull_request(pr) + } } const CHECKLIST_START: &str = ""; @@ -923,6 +1034,12 @@ mod tests { effects: Vec<(String, bool)>, retarget_after_edit: bool, grace: Duration, + /// Review eligibility reads, answered like `observations`. + eligibility: VecDeque, + /// Review eligibility reads, numbered observations and pauses, in order. + reads: Vec<&'static str>, + /// The pause after this many pauses is interrupted. + interrupt_pause: Option, } impl Default for World { fn default() -> Self { @@ -946,6 +1063,9 @@ mod tests { effects: Vec::new(), retarget_after_edit: false, grace: Duration::ZERO, + eligibility: VecDeque::from([Ok(eligible())]), + reads: Vec::new(), + interrupt_pause: None, } } } @@ -965,6 +1085,7 @@ mod tests { assert_eq!(issue.repo_slug(), "acme/widgets"); let mut world = self.0.borrow_mut(); world.selected.push((selector.to_string(), completion)); + world.reads.push("observe"); let answers = if completion { &mut world.completion } else { @@ -1066,7 +1187,32 @@ mod tests { } Ok(()) } + fn review_eligibility( + &mut self, + pr: &crate::review::PullRequestUrl, + ) -> Result { + crate::process::Interruption::Ordinary.check()?; + assert_eq!((pr.url.as_str(), pr.issue.number), (URL, 12)); + let mut world = self.0.borrow_mut(); + world.reads.push("eligibility"); + let answer = if world.eligibility.len() > 1 { + world.eligibility.pop_front().unwrap() + } else { + world + .eligibility + .front() + .expect("no eligibility scripted") + .clone() + }; + answer.map_err(anyhow::Error::msg) + } fn pause(&mut self, _: Duration) -> Result<()> { + let mut world = self.0.borrow_mut(); + world.reads.push("pause"); + let pauses = world.reads.iter().filter(|read| **read == "pause").count(); + if world.interrupt_pause.is_some_and(|after| pauses > after) { + bail!("interrupted"); + } Ok(()) } fn grace_period(&self) -> Duration { @@ -1090,6 +1236,43 @@ mod tests { observed } + type Eligibility = std::result::Result; + + /// The Review eligibility read of the PR [`snapshot`] observes. + fn eligible() -> ReviewPullRequest { + ReviewPullRequest { + title: "Widgets".to_string(), + body: String::new(), + head: "watched".to_string(), + branch: "issue-7".to_string(), + base: "main".to_string(), + base_head: "base".to_string(), + } + } + + /// A Review run's owner of the captured PR, as Review constructs it. + fn review_owner(world: World) -> (PullRequest, Rc>) { + let world = Rc::new(RefCell::new(world)); + let pr = crate::review::PullRequestUrl::parse(URL).unwrap(); + ( + PullRequest::on_pull_request(&pr, "issue-7", "main", Scripted(world.clone())), + world, + ) + } + + fn unknown() -> PrSnapshot { + let mut unknown = snapshot(); + unknown.mergeable = Mergeable::Unknown; + unknown + } + + /// Readiness is read-only: no transition, body edit or merge request. + fn assert_read_only(world: &World) { + assert!(world.ready.is_empty() && world.draft.is_empty()); + assert!(world.merges.is_empty() && world.edits.is_empty()); + assert!(world.effects.is_empty() && world.created.is_empty()); + } + #[derive(Clone, Copy, Debug)] enum ObservationChange { WrongBase, @@ -2040,4 +2223,272 @@ mod tests { false, ); } + + #[test] + fn review_readiness_reads_eligibility_around_one_validated_numbered_observation() { + let (mut pr, world) = review_owner(World::default()); + pr.ensure_review_ready("watched").unwrap(); + let world = world.borrow(); + assert_eq!(world.reads, ["eligibility", "observe", "eligibility"]); + assert_eq!(world.selected, [("12".to_string(), false)]); + assert_read_only(&world); + } + + #[test] + fn review_readiness_requires_the_exact_head_and_url_that_ordinary_readiness_does_not() { + let mut moved = snapshot(); + moved.head_commit = "later".to_string(); + let mut spelled = snapshot(); + spelled.pr.url = format!("{URL}/"); + for observed in [moved, spelled] { + let (mut ordinary, _) = owner(World { + observations: VecDeque::from([Ok(Some(observed.clone()))]), + ..World::default() + }); + ordinary.ensure_ready_and_mergeable().unwrap(); + let (mut review, world) = review_owner(World { + observations: VecDeque::from([Ok(Some(observed))]), + ..World::default() + }); + let error = review.ensure_review_ready("watched").unwrap_err(); + assert_eq!( + error.to_string(), + "pull request changed during Review run readiness" + ); + // Validation failure stops before the final eligibility read. + assert_eq!(world.borrow().reads, ["eligibility", "observe"]); + } + } + + #[test] + fn review_eligibility_changes_before_the_numbered_observation_stop_without_observing() { + let mut moved = eligible(); + moved.head = "later".to_string(); + let mut renamed = eligible(); + renamed.branch = "renamed".to_string(); + let mut retargeted = eligible(); + retargeted.base = "develop".to_string(); + for (answer, message) in [ + (Ok(moved), "pull request changed during Review run CI watch"), + (Ok(renamed), "pull request changed during Review run"), + (Ok(retargeted), "pull request changed during Review run"), + ( + Err("author is not a Trusted account"), + "author is not a Trusted account", + ), + (Err("pull request is merged"), "pull request is merged"), + ] { + let (mut pr, world) = review_owner(World { + eligibility: VecDeque::from([answer]), + ..World::default() + }); + let error = pr.ensure_review_ready("watched").unwrap_err(); + assert_eq!(error.to_string(), message); + assert_eq!(world.borrow().reads, ["eligibility"]); + assert_read_only(&world.borrow()); + } + } + + #[test] + fn review_eligibility_changes_after_a_valid_numbered_observation_refuse_readiness() { + let mut moved = eligible(); + moved.head = "later".to_string(); + let mut retargeted = eligible(); + retargeted.base = "develop".to_string(); + for after in [ + Ok(moved), + Ok(retargeted), + Err("author is not a Trusted account"), + ] { + let (mut pr, world) = review_owner(World { + eligibility: VecDeque::from([Ok(eligible()), after]), + ..World::default() + }); + assert!(pr.ensure_review_ready("watched").is_err()); + assert_eq!( + world.borrow().reads, + ["eligibility", "observe", "eligibility"] + ); + } + } + + #[test] + fn every_unknown_review_poll_refuses_each_common_snapshot_change() { + for change in [ + ObservationChange::WrongBase, + ObservationChange::WrongHead, + ObservationChange::Fork, + ObservationChange::WrongRepository, + ObservationChange::Replacement, + ObservationChange::Closed, + ObservationChange::Merged, + ObservationChange::Missing, + ObservationChange::Unreadable, + ObservationChange::Draft, + ] { + let (mut pr, world) = review_owner(World { + observations: VecDeque::from([Ok(Some(unknown())), change.answer(unknown())]), + ..World::default() + }); + let error = pr.ensure_review_ready("watched").unwrap_err(); + let refusal = match change { + ObservationChange::Missing => "captured pull request disappeared during Review run", + ObservationChange::Unreadable => "read failed", + ObservationChange::WrongBase + | ObservationChange::WrongHead + | ObservationChange::Fork + | ObservationChange::WrongRepository + | ObservationChange::Replacement + | ObservationChange::Closed + | ObservationChange::Merged + | ObservationChange::Ready + | ObservationChange::Draft => "pull request changed during Review run readiness", + }; + assert_eq!(format!("{error:#}"), refusal, "{change:?}"); + // A merged target fails Review; it is never merged completion. + assert!(!error.is::(), "{change:?}"); + assert!( + error.chain().all(|cause| !cause.is::()), + "{change:?}" + ); + let world = world.borrow(); + assert_eq!( + world.reads, + [ + "eligibility", + "observe", + "eligibility", + "pause", + "eligibility", + "observe" + ], + "{change:?}" + ); + assert!(world.selected.iter().all(|(selector, _)| selector == "12")); + assert_read_only(&world); + } + } + + #[test] + fn review_readiness_allows_the_base_branch_tip_to_advance() { + let mut advanced = eligible(); + advanced.base_head = "newer-base".to_string(); + let (mut pr, _) = review_owner(World { + eligibility: VecDeque::from([Ok(eligible()), Ok(advanced)]), + ..World::default() + }); + pr.ensure_review_ready("watched").unwrap(); + } + + #[test] + fn review_waits_through_unknown_mergeability_that_ordinary_readiness_does_not() { + let observations = || { + VecDeque::from([ + Ok(Some(unknown())), + Ok(Some(unknown())), + Ok(Some(unknown())), + Ok(Some(snapshot())), + ]) + }; + let (mut ordinary, _) = owner(World { + observations: observations(), + ..World::default() + }); + let error = ordinary.ensure_ready_and_mergeable().unwrap_err(); + assert!(error.to_string().contains("has not worked out"), "{error}"); + let (mut review, world) = review_owner(World { + observations: observations(), + ..World::default() + }); + review.ensure_review_ready("watched").unwrap(); + let world = world.borrow(); + assert_eq!( + world.reads.iter().filter(|read| **read == "pause").count(), + 3 + ); + assert_eq!( + world + .reads + .iter() + .filter(|read| **read == "eligibility") + .count(), + 8 + ); + assert_read_only(&world); + } + + #[test] + fn an_interrupted_review_wait_stops_without_another_read() { + let (mut pr, world) = review_owner(World { + observations: VecDeque::from([Ok(Some(unknown()))]), + interrupt_pause: Some(1), + ..World::default() + }); + let error = pr.ensure_review_ready("watched").unwrap_err(); + assert_eq!(error.to_string(), "interrupted"); + let reads = world.borrow().reads.clone(); + assert_eq!(reads.last(), Some(&"pause")); + assert_eq!(reads.iter().filter(|read| **read == "pause").count(), 2); + } + + #[test] + fn a_conflict_asks_the_review_author_to_update_the_branch() { + let mut conflicting = snapshot(); + conflicting.mergeable = Mergeable::No; + let (mut pr, world) = review_owner(World { + observations: VecDeque::from([Ok(Some(conflicting))]), + ..World::default() + }); + let error = pr.ensure_review_ready("watched").unwrap_err(); + assert_eq!( + error.to_string(), + "pull request needs updating before merge; the author must resolve the conflict with main" + ); + assert_eq!( + world.borrow().reads, + ["eligibility", "observe", "eligibility"] + ); + assert_read_only(&world.borrow()); + } + + #[test] + fn review_readiness_refusal_keeps_its_single_line_diagnostic() { + let mut moved = snapshot(); + moved.head_commit = "later".to_string(); + let (mut pr, _) = review_owner(World { + observations: VecDeque::from([Ok(Some(moved))]), + ..World::default() + }); + let error = pr.ensure_review_ready("watched").unwrap_err(); + assert_eq!( + format!("{error:#}"), + "pull request changed during Review run readiness" + ); + } + + #[test] + fn a_head_commit_moved_during_unknown_review_polling_refuses_readiness() { + let mut moved = unknown(); + moved.head_commit = "later".to_string(); + let (mut pr, world) = review_owner(World { + observations: VecDeque::from([Ok(Some(unknown())), Ok(Some(moved))]), + ..World::default() + }); + let error = pr.ensure_review_ready("watched").unwrap_err(); + assert_eq!( + error.to_string(), + "pull request changed during Review run readiness" + ); + assert_eq!( + world.borrow().reads, + [ + "eligibility", + "observe", + "eligibility", + "pause", + "eligibility", + "observe" + ] + ); + } }