diff --git a/CONTEXT.md b/CONTEXT.md index bc699c467..5fc0aef88 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -62,7 +62,7 @@ _Avoid_: task, sub-task One invocation of the factory on a single issue that is not a **Spec**, from launch to cleanup, ending in one pull request. Started directly on an **Issue URL**, by a **Spec run** for one of its **Tickets**, by an **Architect run** for the Ticket its **Architecture review** published, or by a **Pickup run** for the **Ready issue** it took. **Claim**: -The mark that the factory has taken an issue: thirdshift labels the issue `in-progress`, in place of `ready-for-agent` if it has that, when a **Run** or a **Spec run** starts on it, whether started on its **Issue URL** or by an **Architect run** or a **Pickup run**. A **Ticket**'s Run in a Spec run and a **Base fix** make none. A Claim is released, `ready-for-agent` put back, only when the Run ends with nothing on `origin` to take over: no **Issue branch** and no pull request. Otherwise it stays while the issue is open, whether its pull request is ready for review or the Run failed, until the **Day shift** relabels it, and thirdshift takes the label off once the issue is closed. +The mark that the factory has taken an issue: thirdshift labels the issue `in-progress`, in place of `ready-for-agent` if it has that, when a **Run** or a **Spec run** starts on it, whether started on its **Issue URL** or by an **Architect run** or a **Pickup run**. A **Ticket**'s Run in a Spec run and a **Base fix** make none. Ending a Claim finishes even after Command interruption. A Claim is released, `ready-for-agent` put back, only when the Run ends with nothing on `origin` to take over: no **Issue branch** and no pull request. Otherwise it stays while the issue is open, whether its pull request is ready for review or the Run failed, until the **Day shift** relabels it, and thirdshift takes the label off once the issue is closed. _Avoid_: lock (a lock is per machine and ends with its process), assignment **Spec run**: @@ -126,7 +126,7 @@ An open **Ticket** labelled with a triage role that says it is not agent work: ` The agent session a **Spec run** starts once every **Ticket** is done: it reviews the whole **Spec branch** against the **Base branch** and the **Spec**, fixes what it agrees with, and writes the **Spec PR**'s description, listing the rest as **Unaddressed findings**. **Failed spec run**: -A **Spec run** that ends, including by interruption, with any of its **Tickets** not done, or with its **Spec PR** not ready, mergeable and green (or, when asked to merge, not merged). It still takes every Ticket it can reach, and a failed Ticket stops only the Tickets it blocks. +A **Spec run** that ends, including by interruption, with any of its **Tickets** not done, or with its **Spec PR** not ready, mergeable and green (or, when asked to merge, not merged). It still takes every Ticket it can reach, and a failed Ticket stops only the Tickets it blocks. After Command interruption it starts no new Ticket or Spec review, drains its active Tickets, and finishes publishing their final Tickets checklist and outcome lines. **Merge run**: A **Run** asked to end with its pull request merged rather than left for review, by the command it was started with or by the **User config**. It does everything a **Run** does, then a **Self-merge**. A **Ticket**'s Run in a **Spec run** is always a Merge run into the **Spec branch**; the command's ask applies to the **Spec PR**. @@ -148,7 +148,7 @@ Writing the **User config** by answering a few questions, one per setting that m _Avoid_: init, onboarding, configure **Self-merge**: -The step at the end of a **Merge run** in which thirdshift itself merges the pull request into the **Base branch** with a merge commit, once it is open, ready for review, mergeable and green on the head commit it merges. No human reviews it first. It ends with the **Issue branch** deleted, the issue closed by thirdshift, unless it is already closed, and the issue's **Claim** removed. +The step at the end of a **Merge run** in which thirdshift itself merges the pull request into the **Base branch** with a merge commit, once it is open, ready for review, mergeable and green on the head commit it merges. No human reviews it first. Once the merge is confirmed at the requested head, completion continues even after Command interruption. It ends with the **Issue branch** deleted, the issue closed by thirdshift, unless it is already closed, and the issue's **Claim** removed. **Policy refusal**: A merge the **Self-merge** tried that failed, where the round of the Repair loop that followed found nothing to fix: the **Base branch** unchanged, no conflict, CI green or absent, the pull request mergeable. The cause is a repository setting or rule, such as merge commits disallowed or a review required. thirdshift never reads GitHub's error text to decide it. The **Merge run** is a **Failed run** that leaves the pull request ready for review. @@ -169,7 +169,7 @@ A **Merge run** into a **Run**'s **Base branch** that thirdshift starts when tha _Avoid_: hotfix, base repair (a **Repair** works on the Issue branch) **Failed run**: -A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI. For a **Merge run**, it is also a Run that ends without its pull request merged. Its work is still pushed so nothing is lost (or, if the push fails, its worktree and local **Issue branch** are kept), and its open pull request, if any, is converted back to a draft, unless the pull request is ready, mergeable and green and only the **Self-merge** could not happen. +A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI. For a **Merge run**, it is also a Run that ends without its pull request merged. Even after Command interruption, any unfinished merge is aborted and its work is still pushed so nothing is lost (or, if the push fails, its worktree and local **Issue branch** are kept), and its open pull request, if any, is converted back to a draft through the same finishing path, unless the pull request is ready, mergeable and green and only the **Self-merge** could not happen. **Issue branch**: A branch a **Run** works on for one issue: `issue-` (the first), then `issue--branch-` for k ≥ 2. A number counts as used once its pull request is merged or closed, even if the branch itself was deleted. @@ -197,7 +197,7 @@ The full transcript of one agent session, one per session, a **Resume** and each _Avoid_: transcript, session file **Command**: -One invocation of thirdshift that does factory work: a **Run** or **Spec run** started on an **Issue URL**, an **Architect run**, or a **Pickup run**. A child Run, a **Ticket**'s Run or a **Base fix**, is part of the command that started it, not one of its own. +One invocation of thirdshift that does factory work: a **Run** or **Spec run** started on an **Issue URL**, an **Architect run**, or a **Pickup run**. A child Run, a **Ticket**'s Run or a **Base fix**, is part of the command that started it, not one of its own. Interruption stops its ordinary CLI work and prevents more from starting, while Failed run salvage, confirmed Self-merge completion, Claim ending and Spec accounting still finish; finishing never clears the recorded interruption. _Avoid_: invocation, job **Command log**: diff --git a/src/base_fix.rs b/src/base_fix.rs index d3716d53b..61e9d801f 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -17,7 +17,7 @@ use anyhow::{Result, bail}; use crate::child_run::{self, Ended, Kind}; use crate::ci::{self, FailedChecks}; use crate::git::Git; -use crate::github::{self, Check, ListedIssue}; +use crate::github::{Check, GitHub, ListedIssue}; use crate::harness::Choice; use crate::issue::IssueUrl; use crate::labels::{Label, Labels, READY_FOR_AGENT}; @@ -484,7 +484,7 @@ impl Outside for LaunchAndGitHub<'_> { /// One `gh issue list`. fn open_fixes(&mut self) -> Result> { - github::open_issues_labelled(&self.issue.repo_slug(), BASE_FIX) + GitHub::new().open_issues_labelled(&self.issue.repo_slug(), BASE_FIX) } /// Left if the mark file is there and its lock free when first opened. @@ -508,11 +508,11 @@ impl Outside for LaunchAndGitHub<'_> { } fn issue_is_open(&mut self, issue: &IssueUrl) -> Result { - github::issue_is_open(issue) + GitHub::new().issue_is_open(issue) } fn create_issue(&mut self, title: &str, body: &str, labels: &[Label]) -> Result { - github::create_issue(self.issue, title, body, labels) + GitHub::new().create_issue(self.issue, title, body, labels) } fn mark_running(&mut self, number: u64) -> Result { diff --git a/src/branch.rs b/src/branch.rs index 64c3e3842..8eb1af8ca 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -4,7 +4,7 @@ use anyhow::{Result, bail}; use crate::git::Git; -use crate::github::{self, PrState, PullRequest}; +use crate::github::{GitHub, PrState, PullRequest}; use crate::issue::IssueUrl; pub enum Selection { @@ -66,8 +66,8 @@ impl Selection { /// their PRs in any state: the highest number seen on either decides. Fails /// if a local copy of the chosen branch in the Launch directory differs from /// origin's: the Run replaces it and deletes it at cleanup. -pub fn select(launch: &Git, issue: &IssueUrl) -> Result { - select_through(&GitHubAndOrigin { launch }, issue) +pub fn select(launch: &Git, github: &GitHub, issue: &IssueUrl) -> Result { + select_through(&GitHubAndOrigin { launch, github }, issue) } /// What shows an issue was started. @@ -82,8 +82,8 @@ pub enum Started { /// What shows `issue` was ever started, if it was: the first Issue branch for /// it on origin, or else the newest pull request from one, open, merged or /// closed. -pub fn started(launch: &Git, issue: &IssueUrl) -> Result> { - started_through(&GitHubAndOrigin { launch }, issue) +pub fn started(launch: &Git, github: &GitHub, issue: &IssueUrl) -> Result> { + started_through(&GitHubAndOrigin { launch, github }, issue) } /// What selection reads of origin, GitHub and the Launch directory. Each @@ -109,6 +109,7 @@ struct OnOrigin { /// The reads of GitHub, of the Launch directory `launch` and of its origin. struct GitHubAndOrigin<'a> { launch: &'a Git, + github: &'a GitHub, } impl Reads for GitHubAndOrigin<'_> { @@ -135,7 +136,8 @@ impl Reads for GitHubAndOrigin<'_> { /// One `gh pr list`. fn pull_requests(&self, issue: &IssueUrl, first_branch: &str) -> Result> { - github::pull_requests_with_head_prefix(issue, first_branch) + self.github + .pull_requests_with_head_prefix(issue, first_branch) } fn local_head(&self, branch: &str) -> Result> { diff --git a/src/ci.rs b/src/ci.rs index 617ecd254..5f5a8426d 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -9,7 +9,7 @@ use std::time::{Duration, Instant}; use anyhow::Result; -use crate::github::{self, ActionsJob, Check, CheckState}; +use crate::github::{ActionsJob, Check, CheckState, GitHub}; use crate::issue::IssueUrl; use crate::poll; use crate::progress; @@ -109,11 +109,11 @@ impl Outside for OnGitHub<'_> { type Deadline = Instant; fn checks_on(&mut self, sha: &str) -> Result> { - github::checks_on(self.issue, sha) + GitHub::new().checks_on(self.issue, sha) } fn rerun_failed_jobs(&mut self, workflow_run: u64) -> Result<()> { - github::rerun_failed_jobs(self.issue, workflow_run) + GitHub::new().rerun_failed_jobs(self.issue, workflow_run) } fn pause(&mut self) -> Result<()> { diff --git a/src/claim.rs b/src/claim.rs index 0255809e2..351eb0edd 100644 --- a/src/claim.rs +++ b/src/claim.rs @@ -14,7 +14,7 @@ use anyhow::{Context, Result}; use crate::branch; use crate::git::Git; -use crate::github; +use crate::github::GitHub; use crate::interrupt; use crate::issue::IssueUrl; use crate::labels::{self, Edit, Label, Labels, READY_FOR_AGENT}; @@ -45,7 +45,13 @@ pub struct Claim<'a> { /// `ready-for-agent`, is left as it is, with no request made. A failure names /// the Claim as what could not be made. pub fn make<'a>(issue: &'a IssueUrl, launch: &'a Git) -> Result> { - let claimed = Claimed::make(&mut OnGitHub { launch }, issue)?; + let claimed = Claimed::make( + &mut OnGitHub { + launch, + github: GitHub::new(), + }, + issue, + )?; Ok(Claim { launch, claimed }) } @@ -63,7 +69,13 @@ impl Claim<'_> { /// warning naming what to run by hand, and an interrupt doesn't stop it. pub fn end(self, reached: Option) { let launch = self.launch.completion(); - self.claimed.end(&mut OnGitHub { launch: &launch }, reached); + self.claimed.end( + &mut OnGitHub { + launch: &launch, + github: GitHub::new().completion(), + }, + reached, + ); } } @@ -98,15 +110,16 @@ struct State { /// The issue on GitHub, and origin as the Launch directory `launch` sees it. struct OnGitHub<'a> { launch: &'a Git, + github: GitHub, } impl Outside for OnGitHub<'_> { fn labels(&mut self, issue: &IssueUrl) -> Result { - github::issue_labels(issue) + self.github.issue_labels(issue) } fn state(&mut self, issue: &IssueUrl) -> Result { - let issue = github::issue(issue)?; + let issue = self.github.issue(issue)?; Ok(State { is_open: issue.is_open, labels: issue.labels, @@ -114,11 +127,11 @@ impl Outside for OnGitHub<'_> { } fn started(&mut self, issue: &IssueUrl) -> Result { - Ok(branch::started(self.launch, issue)?.is_some()) + Ok(branch::started(self.launch, &self.github, issue)?.is_some()) } fn apply(&mut self, edit: &Edit) -> Result<()> { - edit.apply() + edit.apply(&self.github) } fn interrupted(&mut self) -> bool { diff --git a/src/delivery.rs b/src/delivery.rs index 774991160..dea30caec 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -20,7 +20,7 @@ mod repair_loop; use crate::base_fix::BaseFix; use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::{FailedRun, PolicyRefusal, interrupted_or}; -use crate::github::{self, Mergeable, PullRequest}; +use crate::github::{GitHub, Mergeable, PullRequest}; use crate::harness::Choice; use crate::host; use crate::interrupt; @@ -433,19 +433,19 @@ impl Outside for InWorktree<'_> { } fn pull_request(&mut self) -> Result> { - github::pull_request_for(self.issue, self.worktree.branch()) + GitHub::new().pull_request_for(self.issue, self.worktree.branch()) } fn pr_body(&mut self, number: u64) -> Result { - github::pr_body(self.issue, number) + GitHub::new().pr_body(self.issue, number) } fn set_pr_body(&mut self, number: u64, body: &str) -> Result<()> { - github::set_pr_body(self.issue, number, body) + GitHub::new().set_pr_body(self.issue, number, body) } fn mark_ready(&mut self) -> Result<()> { - github::mark_ready(self.issue, self.worktree.branch()) + GitHub::new().mark_ready(self.issue, self.worktree.branch()) } fn take_to_goal(&mut self, pr_url: &str, goal: Goal) -> Result<()> { @@ -465,11 +465,11 @@ impl Outside for InWorktree<'_> { } fn issue_is_open(&mut self) -> Result { - github::issue_is_open(self.issue) + GitHub::new().completion().issue_is_open(self.issue) } fn close_issue(&mut self, comment: &str) -> Result<()> { - github::close_issue(self.issue, comment) + GitHub::new().completion().close_issue(self.issue, comment) } fn interrupted(&mut self) -> bool { @@ -529,11 +529,15 @@ impl FailedOutside for OnFailure<'_> { } fn pull_request(&mut self) -> Result> { - github::pull_request_for(self.issue, self.worktree.branch()) + GitHub::new() + .completion() + .pull_request_for(self.issue, self.worktree.branch()) } fn convert_to_draft(&mut self) -> Result<()> { - github::convert_to_draft(self.issue, self.worktree.branch()) + GitHub::new() + .completion() + .convert_to_draft(self.issue, self.worktree.branch()) } fn keep(&mut self) { @@ -648,7 +652,7 @@ impl repair_loop::Outside for RunOutside<'_> { } fn merge(&mut self, head: &str) -> Result<()> { - github::merge(self.issue, self.worktree.branch(), head) + GitHub::new().merge(self.issue, self.worktree.branch(), head) } fn interrupt_requested(&mut self) -> bool { @@ -672,12 +676,13 @@ fn pending(merge: Merge) -> Option { /// mergeable, waiting up to the grace period for GitHub to work out the last. fn ensure_pr_ready_and_mergeable(issue: &IssueUrl, branch: &str) -> Result<()> { progress::step("checking the PR is open, ready and mergeable"); - let pr = open(github::pull_request_for(issue, branch)?)?; + let github = GitHub::new(); + let pr = open(github.pull_request_for(issue, branch)?)?; if pr.is_draft { bail!("PR {} is a draft", pr.url); } let mergeable = poll::within(poll::grace_period(), || { - Ok(Some(github::mergeable(issue, branch)?).filter(|m| *m != Mergeable::Unknown)) + Ok(Some(github.mergeable(issue, branch)?).filter(|m| *m != Mergeable::Unknown)) })?; match mergeable { Some(Mergeable::Yes) => Ok(()), diff --git a/src/github.rs b/src/github.rs index f7b320aeb..44793362a 100644 --- a/src/github.rs +++ b/src/github.rs @@ -2,7 +2,7 @@ //! user. use std::fmt; -use std::process::Command; +use std::process::{Command, Output}; use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; @@ -10,37 +10,629 @@ use serde_json::Value; use crate::issue::IssueUrl; use crate::labels::{Label, Labels}; +use crate::process::{self, Control, Interruption}; -/// The `fields` of `issue`, as JSON. -fn issue_view(issue: &IssueUrl, fields: &str) -> Result { - gh_json(&[ - "issue", - "view", - &issue.number.to_string(), - "--repo", - &issue.repo_slug(), - "--json", - fields, - ]) -} +#[cfg(test)] +mod execution_tests; -/// Whether the issue `json` describes, with its `state`, is open. -fn state_is_open(json: &Value) -> Result { - Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") +/// GitHub CLI operations bound to an immutable interruption policy. +/// Ordinary views stop for Command interruption; completion views finish +/// independently without changing the recorded request or other views. +pub struct GitHub { + interruption: Interruption, } -/// Whether `issue` is open. -pub fn issue_is_open(issue: &IssueUrl) -> Result { - state_is_open(&issue_view(issue, "state")?) +impl GitHub { + pub fn new() -> Self { + Self { + interruption: Interruption::Ordinary, + } + } + + pub fn completion(&self) -> Self { + Self { + interruption: Interruption::Completion, + } + } + + /// The `fields` of `issue`, as JSON. + fn issue_view(&self, issue: &IssueUrl, fields: &str) -> Result { + self.gh_json(&[ + "issue", + "view", + &issue.number.to_string(), + "--repo", + &issue.repo_slug(), + "--json", + fields, + ]) + } + + /// Whether `issue` is open. + pub fn issue_is_open(&self, issue: &IssueUrl) -> Result { + state_is_open(&self.issue_view(issue, "state")?) + } + + /// The title of `issue`. + pub fn issue_title(&self, issue: &IssueUrl) -> Result { + let json = self.issue_view(issue, "title")?; + Ok(json["title"] + .as_str() + .context("gh output has no title")? + .to_string()) + } + + /// `issue`'s state, labels and when it was created. + pub fn issue(&self, issue: &IssueUrl) -> Result { + let json = self.issue_view(issue, "state,labels,createdAt")?; + let created = json["createdAt"] + .as_str() + .context("gh output has no createdAt")?; + Ok(Issue { + is_open: state_is_open(&json)?, + labels: labels_of(&json)?, + created: DateTime::parse_from_rfc3339(created) + .with_context(|| format!("gh output has an unreadable createdAt {created}"))? + .to_utc(), + }) + } + + /// The labels of `issue`. + pub fn issue_labels(&self, issue: &IssueUrl) -> Result { + labels_of(&self.issue_view(issue, "labels")?) + } + + /// Set `issue`'s labels to exactly `labels`, in one request, so a swap of + /// one label for another can't stop halfway. Through the REST API: `gh issue + /// edit` fails on the GitHub Projects (classic) sunset in older `gh`. + pub fn set_labels(&self, issue: &IssueUrl, labels: &Labels) -> Result<()> { + let path = labels_path(issue); + let fields: Vec = labels + .names() + .map(|label| format!("labels[]={label}")) + .collect(); + let mut args = vec!["api", "--method", "PUT", &path, "--silent"]; + for field in &fields { + args.extend(["-f", field]); + } + self.gh(&args) + } + + /// Take `label` off `issue`, in one request that keeps its other labels. + /// Through the REST API, as [`GitHub::set_labels`] is. GitHub refuses it if the issue + /// does not have the label. + pub fn remove_label(&self, issue: &IssueUrl, label: &str) -> Result<()> { + self.gh(&[ + "api", + "--method", + "DELETE", + &label_path(issue, label), + "--silent", + ]) + } + + /// The sub-issues of `issue`, its Tickets if it has any, in one query. + pub fn tickets(&self, issue: &IssueUrl) -> Result> { + let json = self.issue_query(issue, TICKETS_QUERY)?; + nodes( + &json["data"]["repository"]["issue"]["subIssues"], + "subIssues", + )? + .iter() + .map(|node| { + let mut blockers = Vec::new(); + let mut open_blockers = Vec::new(); + for blocker in nodes(&node["blockedBy"], "blockedBy")? { + blockers.push(node_number(blocker)?); + if node_is_open(blocker)? { + open_blockers.push(node_number(blocker)?); + } + } + Ok(Ticket { + number: node_number(node)?, + is_open: node_is_open(node)?, + labels: named(nodes(&node["labels"], "labels")?), + has_sub_issues: sub_issue_count(node)? > 0, + blockers, + open_blockers, + }) + }) + .collect() + } + + /// The answer to `query`, a GraphQL query about `issue` that takes its + /// `$owner`, `$repo` and `$number`. + fn issue_query(&self, issue: &IssueUrl, query: &str) -> Result { + self.gh_json(&[ + "api", + "graphql", + "-f", + &format!("query={query}"), + "-f", + &format!("owner={}", issue.owner), + "-f", + &format!("repo={}", issue.repo), + "-F", + &format!("number={}", issue.number), + ]) + } + + /// Close `issue` with `comment`. + pub fn close_issue(&self, issue: &IssueUrl, comment: &str) -> Result<()> { + self.gh(&[ + "issue", + "close", + &issue.number.to_string(), + "--repo", + &issue.repo_slug(), + "--comment", + comment, + ]) + } + + /// Every open issue labelled `label` in the repository `repo`, an + /// `owner/repo`, newest first. They come from GitHub's issue list, not its + /// search, whose index can be a while behind an issue just opened. + pub fn open_issues_labelled(&self, repo: &str, label: Label) -> Result> { + self.issues_labelled(repo, label, "open") + } + + /// Every closed issue labelled `label` in the repository `repo`, an + /// `owner/repo`, newest first. + pub fn closed_issues_labelled(&self, repo: &str, label: Label) -> Result> { + self.issues_labelled(repo, label, "closed") + } + + /// Every issue labelled `label` in the repository `repo` whose state is + /// `state`, `open` or `closed`, newest first. + fn issues_labelled(&self, repo: &str, label: Label, state: &str) -> Result> { + let json = self.gh_json(&[ + "issue", + "list", + "--state", + state, + "--repo", + repo, + "--label", + label.name(), + "--json", + "url,title,labels", + "--limit", + "1000", + ])?; + json.as_array() + .context("gh issue list did not return a list")? + .iter() + .map(|listed| { + let field = |name: &str| { + listed[name] + .as_str() + .with_context(|| format!("gh issue list output has no {name}")) + }; + Ok(ListedIssue { + issue: IssueUrl::parse(field("url")?)?, + title: field("title")?.to_string(), + labels: labels_of(listed)?, + }) + }) + .collect() + } + + /// `issue` as a Pickup run that lists issues by `label` reads it, in one + /// query. + pub fn candidate(&self, issue: &IssueUrl, label: Label) -> Result { + let json = self.issue_query(issue, CANDIDATE_QUERY)?; + let issue = &json["data"]["repository"]["issue"]; + let parent = issue["parent"]["url"] + .as_str() + .map(IssueUrl::parse) + .transpose()?; + let mut open_blockers = Vec::new(); + for blocker in nodes(&issue["blockedBy"], "blockedBy")? { + if node_is_open(blocker)? { + open_blockers.push(node_number(blocker)?); + } + } + let at = |event: &Value| -> Result> { + let at = event["createdAt"] + .as_str() + .context("gh api graphql output has an event with no createdAt")?; + Ok(DateTime::parse_from_rfc3339(at) + .with_context(|| format!("gh api graphql output has an unreadable createdAt {at}"))? + .to_utc()) + }; + let mut shaped = Vec::new(); + for event in nodes(&issue["labelled"], "labelled")? { + let name = event["label"]["name"].as_str(); + if name.is_some_and(|name| label.is_named(name)) { + shaped.push(Shaped { + by: Shaping::Labelled, + at: at(event)?, + }); + } + } + for event in nodes(&issue["linked"], "linked")? { + let by = match event["__typename"].as_str() { + Some("SubIssueAddedEvent" | "SubIssueRemovedEvent") => Shaping::SubIssues, + Some("BlockedByAddedEvent" | "BlockedByRemovedEvent") => Shaping::Blockers, + other => bail!("gh api graphql output has an unknown event {other:?}"), + }; + shaped.push(Shaped { by, at: at(event)? }); + } + Ok(Candidate { + parent, + sub_issue_is_open: nodes(&issue["subIssues"], "subIssues")? + .iter() + .map(node_is_open) + .collect::>()?, + open_blockers, + last_shaped: shaped.into_iter().max_by_key(|shaped| shaped.at), + }) + } + + /// Add each of `labels` to the repository `repo`, an `owner/repo`, with its + /// description, unless the repository has it already. + pub fn ensure_labels(&self, repo: &str, labels: &[Label]) -> Result<()> { + let known = self.gh_json(&[ + "label", "list", "--repo", repo, "--json", "name", "--limit", "1000", + ])?; + let known = named( + known + .as_array() + .context("gh label list did not return a list")?, + ); + for label in labels { + if !known.has(*label) { + self.gh(&[ + "label", + "create", + label.name(), + "--repo", + repo, + "--description", + label.description(), + ])?; + } + } + Ok(()) + } + + /// Open an issue titled `title`, with `body` and `labels`, in the repository + /// of `issue`, and return it. Each label is first added to the repository, + /// with its description, if the repository lacks it: `gh` refuses a label it + /// doesn't know. + pub fn create_issue( + &self, + issue: &IssueUrl, + title: &str, + body: &str, + labels: &[Label], + ) -> Result { + let repo = issue.repo_slug(); + self.ensure_labels(&repo, labels)?; + let names: Vec<&str> = labels.iter().map(|label| label.name()).collect(); + let url = self.gh_stdout(&[ + "issue", + "create", + "--repo", + &repo, + "--title", + title, + "--body", + body, + "--label", + &names.join(","), + ])?; + IssueUrl::parse(&url).context("gh issue create did not print the issue's URL") + } + + /// The public email of the signed-in user's GitHub profile, or `None` if it + /// is private. Private addresses need the `user` scope, which a default + /// `gh auth login` token lacks, so thirdshift never asks for them. + pub fn profile_email(&self) -> Result> { + let json = self.gh_json(&["api", "user"])?; + Ok(json["email"].as_str().map(str::to_string)) + } + + /// The `fields` of the pull request `pr`, its head branch or its number, as + /// JSON. + fn pr_view(&self, issue: &IssueUrl, pr: &str, fields: &str) -> Result { + self.gh_json(&[ + "pr", + "view", + pr, + "--repo", + &issue.repo_slug(), + "--json", + fields, + ]) + } + + /// The pull request whose head is `branch`, if `gh` finds one. + pub fn pull_request_for(&self, issue: &IssueUrl, branch: &str) -> Result> { + let json = match self.pr_view(issue, branch, PR_FIELDS) { + Ok(json) => json, + Err(error) if format!("{error:#}").contains("no pull requests found") => { + return Ok(None); + } + Err(error) => return Err(error), + }; + PullRequest::from_json(&json).map(Some) + } + + /// Every pull request in this repository, in any state, whose head branch + /// starts with `prefix`. PRs from forks are left out: their heads are not + /// this repository's branches. + pub fn pull_requests_with_head_prefix( + &self, + issue: &IssueUrl, + prefix: &str, + ) -> Result> { + let json = self.gh_json(&[ + "pr", + "list", + "--repo", + &issue.repo_slug(), + "--state", + "all", + "--search", + &format!("head:{prefix}"), + "--limit", + "1000", + "--json", + &format!("{PR_FIELDS},isCrossRepository"), + ])?; + json.as_array() + .context("gh pr list did not return a list")? + .iter() + .filter(|pr| pr["isCrossRepository"] != Value::Bool(true)) + .map(PullRequest::from_json) + .collect() + } + + /// Open a draft pull request from `head` into `base`, and return its URL. + pub fn create_draft_pr( + &self, + issue: &IssueUrl, + head: &str, + base: &str, + title: &str, + body: &str, + ) -> Result { + self.gh_stdout(&[ + "pr", + "create", + "--repo", + &issue.repo_slug(), + "--head", + head, + "--base", + base, + "--title", + title, + "--body", + body, + "--draft", + ]) + } + + /// The body of pull request `number`. + pub fn pr_body(&self, issue: &IssueUrl, number: u64) -> Result { + let json = self.pr_view(issue, &number.to_string(), "body")?; + Ok(json["body"] + .as_str() + .context("gh output has no body")? + .to_string()) + } + + /// Set the body of pull request `number` to `body`, through the REST API: + /// `gh pr edit` fails on the GitHub Projects (classic) sunset in older `gh`. + pub fn set_pr_body(&self, issue: &IssueUrl, number: u64, body: &str) -> Result<()> { + self.gh(&[ + "api", + "--method", + "PATCH", + &format!("repos/{}/pulls/{number}", issue.repo_slug()), + "-f", + &format!("body={body}"), + "--silent", + ]) + } + + /// Mark the pull request whose head is `branch` ready for review. + pub fn mark_ready(&self, issue: &IssueUrl, branch: &str) -> Result<()> { + self.gh(&["pr", "ready", branch, "--repo", &issue.repo_slug()]) + } + + /// Convert the pull request whose head is `branch` back to a draft. + pub fn convert_to_draft(&self, issue: &IssueUrl, branch: &str) -> Result<()> { + self.gh(&[ + "pr", + "ready", + branch, + "--undo", + "--repo", + &issue.repo_slug(), + ]) + } + + /// Merge the pull request whose head is `branch` into its base with a merge + /// commit, but only if its head is still `head`. Never GitHub's auto-merge + /// (ADR-0004), and never `--delete-branch`, whose local deletion would + /// interfere with the worktree. If `gh` fails but the pull request merged at + /// `head` anyway, e.g. because `gh` was interrupted once GitHub had merged, + /// that is a merge. + pub fn merge(&self, issue: &IssueUrl, branch: &str, head: &str) -> Result<()> { + let Err(error) = self.gh(&[ + "pr", + "merge", + branch, + "--repo", + &issue.repo_slug(), + "--merge", + "--match-head-commit", + head, + ]) else { + return Ok(()); + }; + match self.completion().merged_head(issue, branch) { + Ok(Some(merged)) if merged == head => Ok(()), + _ => Err(error), + } + } + + /// The head commit the pull request whose head is `branch` was merged at, if + /// it is merged. + fn merged_head(&self, issue: &IssueUrl, branch: &str) -> Result> { + let json = self.pr_view(issue, branch, "state,headRefOid")?; + if json["state"] != "MERGED" { + return Ok(None); + } + let head = json["headRefOid"] + .as_str() + .context("gh output has no headRefOid")?; + Ok(Some(head.to_string())) + } + + /// Whether the pull request whose head is `branch` can be merged. + pub fn mergeable(&self, issue: &IssueUrl, branch: &str) -> Result { + let json = self.pr_view(issue, branch, "mergeable")?; + match json["mergeable"].as_str() { + Some("MERGEABLE") => Ok(Mergeable::Yes), + Some("CONFLICTING") => Ok(Mergeable::No), + Some("UNKNOWN") => Ok(Mergeable::Unknown), + other => bail!("gh output has an unknown mergeable state {other:?}"), + } + } + + /// Every check run and commit status on commit `sha`. Of the check runs + /// sharing a name, as the attempts of a re-run job do, only the latest is + /// here. + pub fn checks_on(&self, issue: &IssueUrl, sha: &str) -> Result> { + let commit = format!("repos/{}/commits/{sha}", issue.repo_slug()); + let runs = self.gh_api_items(&format!("{commit}/check-runs?per_page=100"), "check_runs")?; + let statuses = self.gh_api_items(&format!("{commit}/status?per_page=100"), "statuses")?; + let url = |value: &Value| { + value + .as_str() + .filter(|url| !url.is_empty()) + .map(String::from) + }; + + let mut checks = Vec::new(); + for run in &runs { + let state = match (run["status"].as_str(), run["conclusion"].as_str()) { + (Some("completed"), Some("success" | "neutral" | "skipped")) => CheckState::Passed, + (Some("completed"), _) => CheckState::Failed, + _ => CheckState::Pending, + }; + checks.push(Check { + name: run["name"] + .as_str() + .context("a check run has no name")? + .to_string(), + state, + job: ActionsJob::of(run["id"].as_u64(), run["details_url"].as_str()), + url: url(&run["details_url"]).or_else(|| url(&run["html_url"])), + }); + } + for status in &statuses { + let state = match status["state"].as_str() { + Some("success") => CheckState::Passed, + Some("pending") => CheckState::Pending, + _ => CheckState::Failed, + }; + checks.push(Check { + name: status["context"] + .as_str() + .context("a commit status has no context")? + .to_string(), + state, + url: url(&status["target_url"]), + job: None, + }); + } + Ok(checks) + } + + /// Ask GitHub to re-run the failed jobs of the GitHub Actions workflow run + /// `workflow_run`, as a new attempt of it. + pub fn rerun_failed_jobs(&self, issue: &IssueUrl, workflow_run: u64) -> Result<()> { + self.gh(&[ + "run", + "rerun", + &workflow_run.to_string(), + "--failed", + "--repo", + &issue.repo_slug(), + ]) + } + + /// Run `gh `, failing with its stderr if it exits non-zero. + fn gh(&self, args: &[&str]) -> Result<()> { + self.gh_stdout(args).map(drop) + } + + /// Run `gh ` and return its trimmed stdout, failing with its stderr if + /// it exits non-zero. + fn gh_stdout(&self, args: &[&str]) -> Result { + let output = self.output(args)?; + if !output.status.success() { + bail!( + "gh {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) + } + + /// Every item of the `field` list of the GitHub API's `path`, across all its + /// pages. + fn gh_api_items(&self, path: &str, field: &str) -> Result> { + let output = self.output(&["api", "--paginate", "--jq", &format!(".{field}[]"), path])?; + if !output.status.success() { + bail!( + "gh api {path} failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + serde_json::Deserializer::from_slice(&output.stdout) + .into_iter() + .collect::>() + .with_context(|| format!("gh api {path} returned invalid JSON")) + } + + /// Run `gh ` and parse its stdout as JSON, failing with gh's stderr if + /// it exits non-zero. + fn gh_json(&self, args: &[&str]) -> Result { + let command = format!("gh {}", args[..2].join(" ")); + let output = self.output(args)?; + if !output.status.success() { + bail!( + "{command} failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + serde_json::from_slice(&output.stdout) + .with_context(|| format!("{command} returned invalid JSON")) + } + fn output(&self, args: &[&str]) -> Result { + process::output( + Command::new("gh").args(args), + None, + Control { + name: "gh", + interruption: self.interruption, + stop: &|child| process::stop(child, &[libc::SIGTERM]), + }, + ) + } } -/// The title of `issue`. -pub fn issue_title(issue: &IssueUrl) -> Result { - let json = issue_view(issue, "title")?; - Ok(json["title"] - .as_str() - .context("gh output has no title")? - .to_string()) +/// Whether the issue `json` describes, with its `state`, is open. +fn state_is_open(json: &Value) -> Result { + Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") } /// An issue as an Architect run reads the issue its Architecture review @@ -52,21 +644,6 @@ pub struct Issue { pub created: DateTime, } -/// `issue`'s state, labels and when it was created. -pub fn issue(issue: &IssueUrl) -> Result { - let json = issue_view(issue, "state,labels,createdAt")?; - let created = json["createdAt"] - .as_str() - .context("gh output has no createdAt")?; - Ok(Issue { - is_open: state_is_open(&json)?, - labels: labels_of(&json)?, - created: DateTime::parse_from_rfc3339(created) - .with_context(|| format!("gh output has an unreadable createdAt {created}"))? - .to_utc(), - }) -} - /// The labels of the issue `json` describes, with its `labels`. fn labels_of(json: &Value) -> Result { Ok(named( @@ -84,57 +661,23 @@ fn named(labels: &[Value]) -> Labels { .collect() } -/// The labels of `issue`. -pub fn issue_labels(issue: &IssueUrl) -> Result { - labels_of(&issue_view(issue, "labels")?) -} - /// The REST API's path for the labels of `issue`. fn labels_path(issue: &IssueUrl) -> String { format!("repos/{}/issues/{}/labels", issue.repo_slug(), issue.number) } -/// Set `issue`'s labels to exactly `labels`, in one request, so a swap of -/// one label for another can't stop halfway. Through the REST API: `gh issue -/// edit` fails on the GitHub Projects (classic) sunset in older `gh`. -pub fn set_labels(issue: &IssueUrl, labels: &Labels) -> Result<()> { - let path = labels_path(issue); - let fields: Vec = labels - .names() - .map(|label| format!("labels[]={label}")) - .collect(); - let mut args = vec!["api", "--method", "PUT", &path, "--silent"]; - for field in &fields { - args.extend(["-f", field]); - } - gh(&args) -} - /// The REST API's path for `label` on `issue`. fn label_path(issue: &IssueUrl, label: &str) -> String { format!("{}/{label}", labels_path(issue)) } -/// Take `label` off `issue`, in one request that keeps its other labels. -/// Through the REST API, as [`set_labels`] is. GitHub refuses it if the issue -/// does not have the label. -pub fn remove_label(issue: &IssueUrl, label: &str) -> Result<()> { - gh(&[ - "api", - "--method", - "DELETE", - &label_path(issue, label), - "--silent", - ]) -} - -/// The command that does what [`remove_label`] does, to run by hand. +/// The command that does what [`GitHub::remove_label`] does, to run by hand. pub fn remove_label_command(issue: &IssueUrl, label: Label) -> String { format!("gh api --method DELETE {}", label_path(issue, label.name())) } /// The command that adds `label` to `issue`, keeping its other labels, to -/// run by hand: unlike [`set_labels`], it needs none of the others named. +/// run by hand: unlike [`GitHub::set_labels`], it needs none of the others named. pub fn add_label_command(issue: &IssueUrl, label: Label) -> String { format!( "gh api --method POST {} -f 'labels[]={label}'", @@ -175,52 +718,6 @@ query($owner: String!, $repo: String!, $number: Int!) { } }"; -/// The sub-issues of `issue`, its Tickets if it has any, in one query. -pub fn tickets(issue: &IssueUrl) -> Result> { - let json = issue_query(issue, TICKETS_QUERY)?; - nodes( - &json["data"]["repository"]["issue"]["subIssues"], - "subIssues", - )? - .iter() - .map(|node| { - let mut blockers = Vec::new(); - let mut open_blockers = Vec::new(); - for blocker in nodes(&node["blockedBy"], "blockedBy")? { - blockers.push(node_number(blocker)?); - if node_is_open(blocker)? { - open_blockers.push(node_number(blocker)?); - } - } - Ok(Ticket { - number: node_number(node)?, - is_open: node_is_open(node)?, - labels: named(nodes(&node["labels"], "labels")?), - has_sub_issues: sub_issue_count(node)? > 0, - blockers, - open_blockers, - }) - }) - .collect() -} - -/// The answer to `query`, a GraphQL query about `issue` that takes its -/// `$owner`, `$repo` and `$number`. -fn issue_query(issue: &IssueUrl, query: &str) -> Result { - gh_json(&[ - "api", - "graphql", - "-f", - &format!("query={query}"), - "-f", - &format!("owner={}", issue.owner), - "-f", - &format!("repo={}", issue.repo), - "-F", - &format!("number={}", issue.number), - ]) -} - /// The `nodes` of `connection`, the `what` of a GraphQL answer. fn nodes<'a>(connection: &'a Value, what: &str) -> Result<&'a Vec> { connection["nodes"] @@ -250,19 +747,6 @@ fn sub_issue_count(node: &Value) -> Result { .context("gh api graphql output has an issue with no subIssues count") } -/// Close `issue` with `comment`. -pub fn close_issue(issue: &IssueUrl, comment: &str) -> Result<()> { - gh(&[ - "issue", - "close", - &issue.number.to_string(), - "--repo", - &issue.repo_slug(), - "--comment", - comment, - ]) -} - /// An issue as a listing of its repository's issues gives it. #[derive(Debug, Clone)] pub struct ListedIssue { @@ -271,54 +755,6 @@ pub struct ListedIssue { pub labels: Labels, } -/// Every open issue labelled `label` in the repository `repo`, an -/// `owner/repo`, newest first. They come from GitHub's issue list, not its -/// search, whose index can be a while behind an issue just opened. -pub fn open_issues_labelled(repo: &str, label: Label) -> Result> { - issues_labelled(repo, label, "open") -} - -/// Every closed issue labelled `label` in the repository `repo`, an -/// `owner/repo`, newest first. -pub fn closed_issues_labelled(repo: &str, label: Label) -> Result> { - issues_labelled(repo, label, "closed") -} - -/// Every issue labelled `label` in the repository `repo` whose state is -/// `state`, `open` or `closed`, newest first. -fn issues_labelled(repo: &str, label: Label, state: &str) -> Result> { - let json = gh_json(&[ - "issue", - "list", - "--state", - state, - "--repo", - repo, - "--label", - label.name(), - "--json", - "url,title,labels", - "--limit", - "1000", - ])?; - json.as_array() - .context("gh issue list did not return a list")? - .iter() - .map(|listed| { - let field = |name: &str| { - listed[name] - .as_str() - .with_context(|| format!("gh issue list output has no {name}")) - }; - Ok(ListedIssue { - issue: IssueUrl::parse(field("url")?)?, - title: field("title")?.to_string(), - labels: labels_of(listed)?, - }) - }) - .collect() -} - /// An open issue labelled for a Pickup run to take, as the Pickup run reads /// what its listing doesn't give. pub struct Candidate { @@ -396,121 +832,6 @@ query($owner: String!, $repo: String!, $number: Int!) { } }"; -/// `issue` as a Pickup run that lists issues by `label` reads it, in one -/// query. -pub fn candidate(issue: &IssueUrl, label: Label) -> Result { - let json = issue_query(issue, CANDIDATE_QUERY)?; - let issue = &json["data"]["repository"]["issue"]; - let parent = issue["parent"]["url"] - .as_str() - .map(IssueUrl::parse) - .transpose()?; - let mut open_blockers = Vec::new(); - for blocker in nodes(&issue["blockedBy"], "blockedBy")? { - if node_is_open(blocker)? { - open_blockers.push(node_number(blocker)?); - } - } - let at = |event: &Value| -> Result> { - let at = event["createdAt"] - .as_str() - .context("gh api graphql output has an event with no createdAt")?; - Ok(DateTime::parse_from_rfc3339(at) - .with_context(|| format!("gh api graphql output has an unreadable createdAt {at}"))? - .to_utc()) - }; - let mut shaped = Vec::new(); - for event in nodes(&issue["labelled"], "labelled")? { - let name = event["label"]["name"].as_str(); - if name.is_some_and(|name| label.is_named(name)) { - shaped.push(Shaped { - by: Shaping::Labelled, - at: at(event)?, - }); - } - } - for event in nodes(&issue["linked"], "linked")? { - let by = match event["__typename"].as_str() { - Some("SubIssueAddedEvent" | "SubIssueRemovedEvent") => Shaping::SubIssues, - Some("BlockedByAddedEvent" | "BlockedByRemovedEvent") => Shaping::Blockers, - other => bail!("gh api graphql output has an unknown event {other:?}"), - }; - shaped.push(Shaped { by, at: at(event)? }); - } - Ok(Candidate { - parent, - sub_issue_is_open: nodes(&issue["subIssues"], "subIssues")? - .iter() - .map(node_is_open) - .collect::>()?, - open_blockers, - last_shaped: shaped.into_iter().max_by_key(|shaped| shaped.at), - }) -} - -/// Add each of `labels` to the repository `repo`, an `owner/repo`, with its -/// description, unless the repository has it already. -pub fn ensure_labels(repo: &str, labels: &[Label]) -> Result<()> { - let known = gh_json(&[ - "label", "list", "--repo", repo, "--json", "name", "--limit", "1000", - ])?; - let known = named( - known - .as_array() - .context("gh label list did not return a list")?, - ); - for label in labels { - if !known.has(*label) { - gh(&[ - "label", - "create", - label.name(), - "--repo", - repo, - "--description", - label.description(), - ])?; - } - } - Ok(()) -} - -/// Open an issue titled `title`, with `body` and `labels`, in the repository -/// of `issue`, and return it. Each label is first added to the repository, -/// with its description, if the repository lacks it: `gh` refuses a label it -/// doesn't know. -pub fn create_issue( - issue: &IssueUrl, - title: &str, - body: &str, - labels: &[Label], -) -> Result { - let repo = issue.repo_slug(); - ensure_labels(&repo, labels)?; - let names: Vec<&str> = labels.iter().map(|label| label.name()).collect(); - let url = gh_stdout(&[ - "issue", - "create", - "--repo", - &repo, - "--title", - title, - "--body", - body, - "--label", - &names.join(","), - ])?; - IssueUrl::parse(&url).context("gh issue create did not print the issue's URL") -} - -/// The public email of the signed-in user's GitHub profile, or `None` if it -/// is private. Private addresses need the `user` scope, which a default -/// `gh auth login` token lacks, so thirdshift never asks for them. -pub fn profile_email() -> Result> { - let json = gh_json(&["api", "user"])?; - Ok(json["email"].as_str().map(str::to_string)) -} - const PR_FIELDS: &str = "number,url,state,headRefName,baseRefName,isDraft"; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -570,159 +891,6 @@ impl PullRequest { } } -/// The `fields` of the pull request `pr`, its head branch or its number, as -/// JSON. -fn pr_view(issue: &IssueUrl, pr: &str, fields: &str) -> Result { - gh_json(&[ - "pr", - "view", - pr, - "--repo", - &issue.repo_slug(), - "--json", - fields, - ]) -} - -/// The pull request whose head is `branch`, if `gh` finds one. -pub fn pull_request_for(issue: &IssueUrl, branch: &str) -> Result> { - let json = match pr_view(issue, branch, PR_FIELDS) { - Ok(json) => json, - Err(error) if format!("{error:#}").contains("no pull requests found") => return Ok(None), - Err(error) => return Err(error), - }; - PullRequest::from_json(&json).map(Some) -} - -/// Every pull request in this repository, in any state, whose head branch -/// starts with `prefix`. PRs from forks are left out: their heads are not -/// this repository's branches. -pub fn pull_requests_with_head_prefix(issue: &IssueUrl, prefix: &str) -> Result> { - let json = gh_json(&[ - "pr", - "list", - "--repo", - &issue.repo_slug(), - "--state", - "all", - "--search", - &format!("head:{prefix}"), - "--limit", - "1000", - "--json", - &format!("{PR_FIELDS},isCrossRepository"), - ])?; - json.as_array() - .context("gh pr list did not return a list")? - .iter() - .filter(|pr| pr["isCrossRepository"] != Value::Bool(true)) - .map(PullRequest::from_json) - .collect() -} - -/// Open a draft pull request from `head` into `base`, and return its URL. -pub fn create_draft_pr( - issue: &IssueUrl, - head: &str, - base: &str, - title: &str, - body: &str, -) -> Result { - gh_stdout(&[ - "pr", - "create", - "--repo", - &issue.repo_slug(), - "--head", - head, - "--base", - base, - "--title", - title, - "--body", - body, - "--draft", - ]) -} - -/// The body of pull request `number`. -pub fn pr_body(issue: &IssueUrl, number: u64) -> Result { - let json = pr_view(issue, &number.to_string(), "body")?; - Ok(json["body"] - .as_str() - .context("gh output has no body")? - .to_string()) -} - -/// Set the body of pull request `number` to `body`, through the REST API: -/// `gh pr edit` fails on the GitHub Projects (classic) sunset in older `gh`. -pub fn set_pr_body(issue: &IssueUrl, number: u64, body: &str) -> Result<()> { - gh(&[ - "api", - "--method", - "PATCH", - &format!("repos/{}/pulls/{number}", issue.repo_slug()), - "-f", - &format!("body={body}"), - "--silent", - ]) -} - -/// Mark the pull request whose head is `branch` ready for review. -pub fn mark_ready(issue: &IssueUrl, branch: &str) -> Result<()> { - gh(&["pr", "ready", branch, "--repo", &issue.repo_slug()]) -} - -/// Convert the pull request whose head is `branch` back to a draft. -pub fn convert_to_draft(issue: &IssueUrl, branch: &str) -> Result<()> { - gh(&[ - "pr", - "ready", - branch, - "--undo", - "--repo", - &issue.repo_slug(), - ]) -} - -/// Merge the pull request whose head is `branch` into its base with a merge -/// commit, but only if its head is still `head`. Never GitHub's auto-merge -/// (ADR-0004), and never `--delete-branch`, whose local deletion would -/// interfere with the worktree. If `gh` fails but the pull request merged at -/// `head` anyway, e.g. because `gh` was interrupted once GitHub had merged, -/// that is a merge. -pub fn merge(issue: &IssueUrl, branch: &str, head: &str) -> Result<()> { - let Err(error) = gh(&[ - "pr", - "merge", - branch, - "--repo", - &issue.repo_slug(), - "--merge", - "--match-head-commit", - head, - ]) else { - return Ok(()); - }; - match merged_head(issue, branch) { - Ok(Some(merged)) if merged == head => Ok(()), - _ => Err(error), - } -} - -/// The head commit the pull request whose head is `branch` was merged at, if -/// it is merged. -fn merged_head(issue: &IssueUrl, branch: &str) -> Result> { - let json = pr_view(issue, branch, "state,headRefOid")?; - if json["state"] != "MERGED" { - return Ok(None); - } - let head = json["headRefOid"] - .as_str() - .context("gh output has no headRefOid")?; - Ok(Some(head.to_string())) -} - /// Whether a pull request can be merged into its base. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Mergeable { @@ -733,17 +901,6 @@ pub enum Mergeable { Unknown, } -/// Whether the pull request whose head is `branch` can be merged. -pub fn mergeable(issue: &IssueUrl, branch: &str) -> Result { - let json = pr_view(issue, branch, "mergeable")?; - match json["mergeable"].as_str() { - Some("MERGEABLE") => Ok(Mergeable::Yes), - Some("CONFLICTING") => Ok(Mergeable::No), - Some("UNKNOWN") => Ok(Mergeable::Unknown), - other => bail!("gh output has an unknown mergeable state {other:?}"), - } -} - /// Where a check or status stands. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CheckState { @@ -785,128 +942,6 @@ impl ActionsJob { } } -/// Every check run and commit status on commit `sha`. Of the check runs -/// sharing a name, as the attempts of a re-run job do, only the latest is -/// here. -pub fn checks_on(issue: &IssueUrl, sha: &str) -> Result> { - let commit = format!("repos/{}/commits/{sha}", issue.repo_slug()); - let runs = gh_api_items(&format!("{commit}/check-runs?per_page=100"), "check_runs")?; - let statuses = gh_api_items(&format!("{commit}/status?per_page=100"), "statuses")?; - let url = |value: &Value| { - value - .as_str() - .filter(|url| !url.is_empty()) - .map(String::from) - }; - - let mut checks = Vec::new(); - for run in &runs { - let state = match (run["status"].as_str(), run["conclusion"].as_str()) { - (Some("completed"), Some("success" | "neutral" | "skipped")) => CheckState::Passed, - (Some("completed"), _) => CheckState::Failed, - _ => CheckState::Pending, - }; - checks.push(Check { - name: run["name"] - .as_str() - .context("a check run has no name")? - .to_string(), - state, - job: ActionsJob::of(run["id"].as_u64(), run["details_url"].as_str()), - url: url(&run["details_url"]).or_else(|| url(&run["html_url"])), - }); - } - for status in &statuses { - let state = match status["state"].as_str() { - Some("success") => CheckState::Passed, - Some("pending") => CheckState::Pending, - _ => CheckState::Failed, - }; - checks.push(Check { - name: status["context"] - .as_str() - .context("a commit status has no context")? - .to_string(), - state, - url: url(&status["target_url"]), - job: None, - }); - } - Ok(checks) -} - -/// Ask GitHub to re-run the failed jobs of the GitHub Actions workflow run -/// `workflow_run`, as a new attempt of it. -pub fn rerun_failed_jobs(issue: &IssueUrl, workflow_run: u64) -> Result<()> { - gh(&[ - "run", - "rerun", - &workflow_run.to_string(), - "--failed", - "--repo", - &issue.repo_slug(), - ]) -} - -/// Run `gh `, failing with its stderr if it exits non-zero. -fn gh(args: &[&str]) -> Result<()> { - gh_stdout(args).map(drop) -} - -/// Run `gh ` and return its trimmed stdout, failing with its stderr if -/// it exits non-zero. -fn gh_stdout(args: &[&str]) -> Result { - let output = Command::new("gh") - .args(args) - .output() - .context("could not run gh")?; - if !output.status.success() { - bail!( - "gh {} failed: {}", - args.join(" "), - String::from_utf8_lossy(&output.stderr).trim() - ); - } - Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) -} - -/// Every item of the `field` list of the GitHub API's `path`, across all its -/// pages. -fn gh_api_items(path: &str, field: &str) -> Result> { - let output = Command::new("gh") - .args(["api", "--paginate", "--jq", &format!(".{field}[]"), path]) - .output() - .context("could not run gh")?; - if !output.status.success() { - bail!( - "gh api {path} failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); - } - serde_json::Deserializer::from_slice(&output.stdout) - .into_iter() - .collect::>() - .with_context(|| format!("gh api {path} returned invalid JSON")) -} - -/// Run `gh ` and parse its stdout as JSON, failing with gh's stderr if -/// it exits non-zero. -fn gh_json(args: &[&str]) -> Result { - let command = format!("gh {}", args[..2].join(" ")); - let output = Command::new("gh") - .args(args) - .output() - .context("could not run gh")?; - if !output.status.success() { - bail!( - "{command} failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); - } - serde_json::from_slice(&output.stdout) - .with_context(|| format!("{command} returned invalid JSON")) -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs new file mode 100644 index 000000000..79559e4a2 --- /dev/null +++ b/src/github/execution_tests.rs @@ -0,0 +1,379 @@ +use super::*; +use crate::interrupt; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::PathBuf; +use std::time::{Duration, Instant}; + +fn isolated(name: &str, test: impl FnOnce(libc::c_int)) { + if std::env::var("THIRDSHIFT_TEST_GITHUB").as_deref() == Ok(name) { + interrupt::install().unwrap(); + test( + std::env::var("THIRDSHIFT_TEST_GITHUB_SIGNAL") + .unwrap() + .parse() + .unwrap(), + ); + return; + } + for signal in [libc::SIGINT, libc::SIGTERM, libc::SIGHUP] { + let output = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + &format!("github::execution_tests::{name}"), + "--nocapture", + ]) + .env("THIRDSHIFT_TEST_GITHUB", name) + .env("THIRDSHIFT_TEST_GITHUB_SIGNAL", signal.to_string()) + .output() + .unwrap(); + assert!(output.status.success(), "signal {signal}: {output:?}"); + } +} + +/// PATH changes happen only in the isolated process. Cleanup uses the +/// executable's recorded PID, including after a failed assertion. +struct Fixture { + temp: tempfile::TempDir, +} + +impl Fixture { + fn new(script: &str) -> Self { + let fixture = Self { + temp: tempfile::tempdir().unwrap(), + }; + fixture.script(script); + let path = format!( + "{}:{}", + fixture.temp.path().display(), + std::env::var("PATH").unwrap() + ); + // SAFETY: this isolated process runs one test; no environment readers + // or execution workers have started. + unsafe { std::env::set_var("PATH", path) }; + fixture + } + + fn path(&self, name: &str) -> PathBuf { + self.temp.path().join(name) + } + + fn script(&self, script: &str) { + let gh = self.path("gh"); + fs::write( + &gh, + format!( + "#!/bin/sh\necho $$ > '{}'\n{script}\n", + self.path("pid").display() + ), + ) + .unwrap(); + fs::set_permissions(gh, fs::Permissions::from_mode(0o755)).unwrap(); + } + + fn pid(&self) -> Option { + fs::read_to_string(self.path("pid")) + .ok()? + .trim() + .parse() + .ok() + } +} + +fn exists(pid: libc::pid_t) -> bool { + // SAFETY: signal 0 only probes the fixture's recorded PID. + unsafe { libc::kill(pid, 0) == 0 } +} + +impl Drop for Fixture { + fn drop(&mut self) { + if let Some(pid) = self.pid() + && exists(pid) + { + // SAFETY: this PID belongs to the fixture executable. + unsafe { libc::kill(pid, libc::SIGKILL) }; + } + } +} + +fn issue() -> IssueUrl { + IssueUrl::parse("https://github.com/acme/widgets/issues/7").unwrap() +} + +#[test] +fn parent_only_interruption_stops_active_github() { + isolated("parent_only_interruption_stops_active_github", |signal| { + let fixture = Fixture::new(&format!( + "kill -{signal} {}\nexec sleep 3", + std::process::id() + )); + let started = Instant::now(); + assert_eq!( + GitHub::new().issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + assert!( + started.elapsed() < Duration::from_secs(2), + "GitHub waited for natural expiry" + ); + assert!( + !exists(fixture.pid().expect("the executable never started")), + "GitHub left its executable alive" + ); + }); +} + +#[test] +fn ordinary_github_refuses_reads_and_writes_before_spawn() { + isolated( + "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() + ); + fs::remove_file(fixture.path("pid")).unwrap(); + signal_hook::low_level::raise(signal).unwrap(); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + assert_eq!( + github + .mark_ready(&issue(), "issue-7") + .unwrap_err() + .to_string(), + "interrupted" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "interrupted" + ); + assert!(github.pull_request_for(&issue(), "issue-7").is_err()); + assert!( + !fixture.path("pid").exists(), + "an ordinary operation spawned after interruption" + ); + }, + ); +} + +fn finish_issue_creation(name: &str, already_interrupted: bool) { + isolated(name, |signal| { + let github = GitHub::new(); + let completion = github.completion(); + let fixture = Fixture::new(""); + fixture.script(&format!( + r#" +case "$1 $2" in + 'label list') + kill -{signal} {parent} + sleep 0.1 + printf '[]' + ;; + 'label create') touch '{label}' ;; + 'issue create') printf ' https://github.com/acme/widgets/issues/8\n\n' ;; + *) exit 1 ;; +esac +"#, + parent = std::process::id(), + label = fixture.path("label-created").display() + )); + if already_interrupted { + signal_hook::low_level::raise(signal).unwrap(); + } + let created = completion + .create_issue( + &issue(), + "Finish", + "Work", + &[crate::labels::READY_FOR_AGENT], + ) + .unwrap(); + assert_eq!(created.url, "https://github.com/acme/widgets/issues/8"); + assert!(fixture.path("label-created").exists()); + assert!(interrupt::requested()); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + assert_eq!( + GitHub::new().issue_title(&issue()).unwrap_err().to_string(), + "interrupted" + ); + }); +} + +#[test] +fn completion_finishes_nested_label_reads_and_writes_without_permitting_ordinary_views() { + finish_issue_creation( + "completion_finishes_nested_label_reads_and_writes_without_permitting_ordinary_views", + true, + ); +} + +#[test] +fn completion_finishes_with_a_newly_arriving_interrupt() { + finish_issue_creation("completion_finishes_with_a_newly_arriving_interrupt", false); +} + +#[test] +fn an_optional_pr_probe_propagates_active_cancellation_instead_of_absence() { + isolated( + "an_optional_pr_probe_propagates_active_cancellation_instead_of_absence", + |signal| { + let fixture = Fixture::new(&format!( + "echo 'no pull requests found' >&2\nkill -{signal} {}\nexec sleep 3", + std::process::id() + )); + let error = GitHub::new() + .pull_request_for(&issue(), "issue-7") + .err() + .expect("cancellation became absence"); + assert_eq!(error.to_string(), "interrupted"); + assert!(!exists(fixture.pid().unwrap())); + }, + ); +} + +fn cancelled_merge(name: &str, state: &str, head: &str, confirmed: bool) { + isolated(name, |signal| { + let fixture = Fixture::new(&format!( + r#" +case "$1 $2" in + 'pr merge') kill -{signal} {parent}; exec sleep 3 ;; + 'pr view') printf '%s' '{{"state":"{state}","headRefOid":"{head}"}}' ;; + *) exit 1 ;; +esac +"#, + parent = std::process::id() + )); + let github = GitHub::new(); + let result = github.merge(&issue(), "issue-7", "requested-head"); + if confirmed { + result.unwrap(); + } else { + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + } + assert!(interrupt::requested()); + assert_eq!( + github + .mark_ready(&issue(), "issue-7") + .unwrap_err() + .to_string(), + "interrupted" + ); + assert!(!exists(fixture.pid().unwrap())); + }); +} + +#[test] +fn a_cancelled_merge_is_confirmed_only_at_the_requested_head() { + cancelled_merge( + "a_cancelled_merge_is_confirmed_only_at_the_requested_head", + "MERGED", + "requested-head", + true, + ); +} + +#[test] +fn a_cancelled_merge_at_another_head_remains_a_failure() { + cancelled_merge( + "a_cancelled_merge_at_another_head_remains_a_failure", + "MERGED", + "foreign-head", + false, + ); +} + +#[test] +fn an_unconfirmed_cancelled_merge_remains_a_failure() { + cancelled_merge( + "an_unconfirmed_cancelled_merge_remains_a_failure", + "OPEN", + "requested-head", + false, + ); +} + +#[test] +fn github_preserves_trimmed_text_json_and_paginated_items() { + isolated( + "github_preserves_trimmed_text_json_and_paginated_items", + |_| { + let _fixture = Fixture::new( + r#" +case "$1 $2" in + 'issue view') printf ' {"title":"Widgets"}\n' ;; + 'pr create') printf ' https://github.com/acme/widgets/pull/1\n\n' ;; + 'api --paginate') + if test "$4" = '.check_runs[]'; then + printf '%s\n' '{"name":"build","status":"completed","conclusion":"success"}' '{"name":"lint","status":"completed","conclusion":"failure"}' + else + printf '%s\n' '{"context":"external","state":"pending"}' + fi ;; + *) exit 1 ;; +esac +"#, + ); + let github = GitHub::new(); + assert_eq!(github.issue_title(&issue()).unwrap(), "Widgets"); + assert_eq!( + github + .create_draft_pr(&issue(), "issue-7", "main", "Widgets", "Work") + .unwrap(), + "https://github.com/acme/widgets/pull/1" + ); + let checks = github.checks_on(&issue(), "abc").unwrap(); + let answers: Vec<_> = checks + .iter() + .map(|check| (check.name.as_str(), check.state)) + .collect(); + assert_eq!( + answers, + [ + ("build", CheckState::Passed), + ("lint", CheckState::Failed), + ("external", CheckState::Pending) + ] + ); + }, + ); +} + +#[test] +fn github_keeps_its_exit_and_parse_diagnostics() { + isolated("github_keeps_its_exit_and_parse_diagnostics", |_| { + let fixture = Fixture::new("printf ' refused \\n' >&2; exit 1"); + let github = GitHub::new(); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "gh issue view failed: refused" + ); + assert_eq!( + github + .mark_ready(&issue(), "issue-7") + .unwrap_err() + .to_string(), + "gh pr ready issue-7 --repo acme/widgets failed: refused" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 failed: refused" + ); + fixture.script("printf 'invalid'"); + assert_eq!( + github.issue_title(&issue()).unwrap_err().to_string(), + "gh issue view returned invalid JSON" + ); + assert_eq!( + github.checks_on(&issue(), "abc").err().unwrap().to_string(), + "gh api repos/acme/widgets/commits/abc/check-runs?per_page=100 returned invalid JSON" + ); + }); +} diff --git a/src/labels.rs b/src/labels.rs index effbaa166..6aaf91c1c 100644 --- a/src/labels.rs +++ b/src/labels.rs @@ -8,7 +8,7 @@ use std::fmt; use anyhow::Result; -use crate::github; +use crate::github::{self, GitHub}; use crate::issue::IssueUrl; /// A label thirdshift knows. It prints as its name. @@ -202,15 +202,15 @@ impl<'a> Edit<'a> { /// puts on to the repository, with its description, if the repository /// lacks it. A change of no label makes no request, and taking a single /// label off, putting none on, keeps the labels added since the read. - pub fn apply(&self) -> Result<()> { + pub fn apply(&self, github: &GitHub) -> Result<()> { match request(&self.labels, &self.off, &self.on) { Request::None => Ok(()), - Request::Delete(label) => github::remove_label(self.issue, &label), + Request::Delete(label) => github.remove_label(self.issue, &label), Request::Put { missing, labels } => { if !missing.is_empty() { - github::ensure_labels(&self.issue.repo_slug(), &missing)?; + github.ensure_labels(&self.issue.repo_slug(), &missing)?; } - github::set_labels(self.issue, &labels) + github.set_labels(self.issue, &labels) } } } diff --git a/src/launch.rs b/src/launch.rs index 9fe1b74fb..3f1276b70 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -19,7 +19,7 @@ use anyhow::{Context, Result, bail}; use crate::config; use crate::git::Git; -use crate::github; +use crate::github::GitHub; use crate::issue::{IssueUrl, Repo}; use crate::progress; @@ -61,7 +61,7 @@ impl LaunchDirectory { directory.origin ); } - if !github::issue_is_open(issue)? { + if !GitHub::new().issue_is_open(issue)? { bail!("issue #{} is closed", issue.number); } directory.check_identity()?; diff --git a/src/notification.rs b/src/notification.rs index 6a612d702..2971e4590 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -9,7 +9,7 @@ use anyhow::Result; use crate::config::EmailSettings; use crate::email::Resend; -use crate::github; +use crate::github::GitHub; use crate::harness::Choice; use crate::host; use crate::issue::{IssueUrl, Repo}; @@ -96,7 +96,7 @@ impl RunNotification { issue: issue.clone(), // Left out of the subject if GitHub can't be asked; the Run's // own preflight reports why. - title: github::issue_title(issue).ok(), + title: GitHub::new().issue_title(issue).ok(), }, // Left out of the subject if origin names none; the Architect // run's own preflight reports why. diff --git a/src/pass.rs b/src/pass.rs index aa7db3ea3..0256f9889 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -18,7 +18,7 @@ use anyhow::Result; use crate::asks::{Asks, Flags}; use crate::config::UserConfig; -use crate::github::{self, Issue, ListedIssue}; +use crate::github::{GitHub, Issue, ListedIssue}; use crate::harness::Choice; use crate::interrupt; use crate::issue::{IssueUrl, Repo}; @@ -114,19 +114,19 @@ pub struct LaunchAndGitHub<'a> { impl Outside for LaunchAndGitHub<'_> { fn open_issues(&mut self, label: Label) -> Result> { - github::open_issues_labelled(&self.repo.slug(), label) + GitHub::new().open_issues_labelled(&self.repo.slug(), label) } fn closed_issues(&mut self, label: Label) -> Result> { - github::closed_issues_labelled(&self.repo.slug(), label) + GitHub::new().closed_issues_labelled(&self.repo.slug(), label) } fn issue(&mut self, issue: &IssueUrl) -> Result { - github::issue(issue) + GitHub::new().issue(issue) } fn apply(&mut self, edit: &Edit) -> Result<()> { - edit.apply() + edit.apply(&GitHub::new()) } fn interrupted(&mut self) -> bool { diff --git a/src/ready.rs b/src/ready.rs index 812606bc6..fbfe51647 100644 --- a/src/ready.rs +++ b/src/ready.rs @@ -10,7 +10,7 @@ use crate::base_fix; use crate::branch::{self, Started}; use crate::claim; use crate::git::Git; -use crate::github::{self, Candidate, ListedIssue, Shaping}; +use crate::github::{Candidate, GitHub, ListedIssue, Shaping}; use crate::issue::{IssueUrl, Repo}; use crate::labels::{Label, READY_FOR_AGENT}; use crate::progress; @@ -29,8 +29,13 @@ pub struct ReadyIssue { /// issue labelled `ready-for-agent` passed over before it, with why. /// Nothing is changed, on GitHub or in `launch`. pub fn first(launch: &Git, repo: &Repo) -> Result> { - let candidates = github::open_issues_labelled(&repo.slug(), READY_FOR_AGENT)?; - let reads = GitHubAndOrigin { launch }; + let reads = GitHubAndOrigin { + launch, + github: GitHub::new(), + }; + let candidates = reads + .github + .open_issues_labelled(&repo.slug(), READY_FOR_AGENT)?; Search::new(&reads, candidates, Utc::now(), progress::step).first_ready() } @@ -47,15 +52,16 @@ trait Reads { /// The reads of GitHub and of the Launch directory `launch`'s origin. struct GitHubAndOrigin<'a> { launch: &'a Git, + github: GitHub, } impl Reads for GitHubAndOrigin<'_> { fn candidate(&self, issue: &IssueUrl) -> Result { - github::candidate(issue, READY_FOR_AGENT) + self.github.candidate(issue, READY_FOR_AGENT) } fn started(&self, issue: &IssueUrl) -> Result> { - branch::started(self.launch, issue) + branch::started(self.launch, &self.github, issue) } } diff --git a/src/run.rs b/src/run.rs index fab341e20..140d6416a 100644 --- a/src/run.rs +++ b/src/run.rs @@ -13,7 +13,7 @@ use crate::child_run::Kind; use crate::claim::{self, Claim}; use crate::delivery::{Delivery, Opening}; use crate::failed_run::FailedRun; -use crate::github::{self, Ticket}; +use crate::github::{GitHub, Ticket}; use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; @@ -385,7 +385,7 @@ impl<'a> Outside for LaunchAndGitHub<'a> { type Worktree = Worktree; fn tickets(&mut self) -> Result> { - github::tickets(self.issue) + GitHub::new().tickets(self.issue) } fn started(&mut self, work: Work) { @@ -393,7 +393,7 @@ impl<'a> Outside for LaunchAndGitHub<'a> { } fn select(&mut self) -> Result { - branch::select(self.directory.git(), self.issue) + branch::select(self.directory.git(), &GitHub::new(), self.issue) } fn base_branch(&mut self, named: Option<&str>) -> Result { diff --git a/src/setup.rs b/src/setup.rs index cf7f39e0d..10b420490 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -19,12 +19,13 @@ use anyhow::{Context, Result}; use crate::config::{self, EmailSettings, UserConfig, UserConfigDocument}; use crate::git::Git; +use crate::github::GitHub; use crate::harness::{ Harness, ModelAndEffort, Settings, settings::{self, Terminal}, }; use crate::resend_key::{Credentials, Source}; -use crate::{email, github, interrupt, progress}; +use crate::{email, interrupt, progress}; use questions::Answers; @@ -326,7 +327,7 @@ impl Outside for OnMachine<'_> { } fn github_email(&mut self) -> Result> { - github::profile_email() + GitHub::new().profile_email() } fn git_email(&mut self) -> Result { diff --git a/src/spec_run.rs b/src/spec_run.rs index cee2283f4..5b3abb77f 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -23,7 +23,7 @@ use crate::base_fix::BaseFixAsk; use crate::child_run::{self, Ended, Handle, Kind}; use crate::delivery::{Delivery, Opening}; use crate::failed_run::{FailedRun, interrupted_or}; -use crate::github::{self, Ticket}; +use crate::github::{GitHub, Ticket}; use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; @@ -133,6 +133,13 @@ fn review_and_deliver( checklist: &str, spec_pr_url: &str, ) -> Result { + if outside.interrupted() { + return Err(FailedRun { + interrupted: true, + pr_url: Some(spec_pr_url.to_string()), + ..FailedRun::from(anyhow::anyhow!("interrupted")) + }); + } let opening = Opening { kind: SPEC_REVIEW, prompt: prompt::spec_review(spec, base, branch, spec_pr_url), @@ -270,7 +277,7 @@ impl Outside for ChildRunsAndGitHub<'_> { } fn tickets(&mut self) -> Result> { - github::tickets(self.spec) + GitHub::new().completion().tickets(self.spec) } fn show(&mut self, checklist: &str) { diff --git a/src/spec_run/spec_pr.rs b/src/spec_run/spec_pr.rs index 541ee5987..10a9abcea 100644 --- a/src/spec_run/spec_pr.rs +++ b/src/spec_run/spec_pr.rs @@ -10,7 +10,7 @@ use anyhow::{Context, Result}; -use crate::github::{self, PullRequest}; +use crate::github::{GitHub, PullRequest}; use crate::issue::IssueUrl; use crate::progress; @@ -39,8 +39,13 @@ impl<'a> SpecPr<'a> { spec, branch: branch.to_string(), base, + github: GitHub::new(), }; - Self::resume_through(on_github, spec, branch, base) + 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) } } @@ -168,6 +173,7 @@ pub(super) trait Outside { /// 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, @@ -175,27 +181,29 @@ pub(super) struct OnGitHub<'a> { impl Outside for OnGitHub<'_> { fn pull_request(&mut self) -> Result> { - github::pull_request_for(self.spec, &self.branch) + self.github.pull_request_for(self.spec, &self.branch) } fn convert_to_draft(&mut self) -> Result<()> { - github::convert_to_draft(self.spec, &self.branch) + self.github.convert_to_draft(self.spec, &self.branch) } fn spec_title(&mut self) -> Result { - github::issue_title(self.spec) + self.github.issue_title(self.spec) } fn create_draft(&mut self, title: &str, body: &str) -> Result<()> { - github::create_draft_pr(self.spec, &self.branch, self.base, title, body).map(|_| ()) + self.github + .create_draft_pr(self.spec, &self.branch, self.base, title, body) + .map(|_| ()) } fn pr_body(&mut self, number: u64) -> Result { - github::pr_body(self.spec, number) + self.github.pr_body(self.spec, number) } fn set_pr_body(&mut self, number: u64, body: &str) -> Result<()> { - github::set_pr_body(self.spec, number, body) + self.github.set_pr_body(self.spec, number, body) } fn step(&mut self, line: String) { diff --git a/tests/spec_run.rs b/tests/spec_run.rs index d76d2deb2..83eff4f3e 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1574,6 +1574,103 @@ fn a_signal_to_the_spec_run_alone_fails_the_ticket_run_then_ends_the_spec_run_as ); } +#[test] +fn the_first_landing_opens_its_spec_pr_when_accounting_is_interrupted() { + let scenario = linear_spec(); + scenario.agent_does_for( + 21, + &format!( + "{}gh fake on-issue-view 20 'touch {}; sleep 1'\n", + agent_lands(21, "first.txt"), + scenario.path("reading-spec-title").display() + ), + ); + + // The Ticket has landed; interruption arrives while accounting reads + // the title, before opening the Spec PR. No subsequent work may start. + let result = scenario.run_and_signal(&[&spec_url(&scenario)], "reading-spec-title", "TERM"); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(sessions_by_issue(&scenario), ["21"]); + let pr = spec_pr(&scenario); + let pr_url = pr["url"].as_str().unwrap(); + assert_eq!(result.stdout, format!("{pr_url}\n")); + assert_eq!(pr["isDraft"], true); + let checklist = checklist_in(pr["body"].as_str().unwrap()); + assert_contains( + checklist, + "- [x] #21 landed with https://github.com/acme/widgets/pull/1\n", + ); + assert_contains(checklist, "- [ ] #22 not started\n"); + assert_contains(&result.stderr, "thirdshift: #21 landed\n"); + assert_eq!( + support::before_command_log(&result.stderr).last(), + Some(&"thirdshift: interrupted") + ); + scenario.assert_cleaned_up("issue-20"); +} + +#[test] +fn an_existing_spec_pr_accounts_for_every_drained_ticket_after_interruption() { + let scenario = diamond_spec(); + scenario.origin_has_branch("issue-20", "main", &[]); + let pr_url = scenario.github_has_pr("issue-20", "main", "OPEN"); + for (ticket, other) in [(21, 22), (22, 21)] { + scenario.agent_does_for( + ticket, + &format!( + "echo 'half done {ticket}' > wip.txt\n{}touch {root}/both-started\nsleep 30\n", + waits_for_other_session(&scenario, ticket, other), + root = scenario.path("").display() + ), + ); + } + + let result = scenario.run_and_signal( + &[&spec_url(&scenario), "parallel", "2"], + "both-started", + "TERM", + ); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.stdout, format!("{pr_url}\n")); + let mut sessions = sessions_by_issue(&scenario); + sessions.sort(); + assert_eq!( + sessions, + ["21", "22"], + "no new Ticket or Spec review may start" + ); + let pr = spec_pr(&scenario); + assert_eq!(pr["url"], pr_url); + assert_eq!(pr["isDraft"], true); + assert_eq!( + checklist_in(pr["body"].as_str().unwrap()), + "\n## Tickets\n\n- [ ] #21 interrupted\n- [ ] #22 interrupted\n- [ ] #23 blocked by #21, #22\n" + ); + for ticket in [21, 22] { + assert_contains( + &result.stderr, + &format!("thirdshift: #{ticket} interrupted\n"), + ); + assert_eq!( + scenario.origin_log(&format!("issue-{ticket}")).unwrap()[0], + "thirdshift: failed run (interrupted)" + ); + scenario.assert_cleaned_up(&format!("issue-{ticket}")); + } + assert_eq!( + support::before_command_log(&result.stderr).last(), + Some(&"thirdshift: interrupted") + ); + assert!( + !result.stderr.contains("could not update the Spec PR"), + "{}", + result.stderr + ); + scenario.assert_cleaned_up("issue-20"); +} + #[test] fn parent_only_interruption_reaches_all_active_tickets_before_waiting_and_saves_their_work() { let scenario = diamond_spec();