From dd51f339a6388f46f0732474fc8aa19b67434620 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sat, 17 Oct 2026 18:25:30 +0000 Subject: [PATCH 1/8] Report Security pre-audit failures truthfully (#740) --- src/github.rs | 23 +++++++--- src/github/failure.rs | 39 ++++++++++++++++ src/notification.rs | 31 +++++++++++-- src/run_ending.rs | 25 +++++++++- src/security.rs | 6 +++ tests/fakes/gh.rs | 9 +++- tests/security_run.rs | 103 +++++++++++++++++++++++++++++++++++++++++- 7 files changed, 222 insertions(+), 14 deletions(-) create mode 100644 src/github/failure.rs diff --git a/src/github.rs b/src/github.rs index f32fe850..291a955f 100644 --- a/src/github.rs +++ b/src/github.rs @@ -12,6 +12,9 @@ use crate::issue::{IssueUrl, Repo}; use crate::labels::{Label, Labels}; use crate::process::{self, Control, Interruption}; +mod failure; +pub(crate) use failure::safe_cause; + mod check_read; pub(crate) use check_read::is_transient as check_read_is_transient; @@ -760,15 +763,21 @@ impl GitHub { /// it exits non-zero. fn gh_json(&self, args: &[&str]) -> Result { let command = format!("gh {}", args[..2].join(" ")); - let output = self.output(args)?; + let output = self.output(args).with_context(|| failure::QueryFailure { + message: format!("{command} could not run"), + stderr: None, + })?; if !output.status.success() { - bail!( - "{command} failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + bail!(failure::QueryFailure { + message: format!("{command} failed: {stderr}"), + stderr: Some(stderr), + }); } - serde_json::from_slice(&output.stdout) - .with_context(|| format!("{command} returned invalid JSON")) + serde_json::from_slice(&output.stdout).with_context(|| failure::QueryFailure { + message: format!("{command} returned invalid JSON"), + stderr: None, + }) } fn output(&self, args: &[&str]) -> Result { self.output_with_input(args, None) diff --git a/src/github/failure.rs b/src/github/failure.rs new file mode 100644 index 00000000..d89b07f9 --- /dev/null +++ b/src/github/failure.rs @@ -0,0 +1,39 @@ +//! Safe operational categories, derived only from GitHub response evidence. + +use std::fmt; + +#[derive(Debug)] +pub(super) struct QueryFailure { + pub message: String, + pub stderr: Option, +} + +impl fmt::Display for QueryFailure { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.message) + } +} + +impl std::error::Error for QueryFailure {} + +/// Never return diagnostics: GitHub may include credentials or private data. +pub(crate) fn safe_cause(error: &anyhow::Error) -> Option<&'static str> { + let stderr = if let Some(failure) = error.downcast_ref::() { + failure.stderr.as_deref().unwrap_or_default() + } else if let Some(failure) = error.downcast_ref::() { + &failure.stderr + } else { + return None; + }; + let stderr = stderr.to_ascii_lowercase(); + Some( + if ["api rate limit exceeded", "api rate limit already exceeded"] + .iter() + .any(|message| stderr.contains(message)) + { + "GitHub rate-limit exhaustion" + } else { + "GitHub dependency failure" + }, + ) +} diff --git a/src/notification.rs b/src/notification.rs index ff15f1e2..bca48294 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -34,6 +34,7 @@ pub enum NotificationAsk { struct Checked { resend: Resend, started: Instant, + started_at: chrono::DateTime, } impl Checked { @@ -42,8 +43,13 @@ impl Checked { /// address is known and a Resend API key is found. fn new(to: Option, settings: &EmailSettings) -> Result { let started = Instant::now(); + let started_at = chrono::Utc::now(); let resend = Resend::new(to, settings)?; - Ok(Checked { resend, started }) + Ok(Checked { + resend, + started, + started_at, + }) } } @@ -151,6 +157,13 @@ impl RunNotification { host.as_deref().unwrap_or("unknown host"), self.checked.started.elapsed(), ); + if account.pre_audit.is_some() { + text += &format!( + "Diagnostics: Check this command's stderr on {} around {} UTC; for a scheduled command, check the scheduler's captured stderr.\n", + host.as_deref().unwrap_or("unknown host"), + self.checked.started_at.format("%Y-%m-%d %H:%M:%S"), + ); + } let lines = crate::session::recording::Recording::capture().notification_lines(); if !lines.is_empty() { text += "\nSession models:\n"; @@ -226,6 +239,7 @@ fn body( } if let Some(findings) = account.security_findings { let audit = match &account.ended { + _ if account.pre_audit.is_some() => "not started", Err(_) if account.failed_reproduction.is_some() => "audit complete", Ok(line) => line.as_str(), Err(_) => account.outcome, @@ -273,12 +287,12 @@ fn body( text += &format!("Pull request: {pr_url}\n"); } // Security failure causes can quote private verifier work or Harness - // diagnostics. Only the fixed safeguard category is safe to send. + // diagnostics. Only fixed operational and safeguard categories are safe to send. if let Err(cause) = &account.ended && !account.interrupted { let cause = if account.security_findings.is_some() { - cause.safeguard_refusal() + account.pre_audit.or_else(|| cause.safeguard_refusal()) } else { Some(cause.full()) }; @@ -299,7 +313,14 @@ fn body( text += &format!("Command log: {}\n", log.display()); } if let Some(built_with) = built_with { - text += &format!("{built_with}\n"); + if account.pre_audit.is_some() { + text += &format!( + "Configured: {}\n", + built_with.strip_prefix("Built with ").unwrap_or(built_with) + ); + } else { + text += &format!("{built_with}\n"); + } } text += &format!("Host: {host}\n"); text += &format!("Took: {}\n", progress::duration(took)); @@ -357,6 +378,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: vec![PR], @@ -377,6 +399,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: Vec::new(), diff --git a/src/run_ending.rs b/src/run_ending.rs index bd3eb8cf..664a7554 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -54,13 +54,16 @@ pub fn read(ending: &Ending) -> Result, &Skip> { ticket_lines: &[], review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: Vec::new(), }, Err(failed) => Account::of_failure( failed, - if audited.failed_reproduction.is_some() { + if !audited.audit_started { + "pre-audit failed" + } else if audited.failed_reproduction.is_some() { "reproduction failed" } else { "audit failed" @@ -68,6 +71,18 @@ pub fn read(ending: &Ending) -> Result, &Skip> { ), }; Ok(Account { + pre_audit: if audited.audit_started { + None + } else { + Some( + audited + .outcome + .as_ref() + .err() + .and_then(|failed| crate::github::safe_cause(&failed.error)) + .unwrap_or("Security run pre-audit checks failed"), + ) + }, failed_reproduction: audited.failed_reproduction, security_findings: Some(&audited.findings), advice: &audited.advice, @@ -117,6 +132,8 @@ pub struct Account<'a> { pub review: Option>, /// The failed reproduction, after a completed Security audit and private recording. pub failed_reproduction: Option, + /// Fixed safe cause when the Security audit was never entered. + pub pre_audit: Option<&'static str>, /// Safe metadata from a Security run's private records, even if the audit failed. pub security_findings: Option<&'a [crate::security::RecordedFinding]>, /// The URLs on stdout, a line each: the pull request's, or that of the @@ -189,6 +206,7 @@ impl<'a> Account<'a> { ticket_lines: &reached.ticket_lines, review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: vec![&reached.pr_url], @@ -215,6 +233,7 @@ impl<'a> Account<'a> { ticket_lines: &failed.ticket_lines, review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: failed.recovery.as_deref(), urls: failed.pr_url.as_deref().into_iter().collect(), @@ -245,6 +264,7 @@ impl<'a> Account<'a> { ticket_lines: &[], review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: vec![reviewed.url()], @@ -536,6 +556,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: vec![PR], @@ -556,6 +577,7 @@ mod tests { ticket_lines: &[], review: None, security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: Vec::new(), @@ -738,6 +760,7 @@ mod tests { dispatched: None, }), security_findings: None, + pre_audit: None, failed_reproduction: None, recovery: None, urls: vec![PLAN], diff --git a/src/security.rs b/src/security.rs index 12dad396..74c4cd47 100644 --- a/src/security.rs +++ b/src/security.rs @@ -45,6 +45,8 @@ pub enum Outcome { /// The Security run's outcome and the private records it reached, including before a failure. pub struct Ended { + /// Whether the audit operation was entered, independently of its result. + pub audit_started: bool, pub outcome: Result, /// A failed reproduction's number; its audit and private recording completed. pub failed_reproduction: Option, @@ -55,6 +57,7 @@ pub struct Ended { impl From for Ended { fn from(error: anyhow::Error) -> Self { Self { + audit_started: false, outcome: Err(error.into()), failed_reproduction: None, findings: Vec::new(), @@ -290,10 +293,12 @@ fn audit_and_record( let mut findings = Vec::new(); let mut log = None; let mut reproduction = None; + let mut audit_started = false; let recorded = (|| -> Result { outside.check_harness()?; outside.check_node()?; outside.started(Work::SecurityRun(repo)); + audit_started = true; let (audited, session_log) = outside.audit(base); log = session_log; let audited = audited?; @@ -376,6 +381,7 @@ fn audit_and_record( None }; Ended { + audit_started, outcome: recorded.map_err(|error| FailedRun { log, ..error.into() diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 0591bb05..e3ebf023 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -26,6 +26,7 @@ //! "refuse_merges"?: {"times", "error"}, //! "after_merge"?: "