mirror of
https://github.com/openai/codex.git
synced 2026-11-01 19:33:35 +00:00
codex: preserve request ids on invalid requests
Carry optional request IDs on core invalid-request errors so pre-stream HTTP 400 failures still emit rollout response metadata. Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
co-authored by
Codex
parent
d80986824e
commit
21c1cf6a60
@@ -3509,7 +3509,7 @@ impl CodexMessageProcessor {
|
||||
INVALID_REQUEST_ERROR_CODE,
|
||||
format!("failed to load rollout `{}`: {err}", rollout_path.display()),
|
||||
),
|
||||
CodexErr::InvalidRequest(message) => (INVALID_REQUEST_ERROR_CODE, message),
|
||||
CodexErr::InvalidRequest(err) => (INVALID_REQUEST_ERROR_CODE, err.message),
|
||||
_ => (INTERNAL_ERROR_CODE, format!("error forking thread: {err}")),
|
||||
};
|
||||
let error = JSONRPCErrorError {
|
||||
|
||||
@@ -10,6 +10,7 @@ use serde::Deserialize;
|
||||
|
||||
use crate::auth::CodexAuth;
|
||||
use crate::error::CodexErr;
|
||||
use crate::error::InvalidRequestError;
|
||||
use crate::error::RetryLimitReachedError;
|
||||
use crate::error::UnexpectedResponseError;
|
||||
use crate::error::UsageLimitReachedError;
|
||||
@@ -31,7 +32,7 @@ pub(crate) fn map_api_error(err: ApiError) -> CodexErr {
|
||||
cf_ray: None,
|
||||
request_id: None,
|
||||
}),
|
||||
ApiError::InvalidRequest { message } => CodexErr::InvalidRequest(message),
|
||||
ApiError::InvalidRequest { message } => CodexErr::InvalidRequest(message.into()),
|
||||
ApiError::Transport(transport) => match transport {
|
||||
TransportError::Http {
|
||||
status,
|
||||
@@ -60,7 +61,10 @@ pub(crate) fn map_api_error(err: ApiError) -> CodexErr {
|
||||
{
|
||||
CodexErr::InvalidImageRequest()
|
||||
} else {
|
||||
CodexErr::InvalidRequest(body_text)
|
||||
CodexErr::InvalidRequest(InvalidRequestError {
|
||||
message: body_text,
|
||||
request_id: extract_request_id(headers.as_ref()),
|
||||
})
|
||||
}
|
||||
} else if status == http::StatusCode::INTERNAL_SERVER_ERROR {
|
||||
CodexErr::InternalServerError
|
||||
@@ -217,6 +221,37 @@ mod tests {
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn map_api_error_preserves_request_id_for_bad_request() {
|
||||
let mut headers = HeaderMap::new();
|
||||
headers.insert(
|
||||
REQUEST_ID_HEADER,
|
||||
http::HeaderValue::from_static("req_bad_123"),
|
||||
);
|
||||
let body = serde_json::json!({
|
||||
"error": {
|
||||
"message": "bad request"
|
||||
}
|
||||
})
|
||||
.to_string();
|
||||
|
||||
let err = map_api_error(ApiError::Transport(TransportError::Http {
|
||||
status: http::StatusCode::BAD_REQUEST,
|
||||
url: Some("http://example.com/v1/responses".to_string()),
|
||||
headers: Some(headers),
|
||||
body: Some(body),
|
||||
}));
|
||||
|
||||
let CodexErr::InvalidRequest(invalid_request) = err else {
|
||||
panic!("expected CodexErr::InvalidRequest, got {err:?}");
|
||||
};
|
||||
assert_eq!(
|
||||
invalid_request.message,
|
||||
r#"{"error":{"message":"bad request"}}"#
|
||||
);
|
||||
assert_eq!(invalid_request.request_id.as_deref(), Some("req_bad_123"));
|
||||
}
|
||||
}
|
||||
|
||||
fn extract_request_tracking_id(headers: Option<&HeaderMap>) -> Option<String> {
|
||||
|
||||
@@ -108,7 +108,7 @@ pub enum CodexErr {
|
||||
|
||||
/// Invalid request.
|
||||
#[error("{0}")]
|
||||
InvalidRequest(String),
|
||||
InvalidRequest(InvalidRequestError),
|
||||
|
||||
/// Invalid image.
|
||||
#[error("Image poisoning")]
|
||||
@@ -191,6 +191,22 @@ impl From<CancelErr> for CodexErr {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
#[error("{message}")]
|
||||
pub struct InvalidRequestError {
|
||||
pub message: String,
|
||||
pub request_id: Option<String>,
|
||||
}
|
||||
|
||||
impl From<String> for InvalidRequestError {
|
||||
fn from(message: String) -> Self {
|
||||
Self {
|
||||
message,
|
||||
request_id: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl CodexErr {
|
||||
pub fn is_retryable(&self) -> bool {
|
||||
match self {
|
||||
@@ -234,6 +250,7 @@ impl CodexErr {
|
||||
CodexErr::UnexpectedStatus(err) => err.request_id.as_deref(),
|
||||
CodexErr::RetryLimit(err) => err.request_id.as_deref(),
|
||||
CodexErr::ResponseStreamFailed(err) => err.request_id.as_deref(),
|
||||
CodexErr::InvalidRequest(err) => err.request_id.as_deref(),
|
||||
CodexErr::TurnAborted
|
||||
| CodexErr::Interrupted
|
||||
| CodexErr::Stream(..)
|
||||
@@ -249,7 +266,6 @@ impl CodexErr {
|
||||
| CodexErr::UsageNotIncluded
|
||||
| CodexErr::QuotaExceeded
|
||||
| CodexErr::InvalidImageRequest()
|
||||
| CodexErr::InvalidRequest(_)
|
||||
| CodexErr::RefreshTokenFailed(_)
|
||||
| CodexErr::UnsupportedOperation(_)
|
||||
| CodexErr::Sandbox(_)
|
||||
@@ -1106,6 +1122,21 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn invalid_request_request_id_defaults_to_none() {
|
||||
let err = CodexErr::InvalidRequest("plain invalid request".to_string().into());
|
||||
assert_eq!(err.request_id(), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn invalid_request_request_id_is_preserved() {
|
||||
let err = CodexErr::InvalidRequest(InvalidRequestError {
|
||||
message: "invalid request".to_string(),
|
||||
request_id: Some("req_invalid_123".to_string()),
|
||||
});
|
||||
assert_eq!(err.request_id(), Some("req_invalid_123"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn usage_limit_reached_includes_hours_and_minutes() {
|
||||
let base = Utc.with_ymd_and_hms(2024, 1, 1, 0, 0, 0).unwrap();
|
||||
|
||||
@@ -454,7 +454,7 @@ mod job {
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
serde_json::to_string(&filtered).map_err(|err| {
|
||||
CodexErr::InvalidRequest(format!("failed to serialize rollout memory: {err}"))
|
||||
CodexErr::InvalidRequest(format!("failed to serialize rollout memory: {err}").into())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -54,11 +54,14 @@ pub async fn build_memories_from_trace_files(
|
||||
.summarize_memories(raw_memories, model_info, effort, otel_manager)
|
||||
.await?;
|
||||
if output.len() != prepared.len() {
|
||||
return Err(CodexErr::InvalidRequest(format!(
|
||||
"unexpected memory summarize output length: expected {}, got {}",
|
||||
prepared.len(),
|
||||
output.len()
|
||||
)));
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
format!(
|
||||
"unexpected memory summarize output length: expected {}, got {}",
|
||||
prepared.len(),
|
||||
output.len()
|
||||
)
|
||||
.into(),
|
||||
));
|
||||
}
|
||||
|
||||
Ok(prepared
|
||||
@@ -116,10 +119,9 @@ fn load_trace_items(path: &Path, text: &str) -> Result<Vec<Value>> {
|
||||
.filter(serde_json::Value::is_object)
|
||||
.collect::<Vec<_>>();
|
||||
if dict_items.is_empty() {
|
||||
return Err(CodexErr::InvalidRequest(format!(
|
||||
"no object items found in trace file: {}",
|
||||
path.display()
|
||||
)));
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
format!("no object items found in trace file: {}", path.display()).into(),
|
||||
));
|
||||
}
|
||||
return normalize_trace_items(dict_items, path);
|
||||
}
|
||||
@@ -145,10 +147,9 @@ fn load_trace_items(path: &Path, text: &str) -> Result<Vec<Value>> {
|
||||
}
|
||||
|
||||
if parsed_items.is_empty() {
|
||||
return Err(CodexErr::InvalidRequest(format!(
|
||||
"no JSON items parsed from trace file: {}",
|
||||
path.display()
|
||||
)));
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
format!("no JSON items parsed from trace file: {}", path.display()).into(),
|
||||
));
|
||||
}
|
||||
|
||||
normalize_trace_items(parsed_items, path)
|
||||
@@ -193,10 +194,13 @@ fn normalize_trace_items(items: Vec<Value>, path: &Path) -> Result<Vec<Value>> {
|
||||
}
|
||||
|
||||
if normalized.is_empty() {
|
||||
return Err(CodexErr::InvalidRequest(format!(
|
||||
"no valid trace items after normalization: {}",
|
||||
path.display()
|
||||
)));
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
format!(
|
||||
"no valid trace items after normalization: {}",
|
||||
path.display()
|
||||
)
|
||||
.into(),
|
||||
));
|
||||
}
|
||||
Ok(normalized)
|
||||
}
|
||||
|
||||
@@ -79,7 +79,9 @@ impl RealtimeHandoffState {
|
||||
output_text,
|
||||
})
|
||||
.await
|
||||
.map_err(|_| CodexErr::InvalidRequest("conversation is not running".to_string()))?;
|
||||
.map_err(|_| {
|
||||
CodexErr::InvalidRequest("conversation is not running".to_string().into())
|
||||
})?;
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
@@ -183,7 +185,7 @@ impl RealtimeConversationManager {
|
||||
|
||||
let Some(sender) = sender else {
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
"conversation is not running".to_string(),
|
||||
"conversation is not running".to_string().into(),
|
||||
));
|
||||
};
|
||||
|
||||
@@ -194,7 +196,7 @@ impl RealtimeConversationManager {
|
||||
Ok(())
|
||||
}
|
||||
Err(TrySendError::Closed(_)) => Err(CodexErr::InvalidRequest(
|
||||
"conversation is not running".to_string(),
|
||||
"conversation is not running".to_string().into(),
|
||||
)),
|
||||
}
|
||||
}
|
||||
@@ -207,14 +209,13 @@ impl RealtimeConversationManager {
|
||||
|
||||
let Some(sender) = sender else {
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
"conversation is not running".to_string(),
|
||||
"conversation is not running".to_string().into(),
|
||||
));
|
||||
};
|
||||
|
||||
sender
|
||||
.send(text)
|
||||
.await
|
||||
.map_err(|_| CodexErr::InvalidRequest("conversation is not running".to_string()))?;
|
||||
sender.send(text).await.map_err(|_| {
|
||||
CodexErr::InvalidRequest("conversation is not running".to_string().into())
|
||||
})?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -223,7 +224,7 @@ impl RealtimeConversationManager {
|
||||
let guard = self.state.lock().await;
|
||||
let Some(state) = guard.as_ref() else {
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
"conversation is not running".to_string(),
|
||||
"conversation is not running".to_string().into(),
|
||||
));
|
||||
};
|
||||
state.handoff.clone()
|
||||
@@ -415,7 +416,9 @@ fn realtime_api_key(
|
||||
}
|
||||
|
||||
Err(CodexErr::InvalidRequest(
|
||||
"realtime conversation requires API key auth".to_string(),
|
||||
"realtime conversation requires API key auth"
|
||||
.to_string()
|
||||
.into(),
|
||||
))
|
||||
}
|
||||
|
||||
@@ -432,7 +435,7 @@ fn realtime_request_headers(
|
||||
}
|
||||
|
||||
let auth_value = HeaderValue::from_str(&format!("Bearer {api_key}")).map_err(|err| {
|
||||
CodexErr::InvalidRequest(format!("invalid realtime api key header: {err}"))
|
||||
CodexErr::InvalidRequest(format!("invalid realtime api key header: {err}").into())
|
||||
})?;
|
||||
headers.insert(AUTHORIZATION, auth_value);
|
||||
|
||||
|
||||
@@ -212,11 +212,11 @@ async fn rollout_persists_request_id_when_responses_request_errors() -> anyhow::
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = MockServer::start().await;
|
||||
let response = ResponseTemplate::new(418)
|
||||
let response = ResponseTemplate::new(400)
|
||||
.insert_header("x-request-id", "req_error_123")
|
||||
.set_body_json(json!({
|
||||
"error": {
|
||||
"message": "teapot"
|
||||
"message": "bad request"
|
||||
}
|
||||
}));
|
||||
Mock::given(method("POST"))
|
||||
|
||||
Reference in New Issue
Block a user