From a98cfc370765d1b06560d5272d1d97e76fcdc38f Mon Sep 17 00:00:00 2001 From: agentrelaybot Date: Sat, 19 Sep 2026 09:09:28 -0700 Subject: [PATCH 1/2] fix(sdk-rust): classify an undecodable registration response as InvalidResponse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent registration in AgentWorkforce/relay's PR proof gate failed like this, three times identically, blocking every runtime PR: register transport error: HTTP error: error decoding response body That is reqwest's message for a response that ARRIVED and could not be parsed. It is not a transport failure. The catch-all in register_agent_token collapsed every unclassified RelayError into Transport with `error.to_string()`, which: - misnamed a server fault as a network one - discarded the URL, the status, and the decode cause underneath the outermost message — leaving nothing to diagnose with - made registration_is_retryable() say yes, so it was retried as though a retry could help The last point is the dangerous one. An undecodable response to a registration may be a response to a registration that COMMITTED. Retrying it risks a second registration, not recovery from a blip. Add AgentRegistrationError::InvalidResponse, carrying status, URL, and the full source chain. Classify into it when the reqwest error is a decode failure, when the body was JSON that did not match the schema, or when the SDK already knew the response was invalid. It is not retryable. Genuine transport failures keep their classification and stay retryable: this narrows what counts as transport, it does not stop retrying real network faults. A test pins that. Downstream: relay's broker matches this enum with wildcard arms throughout and delegates retryability to registration_is_retryable(), so it inherits the change without modification. What this does not do: establish what the server actually sent. Codex reviewed the response paths on POST /v1/agents and found no intentional non-JSON path; the Cloudflare entrypoint rethrowing unclassified engine exceptions is a plausible but unconfirmed boundary. The next failure will now name the status, URL and cause, which is what settles it. Co-Authored-By: Claude Opus 5 (1M context) Session-Id: d458bd97-53d8-4f02-be9c-48b67b93c916 --- packages/sdk-rust/src/registration.rs | 138 +++++++++++++++++++++++++- 1 file changed, 134 insertions(+), 4 deletions(-) diff --git a/packages/sdk-rust/src/registration.rs b/packages/sdk-rust/src/registration.rs index 2ff1ff58..767a9fb4 100644 --- a/packages/sdk-rust/src/registration.rs +++ b/packages/sdk-rust/src/registration.rs @@ -122,6 +122,31 @@ pub enum AgentRegistrationError { }, #[error("registration transport error for '{agent_name}': {detail}")] Transport { agent_name: String, detail: String }, + /// The server answered, and the answer could not be understood. + /// + /// Distinct from `Transport`, which means the exchange did not complete. + /// Here it did: a body arrived and failed to decode, or decoded into + /// something this client's schema does not accept. Collapsing the two lost + /// the difference between "the network failed" and "the server is wrong", + /// and reported the second as the first: + /// + /// register transport error: HTTP error: error decoding response body + /// + /// which named no status, no URL, and no cause — and was retried as though + /// a retry could help. + #[error( + "registration for '{agent_name}' got a response it could not understand{}: {detail}", + status.map(|code| format!(" ({code})")).unwrap_or_default() + )] + InvalidResponse { + agent_name: String, + /// HTTP status, when the failure happened after one was read. + status: Option, + /// Endpoint that produced it, for correlating with server logs. + url: Option, + /// The decode failure and its source chain. + detail: String, + }, #[error("registration response missing token for '{agent_name}'")] MissingToken { agent_name: String }, /// The name is taken and registration is create-only. @@ -333,10 +358,7 @@ impl AgentRegistrationClient { status, detail: api_error_detail(message, code, request_id, attempts), }), - Err(error) => Err(AgentRegistrationError::Transport { - agent_name: trimmed_name.to_string(), - detail: error.to_string(), - }), + Err(error) => Err(classify_registration_failure(trimmed_name, error)), } } @@ -380,6 +402,56 @@ pub fn registration_is_retryable(error: &AgentRegistrationError) -> bool { ) } +/// Describe a `reqwest` failure with its source chain. +/// +/// `to_string()` on a reqwest error yields only the outermost layer — "error +/// decoding response body" — while the cause that names the offending byte or +/// field sits underneath it. Whoever reads the log needs the chain. +fn describe_with_sources(error: &(dyn std::error::Error + 'static)) -> String { + let mut description = error.to_string(); + let mut source = error.source(); + while let Some(cause) = source { + description.push_str(&format!(": {cause}")); + source = cause.source(); + } + description +} + +/// Separate "the exchange failed" from "the answer was unusable". +/// +/// A decode failure means the server replied and the reply could not be +/// understood. Retrying cannot fix that, and worse, the registration it +/// belonged to may already have committed server-side — so a retry risks a +/// second registration rather than recovering from a blip. +fn classify_registration_failure(agent_name: &str, error: RelayError) -> AgentRegistrationError { + match error { + RelayError::Http(http_error) if http_error.is_decode() => { + AgentRegistrationError::InvalidResponse { + agent_name: agent_name.to_string(), + status: http_error.status().map(|status| status.as_u16()), + url: http_error.url().map(|url| url.to_string()), + detail: describe_with_sources(&http_error), + } + } + RelayError::Json(json_error) => AgentRegistrationError::InvalidResponse { + agent_name: agent_name.to_string(), + status: None, + url: None, + detail: describe_with_sources(&json_error), + }, + RelayError::InvalidResponse(detail) => AgentRegistrationError::InvalidResponse { + agent_name: agent_name.to_string(), + status: None, + url: None, + detail, + }, + other => AgentRegistrationError::Transport { + agent_name: agent_name.to_string(), + detail: describe_with_sources(&other), + }, + } +} + /// Format a human-readable registration error message. pub fn format_registration_error(agent_name: &str, error: &AgentRegistrationError) -> String { let mut message = format!("failed to register agent '{}': {}", agent_name, error); @@ -839,3 +911,61 @@ mod tests { assert!(message.contains("retry after")); } } + +#[cfg(test)] +mod invalid_response_classification { + use super::*; + + /// A decode failure is the server answering unusably, not the exchange + /// failing. It was reported as `Transport` and retried three times, which + /// named no status, no URL and no cause — and could not have helped. + #[test] + fn a_decode_failure_is_not_transport() { + let error = classify_registration_failure("probe", RelayError::InvalidResponse("body was not JSON".into())); + assert!( + matches!(error, AgentRegistrationError::InvalidResponse { .. }), + "expected InvalidResponse, got {error:?}" + ); + } + + /// Retrying cannot fix an unusable answer, and registration may already + /// have committed server-side — so a retry risks a second registration + /// rather than recovering from a blip. + #[test] + fn an_unusable_answer_is_not_retried() { + let error = AgentRegistrationError::InvalidResponse { + agent_name: "probe".into(), + status: Some(200), + url: Some("https://cast.agentrelay.com/v1/agents".into()), + detail: "error decoding response body".into(), + }; + assert!(!registration_is_retryable(&error)); + } + + /// A genuine transport failure keeps its old classification and stays + /// retryable: this change narrows what counts as transport, it does not + /// stop retrying real network faults. + #[test] + fn a_real_transport_failure_is_still_retried() { + let error = AgentRegistrationError::Transport { + agent_name: "probe".into(), + detail: "connection reset".into(), + }; + assert!(registration_is_retryable(&error)); + } + + /// The message has to name what the old one omitted, or the next failure + /// is as undiagnosable as this one was. + #[test] + fn the_message_names_status_and_cause() { + let rendered = AgentRegistrationError::InvalidResponse { + agent_name: "probe".into(), + status: Some(503), + url: Some("https://cast.agentrelay.com/v1/agents".into()), + detail: "error decoding response body: expected value at line 1 column 1".into(), + } + .to_string(); + assert!(rendered.contains("503"), "status missing from {rendered}"); + assert!(rendered.contains("expected value"), "cause missing from {rendered}"); + } +} From 0fe415001ca377c37f065562ea22428330c96899 Mon Sep 17 00:00:00 2001 From: agentrelaybot Date: Sat, 19 Sep 2026 13:52:10 -0700 Subject: [PATCH 2/2] fix(sdk-rust): keep a body-read failure as Transport; render the URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the first revision's classification was inverted for the very error that motivated it. Bugbot: in reqwest 0.12, `is_decode()` is true for `bytes()` failures, not only parse errors. This client reads every body with `bytes()` and parses afterwards with serde_json, so a decode-kind reqwest error here can ONLY mean the body was never fully read — a reset or timeout after headers. That is the exchange failing. Routing it to InvalidResponse and refusing to retry would have made every mid-body reset permanent. It stays Transport and retryable, now carrying URL, status and the source chain in its detail so the log names what it was talking to. The genuine "server answered unusably" case is `RelayError::Json`: a success status whose body did not parse. That one is InvalidResponse and not retryable, as before. And a non-JSON body on an error status already surfaces as `Api { code: "invalid_response_body", status }` with its status intact, so it was never in this path. This also sharpens the original diagnosis. "HTTP error: error decoding response body" three times running means the body never fully arrived three times — the server or edge closing the connection after headers — rather than the server sending something unparseable. Devin and CodeRabbit: InvalidResponse stored the URL and did not render it, so format_registration_error could not say which endpoint answered. It is in the Display now, and the test asserts it. Tests use real reqwest errors from a refused connection rather than constructed ones, because reqwest::Error is not constructible directly and the point is the classification of what reqwest actually produces. Co-Authored-By: Claude Opus 5 (1M context) Session-Id: d458bd97-53d8-4f02-be9c-48b67b93c916 --- packages/sdk-rust/src/registration.rs | 101 +++++++++++++++++++++----- 1 file changed, 81 insertions(+), 20 deletions(-) diff --git a/packages/sdk-rust/src/registration.rs b/packages/sdk-rust/src/registration.rs index 767a9fb4..b9954cec 100644 --- a/packages/sdk-rust/src/registration.rs +++ b/packages/sdk-rust/src/registration.rs @@ -125,18 +125,17 @@ pub enum AgentRegistrationError { /// The server answered, and the answer could not be understood. /// /// Distinct from `Transport`, which means the exchange did not complete. - /// Here it did: a body arrived and failed to decode, or decoded into - /// something this client's schema does not accept. Collapsing the two lost - /// the difference between "the network failed" and "the server is wrong", - /// and reported the second as the first: + /// Here it did: a body arrived on a success status and was not valid JSON, + /// or the client already knew the response was malformed. Retrying cannot + /// help, and the registration it answered may already have committed. /// - /// register transport error: HTTP error: error decoding response body - /// - /// which named no status, no URL, and no cause — and was retried as though - /// a retry could help. + /// Note what is NOT this: a reqwest error whose kind is `decode`. In this + /// client that arises from `bytes()` failing to read the body — a reset or + /// timeout after headers — which is a transport fault and stays retryable. #[error( - "registration for '{agent_name}' got a response it could not understand{}: {detail}", - status.map(|code| format!(" ({code})")).unwrap_or_default() + "registration for '{agent_name}' got a response it could not understand{}{}: {detail}", + status.map(|code| format!(" ({code})")).unwrap_or_default(), + url.as_ref().map(|url| format!(" from {url}")).unwrap_or_default() )] InvalidResponse { agent_name: String, @@ -425,14 +424,31 @@ fn describe_with_sources(error: &(dyn std::error::Error + 'static)) -> String { /// second registration rather than recovering from a blip. fn classify_registration_failure(agent_name: &str, error: RelayError) -> AgentRegistrationError { match error { - RelayError::Http(http_error) if http_error.is_decode() => { - AgentRegistrationError::InvalidResponse { + // A reqwest error — decode kind included — is the exchange failing, not + // the answer being wrong. In this client every body is read with + // `bytes()`, which routes a read failure (connection reset mid-body, + // timeout after headers) through reqwest's `decode` kind, so + // `is_decode()` here means "the body never fully arrived". Parsing + // happens afterwards with serde_json and fails as `RelayError::Json`. + // Treating a decode-kind reqwest error as InvalidResponse would make + // every mid-body reset permanent. Keep it Transport, keep it retryable, + // but carry the URL, status, and cause chain so the log is diagnosable. + RelayError::Http(http_error) => { + let mut detail = describe_with_sources(&http_error); + if let Some(status) = http_error.status() { + detail = format!("{detail} (status {})", status.as_u16()); + } + if let Some(url) = http_error.url() { + detail = format!("{detail} [{url}]"); + } + AgentRegistrationError::Transport { agent_name: agent_name.to_string(), - status: http_error.status().map(|status| status.as_u16()), - url: http_error.url().map(|url| url.to_string()), - detail: describe_with_sources(&http_error), + detail, } } + // The body arrived on a success status and was not valid JSON. The + // server answered; the answer is unusable. Registration may already + // have committed, so this must not be retried. RelayError::Json(json_error) => AgentRegistrationError::InvalidResponse { agent_name: agent_name.to_string(), status: None, @@ -916,16 +932,60 @@ mod tests { mod invalid_response_classification { use super::*; - /// A decode failure is the server answering unusably, not the exchange - /// failing. It was reported as `Transport` and retried three times, which - /// named no status, no URL and no cause — and could not have helped. + /// A body that arrived on a success status and was not JSON is the server + /// answering unusably, not the exchange failing. That is `RelayError::Json` + /// in this client, and it must not be retried: the registration it + /// answered may already have committed. #[test] - fn a_decode_failure_is_not_transport() { - let error = classify_registration_failure("probe", RelayError::InvalidResponse("body was not JSON".into())); + fn an_unparseable_success_body_is_invalid_response() { + let json_error = serde_json::from_str::("").unwrap_err(); + let error = classify_registration_failure("probe", RelayError::Json(json_error)); assert!( matches!(error, AgentRegistrationError::InvalidResponse { .. }), "expected InvalidResponse, got {error:?}" ); + assert!(!registration_is_retryable(&error)); + } + + /// The client already knew the response was malformed. + #[test] + fn a_known_malformed_response_is_invalid_response() { + let error = classify_registration_failure("probe", RelayError::InvalidResponse("body was not JSON".into())); + assert!(matches!(error, AgentRegistrationError::InvalidResponse { .. })); + } + + /// A reqwest error stays Transport — the decode kind included. In this + /// client `is_decode()` arises from `bytes()` failing to read the body (a + /// reset or timeout after headers), which is the exchange failing, not the + /// answer being wrong. An earlier revision of this change routed it to + /// InvalidResponse and would have made every mid-body reset permanent. + #[test] + fn a_reqwest_error_stays_transport_and_retryable() { + // A reqwest error is not constructible directly; a failed request to an + // unroutable address yields a real one. + let rt = tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap(); + let http_error = rt + .block_on(reqwest::Client::new().get("http://127.0.0.1:1/").send()) + .expect_err("connecting to port 1 must fail"); + let error = classify_registration_failure("probe", RelayError::Http(http_error)); + assert!( + matches!(error, AgentRegistrationError::Transport { .. }), + "expected Transport, got {error:?}" + ); + assert!(registration_is_retryable(&error)); + } + + /// Transport detail now carries the URL, so a transport failure in a log + /// names the endpoint it was talking to. + #[test] + fn transport_detail_names_the_endpoint() { + let rt = tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap(); + let http_error = rt + .block_on(reqwest::Client::new().get("http://127.0.0.1:1/v1/agents").send()) + .expect_err("connecting to port 1 must fail"); + let error = classify_registration_failure("probe", RelayError::Http(http_error)); + let rendered = error.to_string(); + assert!(rendered.contains("127.0.0.1:1/v1/agents"), "url missing from {rendered}"); } /// Retrying cannot fix an unusable answer, and registration may already @@ -966,6 +1026,7 @@ mod invalid_response_classification { } .to_string(); assert!(rendered.contains("503"), "status missing from {rendered}"); + assert!(rendered.contains("cast.agentrelay.com/v1/agents"), "url missing from {rendered}"); assert!(rendered.contains("expected value"), "cause missing from {rendered}"); } }