From 5ae98da0bd574dd5a6184d1d61facdca1a97d05c Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 01:55:12 +0000 Subject: [PATCH 1/2] Start a Base fix for Inherited failures when given base-fix Co-Authored-By: Claude Opus 5.5 --- README.md | 22 ++- src/args.rs | 26 +++ src/base_fix.rs | 163 +++++++++++++++++++ src/child_run.rs | 110 +++++++++++++ src/ci.rs | 12 +- src/failed_run.rs | 4 + src/github.rs | 50 ++++++ src/main.rs | 33 +++- src/notification.rs | 33 +++- src/run.rs | 80 ++++++--- src/spec_run.rs | 123 ++++---------- tests/base_fix.rs | 342 +++++++++++++++++++++++++++++++++++++++ tests/command_surface.rs | 2 + tests/fakes/gh.rs | 102 ++++++++++++ tests/spec_run.rs | 39 +++++ 15 files changed, 1020 insertions(+), 121 deletions(-) create mode 100644 src/base_fix.rs create mode 100644 src/child_run.rs create mode 100644 tests/base_fix.rs diff --git a/README.md b/README.md index 4c0d787e5..1114ec179 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ From a clone of the issue's repository, `thirdshift `: 3. Creates a git worktree next to your clone, named `-`, so your own checkout is never touched. 4. Runs a headless Claude Code session in that worktree with the **Factory skills** in [`skills/`](skills/) loaded, started with one of the **Session prompts** in [`prompts/`](prompts/). The agent implements the issue, reviews its work against the Base branch, addresses the **Standards findings** and **Spec findings** it agrees with, and opens a ready-for-review pull request that lists every **Unaddressed finding** with a reason and closes the issue. 5. Takes over deterministically: pushes the Issue branch, skipping your repo's git hooks since the session runs the tests and CI gates the pull request, checks through `gh` that the pull request exists, is open and targets the Base branch, and marks it ready for review (`gh pr ready`) if the agent left it as a draft. -6. Keeps the pull request mergeable and green: merges the Base branch in and watches CI, starting a **Repair** session for a merge conflict or failing checks, at most 5 per Run. A failed check that also failed, under the same name, on the Base branch commit the head last merged in is an **Inherited failure**, not the branch's to fix: the CI-fix Repair is given only the branch's own failures, with the Inherited failures listed as not to fix. If every failed check is an Inherited failure, no Repair starts: the Run merges the Base branch again if it has moved since, and otherwise ends as a [Failed run](#failed-runs) that says to fix the Base branch first. A check that is pending, passed or missing on that Base branch commit is the branch's own, and thirdshift never triggers or waits for the Base branch's CI. If a CI-fix Repair leaves the head unchanged, with no commit of its own and no Base branch move to merge, the Run ends as a Failed run, since CI would stay red on the same head. If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. In a Merge run, each round also takes in **Foreign commits** first: see [Foreign commits in a Merge run](#foreign-commits-in-a-merge-run). +6. Keeps the pull request mergeable and green: merges the Base branch in and watches CI, starting a **Repair** session for a merge conflict or failing checks, at most 5 per Run. A failed check that also failed, under the same name, on the Base branch commit the head last merged in is an **Inherited failure**, not the branch's to fix: the CI-fix Repair is given only the branch's own failures, with the Inherited failures listed as not to fix. If every failed check is an Inherited failure, no Repair starts: the Run merges the Base branch again if it has moved since, and otherwise ends as a [Failed run](#failed-runs) that says to fix the Base branch first, unless it was given `base-fix`, when it starts a [Base fix](#base-fix) first. A check that is pending, passed or missing on that Base branch commit is the branch's own, and thirdshift never triggers or waits for the Base branch's CI. If a CI-fix Repair leaves the head unchanged, with no commit of its own and no Base branch move to merge, the Run ends as a Failed run, since CI would stay red on the same head. If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. In a Merge run, each round also takes in **Foreign commits** first: see [Foreign commits in a Merge run](#foreign-commits-in-a-merge-run). 7. In a **Merge run** (`thirdshift merge `), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit `). It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails goes back round step 6, within the same budgets, and is tried again on the new green head. If that round finds nothing to fix, the refusal is a **policy refusal**, such as merge commits being disallowed or a review being required. After the merge, thirdshift deletes the Issue branch on `origin`, and closes the issue if it is still open, with the comment `Closed by #, merged into by a thirdshift Merge run.` GitHub closes it on its own only for a merge into the repository's default branch, and then thirdshift leaves it alone. 8. Cleans up: removes the worktree, the local Issue branch and the temporary plugin directory, whether the Run succeeded or not. The one exception is a **Failed run** whose work could not be pushed: see below. @@ -280,6 +280,26 @@ Review Repairs are logged as `repair-`, numbered with the other Repairs, coun A Run without `merge` does none of this: it never fetches someone else's commits into its branch. +## Base fix + +A Base branch whose CI is broken fails every Run on it with an Inherited failure. To have a Run fix the Base branch itself and carry on, give it `base-fix` (or `--base-fix`), before or after the URL: + +```sh +thirdshift base-fix https://github.com/acme/widgets/issues/7 +``` + +When the Run's only red checks are Inherited failures and the Base branch has not moved since, it then starts a **Base fix**, at most one per Run ([ADR-0008](docs/adr/0008-inherited-failures-fail-the-run.md)): + +1. thirdshift opens an issue from a fixed template, with no agent session: titled `CI red on : [, …]`, its body naming the checks with their URLs, the Base branch and its short sha, and the Run's pull request, labelled `base-fix` and `ready-for-agent`. A label the repository lacks is added to it first. +2. It starts a child `thirdshift` on that issue from the same Launch directory, as a Merge run into the Run's Base branch, whatever the Run's own goal. Its progress lines are relayed with a `#: ` prefix, after `starting Base fix # into : ` and `waiting on Base fix #`. The Base fix sends no Run notification, treats every red check as its own to fix rather than as an Inherited failure, and never starts a Base fix of its own. +3. Once the Base fix has merged, and its Self-merge has closed its issue, the Run merges the Base branch in again and watches CI, and goes on as usual: ready for review, or merged in a Merge run. + +If the Base fix fails, the Run is a [Failed run](#failed-runs) with the cause `Base fix failed: `. If the checks are still Inherited failures once the Base fix has merged, there is no second one, and the cause is `CI red on , which also fails on at , even after Base fix merged; fix first`. The Run's own [Run notification](#run-notifications) has a `Base fix:` line with the issue's URL and whether it merged. + +On a Spec, `base-fix` is for the Spec PR, whose Base fix goes into the Base branch. A Ticket's Run starts no Base fix: an Inherited failure from the Spec branch fails it. + +Without `base-fix`, an Inherited failure fails the Run as described in [What a Run does](#what-a-run-does). + ## Continuation Running thirdshift again on an issue picks up where the last Run stopped ([ADR-0002](docs/adr/0002-existing-issue-branch-means-continue.md)). It looks at the highest-numbered Issue branch: diff --git a/src/args.rs b/src/args.rs index 86fd13353..d21f9980b 100644 --- a/src/args.rs +++ b/src/args.rs @@ -26,6 +26,12 @@ pub enum Command { /// help. pub const SPEC_BRANCH: &str = "--spec-branch"; +/// The hidden argument a Run starts its Base fix with, followed by the Run's +/// Base branch: it makes the Base fix a Merge run into that branch that sends +/// no Run notification, leaves the Launch directory alone, sees no Inherited +/// failures and starts no Base fix of its own. Not in help. +pub const BASE_FIX_INTO: &str = "--base-fix-into"; + /// A Run's arguments. pub struct RunArgs { pub issue: IssueUrl, @@ -38,8 +44,12 @@ pub struct RunArgs { /// How many Tickets a Spec run runs at once, if `parallel ` was /// given; without it, the User config decides. pub parallel: Option, + /// Whether `base-fix` was given: the Run may start a Base fix. + pub base_fix: bool, /// The Spec branch, given with [`SPEC_BRANCH`] to a Ticket's Run. pub spec_branch: Option, + /// The Base branch, given with [`BASE_FIX_INTO`] to a Base fix. + pub base_fix_into: Option, } /// Parse the arguments after the program name. `help`, `version`, `update`, @@ -73,7 +83,9 @@ pub fn parse(args: &[String]) -> Result { let mut goal = None; let mut email = None; let mut parallel = None; + let mut base_fix = false; let mut spec_branch = None; + let mut base_fix_into = None; let mut args = args.iter().peekable(); while let Some(arg) = args.next() { match arg.as_str() { @@ -99,12 +111,24 @@ pub fn parse(args: &[String]) -> Result { }; parallel = Some(n); } + "base-fix" | "--base-fix" => { + if base_fix { + bail!("repeated argument: {arg}"); + } + base_fix = true; + } SPEC_BRANCH => { if spec_branch.is_some() { bail!("repeated argument: {arg}"); } spec_branch = Some(args.next().context("missing Spec branch")?.clone()); } + BASE_FIX_INTO => { + if base_fix_into.is_some() { + bail!("repeated argument: {arg}"); + } + base_fix_into = Some(args.next().context("missing Base branch")?.clone()); + } _ => { if issue.is_some() { bail!("unexpected argument after the Issue URL: {arg}"); @@ -121,7 +145,9 @@ pub fn parse(args: &[String]) -> Result { goal, email, parallel, + base_fix, spec_branch, + base_fix_into, })) } diff --git a/src/base_fix.rs b/src/base_fix.rs new file mode 100644 index 000000000..014c5a744 --- /dev/null +++ b/src/base_fix.rs @@ -0,0 +1,163 @@ +//! The Base fix (ADR-0008): a Merge run into a Run's Base branch, on an issue +//! thirdshift writes, that a Run asked to starts when its only red checks are +//! Inherited failures, and waits for. + +use anyhow::{Result, bail}; + +use crate::args; +use crate::child_run::{self, Ended}; +use crate::ci; +use crate::github::{self, Check}; +use crate::issue::IssueUrl; +use crate::progress; + +/// The labels of a Base fix issue, each with the description it is added to +/// the repository with if the repository lacks it. +const LABELS: [(&str, &str); 2] = [ + ("base-fix", "A Base fix: CI is red on a Base branch"), + ("ready-for-agent", "Ready for an agent to take on"), +]; + +/// What a Run does when its only red checks are Inherited failures. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Policy { + /// Fail, saying to fix the Base branch first. + FailTheRun, + /// Start a Base fix, once, and go on when it has merged. + StartBaseFix, + /// The Run is itself a Base fix: every red check is its own to fix, so it + /// sees no Inherited failures and starts no Base fix. + IsBaseFix, +} + +/// A Run's one Base fix: whether it may start one, and the one it started. +pub struct BaseFix { + policy: Policy, + started: Option, +} + +/// A Base fix a Run started. +struct Started { + issue: IssueUrl, + merged: bool, +} + +impl BaseFix { + pub fn new(policy: Policy) -> Self { + BaseFix { + policy, + started: None, + } + } + + /// Whether the Run compares its red checks with the Base branch's, so + /// that some may be Inherited failures. + pub fn sees_inherited_failures(&self) -> bool { + self.policy != Policy::IsBaseFix + } + + /// Fix the checks `inherited`, the only red ones on the PR `pr_url` of + /// the Run on `run` and all Inherited failures from `base` at + /// `base_commit`, with a Base fix: write its issue, start it as a child + /// `thirdshift` and wait for it to merge, after which the Run is to merge + /// `base` in and watch CI again. Fails, with the Run's cause, if the Run + /// was not asked to start one, if the Base fix fails, in which case the + /// cause names its issue, or if the Run has had its one Base fix, in + /// which case the cause names that one's issue. + pub fn fix( + &mut self, + run: &IssueUrl, + pr_url: &str, + base: &str, + base_commit: &str, + inherited: &[Check], + ) -> Result<()> { + let checks = ci::check_names(inherited); + let base_at = ci::short(base_commit); + if let Some(started) = &self.started { + bail!( + "CI red on {checks}, which also fails on {base} at {base_at}, \ + even after Base fix {} merged; fix {base} first", + started.issue.url + ); + } + if self.policy != Policy::StartBaseFix { + bail!("CI red on {checks}, which also fails on {base} at {base_at}; fix {base} first"); + } + let issue = github::create_issue( + run, + &format!("CI red on {base}: {checks}"), + &issue_body(run, pr_url, base, base_at, inherited), + &LABELS, + )?; + let number = issue.number; + progress::step(format_args!( + "starting Base fix #{number} into {base}: {}", + issue.url + )); + let started = self.started.insert(Started { + issue, + merged: false, + }); + let child = child_run::start(&started.issue, [args::BASE_FIX_INTO, base])?; + progress::step(format_args!("waiting on Base fix #{number}")); + match child_run::wait(number, child)? { + Ended::Reached(_) => { + started.merged = true; + progress::step(format_args!( + "Base fix #{number} merged; merging {base} in again" + )); + Ok(()) + } + Ended::Interrupted => bail!("interrupted"), + Ended::Failed { cause, log } => { + if let Some(log) = log { + progress::step(format_args!("Base fix #{number} session log: {log}")); + } + bail!("Base fix {} failed: {cause}", started.issue.url) + } + } + } + + /// What became of the Base fix the Run started, if it started one, for + /// its Run notification: the issue's URL, then `merged` or `not merged`. + pub fn report(&self) -> Option { + let started = self.started.as_ref()?; + let outcome = if started.merged { + "merged" + } else { + "not merged" + }; + Some(format!("{} {outcome}", started.issue.url)) + } +} + +/// The body of the Base fix issue, from a fixed template: the checks +/// `inherited` with their URLs, the Base branch `base` and its commit +/// `base_at`, shortened, and the Run that found them, on `run` with the PR +/// `pr_url`. +fn issue_body( + run: &IssueUrl, + pr_url: &str, + base: &str, + base_at: &str, + inherited: &[Check], +) -> String { + let mut checks = String::new(); + for check in inherited { + checks += &match &check.url { + Some(url) => format!("- {}: {url}\n", check.name), + None => format!("- {}\n", check.name), + }; + } + format!( + "CI is red on `{base}` at {base_at}: these checks fail there, so every pull request into `{base}` inherits them.\n\ + \n\ + {checks}\ + \n\ + Found by the thirdshift Run on #{}, whose pull request is {pr_url}.\n\ + \n\ + Fix the checks on `{base}`. Do not skip, disable, or weaken tests or checks to make them pass.\n", + run.number + ) +} diff --git a/src/child_run.rs b/src/child_run.rs new file mode 100644 index 000000000..a984004ef --- /dev/null +++ b/src/child_run.rs @@ -0,0 +1,110 @@ +//! A Run in a child `thirdshift`, started from the same Launch directory by a +//! Spec run for one of its Tickets (ADR-0006) or by a Run for its Base fix +//! (ADR-0008). + +use std::io::{BufRead, BufReader, Read}; +use std::process::{Child, Command, Stdio}; +use std::thread; +use std::time::Duration; + +use anyhow::{Context, Result, anyhow}; + +use crate::failed_run; +use crate::interrupt; +use crate::issue::IssueUrl; +use crate::progress; + +/// How often to check whether a child Run has ended, or been interrupted. +const POLL: Duration = Duration::from_millis(100); + +/// How a child Run ended. +pub enum Ended { + /// It reached its goal, with its PR as it printed it. + Reached(Option), + /// Why, and its session log, as it reported them. + Failed { cause: String, log: Option }, + /// Ended by an interrupt passed on to it. + Interrupted, +} + +/// Start a Run on `issue` in a child `thirdshift`, from the same Launch +/// directory, given `hidden`: the argument that says what started it, and the +/// Base branch that goes with it. +pub fn start(issue: &IssueUrl, hidden: [&str; 2]) -> Result { + Command::new(std::env::current_exe().context("no thirdshift executable")?) + .args(hidden) + .arg(&issue.url) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .with_context(|| format!("could not start the Run for #{}", issue.number)) +} + +/// Relay the stderr of `child`, the Run for issue `number`, with a +/// `#: ` prefix until it exits. An interrupt is passed on to the +/// child, which is waited for as it goes down its Failed run path, and it +/// ended `Interrupted`. Otherwise it reached its goal if it exits 0, and +/// failed otherwise, with the cause and session log it ended on. +pub fn wait(number: u64, mut child: Child) -> Result { + // Relay on its own thread, so this one can watch for an interrupt. + let stderr = child.stderr.take().context("no stderr from the Run")?; + // A failed Run ends on its error, then its session log if it has one. + let relay = thread::spawn(move || -> std::io::Result<[Option; 2]> { + let mut last_lines: [Option; 2] = [None, None]; + for line in BufReader::new(stderr).lines() { + let line = line?; + let message = progress::relay(format_args!("#{number}"), &line).to_string(); + last_lines = [last_lines[1].take(), Some(message)]; + } + Ok(last_lines) + }); + let mut passed_on = false; + let status = loop { + // The child shares the process group, so a Ctrl-C or a closed + // terminal reaches it too, but a signal sent to this process alone + // doesn't. A second one is harmless: it only records the interrupt. + if !passed_on && interrupt::requested() { + // SAFETY: kill has no memory-safety preconditions, and the child + // is not yet reaped, so its pid is still its own. + unsafe { libc::kill(child.id() as libc::pid_t, libc::SIGTERM) }; + passed_on = true; + } + if let Some(status) = child + .try_wait() + .with_context(|| format!("could not wait for the Run for #{number}"))? + { + break status; + } + thread::sleep(POLL); + }; + let last_lines = relay + .join() + .map_err(|_| anyhow!("the relay of the Run for #{number} panicked"))? + .with_context(|| format!("could not read the Run for #{number}"))?; + let mut stdout = String::new(); + child + .stdout + .take() + .context("no stdout from the Run")? + .read_to_string(&mut stdout) + .with_context(|| format!("could not read the Run for #{number}"))?; + if status.success() { + return Ok(Ended::Reached(stdout.lines().last().map(String::from))); + } + if interrupt::requested() { + return Ok(Ended::Interrupted); + } + let [before, last] = last_lines; + let (cause, log) = match last { + Some(last) => match last.strip_prefix(failed_run::SESSION_LOG) { + Some(log) => (before, Some(log.to_string())), + None => (Some(last), None), + }, + None => (None, None), + }; + Ok(Ended::Failed { + cause: cause.unwrap_or_else(|| format!("the Run {status}")), + log, + }) +} diff --git a/src/ci.rs b/src/ci.rs index e1dfff40e..105b8c130 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -30,8 +30,10 @@ pub struct FailedChecks { /// Wait up to the grace period for any check or status on `sha`, then watch /// them until they have all finished. Checks that failed are compared, by /// name, with the checks on `base_commit` as they stand: the Base branch's CI -/// is never waited for or triggered, and no log text is read. -pub fn watch(issue: &IssueUrl, sha: &str, base_commit: &str) -> Result { +/// is never waited for or triggered, and no log text is read. With no +/// `base_commit`, as in a Base fix, every check that failed is the branch's +/// own. +pub fn watch(issue: &IssueUrl, sha: &str, base_commit: Option<&str>) -> Result { let grace = poll::grace_period(); let short = short(sha); progress::step(format_args!( @@ -73,6 +75,12 @@ pub fn watch(issue: &IssueUrl, sha: &str, base_commit: &str) -> Result { "CI failed on {short}: {}", check_names(&failed) )); + let Some(base_commit) = base_commit else { + return Ok(Ci::Failed(FailedChecks { + own: failed, + inherited: Vec::new(), + })); + }; let on_base = github::checks_on(issue, base_commit)?; let (inherited, own) = failed.into_iter().partition(|check| { let mut same_name = on_base.iter().filter(|base| base.name == check.name); diff --git a/src/failed_run.rs b/src/failed_run.rs index 901934a81..4f0a074aa 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -29,6 +29,8 @@ pub struct FailedRun { /// In a Spec run, a line on each Ticket it landed or did not get done; /// empty in a Run. pub ticket_lines: Vec, + /// What became of the Base fix it started, if it started one. + pub base_fix: Option, } /// A merge GitHub refused when a round of the Repair loop found nothing to @@ -55,6 +57,7 @@ impl From for FailedRun { log: None, interrupted, ticket_lines: Vec::new(), + base_fix: None, } } } @@ -121,6 +124,7 @@ pub fn fail( log: log.exists().then(|| log.to_path_buf()), interrupted, ticket_lines: Vec::new(), + base_fix: None, } } diff --git a/src/github.rs b/src/github.rs index 6d26c03e4..76b379687 100644 --- a/src/github.rs +++ b/src/github.rs @@ -149,6 +149,56 @@ pub fn close_issue(issue: &IssueUrl, comment: &str) -> Result<()> { ]) } +/// 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: &[(&str, &str)], +) -> Result { + let repo = issue.repo_slug(); + let known = gh_json(&[ + "label", "list", "--repo", &repo, "--json", "name", "--limit", "1000", + ])?; + let known: Vec<&str> = known + .as_array() + .context("gh label list did not return a list")? + .iter() + .filter_map(|label| label["name"].as_str()) + .collect(); + for (name, description) in labels { + // GitHub's label names are case-insensitive. + if !known.iter().any(|known| known.eq_ignore_ascii_case(name)) { + gh(&[ + "label", + "create", + name, + "--repo", + &repo, + "--description", + description, + ])?; + } + } + let names: Vec<&str> = labels.iter().map(|(name, _)| *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. diff --git a/src/main.rs b/src/main.rs index 51ded0de2..36c0aae16 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,7 @@ mod args; +mod base_fix; mod branch; +mod child_run; mod ci; mod config; mod email; @@ -29,6 +31,7 @@ use std::io::Write; use std::process::ExitCode; use args::{Command, RunArgs}; +use base_fix::Policy; use config::UserConfig; use notification::{NotificationAsk, RunNotification}; use run::Goal; @@ -47,13 +50,21 @@ usage: thirdshift Run the factory on the issue, from th thirdshift version Print thirdshift's version thirdshift help Print this help -merge, --no-merge, --email, --no-email and parallel (or --parallel ) go before or -after the Issue URL, in any order. +merge, --no-merge, --email, --no-email, base-fix and parallel (or --parallel ) go +before or after the Issue URL, in any order. --email sends one Run notification when the Run ends, whatever the outcome: ready for review, merged, failed or interrupted. --email
sends it to
; a word after --email is the address only if it has an @ and isn't a URL. +A check that fails on the pull request and also on the Base branch commit it last merged in +is an Inherited failure, not the branch's to fix: a Run whose only red checks are Inherited +failures fails, saying to fix the Base branch first. With base-fix, it starts a Base fix +instead, once: it opens an issue for those checks, labelled base-fix and ready-for-agent, +runs a Merge run on it into the Base branch, waits for it to merge, then merges the Base +branch in and watches CI again. If the Base fix fails, or the checks still fail on the Base +branch once it has merged, the Run fails, naming the Base fix issue. + On a Spec, an issue with sub-issues, the Run is a Spec run: it takes every Ticket (sub-issue) it can reach, in the order their \"blocked by\" links allow, each merged into the Spec branch. Its Spec PR opens as a draft, with a Tickets checklist, once the first Ticket lands, and is marked ready @@ -117,7 +128,9 @@ fn main() -> ExitCode { goal, email, parallel, + base_fix, spec_branch, + base_fix_into, } = match args::parse(&args) { Ok(Command::Help) => { print!("{HELP}"); @@ -144,9 +157,16 @@ fn main() -> ExitCode { return ExitCode::FAILURE; } }; - // A Ticket's Run in a Spec run is always a Merge run, and leaves the Run - // notification and the Launch directory to the Spec run. - let (goal, email, launch_pull) = match spec_branch { + let inherited_failures = match (&base_fix_into, base_fix) { + (Some(_), _) => Policy::IsBaseFix, + (None, true) => Policy::StartBaseFix, + (None, false) => Policy::FailTheRun, + }; + // A Ticket's Run in a Spec run, like a Base fix, is always a Merge run + // into the Base branch it was given, and leaves the Run notification and + // the Launch directory to what started it. + let given_base = spec_branch.or(base_fix_into); + let (goal, email, launch_pull) = match given_base { Some(_) => (Goal::Merged, NotificationAsk::Skip, false), None => ( goal.unwrap_or(config.default_goal()), @@ -180,7 +200,8 @@ fn main() -> ExitCode { &config.logs_dir, launch_pull, parallel, - spec_branch.as_deref(), + given_base.as_deref(), + inherited_failures, ); let code = match &ended { Ok(reached) => { diff --git a/src/notification.rs b/src/notification.rs index 2da5b25b3..d78456dac 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -57,13 +57,14 @@ impl RunNotification { /// Send the notification for the Run that `ended`. A failed send is only /// a warning: it never changes the Run's outcome. pub fn send(self, ended: &Result) { - let (outcome, pr_url, cause, log, tickets) = match ended { + let (outcome, pr_url, cause, log, tickets, base_fix) = match ended { Ok(reached) => ( reached.goal.outcome(), Some(reached.pr_url.as_str()), None, reached.log.as_deref(), &reached.ticket_lines[..], + reached.base_fix.as_deref(), ), Err(failed) => { let (outcome, cause) = if failed.interrupted { @@ -77,6 +78,7 @@ impl RunNotification { cause, failed.log.as_deref(), &failed.ticket_lines[..], + failed.base_fix.as_deref(), ) } }; @@ -84,6 +86,7 @@ impl RunNotification { let body = Body { pr_url, cause: cause.as_deref(), + base_fix, log, host: host.as_deref().unwrap_or("unknown host"), took: self.started.elapsed(), @@ -113,6 +116,8 @@ fn subject(issue: &IssueUrl, title: Option<&str>, outcome: &str) -> String { struct Body<'a> { pr_url: Option<&'a str>, cause: Option<&'a str>, + /// What became of the Base fix the Run started, if it started one. + base_fix: Option<&'a str>, log: Option<&'a Path>, host: &'a str, took: Duration, @@ -129,6 +134,9 @@ impl Body<'_> { if let Some(cause) = self.cause { text += &format!("Cause: {cause}\n"); } + if let Some(base_fix) = self.base_fix { + text += &format!("Base fix: {base_fix}\n"); + } if let Some(log) = self.log { text += &format!("Session log: {}\n", log.display()); } @@ -182,6 +190,7 @@ mod tests { let body = Body { pr_url: None, cause: Some("origin mismatch"), + base_fix: None, log: None, host: "droplet-1", took: Duration::from_secs(4), @@ -208,6 +217,27 @@ mod tests { ); } + #[test] + fn the_body_reports_the_base_fix_after_the_cause() { + let body = Body { + pr_url: Some("https://github.com/acme/widgets/pull/1"), + cause: Some("claude exited 1"), + base_fix: Some("https://github.com/acme/widgets/issues/8 merged"), + log: None, + host: "droplet-1", + took: Duration::from_secs(4), + tickets: &[], + }; + assert_eq!( + body.text(), + "Pull request: https://github.com/acme/widgets/pull/1\n\ + Cause: claude exited 1\n\ + Base fix: https://github.com/acme/widgets/issues/8 merged\n\ + Host: droplet-1\n\ + Took: 4s\n" + ); + } + #[test] fn a_spec_runs_body_ends_with_a_line_per_ticket() { let tickets = [ @@ -217,6 +247,7 @@ mod tests { let body = Body { pr_url: None, cause: Some("Tickets not done: #21, #22"), + base_fix: None, log: None, host: "droplet-1", took: Duration::from_secs(4), diff --git a/src/run.rs b/src/run.rs index 01bb61c1c..f14b966ef 100644 --- a/src/run.rs +++ b/src/run.rs @@ -5,6 +5,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; +use crate::base_fix::{BaseFix, Policy}; use crate::branch::{self, Selection}; use crate::ci::{self, Ci}; use crate::failed_run::{self, FailedRun, PolicyRefusal}; @@ -48,6 +49,8 @@ pub struct Reached { pub log: Option, /// In a Spec run, a line on each Ticket it landed; empty in a Run. pub ticket_lines: Vec, + /// What became of the Base fix it started, if it started one. + pub base_fix: Option, } /// Take `issue` to a ready PR, or in a Merge run a merged one. Any failure @@ -58,26 +61,31 @@ pub struct Reached { /// branch. With `launch_pull`, the Launch directory's checkout of the /// Base branch is first brought up to date with origin. /// -/// With `spec_branch`, this is a Ticket's Run in a Spec run, and the Spec -/// branch stands in for the checked-out branch as the Base branch. Otherwise -/// an issue with sub-issues is a Spec, taken on by a Spec run instead, whose -/// Spec branch is picked like an Issue branch, running as many Tickets at once -/// as `parallel` says. A `parallel` the command asked for on an issue with no -/// sub-issues fails before any work, as does a Spec whose Tickets are all -/// closed with no Spec branch to continue. +/// With `given_base`, this is a Run another thirdshift started, a Ticket's Run +/// in a Spec run or a Base fix, and `given_base`, the Spec branch or that +/// Run's Base branch, stands in for the checked-out branch as the Base branch. +/// Otherwise an issue with sub-issues is a Spec, taken on by a Spec run +/// instead, whose Spec branch is picked like an Issue branch, running as many +/// Tickets at once as `parallel` says. A `parallel` the command asked for on +/// an issue with no sub-issues fails before any work, as does a Spec whose +/// Tickets are all closed with no Spec branch to continue. +/// +/// `inherited_failures` says what the Run, or a Spec run on its Spec PR, does +/// when its only red checks are Inherited failures. pub fn run( issue: &IssueUrl, goal: Goal, logs_dir: &Path, launch_pull: bool, parallel: Parallel, - spec_branch: Option<&str>, + given_base: Option<&str>, + inherited_failures: Policy, ) -> Result { let timestamp = chrono::Utc::now().format("%Y%m%dT%H%M%SZ").to_string(); let launch = Git::new(std::env::current_dir().context("no current directory")?); preflight::check(&launch, issue)?; - let tickets = match spec_branch { + let tickets = match given_base { Some(_) => Vec::new(), None => github::tickets(issue)?, }; @@ -88,8 +96,8 @@ pub fn run( ) .into()); } - let checked_out = match spec_branch { - Some(spec_branch) => Some(spec_branch.to_string()), + let checked_out = match given_base { + Some(given_base) => Some(given_base.to_string()), None => launch .run(&["symbolic-ref", "--quiet", "--short", "HEAD"]) .ok(), @@ -122,6 +130,7 @@ pub fn run( dir: logs_dir, timestamp: ×tamp, }; + let mut base_fix = BaseFix::new(inherited_failures); if !tickets.is_empty() { return spec_run::run( issue, @@ -129,6 +138,7 @@ pub fn run( worktree, &base, goal, + &mut base_fix, &logs, parallel.tickets, ); @@ -140,14 +150,28 @@ pub fn run( } }; let mut log = logs.path("implement"); - match implement(issue, &worktree, &base, &prompt, goal, &logs, &mut log) { + let implemented = implement( + issue, + &worktree, + &base, + &prompt, + goal, + &mut base_fix, + &logs, + &mut log, + ); + match implemented { Ok(pr_url) => Ok(Reached { pr_url, goal, log: Some(log), ticket_lines: Vec::new(), + base_fix: base_fix.report(), + }), + Err(error) => Err(FailedRun { + base_fix: base_fix.report(), + ..failed_run::fail(issue, worktree, &base, &log, error) }), - Err(error) => Err(failed_run::fail(issue, worktree, &base, &log, error)), } } @@ -182,12 +206,14 @@ fn pull_base_branch(launch: &Git, checked_out: Option<&str>, base: &str) { /// The implement session given `prompt`, then [`deliver`] on the PR it /// opened or updated. `log` is left at the most recent session's log. +#[allow(clippy::too_many_arguments)] fn implement( issue: &IssueUrl, worktree: &Worktree, base: &str, prompt: &str, goal: Goal, + base_fix: &mut BaseFix, logs: &Logs, log: &mut PathBuf, ) -> Result { @@ -202,13 +228,14 @@ fn implement( run_session("implement", prompt)?; worktree.push()?; let pr = mark_pr_ready(issue, worktree.branch(), base)?; - deliver(issue, worktree, base, &pr, goal, &mut run_session)?; + deliver(issue, worktree, base, &pr, goal, base_fix, &mut run_session)?; Ok(pr.url) } /// Take the ready PR `pr` for `issue`, from the branch checked out in /// `worktree` into `base`, to `goal`: keep it mergeable and its CI green -/// through the Repair loop, starting each Repair through `run_session`, and +/// through the Repair loop, starting each Repair through `run_session` and +/// its one Base fix, if any, through `base_fix`, 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, this fails with a `PolicyRefusal`. @@ -218,6 +245,7 @@ pub fn deliver( base: &str, pr: &PullRequest, goal: Goal, + base_fix: &mut BaseFix, run_session: &mut impl FnMut(&str, &str) -> Result<()>, ) -> Result<()> { let branch = worktree.branch(); @@ -228,6 +256,7 @@ pub fn deliver( pr_url: &pr.url, goal, budgets: Budgets::default(), + base_fix, }; let mut watched = repair_loop.run(run_session)?; loop { @@ -387,6 +416,8 @@ struct RepairLoop<'a> { pr_url: &'a str, goal: Goal, budgets: Budgets, + /// The Run's one Base fix, kept apart from its budgets. + base_fix: &'a mut BaseFix, } impl RepairLoop<'_> { @@ -400,8 +431,9 @@ impl RepairLoop<'_> { /// merged in is an Inherited failure, which no Repair is started for: the /// CI-fix Repair is given the branch's own failures, and when there are /// none, the loop goes round again if the Base branch moved since, counted - /// as a Base move, and otherwise fails naming the checks and the Base - /// branch commit. + /// as a Base move, and otherwise once its Base fix has merged, or fails + /// naming the checks and the Base branch commit: see [`BaseFix::fix`]. A + /// Base fix's own Run sees no Inherited failures. /// Returns the head commit whose CI was last watched and found green or /// absent. Fails with a Declined CI fix if, after a CI-fix Repair and the /// Base branch merged again, the head is still the one whose CI failed, @@ -431,7 +463,11 @@ impl RepairLoop<'_> { ); } let base_commit = worktree.merged_base_commit(base)?; - match ci::watch(issue, &head, &base_commit)? { + let compared_with = self + .base_fix + .sees_inherited_failures() + .then_some(base_commit.as_str()); + match ci::watch(issue, &head, compared_with)? { Ci::Absent | Ci::Passed => { if worktree.base_branch_moved(base)? { self.budgets.count_base_move(base, "while CI ran")?; @@ -455,11 +491,9 @@ impl RepairLoop<'_> { self.budgets.count_base_move(base, "while CI ran")?; continue; } - bail!( - "CI red on {}, which also fails on {base} at {}; fix {base} first", - ci::check_names(&failed.inherited), - ci::short(&base_commit) - ); + self.base_fix + .fix(issue, pr_url, base, &base_commit, &failed.inherited)?; + continue; } let kind = self.budgets.next_repair("CI red")?; run_session( diff --git a/src/spec_run.rs b/src/spec_run.rs index 6a4ee6d8e..f65e06b6c 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -6,17 +6,17 @@ use std::collections::hash_map::Entry; use std::collections::{BTreeMap, HashMap, HashSet, VecDeque}; -use std::io::{BufRead, BufReader, Read}; use std::num::NonZeroUsize; use std::path::PathBuf; -use std::process::{Child, Command, Stdio}; +use std::process::Child; use std::sync::mpsc::{self, Sender}; use std::thread; -use std::time::Duration; -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::{Context, Result, bail}; use crate::args; +use crate::base_fix::BaseFix; +use crate::child_run::{self, Ended}; use crate::failed_run::{self, FailedRun}; use crate::github::{self, PullRequest, Ticket}; use crate::interrupt; @@ -28,9 +28,6 @@ use crate::run::{self, Goal, Reached}; use crate::session::{Logs, Sessions}; use crate::worktree::Worktree; -/// How often to check whether a Ticket's Run has ended, or been interrupted. -const POLL: Duration = Duration::from_millis(100); - /// The triage labels that make an open Ticket an Unready Ticket. const UNREADY_LABELS: [&str; 4] = ["ready-for-human", "needs-info", "wontfix", "needs-triage"]; @@ -72,13 +69,16 @@ pub fn all_closed(tickets: &[Ticket]) -> bool { /// from the Spec review on goes through the Failed run path. The worktree is /// cleaned up when this returns, or kept by the Failed run path if its work /// did not reach origin. However it ends, it carries a line on each Ticket -/// it landed or did not get done, for the Run notification. +/// it landed or did not get done, for the Run notification. `base_fix` is for +/// the Spec PR's Repair loop: a Ticket's Run starts no Base fix. +#[allow(clippy::too_many_arguments)] pub fn run( spec: &IssueUrl, tickets: Vec, worktree: Worktree, base: &str, goal: Goal, + base_fix: &mut BaseFix, logs: &Logs, parallel: NonZeroUsize, ) -> Result { @@ -86,7 +86,9 @@ pub fn run( let (ticket_lines, landed) = land_tickets(spec, tickets, &worktree, base, parallel, &mut spec_pr); let ended = match landed { - Ok(checklist) => open_and_review(spec, worktree, base, spec_pr, &checklist, goal, logs), + Ok(checklist) => open_and_review( + spec, worktree, base, spec_pr, &checklist, goal, base_fix, logs, + ), Err(error) => Err(FailedRun { pr_url: spec_pr.map(|pr| pr.url), ..FailedRun::from(error) @@ -95,10 +97,12 @@ pub fn run( match ended { Ok(reached) => Ok(Reached { ticket_lines, + base_fix: base_fix.report(), ..reached }), Err(failed) => Err(FailedRun { ticket_lines, + base_fix: base_fix.report(), ..failed }), } @@ -107,6 +111,7 @@ pub fn run( /// Once every Ticket has landed: open the Spec PR as a draft with /// `checklist` if `spec_pr` is none, run the Spec review, put `checklist` /// back, mark the Spec PR ready and take it to `goal`, as [`run`] does. +#[allow(clippy::too_many_arguments)] fn open_and_review( spec: &IssueUrl, worktree: Worktree, @@ -114,6 +119,7 @@ fn open_and_review( spec_pr: Option, checklist: &str, goal: Goal, + base_fix: &mut BaseFix, logs: &Logs, ) -> Result { let spec_pr = match spec_pr { @@ -122,7 +128,7 @@ fn open_and_review( }; let mut log = logs.path(SPEC_REVIEW); let delivered = review_and_deliver( - spec, &worktree, base, &spec_pr, checklist, goal, logs, &mut log, + spec, &worktree, base, &spec_pr, checklist, goal, base_fix, logs, &mut log, ); match delivered { Ok(()) => Ok(Reached { @@ -130,6 +136,7 @@ fn open_and_review( goal, log: Some(log), ticket_lines: Vec::new(), + base_fix: None, }), Err(error) => { // The Spec review may have rewritten the body without it. @@ -470,90 +477,29 @@ fn start_ticket( ended: Sender<(u64, Result)>, ) -> Result<()> { progress::step(format_args!("starting #{number}")); - let ticket = spec.sibling(number); - let child = Command::new(std::env::current_exe().context("no thirdshift executable")?) - .args([args::SPEC_BRANCH, spec_branch, &ticket.url]) - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .with_context(|| format!("could not start the Run for #{number}"))?; + let child = child_run::start(&spec.sibling(number), [args::SPEC_BRANCH, spec_branch])?; thread::spawn(move || { let _ = ended.send((number, finish_ticket(number, child))); }); Ok(()) } -/// Relay the stderr of Ticket `number`'s Run `child` with a `#: ` -/// prefix until it exits. An interrupt is passed on to the child, which is -/// waited for as it goes down its Failed run path, and its outcome is -/// `Interrupted`. Otherwise it landed if it exits 0, and failed otherwise, -/// with the cause and session log it ended on. -fn finish_ticket(number: u64, mut child: Child) -> Result { - // Relay on its own thread, so this one can watch for an interrupt. - let stderr = child.stderr.take().context("no stderr from the Run")?; - // A failed Run ends on its error, then its session log if it has one. - let relay = thread::spawn(move || -> std::io::Result<[Option; 2]> { - let mut last_lines: [Option; 2] = [None, None]; - for line in BufReader::new(stderr).lines() { - let line = line?; - let message = progress::relay(format_args!("#{number}"), &line).to_string(); - last_lines = [last_lines[1].take(), Some(message)]; +/// Wait for Ticket `number`'s Run `child`, relaying its stderr, and say how +/// it ended. +fn finish_ticket(number: u64, child: Child) -> Result { + Ok(match child_run::wait(number, child)? { + Ended::Reached(pr_url) => { + progress::step(format_args!("#{number} landed")); + TicketOutcome::Landed(pr_url) } - Ok(last_lines) - }); - let mut passed_on = false; - let status = loop { - // The child shares the process group, so a Ctrl-C or a closed - // terminal reaches it too, but a signal sent to this process alone - // doesn't. A second one is harmless: it only records the interrupt. - if !passed_on && interrupt::requested() { - // SAFETY: kill has no memory-safety preconditions, and the child - // is not yet reaped, so its pid is still its own. - unsafe { libc::kill(child.id() as libc::pid_t, libc::SIGTERM) }; - passed_on = true; + Ended::Interrupted => { + progress::step(format_args!("#{number} interrupted")); + TicketOutcome::Interrupted } - if let Some(status) = child - .try_wait() - .with_context(|| format!("could not wait for the Run for #{number}"))? - { - break status; + Ended::Failed { cause, log } => { + progress::step(format_args!("#{number} failed")); + TicketOutcome::Failed { cause, log } } - thread::sleep(POLL); - }; - let last_lines = relay - .join() - .map_err(|_| anyhow!("the relay of the Run for #{number} panicked"))? - .with_context(|| format!("could not read the Run for #{number}"))?; - let mut stdout = String::new(); - child - .stdout - .take() - .context("no stdout from the Run")? - .read_to_string(&mut stdout) - .with_context(|| format!("could not read the Run for #{number}"))?; - if status.success() { - progress::step(format_args!("#{number} landed")); - return Ok(TicketOutcome::Landed( - stdout.lines().last().map(String::from), - )); - } - if interrupt::requested() { - progress::step(format_args!("#{number} interrupted")); - return Ok(TicketOutcome::Interrupted); - } - progress::step(format_args!("#{number} failed")); - let [before, last] = last_lines; - let (cause, log) = match last { - Some(last) => match last.strip_prefix(failed_run::SESSION_LOG) { - Some(log) => (before, Some(log.to_string())), - None => (Some(last), None), - }, - None => (None, None), - }; - Ok(TicketOutcome::Failed { - cause: cause.unwrap_or_else(|| format!("the Run {status}")), - log, }) } @@ -615,8 +561,8 @@ fn write_checklist_or_warn(spec: &IssueUrl, pr: &PullRequest, checklist: &str) { /// origin, and run the Spec review on it. Then push the Spec branch, for any /// commit the session left unpushed, put `checklist` back in the body the /// session wrote for the Spec PR `spec_pr` into `base`, mark it ready, and -/// [`run::deliver`] it to `goal`, with the Spec as the issue. `log` is left -/// at the most recent session's log. +/// [`run::deliver`] it to `goal`, with the Spec as the issue and `base_fix` +/// as its one Base fix. `log` is left at the most recent session's log. #[allow(clippy::too_many_arguments)] fn review_and_deliver( spec: &IssueUrl, @@ -625,6 +571,7 @@ fn review_and_deliver( spec_pr: &PullRequest, checklist: &str, goal: Goal, + base_fix: &mut BaseFix, logs: &Logs, log: &mut PathBuf, ) -> Result<()> { @@ -644,7 +591,7 @@ fn review_and_deliver( worktree.push()?; write_checklist(spec, spec_pr, checklist)?; let pr = run::mark_pr_ready(spec, spec_branch, base)?; - run::deliver(spec, worktree, base, &pr, goal, &mut run_session) + run::deliver(spec, worktree, base, &pr, goal, base_fix, &mut run_session) } #[cfg(test)] diff --git a/tests/base_fix.rs b/tests/base_fix.rs new file mode 100644 index 000000000..fe8c1693b --- /dev/null +++ b/tests/base_fix.rs @@ -0,0 +1,342 @@ +//! The Base fix: with `base-fix`, a Run whose only red checks are Inherited +//! failures writes an issue for them, starts a Merge run into its Base branch +//! on that issue, waits for it, then merges the Base branch in and watches CI +//! again. One Base fix per Run; if it fails, or the checks are still +//! Inherited failures once it has merged, the Run fails naming its issue. + +mod support; + +use support::Scenario; +use support::resend::ResendStandIn; + +/// The agent adds feature.txt and opens the PR for #7, with `test` red on its +/// head and on `main`'s tip. +const RUN_OPENS_PR_WITH_INHERITED_FAILURE: &str = r#" +echo "feature" > feature.txt +git add feature.txt +git commit -q -m "Add feature" +gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" +gh fake checks "$(git rev-parse HEAD)" '[{"name": "test", "conclusion": "failure", "url": "https://ci.example/test"}]' +gh fake checks "$(git rev-parse origin/main)" '[{"name": "test", "conclusion": "failure", "url": "https://ci.example/test"}]' +"#; + +/// The agent, on the Base fix issue #8, commits a fix and opens its PR into +/// `main`. +const BASE_FIX_OPENS_PR: &str = r#" +echo "fixed" > ci-fix.txt +git add ci-fix.txt +git commit -q -m "Fix CI on main" +gh pr create --base main --head issue-8 --title "Fix CI on main" --body "Closes #8" +"#; + +const GREEN_ON_HEAD: &str = "gh fake checks \"$(git rev-parse HEAD)\" '[{\"name\": \"test\", \"conclusion\": \"success\"}]'\n"; + +const RED_ON_HEAD: &str = "gh fake checks \"$(git rev-parse HEAD)\" '[{\"name\": \"test\", \"conclusion\": \"failure\"}]'\n"; + +const BASE_FIX_URL: &str = "https://github.com/acme/widgets/issues/8"; + +/// Every `gh ` call made, e.g. `gh issue create`. +fn gh_calls_of(scenario: &Scenario, command: &str, subcommand: &str) -> Vec> { + scenario + .gh_calls() + .into_iter() + .filter(|call| call.starts_with(&[command.to_string(), subcommand.to_string()])) + .collect() +} + +/// The prompt of each session, in order. +fn prompts(scenario: &Scenario) -> Vec { + scenario + .claude_calls() + .iter() + .map(|call| call["prompt"].as_str().unwrap().to_string()) + .collect() +} + +#[test] +fn with_base_fix_an_inherited_failure_gets_an_issue_a_merged_fix_and_the_run_finishes_green() { + let scenario = Scenario::new(); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + scenario.agent_does_for(8, &format!("{BASE_FIX_OPENS_PR}{GREEN_ON_HEAD}")); + let red_base = scenario.origin_git(&["rev-parse", "main"]); + + let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + let gh = scenario.gh_state(); + assert_eq!(gh["titles"]["8"], "CI red on main: test"); + assert_eq!( + gh["bodies"]["8"], + format!( + "CI is red on `main` at {}: these checks fail there, so every pull request into `main` inherits them.\n\ + \n\ + - test: https://ci.example/test\n\ + \n\ + Found by the thirdshift Run on #7, whose pull request is https://github.com/acme/widgets/pull/1.\n\ + \n\ + Fix the checks on `main`. Do not skip, disable, or weaken tests or checks to make them pass.\n", + &red_base[..7] + ) + ); + assert_eq!( + gh["labels"]["8"], + serde_json::json!(["base-fix", "ready-for-agent"]) + ); + for line in [ + format!("thirdshift: starting Base fix #8 into main: {BASE_FIX_URL}\n"), + "thirdshift: waiting on Base fix #8\n".to_string(), + "thirdshift: Base fix #8 merged; merging main in again\n".to_string(), + ] { + assert!(result.stderr.contains(&line), "stderr: {}", result.stderr); + } + // The Base fix was a Merge run into main, whose Self-merge closed its issue. + assert_eq!(gh["prs"][1]["base"], "main"); + assert_eq!(gh["prs"][1]["state"], "MERGED"); + assert_eq!(gh["issues"]["8"], "CLOSED"); + // The Run merged the fixed main in, and left its PR ready for review. + assert_eq!( + scenario.origin_file("issue-7", "ci-fix.txt").as_deref(), + Some("fixed\n") + ); + assert_eq!(gh["prs"][0]["state"], "OPEN"); + assert_eq!(gh["prs"][0]["isDraft"], false); + let prompts = prompts(&scenario); + assert_eq!(prompts.len(), 2, "one implement session each: {prompts:?}"); + assert!(prompts[1].contains(BASE_FIX_URL), "prompt: {}", prompts[1]); + scenario.assert_cleaned_up("issue-7"); + scenario.assert_cleaned_up("issue-8"); +} + +#[test] +fn the_base_fix_repairs_the_check_as_its_own_and_only_the_run_sends_a_notification() { + let scenario = Scenario::new(); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + // On the Base fix's head, test is as red as on main, which it branched + // off: no Inherited failure to the Base fix, whose Repair turns it green. + scenario.agent_does_for_in_session(8, 1, &format!("{BASE_FIX_OPENS_PR}{RED_ON_HEAD}")); + scenario.agent_does_for_in_session( + 8, + 2, + &format!( + "echo more > more.txt\ngit add more.txt\ngit commit -q -m 'Fix more'\n{GREEN_ON_HEAD}" + ), + ); + let resend = ResendStandIn::replying(200, r#"{"id":"49a3999c-0ce1-4ea6-ab68-afcd6dc2e794"}"#); + + let result = scenario.run_with_env( + &[ + &scenario.issue_url(7), + "--base-fix", + "--email", + "me@example.com", + ], + &[ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", "re_test_123"), + ], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let prompts = prompts(&scenario); + assert_eq!(prompts.len(), 3, "prompts: {prompts:?}"); + assert!( + prompts[2].starts_with("CI failed on pull request https://github.com/acme/widgets/pull/2") + && prompts[2].contains("- test") + && !prompts[2].contains("Also failing on"), + "prompt: {}", + prompts[2] + ); + assert!( + !result.stderr.contains("#8: Inherited failures"), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.gh_state()["prs"][1]["state"], "MERGED"); + assert_eq!(gh_calls_of(&scenario, "issue", "create").len(), 1); + // One Run notification: the Run's, which reports the Base fix. + let requests = resend.requests(); + assert_eq!(requests.len(), 1, "{requests:?}"); + assert_eq!( + requests[0].body["subject"], + "[thirdshift] acme/widgets#7 Issue 7: ready for review" + ); + let text = requests[0].body["text"].as_str().unwrap(); + assert!( + text.contains(&format!("Base fix: {BASE_FIX_URL} merged\n")), + "text: {text}" + ); +} + +#[test] +fn a_merge_run_self_merges_once_its_base_fix_has_merged() { + let scenario = Scenario::new(); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + scenario.agent_does_for(8, &format!("{BASE_FIX_OPENS_PR}{GREEN_ON_HEAD}")); + + let result = scenario.run(&["merge", "base-fix", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let gh = scenario.gh_state(); + assert_eq!(gh["prs"][0]["state"], "MERGED"); + assert_eq!(gh["prs"][1]["state"], "MERGED"); + assert_eq!(gh["issues"]["7"], "CLOSED"); + for file in ["feature.txt", "ci-fix.txt"] { + assert!(scenario.origin_file("main", file).is_some(), "{file}"); + } +} + +#[test] +fn the_base_fix_merges_into_the_runs_base_branch_whatever_the_launch_directory_has_checked_out() { + let scenario = Scenario::new(); + // A Continuation whose open PR targets develop, started from main. + scenario.origin_has_branch("develop", "main", &[]); + scenario.origin_has_branch("issue-7", "develop", &["Add feature"]); + scenario.github_has_pr("issue-7", "develop", "OPEN"); + scenario.agent_does( + r#" +gh fake checks "$(git rev-parse HEAD)" '[{"name": "test", "conclusion": "failure"}]' +gh fake checks "$(git rev-parse origin/develop)" '[{"name": "test", "conclusion": "failure"}]' +"#, + ); + scenario.agent_does_for( + 8, + &format!( + "{}{GREEN_ON_HEAD}", + BASE_FIX_OPENS_PR.replace("--base main", "--base develop") + ), + ); + + let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let gh = scenario.gh_state(); + assert_eq!(gh["titles"]["8"], "CI red on develop: test"); + assert_eq!(gh["prs"][1]["base"], "develop"); + assert_eq!(gh["prs"][1]["state"], "MERGED"); + assert!(scenario.origin_file("develop", "ci-fix.txt").is_some()); + assert!(scenario.origin_file("main", "ci-fix.txt").is_none()); + assert!(scenario.origin_file("issue-7", "ci-fix.txt").is_some()); +} + +#[test] +fn a_failing_base_fix_fails_the_run_naming_its_issue() { + let scenario = Scenario::new(); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + scenario.agent_does_for(8, "exit 3\n"); + + let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + let cause = format!("Base fix {BASE_FIX_URL} failed: "); + assert!( + result + .stderr + .lines() + .any(|line| line.starts_with(&format!("thirdshift: {cause}"))), + "stderr: {}", + result.stderr + ); + let gh = scenario.gh_state(); + assert_eq!(gh["issues"]["8"], "OPEN"); + assert_eq!(gh["prs"][0]["isDraft"], true); + assert!( + scenario.origin_log("issue-7").unwrap()[0] + .starts_with(&format!("thirdshift: failed run ({cause}")), + "log: {:?}", + scenario.origin_log("issue-7") + ); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn checks_still_inherited_failures_after_the_base_fix_merged_fail_the_run_with_no_second_base_fix() +{ + let scenario = Scenario::new(); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + scenario.agent_does_for(8, &format!("{BASE_FIX_OPENS_PR}{GREEN_ON_HEAD}")); + // The Base fix merges, but test is red on main's merge commit all the + // same, and on the head of the Run that merges it in. + const RED: &str = r#"[{"name": "test", "conclusion": "failure"}]"#; + let mut gh = scenario.gh_state(); + gh["after_merge"] = serde_json::json!(format!( + "gh fake checks \"$FAKE_MERGE_SHA\" '{RED}'\n\ + gh fake on-ci-read 2 'gh fake checks \"$FAKE_CI_SHA\" '\\''{RED}'\\'''\n" + )); + scenario.write_gh_state(&gh); + + let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let fixed_base = scenario.origin_git(&["rev-parse", "main"]); + let cause = format!( + "CI red on test, which also fails on main at {}, even after Base fix {BASE_FIX_URL} merged; fix main first", + &fixed_base[..7] + ); + assert!(result.stderr.contains(&cause), "stderr: {}", result.stderr); + let gh = scenario.gh_state(); + assert_eq!(gh["prs"][1]["state"], "MERGED"); + assert_eq!(gh_calls_of(&scenario, "issue", "create").len(), 1); + assert_eq!(gh["issues"].as_object().unwrap().len(), 2, "#7 and #8"); + assert_eq!(gh["prs"][0]["isDraft"], true); + assert_eq!(prompts(&scenario).len(), 2, "no Repair"); +} + +#[test] +fn without_base_fix_an_inherited_failure_fails_the_run_and_writes_no_issue() { + let scenario = Scenario::new(); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + let red_base = scenario.origin_git(&["rev-parse", "main"]); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert!( + result.stderr.contains(&format!( + "CI red on test, which also fails on main at {}; fix main first", + &red_base[..7] + )), + "stderr: {}", + result.stderr + ); + assert!(gh_calls_of(&scenario, "issue", "create").is_empty()); + assert!(gh_calls_of(&scenario, "label", "list").is_empty()); + assert_eq!(scenario.gh_state()["issues"].as_object().unwrap().len(), 1); +} + +#[test] +fn a_label_the_repository_lacks_is_added_and_one_it_has_is_left_alone() { + let scenario = Scenario::new(); + let mut gh = scenario.gh_state(); + gh["repo_labels"] = serde_json::json!(["bug", "Ready-For-Agent"]); + scenario.write_gh_state(&gh); + scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); + scenario.agent_does_for(8, &format!("{BASE_FIX_OPENS_PR}{GREEN_ON_HEAD}")); + + let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let created = gh_calls_of(&scenario, "label", "create"); + assert_eq!(created.len(), 1, "{created:?}"); + assert_eq!(created[0][2], "base-fix"); + assert_eq!( + scenario.gh_state()["repo_labels"], + serde_json::json!(["bug", "Ready-For-Agent", "base-fix"]) + ); +} + +#[test] +fn help_names_base_fix_and_not_the_base_fixs_hidden_argument() { + let scenario = Scenario::new(); + + let result = scenario.run(&["help"]); + + assert_eq!(result.code, Some(0)); + assert!(result.stdout.contains("base-fix"), "{}", result.stdout); + assert!( + !result.stdout.contains("base-fix-into"), + "{}", + result.stdout + ); +} diff --git a/tests/command_surface.rs b/tests/command_surface.rs index be4797fa3..3f2063091 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -186,6 +186,8 @@ fn a_repeated_flag_prints_an_error_and_the_help_to_stderr() { vec![url.as_str(), "email", "--email", "b@example.com"], "--email", ), + (vec!["base-fix", url.as_str(), "base-fix"], "base-fix"), + (vec![url.as_str(), "base-fix", "--base-fix"], "--base-fix"), ( vec!["parallel", "2", url.as_str(), "parallel", "2"], "parallel", diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index ab776d5ef..3aaa3d453 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -6,6 +6,7 @@ //! {"repo": "owner/repo", //! "issues": {"": "OPEN" | "CLOSED"}, //! "titles"?: {"": ""}, +//! "bodies"?: {"<number>": "<body>"}, //! "comments"?: {"<number>": ["<body>", ...]}, //! "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body", //! "mergeable"?, "unknown_polls"?}], @@ -21,6 +22,7 @@ //! "user_email"?: "<address>" | null, //! "sub_issues"?: {"<number>": [<number>, ...]}, //! "labels"?: {"<number>": ["<label>", ...]}, +//! "repo_labels"?: ["<label>", ...], //! "blocked_by"?: {"<number>": [<number>, ...]}} //! ``` //! @@ -74,6 +76,13 @@ //! `gh issue close` closes the issue, recording its `--comment`; on an issue //! already closed it only warns, as gh does. //! +//! `gh issue create --title <title> --body <body> --label <label>,<label>` +//! opens an issue numbered one past the highest issue, with that title, body +//! (in `bodies`) and labels, and prints its URL. A label the repository does +//! not have, one not in `repo_labels`, fails it, as gh does. `gh label list +//! --json name` lists the repository's labels, and `gh label create <name>` +//! adds one, failing if the repository has it already. +//! //! A PR's mergeable state reports as UNKNOWN for its first `unknown_polls` //! reads. A check run reports as in progress for its first `pending_polls` //! reads, then as completed with its `conclusion`. @@ -111,6 +120,7 @@ //! gh fake user-email '<JSON>' set the public profile email, an //! address or null //! gh fake labels <number> '<JSON list>' set issue <number>'s labels +//! gh fake repo-labels '<JSON list>' set the repository's labels //! ``` //! //! Every call's argv is appended to the JSON list in $FAKE_GH_RECORD. @@ -605,6 +615,85 @@ fn issue_close(state: &mut Json, positional: &[String], flags: &Flags) { save(state); } +/// Open an issue with `--title`, `--body` and the comma-separated `--label`s, +/// numbered one past the highest issue, and print its URL. +fn issue_create(state: &mut Json, flags: &Flags) { + let labels: Vec<&str> = flag(flags, "label") + .map(|labels| labels.split(',').collect()) + .unwrap_or_default(); + for label in &labels { + if !has_repo_label(state, label) { + die(&format!("could not add label: '{label}' not found"), 1); + } + } + let Json::Object(issues) = state.at("issues") else { + panic!("issues is an object") + }; + let highest = issues + .iter() + .filter_map(|(n, _)| n.parse::<i64>().ok()) + .max(); + let n = (highest.unwrap_or(0) + 1).to_string(); + state.at_mut("issues").set(&n, string("OPEN")); + let title = flag(flags, "title").expect("no --title"); + state.entry("titles", object([])).set(&n, string(title)); + let body = flag(flags, "body").expect("no --body"); + state.entry("bodies", object([])).set(&n, string(body)); + let labels = labels.into_iter().map(string).collect(); + state.entry("labels", object([])).set(&n, Array(labels)); + save(state); + println!("https://github.com/{}/issues/{n}", state.at("repo").str()); +} + +/// Whether the repository has the label `name`, whatever its case. +fn has_repo_label(state: &Json, name: &str) -> bool { + let labels = state + .get("repo_labels") + .map(Json::items) + .unwrap_or_default(); + labels + .iter() + .any(|label| label.str().eq_ignore_ascii_case(name)) +} + +/// `gh label list --json name`: the repository's labels. +fn label_list(state: &Json, flags: &Flags) { + if flag(flags, "json") != Some("name") { + die( + &format!("fake gh: unsupported label list flags {flags:?}"), + 2, + ); + } + let labels = state + .get("repo_labels") + .map(Json::items) + .unwrap_or_default(); + let listed = labels + .iter() + .map(|label| object([("name", label.clone())])) + .collect(); + println!("{}", Array(listed)); +} + +/// `gh label create <name>`: add a label to the repository, failing if it has +/// it already, as gh does without `--force`. +fn label_create(state: &mut Json, positional: &[String]) { + let name = &positional[0]; + if has_repo_label(state, name) { + die( + &format!( + "label with name \"{name}\" already exists; use `--force` to update its color and description" + ), + 1, + ); + } + state + .entry("repo_labels", Array(Vec::new())) + .items_mut() + .push(string(name)); + save(state); +} + /// A commit endpoint `gh api` answers. enum Endpoint { CheckRuns, @@ -1192,6 +1281,7 @@ fn fake_command(state: &mut Json, args: &[&str]) { ["after-merge", script] => state.set("after_merge", string(*script)), ["issue", n, issue_state] => state.at_mut("issues").set(n, string(*issue_state)), ["labels", n, labels] => state.entry("labels", object([])).set(n, parse_json(labels)), + ["repo-labels", labels] => state.set("repo_labels", parse_json(labels)), ["user-email", email] => state.set("user_email", parse_json(email)), ["fails", call] => state .entry("failing", Array(Vec::new())) @@ -1256,6 +1346,18 @@ pub fn main(args: Vec<String>) { let (positional, flags) = parsed(rest); issue_close(&mut state, &positional, &flags); } + ["issue", "create", rest @ ..] => { + let (_, flags) = parsed(rest); + issue_create(&mut state, &flags); + } + ["label", "list", rest @ ..] => { + let (_, flags) = parsed(rest); + label_list(&state, &flags); + } + ["label", "create", rest @ ..] => { + let (positional, _) = parsed(rest); + label_create(&mut state, &positional); + } ["run", "list", rest @ ..] => { let (_, flags) = parsed(rest); run_list(&mut state, &flags); diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 29e75be50..72dfc6ae6 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -499,6 +499,45 @@ fn a_spec_pr_whose_red_check_also_fails_on_the_base_branch_commit_gets_no_repair assert_eq!(spec["isDraft"], true); } +#[test] +fn with_base_fix_the_spec_prs_inherited_failure_gets_a_base_fix_into_the_base_branch() { + let scenario = linear_spec(); + scenario.agent_does_for( + SPEC, + &format!("{}{}", checks_on_head(RED), checks_on_origin("main", RED)), + ); + // The Base fix issue is the next after the Tickets. + scenario.agent_does_for( + 24, + &format!( + r#" +echo "fixed" > ci-fix.txt +git add ci-fix.txt +git commit -q -m "Fix CI on main" +gh pr create --base main --head issue-24 --title "Fix CI on main" --body "Closes #24" +{}"#, + checks_on_head(GREEN) + ), + ); + + let result = scenario.run(&[&spec_url(&scenario), "base-fix"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_contains( + &result.stderr, + "thirdshift: starting Base fix #24 into main: https://github.com/acme/widgets/issues/24\n", + ); + let gh = scenario.gh_state(); + assert_eq!(gh["titles"]["24"], "CI red on main: test"); + let fix = pr_from(&scenario, "issue-24").expect("the Base fix's PR"); + assert_eq!(fix["base"], "main"); + assert_eq!(fix["state"], "MERGED"); + let spec = spec_pr(&scenario); + assert_eq!(spec["state"], "OPEN"); + assert_eq!(spec["isDraft"], false); + assert!(scenario.origin_file("issue-20", "ci-fix.txt").is_some()); +} + #[test] fn a_policy_refusal_on_the_spec_pr_leaves_it_ready_and_exits_1() { let scenario = linear_spec(); From 73ea24b6b788275bc47f28fe0fd43bbcbd908f2b Mon Sep 17 00:00:00 2001 From: Jacob Stephens <jacob@stephens.page> Date: Thu, 1 Oct 2026 02:03:55 +0000 Subject: [PATCH 2/2] Address review: Ticket Runs get base-fix, link the Base branch's checks, name the child Run's kind Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- README.md | 4 +-- src/args.rs | 39 ++++++++++------------- src/base_fix.rs | 73 +++++++++++++++++++++++++++++--------------- src/child_run.rs | 42 ++++++++++++++++++++++--- src/failed_run.rs | 4 --- src/main.rs | 27 +++++++--------- src/notification.rs | 11 +++---- src/run.rs | 28 +++++------------ src/spec_run.rs | 40 ++++++++++++++++-------- tests/base_fix.rs | 50 +++++++++++++++++++----------- tests/merge_run.rs | 41 ++++++++++--------------- tests/spec_run.rs | 47 ++++++++++++++++++++++++++++ tests/support/mod.rs | 8 +++++ 13 files changed, 256 insertions(+), 158 deletions(-) diff --git a/README.md b/README.md index 1114ec179..dcb1ac178 100644 --- a/README.md +++ b/README.md @@ -290,13 +290,13 @@ thirdshift base-fix https://github.com/acme/widgets/issues/7 When the Run's only red checks are Inherited failures and the Base branch has not moved since, it then starts a **Base fix**, at most one per Run ([ADR-0008](docs/adr/0008-inherited-failures-fail-the-run.md)): -1. thirdshift opens an issue from a fixed template, with no agent session: titled `CI red on <base>: <check>[, <check>…]`, its body naming the checks with their URLs, the Base branch and its short sha, and the Run's pull request, labelled `base-fix` and `ready-for-agent`. A label the repository lacks is added to it first. +1. thirdshift opens an issue from a fixed template, with no agent session: titled `CI red on <base>: <check>[, <check>…]`, its body naming the checks with their URLs on the Base branch, the Base branch and its short sha, and the Run's pull request, labelled `base-fix` and `ready-for-agent`. A label the repository lacks is added to it first. 2. It starts a child `thirdshift` on that issue from the same Launch directory, as a Merge run into the Run's Base branch, whatever the Run's own goal. Its progress lines are relayed with a `#<n>: ` prefix, after `starting Base fix #<n> into <base>: <issue URL>` and `waiting on Base fix #<n>`. The Base fix sends no Run notification, treats every red check as its own to fix rather than as an Inherited failure, and never starts a Base fix of its own. 3. Once the Base fix has merged, and its Self-merge has closed its issue, the Run merges the Base branch in again and watches CI, and goes on as usual: ready for review, or merged in a Merge run. If the Base fix fails, the Run is a [Failed run](#failed-runs) with the cause `Base fix <issue URL> failed: <its cause>`. If the checks are still Inherited failures once the Base fix has merged, there is no second one, and the cause is `CI red on <check>, which also fails on <base> at <short sha>, even after Base fix <issue URL> merged; fix <base> first`. The Run's own [Run notification](#run-notifications) has a `Base fix:` line with the issue's URL and whether it merged. -On a Spec, `base-fix` is for the Spec PR, whose Base fix goes into the Base branch. A Ticket's Run starts no Base fix: an Inherited failure from the Spec branch fails it. +On a Spec, `base-fix` goes to each Ticket's Run, whose Base fix goes into the Spec branch, its Base branch, and to the Spec PR, whose Base fix goes into the Base branch. Tickets running at once that meet the same Inherited failure each start a Base fix of their own. Without `base-fix`, an Inherited failure fails the Run as described in [What a Run does](#what-a-run-does). diff --git a/src/args.rs b/src/args.rs index d21f9980b..e8130eb60 100644 --- a/src/args.rs +++ b/src/args.rs @@ -5,6 +5,7 @@ use std::num::NonZeroUsize; use anyhow::{Context, Result, bail}; +use crate::child_run::Kind; use crate::issue::IssueUrl; use crate::notification::NotificationAsk; use crate::run::Goal; @@ -21,15 +22,11 @@ pub enum Command { } /// The hidden argument a Spec run starts each Ticket's Run with, followed by -/// the Spec branch: it makes the Run a Merge run into the Spec branch that -/// sends no Run notification and leaves the Launch directory alone. Not in -/// help. +/// the Spec branch: it makes the Run a [`Kind::Ticket`]. Not in help. pub const SPEC_BRANCH: &str = "--spec-branch"; /// The hidden argument a Run starts its Base fix with, followed by the Run's -/// Base branch: it makes the Base fix a Merge run into that branch that sends -/// no Run notification, leaves the Launch directory alone, sees no Inherited -/// failures and starts no Base fix of its own. Not in help. +/// Base branch: it makes the Run a [`Kind::BaseFix`]. Not in help. pub const BASE_FIX_INTO: &str = "--base-fix-into"; /// A Run's arguments. @@ -46,10 +43,9 @@ pub struct RunArgs { pub parallel: Option<NonZeroUsize>, /// Whether `base-fix` was given: the Run may start a Base fix. pub base_fix: bool, - /// The Spec branch, given with [`SPEC_BRANCH`] to a Ticket's Run. - pub spec_branch: Option<String>, - /// The Base branch, given with [`BASE_FIX_INTO`] to a Base fix. - pub base_fix_into: Option<String>, + /// What the Run is, if another thirdshift started it, given with + /// [`SPEC_BRANCH`] or [`BASE_FIX_INTO`]. + pub child: Option<Kind>, } /// Parse the arguments after the program name. `help`, `version`, `update`, @@ -84,8 +80,7 @@ pub fn parse(args: &[String]) -> Result<Command> { let mut email = None; let mut parallel = None; let mut base_fix = false; - let mut spec_branch = None; - let mut base_fix_into = None; + let mut child = None; let mut args = args.iter().peekable(); while let Some(arg) = args.next() { match arg.as_str() { @@ -117,17 +112,16 @@ pub fn parse(args: &[String]) -> Result<Command> { } base_fix = true; } - SPEC_BRANCH => { - if spec_branch.is_some() { + SPEC_BRANCH | BASE_FIX_INTO => { + if child.is_some() { bail!("repeated argument: {arg}"); } - spec_branch = Some(args.next().context("missing Spec branch")?.clone()); - } - BASE_FIX_INTO => { - if base_fix_into.is_some() { - bail!("repeated argument: {arg}"); - } - base_fix_into = Some(args.next().context("missing Base branch")?.clone()); + let base = args.next().context("missing Base branch")?.clone(); + child = Some(if arg == SPEC_BRANCH { + Kind::Ticket { spec_branch: base } + } else { + Kind::BaseFix { base } + }); } _ => { if issue.is_some() { @@ -146,8 +140,7 @@ pub fn parse(args: &[String]) -> Result<Command> { email, parallel, base_fix, - spec_branch, - base_fix_into, + child, })) } diff --git a/src/base_fix.rs b/src/base_fix.rs index 014c5a744..41221187c 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -4,10 +4,9 @@ use anyhow::{Result, bail}; -use crate::args; -use crate::child_run::{self, Ended}; +use crate::child_run::{self, Ended, Kind}; use crate::ci; -use crate::github::{self, Check}; +use crate::github::{self, Check, CheckState}; use crate::issue::IssueUrl; use crate::progress; @@ -20,7 +19,7 @@ const LABELS: [(&str, &str); 2] = [ /// What a Run does when its only red checks are Inherited failures. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Policy { +enum OnInheritedFailures { /// Fail, saying to fix the Base branch first. FailTheRun, /// Start a Base fix, once, and go on when it has merged. @@ -32,7 +31,7 @@ pub enum Policy { /// A Run's one Base fix: whether it may start one, and the one it started. pub struct BaseFix { - policy: Policy, + on_inherited_failures: OnInheritedFailures, started: Option<Started>, } @@ -43,9 +42,17 @@ struct Started { } impl BaseFix { - pub fn new(policy: Policy) -> Self { + /// The Base fix of a Run that is the child Run `child`, if it is one, + /// and that, with `asked`, was given `base-fix`. A Base fix starts none + /// of its own, whatever it was given. + pub fn new(child: Option<&Kind>, asked: bool) -> Self { + let on_inherited_failures = match (child, asked) { + (Some(Kind::BaseFix { .. }), _) => OnInheritedFailures::IsBaseFix, + (_, true) => OnInheritedFailures::StartBaseFix, + (_, false) => OnInheritedFailures::FailTheRun, + }; BaseFix { - policy, + on_inherited_failures, started: None, } } @@ -53,11 +60,17 @@ impl BaseFix { /// Whether the Run compares its red checks with the Base branch's, so /// that some may be Inherited failures. pub fn sees_inherited_failures(&self) -> bool { - self.policy != Policy::IsBaseFix + self.on_inherited_failures != OnInheritedFailures::IsBaseFix + } + + /// Whether the Run may start a Base fix, as may then each Ticket's Run + /// of a Spec run. + pub fn may_start(&self) -> bool { + self.on_inherited_failures == OnInheritedFailures::StartBaseFix } /// Fix the checks `inherited`, the only red ones on the PR `pr_url` of - /// the Run on `run` and all Inherited failures from `base` at + /// the Run on `issue` and all Inherited failures from `base` at /// `base_commit`, with a Base fix: write its issue, start it as a child /// `thirdshift` and wait for it to merge, after which the Run is to merge /// `base` in and watch CI again. Fails, with the Run's cause, if the Run @@ -66,7 +79,7 @@ impl BaseFix { /// which case the cause names that one's issue. pub fn fix( &mut self, - run: &IssueUrl, + issue: &IssueUrl, pr_url: &str, base: &str, base_commit: &str, @@ -81,25 +94,36 @@ impl BaseFix { started.issue.url ); } - if self.policy != Policy::StartBaseFix { + if !self.may_start() { bail!("CI red on {checks}, which also fails on {base} at {base_at}; fix {base} first"); } - let issue = github::create_issue( - run, + // The issue links the Base branch's own failed checks, not the PR's. + let on_base: Vec<Check> = github::checks_on(issue, base_commit)? + .into_iter() + .filter(|check| { + check.state == CheckState::Failed + && inherited.iter().any(|failed| failed.name == check.name) + }) + .collect(); + let fix = github::create_issue( + issue, &format!("CI red on {base}: {checks}"), - &issue_body(run, pr_url, base, base_at, inherited), + &issue_body(issue, pr_url, base, base_at, &on_base), &LABELS, )?; - let number = issue.number; + let number = fix.number; progress::step(format_args!( "starting Base fix #{number} into {base}: {}", - issue.url + fix.url )); let started = self.started.insert(Started { - issue, + issue: fix, merged: false, }); - let child = child_run::start(&started.issue, [args::BASE_FIX_INTO, base])?; + let kind = Kind::BaseFix { + base: base.to_string(), + }; + let child = child_run::start(&started.issue, &kind, false)?; progress::step(format_args!("waiting on Base fix #{number}")); match child_run::wait(number, child)? { Ended::Reached(_) => { @@ -133,18 +157,17 @@ impl BaseFix { } /// The body of the Base fix issue, from a fixed template: the checks -/// `inherited` with their URLs, the Base branch `base` and its commit -/// `base_at`, shortened, and the Run that found them, on `run` with the PR -/// `pr_url`. +/// `failed` on the Base branch `base` at its commit `base_at`, shortened, with +/// their URLs, and the Run that found them, on `issue` with the PR `pr_url`. fn issue_body( - run: &IssueUrl, + issue: &IssueUrl, pr_url: &str, base: &str, base_at: &str, - inherited: &[Check], + failed: &[Check], ) -> String { let mut checks = String::new(); - for check in inherited { + for check in failed { checks += &match &check.url { Some(url) => format!("- {}: {url}\n", check.name), None => format!("- {}\n", check.name), @@ -158,6 +181,6 @@ fn issue_body( Found by the thirdshift Run on #{}, whose pull request is {pr_url}.\n\ \n\ Fix the checks on `{base}`. Do not skip, disable, or weaken tests or checks to make them pass.\n", - run.number + issue.number ) } diff --git a/src/child_run.rs b/src/child_run.rs index a984004ef..fe16209e2 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -9,6 +9,7 @@ use std::time::Duration; use anyhow::{Context, Result, anyhow}; +use crate::args; use crate::failed_run; use crate::interrupt; use crate::issue::IssueUrl; @@ -17,6 +18,36 @@ use crate::progress; /// How often to check whether a child Run has ended, or been interrupted. const POLL: Duration = Duration::from_millis(100); +/// What a child Run is, with the Base branch it is given: a Merge run into +/// that branch that sends no Run notification and leaves the Launch directory +/// alone. +#[derive(Debug, PartialEq, Eq)] +pub enum Kind { + /// A Ticket's Run in a Spec run, into the Spec branch. + Ticket { spec_branch: String }, + /// A Base fix, into the Base branch of the Run that started it. It sees + /// no Inherited failures and starts no Base fix of its own. + BaseFix { base: String }, +} + +impl Kind { + /// The Base branch the child Run is given. + pub fn base(&self) -> &str { + match self { + Kind::Ticket { spec_branch } => spec_branch, + Kind::BaseFix { base } => base, + } + } + + /// The hidden argument that says which kind the child Run is. + fn hidden_argument(&self) -> &'static str { + match self { + Kind::Ticket { .. } => args::SPEC_BRANCH, + Kind::BaseFix { .. } => args::BASE_FIX_INTO, + } + } +} + /// How a child Run ended. pub enum Ended { /// It reached its goal, with its PR as it printed it. @@ -27,12 +58,13 @@ pub enum Ended { Interrupted, } -/// Start a Run on `issue` in a child `thirdshift`, from the same Launch -/// directory, given `hidden`: the argument that says what started it, and the -/// Base branch that goes with it. -pub fn start(issue: &IssueUrl, hidden: [&str; 2]) -> Result<Child> { +/// Start a Run of `kind` on `issue` in a child `thirdshift`, from the same +/// Launch directory. With `base_fix`, it is given `base-fix`, so it may start +/// a Base fix. +pub fn start(issue: &IssueUrl, kind: &Kind, base_fix: bool) -> Result<Child> { Command::new(std::env::current_exe().context("no thirdshift executable")?) - .args(hidden) + .args([kind.hidden_argument(), kind.base()]) + .args(base_fix.then_some("base-fix")) .arg(&issue.url) .stdin(Stdio::null()) .stdout(Stdio::piped()) diff --git a/src/failed_run.rs b/src/failed_run.rs index 4f0a074aa..901934a81 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -29,8 +29,6 @@ pub struct FailedRun { /// In a Spec run, a line on each Ticket it landed or did not get done; /// empty in a Run. pub ticket_lines: Vec<String>, - /// What became of the Base fix it started, if it started one. - pub base_fix: Option<String>, } /// A merge GitHub refused when a round of the Repair loop found nothing to @@ -57,7 +55,6 @@ impl From<anyhow::Error> for FailedRun { log: None, interrupted, ticket_lines: Vec::new(), - base_fix: None, } } } @@ -124,7 +121,6 @@ pub fn fail( log: log.exists().then(|| log.to_path_buf()), interrupted, ticket_lines: Vec::new(), - base_fix: None, } } diff --git a/src/main.rs b/src/main.rs index 36c0aae16..666a7c33d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -31,7 +31,8 @@ use std::io::Write; use std::process::ExitCode; use args::{Command, RunArgs}; -use base_fix::Policy; +use base_fix::BaseFix; +use child_run::Kind; use config::UserConfig; use notification::{NotificationAsk, RunNotification}; use run::Goal; @@ -129,8 +130,7 @@ fn main() -> ExitCode { email, parallel, base_fix, - spec_branch, - base_fix_into, + child, } = match args::parse(&args) { Ok(Command::Help) => { print!("{HELP}"); @@ -157,16 +157,10 @@ fn main() -> ExitCode { return ExitCode::FAILURE; } }; - let inherited_failures = match (&base_fix_into, base_fix) { - (Some(_), _) => Policy::IsBaseFix, - (None, true) => Policy::StartBaseFix, - (None, false) => Policy::FailTheRun, - }; - // A Ticket's Run in a Spec run, like a Base fix, is always a Merge run - // into the Base branch it was given, and leaves the Run notification and - // the Launch directory to what started it. - let given_base = spec_branch.or(base_fix_into); - let (goal, email, launch_pull) = match given_base { + // A child Run, a Ticket's Run in a Spec run or a Base fix, is always a + // Merge run, and leaves the Run notification and the Launch directory to + // what started it. + let (goal, email, launch_pull) = match child { Some(_) => (Goal::Merged, NotificationAsk::Skip, false), None => ( goal.unwrap_or(config.default_goal()), @@ -190,6 +184,7 @@ fn main() -> ExitCode { return ExitCode::FAILURE; } }; + let mut base_fix = BaseFix::new(child.as_ref(), base_fix); let parallel = Parallel { tickets: parallel.unwrap_or(config.spec_parallel), asked: parallel.is_some(), @@ -200,8 +195,8 @@ fn main() -> ExitCode { &config.logs_dir, launch_pull, parallel, - given_base.as_deref(), - inherited_failures, + child.as_ref().map(Kind::base), + &mut base_fix, ); let code = match &ended { Ok(reached) => { @@ -226,7 +221,7 @@ fn main() -> ExitCode { } }; if let Some(notification) = notification { - notification.send(&ended); + notification.send(&ended, base_fix.report().as_deref()); } code } diff --git a/src/notification.rs b/src/notification.rs index d78456dac..900bed8ce 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -54,17 +54,17 @@ impl RunNotification { }) } - /// Send the notification for the Run that `ended`. A failed send is only - /// a warning: it never changes the Run's outcome. - pub fn send(self, ended: &Result<Reached, FailedRun>) { - let (outcome, pr_url, cause, log, tickets, base_fix) = match ended { + /// Send the notification for the Run that `ended`, with `base_fix`, what + /// became of the Base fix it started, if it started one. A failed send is + /// only a warning: it never changes the Run's outcome. + pub fn send(self, ended: &Result<Reached, FailedRun>, base_fix: Option<&str>) { + let (outcome, pr_url, cause, log, tickets) = match ended { Ok(reached) => ( reached.goal.outcome(), Some(reached.pr_url.as_str()), None, reached.log.as_deref(), &reached.ticket_lines[..], - reached.base_fix.as_deref(), ), Err(failed) => { let (outcome, cause) = if failed.interrupted { @@ -78,7 +78,6 @@ impl RunNotification { cause, failed.log.as_deref(), &failed.ticket_lines[..], - failed.base_fix.as_deref(), ) } }; diff --git a/src/run.rs b/src/run.rs index f14b966ef..de260f531 100644 --- a/src/run.rs +++ b/src/run.rs @@ -5,7 +5,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; -use crate::base_fix::{BaseFix, Policy}; +use crate::base_fix::BaseFix; use crate::branch::{self, Selection}; use crate::ci::{self, Ci}; use crate::failed_run::{self, FailedRun, PolicyRefusal}; @@ -49,8 +49,6 @@ pub struct Reached { pub log: Option<PathBuf>, /// In a Spec run, a line on each Ticket it landed; empty in a Run. pub ticket_lines: Vec<String>, - /// What became of the Base fix it started, if it started one. - pub base_fix: Option<String>, } /// Take `issue` to a ready PR, or in a Merge run a merged one. Any failure @@ -70,8 +68,8 @@ pub struct Reached { /// an issue with no sub-issues fails before any work, as does a Spec whose /// Tickets are all closed with no Spec branch to continue. /// -/// `inherited_failures` says what the Run, or a Spec run on its Spec PR, does -/// when its only red checks are Inherited failures. +/// `base_fix` is the one Base fix the Run, or a Spec run for its Spec PR, may +/// start when its only red checks are Inherited failures. pub fn run( issue: &IssueUrl, goal: Goal, @@ -79,7 +77,7 @@ pub fn run( launch_pull: bool, parallel: Parallel, given_base: Option<&str>, - inherited_failures: Policy, + base_fix: &mut BaseFix, ) -> Result<Reached, FailedRun> { let timestamp = chrono::Utc::now().format("%Y%m%dT%H%M%SZ").to_string(); let launch = Git::new(std::env::current_dir().context("no current directory")?); @@ -130,7 +128,6 @@ pub fn run( dir: logs_dir, timestamp: ×tamp, }; - let mut base_fix = BaseFix::new(inherited_failures); if !tickets.is_empty() { return spec_run::run( issue, @@ -138,7 +135,7 @@ pub fn run( worktree, &base, goal, - &mut base_fix, + base_fix, &logs, parallel.tickets, ); @@ -151,14 +148,7 @@ pub fn run( }; let mut log = logs.path("implement"); let implemented = implement( - issue, - &worktree, - &base, - &prompt, - goal, - &mut base_fix, - &logs, - &mut log, + issue, &worktree, &base, &prompt, goal, base_fix, &logs, &mut log, ); match implemented { Ok(pr_url) => Ok(Reached { @@ -166,12 +156,8 @@ pub fn run( goal, log: Some(log), ticket_lines: Vec::new(), - base_fix: base_fix.report(), - }), - Err(error) => Err(FailedRun { - base_fix: base_fix.report(), - ..failed_run::fail(issue, worktree, &base, &log, error) }), + Err(error) => Err(failed_run::fail(issue, worktree, &base, &log, error)), } } diff --git a/src/spec_run.rs b/src/spec_run.rs index f65e06b6c..2a346955e 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -14,9 +14,8 @@ use std::thread; use anyhow::{Context, Result, bail}; -use crate::args; use crate::base_fix::BaseFix; -use crate::child_run::{self, Ended}; +use crate::child_run::{self, Ended, Kind}; use crate::failed_run::{self, FailedRun}; use crate::github::{self, PullRequest, Ticket}; use crate::interrupt; @@ -70,7 +69,8 @@ pub fn all_closed(tickets: &[Ticket]) -> bool { /// cleaned up when this returns, or kept by the Failed run path if its work /// did not reach origin. However it ends, it carries a line on each Ticket /// it landed or did not get done, for the Run notification. `base_fix` is for -/// the Spec PR's Repair loop: a Ticket's Run starts no Base fix. +/// the Spec PR's Repair loop, and if it may start a Base fix, so may each +/// Ticket's Run, into the Spec branch. #[allow(clippy::too_many_arguments)] pub fn run( spec: &IssueUrl, @@ -83,8 +83,15 @@ pub fn run( parallel: NonZeroUsize, ) -> Result<Reached, FailedRun> { let mut spec_pr = draft_spec_pr(spec, worktree.branch())?; - let (ticket_lines, landed) = - land_tickets(spec, tickets, &worktree, base, parallel, &mut spec_pr); + let (ticket_lines, landed) = land_tickets( + spec, + tickets, + &worktree, + base, + parallel, + base_fix.may_start(), + &mut spec_pr, + ); let ended = match landed { Ok(checklist) => open_and_review( spec, worktree, base, spec_pr, &checklist, goal, base_fix, logs, @@ -97,12 +104,10 @@ pub fn run( match ended { Ok(reached) => Ok(Reached { ticket_lines, - base_fix: base_fix.report(), ..reached }), Err(failed) => Err(FailedRun { ticket_lines, - base_fix: base_fix.report(), ..failed }), } @@ -136,7 +141,6 @@ fn open_and_review( goal, log: Some(log), ticket_lines: Vec::new(), - base_fix: None, }), Err(error) => { // The Spec review may have rewritten the body without it. @@ -166,12 +170,14 @@ enum TicketOutcome { /// not done, none if the Spec branch could not be pushed, and the last /// Tickets checklist if every Ticket is done: if not, it fails, after putting /// those lines on stderr unless an interrupt or another error ended it first. +/// With `base_fix`, each Ticket's Run may start a Base fix. fn land_tickets( spec: &IssueUrl, mut tickets: Vec<Ticket>, worktree: &Worktree, base: &str, parallel: NonZeroUsize, + base_fix: bool, spec_pr: &mut Option<PullRequest>, ) -> (Vec<String>, Result<String>) { if let Err(error) = worktree.push() { @@ -185,6 +191,7 @@ fn land_tickets( worktree.branch(), base, parallel, + base_fix, spec_pr, ); let lines = summarize(&tickets, &outcomes); @@ -210,7 +217,8 @@ fn land_tickets( /// Ticket's outcome in `outcomes`, and the Tickets checklist of `spec_pr`, /// opened into `base` once a Ticket lands, up to date. An interrupt, or an /// error other than a Ticket failing, stops any more from starting, and -/// fails this once those running have ended. +/// fails this once those running have ended. With `base_fix`, each Ticket's +/// Run may start a Base fix. #[allow(clippy::too_many_arguments)] fn run_ready_tickets( spec: &IssueUrl, @@ -219,6 +227,7 @@ fn run_ready_tickets( spec_branch: &str, base: &str, parallel: NonZeroUsize, + base_fix: bool, spec_pr: &mut Option<PullRequest>, ) -> Result<()> { let (ended, endings) = mpsc::channel(); @@ -230,7 +239,7 @@ fn run_ready_tickets( let Some(ticket) = next_ready(tickets, outcomes, &running) else { break; }; - match start_ticket(spec, ticket, spec_branch, ended.clone()) { + match start_ticket(spec, ticket, spec_branch, base_fix, ended.clone()) { Ok(()) => { running.insert(ticket); outcomes.insert(ticket, TicketOutcome::Running); @@ -468,16 +477,21 @@ fn cycle_through(start: u64, tickets: &[Ticket]) -> Option<Vec<u64>> { } /// Start Ticket `number` of `spec` as a child `thirdshift`, a Merge run into -/// `spec_branch` from the same Launch directory, and a thread that sends -/// `number` and how it ended on `ended`. +/// `spec_branch` from the same Launch directory that, with `base_fix`, may +/// start a Base fix, and a thread that sends `number` and how it ended on +/// `ended`. fn start_ticket( spec: &IssueUrl, number: u64, spec_branch: &str, + base_fix: bool, ended: Sender<(u64, Result<TicketOutcome>)>, ) -> Result<()> { progress::step(format_args!("starting #{number}")); - let child = child_run::start(&spec.sibling(number), [args::SPEC_BRANCH, spec_branch])?; + let kind = Kind::Ticket { + spec_branch: spec_branch.to_string(), + }; + let child = child_run::start(&spec.sibling(number), &kind, base_fix)?; thread::spawn(move || { let _ = ended.send((number, finish_ticket(number, child))); }); diff --git a/tests/base_fix.rs b/tests/base_fix.rs index fe8c1693b..4dc4136e6 100644 --- a/tests/base_fix.rs +++ b/tests/base_fix.rs @@ -17,7 +17,7 @@ git add feature.txt git commit -q -m "Add feature" gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" gh fake checks "$(git rev-parse HEAD)" '[{"name": "test", "conclusion": "failure", "url": "https://ci.example/test"}]' -gh fake checks "$(git rev-parse origin/main)" '[{"name": "test", "conclusion": "failure", "url": "https://ci.example/test"}]' +gh fake checks "$(git rev-parse origin/main)" '[{"name": "test", "conclusion": "failure", "url": "https://ci.example/main/test"}]' "#; /// The agent, on the Base fix issue #8, commits a fix and opens its PR into @@ -33,16 +33,10 @@ const GREEN_ON_HEAD: &str = "gh fake checks \"$(git rev-parse HEAD)\" '[{\"name\ const RED_ON_HEAD: &str = "gh fake checks \"$(git rev-parse HEAD)\" '[{\"name\": \"test\", \"conclusion\": \"failure\"}]'\n"; -const BASE_FIX_URL: &str = "https://github.com/acme/widgets/issues/8"; +/// Resend's reply to an email it accepted. +const ACCEPTED: &str = r#"{"id":"49a3999c-0ce1-4ea6-ab68-afcd6dc2e794"}"#; -/// Every `gh <command> <subcommand>` call made, e.g. `gh issue create`. -fn gh_calls_of(scenario: &Scenario, command: &str, subcommand: &str) -> Vec<Vec<String>> { - scenario - .gh_calls() - .into_iter() - .filter(|call| call.starts_with(&[command.to_string(), subcommand.to_string()])) - .collect() -} +const BASE_FIX_URL: &str = "https://github.com/acme/widgets/issues/8"; /// The prompt of each session, in order. fn prompts(scenario: &Scenario) -> Vec<String> { @@ -71,7 +65,7 @@ fn with_base_fix_an_inherited_failure_gets_an_issue_a_merged_fix_and_the_run_fin format!( "CI is red on `main` at {}: these checks fail there, so every pull request into `main` inherits them.\n\ \n\ - - test: https://ci.example/test\n\ + - test: https://ci.example/main/test\n\ \n\ Found by the thirdshift Run on #7, whose pull request is https://github.com/acme/widgets/pull/1.\n\ \n\ @@ -122,7 +116,7 @@ fn the_base_fix_repairs_the_check_as_its_own_and_only_the_run_sends_a_notificati "echo more > more.txt\ngit add more.txt\ngit commit -q -m 'Fix more'\n{GREEN_ON_HEAD}" ), ); - let resend = ResendStandIn::replying(200, r#"{"id":"49a3999c-0ce1-4ea6-ab68-afcd6dc2e794"}"#); + let resend = ResendStandIn::replying(200, ACCEPTED); let result = scenario.run_with_env( &[ @@ -153,7 +147,7 @@ fn the_base_fix_repairs_the_check_as_its_own_and_only_the_run_sends_a_notificati result.stderr ); assert_eq!(scenario.gh_state()["prs"][1]["state"], "MERGED"); - assert_eq!(gh_calls_of(&scenario, "issue", "create").len(), 1); + assert_eq!(scenario.gh_calls_of("issue", "create").len(), 1); // One Run notification: the Run's, which reports the Base fix. let requests = resend.requests(); assert_eq!(requests.len(), 1, "{requests:?}"); @@ -224,8 +218,20 @@ fn a_failing_base_fix_fails_the_run_naming_its_issue() { let scenario = Scenario::new(); scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); scenario.agent_does_for(8, "exit 3\n"); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); + let result = scenario.run_with_env( + &[ + &scenario.issue_url(7), + "base-fix", + "email", + "me@example.com", + ], + &[ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", "re_test_123"), + ], + ); assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); @@ -248,6 +254,14 @@ fn a_failing_base_fix_fails_the_run_naming_its_issue() { scenario.origin_log("issue-7") ); scenario.assert_cleaned_up("issue-7"); + let requests = resend.requests(); + assert_eq!(requests.len(), 1, "{requests:?}"); + let text = requests[0].body["text"].as_str().unwrap(); + assert!( + text.contains(&cause) + && text.contains(&format!("Base fix: {BASE_FIX_URL} not merged\n")), + "text: {text}" + ); } #[test] @@ -277,7 +291,7 @@ fn checks_still_inherited_failures_after_the_base_fix_merged_fail_the_run_with_n assert!(result.stderr.contains(&cause), "stderr: {}", result.stderr); let gh = scenario.gh_state(); assert_eq!(gh["prs"][1]["state"], "MERGED"); - assert_eq!(gh_calls_of(&scenario, "issue", "create").len(), 1); + assert_eq!(scenario.gh_calls_of("issue", "create").len(), 1); assert_eq!(gh["issues"].as_object().unwrap().len(), 2, "#7 and #8"); assert_eq!(gh["prs"][0]["isDraft"], true); assert_eq!(prompts(&scenario).len(), 2, "no Repair"); @@ -300,8 +314,8 @@ fn without_base_fix_an_inherited_failure_fails_the_run_and_writes_no_issue() { "stderr: {}", result.stderr ); - assert!(gh_calls_of(&scenario, "issue", "create").is_empty()); - assert!(gh_calls_of(&scenario, "label", "list").is_empty()); + assert!(scenario.gh_calls_of("issue", "create").is_empty()); + assert!(scenario.gh_calls_of("label", "list").is_empty()); assert_eq!(scenario.gh_state()["issues"].as_object().unwrap().len(), 1); } @@ -317,7 +331,7 @@ fn a_label_the_repository_lacks_is_added_and_one_it_has_is_left_alone() { let result = scenario.run(&[&scenario.issue_url(7), "base-fix"]); assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - let created = gh_calls_of(&scenario, "label", "create"); + let created = scenario.gh_calls_of("label", "create"); assert_eq!(created.len(), 1, "{created:?}"); assert_eq!(created[0][2], "base-fix"); assert_eq!( diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 2e57c98b1..25f8a4520 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -18,15 +18,6 @@ gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" const PR_URL: &str = "https://github.com/acme/widgets/pull/1"; -/// Every `gh <command> <subcommand>` call made, e.g. `gh pr merge`. -fn gh_calls_of(scenario: &Scenario, command: &str, subcommand: &str) -> Vec<Vec<String>> { - scenario - .gh_calls() - .into_iter() - .filter(|call| call.starts_with(&[command.to_string(), subcommand.to_string()])) - .collect() -} - /// Launch from `develop`, a branch other than the default, and have the agent /// open its PR into it, then run `then`. fn merge_into_develop(scenario: &Scenario, then: &str) { @@ -43,7 +34,7 @@ const CLOSING_COMMENT: &str = "Closed by #1, merged into develop by a thirdshift /// Assert that `base`'s tip on origin is a merge commit of the head commit /// thirdshift asked `gh pr merge` to match, and return that head. fn assert_issue_7_merged_into(scenario: &Scenario, base: &str) -> String { - let calls = gh_calls_of(scenario, "pr", "merge"); + let calls = scenario.gh_calls_of("pr", "merge"); let head = calls .last() .and_then(|call| call.last()) @@ -111,7 +102,7 @@ fn an_issue_already_closed_after_the_merge_is_not_closed_again() { assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); assert_eq!(scenario.gh_state()["issues"]["7"], "CLOSED"); - assert!(gh_calls_of(&scenario, "issue", "close").is_empty()); + assert!(scenario.gh_calls_of("issue", "close").is_empty()); assert_eq!(scenario.gh_state()["comments"], serde_json::Value::Null); } @@ -150,7 +141,7 @@ fn the_merge_is_a_merge_commit_of_exactly_the_watched_head() { result.stderr ); assert_eq!( - gh_calls_of(&scenario, "pr", "merge"), + scenario.gh_calls_of("pr", "merge"), vec![vec![ "pr", "merge", @@ -202,7 +193,7 @@ fn a_run_without_merge_leaves_the_pr_open_and_ready() { result.stderr.lines().last(), Some(format!("thirdshift: PR {PR_URL} is ready for review").as_str()) ); - assert!(gh_calls_of(&scenario, "pr", "merge").is_empty()); + assert!(scenario.gh_calls_of("pr", "merge").is_empty()); let pr = &scenario.gh_state()["prs"][0]; assert_eq!(pr["state"], "OPEN"); assert_eq!(pr["isDraft"], false); @@ -374,7 +365,7 @@ fn ctrl_c_before_the_merge_is_a_failed_run() { "stderr: {}", result.stderr ); - assert!(gh_calls_of(&scenario, "pr", "merge").is_empty()); + assert!(scenario.gh_calls_of("pr", "merge").is_empty()); assert_eq!(scenario.gh_state()["prs"][0]["state"], "OPEN"); assert_eq!(scenario.origin_log("main").unwrap(), vec!["Initial commit"]); assert_eq!(scenario.gh_state()["issues"]["7"], "OPEN"); @@ -478,7 +469,7 @@ fn a_foreign_commit_pushed_during_ci_is_merged_in_reviewed_then_merged() { result.stderr ); // Found before the merge was tried, so the one merge is of the reviewed head. - assert_eq!(gh_calls_of(&scenario, "pr", "merge").len(), 1); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 1); let head = assert_issue_7_merged_into(&scenario, "main"); assert_eq!(head, late); assert_eq!(scenario.origin_file("main", "late.txt").unwrap(), "late\n"); @@ -511,7 +502,7 @@ fn a_foreign_commit_that_lands_as_the_merge_is_tried_is_reviewed_then_merged() { let calls = scenario.claude_calls(); assert_eq!(calls.len(), 2); assert_review_repair_from(&calls[1]["prompt"], &parent_of(&scenario, &late)); - assert_eq!(gh_calls_of(&scenario, "pr", "merge").len(), 2); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 2); assert_eq!(assert_issue_7_merged_into(&scenario, "main"), late); } @@ -698,7 +689,7 @@ fn foreign_commits_that_keep_coming_spend_the_upstream_move_budget() { 6, "the implement session and a review Repair for each of 5 rounds" ); - assert!(gh_calls_of(&scenario, "pr", "merge").is_empty()); + assert!(scenario.gh_calls_of("pr", "merge").is_empty()); assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); } @@ -742,7 +733,7 @@ fn a_review_repair_counts_against_the_repair_cap() { result.stderr ); assert_eq!(scenario.claude_calls().len(), 6); - assert!(gh_calls_of(&scenario, "pr", "merge").is_empty()); + assert!(scenario.gh_calls_of("pr", "merge").is_empty()); } #[test] @@ -844,7 +835,7 @@ fn a_merge_that_fails_on_a_base_branch_conflict_gets_a_conflict_repair_then_merg let calls = scenario.claude_calls(); assert_eq!(calls.len(), 2); assert_eq!(calls[1]["merging"], true); - assert_eq!(gh_calls_of(&scenario, "pr", "merge").len(), 2); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 2); assert_eq!(scenario.gh_state()["prs"][0]["state"], "MERGED"); assert_issue_7_merged_into(&scenario, "main"); assert_eq!( @@ -888,8 +879,8 @@ fn a_merge_that_fails_then_finds_red_ci_on_a_new_head_gets_a_ci_fix_repair_then_ calls[1]["prompt"] ); let head = assert_issue_7_merged_into(&scenario, "main"); - assert_eq!(gh_calls_of(&scenario, "pr", "merge").len(), 2); - assert_eq!(gh_calls_of(&scenario, "pr", "merge")[1].last(), Some(&head)); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 2); + assert_eq!(scenario.gh_calls_of("pr", "merge")[1].last(), Some(&head)); assert!( scenario .origin_log("main") @@ -923,7 +914,7 @@ fn merge_failures_that_keep_moving_the_base_branch_spend_the_base_move_budget() "stderr: {}", result.stderr ); - assert_eq!(gh_calls_of(&scenario, "pr", "merge").len(), 6); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 6); let pr = &scenario.gh_state()["prs"][0]; assert_eq!(pr["state"], "OPEN"); assert_eq!( @@ -961,7 +952,7 @@ fn a_repair_needed_after_a_failed_merge_counts_against_the_repair_cap() { result.stderr ); assert_eq!(scenario.claude_calls().len(), 6); - assert_eq!(gh_calls_of(&scenario, "pr", "merge").len(), 1); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 1); assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); } @@ -998,7 +989,7 @@ fn a_merge_refused_with_nothing_left_to_fix_leaves_the_pr_ready_for_review() { .origin_git(&["rev-parse", "refs/heads/issue-7"]) .trim() .to_string(); - assert_eq!(gh_calls_of(&scenario, "pr", "merge")[0].last(), Some(&head)); + assert_eq!(scenario.gh_calls_of("pr", "merge")[0].last(), Some(&head)); assert_eq!(scenario.origin_log("main").unwrap(), vec!["Initial commit"]); assert_eq!(scenario.gh_state()["issues"]["7"], "OPEN"); scenario.assert_cleaned_up("issue-7"); @@ -1023,7 +1014,7 @@ fn a_merge_run_whose_red_checks_also_fail_on_the_base_branch_commit_does_not_mer ); assert!(result.stderr.contains(&cause), "stderr: {}", result.stderr); assert_eq!(scenario.claude_calls().len(), 1, "the implement session"); - assert!(gh_calls_of(&scenario, "pr", "merge").is_empty()); + assert!(scenario.gh_calls_of("pr", "merge").is_empty()); let gh = scenario.gh_state(); assert_eq!(gh["prs"][0]["state"], "OPEN"); assert_eq!(gh["prs"][0]["isDraft"], true); diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 72dfc6ae6..69979b00f 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -538,6 +538,53 @@ gh pr create --base main --head issue-24 --title "Fix CI on main" --body "Closes assert!(scenario.origin_file("issue-20", "ci-fix.txt").is_some()); } +#[test] +fn with_base_fix_a_tickets_inherited_failure_gets_a_base_fix_into_the_spec_branch() { + let scenario = spec_of(&[(21, &[])]); + scenario.agent_does_for( + 21, + &format!( + "{}{}{}", + agent_lands(21, "first.txt"), + checks_on_head(RED), + checks_on_origin("issue-20", RED) + ), + ); + // The Base fix issue is the next after the Ticket. + scenario.agent_does_for( + 22, + &format!( + r#" +echo "fixed" > ci-fix.txt +git add ci-fix.txt +git commit -q -m "Fix CI on the Spec branch" +gh pr create --base issue-20 --head issue-22 --title "Fix CI" --body "Closes #22" +{}"#, + checks_on_head(GREEN) + ), + ); + + let result = scenario.run(&[&spec_url(&scenario), "base-fix"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_contains( + &result.stderr, + "#21: starting Base fix #22 into issue-20: https://github.com/acme/widgets/issues/22\n", + ); + assert_eq!( + scenario.gh_state()["titles"]["22"], + "CI red on issue-20: test" + ); + let fix = pr_from(&scenario, "issue-22").expect("the Base fix's PR"); + assert_eq!(fix["base"], "issue-20"); + assert_eq!(fix["state"], "MERGED"); + assert_eq!(pr_from(&scenario, "issue-21").unwrap()["state"], "MERGED"); + for file in ["first.txt", "ci-fix.txt"] { + assert!(scenario.origin_file("issue-20", file).is_some(), "{file}"); + } + assert_eq!(spec_pr(&scenario)["isDraft"], false); +} + #[test] fn a_policy_refusal_on_the_spec_pr_leaves_it_ready_and_exits_1() { let scenario = linear_spec(); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 24151a725..534b3a173 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -534,6 +534,14 @@ impl Scenario { } } + /// Every `gh <command> <subcommand>` call made, e.g. `gh pr merge`. + pub fn gh_calls_of(&self, command: &str, subcommand: &str) -> Vec<Vec<String>> { + self.gh_calls() + .into_iter() + .filter(|call| call.starts_with(&[command.to_string(), subcommand.to_string()])) + .collect() + } + /// Push `branch` to origin: `from` plus one commit per subject in /// `commits`, oldest first. pub fn origin_has_branch(&self, branch: &str, from: &str, commits: &[&str]) {