From 0130f8735170f9b40b667302ca669c2e43bf9dd0 Mon Sep 17 00:00:00 2001 From: Yujong Lee Date: Fri, 23 Oct 2026 10:25:57 -0700 Subject: [PATCH] docs(rust): split rust-error skill into thin SKILL.md with references Co-Authored-By: Claude Opus 5.5 --- .../.agents/skills/rust-error/SKILL.md | 92 ++++------------ .../rust-error/references/boundaries.md | 37 +++++++ .../rust-error/references/classification.md | 46 ++++++++ .../rust-error/references/conversions.md | 88 +++++++++++++++ .../references/messages-and-causes.md | 100 ++++++++++++++++++ .../skills/rust-error/references/recipes.md | 56 ++++++++++ .../skills/rust-error/references/shape.md | 93 ++++++++++++++++ .../skills/rust-error/references/testing.md | 34 ++++++ 8 files changed, 476 insertions(+), 70 deletions(-) create mode 100644 litellm-rust/.agents/skills/rust-error/references/boundaries.md create mode 100644 litellm-rust/.agents/skills/rust-error/references/classification.md create mode 100644 litellm-rust/.agents/skills/rust-error/references/conversions.md create mode 100644 litellm-rust/.agents/skills/rust-error/references/messages-and-causes.md create mode 100644 litellm-rust/.agents/skills/rust-error/references/recipes.md create mode 100644 litellm-rust/.agents/skills/rust-error/references/shape.md create mode 100644 litellm-rust/.agents/skills/rust-error/references/testing.md diff --git a/litellm-rust/.agents/skills/rust-error/SKILL.md b/litellm-rust/.agents/skills/rust-error/SKILL.md index ea44704363c..2eff7e2bdab 100644 --- a/litellm-rust/.agents/skills/rust-error/SKILL.md +++ b/litellm-rust/.agents/skills/rust-error/SKILL.md @@ -1,78 +1,30 @@ --- name: rust-error -description: Define, wrap or refactor Rust error types in litellm-rust with thiserror, keeping messages in one place, causes typed, and classification done once +description: Define, wrap, convert, classify, report or test Rust error types in litellm-rust with thiserror. Use when adding an error type or variant, mapping one crate's error into another, turning an error into a gateway response or Python exception, deciding retry or fault from an error, or replacing call sites that build errors from strings --- # Rust errors -Use this skill when adding an error type or variant, mapping one crate's error into another, or replacing call sites that build errors by hand. `crates/http/src/error.rs` is the reference implementation of everything below +`crates/http/src/error.rs` is the reference implementation. Read the reference for the part you are touching before editing, each one carries the reasoning and worked examples behind its rules -## Where errors live +- [shape](references/shape.md) + - One top-level `Error` per crate in `src/error.rs`. A public enum by default, an opaque struct over a private `Repr` enum when the variants must stay private + - A variant is a failure mode a caller handles differently. Provider names and guidance are data, never variants +- [messages and causes](references/messages-and-causes.md) + - Every message is written once, in `#[error(...)]`. Call sites pass typed facts, never a string + - Store the cause itself, never its string, and render it once + - Strip `reqwest::Error` URLs where they enter our types +- [conversions](references/conversions.md) + - One `From` per source and target pair, only when the conversion needs no context. It classifies and sanitizes once + - Nest a lower crate's error in one variant. Translate when the meaning changes, never mirror its variants +- [classification](references/classification.md) + - Callers ask predicates named after their decision, they do not match nested variants + - Classification states facts. Retry and fallback policy lives with the caller that knows idempotency +- [boundaries](references/boundaries.md) + - The gateway and the Python bridge are the only places an error becomes text for someone else +- [testing](references/testing.md) + - Test predicates, chain shape and redaction on errors built the way production builds them +- [recipes](references/recipes.md) + - Common tasks walked through end to end -A crate's errors live in `src/error.rs`, defined with `thiserror`, and are re-exported from `lib.rs`. Default to one top-level `Error` per crate with one variant per failure mode. A failure mode is something a caller handles differently, such as phase, status code, retry, or a message Python parity pins exactly. Failures no caller tells apart share one variant - -Keep shared error types minimal and provider-neutral. Provider names, credential types, configuration fields and setup guidance belong in caller-supplied data such as `MissingApiBase { provider: "Azure", guidance: "..." }`, not in dedicated variants or hardcoded shared messages. An exact parity message does not justify a provider-specific variant when caller-supplied context can preserve it - -Split into separate types only when different functions fail in disjoint ways that different callers see, so no caller has to match variants its function can never return. Name a split type after what went wrong, not after the function that returns it. A type whose fields are already typed data with the message in `#[error(...)]`, such as `litellm_http::request::HeaderError`, does not need folding into the crate error - -## Messages belong to the variant - -Every message template is written once, in the variant's `#[error(...)]`. A caller passes the typed facts the template needs and nothing else. None of these may appear at a call site - -```rust -Error::Network("WebSocket handshake returned a wrong Sec-WebSocket-Accept".into()) -Error::Request(format!("bad header {name}")) -Error::Signature(error.to_string()) -``` - -When a check fails for a distinct reason, that reason is a variant (`Repr::HandshakeAccept`), not a string inside a generic one. Format arguments can call functions and reach into fields (`#[error("could not read {}: {cause}", path.display())]`), so a hand-written `Display` is rarely needed - -## Keep causes typed - -Store the lower-level error itself, never its string. Flattening a cause loses `source()`, loses its type for downcasting, and for `reqwest::Error` leaks the request URL, which can carry credentials in the path or query. Strip the URL with `without_url()` where a `reqwest::Error` enters the type - -Render each cause once. Either the variant's message includes it or `source()` returns it, never both, or a reporter that walks the chain prints it twice. Our boundaries (the gateway error body, the Python bridge) stringify with `to_string()`, so a leaf variant shows its cause in its message and names the field `cause`. A field named `source` becomes the source automatically, and `#[from]` implies `#[source]`. A variant that only wraps another of our errors uses `#[error(transparent)]` so it adds no text - -## One conversion per foreign error - -Each foreign error type gets exactly one `From` impl in the crate that owns the wrapper, and that impl does all classification and sanitizing. `From for litellm_http::Error` decides connect versus network versus timeout and strips the URL, so every call site is `?` or `.map_err(litellm_http::Error::from)`. A call site never re-derives the classification with `is_connect()` checks or builds a variant from `error.to_string()` - -The same holds one layer up. When a lower error needs reclassifying for every caller, do it in the shared type's `From`, not per route. `From for litellm_inference::Error` turns a header the signer computes into `Unsupported`, so all four routes classify that input the same way - -## Nest, don't mirror - -Wrap a lower crate's error in one variant and delegate to it. Never re-declare its variants in the upper type with an upper-layer name and a 1:1 mapping function, as OCR once did for every media download failure. The upper type's predicates call through, as in `Self::Http(error) => error.is_request()` - -## Ask, don't match - -Downstream crates ask a lower error questions instead of matching its nested variants. `litellm_http::Error` answers `dispatched()`, `is_request()`, `is_response()`, `status()`, `upstream_response()` and `tls_source()`. Add a predicate when a real caller needs a new distinction, and name it after what the caller decides, not after the variant - -## Opaque errors - -Use the opaque newtype only when a crate wraps third-party causes that are not `Clone` or `PartialEq` and a consumer needs those traits, or when the variants must not become public API. `reqwest::Error`, `std::io::Error` and `serde_json::Error` are built this way - -```rust -#[derive(Clone, Debug, thiserror::Error)] -#[error(transparent)] -pub struct Error(Arc); - -#[derive(Debug, thiserror::Error)] -pub(crate) enum Repr { - #[error("upstream network error: {}", Chain(.0))] - Network(reqwest::Error), - #[error("upstream request failed with status {status}: {body}")] - Status { status: u16, body: String }, -} -``` - -`Repr` holds the real causes. `Arc` makes `Clone` a refcount bump, so `Repr` never has to be `Clone`. `#[error(transparent)]` forwards `Display` and `source()`. Other crates cannot name a variant, so they cannot build an error with a message of their own, and the compiler enforces the rules above. Public constructors exist only for errors another crate genuinely produces, take typed data (`Error::upstream(status, body)`, `Error::computed_header(name)`, `Error::signature(cause)`), and never take a message. `PartialEq` compares by `Arc::ptr_eq`, the same as `SecretError` and `ErrorSource`, so a wrapping enum can keep its derives - -A crate whose errors are its own data, which nobody outside matches on, keeps a plain `#[derive(Clone, Debug, thiserror::Error)]` enum - -## Testing errors - -Assert behavior through the predicates and accessors (`error.upstream_response() == Some((429, "slow down"))`, `!error.dispatched()`), not by constructing an expected variant with a guessed message. Produce errors the way production does, through a public constructor, a real failing call, or a real cause such as `tungstenite::Error::ConnectionClosed`. Inside the owning crate a test may match the private representation through a `#[cfg(test)]` accessor - -## thiserror reference - -The workspace used thiserror 2.0 when this was written. The [derive docs](https://docs.rs/thiserror/latest/thiserror/) cover format arguments, `#[from]`, `#[source]` and `#[error(transparent)]`. `#[from]` only works on a variant with no other fields besides the source and an optional backtrace, so a variant that needs context such as a path takes its cause through an explicit `map_err`. `#[backtrace]` needs a nightly compiler and is not used here +The workspace uses thiserror 2. The [derive docs](https://docs.rs/thiserror/latest/thiserror/) are the authority on macro behavior diff --git a/litellm-rust/.agents/skills/rust-error/references/boundaries.md b/litellm-rust/.agents/skills/rust-error/references/boundaries.md new file mode 100644 index 00000000000..d7f5c8d64ac --- /dev/null +++ b/litellm-rust/.agents/skills/rust-error/references/boundaries.md @@ -0,0 +1,37 @@ +# Boundaries + +An error becomes text for someone else in exactly two places, and both are explicit mappings. Nothing else formats an error for a client + +| Boundary | File | Maps | +|---|---|---| +| Gateway HTTP response | `crates/gateway-inference/src/error.rs` | `status()` picks the code, `body()` builds the Anthropic or OpenAI envelope | +| Python bridge | `crates/python-bridge/src/errors.rs` and `crates/python-bridge/src/routes/ocr/errors.rs` | `RustUpstreamError(status, body)` for provider answers, `ValueError` for request faults and `RuntimeError` otherwise | + +Both follow the same order + +```rust +match error.upstream_response() { + Some((status, body)) => pass the provider's status and body through, + None => by_fault(error.is_request(), error.to_string()), +} +``` + +So a new failure only needs the right predicate answers to land correctly on both boundaries. If you find yourself adding an arm for your variant at a boundary, check whether a predicate is missing first + +## What reaches the client + +`to_string()` of our error is what a client sees for anything that is not a provider answer. That is why messages are written for a reader, causes are rendered once and reqwest URLs are stripped on entry + +Provider bodies are passed through, so bound them. Build `Error::upstream` with `truncate_error_body(&text)` from `litellm_http::request`, which caps the body at 256 characters + +```rust +if !status.is_success() { + return Err(litellm_http::Error::upstream(status.as_u16(), truncate_error_body(&text)).into()); +} +``` + +Never format `Debug` of an error into a response or a Python exception + +## Logging + +Propagate with context, and let the terminal handler own the one log or trace event for a failed call. Do not log at every `?`. Diagnostic tracing excludes raw error strings and bodies, see [`rust-tracing`](../../rust-tracing/SKILL.md) diff --git a/litellm-rust/.agents/skills/rust-error/references/classification.md b/litellm-rust/.agents/skills/rust-error/references/classification.md new file mode 100644 index 00000000000..cf348c2774a --- /dev/null +++ b/litellm-rust/.agents/skills/rust-error/references/classification.md @@ -0,0 +1,46 @@ +# Classification + +## Ask, don't match + +Downstream crates ask a lower error questions instead of matching its nested variants. `litellm_http::Error` answers these + +| Predicate | Caller decides | +|---|---| +| `dispatched()` | whether retrying elsewhere could repeat a billed call | +| `is_request()` | the caller's input is at fault, so 400 and no reroute | +| `is_response()` | the provider answered with something we cannot use | +| `upstream_response()` | the provider's own status and body to pass through | +| `status()` | an HTTP status to report | +| `tls_source()` | which TLS setting to blame in a config error | + +```rust +// reaches through two crates' variants and breaks when either changes +Self::Route(InferenceError::Transport(TransportError::Http { status, .. })) => status, + +// asks the question it cares about +Self::Route(error) if let Some((status, _)) = error.upstream_response() => status, +``` + +Add a predicate when a real caller needs a new distinction, and name it after what the caller decides, not after the variant. An upper type's predicates call through to the nested error, as in `Self::Http(error) => error.is_request()` + +A public enum may still be matched where matching is the contract, such as `litellm_auth::Error::MissingApiKey` mapping to 401. Matching is fine at the top of the variant tree, reaching through a nested error is what to avoid + +## Facts, not policy + +A predicate reports what happened. The decision to retry, reroute or fall back depends on things the error cannot know, such as whether the request is idempotent, whether its body can be replayed, the deadline and the attempt budget. That decision lives with the caller + +A timeout is the main trap. A request that timed out may already have reached the provider and been billed, so `Timeout` answers `dispatched() == true` and is not safe to retry as if it were a connect failure. Only a failure that provably happened before any byte left, such as connect or request building, answers `false` + +```rust +pub fn dispatched(&self) -> bool { + matches!(*self.0, Repr::Network(_) | Repr::Timeout(_) | Repr::Status { .. } | ..) +} +``` + +When the transport cannot tell whether a request went out, it must answer `true`. Wrongly answering `false` risks a duplicate billed call, wrongly answering `true` only costs a reroute + +Keep one predicate per question. `status()` currently mixes the provider's status, the media download's status and a synthesized 408 for timeouts, so a caller that wants "did the provider answer" must use `upstream_response()` instead. Do not add more meanings to an existing predicate, add a new one + +## Equality + +`Arc` errors compare with `Arc::ptr_eq`, so a clone equals its original and two separately built errors never compare equal. That exists only so a wrapping enum such as `litellm_inference::Error` can keep its `PartialEq` derive. Do not give a new type pointer equality for any other reason, and do not rely on it in tests. Assert predicates instead, see [testing](testing.md) diff --git a/litellm-rust/.agents/skills/rust-error/references/conversions.md b/litellm-rust/.agents/skills/rust-error/references/conversions.md new file mode 100644 index 00000000000..817ae082729 --- /dev/null +++ b/litellm-rust/.agents/skills/rust-error/references/conversions.md @@ -0,0 +1,88 @@ +# Conversions + +## One From per source and target pair + +A `From for Ours` exists when the conversion is obvious, infallible and needs no context from the call site. It does all classification and sanitizing for that pair, so every call site is `?` or `.map_err(Ours::from)` + +```rust +impl From for Error { + fn from(error: reqwest::Error) -> Self { + let error = error.without_url(); + if error.is_timeout() { + Repr::Timeout(error).into() + } else if error.is_connect() || error.is_builder() { + Repr::Connect(error).into() + } else { + Repr::Network(error).into() + } + } +} +``` + +A call site never re-derives that classification + +```rust +// every route used to repeat this, and each drifted a little +.map_err(|err| { + if err.is_connect() || err.is_builder() { + Error::Transport(Connect(err.to_string())) + } else { + Error::Transport(Network(err.to_string())) + } +})? + +// now +.map_err(litellm_http::Error::from)? +``` + +The same foreign type may convert into several of our types, one `From` each. What is not allowed is two places converting the same pair differently + +## When the call site knows something the From cannot + +One `io::Error` can mean "reading the CA bundle failed" or "resolving the media host failed". A global `From` cannot know which, so there is none. Name the operation at the call site instead + +```rust +.map_err(Repr::Resolve)? +.map_err(|cause| Repr::Read { path, tls_source, cause })? +``` + +Do not drop meaningful context just to use `?` + +## Reclassify once, in the shared From + +When a lower error needs a different meaning for every caller, do it in the upper type's `From`, not per route + +```rust +impl From for Error { + fn from(error: litellm_http::Error) -> Self { + if error.is_computed_header() { + return Self::Unsupported("request forwards a header AWS SigV4 computes"); + } + Self::Http(error) + } +} +``` + +All four inference routes classify a header the signer computes the same way, because none of them does it themselves + +## Nest, translate or add context, never mirror + +| The upper layer | Do | Example | +|---|---|---| +| has nothing to add | nest in one transparent variant and delegate predicates | `Self::Http(error) => error.is_request()` | +| changes what the failure means | translate to an upper variant, keep the cause attached when it helps | computed header -> `Unsupported` | +| knows something the lower one does not | a variant with the extra fields plus the cause | `Read { path, tls_source, cause }` | + +Never re-declare a lower crate's variants in the upper type with an upper-layer name and a 1:1 mapping function. OCR once did that for every media download failure, `MediaError::BlockedUrl => Error::BlockedDocumentUrl` and so on, which doubled every change and let the two lists drift + +## Third-party and host faults + +Converting a fault from a crate we do not own into a string variant is still flattening. Prefer a variant that carries the fault, and when an existing `ErrorDetail` constructor fits, use it + +```rust +// flattens +.map_err(|error| Error::InvalidRequest(error.to_string().into())) + +// keeps the cause typed +.map_err(|error| Error::InvalidRequest(ErrorDetail::invalid("header", error))) +``` diff --git a/litellm-rust/.agents/skills/rust-error/references/messages-and-causes.md b/litellm-rust/.agents/skills/rust-error/references/messages-and-causes.md new file mode 100644 index 00000000000..c30c81d84ab --- /dev/null +++ b/litellm-rust/.agents/skills/rust-error/references/messages-and-causes.md @@ -0,0 +1,100 @@ +# Messages and causes + +## Messages belong to the variant + +Every message template is written once, in the variant's `#[error(...)]`. A caller passes the typed facts the template needs and nothing else + +```rust +// none of these may appear at a call site +Error::Network("WebSocket handshake returned a wrong Sec-WebSocket-Accept".into()) +Error::Request(format!("bad header {name}")) +Error::Signature(error.to_string()) +Error::AwsSignableRequest(format!("{name} is not visible ASCII")) + +// the call site names what happened, the variant owns the words +Repr::HandshakeAccept.into() +Error::signature(error) +Error::NonAsciiHeader { name } +``` + +When a check fails for a distinct reason, that reason is a variant or a field, never a string inside a generic variant + +Format arguments can call functions and reach into fields, so a hand-written `Display` is rarely needed + +```rust +#[error("could not read {}: {cause}", path.display())] +#[error("invalid model: {provider} has no model {model:?} - use one of: {}", supported.join(", "))] +``` + +Keep messages short and lowercase unless Python parity pins the exact text. Do not end them with a period, since a reporter may append `: {cause}` + +## Keep causes typed + +Store the lower-level error itself, never its string + +| Flattening loses | Because | +|---|---| +| `source()` | a `String` has no cause | +| downcasting | the type is gone | +| redaction | `reqwest::Error`'s `Display` includes the request URL, which can carry credentials in the path or query | + +If the cause is not `Clone` and the error must be, wrap the cause in `Arc` or make the crate's error opaque. Do not stringify to get `Clone` + +```rust +// loses the type and leaks the URL +.map_err(|error| Error::Network(error.to_string())) + +// keeps the cause, the From impl strips the URL +.map_err(litellm_http::Error::from) +``` + +## Strip reqwest URLs on entry + +Call `without_url()` where a `reqwest::Error` enters our type, which in practice is `From for litellm_http::Error`. Once it is inside, every later `Display`, `Debug` or chain walk is safe from that URL + +`without_url()` only removes the URL reqwest attached. It does not scrub headers, response bodies, provider messages, file paths or anything a variant stores next to it. Those need their own policy, see [boundaries](boundaries.md) + +## Render each cause once + +Either the variant's message includes the cause or `source()` returns it, never both, or a reporter that walks the chain prints it twice. This is the rule from the [`std::error::Error` docs](https://doc.rust-lang.org/std/error/trait.Error.html) + +Our boundaries (the gateway error body and the Python bridge) render with `to_string()` and never walk the chain. So in this workspace a leaf variant puts its cause in its message, and stores it in a field named `cause` so thiserror does not also expose it as the source + +| Field | thiserror does | Use for | +|---|---|---| +| `cause: io::Error` rendered in `#[error]` | nothing, `source()` stays `None` | leaf foreign causes, the default here | +| `source: io::Error` or `#[source]` | exposes it as `source()` | causes the message does not mention | +| `#[from] io::Error` | implies `#[source]` and adds `From` | context-free wrapping, message must not mention it | +| `#[error(transparent)]` | forwards both `Display` and `source()` to the inner value | wrapping another of our errors that adds no text | + +```rust +// prints the reqwest error twice under a chain-walking reporter +#[error("request failed: {source}")] +Request { source: reqwest::Error }, + +// leaf, rendered once in the message +#[error("could not read {}: {cause}", path.display())] +Read { path: PathBuf, cause: io::Error }, + +// nested, adds no text of its own +#[error(transparent)] +Http(litellm_http::Error), +``` + +`#[error(transparent)]` is an invisible layer, not an extra chain node. A transparent wrapper's `source()` is the inner value's `source()`, so a transparent wrapper over a leaf has no source at all + +`reqwest::Error` keeps the useful part of its message in its own source chain (the hyper and io errors), so the `Chain` helper in `crates/http/src/error.rs` renders `reqwest::Error` plus its sources into one message. Use it for any reqwest cause in a message + +## #[from] limits + +`#[from]` only works on a variant whose only fields are the source and an optional backtrace. A variant that needs context such as a path, a phase or a `TlsSource` takes its cause through an explicit `map_err` or a named constructor + +```rust +std::fs::read(&path).map_err(|cause| Repr::Read { + path: path.clone(), + tls_source: TlsSource::CaBundle, + cause, +})?; +``` + +`#[backtrace]` and automatic `provide()` need a nightly compiler and are not used here. `std::backtrace::Backtrace` itself is stable, but do not add a field of that type to a derived error without checking how thiserror treats it diff --git a/litellm-rust/.agents/skills/rust-error/references/recipes.md b/litellm-rust/.agents/skills/rust-error/references/recipes.md new file mode 100644 index 00000000000..4511e8c972e --- /dev/null +++ b/litellm-rust/.agents/skills/rust-error/references/recipes.md @@ -0,0 +1,56 @@ +# Recipes + +## A new failure in litellm_http + +Say media downloads must reject a `Content-Type` outside an allow-list + +1. Add a `Repr` variant with typed facts and the message, `#[error("media type {0:?} is not allowed")] DisallowedMediaType(String)` +2. Return it where the check fails, `return Err(Repr::DisallowedMediaType(content_type.to_owned()).into())` +3. Answer every predicate on purpose, the way its neighbor `DownloadTooLarge` does. The download request went out, so it is in `dispatched()`. The caller chose the URL, so it is in `is_request()` +4. Leave both boundaries alone. With those answers the gateway returns 400 and Python raises `ValueError` +5. Test it with a mock server answering `Content-Type: application/x-msdownload` and assert `error.is_request()` + +## A provider-specific failure with a parity message + +Python raises `Missing REDUCTO_API_KEY - set it in the environment or pass api_key to litellm.ocr()/litellm.aocr()` and a test pins the text. The OCR error carries this today as `MissingReductoApiKey`, which is the shape to avoid + +1. Look for an existing provider-neutral variant whose template produces the exact text. `litellm_auth::Error::MissingApiKey { provider, environment_variable }` renders `Missing {provider} API Key - Set ...`, which does not match +2. Find what varies between providers with the same failure. Here it is the environment variable and the guidance after the dash +3. Add one neutral variant with those as fields, `#[error("Missing {environment_variable} - {guidance}")] MissingCredential { environment_variable: &'static str, guidance: &'static str }`, and pass Reducto's values from the Reducto transformation +4. The next provider with the same failure passes its own values and adds no variant. Boundaries that mapped `MissingReductoApiKey` to 401 now match `MissingCredential { .. }` once + +## Wrapping a new lower crate in litellm_inference + +Say routes start calling `litellm_cache` and its errors must surface + +1. Add one variant, `#[error(transparent)] Cache(litellm_cache::Error)`, with `#[from]` if no context is needed +2. Extend `is_request()` and `upstream_response()` with an arm that calls through to the cache error's own predicates, adding a predicate there if it has none +3. Do not copy the cache error's variants into `litellm_inference::Error` + +## A foreign error that needs context + +Reading a file fails with `io::Error`, and the message must name the file + +1. A variant with the context and the cause, `Read { path: PathBuf, cause: io::Error }` with `#[error("could not read {}: {cause}", path.display())]` +2. Build it with `map_err(|cause| Repr::Read { path: path.clone(), cause })`, not a `From` +3. If the error must be `Clone`, the crate's error is opaque or the field is `Arc` + +## Migrating a stringly error + +Call sites look like `Error::Network(error.to_string())` or `Error::Request(format!(...))` + +1. List every call site of the string variant and group them by what actually failed +2. Each group becomes a variant holding the typed cause or facts. Groups no caller tells apart share one +3. Where a foreign error was stringified with classification logic around it, move that logic into the `From` and turn the call sites into `?` +4. Delete the string variant. If it cannot be deleted yet, nothing new may construct it +5. Re-run the boundary tests, since messages a client sees may change + +## Making a public enum opaque + +Downstream crates match nested variants, or a variant needs a non-`Clone` cause + +1. Rename the enum to `pub(crate) enum Repr` and add `pub struct Error(Arc)` with `#[error(transparent)]` and `From` +2. For each downstream `match`, find the question it asks and add a predicate. Replace the `match` with the predicate +3. For each downstream construction, add a constructor taking typed data, or move the failure into this crate +4. Swap test assertions to predicates and add the `#[cfg(test)] repr()` accessor for in-crate tests +5. Implement `PartialEq` with `Arc::ptr_eq` only if a wrapping enum's derive needs it diff --git a/litellm-rust/.agents/skills/rust-error/references/shape.md b/litellm-rust/.agents/skills/rust-error/references/shape.md new file mode 100644 index 00000000000..a0e3768c551 --- /dev/null +++ b/litellm-rust/.agents/skills/rust-error/references/shape.md @@ -0,0 +1,93 @@ +# Error shape + +## One top-level Error per crate + +A crate's errors live in `src/error.rs` and are re-exported from `lib.rs` as `Error`. A caller of any public function in the crate should only ever see `Result<_, crate::Error>`, so one `?` and one set of predicates cover the whole crate + +That one type takes one of two forms + +| Form | Use when | Example | +|---|---|---| +| Public enum | Callers outside the crate match variants, and every variant's data is ours, cheap to clone and compare | `litellm_inference::Error`, `litellm_auth::Error` | +| Opaque struct over a private `Repr` | The variants hold third-party causes, or the variants must be free to change without breaking callers | `litellm_http::Error` | + +Both forms are still one top-level `Error`. The opaque form just moves the enum behind `Repr`, where it is `pub(crate)` + +```rust +// public enum +#[derive(Clone, Debug, PartialEq, Eq, thiserror::Error)] +pub enum Error { + #[error("missing required field: {0}")] + MissingField(&'static str), + #[error(transparent)] + Http(litellm_http::Error), +} + +// opaque struct +#[derive(Clone, Debug, thiserror::Error)] +#[error(transparent)] +pub struct Error(Arc); + +#[derive(Debug, thiserror::Error)] +pub(crate) enum Repr { + #[error("could not reach the provider: {}", Chain(.0))] + Connect(reqwest::Error), + #[error("upstream request failed with status {status}: {body}")] + Status { status: u16, body: String }, +} +``` + +Start with the public enum. Move to opaque when a variant needs a non-`Clone` third-party cause, or when downstream crates keep matching variants in ways that duplicate classification. `reqwest::Error`, `std::io::Error` and `serde_json::Error` are built the opaque way + +In the opaque form other crates cannot name a variant, so they cannot build an error with their own message. Public constructors exist only for errors another crate genuinely produces, take typed data and never a message, as in `Error::upstream(status, body)`, `Error::computed_header(name)` and `Error::signature(cause)` + +`Arc` makes cloning a refcount bump, so `Repr` never has to be `Clone`. `Box` is enough when the goal is only a small `Result`. `Arc` is for when callers clone + +## What else may sit next to Error + +These are not a second top-level error and need no folding in + +- Leaf data types whose fields are typed and whose message is in `#[error(...)]`, such as `litellm_http::request::HeaderError`. A wrapping crate nests it as one variant +- Plain data used by predicates, such as `litellm_http::TlsSource` +- A private fault enum one variant carries, such as `PemFault` inside `Repr::InvalidPem` + +## Variants are failure modes + +A public variant is something a caller handles differently: a phase, a status code, retry versus no retry, fault on the request versus the environment, or a message Python parity pins exactly. Failures no caller tells apart share one variant and differ in their data + +```rust +// one variant per provider, so every new provider adds a variant and a match arm +#[error("Missing Azure API Base - set AZURE_API_BASE")] +MissingAzureApiBase, +#[error("Missing Vertex API Base - set VERTEXAI_API_BASE")] +MissingVertexApiBase, + +// one failure mode, the provider and its guidance are data +#[error("Missing {provider} API Base - {guidance}")] +MissingApiBase { provider: &'static str, guidance: &'static str }, +``` + +An exact parity message does not justify a provider-specific variant when caller-supplied data can produce the same text + +Inside a private `Repr` a variant costs no API, so one per distinct reason is fine when each one carries a different message. When several only differ in a word and share every predicate answer, prefer one variant with a fieldless reason enum, so predicates list one arm instead of five + +```rust +#[error("WebSocket handshake returned {0}")] +Handshake(HandshakeFault), + +#[derive(Debug, thiserror::Error)] +pub(crate) enum HandshakeFault { + #[error("a wrong Sec-WebSocket-Accept")] + Accept, + #[error("an invalid Upgrade header")] + Upgrade, +} +``` + +## When to split + +Split into a separate type only when different functions fail in disjoint ways and different callers see them, so no caller matches variants its function can never return. A type per function overdoes it + +Name a split type after what went wrong, not after the function that returns it. `ConnectError` reads better than `OpenSessionError`, and a unit struct is fine for a single failure mode + +A split type still converts into the crate's `Error` when a caller further up needs one `Result` type diff --git a/litellm-rust/.agents/skills/rust-error/references/testing.md b/litellm-rust/.agents/skills/rust-error/references/testing.md new file mode 100644 index 00000000000..e5e1eb6deb4 --- /dev/null +++ b/litellm-rust/.agents/skills/rust-error/references/testing.md @@ -0,0 +1,34 @@ +# Testing errors + +Assert behavior through predicates and accessors, on errors produced the way production produces them + +```rust +// guesses the message and the variant, and cannot be written once the type is opaque +assert_eq!(error, Error::Transport(TransportError::Http { status: 429, body: "slow down".into() })); + +// asserts what callers rely on +assert_eq!(error.upstream_response(), Some((429, "slow down"))); +assert!(!error.dispatched()); +``` + +## Producing the error + +In order of preference + +1. A real failing call. Bind a `TcpListener`, drop it, then connect to its address for a connect failure. Accept and never answer for a timeout +2. A real cause, such as `tungstenite::Error::ConnectionClosed` or a `serde_json::from_str("{")` error, passed through our `From` +3. A public constructor, such as `Error::upstream(500, "boom".into())` + +Inside the owning crate a test may match the private representation through a `#[cfg(test)] fn repr(&self) -> &Repr` accessor, as `crates/http/src/media.rs` does + +## What to cover + +| Property | Test shape | +|---|---| +| classification | one `#[case]` per input class, asserting the predicate | +| redaction | build from a URL holding `api_key=secret`, assert the message lacks `secret` and the path | +| cause kept | walk `source()` or downcast and find the original type, or assert the message contains the root cause's text | +| no double rendering | the cause's text appears once in the fully rendered chain | +| boundary mapping | the gateway status and body, or the Python exception type and its `status` attribute | + +Assert exact message text only where it is a contract, such as a message Python parity pins. Everywhere else assert that the message contains the facts it must carry