Include request URL in provider error messages#9232
Conversation
When a provider request fails (404, auth errors, server errors, etc.), the error message now includes the URL that was requested. This makes it much easier to troubleshoot configuration issues — especially 404s where the user needs to know which endpoint was hit. Fixes #7453 Signed-off-by: Douwe Osinga <douwe@squareup.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3634cbb174
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| StatusCode::OK => unreachable!("Should not call this function with OK status"), | ||
| StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => ProviderError::Authentication(format!( | ||
| "Authentication failed. Status: {}. Response: {}", | ||
| "Authentication failed for {url}. Status: {}. Response: {}", |
There was a problem hiding this comment.
Avoid interpolating raw url into user-facing ProviderError strings here, because several providers accept base URLs that preserve credentials and query parameters (see OpenAiProvider::parse_base_url tests around key=... and user:pass@... in crates/goose/src/providers/openai.rs). With this change, any non-2xx response now echoes the full request URL into errors/logs, so failed requests can leak API keys or passwords to telemetry and terminal output. Sanitize the URL (drop userinfo and sensitive query values) before including it in error text.
Useful? React with 👍 / 👎.
Strip userinfo (user:pass@) and query parameters (?key=...) from URLs before including them in ProviderError strings, since several providers accept base URLs that may contain embedded API keys or passwords. Signed-off-by: Douwe Osinga <douwe@squareup.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| let status = reqwest::StatusCode::from_u16(code) | ||
| .unwrap_or(reqwest::StatusCode::INTERNAL_SERVER_ERROR); | ||
| Self::enrich_credits_error(map_http_error_to_provider_error(status, Some(payload))) | ||
| Self::enrich_credits_error(map_http_error_to_provider_error(status, Some(payload), url)) |
There was a problem hiding this comment.
Did it forget to sanitize here? Or is it that tetrate URLs don't have secrets in them.
I wonder whether having the URL be part of the error type (outside of the string) and having it be sanitised in the Display/Debug impl might be safer than needing to remember to sanitise it before constructing the error?
There was a problem hiding this comment.
yeah, that's probably better. going to merge it though to get ahead
…in-error-messages # Conflicts: # crates/goose/src/providers/venice.rs
2 participants
When a provider request fails (404, auth errors, server errors, etc.), the error message now includes the URL that was requested. This makes it much easier to troubleshoot configuration issues — especially 404s where the user needs to know which endpoint was hit.
The change threads the request URL through
map_http_error_to_provider_error()andhandle_status()in the central HTTP error handling path, as well as all direct callers across provider implementations (Anthropic, Databricks, GCP Vertex AI, Google, Snowflake, Tetrate, Venice) and the Google-compat error handler in utils.rs.Fixes #7453