-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Upgrade error handling #6747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade error handling #6747
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR upgrades goose-server route error handling to return structured JSON error responses (with messages) back to the client instead of bare HTTP status codes.
Changes:
- Introduces additional
ErrorResponseconstructors andFrom<...>conversions to map common errors into HTTP responses. - Updates config management endpoints to propagate errors via
?intoErrorResponse, providing more informative client-visible failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/goose-server/src/routes/errors.rs | Extends ErrorResponse with helpers and additional error-to-response conversions (including provider/config/IO/serde errors). |
| crates/goose-server/src/routes/config_management.rs | Switches many handlers from StatusCode errors to ErrorResponse and propagates errors with ? to surface messages to clients. |
| impl From<std::io::Error> for ErrorResponse { | ||
| fn from(err: std::io::Error) -> Self { | ||
| Self::internal(format!("IO error: {}", err)) | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From<std::io::Error> (and the other internal(err.to_string()) conversions) will send raw OS/internal error details back to the client, which can expose filesystem paths or other sensitive context; consider returning a generic client-safe message for 500s and logging the underlying error server-side instead.
| let (status, message) = match err { | ||
| ProviderError::Authentication(_) => ( | ||
| StatusCode::BAD_REQUEST, | ||
| format!("Authentication failed: {}", err), | ||
| ), | ||
| ProviderError::UsageError(_) => ( | ||
| StatusCode::BAD_REQUEST, | ||
| format!("Usage error: {}", err), | ||
| ), | ||
| ProviderError::RateLimitExceeded { .. } => ( | ||
| StatusCode::TOO_MANY_REQUESTS, | ||
| format!("Rate limit exceeded: {}", err), | ||
| ), | ||
| _ => ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| format!("Provider error: {}", err), |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From<ProviderError> currently echoes the provider error text into the HTTP response (including authentication failures), which may leak upstream response details or credential hints to clients; consider mapping to client-safe messages (and possibly different status codes) while keeping the full ProviderError only in server logs/telemetry.
| let (status, message) = match err { | |
| ProviderError::Authentication(_) => ( | |
| StatusCode::BAD_REQUEST, | |
| format!("Authentication failed: {}", err), | |
| ), | |
| ProviderError::UsageError(_) => ( | |
| StatusCode::BAD_REQUEST, | |
| format!("Usage error: {}", err), | |
| ), | |
| ProviderError::RateLimitExceeded { .. } => ( | |
| StatusCode::TOO_MANY_REQUESTS, | |
| format!("Rate limit exceeded: {}", err), | |
| ), | |
| _ => ( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| format!("Provider error: {}", err), | |
| // Log full provider error for server-side diagnostics without exposing it to clients. | |
| eprintln!("ProviderError occurred: {:?}", err); | |
| let (status, message) = match err { | |
| ProviderError::Authentication(_) => ( | |
| StatusCode::BAD_REQUEST, | |
| "Authentication with upstream provider failed".to_string(), | |
| ), | |
| ProviderError::UsageError(_) => ( | |
| StatusCode::BAD_REQUEST, | |
| "Provider request was invalid for the upstream provider".to_string(), | |
| ), | |
| ProviderError::RateLimitExceeded { .. } => ( | |
| StatusCode::TOO_MANY_REQUESTS, | |
| "Upstream provider rate limit exceeded".to_string(), | |
| ), | |
| _ => ( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| "Upstream provider error".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
| let _ = goose::providers::refresh_custom_providers().await; | ||
|
|
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring the result of refresh_custom_providers() can cause this endpoint to report successful deletion even though the registry wasn’t refreshed; propagate the error (e.g., await?) so state stays consistent for callers.
| let _ = goose::providers::refresh_custom_providers().await; | ||
|
|
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring the result of refresh_custom_providers() can cause this endpoint to return success even if the updated provider definition isn’t loaded into the registry; propagate the error (e.g., await?) to avoid returning a false-positive success.
| let api_key: String = config.get_secret("OPENAI_API_KEY").map_err(|e| { | ||
| tracing::error!("Failed to get OpenAI API key: {:?}", e); | ||
| StatusCode::PRECONDITION_FAILED | ||
| ErrorResponse::bad_request(&format!("Failed to get OpenAI API key: {:?}", e)) | ||
| })?; |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_openai_config() maps any get_secret("OPENAI_API_KEY") failure to a 400 Bad Request, but non-client errors (e.g., config file/keyring errors) should likely be 5xx and a missing key should be a precondition-type status; consider matching on ConfigError to return PRECONDITION_FAILED/clear message for NotFound and INTERNAL_SERVER_ERROR for other variants.
| goose::scheduler::SchedulerError::JobNotFound(msg) => { | ||
| ErrorResponse::not_found(format!("Job not found: {}", msg)) | ||
| } |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scheduler.add_scheduled_job() cannot return SchedulerError::JobNotFound, so this match arm is unreachable and should be removed (or replaced with handling for the actual error variants like StorageError/PersistError).
| goose::scheduler::SchedulerError::JobNotFound(msg) => { | |
| ErrorResponse::not_found(format!("Job not found: {}", msg)) | |
| } |
| if let Err(e) = goose::providers::refresh_custom_providers().await { | ||
| tracing::warn!("Failed to refresh custom providers after creation: {}", e); | ||
| } | ||
| let _ = goose::providers::refresh_custom_providers().await; |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring the result of refresh_custom_providers() can cause this endpoint to return success even though the new provider won’t be available; propagate the error (e.g., await?) so clients can retry/fix the underlying issue.
| let _ = goose::providers::refresh_custom_providers().await; | |
| goose::providers::refresh_custom_providers() | |
| .await | |
| .map_err(|e| { | |
| ErrorResponse::internal(format!("Failed to refresh custom providers: {}", e)) | |
| })?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
| let _ = goose::providers::refresh_custom_providers().await; | ||
|
|
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refresh_custom_providers() errors are silently ignored here, which can leave the server and client out of sync (provider created but not loaded); handle the error explicitly (return an internal ErrorResponse or at least log and surface a warning to the client).
| let _ = goose::providers::refresh_custom_providers().await; | ||
|
|
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refresh_custom_providers() errors are silently ignored here, which can leave the server running with stale provider state after deletion; propagate the failure (or at least log it) so callers don’t get a false success response.
| let _ = goose::providers::refresh_custom_providers().await; | ||
|
|
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refresh_custom_providers() errors are silently ignored here, so an updated provider might not actually be reloaded; consider returning an error when refresh fails (or logging it) to avoid reporting success when the change hasn’t taken effect.
| pub async fn backup_config() -> Result<Json<String>, ErrorResponse> { | ||
| let config_path = Paths::config_dir().join("config.yaml"); | ||
|
|
||
| if config_path.exists() { | ||
| let file_name = config_path | ||
| .file_name() | ||
| .ok_or(StatusCode::INTERNAL_SERVER_ERROR)?; | ||
| if !config_path.exists() { | ||
| return Err(ErrorResponse::not_found("Config file does not exist")); | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backup_config can now return ErrorResponse::not_found (404) when config.yaml is missing, but the utoipa::path docs only declare a 500 response; update the documented responses to include 404 (or keep returning 500 if that’s intended).
| return Err(ErrorResponse::bad_request(format!( | ||
| "ElevenLabs API key is not a string, found: {:?}", | ||
| value | ||
| ); | ||
| return Err(StatusCode::PRECONDITION_FAILED); | ||
| ))); |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message includes the raw value for ELEVENLABS_API_KEY when it isn’t a string, which can leak a secret back to the client; avoid echoing the stored value (return a generic message and keep details in server logs only).
jamadeo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
* main: (47 commits) Upgrade error handling (#6747) Fix/filter audience 6703 local (#6773) chore: re-sync package-lock.json (#6783) upgrade electron to 39.3.0 (#6779) allow skipping providers in test_providers.sh (#6778) fix: enable custom model entry for OpenRouter provider (#6761) Remove codex skills flag support (#6775) Improve mcp test (#6671) Feat/anthropic custom headers (#6774) Fix/GitHub copilot error handling 5845 (#6771) fix(ui): respect width parameter in MCP app size-changed notifications (#6376) fix: address compilation issue in main (#6776) Upgrade GitHub Actions for Node 24 compatibility (#6699) fix(google): preserve thought signatures in streaming responses (#6708) added reduce motion support for css animations and streaming text (#6551) fix: Re-enable subagents for Gemini models (#6513) fix(google): use parametersJsonSchema for full JSON Schema support (#6555) fix: respect GOOSE_CLI_MIN_PRIORITY for shell streaming output (#6558) feat: add requires_auth flag for custom providers without authentication (#6705) fix: normalize extension names consistently in ExtensionManager (#6529) ...
* main: (30 commits) Different approach to determining final confidence level of prompt injection evaluation outcomes (#6729) fix: read_resource_tool deadlock causing test_compaction to hang (#6737) Upgrade error handling (#6747) Fix/filter audience 6703 local (#6773) chore: re-sync package-lock.json (#6783) upgrade electron to 39.3.0 (#6779) allow skipping providers in test_providers.sh (#6778) fix: enable custom model entry for OpenRouter provider (#6761) Remove codex skills flag support (#6775) Improve mcp test (#6671) Feat/anthropic custom headers (#6774) Fix/GitHub copilot error handling 5845 (#6771) fix(ui): respect width parameter in MCP app size-changed notifications (#6376) fix: address compilation issue in main (#6776) Upgrade GitHub Actions for Node 24 compatibility (#6699) fix(google): preserve thought signatures in streaming responses (#6708) added reduce motion support for css animations and streaming text (#6551) fix: Re-enable subagents for Gemini models (#6513) fix(google): use parametersJsonSchema for full JSON Schema support (#6555) fix: respect GOOSE_CLI_MIN_PRIORITY for shell streaming output (#6558) ...
* 'main' of github.com:block/goose: (62 commits) Swap canonical model from openrouter to models.dev (#6625) Hook thinking status (#6815) Fetch new skills hourly (#6814) copilot instructions: Update "No prerelease docs" instruction (#6795) refactor: centralize audience filtering before providers receive messages (#6728) update doc to remind contributors to activate hermit and document minimal npm and node version (#6727) nit: don't spit out compaction when in term mode as it fills up the screen (#6799) fix: correct tool support detection in Tetrate provider model fetching (#6808) Session manager fixes (#6809) fix(desktop): handle quoted paths with spaces in extension commands (#6430) fix: we can default gooseignore without writing it out (#6802) fix broken link (#6810) docs: add Beads MCP extension tutorial (#6792) feat(goose): add support for AWS_BEARER_TOKEN_BEDROCK environment variable (#6739) [docs] Add OSS Skills Marketplace (#6752) feat: make skills available in codemode (#6763) Fix: Recipe Extensions Not Loading in Desktop (#6777) Different approach to determining final confidence level of prompt injection evaluation outcomes (#6729) fix: read_resource_tool deadlock causing test_compaction to hang (#6737) Upgrade error handling (#6747) ...
…sion-session * 'main' of github.com:block/goose: (78 commits) copilot instructions: Update "No prerelease docs" instruction (#6795) refactor: centralize audience filtering before providers receive messages (#6728) update doc to remind contributors to activate hermit and document minimal npm and node version (#6727) nit: don't spit out compaction when in term mode as it fills up the screen (#6799) fix: correct tool support detection in Tetrate provider model fetching (#6808) Session manager fixes (#6809) fix(desktop): handle quoted paths with spaces in extension commands (#6430) fix: we can default gooseignore without writing it out (#6802) fix broken link (#6810) docs: add Beads MCP extension tutorial (#6792) feat(goose): add support for AWS_BEARER_TOKEN_BEDROCK environment variable (#6739) [docs] Add OSS Skills Marketplace (#6752) feat: make skills available in codemode (#6763) Fix: Recipe Extensions Not Loading in Desktop (#6777) Different approach to determining final confidence level of prompt injection evaluation outcomes (#6729) fix: read_resource_tool deadlock causing test_compaction to hang (#6737) Upgrade error handling (#6747) Fix/filter audience 6703 local (#6773) chore: re-sync package-lock.json (#6783) upgrade electron to 39.3.0 (#6779) ...
Summary
Report error messages back to the client