From 29917ee3357345c293f724a77a3a3fed1d4c85be Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sun, 4 Oct 2026 19:50:53 -0400 Subject: [PATCH 1/2] Test the Spec run's Ticket loop in-process behind a seam for its outside effects (#380) The Ticket loop now reaches the Spec branch push, the Tickets' child Runs and their channel, the graph re-read, the Spec PR, the interrupt and its progress lines only through a private Outside seam, with a production adapter over child Runs, GitHub and the Spec PR and a scripted fake for unit tests driving a real Ticket board. End-to-end tests that only checked a loop rule or a board line through the loop are replaced by unit tests. Co-Authored-By: Claude Opus 5.5 --- src/spec_run.rs | 903 ++++++++++++++++++++++++++++++++++++++++++---- tests/spec_run.rs | 210 ----------- 2 files changed, 825 insertions(+), 288 deletions(-) diff --git a/src/spec_run.rs b/src/spec_run.rs index 0a06b8dcb..93e26406d 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -8,7 +8,7 @@ mod spec_pr; mod ticket_board; use std::num::NonZeroUsize; -use std::sync::mpsc::{self, Sender}; +use std::sync::mpsc::{self, Receiver, Sender}; use std::thread; use anyhow::{Result, bail}; @@ -65,14 +65,17 @@ pub fn run( ) -> Result { let (spec, base) = (delivery.issue, delivery.base); let mut spec_pr = SpecPr::resume(spec, worktree.branch(), base)?; - let (ticket_lines, landed) = land_tickets( + let base_fix = delivery.base_fix.ask_of_tickets(); + let (ended, endings) = mpsc::channel(); + let mut outside = ChildRunsAndGitHub { spec, - tickets, - &worktree, - parallel, - &delivery.base_fix.ask_of_tickets(), - &mut spec_pr, - ); + worktree: &worktree, + base_fix: &base_fix, + spec_pr: &mut spec_pr, + ended, + endings, + }; + let (ticket_lines, landed) = land_tickets(&mut outside, tickets, parallel); let ended = match landed { Ok(checklist) => review_and_deliver(worktree, spec_pr, &checklist, delivery), Err(error) => Err(FailedRun { @@ -116,28 +119,121 @@ fn review_and_deliver( delivered } -/// Push the Spec branch, then keep up to `parallel` Tickets running, each -/// time one ends starting ready ones, lowest number first, until none is -/// ready and none is running, telling `spec_pr` the Tickets checklist as -/// Tickets start and end, and when a Ticket lands. Returns a line on each -/// Ticket that landed or is 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. -/// Each Ticket's Run may start a Base fix if `base_fix` allows one. +/// What the Spec run's Ticket loop does or reads outside itself: the push of +/// the Spec branch, the Tickets' child Runs, the graph read again, the Spec +/// PR, the interrupt and its progress lines. +trait Outside { + /// Push the Spec branch to origin. + fn push(&mut self) -> Result<()>; + /// Start Ticket `number`'s Run, a Merge run into the Spec branch. + fn start_ticket(&mut self, number: u64) -> Result<()>; + /// Wait for the next running Ticket's Run to end: its number and how it + /// ended. + fn next_ending(&mut self) -> (u64, Result); + /// The Spec's Tickets, read again. + fn tickets(&mut self) -> Result>; + /// Show `checklist` as the Spec PR's Tickets checklist, if it is open. + fn show(&mut self, checklist: &str); + /// A Ticket landed: open the Spec PR as a draft with `checklist`, or + /// show it. + fn landed(&mut self, checklist: &str) -> Result<()>; + /// Whether the Spec run was interrupted. + fn interrupted(&mut self) -> bool; + /// Write the progress line `line`. + fn step(&mut self, line: String); +} + +/// The outside world of the Ticket loop of a Spec run on `spec`, from its +/// Spec branch checked out in `worktree`: child `thirdshift` Runs, each of +/// which may start a Base fix if `base_fix` allows one, whose threads send +/// how each ended on `ended`, received from `endings`, GitHub and the Spec +/// PR. +struct ChildRunsAndGitHub<'a, 'pr> { + spec: &'a IssueUrl, + worktree: &'a Worktree, + base_fix: &'a BaseFixAsk, + spec_pr: &'a mut SpecPr<'pr>, + ended: Sender<(u64, Result)>, + endings: Receiver<(u64, Result)>, +} + +impl Outside for ChildRunsAndGitHub<'_, '_> { + fn push(&mut self) -> Result<()> { + self.worktree.push() + } + + /// A child `thirdshift` from the same Launch directory, and a thread + /// that waits for it, relaying its stderr, and sends how it ended. + fn start_ticket(&mut self, number: u64) -> Result<()> { + let kind = Kind::Ticket { + spec_branch: self.worktree.branch().to_string(), + }; + let child = child_run::start(&self.spec.sibling(number), kind, self.base_fix.clone())?; + let ended = self.ended.clone(); + thread::spawn(move || { + let _ = ended.send((number, finish_ticket(number, child))); + }); + Ok(()) + } + + fn next_ending(&mut self) -> (u64, Result) { + self.endings + .recv() + .expect("a running Ticket's thread holds a sender") + } + + fn tickets(&mut self) -> Result> { + github::tickets(self.spec) + } + + fn show(&mut self, checklist: &str) { + self.spec_pr.show(checklist); + } + + fn landed(&mut self, checklist: &str) -> Result<()> { + self.spec_pr.landed(checklist).map(|_| ()) + } + + fn interrupted(&mut self) -> bool { + interrupt::requested() + } + + fn step(&mut self, line: String) { + progress::step(line); + } +} + +/// Wait for Ticket `number`'s Run `child`, relaying its stderr, and say how +/// it ended. +fn finish_ticket(number: u64, child: Handle) -> Result { + let ended = child.wait()?; + progress::step(match ended { + Ended::Reached { .. } => format!("#{number} landed"), + Ended::Interrupted => format!("#{number} interrupted"), + Ended::Failed { .. } => format!("#{number} failed"), + }); + Ok(ended) +} + +/// Push the Spec branch, then keep up to `parallel` of the Tickets, last +/// read as `tickets`, running, each time one ends starting ready ones, +/// lowest number first, until none is ready and none is running, showing +/// the Tickets checklist on the Spec PR as Tickets start and end, and +/// telling it when a Ticket lands, all through `outside`. Returns a line on +/// each Ticket that landed or is 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. fn land_tickets( - spec: &IssueUrl, + outside: &mut impl Outside, tickets: Vec, - worktree: &Worktree, parallel: NonZeroUsize, - base_fix: &BaseFixAsk, - spec_pr: &mut SpecPr, ) -> (Vec, Result) { - if let Err(error) = worktree.push() { + if let Err(error) = outside.push() { return (Vec::new(), Err(error)); } let mut board = TicketBoard::new(tickets, parallel); - let landing = run_ready_tickets(spec, &mut board, worktree.branch(), base_fix, spec_pr); + let landing = run_ready_tickets(outside, &mut board); let lines = board.lines(); let landed = landing.and_then(|()| { let not_done: Vec = board @@ -149,7 +245,7 @@ fn land_tickets( return Ok(board.checklist()); } for line in &lines { - progress::step(line); + outside.step(line.clone()); } bail!("Tickets not done: {}", not_done.join(", ")); }); @@ -158,30 +254,23 @@ fn land_tickets( /// Keep the Tickets on `board` running as it offers them, until it offers /// none and none is running, telling it as each starts and ends and as the -/// graph is read again, and telling `spec_pr` the Tickets checklist as they -/// start and end, and when one lands. An interrupt, or an error other than a -/// Ticket failing, stops any more from starting, and fails this once those -/// running have ended. Each Ticket's Run may start a Base fix if `base_fix` -/// allows one. -fn run_ready_tickets( - spec: &IssueUrl, - board: &mut TicketBoard, - spec_branch: &str, - base_fix: &BaseFixAsk, - spec_pr: &mut SpecPr, -) -> Result<()> { - let (ended, endings) = mpsc::channel(); +/// graph is read again, and showing the Spec PR the Tickets checklist as +/// they start and end, and telling it when one lands, all through +/// `outside`. An interrupt, or an error other than a Ticket failing, stops +/// any more from starting, and fails this once those running have ended. +fn run_ready_tickets(outside: &mut impl Outside, board: &mut TicketBoard) -> Result<()> { let mut error = None; loop { let mut started = false; loop { - if error.is_some() || interrupt::requested() { + if error.is_some() || outside.interrupted() { board.stop(); } let Some(ticket) = board.next_to_start() else { break; }; - match start_ticket(spec, ticket, spec_branch, base_fix, ended.clone()) { + outside.step(format!("starting #{ticket}")); + match outside.start_ticket(ticket) { Ok(()) => { board.started(ticket); started = true; @@ -190,14 +279,12 @@ fn run_ready_tickets( } } if started { - spec_pr.show(&board.checklist()); + outside.show(&board.checklist()); } if !board.any_running() { break; } - let (ticket, result) = endings - .recv() - .expect("a running Ticket's thread holds a sender"); + let (ticket, result) = outside.next_ending(); // Once there is an error, the rest are only waited for, but each // still has its line in the checklist, from the last graph read. let ending = result.unwrap_or_else(|finish_error| { @@ -207,7 +294,7 @@ fn run_ready_tickets( }); let landed = matches!(ending, Ended::Reached { .. }); board.ended(ticket, ending); - match github::tickets(spec) { + match outside.tickets() { Ok(reread) => board.reread(reread), Err(reread_error) => { error.get_or_insert(reread_error); @@ -215,52 +302,712 @@ fn run_ready_tickets( } let list = board.checklist(); if landed { - if let Err(open_error) = spec_pr.landed(&list) { + if let Err(open_error) = outside.landed(&list) { error.get_or_insert(open_error); } } else { - spec_pr.show(&list); + outside.show(&list); } } if let Some(error) = error { return Err(error); } - if interrupt::requested() { + if outside.interrupted() { bail!("interrupted"); } Ok(()) } -/// Start Ticket `number` of `spec` as a child `thirdshift`, a Merge run into -/// `spec_branch` from the same Launch directory that may start a Base fix if -/// `base_fix` allows one, and a thread that sends `number` and how it ended -/// on `ended`. -fn start_ticket( - spec: &IssueUrl, - number: u64, - spec_branch: &str, - base_fix: &BaseFixAsk, - ended: Sender<(u64, Result)>, -) -> Result<()> { - progress::step(format_args!("starting #{number}")); - let kind = Kind::Ticket { - spec_branch: spec_branch.to_string(), - }; - let child = child_run::start(&spec.sibling(number), kind, base_fix.clone())?; - thread::spawn(move || { - let _ = ended.send((number, finish_ticket(number, child))); - }); - Ok(()) -} +#[cfg(test)] +mod tests { + use std::collections::VecDeque; -/// Wait for Ticket `number`'s Run `child`, relaying its stderr, and say how -/// it ended. -fn finish_ticket(number: u64, child: Handle) -> Result { - let ended = child.wait()?; - progress::step(match ended { - Ended::Reached { .. } => format!("#{number} landed"), - Ended::Interrupted => format!("#{number} interrupted"), - Ended::Failed { .. } => format!("#{number} failed"), - }); - Ok(ended) + use anyhow::anyhow; + + use super::*; + + /// What the Ticket loop did outside itself, in the order it did it. + #[derive(Debug, Clone, PartialEq, Eq)] + enum Did { + /// Pushed the Spec branch. + Push, + /// Started this Ticket's Run, or tried to. + Start(u64), + /// Received that this Ticket's Run ended. + Ended(u64), + /// Read the Spec's Tickets again. + Read, + /// Showed this checklist on the Spec PR. + Show(String), + /// Told the Spec PR a Ticket landed, with this checklist. + Landed(String), + /// Wrote this progress line. + Line(String), + } + + /// A Ticket as GitHub has it: its open blockers follow from the graph. + struct Node { + number: u64, + is_open: bool, + labels: Vec<&'static str>, + blockers: Vec, + } + + /// How a read of the Spec's Tickets goes. + enum Read { + /// It reads the graph as it stands. + Graph, + /// It reads the graph once this has changed it. + Changes(fn(&mut Vec)), + /// It fails with this error. + Fails(&'static str), + } + + /// What the outside world answers, call by call. The push, each start, + /// each read and each Spec PR opening work, and the Run started first of + /// those running reaches its goal, unless scripted otherwise. + #[derive(Default)] + struct Script { + /// The Spec's Tickets. + tickets: Vec, + /// The issues outside the Spec that are open. + open_outside: Vec, + push_fails: bool, + /// The Tickets whose Runs cannot be started. + start_fails: Vec, + /// Which Run ends next, and how, in order. + endings: VecDeque<(u64, Result)>, + /// How each read of the Spec's Tickets goes, in order. + reads: VecDeque, + landed_fails: bool, + /// How many times the Spec run is found not interrupted before it is. + interrupted_after: Option, + } + + /// The outside world as `script` answers it, with what the Ticket loop + /// did there recorded in `did`. A Ticket whose Run reached its goal is + /// closed, as its Run's Self-merge closes it on GitHub. + struct Scripted { + script: Script, + did: Vec, + /// The Tickets whose Runs are running, in the order they started. + running: Vec, + /// How many times the Spec run was found not interrupted. + not_interrupted: usize, + } + + impl Scripted { + fn new(script: Script) -> Self { + Scripted { + script, + did: Vec::new(), + running: Vec::new(), + not_interrupted: 0, + } + } + + /// The Spec's Tickets as GitHub has them now. + fn graph(&self) -> Vec { + let is_open = |number: u64| match self + .script + .tickets + .iter() + .find(|node| node.number == number) + { + Some(node) => node.is_open, + None => self.script.open_outside.contains(&number), + }; + self.script + .tickets + .iter() + .map(|node| Ticket { + number: node.number, + is_open: node.is_open, + labels: node.labels.iter().copied().collect(), + has_sub_issues: false, + blockers: node.blockers.clone(), + open_blockers: node + .blockers + .iter() + .copied() + .filter(|blocker| is_open(*blocker)) + .collect(), + }) + .collect() + } + + /// What the Ticket loop did that is one of `kinds`. + fn did_only(&self, kinds: fn(&Did) -> bool) -> Vec { + self.did.iter().filter(|did| kinds(did)).cloned().collect() + } + } + + impl Outside for Scripted { + fn push(&mut self) -> Result<()> { + self.did.push(Did::Push); + if self.script.push_fails { + bail!("git push origin issue-20 failed"); + } + Ok(()) + } + + fn start_ticket(&mut self, number: u64) -> Result<()> { + self.did.push(Did::Start(number)); + if self.script.start_fails.contains(&number) { + bail!("could not start #{number}'s Run"); + } + self.running.push(number); + Ok(()) + } + + fn next_ending(&mut self) -> (u64, Result) { + let (number, ended) = self.script.endings.pop_front().unwrap_or_else(|| { + let first = *self.running.first().expect("no Ticket's Run is running"); + (first, Ok(reached())) + }); + let at = self + .running + .iter() + .position(|running| *running == number) + .unwrap_or_else(|| panic!("#{number} ends, but its Run is not running")); + self.running.remove(at); + self.did.push(Did::Ended(number)); + if let Ok(Ended::Reached { .. }) = ended { + let node = self + .script + .tickets + .iter_mut() + .find(|node| node.number == number); + node.expect("a Ticket of the Spec").is_open = false; + } + (number, ended) + } + + fn tickets(&mut self) -> Result> { + self.did.push(Did::Read); + match self.script.reads.pop_front().unwrap_or(Read::Graph) { + Read::Graph => {} + Read::Changes(change) => change(&mut self.script.tickets), + Read::Fails(error) => return Err(anyhow!(error)), + } + Ok(self.graph()) + } + + fn show(&mut self, checklist: &str) { + self.did.push(Did::Show(checklist.to_string())); + } + + fn landed(&mut self, checklist: &str) -> Result<()> { + self.did.push(Did::Landed(checklist.to_string())); + if self.script.landed_fails { + bail!("could not open the Spec PR"); + } + Ok(()) + } + + fn interrupted(&mut self) -> bool { + if self + .script + .interrupted_after + .is_some_and(|after| self.not_interrupted >= after) + { + return true; + } + self.not_interrupted += 1; + false + } + + fn step(&mut self, line: String) { + self.did.push(Did::Line(line)); + } + } + + /// Open Ticket `number`, blocked by `blockers`. + fn open(number: u64, blockers: &[u64]) -> Node { + Node { + number, + is_open: true, + labels: Vec::new(), + blockers: blockers.to_vec(), + } + } + + /// Open Ticket `number`, labelled `label`. + fn labelled(number: u64, label: &'static str) -> Node { + Node { + labels: vec![label], + ..open(number, &[]) + } + } + + fn closed(number: u64) -> Node { + Node { + is_open: false, + ..open(number, &[]) + } + } + + fn reached() -> Ended { + Ended::Reached { + pr_url: None, + base_fix: None, + } + } + + fn failed(cause: &str) -> Ended { + Ended::Failed { + cause: cause.to_string(), + log: Some("/logs/session.jsonl".to_string()), + } + } + + /// Land the Spec's Tickets, as `outside` has them, up to `parallel` at + /// once. + fn land(outside: &mut Scripted, parallel: usize) -> (Vec, Result) { + let tickets = outside.graph(); + land_tickets(outside, tickets, NonZeroUsize::new(parallel).unwrap()) + } + + /// The error of `result`, which must be one. + fn error(result: Result) -> String { + format!("{:#}", result.expect_err("did not fail")) + } + + /// The Tickets checklist of `lines`. + fn checklist(lines: &[&str]) -> String { + let mut list = "## Tickets\n\n".to_string(); + for line in lines { + list += &format!("- {line}\n"); + } + list + } + + fn line(line: &str) -> Did { + Did::Line(line.to_string()) + } + + /// Starts and endings only. + fn runs(did: &Did) -> bool { + matches!(did, Did::Start(_) | Did::Ended(_)) + } + + /// Spec PR calls only. + fn spec_pr(did: &Did) -> bool { + matches!(did, Did::Show(_) | Did::Landed(_)) + } + + /// Progress lines only. + fn lines(did: &Did) -> bool { + matches!(did, Did::Line(_)) + } + + #[test] + fn it_pushes_then_lands_each_ticket_telling_the_spec_pr_the_checklist_read_after_each() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[21])], + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + let done = checklist(&["[x] #21 landed", "[x] #22 landed"]); + assert_eq!(landed.unwrap(), done); + assert_eq!(lines, ["#21 landed", "#22 landed"]); + assert_eq!( + outside.did, + [ + Did::Push, + line("starting #21"), + Did::Start(21), + Did::Show(checklist(&["[ ] #21 running", "[ ] #22 blocked by #21"])), + Did::Ended(21), + Did::Read, + Did::Landed(checklist(&["[x] #21 landed", "[ ] #22 not started"])), + line("starting #22"), + Did::Start(22), + Did::Show(checklist(&["[x] #21 landed", "[ ] #22 running"])), + Did::Ended(22), + Did::Read, + Did::Landed(done), + ] + ); + } + + #[test] + fn a_push_that_fails_starts_nothing_and_returns_no_lines() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[])], + push_fails: true, + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + assert_eq!(error(landed), "git push origin issue-20 failed"); + assert!(lines.is_empty()); + assert_eq!(outside.did, [Did::Push]); + } + + #[test] + fn up_to_parallel_start_lowest_first_and_the_ticket_blocked_by_two_waits_for_both() { + let mut outside = Scripted::new(Script { + tickets: vec![ + open(21, &[]), + open(22, &[]), + open(23, &[21, 22]), + open(24, &[]), + ], + endings: VecDeque::from([(22, Ok(reached()))]), + ..Script::default() + }); + + let (_, landed) = land(&mut outside, 2); + + landed.unwrap(); + assert_eq!( + outside.did_only(runs), + [ + Did::Start(21), + Did::Start(22), + Did::Ended(22), + Did::Start(24), + Did::Ended(21), + Did::Start(23), + Did::Ended(24), + Did::Ended(23), + ] + ); + } + + #[test] + fn closed_tickets_are_never_started() { + let mut outside = Scripted::new(Script { + tickets: vec![closed(21), open(22, &[21])], + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + landed.unwrap(); + assert_eq!(lines, ["#22 landed"]); + assert_eq!(outside.did_only(runs), [Did::Start(22), Did::Ended(22)]); + } + + #[test] + fn a_failed_ticket_is_not_started_again_nor_what_it_blocks_while_an_independent_one_lands() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[21]), open(23, &[])], + endings: VecDeque::from([(21, Ok(failed("claude exited 1")))]), + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 1); + + assert_eq!(error(landed), "Tickets not done: #21, #22"); + let expected = [ + "#21 failed: claude exited 1 (session log: /logs/session.jsonl)", + "#22 blocked by #21", + "#23 landed", + ]; + assert_eq!(lines, expected); + assert_eq!( + outside.did_only(runs), + [ + Did::Start(21), + Did::Ended(21), + Did::Start(23), + Did::Ended(23) + ] + ); + assert!(outside.did.ends_with(&expected.map(line))); + } + + #[test] + fn an_unready_ticket_and_what_it_blocks_are_not_started_and_are_named_in_the_lines() { + let mut outside = Scripted::new(Script { + tickets: vec![labelled(21, "needs-info"), open(22, &[21]), open(23, &[])], + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + assert_eq!(error(landed), "Tickets not done: #21, #22"); + assert_eq!( + lines, + [ + "#21 unready: labelled needs-info", + "#22 blocked by #21", + "#23 landed" + ] + ); + assert_eq!(outside.did_only(runs), [Did::Start(23), Did::Ended(23)]); + } + + #[test] + fn an_open_outside_blocker_holds_a_ticket_back_and_a_closed_one_does_not() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[90]), open(22, &[91])], + open_outside: vec![90], + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + assert_eq!(error(landed), "Tickets not done: #21"); + assert_eq!( + lines, + ["#21 blocked by #90 (outside the Spec)", "#22 landed"] + ); + assert_eq!(outside.did_only(runs), [Did::Start(22), Did::Ended(22)]); + } + + #[test] + fn tickets_in_a_cycle_are_not_started_and_their_lines_name_the_cycle() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[22]), open(22, &[21]), open(23, &[])], + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + assert_eq!(error(landed), "Tickets not done: #21, #22"); + assert_eq!( + lines, + [ + "#21 in a cycle: #21 blocked by #22 blocked by #21", + "#22 in a cycle: #22 blocked by #21 blocked by #22", + "#23 landed", + ] + ); + assert_eq!(outside.did_only(runs), [Did::Start(23), Did::Ended(23)]); + } + + #[test] + fn a_reread_that_removes_an_unready_label_lets_that_ticket_start_once_another_ends() { + let mut outside = Scripted::new(Script { + tickets: vec![labelled(21, "needs-info"), open(22, &[])], + reads: VecDeque::from([Read::Changes(|tickets| tickets[0].labels.clear())]), + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + landed.unwrap(); + assert_eq!(lines, ["#21 landed", "#22 landed"]); + assert_eq!( + outside.did_only(runs), + [ + Did::Start(22), + Did::Ended(22), + Did::Start(21), + Did::Ended(21) + ] + ); + } + + #[test] + fn a_reread_that_fails_is_the_error_once_the_running_tickets_end_and_starts_nothing_more() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[]), open(23, &[])], + reads: VecDeque::from([Read::Fails("gh: GitHub stopped answering")]), + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 2); + + assert_eq!(error(landed), "gh: GitHub stopped answering"); + assert_eq!(lines, ["#21 landed", "#22 landed", "#23 not started"]); + assert_eq!( + outside.did_only(runs), + [ + Did::Start(21), + Did::Start(22), + Did::Ended(21), + Did::Ended(22) + ] + ); + assert_eq!( + outside.did_only(self::lines), + [line("starting #21"), line("starting #22")] + ); + } + + #[test] + fn a_ticket_that_could_not_be_started_is_the_error_and_nothing_more_starts() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[]), open(23, &[])], + start_fails: vec![22], + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + assert_eq!(error(landed), "could not start #22's Run"); + assert_eq!(lines, ["#21 landed", "#22 not started", "#23 not started"]); + assert_eq!( + outside.did_only(runs), + [Did::Start(21), Did::Start(22), Did::Ended(21)] + ); + } + + #[test] + fn a_run_that_could_not_be_waited_on_ends_failed_with_its_error_as_cause_and_the_loops_error() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[])], + endings: VecDeque::from([(21, Err(anyhow!("could not wait for #21's Run")))]), + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 1); + + assert_eq!(error(landed), "could not wait for #21's Run"); + assert_eq!( + lines, + [ + "#21 failed: could not wait for #21's Run", + "#22 not started" + ] + ); + assert_eq!(outside.did_only(runs), [Did::Start(21), Did::Ended(21)]); + } + + #[test] + fn the_first_error_wins_over_later_ones() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[])], + endings: VecDeque::from([ + (21, Err(anyhow!("could not wait for #21's Run"))), + (22, Err(anyhow!("could not wait for #22's Run"))), + ]), + reads: VecDeque::from([Read::Graph, Read::Fails("gh: GitHub stopped answering")]), + landed_fails: true, + ..Script::default() + }); + + let (_, landed) = land(&mut outside, 2); + + assert_eq!(error(landed), "could not wait for #21's Run"); + } + + #[test] + fn the_checklist_is_shown_after_each_round_that_started_a_ticket_and_not_after_one_that_started_none() + { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[]), open(23, &[21, 22])], + endings: VecDeque::from([(22, Ok(failed("claude exited 1")))]), + ..Script::default() + }); + + let (_, landed) = land(&mut outside, 2); + + assert_eq!(error(landed), "Tickets not done: #22, #23"); + assert_eq!( + outside.did_only(spec_pr), + [ + Did::Show(checklist(&[ + "[ ] #21 running", + "[ ] #22 running", + "[ ] #23 blocked by #21, #22" + ])), + Did::Show(checklist(&[ + "[ ] #21 running", + "[ ] #22 failed: claude exited 1", + "[ ] #23 blocked by #21, #22" + ])), + Did::Landed(checklist(&[ + "[x] #21 landed", + "[ ] #22 failed: claude exited 1", + "[ ] #23 blocked by #22" + ])), + ] + ); + } + + #[test] + fn a_ticket_that_failed_or_was_interrupted_only_shows_the_checklist() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[])], + endings: VecDeque::from([ + (21, Ok(failed("claude exited 1"))), + (22, Ok(Ended::Interrupted)), + ]), + ..Script::default() + }); + + let (_, landed) = land(&mut outside, 2); + + assert_eq!(error(landed), "Tickets not done: #21, #22"); + assert_eq!( + outside.did_only(spec_pr), + [ + Did::Show(checklist(&["[ ] #21 running", "[ ] #22 running"])), + Did::Show(checklist(&[ + "[ ] #21 failed: claude exited 1", + "[ ] #22 running" + ])), + Did::Show(checklist(&[ + "[ ] #21 failed: claude exited 1", + "[ ] #22 interrupted" + ])), + ] + ); + } + + #[test] + fn a_spec_pr_that_could_not_be_opened_is_the_error_and_nothing_more_starts() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[])], + landed_fails: true, + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 1); + + assert_eq!(error(landed), "could not open the Spec PR"); + assert_eq!(lines, ["#21 landed", "#22 not started"]); + assert_eq!(outside.did_only(runs), [Did::Start(21), Did::Ended(21)]); + } + + #[test] + fn an_interrupt_before_any_ticket_starts_starts_none_and_each_is_not_started() { + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[])], + interrupted_after: Some(0), + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 3); + + assert_eq!(error(landed), "interrupted"); + assert_eq!(lines, ["#21 not started", "#22 not started"]); + assert_eq!(outside.did, [Did::Push]); + } + + #[test] + fn an_interrupt_while_tickets_run_lets_them_end_and_starts_no_more() { + // Found not interrupted before #21, before #22 and once two run. + let mut outside = Scripted::new(Script { + tickets: vec![open(21, &[]), open(22, &[]), open(23, &[])], + interrupted_after: Some(3), + ..Script::default() + }); + + let (lines, landed) = land(&mut outside, 2); + + assert_eq!(error(landed), "interrupted"); + assert_eq!(lines, ["#21 landed", "#22 landed", "#23 not started"]); + assert_eq!( + outside.did_only(runs), + [ + Did::Start(21), + Did::Start(22), + Did::Ended(21), + Did::Ended(22) + ] + ); + assert_eq!( + outside.did_only(self::lines), + [line("starting #21"), line("starting #22")] + ); + } } diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 709b12dee..743a9fc8f 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -300,36 +300,6 @@ fn a_tickets_run_started_after_thirdshift_was_replaced_can_still_start_its_base_ ); } -#[test] -fn closed_tickets_are_not_run() { - let scenario = linear_spec(); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(!sessions_by_issue(&scenario).contains(&"23".to_string())); - assert!(!result.stderr.contains("#23"), "stderr: {}", result.stderr); -} - -#[test] -fn the_spec_branch_is_pushed_before_the_first_ticket_starts() { - let scenario = linear_spec(); - scenario.agent_does_for( - 21, - &format!( - "git ls-remote --exit-code origin refs/heads/issue-20 >/dev/null\n{}", - agent_lands(21, "first.txt") - ), - ); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - let pushed = result.stderr.find("thirdshift: pushing issue-20").unwrap(); - let started = result.stderr.find("thirdshift: starting #21").unwrap(); - assert!(pushed < started, "stderr: {}", result.stderr); -} - #[test] fn ticket_lines_are_relayed_with_the_ticket_number_and_the_spec_run_says_what_it_does() { let scenario = linear_spec(); @@ -989,29 +959,6 @@ fn repairs_exhausted_on_the_spec_pr_send_it_back_to_draft_and_exit_1() { assert_eq!(spec["isDraft"], true); } -#[test] -fn a_failed_ticket_ends_the_spec_run_before_the_tickets_it_blocks() { - let scenario = linear_spec(); - scenario.agent_does_for(21, "exit 1"); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); - assert_eq!(result.stdout, ""); - assert_eq!(sessions_by_issue(&scenario), ["21"]); - assert_contains(&result.stderr, "thirdshift: #21 failed"); - let gh = scenario.gh_state(); - assert!( - gh["prs"] - .as_array() - .unwrap() - .iter() - .all(|pr| pr["head"] != "issue-20"), - "a Spec PR was opened: {}", - gh["prs"] - ); -} - #[test] fn once_the_last_ticket_lands_the_spec_review_reviews_the_spec_branch_against_the_base_branch() { let scenario = linear_spec(); @@ -1543,31 +1490,6 @@ fn parallel_on_an_issue_with_no_sub_issues_stops_before_any_work_naming_the_flag ); } -#[test] -fn a_needs_info_ticket_and_what_it_blocks_are_not_run_while_an_independent_ticket_lands() { - let scenario = spec_of(&[(21, &[]), (22, &[21]), (23, &[])]); - scenario.issue_labelled(21, &["needs-info"]); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_failed_spec_run( - &scenario, - &result, - "- [ ] #21 unready: labelled needs-info\n\ - - [ ] #22 blocked by #21\n\ - - [x] #23 landed with https://github.com/acme/widgets/pull/1\n", - ); - assert_eq!(sessions_by_issue(&scenario), ["23"]); - assert_eq!(scenario.gh_state()["issues"]["23"], "CLOSED"); - for line in [ - "thirdshift: #21 unready: labelled needs-info\n", - "thirdshift: #22 blocked by #21\n", - "thirdshift: #23 landed with https://github.com/acme/widgets/pull/1\n", - ] { - assert_contains(&result.stderr, line); - } -} - #[test] fn an_unready_label_keeps_a_ticket_from_running() { // One label end to end; the Spec run's unit tests take each of them. @@ -1627,50 +1549,6 @@ fn a_failed_ticket_stops_only_its_dependents_and_is_not_started_again() { assert_contains(&result.stderr, "thirdshift: #22 blocked by #21\n"); } -#[test] -fn an_open_outside_blocker_holds_a_ticket_back_and_a_closed_one_does_not() { - let scenario = spec_of(&[(21, &[99]), (22, &[98])]); - scenario.issue_is(98, "CLOSED"); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_failed_spec_run( - &scenario, - &result, - "- [ ] #21 blocked by #99 (outside the Spec)\n\ - - [x] #22 landed with https://github.com/acme/widgets/pull/1\n", - ); - assert_eq!(sessions_by_issue(&scenario), ["22"]); - assert_contains( - &result.stderr, - "thirdshift: #21 blocked by #99 (outside the Spec)\n", - ); -} - -#[test] -fn tickets_in_a_cycle_are_not_run_and_the_summary_names_the_cycle() { - let scenario = spec_of(&[(21, &[22]), (22, &[21]), (23, &[21]), (24, &[])]); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_failed_spec_run( - &scenario, - &result, - "- [ ] #21 in a cycle: #21 blocked by #22 blocked by #21\n\ - - [ ] #22 in a cycle: #22 blocked by #21 blocked by #22\n\ - - [ ] #23 blocked by #21\n\ - - [x] #24 landed with https://github.com/acme/widgets/pull/1\n", - ); - assert_eq!(sessions_by_issue(&scenario), ["24"]); - for line in [ - "thirdshift: #21 in a cycle: #21 blocked by #22 blocked by #21\n", - "thirdshift: #22 in a cycle: #22 blocked by #21 blocked by #22\n", - "thirdshift: #23 blocked by #21\n", - ] { - assert_contains(&result.stderr, line); - } -} - #[test] fn a_ticket_with_its_own_sub_issues_is_unready() { let scenario = spec_of(&[(21, &[]), (22, &[])]); @@ -1688,21 +1566,6 @@ fn a_ticket_with_its_own_sub_issues_is_unready() { assert_contains(&result.stderr, "thirdshift: #21 unready: has sub-issues\n"); } -#[test] -fn removing_a_needs_info_label_while_another_ticket_runs_lets_it_run() { - let scenario = spec_of(&[(21, &[]), (22, &[])]); - scenario.issue_labelled(21, &["needs-info"]); - scenario.agent_does_for( - 22, - &format!("gh fake labels 21 '[]'\n{}", agent_lands(22, "22.txt")), - ); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(sessions_by_issue(&scenario), ["22", "21", "20"]); -} - #[test] fn help_explains_unready_tickets_and_that_a_spec_run_takes_every_ticket_it_can_reach() { let scenario = Scenario::new(); @@ -1780,26 +1643,6 @@ fn a_signal_to_the_spec_run_alone_fails_the_ticket_run_then_ends_the_spec_run_as ); } -#[test] -fn a_ticket_that_fails_once_the_spec_pr_is_open_is_shown_failed_in_its_checklist() { - let scenario = spec_of(&[(21, &[]), (22, &[])]); - scenario.agent_does_for(22, "exit 1"); - - // One at a time, so the Spec PR is open when #22 starts. - let result = scenario.run(&["parallel", "1", &spec_url(&scenario)]); - - assert_failed_spec_run( - &scenario, - &result, - "- [x] #21 landed with https://github.com/acme/widgets/pull/1\n\ - - [ ] #22 failed: claude exited 1\n", - ); - assert_contains( - &result.stderr, - "thirdshift: updating the Spec PR's Tickets checklist\n", - ); -} - #[test] fn a_ticket_whose_red_check_also_fails_on_the_spec_branch_shows_the_cause_in_the_checklist_and_the_notification() { @@ -1846,25 +1689,6 @@ fn a_ticket_whose_red_check_also_fails_on_the_spec_branch_shows_the_cause_in_the assert_eq!(scenario.origin_file("issue-20", "22.txt"), None); } -#[test] -fn a_ticket_that_lands_just_before_github_stops_answering_still_gets_the_draft_spec_pr() { - let scenario = spec_of(&[(21, &[]), (22, &[21])]); - scenario.agent_does_for( - 21, - &format!("gh fake fails 'api graphql'\n{}", agent_lands(21, "21.txt")), - ); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_failed_spec_run( - &scenario, - &result, - "- [ ] #21 landed with https://github.com/acme/widgets/pull/1, but is still open\n\ - - [ ] #22 blocked by #21\n", - ); - assert_eq!(sessions_by_issue(&scenario), ["21"]); -} - #[test] fn a_failed_spec_review_that_rewrote_the_body_has_the_checklist_put_back_in_the_draft() { let scenario = linear_spec(); @@ -2023,40 +1847,6 @@ fn an_interrupted_spec_run_sends_one_notification_with_each_tickets_outcome() { assert_contains(&text, "#22 blocked by #21\n"); } -#[test] -fn a_ready_ticket_an_interrupt_kept_from_starting_is_not_started_in_the_notification() { - let scenario = spec_of(&[(21, &[]), (22, &[])]); - scenario.agent_does_for( - 21, - &format!( - "touch {}\nsleep 30\n", - scenario.path("agent-started").display() - ), - ); - let resend = ResendStandIn::replying(200, r#"{"id":"1"}"#); - - let result = scenario.run_and_signal_with_env( - &[ - "--email", - "me@example.com", - "parallel", - "1", - &spec_url(&scenario), - ], - &[ - ("THIRDSHIFT_RESEND_URL", resend.url()), - ("RESEND_API_KEY", "re_test_123"), - ], - "agent-started", - "TERM", - ); - - assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); - let (_, text) = the_one_notification(&resend); - assert_contains(&text, "#21 interrupted\n"); - assert_contains(&text, "#22 not started\n"); -} - #[test] fn a_missing_resend_api_key_stops_the_spec_run_before_any_ticket_starts() { let scenario = linear_spec(); From 2c871bfcd5b38df65995565887513636ce124169 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sun, 4 Oct 2026 19:53:22 -0400 Subject: [PATCH 2/2] Review clean-ups for the Ticket loop seam (#380) Co-Authored-By: Claude Opus 5.5 --- src/spec_run.rs | 83 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 59 insertions(+), 24 deletions(-) diff --git a/src/spec_run.rs b/src/spec_run.rs index 93e26406d..9915374eb 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -66,15 +66,7 @@ pub fn run( let (spec, base) = (delivery.issue, delivery.base); let mut spec_pr = SpecPr::resume(spec, worktree.branch(), base)?; let base_fix = delivery.base_fix.ask_of_tickets(); - let (ended, endings) = mpsc::channel(); - let mut outside = ChildRunsAndGitHub { - spec, - worktree: &worktree, - base_fix: &base_fix, - spec_pr: &mut spec_pr, - ended, - endings, - }; + let mut outside = ChildRunsAndGitHub::new(spec, &worktree, &base_fix, &mut spec_pr); let (ticket_lines, landed) = land_tickets(&mut outside, tickets, parallel); let ended = match landed { Ok(checklist) => review_and_deliver(worktree, spec_pr, &checklist, delivery), @@ -121,7 +113,7 @@ fn review_and_deliver( /// What the Spec run's Ticket loop does or reads outside itself: the push of /// the Spec branch, the Tickets' child Runs, the graph read again, the Spec -/// PR, the interrupt and its progress lines. +/// PR, the interrupt and the loop's progress lines. trait Outside { /// Push the Spec branch to origin. fn push(&mut self) -> Result<()>; @@ -147,7 +139,7 @@ trait Outside { /// Spec branch checked out in `worktree`: child `thirdshift` Runs, each of /// which may start a Base fix if `base_fix` allows one, whose threads send /// how each ended on `ended`, received from `endings`, GitHub and the Spec -/// PR. +/// PR `spec_pr`. struct ChildRunsAndGitHub<'a, 'pr> { spec: &'a IssueUrl, worktree: &'a Worktree, @@ -157,6 +149,27 @@ struct ChildRunsAndGitHub<'a, 'pr> { endings: Receiver<(u64, Result)>, } +impl<'a, 'pr> ChildRunsAndGitHub<'a, 'pr> { + /// The outside world of the Ticket loop of a Spec run on `spec`, as the + /// struct says, with no Ticket's Run started yet. + fn new( + spec: &'a IssueUrl, + worktree: &'a Worktree, + base_fix: &'a BaseFixAsk, + spec_pr: &'a mut SpecPr<'pr>, + ) -> Self { + let (ended, endings) = mpsc::channel(); + Self { + spec, + worktree, + base_fix, + spec_pr, + ended, + endings, + } + } +} + impl Outside for ChildRunsAndGitHub<'_, '_> { fn push(&mut self) -> Result<()> { self.worktree.push() @@ -336,7 +349,7 @@ mod tests { /// Received that this Ticket's Run ended. Ended(u64), /// Read the Spec's Tickets again. - Read, + ReadTickets, /// Showed this checklist on the Spec PR. Show(String), /// Told the Spec PR a Ticket landed, with this checklist. @@ -372,6 +385,7 @@ mod tests { tickets: Vec, /// The issues outside the Spec that are open. open_outside: Vec, + /// Whether the push of the Spec branch fails. push_fails: bool, /// The Tickets whose Runs cannot be started. start_fails: Vec, @@ -379,6 +393,7 @@ mod tests { endings: VecDeque<(u64, Result)>, /// How each read of the Spec's Tickets goes, in order. reads: VecDeque, + /// Whether opening the Spec PR, once a Ticket lands, fails. landed_fails: bool, /// How many times the Spec run is found not interrupted before it is. interrupted_after: Option, @@ -484,7 +499,7 @@ mod tests { } fn tickets(&mut self) -> Result> { - self.did.push(Did::Read); + self.did.push(Did::ReadTickets); match self.script.reads.pop_front().unwrap_or(Read::Graph) { Read::Graph => {} Read::Changes(change) => change(&mut self.script.tickets), @@ -540,6 +555,7 @@ mod tests { } } + /// Closed Ticket `number`, blocked by none. fn closed(number: u64) -> Node { Node { is_open: false, @@ -592,12 +608,12 @@ mod tests { } /// Spec PR calls only. - fn spec_pr(did: &Did) -> bool { + fn spec_pr_calls(did: &Did) -> bool { matches!(did, Did::Show(_) | Did::Landed(_)) } /// Progress lines only. - fn lines(did: &Did) -> bool { + fn progress_lines(did: &Did) -> bool { matches!(did, Did::Line(_)) } @@ -621,13 +637,13 @@ mod tests { Did::Start(21), Did::Show(checklist(&["[ ] #21 running", "[ ] #22 blocked by #21"])), Did::Ended(21), - Did::Read, + Did::ReadTickets, Did::Landed(checklist(&["[x] #21 landed", "[ ] #22 not started"])), line("starting #22"), Did::Start(22), Did::Show(checklist(&["[x] #21 landed", "[ ] #22 running"])), Did::Ended(22), - Did::Read, + Did::ReadTickets, Did::Landed(done), ] ); @@ -827,7 +843,7 @@ mod tests { ] ); assert_eq!( - outside.did_only(self::lines), + outside.did_only(progress_lines), [line("starting #21"), line("starting #22")] ); } @@ -848,6 +864,10 @@ mod tests { outside.did_only(runs), [Did::Start(21), Did::Start(22), Did::Ended(21)] ); + assert_eq!( + outside.did_only(progress_lines), + [line("starting #21"), line("starting #22")] + ); } #[test] @@ -874,19 +894,34 @@ mod tests { #[test] fn the_first_error_wins_over_later_ones() { let mut outside = Scripted::new(Script { - tickets: vec![open(21, &[]), open(22, &[])], + tickets: vec![open(21, &[]), open(22, &[]), open(23, &[])], endings: VecDeque::from([ (21, Err(anyhow!("could not wait for #21's Run"))), - (22, Err(anyhow!("could not wait for #22's Run"))), + (22, Ok(reached())), + (23, Err(anyhow!("could not wait for #23's Run"))), ]), reads: VecDeque::from([Read::Graph, Read::Fails("gh: GitHub stopped answering")]), landed_fails: true, ..Script::default() }); - let (_, landed) = land(&mut outside, 2); + let (_, landed) = land(&mut outside, 3); assert_eq!(error(landed), "could not wait for #21's Run"); + // After #22 lands, its re-read fails and so does opening the Spec PR. + assert_eq!( + outside.did_only(|did| matches!(did, Did::Ended(_) | Did::Landed(_))), + [ + Did::Ended(21), + Did::Ended(22), + Did::Landed(checklist(&[ + "[ ] #21 failed: could not wait for #21's Run", + "[ ] #22 landed, but is still open", + "[ ] #23 running" + ])), + Did::Ended(23), + ] + ); } #[test] @@ -902,7 +937,7 @@ mod tests { assert_eq!(error(landed), "Tickets not done: #22, #23"); assert_eq!( - outside.did_only(spec_pr), + outside.did_only(spec_pr_calls), [ Did::Show(checklist(&[ "[ ] #21 running", @@ -938,7 +973,7 @@ mod tests { assert_eq!(error(landed), "Tickets not done: #21, #22"); assert_eq!( - outside.did_only(spec_pr), + outside.did_only(spec_pr_calls), [ Did::Show(checklist(&["[ ] #21 running", "[ ] #22 running"])), Did::Show(checklist(&[ @@ -1006,7 +1041,7 @@ mod tests { ] ); assert_eq!( - outside.did_only(self::lines), + outside.did_only(progress_lines), [line("starting #21"), line("starting #22")] ); }