mirror of
https://github.com/instructkr/claw-code.git
synced 2026-04-07 00:24:50 +08:00
Expose actionable ids for opaque provider failures
Issue #22 was triggered by generic upstream fatal wrappers that only surfaced 'Something went wrong', which left repeated Jobdori-style failures opaque in the CLI. Capture provider request ids on error responses, classify the known generic wrapper as provider_internal, and prefix the user-visible runtime error with the failure class plus session/trace identifiers so operators can correlate the failure quickly. Constraint: Keep the fix small and user-safe without redesigning the broader runtime error taxonomy Constraint: Preserve existing non-generic error text unless the wrapper is the known opaque fatal surface Rejected: Broadly rewriting every runtime error into classified envelopes | unnecessary scope expansion for issue #22 Confidence: high Scope-risk: narrow Reversibility: clean Directive: If more opaque wrappers appear, extend the marker list and classification helper rather than reintroducing raw wrapper text alone Tested: cargo test -p api detects_generic_fatal_wrapper_and_classifies_it_as_provider_internal -- --nocapture; cargo test -p api retries_exhausted_preserves_nested_request_id_and_failure_class -- --nocapture; cargo test -p rusty-claude-cli opaque_provider_wrapper_surfaces_failure_class_session_and_trace -- --nocapture; cargo test -p rusty-claude-cli retry_exhaustion_preserves_internal_failure_class_for_generic_provider_wrapper -- --nocapture; cargo test --workspace Not-tested: Live upstream reproduction of the Jobdori failure against a real provider session
This commit is contained in:
@@ -2,6 +2,11 @@ use std::env::VarError;
|
|||||||
use std::fmt::{Display, Formatter};
|
use std::fmt::{Display, Formatter};
|
||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
|
|
||||||
|
const GENERIC_FATAL_WRAPPER_MARKERS: &[&str] = &[
|
||||||
|
"something went wrong while processing your request",
|
||||||
|
"please try again, or use /new to start a fresh session",
|
||||||
|
];
|
||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
pub enum ApiError {
|
pub enum ApiError {
|
||||||
MissingCredentials {
|
MissingCredentials {
|
||||||
@@ -25,6 +30,7 @@ pub enum ApiError {
|
|||||||
status: reqwest::StatusCode,
|
status: reqwest::StatusCode,
|
||||||
error_type: Option<String>,
|
error_type: Option<String>,
|
||||||
message: Option<String>,
|
message: Option<String>,
|
||||||
|
request_id: Option<String>,
|
||||||
body: String,
|
body: String,
|
||||||
retryable: bool,
|
retryable: bool,
|
||||||
},
|
},
|
||||||
@@ -65,6 +71,68 @@ impl ApiError {
|
|||||||
| Self::BackoffOverflow { .. } => false,
|
| Self::BackoffOverflow { .. } => false,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[must_use]
|
||||||
|
pub fn request_id(&self) -> Option<&str> {
|
||||||
|
match self {
|
||||||
|
Self::Api { request_id, .. } => request_id.as_deref(),
|
||||||
|
Self::RetriesExhausted { last_error, .. } => last_error.request_id(),
|
||||||
|
Self::MissingCredentials { .. }
|
||||||
|
| Self::ContextWindowExceeded { .. }
|
||||||
|
| Self::ExpiredOAuthToken
|
||||||
|
| Self::Auth(_)
|
||||||
|
| Self::InvalidApiKeyEnv(_)
|
||||||
|
| Self::Http(_)
|
||||||
|
| Self::Io(_)
|
||||||
|
| Self::Json(_)
|
||||||
|
| Self::InvalidSseFrame(_)
|
||||||
|
| Self::BackoffOverflow { .. } => None,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[must_use]
|
||||||
|
pub fn safe_failure_class(&self) -> &'static str {
|
||||||
|
match self {
|
||||||
|
Self::MissingCredentials { .. } | Self::ExpiredOAuthToken | Self::Auth(_) => {
|
||||||
|
"provider_auth"
|
||||||
|
}
|
||||||
|
Self::Api { status, .. } if matches!(status.as_u16(), 401 | 403) => "provider_auth",
|
||||||
|
Self::ContextWindowExceeded { .. } => "context_window",
|
||||||
|
Self::Api { status, .. } if status.as_u16() == 429 => "provider_rate_limit",
|
||||||
|
Self::Api { .. } | Self::RetriesExhausted { .. } if self.is_generic_fatal_wrapper() => {
|
||||||
|
"provider_internal"
|
||||||
|
}
|
||||||
|
Self::Api { .. } => "provider_error",
|
||||||
|
Self::Http(_) | Self::InvalidSseFrame(_) | Self::BackoffOverflow { .. } => {
|
||||||
|
"provider_transport"
|
||||||
|
}
|
||||||
|
Self::RetriesExhausted { .. } => "provider_retry_exhausted",
|
||||||
|
Self::InvalidApiKeyEnv(_) | Self::Io(_) | Self::Json(_) => "runtime_io",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[must_use]
|
||||||
|
pub fn is_generic_fatal_wrapper(&self) -> bool {
|
||||||
|
match self {
|
||||||
|
Self::Api { message, body, .. } => {
|
||||||
|
message
|
||||||
|
.as_deref()
|
||||||
|
.is_some_and(looks_like_generic_fatal_wrapper)
|
||||||
|
|| looks_like_generic_fatal_wrapper(body)
|
||||||
|
}
|
||||||
|
Self::RetriesExhausted { last_error, .. } => last_error.is_generic_fatal_wrapper(),
|
||||||
|
Self::MissingCredentials { .. }
|
||||||
|
| Self::ContextWindowExceeded { .. }
|
||||||
|
| Self::ExpiredOAuthToken
|
||||||
|
| Self::Auth(_)
|
||||||
|
| Self::InvalidApiKeyEnv(_)
|
||||||
|
| Self::Http(_)
|
||||||
|
| Self::Io(_)
|
||||||
|
| Self::Json(_)
|
||||||
|
| Self::InvalidSseFrame(_)
|
||||||
|
| Self::BackoffOverflow { .. } => false,
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Display for ApiError {
|
impl Display for ApiError {
|
||||||
@@ -102,13 +170,24 @@ impl Display for ApiError {
|
|||||||
status,
|
status,
|
||||||
error_type,
|
error_type,
|
||||||
message,
|
message,
|
||||||
|
request_id,
|
||||||
body,
|
body,
|
||||||
..
|
..
|
||||||
} => match (error_type, message) {
|
} => match (error_type, message) {
|
||||||
(Some(error_type), Some(message)) => {
|
(Some(error_type), Some(message)) => {
|
||||||
write!(f, "api returned {status} ({error_type}): {message}")
|
write!(f, "api returned {status} ({error_type})")?;
|
||||||
|
if let Some(request_id) = request_id {
|
||||||
|
write!(f, " [trace {request_id}]")?;
|
||||||
|
}
|
||||||
|
write!(f, ": {message}")
|
||||||
|
}
|
||||||
|
_ => {
|
||||||
|
write!(f, "api returned {status}")?;
|
||||||
|
if let Some(request_id) = request_id {
|
||||||
|
write!(f, " [trace {request_id}]")?;
|
||||||
|
}
|
||||||
|
write!(f, ": {body}")
|
||||||
}
|
}
|
||||||
_ => write!(f, "api returned {status}: {body}"),
|
|
||||||
},
|
},
|
||||||
Self::RetriesExhausted {
|
Self::RetriesExhausted {
|
||||||
attempts,
|
attempts,
|
||||||
@@ -151,3 +230,57 @@ impl From<VarError> for ApiError {
|
|||||||
Self::InvalidApiKeyEnv(value)
|
Self::InvalidApiKeyEnv(value)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn looks_like_generic_fatal_wrapper(text: &str) -> bool {
|
||||||
|
let lowered = text.to_ascii_lowercase();
|
||||||
|
GENERIC_FATAL_WRAPPER_MARKERS
|
||||||
|
.iter()
|
||||||
|
.any(|marker| lowered.contains(marker))
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use super::ApiError;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn detects_generic_fatal_wrapper_and_classifies_it_as_provider_internal() {
|
||||||
|
let error = ApiError::Api {
|
||||||
|
status: reqwest::StatusCode::INTERNAL_SERVER_ERROR,
|
||||||
|
error_type: Some("api_error".to_string()),
|
||||||
|
message: Some(
|
||||||
|
"Something went wrong while processing your request. Please try again, or use /new to start a fresh session."
|
||||||
|
.to_string(),
|
||||||
|
),
|
||||||
|
request_id: Some("req_jobdori_123".to_string()),
|
||||||
|
body: String::new(),
|
||||||
|
retryable: true,
|
||||||
|
};
|
||||||
|
|
||||||
|
assert!(error.is_generic_fatal_wrapper());
|
||||||
|
assert_eq!(error.safe_failure_class(), "provider_internal");
|
||||||
|
assert_eq!(error.request_id(), Some("req_jobdori_123"));
|
||||||
|
assert!(error.to_string().contains("[trace req_jobdori_123]"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn retries_exhausted_preserves_nested_request_id_and_failure_class() {
|
||||||
|
let error = ApiError::RetriesExhausted {
|
||||||
|
attempts: 3,
|
||||||
|
last_error: Box::new(ApiError::Api {
|
||||||
|
status: reqwest::StatusCode::BAD_GATEWAY,
|
||||||
|
error_type: Some("api_error".to_string()),
|
||||||
|
message: Some(
|
||||||
|
"Something went wrong while processing your request. Please try again, or use /new to start a fresh session."
|
||||||
|
.to_string(),
|
||||||
|
),
|
||||||
|
request_id: Some("req_nested_456".to_string()),
|
||||||
|
body: String::new(),
|
||||||
|
retryable: true,
|
||||||
|
}),
|
||||||
|
};
|
||||||
|
|
||||||
|
assert!(error.is_generic_fatal_wrapper());
|
||||||
|
assert_eq!(error.safe_failure_class(), "provider_internal");
|
||||||
|
assert_eq!(error.request_id(), Some("req_nested_456"));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -808,6 +808,7 @@ async fn expect_success(response: reqwest::Response) -> Result<reqwest::Response
|
|||||||
return Ok(response);
|
return Ok(response);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let request_id = request_id_from_headers(response.headers());
|
||||||
let body = response.text().await.unwrap_or_else(|_| String::new());
|
let body = response.text().await.unwrap_or_else(|_| String::new());
|
||||||
let parsed_error = serde_json::from_str::<AnthropicErrorEnvelope>(&body).ok();
|
let parsed_error = serde_json::from_str::<AnthropicErrorEnvelope>(&body).ok();
|
||||||
let retryable = is_retryable_status(status);
|
let retryable = is_retryable_status(status);
|
||||||
@@ -820,6 +821,7 @@ async fn expect_success(response: reqwest::Response) -> Result<reqwest::Response
|
|||||||
message: parsed_error
|
message: parsed_error
|
||||||
.as_ref()
|
.as_ref()
|
||||||
.map(|error| error.error.message.clone()),
|
.map(|error| error.error.message.clone()),
|
||||||
|
request_id,
|
||||||
body,
|
body,
|
||||||
retryable,
|
retryable,
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -906,6 +906,7 @@ async fn expect_success(response: reqwest::Response) -> Result<reqwest::Response
|
|||||||
return Ok(response);
|
return Ok(response);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let request_id = request_id_from_headers(response.headers());
|
||||||
let body = response.text().await.unwrap_or_default();
|
let body = response.text().await.unwrap_or_default();
|
||||||
let parsed_error = serde_json::from_str::<ErrorEnvelope>(&body).ok();
|
let parsed_error = serde_json::from_str::<ErrorEnvelope>(&body).ok();
|
||||||
let retryable = is_retryable_status(status);
|
let retryable = is_retryable_status(status);
|
||||||
@@ -918,6 +919,7 @@ async fn expect_success(response: reqwest::Response) -> Result<reqwest::Response
|
|||||||
message: parsed_error
|
message: parsed_error
|
||||||
.as_ref()
|
.as_ref()
|
||||||
.and_then(|error| error.error.message.clone()),
|
.and_then(|error| error.error.message.clone()),
|
||||||
|
request_id,
|
||||||
body,
|
body,
|
||||||
retryable,
|
retryable,
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -5239,6 +5239,7 @@ impl runtime::PermissionPrompter for CliPermissionPrompter {
|
|||||||
struct AnthropicRuntimeClient {
|
struct AnthropicRuntimeClient {
|
||||||
runtime: tokio::runtime::Runtime,
|
runtime: tokio::runtime::Runtime,
|
||||||
client: AnthropicClient,
|
client: AnthropicClient,
|
||||||
|
session_id: String,
|
||||||
model: String,
|
model: String,
|
||||||
enable_tools: bool,
|
enable_tools: bool,
|
||||||
emit_output: bool,
|
emit_output: bool,
|
||||||
@@ -5262,6 +5263,7 @@ impl AnthropicRuntimeClient {
|
|||||||
client: AnthropicClient::from_auth(resolve_cli_auth_source()?)
|
client: AnthropicClient::from_auth(resolve_cli_auth_source()?)
|
||||||
.with_base_url(api::read_base_url())
|
.with_base_url(api::read_base_url())
|
||||||
.with_prompt_cache(PromptCache::new(session_id)),
|
.with_prompt_cache(PromptCache::new(session_id)),
|
||||||
|
session_id: session_id.to_string(),
|
||||||
model,
|
model,
|
||||||
enable_tools,
|
enable_tools,
|
||||||
emit_output,
|
emit_output,
|
||||||
@@ -5301,11 +5303,13 @@ impl ApiClient for AnthropicRuntimeClient {
|
|||||||
};
|
};
|
||||||
|
|
||||||
self.runtime.block_on(async {
|
self.runtime.block_on(async {
|
||||||
let mut stream = self
|
let mut stream =
|
||||||
.client
|
self.client
|
||||||
.stream_message(&message_request)
|
.stream_message(&message_request)
|
||||||
.await
|
.await
|
||||||
.map_err(|error| RuntimeError::new(error.to_string()))?;
|
.map_err(|error| {
|
||||||
|
RuntimeError::new(format_user_visible_api_error(&self.session_id, &error))
|
||||||
|
})?;
|
||||||
let mut stdout = io::stdout();
|
let mut stdout = io::stdout();
|
||||||
let mut sink = io::sink();
|
let mut sink = io::sink();
|
||||||
let out: &mut dyn Write = if self.emit_output {
|
let out: &mut dyn Write = if self.emit_output {
|
||||||
@@ -5319,11 +5323,9 @@ impl ApiClient for AnthropicRuntimeClient {
|
|||||||
let mut pending_tool: Option<(String, String, String)> = None;
|
let mut pending_tool: Option<(String, String, String)> = None;
|
||||||
let mut saw_stop = false;
|
let mut saw_stop = false;
|
||||||
|
|
||||||
while let Some(event) = stream
|
while let Some(event) = stream.next_event().await.map_err(|error| {
|
||||||
.next_event()
|
RuntimeError::new(format_user_visible_api_error(&self.session_id, &error))
|
||||||
.await
|
})? {
|
||||||
.map_err(|error| RuntimeError::new(error.to_string()))?
|
|
||||||
{
|
|
||||||
match event {
|
match event {
|
||||||
ApiStreamEvent::MessageStart(start) => {
|
ApiStreamEvent::MessageStart(start) => {
|
||||||
for block in start.message.content {
|
for block in start.message.content {
|
||||||
@@ -5418,7 +5420,9 @@ impl ApiClient for AnthropicRuntimeClient {
|
|||||||
..message_request.clone()
|
..message_request.clone()
|
||||||
})
|
})
|
||||||
.await
|
.await
|
||||||
.map_err(|error| RuntimeError::new(error.to_string()))?;
|
.map_err(|error| {
|
||||||
|
RuntimeError::new(format_user_visible_api_error(&self.session_id, &error))
|
||||||
|
})?;
|
||||||
let mut events = response_to_events(response, out)?;
|
let mut events = response_to_events(response, out)?;
|
||||||
push_prompt_cache_record(&self.client, &mut events);
|
push_prompt_cache_record(&self.client, &mut events);
|
||||||
Ok(events)
|
Ok(events)
|
||||||
@@ -5426,6 +5430,23 @@ impl ApiClient for AnthropicRuntimeClient {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn format_user_visible_api_error(session_id: &str, error: &api::ApiError) -> String {
|
||||||
|
if error.is_generic_fatal_wrapper() {
|
||||||
|
let mut qualifiers = vec![format!("session {session_id}")];
|
||||||
|
if let Some(request_id) = error.request_id() {
|
||||||
|
qualifiers.push(format!("trace {request_id}"));
|
||||||
|
}
|
||||||
|
format!(
|
||||||
|
"{} ({}): {}",
|
||||||
|
error.safe_failure_class(),
|
||||||
|
qualifiers.join(", "),
|
||||||
|
error
|
||||||
|
)
|
||||||
|
} else {
|
||||||
|
error.to_string()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
fn final_assistant_text(summary: &runtime::TurnSummary) -> String {
|
fn final_assistant_text(summary: &runtime::TurnSummary) -> String {
|
||||||
summary
|
summary
|
||||||
.assistant_messages
|
.assistant_messages
|
||||||
@@ -6424,18 +6445,19 @@ mod tests {
|
|||||||
format_permissions_report, format_permissions_switch_report, format_pr_report,
|
format_permissions_report, format_permissions_switch_report, format_pr_report,
|
||||||
format_resume_report, format_status_report, format_tool_call_start, format_tool_result,
|
format_resume_report, format_status_report, format_tool_call_start, format_tool_result,
|
||||||
format_ultraplan_report, format_unknown_slash_command,
|
format_ultraplan_report, format_unknown_slash_command,
|
||||||
format_unknown_slash_command_message, normalize_permission_mode, parse_args,
|
format_unknown_slash_command_message, format_user_visible_api_error,
|
||||||
parse_git_status_branch, parse_git_status_metadata_for, parse_git_workspace_summary,
|
normalize_permission_mode, parse_args, parse_git_status_branch,
|
||||||
permission_policy, print_help_to, push_output_block, render_config_report,
|
parse_git_status_metadata_for, parse_git_workspace_summary, permission_policy,
|
||||||
render_diff_report, render_diff_report_for, render_memory_report, render_repl_help,
|
print_help_to, push_output_block, render_config_report, render_diff_report,
|
||||||
render_resume_usage, resolve_model_alias, resolve_session_reference, response_to_events,
|
render_diff_report_for, render_memory_report, render_repl_help, render_resume_usage,
|
||||||
|
resolve_model_alias, resolve_session_reference, response_to_events,
|
||||||
resume_supported_slash_commands, run_resume_command,
|
resume_supported_slash_commands, run_resume_command,
|
||||||
slash_command_completion_candidates_with_sessions, status_context, validate_no_args,
|
slash_command_completion_candidates_with_sessions, status_context, validate_no_args,
|
||||||
write_mcp_server_fixture, CliAction, CliOutputFormat, CliToolExecutor, GitWorkspaceSummary,
|
write_mcp_server_fixture, CliAction, CliOutputFormat, CliToolExecutor, GitWorkspaceSummary,
|
||||||
InternalPromptProgressEvent, InternalPromptProgressState, LiveCli, LocalHelpTopic,
|
InternalPromptProgressEvent, InternalPromptProgressState, LiveCli, LocalHelpTopic,
|
||||||
SlashCommand, StatusUsage, DEFAULT_MODEL,
|
SlashCommand, StatusUsage, DEFAULT_MODEL,
|
||||||
};
|
};
|
||||||
use api::{MessageResponse, OutputContentBlock, Usage};
|
use api::{ApiError, MessageResponse, OutputContentBlock, Usage};
|
||||||
use plugins::{
|
use plugins::{
|
||||||
PluginManager, PluginManagerConfig, PluginTool, PluginToolDefinition, PluginToolPermission,
|
PluginManager, PluginManagerConfig, PluginTool, PluginToolDefinition, PluginToolPermission,
|
||||||
};
|
};
|
||||||
@@ -6475,6 +6497,49 @@ mod tests {
|
|||||||
.expect("plugin tool registry should build")
|
.expect("plugin tool registry should build")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn opaque_provider_wrapper_surfaces_failure_class_session_and_trace() {
|
||||||
|
let error = ApiError::Api {
|
||||||
|
status: "500".parse().expect("status"),
|
||||||
|
error_type: Some("api_error".to_string()),
|
||||||
|
message: Some(
|
||||||
|
"Something went wrong while processing your request. Please try again, or use /new to start a fresh session."
|
||||||
|
.to_string(),
|
||||||
|
),
|
||||||
|
request_id: Some("req_jobdori_789".to_string()),
|
||||||
|
body: String::new(),
|
||||||
|
retryable: true,
|
||||||
|
};
|
||||||
|
|
||||||
|
let rendered = format_user_visible_api_error("session-issue-22", &error);
|
||||||
|
assert!(rendered.contains("provider_internal"));
|
||||||
|
assert!(rendered.contains("session session-issue-22"));
|
||||||
|
assert!(rendered.contains("trace req_jobdori_789"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn retry_exhaustion_uses_retry_failure_class_for_generic_provider_wrapper() {
|
||||||
|
let error = ApiError::RetriesExhausted {
|
||||||
|
attempts: 3,
|
||||||
|
last_error: Box::new(ApiError::Api {
|
||||||
|
status: "502".parse().expect("status"),
|
||||||
|
error_type: Some("api_error".to_string()),
|
||||||
|
message: Some(
|
||||||
|
"Something went wrong while processing your request. Please try again, or use /new to start a fresh session."
|
||||||
|
.to_string(),
|
||||||
|
),
|
||||||
|
request_id: Some("req_jobdori_790".to_string()),
|
||||||
|
body: String::new(),
|
||||||
|
retryable: true,
|
||||||
|
}),
|
||||||
|
};
|
||||||
|
|
||||||
|
let rendered = format_user_visible_api_error("session-issue-22", &error);
|
||||||
|
assert!(rendered.contains("provider_retry_exhausted"), "{rendered}");
|
||||||
|
assert!(rendered.contains("session session-issue-22"));
|
||||||
|
assert!(rendered.contains("trace req_jobdori_790"));
|
||||||
|
}
|
||||||
|
|
||||||
fn temp_dir() -> PathBuf {
|
fn temp_dir() -> PathBuf {
|
||||||
let nanos = SystemTime::now()
|
let nanos = SystemTime::now()
|
||||||
.duration_since(UNIX_EPOCH)
|
.duration_since(UNIX_EPOCH)
|
||||||
|
|||||||
Reference in New Issue
Block a user