diff --git a/src/github.rs b/src/github.rs index 5393200f..191c1d34 100644 --- a/src/github.rs +++ b/src/github.rs @@ -25,6 +25,59 @@ pub use advisories::{ #[cfg(test)] mod execution_tests; +/// A failed gh command keeps its diagnostic locally; notifications use only +/// its fixed operational category. +#[derive(Debug)] +enum CommandFailure { + Rejected { command: String, stderr: String }, + InvalidJson { command: String }, + InvalidResponse(String), + Execution, +} + +impl CommandFailure { + fn rejected(command: String, output: &Output) -> Self { + Self::Rejected { + command, + stderr: String::from_utf8_lossy(&output.stderr).trim().to_string(), + } + } +} + +impl fmt::Display for CommandFailure { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Rejected { command, stderr } => write!(f, "{command} failed: {stderr}"), + Self::InvalidJson { command } => write!(f, "{command} returned invalid JSON"), + Self::InvalidResponse(message) => f.write_str(message), + Self::Execution => f.write_str("could not run gh"), + } + } +} + +impl std::error::Error for CommandFailure {} + +/// A fixed cause for failed GitHub queries, never their original diagnostics. +/// Only explicit quota messages establish exhaustion; a bare 403 does not. +pub(crate) fn operational_cause(error: &anyhow::Error) -> Option<&'static str> { + let stderr = match error.downcast_ref::() { + Some(CommandFailure::Rejected { stderr, .. }) => stderr, + Some(_) => return Some("GitHub dependency failure"), + None => &error.downcast_ref::()?.stderr, + }; + let stderr = stderr.to_ascii_lowercase(); + Some( + if stderr.contains("api rate limit exceeded") + || stderr.contains("api rate limit already exceeded") + || stderr.contains("secondary rate limit") + { + "GitHub rate-limit exhaustion" + } else { + "GitHub dependency failure" + }, + ) +} + /// GitHub CLI operations bound to an immutable interruption policy. /// Ordinary views stop for Command interruption; completion views finish /// independently without changing the recorded request or other views. @@ -68,7 +121,9 @@ impl GitHub { let json = self.issue_view(issue, "title")?; Ok(json["title"] .as_str() - .context("gh output has no title")? + .context(CommandFailure::InvalidResponse( + "gh output has no title".into(), + ))? .to_string()) } @@ -77,13 +132,19 @@ impl GitHub { let json = self.issue_view(issue, "title,state,labels,createdAt")?; let created = json["createdAt"] .as_str() - .context("gh output has no createdAt")?; + .context(CommandFailure::InvalidResponse( + "gh output has no createdAt".into(), + ))?; Ok(Issue { title: json["title"].as_str().map(String::from), is_open: state_is_open(&json)?, labels: labels_of(&json)?, created: DateTime::parse_from_rfc3339(created) - .with_context(|| format!("gh output has an unreadable createdAt {created}"))? + .with_context(|| { + CommandFailure::InvalidResponse(format!( + "gh output has an unreadable createdAt {created}" + )) + })? .to_utc(), }) } @@ -191,8 +252,9 @@ impl GitHub { "--jq", ".id", ])?; - json.as_u64() - .context("gh api did not return the issue's database id") + json.as_u64().context(CommandFailure::InvalidResponse( + "gh api did not return the issue's database id".into(), + )) } /// Link `child` as a native sub-issue of `parent`. @@ -262,16 +324,24 @@ impl GitHub { "1000", ])?; json.as_array() - .context("gh issue list did not return a list")? + .context(CommandFailure::InvalidResponse( + "gh issue list did not return a list".into(), + ))? .iter() .map(|listed| { let field = |name: &str| { - listed[name] - .as_str() - .with_context(|| format!("gh issue list output has no {name}")) + listed[name].as_str().with_context(|| { + CommandFailure::InvalidResponse(format!( + "gh issue list output has no {name}" + )) + }) }; Ok(ListedIssue { - issue: IssueUrl::parse(field("url")?)?, + issue: IssueUrl::parse(field("url")?).context( + CommandFailure::InvalidResponse( + "gh issue list output has an invalid issue URL".into(), + ), + )?, title: field("title")?.to_string(), labels: labels_of(listed)?, }) @@ -299,23 +369,32 @@ impl GitHub { "--limit", "1000", ])?; - for listed in json - .as_array() - .context("gh issue list did not return a list")? - { + for listed in json.as_array().context(CommandFailure::InvalidResponse( + "gh issue list did not return a list".into(), + ))? { let url = listed["url"] .as_str() - .context("gh issue list output has no url")?; + .context(CommandFailure::InvalidResponse( + "gh issue list output has no url".into(), + ))?; let at = listed["createdAt"] .as_str() - .context("gh issue list output has no createdAt")?; + .context(CommandFailure::InvalidResponse( + "gh issue list output has no createdAt".into(), + ))?; let at = DateTime::parse_from_rfc3339(at) .with_context(|| { - format!("gh issue list output has an unreadable createdAt {at}") + CommandFailure::InvalidResponse(format!( + "gh issue list output has an unreadable createdAt {at}" + )) })? .to_utc(); if at.timestamp() >= since.timestamp() { - found.push(IssueUrl::parse(url)?); + found.push( + IssueUrl::parse(url).context(CommandFailure::InvalidResponse( + "gh issue list output has an invalid issue URL".into(), + ))?, + ); } } } @@ -335,15 +414,21 @@ impl GitHub { let info = &page["pageInfo"]; if !info["hasNextPage"] .as_bool() - .context("label history has no hasNextPage")? + .context(CommandFailure::InvalidResponse( + "label history has no hasNextPage".into(), + ))? { break json; } let next = info["endCursor"] .as_str() - .context("label history has no endCursor")?; + .context(CommandFailure::InvalidResponse( + "label history has no endCursor".into(), + ))?; if Some(next) == after.as_deref() { - bail!("label history cursor did not advance"); + bail!(CommandFailure::InvalidResponse( + "label history cursor did not advance".into() + )); } after = Some(next.to_string()); }; @@ -351,7 +436,10 @@ impl GitHub { let parent = issue["parent"]["url"] .as_str() .map(IssueUrl::parse) - .transpose()?; + .transpose() + .context(CommandFailure::InvalidResponse( + "gh api graphql output has an invalid parent issue URL".into(), + ))?; let mut open_blockers = Vec::new(); for blocker in nodes(&issue["blockedBy"], "blockedBy")? { if node_is_open(blocker)? { @@ -361,9 +449,15 @@ impl GitHub { let at = |event: &Value| -> Result> { let at = event["createdAt"] .as_str() - .context("gh api graphql output has an event with no createdAt")?; + .context(CommandFailure::InvalidResponse( + "gh api graphql output has an event with no createdAt".into(), + ))?; Ok(DateTime::parse_from_rfc3339(at) - .with_context(|| format!("gh api graphql output has an unreadable createdAt {at}"))? + .with_context(|| { + CommandFailure::InvalidResponse(format!( + "gh api graphql output has an unreadable createdAt {at}" + )) + })? .to_utc()) }; let mut shaped = Vec::new(); @@ -373,7 +467,9 @@ impl GitHub { let applied = match event["__typename"].as_str() { Some("LabeledEvent") => true, Some("UnlabeledEvent") => false, - other => bail!("gh api graphql output has an unknown label event {other:?}"), + other => bail!(CommandFailure::InvalidResponse(format!( + "gh api graphql output has an unknown label event {other:?}" + ))), }; let name = event["label"]["name"].as_str(); if applied && name.is_some_and(|name| crate::claim::IN_PROGRESS.is_named(name)) { @@ -391,7 +487,9 @@ impl GitHub { let by = match event["__typename"].as_str() { Some("SubIssueAddedEvent" | "SubIssueRemovedEvent") => Shaping::SubIssues, Some("BlockedByAddedEvent" | "BlockedByRemovedEvent") => Shaping::Blockers, - other => bail!("gh api graphql output has an unknown event {other:?}"), + other => bail!(CommandFailure::InvalidResponse(format!( + "gh api graphql output has an unknown event {other:?}" + ))), }; shaped.push(Shaped { by, at: at(event)? }); } @@ -415,11 +513,9 @@ impl GitHub { let known = self.gh_json(&[ "label", "list", "--repo", repo, "--json", "name", "--limit", "1000", ])?; - let known = named( - known - .as_array() - .context("gh label list did not return a list")?, - ); + let known = named(known.as_array().context(CommandFailure::InvalidResponse( + "gh label list did not return a list".into(), + ))?); for label in labels { if !known.has(*label) { self.gh(&[ @@ -474,7 +570,9 @@ impl GitHub { "--label", &names.join(","), ])?; - IssueUrl::parse(&url).context("gh issue create did not print the issue's URL") + IssueUrl::parse(&url).context(CommandFailure::InvalidResponse( + "gh issue create did not print the issue's URL".into(), + )) } /// The public email of the signed-in user's GitHub profile, or `None` if it @@ -525,18 +623,22 @@ impl GitHub { Some("MERGEABLE") => Mergeable::Yes, Some("CONFLICTING") => Mergeable::No, Some("UNKNOWN") => Mergeable::Unknown, - other => bail!("gh output has an unknown mergeable state {other:?}"), + other => bail!(CommandFailure::InvalidResponse(format!( + "gh output has an unknown mergeable state {other:?}" + ))), }; Ok(Some(PrSnapshot { pr: PullRequest::from_json(&json)?, mergeable, head_commit: json["headRefOid"] .as_str() - .context("gh output has no headRefOid")? + .context(CommandFailure::InvalidResponse( + "gh output has no headRefOid".into(), + ))? .to_string(), - from_fork: json["isCrossRepository"] - .as_bool() - .context("gh output has no isCrossRepository")?, + from_fork: json["isCrossRepository"].as_bool().context( + CommandFailure::InvalidResponse("gh output has no isCrossRepository".into()), + )?, })) } @@ -563,7 +665,9 @@ impl GitHub { &format!("{PR_FIELDS},isCrossRepository"), ])?; json.as_array() - .context("gh pr list did not return a list")? + .context(CommandFailure::InvalidResponse( + "gh pr list did not return a list".into(), + ))? .iter() .filter(|pr| pr["isCrossRepository"] != Value::Bool(true)) .map(PullRequest::from_json) @@ -601,7 +705,9 @@ impl GitHub { let json = self.pr_view(issue, &number.to_string(), "body")?; Ok(json["body"] .as_str() - .context("gh output has no body")? + .context(CommandFailure::InvalidResponse( + "gh output has no body".into(), + ))? .to_string()) } @@ -692,7 +798,9 @@ impl GitHub { checks.push(Check { name: run["name"] .as_str() - .context("a check run has no name")? + .context(CommandFailure::InvalidResponse( + "a check run has no name".into(), + ))? .to_string(), state, job: ActionsJob::of(run["id"].as_u64(), run["details_url"].as_str()), @@ -708,7 +816,9 @@ impl GitHub { checks.push(Check { name: status["context"] .as_str() - .context("a commit status has no context")? + .context(CommandFailure::InvalidResponse( + "a commit status has no context".into(), + ))? .to_string(), state, url: url(&status["target_url"]), @@ -741,11 +851,10 @@ impl GitHub { fn gh_stdout(&self, args: &[&str]) -> Result { let output = self.output(args)?; if !output.status.success() { - bail!( - "gh {} failed: {}", - args.join(" "), - String::from_utf8_lossy(&output.stderr).trim() - ); + bail!(CommandFailure::rejected( + format!("gh {}", args.join(" ")), + &output + )); } Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) } @@ -770,7 +879,9 @@ impl GitHub { serde_json::Deserializer::from_slice(&output.stdout) .into_iter() .collect::>() - .with_context(|| format!("gh api {path} returned invalid JSON")) + .with_context(|| CommandFailure::InvalidJson { + command: format!("gh api {path}"), + }) } /// Read-only JSON commands get three attempts, separated by one then two @@ -811,8 +922,9 @@ impl GitHub { private_status: None, }); } - serde_json::from_slice(&output.stdout) - .with_context(|| format!("{command} returned invalid JSON")) + serde_json::from_slice(&output.stdout).with_context(|| CommandFailure::InvalidJson { + command: command.to_string(), + }) } fn output(&self, args: &[&str]) -> Result { self.output_with_input(args, None) @@ -828,12 +940,23 @@ impl GitHub { stop: &|child| process::stop(child, &[libc::SIGTERM]), }, ) + .map_err(|error| { + self.interruption + .check() + .err() + .unwrap_or_else(|| error.context(CommandFailure::Execution)) + }) } } /// Whether the issue `json` describes, with its `state`, is open. fn state_is_open(json: &Value) -> Result { - Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") + Ok(json["state"] + .as_str() + .context(CommandFailure::InvalidResponse( + "gh output has no state".into(), + ))? + == "OPEN") } /// An issue as an Architect run reads the issue its Architecture review @@ -848,11 +971,9 @@ pub struct Issue { /// The labels of the issue `json` describes, with its `labels`. fn labels_of(json: &Value) -> Result { - Ok(named( - json["labels"] - .as_array() - .context("gh output has no labels")?, - )) + Ok(named(json["labels"].as_array().context( + CommandFailure::InvalidResponse("gh output has no labels".into()), + )?)) } /// The labels `labels` describe, each with its `name`. @@ -922,16 +1043,18 @@ query($owner: String!, $repo: String!, $number: Int!) { /// The `nodes` of `connection`, the `what` of a GraphQL answer. fn nodes<'a>(connection: &'a Value, what: &str) -> Result<&'a Vec> { - connection["nodes"] - .as_array() - .with_context(|| format!("gh api graphql output has no {what}")) + connection["nodes"].as_array().with_context(|| { + CommandFailure::InvalidResponse(format!("gh api graphql output has no {what}")) + }) } /// Whether the issue a GraphQL answer's `node` describes is open. fn node_is_open(node: &Value) -> Result { Ok(node["state"] .as_str() - .context("gh api graphql output has an issue with no state")? + .context(CommandFailure::InvalidResponse( + "gh api graphql output has an issue with no state".into(), + ))? == "OPEN") } @@ -940,22 +1063,31 @@ fn node_issue(node: &Value) -> Result { IssueUrl::parse( node["url"] .as_str() - .context("gh api graphql output has an issue with no URL")?, + .context(CommandFailure::InvalidResponse( + "gh api graphql output has an issue with no URL".into(), + ))?, ) + .context(CommandFailure::InvalidResponse( + "gh api graphql output has an invalid issue URL".into(), + )) } /// The number of the issue a GraphQL answer's `node` describes. fn node_number(node: &Value) -> Result { node["number"] .as_u64() - .context("gh api graphql output has an issue with no number") + .context(CommandFailure::InvalidResponse( + "gh api graphql output has an issue with no number".into(), + )) } /// How many sub-issues the issue a GraphQL answer's `node` describes has. fn sub_issue_count(node: &Value) -> Result { node["subIssues"]["totalCount"] .as_u64() - .context("gh api graphql output has an issue with no subIssues count") + .context(CommandFailure::InvalidResponse( + "gh api graphql output has an issue with no subIssues count".into(), + )) } /// An issue as a listing of its repository's issues gives it. @@ -1135,26 +1267,33 @@ impl PullRequest { fn from_json(json: &Value) -> Result { let field = |name: &str| -> Result { - json[name] - .as_str() - .map(String::from) - .with_context(|| format!("gh output has no {name}")) + json[name].as_str().map(String::from).with_context(|| { + CommandFailure::InvalidResponse(format!("gh output has no {name}")) + }) }; let state = match field("state")?.as_str() { "OPEN" => PrState::Open, "CLOSED" => PrState::Closed, "MERGED" => PrState::Merged, - other => bail!("gh output has an unknown PR state {other}"), + other => bail!(CommandFailure::InvalidResponse(format!( + "gh output has an unknown PR state {other}" + ))), }; Ok(PullRequest { - number: json["number"].as_u64().context("gh output has no number")?, + number: json["number"] + .as_u64() + .context(CommandFailure::InvalidResponse( + "gh output has no number".into(), + ))?, url: field("url")?, state, head: field("headRefName")?, base: field("baseRefName")?, is_draft: json["isDraft"] .as_bool() - .context("gh output has no isDraft")?, + .context(CommandFailure::InvalidResponse( + "gh output has no isDraft".into(), + ))?, }) } } diff --git a/src/notification.rs b/src/notification.rs index ff15f1e2..5d776116 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, + }) } } @@ -82,7 +88,7 @@ pub struct RunNotification { checked: Checked, subject: Subject, /// The command's configured Harness, Model and Effort, once known. - built_with: Option, + configured: Option, } impl RunNotification { @@ -111,7 +117,7 @@ impl RunNotification { Ok(RunNotification { checked, subject, - built_with: None, + configured: None, }) } @@ -129,7 +135,7 @@ impl RunNotification { /// Name the configured Harness, Model and Effort without claiming /// that the Harness confirmed which Model executed. pub fn built_with(&mut self, harness: &Choice) { - self.built_with = Some(format!("{} (configured)", harness.built_with())); + self.configured = Some(format!("{harness} (configured)")); } /// Send the one notification for the command that ended as `account` @@ -146,11 +152,18 @@ impl RunNotification { let mut text = body( &self.subject, account, - self.built_with.as_deref(), + self.configured.as_deref(), command_log.as_deref(), host.as_deref().unwrap_or("unknown host"), self.checked.started.elapsed(), ); + if account.pre_audit_failure.is_some() { + text += &format!( + "Started (UTC): {}\nDiagnostics: Check this command's stderr on {} at that time (or the scheduler's captured stderr, if scheduled).\n", + self.checked.started_at.format("%Y-%m-%d %H:%M:%S UTC"), + host.as_deref().unwrap_or("unknown host"), + ); + } let lines = crate::session::recording::Recording::capture().notification_lines(); if !lines.is_empty() { text += "\nSession models:\n"; @@ -205,7 +218,7 @@ fn architect_subject(repo: Option<&Repo>, outcome: &str) -> String { } /// The plain-text body of the notification for a command that ended as -/// `account` tells, about `subject`, with its configured `built_with` line, if known, +/// `account` tells, about `subject`, with its `configured` Harness, Model and Effort, if known, /// naming `command_log`, if the command keeps one, /// and `host`, for a command that `took` so long. It starts with the command's /// outcome, as the subject gives it, then the original Issue URL when the @@ -215,7 +228,7 @@ fn architect_subject(repo: Option<&Repo>, outcome: &str) -> String { fn body( subject: &Subject, account: &Account, - built_with: Option<&str>, + configured: Option<&str>, command_log: Option<&Path>, host: &str, took: Duration, @@ -226,6 +239,7 @@ fn body( } if let Some(findings) = account.security_findings { let audit = match &account.ended { + Err(_) if account.pre_audit_failure.is_some() => "not started", Err(_) if account.failed_reproduction.is_some() => "audit complete", Ok(line) => line.as_str(), Err(_) => account.outcome, @@ -273,12 +287,14 @@ 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_failure + .or_else(|| cause.safeguard_refusal()) } else { Some(cause.full()) }; @@ -298,8 +314,12 @@ fn body( if let Some(log) = command_log { text += &format!("Command log: {}\n", log.display()); } - if let Some(built_with) = built_with { - text += &format!("{built_with}\n"); + if let Some(configured) = configured { + if account.pre_audit_failure.is_some() { + text += &format!("Configured: {configured}\n"); + } else { + text += &format!("Built with {configured}\n"); + } } text += &format!("Host: {host}\n"); text += &format!("Took: {}\n", progress::duration(took)); @@ -358,6 +378,7 @@ mod tests { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: None, urls: vec![PR], } @@ -378,6 +399,7 @@ mod tests { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: None, urls: Vec::new(), } @@ -582,7 +604,7 @@ mod tests { body( &Subject::Pending, &account, - Some("Built with claude · opus · high"), + Some("claude · opus · high"), Some(Path::new("/home/me/.thirdshift/logs/commands/issue/x.log")), "droplet-1", Duration::from_secs(4) diff --git a/src/pass.rs b/src/pass.rs index 331dafe9..141e4cd9 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -104,7 +104,7 @@ pub trait Outside { /// Check Node.js before a Security audit needs its validators. fn check_node(&mut self) -> Result<()>; /// Audit the Base branch in a disposable checkout and validate its artifacts. - fn audit(&mut self, base: &str) -> (Result, Option); + fn audit(&mut self, base: &str) -> crate::security::audit::Attempt; /// Private advisory or issue records in every state. fn security_records(&mut self) -> Result; /// Whether origin's prepared Base branch is the last completed audit's commit. @@ -234,7 +234,7 @@ impl Outside for LaunchAndGitHub<'_> { crate::security::audit::check_node() } - fn audit(&mut self, base: &str) -> (Result, Option) { + fn audit(&mut self, base: &str) -> crate::security::audit::Attempt { logs::stage(logs::Stage::SecurityAudit); crate::security::audit::run(self.launch.git(), self.repo, base, self.harness) } @@ -766,10 +766,7 @@ mod in_memory { Ok(()) } - fn audit( - &mut self, - base: &str, - ) -> (Result, Option) { + fn audit(&mut self, base: &str) -> crate::security::audit::Attempt { self.calls.push(Call::SecurityAudit(base.to_string())); let audited = match self.audit_error.take() { Some(cause) => Err(anyhow!(cause)), @@ -777,7 +774,11 @@ mod in_memory { findings: std::mem::take(&mut self.audit_findings), }), }; - (audited, self.session_log.clone()) + crate::security::audit::Attempt { + started: true, + outcome: audited, + log: self.session_log.clone(), + } } fn security_records(&mut self) -> Result { diff --git a/src/run_ending.rs b/src/run_ending.rs index 12efcf89..147072a3 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -55,12 +55,15 @@ pub fn read(ending: &Ending) -> Result, &Skip> { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: 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,14 @@ pub fn read(ending: &Ending) -> Result, &Skip> { ), }; Ok(Account { + pre_audit_failure: if !audited.audit_started { + audited.outcome.as_ref().err().map(|failed| { + crate::github::operational_cause(&failed.error) + .unwrap_or("pre-audit checks failed") + }) + } else { + None + }, failed_reproduction: audited.failed_reproduction, security_findings: Some(&audited.findings), advice: &audited.advice, @@ -117,6 +128,8 @@ pub struct Account<'a> { pub review: Option>, /// The failed reproduction, after a completed Security audit and private recording. pub failed_reproduction: Option, + /// A fixed operational cause when a Security audit never started. + pub pre_audit_failure: 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 @@ -190,6 +203,7 @@ impl<'a> Account<'a> { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: None, urls: vec![&reached.pr_url], }, @@ -216,6 +230,7 @@ impl<'a> Account<'a> { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: failed.recovery.as_deref(), urls: failed.pr_url.as_deref().into_iter().collect(), } @@ -246,6 +261,7 @@ impl<'a> Account<'a> { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: None, urls: vec![reviewed.url()], }, @@ -537,6 +553,7 @@ mod tests { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: None, urls: vec![PR], } @@ -557,6 +574,7 @@ mod tests { review: None, security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: None, urls: Vec::new(), } @@ -739,6 +757,7 @@ mod tests { }), security_findings: None, failed_reproduction: None, + pre_audit_failure: None, recovery: None, urls: vec![PLAN], } diff --git a/src/security.rs b/src/security.rs index 12dad396..2e95496e 100644 --- a/src/security.rs +++ b/src/security.rs @@ -46,6 +46,8 @@ pub enum Outcome { /// The Security run's outcome and the private records it reached, including before a failure. pub struct Ended { pub outcome: Result, + /// Whether an audit session was requested after preparation succeeded. + pub audit_started: bool, /// A failed reproduction's number; its audit and private recording completed. pub failed_reproduction: Option, pub findings: Vec, @@ -56,6 +58,7 @@ impl From for Ended { fn from(error: anyhow::Error) -> Self { Self { outcome: Err(error.into()), + audit_started: false, failed_reproduction: None, findings: Vec::new(), advice: Vec::new(), @@ -290,13 +293,15 @@ 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)); - let (audited, session_log) = outside.audit(base); - log = session_log; - let audited = audited?; + let attempt = outside.audit(base); + audit_started = attempt.started; + log = attempt.log; + let audited = attempt.outcome?; let mut recorded = Recorded { created: 0, existing: 0, @@ -380,6 +385,7 @@ fn audit_and_record( log, ..error.into() }), + audit_started, failed_reproduction, findings, advice, diff --git a/src/security/audit.rs b/src/security/audit.rs index 5b027ca0..4cce6eb3 100644 --- a/src/security/audit.rs +++ b/src/security/audit.rs @@ -20,6 +20,14 @@ use crate::session::{Logs, Purpose, Sessions}; use crate::skills; use crate::worktree::ReviewWorktree; +/// Audit startup and its result stay together: preparation can fail before +/// any audit session is requested. +pub struct Attempt { + pub started: bool, + pub outcome: Result, + pub log: Option, +} + pub struct Audited { pub findings: Vec, } @@ -67,12 +75,7 @@ pub fn check_node() -> Result<()> { Ok(()) } -pub fn run( - launch: &Git, - repo: &Repo, - base: &str, - harness: &Choice, -) -> (Result, Option) { +pub fn run(launch: &Git, repo: &Repo, base: &str, harness: &Choice) -> Attempt { let prepared = (|| -> Result<_> { let worktree = ReviewWorktree::create(launch, &repo.name, base)?; let commit = Git::new(worktree.path()).run(&["rev-parse", "HEAD"])?; @@ -104,11 +107,19 @@ pub fn run( })(); let (worktree, output, prompt, commit) = match prepared { Ok(prepared) => prepared, - Err(error) => return (Err(error), None), + Err(error) => { + return Attempt { + started: false, + outcome: Err(error), + log: None, + }; + } }; let logs = Logs::of_security_run(repo); + let mut started = false; let (audited, log) = Sessions::within(&logs, worktree.path(), harness, |sessions| { progress::step(format!("starting the Security audit of {base}")); + started = true; let message = sessions.run_to_final_message(Purpose::Security, "security-audit", &prompt)?; let line = message.as_deref().and_then(|message| { @@ -185,7 +196,11 @@ pub fn run( (Ok(audited), Ok(())) => Ok(audited), (Err(error), _) | (_, Err(error)) => Err(error), }; - (audited, log) + Attempt { + started, + outcome: audited, + log, + } } fn finish_record(output: &Path, commit: &str, base: &str, complete: bool) -> Result<()> { diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 6dd9fb75..62592d7e 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -26,6 +26,7 @@ //! "refuse_merges"?: {"times", "error"}, //! "after_merge"?: "