From 4782f69110b187081ba4891c08b294d1f9bbd346 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 11 Nov 2026 19:01:47 +0100 Subject: [PATCH] a review-classifier key in the wrong shape or rejected by anthropic logs how to fix it --- Cargo.lock | 1 + swarm-review-classifier/Cargo.toml | 2 + swarm-review-classifier/src/run.rs | 81 ++++++++++++++++++++++++++++-- swarm-secret-client/src/lib.rs | 28 +++++++++++ 4 files changed, 108 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f4c4d4c3f..2a5d4f33d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5337,6 +5337,7 @@ dependencies = [ "tempfile", "tokio", "tracing", + "vaultrs", ] [[package]] diff --git a/swarm-review-classifier/Cargo.toml b/swarm-review-classifier/Cargo.toml index 5aa5e6dcc..72ecf2194 100644 --- a/swarm-review-classifier/Cargo.toml +++ b/swarm-review-classifier/Cargo.toml @@ -36,6 +36,8 @@ tracing.workspace = true tempfile = "3" # `InMemoryMetricExporter` for the cost counter test. opentelemetry_sdk = { workspace = true, features = ["testing"] } +# Builds the decode failure the store client reports for a wrong-shape secret. +vaultrs.workspace = true [lints] workspace = true diff --git a/swarm-review-classifier/src/run.rs b/swarm-review-classifier/src/run.rs index 1f60e91c1..10d8d5eea 100644 --- a/swarm-review-classifier/src/run.rs +++ b/swarm-review-classifier/src/run.rs @@ -164,7 +164,7 @@ impl KeySource for Store { match runtime { Runtime::ApiKey => { let path = api_key_path().map_err(|e| e.to_string())?; - let key = read_key(&path, "API key", |k: ApiKey| k.api_key).await?; + let key = read_key(&path, "API key", "api_key", |k: ApiKey| k.api_key).await?; key.ok_or_else(|| { format!( "no API key at secret/{path}: write it in the secret store's UI, field \ @@ -174,7 +174,10 @@ impl KeySource for Store { } Runtime::Acp(_) => { let path = acp_provider_key_path().map_err(|e| e.to_string())?; - let key = read_key(&path, "ACP provider key", |k: ProviderKey| k.api_key).await?; + let key = read_key(&path, "ACP provider key", "api_key", |k: ProviderKey| { + k.api_key + }) + .await?; key.ok_or_else(|| { format!( "no ACP provider key at secret/{path}: write it in the secret store's \ @@ -215,11 +218,27 @@ fn read_failure(path: &str, e: &dyn std::error::Error) -> String { format!("reading secret/{path}: {e}") } +/// The run failure for a store read that failed with `e`, where the object at +/// `path` should hold the string field `field`. An object that does not decode +/// gets a message naming the shape and the write that fixes it, without `e`: +/// its sources quote the stored value. +fn read_error(path: &str, field: &str, e: &swarm_secret_client::Error) -> String { + if e.is_undecodable() { + format!( + "secret/{path} does not hold a string field `{field}`: write it with `bao kv put \ + secret/{path} {field}=-`" + ) + } else { + read_failure(path, e) + } +} + /// The key `field` takes from the object at `path`, trimmed; `None` when /// nothing or a blank is stored. async fn read_key( path: &str, what: &str, + field_name: &str, field: impl FnOnce(T) -> String, ) -> Result, String> { let store = swarm_secret_client::SecretStore::from_env(crate::CERT_ROLE) @@ -228,7 +247,7 @@ async fn read_key( let stored: Option = store .read_optional(path) .await - .map_err(|e| read_failure(path, &e))?; + .map_err(|e| read_error(path, field_name, &e))?; Ok(stored .map(field) .map(|key| key.trim().to_owned()) @@ -260,7 +279,19 @@ impl Model for Claude { if let Err(e) = tokio::fs::remove_dir_all(&dir).await { tracing::warn!(dir = %dir.display(), error = %e, "removing a run's directory failed"); } - outcome + outcome.map_err(key_rejection) + } +} + +/// `error` as the run failure for a model call: when the Anthropic API +/// refused the key (HTTP 401), it says where to rotate it, followed by the +/// original text. That text names the status and no key. +fn key_rejection(error: String) -> String { + match api_key_path() { + Ok(path) if error.contains("(HTTP 401)") => format!( + "the Anthropic API key was rejected (HTTP 401): rotate it at secret/{path} ({error})" + ), + _ => error, } } @@ -449,6 +480,48 @@ mod tests { assert_eq!(text, "reading secret/p: error parsing the response"); } + #[test] + fn an_undecodable_secret_names_the_shape_and_leaves_out_the_value() { + let cause = serde_json::from_str::("\"hunter2\"").unwrap_err(); + assert!( + cause.to_string().contains("hunter2"), + "control: the source quotes the value" + ); + let undecodable = + swarm_secret_client::Error::from(vaultrs::error::ClientError::JsonParseError { + source: cause, + }); + let text = read_error("swarm/p", "api_key", &undecodable); + assert_eq!( + text, + "secret/swarm/p does not hold a string field `api_key`: write it with `bao kv put \ + secret/swarm/p api_key=-`" + ); + // The control: any other store failure keeps the generic text. + let other = swarm_secret_client::Error::MissingEnv("BAO_ADDR"); + assert_eq!( + read_error("swarm/p", "api_key", &other), + "reading secret/swarm/p: BAO_ADDR is unset or empty" + ); + } + + #[test] + fn a_rejected_key_says_where_to_rotate_it() { + let path = api_key_path().unwrap(); + assert_eq!( + key_rejection("claude failed: authentication failed (HTTP 401)".to_owned()), + format!( + "the Anthropic API key was rejected (HTTP 401): rotate it at secret/{path} \ + (claude failed: authentication failed (HTTP 401))" + ) + ); + // The control: other failures pass through. + assert_eq!( + key_rejection("claude failed: exit 1".to_owned()), + "claude failed: exit 1" + ); + } + #[test] fn chain_renders_every_level() { let e = Level( diff --git a/swarm-secret-client/src/lib.rs b/swarm-secret-client/src/lib.rs index f499a8ed2..7e059ee81 100644 --- a/swarm-secret-client/src/lib.rs +++ b/swarm-secret-client/src/lib.rs @@ -140,6 +140,18 @@ impl Error { Self::Vault(e) if matches!(**e, vaultrs::error::ClientError::APIError { code: 403, .. }) ) } + + /// Whether the store answered, but what it holds did not decode as the + /// type the caller asked for: a field is missing or has another type. + /// The error's own text quotes the stored value, so a caller that reports + /// this case writes its own message rather than showing the error. + #[must_use] + pub fn is_undecodable(&self) -> bool { + matches!( + self, + Self::Vault(e) if matches!(**e, vaultrs::error::ClientError::JsonParseError { .. }) + ) + } } /// Install `aws-lc-rs` as the process-wide rustls crypto provider. Call it @@ -175,6 +187,22 @@ mod tests { assert!(!Error::MissingEnv("BAO_ADDR").is_denied()); } + #[test] + fn only_a_decode_failure_is_undecodable() { + let decode = + serde_json::from_value::(serde_json::json!({ "value": "k" })).unwrap_err(); + let undecodable = + Error::from(vaultrs::error::ClientError::JsonParseError { source: decode }); + assert!(undecodable.is_undecodable()); + // The controls: a denial and a local failure are not decode failures. + let denied = Error::from(vaultrs::error::ClientError::APIError { + code: 403, + errors: Vec::new(), + }); + assert!(!denied.is_undecodable()); + assert!(!Error::MissingEnv("BAO_ADDR").is_undecodable()); + } + #[test] fn a_tls_client_config_builds_once_the_provider_is_installed() { install_crypto_provider();