diff --git a/src/github.rs b/src/github.rs index 4a01614bd..55f2b2323 100644 --- a/src/github.rs +++ b/src/github.rs @@ -130,13 +130,13 @@ impl GitHub { let mut blockers = Vec::new(); let mut open_blockers = Vec::new(); for blocker in nodes(&node["blockedBy"], "blockedBy")? { - blockers.push(node_number(blocker)?); + blockers.push(node_issue(blocker)?); if node_is_open(blocker)? { - open_blockers.push(node_number(blocker)?); + open_blockers.push(node_issue(blocker)?); } } Ok(Ticket { - number: node_number(node)?, + issue: node_issue(node)?, is_open: node_is_open(node)?, labels: named(nodes(&node["labels"], "labels")?), has_sub_issues: sub_issue_count(node)? > 0, @@ -799,15 +799,15 @@ pub fn add_label_command(issue: &IssueUrl, label: Label) -> String { /// A Spec's sub-issue, as a Spec run reads it. pub struct Ticket { - pub number: u64, + pub issue: IssueUrl, pub is_open: bool, pub labels: Labels, /// It has sub-issues of its own. pub has_sub_issues: bool, - /// The numbers of the issues it is blocked by, in the Spec or not. - pub blockers: Vec, - /// The numbers of those blockers that are open. - pub open_blockers: Vec, + /// The issues it is blocked by, in the Spec or not. + pub blockers: Vec, + /// Those blockers that are open. + pub open_blockers: Vec, } /// Every sub-issue of `issue`, open or closed, with its labels, whether it @@ -819,11 +819,11 @@ query($owner: String!, $repo: String!, $number: Int!) { issue(number: $number) { subIssues(first: 100) { nodes { - number + url state labels(first: 100) { nodes { name } } subIssues { totalCount } - blockedBy(first: 100) { nodes { number state } } + blockedBy(first: 100) { nodes { url state } } } } } @@ -845,6 +845,15 @@ fn node_is_open(node: &Value) -> Result { == "OPEN") } +/// The full Issue URL GitHub returned for a Ticket or blocker. +fn node_issue(node: &Value) -> Result { + IssueUrl::parse( + node["url"] + .as_str() + .context("gh api graphql output has an issue with no URL")?, + ) +} + /// The number of the issue a GraphQL answer's `node` describes. fn node_number(node: &Value) -> Result { node["number"] diff --git a/src/github/execution_tests.rs b/src/github/execution_tests.rs index 0dfca6711..8d4759288 100644 --- a/src/github/execution_tests.rs +++ b/src/github/execution_tests.rs @@ -405,3 +405,80 @@ esac }, ); } + +#[test] +fn tickets_keep_the_urls_of_sub_issues_and_their_open_and_closed_blockers() { + with_recorded_signal( + "github::execution_tests::tickets_keep_the_urls_of_sub_issues_and_their_open_and_closed_blockers", + |_| { + let _fixture = Fixture::new( + r#" +case "$*" in + *'nodes { url state }'*) + cat <<'JSON' +{"data":{"repository":{"issue":{"subIssues":{"nodes":[{"url":"https://github.com/other/widgets/issues/21","state":"OPEN","labels":{"nodes":[]},"subIssues":{"totalCount":0},"blockedBy":{"nodes":[{"url":"https://github.com/acme/widgets/issues/21","state":"OPEN"},{"url":"https://github.com/another/widgets/issues/21","state":"CLOSED"}]}}]}}}}} +JSON + ;; + *) echo '{}' ;; +esac +"#, + ); + let tickets = GitHub::new().tickets(&issue()).unwrap(); + assert_eq!(tickets.len(), 1); + assert_eq!( + tickets[0].issue.url, + "https://github.com/other/widgets/issues/21" + ); + assert_eq!( + tickets[0] + .blockers + .iter() + .map(|issue| issue.url.as_str()) + .collect::>(), + [ + "https://github.com/acme/widgets/issues/21", + "https://github.com/another/widgets/issues/21" + ] + ); + assert_eq!( + tickets[0] + .open_blockers + .iter() + .map(|issue| issue.url.as_str()) + .collect::>(), + ["https://github.com/acme/widgets/issues/21"] + ); + }, + ); +} + +#[test] +fn a_ticket_or_blocker_without_a_valid_url_fails_the_graph_read() { + with_recorded_signal( + "github::execution_tests::a_ticket_or_blocker_without_a_valid_url_fails_the_graph_read", + |_| { + let fixture = Fixture::new(""); + for (ticket_url, blocker_url) in [ + ( + serde_json::Value::Null, + serde_json::json!("https://github.com/acme/widgets/issues/21"), + ), + ( + serde_json::json!("https://github.com/acme/widgets/issues/22"), + serde_json::Value::Null, + ), + ( + serde_json::json!("https://github.com/acme/widgets/issues/22"), + serde_json::json!("https://example.invalid/issues/21"), + ), + ] { + let response = serde_json::json!({"data":{"repository":{"issue":{"subIssues":{"nodes":[{"url":ticket_url,"state":"OPEN","labels":{"nodes":[]},"subIssues":{"totalCount":0},"blockedBy":{"nodes":[{"url":blocker_url,"state":"CLOSED"}]}}]}}}}}); + fixture.script(&format!("cat <<'JSON'\n{response}\nJSON")); + assert!( + GitHub::new().tickets(&issue()).is_err(), + "accepted {response}" + ); + } + }, + ); +} diff --git a/src/issue.rs b/src/issue.rs index d6aee56d6..f5b6ea3c0 100644 --- a/src/issue.rs +++ b/src/issue.rs @@ -33,13 +33,12 @@ impl IssueUrl { } } - /// Issue `number` in the same repository. - pub fn sibling(&self, number: u64) -> IssueUrl { - IssueUrl { - url: format!("https://github.com/{}/issues/{number}", self.repo_slug()), - owner: self.owner.clone(), - repo: self.repo.clone(), - number, + /// Repository and number identity, independent of URL spelling. + pub(crate) fn identity(&self) -> IssueIdentity { + IssueIdentity { + owner: self.owner.to_ascii_lowercase(), + repo: self.repo.to_ascii_lowercase(), + number: self.number, } } @@ -72,6 +71,15 @@ impl IssueUrl { } } +/// An issue's identity for graph comparisons and keyed state. GitHub +/// repository names are case-insensitive; URL spelling is kept separately. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(crate) struct IssueIdentity { + owner: String, + repo: String, + number: u64, +} + /// The GitHub repository an `origin` remote names, spelled as its URL spells /// it. #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/run.rs b/src/run.rs index 89d7b9a35..a1b52b1a1 100644 --- a/src/run.rs +++ b/src/run.rs @@ -668,7 +668,8 @@ mod tests { fn ticket(number: u64, is_open: bool) -> Ticket { Ticket { - number, + issue: IssueUrl::parse(&format!("https://github.com/acme/widgets/issues/{number}")) + .unwrap(), is_open, labels: Labels::default(), has_sub_issues: false, diff --git a/src/security/fixing.rs b/src/security/fixing.rs index 5853194f7..dc4ec07e7 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -6,7 +6,7 @@ use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; use crate::git::Git; -use crate::github::{GitHub, SecurityRecord}; +use crate::github::{GitHub, SecurityRecord, Ticket}; use crate::harness::Choice; use crate::issue::{IssueUrl, Repo}; use crate::labels::{Edit, Label, NEEDS_TRIAGE, READY_FOR_AGENT}; @@ -152,9 +152,9 @@ pub fn publish( if !private { check_issue_text(&github, &issue, record, url)?; } - for child in &tickets { - let issue = issue.sibling(child.number); - let viewed = github.issue(&issue)?; + check_spec_tickets(&issue, &tickets, |child| { + let issue = &child.issue; + let viewed = github.issue(issue)?; if !viewed.is_open || viewed.created.timestamp() < started.timestamp() { bail!("a Security fix Spec's Ticket must be new and open"); } @@ -164,11 +164,11 @@ pub fn publish( { bail!("a Security fix Spec's Ticket must be ready-for-agent with no sub-issues"); } - check_issue_text(&github, &issue, record, url)?; - } + check_issue_text(&github, issue, record, url) + })?; for child in tickets { - let issue = issue.sibling(child.number); - Edit::of(&issue, github.issue_labels(&issue)?, &[], &[SECURITY_FIX]).apply(&github)?; + let issue = &child.issue; + Edit::of(issue, github.issue_labels(issue)?, &[], &[SECURITY_FIX]).apply(&github)?; } if crate::interrupt::requested() { bail!("interrupted"); @@ -184,6 +184,22 @@ pub fn publish( }) } +/// Validate every Security fix Spec Ticket before any Ticket is labelled. +/// The callback reads and checks the Ticket on GitHub. +fn check_spec_tickets( + spec: &IssueUrl, + tickets: &[Ticket], + mut check_on_github: impl FnMut(&Ticket) -> Result<()>, +) -> Result<()> { + for ticket in tickets { + if !spec.in_same_repo(&ticket.issue) { + bail!("a Security fix Spec's Ticket must be in the fix issue's repository"); + } + check_on_github(ticket)?; + } + Ok(()) +} + /// One staged Spec Ticket the publishing session proposed: its title, its /// body, and the 0-based indices of the Tickets it is blocked by. struct StagedTicket { @@ -406,6 +422,32 @@ fn check_issue_text( mod tests { use super::*; + #[test] + fn a_foreign_security_fix_ticket_is_rejected_before_its_github_checks() { + let spec = IssueUrl::parse("https://github.com/acme/widgets/issues/20").unwrap(); + let tickets = [Ticket { + issue: IssueUrl::parse("https://github.com/other/widgets/issues/21").unwrap(), + is_open: true, + labels: ["ready-for-agent"].into_iter().collect(), + has_sub_issues: false, + blockers: Vec::new(), + open_blockers: Vec::new(), + }]; + let mut checked = Vec::new(); + let result = check_spec_tickets(&spec, &tickets, |ticket| { + checked.push(ticket.issue.clone()); + Ok(()) + }); + assert_eq!( + result.unwrap_err().to_string(), + "a Security fix Spec's Ticket must be in the fix issue's repository" + ); + assert!( + checked.is_empty(), + "foreign Ticket reached GitHub validation" + ); + } + #[test] fn staged_proposal_parses_title_and_body() { let message = "Some reasoning\nProposed public issue:\nTitle: Bound accepted input\nBody:\nReject oversized input. Private record: https://example.invalid/advisory\n"; diff --git a/src/spec_run.rs b/src/spec_run.rs index f71db799f..fecf019f8 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -86,7 +86,7 @@ fn run_through( tickets: Vec, parallel: NonZeroUsize, ) -> Result { - let (ticket_lines, landed) = land_tickets(outside, tickets, parallel); + let (ticket_lines, landed) = land_tickets(outside, spec, tickets, parallel); // Every Ticket landed only once the Spec PR is told so. let landed = landed.and_then(|checklist| { let spec_pr_url = outside.landed(&checklist)?; @@ -156,11 +156,11 @@ fn review_and_deliver( 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 + /// Start `ticket`'s Run, a Merge run into the Spec branch. + fn start_ticket(&mut self, ticket: &IssueUrl) -> Result<()>; + /// Wait for the next running Ticket's Run to end: its Issue URL and how it /// ended. - fn next_ending(&mut self) -> (u64, Result); + fn next_ending(&mut self) -> (IssueUrl, 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. @@ -190,6 +190,8 @@ struct ChildRunsAndGitHub<'a> { /// Fields drop in declaration order: stop and drain child salvage before /// Worktree cleanup, including when the Spec run owner unwinds. children: Runs, + /// Full identities of local Tickets keyed by the child supervisor's number. + active: std::collections::BTreeMap, /// Held until the Delivery starts, which takes it. worktree: Option, /// Held until it starts. @@ -209,6 +211,7 @@ impl<'a> ChildRunsAndGitHub<'a> { Self { spec: delivery.issue, children: Runs::default(), + active: std::collections::BTreeMap::new(), worktree: Some(worktree), base_fix: delivery.base_fix.ask_of_tickets(), security_fix: delivery.security_fix, @@ -232,13 +235,15 @@ impl Outside for ChildRunsAndGitHub<'_> { self.worktree().push() } - fn start_ticket(&mut self, number: u64) -> Result<()> { + fn start_ticket(&mut self, ticket: &IssueUrl) -> Result<()> { + if !self.spec.in_same_repo(ticket) { + bail!("a Ticket outside the Spec's repository cannot start"); + } let kind = Kind::Ticket { spec_branch: self.worktree().branch().to_string(), }; - let ticket = self.spec.sibling(number); self.children.start( - &ticket, + ticket, kind, self.base_fix.clone(), crate::security::Options { @@ -246,10 +251,12 @@ impl Outside for ChildRunsAndGitHub<'_> { review: self.security_review, }, self.harness, - ) + )?; + self.active.insert(ticket.number, ticket.clone()); + Ok(()) } - fn next_ending(&mut self) -> (u64, Result) { + fn next_ending(&mut self) -> (IssueUrl, Result) { let (number, ending) = self .children .next_ending() @@ -261,7 +268,11 @@ impl Outside for ChildRunsAndGitHub<'_> { Ended::Failed { .. } => format!("#{number} failed"), }); } - (number, ending) + let ticket = self + .active + .remove(&number) + .expect("an outstanding Ticket keeps its Issue URL"); + (ticket, ending) } fn tickets(&mut self) -> Result> { @@ -306,21 +317,18 @@ impl Outside for ChildRunsAndGitHub<'_> { /// another error ended it first. fn land_tickets( outside: &mut impl Outside, + spec: &IssueUrl, tickets: Vec, parallel: NonZeroUsize, ) -> (Vec, Result) { if let Err(error) = outside.push() { return (Vec::new(), Err(error)); } - let mut board = TicketBoard::new(tickets, parallel); + let mut board = TicketBoard::new(spec, tickets, parallel); let landing = run_ready_tickets(outside, &mut board); let lines = board.lines(); let landed = landing.and_then(|()| { - let not_done: Vec = board - .not_done() - .iter() - .map(|number| format!("#{number}")) - .collect(); + let not_done = board.not_done(); if not_done.is_empty() { return Ok(board.checklist()); } @@ -349,10 +357,10 @@ fn run_ready_tickets(outside: &mut impl Outside, board: &mut TicketBoard) -> Res let Some(ticket) = board.next_to_start() else { break; }; - outside.step(format!("starting #{ticket}")); - match outside.start_ticket(ticket) { + outside.step(format!("starting #{}", ticket.number)); + match outside.start_ticket(&ticket) { Ok(()) => { - board.started(ticket); + board.started(&ticket); started = true; } Err(start_error) => error = Some(start_error), @@ -373,7 +381,7 @@ fn run_ready_tickets(outside: &mut impl Outside, board: &mut TicketBoard) -> Res Ended::Failed { cause, log: None } }); let landed = matches!(ending, Ended::Reached { .. }); - board.ended(ticket, ending); + board.ended(&ticket, ending); match outside.tickets() { Ok(reread) => board.reread(reread), Err(reread_error) => { @@ -446,10 +454,10 @@ mod tests { /// A Ticket as GitHub has it: its open blockers follow from the graph. struct Node { - number: u64, + issue: IssueUrl, is_open: bool, labels: Vec<&'static str>, - blockers: Vec, + blockers: Vec, } /// How a read of the Spec's Tickets goes. @@ -470,7 +478,7 @@ mod tests { /// The Spec's Tickets. tickets: Vec, /// The issues outside the Spec that are open. - open_outside: Vec, + open_outside: Vec, /// Whether the push of the Spec branch fails. push_fails: bool, /// The Tickets whose Runs cannot be started. @@ -501,6 +509,7 @@ mod tests { struct Scripted { script: Script, did: Vec, + started_urls: Vec, /// The Tickets whose Runs are running, in the order they started. running: Vec, /// How many times the Spec run was found not interrupted. @@ -515,6 +524,7 @@ mod tests { spec_pr_open: script.spec_pr_open, script, did: Vec::new(), + started_urls: Vec::new(), running: Vec::new(), not_interrupted: 0, } @@ -522,20 +532,24 @@ mod tests { /// The Spec's Tickets as GitHub has them now. fn graph(&self) -> Vec { - let is_open = |number: u64| match self + let is_open = |issue: &IssueUrl| match self .script .tickets .iter() - .find(|node| node.number == number) + .find(|node| node.issue.identity() == issue.identity()) { Some(node) => node.is_open, - None => self.script.open_outside.contains(&number), + None => self + .script + .open_outside + .iter() + .any(|open| open.identity() == issue.identity()), }; self.script .tickets .iter() .map(|node| Ticket { - number: node.number, + issue: node.issue.clone(), is_open: node.is_open, labels: node.labels.iter().copied().collect(), has_sub_issues: false, @@ -543,8 +557,8 @@ mod tests { open_blockers: node .blockers .iter() - .copied() - .filter(|blocker| is_open(*blocker)) + .filter(|blocker| is_open(blocker)) + .cloned() .collect(), }) .collect() @@ -565,7 +579,9 @@ mod tests { Ok(()) } - fn start_ticket(&mut self, number: u64) -> Result<()> { + fn start_ticket(&mut self, ticket: &IssueUrl) -> Result<()> { + let number = ticket.number; + self.started_urls.push(ticket.clone()); self.did.push(Did::Start(number)); if self.script.start_fails.contains(&number) { bail!("could not start #{number}'s Run"); @@ -574,7 +590,7 @@ mod tests { Ok(()) } - fn next_ending(&mut self) -> (u64, Result) { + fn next_ending(&mut self) -> (IssueUrl, 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())) @@ -586,15 +602,21 @@ mod tests { .unwrap_or_else(|| panic!("#{number} ends, but its Run is not running")); self.running.remove(at); self.did.push(Did::Ended(number)); + let issue = self + .started_urls + .iter() + .find(|issue| issue.number == number) + .unwrap() + .clone(); if let Ok(Ended::Reached { .. }) = ended { let node = self .script .tickets .iter_mut() - .find(|node| node.number == number); + .find(|node| node.issue.identity() == issue.identity()); node.expect("a Ticket of the Spec").is_open = false; } - (number, ended) + (issue, ended) } fn tickets(&mut self) -> Result> { @@ -681,13 +703,17 @@ mod tests { } } + fn issue(number: u64) -> IssueUrl { + IssueUrl::parse(&format!("https://github.com/acme/widgets/issues/{number}")).unwrap() + } + /// Open Ticket `number`, blocked by `blockers`. fn open(number: u64, blockers: &[u64]) -> Node { Node { - number, + issue: issue(number), is_open: true, labels: Vec::new(), - blockers: blockers.to_vec(), + blockers: blockers.iter().copied().map(issue).collect(), } } @@ -725,7 +751,12 @@ mod tests { /// once. fn land(outside: &mut Scripted, parallel: usize) -> (Vec, Result) { let tickets = outside.graph(); - land_tickets(outside, tickets, NonZeroUsize::new(parallel).unwrap()) + land_tickets( + outside, + &IssueUrl::parse("https://github.com/acme/widgets/issues/20").unwrap(), + tickets, + NonZeroUsize::new(parallel).unwrap(), + ) } /// Take Spec #20, its Tickets as `outside` has them, up to `parallel` @@ -804,6 +835,88 @@ mod tests { matches!(did, Did::Line(_)) } + #[test] + fn scripted_endings_close_the_started_issue_without_a_foreign_number_collision() { + let mut foreign = open(21, &[]); + foreign.issue = IssueUrl::parse("https://github.com/other/widgets/issues/21").unwrap(); + + let mut outside = Scripted::new(Script { + tickets: vec![foreign, open(21, &[])], + ..Script::default() + }); + + outside.start_ticket(&issue(21)).unwrap(); + let (ended_issue, ending) = outside.next_ending(); + assert_eq!(ended_issue.identity(), issue(21).identity()); + assert!(matches!(ending, Ok(Ended::Reached { .. }))); + + let tickets = outside.graph(); + assert!( + tickets[0].is_open, + "ending the local Run closed the foreign Ticket" + ); + assert!(!tickets[1].is_open, "the local Ticket stayed open"); + } + + #[test] + fn start_ticket_receives_the_graphs_full_issue_url() { + let mut ticket = open(21, &[]); + ticket.issue = IssueUrl::parse("https://github.com/Acme/Widgets/issues/21/").unwrap(); + let mut outside = Scripted::new(Script { + tickets: vec![ticket], + ..Script::default() + }); + reached_of(spec_run(&mut outside, 1)); + assert_eq!( + outside.started_urls, + [IssueUrl::parse("https://github.com/Acme/Widgets/issues/21/").unwrap()] + ); + } + + #[test] + fn a_foreign_blocker_with_a_tickets_number_holds_its_dependent_without_a_false_cycle() { + let mut blocked = open(21, &[]); + let foreign = IssueUrl::parse("https://github.com/other/widgets/issues/21").unwrap(); + blocked.blockers = vec![foreign.clone()]; + let mut outside = Scripted::new(Script { + tickets: vec![blocked, open(22, &[])], + open_outside: vec![foreign], + ..Script::default() + }); + let failed = failed_of(spec_run(&mut outside, 2)); + assert_eq!(outside.did_only(runs), [Did::Start(22), Did::Ended(22)]); + assert_eq!( + failed.ticket_lines, + [ + "#21 blocked by https://github.com/other/widgets/issues/21 (outside the Spec)", + "#22 landed" + ] + ); + } + + #[test] + fn a_foreign_sub_issue_is_unready_and_never_starts_a_run() { + let mut foreign = open(21, &[]); + foreign.issue = IssueUrl::parse("https://github.com/other/widgets/issues/21").unwrap(); + let mut outside = Scripted::new(Script { + tickets: vec![foreign, open(22, &[])], + ..Script::default() + }); + + let failed = failed_of(spec_run(&mut outside, 2)); + + assert_eq!(outside.did_only(runs), [Did::Start(22), Did::Ended(22)]); + assert_eq!( + failed.ticket_lines, + [ + "https://github.com/other/widgets/issues/21 unready: outside the Spec's repository", + "#22 landed" + ] + ); + assert!(outside.did.iter().any(|did| matches!(did, Did::Landed(list) + if list.contains("- [ ] https://github.com/other/widgets/issues/21 unready: outside the Spec's repository")))); + } + #[test] fn it_pushes_then_lands_each_ticket_telling_the_spec_pr_the_checklist_read_after_each() { let mut outside = Scripted::new(Script { @@ -950,7 +1063,7 @@ mod tests { 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], + open_outside: vec![issue(90)], ..Script::default() }); diff --git a/src/spec_run/ticket_board.rs b/src/spec_run/ticket_board.rs index bb59a1d19..dc4a09063 100644 --- a/src/spec_run/ticket_board.rs +++ b/src/spec_run/ticket_board.rs @@ -10,6 +10,7 @@ use std::num::NonZeroUsize; use crate::child_run::Ended; use crate::github::Ticket; +use crate::issue::{IssueIdentity, IssueUrl}; /// Where a Ticket this Spec run started stands. enum Standing { @@ -21,16 +22,18 @@ enum Standing { /// A Spec run's Tickets, as last read, and where each one it started stands. pub(super) struct TicketBoard { + spec: IssueUrl, tickets: Vec, - standings: BTreeMap, + standings: BTreeMap, parallel: NonZeroUsize, stopped: bool, } impl TicketBoard { /// A board of `tickets`, none started, running up to `parallel` at once. - pub(super) fn new(tickets: Vec, parallel: NonZeroUsize) -> Self { + pub(super) fn new(spec: &IssueUrl, tickets: Vec, parallel: NonZeroUsize) -> Self { Self { + spec: spec.clone(), tickets, standings: BTreeMap::new(), parallel, @@ -44,35 +47,37 @@ impl TicketBoard { /// none running, and was not started in this Spec run. A blocker's Run /// closes its issue before it ends, so a closed blocker may still be /// running. - pub(super) fn next_to_start(&self) -> Option { + pub(super) fn next_to_start(&self) -> Option { if self.stopped || self.running() >= self.parallel.get() { return None; } self.tickets .iter() .filter(|ticket| { - ticket.is_open + self.spec.in_same_repo(&ticket.issue) + && ticket.is_open && !ticket.has_sub_issues && ticket.open_blockers.is_empty() && !ticket .blockers .iter() - .any(|blocker| self.is_running(*blocker)) - && !self.standings.contains_key(&ticket.number) + .any(|blocker| self.is_running(blocker)) + && !self.standings.contains_key(&ticket.issue.identity()) && ticket.labels.unready().is_none() }) - .map(|ticket| ticket.number) - .min() + .min_by_key(|ticket| ticket.issue.number) + .map(|ticket| ticket.issue.clone()) } - /// Ticket `number`'s Run has started. - pub(super) fn started(&mut self, number: u64) { - self.standings.insert(number, Standing::Running); + /// Ticket `issue`'s Run has started. + pub(super) fn started(&mut self, issue: &IssueUrl) { + self.standings.insert(issue.identity(), Standing::Running); } - /// Ticket `number`'s Run has ended, as `ended` tells. - pub(super) fn ended(&mut self, number: u64, ended: Ended) { - self.standings.insert(number, Standing::Ended(ended)); + /// Ticket `issue`'s Run has ended, as `ended` tells. + pub(super) fn ended(&mut self, issue: &IssueUrl, ended: Ended) { + self.standings + .insert(issue.identity(), Standing::Ended(ended)); } /// Start no more Tickets: those running are still waited for. @@ -92,11 +97,11 @@ impl TicketBoard { } /// The Tickets not done, the open ones, in the order they were read. - pub(super) fn not_done(&self) -> Vec { + pub(super) fn not_done(&self) -> Vec { self.tickets .iter() .filter(|ticket| ticket.is_open) - .map(|ticket| ticket.number) + .map(|ticket| self.name(&ticket.issue)) .collect() } @@ -105,16 +110,17 @@ impl TicketBoard { pub(super) fn lines(&self) -> Vec { let mut lines = Vec::new(); for ticket in self.by_number() { - let number = ticket.number; - if !ticket.is_open && !self.standings.contains_key(&number) { + let identity = ticket.issue.identity(); + let reference = self.name(&ticket.issue); + if !ticket.is_open && !self.standings.contains_key(&identity) { continue; } let standing = self.standing(ticket); - lines.push(match self.standings.get(&number) { + lines.push(match self.standings.get(&identity) { Some(Standing::Ended(Ended::Failed { log: Some(log), .. })) if ticket.is_open => { - format!("#{number} {standing} (session log: {log})") + format!("{reference} {standing} (session log: {log})") } - _ => format!("#{number} {standing}"), + _ => format!("{reference} {standing}"), }); } lines @@ -126,11 +132,24 @@ impl TicketBoard { let mut list = "## Tickets\n\n".to_string(); for ticket in self.by_number() { let tick = if ticket.is_open { ' ' } else { 'x' }; - list += &format!("- [{tick}] #{} {}\n", ticket.number, self.standing(ticket)); + list += &format!( + "- [{tick}] {} {}\n", + self.name(&ticket.issue), + self.standing(ticket) + ); } list } + /// Local issues use GitHub's short reference; foreign ones need their URL. + fn name(&self, issue: &IssueUrl) -> String { + if self.spec.in_same_repo(issue) { + format!("#{}", issue.number) + } else { + issue.url.clone() + } + } + /// How many Tickets' Runs are running. fn running(&self) -> usize { self.standings @@ -139,23 +158,26 @@ impl TicketBoard { .count() } - /// Whether Ticket `number`'s Run is running. - fn is_running(&self, number: u64) -> bool { - matches!(self.standings.get(&number), Some(Standing::Running)) + /// Whether Ticket `issue`'s Run is running. + fn is_running(&self, issue: &IssueUrl) -> bool { + matches!( + self.standings.get(&issue.identity()), + Some(Standing::Running) + ) } /// The Tickets, lowest number first. fn by_number(&self) -> Vec<&Ticket> { let mut sorted: Vec<&Ticket> = self.tickets.iter().collect(); - sorted.sort_by_key(|ticket| ticket.number); + sorted.sort_by_key(|ticket| ticket.issue.number); sorted } /// Where `ticket` stands in this Spec run, as in "#21 ": done, /// with its PR if it landed in this Spec run, or why it is not done. fn standing(&self, ticket: &Ticket) -> String { - let number = ticket.number; - let landed = match self.standings.get(&number) { + let identity = ticket.issue.identity(); + let landed = match self.standings.get(&identity) { Some(Standing::Ended(Ended::Reached { pr_url, base_fix })) => { let mut landed = "landed".to_string(); if let Some(pr_url) = pr_url { @@ -171,7 +193,7 @@ impl TicketBoard { if !ticket.is_open { return landed.unwrap_or_else(|| "done".to_string()); } - match self.standings.get(&number) { + match self.standings.get(&identity) { Some(Standing::Running) => return "running".to_string(), Some(Standing::Ended(Ended::Interrupted)) => return "interrupted".to_string(), Some(Standing::Ended(Ended::Failed { cause, .. })) => { @@ -179,14 +201,17 @@ impl TicketBoard { } _ => {} } + if !self.spec.in_same_repo(&ticket.issue) { + return "unready: outside the Spec's repository".to_string(); + } if let Some(landed) = landed { format!("{landed}, but is still open") } else if let Some(label) = ticket.labels.unready() { format!("unready: labelled {label}") } else if ticket.has_sub_issues { "unready: has sub-issues".to_string() - } else if let Some(cycle) = cycle_through(number, &self.tickets) { - let cycle: Vec = cycle.iter().map(|number| format!("#{number}")).collect(); + } else if let Some(cycle) = cycle_through(&ticket.issue, &self.tickets) { + let cycle: Vec = cycle.iter().map(|issue| self.name(issue)).collect(); format!("in a cycle: {}", cycle.join(" blocked by ")) } else if ticket.open_blockers.is_empty() { // Ready, but not started yet, or the Spec run ended before it could. @@ -196,10 +221,14 @@ impl TicketBoard { .open_blockers .iter() .map(|blocker| { - if self.tickets.iter().any(|ticket| ticket.number == *blocker) { - format!("#{blocker}") + if self + .tickets + .iter() + .any(|ticket| ticket.issue.identity() == blocker.identity()) + { + self.name(blocker) } else { - format!("#{blocker} (outside the Spec)") + format!("{} (outside the Spec)", self.name(blocker)) } }) .collect(); @@ -210,30 +239,30 @@ impl TicketBoard { /// The shortest cycle of "blocked by" links among `tickets` from Ticket /// `start` back to itself, `start` first and last, if there is one. -fn cycle_through(start: u64, tickets: &[Ticket]) -> Option> { - let blockers = |number: u64| { +fn cycle_through<'a>(start: &'a IssueUrl, tickets: &'a [Ticket]) -> Option> { + let blockers = |issue: &IssueUrl| { tickets .iter() - .find(|ticket| ticket.number == number) + .find(|ticket| ticket.issue.identity() == issue.identity()) .map_or(&[][..], |ticket| &ticket.open_blockers[..]) }; let mut reached_from = HashMap::new(); let mut queue = VecDeque::from([start]); - while let Some(number) = queue.pop_front() { - for &blocker in blockers(number) { - if blocker == start { + while let Some(issue) = queue.pop_front() { + for blocker in blockers(issue) { + if blocker.identity() == start.identity() { let mut cycle = vec![start]; - let mut at = number; - while at != start { + let mut at = issue; + while at.identity() != start.identity() { cycle.push(at); - at = reached_from[&at]; + at = reached_from[&at.identity()]; } cycle.push(start); cycle.reverse(); return Some(cycle); } - if let Entry::Vacant(entry) = reached_from.entry(blocker) { - entry.insert(number); + if let Entry::Vacant(entry) = reached_from.entry(blocker.identity()) { + entry.insert(issue); queue.push_back(blocker); } } @@ -246,19 +275,23 @@ mod tests { use super::*; use crate::labels::{Label, Labels, READY_FOR_AGENT, UNREADY}; + fn issue(number: u64) -> IssueUrl { + IssueUrl::parse(&format!("https://github.com/acme/widgets/issues/{number}")).unwrap() + } + fn ticket(number: u64, is_open: bool, blockers: &[u64], open_blockers: &[u64]) -> Ticket { Ticket { - number, + issue: issue(number), is_open, labels: Labels::default(), has_sub_issues: false, - blockers: blockers.to_vec(), - open_blockers: open_blockers.to_vec(), + blockers: blockers.iter().copied().map(issue).collect(), + open_blockers: open_blockers.iter().copied().map(issue).collect(), } } fn board(tickets: Vec, parallel: usize) -> TicketBoard { - TicketBoard::new(tickets, NonZeroUsize::new(parallel).unwrap()) + TicketBoard::new(&issue(20), tickets, NonZeroUsize::new(parallel).unwrap()) } fn reached(pr_url: Option<&str>) -> Ended { @@ -275,6 +308,56 @@ mod tests { } } + #[test] + fn a_foreign_ticket_with_a_local_number_is_never_offered_or_reported_as_running() { + let mut foreign = ticket(21, true, &[], &[]); + foreign.issue = IssueUrl::parse("https://github.com/other/widgets/issues/21").unwrap(); + let mut board = board(vec![foreign, ticket(21, true, &[], &[])], 2); + assert_eq!(board.next_to_start(), Some(issue(21))); + board.started(&issue(21)); + assert_eq!(board.next_to_start(), None); + assert_eq!( + board.checklist(), + "## Tickets\n\n- [ ] https://github.com/other/widgets/issues/21 unready: outside the Spec's repository\n- [ ] #21 running\n" + ); + } + + #[test] + fn a_closed_foreign_blocker_does_not_wait_for_a_same_numbered_local_run() { + let mut blocked = ticket(22, true, &[], &[]); + blocked.blockers = + vec![IssueUrl::parse("https://github.com/other/widgets/issues/21").unwrap()]; + let mut board = board(vec![ticket(21, false, &[], &[]), blocked], 2); + board.started(&issue(21)); + assert_eq!(board.next_to_start(), Some(issue(22))); + } + + #[test] + fn a_foreign_open_blocker_is_reported_without_a_false_local_cycle() { + let mut blocked = ticket(21, true, &[], &[]); + let foreign = IssueUrl::parse("https://github.com/other/widgets/issues/21").unwrap(); + blocked.blockers = vec![foreign.clone()]; + blocked.open_blockers = vec![foreign]; + let board = board(vec![blocked], 1); + assert_eq!(board.next_to_start(), None); + assert_eq!( + board.lines(), + ["#21 blocked by https://github.com/other/widgets/issues/21 (outside the Spec)"] + ); + } + + #[test] + fn a_reread_with_different_url_spelling_keeps_the_same_ticket_standing() { + let mut board = board(vec![ticket(21, true, &[], &[])], 1); + board.started(&issue(21)); + board.ended(&issue(21), failed("Run failed", None)); + let mut reread = ticket(21, true, &[], &[]); + reread.issue = IssueUrl::parse("https://github.com/ACME/Widgets/issues/21/").unwrap(); + board.reread(vec![reread]); + assert_eq!(board.next_to_start(), None); + assert_eq!(board.lines(), ["#21 failed: Run failed"]); + } + #[test] fn it_never_offers_more_than_parallel_tickets_at_once() { let mut board = board( @@ -286,14 +369,14 @@ mod tests { 2, ); - assert_eq!(board.next_to_start(), Some(21)); - board.started(21); - assert_eq!(board.next_to_start(), Some(22)); - board.started(22); + assert_eq!(board.next_to_start(), Some(issue(21))); + board.started(&issue(21)); + assert_eq!(board.next_to_start(), Some(issue(22))); + board.started(&issue(22)); assert_eq!(board.next_to_start(), None); - board.ended(21, failed("claude exited 1", None)); - assert_eq!(board.next_to_start(), Some(23)); + board.ended(&issue(21), failed("claude exited 1", None)); + assert_eq!(board.next_to_start(), Some(issue(23))); } #[test] @@ -302,13 +385,13 @@ mod tests { vec![ticket(21, true, &[], &[]), ticket(22, true, &[], &[])], 2, ); - board.started(21); + board.started(&issue(21)); board.stop(); assert_eq!(board.next_to_start(), None); assert!(board.any_running()); - board.ended(21, Ended::Interrupted); + board.ended(&issue(21), Ended::Interrupted); assert!(!board.any_running()); assert_eq!(board.next_to_start(), None); } @@ -319,10 +402,10 @@ mod tests { vec![ticket(21, true, &[], &[]), ticket(22, true, &[], &[])], 2, ); - board.started(21); - board.started(22); - board.ended(21, failed("claude exited 1", None)); - board.ended(22, reached(None)); + board.started(&issue(21)); + board.started(&issue(22)); + board.ended(&issue(21), failed("claude exited 1", None)); + board.ended(&issue(22), reached(None)); assert_eq!(board.next_to_start(), None); } @@ -338,13 +421,13 @@ mod tests { ], 2, ); - board.started(21); - board.ended(21, reached(None)); - board.started(22); + board.started(&issue(21)); + board.ended(&issue(21), reached(None)); + board.started(&issue(22)); assert_eq!(board.next_to_start(), None); - board.ended(22, reached(None)); - assert_eq!(board.next_to_start(), Some(23)); + board.ended(&issue(22), reached(None)); + assert_eq!(board.next_to_start(), Some(issue(23))); } #[test] @@ -353,8 +436,8 @@ mod tests { vec![ticket(21, true, &[], &[]), ticket(22, true, &[21], &[21])], 1, ); - board.started(21); - board.ended(21, reached(Some("https://x/pull/1"))); + board.started(&issue(21)); + board.ended(&issue(21), reached(Some("https://x/pull/1"))); board.reread(vec![ ticket(21, false, &[], &[]), @@ -365,7 +448,7 @@ mod tests { board.lines(), ["#21 landed with https://x/pull/1", "#22 not started"] ); - assert_eq!(board.next_to_start(), Some(22)); + assert_eq!(board.next_to_start(), Some(issue(22))); } #[test] @@ -379,7 +462,7 @@ mod tests { 1, ); - assert_eq!(board.not_done(), [23, 22]); + assert_eq!(board.not_done(), ["#23", "#22"]); } #[test] @@ -392,7 +475,7 @@ mod tests { .collect(); let board = board(vec![unready, ticket(22, true, &[], &[])], 1); - assert_eq!(board.next_to_start(), Some(22), "{label}"); + assert_eq!(board.next_to_start(), Some(issue(22)), "{label}"); assert_eq!(board.lines()[0], format!("#21 unready: labelled {label}")); } } @@ -400,9 +483,9 @@ mod tests { #[test] fn a_failed_tickets_line_says_its_cause() { let mut board = board(vec![ticket(23, true, &[], &[])], 1); - board.started(23); + board.started(&issue(23)); board.ended( - 23, + &issue(23), failed( "git fetch origin issue-21 failed: \ error: fetching ref refs/remotes/origin/issue-21 failed", @@ -433,11 +516,14 @@ mod tests { ], 3, ); - board.started(21); - board.ended(21, reached(Some("https://x/pull/1"))); - board.started(23); - board.ended(23, failed("claude exited 1", Some("/logs/23.jsonl"))); - board.started(25); + board.started(&issue(21)); + board.ended(&issue(21), reached(Some("https://x/pull/1"))); + board.started(&issue(23)); + board.ended( + &issue(23), + failed("claude exited 1", Some("/logs/23.jsonl")), + ); + board.started(&issue(25)); assert_eq!( board.checklist(), diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 1b83ed9de..5626a746c 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -31,6 +31,7 @@ //! "sub_issues"?: {"": [, ...]}, //! "labels"?: {"": ["