Keep Ticket repository identities through Spec runs

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<u64>,
-    /// The numbers of those blockers that are open.
-    pub open_blockers: Vec<u64>,
+    /// The issues it is blocked by, in the Spec or not.
+    pub blockers: Vec<IssueUrl>,
+    /// Those blockers that are open.
+    pub open_blockers: Vec<IssueUrl>,
 }
 
 /// 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<bool> {
         == "OPEN")
 }
 
+/// The full Issue URL GitHub returned for a Ticket or blocker.
+fn node_issue(node: &Value) -> Result<IssueUrl> {
+    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<u64> {
     node["number"]

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::<Vec<_>>(),
+                [
+                    "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::<Vec<_>>(),
+                ["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}"
+                );
+            }
+        },
+    );
+}

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)]

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,

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";