diff --git a/src/harness/providers/openai/README.md b/src/harness/providers/openai/README.md index 297ec96e..53a944e8 100644 --- a/src/harness/providers/openai/README.md +++ b/src/harness/providers/openai/README.md @@ -224,9 +224,18 @@ into a structured `ProviderError` (HTTP status, provider error code, and a everything else — including 401/400 — is not) and surfaced as `TinyAgentsError::Provider`, so `harness::retry::is_retryable` can classify retryability instead of retrying every provider failure indiscriminately. -Transport-level failures (connection errors, body-read failures) have no such -structure to preserve and surface as a plain `TinyAgentsError::Model` string -via `provider_failure_message`. Malformed JSON bodies surface as +A **send** failure — no final HTTP status was available, so there is nothing to +report as one — still carries a provider, a model, and a computed `retryable`, +so `send_checked` raises it as `TinyAgentsError::Provider` with `status: None` +rather than flattening it; a host that classifies on the variant would +otherwise see a connection reset as an unstructured error. `Display` is +identical either way. Note that "no final status" is not the same as "never +reached a server": the client keeps reqwest's default redirect policy, so a +redirect loop or an exceeded redirect limit lands here too, after servers have +answered with 3xx. + +**Body-read** failures after a 2xx (`list_models`, `invoke_responses`) remain +plain `TinyAgentsError::Model` strings. Malformed JSON bodies surface as `TinyAgentsError::Serialization`. ## Operational constraints diff --git a/src/harness/providers/openai/test.rs b/src/harness/providers/openai/test.rs index a4f402b5..a2b21d4c 100644 --- a/src/harness/providers/openai/test.rs +++ b/src/harness/providers/openai/test.rs @@ -2915,3 +2915,63 @@ fn stream_cleanup_scrubs_leaked_markup_from_live_deltas() { assert_eq!(completed.tool_calls().len(), 1); assert_eq!(completed.tool_calls()[0].name, "lookup"); } + +// ── transport-level failures keep their structure ─────────────────────────── + +/// A transport failure must reach the caller as `TinyAgentsError::Provider`. +/// +/// It carries no HTTP status — nothing answered — but it does carry the +/// provider, the model, and a computed `retryable`, and a host that classifies +/// on the variant needs all three. Flattening it into +/// `TinyAgentsError::Model(String)` sends a connection reset down the generic +/// arm of a host's logger, where it is recorded with no status, no provider and +/// no retryability, and the operator reading it cannot tell a dead endpoint +/// from a rejected request. +#[tokio::test] +async fn a_transport_failure_is_reported_as_a_structured_provider_error() { + // Bind, read the port, then drop the listener: the port is now almost + // certainly closed, so the connect fails without waiting on a timeout. + let port = { + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind an ephemeral port"); + listener.local_addr().expect("read the bound port").port() + }; + + let model = OpenAiModel::compatible_provider( + "OpenHuman", + "test-key", + format!("http://127.0.0.1:{port}/openai/v1"), + "chat-v1", + ); + + let error = ChatModel::<()>::invoke(&model, &(), ModelRequest::new(vec![Message::user("hi")])) + .await + .expect_err("a closed port cannot answer"); + + let TinyAgentsError::Provider(provider_error) = &error else { + panic!("expected a structured Provider error, got: {error:?}"); + }; + assert_eq!(provider_error.provider, "OpenHuman"); + assert_eq!(provider_error.model.as_deref(), Some("chat-v1")); + assert!( + provider_error.status.is_none(), + "nothing answered, so there is no status to report" + ); + assert!( + provider_error.retryable, + "a transport failure is worth retrying; the flag is what says so" + ); + + // The rendered text is the no-regression half: `Display for ProviderError` + // reproduces what `provider_failure_message` used to build by hand, and both + // error variants render as `model error: {0}`, so nothing a user or a log + // reader sees changes. + let rendered = error.to_string(); + assert!( + rendered.starts_with("model error: OpenHuman returned: "), + "the user-facing wording must not change, got: {rendered}" + ); + assert!( + rendered.contains("/openai/v1/chat/completions"), + "the failing URL is what makes this diagnosable, got: {rendered}" + ); +} diff --git a/src/harness/providers/openai/transport.rs b/src/harness/providers/openai/transport.rs index 00ef30d3..2b2abfb0 100644 --- a/src/harness/providers/openai/transport.rs +++ b/src/harness/providers/openai/transport.rs @@ -1993,9 +1993,19 @@ impl OpenAiModel { url: &str, ) -> Result { let response = builder.send().await.map_err(|e| { - let error = - self.provider_error(format!("{what} to {url} failed: {e}"), None, None, None); - TinyAgentsError::Model(self.provider_failure_message(&error)) + // Structured, not flattened. A transport failure has no HTTP status, + // but it does have a provider, a model, and a computed `retryable` — + // and a host that matches on `TinyAgentsError::Provider` to log + // those fields would otherwise fall through to its generic arm and + // record a connection reset as "(non-provider error)" with nothing + // to act on. `Display` renders identically either way, so only the + // machine-readable half changes. + TinyAgentsError::from_provider_error(self.provider_error( + format!("{what} to {url} failed: {e}"), + None, + None, + None, + )) })?; let status = response.status(); @@ -2133,23 +2143,6 @@ impl OpenAiModel { } } - fn provider_failure_message(&self, error: &ProviderError) -> String { - format!( - "{} returned{}{}: {}", - error.provider, - error - .status - .map(|status| format!(" HTTP {status}")) - .unwrap_or_default(), - error - .code - .as_deref() - .map(|code| format!(" ({code})")) - .unwrap_or_default(), - error.message - ) - } - /// Decodes a non-2xx body into a structured [`ProviderError`]. /// /// Two classifications are applied on top of the raw decode: