From 10a6f922446273e2063adcc4e875d70396c479c7 Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 1 Nov 2026 12:33:03 +0100 Subject: [PATCH 1/2] review-classifier: a failed store login names its cause chain --- swarm-review-classifier/src/run.rs | 65 +++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 2 deletions(-) diff --git a/swarm-review-classifier/src/run.rs b/swarm-review-classifier/src/run.rs index afe358e9b..342bf6351 100644 --- a/swarm-review-classifier/src/run.rs +++ b/swarm-review-classifier/src/run.rs @@ -186,6 +186,28 @@ impl KeySource for Store { } } +/// `e` and each of its sources, joined by `: `. The store client's errors +/// carry only a generic message at the top; the URL and the failure kind sit +/// below it. Neither holds a token or key value. +fn chain(e: &dyn std::error::Error) -> String { + let mut out = e.to_string(); + let mut next = e.source(); + while let Some(cause) = next { + out.push_str(": "); + out.push_str(&cause.to_string()); + next = cause.source(); + } + out +} + +/// The run failure for a store login that failed with `e`. +fn login_failure(what: &str, e: &dyn std::error::Error) -> String { + format!( + "logging in to the secret store to read the {what}: {}", + chain(e) + ) +} + /// The key `field` takes from the object at `path`, trimmed; `None` when /// nothing or a blank is stored. async fn read_key( @@ -195,11 +217,11 @@ async fn read_key( ) -> Result, String> { let store = swarm_secret_client::SecretStore::from_env(crate::CERT_ROLE) .await - .map_err(|e| format!("logging in to the secret store to read the {what}: {e}"))?; + .map_err(|e| login_failure(what, &e))?; let stored: Option = store .read_optional(path) .await - .map_err(|e| format!("reading secret/{path}: {e}"))?; + .map_err(|e| format!("reading secret/{path}: {}", chain(&e)))?; Ok(stored .map(field) .map(|key| key.trim().to_owned()) @@ -380,6 +402,45 @@ mod tests { use super::*; + /// A failure with a cause, as the store client's errors nest them. + #[derive(Debug)] + struct Level(&'static str, Option>); + + impl std::fmt::Display for Level { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0) + } + } + + impl std::error::Error for Level { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.1.as_deref().map(|l| l as _) + } + } + + #[test] + fn chain_renders_every_level() { + let e = Level( + "An error occurred with the request", + Some(Box::new(Level( + "error sending request for url (https://bao/v1/auth/cert/login)", + Some(Box::new(Level("tls handshake eof", None))), + ))), + ); + assert_eq!( + chain(&e), + "An error occurred with the request: error sending request for url \ + (https://bao/v1/auth/cert/login): tls handshake eof" + ); + assert_eq!( + login_failure("API key", &e), + "logging in to the secret store to read the API key: An error occurred with the \ + request: error sending request for url (https://bao/v1/auth/cert/login): tls \ + handshake eof" + ); + assert_eq!(chain(&Level("alone", None)), "alone"); + } + /// The API key runtime's key is `k` and the ACP runtime's `p`, or both /// fail with the same error. struct Key(Result<(), &'static str>); From 8cdaf433a9063e73ce06ae454edf45eb11b1fabc Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 1 Nov 2026 13:34:24 +0100 Subject: [PATCH 2/2] review-classifier: keep the stored value out of a failed read's text --- swarm-review-classifier/src/run.rs | 35 ++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/swarm-review-classifier/src/run.rs b/swarm-review-classifier/src/run.rs index 342bf6351..1f60e91c1 100644 --- a/swarm-review-classifier/src/run.rs +++ b/swarm-review-classifier/src/run.rs @@ -188,7 +188,7 @@ impl KeySource for Store { /// `e` and each of its sources, joined by `: `. The store client's errors /// carry only a generic message at the top; the URL and the failure kind sit -/// below it. Neither holds a token or key value. +/// below it. For login errors only: they hold URLs and error kinds, not values. fn chain(e: &dyn std::error::Error) -> String { let mut out = e.to_string(); let mut next = e.source(); @@ -208,6 +208,13 @@ fn login_failure(what: &str, e: &dyn std::error::Error) -> String { ) } +/// The run failure for a store read that failed with `e`. Shows `e` alone: +/// a parse error's sources quote the stored value, and the failure text ends +/// up in a review comment. +fn read_failure(path: &str, e: &dyn std::error::Error) -> String { + format!("reading secret/{path}: {e}") +} + /// The key `field` takes from the object at `path`, trimmed; `None` when /// nothing or a blank is stored. async fn read_key( @@ -221,7 +228,7 @@ async fn read_key( let stored: Option = store .read_optional(path) .await - .map_err(|e| format!("reading secret/{path}: {}", chain(&e)))?; + .map_err(|e| read_failure(path, &e))?; Ok(stored .map(field) .map(|key| key.trim().to_owned()) @@ -418,6 +425,30 @@ mod tests { } } + /// A parse failure whose source quotes the stored value. + #[derive(Debug)] + struct Unparsable(serde_json::Error); + + impl std::fmt::Display for Unparsable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("error parsing the response") + } + } + + impl std::error::Error for Unparsable { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.0) + } + } + + #[test] + fn read_failure_leaves_out_the_stored_value() { + let cause = serde_json::from_str::("\"hunter2\"").unwrap_err(); + assert!(cause.to_string().contains("hunter2"), "control"); + let text = read_failure("p", &Unparsable(cause)); + assert_eq!(text, "reading secret/p: error parsing the response"); + } + #[test] fn chain_renders_every_level() { let e = Level(