From 00614e5fe8b46a3b5493f29a08eb76f046fce4ee Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 7 Oct 2026 10:24:49 -0400 Subject: [PATCH 1/2] Unify Spec PR accounting and Delivery ownership (#515) --- CONTEXT.md | 4 +- src/delivery.rs | 99 +-- src/delivery/pull_request.rs | 844 ------------------ src/delivery/repair_loop.rs | 2 +- src/github.rs | 15 +- src/github/execution_tests.rs | 58 +- src/main.rs | 1 + src/pull_request.rs | 1533 +++++++++++++++++++++++++++++++++ src/run.rs | 9 +- src/spec_run.rs | 80 +- src/spec_run/spec_pr.rs | 681 --------------- tests/spec_run.rs | 59 ++ 12 files changed, 1716 insertions(+), 1669 deletions(-) delete mode 100644 src/delivery/pull_request.rs create mode 100644 src/pull_request.rs delete mode 100644 src/spec_run/spec_pr.rs diff --git a/CONTEXT.md b/CONTEXT.md index 778e4356b..0a851e9f8 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -117,7 +117,7 @@ The **Issue branch** of the **Spec** in a **Spec run**, branched off the **Base _Avoid_: integration branch, feature branch **Spec PR**: -The pull request from the **Spec branch** into the **Base branch** that closes the **Spec**. It is left ready for review once every **Ticket** is done, or merged by a **Self-merge** when the **Spec run** was asked to merge; a draft listing what is missing otherwise. It opens as a draft once the first Ticket lands, with the **Tickets checklist** in its body. +The pull request from the **Spec branch** into the **Base branch** that closes the **Spec**. It is left ready for review once every **Ticket** is done, or merged by a **Self-merge** when the **Spec run** was asked to merge; a draft listing what is missing otherwise. It opens as a draft once the first Ticket lands, with the **Tickets checklist** in its body. Ticket accounting captures the resumed or created PR by number in the Spec's repository. Every draft or checklist operation reobserves that number and checks its open state, Spec branch, Base branch and repository ownership; an invalid or missing PR is never replaced during accounting. A landing must pass those checks before the Spec review can start. Delivery explicitly ends this accounting capture before the Spec review and captures its own PR afterward. **Tickets checklist**: The part of the **Spec PR**'s body, between fixed markers, that thirdshift writes: one line per **Ticket**, ticked once it is done, with its pull request, or saying whether it is not started, running, failed, interrupted, blocked, in a cycle or unready. It is rewritten as each Ticket starts and ends, and put back after the **Spec review** rewrites the body. @@ -184,7 +184,7 @@ A **Run** that picks up an existing **Issue branch**, one with no pull request o A follow-up agent session a **Run** starts after the pull request exists, or a **Spec run** starts on its **Spec PR** after the **Spec review**: to resolve a merge conflict with the **Base branch** or the **Issue branch** on `origin`, to fix failing CI checks, or, in a **Merge run**, to review and fix **Foreign commits**. **Delivery**: -What a **Run**, or a **Spec run** for its **Spec PR**, does from its worktree once its work begins. It runs the opening session (the implement session, or the **Spec review**), pushes, marks the pull request ready, keeps it mergeable and green through the **Repair loop**, and, in a **Merge run**, does the **Self-merge**. The first successful observation after the opening session captures one pull request number and URL in the issue's repository; all later observations and transitions use that identity. Readiness and every mergeability poll validate its expected **Issue branch** and **Base branch**, open state and readiness where required. A missing, closed or retargeted captured PR fails Delivery; a replacement from the same branch is never substituted. A Delivery that fails takes the **Failed run** path, drafting that same captured open PR after preserving work. +What a **Run**, or a **Spec run** for its **Spec PR**, does from its worktree once its work begins. It runs the opening session (the implement session, or the **Spec review**), pushes, marks the pull request ready, keeps it mergeable and green through the **Repair loop**, and, in a **Merge run**, does the **Self-merge**. For a Spec PR, Delivery ends Ticket accounting's capture before the Spec review, allowing that session to replace the PR. The first successful observation after the opening session, or during Failed run completion after an opening failure, captures one pull request number and URL in the issue's repository; all later observations and transitions use that identity. Readiness and every mergeability poll validate its expected **Issue branch** and **Base branch**, open state and readiness where required. A missing, closed or retargeted captured PR fails Delivery; a replacement from the same branch is never substituted. A Delivery that fails takes the **Failed run** path, drafting that same captured open PR after preserving work. Spec PR checklist restoration uses that Delivery capture before readiness and for final accounting after failure. **Repair loop**: The rounds a **Delivery** goes through until the pull request's head is mergeable with the **Base branch** and its CI is green or absent. Each round merges the Base branch in (and, in a **Merge run**, any **Foreign commits**), pushes, watches CI, and starts a **Repair** for a conflict or for the branch's own red checks. Repairs and upstream moves have fixed budgets. diff --git a/src/delivery.rs b/src/delivery.rs index c54904a22..e1c0a05fe 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -14,11 +14,9 @@ use std::path::PathBuf; use anyhow::Result; -mod pull_request; mod repair_loop; -pub(crate) use pull_request::Identified; -use pull_request::{MergeAttempt, PullRequest}; +use crate::pull_request::{Identified, MergeAttempt, PullRequest}; use crate::base_fix::BaseFix; use crate::ci::{self, Ci, FailedChecks}; @@ -51,13 +49,6 @@ pub struct Delivery<'a> { pub harness: &'a Choice, } -/// The outcome and immutable captured PR information. The Spec PR uses the -/// latter for checklist accounting even when capture happened during salvage. -pub(crate) struct Delivered { - pub outcome: Result, - pub pull_request: Option, -} - /// The session a Delivery opens with: the implement session, or the Spec /// review. pub struct Opening<'a> { @@ -75,8 +66,8 @@ impl Delivery<'_> { /// branch to origin. Then run the opening session, push the branch, for /// any commit the session left unpushed, write the line that says what it /// was built with in the pull request's body, only warning if that fails, - /// run `before_ready`, and mark the pull request ready, failing unless it - /// exists, is open and targets the Base branch. Then keep it mergeable + /// restore the optional Tickets checklist, and mark the pull request ready, + /// failing unless it exists, is open and targets the Base branch. Then keep it mergeable /// and its CI green through the Repair loop, and for [`Goal::Merged`], /// Self-merge it. A merge that fails goes back round the Repair loop and /// is tried again on the new head; if that round finds nothing to fix, @@ -87,8 +78,9 @@ impl Delivery<'_> { self, worktree: Worktree, opening: Opening, - before_ready: impl FnOnce(&Identified) -> Result<()>, - ) -> Delivered { + pull_request: &mut PullRequest, + checklist: Option<&str>, + ) -> Result { let route = Route { issue: self.issue, base: self.base, @@ -96,8 +88,7 @@ impl Delivery<'_> { goal: self.goal, harness: self.harness, }; - let mut pull_request = - PullRequest::new(self.issue, worktree.branch(), self.base, GitHub::new()); + pull_request.begin_delivery(); let (delivered, log) = Sessions::within(self.logs, worktree.path(), self.harness, |sessions| { let mut outside = InWorktree { @@ -106,11 +97,11 @@ impl Delivery<'_> { base: self.base, base_fix: self.base_fix, sessions, - pull_request: &mut pull_request, + pull_request, }; - route.steps(&mut outside, &opening, before_ready) + route.steps(&mut outside, &opening, checklist) }); - let outcome = match delivered { + match delivered { Ok(pr_url) => Ok(Reached { pr_url, goal: self.goal, @@ -121,14 +112,10 @@ impl Delivery<'_> { let mut outside = OnFailure { base: self.base, worktree, - pull_request: &mut pull_request, + pull_request, }; Err(fail(&mut outside, log, error)) } - }; - Delivered { - outcome, - pull_request: pull_request.info(), } } } @@ -146,15 +133,14 @@ struct Route<'a> { impl Route<'_> { /// [`Delivery::deliver`]'s steps, through `outside`, in order: catch up /// from origin if `opening` says to, the opening session, the push, the - /// line saying what it was built with, `before_ready`, marking the pull - /// request ready, taking it to the goal - /// through the Repair loop, then for [`Goal::Merged`], the steps after + /// line saying what it was built with, checklist restoration, marking the + /// pull request ready, taking it to the goal through the Repair loop, then for [`Goal::Merged`], the steps after /// the merge. Returns the pull request's URL. fn steps( &self, outside: &mut O, opening: &Opening, - before_ready: impl FnOnce(&Identified) -> Result<()>, + checklist: Option<&str>, ) -> Result { if opening.catch_up_from_origin { outside.catch_up()?; @@ -162,7 +148,7 @@ impl Route<'_> { outside.session(opening.kind, &opening.prompt)?; outside.push()?; self.write_built_with(outside); - let pr = outside.mark_pr_ready(before_ready)?; + let pr = outside.mark_pr_ready(checklist)?; outside.take_to_goal(&pr.url, self.goal)?; if self.goal == Goal::Merged { self.after_merge(outside, &pr); @@ -296,10 +282,7 @@ trait Outside { /// Write the annotation through the captured pull request module. fn write_built_with(&mut self, choice: &Choice) -> Result<()>; /// Validate and mark the captured pull request ready. - fn mark_pr_ready( - &mut self, - before_ready: impl FnOnce(&Identified) -> Result<()>, - ) -> Result; + fn mark_pr_ready(&mut self, checklist: Option<&str>) -> Result; /// Take the ready pull request at `pr_url` to `goal` through the Repair /// loop. fn take_to_goal(&mut self, pr_url: &str, goal: Goal) -> Result<()>; @@ -360,11 +343,8 @@ impl Outside for InWorktree<'_> { self.pull_request.write_built_with(choice) } - fn mark_pr_ready( - &mut self, - before_ready: impl FnOnce(&Identified) -> Result<()>, - ) -> Result { - self.pull_request.mark_ready(before_ready) + fn mark_pr_ready(&mut self, checklist: Option<&str>) -> Result { + self.pull_request.mark_ready(checklist) } fn take_to_goal(&mut self, pr_url: &str, goal: Goal) -> Result<()> { @@ -548,7 +528,7 @@ mod tests { Session, Push, Annotation, - BeforeReady, + Checklist, Ready, Goal, Preserve, @@ -563,7 +543,7 @@ mod tests { Session(String), Push, Annotation, - BeforeReady, + Checklist, Ready, Goal(Goal), Preserve(String), @@ -613,17 +593,11 @@ mod tests { self.calls.push(Call::Annotation); self.check(Fails::Annotation) } - fn mark_pr_ready( - &mut self, - before_ready: impl FnOnce(&Identified) -> Result<()>, - ) -> Result { - let pr = Identified { - number: 12, - url: PR_URL.to_string(), - }; - self.calls.push(Call::BeforeReady); - self.check(Fails::BeforeReady)?; - before_ready(&pr)?; + fn mark_pr_ready(&mut self, checklist: Option<&str>) -> Result { + if checklist.is_some() { + self.calls.push(Call::Checklist); + self.check(Fails::Checklist)?; + } self.calls.push(Call::Ready); self.check(Fails::Ready)?; Ok(Identified { @@ -693,15 +667,15 @@ mod tests { catch_up_from_origin: spec, }; route - .steps(outside, &opening, |_| Ok(())) + .steps(outside, &opening, spec.then_some("Tickets")) .map_err(|error| fail(outside, outside.log.clone(), error)) } #[test] - fn delivery_pushes_then_annotates_then_runs_the_callback_before_readiness() { + fn spec_delivery_pushes_then_annotates_then_restores_checklist_before_readiness() { let mut outside = Scripted::default(); assert_eq!( - deliver(&mut outside, Goal::ReadyForReview, false) + deliver(&mut outside, Goal::ReadyForReview, true) .ok() .as_deref(), Some(PR_URL) @@ -709,10 +683,11 @@ mod tests { assert_eq!( outside.calls, [ - Call::Session("implement".to_string()), + Call::CatchUp, + Call::Session("spec-review".to_string()), Call::Push, Call::Annotation, - Call::BeforeReady, + Call::Checklist, Call::Ready, Call::Goal(Goal::ReadyForReview) ] @@ -730,19 +705,19 @@ mod tests { } #[test] - fn annotation_failure_warns_and_still_runs_the_callback_and_readiness() { + fn annotation_failure_warns_and_still_restores_checklist_and_marks_ready() { let mut outside = Scripted { failing: Some(Fails::Annotation), ..Default::default() }; - assert!(deliver(&mut outside, Goal::ReadyForReview, false).is_ok()); + assert!(deliver(&mut outside, Goal::ReadyForReview, true).is_ok()); assert!( - matches!(&outside.calls[3], Call::Warning(line) if line.contains("could not write")) + matches!(&outside.calls[4], Call::Warning(line) if line.contains("could not write")) ); assert_eq!( - outside.calls[4..], + outside.calls[5..], [ - Call::BeforeReady, + Call::Checklist, Call::Ready, Call::Goal(Goal::ReadyForReview) ] @@ -765,7 +740,7 @@ mod tests { Fails::CatchUp, Fails::Session, Fails::Push, - Fails::BeforeReady, + Fails::Checklist, Fails::Ready, Fails::Goal, ] { diff --git a/src/delivery/pull_request.rs b/src/delivery/pull_request.rs deleted file mode 100644 index 52f9b924b..000000000 --- a/src/delivery/pull_request.rs +++ /dev/null @@ -1,844 +0,0 @@ -//! Delivery's captured pull request and its transitions. Callers request -//! annotation, readiness, Self-merge or Failed run finishing; discovery, -//! numbered observations, target validation and reconciliation stay here. - -use anyhow::{Context, Result, bail}; - -use crate::github::{GitHub, Mergeable, PrSnapshot, PrState}; -use crate::harness::Choice; -use crate::issue::IssueUrl; -use crate::{poll, progress}; - -/// Immutable information for reporting and confirmed Self-merge finishing. -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct Identified { - pub number: u64, - pub url: String, -} - -/// A gate error is terminal (`Err`); only a refused request can return to -/// the Repair loop. A successful guarded command needs no additional read. -pub(super) enum MergeAttempt { - Merged, - Refused(anyhow::Error), -} - -/// Created before the opening session; binds lazily on the first successful -/// observation of the expected repository's own Issue branch. A wrong Base -/// branch still binds identity so Failed run salvage can draft that PR. -pub(super) struct PullRequest { - issue: IssueUrl, - branch: String, - base: String, - identified: Option, - adapter: A, -} - -impl PullRequest { - pub fn new(issue: &IssueUrl, branch: &str, base: &str, adapter: A) -> Self { - Self { - issue: issue.clone(), - branch: branch.to_string(), - base: base.to_string(), - identified: None, - adapter, - } - } - - /// Missing PRs are a no-op; the caller warns on errors, then readiness - /// supplies the decisive gate. Body reads and edits always use the number. - pub fn write_built_with(&mut self, choice: &Choice) -> Result<()> { - let Some(snapshot) = self.observe(false)? else { - return Ok(()); - }; - let body = self.adapter.body(&self.issue, snapshot.pr.number)?; - let written = with_built_with(&body, choice); - if written != body { - progress::step(format!( - "writing \"{}\" in the pull request's body", - choice.built_with() - )); - self.adapter - .set_body(&self.issue, snapshot.pr.number, &written)?; - } - Ok(()) - } - - /// Observe afresh and validate the expected branches and open state - /// before marking this number ready. Already-ready PRs need no request. - pub fn mark_ready( - &mut self, - before_ready: impl FnOnce(&Identified) -> Result<()>, - ) -> Result { - // Annotation normally captured identity already. If it was absent or - // unreadable, discover before handing the callback a numbered identity. - if self.identified.is_none() { - self.observe(false)?.context("no PR found")?; - } - let identified = self.info().expect("observation captured identity"); - before_ready(&identified)?; - progress::step("checking the PR"); - let snapshot = self.observe(false)?.context("no PR found")?; - self.validate(&snapshot, false)?; - if snapshot.pr.is_draft { - self.adapter.ready(&self.issue, snapshot.pr.number)?; - } - Ok(identified) - } - - /// Immutable information for Delivery's final accounting, including - /// identity captured only by Failed run completion discovery. - pub fn info(&self) -> Option { - self.identified.clone() - } - - /// Every unknown-mergeability poll repeats all readiness checks on one - /// 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 deadline = std::time::Instant::now() + self.adapter.grace_period(); - loop { - let snapshot = self.observe(false)?.context("no PR found")?; - self.validate(&snapshot, true)?; - match snapshot.mergeable { - Mergeable::Yes => return Ok(()), - Mergeable::No => bail!("PR {} is not mergeable", snapshot.pr.url), - Mergeable::Unknown if std::time::Instant::now() >= deadline => { - bail!( - "GitHub has not worked out whether PR {} is mergeable", - snapshot.pr.url - ); - } - Mergeable::Unknown => self.adapter.pause()?, - } - } - } - - /// A fresh gate immediately precedes the numbered request. 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 { - self.ensure_ready_and_mergeable()?; - let number = self - .identified - .as_ref() - .expect("gate captured identity") - .number; - let Err(error) = self.adapter.merge(&self.issue, number, watched) else { - return Ok(MergeAttempt::Merged); - }; - // Completion never clears or changes the recorded interruption. Only - // this PR, merged at the watched head and expected branches, counts. - if let Ok(Some(snapshot)) = self.observe(true) - && snapshot.pr.state == PrState::Merged - && snapshot.pr.base == self.base - && snapshot.head_commit == watched - { - return Ok(MergeAttempt::Merged); - } - Ok(MergeAttempt::Refused(error)) - } - - /// After Worktree preservation, finish on the captured identity. Before - /// any capture, make one completion discovery. A wrong target may be - /// drafted for salvage; it never authorizes readiness or Self-merge. - pub fn finish_failed_run(&mut self, keep_ready: bool) -> Result> { - let Some(snapshot) = self.observe(true)? else { - return Ok(None); - }; - if !snapshot.pr.is_open() { - return Ok(None); - } - if !snapshot.pr.is_draft && !keep_ready { - self.adapter.draft(&self.issue, snapshot.pr.number)?; - } - Ok(Some( - self.identified - .as_ref() - .expect("observation captured identity") - .url - .clone(), - )) - } - - fn observe(&mut self, completion: bool) -> Result> { - let selector = self - .identified - .as_ref() - .map(|pr| pr.number.to_string()) - .unwrap_or_else(|| self.branch.clone()); - let snapshot = self.adapter.observe(&self.issue, &selector, completion)?; - if let Some(snapshot) = &snapshot { - if snapshot.from_fork { - bail!("PR {} is from a fork", snapshot.pr.url); - } - if snapshot.pr.head != self.branch { - bail!("PR head is {}, not {}", snapshot.pr.head, self.branch); - } - if let Some(pr) = &self.identified { - if snapshot.pr.number != pr.number { - bail!( - "PR identity changed from #{} to #{}", - pr.number, - snapshot.pr.number - ); - } - } else { - self.identified = Some(Identified { - number: snapshot.pr.number, - url: snapshot.pr.url.clone(), - }); - } - } - Ok(snapshot) - } - - fn validate(&self, snapshot: &PrSnapshot, ready: bool) -> Result<()> { - let pr = &snapshot.pr; - if !pr.is_open() { - bail!("PR {} is {}, not open", pr.url, pr.state); - } - if pr.base != self.base { - bail!("PR targets {}, not {}", pr.base, self.base); - } - if ready && pr.is_draft { - bail!("PR {} is a draft", pr.url); - } - Ok(()) - } -} - -/// The existing GitHub transport and a scripted test adapter vary here. -/// Selection and transition policy stay in PullRequest. -pub(super) trait Adapter { - fn observe( - &mut self, - issue: &IssueUrl, - selector: &str, - completion: bool, - ) -> Result>; - fn body(&mut self, issue: &IssueUrl, number: u64) -> Result; - fn set_body(&mut self, issue: &IssueUrl, number: u64, body: &str) -> Result<()>; - fn ready(&mut self, issue: &IssueUrl, number: u64) -> Result<()>; - fn draft(&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() - } - fn grace_period(&self) -> std::time::Duration { - poll::grace_period() - } -} - -impl Adapter for GitHub { - fn observe( - &mut self, - issue: &IssueUrl, - selector: &str, - completion: bool, - ) -> Result> { - if completion { - self.completion().pr_snapshot(issue, selector) - } else { - self.pr_snapshot(issue, selector) - } - } - fn body(&mut self, issue: &IssueUrl, number: u64) -> Result { - self.pr_body(issue, number) - } - fn set_body(&mut self, issue: &IssueUrl, number: u64, body: &str) -> Result<()> { - self.set_pr_body(issue, number, body) - } - fn ready(&mut self, issue: &IssueUrl, number: u64) -> Result<()> { - self.mark_ready(issue, &number.to_string()) - } - fn draft(&mut self, issue: &IssueUrl, number: u64) -> Result<()> { - self.completion() - .convert_to_draft(issue, &number.to_string()) - } - fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()> { - GitHub::merge(self, issue, number, head) - } -} - -const BUILT_WITH_MARKER: &str = ""; - -/// Preserve unrelated text and replace only thirdshift's annotation line. -fn with_built_with(body: &str, choice: &Choice) -> String { - let line = format!("{} {BUILT_WITH_MARKER}", choice.built_with()); - if body.contains(BUILT_WITH_MARKER) { - return body - .split_inclusive('\n') - .map(|old| { - if old.trim_end().ends_with(BUILT_WITH_MARKER) { - let end = &old[old.trim_end_matches(['\r', '\n']).len()..]; - format!("{line}{end}") - } else { - old.to_string() - } - }) - .collect(); - } - if body.trim().is_empty() { - return format!("{line}\n"); - } - format!("{}\n\n{line}\n", body.trim_end()) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::github::PullRequest as Record; - use std::{cell::RefCell, collections::VecDeque, rc::Rc, time::Duration}; - - const URL: &str = "https://github.com/acme/widgets/pull/12"; - type Answer = std::result::Result, &'static str>; - - fn snapshot() -> PrSnapshot { - PrSnapshot { - pr: Record { - number: 12, - url: URL.to_string(), - state: PrState::Open, - head: "issue-7".to_string(), - base: "main".to_string(), - is_draft: false, - }, - mergeable: Mergeable::Yes, - head_commit: "watched".to_string(), - from_fork: false, - } - } - - /// Scripted GitHub observations and affected PRs. Outcomes are inspected - /// at the external transport, never through the owner's private identity. - struct World { - observations: VecDeque, - completion: VecDeque, - selected: Vec<(String, bool)>, - ready: Vec, - draft: Vec, - merges: Vec<(u64, String)>, - edits: Vec<(u64, String)>, - body: String, - refused: bool, - merge_signal: Option, - body_error: bool, - grace: Duration, - } - impl Default for World { - fn default() -> Self { - Self { - observations: VecDeque::from([Ok(Some(snapshot()))]), - completion: VecDeque::from([Ok(Some(snapshot()))]), - selected: Vec::new(), - ready: Vec::new(), - draft: Vec::new(), - merges: Vec::new(), - edits: Vec::new(), - body: "Closes #7\n".to_string(), - refused: false, - merge_signal: None, - body_error: false, - grace: Duration::ZERO, - } - } - } - - #[derive(Clone)] - struct Scripted(Rc>); - impl Adapter for Scripted { - fn observe( - &mut self, - issue: &IssueUrl, - selector: &str, - completion: bool, - ) -> Result> { - if !completion { - crate::process::Interruption::Ordinary.check()?; - } - assert_eq!(issue.repo_slug(), "acme/widgets"); - let mut world = self.0.borrow_mut(); - world.selected.push((selector.to_string(), completion)); - let answers = if completion { - &mut world.completion - } else { - &mut world.observations - }; - let answer = if answers.len() > 1 { - answers.pop_front().unwrap() - } else { - answers.front().expect("no snapshot scripted").clone() - }; - answer.map_err(anyhow::Error::msg) - } - fn body(&mut self, _: &IssueUrl, number: u64) -> Result { - assert_eq!(number, 12); - let world = self.0.borrow(); - if world.body_error { - bail!("body unavailable"); - } - Ok(world.body.clone()) - } - fn set_body(&mut self, _: &IssueUrl, number: u64, body: &str) -> Result<()> { - let mut world = self.0.borrow_mut(); - world.edits.push((number, body.to_string())); - world.body = body.to_string(); - Ok(()) - } - fn ready(&mut self, _: &IssueUrl, number: u64) -> Result<()> { - self.0.borrow_mut().ready.push(number); - Ok(()) - } - fn draft(&mut self, _: &IssueUrl, number: u64) -> Result<()> { - self.0.borrow_mut().draft.push(number); - Ok(()) - } - fn merge(&mut self, _: &IssueUrl, number: u64, head: &str) -> Result<()> { - let mut world = self.0.borrow_mut(); - world.merges.push((number, head.to_string())); - if let Some(signal) = world.merge_signal { - signal_hook::low_level::raise(signal).unwrap(); - bail!("interrupted"); - } - if world.refused { - bail!("merge request failed"); - } - Ok(()) - } - fn pause(&mut self) -> Result<()> { - Ok(()) - } - fn grace_period(&self) -> Duration { - self.0.borrow().grace - } - } - - fn owner(world: World) -> (PullRequest, Rc>) { - let world = Rc::new(RefCell::new(world)); - let issue = IssueUrl::parse("https://github.com/acme/widgets/issues/7").unwrap(); - ( - PullRequest::new(&issue, "issue-7", "main", Scripted(world.clone())), - world, - ) - } - - #[test] - fn annotation_preserves_text_is_idempotent_and_replaces_the_previous_choice() { - let (mut pr, world) = owner(World::default()); - let choice = Choice { - model: Some("opus".to_string()), - effort: Some("high".to_string()), - ..Choice::default() - }; - pr.write_built_with(&Choice::default()).unwrap(); - pr.write_built_with(&Choice::default()).unwrap(); - pr.write_built_with(&choice).unwrap(); - let expected = - "Closes #7\n\nBuilt with claude · opus · high \n"; - assert_eq!(world.borrow().body, expected); - assert_eq!(world.borrow().edits.len(), 2); - assert!(world.borrow().edits.iter().all(|(number, _)| *number == 12)); - } - - #[test] - fn annotation_replaces_its_line_in_place_preserving_unrelated_text_and_line_endings() { - let body = "Summary\r\nBuilt with old \r\n\r\nCloses #7\r\n"; - let (mut pr, world) = owner(World { - body: body.to_string(), - ..World::default() - }); - pr.write_built_with(&Choice::default()).unwrap(); - assert_eq!( - world.borrow().body, - "Summary\r\nBuilt with claude · default model · default effort \r\n\r\nCloses #7\r\n" - ); - } - - #[test] - fn missing_or_failed_initial_observations_allow_discovery_at_readiness() { - for first in [Ok(None), Err("transport failed")] { - let (mut pr, world) = owner(World { - observations: VecDeque::from([first, Ok(Some(snapshot()))]), - ..World::default() - }); - let _ = pr.write_built_with(&Choice::default()); - assert_eq!(pr.mark_ready(|_| Ok(())).unwrap().url, URL); - assert_eq!( - world.borrow().selected[..2], - [ - ("issue-7".to_string(), false), - ("issue-7".to_string(), false) - ] - ); - assert!( - world.borrow().selected[2..] - .iter() - .all(|(selector, _)| selector == "12") - ); - } - } - - #[test] - fn a_body_read_failure_still_binds_identity_for_readiness_and_failure_finishing() { - let (mut pr, world) = owner(World { - body_error: true, - ..World::default() - }); - assert!(pr.write_built_with(&Choice::default()).is_err()); - pr.mark_ready(|_| Ok(())).unwrap(); - assert_eq!(pr.finish_failed_run(false).unwrap().as_deref(), Some(URL)); - assert_eq!(world.borrow().draft, [12]); - assert_eq!( - world.borrow().selected[1..], - [("12".to_string(), false), ("12".to_string(), true)] - ); - } - - #[test] - fn initial_wrong_target_binds_identity_but_never_marks_ready_and_can_be_drafted() { - let mut wrong = snapshot(); - wrong.pr.base = "develop".to_string(); - let (mut pr, world) = owner(World { - observations: VecDeque::from([Ok(Some(wrong.clone()))]), - completion: VecDeque::from([Ok(Some(wrong))]), - ..World::default() - }); - assert_eq!( - pr.mark_ready(|_| Ok(())).unwrap_err().to_string(), - "PR targets develop, not main" - ); - assert_eq!(pr.finish_failed_run(false).unwrap().as_deref(), Some(URL)); - assert!(world.borrow().ready.is_empty()); - assert_eq!(world.borrow().draft, [12]); - assert_eq!( - world.borrow().selected.last(), - Some(&("12".to_string(), true)) - ); - } - - #[test] - fn discovery_rejects_fork_and_unexpected_issue_branch() { - for fork in [false, true] { - let mut wrong = snapshot(); - if fork { - wrong.from_fork = true; - } else { - wrong.pr.head = "other".to_string(); - } - let (mut pr, world) = owner(World { - observations: VecDeque::from([Ok(Some(wrong))]), - ..World::default() - }); - assert!(pr.mark_ready(|_| Ok(())).is_err()); - assert!(world.borrow().ready.is_empty()); - } - } - - #[test] - fn readiness_marks_only_a_draft_and_returns_immutable_identity() { - for draft in [false, true] { - let mut observed = snapshot(); - observed.pr.is_draft = draft; - let (mut pr, world) = owner(World { - observations: VecDeque::from([Ok(Some(observed))]), - ..World::default() - }); - assert_eq!( - pr.mark_ready(|_| Ok(())).unwrap(), - Identified { - number: 12, - url: URL.to_string() - } - ); - assert_eq!( - world.borrow().ready, - if draft { vec![12] } else { Vec::new() } - ); - } - } - - #[test] - fn the_callback_receives_captured_identity_and_readiness_is_rechecked_after_it() { - let (mut pr, world) = owner(World::default()); - pr.write_built_with(&Choice::default()).unwrap(); - let error = pr - .mark_ready(|identified| { - assert_eq!( - identified, - &Identified { - number: 12, - url: URL.to_string() - } - ); - let mut retargeted = snapshot(); - retargeted.pr.base = "develop".to_string(); - world.borrow_mut().observations = VecDeque::from([Ok(Some(retargeted))]); - Ok(()) - }) - .unwrap_err(); - assert_eq!(error.to_string(), "PR targets develop, not main"); - assert!(world.borrow().ready.is_empty()); - } - - #[test] - fn every_unknown_mergeability_snapshot_revalidates_target_state_and_ownership() { - // First UNKNOWN is followed immediately by the changed snapshot; - // no pause is needed to script a change during polling. - for change in ["base", "closed", "draft", "head", "fork", "number"] { - let mut unknown = snapshot(); - unknown.mergeable = Mergeable::Unknown; - let mut changed = unknown.clone(); - match change { - "base" => changed.pr.base = "develop".to_string(), - "closed" => changed.pr.state = PrState::Closed, - "draft" => changed.pr.is_draft = true, - "head" => changed.pr.head = "other".to_string(), - "fork" => changed.from_fork = true, - "number" => changed.pr.number = 13, - _ => unreachable!(), - } - let (mut pr, world) = owner(World { - observations: VecDeque::from([ - Ok(Some(snapshot())), - Ok(Some(snapshot())), - Ok(Some(unknown)), - Ok(Some(changed)), - ]), - grace: Duration::from_secs(60), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - assert!(pr.ensure_ready_and_mergeable().is_err(), "{change}"); - assert!( - world.borrow().selected[1..] - .iter() - .all(|(selector, _)| selector == "12") - ); - } - } - - #[test] - fn missing_closed_draft_conflicting_and_unknown_prs_fail_final_readiness() { - let mut closed = snapshot(); - closed.pr.state = PrState::Closed; - let mut draft = snapshot(); - draft.pr.is_draft = true; - let mut conflicting = snapshot(); - conflicting.mergeable = Mergeable::No; - let mut unknown = snapshot(); - unknown.mergeable = Mergeable::Unknown; - for (answer, message) in [ - (Ok(None), "no PR found"), - (Ok(Some(closed)), "is closed, not open"), - (Ok(Some(draft)), "is a draft"), - (Ok(Some(conflicting)), "is not mergeable"), - (Ok(Some(unknown)), "has not worked out"), - ] { - let (mut pr, _) = owner(World { - observations: VecDeque::from([Ok(Some(snapshot())), Ok(Some(snapshot())), answer]), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - assert!( - pr.ensure_ready_and_mergeable() - .unwrap_err() - .to_string() - .contains(message) - ); - } - } - - #[test] - fn disappearance_or_transport_failure_after_capture_never_rediscovers_by_branch() { - for missing in [Ok(None), Err("gone")] { - let (mut pr, world) = owner(World { - observations: VecDeque::from([ - Ok(Some(snapshot())), - Ok(Some(snapshot())), - missing.clone(), - ]), - completion: VecDeque::from([missing]), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - assert!(pr.ensure_ready_and_mergeable().is_err()); - let _ = pr.finish_failed_run(false); - assert_eq!( - world.borrow().selected[2..], - [("12".to_string(), false), ("12".to_string(), true)] - ); - assert!(world.borrow().draft.is_empty()); - } - } - - #[test] - fn closed_original_or_unexpected_replacement_cannot_be_reported_or_modified() { - for replaced in [false, true] { - let mut original = snapshot(); - if replaced { - original.pr.number = 13; - } else { - original.pr.state = PrState::Closed; - } - let (mut pr, world) = owner(World { - completion: VecDeque::from([Ok(Some(original))]), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - assert!(pr.finish_failed_run(false).ok().flatten().is_none()); - assert!(world.borrow().draft.is_empty()); - } - } - - #[test] - fn a_fresh_invalid_or_unreadable_merge_gate_issues_no_request() { - let mut wrong = snapshot(); - wrong.pr.base = "develop".to_string(); - for gate in [Ok(Some(wrong)), Err("unreadable")] { - let (mut pr, world) = owner(World { - observations: VecDeque::from([Ok(Some(snapshot())), Ok(Some(snapshot())), gate]), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - assert!(pr.merge("watched").is_err()); - assert!(world.borrow().merges.is_empty()); - } - } - - #[test] - fn successful_guarded_merge_requires_no_completion_observation() { - let (mut pr, world) = owner(World { - completion: VecDeque::from([Err("completion unavailable")]), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - assert!(matches!(pr.merge("watched").unwrap(), MergeAttempt::Merged)); - assert_eq!(world.borrow().merges, [(12, "watched".to_string())]); - assert!( - world - .borrow() - .selected - .iter() - .all(|(_, completion)| !completion) - ); - } - - #[test] - fn failed_requests_are_confirmed_only_for_the_same_pr_head_and_expected_branches() { - for change in [ - "none", - "number", - "head", - "base", - "branch", - "fork", - "open", - "closed", - "missing", - "unreadable", - ] { - let mut merged = snapshot(); - merged.pr.state = PrState::Merged; - match change { - "number" => merged.pr.number = 13, - "head" => merged.head_commit = "foreign".to_string(), - "base" => merged.pr.base = "develop".to_string(), - "branch" => merged.pr.head = "other".to_string(), - "fork" => merged.from_fork = true, - "open" => merged.pr.state = PrState::Open, - "closed" => merged.pr.state = PrState::Closed, - _ => (), - } - let completion = match change { - "missing" => Ok(None), - "unreadable" => Err("unreadable"), - _ => Ok(Some(merged)), - }; - let (mut pr, world) = owner(World { - refused: true, - completion: VecDeque::from([completion]), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - let attempt = pr.merge("watched").unwrap(); - assert_eq!( - matches!(attempt, MergeAttempt::Merged), - change == "none", - "{change}" - ); - if let MergeAttempt::Refused(error) = attempt { - assert_eq!(error.to_string(), "merge request failed"); - } - assert_eq!( - world.borrow().selected.last(), - Some(&("12".to_string(), true)) - ); - } - } - - #[test] - fn failed_run_can_discover_before_capture_and_policy_refusal_keeps_ready() { - for keep_ready in [false, true] { - let (mut pr, world) = owner(World::default()); - assert_eq!( - pr.finish_failed_run(keep_ready).unwrap().as_deref(), - Some(URL) - ); - assert_eq!(world.borrow().selected, [("issue-7".to_string(), true)]); - assert_eq!( - world.borrow().draft, - if keep_ready { Vec::new() } else { vec![12] } - ); - } - let mut draft = snapshot(); - draft.pr.is_draft = true; - let (mut pr, world) = owner(World { - completion: VecDeque::from([Ok(Some(draft))]), - ..World::default() - }); - assert_eq!(pr.finish_failed_run(false).unwrap().as_deref(), Some(URL)); - assert!(world.borrow().draft.is_empty()); - } - fn interrupted_merge(name: &str, confirmed: bool) { - crate::test_support::with_recorded_signal(name, |signal| { - let mut completion = snapshot(); - if confirmed { - completion.pr.state = PrState::Merged; - } - let (mut pr, world) = owner(World { - merge_signal: Some(signal), - completion: VecDeque::from([Ok(Some(completion))]), - ..World::default() - }); - pr.mark_ready(|_| Ok(())).unwrap(); - let attempt = pr.merge("watched").unwrap(); - assert_eq!(matches!(attempt, MergeAttempt::Merged), confirmed); - if let MergeAttempt::Refused(error) = attempt { - assert_eq!(error.to_string(), "interrupted"); - } - assert!(crate::interrupt::requested()); - assert_eq!( - pr.ensure_ready_and_mergeable().unwrap_err().to_string(), - "interrupted" - ); - assert!(world.borrow().selected.contains(&("12".to_string(), true))); - }); - } - - #[test] - fn an_interrupted_request_can_be_confirmed_without_clearing_interruption() { - interrupted_merge( - "delivery::pull_request::tests::an_interrupted_request_can_be_confirmed_without_clearing_interruption", - true, - ); - } - - #[test] - fn an_unconfirmed_interrupted_request_keeps_its_failure_and_interruption() { - interrupted_merge( - "delivery::pull_request::tests::an_unconfirmed_interrupted_request_keeps_its_failure_and_interruption", - false, - ); - } -} diff --git a/src/delivery/repair_loop.rs b/src/delivery/repair_loop.rs index 8b422182d..9ef2ca7c4 100644 --- a/src/delivery/repair_loop.rs +++ b/src/delivery/repair_loop.rs @@ -7,7 +7,7 @@ use anyhow::{Result, bail}; -use super::pull_request::MergeAttempt; +use crate::pull_request::MergeAttempt; use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::PolicyRefusal; diff --git a/src/github.rs b/src/github.rs index f34911566..e8af0e71b 100644 --- a/src/github.rs +++ b/src/github.rs @@ -363,16 +363,9 @@ impl GitHub { } } - /// The pull request whose head is `branch`, if `gh` finds one. - pub fn pull_request_for(&self, issue: &IssueUrl, branch: &str) -> Result> { - let Some(json) = self.optional_pr_view(issue, branch, PR_FIELDS)? else { - return Ok(None); - }; - PullRequest::from_json(&json).map(Some) - } - - /// A coherent Delivery observation, selected by head branch for initial - /// discovery and by number after capture. Policy belongs to Delivery. + /// A coherent pull request observation, selected by head branch for initial + /// discovery and by number after capture. Policy belongs to the pull + /// request owner. pub fn pr_snapshot(&self, issue: &IssueUrl, selector: &str) -> Result> { let Some(json) = self.optional_pr_view(issue, selector, SNAPSHOT_FIELDS)? else { return Ok(None); @@ -840,7 +833,7 @@ const PR_FIELDS: &str = "number,url,state,headRefName,baseRefName,isDraft"; const SNAPSHOT_FIELDS: &str = "number,url,state,headRefName,baseRefName,isDraft,mergeable,headRefOid,isCrossRepository"; -/// The fields needed for one Delivery gate, read together. +/// The fields needed for one pull request observation, read together. #[derive(Debug, Clone)] pub struct PrSnapshot { pub pr: PullRequest, diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index 203b8f690..76a17b6e8 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -108,12 +108,7 @@ fn ordinary_github_refuses_reads_and_writes_before_spawn() { |signal| { let fixture = Fixture::new("echo 'no pull requests found' >&2; exit 1"); let github = GitHub::new(); - assert!( - github - .pull_request_for(&issue(), "issue-7") - .unwrap() - .is_none() - ); + assert!(github.pr_snapshot(&issue(), "issue-7").unwrap().is_none()); fs::remove_file(fixture.path("pid")).unwrap(); signal_hook::low_level::raise(signal).unwrap(); assert_eq!( @@ -131,7 +126,7 @@ fn ordinary_github_refuses_reads_and_writes_before_spawn() { github.checks_on(&issue(), "abc").err().unwrap().to_string(), "interrupted" ); - assert!(github.pull_request_for(&issue(), "issue-7").is_err()); + assert!(github.pr_snapshot(&issue(), "issue-7").is_err()); assert!( !fixture.path("pid").exists(), "an ordinary operation spawned after interruption" @@ -212,7 +207,7 @@ fn an_optional_pr_probe_propagates_active_cancellation_instead_of_absence() { std::process::id() )); let error = GitHub::new() - .pull_request_for(&issue(), "issue-7") + .pr_snapshot(&issue(), "issue-7") .expect_err("cancellation became absence"); assert_eq!(error.to_string(), "interrupted"); assert!(!exists(fixture.pid().unwrap())); @@ -366,3 +361,50 @@ fn github_keeps_its_exit_and_parse_diagnostics() { }, ); } + +#[test] +fn spec_creation_observes_the_returned_number_with_completion_after_interruption() { + with_recorded_signal( + "github::execution_tests::spec_creation_observes_the_returned_number_with_completion_after_interruption", + |signal| { + let fixture = Fixture::new(""); + fixture.script(&format!( + r#" +printf '%s\n' "$1 $2 $3" >> '{calls}' +case "$1 $2 $3" in + 'pr view issue-7') + if test -f '{discovered}'; then + printf '%s' '{{"number":13,"url":"https://github.com/acme/widgets/pull/13","state":"OPEN","headRefName":"issue-7","baseRefName":"main","isDraft":true,"mergeable":"MERGEABLE","headRefOid":"head","isCrossRepository":false}}' + else + touch '{discovered}' + echo 'no pull requests found' >&2 + exit 1 + fi ;; + 'issue view 7') printf '%s' '{{"title":"Widgets"}}' ;; + 'pr create --repo') printf ' https://github.com/acme/widgets/pull/12\n\n' ;; + 'pr view 12') printf '%s' '{{"number":12,"url":"https://github.com/acme/widgets/pull/12","state":"OPEN","headRefName":"issue-7","baseRefName":"main","isDraft":true,"mergeable":"MERGEABLE","headRefOid":"head","isCrossRepository":false}}' ;; + *) exit 1 ;; +esac +"#, + calls = fixture.path("calls").display(), + discovered = fixture.path("discovered").display(), + )); + let mut pr = + crate::pull_request::PullRequest::new(&issue(), "issue-7", "main", GitHub::new()); + pr.resume_spec().unwrap(); + signal_hook::low_level::raise(signal).unwrap(); + + assert_eq!( + pr.land_spec("- [x] #8 landed\n").unwrap(), + "https://github.com/acme/widgets/pull/12" + ); + assert_eq!( + fs::read_to_string(fixture.path("calls")).unwrap(), + "pr view issue-7\nissue view 7\npr create --repo\npr view 12\n" + ); + assert!(interrupt::requested()); + pr.begin_delivery(); + assert_eq!(pr.mark_ready(None).unwrap_err().to_string(), "interrupted"); + }, + ); +} diff --git a/src/main.rs b/src/main.rs index 7257b725d..11486b7ca 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,6 +29,7 @@ mod progress; mod prompt; #[cfg(test)] mod prompts_page; +mod pull_request; mod ready; mod resend_key; mod run; diff --git a/src/pull_request.rs b/src/pull_request.rs new file mode 100644 index 000000000..7d19d0b0f --- /dev/null +++ b/src/pull_request.rs @@ -0,0 +1,1533 @@ +//! Pull request ownership for Spec Ticket accounting and Delivery. Numbered +//! observations, checklist edits, capture transitions, readiness and merge +//! reconciliation stay behind this interface. + +use anyhow::{Context, Result, bail}; + +use crate::github::{GitHub, Mergeable, PrSnapshot, PrState}; +use crate::harness::Choice; +use crate::issue::IssueUrl; +use crate::{poll, progress}; + +/// Immutable information for reporting and confirmed Self-merge finishing. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct Identified { + pub number: u64, + pub url: String, +} + +/// A gate error is terminal (`Err`); only a refused request can return to +/// the Repair loop. A successful guarded command needs no additional read. +pub(crate) enum MergeAttempt { + Merged, + Refused(anyhow::Error), +} + +/// Ticket accounting binds on resume or creation. Delivery binds lazily on +/// the first successful observation of the repository's own Issue branch. A wrong Base +/// branch still binds identity so Failed run salvage can draft that PR. +pub(crate) struct PullRequest { + issue: IssueUrl, + branch: String, + base: String, + identified: Option, + scope: CaptureScope, + adapter: A, +} + +#[derive(PartialEq, Eq)] +enum CaptureScope { + TicketAccounting, + Delivery, +} + +impl PullRequest { + pub fn new(issue: &IssueUrl, branch: &str, base: &str, adapter: A) -> Self { + Self { + issue: issue.clone(), + branch: branch.to_string(), + base: base.to_string(), + identified: None, + scope: CaptureScope::Delivery, + adapter, + } + } + + /// Resume accounting with an ordinary discovery, then draft the observed + /// number only after a fresh numbered gate. Closed PRs allow a new landing. + pub fn resume_spec(&mut self) -> Result<()> { + self.scope = CaptureScope::TicketAccounting; + let snapshot = self.adapter.observe(&self.issue, &self.branch, false)?; + let Some(snapshot) = snapshot.filter(|snapshot| snapshot.pr.is_open()) else { + return Ok(()); + }; + self.capture(&snapshot)?; + self.validate(&snapshot, false)?; + if !snapshot.pr.is_draft { + let snapshot = self.accounting_snapshot(false)?; + if !snapshot.pr.is_draft { + self.adapter.draft(&self.issue, snapshot.pr.number, false)?; + } + } + Ok(()) + } + + /// End Ticket accounting once, before Delivery's opening work. That + /// session may replace the PR; the first later observation captures it. + pub fn begin_delivery(&mut self) { + if self.scope == CaptureScope::TicketAccounting { + self.identified = None; + self.scope = CaptureScope::Delivery; + } + } + + /// Progress text is optional. Invalid observations and failed writes warn + /// without replacing identity or granting readiness/merge authority. + pub fn show_checklist(&mut self, checklist: &str) { + if self.identified.is_none() { + return; + } + if let Err(error) = self.restore_checklist(checklist, true) { + Self::warn_checklist(&error); + } + } + + /// A landing's identity gate is decisive; its body write is optional. + /// Creation binds the returned URL before any numbered observation, so + /// neither missing information nor a replacement can trigger discovery. + pub fn land_spec(&mut self, checklist: &str) -> Result { + if self.identified.is_none() { + progress::step(format!("opening the Spec PR into {} as a draft", self.base)); + let title = self.adapter.spec_title(&self.issue)?; + let body = format!( + "The work on Spec #{number}, gathered from its Tickets on {branch}.\n\n\ + Closes #{number}\n\n{checklist}", + number = self.issue.number, + branch = self.branch, + checklist = between_markers(checklist), + ); + let url = + self.adapter + .create_draft(&self.issue, &self.branch, &self.base, &title, &body)?; + let url = url.trim().to_string(); + let number = self.number_from_url(&url)?; + self.identified = Some(Identified { number, url }); + let snapshot = self.accounting_snapshot(true)?; + if !snapshot.pr.is_draft { + bail!("the Spec PR just opened is not a draft"); + } + } else { + let snapshot = self.accounting_snapshot(true)?; + if let Err(error) = self.write_checklist(snapshot.pr.number, checklist, true) { + Self::warn_checklist(&error); + } + } + Ok(self + .identified + .as_ref() + .expect("landing captured identity") + .url + .clone()) + } + + fn warn_checklist(error: &anyhow::Error) { + progress::step(format!( + "could not update the Spec PR's Tickets checklist: {error:#}" + )); + } + + fn accounting_snapshot(&mut self, completion: bool) -> Result { + let snapshot = self + .observe(completion)? + .context("the Spec PR is not found")?; + self.validate(&snapshot, false)?; + Ok(snapshot) + } + + fn restore_checklist(&mut self, checklist: &str, completion: bool) -> Result<()> { + let snapshot = self.accounting_snapshot(completion)?; + self.write_checklist(snapshot.pr.number, checklist, completion) + } + + fn write_checklist(&mut self, number: u64, checklist: &str, completion: bool) -> Result<()> { + let body = self.adapter.body(&self.issue, number, completion)?; + let updated = with_checklist(&body, checklist); + if updated != body { + progress::step("updating the Spec PR's Tickets checklist"); + self.adapter + .set_body(&self.issue, number, &updated, completion)?; + } + Ok(()) + } + + /// Missing PRs are a no-op; the caller warns on errors, then readiness + /// supplies the decisive gate. Body reads and edits always use the number. + pub fn write_built_with(&mut self, choice: &Choice) -> Result<()> { + let Some(snapshot) = self.observe(false)? else { + return Ok(()); + }; + let body = self.adapter.body(&self.issue, snapshot.pr.number, false)?; + let written = with_built_with(&body, choice); + if written != body { + progress::step(format!( + "writing \"{}\" in the pull request's body", + choice.built_with() + )); + self.adapter + .set_body(&self.issue, snapshot.pr.number, &written, false)?; + } + Ok(()) + } + + /// Observe afresh and validate the expected branches and open state + /// before marking this number ready. Already-ready PRs need no request. + pub fn mark_ready(&mut self, checklist: Option<&str>) -> Result { + // Annotation normally captured identity already. If it was absent or + // unreadable, discover before restoring any checklist. + if self.identified.is_none() { + self.observe(false)?.context("no PR found")?; + } + let identified = self.info().expect("observation captured identity"); + if let Some(checklist) = checklist { + self.restore_checklist(checklist, false)?; + } + progress::step("checking the PR"); + let snapshot = self.observe(false)?.context("no PR found")?; + self.validate(&snapshot, false)?; + if snapshot.pr.is_draft { + self.adapter.ready(&self.issue, snapshot.pr.number)?; + } + Ok(identified) + } + + /// Immutable information for Delivery's final accounting, including + /// identity captured only by Failed run completion discovery. + pub fn info(&self) -> Option { + self.identified.clone() + } + + /// Every unknown-mergeability poll repeats all readiness checks on one + /// 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 deadline = std::time::Instant::now() + self.adapter.grace_period(); + loop { + let snapshot = self.observe(false)?.context("no PR found")?; + self.validate(&snapshot, true)?; + match snapshot.mergeable { + Mergeable::Yes => return Ok(()), + Mergeable::No => bail!("PR {} is not mergeable", snapshot.pr.url), + Mergeable::Unknown if std::time::Instant::now() >= deadline => { + bail!( + "GitHub has not worked out whether PR {} is mergeable", + snapshot.pr.url + ); + } + Mergeable::Unknown => self.adapter.pause()?, + } + } + } + + /// A fresh gate immediately precedes the numbered request. 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 { + self.ensure_ready_and_mergeable()?; + let number = self + .identified + .as_ref() + .expect("gate captured identity") + .number; + let Err(error) = self.adapter.merge(&self.issue, number, watched) else { + return Ok(MergeAttempt::Merged); + }; + // Completion never clears or changes the recorded interruption. Only + // this PR, merged at the watched head and expected branches, counts. + if let Ok(Some(snapshot)) = self.observe(true) + && snapshot.pr.state == PrState::Merged + && snapshot.pr.base == self.base + && snapshot.head_commit == watched + { + return Ok(MergeAttempt::Merged); + } + Ok(MergeAttempt::Refused(error)) + } + + /// After Worktree preservation, finish on the captured identity. Before + /// any capture, make one completion discovery. A wrong target may be + /// drafted for salvage; it never authorizes readiness or Self-merge. + pub fn finish_failed_run(&mut self, keep_ready: bool) -> Result> { + let Some(snapshot) = self.observe(true)? else { + return Ok(None); + }; + if !snapshot.pr.is_open() { + return Ok(None); + } + if !snapshot.pr.is_draft && !keep_ready { + self.adapter.draft(&self.issue, snapshot.pr.number, true)?; + } + Ok(Some( + self.identified + .as_ref() + .expect("observation captured identity") + .url + .clone(), + )) + } + + fn observe(&mut self, completion: bool) -> Result> { + let selector = self + .identified + .as_ref() + .map(|pr| pr.number.to_string()) + .unwrap_or_else(|| self.branch.clone()); + let snapshot = self.adapter.observe(&self.issue, &selector, completion)?; + if let Some(snapshot) = &snapshot { + self.capture(snapshot)?; + } + Ok(snapshot) + } + + fn capture(&mut self, snapshot: &PrSnapshot) -> Result<()> { + if snapshot.from_fork { + bail!("PR {} is from a fork", snapshot.pr.url); + } + if snapshot.pr.head != self.branch { + bail!("PR head is {}, not {}", snapshot.pr.head, self.branch); + } + if let Some(pr) = &self.identified + && snapshot.pr.number != pr.number + { + bail!( + "PR identity changed from #{} to #{}", + pr.number, + snapshot.pr.number + ); + } + if self.number_from_url(&snapshot.pr.url)? != snapshot.pr.number { + bail!( + "PR URL does not match observed number #{}", + snapshot.pr.number + ); + } + if self.identified.is_none() { + self.identified = Some(Identified { + number: snapshot.pr.number, + url: snapshot.pr.url.clone(), + }); + } + Ok(()) + } + + fn number_from_url(&self, url: &str) -> Result { + let parts = url + .strip_prefix("https://github.com/") + .map(|path| path.trim_end_matches('/').split('/').collect::>()); + if let Some(parts) = parts + && let [owner, repo, "pull", number] = parts.as_slice() + && owner.eq_ignore_ascii_case(&self.issue.owner) + && repo.eq_ignore_ascii_case(&self.issue.repo) + && number.bytes().all(|byte| byte.is_ascii_digit()) + && let Ok(number) = number.parse::() + && number > 0 + { + return Ok(number); + } + bail!( + "PR URL {url:?} does not identify a PR in {}", + self.issue.repo_slug() + ) + } + + fn validate(&self, snapshot: &PrSnapshot, ready: bool) -> Result<()> { + let pr = &snapshot.pr; + if !pr.is_open() { + bail!("PR {} is {}, not open", pr.url, pr.state); + } + if pr.base != self.base { + bail!("PR targets {}, not {}", pr.base, self.base); + } + if ready && pr.is_draft { + bail!("PR {} is a draft", pr.url); + } + Ok(()) + } +} + +/// The existing GitHub transport and a scripted test adapter vary here. +/// Selection and transition policy stay in PullRequest. +pub(crate) trait Adapter { + fn observe( + &mut self, + issue: &IssueUrl, + selector: &str, + completion: bool, + ) -> Result>; + fn body(&mut self, issue: &IssueUrl, number: u64, completion: bool) -> Result; + fn set_body( + &mut self, + issue: &IssueUrl, + number: u64, + body: &str, + completion: bool, + ) -> Result<()>; + fn ready(&mut self, issue: &IssueUrl, number: u64) -> Result<()>; + fn draft(&mut self, issue: &IssueUrl, number: u64, completion: bool) -> Result<()>; + fn spec_title(&mut self, issue: &IssueUrl) -> Result; + fn create_draft( + &mut self, + issue: &IssueUrl, + branch: &str, + base: &str, + title: &str, + body: &str, + ) -> Result; + fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()>; + fn pause(&mut self) -> Result<()> { + poll::pause() + } + fn grace_period(&self) -> std::time::Duration { + poll::grace_period() + } +} + +impl Adapter for GitHub { + fn observe( + &mut self, + issue: &IssueUrl, + selector: &str, + completion: bool, + ) -> Result> { + if completion { + self.completion().pr_snapshot(issue, selector) + } else { + self.pr_snapshot(issue, selector) + } + } + fn body(&mut self, issue: &IssueUrl, number: u64, completion: bool) -> Result { + if completion { + self.completion().pr_body(issue, number) + } else { + self.pr_body(issue, number) + } + } + fn set_body( + &mut self, + issue: &IssueUrl, + number: u64, + body: &str, + completion: bool, + ) -> Result<()> { + if completion { + self.completion().set_pr_body(issue, number, body) + } else { + self.set_pr_body(issue, number, body) + } + } + fn ready(&mut self, issue: &IssueUrl, number: u64) -> Result<()> { + self.mark_ready(issue, &number.to_string()) + } + fn draft(&mut self, issue: &IssueUrl, number: u64, completion: bool) -> Result<()> { + if completion { + self.completion() + .convert_to_draft(issue, &number.to_string()) + } else { + self.convert_to_draft(issue, &number.to_string()) + } + } + fn spec_title(&mut self, issue: &IssueUrl) -> Result { + self.completion().issue_title(issue) + } + fn create_draft( + &mut self, + issue: &IssueUrl, + branch: &str, + base: &str, + title: &str, + body: &str, + ) -> Result { + self.completion() + .create_draft_pr(issue, branch, base, title, body) + } + fn merge(&mut self, issue: &IssueUrl, number: u64, head: &str) -> Result<()> { + GitHub::merge(self, issue, number, head) + } +} + +const CHECKLIST_START: &str = ""; +const CHECKLIST_END: &str = ""; + +/// `checklist` wrapped in the Tickets checklist markers. +fn between_markers(checklist: &str) -> String { + format!("{CHECKLIST_START}\n{checklist}{CHECKLIST_END}\n") +} + +/// `body` with its Tickets checklist, the text from its first start marker +/// to the next end marker, replaced by `checklist` between the markers, or +/// with that appended if it has no such pair of markers. +fn with_checklist(body: &str, checklist: &str) -> String { + let checklist = between_markers(checklist); + if let Some(start) = body.find(CHECKLIST_START) + && let Some(end) = body[start..].find(CHECKLIST_END) + { + let end = start + end + CHECKLIST_END.len(); + let end = if body[end..].starts_with('\n') { + end + 1 + } else { + end + }; + return format!("{}{checklist}{}", &body[..start], &body[end..]); + } + if body.is_empty() { + return checklist; + } + format!("{}\n\n{checklist}", body.trim_end()) +} + +const BUILT_WITH_MARKER: &str = ""; + +/// Preserve unrelated text and replace only thirdshift's annotation line. +fn with_built_with(body: &str, choice: &Choice) -> String { + let line = format!("{} {BUILT_WITH_MARKER}", choice.built_with()); + if body.contains(BUILT_WITH_MARKER) { + return body + .split_inclusive('\n') + .map(|old| { + if old.trim_end().ends_with(BUILT_WITH_MARKER) { + let end = &old[old.trim_end_matches(['\r', '\n']).len()..]; + format!("{line}{end}") + } else { + old.to_string() + } + }) + .collect(); + } + if body.trim().is_empty() { + return format!("{line}\n"); + } + format!("{}\n\n{line}\n", body.trim_end()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::github::PullRequest as Record; + use std::{cell::RefCell, collections::VecDeque, rc::Rc, time::Duration}; + + const URL: &str = "https://github.com/acme/widgets/pull/12"; + type Answer = std::result::Result, &'static str>; + + fn snapshot() -> PrSnapshot { + PrSnapshot { + pr: Record { + number: 12, + url: URL.to_string(), + state: PrState::Open, + head: "issue-7".to_string(), + base: "main".to_string(), + is_draft: false, + }, + mergeable: Mergeable::Yes, + head_commit: "watched".to_string(), + from_fork: false, + } + } + + /// Scripted GitHub observations and affected PRs. Outcomes are inspected + /// at the external transport, never through the owner's private identity. + struct World { + observations: VecDeque, + completion: VecDeque, + selected: Vec<(String, bool)>, + ready: Vec, + draft: Vec, + merges: Vec<(u64, String)>, + edits: Vec<(u64, String)>, + body: String, + refused: bool, + merge_signal: Option, + body_error: bool, + write_error: bool, + draft_error: bool, + title: std::result::Result, + creation: std::result::Result, + created: Vec<(String, String, String, String)>, + effects: Vec<(String, bool)>, + retarget_after_edit: bool, + grace: Duration, + } + impl Default for World { + fn default() -> Self { + Self { + observations: VecDeque::from([Ok(Some(snapshot()))]), + completion: VecDeque::from([Ok(Some(snapshot()))]), + selected: Vec::new(), + ready: Vec::new(), + draft: Vec::new(), + merges: Vec::new(), + edits: Vec::new(), + body: "Closes #7\n".to_string(), + refused: false, + merge_signal: None, + body_error: false, + write_error: false, + draft_error: false, + title: Ok("Widgets".to_string()), + creation: Ok(URL.to_string()), + created: Vec::new(), + effects: Vec::new(), + retarget_after_edit: false, + grace: Duration::ZERO, + } + } + } + + #[derive(Clone)] + struct Scripted(Rc>); + impl Adapter for Scripted { + fn observe( + &mut self, + issue: &IssueUrl, + selector: &str, + completion: bool, + ) -> Result> { + if !completion { + crate::process::Interruption::Ordinary.check()?; + } + assert_eq!(issue.repo_slug(), "acme/widgets"); + let mut world = self.0.borrow_mut(); + world.selected.push((selector.to_string(), completion)); + let answers = if completion { + &mut world.completion + } else { + &mut world.observations + }; + let answer = if answers.len() > 1 { + answers.pop_front().unwrap() + } else { + answers.front().expect("no snapshot scripted").clone() + }; + answer.map_err(anyhow::Error::msg) + } + fn body(&mut self, _: &IssueUrl, number: u64, completion: bool) -> Result { + if !completion { + crate::process::Interruption::Ordinary.check()?; + } + let mut world = self.0.borrow_mut(); + world.effects.push((format!("body {number}"), completion)); + if world.body_error { + bail!("body unavailable"); + } + Ok(world.body.clone()) + } + fn set_body( + &mut self, + _: &IssueUrl, + number: u64, + body: &str, + completion: bool, + ) -> Result<()> { + if !completion { + crate::process::Interruption::Ordinary.check()?; + } + let mut world = self.0.borrow_mut(); + world.effects.push((format!("edit {number}"), completion)); + if world.write_error { + bail!("write failed"); + } + if world.retarget_after_edit { + let mut retargeted = snapshot(); + retargeted.pr.base = "develop".to_string(); + world.observations = VecDeque::from([Ok(Some(retargeted))]); + } + world.edits.push((number, body.to_string())); + world.body = body.to_string(); + Ok(()) + } + fn ready(&mut self, _: &IssueUrl, number: u64) -> Result<()> { + self.0.borrow_mut().ready.push(number); + Ok(()) + } + fn draft(&mut self, _: &IssueUrl, number: u64, completion: bool) -> Result<()> { + if !completion { + crate::process::Interruption::Ordinary.check()?; + } + let mut world = self.0.borrow_mut(); + world.effects.push((format!("draft {number}"), completion)); + if world.draft_error { + bail!("draft failed"); + } + world.draft.push(number); + Ok(()) + } + fn spec_title(&mut self, _: &IssueUrl) -> Result { + let mut world = self.0.borrow_mut(); + world.effects.push(("title".to_string(), true)); + world.title.clone().map_err(anyhow::Error::msg) + } + fn create_draft( + &mut self, + _: &IssueUrl, + branch: &str, + base: &str, + title: &str, + body: &str, + ) -> Result { + let mut world = self.0.borrow_mut(); + world.effects.push(("create".to_string(), true)); + world.created.push(( + branch.to_string(), + base.to_string(), + title.to_string(), + body.to_string(), + )); + world.creation.clone().map_err(anyhow::Error::msg) + } + fn merge(&mut self, _: &IssueUrl, number: u64, head: &str) -> Result<()> { + let mut world = self.0.borrow_mut(); + world.merges.push((number, head.to_string())); + if let Some(signal) = world.merge_signal { + signal_hook::low_level::raise(signal).unwrap(); + bail!("interrupted"); + } + if world.refused { + bail!("merge request failed"); + } + Ok(()) + } + fn pause(&mut self) -> Result<()> { + Ok(()) + } + fn grace_period(&self) -> Duration { + self.0.borrow().grace + } + } + + fn owner(world: World) -> (PullRequest, Rc>) { + let world = Rc::new(RefCell::new(world)); + let issue = IssueUrl::parse("https://github.com/acme/widgets/issues/7").unwrap(); + ( + PullRequest::new(&issue, "issue-7", "main", Scripted(world.clone())), + world, + ) + } + + fn numbered(number: u64) -> PrSnapshot { + let mut observed = snapshot(); + observed.pr.number = number; + observed.pr.url = format!("https://github.com/acme/widgets/pull/{number}"); + observed + } + + #[test] + fn resume_drafts_the_observed_number_even_when_branch_discovery_would_find_a_replacement() { + let (mut pr, world) = owner(World { + observations: VecDeque::from([ + Ok(Some(snapshot())), + Ok(Some(snapshot())), + Ok(Some(numbered(13))), + ]), + ..World::default() + }); + pr.resume_spec().unwrap(); + assert_eq!(world.borrow().draft, [12]); + assert_eq!( + world.borrow().selected, + [("issue-7".to_string(), false), ("12".to_string(), false)] + ); + assert_eq!(world.borrow().effects, [("draft 12".to_string(), false)]); + } + + #[test] + fn resume_revalidates_before_drafting_and_never_follows_a_replacement() { + for change in [ + "base", + "head", + "fork", + "repository", + "number", + "closed", + "missing", + "unreadable", + ] { + let mut changed = snapshot(); + match change { + "base" => changed.pr.base = "develop".to_string(), + "head" => changed.pr.head = "other".to_string(), + "fork" => changed.from_fork = true, + "repository" => { + changed.pr.url = "https://github.com/other/widgets/pull/12".to_string() + } + "number" => changed = numbered(13), + "closed" => changed.pr.state = PrState::Closed, + _ => (), + } + let next = match change { + "missing" => Ok(None), + "unreadable" => Err("read failed"), + _ => Ok(Some(changed)), + }; + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(Some(snapshot())), next]), + ..World::default() + }); + assert!(pr.resume_spec().is_err(), "{change}"); + assert!(world.borrow().draft.is_empty(), "{change}"); + assert_eq!( + world.borrow().selected.last(), + Some(&("12".to_string(), false)) + ); + } + } + + #[test] + fn resume_rejects_initial_wrong_branches_forks_and_repository_before_any_transition() { + for change in ["base", "head", "fork", "repository"] { + let mut changed = snapshot(); + match change { + "base" => changed.pr.base = "develop".to_string(), + "head" => changed.pr.head = "other".to_string(), + "fork" => changed.from_fork = true, + "repository" => { + changed.pr.url = "https://github.com/other/widgets/pull/12".to_string() + } + _ => unreachable!(), + } + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(Some(changed))]), + ..World::default() + }); + assert!(pr.resume_spec().is_err(), "{change}"); + assert!(world.borrow().draft.is_empty()); + assert!(world.borrow().edits.is_empty()); + } + } + + #[test] + fn absent_or_closed_resume_has_no_active_pr_and_first_landing_uses_the_creation_number() { + let mut closed = snapshot(); + closed.pr.state = PrState::Closed; + for resumed in [None, Some(closed)] { + let mut created = numbered(13); + created.pr.is_draft = true; + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(resumed)]), + completion: VecDeque::from([Ok(Some(created))]), + creation: Ok(" https://github.com/ACME/Widgets/pull/13\n".to_string()), + ..World::default() + }); + pr.resume_spec().unwrap(); + pr.show_checklist("- [ ] #8 running\n"); + assert_eq!(pr.info(), None); + assert_eq!( + pr.land_spec("- [x] #8 landed\n").unwrap(), + "https://github.com/ACME/Widgets/pull/13" + ); + assert_eq!( + world.borrow().selected, + [("issue-7".to_string(), false), ("13".to_string(), true)] + ); + assert_eq!(world.borrow().created, [( + "issue-7".to_string(), "main".to_string(), "Widgets".to_string(), + "The work on Spec #7, gathered from its Tickets on issue-7.\n\nCloses #7\n\n\n- [x] #8 landed\n\n".to_string(), + )]); + assert!(world.borrow().draft.is_empty()); + assert!(world.borrow().ready.is_empty()); + assert!(world.borrow().merges.is_empty()); + } + } + + #[test] + fn unreadable_title_creation_or_invalid_creation_url_fails_without_discovery() { + for problem in [ + "title", + "creation", + "", + "garbage", + "https://github.com/acme/other/pull/12", + "https://github.com/acme/widgets/pull/0", + "https://github.com/acme/widgets/pull/+12", + "https://github.com/acme/widgets/issues/12", + "https://github.com/acme/widgets/pull/12?x=1", + ] { + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(None)]), + title: if problem == "title" { + Err("title unreadable") + } else { + Ok("Widgets".to_string()) + }, + creation: if problem == "creation" { + Err("creation unreadable") + } else { + Ok(problem.to_string()) + }, + ..World::default() + }); + pr.resume_spec().unwrap(); + assert!(pr.land_spec("landed\n").is_err(), "{problem}"); + assert_eq!(world.borrow().selected, [("issue-7".to_string(), false)]); + assert!(world.borrow().edits.is_empty()); + } + } + + #[test] + fn creation_requires_an_open_draft_with_the_returned_number_and_expected_ownership() { + for change in [ + "missing", + "unreadable", + "number", + "base", + "head", + "fork", + "repository", + "closed", + "ready", + ] { + let mut created = snapshot(); + created.pr.is_draft = true; + match change { + "number" => created = numbered(13), + "base" => created.pr.base = "develop".to_string(), + "head" => created.pr.head = "other".to_string(), + "fork" => created.from_fork = true, + "repository" => { + created.pr.url = "https://github.com/other/widgets/pull/12".to_string() + } + "closed" => created.pr.state = PrState::Closed, + "ready" => created.pr.is_draft = false, + _ => (), + } + let observation = match change { + "missing" => Ok(None), + "unreadable" => Err("read failed"), + _ => Ok(Some(created)), + }; + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(None)]), + completion: VecDeque::from([observation]), + ..World::default() + }); + pr.resume_spec().unwrap(); + assert!(pr.land_spec("landed\n").is_err(), "{change}"); + assert_eq!( + world.borrow().selected.last(), + Some(&("12".to_string(), true)) + ); + assert!(world.borrow().edits.is_empty()); + } + } + + #[test] + fn accounting_never_edits_or_lands_an_invalid_numbered_observation() { + for change in [ + "base", + "head", + "fork", + "repository", + "number", + "closed", + "merged", + "missing", + "unreadable", + ] { + let mut changed = snapshot(); + match change { + "base" => changed.pr.base = "develop".to_string(), + "head" => changed.pr.head = "other".to_string(), + "fork" => changed.from_fork = true, + "repository" => { + changed.pr.url = "https://github.com/other/widgets/pull/12".to_string() + } + "number" => changed = numbered(13), + "closed" => changed.pr.state = PrState::Closed, + "merged" => changed.pr.state = PrState::Merged, + _ => (), + } + let observation = match change { + "missing" => Ok(None), + "unreadable" => Err("read failed"), + _ => Ok(Some(changed)), + }; + let (mut pr, world) = owner(World { + completion: VecDeque::from([observation]), + ..World::default() + }); + pr.resume_spec().unwrap(); + pr.show_checklist("new\n"); + assert!(pr.land_spec("new\n").is_err(), "{change}"); + assert_eq!(world.borrow().body, "Closes #7\n"); + assert!(world.borrow().edits.is_empty()); + assert!(world.borrow().created.is_empty()); + assert!( + world.borrow().selected[1..] + .iter() + .all(|(selector, _)| selector == "12") + ); + } + } + + #[test] + fn checklist_preserves_surrounding_text_is_idempotent_and_never_marks_ready_or_merges() { + let (mut pr, world) = owner(World { + body: + "Summary\n\n\nold\n\nTail\n" + .to_string(), + ..World::default() + }); + pr.resume_spec().unwrap(); + pr.show_checklist("- [x] #8 done\n"); + pr.show_checklist("- [x] #8 done\n"); + assert_eq!(pr.land_spec("- [x] #8 done\n").unwrap(), URL); + assert_eq!( + world.borrow().body, + "Summary\n\n\n- [x] #8 done\n\nTail\n" + ); + assert_eq!(world.borrow().edits.len(), 1); + assert_eq!(world.borrow().edits[0].0, 12); + assert!(world.borrow().ready.is_empty()); + assert!(world.borrow().merges.is_empty()); + } + + #[test] + fn checklist_read_and_write_failures_warn_during_accounting_but_fail_before_readiness() { + for read in [false, true] { + let (mut pr, world) = owner(World { + body_error: read, + write_error: !read, + ..World::default() + }); + pr.resume_spec().unwrap(); + pr.show_checklist("new\n"); + assert_eq!(pr.land_spec("new\n").unwrap(), URL); + pr.begin_delivery(); + assert!(pr.mark_ready(Some("new\n")).is_err()); + assert!(world.borrow().ready.is_empty()); + assert_eq!(world.borrow().body, "Closes #7\n"); + } + } + + #[test] + fn delivery_capture_replaces_accounting_once_and_final_checklists_use_that_same_owner() { + for opening_failed in [false, true] { + let (mut pr, world) = owner(World::default()); + pr.resume_spec().unwrap(); + pr.begin_delivery(); + assert_eq!(pr.info(), None); + let mut replacement = numbered(13); + replacement.pr.is_draft = true; + world.borrow_mut().observations = VecDeque::from([Ok(Some(replacement.clone()))]); + world.borrow_mut().completion = VecDeque::from([Ok(Some(replacement))]); + if opening_failed { + assert_eq!( + pr.finish_failed_run(false).unwrap().as_deref(), + Some("https://github.com/acme/widgets/pull/13") + ); + } else { + assert_eq!(pr.mark_ready(Some("done\n")).unwrap().number, 13); + } + pr.begin_delivery(); // A later call cannot reset a Delivery capture. + pr.show_checklist("final\n"); + assert_eq!(world.borrow().edits.last().unwrap().0, 13); + assert_eq!( + world.borrow().selected.last(), + Some(&("13".to_string(), true)) + ); + assert_eq!(pr.info().unwrap().number, 13); + } + } + + #[test] + fn checklist_appends_to_unmarked_empty_or_incomplete_bodies_without_removing_text() { + for (body, expected) in [ + ( + "", + "\nnew\n\n", + ), + ( + "Closes #7\n", + "Closes #7\n\n\nnew\n\n", + ), + ( + "Intro\n\nold\n", + "Intro\n\nold\n\n\nnew\n\n", + ), + ] { + let (mut pr, world) = owner(World { + body: body.to_string(), + ..World::default() + }); + pr.resume_spec().unwrap(); + pr.show_checklist("new\n"); + assert_eq!(world.borrow().body, expected); + } + } + + #[test] + fn accounting_finishes_after_interruption_without_enabling_ordinary_delivery() { + crate::test_support::with_recorded_signal( + "pull_request::tests::accounting_finishes_after_interruption_without_enabling_ordinary_delivery", + |signal| { + let mut created = snapshot(); + created.pr.is_draft = true; + let (mut first_landing, first_world) = owner(World { + observations: VecDeque::from([Ok(None)]), + completion: VecDeque::from([Ok(Some(created))]), + ..World::default() + }); + let (mut existing, existing_world) = owner(World::default()); + first_landing.resume_spec().unwrap(); + existing.resume_spec().unwrap(); + existing_world.borrow_mut().effects.clear(); + signal_hook::low_level::raise(signal).unwrap(); + for (pr, world) in [ + (&mut first_landing, first_world), + (&mut existing, existing_world), + ] { + assert_eq!(pr.land_spec("done\n").unwrap(), URL); + pr.show_checklist("final\n"); + assert!( + world + .borrow() + .effects + .iter() + .all(|(_, completion)| *completion) + ); + assert!(world.borrow().body.contains("final\n")); + pr.begin_delivery(); + assert_eq!( + pr.mark_ready(Some("final\n")).unwrap_err().to_string(), + "interrupted" + ); + assert!(crate::interrupt::requested()); + assert!(world.borrow().ready.is_empty()); + } + let (mut cancelled_resume, _) = owner(World::default()); + assert_eq!( + cancelled_resume.resume_spec().unwrap_err().to_string(), + "interrupted" + ); + }, + ); + } + + #[test] + fn draft_failure_on_resume_is_terminal() { + let (mut pr, world) = owner(World { + draft_error: true, + ..World::default() + }); + assert_eq!(pr.resume_spec().unwrap_err().to_string(), "draft failed"); + assert!(world.borrow().draft.is_empty()); + } + + #[test] + fn annotation_preserves_text_is_idempotent_and_replaces_the_previous_choice() { + let (mut pr, world) = owner(World::default()); + let choice = Choice { + model: Some("opus".to_string()), + effort: Some("high".to_string()), + ..Choice::default() + }; + pr.write_built_with(&Choice::default()).unwrap(); + pr.write_built_with(&Choice::default()).unwrap(); + pr.write_built_with(&choice).unwrap(); + let expected = + "Closes #7\n\nBuilt with claude · opus · high \n"; + assert_eq!(world.borrow().body, expected); + assert_eq!(world.borrow().edits.len(), 2); + assert!(world.borrow().edits.iter().all(|(number, _)| *number == 12)); + } + + #[test] + fn annotation_replaces_its_line_in_place_preserving_unrelated_text_and_line_endings() { + let body = "Summary\r\nBuilt with old \r\n\r\nCloses #7\r\n"; + let (mut pr, world) = owner(World { + body: body.to_string(), + ..World::default() + }); + pr.write_built_with(&Choice::default()).unwrap(); + assert_eq!( + world.borrow().body, + "Summary\r\nBuilt with claude · default model · default effort \r\n\r\nCloses #7\r\n" + ); + } + + #[test] + fn missing_or_failed_initial_observations_allow_discovery_at_readiness() { + for first in [Ok(None), Err("transport failed")] { + let (mut pr, world) = owner(World { + observations: VecDeque::from([first, Ok(Some(snapshot()))]), + ..World::default() + }); + let _ = pr.write_built_with(&Choice::default()); + assert_eq!(pr.mark_ready(None).unwrap().url, URL); + assert_eq!( + world.borrow().selected[..2], + [ + ("issue-7".to_string(), false), + ("issue-7".to_string(), false) + ] + ); + assert!( + world.borrow().selected[2..] + .iter() + .all(|(selector, _)| selector == "12") + ); + } + } + + #[test] + fn a_body_read_failure_still_binds_identity_for_readiness_and_failure_finishing() { + let (mut pr, world) = owner(World { + body_error: true, + ..World::default() + }); + assert!(pr.write_built_with(&Choice::default()).is_err()); + pr.mark_ready(None).unwrap(); + assert_eq!(pr.finish_failed_run(false).unwrap().as_deref(), Some(URL)); + assert_eq!(world.borrow().draft, [12]); + assert_eq!( + world.borrow().selected[1..], + [("12".to_string(), false), ("12".to_string(), true)] + ); + } + + #[test] + fn initial_wrong_target_binds_identity_but_never_marks_ready_and_can_be_drafted() { + let mut wrong = snapshot(); + wrong.pr.base = "develop".to_string(); + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(Some(wrong.clone()))]), + completion: VecDeque::from([Ok(Some(wrong))]), + ..World::default() + }); + assert_eq!( + pr.mark_ready(None).unwrap_err().to_string(), + "PR targets develop, not main" + ); + assert_eq!(pr.finish_failed_run(false).unwrap().as_deref(), Some(URL)); + assert!(world.borrow().ready.is_empty()); + assert_eq!(world.borrow().draft, [12]); + assert_eq!( + world.borrow().selected.last(), + Some(&("12".to_string(), true)) + ); + } + + #[test] + fn discovery_rejects_fork_and_unexpected_issue_branch() { + for fork in [false, true] { + let mut wrong = snapshot(); + if fork { + wrong.from_fork = true; + } else { + wrong.pr.head = "other".to_string(); + } + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(Some(wrong))]), + ..World::default() + }); + assert!(pr.mark_ready(None).is_err()); + assert!(world.borrow().ready.is_empty()); + } + } + + #[test] + fn readiness_marks_only_a_draft_and_returns_immutable_identity() { + for draft in [false, true] { + let mut observed = snapshot(); + observed.pr.is_draft = draft; + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(Some(observed))]), + ..World::default() + }); + assert_eq!( + pr.mark_ready(None).unwrap(), + Identified { + number: 12, + url: URL.to_string() + } + ); + assert_eq!( + world.borrow().ready, + if draft { vec![12] } else { Vec::new() } + ); + } + } + + #[test] + fn checklist_restoration_rechecks_readiness_after_the_body_edit() { + let (mut pr, world) = owner(World { + retarget_after_edit: true, + ..World::default() + }); + pr.write_built_with(&Choice::default()).unwrap(); + // Retarget on the checklist write, rather than the annotation write. + world.borrow_mut().observations = VecDeque::from([Ok(Some(snapshot()))]); + let error = pr.mark_ready(Some("- [x] #8 done\n")).unwrap_err(); + assert_eq!(error.to_string(), "PR targets develop, not main"); + assert!(world.borrow().ready.is_empty()); + } + + #[test] + fn every_unknown_mergeability_snapshot_revalidates_target_state_and_ownership() { + // First UNKNOWN is followed immediately by the changed snapshot; + // no pause is needed to script a change during polling. + for change in ["base", "closed", "draft", "head", "fork", "number"] { + let mut unknown = snapshot(); + unknown.mergeable = Mergeable::Unknown; + let mut changed = unknown.clone(); + match change { + "base" => changed.pr.base = "develop".to_string(), + "closed" => changed.pr.state = PrState::Closed, + "draft" => changed.pr.is_draft = true, + "head" => changed.pr.head = "other".to_string(), + "fork" => changed.from_fork = true, + "number" => changed.pr.number = 13, + _ => unreachable!(), + } + let (mut pr, world) = owner(World { + observations: VecDeque::from([ + Ok(Some(snapshot())), + Ok(Some(snapshot())), + Ok(Some(unknown)), + Ok(Some(changed)), + ]), + grace: Duration::from_secs(60), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + assert!(pr.ensure_ready_and_mergeable().is_err(), "{change}"); + assert!( + world.borrow().selected[1..] + .iter() + .all(|(selector, _)| selector == "12") + ); + } + } + + #[test] + fn missing_closed_draft_conflicting_and_unknown_prs_fail_final_readiness() { + let mut closed = snapshot(); + closed.pr.state = PrState::Closed; + let mut draft = snapshot(); + draft.pr.is_draft = true; + let mut conflicting = snapshot(); + conflicting.mergeable = Mergeable::No; + let mut unknown = snapshot(); + unknown.mergeable = Mergeable::Unknown; + for (answer, message) in [ + (Ok(None), "no PR found"), + (Ok(Some(closed)), "is closed, not open"), + (Ok(Some(draft)), "is a draft"), + (Ok(Some(conflicting)), "is not mergeable"), + (Ok(Some(unknown)), "has not worked out"), + ] { + let (mut pr, _) = owner(World { + observations: VecDeque::from([Ok(Some(snapshot())), Ok(Some(snapshot())), answer]), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + assert!( + pr.ensure_ready_and_mergeable() + .unwrap_err() + .to_string() + .contains(message) + ); + } + } + + #[test] + fn disappearance_or_transport_failure_after_capture_never_rediscovers_by_branch() { + for missing in [Ok(None), Err("gone")] { + let (mut pr, world) = owner(World { + observations: VecDeque::from([ + Ok(Some(snapshot())), + Ok(Some(snapshot())), + missing.clone(), + ]), + completion: VecDeque::from([missing]), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + assert!(pr.ensure_ready_and_mergeable().is_err()); + let _ = pr.finish_failed_run(false); + assert_eq!( + world.borrow().selected[2..], + [("12".to_string(), false), ("12".to_string(), true)] + ); + assert!(world.borrow().draft.is_empty()); + } + } + + #[test] + fn closed_original_or_unexpected_replacement_cannot_be_reported_or_modified() { + for replaced in [false, true] { + let mut original = snapshot(); + if replaced { + original.pr.number = 13; + } else { + original.pr.state = PrState::Closed; + } + let (mut pr, world) = owner(World { + completion: VecDeque::from([Ok(Some(original))]), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + assert!(pr.finish_failed_run(false).ok().flatten().is_none()); + assert!(world.borrow().draft.is_empty()); + } + } + + #[test] + fn a_fresh_invalid_or_unreadable_merge_gate_issues_no_request() { + let mut wrong = snapshot(); + wrong.pr.base = "develop".to_string(); + for gate in [Ok(Some(wrong)), Err("unreadable")] { + let (mut pr, world) = owner(World { + observations: VecDeque::from([Ok(Some(snapshot())), Ok(Some(snapshot())), gate]), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + assert!(pr.merge("watched").is_err()); + assert!(world.borrow().merges.is_empty()); + } + } + + #[test] + fn successful_guarded_merge_requires_no_completion_observation() { + let (mut pr, world) = owner(World { + completion: VecDeque::from([Err("completion unavailable")]), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + assert!(matches!(pr.merge("watched").unwrap(), MergeAttempt::Merged)); + assert_eq!(world.borrow().merges, [(12, "watched".to_string())]); + assert!( + world + .borrow() + .selected + .iter() + .all(|(_, completion)| !completion) + ); + } + + #[test] + fn failed_requests_are_confirmed_only_for_the_same_pr_head_and_expected_branches() { + for change in [ + "none", + "number", + "head", + "base", + "branch", + "fork", + "open", + "closed", + "missing", + "unreadable", + ] { + let mut merged = snapshot(); + merged.pr.state = PrState::Merged; + match change { + "number" => merged.pr.number = 13, + "head" => merged.head_commit = "foreign".to_string(), + "base" => merged.pr.base = "develop".to_string(), + "branch" => merged.pr.head = "other".to_string(), + "fork" => merged.from_fork = true, + "open" => merged.pr.state = PrState::Open, + "closed" => merged.pr.state = PrState::Closed, + _ => (), + } + let completion = match change { + "missing" => Ok(None), + "unreadable" => Err("unreadable"), + _ => Ok(Some(merged)), + }; + let (mut pr, world) = owner(World { + refused: true, + completion: VecDeque::from([completion]), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + let attempt = pr.merge("watched").unwrap(); + assert_eq!( + matches!(attempt, MergeAttempt::Merged), + change == "none", + "{change}" + ); + if let MergeAttempt::Refused(error) = attempt { + assert_eq!(error.to_string(), "merge request failed"); + } + assert_eq!( + world.borrow().selected.last(), + Some(&("12".to_string(), true)) + ); + } + } + + #[test] + fn failed_run_can_discover_before_capture_and_policy_refusal_keeps_ready() { + for keep_ready in [false, true] { + let (mut pr, world) = owner(World::default()); + assert_eq!( + pr.finish_failed_run(keep_ready).unwrap().as_deref(), + Some(URL) + ); + assert_eq!(world.borrow().selected, [("issue-7".to_string(), true)]); + assert_eq!( + world.borrow().draft, + if keep_ready { Vec::new() } else { vec![12] } + ); + } + let mut draft = snapshot(); + draft.pr.is_draft = true; + let (mut pr, world) = owner(World { + completion: VecDeque::from([Ok(Some(draft))]), + ..World::default() + }); + assert_eq!(pr.finish_failed_run(false).unwrap().as_deref(), Some(URL)); + assert!(world.borrow().draft.is_empty()); + } + fn interrupted_merge(name: &str, confirmed: bool) { + crate::test_support::with_recorded_signal(name, |signal| { + let mut completion = snapshot(); + if confirmed { + completion.pr.state = PrState::Merged; + } + let (mut pr, world) = owner(World { + merge_signal: Some(signal), + completion: VecDeque::from([Ok(Some(completion))]), + ..World::default() + }); + pr.mark_ready(None).unwrap(); + let attempt = pr.merge("watched").unwrap(); + assert_eq!(matches!(attempt, MergeAttempt::Merged), confirmed); + if let MergeAttempt::Refused(error) = attempt { + assert_eq!(error.to_string(), "interrupted"); + } + assert!(crate::interrupt::requested()); + assert_eq!( + pr.ensure_ready_and_mergeable().unwrap_err().to_string(), + "interrupted" + ); + assert!(world.borrow().selected.contains(&("12".to_string(), true))); + }); + } + + #[test] + fn an_interrupted_request_can_be_confirmed_without_clearing_interruption() { + interrupted_merge( + "pull_request::tests::an_interrupted_request_can_be_confirmed_without_clearing_interruption", + true, + ); + } + + #[test] + fn an_unconfirmed_interrupted_request_keeps_its_failure_and_interruption() { + interrupted_merge( + "pull_request::tests::an_unconfirmed_interrupted_request_keeps_its_failure_and_interruption", + false, + ); + } +} diff --git a/src/run.rs b/src/run.rs index a02fe05b0..4861d29b5 100644 --- a/src/run.rs +++ b/src/run.rs @@ -438,9 +438,14 @@ impl<'a> Outside for LaunchAndGitHub<'a> { base: &str, opening: Opening, ) -> Result { + let mut pull_request = crate::pull_request::PullRequest::new( + self.issue, + worktree.branch(), + base, + GitHub::new(), + ); self.delivery(base) - .deliver(worktree, opening, |_| Ok(())) - .outcome + .deliver(worktree, opening, &mut pull_request, None) } fn step(&mut self, line: String) { diff --git a/src/spec_run.rs b/src/spec_run.rs index e9170e688..bdf766712 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -9,7 +9,6 @@ //! [`ChildRunsAndGitHub`] does that for real; `Scripted`, in tests, from a //! script, recording each call. -mod spec_pr; mod ticket_board; use std::num::NonZeroUsize; @@ -18,7 +17,7 @@ use anyhow::{Result, bail}; use crate::base_fix::BaseFixAsk; use crate::child_run::{Ended, Kind, Runs}; -use crate::delivery::{Delivery, Identified, Opening}; +use crate::delivery::{Delivery, Opening}; use crate::failed_run::{FailedRun, interrupted_or}; use crate::github::{GitHub, Ticket}; use crate::harness::Choice; @@ -26,10 +25,10 @@ use crate::interrupt; use crate::issue::IssueUrl; use crate::progress; use crate::prompt; +use crate::pull_request::PullRequest; use crate::run::Reached; use crate::worktree::Worktree; -use spec_pr::SpecPr; use ticket_board::TicketBoard; /// The Spec review session's kind, in its progress lines and log name. @@ -57,7 +56,8 @@ pub fn run( ) -> Result { let (spec, base) = (delivery.issue, delivery.base); let branch = worktree.branch().to_string(); - let spec_pr = SpecPr::resume(spec, &branch, base)?; + let mut spec_pr = PullRequest::new(spec, &branch, base, GitHub::new()); + spec_pr.resume_spec()?; let mut outside = ChildRunsAndGitHub::new(worktree, delivery, spec_pr); run_through(&mut outside, spec, base, &branch, tickets, parallel) } @@ -143,7 +143,7 @@ fn review_and_deliver( catch_up_from_origin: true, }; // The Spec review may have rewritten the body without the checklist. - let delivered = outside.deliver(opening, |outside, pr| outside.put_back(checklist, pr)); + let delivered = outside.deliver(opening, checklist); if delivered.is_err() { outside.show(checklist); } @@ -170,16 +170,9 @@ trait Outside { fn landed(&mut self, checklist: &str) -> Result; /// The Spec PR's URL, if it is open. fn spec_pr_url(&mut self) -> Option; - /// Put `checklist` back in the Spec PR's body. - fn put_back(&mut self, checklist: &str, pr: &Identified) -> Result<()>; - /// Take the Spec PR to its goal by the Delivery, opening with `opening`, - /// running `before_ready` before the Spec PR is marked ready. Called at - /// most once, after the Ticket loop. - fn deliver( - &mut self, - opening: Opening, - before_ready: impl FnOnce(&mut Self, &Identified) -> Result<()>, - ) -> Result; + /// Take the Spec PR to its goal, restoring `checklist` before readiness. + /// Called at most once, after the Ticket loop. + fn deliver(&mut self, opening: Opening, checklist: &str) -> Result; /// Whether the Spec run was interrupted. fn interrupted(&mut self) -> bool; /// Write the progress line `line`. @@ -203,13 +196,13 @@ struct ChildRunsAndGitHub<'a> { delivery: Option>, base_fix: BaseFixAsk, harness: &'a Choice, - spec_pr: SpecPr<'a>, + spec_pr: PullRequest, } impl<'a> ChildRunsAndGitHub<'a> { /// The outside world of a Spec run, as the struct says, with no Ticket's /// Run started yet. - fn new(worktree: Worktree, delivery: Delivery<'a>, spec_pr: SpecPr<'a>) -> Self { + fn new(worktree: Worktree, delivery: Delivery<'a>, spec_pr: PullRequest) -> Self { Self { spec: delivery.issue, children: Runs::default(), @@ -263,35 +256,21 @@ impl Outside for ChildRunsAndGitHub<'_> { } fn show(&mut self, checklist: &str) { - self.spec_pr.show(checklist); + self.spec_pr.show_checklist(checklist); } fn landed(&mut self, checklist: &str) -> Result { - self.spec_pr.landed(checklist).map(str::to_string) + self.spec_pr.land_spec(checklist) } fn spec_pr_url(&mut self) -> Option { - self.spec_pr.url().map(str::to_string) + self.spec_pr.info().map(|pr| pr.url) } - fn put_back(&mut self, checklist: &str, pr: &Identified) -> Result<()> { - self.spec_pr.put_back(checklist, pr) - } - - /// Hands the worktree and the Delivery over, then lends `self` to - /// `before_ready`, as the Delivery no longer borrows it. - fn deliver( - &mut self, - opening: Opening, - before_ready: impl FnOnce(&mut Self, &Identified) -> Result<()>, - ) -> Result { + fn deliver(&mut self, opening: Opening, checklist: &str) -> Result { let worktree = self.worktree.take().expect("the Delivery starts once"); let delivery = self.delivery.take().expect("the Delivery starts once"); - let delivered = delivery.deliver(worktree, opening, |pr| before_ready(self, pr)); - if let Some(pr) = delivered.pull_request { - self.spec_pr.follow(&pr); - } - delivered.outcome + delivery.deliver(worktree, opening, &mut self.spec_pr, Some(checklist)) } fn interrupted(&mut self) -> bool { @@ -632,21 +611,7 @@ mod tests { self.spec_pr_open.then(|| SPEC_PR.to_string()) } - fn put_back(&mut self, checklist: &str, _: &Identified) -> Result<()> { - self.did.push(Did::PutBack(checklist.to_string())); - if self.script.put_back_fails { - bail!("could not put the Tickets checklist back"); - } - Ok(()) - } - - /// Runs `before_ready`, then marks the Spec PR ready, as the - /// Delivery does, failing as the Failed run path would. - fn deliver( - &mut self, - opening: Opening, - before_ready: impl FnOnce(&mut Self, &Identified) -> Result<()>, - ) -> Result { + fn deliver(&mut self, opening: Opening, checklist: &str) -> Result { self.did.push(Did::Deliver { kind: opening.kind.to_string(), prompt: opening.prompt, @@ -655,13 +620,12 @@ mod tests { let delivered = if self.script.review_fails { Err(anyhow!("the spec-review session failed")) } else { - before_ready( - self, - &Identified { - number: 30, - url: SPEC_PR.to_string(), - }, - ) + self.did.push(Did::PutBack(checklist.to_string())); + if self.script.put_back_fails { + Err(anyhow!("could not put the Tickets checklist back")) + } else { + Ok(()) + } }; let delivered = delivered.and_then(|()| { self.did.push(Did::Ready); diff --git a/src/spec_run/spec_pr.rs b/src/spec_run/spec_pr.rs deleted file mode 100644 index 9435a207a..000000000 --- a/src/spec_run/spec_pr.rs +++ /dev/null @@ -1,681 +0,0 @@ -//! The Spec PR in a Spec run: resumed as a draft while the Tickets run, -//! opened as a draft once the first Ticket lands, and its Tickets checklist -//! kept up to date between its markers and put back after the Spec review. -//! The Spec run's loop says what happened; this says what that does to the -//! Spec PR. -//! -//! Its rules reach GitHub and the progress lines only through [`Outside`]: -//! [`OnGitHub`] does that for real; `InMemory`, in tests, holds the pull -//! request and records each call. - -use anyhow::{Context, Result}; - -use crate::delivery::Identified; -use crate::github::{GitHub, PullRequest}; -use crate::issue::IssueUrl; -use crate::progress; - -/// The markers around the Tickets checklist in the Spec PR's body, so it can -/// be replaced without touching the text around it. -const CHECKLIST_START: &str = ""; -const CHECKLIST_END: &str = ""; - -/// A Spec run's Spec PR, from its Spec branch into its Base branch, if it is -/// open, reached through `O`. -pub(super) struct SpecPr<'a, O = OnGitHub<'a>> { - spec: &'a IssueUrl, - /// Owned: the worktree it is borrowed from moves into the Delivery, - /// while the Spec PR is still needed after it. - branch: String, - base: &'a str, - pr: Option, - outside: O, -} - -impl<'a> SpecPr<'a> { - /// The Spec PR of `spec` from `branch` into `base`, on GitHub: the open - /// one, if there is one, converted back to a draft while the Tickets run. - pub(super) fn resume(spec: &'a IssueUrl, branch: &str, base: &'a str) -> Result { - let on_github = OnGitHub { - spec, - branch: branch.to_string(), - base, - github: GitHub::new(), - }; - let mut resumed = Self::resume_through(on_github, spec, branch, base)?; - // Resume is ordinary; subsequent writes account for child endings, - // even when interruption prevents more implementation work. - resumed.outside.github = resumed.outside.github.completion(); - Ok(resumed) - } -} - -impl<'a, O: Outside> SpecPr<'a, O> { - /// [`SpecPr::resume`], reaching the Spec PR through `outside`. - fn resume_through( - mut outside: O, - spec: &'a IssueUrl, - branch: &str, - base: &'a str, - ) -> Result { - let pr = outside.pull_request()?.filter(|pr| pr.is_open()); - if let Some(pr) = &pr - && !pr.is_draft - { - outside.convert_to_draft()?; - } - Ok(Self { - spec, - branch: branch.to_string(), - base, - pr, - outside, - }) - } - - /// Show `checklist` as the Spec PR's Tickets checklist if it is open, - /// only warning if that fails: the Spec run goes on without it. - pub(super) fn show(&mut self, checklist: &str) { - let Some(number) = self.number() else { - return; - }; - if let Err(error) = self.write(number, checklist) { - self.outside.step(format!( - "could not update the Spec PR's Tickets checklist: {error:#}" - )); - } - } - - /// A Ticket landed, and the Tickets checklist is now `checklist`: open - /// the Spec PR as a draft with it if it is not open, else show it. - /// Returns the Spec PR's URL. - pub(super) fn landed(&mut self, checklist: &str) -> Result<&str> { - if self.pr.is_some() { - self.show(checklist); - } else { - self.pr = Some(self.open(checklist)?); - } - Ok(&self.pr.as_ref().expect("the Spec PR is open").url) - } - - /// Put `checklist` back in the Spec PR's body, as the Spec review may - /// have rewritten it without the checklist. - pub(super) fn put_back(&mut self, checklist: &str, identified: &Identified) -> Result<()> { - self.follow(identified); - let number = self.number().context("the Spec PR is not open")?; - self.write(number, checklist) - } - - /// Once Delivery captures its PR, checklist writes follow that identity, - /// including after an opening-session failure and completion discovery. - pub(super) fn follow(&mut self, identified: &Identified) { - if let Some(pr) = &mut self.pr { - pr.number = identified.number; - pr.url = identified.url.clone(); - } - } - - /// The Spec PR's URL, if it is open. - pub(super) fn url(&self) -> Option<&str> { - self.pr.as_ref().map(|pr| pr.url.as_str()) - } - - /// The Spec PR's number, if it is open. - fn number(&self) -> Option { - self.pr.as_ref().map(|pr| pr.number) - } - - /// Open the Spec PR as a draft, titled from the Spec, closing it, with - /// `checklist` as its Tickets checklist. - fn open(&mut self, checklist: &str) -> Result { - let (spec, branch, base) = (self.spec, &self.branch, self.base); - self.outside - .step(format!("opening the Spec PR into {base} as a draft")); - let title = self.outside.spec_title()?; - let body = format!( - "The work on Spec #{number}, gathered from its Tickets on {branch}.\n\n\ - Closes #{number}\n\n\ - {checklist}", - number = spec.number, - checklist = between_markers(checklist), - ); - self.outside.create_draft(&title, &body)?; - self.outside - .pull_request()? - .context("the Spec PR just opened is not found") - } - - /// Put `checklist` in the body of the Spec PR `number`, in place of its - /// Tickets checklist, leaving the rest of the body as it is. - fn write(&mut self, number: u64, checklist: &str) -> Result<()> { - let body = self.outside.pr_body(number)?; - let updated = with_checklist(&body, checklist); - if updated != body { - self.outside - .step("updating the Spec PR's Tickets checklist".to_string()); - self.outside.set_pr_body(number, &updated)?; - } - Ok(()) - } -} - -/// What the Spec PR's rules do or read outside the Spec PR: the pull request -/// from the Spec branch, the Spec's title, and the progress lines. Visible -/// to the Spec run only because [`SpecPr`]'s type parameter names it; the -/// Spec run never calls it. -pub(super) trait Outside { - /// The pull request from the Spec branch, if it has one. - fn pull_request(&mut self) -> Result>; - /// Convert the pull request from the Spec branch back to a draft. - fn convert_to_draft(&mut self) -> Result<()>; - /// The Spec's title. - fn spec_title(&mut self) -> Result; - /// Open a draft pull request from the Spec branch into the Base branch, - /// titled `title`, with `body`. - fn create_draft(&mut self, title: &str, body: &str) -> Result<()>; - /// The body of pull request `number`. - fn pr_body(&mut self, number: u64) -> Result; - /// Set the body of pull request `number` to `body`. - fn set_pr_body(&mut self, number: u64, body: &str) -> Result<()>; - /// Hand on the progress line `line`. - fn step(&mut self, line: String); -} - -/// The Spec PR of `spec`, from `branch` into `base`, on GitHub, and progress -/// lines printed on stderr. -pub(super) struct OnGitHub<'a> { - github: GitHub, - spec: &'a IssueUrl, - branch: String, - base: &'a str, -} - -impl Outside for OnGitHub<'_> { - fn pull_request(&mut self) -> Result> { - self.github.pull_request_for(self.spec, &self.branch) - } - - fn convert_to_draft(&mut self) -> Result<()> { - self.github.convert_to_draft(self.spec, &self.branch) - } - - fn spec_title(&mut self) -> Result { - self.github.issue_title(self.spec) - } - - fn create_draft(&mut self, title: &str, body: &str) -> Result<()> { - self.github - .create_draft_pr(self.spec, &self.branch, self.base, title, body) - .map(|_| ()) - } - - fn pr_body(&mut self, number: u64) -> Result { - self.github.pr_body(self.spec, number) - } - - fn set_pr_body(&mut self, number: u64, body: &str) -> Result<()> { - self.github.set_pr_body(self.spec, number, body) - } - - fn step(&mut self, line: String) { - progress::step(line); - } -} - -/// `checklist` wrapped in the Tickets checklist markers. -fn between_markers(checklist: &str) -> String { - format!("{CHECKLIST_START}\n{checklist}{CHECKLIST_END}\n") -} - -/// `body` with its Tickets checklist, the text from its first start marker -/// to the next end marker, replaced by `checklist` between the markers, or -/// with that appended if it has no such pair of markers. -fn with_checklist(body: &str, checklist: &str) -> String { - let checklist = between_markers(checklist); - if let Some(start) = body.find(CHECKLIST_START) - && let Some(end) = body[start..].find(CHECKLIST_END) - { - let end = start + end + CHECKLIST_END.len(); - let end = if body[end..].starts_with('\n') { - end + 1 - } else { - end - }; - return format!("{}{checklist}{}", &body[..start], &body[end..]); - } - if body.is_empty() { - return checklist; - } - format!("{}\n\n{checklist}", body.trim_end()) -} - -#[cfg(test)] -mod in_memory { - use anyhow::{Result, bail}; - - use super::Outside; - use crate::github::{PrState, PullRequest}; - - /// A call of [`Outside`] that can fail. - #[derive(Debug, Clone, Copy, PartialEq, Eq)] - pub enum Fails { - PullRequest, - ConvertToDraft, - SpecTitle, - CreateDraft, - PrBody, - SetPrBody, - } - - /// A call the Spec PR made, in the order it made it. - #[derive(Debug, PartialEq, Eq)] - pub enum Call { - /// It read the pull request from the Spec branch. - PullRequest, - ConvertToDraft, - SpecTitle, - /// It opened a draft pull request with this title and body. - CreateDraft { - title: String, - body: String, - }, - /// It read the body of this pull request. - PrBody(u64), - /// It set the body of this pull request to this. - SetPrBody(u64, String), - /// It handed on this progress line. - Step(String), - } - - /// The pull request from the Spec branch, as the adapter holds it. - #[derive(Clone, Copy)] - pub struct Pr { - pub number: u64, - pub draft: bool, - pub open: bool, - } - - impl Pr { - /// An open draft, numbered `number`. - pub fn draft(number: u64) -> Self { - Pr { - number, - draft: true, - open: true, - } - } - } - - /// The pull request from the Spec branch held in memory, if there is - /// one, with its body, and the Spec's title. A draft it opens is - /// numbered 30. - pub struct InMemory { - pub pr: Option, - pub body: String, - pub title: String, - /// Whether the pull request it opens is then not found. - pub opened_is_lost: bool, - /// The calls that fail, every time. - pub failing: Vec, - /// Every call made, in order. - pub calls: Vec, - } - - impl Default for InMemory { - /// No pull request, and every call succeeding. - fn default() -> Self { - InMemory { - pr: None, - body: String::new(), - title: "Widgets".to_string(), - opened_is_lost: false, - failing: Vec::new(), - calls: Vec::new(), - } - } - } - - impl InMemory { - /// Record `call`, then fail if `what` is to fail. - fn call(&mut self, call: Call, what: Fails) -> Result<()> { - self.calls.push(call); - if self.failing.contains(&what) { - bail!("{what:?} failed"); - } - Ok(()) - } - } - - /// The URL of pull request `number`. - pub fn url(number: u64) -> String { - format!("https://github.com/acme/widgets/pull/{number}") - } - - impl Outside for InMemory { - fn pull_request(&mut self) -> Result> { - self.call(Call::PullRequest, Fails::PullRequest)?; - Ok(self.pr.map(|pr| PullRequest { - number: pr.number, - url: url(pr.number), - state: if pr.open { - PrState::Open - } else { - PrState::Closed - }, - head: "issue-20".to_string(), - base: "main".to_string(), - is_draft: pr.draft, - })) - } - - fn convert_to_draft(&mut self) -> Result<()> { - self.call(Call::ConvertToDraft, Fails::ConvertToDraft)?; - if let Some(pr) = &mut self.pr { - pr.draft = true; - } - Ok(()) - } - - fn spec_title(&mut self) -> Result { - self.call(Call::SpecTitle, Fails::SpecTitle)?; - Ok(self.title.clone()) - } - - fn create_draft(&mut self, title: &str, body: &str) -> Result<()> { - let call = Call::CreateDraft { - title: title.to_string(), - body: body.to_string(), - }; - self.call(call, Fails::CreateDraft)?; - if !self.opened_is_lost { - self.pr = Some(Pr::draft(30)); - } - self.body = body.to_string(); - Ok(()) - } - - fn pr_body(&mut self, number: u64) -> Result { - self.call(Call::PrBody(number), Fails::PrBody)?; - Ok(self.body.clone()) - } - - fn set_pr_body(&mut self, number: u64, body: &str) -> Result<()> { - self.call(Call::SetPrBody(number, body.to_string()), Fails::SetPrBody)?; - self.body = body.to_string(); - Ok(()) - } - - fn step(&mut self, line: String) { - self.calls.push(Call::Step(line)); - } - } -} - -#[cfg(test)] -mod tests { - use super::in_memory::{Call, Fails, InMemory, Pr, url}; - use super::*; - - fn spec() -> IssueUrl { - IssueUrl::parse("https://github.com/acme/widgets/issues/20").unwrap() - } - - /// The Spec PR of Spec #20 from `issue-20` into `main`, resumed through - /// `outside`. - fn resume(spec: &IssueUrl, outside: InMemory) -> Result> { - SpecPr::resume_through(outside, spec, "issue-20", "main") - } - - /// The calls made after resuming. - fn after_resume<'a>(spec_pr: &'a SpecPr<'_, InMemory>) -> &'a [Call] { - let calls = &spec_pr.outside.calls; - assert_eq!(calls.first(), Some(&Call::PullRequest), "{calls:?}"); - &calls[1..] - } - - fn step(line: &str) -> Call { - Call::Step(line.to_string()) - } - - /// An open PR, numbered 9, holding `body`. - fn open_with(body: &str) -> InMemory { - InMemory { - pr: Some(Pr::draft(9)), - body: body.to_string(), - ..InMemory::default() - } - } - - const UPDATING: &str = "updating the Spec PR's Tickets checklist"; - - const LIST: &str = "\nnew\n\n"; - - #[test] - fn the_checklist_replaces_the_one_between_the_markers_and_leaves_the_rest() { - let body = "Intro.\n\n\nold\n\n\nCloses #20\n"; - - assert_eq!( - with_checklist(body, "new\n"), - format!("Intro.\n\n{LIST}\nCloses #20\n") - ); - } - - #[test] - fn the_checklist_is_appended_when_the_markers_are_gone() { - assert_eq!( - with_checklist("A rewritten body.\n\nCloses #20\n", "new\n"), - format!("A rewritten body.\n\nCloses #20\n\n{LIST}") - ); - assert_eq!(with_checklist("", "new\n"), LIST); - } - - #[test] - fn the_checklist_is_appended_when_only_one_marker_is_left() { - let body = "Text\n\n\nmore"; - - assert_eq!(with_checklist(body, "new\n"), format!("{body}\n\n{LIST}")); - } - - #[test] - fn resuming_with_a_ready_open_pr_converts_it_to_a_draft() { - let spec = spec(); - let outside = InMemory { - pr: Some(Pr { - draft: false, - ..Pr::draft(9) - }), - ..InMemory::default() - }; - - let spec_pr = resume(&spec, outside).unwrap(); - - assert_eq!(after_resume(&spec_pr), [Call::ConvertToDraft]); - assert_eq!(spec_pr.url(), Some(url(9).as_str())); - } - - #[test] - fn resuming_with_a_draft_a_closed_pr_or_none_converts_nothing() { - let closed = Pr { - draft: false, - open: false, - ..Pr::draft(9) - }; - for (pr, url) in [ - (Some(Pr::draft(9)), Some(url(9))), - (Some(closed), None), - (None, None), - ] { - let spec = spec(); - let outside = InMemory { - pr, - ..InMemory::default() - }; - - let spec_pr = resume(&spec, outside).unwrap(); - - assert_eq!(after_resume(&spec_pr), []); - assert_eq!(spec_pr.url(), url.as_deref()); - } - } - - #[test] - fn landing_with_no_pr_opens_one_as_a_draft_titled_from_the_spec_and_closing_it() { - let spec = spec(); - let mut spec_pr = resume(&spec, InMemory::default()).unwrap(); - - let landed = spec_pr.landed("- [x] #21 landed\n").unwrap().to_string(); - - assert_eq!(landed, url(30)); - assert_eq!( - after_resume(&spec_pr), - [ - step("opening the Spec PR into main as a draft"), - Call::SpecTitle, - Call::CreateDraft { - title: "Widgets".to_string(), - body: "The work on Spec #20, gathered from its Tickets on issue-20.\n\n\ - Closes #20\n\n\ - \n- [x] #21 landed\n\ - \n" - .to_string(), - }, - Call::PullRequest, - ] - ); - assert_eq!(spec_pr.url(), Some(url(30).as_str())); - } - - #[test] - fn landing_when_the_pr_just_opened_is_not_found_fails() { - let spec = spec(); - let outside = InMemory { - opened_is_lost: true, - ..InMemory::default() - }; - let mut spec_pr = resume(&spec, outside).unwrap(); - - let error = spec_pr.landed("new\n").unwrap_err(); - - assert_eq!(format!("{error:#}"), "the Spec PR just opened is not found"); - assert_eq!(spec_pr.url(), None); - } - - #[test] - fn landing_with_a_pr_open_writes_the_checklist_and_creates_nothing() { - let spec = spec(); - let mut spec_pr = resume(&spec, open_with("Closes #20\n")).unwrap(); - - let landed = spec_pr.landed("new\n").unwrap().to_string(); - - assert_eq!(landed, url(9)); - assert_eq!( - after_resume(&spec_pr), - [ - Call::PrBody(9), - step(UPDATING), - Call::SetPrBody(9, format!("Closes #20\n\n{LIST}")), - ] - ); - } - - #[test] - fn showing_with_no_pr_makes_no_call() { - let spec = spec(); - let mut spec_pr = resume(&spec, InMemory::default()).unwrap(); - - spec_pr.show("new\n"); - - assert_eq!(after_resume(&spec_pr), []); - } - - #[test] - fn showing_an_unchanged_checklist_reads_the_body_but_does_not_set_it() { - let spec = spec(); - let mut spec_pr = resume(&spec, open_with(&format!("Closes #20\n\n{LIST}"))).unwrap(); - - spec_pr.show("new\n"); - - assert_eq!(after_resume(&spec_pr), [Call::PrBody(9)]); - } - - #[test] - fn showing_a_checklist_that_cannot_be_set_is_only_a_progress_line() { - let spec = spec(); - let outside = InMemory { - failing: vec![Fails::SetPrBody], - ..open_with("Closes #20\n") - }; - let mut spec_pr = resume(&spec, outside).unwrap(); - - spec_pr.show("new\n"); - - assert_eq!( - after_resume(&spec_pr).last(), - Some(&step( - "could not update the Spec PR's Tickets checklist: SetPrBody failed" - )) - ); - } - - #[test] - fn putting_back_with_no_pr_fails() { - let spec = spec(); - let mut spec_pr = resume(&spec, InMemory::default()).unwrap(); - - let error = spec_pr - .put_back( - "new\n", - &Identified { - number: 30, - url: "https://github.com/acme/widgets/pull/30".to_string(), - }, - ) - .unwrap_err(); - - assert_eq!(format!("{error:#}"), "the Spec PR is not open"); - assert_eq!(after_resume(&spec_pr), []); - } - - #[test] - fn putting_back_writes_the_checklist() { - let spec = spec(); - let mut spec_pr = resume(&spec, open_with("Rewritten. Closes #20\n")).unwrap(); - - spec_pr - .put_back( - "new\n", - &Identified { - number: 30, - url: "https://github.com/acme/widgets/pull/30".to_string(), - }, - ) - .unwrap(); - - assert_eq!( - spec_pr.outside.body, - format!("Rewritten. Closes #20\n\n{LIST}") - ); - } - - #[test] - fn putting_back_a_checklist_that_cannot_be_set_fails() { - let spec = spec(); - let outside = InMemory { - failing: vec![Fails::SetPrBody], - ..open_with("Rewritten. Closes #20\n") - }; - let mut spec_pr = resume(&spec, outside).unwrap(); - - let error = spec_pr - .put_back( - "new\n", - &Identified { - number: 30, - url: "https://github.com/acme/widgets/pull/30".to_string(), - }, - ) - .unwrap_err(); - - assert_eq!(format!("{error:#}"), "SetPrBody failed"); - } -} diff --git a/tests/spec_run.rs b/tests/spec_run.rs index fd8cef795..0b5eeabd1 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1200,6 +1200,65 @@ fn a_spec_pr_closed_during_the_spec_review_fails_the_spec_run() { assert_eq!(spec_pr(&scenario)["isDraft"], true); } +#[test] +fn a_failed_ticket_that_retargets_the_spec_pr_leaves_its_body_untouched() { + let scenario = linear_spec(); + scenario.origin_has_branch("develop", "main", &[]); + scenario.agent_does_for( + 22, + r#"gh fake pr issue-20 base '"develop"' +gh fake pr issue-20 body '"untouched"' +exit 1 +"#, + ); + + let result = scenario.run(&[&spec_url(&scenario)]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(spec_pr(&scenario)["body"], "untouched"); + assert_contains(&result.stderr, "PR targets develop, not main"); + assert_contains(&result.stderr, "#22 failed"); + assert!(!sessions_by_issue(&scenario).contains(&SPEC.to_string())); +} + +#[test] +fn successful_ticket_completion_with_an_invalid_spec_pr_starts_no_review() { + for replacement in [false, true] { + let scenario = linear_spec(); + scenario.origin_has_branch("develop", "main", &[]); + let change = if replacement { + r#"gh fake pr issue-20 state '"CLOSED"' +gh pr create --base main --head issue-20 --title Replacement --body untouched"# + } else { + r#"gh fake pr issue-20 base '"develop"' +gh fake pr issue-20 body '"untouched"'"# + }; + scenario.agent_does_for( + 22, + &format!("{}\n{change}\n", agent_lands(22, "second.txt")), + ); + + let result = scenario.run(&[&spec_url(&scenario)]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!(!sessions_by_issue(&scenario).contains(&SPEC.to_string())); + assert_eq!(spec_pr(&scenario)["body"], "untouched"); + assert_contains( + &result.stderr, + if replacement { + "is closed, not open" + } else { + "PR targets develop, not main" + }, + ); + assert_contains(&result.stderr, "#22 landed"); + assert_eq!(scenario.gh_state()["issues"]["22"], "CLOSED"); + if replacement { + assert_eq!(spec_pr(&scenario)["isDraft"], false); + } + } +} + #[test] fn a_spec_pr_replaced_during_review_gets_its_checklist_on_the_delivered_identity() { for failing_review in [false, true] { From 3820d9555e1e680f94fddb95d1b5f1b72a427567 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 7 Oct 2026 10:29:13 -0400 Subject: [PATCH 2/2] Share typed PR observation fixtures after Standards review --- src/pull_request.rs | 199 ++++++++++++++++++++------------------------ 1 file changed, 91 insertions(+), 108 deletions(-) diff --git a/src/pull_request.rs b/src/pull_request.rs index 7d19d0b0f..882183b4b 100644 --- a/src/pull_request.rs +++ b/src/pull_request.rs @@ -719,6 +719,45 @@ mod tests { observed } + #[derive(Clone, Copy, Debug)] + enum ObservationChange { + WrongBase, + WrongHead, + Fork, + WrongRepository, + Replacement, + Closed, + Merged, + Missing, + Unreadable, + Ready, + Draft, + } + + impl ObservationChange { + fn answer(self, mut observed: PrSnapshot) -> Answer { + match self { + Self::WrongBase => observed.pr.base = "develop".to_string(), + Self::WrongHead => observed.pr.head = "other".to_string(), + Self::Fork => observed.from_fork = true, + Self::WrongRepository => { + observed.pr.url = "https://github.com/other/widgets/pull/12".to_string() + } + Self::Replacement => { + observed.pr.number = 13; + observed.pr.url = "https://github.com/acme/widgets/pull/13".to_string(); + } + Self::Closed => observed.pr.state = PrState::Closed, + Self::Merged => observed.pr.state = PrState::Merged, + Self::Missing => return Ok(None), + Self::Unreadable => return Err("read failed"), + Self::Ready => observed.pr.is_draft = false, + Self::Draft => observed.pr.is_draft = true, + } + Ok(Some(observed)) + } + } + #[test] fn resume_drafts_the_observed_number_even_when_branch_discovery_would_find_a_replacement() { let (mut pr, world) = owner(World { @@ -741,38 +780,22 @@ mod tests { #[test] fn resume_revalidates_before_drafting_and_never_follows_a_replacement() { for change in [ - "base", - "head", - "fork", - "repository", - "number", - "closed", - "missing", - "unreadable", + ObservationChange::WrongBase, + ObservationChange::WrongHead, + ObservationChange::Fork, + ObservationChange::WrongRepository, + ObservationChange::Replacement, + ObservationChange::Closed, + ObservationChange::Missing, + ObservationChange::Unreadable, ] { - let mut changed = snapshot(); - match change { - "base" => changed.pr.base = "develop".to_string(), - "head" => changed.pr.head = "other".to_string(), - "fork" => changed.from_fork = true, - "repository" => { - changed.pr.url = "https://github.com/other/widgets/pull/12".to_string() - } - "number" => changed = numbered(13), - "closed" => changed.pr.state = PrState::Closed, - _ => (), - } - let next = match change { - "missing" => Ok(None), - "unreadable" => Err("read failed"), - _ => Ok(Some(changed)), - }; + let next = change.answer(snapshot()); let (mut pr, world) = owner(World { observations: VecDeque::from([Ok(Some(snapshot())), next]), ..World::default() }); - assert!(pr.resume_spec().is_err(), "{change}"); - assert!(world.borrow().draft.is_empty(), "{change}"); + assert!(pr.resume_spec().is_err(), "{change:?}"); + assert!(world.borrow().draft.is_empty(), "{change:?}"); assert_eq!( world.borrow().selected.last(), Some(&("12".to_string(), false)) @@ -782,22 +805,17 @@ mod tests { #[test] fn resume_rejects_initial_wrong_branches_forks_and_repository_before_any_transition() { - for change in ["base", "head", "fork", "repository"] { - let mut changed = snapshot(); - match change { - "base" => changed.pr.base = "develop".to_string(), - "head" => changed.pr.head = "other".to_string(), - "fork" => changed.from_fork = true, - "repository" => { - changed.pr.url = "https://github.com/other/widgets/pull/12".to_string() - } - _ => unreachable!(), - } + for change in [ + ObservationChange::WrongBase, + ObservationChange::WrongHead, + ObservationChange::Fork, + ObservationChange::WrongRepository, + ] { let (mut pr, world) = owner(World { - observations: VecDeque::from([Ok(Some(changed))]), + observations: VecDeque::from([change.answer(snapshot())]), ..World::default() }); - assert!(pr.resume_spec().is_err(), "{change}"); + assert!(pr.resume_spec().is_err(), "{change:?}"); assert!(world.borrow().draft.is_empty()); assert!(world.borrow().edits.is_empty()); } @@ -874,42 +892,26 @@ mod tests { #[test] fn creation_requires_an_open_draft_with_the_returned_number_and_expected_ownership() { for change in [ - "missing", - "unreadable", - "number", - "base", - "head", - "fork", - "repository", - "closed", - "ready", + ObservationChange::Missing, + ObservationChange::Unreadable, + ObservationChange::Replacement, + ObservationChange::WrongBase, + ObservationChange::WrongHead, + ObservationChange::Fork, + ObservationChange::WrongRepository, + ObservationChange::Closed, + ObservationChange::Ready, ] { let mut created = snapshot(); created.pr.is_draft = true; - match change { - "number" => created = numbered(13), - "base" => created.pr.base = "develop".to_string(), - "head" => created.pr.head = "other".to_string(), - "fork" => created.from_fork = true, - "repository" => { - created.pr.url = "https://github.com/other/widgets/pull/12".to_string() - } - "closed" => created.pr.state = PrState::Closed, - "ready" => created.pr.is_draft = false, - _ => (), - } - let observation = match change { - "missing" => Ok(None), - "unreadable" => Err("read failed"), - _ => Ok(Some(created)), - }; + let observation = change.answer(created); let (mut pr, world) = owner(World { observations: VecDeque::from([Ok(None)]), completion: VecDeque::from([observation]), ..World::default() }); pr.resume_spec().unwrap(); - assert!(pr.land_spec("landed\n").is_err(), "{change}"); + assert!(pr.land_spec("landed\n").is_err(), "{change:?}"); assert_eq!( world.borrow().selected.last(), Some(&("12".to_string(), true)) @@ -921,41 +923,24 @@ mod tests { #[test] fn accounting_never_edits_or_lands_an_invalid_numbered_observation() { for change in [ - "base", - "head", - "fork", - "repository", - "number", - "closed", - "merged", - "missing", - "unreadable", + ObservationChange::WrongBase, + ObservationChange::WrongHead, + ObservationChange::Fork, + ObservationChange::WrongRepository, + ObservationChange::Replacement, + ObservationChange::Closed, + ObservationChange::Merged, + ObservationChange::Missing, + ObservationChange::Unreadable, ] { - let mut changed = snapshot(); - match change { - "base" => changed.pr.base = "develop".to_string(), - "head" => changed.pr.head = "other".to_string(), - "fork" => changed.from_fork = true, - "repository" => { - changed.pr.url = "https://github.com/other/widgets/pull/12".to_string() - } - "number" => changed = numbered(13), - "closed" => changed.pr.state = PrState::Closed, - "merged" => changed.pr.state = PrState::Merged, - _ => (), - } - let observation = match change { - "missing" => Ok(None), - "unreadable" => Err("read failed"), - _ => Ok(Some(changed)), - }; + let observation = change.answer(snapshot()); let (mut pr, world) = owner(World { completion: VecDeque::from([observation]), ..World::default() }); pr.resume_spec().unwrap(); pr.show_checklist("new\n"); - assert!(pr.land_spec("new\n").is_err(), "{change}"); + assert!(pr.land_spec("new\n").is_err(), "{change:?}"); assert_eq!(world.borrow().body, "Closes #7\n"); assert!(world.borrow().edits.is_empty()); assert!(world.borrow().created.is_empty()); @@ -1274,31 +1259,29 @@ mod tests { fn every_unknown_mergeability_snapshot_revalidates_target_state_and_ownership() { // First UNKNOWN is followed immediately by the changed snapshot; // no pause is needed to script a change during polling. - for change in ["base", "closed", "draft", "head", "fork", "number"] { + for change in [ + ObservationChange::WrongBase, + ObservationChange::Closed, + ObservationChange::Draft, + ObservationChange::WrongHead, + ObservationChange::Fork, + ObservationChange::Replacement, + ] { let mut unknown = snapshot(); unknown.mergeable = Mergeable::Unknown; - let mut changed = unknown.clone(); - match change { - "base" => changed.pr.base = "develop".to_string(), - "closed" => changed.pr.state = PrState::Closed, - "draft" => changed.pr.is_draft = true, - "head" => changed.pr.head = "other".to_string(), - "fork" => changed.from_fork = true, - "number" => changed.pr.number = 13, - _ => unreachable!(), - } + let changed = change.answer(unknown.clone()); let (mut pr, world) = owner(World { observations: VecDeque::from([ Ok(Some(snapshot())), Ok(Some(snapshot())), Ok(Some(unknown)), - Ok(Some(changed)), + changed, ]), grace: Duration::from_secs(60), ..World::default() }); pr.mark_ready(None).unwrap(); - assert!(pr.ensure_ready_and_mergeable().is_err(), "{change}"); + assert!(pr.ensure_ready_and_mergeable().is_err(), "{change:?}"); assert!( world.borrow().selected[1..] .iter()