-
Notifications
You must be signed in to change notification settings - Fork 6k
fix: detect low balance and prompt for top up #7166
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
Changes from 4 commits
905430a
7261b7e
d7eb9d1
abba9cb
5e27201
6851b02
1430bcb
57f6a23
6fd91e3
a26dda3
0908e72
72f51c2
bc182c6
51ae6dd
9b41f7b
87f4c5a
b8a53c3
c5a654b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1433,6 +1433,41 @@ impl Agent { | |
| } | ||
| } | ||
| } | ||
| Err(ref provider_err @ ProviderError::CreditsExhausted { ref details, ref top_up_url }) => { | ||
| crate::posthog::emit_error(provider_err.telemetry_type(), &provider_err.to_string()); | ||
| error!("Credits exhausted: {}", details); | ||
|
|
||
| // Surface the error as a structured CreditsExhausted | ||
| // notification so the UI layer (CLI, desktop app, API) | ||
| // can decide how to present it — e.g. opening a browser, | ||
| // showing a dialog, or returning it in a JSON response. | ||
| let user_msg = if let Some(url) = top_up_url.as_deref() { | ||
| format!( | ||
| "Your credits have been exhausted: {details}\n\n\ | ||
| To add more credits, visit: {url}\n\n\ | ||
| Once you've topped up, retry your last message to continue." | ||
| ) | ||
| } else { | ||
| format!( | ||
| "Your credits have been exhausted: {details}\n\n\ | ||
| Please check your account with your provider to add more \ | ||
| credits, then retry your last message to continue." | ||
| ) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could remove the one line duplication here |
||
| }; | ||
|
|
||
| let notification_data = serde_json::json!({ | ||
| "top_up_url": top_up_url, | ||
| }); | ||
|
|
||
| yield AgentEvent::Message( | ||
| Message::assistant().with_system_notification_with_data( | ||
| SystemNotificationType::CreditsExhausted, | ||
| user_msg, | ||
| notification_data, | ||
| ) | ||
| ); | ||
| break; | ||
| } | ||
| Err(ref provider_err) => { | ||
| crate::posthog::emit_error(provider_err.telemetry_type(), &provider_err.to_string()); | ||
| error!("Error: {}", provider_err); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,6 +164,10 @@ pub struct FrontendToolRequest { | |
| pub enum SystemNotificationType { | ||
| ThinkingMessage, | ||
| InlineMessage, | ||
| /// Provider credits have been exhausted. The `data` field of the | ||
| /// notification may contain `{"top_up_url": "..."}` so the UI layer | ||
| /// can open the user's browser or show a clickable link. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this comment should go - or we need to document the others too |
||
| CreditsExhausted, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, ToSchema)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,6 +219,10 @@ | |
| StatusCode::NOT_FOUND => { | ||
| ProviderError::RequestFailed(format!("Resource not found (404): {}", extract_message())) | ||
| } | ||
| StatusCode::PAYMENT_REQUIRED => ProviderError::CreditsExhausted { | ||
| details: extract_message(), | ||
| top_up_url: None, | ||
| }, | ||
| StatusCode::PAYLOAD_TOO_LARGE => ProviderError::ContextLengthExceeded(extract_message()), | ||
| StatusCode::BAD_REQUEST => { | ||
| let payload_str = extract_message(); | ||
|
|
@@ -294,3 +298,98 @@ | |
| } | ||
| })) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use serde_json::json; | ||
|
|
||
| #[test] | ||
| fn http_402_maps_to_credits_exhausted() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see these tests fail unless we change functionality, but if you want to keep them, can we change them into the test_case pattern? |
||
| let payload = json!({ | ||
| "error": { | ||
| "message": "Insufficient credits to complete this request" | ||
| } | ||
| }); | ||
| let err = map_http_error_to_provider_error( | ||
| StatusCode::PAYMENT_REQUIRED, | ||
| Some(payload), | ||
| ); | ||
| match err { | ||
| ProviderError::CreditsExhausted { | ||
| ref details, | ||
| ref top_up_url, | ||
| } => { | ||
| assert!( | ||
| details.contains("Insufficient credits"), | ||
| "Expected details to contain error message, got: {details}" | ||
| ); | ||
| // Generic handler doesn't know the provider, so no URL | ||
| assert_eq!(*top_up_url, None); | ||
| } | ||
| other => panic!("Expected CreditsExhausted, got: {:?}", other), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn http_402_with_no_payload_maps_to_credits_exhausted() { | ||
| let err = map_http_error_to_provider_error(StatusCode::PAYMENT_REQUIRED, None); | ||
| assert!( | ||
| matches!(err, ProviderError::CreditsExhausted { .. }), | ||
| "Expected CreditsExhausted, got: {:?}", | ||
| err | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn http_429_maps_to_rate_limit_not_credits() { | ||
| let payload = json!({ | ||
| "error": { | ||
| "message": "Rate limit exceeded" | ||
| } | ||
| }); | ||
| let err = | ||
| map_http_error_to_provider_error(StatusCode::TOO_MANY_REQUESTS, Some(payload)); | ||
| assert!( | ||
| matches!(err, ProviderError::RateLimitExceeded { .. }), | ||
| "Expected RateLimitExceeded, got: {:?}", | ||
| err | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn http_401_maps_to_authentication() { | ||
| let err = map_http_error_to_provider_error(StatusCode::UNAUTHORIZED, None); | ||
| assert!( | ||
| matches!(err, ProviderError::Authentication(_)), | ||
| "Expected Authentication, got: {:?}", | ||
| err | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn http_400_with_context_length_maps_correctly() { | ||
| let payload = json!({ | ||
| "error": { | ||
| "message": "This request exceeds the maximum context length" | ||
| } | ||
| }); | ||
| let err = map_http_error_to_provider_error(StatusCode::BAD_REQUEST, Some(payload)); | ||
| assert!( | ||
| matches!(err, ProviderError::ContextLengthExceeded(_)), | ||
| "Expected ContextLengthExceeded, got: {:?}", | ||
| err | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn http_500_maps_to_server_error() { | ||
| let err = | ||
| map_http_error_to_provider_error(StatusCode::INTERNAL_SERVER_ERROR, None); | ||
| assert!( | ||
| matches!(err, ProviderError::ServerError(_)), | ||
| "Expected ServerError, got: {:?}", | ||
| err | ||
| ); | ||
| } | ||
| } | ||
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.
I could do without that comment (but also realizing I'm losing that battle0
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.
you know - I didn't look at a single bit of this PR, not even once (at least other than having AI.staged describe it too me - as wasn't sure if people cared!) but now they do, will tidy up