From c05ed62ce4af5e9534ab0b859b515e49758e99e2 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 19 Oct 2026 23:29:53 -0400 Subject: [PATCH 1/2] Settle the Trusted account rule in its own trust module The rule moves out of the GitHub adapter into src/trust.rs, over a four-read Reads interface the adapter implements, so each edge case is tested in memory. Integration tests that only varied the rule give way to those unit tests; each caller's wiring and the adapter's reading stay, with edit history now read past its first page in a test. Co-Authored-By: Claude Opus 5.5 --- docs/code-navigation.md | 2 +- src/delivery.rs | 5 +- src/github.rs | 1 - src/github/session_issue.rs | 2 +- src/github/trust.rs | 226 +++----------- src/main.rs | 1 + src/ready.rs | 5 +- src/run.rs | 3 +- src/spec_run.rs | 3 +- src/trust.rs | 576 ++++++++++++++++++++++++++++++++++++ tests/fresh_run.rs | 22 -- tests/merge_run.rs | 19 -- tests/pickup_run.rs | 129 ++------ tests/spec_run.rs | 72 ----- 14 files changed, 651 insertions(+), 415 deletions(-) create mode 100644 src/trust.rs diff --git a/docs/code-navigation.md b/docs/code-navigation.md index 339754f5..b7267fee 100644 --- a/docs/code-navigation.md +++ b/docs/code-navigation.md @@ -16,7 +16,7 @@ Start with the issue's terms in `GLOSSARY.md`, then use this index to find their | Run notification | `src/notification.rs` — `RunNotification`; `src/email.rs` | `tests/run_notification.rs`, `tests/email_test.rs` | | Harness, Model, Effort | `src/harness.rs` and `src/harness/` | `tests/harness_choice.rs`, `tests/harness.rs` | | Session prompt, Factory skills | `src/prompt.rs`; `src/skills.rs`; `skills/` | `src/skills/tests.rs`, `tests/packaging.rs` | -| Trusted account, the issue a session gets | `src/github/trust.rs`; `src/github/session_issue.rs` — `issue_for_session`; `src/delivery.rs` — `hand_over_issue` | Inline tests, including `src/prompt.rs`; `tests/fresh_run.rs` | +| Trusted account, the issue a session gets | `src/trust.rs` — `issue_trust`, `ticket_trust`, `untrusted_edit` over `Reads`; the GitHub adapter's trust reads, `src/github/trust.rs`; `src/github/session_issue.rs` — `issue_for_session`; `src/delivery.rs` — `hand_over_issue` | Inline tests, including `src/trust.rs` and `src/prompt.rs`; `tests/fresh_run.rs`, `tests/pickup_run.rs` | | Session log, Resume | `src/session.rs` and `src/session/` | `src/session/execution_tests.rs`, `tests/background_work.rs` | | Command log, Activity log | `src/logs.rs`; `src/logs/recording.rs` — `Record` | `src/logs/recording/tests.rs`, `tests/command_logs.rs`, `tests/activity_log.rs` | | Command-log allocation, Session naming, child inheritance and filename decoding | `src/logs/owner.rs` — `allocate`, `Owner`, `decode`, `session_matches`; `src/session/recording.rs` — captured Model recording | `src/logs/owner/tests.rs`, `src/logs/recording/tests.rs`, `src/session/recording/tests.rs`, `tests/command_logs.rs`, `tests/status.rs`, `tests/show_logs.rs` | diff --git a/src/delivery.rs b/src/delivery.rs index 1299090a..68f24137 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -26,7 +26,7 @@ use crate::base_fix::BaseFix; use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::{FailedRun, PolicyRefusal, interrupted_or}; use crate::git::Git; -use crate::github::{GitHub, IssueText}; +use crate::github::GitHub; use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; @@ -39,6 +39,7 @@ use crate::run_ending::Cause; use crate::security::review::{Hold as SecurityHold, Outcome as SecurityOutcome, Review}; use crate::session::{Logs, Sessions}; use crate::skills; +use crate::trust::{self, IssueText}; use crate::worktree::{ForeignCommits, Merge, PendingMerge, Preserved, Worktree}; use repair_loop::{Repair, Upstream}; @@ -419,7 +420,7 @@ trait FailedOutside { /// account: a session started now would work from text no Trusted account /// wrote. fn no_untrusted_edit_since(issue: &IssueUrl, started: DateTime) -> Result<()> { - match GitHub::new().untrusted_edit(issue, Some(started))? { + match trust::untrusted_edit(&GitHub::new(), issue, Some(started))? { Some(text) => Err(UntrustedEdit { number: issue.number, text, diff --git a/src/github.rs b/src/github.rs index 3ce89637..17177185 100644 --- a/src/github.rs +++ b/src/github.rs @@ -23,7 +23,6 @@ pub use advisories::{ DraftAdvisory, FindingDraft, FindingProvenance, Package, RecordedFinding, SecurityRecord, SecurityRecords, }; -pub use trust::{IssueText, UntrustedIssue}; #[cfg(test)] mod execution_tests; diff --git a/src/github/session_issue.rs b/src/github/session_issue.rs index 567a90ee..b01df27d 100644 --- a/src/github/session_issue.rs +++ b/src/github/session_issue.rs @@ -4,9 +4,9 @@ use anyhow::{Context, Result}; use serde_json::Value; -use super::trust::trusted_association; use super::{CommandFailure, GitHub}; use crate::issue::IssueUrl; +use crate::trust::trusted_association; /// What stands in for each comment by an account that is not a Trusted /// account. diff --git a/src/github/trust.rs b/src/github/trust.rs index fa552106..ff4bf9f5 100644 --- a/src/github/trust.rs +++ b/src/github/trust.rs @@ -1,6 +1,6 @@ -//! The fixed Trusted account rule, shared by every path that starts work. - -use std::fmt; +//! The GitHub adapter's reads for the Trusted account rule in +//! [`crate::trust`]: the queries, their paging and parsing, and the reading +//! of membership endpoints. use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; @@ -9,157 +9,23 @@ use serde_json::Value; use super::{CommandFailure, GitHub}; use crate::issue::IssueUrl; use crate::labels::READY_FOR_AGENT; +use crate::trust::{Action, IssueText, Reads}; -/// The first trust check an issue failed. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum UntrustedIssue { - Author, - ReadyLabel, - /// Its `IssueText` was edited, after it was last marked ready, by an account - /// that is not a Trusted account. - Edited(IssueText), -} - -impl fmt::Display for UntrustedIssue { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::Author => f.write_str("issue author is not a Trusted account"), - Self::ReadyLabel => { - f.write_str("most recent ready-for-agent label actor is not a Trusted account") - } - Self::Edited(edit) => write!( - f, - "{edit} edited after it was marked ready by an account that is not a Trusted account" - ), - } - } -} - -/// The part of an issue an edit changed: its body or its title. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum IssueText { - Body, - Title, -} - -impl fmt::Display for IssueText { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(match self { - Self::Body => "body", - Self::Title => "title", - }) - } -} - -/// One `ready-for-agent` labelling from an issue's event history. -struct Labelling { - actor: Option, - at: DateTime, -} - -impl GitHub { - /// Check the issue author, the most recent ready-for-agent label actor, - /// and every edit to its body or title made since that labelling. - pub fn issue_trust(&self, issue: &IssueUrl) -> Result> { - if let Some(check) = self.author_trust(issue)? { - return Ok(Some(check)); - } - let Some(Labelling { - actor: Some(actor), - at, - }) = self.ready_labellings(issue)?.pop() - else { - return Ok(Some(UntrustedIssue::ReadyLabel)); - }; - if !self.trusted_actor(issue, &actor)? { - return Ok(Some(UntrustedIssue::ReadyLabel)); - } - Ok(self - .untrusted_edit(issue, Some(at))? - .map(UntrustedIssue::Edited)) - } - - /// Check a Ticket before its Run starts: its author, and every edit to - /// its body or title made since the later of the Ticket's and its - /// `spec`'s last ready-for-agent labellings by a Trusted account, or - /// every edit if neither has one. A Ticket needs no label of its own: - /// its Spec's labelling already passed the full check. - pub fn ticket_trust( - &self, - ticket: &IssueUrl, - spec: &IssueUrl, - ) -> Result> { - if let Some(check) = self.author_trust(ticket)? { - return Ok(Some(check)); - } - let since = self - .trusted_ready_at(ticket)? - .max(self.trusted_ready_at(spec)?); - Ok(self - .untrusted_edit(ticket, since)? - .map(UntrustedIssue::Edited)) - } - - /// The first edit to `issue`'s body or title made after `since`, or ever - /// if `since` is `None`, by an account that is not a Trusted account, if - /// there is one. An edit whose account GitHub no longer knows counts. - pub fn untrusted_edit( - &self, - issue: &IssueUrl, - since: Option>, - ) -> Result> { - let mut trusted: Vec = Vec::new(); - for (edit, query, field) in [ - (IssueText::Body, BODY_EDITS_QUERY, "editedAt"), - (IssueText::Title, TITLE_EDITS_QUERY, "createdAt"), - ] { - for node in self.edits(issue, query)? { - if let Some(since) = since - && time_at(&node, field)? <= since - { - continue; - } - let Some(login) = node["actor"]["login"].as_str() else { - return Ok(Some(edit)); - }; - if trusted - .iter() - .any(|known| known.eq_ignore_ascii_case(login)) - { - continue; - } - if !self.trusted_actor(issue, login)? { - return Ok(Some(edit)); - } - trusted.push(login.to_string()); - } - } - Ok(None) - } - - /// Every edit `query` reads of `issue`, its `edits` connection read page - /// by page so that no edit is missed. - fn edits(&self, issue: &IssueUrl, query: &str) -> Result> { - Ok(self.paged_nodes(issue, query, "edits", "edit history")?.1) - } - - /// When `issue` was last labelled ready-for-agent by a Trusted account, - /// if it ever was. - fn trusted_ready_at(&self, issue: &IssueUrl) -> Result>> { - for labelling in self.ready_labellings(issue)?.into_iter().rev() { - if let Some(actor) = &labelling.actor - && self.trusted_actor(issue, actor)? - { - return Ok(Some(labelling.at)); - } - } - Ok(None) +impl Reads for GitHub { + fn author_association(&self, issue: &IssueUrl) -> Result { + let json = self.gh_json(&[ + "api", + &format!("repos/{}/issues/{}", issue.repo_slug(), issue.number), + ])?; + Ok(json["author_association"] + .as_str() + .context("gh api output has no author_association")? + .to_string()) } - /// Every ready-for-agent labelling of `issue`, oldest first. A Claim - /// removes the label, but its historical event still matters. Every page - /// is read: a later reapplication supersedes an earlier one. - fn ready_labellings(&self, issue: &IssueUrl) -> Result> { + /// A Claim removes the label, but its historical event still matters. + /// Every page is read: a later reapplication supersedes an earlier one. + fn ready_labellings(&self, issue: &IssueUrl) -> Result> { let events = self.gh_api_items( &format!( "repos/{}/issues/{}/events?per_page=100", @@ -176,49 +42,41 @@ impl GitHub { .as_str() .is_some_and(|name| READY_FOR_AGENT.is_named(name)) }) - .map(|event| { - Ok(Labelling { - actor: event["actor"]["login"].as_str().map(str::to_string), - at: time_at(event, "created_at")?, - }) - }) + .map(|event| action(event, "created_at")) .collect() } - /// Check only the issue author. - fn author_trust(&self, issue: &IssueUrl) -> Result> { - let json = self.gh_json(&[ - "api", - &format!("repos/{}/issues/{}", issue.repo_slug(), issue.number), - ])?; - let association = json["author_association"] - .as_str() - .context("gh api output has no author_association")?; - if !trusted_association(association) { - return Ok(Some(UntrustedIssue::Author)); - } - Ok(None) + /// Its `edits` connection is read page by page so that no edit is missed. + fn edits(&self, issue: &IssueUrl, text: IssueText) -> Result> { + let (query, field) = match text { + IssueText::Body => (BODY_EDITS_QUERY, "editedAt"), + IssueText::Title => (TITLE_EDITS_QUERY, "createdAt"), + }; + self.paged_nodes(issue, query, "edits", "edit history")? + .1 + .iter() + .map(|node| action(node, field)) + .collect() } - /// Events have no author association. Check actual membership: public - /// read access alone does not make an actor a repository collaborator. - fn trusted_actor(&self, issue: &IssueUrl, actor: &str) -> Result { - if actor.eq_ignore_ascii_case(&issue.owner) { - return Ok(true); - } + /// Public read access alone does not make an actor a repository + /// collaborator: this checks actual membership. + fn is_member(&self, issue: &IssueUrl, login: &str) -> Result { if self.membership(&format!( - "repos/{}/collaborators/{actor}", + "repos/{}/collaborators/{login}", issue.repo_slug() ))? { return Ok(true); } let repo = self.gh_json(&["api", &format!("repos/{}", issue.repo_slug())])?; if repo["owner"]["type"] == "Organization" { - return self.membership(&format!("orgs/{}/members/{actor}", issue.owner)); + return self.membership(&format!("orgs/{}/members/{login}", issue.owner)); } Ok(false) } +} +impl GitHub { /// Membership endpoints return 204 for a member and 404 otherwise. /// Other failures must stop unattended selection instead of granting trust. fn membership(&self, path: &str) -> Result { @@ -239,11 +97,13 @@ impl GitHub { } } -/// Whether an author's `authorAssociation` with the repository makes them a -/// Trusted account: its owner, a member of the organization that owns it, or -/// a collaborator on it. -pub(super) fn trusted_association(association: &str) -> bool { - matches!(association, "OWNER" | "MEMBER" | "COLLABORATOR") +/// The account that made the event or edit `node`, if GitHub still knows it, +/// and when, from its `field`. +fn action(node: &Value, field: &str) -> Result { + Ok(Action { + actor: node["actor"]["login"].as_str().map(str::to_string), + at: time_at(node, field)?, + }) } /// The time in `node`'s `field`, when an event or edit was made. diff --git a/src/main.rs b/src/main.rs index 8746264d..92c74f72 100644 --- a/src/main.rs +++ b/src/main.rs @@ -42,6 +42,7 @@ mod skills; mod spec_run; #[cfg(test)] mod test_support; +mod trust; mod update; mod worktree; diff --git a/src/ready.rs b/src/ready.rs index c77d1a43..5af1f3a1 100644 --- a/src/ready.rs +++ b/src/ready.rs @@ -10,11 +10,12 @@ use crate::base_fix; use crate::branch::{self, Started}; use crate::claim; use crate::git::Git; -use crate::github::{Candidate, GitHub, ListedIssue, Shaping, UntrustedIssue}; +use crate::github::{Candidate, GitHub, ListedIssue, Shaping}; use crate::issue::{IssueUrl, Repo}; use crate::labels::{Label, READY_FOR_AGENT}; use crate::progress; use crate::spec_run; +use crate::trust::{self, UntrustedIssue}; /// The lowest-numbered Ready issue a search found. pub struct ReadyIssue { @@ -96,7 +97,7 @@ struct GitHubAndOrigin<'a> { impl Reads for GitHubAndOrigin<'_> { fn issue_trust(&self, issue: &IssueUrl) -> Result> { - self.github.issue_trust(issue) + trust::issue_trust(&self.github, issue) } fn candidate(&self, issue: &IssueUrl) -> Result { diff --git a/src/run.rs b/src/run.rs index 8f5f434b..a3073095 100644 --- a/src/run.rs +++ b/src/run.rs @@ -24,6 +24,7 @@ use crate::progress; use crate::prompt; use crate::session::Logs; use crate::spec_run; +use crate::trust; use crate::worktree::Worktree; /// The implement session's kind, in its progress lines and log name. @@ -172,7 +173,7 @@ fn run( ) -> Result { let directory = LaunchDirectory::open_for_run(issue)?; if let StartedBy::Command = started_by { - match GitHub::new().issue_trust(issue) { + match trust::issue_trust(&GitHub::new(), issue) { Ok(Some(check)) => progress::step(format!("warning: #{} {check}", issue.number)), Ok(None) => {} Err(error) => progress::step(format!( diff --git a/src/spec_run.rs b/src/spec_run.rs index f9f7e1ad..5d7599f6 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -28,6 +28,7 @@ use crate::prompt; use crate::pull_request::PullRequest; use crate::recovery::{Preservation, Recovery, Stage, TicketProgress}; use crate::run::Reached; +use crate::trust; use crate::worktree::Worktree; use ticket_board::TicketBoard; @@ -270,7 +271,7 @@ impl Outside for ChildRunsAndGitHub<'_> { let kind = Kind::Ticket { spec_branch: self.worktree().branch().to_string(), }; - if let Some(check) = GitHub::new().ticket_trust(ticket, self.spec)? { + if let Some(check) = trust::ticket_trust(&GitHub::new(), ticket, self.spec)? { bail!("Ticket #{}: {check}", ticket.number); } self.children.start( diff --git a/src/trust.rs b/src/trust.rs new file mode 100644 index 00000000..78924a98 --- /dev/null +++ b/src/trust.rs @@ -0,0 +1,576 @@ +//! The fixed Trusted account rule, shared by every path that starts work. +//! +//! The rule reads GitHub only through [`Reads`], each read made only when the +//! check reaches it: the author first, then the latest `ready-for-agent` +//! labelling, then the edits, body before title, stopping at the first +//! failure. + +use std::fmt; + +use anyhow::Result; +use chrono::{DateTime, Utc}; + +use crate::issue::IssueUrl; + +/// The first trust check an issue failed. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum UntrustedIssue { + Author, + ReadyLabel, + /// Its `IssueText` was edited, after it was last marked ready, by an account + /// that is not a Trusted account. + Edited(IssueText), +} + +impl fmt::Display for UntrustedIssue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Author => f.write_str("issue author is not a Trusted account"), + Self::ReadyLabel => { + f.write_str("most recent ready-for-agent label actor is not a Trusted account") + } + Self::Edited(edit) => write!( + f, + "{edit} edited after it was marked ready by an account that is not a Trusted account" + ), + } + } +} + +/// The part of an issue an edit changed: its body or its title. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum IssueText { + Body, + Title, +} + +impl fmt::Display for IssueText { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(match self { + Self::Body => "body", + Self::Title => "title", + }) + } +} + +/// A `ready-for-agent` labelling or an edit of an issue: the account that +/// made it, if GitHub still knows it, and when. +#[derive(Clone, Debug)] +pub struct Action { + pub actor: Option, + pub at: DateTime, +} + +/// What the rule reads of GitHub about an issue. +pub trait Reads { + /// The `authorAssociation` of `issue`'s author with its repository. + fn author_association(&self, issue: &IssueUrl) -> Result; + /// Every `ready-for-agent` labelling of `issue`, oldest first. + fn ready_labellings(&self, issue: &IssueUrl) -> Result>; + /// Every edit of `issue`'s `text`. + fn edits(&self, issue: &IssueUrl, text: IssueText) -> Result>; + /// Whether `login` is a collaborator on `issue`'s repository or a member + /// of the organization that owns it. + fn is_member(&self, issue: &IssueUrl, login: &str) -> Result; +} + +/// Check the issue author, the most recent ready-for-agent label actor, +/// and every edit to its body or title made since that labelling. +pub fn issue_trust(reads: &impl Reads, issue: &IssueUrl) -> Result> { + let mut check = Check::new(reads); + if !check.trusted_author(issue)? { + return Ok(Some(UntrustedIssue::Author)); + } + let Some(Action { + actor: Some(actor), + at, + }) = reads.ready_labellings(issue)?.pop() + else { + return Ok(Some(UntrustedIssue::ReadyLabel)); + }; + if !check.trusted_actor(issue, &actor)? { + return Ok(Some(UntrustedIssue::ReadyLabel)); + } + Ok(check + .untrusted_edit(issue, Some(at))? + .map(UntrustedIssue::Edited)) +} + +/// Check a Ticket before its Run starts: its author, and every edit to +/// its body or title made since the later of the Ticket's and its +/// `spec`'s last ready-for-agent labellings by a Trusted account, or +/// every edit if neither has one. A Ticket needs no label of its own: +/// its Spec's labelling already passed the full check. +pub fn ticket_trust( + reads: &impl Reads, + ticket: &IssueUrl, + spec: &IssueUrl, +) -> Result> { + let mut check = Check::new(reads); + if !check.trusted_author(ticket)? { + return Ok(Some(UntrustedIssue::Author)); + } + let since = check + .trusted_ready_at(ticket)? + .max(check.trusted_ready_at(spec)?); + Ok(check + .untrusted_edit(ticket, since)? + .map(UntrustedIssue::Edited)) +} + +/// The first edit to `issue`'s body or title made after `since`, or ever +/// if `since` is `None`, by an account that is not a Trusted account, if +/// there is one. An edit whose account GitHub no longer knows counts. +pub fn untrusted_edit( + reads: &impl Reads, + issue: &IssueUrl, + since: Option>, +) -> Result> { + Check::new(reads).untrusted_edit(issue, since) +} + +/// Whether an author's `authorAssociation` with the repository makes them a +/// Trusted account: its owner, a member of the organization that owns it, or +/// a collaborator on it. +pub fn trusted_association(association: &str) -> bool { + matches!(association, "OWNER" | "MEMBER" | "COLLABORATOR") +} + +/// One top-level check, with each account it has found trusted so far. +struct Check<'a, R> { + reads: &'a R, + /// Each account found trusted, with the repository it is trusted on. + trusted: Vec<(String, String)>, +} + +impl<'a, R: Reads> Check<'a, R> { + fn new(reads: &'a R) -> Self { + Self { + reads, + trusted: Vec::new(), + } + } + + fn trusted_author(&self, issue: &IssueUrl) -> Result { + Ok(trusted_association(&self.reads.author_association(issue)?)) + } + + fn untrusted_edit( + &mut self, + issue: &IssueUrl, + since: Option>, + ) -> Result> { + for text in [IssueText::Body, IssueText::Title] { + for edit in self.reads.edits(issue, text)? { + if since.is_some_and(|since| edit.at <= since) { + continue; + } + let Some(actor) = edit.actor else { + return Ok(Some(text)); + }; + if !self.trusted_actor(issue, &actor)? { + return Ok(Some(text)); + } + } + } + Ok(None) + } + + /// When `issue` was last labelled ready-for-agent by a Trusted account, + /// if it ever was. + fn trusted_ready_at(&mut self, issue: &IssueUrl) -> Result>> { + for labelling in self.reads.ready_labellings(issue)?.into_iter().rev() { + if let Some(actor) = &labelling.actor + && self.trusted_actor(issue, actor)? + { + return Ok(Some(labelling.at)); + } + } + Ok(None) + } + + /// Labellings and edits have no author association: the repository's + /// owner is trusted, and anyone else only by actual membership. + fn trusted_actor(&mut self, issue: &IssueUrl, actor: &str) -> Result { + if actor.eq_ignore_ascii_case(&issue.owner) { + return Ok(true); + } + let repo = issue.repo_slug(); + if self.trusted.iter().any(|(known_repo, known)| { + known_repo.eq_ignore_ascii_case(&repo) && known.eq_ignore_ascii_case(actor) + }) { + return Ok(true); + } + if !self.reads.is_member(issue, actor)? { + return Ok(false); + } + self.trusted.push((repo, actor.to_string())); + Ok(true) + } +} + +#[cfg(test)] +mod tests { + use std::cell::RefCell; + use std::collections::HashMap; + + use anyhow::bail; + + use super::*; + + const ISSUE: u64 = 7; + const SPEC: u64 = 20; + + fn url(number: u64) -> IssueUrl { + IssueUrl::parse(&format!("https://github.com/acme/widgets/issues/{number}")).unwrap() + } + + fn at(day: u32) -> DateTime { + DateTime::parse_from_rfc3339(&format!("2020-01-{day:02}T00:00:00Z")) + .unwrap() + .to_utc() + } + + fn by(actor: &str, day: u32) -> Action { + Action { + actor: Some(actor.to_string()), + at: at(day), + } + } + + fn unknown(day: u32) -> Action { + Action { + actor: None, + at: at(day), + } + } + + /// An issue's facts as GitHub would give them. + #[derive(Default)] + struct Issue { + association: Option<&'static str>, + labellings: Vec, + body_edits: Vec, + title_edits: Vec, + } + + /// Reads from memory: `reader` and `member` are members, everyone else + /// is not, and `broken` fails its membership read. Every membership read + /// is recorded in `looked_up`. + #[derive(Default)] + struct InMemory { + issues: HashMap, + looked_up: RefCell>, + edits_read: RefCell>, + } + + impl InMemory { + fn with(issue: Issue) -> Self { + Self::default().and(ISSUE, issue) + } + + fn and(mut self, number: u64, issue: Issue) -> Self { + self.issues.insert(number, issue); + self + } + + fn issue(&self, issue: &IssueUrl) -> &Issue { + &self.issues[&issue.number] + } + } + + impl Reads for InMemory { + fn author_association(&self, issue: &IssueUrl) -> Result { + Ok(self.issue(issue).association.unwrap_or("OWNER").to_string()) + } + + fn ready_labellings(&self, issue: &IssueUrl) -> Result> { + Ok(self.issue(issue).labellings.clone()) + } + + fn edits(&self, issue: &IssueUrl, text: IssueText) -> Result> { + self.edits_read.borrow_mut().push(text); + let issue = self.issue(issue); + Ok(match text { + IssueText::Body => issue.body_edits.clone(), + IssueText::Title => issue.title_edits.clone(), + }) + } + + fn is_member(&self, _issue: &IssueUrl, login: &str) -> Result { + self.looked_up.borrow_mut().push(login.to_string()); + match login { + "reader" | "member" => Ok(true), + "broken" => bail!("could not check Trusted account membership"), + _ => Ok(false), + } + } + } + + /// An issue by a Trusted author, last marked ready by `reader` on day 1. + fn ready() -> Issue { + Issue { + labellings: vec![by("reader", 1)], + ..Issue::default() + } + } + + fn issue_trust_of(reads: &InMemory) -> Option { + issue_trust(reads, &url(ISSUE)).unwrap() + } + + fn ticket_trust_of(reads: &InMemory) -> Option { + ticket_trust(reads, &url(ISSUE), &url(SPEC)).unwrap() + } + + #[test] + fn an_issue_by_an_untrusted_author_is_untrusted_without_reading_further() { + for association in ["NONE", "CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR"] { + let reads = InMemory::with(Issue { + association: Some(association), + ..ready() + }); + + assert_eq!(issue_trust_of(&reads), Some(UntrustedIssue::Author)); + assert!(reads.looked_up.borrow().is_empty()); + assert!(reads.edits_read.borrow().is_empty()); + } + } + + #[test] + fn trusted_associations_are_the_owner_members_and_collaborators() { + for association in ["OWNER", "MEMBER", "COLLABORATOR"] { + assert!(trusted_association(association), "{association}"); + } + for association in ["CONTRIBUTOR", "NONE", "owner", ""] { + assert!(!trusted_association(association), "{association}"); + } + } + + #[test] + fn an_issue_never_labelled_ready_is_untrusted() { + let reads = InMemory::with(Issue::default()); + + assert_eq!(issue_trust_of(&reads), Some(UntrustedIssue::ReadyLabel)); + assert!(reads.edits_read.borrow().is_empty()); + } + + #[test] + fn a_latest_labelling_with_no_actor_is_untrusted() { + let reads = InMemory::with(Issue { + labellings: vec![by("acme", 1), unknown(2)], + ..Issue::default() + }); + + assert_eq!(issue_trust_of(&reads), Some(UntrustedIssue::ReadyLabel)); + } + + #[test] + fn a_latest_labelling_by_an_untrusted_actor_is_untrusted_after_an_earlier_trusted_one() { + let reads = InMemory::with(Issue { + labellings: vec![by("acme", 1), by("outsider", 2)], + ..Issue::default() + }); + + assert_eq!(issue_trust_of(&reads), Some(UntrustedIssue::ReadyLabel)); + assert!(reads.edits_read.borrow().is_empty()); + } + + #[test] + fn a_latest_trusted_labelling_supersedes_an_earlier_untrusted_one() { + let reads = InMemory::with(Issue { + labellings: vec![by("outsider", 1), by("reader", 2)], + ..Issue::default() + }); + + assert_eq!(issue_trust_of(&reads), None); + } + + #[test] + fn edits_at_or_before_the_labelling_are_ignored() { + let reads = InMemory::with(Issue { + body_edits: vec![by("outsider", 1), unknown(1)], + title_edits: vec![by("outsider", 1)], + ..ready() + }); + + assert_eq!(issue_trust_of(&reads), None); + assert_eq!(*reads.looked_up.borrow(), ["reader"]); + } + + #[test] + fn an_edit_after_the_labelling_by_a_trusted_account_leaves_it_trusted() { + let reads = InMemory::with(Issue { + body_edits: vec![by("member", 2), by("ACME", 3)], + title_edits: vec![by("reader", 2)], + ..ready() + }); + + assert_eq!(issue_trust_of(&reads), None); + } + + #[test] + fn an_edit_after_the_labelling_by_an_untrusted_or_unknown_account_is_untrusted() { + for edit in [by("outsider", 2), unknown(2)] { + let reads = InMemory::with(Issue { + body_edits: vec![edit], + ..ready() + }); + + assert_eq!( + issue_trust_of(&reads), + Some(UntrustedIssue::Edited(IssueText::Body)) + ); + assert_eq!( + *reads.edits_read.borrow(), + [IssueText::Body], + "no title edits read after a failing body edit" + ); + } + } + + #[test] + fn a_title_edit_after_the_labelling_by_an_untrusted_account_is_untrusted() { + let reads = InMemory::with(Issue { + body_edits: vec![by("reader", 2)], + title_edits: vec![by("outsider", 3)], + ..ready() + }); + + assert_eq!( + issue_trust_of(&reads), + Some(UntrustedIssue::Edited(IssueText::Title)) + ); + assert_eq!( + *reads.edits_read.borrow(), + [IssueText::Body, IssueText::Title] + ); + } + + #[test] + fn a_ticket_counts_edits_since_the_later_of_its_own_and_its_specs_trusted_labellings() { + for (ticket_labellings, spec_labellings, trusted) in [ + // The Spec's labelling is later than the Ticket's. + (vec![by("acme", 1)], vec![by("acme", 3)], true), + // The Ticket's own labelling is later than the Spec's. + (vec![by("acme", 3)], vec![by("acme", 1)], true), + // Neither's labelling is late enough. + (vec![by("acme", 1)], vec![by("acme", 1)], false), + // Only trusted labellings count, not a later untrusted one. + (vec![], vec![by("acme", 1), by("outsider", 4)], false), + ] { + let reads = InMemory::with(Issue { + labellings: ticket_labellings, + body_edits: vec![by("outsider", 2)], + ..Issue::default() + }) + .and( + SPEC, + Issue { + labellings: spec_labellings, + ..Issue::default() + }, + ); + + let expected = (!trusted).then_some(UntrustedIssue::Edited(IssueText::Body)); + assert_eq!(ticket_trust_of(&reads), expected); + } + } + + #[test] + fn a_ticket_with_no_trusted_labelling_counts_every_edit() { + for (ticket_labellings, edit, trusted) in [ + (vec![], by("outsider", 1), false), + (vec![by("outsider", 3)], by("outsider", 1), false), + (vec![], by("reader", 1), true), + ] { + let reads = InMemory::with(Issue { + labellings: ticket_labellings, + title_edits: vec![edit], + ..Issue::default() + }) + .and(SPEC, Issue::default()); + + let expected = (!trusted).then_some(UntrustedIssue::Edited(IssueText::Title)); + assert_eq!(ticket_trust_of(&reads), expected); + } + } + + #[test] + fn a_ticket_by_an_untrusted_author_is_untrusted() { + let reads = InMemory::with(Issue { + association: Some("NONE"), + ..Issue::default() + }) + .and(SPEC, ready()); + + assert_eq!(ticket_trust_of(&reads), Some(UntrustedIssue::Author)); + assert!(reads.looked_up.borrow().is_empty()); + } + + #[test] + fn the_repository_owner_is_trusted_in_any_case_with_no_read() { + let reads = InMemory::with(Issue { + labellings: vec![by("ACME", 1)], + body_edits: vec![by("Acme", 2)], + title_edits: vec![by("acme", 2)], + ..Issue::default() + }); + + assert_eq!(issue_trust_of(&reads), None); + assert!(reads.looked_up.borrow().is_empty()); + } + + #[test] + fn an_account_found_trusted_is_not_looked_up_again_within_one_check() { + let reads = InMemory::with(Issue { + labellings: vec![by("reader", 1)], + body_edits: vec![by("reader", 2), by("READER", 3)], + title_edits: vec![by("reader", 2), by("member", 3), by("member", 4)], + ..Issue::default() + }); + + assert_eq!(issue_trust_of(&reads), None); + assert_eq!(*reads.looked_up.borrow(), ["reader", "member"]); + } + + #[test] + fn a_failed_read_propagates_and_never_grants_trust() { + for issue in [ + Issue { + labellings: vec![by("broken", 1)], + ..Issue::default() + }, + Issue { + body_edits: vec![by("broken", 2)], + ..ready() + }, + ] { + let reads = InMemory::with(issue); + + let error = issue_trust(&reads, &url(ISSUE)).unwrap_err(); + + assert!( + format!("{error:#}").contains("could not check Trusted account membership"), + "{error:#}" + ); + } + } + + #[test] + fn untrusted_edit_with_no_time_counts_every_edit() { + let reads = InMemory::with(Issue { + title_edits: vec![by("outsider", 1)], + ..Issue::default() + }); + + assert_eq!( + untrusted_edit(&reads, &url(ISSUE), None).unwrap(), + Some(IssueText::Title) + ); + assert_eq!( + untrusted_edit(&reads, &url(ISSUE), Some(at(1))).unwrap(), + None + ); + } +} diff --git a/tests/fresh_run.rs b/tests/fresh_run.rs index c9f41741..229f9921 100644 --- a/tests/fresh_run.rs +++ b/tests/fresh_run.rs @@ -84,28 +84,6 @@ fn a_direct_run_warns_about_an_untrusted_author_and_still_delivers_its_pr() { assert_eq!(scenario.claude_calls().len(), 1); } -#[test] -fn a_direct_run_warns_about_missing_ready_label_history_and_still_delivers_its_pr() { - let scenario = Scenario::new(); - scenario.agent_does(AGENT_COMMITS_AND_OPENS_PR); - let mut state = scenario.gh_state(); - state["trust_events"] = serde_json::json!({"7": []}); - scenario.write_gh_state(&state); - - let result = scenario.run(&[&scenario.issue_url(7)]); - - assert_eq!(result.code, Some(0), "{}", result.stderr); - assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); - assert!( - result.stderr.contains( - "warning: #7 most recent ready-for-agent label actor is not a Trusted account" - ), - "{}", - result.stderr - ); - assert_eq!(scenario.claude_calls().len(), 1); -} - #[test] fn a_direct_run_warns_when_trust_cannot_be_checked_and_still_delivers_its_pr() { let scenario = Scenario::new(); diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 4034b499..660716d0 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -1075,25 +1075,6 @@ fn an_untrusted_edit_while_the_run_is_under_way_starts_no_resume() { assert_eq!(scenario.claude_calls().len(), 1, "no Resume session"); } -#[test] -fn a_trusted_edit_while_the_run_is_under_way_leaves_its_repairs_running() { - let scenario = Scenario::new(); - scenario.agent_does_in_session( - 1, - &format!( - "{AGENT_OPENS_PR}{}{}", - checks_on_head(RED), - edits_issue_7("title", "acme") - ), - ); - scenario.agent_does_in_session(2, "echo fixed > feature.txt\ngit commit -q -am 'Fix CI'\n"); - - let result = scenario.run(&["merge", &scenario.issue_url(7)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(scenario.claude_calls().len(), 2); -} - #[test] fn an_untrusted_edit_dated_just_before_the_runs_local_start_still_stops_repairs() { let scenario = Scenario::new(); diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs index 70294719..e8d58fe9 100644 --- a/tests/pickup_run.rs +++ b/tests/pickup_run.rs @@ -1921,63 +1921,8 @@ fn an_issue_written_by_an_untrusted_account_is_passed_over() { } #[test] -fn an_issue_labelled_by_an_untrusted_account_is_passed_over() { - let scenario = Scenario::new(); - ready_issue(&scenario, 7, &[]); - let mut state = scenario.gh_state(); - state["trust_events"] = serde_json::json!({"7": [ - {"created_at": "2020-01-01T00:00:00Z", "event": "labeled", "label": {"name": "ready-for-agent"}, - "actor": {"login": "acme"}}, - {"created_at": "2020-01-01T00:00:00Z", "event": "unlabeled", "label": {"name": "ready-for-agent"}, - "actor": {"login": "acme"}}, - {"created_at": "2020-01-01T00:00:00Z", "event": "labeled", "label": {"name": "ready-for-agent"}, - "actor": {"login": "outsider"}} - ]}); - scenario.write_gh_state(&state); - - let result = scenario.run(&["pickup"]); - - assert_skipped( - &scenario, - &result, - &no_ready_issue_after(&[ - "#7 most recent ready-for-agent label actor is not a Trusted account", - ]), - ); - assert!(scenario.claude_calls().is_empty()); -} - -#[test] -fn missing_ready_label_history_or_actor_cannot_authorize_pickup() { - for events in [ - serde_json::json!([]), - serde_json::json!([ - {"created_at": "2020-01-01T00:00:00Z", "event": "labeled", "label": {"name": "ready-for-agent"}, "actor": null} - ]), - ] { - let scenario = Scenario::new(); - ready_issue(&scenario, 7, &[]); - let mut state = scenario.gh_state(); - state["trust_events"] = serde_json::json!({"7": events}); - scenario.write_gh_state(&state); - - let result = scenario.run(&["pickup"]); - - assert_skipped( - &scenario, - &result, - &no_ready_issue_after(&[ - "#7 most recent ready-for-agent label actor is not a Trusted account", - ]), - ); - assert!(scenario.claude_calls().is_empty()); - } -} - -#[test] -fn trusted_authors_and_the_latest_trusted_ready_label_authorize_pickup() { +fn ready_labels_are_read_from_label_events_and_their_actors_trusted_by_membership() { for (association, actor, owner_type, collaborators, members) in [ - ("OWNER", "ACME", "User", vec![], vec![]), ("COLLABORATOR", "reader", "User", vec!["reader"], vec![]), ("MEMBER", "member", "Organization", vec![], vec!["member"]), ] { @@ -2005,42 +1950,40 @@ fn trusted_authors_and_the_latest_trusted_ready_label_authorize_pickup() { } #[test] -fn an_issue_edited_by_an_untrusted_account_after_it_was_marked_ready_is_passed_over() { - for (kind, line) in [ - ( - "body", - "#7 body edited after it was marked ready by an account that is not a Trusted account", - ), - ( - "title", - "#7 title edited after it was marked ready by an account that is not a Trusted account", - ), - ] { +fn an_edit_whose_account_github_no_longer_knows_is_untrusted() { + for kind in ["body", "title"] { let scenario = Scenario::new(); ready_issue(&scenario, 7, &[]); let mut state = scenario.gh_state(); state["edits"] = serde_json::json!({"7": [ - {"kind": kind, "by": "outsider", "at": "2019-12-31T00:00:00Z"}, - {"kind": kind, "by": "acme", "at": "2020-01-02T00:00:00Z"}, - {"kind": kind, "by": "outsider", "at": "2020-01-03T00:00:00Z"} + {"kind": kind, "by": null, "at": "2020-01-03T00:00:00Z"} ]}); scenario.write_gh_state(&state); let result = scenario.run(&["pickup"]); - assert_skipped(&scenario, &result, &no_ready_issue_after(&[line])); - assert!(scenario.claude_calls().is_empty()); + assert_skipped( + &scenario, + &result, + &no_ready_issue_after(&[&format!( + "#7 {kind} edited after it was marked ready by an account that is not a Trusted account" + )]), + ); } } #[test] -fn an_edit_whose_account_github_no_longer_knows_is_untrusted() { +fn an_untrusted_edit_past_the_first_page_of_edit_history_is_found() { let scenario = Scenario::new(); ready_issue(&scenario, 7, &[]); let mut state = scenario.gh_state(); - state["edits"] = serde_json::json!({"7": [ - {"kind": "body", "by": null, "at": "2020-01-03T00:00:00Z"} - ]}); + let mut edits: Vec<_> = (0..100) + .map(|_| serde_json::json!({"kind": "body", "by": "acme", "at": "2020-01-02T00:00:00Z"})) + .collect(); + edits.push(serde_json::json!( + {"kind": "body", "by": "outsider", "at": "2020-01-03T00:00:00Z"} + )); + state["edits"] = serde_json::json!({ "7": edits }); scenario.write_gh_state(&state); let result = scenario.run(&["pickup"]); @@ -2054,40 +1997,6 @@ fn an_edit_whose_account_github_no_longer_knows_is_untrusted() { ); } -#[test] -fn edits_by_trusted_accounts_or_before_the_trusted_ready_label_leave_an_issue_ready() { - for relabelled in [false, true] { - let scenario = Scenario::new(); - ready_issue(&scenario, 7, &[]); - scenario.agent_does_for(7, &agent_opens_pr(7, "main")); - let mut state = scenario.gh_state(); - state["collaborators"] = serde_json::json!(["reader"]); - let mut edits = vec![ - serde_json::json!({"kind": "body", "by": "reader", "at": "2020-01-02T00:00:00Z"}), - serde_json::json!({"kind": "title", "by": "acme", "at": "2020-01-02T00:00:00Z"}), - ]; - if relabelled { - // An outsider's edit, then a Trusted account marking it ready again. - edits.push(serde_json::json!( - {"kind": "body", "by": "outsider", "at": "2020-01-03T00:00:00Z"} - )); - state["trust_events"] = serde_json::json!({"7": [ - {"created_at": "2020-01-01T00:00:00Z", "event": "labeled", - "label": {"name": "ready-for-agent"}, "actor": {"login": "acme"}}, - {"created_at": "2020-01-04T00:00:00Z", "event": "labeled", - "label": {"name": "ready-for-agent"}, "actor": {"login": "reader"}} - ]}); - } - state["edits"] = serde_json::json!({ "7": edits }); - scenario.write_gh_state(&state); - - let result = scenario.run(&["pickup"]); - - assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); - assert!(!result.stderr.contains("warning:"), "{}", result.stderr); - } -} - #[test] fn a_failed_label_actor_membership_lookup_starts_no_work() { let scenario = Scenario::new(); diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 67b5bc23..5e2ade05 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -409,78 +409,6 @@ fn a_spec_stops_before_starting_a_ticket_written_by_an_untrusted_account() { assert!(scenario.origin_log("issue-21").is_none()); } -#[test] -fn a_spec_stops_before_starting_a_ticket_edited_by_an_untrusted_account_after_it_was_marked_ready() -{ - let scenario = linear_spec(); - let mut state = scenario.gh_state(); - // The Spec was marked ready on 2020-01-02, after the Ticket's own label. - state["trust_events"] = serde_json::json!({"20": [ - {"created_at": "2020-01-02T00:00:00Z", "event": "labeled", - "label": {"name": "ready-for-agent"}, "actor": {"login": "acme"}} - ]}); - state["edits"] = serde_json::json!({"21": [ - {"kind": "title", "by": "outsider", "at": "2020-01-03T00:00:00Z"} - ]}); - scenario.write_gh_state(&state); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_eq!(result.code, Some(1), "{}", result.stderr); - assert_contains( - &result.stderr, - "Ticket #21: title edited after it was marked ready by an account that is not a Trusted account", - ); - assert!(scenario.claude_calls().is_empty()); - assert!(scenario.origin_log("issue-21").is_none()); -} - -#[test] -fn a_ticket_edited_by_an_untrusted_account_before_its_spec_was_marked_ready_starts() { - let scenario = linear_spec(); - let mut state = scenario.gh_state(); - // The later labelling, by an outsider, is not one that counts. - state["trust_events"] = serde_json::json!({"20": [ - {"created_at": "2020-01-04T00:00:00Z", "event": "labeled", - "label": {"name": "ready-for-agent"}, "actor": {"login": "acme"}}, - {"created_at": "2020-01-05T00:00:00Z", "event": "labeled", - "label": {"name": "ready-for-agent"}, "actor": {"login": "outsider"}} - ]}); - state["edits"] = serde_json::json!({"21": [ - {"kind": "body", "by": "outsider", "at": "2020-01-03T00:00:00Z"} - ]}); - scenario.write_gh_state(&state); - - let result = scenario.run(&["no-email", &spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(!result.stderr.contains("edited"), "{}", result.stderr); -} - -#[test] -fn a_ticket_needs_only_a_trusted_author_not_a_ready_label_of_its_own() { - for events in [ - serde_json::json!([]), - serde_json::json!([ - {"created_at": "2020-01-01T00:00:00Z", "event": "labeled", "label": {"name": "ready-for-agent"}, "actor": {"login": "outsider"}} - ]), - ] { - let scenario = linear_spec(); - let mut state = scenario.gh_state(); - state["trust_events"] = serde_json::json!({"21": events, "22": events}); - scenario.write_gh_state(&state); - - let result = scenario.run(&["no-email", &spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!( - !result.stderr.contains("Trusted account"), - "{}", - result.stderr - ); - } -} - #[test] fn security_review_runs_once_after_spec_review_over_the_whole_spec_branch() { let scenario = linear_spec(); From ab6fac93b166cf538f1731ae0b1a13e03c9773d2 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 19 Oct 2026 23:49:37 -0400 Subject: [PATCH 2/2] Read each edit only when the trust rule reaches it An unreadable edit after the first untrusted one failed the whole check once edits were parsed up front; main returned the untrusted verdict. Reads::edits now yields each edit as the rule reaches it. The Spec run's wiring test for its Spec's labelling is back, and a ready labelling with no actor is read through the GitHub adapter in a test again. Co-Authored-By: Claude Opus 5.5 --- src/github/trust.rs | 14 ++++++++------ src/trust.rs | 22 +++++++++++++++++----- tests/pickup_run.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ tests/spec_run.rs | 22 ++++++++++++++++++++++ 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/github/trust.rs b/src/github/trust.rs index ff4bf9f5..77e52420 100644 --- a/src/github/trust.rs +++ b/src/github/trust.rs @@ -47,16 +47,18 @@ impl Reads for GitHub { } /// Its `edits` connection is read page by page so that no edit is missed. - fn edits(&self, issue: &IssueUrl, text: IssueText) -> Result> { + /// Each edit is parsed only when the rule reaches it. + fn edits( + &self, + issue: &IssueUrl, + text: IssueText, + ) -> Result>> { let (query, field) = match text { IssueText::Body => (BODY_EDITS_QUERY, "editedAt"), IssueText::Title => (TITLE_EDITS_QUERY, "createdAt"), }; - self.paged_nodes(issue, query, "edits", "edit history")? - .1 - .iter() - .map(|node| action(node, field)) - .collect() + let nodes = self.paged_nodes(issue, query, "edits", "edit history")?.1; + Ok(nodes.into_iter().map(move |node| action(&node, field))) } /// Public read access alone does not make an actor a repository diff --git a/src/trust.rs b/src/trust.rs index 78924a98..45f3a2a2 100644 --- a/src/trust.rs +++ b/src/trust.rs @@ -67,8 +67,14 @@ pub trait Reads { fn author_association(&self, issue: &IssueUrl) -> Result; /// Every `ready-for-agent` labelling of `issue`, oldest first. fn ready_labellings(&self, issue: &IssueUrl) -> Result>; - /// Every edit of `issue`'s `text`. - fn edits(&self, issue: &IssueUrl, text: IssueText) -> Result>; + /// Every edit of `issue`'s `text`, each read only when + /// the rule reaches it: an unreadable edit after the first untrusted + /// one cannot fail the check. + fn edits( + &self, + issue: &IssueUrl, + text: IssueText, + ) -> Result>>; /// Whether `login` is a collaborator on `issue`'s repository or a member /// of the organization that owns it. fn is_member(&self, issue: &IssueUrl, login: &str) -> Result; @@ -162,6 +168,7 @@ impl<'a, R: Reads> Check<'a, R> { ) -> Result> { for text in [IssueText::Body, IssueText::Title] { for edit in self.reads.edits(issue, text)? { + let edit = edit?; if since.is_some_and(|since| edit.at <= since) { continue; } @@ -288,13 +295,18 @@ mod tests { Ok(self.issue(issue).labellings.clone()) } - fn edits(&self, issue: &IssueUrl, text: IssueText) -> Result> { + fn edits( + &self, + issue: &IssueUrl, + text: IssueText, + ) -> Result>> { self.edits_read.borrow_mut().push(text); let issue = self.issue(issue); - Ok(match text { + let edits = match text { IssueText::Body => issue.body_edits.clone(), IssueText::Title => issue.title_edits.clone(), - }) + }; + Ok(edits.into_iter().map(Ok)) } fn is_member(&self, _issue: &IssueUrl, login: &str) -> Result { diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs index e8d58fe9..86037279 100644 --- a/tests/pickup_run.rs +++ b/tests/pickup_run.rs @@ -1972,6 +1972,49 @@ fn an_edit_whose_account_github_no_longer_knows_is_untrusted() { } } +#[test] +fn an_untrusted_edit_is_found_before_a_later_edit_with_no_time() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + let mut state = scenario.gh_state(); + state["edits"] = serde_json::json!({"7": [ + {"kind": "body", "by": "outsider", "at": "2020-01-03T00:00:00Z"}, + {"kind": "body", "by": "acme", "at": null} + ]}); + scenario.write_gh_state(&state); + + let result = scenario.run(&["pickup"]); + + assert_skipped( + &scenario, + &result, + &no_ready_issue_after(&[ + "#7 body edited after it was marked ready by an account that is not a Trusted account", + ]), + ); +} + +#[test] +fn a_ready_labelling_whose_account_github_no_longer_knows_is_untrusted() { + let scenario = Scenario::new(); + ready_issue(&scenario, 7, &[]); + let mut state = scenario.gh_state(); + state["trust_events"] = serde_json::json!({"7": [ + {"created_at": "2020-01-01T00:00:00Z", "event": "labeled", "label": {"name": "ready-for-agent"}, "actor": null} + ]}); + scenario.write_gh_state(&state); + + let result = scenario.run(&["pickup"]); + + assert_skipped( + &scenario, + &result, + &no_ready_issue_after(&[ + "#7 most recent ready-for-agent label actor is not a Trusted account", + ]), + ); +} + #[test] fn an_untrusted_edit_past_the_first_page_of_edit_history_is_found() { let scenario = Scenario::new(); diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 5e2ade05..22465e78 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -409,6 +409,28 @@ fn a_spec_stops_before_starting_a_ticket_written_by_an_untrusted_account() { assert!(scenario.origin_log("issue-21").is_none()); } +#[test] +fn a_ticket_edited_by_an_untrusted_account_before_its_spec_was_marked_ready_starts() { + let scenario = linear_spec(); + let mut state = scenario.gh_state(); + // The later labelling, by an outsider, is not one that counts. + state["trust_events"] = serde_json::json!({"20": [ + {"created_at": "2020-01-04T00:00:00Z", "event": "labeled", + "label": {"name": "ready-for-agent"}, "actor": {"login": "acme"}}, + {"created_at": "2020-01-05T00:00:00Z", "event": "labeled", + "label": {"name": "ready-for-agent"}, "actor": {"login": "outsider"}} + ]}); + state["edits"] = serde_json::json!({"21": [ + {"kind": "body", "by": "outsider", "at": "2020-01-03T00:00:00Z"} + ]}); + scenario.write_gh_state(&state); + + let result = scenario.run(&["no-email", &spec_url(&scenario)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!(!result.stderr.contains("edited"), "{}", result.stderr); +} + #[test] fn security_review_runs_once_after_spec_review_over_the_whole_spec_branch() { let scenario = linear_spec();