diff --git a/AGENTS.md b/AGENTS.md index db3216b4ec..4680c71432 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,6 +11,11 @@ In the codex-rs folder where the rust code lives: - Always collapse if statements per https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if - Always inline format! args when possible per https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args - Use method references over closures when possible per https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls +- Avoid bool or ambiguous `Option` parameters that force callers to write hard-to-read code such as `foo(false)` or `bar(None)`. Prefer enums, named methods, newtypes, or other idiomatic Rust API shapes when they keep the callsite self-documenting. +- When you cannot make that API change and still need a small positional-literal callsite in Rust, follow the `argument_comment_lint` convention: + - Use an exact `/*param_name*/` comment before opaque literal arguments such as `None`, booleans, and numeric literals when passing them by position. + - Do not add these comments for string or char literals unless the comment adds real clarity; those literals are intentionally exempt from the lint. + - If you add one of these comments, the parameter name must exactly match the callee signature. - When possible, make `match` statements exhaustive and avoid wildcard arms. - When writing tests, prefer comparing the equality of entire objects over fields one by one. - When making a change that adds or changes an API, ensure that the documentation in the `docs/` folder is up to date if applicable. diff --git a/codex-rs/cloud-requirements/src/lib.rs b/codex-rs/cloud-requirements/src/lib.rs index 18c95c3d37..11df536cc8 100644 --- a/codex-rs/cloud-requirements/src/lib.rs +++ b/codex-rs/cloud-requirements/src/lib.rs @@ -399,7 +399,7 @@ impl CloudRequirementsService { "Cloud requirements request was unauthorized; attempting auth recovery" ); match auth_recovery.next().await { - Ok(()) => { + Ok(_) => { let Some(refreshed_auth) = self.auth_manager.auth().await else { tracing::error!( "Auth recovery succeeded but no auth is available for cloud requirements" diff --git a/codex-rs/codex-api/src/endpoint/responses_websocket.rs b/codex-rs/codex-api/src/endpoint/responses_websocket.rs index 28923238b1..d3b578db69 100644 --- a/codex-rs/codex-api/src/endpoint/responses_websocket.rs +++ b/codex-rs/codex-api/src/endpoint/responses_websocket.rs @@ -214,6 +214,7 @@ impl ResponsesWebsocketConnection { pub async fn stream_request( &self, request: ResponsesWsRequest, + connection_reused: bool, ) -> Result { let (tx_event, rx_event) = mpsc::channel::>(1600); @@ -258,6 +259,7 @@ impl ResponsesWebsocketConnection { request_body, idle_timeout, telemetry, + connection_reused, ) .await }; @@ -534,6 +536,7 @@ async fn run_websocket_response_stream( request_body: Value, idle_timeout: Duration, telemetry: Option>, + connection_reused: bool, ) -> Result<(), ApiError> { let mut last_server_model: Option = None; let request_text = match serde_json::to_string(&request_body) { @@ -553,7 +556,11 @@ async fn run_websocket_response_stream( .map_err(|err| ApiError::Stream(format!("failed to send websocket request: {err}"))); if let Some(t) = telemetry.as_ref() { - t.on_ws_request(request_start.elapsed(), result.as_ref().err()); + t.on_ws_request( + request_start.elapsed(), + result.as_ref().err(), + connection_reused, + ); } result?; diff --git a/codex-rs/codex-api/src/telemetry.rs b/codex-rs/codex-api/src/telemetry.rs index 7b04fd2113..91918a65b9 100644 --- a/codex-rs/codex-api/src/telemetry.rs +++ b/codex-rs/codex-api/src/telemetry.rs @@ -33,7 +33,7 @@ pub trait SseTelemetry: Send + Sync { /// Telemetry for Responses WebSocket transport. pub trait WebsocketTelemetry: Send + Sync { - fn on_ws_request(&self, duration: Duration, error: Option<&ApiError>); + fn on_ws_request(&self, duration: Duration, error: Option<&ApiError>, connection_reused: bool); fn on_ws_event( &self, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index bc407863f0..a10a5a40da 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -338,9 +338,6 @@ "apps": { "type": "boolean" }, - "apps_mcp_gateway": { - "type": "boolean" - }, "artifact": { "type": "boolean" }, @@ -1890,9 +1887,6 @@ "apps": { "type": "boolean" }, - "apps_mcp_gateway": { - "type": "boolean" - }, "artifact": { "type": "boolean" }, diff --git a/codex-rs/core/src/api_bridge.rs b/codex-rs/core/src/api_bridge.rs index f363201ae9..2060b78cf7 100644 --- a/codex-rs/core/src/api_bridge.rs +++ b/codex-rs/core/src/api_bridge.rs @@ -1,3 +1,4 @@ +use base64::Engine; use chrono::DateTime; use chrono::Utc; use codex_api::AuthProvider as ApiAuthProvider; @@ -7,6 +8,7 @@ use codex_api::rate_limits::parse_promo_message; use codex_api::rate_limits::parse_rate_limit_for_limit; use http::HeaderMap; use serde::Deserialize; +use serde_json::Value; use crate::auth::CodexAuth; use crate::error::CodexErr; @@ -30,6 +32,8 @@ pub(crate) fn map_api_error(err: ApiError) -> CodexErr { url: None, cf_ray: None, request_id: None, + identity_authorization_error: None, + identity_error_code: None, }), ApiError::InvalidRequest { message } => CodexErr::InvalidRequest(message), ApiError::Transport(transport) => match transport { @@ -98,6 +102,11 @@ pub(crate) fn map_api_error(err: ApiError) -> CodexErr { url, cf_ray: extract_header(headers.as_ref(), CF_RAY_HEADER), request_id: extract_request_id(headers.as_ref()), + identity_authorization_error: extract_header( + headers.as_ref(), + X_OPENAI_AUTHORIZATION_ERROR_HEADER, + ), + identity_error_code: extract_x_error_json_code(headers.as_ref()), }) } } @@ -118,6 +127,8 @@ const ACTIVE_LIMIT_HEADER: &str = "x-codex-active-limit"; const REQUEST_ID_HEADER: &str = "x-request-id"; const OAI_REQUEST_ID_HEADER: &str = "x-oai-request-id"; const CF_RAY_HEADER: &str = "cf-ray"; +const X_OPENAI_AUTHORIZATION_ERROR_HEADER: &str = "x-openai-authorization-error"; +const X_ERROR_JSON_HEADER: &str = "x-error-json"; #[cfg(test)] #[path = "api_bridge_tests.rs"] @@ -140,6 +151,19 @@ fn extract_header(headers: Option<&HeaderMap>, name: &str) -> Option { }) } +fn extract_x_error_json_code(headers: Option<&HeaderMap>) -> Option { + let encoded = extract_header(headers, X_ERROR_JSON_HEADER)?; + let decoded = base64::engine::general_purpose::STANDARD + .decode(encoded) + .ok()?; + let parsed = serde_json::from_slice::(&decoded).ok()?; + parsed + .get("error") + .and_then(|error| error.get("code")) + .and_then(Value::as_str) + .map(str::to_string) +} + pub(crate) fn auth_provider_from_auth( auth: Option, provider: &ModelProviderInfo, @@ -191,6 +215,26 @@ pub(crate) struct CoreAuthProvider { account_id: Option, } +impl CoreAuthProvider { + pub(crate) fn auth_header_attached(&self) -> bool { + self.token + .as_ref() + .is_some_and(|token| http::HeaderValue::from_str(&format!("Bearer {token}")).is_ok()) + } + + pub(crate) fn auth_header_name(&self) -> Option<&'static str> { + self.auth_header_attached().then_some("authorization") + } + + #[cfg(test)] + pub(crate) fn for_test(token: Option<&str>, account_id: Option<&str>) -> Self { + Self { + token: token.map(str::to_string), + account_id: account_id.map(str::to_string), + } + } +} + impl ApiAuthProvider for CoreAuthProvider { fn bearer_token(&self) -> Option { self.token.clone() diff --git a/codex-rs/core/src/api_bridge_tests.rs b/codex-rs/core/src/api_bridge_tests.rs index e8391021b1..71d3889915 100644 --- a/codex-rs/core/src/api_bridge_tests.rs +++ b/codex-rs/core/src/api_bridge_tests.rs @@ -1,4 +1,5 @@ use super::*; +use base64::Engine; use pretty_assertions::assert_eq; #[test] @@ -94,3 +95,49 @@ fn map_api_error_does_not_fallback_limit_name_to_limit_id() { None ); } + +#[test] +fn map_api_error_extracts_identity_auth_details_from_headers() { + let mut headers = HeaderMap::new(); + headers.insert(REQUEST_ID_HEADER, http::HeaderValue::from_static("req-401")); + headers.insert(CF_RAY_HEADER, http::HeaderValue::from_static("ray-401")); + headers.insert( + X_OPENAI_AUTHORIZATION_ERROR_HEADER, + http::HeaderValue::from_static("missing_authorization_header"), + ); + let x_error_json = + base64::engine::general_purpose::STANDARD.encode(r#"{"error":{"code":"token_expired"}}"#); + headers.insert( + X_ERROR_JSON_HEADER, + http::HeaderValue::from_str(&x_error_json).expect("valid x-error-json header"), + ); + + let err = map_api_error(ApiError::Transport(TransportError::Http { + status: http::StatusCode::UNAUTHORIZED, + url: Some("https://chatgpt.com/backend-api/codex/models".to_string()), + headers: Some(headers), + body: Some(r#"{"detail":"Unauthorized"}"#.to_string()), + })); + + let CodexErr::UnexpectedStatus(err) = err else { + panic!("expected CodexErr::UnexpectedStatus, got {err:?}"); + }; + assert_eq!(err.request_id.as_deref(), Some("req-401")); + assert_eq!(err.cf_ray.as_deref(), Some("ray-401")); + assert_eq!( + err.identity_authorization_error.as_deref(), + Some("missing_authorization_header") + ); + assert_eq!(err.identity_error_code.as_deref(), Some("token_expired")); +} + +#[test] +fn core_auth_provider_reports_when_auth_header_will_attach() { + let auth = CoreAuthProvider { + token: Some("access-token".to_string()), + account_id: None, + }; + + assert!(auth.auth_header_attached()); + assert_eq!(auth.auth_header_name(), Some("authorization")); +} diff --git a/codex-rs/core/src/apps/render.rs b/codex-rs/core/src/apps/render.rs index da146f703b..7cc07c0747 100644 --- a/codex-rs/core/src/apps/render.rs +++ b/codex-rs/core/src/apps/render.rs @@ -4,7 +4,7 @@ use codex_protocol::protocol::APPS_INSTRUCTIONS_OPEN_TAG; pub(crate) fn render_apps_section() -> String { let body = format!( - "## Apps\nApps are mentioned in user messages in the format `[$app-name](app://{{connector_id}})`.\nAn app is equivalent to a set of MCP tools within the `{CODEX_APPS_MCP_SERVER_NAME}` MCP.\nWhen you see an app mention, the app's MCP tools are either available tools in the `{CODEX_APPS_MCP_SERVER_NAME}` MCP server, or the tools do not exist because the user has not installed the app.\nDo not additionally call list_mcp_resources for apps that are already mentioned." + "## Apps (Connectors)\nApps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{{connector_id}})`. Apps can also be implicitly triggered as long as the context suggests usage of available apps, the available apps will be listed by the `tool_search` tool.\nAn app is equivalent to a set of MCP tools within the `{CODEX_APPS_MCP_SERVER_NAME}` MCP.\nAn installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool_search` tool.\nDo not additionally call list_mcp_resources or list_mcp_resource_templates for apps." ); format!("{APPS_INSTRUCTIONS_OPEN_TAG}\n{body}\n{APPS_INSTRUCTIONS_CLOSE_TAG}") } diff --git a/codex-rs/core/src/auth.rs b/codex-rs/core/src/auth.rs index 8818aa5c6c..78aa693c52 100644 --- a/codex-rs/core/src/auth.rs +++ b/codex-rs/core/src/auth.rs @@ -874,6 +874,17 @@ pub struct UnauthorizedRecovery { mode: UnauthorizedRecoveryMode, } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct UnauthorizedRecoveryStepResult { + auth_state_changed: Option, +} + +impl UnauthorizedRecoveryStepResult { + pub fn auth_state_changed(&self) -> Option { + self.auth_state_changed + } +} + impl UnauthorizedRecovery { fn new(manager: Arc) -> Self { let cached_auth = manager.auth_cached(); @@ -917,7 +928,46 @@ impl UnauthorizedRecovery { !matches!(self.step, UnauthorizedRecoveryStep::Done) } - pub async fn next(&mut self) -> Result<(), RefreshTokenError> { + pub fn unavailable_reason(&self) -> &'static str { + if !self + .manager + .auth_cached() + .as_ref() + .is_some_and(CodexAuth::is_chatgpt_auth) + { + return "not_chatgpt_auth"; + } + + if self.mode == UnauthorizedRecoveryMode::External + && !self.manager.has_external_auth_refresher() + { + return "no_external_refresher"; + } + + if matches!(self.step, UnauthorizedRecoveryStep::Done) { + return "recovery_exhausted"; + } + + "ready" + } + + pub fn mode_name(&self) -> &'static str { + match self.mode { + UnauthorizedRecoveryMode::Managed => "managed", + UnauthorizedRecoveryMode::External => "external", + } + } + + pub fn step_name(&self) -> &'static str { + match self.step { + UnauthorizedRecoveryStep::Reload => "reload", + UnauthorizedRecoveryStep::RefreshToken => "refresh_token", + UnauthorizedRecoveryStep::ExternalRefresh => "external_refresh", + UnauthorizedRecoveryStep::Done => "done", + } + } + + pub async fn next(&mut self) -> Result { if !self.has_next() { return Err(RefreshTokenError::Permanent(RefreshTokenFailedError::new( RefreshTokenFailedReason::Other, @@ -931,8 +981,17 @@ impl UnauthorizedRecovery { .manager .reload_if_account_id_matches(self.expected_account_id.as_deref()) { - ReloadOutcome::ReloadedChanged | ReloadOutcome::ReloadedNoChange => { + ReloadOutcome::ReloadedChanged => { self.step = UnauthorizedRecoveryStep::RefreshToken; + return Ok(UnauthorizedRecoveryStepResult { + auth_state_changed: Some(true), + }); + } + ReloadOutcome::ReloadedNoChange => { + self.step = UnauthorizedRecoveryStep::RefreshToken; + return Ok(UnauthorizedRecoveryStepResult { + auth_state_changed: Some(false), + }); } ReloadOutcome::Skipped => { self.step = UnauthorizedRecoveryStep::Done; @@ -946,16 +1005,24 @@ impl UnauthorizedRecovery { UnauthorizedRecoveryStep::RefreshToken => { self.manager.refresh_token_from_authority().await?; self.step = UnauthorizedRecoveryStep::Done; + return Ok(UnauthorizedRecoveryStepResult { + auth_state_changed: Some(true), + }); } UnauthorizedRecoveryStep::ExternalRefresh => { self.manager .refresh_external_auth(ExternalAuthRefreshReason::Unauthorized) .await?; self.step = UnauthorizedRecoveryStep::Done; + return Ok(UnauthorizedRecoveryStepResult { + auth_state_changed: Some(true), + }); } UnauthorizedRecoveryStep::Done => {} } - Ok(()) + Ok(UnauthorizedRecoveryStepResult { + auth_state_changed: None, + }) } } diff --git a/codex-rs/core/src/auth_tests.rs b/codex-rs/core/src/auth_tests.rs index 0c4a574f34..3bc5eb6c78 100644 --- a/codex-rs/core/src/auth_tests.rs +++ b/codex-rs/core/src/auth_tests.rs @@ -13,6 +13,7 @@ use codex_protocol::config_types::ForcedLoginMethod; use pretty_assertions::assert_eq; use serde::Serialize; use serde_json::json; +use std::sync::Arc; use tempfile::tempdir; #[tokio::test] @@ -171,6 +172,33 @@ fn logout_removes_auth_file() -> Result<(), std::io::Error> { Ok(()) } +#[test] +fn unauthorized_recovery_reports_mode_and_step_names() { + let dir = tempdir().unwrap(); + let manager = AuthManager::shared( + dir.path().to_path_buf(), + false, + AuthCredentialsStoreMode::File, + ); + let managed = UnauthorizedRecovery { + manager: Arc::clone(&manager), + step: UnauthorizedRecoveryStep::Reload, + expected_account_id: None, + mode: UnauthorizedRecoveryMode::Managed, + }; + assert_eq!(managed.mode_name(), "managed"); + assert_eq!(managed.step_name(), "reload"); + + let external = UnauthorizedRecovery { + manager, + step: UnauthorizedRecoveryStep::ExternalRefresh, + expected_account_id: None, + mode: UnauthorizedRecoveryMode::External, + }; + assert_eq!(external.mode_name(), "external"); + assert_eq!(external.step_name(), "external_refresh"); +} + struct AuthFileParams { openai_api_key: Option, chatgpt_plan_type: Option, diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 32c1653ae8..c438d72741 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -75,6 +75,7 @@ use http::HeaderValue; use http::StatusCode as HttpStatusCode; use reqwest::StatusCode; use std::time::Duration; +use std::time::Instant; use tokio::sync::mpsc; use tokio::sync::oneshot; use tokio::sync::oneshot::error::TryRecvError; @@ -85,6 +86,7 @@ use tracing::trace; use tracing::warn; use crate::AuthManager; +use crate::auth::AuthMode; use crate::auth::CodexAuth; use crate::auth::RefreshTokenError; use crate::client_common::Prompt; @@ -97,7 +99,14 @@ use crate::error::Result; use crate::flags::CODEX_RS_SSE_FIXTURE; use crate::model_provider_info::ModelProviderInfo; use crate::model_provider_info::WireApi; +use crate::response_debug_context::extract_response_debug_context; +use crate::response_debug_context::extract_response_debug_context_from_api_error; +use crate::response_debug_context::telemetry_api_error_message; +use crate::response_debug_context::telemetry_transport_error_message; use crate::tools::spec::create_tools_json_for_responses_api; +use crate::util::FeedbackRequestTags; +use crate::util::emit_feedback_auth_recovery_tags; +use crate::util::emit_feedback_request_tags; pub const OPENAI_BETA_HEADER: &str = "OpenAI-Beta"; pub const X_CODEX_TURN_STATE_HEADER: &str = "x-codex-turn-state"; @@ -105,7 +114,9 @@ pub const X_CODEX_TURN_METADATA_HEADER: &str = "x-codex-turn-metadata"; pub const X_RESPONSESAPI_INCLUDE_TIMING_METRICS_HEADER: &str = "x-responsesapi-include-timing-metrics"; const RESPONSES_WEBSOCKETS_V2_BETA_HEADER_VALUE: &str = "responses_websockets=2026-02-06"; - +const RESPONSES_ENDPOINT: &str = "/responses"; +const RESPONSES_COMPACT_ENDPOINT: &str = "/responses/compact"; +const MEMORIES_SUMMARIZE_ENDPOINT: &str = "/memories/trace_summarize"; pub fn ws_version_from_features(config: &Config) -> bool { config .features @@ -144,6 +155,17 @@ struct CurrentClientSetup { api_auth: CoreAuthProvider, } +#[derive(Clone, Copy)] +struct RequestRouteTelemetry { + endpoint: &'static str, +} + +impl RequestRouteTelemetry { + fn for_endpoint(endpoint: &'static str) -> Self { + Self { endpoint } + } +} + /// A session-scoped client for model-provider API calls. /// /// This holds configuration and state that should be shared across turns within a Codex session @@ -201,6 +223,23 @@ struct WebsocketSession { connection: Option, last_request: Option, last_response_rx: Option>, + connection_reused: StdMutex, +} + +impl WebsocketSession { + fn set_connection_reused(&self, connection_reused: bool) { + *self + .connection_reused + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) = connection_reused; + } + + fn connection_reused(&self) -> bool { + *self + .connection_reused + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } } enum WebsocketStreamOutcome { @@ -291,7 +330,15 @@ impl ModelClient { } let client_setup = self.current_client_setup().await?; let transport = ReqwestTransport::new(build_reqwest_client()); - let request_telemetry = Self::build_request_telemetry(session_telemetry); + let request_telemetry = Self::build_request_telemetry( + session_telemetry, + AuthRequestTelemetryContext::new( + client_setup.auth.as_ref().map(CodexAuth::auth_mode), + &client_setup.api_auth, + PendingUnauthorizedRetry::default(), + ), + RequestRouteTelemetry::for_endpoint(RESPONSES_COMPACT_ENDPOINT), + ); let client = ApiCompactClient::new(transport, client_setup.api_provider, client_setup.api_auth) .with_telemetry(Some(request_telemetry)); @@ -351,7 +398,15 @@ impl ModelClient { let client_setup = self.current_client_setup().await?; let transport = ReqwestTransport::new(build_reqwest_client()); - let request_telemetry = Self::build_request_telemetry(session_telemetry); + let request_telemetry = Self::build_request_telemetry( + session_telemetry, + AuthRequestTelemetryContext::new( + client_setup.auth.as_ref().map(CodexAuth::auth_mode), + &client_setup.api_auth, + PendingUnauthorizedRetry::default(), + ), + RequestRouteTelemetry::for_endpoint(MEMORIES_SUMMARIZE_ENDPOINT), + ); let client = ApiMemoriesClient::new(transport, client_setup.api_provider, client_setup.api_auth) .with_telemetry(Some(request_telemetry)); @@ -391,8 +446,16 @@ impl ModelClient { } /// Builds request telemetry for unary API calls (e.g., Compact endpoint). - fn build_request_telemetry(session_telemetry: &SessionTelemetry) -> Arc { - let telemetry = Arc::new(ApiTelemetry::new(session_telemetry.clone())); + fn build_request_telemetry( + session_telemetry: &SessionTelemetry, + auth_context: AuthRequestTelemetryContext, + request_route_telemetry: RequestRouteTelemetry, + ) -> Arc { + let telemetry = Arc::new(ApiTelemetry::new( + session_telemetry.clone(), + auth_context, + request_route_telemetry, + )); let request_telemetry: Arc = telemetry; request_telemetry } @@ -458,6 +521,7 @@ impl ModelClient { /// /// Both startup prewarm and in-turn `needs_new` reconnects call this path so handshake /// behavior remains consistent across both flows. + #[allow(clippy::too_many_arguments)] async fn connect_websocket( &self, session_telemetry: &SessionTelemetry, @@ -465,17 +529,69 @@ impl ModelClient { api_auth: CoreAuthProvider, turn_state: Option>>, turn_metadata_header: Option<&str>, + auth_context: AuthRequestTelemetryContext, + request_route_telemetry: RequestRouteTelemetry, ) -> std::result::Result { let headers = self.build_websocket_headers(turn_state.as_ref(), turn_metadata_header); - let websocket_telemetry = ModelClientSession::build_websocket_telemetry(session_telemetry); - ApiWebSocketResponsesClient::new(api_provider, api_auth) + let websocket_telemetry = ModelClientSession::build_websocket_telemetry( + session_telemetry, + auth_context, + request_route_telemetry, + ); + let start = Instant::now(); + let result = ApiWebSocketResponsesClient::new(api_provider, api_auth) .connect( headers, crate::default_client::default_headers(), turn_state, Some(websocket_telemetry), ) - .await + .await; + let error_message = result.as_ref().err().map(telemetry_api_error_message); + let response_debug = result + .as_ref() + .err() + .map(extract_response_debug_context_from_api_error) + .unwrap_or_default(); + let status = result.as_ref().err().and_then(api_error_http_status); + session_telemetry.record_websocket_connect( + start.elapsed(), + status, + error_message.as_deref(), + auth_context.auth_header_attached, + auth_context.auth_header_name, + auth_context.retry_after_unauthorized, + auth_context.recovery_mode, + auth_context.recovery_phase, + request_route_telemetry.endpoint, + false, + response_debug.request_id.as_deref(), + response_debug.cf_ray.as_deref(), + response_debug.auth_error.as_deref(), + response_debug.auth_error_code.as_deref(), + ); + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: request_route_telemetry.endpoint, + auth_header_attached: auth_context.auth_header_attached, + auth_header_name: auth_context.auth_header_name, + auth_mode: auth_context.auth_mode, + auth_retry_after_unauthorized: Some(auth_context.retry_after_unauthorized), + auth_recovery_mode: auth_context.recovery_mode, + auth_recovery_phase: auth_context.recovery_phase, + auth_connection_reused: Some(false), + auth_request_id: response_debug.request_id.as_deref(), + auth_cf_ray: response_debug.cf_ray.as_deref(), + auth_error: response_debug.auth_error.as_deref(), + auth_error_code: response_debug.auth_error_code.as_deref(), + auth_recovery_followup_success: auth_context + .retry_after_unauthorized + .then_some(result.is_ok()), + auth_recovery_followup_status: auth_context + .retry_after_unauthorized + .then_some(status) + .flatten(), + }); + result } /// Builds websocket handshake headers for both prewarm and turn-time reconnect. @@ -718,7 +834,11 @@ impl ModelClientSession { "failed to build websocket prewarm client setup: {err}" )) })?; - + let auth_context = AuthRequestTelemetryContext::new( + client_setup.auth.as_ref().map(CodexAuth::auth_mode), + &client_setup.api_auth, + PendingUnauthorizedRetry::default(), + ); let connection = self .client .connect_websocket( @@ -727,9 +847,12 @@ impl ModelClientSession { client_setup.api_auth, Some(Arc::clone(&self.turn_state)), None, + auth_context, + RequestRouteTelemetry::for_endpoint(RESPONSES_ENDPOINT), ) .await?; self.websocket_session.connection = Some(connection); + self.websocket_session.set_connection_reused(false); Ok(()) } /// Returns a websocket connection for this turn. @@ -742,17 +865,22 @@ impl ModelClientSession { wire_api = %self.client.state.provider.wire_api, transport = "responses_websocket", api.path = "responses", - turn.has_metadata_header = turn_metadata_header.is_some() + turn.has_metadata_header = params.turn_metadata_header.is_some() ) )] async fn websocket_connection( &mut self, - session_telemetry: &SessionTelemetry, - api_provider: codex_api::Provider, - api_auth: CoreAuthProvider, - turn_metadata_header: Option<&str>, - options: &ApiResponsesOptions, + params: WebsocketConnectParams<'_>, ) -> std::result::Result<&ApiWebSocketConnection, ApiError> { + let WebsocketConnectParams { + session_telemetry, + api_provider, + api_auth, + turn_metadata_header, + options, + auth_context, + request_route_telemetry, + } = params; let needs_new = match self.websocket_session.connection.as_ref() { Some(conn) => conn.is_closed().await, None => true, @@ -773,9 +901,14 @@ impl ModelClientSession { api_auth, Some(turn_state), turn_metadata_header, + auth_context, + request_route_telemetry, ) .await?; self.websocket_session.connection = Some(new_conn); + self.websocket_session.set_connection_reused(false); + } else { + self.websocket_session.set_connection_reused(true); } self.websocket_session @@ -840,11 +973,20 @@ impl ModelClientSession { let mut auth_recovery = auth_manager .as_ref() .map(super::auth::AuthManager::unauthorized_recovery); + let mut pending_retry = PendingUnauthorizedRetry::default(); loop { let client_setup = self.client.current_client_setup().await?; let transport = ReqwestTransport::new(build_reqwest_client()); - let (request_telemetry, sse_telemetry) = - Self::build_streaming_telemetry(session_telemetry); + let request_auth_context = AuthRequestTelemetryContext::new( + client_setup.auth.as_ref().map(CodexAuth::auth_mode), + &client_setup.api_auth, + pending_retry, + ); + let (request_telemetry, sse_telemetry) = Self::build_streaming_telemetry( + session_telemetry, + request_auth_context, + RequestRouteTelemetry::for_endpoint(RESPONSES_ENDPOINT), + ); let compression = self.responses_request_compression(client_setup.auth.as_ref()); let options = self.build_responses_options(turn_metadata_header, compression); @@ -872,7 +1014,14 @@ impl ModelClientSession { Err(ApiError::Transport( unauthorized_transport @ TransportError::Http { status, .. }, )) if status == StatusCode::UNAUTHORIZED => { - handle_unauthorized(unauthorized_transport, &mut auth_recovery).await?; + pending_retry = PendingUnauthorizedRetry::from_recovery( + handle_unauthorized( + unauthorized_transport, + &mut auth_recovery, + session_telemetry, + ) + .await?, + ); continue; } Err(err) => return Err(map_api_error(err)), @@ -911,8 +1060,14 @@ impl ModelClientSession { let mut auth_recovery = auth_manager .as_ref() .map(super::auth::AuthManager::unauthorized_recovery); + let mut pending_retry = PendingUnauthorizedRetry::default(); loop { let client_setup = self.client.current_client_setup().await?; + let request_auth_context = AuthRequestTelemetryContext::new( + client_setup.auth.as_ref().map(CodexAuth::auth_mode), + &client_setup.api_auth, + pending_retry, + ); let compression = self.responses_request_compression(client_setup.auth.as_ref()); let options = self.build_responses_options(turn_metadata_header, compression); @@ -933,13 +1088,17 @@ impl ModelClientSession { } match self - .websocket_connection( + .websocket_connection(WebsocketConnectParams { session_telemetry, - client_setup.api_provider, - client_setup.api_auth, + api_provider: client_setup.api_provider, + api_auth: client_setup.api_auth, turn_metadata_header, - &options, - ) + options: &options, + auth_context: request_auth_context, + request_route_telemetry: RequestRouteTelemetry::for_endpoint( + RESPONSES_ENDPOINT, + ), + }) .await { Ok(_) => {} @@ -951,7 +1110,14 @@ impl ModelClientSession { Err(ApiError::Transport( unauthorized_transport @ TransportError::Http { status, .. }, )) if status == StatusCode::UNAUTHORIZED => { - handle_unauthorized(unauthorized_transport, &mut auth_recovery).await?; + pending_retry = PendingUnauthorizedRetry::from_recovery( + handle_unauthorized( + unauthorized_transport, + &mut auth_recovery, + session_telemetry, + ) + .await?, + ); continue; } Err(err) => return Err(map_api_error(err)), @@ -968,7 +1134,7 @@ impl ModelClientSession { "websocket connection is unavailable".to_string(), )) })? - .stream_request(ws_request) + .stream_request(ws_request, self.websocket_session.connection_reused()) .await .map_err(map_api_error)?; let (stream, last_request_rx) = @@ -981,8 +1147,14 @@ impl ModelClientSession { /// Builds request and SSE telemetry for streaming API calls. fn build_streaming_telemetry( session_telemetry: &SessionTelemetry, + auth_context: AuthRequestTelemetryContext, + request_route_telemetry: RequestRouteTelemetry, ) -> (Arc, Arc) { - let telemetry = Arc::new(ApiTelemetry::new(session_telemetry.clone())); + let telemetry = Arc::new(ApiTelemetry::new( + session_telemetry.clone(), + auth_context, + request_route_telemetry, + )); let request_telemetry: Arc = telemetry.clone(); let sse_telemetry: Arc = telemetry; (request_telemetry, sse_telemetry) @@ -991,8 +1163,14 @@ impl ModelClientSession { /// Builds telemetry for the Responses API WebSocket transport. fn build_websocket_telemetry( session_telemetry: &SessionTelemetry, + auth_context: AuthRequestTelemetryContext, + request_route_telemetry: RequestRouteTelemetry, ) -> Arc { - let telemetry = Arc::new(ApiTelemetry::new(session_telemetry.clone())); + let telemetry = Arc::new(ApiTelemetry::new( + session_telemetry.clone(), + auth_context, + request_route_telemetry, + )); let websocket_telemetry: Arc = telemetry; websocket_telemetry } @@ -1126,6 +1304,7 @@ impl ModelClientSession { self.websocket_session.connection = None; self.websocket_session.last_request = None; self.websocket_session.last_response_rx = None; + self.websocket_session.set_connection_reused(false); } activated } @@ -1264,30 +1443,209 @@ where /// /// When refresh succeeds, the caller should retry the API call; otherwise /// the mapped `CodexErr` is returned to the caller. +#[derive(Clone, Copy, Debug)] +struct UnauthorizedRecoveryExecution { + mode: &'static str, + phase: &'static str, +} + +#[derive(Clone, Copy, Debug, Default)] +struct PendingUnauthorizedRetry { + retry_after_unauthorized: bool, + recovery_mode: Option<&'static str>, + recovery_phase: Option<&'static str>, +} + +impl PendingUnauthorizedRetry { + fn from_recovery(recovery: UnauthorizedRecoveryExecution) -> Self { + Self { + retry_after_unauthorized: true, + recovery_mode: Some(recovery.mode), + recovery_phase: Some(recovery.phase), + } + } +} + +#[derive(Clone, Copy, Debug, Default)] +struct AuthRequestTelemetryContext { + auth_mode: Option<&'static str>, + auth_header_attached: bool, + auth_header_name: Option<&'static str>, + retry_after_unauthorized: bool, + recovery_mode: Option<&'static str>, + recovery_phase: Option<&'static str>, +} + +impl AuthRequestTelemetryContext { + fn new( + auth_mode: Option, + api_auth: &CoreAuthProvider, + retry: PendingUnauthorizedRetry, + ) -> Self { + Self { + auth_mode: auth_mode.map(|mode| match mode { + AuthMode::ApiKey => "ApiKey", + AuthMode::Chatgpt => "Chatgpt", + }), + auth_header_attached: api_auth.auth_header_attached(), + auth_header_name: api_auth.auth_header_name(), + retry_after_unauthorized: retry.retry_after_unauthorized, + recovery_mode: retry.recovery_mode, + recovery_phase: retry.recovery_phase, + } + } +} + +struct WebsocketConnectParams<'a> { + session_telemetry: &'a SessionTelemetry, + api_provider: codex_api::Provider, + api_auth: CoreAuthProvider, + turn_metadata_header: Option<&'a str>, + options: &'a ApiResponsesOptions, + auth_context: AuthRequestTelemetryContext, + request_route_telemetry: RequestRouteTelemetry, +} + async fn handle_unauthorized( transport: TransportError, auth_recovery: &mut Option, -) -> Result<()> { + session_telemetry: &SessionTelemetry, +) -> Result { + let debug = extract_response_debug_context(&transport); if let Some(recovery) = auth_recovery && recovery.has_next() { + let mode = recovery.mode_name(); + let phase = recovery.step_name(); return match recovery.next().await { - Ok(_) => Ok(()), - Err(RefreshTokenError::Permanent(failed)) => Err(CodexErr::RefreshTokenFailed(failed)), - Err(RefreshTokenError::Transient(other)) => Err(CodexErr::Io(other)), + Ok(step_result) => { + session_telemetry.record_auth_recovery( + mode, + phase, + "recovery_succeeded", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + None, + step_result.auth_state_changed(), + ); + emit_feedback_auth_recovery_tags( + mode, + phase, + "recovery_succeeded", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + ); + Ok(UnauthorizedRecoveryExecution { mode, phase }) + } + Err(RefreshTokenError::Permanent(failed)) => { + session_telemetry.record_auth_recovery( + mode, + phase, + "recovery_failed_permanent", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + None, + None, + ); + emit_feedback_auth_recovery_tags( + mode, + phase, + "recovery_failed_permanent", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + ); + Err(CodexErr::RefreshTokenFailed(failed)) + } + Err(RefreshTokenError::Transient(other)) => { + session_telemetry.record_auth_recovery( + mode, + phase, + "recovery_failed_transient", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + None, + None, + ); + emit_feedback_auth_recovery_tags( + mode, + phase, + "recovery_failed_transient", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + ); + Err(CodexErr::Io(other)) + } }; } + let (mode, phase, recovery_reason) = match auth_recovery.as_ref() { + Some(recovery) => ( + recovery.mode_name(), + recovery.step_name(), + Some(recovery.unavailable_reason()), + ), + None => ("none", "none", Some("auth_manager_missing")), + }; + session_telemetry.record_auth_recovery( + mode, + phase, + "recovery_not_run", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + recovery_reason, + None, + ); + emit_feedback_auth_recovery_tags( + mode, + phase, + "recovery_not_run", + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), + ); + Err(map_api_error(ApiError::Transport(transport))) } +fn api_error_http_status(error: &ApiError) -> Option { + match error { + ApiError::Transport(TransportError::Http { status, .. }) => Some(status.as_u16()), + _ => None, + } +} + struct ApiTelemetry { session_telemetry: SessionTelemetry, + auth_context: AuthRequestTelemetryContext, + request_route_telemetry: RequestRouteTelemetry, } impl ApiTelemetry { - fn new(session_telemetry: SessionTelemetry) -> Self { - Self { session_telemetry } + fn new( + session_telemetry: SessionTelemetry, + auth_context: AuthRequestTelemetryContext, + request_route_telemetry: RequestRouteTelemetry, + ) -> Self { + Self { + session_telemetry, + auth_context, + request_route_telemetry, + } } } @@ -1299,13 +1657,50 @@ impl RequestTelemetry for ApiTelemetry { error: Option<&TransportError>, duration: Duration, ) { - let error_message = error.map(std::string::ToString::to_string); + let error_message = error.map(telemetry_transport_error_message); + let status = status.map(|s| s.as_u16()); + let debug = error + .map(extract_response_debug_context) + .unwrap_or_default(); self.session_telemetry.record_api_request( attempt, - status.map(|s| s.as_u16()), + status, error_message.as_deref(), duration, + self.auth_context.auth_header_attached, + self.auth_context.auth_header_name, + self.auth_context.retry_after_unauthorized, + self.auth_context.recovery_mode, + self.auth_context.recovery_phase, + self.request_route_telemetry.endpoint, + debug.request_id.as_deref(), + debug.cf_ray.as_deref(), + debug.auth_error.as_deref(), + debug.auth_error_code.as_deref(), ); + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: self.request_route_telemetry.endpoint, + auth_header_attached: self.auth_context.auth_header_attached, + auth_header_name: self.auth_context.auth_header_name, + auth_mode: self.auth_context.auth_mode, + auth_retry_after_unauthorized: Some(self.auth_context.retry_after_unauthorized), + auth_recovery_mode: self.auth_context.recovery_mode, + auth_recovery_phase: self.auth_context.recovery_phase, + auth_connection_reused: None, + auth_request_id: debug.request_id.as_deref(), + auth_cf_ray: debug.cf_ray.as_deref(), + auth_error: debug.auth_error.as_deref(), + auth_error_code: debug.auth_error_code.as_deref(), + auth_recovery_followup_success: self + .auth_context + .retry_after_unauthorized + .then_some(error.is_none()), + auth_recovery_followup_status: self + .auth_context + .retry_after_unauthorized + .then_some(status) + .flatten(), + }); } } @@ -1323,10 +1718,40 @@ impl SseTelemetry for ApiTelemetry { } impl WebsocketTelemetry for ApiTelemetry { - fn on_ws_request(&self, duration: Duration, error: Option<&ApiError>) { - let error_message = error.map(std::string::ToString::to_string); - self.session_telemetry - .record_websocket_request(duration, error_message.as_deref()); + fn on_ws_request(&self, duration: Duration, error: Option<&ApiError>, connection_reused: bool) { + let error_message = error.map(telemetry_api_error_message); + let status = error.and_then(api_error_http_status); + let debug = error + .map(extract_response_debug_context_from_api_error) + .unwrap_or_default(); + self.session_telemetry.record_websocket_request( + duration, + error_message.as_deref(), + connection_reused, + ); + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: self.request_route_telemetry.endpoint, + auth_header_attached: self.auth_context.auth_header_attached, + auth_header_name: self.auth_context.auth_header_name, + auth_mode: self.auth_context.auth_mode, + auth_retry_after_unauthorized: Some(self.auth_context.retry_after_unauthorized), + auth_recovery_mode: self.auth_context.recovery_mode, + auth_recovery_phase: self.auth_context.recovery_phase, + auth_connection_reused: Some(connection_reused), + auth_request_id: debug.request_id.as_deref(), + auth_cf_ray: debug.cf_ray.as_deref(), + auth_error: debug.auth_error.as_deref(), + auth_error_code: debug.auth_error_code.as_deref(), + auth_recovery_followup_success: self + .auth_context + .retry_after_unauthorized + .then_some(error.is_none()), + auth_recovery_followup_status: self + .auth_context + .retry_after_unauthorized + .then_some(status) + .flatten(), + }); } fn on_ws_event( diff --git a/codex-rs/core/src/client_tests.rs b/codex-rs/core/src/client_tests.rs index 138b61ffbb..441a348645 100644 --- a/codex-rs/core/src/client_tests.rs +++ b/codex-rs/core/src/client_tests.rs @@ -1,4 +1,7 @@ +use super::AuthRequestTelemetryContext; use super::ModelClient; +use super::PendingUnauthorizedRetry; +use super::UnauthorizedRecoveryExecution; use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; use codex_protocol::openai_models::ModelInfo; @@ -94,3 +97,22 @@ async fn summarize_memories_returns_empty_for_empty_input() { .expect("empty summarize request should succeed"); assert_eq!(output.len(), 0); } + +#[test] +fn auth_request_telemetry_context_tracks_attached_auth_and_retry_phase() { + let auth_context = AuthRequestTelemetryContext::new( + Some(crate::auth::AuthMode::Chatgpt), + &crate::api_bridge::CoreAuthProvider::for_test(Some("access-token"), Some("workspace-123")), + PendingUnauthorizedRetry::from_recovery(UnauthorizedRecoveryExecution { + mode: "managed", + phase: "refresh_token", + }), + ); + + assert_eq!(auth_context.auth_mode, Some("Chatgpt")); + assert!(auth_context.auth_header_attached); + assert_eq!(auth_context.auth_header_name, Some("authorization")); + assert!(auth_context.retry_after_unauthorized); + assert_eq!(auth_context.recovery_mode, Some("managed")); + assert_eq!(auth_context.recovery_phase, Some("refresh_token")); +} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 4aaa1df28c..490f025537 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -3936,12 +3936,13 @@ impl Session { server: &str, tool: &str, arguments: Option, + meta: Option, ) -> anyhow::Result { self.services .mcp_connection_manager .read() .await - .call_tool(server, tool, arguments) + .call_tool(server, tool, arguments, meta) .await } diff --git a/codex-rs/core/src/error.rs b/codex-rs/core/src/error.rs index f3bb4dc8e5..e8e86defc2 100644 --- a/codex-rs/core/src/error.rs +++ b/codex-rs/core/src/error.rs @@ -292,6 +292,8 @@ pub struct UnexpectedResponseError { pub url: Option, pub cf_ray: Option, pub request_id: Option, + pub identity_authorization_error: Option, + pub identity_error_code: Option, } const CLOUDFLARE_BLOCKED_MESSAGE: &str = @@ -346,6 +348,12 @@ impl UnexpectedResponseError { if let Some(id) = &self.request_id { message.push_str(&format!(", request id: {id}")); } + if let Some(auth_error) = &self.identity_authorization_error { + message.push_str(&format!(", auth error: {auth_error}")); + } + if let Some(error_code) = &self.identity_error_code { + message.push_str(&format!(", auth error code: {error_code}")); + } Some(message) } @@ -368,6 +376,12 @@ impl std::fmt::Display for UnexpectedResponseError { if let Some(id) = &self.request_id { message.push_str(&format!(", request id: {id}")); } + if let Some(auth_error) = &self.identity_authorization_error { + message.push_str(&format!(", auth error: {auth_error}")); + } + if let Some(error_code) = &self.identity_error_code { + message.push_str(&format!(", auth error code: {error_code}")); + } write!(f, "{message}") } } diff --git a/codex-rs/core/src/error_tests.rs b/codex-rs/core/src/error_tests.rs index fa2bd4a6e1..51cbf42dde 100644 --- a/codex-rs/core/src/error_tests.rs +++ b/codex-rs/core/src/error_tests.rs @@ -328,6 +328,8 @@ fn unexpected_status_cloudflare_html_is_simplified() { url: Some("http://example.com/blocked".to_string()), cf_ray: Some("ray-id".to_string()), request_id: None, + identity_authorization_error: None, + identity_error_code: None, }; let status = StatusCode::FORBIDDEN.to_string(); let url = "http://example.com/blocked"; @@ -345,6 +347,8 @@ fn unexpected_status_non_html_is_unchanged() { url: Some("http://example.com/plain".to_string()), cf_ray: None, request_id: None, + identity_authorization_error: None, + identity_error_code: None, }; let status = StatusCode::FORBIDDEN.to_string(); let url = "http://example.com/plain"; @@ -363,6 +367,8 @@ fn unexpected_status_prefers_error_message_when_present() { url: Some("https://chatgpt.com/backend-api/codex/responses".to_string()), cf_ray: None, request_id: Some("req-123".to_string()), + identity_authorization_error: None, + identity_error_code: None, }; let status = StatusCode::UNAUTHORIZED.to_string(); assert_eq!( @@ -382,6 +388,8 @@ fn unexpected_status_truncates_long_body_with_ellipsis() { url: Some("http://example.com/long".to_string()), cf_ray: None, request_id: Some("req-long".to_string()), + identity_authorization_error: None, + identity_error_code: None, }; let status = StatusCode::BAD_GATEWAY.to_string(); let expected_body = format!("{}...", "x".repeat(UNEXPECTED_RESPONSE_BODY_MAX_BYTES)); @@ -401,6 +409,8 @@ fn unexpected_status_includes_cf_ray_and_request_id() { url: Some("https://chatgpt.com/backend-api/codex/responses".to_string()), cf_ray: Some("9c81f9f18f2fa49d-LHR".to_string()), request_id: Some("req-xyz".to_string()), + identity_authorization_error: None, + identity_error_code: None, }; let status = StatusCode::UNAUTHORIZED.to_string(); assert_eq!( @@ -411,6 +421,26 @@ fn unexpected_status_includes_cf_ray_and_request_id() { ); } +#[test] +fn unexpected_status_includes_identity_auth_details() { + let err = UnexpectedResponseError { + status: StatusCode::UNAUTHORIZED, + body: "plain text error".to_string(), + url: Some("https://chatgpt.com/backend-api/codex/models".to_string()), + cf_ray: Some("cf-ray-auth-401-test".to_string()), + request_id: Some("req-auth".to_string()), + identity_authorization_error: Some("missing_authorization_header".to_string()), + identity_error_code: Some("token_expired".to_string()), + }; + let status = StatusCode::UNAUTHORIZED.to_string(); + assert_eq!( + err.to_string(), + format!( + "unexpected status {status}: plain text error, url: https://chatgpt.com/backend-api/codex/models, cf-ray: cf-ray-auth-401-test, request id: req-auth, auth error: missing_authorization_header, auth error code: token_expired" + ) + ); +} + #[test] fn usage_limit_reached_includes_hours_and_minutes() { let base = Utc.with_ymd_and_hms(2024, 1, 1, 0, 0, 0).unwrap(); diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index c0e379593c..7833c19686 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -154,8 +154,6 @@ pub enum Feature { Plugins, /// Allow the model to invoke the built-in image generation tool. ImageGeneration, - /// Route apps MCP calls through the configured gateway. - AppsMcpGateway, /// Allow prompting and installing missing MCP dependencies. SkillMcpDependencyInstall, /// Prompt for missing skill env var dependencies. @@ -753,12 +751,6 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::UnderDevelopment, default_enabled: false, }, - FeatureSpec { - id: Feature::AppsMcpGateway, - key: "apps_mcp_gateway", - stage: Stage::UnderDevelopment, - default_enabled: false, - }, FeatureSpec { id: Feature::SkillMcpDependencyInstall, key: "skill_mcp_dependency_install", diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 98a4450ad4..fb432c426b 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -87,6 +87,7 @@ pub use model_provider_info::WireApi; pub use model_provider_info::built_in_model_providers; pub use model_provider_info::create_oss_provider_with_base_url; mod event_mapping; +mod response_debug_context; pub mod review_format; pub mod review_prompts; mod seatbelt_permissions; diff --git a/codex-rs/core/src/mcp/mod.rs b/codex-rs/core/src/mcp/mod.rs index 3140f5bcff..184f76e40f 100644 --- a/codex-rs/core/src/mcp/mod.rs +++ b/codex-rs/core/src/mcp/mod.rs @@ -21,7 +21,6 @@ use crate::CodexAuth; use crate::config::Config; use crate::config::types::McpServerConfig; use crate::config::types::McpServerTransportConfig; -use crate::features::Feature; use crate::mcp::auth::compute_auth_statuses; use crate::mcp_connection_manager::McpConnectionManager; use crate::mcp_connection_manager::SandboxState; @@ -33,8 +32,6 @@ const MCP_TOOL_NAME_PREFIX: &str = "mcp"; const MCP_TOOL_NAME_DELIMITER: &str = "__"; pub(crate) const CODEX_APPS_MCP_SERVER_NAME: &str = "codex_apps"; const CODEX_CONNECTORS_TOKEN_ENV_VAR: &str = "CODEX_CONNECTORS_TOKEN"; -const OPENAI_CONNECTORS_MCP_BASE_URL: &str = "https://api.openai.com"; -const OPENAI_CONNECTORS_MCP_PATH: &str = "/v1/connectors/gateways/flat/mcp"; #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct ToolPluginProvenance { @@ -94,13 +91,6 @@ impl ToolPluginProvenance { } } -// Legacy vs new MCP gateway -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum CodexAppsMcpGateway { - LegacyMCPGateway, - MCPGateway, -} - fn codex_apps_mcp_bearer_token_env_var() -> Option { match env::var(CODEX_CONNECTORS_TOKEN_ENV_VAR) { Ok(value) if !value.trim().is_empty() => Some(CODEX_CONNECTORS_TOKEN_ENV_VAR.to_string()), @@ -135,14 +125,6 @@ fn codex_apps_mcp_http_headers(auth: Option<&CodexAuth>) -> Option CodexAppsMcpGateway { - if config.features.enabled(Feature::AppsMcpGateway) { - CodexAppsMcpGateway::MCPGateway - } else { - CodexAppsMcpGateway::LegacyMCPGateway - } -} - fn normalize_codex_apps_base_url(base_url: &str) -> String { let mut base_url = base_url.trim_end_matches('/').to_string(); if (base_url.starts_with("https://chatgpt.com") @@ -154,11 +136,7 @@ fn normalize_codex_apps_base_url(base_url: &str) -> String { base_url } -fn codex_apps_mcp_url_for_gateway(base_url: &str, gateway: CodexAppsMcpGateway) -> String { - if gateway == CodexAppsMcpGateway::MCPGateway { - return format!("{OPENAI_CONNECTORS_MCP_BASE_URL}{OPENAI_CONNECTORS_MCP_PATH}"); - } - +fn codex_apps_mcp_url_for_base_url(base_url: &str) -> String { let base_url = normalize_codex_apps_base_url(base_url); if base_url.contains("/backend-api") { format!("{base_url}/wham/apps") @@ -170,10 +148,7 @@ fn codex_apps_mcp_url_for_gateway(base_url: &str, gateway: CodexAppsMcpGateway) } pub(crate) fn codex_apps_mcp_url(config: &Config) -> String { - codex_apps_mcp_url_for_gateway( - &config.chatgpt_base_url, - selected_config_codex_apps_mcp_gateway(config), - ) + codex_apps_mcp_url_for_base_url(&config.chatgpt_base_url) } fn codex_apps_mcp_server_config(config: &Config, auth: Option<&CodexAuth>) -> McpServerConfig { diff --git a/codex-rs/core/src/mcp/mod_tests.rs b/codex-rs/core/src/mcp/mod_tests.rs index cdbcda2ea0..706f8ceb09 100644 --- a/codex-rs/core/src/mcp/mod_tests.rs +++ b/codex-rs/core/src/mcp/mod_tests.rs @@ -1,6 +1,7 @@ use super::*; use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigBuilder; +use crate::features::Feature; use crate::plugins::AppConnectorId; use crate::plugins::PluginCapabilitySummary; use pretty_assertions::assert_eq; @@ -123,67 +124,27 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { } #[test] -fn codex_apps_mcp_url_for_default_gateway_keeps_existing_paths() { +fn codex_apps_mcp_url_for_base_url_keeps_existing_paths() { assert_eq!( - codex_apps_mcp_url_for_gateway( - "https://chatgpt.com/backend-api", - CodexAppsMcpGateway::LegacyMCPGateway - ), + codex_apps_mcp_url_for_base_url("https://chatgpt.com/backend-api"), "https://chatgpt.com/backend-api/wham/apps" ); assert_eq!( - codex_apps_mcp_url_for_gateway( - "https://chat.openai.com", - CodexAppsMcpGateway::LegacyMCPGateway - ), + codex_apps_mcp_url_for_base_url("https://chat.openai.com"), "https://chat.openai.com/backend-api/wham/apps" ); assert_eq!( - codex_apps_mcp_url_for_gateway( - "http://localhost:8080/api/codex", - CodexAppsMcpGateway::LegacyMCPGateway - ), + codex_apps_mcp_url_for_base_url("http://localhost:8080/api/codex"), "http://localhost:8080/api/codex/apps" ); assert_eq!( - codex_apps_mcp_url_for_gateway( - "http://localhost:8080", - CodexAppsMcpGateway::LegacyMCPGateway - ), + codex_apps_mcp_url_for_base_url("http://localhost:8080"), "http://localhost:8080/api/codex/apps" ); } #[test] -fn codex_apps_mcp_url_for_gateway_uses_openai_connectors_gateway() { - let expected_url = format!("{OPENAI_CONNECTORS_MCP_BASE_URL}{OPENAI_CONNECTORS_MCP_PATH}"); - - assert_eq!( - codex_apps_mcp_url_for_gateway( - "https://chatgpt.com/backend-api", - CodexAppsMcpGateway::MCPGateway - ), - expected_url.as_str() - ); - assert_eq!( - codex_apps_mcp_url_for_gateway("https://chat.openai.com", CodexAppsMcpGateway::MCPGateway), - expected_url.as_str() - ); - assert_eq!( - codex_apps_mcp_url_for_gateway( - "http://localhost:8080/api/codex", - CodexAppsMcpGateway::MCPGateway - ), - expected_url.as_str() - ); - assert_eq!( - codex_apps_mcp_url_for_gateway("http://localhost:8080", CodexAppsMcpGateway::MCPGateway), - expected_url.as_str() - ); -} - -#[test] -fn codex_apps_mcp_url_uses_default_gateway_when_feature_is_disabled() { +fn codex_apps_mcp_url_uses_legacy_codex_apps_path() { let mut config = crate::config::test_config(); config.chatgpt_base_url = "https://chatgpt.com".to_string(); @@ -194,22 +155,7 @@ fn codex_apps_mcp_url_uses_default_gateway_when_feature_is_disabled() { } #[test] -fn codex_apps_mcp_url_uses_openai_connectors_gateway_when_feature_is_enabled() { - let mut config = crate::config::test_config(); - config.chatgpt_base_url = "https://chatgpt.com".to_string(); - config - .features - .enable(Feature::AppsMcpGateway) - .expect("test config should allow apps gateway"); - - assert_eq!( - codex_apps_mcp_url(&config), - format!("{OPENAI_CONNECTORS_MCP_BASE_URL}{OPENAI_CONNECTORS_MCP_PATH}") - ); -} - -#[test] -fn codex_apps_server_config_switches_gateway_with_flags() { +fn codex_apps_server_config_uses_legacy_codex_apps_path() { let mut config = crate::config::test_config(); config.chatgpt_base_url = "https://chatgpt.com".to_string(); @@ -231,22 +177,6 @@ fn codex_apps_server_config_switches_gateway_with_flags() { }; assert_eq!(url, "https://chatgpt.com/backend-api/wham/apps"); - - config - .features - .enable(Feature::AppsMcpGateway) - .expect("test config should allow apps gateway"); - servers = with_codex_apps_mcp(servers, true, None, &config); - let server = servers - .get(CODEX_APPS_MCP_SERVER_NAME) - .expect("codex apps should remain present when apps stays enabled"); - let url = match &server.transport { - McpServerTransportConfig::StreamableHttp { url, .. } => url, - _ => panic!("expected streamable http transport for codex apps"), - }; - - let expected_url = format!("{OPENAI_CONNECTORS_MCP_BASE_URL}{OPENAI_CONNECTORS_MCP_PATH}"); - assert_eq!(url, &expected_url); } #[tokio::test] diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 6e2174c4d3..74422d0740 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -1014,6 +1014,7 @@ impl McpConnectionManager { server: &str, tool: &str, arguments: Option, + meta: Option, ) -> Result { let client = self.client_by_name(server).await?; if !client.tool_filter.allows(tool) { @@ -1024,7 +1025,7 @@ impl McpConnectionManager { let result: rmcp::model::CallToolResult = client .client - .call_tool(tool.to_string(), arguments, client.tool_timeout) + .call_tool(tool.to_string(), arguments, meta, client.tool_timeout) .await .with_context(|| format!("tool call failed for `{server}/{tool}`"))?; diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 19982349be..5f6a7a7506 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -117,6 +117,7 @@ pub(crate) async fn handle_mcp_tool_call( .counter("codex.mcp.call", 1, &[("status", status)]); return CallToolResult::from_result(result); } + let request_meta = build_mcp_tool_call_request_meta(&server, metadata.as_ref()); let tool_call_begin_event = EventMsg::McpToolCallBegin(McpToolCallBeginEvent { call_id: call_id.clone(), @@ -142,7 +143,12 @@ pub(crate) async fn handle_mcp_tool_call( let start = Instant::now(); let result = sess - .call_tool(&server, &tool_name, arguments_value.clone()) + .call_tool( + &server, + &tool_name, + arguments_value.clone(), + request_meta.clone(), + ) .await .map_err(|e| format!("tool call error: {e:?}")); let result = sanitize_mcp_tool_result_for_model( @@ -226,7 +232,7 @@ pub(crate) async fn handle_mcp_tool_call( let start = Instant::now(); // Perform the tool call. let result = sess - .call_tool(&server, &tool_name, arguments_value.clone()) + .call_tool(&server, &tool_name, arguments_value.clone(), request_meta) .await .map_err(|e| format!("tool call error: {e:?}")); let result = sanitize_mcp_tool_result_for_model( @@ -374,6 +380,24 @@ pub(crate) struct McpToolApprovalMetadata { connector_description: Option, tool_title: Option, tool_description: Option, + codex_apps_meta: Option>, +} + +const MCP_TOOL_CODEX_APPS_META_KEY: &str = "_codex_apps"; + +fn build_mcp_tool_call_request_meta( + server: &str, + metadata: Option<&McpToolApprovalMetadata>, +) -> Option { + if server != CODEX_APPS_MCP_SERVER_NAME { + return None; + } + + let codex_apps_meta = metadata.and_then(|metadata| metadata.codex_apps_meta.as_ref())?; + + Some(serde_json::json!({ + MCP_TOOL_CODEX_APPS_META_KEY: codex_apps_meta, + })) } #[derive(Clone, Copy)] @@ -750,6 +774,13 @@ pub(crate) async fn lookup_mcp_tool_metadata( connector_description, tool_title: tool_info.tool.title, tool_description: tool_info.tool.description.map(std::borrow::Cow::into_owned), + codex_apps_meta: tool_info + .tool + .meta + .as_ref() + .and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY)) + .and_then(serde_json::Value::as_object) + .cloned(), }) } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 62292b43f3..55f47f674f 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -47,6 +47,7 @@ fn approval_metadata( connector_description: connector_description.map(str::to_string), tool_title: tool_title.map(str::to_string), tool_description: tool_description.map(str::to_string), + codex_apps_meta: None, } } @@ -415,6 +416,39 @@ fn sanitize_mcp_tool_result_for_model_preserves_image_when_supported() { assert_eq!(got, original); } +#[test] +fn codex_apps_tool_call_request_meta_includes_codex_apps_meta() { + let metadata = McpToolApprovalMetadata { + annotations: None, + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + connector_description: Some("Manage events".to_string()), + tool_title: Some("Create Event".to_string()), + tool_description: Some("Create a calendar event.".to_string()), + codex_apps_meta: Some( + serde_json::json!({ + "resource_uri": "connector://calendar/tools/calendar_create_event", + "contains_mcp_source": true, + "connector_id": "calendar", + }) + .as_object() + .cloned() + .expect("_codex_apps metadata should be an object"), + ), + }; + + assert_eq!( + build_mcp_tool_call_request_meta(CODEX_APPS_MCP_SERVER_NAME, Some(&metadata)), + Some(serde_json::json!({ + MCP_TOOL_CODEX_APPS_META_KEY: { + "resource_uri": "connector://calendar/tools/calendar_create_event", + "contains_mcp_source": true, + "connector_id": "calendar", + }, + })) + ); +} + #[test] fn accepted_elicitation_content_converts_to_request_user_input_response() { let response = request_user_input_response_from_elicitation_content(Some(serde_json::json!( @@ -535,6 +569,7 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { connector_description: None, tool_title: None, tool_description: None, + codex_apps_meta: None, }; let request = build_guardian_mcp_tool_review_request("call-1", &invocation, Some(&metadata)); @@ -856,6 +891,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { connector_description: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, + codex_apps_meta: None, }; let decision = maybe_request_mcp_tool_approval( @@ -919,6 +955,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), + codex_apps_meta: None, }; let decision = maybe_request_mcp_tool_approval( @@ -1021,6 +1058,7 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_ connector_description: Some("Manage events".to_string()), tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), + codex_apps_meta: None, }; let decision = maybe_request_mcp_tool_approval( diff --git a/codex-rs/core/src/models_manager/manager.rs b/codex-rs/core/src/models_manager/manager.rs index 9498fff761..411fa83a7d 100644 --- a/codex-rs/core/src/models_manager/manager.rs +++ b/codex-rs/core/src/models_manager/manager.rs @@ -3,6 +3,7 @@ use crate::api_bridge::auth_provider_from_auth; use crate::api_bridge::map_api_error; use crate::auth::AuthManager; use crate::auth::AuthMode; +use crate::auth::CodexAuth; use crate::config::Config; use crate::default_client::build_reqwest_client; use crate::error::CodexErr; @@ -11,8 +12,15 @@ use crate::model_provider_info::ModelProviderInfo; use crate::models_manager::collaboration_mode_presets::CollaborationModesConfig; use crate::models_manager::collaboration_mode_presets::builtin_collaboration_mode_presets; use crate::models_manager::model_info; +use crate::response_debug_context::extract_response_debug_context; +use crate::response_debug_context::telemetry_transport_error_message; +use crate::util::FeedbackRequestTags; +use crate::util::emit_feedback_request_tags; use codex_api::ModelsClient; +use codex_api::RequestTelemetry; use codex_api::ReqwestTransport; +use codex_api::TransportError; +use codex_otel::TelemetryAuthMode; use codex_protocol::config_types::CollaborationModeMask; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; @@ -32,6 +40,82 @@ use tracing::instrument; const MODEL_CACHE_FILE: &str = "models_cache.json"; const DEFAULT_MODEL_CACHE_TTL: Duration = Duration::from_secs(300); const MODELS_REFRESH_TIMEOUT: Duration = Duration::from_secs(5); +const MODELS_ENDPOINT: &str = "/models"; +#[derive(Clone)] +struct ModelsRequestTelemetry { + auth_mode: Option, + auth_header_attached: bool, + auth_header_name: Option<&'static str>, +} + +impl RequestTelemetry for ModelsRequestTelemetry { + fn on_request( + &self, + attempt: u64, + status: Option, + error: Option<&TransportError>, + duration: Duration, + ) { + let success = status.is_some_and(|code| code.is_success()) && error.is_none(); + let error_message = error.map(telemetry_transport_error_message); + let response_debug = error + .map(extract_response_debug_context) + .unwrap_or_default(); + let status = status.map(|status| status.as_u16()); + tracing::event!( + target: "codex_otel.log_only", + tracing::Level::INFO, + event.name = "codex.api_request", + duration_ms = %duration.as_millis(), + http.response.status_code = status, + success = success, + error.message = error_message.as_deref(), + attempt = attempt, + endpoint = MODELS_ENDPOINT, + auth.header_attached = self.auth_header_attached, + auth.header_name = self.auth_header_name, + auth.request_id = response_debug.request_id.as_deref(), + auth.cf_ray = response_debug.cf_ray.as_deref(), + auth.error = response_debug.auth_error.as_deref(), + auth.error_code = response_debug.auth_error_code.as_deref(), + auth.mode = self.auth_mode.as_deref(), + ); + tracing::event!( + target: "codex_otel.trace_safe", + tracing::Level::INFO, + event.name = "codex.api_request", + duration_ms = %duration.as_millis(), + http.response.status_code = status, + success = success, + error.message = error_message.as_deref(), + attempt = attempt, + endpoint = MODELS_ENDPOINT, + auth.header_attached = self.auth_header_attached, + auth.header_name = self.auth_header_name, + auth.request_id = response_debug.request_id.as_deref(), + auth.cf_ray = response_debug.cf_ray.as_deref(), + auth.error = response_debug.auth_error.as_deref(), + auth.error_code = response_debug.auth_error_code.as_deref(), + auth.mode = self.auth_mode.as_deref(), + ); + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: MODELS_ENDPOINT, + auth_header_attached: self.auth_header_attached, + auth_header_name: self.auth_header_name, + auth_mode: self.auth_mode.as_deref(), + auth_retry_after_unauthorized: None, + auth_recovery_mode: None, + auth_recovery_phase: None, + auth_connection_reused: None, + auth_request_id: response_debug.request_id.as_deref(), + auth_cf_ray: response_debug.cf_ray.as_deref(), + auth_error: response_debug.auth_error.as_deref(), + auth_error_code: response_debug.auth_error_code.as_deref(), + auth_recovery_followup_success: None, + auth_recovery_followup_status: None, + }); + } +} /// Strategy for refreshing available models. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -330,11 +414,17 @@ impl ModelsManager { let _timer = codex_otel::start_global_timer("codex.remote_models.fetch_update.duration_ms", &[]); let auth = self.auth_manager.auth().await; - let auth_mode = self.auth_manager.auth_mode(); + let auth_mode = auth.as_ref().map(CodexAuth::auth_mode); let api_provider = self.provider.to_api_provider(auth_mode)?; let api_auth = auth_provider_from_auth(auth.clone(), &self.provider)?; let transport = ReqwestTransport::new(build_reqwest_client()); - let client = ModelsClient::new(transport, api_provider, api_auth); + let request_telemetry: Arc = Arc::new(ModelsRequestTelemetry { + auth_mode: auth_mode.map(|mode| TelemetryAuthMode::from(mode).to_string()), + auth_header_attached: api_auth.auth_header_attached(), + auth_header_name: api_auth.auth_header_name(), + }); + let client = ModelsClient::new(transport, api_provider, api_auth) + .with_telemetry(Some(request_telemetry)); let client_version = crate::models_manager::client_version_to_whole(); let (models, etag) = timeout( diff --git a/codex-rs/core/src/response_debug_context.rs b/codex-rs/core/src/response_debug_context.rs new file mode 100644 index 0000000000..bc7eab172b --- /dev/null +++ b/codex-rs/core/src/response_debug_context.rs @@ -0,0 +1,167 @@ +use base64::Engine; +use codex_api::TransportError; +use codex_api::error::ApiError; + +const REQUEST_ID_HEADER: &str = "x-request-id"; +const OAI_REQUEST_ID_HEADER: &str = "x-oai-request-id"; +const CF_RAY_HEADER: &str = "cf-ray"; +const AUTH_ERROR_HEADER: &str = "x-openai-authorization-error"; +const X_ERROR_JSON_HEADER: &str = "x-error-json"; + +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub(crate) struct ResponseDebugContext { + pub(crate) request_id: Option, + pub(crate) cf_ray: Option, + pub(crate) auth_error: Option, + pub(crate) auth_error_code: Option, +} + +pub(crate) fn extract_response_debug_context(transport: &TransportError) -> ResponseDebugContext { + let mut context = ResponseDebugContext::default(); + + let TransportError::Http { + headers, body: _, .. + } = transport + else { + return context; + }; + + let extract_header = |name: &str| { + headers + .as_ref() + .and_then(|headers| headers.get(name)) + .and_then(|value| value.to_str().ok()) + .map(str::to_string) + }; + + context.request_id = + extract_header(REQUEST_ID_HEADER).or_else(|| extract_header(OAI_REQUEST_ID_HEADER)); + context.cf_ray = extract_header(CF_RAY_HEADER); + context.auth_error = extract_header(AUTH_ERROR_HEADER); + context.auth_error_code = extract_header(X_ERROR_JSON_HEADER).and_then(|encoded| { + let decoded = base64::engine::general_purpose::STANDARD + .decode(encoded) + .ok()?; + let parsed = serde_json::from_slice::(&decoded).ok()?; + parsed + .get("error") + .and_then(|error| error.get("code")) + .and_then(serde_json::Value::as_str) + .map(str::to_string) + }); + + context +} + +pub(crate) fn extract_response_debug_context_from_api_error( + error: &ApiError, +) -> ResponseDebugContext { + match error { + ApiError::Transport(transport) => extract_response_debug_context(transport), + _ => ResponseDebugContext::default(), + } +} + +pub(crate) fn telemetry_transport_error_message(error: &TransportError) -> String { + match error { + TransportError::Http { status, .. } => format!("http {}", status.as_u16()), + TransportError::RetryLimit => "retry limit reached".to_string(), + TransportError::Timeout => "timeout".to_string(), + TransportError::Network(err) => err.to_string(), + TransportError::Build(err) => err.to_string(), + } +} + +pub(crate) fn telemetry_api_error_message(error: &ApiError) -> String { + match error { + ApiError::Transport(transport) => telemetry_transport_error_message(transport), + ApiError::Api { status, .. } => format!("api error {}", status.as_u16()), + ApiError::Stream(err) => err.to_string(), + ApiError::ContextWindowExceeded => "context window exceeded".to_string(), + ApiError::QuotaExceeded => "quota exceeded".to_string(), + ApiError::UsageNotIncluded => "usage not included".to_string(), + ApiError::Retryable { .. } => "retryable error".to_string(), + ApiError::RateLimit(_) => "rate limit".to_string(), + ApiError::InvalidRequest { .. } => "invalid request".to_string(), + ApiError::ServerOverloaded => "server overloaded".to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::ResponseDebugContext; + use super::extract_response_debug_context; + use super::telemetry_api_error_message; + use super::telemetry_transport_error_message; + use codex_api::TransportError; + use codex_api::error::ApiError; + use http::HeaderMap; + use http::HeaderValue; + use http::StatusCode; + use pretty_assertions::assert_eq; + + #[test] + fn extract_response_debug_context_decodes_identity_headers() { + let mut headers = HeaderMap::new(); + headers.insert("x-oai-request-id", HeaderValue::from_static("req-auth")); + headers.insert("cf-ray", HeaderValue::from_static("ray-auth")); + headers.insert( + "x-openai-authorization-error", + HeaderValue::from_static("missing_authorization_header"), + ); + headers.insert( + "x-error-json", + HeaderValue::from_static("eyJlcnJvciI6eyJjb2RlIjoidG9rZW5fZXhwaXJlZCJ9fQ=="), + ); + + let context = extract_response_debug_context(&TransportError::Http { + status: StatusCode::UNAUTHORIZED, + url: Some("https://chatgpt.com/backend-api/codex/models".to_string()), + headers: Some(headers), + body: Some(r#"{"error":{"message":"plain text error"},"status":401}"#.to_string()), + }); + + assert_eq!( + context, + ResponseDebugContext { + request_id: Some("req-auth".to_string()), + cf_ray: Some("ray-auth".to_string()), + auth_error: Some("missing_authorization_header".to_string()), + auth_error_code: Some("token_expired".to_string()), + } + ); + } + + #[test] + fn telemetry_error_messages_omit_http_bodies() { + let transport = TransportError::Http { + status: StatusCode::UNAUTHORIZED, + url: Some("https://chatgpt.com/backend-api/codex/responses".to_string()), + headers: None, + body: Some(r#"{"error":{"message":"secret token leaked"}}"#.to_string()), + }; + + assert_eq!(telemetry_transport_error_message(&transport), "http 401"); + assert_eq!( + telemetry_api_error_message(&ApiError::Transport(transport)), + "http 401" + ); + } + + #[test] + fn telemetry_error_messages_preserve_non_http_details() { + let network = TransportError::Network("dns lookup failed".to_string()); + let build = TransportError::Build("invalid header value".to_string()); + let stream = ApiError::Stream("socket closed".to_string()); + + assert_eq!( + telemetry_transport_error_message(&network), + "dns lookup failed" + ); + assert_eq!( + telemetry_transport_error_message(&build), + "invalid header value" + ); + assert_eq!(telemetry_api_error_message(&stream), "socket closed"); + } +} diff --git a/codex-rs/core/src/seatbelt_tests.rs b/codex-rs/core/src/seatbelt_tests.rs index 9ac5eaa7b0..bab1362017 100644 --- a/codex-rs/core/src/seatbelt_tests.rs +++ b/codex-rs/core/src/seatbelt_tests.rs @@ -126,6 +126,8 @@ fn explicit_unreadable_paths_are_excluded_from_full_disk_read_and_write_access() ); let policy = seatbelt_policy_arg(&args); + let unreadable_roots = file_system_policy.get_unreadable_roots_with_cwd(Path::new("/")); + let unreadable_root = unreadable_roots.first().expect("expected unreadable root"); assert!( policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_RO_0\")))"), "expected read carveout in policy:\n{policy}" @@ -136,12 +138,12 @@ fn explicit_unreadable_paths_are_excluded_from_full_disk_read_and_write_access() ); assert!( args.iter() - .any(|arg| arg == "-DREADABLE_ROOT_0_RO_0=/tmp/codex-unreadable"), + .any(|arg| arg == &format!("-DREADABLE_ROOT_0_RO_0={}", unreadable_root.display())), "expected read carveout parameter in args: {args:#?}" ); assert!( args.iter() - .any(|arg| arg == "-DWRITABLE_ROOT_0_RO_0=/tmp/codex-unreadable"), + .any(|arg| arg == &format!("-DWRITABLE_ROOT_0_RO_0={}", unreadable_root.display())), "expected write carveout parameter in args: {args:#?}" ); } @@ -172,18 +174,22 @@ fn explicit_unreadable_paths_are_excluded_from_readable_roots() { ); let policy = seatbelt_policy_arg(&args); + let readable_roots = file_system_policy.get_readable_roots_with_cwd(Path::new("/")); + let readable_root = readable_roots.first().expect("expected readable root"); + let unreadable_roots = file_system_policy.get_unreadable_roots_with_cwd(Path::new("/")); + let unreadable_root = unreadable_roots.first().expect("expected unreadable root"); assert!( policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_RO_0\")))"), "expected read carveout in policy:\n{policy}" ); assert!( args.iter() - .any(|arg| arg == "-DREADABLE_ROOT_0=/tmp/codex-readable"), + .any(|arg| arg == &format!("-DREADABLE_ROOT_0={}", readable_root.display())), "expected readable root parameter in args: {args:#?}" ); assert!( args.iter() - .any(|arg| arg == "-DREADABLE_ROOT_0_RO_0=/tmp/codex-readable/private"), + .any(|arg| arg == &format!("-DREADABLE_ROOT_0_RO_0={}", unreadable_root.display())), "expected read carveout parameter in args: {args:#?}" ); } diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 2cf4e16d49..331a55f0cb 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -5,6 +5,7 @@ use crate::client_common::tools::ToolSpec; use crate::config::AgentRoleConfig; use crate::features::Feature; use crate::features::Features; +use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; use crate::mcp_connection_manager::ToolInfo; use crate::models_manager::collaboration_mode_presets::CollaborationModesConfig; use crate::original_image_detail::can_request_original_image_detail; @@ -1673,22 +1674,58 @@ fn create_tool_search_tool(app_tools: &HashMap) -> ToolSpec { }, ), ]); - let mut app_names = app_tools - .values() - .filter_map(|tool| tool.connector_name.clone()) - .collect::>(); - app_names.sort(); - app_names.dedup(); - let app_names = app_names.join(", "); + let mut app_descriptions = BTreeMap::new(); + for tool in app_tools.values() { + if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { + continue; + } - let description = if app_names.is_empty() { - TOOL_SEARCH_DESCRIPTION_TEMPLATE - .replace("({{app_names}})", "(None currently enabled)") - .replace("{{app_names}}", "available apps") + let Some(connector_name) = tool + .connector_name + .as_deref() + .map(str::trim) + .filter(|connector_name| !connector_name.is_empty()) + else { + continue; + }; + + let connector_description = tool + .connector_description + .as_deref() + .map(str::trim) + .filter(|connector_description| !connector_description.is_empty()) + .map(str::to_string); + + app_descriptions + .entry(connector_name.to_string()) + .and_modify(|existing: &mut Option| { + if existing.is_none() { + *existing = connector_description.clone(); + } + }) + .or_insert(connector_description); + } + + let app_descriptions = if app_descriptions.is_empty() { + "None currently enabled.".to_string() } else { - TOOL_SEARCH_DESCRIPTION_TEMPLATE.replace("{{app_names}}", app_names.as_str()) + app_descriptions + .into_iter() + .map( + |(connector_name, connector_description)| match connector_description { + Some(connector_description) => { + format!("- {connector_name}: {connector_description}") + } + None => format!("- {connector_name}"), + }, + ) + .collect::>() + .join("\n") }; + let description = + TOOL_SEARCH_DESCRIPTION_TEMPLATE.replace("{{app_descriptions}}", app_descriptions.as_str()); + ToolSpec::ToolSearch { execution: "client".to_string(), description, diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index a5a904d2aa..c3c228703a 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -1690,7 +1690,7 @@ fn test_build_specs_mcp_tools_sorted_by_name() { } #[test] -fn search_tool_description_includes_only_codex_apps_connector_names() { +fn search_tool_description_lists_each_codex_apps_connector_once() { let model_info = search_capable_model_info(); let mut features = Features::with_defaults(); features.enable(Feature::Apps); @@ -1736,7 +1736,45 @@ fn search_tool_description_includes_only_codex_apps_connector_names() { connector_id: Some("calendar".to_string()), connector_name: Some("Calendar".to_string()), plugin_display_names: Vec::new(), - connector_description: None, + connector_description: Some( + "Plan events and manage your calendar.".to_string(), + ), + }, + ), + ( + "mcp__codex_apps__calendar_list_events".to_string(), + ToolInfo { + server_name: crate::mcp::CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool_name: "_list_events".to_string(), + tool_namespace: "mcp__codex_apps__calendar".to_string(), + tool: mcp_tool( + "calendar-list-events", + "List calendar events", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + connector_description: Some( + "Plan events and manage your calendar.".to_string(), + ), + }, + ), + ( + "mcp__codex_apps__gmail_search_threads".to_string(), + ToolInfo { + server_name: crate::mcp::CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool_name: "_search_threads".to_string(), + tool_namespace: "mcp__codex_apps__gmail".to_string(), + tool: mcp_tool( + "gmail-search-threads", + "Search email threads", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("gmail".to_string()), + connector_name: Some("Gmail".to_string()), + plugin_display_names: Vec::new(), + connector_description: Some("Find and summarize email threads.".to_string()), }, ), ( @@ -1762,7 +1800,14 @@ fn search_tool_description_includes_only_codex_apps_connector_names() { panic!("expected tool_search tool"); }; let description = description.as_str(); - assert!(description.contains("Calendar")); + assert!(description.contains("- Calendar: Plan events and manage your calendar.")); + assert!(description.contains("- Gmail: Find and summarize email threads.")); + assert_eq!( + description + .matches("- Calendar: Plan events and manage your calendar.") + .count(), + 1 + ); assert!(!description.contains("mcp__rmcp__echo")); } @@ -1874,8 +1919,56 @@ fn search_tool_description_handles_no_enabled_apps() { panic!("expected tool_search tool"); }; - assert!(description.contains("(None currently enabled)")); - assert!(!description.contains("{{app_names}}")); + assert!(description.contains("None currently enabled.")); + assert!(!description.contains("{{app_descriptions}}")); +} + +#[test] +fn search_tool_description_falls_back_to_connector_name_without_description() { + let model_info = search_capable_model_info(); + let mut features = Features::with_defaults(); + features.enable(Feature::Apps); + let available_models = Vec::new(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + available_models: &available_models, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + sandbox_policy: &SandboxPolicy::DangerFullAccess, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + }); + + let (tools, _) = build_specs( + &tools_config, + None, + Some(HashMap::from([( + "mcp__codex_apps__calendar_create_event".to_string(), + ToolInfo { + server_name: crate::mcp::CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool_name: "_create_event".to_string(), + tool_namespace: "mcp__codex_apps__calendar".to_string(), + tool: mcp_tool( + "calendar_create_event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + connector_description: None, + }, + )])), + &[], + ) + .build(); + let search_tool = find_tool(&tools, TOOL_SEARCH_TOOL_NAME); + let ToolSpec::ToolSearch { description, .. } = &search_tool.spec else { + panic!("expected tool_search tool"); + }; + + assert!(description.contains("- Calendar")); + assert!(!description.contains("- Calendar:")); } #[test] diff --git a/codex-rs/core/src/util.rs b/codex-rs/core/src/util.rs index 62e872ae6c..43c6d85222 100644 --- a/codex-rs/core/src/util.rs +++ b/codex-rs/core/src/util.rs @@ -37,6 +37,111 @@ macro_rules! feedback_tags { }; } +pub(crate) struct FeedbackRequestTags<'a> { + pub endpoint: &'a str, + pub auth_header_attached: bool, + pub auth_header_name: Option<&'a str>, + pub auth_mode: Option<&'a str>, + pub auth_retry_after_unauthorized: Option, + pub auth_recovery_mode: Option<&'a str>, + pub auth_recovery_phase: Option<&'a str>, + pub auth_connection_reused: Option, + pub auth_request_id: Option<&'a str>, + pub auth_cf_ray: Option<&'a str>, + pub auth_error: Option<&'a str>, + pub auth_error_code: Option<&'a str>, + pub auth_recovery_followup_success: Option, + pub auth_recovery_followup_status: Option, +} + +struct Auth401FeedbackSnapshot<'a> { + request_id: &'a str, + cf_ray: &'a str, + error: &'a str, + error_code: &'a str, +} + +impl<'a> Auth401FeedbackSnapshot<'a> { + fn from_optional_fields( + request_id: Option<&'a str>, + cf_ray: Option<&'a str>, + error: Option<&'a str>, + error_code: Option<&'a str>, + ) -> Self { + Self { + request_id: request_id.unwrap_or(""), + cf_ray: cf_ray.unwrap_or(""), + error: error.unwrap_or(""), + error_code: error_code.unwrap_or(""), + } + } +} + +pub(crate) fn emit_feedback_request_tags(tags: &FeedbackRequestTags<'_>) { + let auth_header_name = tags.auth_header_name.unwrap_or(""); + let auth_mode = tags.auth_mode.unwrap_or(""); + let auth_retry_after_unauthorized = tags + .auth_retry_after_unauthorized + .map_or_else(String::new, |value| value.to_string()); + let auth_recovery_mode = tags.auth_recovery_mode.unwrap_or(""); + let auth_recovery_phase = tags.auth_recovery_phase.unwrap_or(""); + let auth_connection_reused = tags + .auth_connection_reused + .map_or_else(String::new, |value| value.to_string()); + let auth_request_id = tags.auth_request_id.unwrap_or(""); + let auth_cf_ray = tags.auth_cf_ray.unwrap_or(""); + let auth_error = tags.auth_error.unwrap_or(""); + let auth_error_code = tags.auth_error_code.unwrap_or(""); + let auth_recovery_followup_success = tags + .auth_recovery_followup_success + .map_or_else(String::new, |value| value.to_string()); + let auth_recovery_followup_status = tags + .auth_recovery_followup_status + .map_or_else(String::new, |value| value.to_string()); + feedback_tags!( + endpoint = tags.endpoint, + auth_header_attached = tags.auth_header_attached, + auth_header_name = auth_header_name, + auth_mode = auth_mode, + auth_retry_after_unauthorized = auth_retry_after_unauthorized, + auth_recovery_mode = auth_recovery_mode, + auth_recovery_phase = auth_recovery_phase, + auth_connection_reused = auth_connection_reused, + auth_request_id = auth_request_id, + auth_cf_ray = auth_cf_ray, + auth_error = auth_error, + auth_error_code = auth_error_code, + auth_recovery_followup_success = auth_recovery_followup_success, + auth_recovery_followup_status = auth_recovery_followup_status + ); +} + +pub(crate) fn emit_feedback_auth_recovery_tags( + auth_recovery_mode: &str, + auth_recovery_phase: &str, + auth_recovery_outcome: &str, + auth_request_id: Option<&str>, + auth_cf_ray: Option<&str>, + auth_error: Option<&str>, + auth_error_code: Option<&str>, +) { + let auth_401 = Auth401FeedbackSnapshot::from_optional_fields( + auth_request_id, + auth_cf_ray, + auth_error, + auth_error_code, + ); + feedback_tags!( + auth_recovery_mode = auth_recovery_mode, + auth_recovery_phase = auth_recovery_phase, + auth_recovery_outcome = auth_recovery_outcome, + auth_401_request_id = auth_401.request_id, + auth_401_cf_ray = auth_401.cf_ray, + auth_401_error = auth_401.error, + auth_401_error_code = auth_401.error_code + ); +} + pub fn backoff(attempt: u64) -> Duration { let exp = BACKOFF_FACTOR.powi(attempt.saturating_sub(1) as i32); let base = (INITIAL_DELAY_MS as f64 * exp) as u64; diff --git a/codex-rs/core/src/util_tests.rs b/codex-rs/core/src/util_tests.rs index dd5956bf61..9df8c67e89 100644 --- a/codex-rs/core/src/util_tests.rs +++ b/codex-rs/core/src/util_tests.rs @@ -1,4 +1,15 @@ use super::*; +use std::collections::BTreeMap; +use std::sync::Arc; +use std::sync::Mutex; +use tracing::Event; +use tracing::Subscriber; +use tracing::field::Visit; +use tracing_subscriber::Layer; +use tracing_subscriber::layer::Context; +use tracing_subscriber::layer::SubscriberExt; +use tracing_subscriber::registry::LookupSpan; +use tracing_subscriber::util::SubscriberInitExt; #[test] fn test_try_parse_error_message() { @@ -32,6 +43,298 @@ fn feedback_tags_macro_compiles() { feedback_tags!(model = "gpt-5", cached = true, debug_only = OnlyDebug); } +#[derive(Default)] +struct TagCollectorVisitor { + tags: BTreeMap, +} + +impl Visit for TagCollectorVisitor { + fn record_bool(&mut self, field: &tracing::field::Field, value: bool) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_str(&mut self, field: &tracing::field::Field, value: &str) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) { + self.tags + .insert(field.name().to_string(), format!("{value:?}")); + } +} + +#[derive(Clone)] +struct TagCollectorLayer { + tags: Arc>>, +} + +impl Layer for TagCollectorLayer +where + S: Subscriber + for<'a> LookupSpan<'a>, +{ + fn on_event(&self, event: &Event<'_>, _ctx: Context<'_, S>) { + if event.metadata().target() != "feedback_tags" { + return; + } + let mut visitor = TagCollectorVisitor::default(); + event.record(&mut visitor); + self.tags.lock().unwrap().extend(visitor.tags); + } +} + +#[test] +fn emit_feedback_request_tags_records_sentry_feedback_fields() { + let tags = Arc::new(Mutex::new(BTreeMap::new())); + let _guard = tracing_subscriber::registry() + .with(TagCollectorLayer { tags: tags.clone() }) + .set_default(); + + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: "/responses", + auth_header_attached: true, + auth_header_name: Some("authorization"), + auth_mode: Some("chatgpt"), + auth_retry_after_unauthorized: Some(false), + auth_recovery_mode: Some("managed"), + auth_recovery_phase: Some("refresh_token"), + auth_connection_reused: Some(true), + auth_request_id: Some("req-123"), + auth_cf_ray: Some("ray-123"), + auth_error: Some("missing_authorization_header"), + auth_error_code: Some("token_expired"), + auth_recovery_followup_success: Some(true), + auth_recovery_followup_status: Some(200), + }); + + let tags = tags.lock().unwrap().clone(); + assert_eq!( + tags.get("endpoint").map(String::as_str), + Some("\"/responses\"") + ); + assert_eq!( + tags.get("auth_header_attached").map(String::as_str), + Some("true") + ); + assert_eq!( + tags.get("auth_header_name").map(String::as_str), + Some("\"authorization\"") + ); + assert_eq!( + tags.get("auth_request_id").map(String::as_str), + Some("\"req-123\"") + ); + assert_eq!( + tags.get("auth_error_code").map(String::as_str), + Some("\"token_expired\"") + ); + assert_eq!( + tags.get("auth_recovery_followup_success") + .map(String::as_str), + Some("\"true\"") + ); + assert_eq!( + tags.get("auth_recovery_followup_status") + .map(String::as_str), + Some("\"200\"") + ); +} + +#[test] +fn emit_feedback_auth_recovery_tags_preserves_401_specific_fields() { + let tags = Arc::new(Mutex::new(BTreeMap::new())); + let _guard = tracing_subscriber::registry() + .with(TagCollectorLayer { tags: tags.clone() }) + .set_default(); + + emit_feedback_auth_recovery_tags( + "managed", + "refresh_token", + "recovery_succeeded", + Some("req-401"), + Some("ray-401"), + Some("missing_authorization_header"), + Some("token_expired"), + ); + + let tags = tags.lock().unwrap().clone(); + assert_eq!( + tags.get("auth_401_request_id").map(String::as_str), + Some("\"req-401\"") + ); + assert_eq!( + tags.get("auth_401_cf_ray").map(String::as_str), + Some("\"ray-401\"") + ); + assert_eq!( + tags.get("auth_401_error").map(String::as_str), + Some("\"missing_authorization_header\"") + ); + assert_eq!( + tags.get("auth_401_error_code").map(String::as_str), + Some("\"token_expired\"") + ); +} + +#[test] +fn emit_feedback_auth_recovery_tags_clears_stale_401_fields() { + let tags = Arc::new(Mutex::new(BTreeMap::new())); + let _guard = tracing_subscriber::registry() + .with(TagCollectorLayer { tags: tags.clone() }) + .set_default(); + + emit_feedback_auth_recovery_tags( + "managed", + "refresh_token", + "recovery_failed_transient", + Some("req-401-a"), + Some("ray-401-a"), + Some("missing_authorization_header"), + Some("token_expired"), + ); + emit_feedback_auth_recovery_tags( + "managed", + "done", + "recovery_not_run", + Some("req-401-b"), + None, + None, + None, + ); + + let tags = tags.lock().unwrap().clone(); + assert_eq!( + tags.get("auth_401_request_id").map(String::as_str), + Some("\"req-401-b\"") + ); + assert_eq!( + tags.get("auth_401_cf_ray").map(String::as_str), + Some("\"\"") + ); + assert_eq!(tags.get("auth_401_error").map(String::as_str), Some("\"\"")); + assert_eq!( + tags.get("auth_401_error_code").map(String::as_str), + Some("\"\"") + ); +} + +#[test] +fn emit_feedback_request_tags_preserves_latest_auth_fields_after_unauthorized() { + let tags = Arc::new(Mutex::new(BTreeMap::new())); + let _guard = tracing_subscriber::registry() + .with(TagCollectorLayer { tags: tags.clone() }) + .set_default(); + + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: "/responses", + auth_header_attached: true, + auth_header_name: Some("authorization"), + auth_mode: Some("chatgpt"), + auth_retry_after_unauthorized: Some(true), + auth_recovery_mode: Some("managed"), + auth_recovery_phase: Some("refresh_token"), + auth_connection_reused: None, + auth_request_id: Some("req-123"), + auth_cf_ray: Some("ray-123"), + auth_error: Some("missing_authorization_header"), + auth_error_code: Some("token_expired"), + auth_recovery_followup_success: Some(false), + auth_recovery_followup_status: Some(401), + }); + + let tags = tags.lock().unwrap().clone(); + assert_eq!( + tags.get("auth_request_id").map(String::as_str), + Some("\"req-123\"") + ); + assert_eq!( + tags.get("auth_cf_ray").map(String::as_str), + Some("\"ray-123\"") + ); + assert_eq!( + tags.get("auth_error").map(String::as_str), + Some("\"missing_authorization_header\"") + ); + assert_eq!( + tags.get("auth_error_code").map(String::as_str), + Some("\"token_expired\"") + ); + assert_eq!( + tags.get("auth_recovery_followup_success") + .map(String::as_str), + Some("\"false\"") + ); +} + +#[test] +fn emit_feedback_request_tags_clears_stale_latest_auth_fields() { + let tags = Arc::new(Mutex::new(BTreeMap::new())); + let _guard = tracing_subscriber::registry() + .with(TagCollectorLayer { tags: tags.clone() }) + .set_default(); + + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: "/responses", + auth_header_attached: true, + auth_header_name: Some("authorization"), + auth_mode: Some("chatgpt"), + auth_retry_after_unauthorized: Some(false), + auth_recovery_mode: Some("managed"), + auth_recovery_phase: Some("refresh_token"), + auth_connection_reused: Some(true), + auth_request_id: Some("req-123"), + auth_cf_ray: Some("ray-123"), + auth_error: Some("missing_authorization_header"), + auth_error_code: Some("token_expired"), + auth_recovery_followup_success: Some(true), + auth_recovery_followup_status: Some(200), + }); + emit_feedback_request_tags(&FeedbackRequestTags { + endpoint: "/responses", + auth_header_attached: true, + auth_header_name: None, + auth_mode: None, + auth_retry_after_unauthorized: None, + auth_recovery_mode: None, + auth_recovery_phase: None, + auth_connection_reused: None, + auth_request_id: None, + auth_cf_ray: None, + auth_error: None, + auth_error_code: None, + auth_recovery_followup_success: None, + auth_recovery_followup_status: None, + }); + + let tags = tags.lock().unwrap().clone(); + assert_eq!( + tags.get("auth_header_name").map(String::as_str), + Some("\"\"") + ); + assert_eq!(tags.get("auth_mode").map(String::as_str), Some("\"\"")); + assert_eq!( + tags.get("auth_request_id").map(String::as_str), + Some("\"\"") + ); + assert_eq!(tags.get("auth_cf_ray").map(String::as_str), Some("\"\"")); + assert_eq!(tags.get("auth_error").map(String::as_str), Some("\"\"")); + assert_eq!( + tags.get("auth_error_code").map(String::as_str), + Some("\"\"") + ); + assert_eq!( + tags.get("auth_recovery_followup_success") + .map(String::as_str), + Some("\"\"") + ); + assert_eq!( + tags.get("auth_recovery_followup_status") + .map(String::as_str), + Some("\"\"") + ); +} + #[test] fn normalize_thread_name_trims_and_rejects_empty() { assert_eq!(normalize_thread_name(" "), None); diff --git a/codex-rs/core/templates/search_tool/tool_description.md b/codex-rs/core/templates/search_tool/tool_description.md index db6b4e34aa..6472011c20 100644 --- a/codex-rs/core/templates/search_tool/tool_description.md +++ b/codex-rs/core/templates/search_tool/tool_description.md @@ -2,5 +2,6 @@ Searches over apps/connectors tool metadata with BM25 and exposes matching tools for the next model call. -Tools of the apps ({{app_names}}) are hidden until you search for them with this tool (`tool_search`). -When the request needs one of these connectors and you don't already have the required tools from it, use this tool to load them. For the apps mentioned above, always prefer `tool_search` over `list_mcp_resources` or `list_mcp_resource_templates` for tool discovery. +You have access to all the tools of the following apps/connectors: +{{app_descriptions}} +Some of the tools may not have been provided to you upfront, and you should use this tool (`tool_search`) to search for the required tools and load them for the apps mentioned above. For the apps mentioned above, always use `tool_search` instead of `list_mcp_resources` or `list_mcp_resource_templates` for tool discovery. diff --git a/codex-rs/core/tests/common/apps_test_server.rs b/codex-rs/core/tests/common/apps_test_server.rs index 83ce020bef..8ac60ffb13 100644 --- a/codex-rs/core/tests/common/apps_test_server.rs +++ b/codex-rs/core/tests/common/apps_test_server.rs @@ -18,6 +18,9 @@ const CONNECTOR_DESCRIPTION: &str = "Plan events and manage your calendar."; const PROTOCOL_VERSION: &str = "2025-11-25"; const SERVER_NAME: &str = "codex-apps-test"; const SERVER_VERSION: &str = "1.0.0"; +pub const CALENDAR_CREATE_EVENT_RESOURCE_URI: &str = + "connector://calendar/tools/calendar_create_event"; +const CALENDAR_LIST_EVENTS_RESOURCE_URI: &str = "connector://calendar/tools/calendar_list_events"; #[derive(Clone)] pub struct AppsTestServer { @@ -175,7 +178,12 @@ impl Respond for CodexAppsJsonRpcResponder { "_meta": { "connector_id": CONNECTOR_ID, "connector_name": self.connector_name.clone(), - "connector_description": self.connector_description.clone() + "connector_description": self.connector_description.clone(), + "_codex_apps": { + "resource_uri": CALENDAR_CREATE_EVENT_RESOURCE_URI, + "contains_mcp_source": true, + "connector_id": CONNECTOR_ID + } } }, { @@ -192,7 +200,12 @@ impl Respond for CodexAppsJsonRpcResponder { "_meta": { "connector_id": CONNECTOR_ID, "connector_name": self.connector_name.clone(), - "connector_description": self.connector_description.clone() + "connector_description": self.connector_description.clone(), + "_codex_apps": { + "resource_uri": CALENDAR_LIST_EVENTS_RESOURCE_URI, + "contains_mcp_source": true, + "connector_id": CONNECTOR_ID + } } } ], @@ -214,6 +227,7 @@ impl Respond for CodexAppsJsonRpcResponder { .pointer("/params/arguments/starts_at") .and_then(Value::as_str) .unwrap_or_default(); + let codex_apps_meta = body.pointer("/params/_meta/_codex_apps").cloned(); ResponseTemplate::new(200).set_body_json(json!({ "jsonrpc": "2.0", @@ -223,6 +237,9 @@ impl Respond for CodexAppsJsonRpcResponder { "type": "text", "text": format!("called {tool_name} for {title} at {starts_at}") }], + "structuredContent": { + "_codex_apps": codex_apps_meta, + }, "isError": false } })) diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index a94ad9bf9c..2fc5f8b9d1 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -943,10 +943,6 @@ async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() { .features .enable(Feature::Apps) .expect("test config should allow feature update"); - config - .features - .disable(Feature::AppsMcpGateway) - .expect("test config should allow feature update"); config.chatgpt_base_url = apps_base_url; }); let codex = builder @@ -971,7 +967,8 @@ async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() { let request = resp_mock.single_request(); let request_body = request.body_json(); let input = request_body["input"].as_array().expect("input array"); - let apps_snippet = "Apps are mentioned in user messages in the format"; + let apps_snippet = + "Apps (Connectors) can be explicitly triggered in user messages in the format"; let has_developer_apps_guidance = input.iter().any(|item| { item.get("role").and_then(|value| value.as_str()) == Some("developer") @@ -1034,10 +1031,6 @@ async fn omits_apps_guidance_for_api_key_auth_even_when_feature_enabled() { .features .enable(Feature::Apps) .expect("test config should allow feature update"); - config - .features - .disable(Feature::AppsMcpGateway) - .expect("test config should allow feature update"); config.chatgpt_base_url = apps_base_url; }); let codex = builder @@ -1062,7 +1055,8 @@ async fn omits_apps_guidance_for_api_key_auth_even_when_feature_enabled() { let request = resp_mock.single_request(); let request_body = request.body_json(); let input = request_body["input"].as_array().expect("input array"); - let apps_snippet = "Apps are mentioned in the prompt in the format"; + let apps_snippet = + "Apps (Connectors) can be explicitly triggered in user messages in the format"; let has_apps_guidance = input.iter().any(|item| { item.get("content") diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index 71a9f166a8..0eba6e3234 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -142,10 +142,6 @@ async fn build_apps_enabled_plugin_test_codex( .features .enable(Feature::Apps) .expect("test config should allow feature update"); - config - .features - .disable(Feature::AppsMcpGateway) - .expect("test config should allow feature update"); config.chatgpt_base_url = chatgpt_base_url; }); Ok(builder diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index d23d2f2669..485a216b4a 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -13,6 +13,7 @@ use codex_protocol::protocol::Op; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::user_input::UserInput; use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::CALENDAR_CREATE_EVENT_RESOURCE_URI; use core_test_support::responses::ResponsesRequest; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -30,8 +31,9 @@ use pretty_assertions::assert_eq; use serde_json::Value; use serde_json::json; -const SEARCH_TOOL_DESCRIPTION_SNIPPETS: [&str; 1] = [ - "Tools of the apps (Calendar) are hidden until you search for them with this tool (`tool_search`).", +const SEARCH_TOOL_DESCRIPTION_SNIPPETS: [&str; 2] = [ + "You have access to all the tools of the following apps/connectors", + "- Calendar: Plan events and manage your calendar.", ]; const TOOL_SEARCH_TOOL_NAME: &str = "tool_search"; const CALENDAR_CREATE_TOOL: &str = "mcp__codex_apps__calendar_create_event"; @@ -89,10 +91,6 @@ fn configure_apps(config: &mut Config, apps_base_url: &str) { .features .enable(Feature::Apps) .expect("test config should allow feature update"); - config - .features - .disable(Feature::AppsMcpGateway) - .expect("test config should allow feature update"); config.chatgpt_base_url = apps_base_url.to_string(); config.model = Some("gpt-5-codex".to_string()); @@ -404,6 +402,19 @@ async fn tool_search_returns_deferred_tools_without_follow_up_tool_injection() - })), } ); + assert_eq!( + end.result + .as_ref() + .expect("tool call should succeed") + .structured_content, + Some(json!({ + "_codex_apps": { + "resource_uri": CALENDAR_CREATE_EVENT_RESOURCE_URI, + "contains_mcp_source": true, + "connector_id": "calendar", + }, + })) + ); wait_for_event(&test.codex, |event| { matches!(event, EventMsg::TurnComplete(_)) diff --git a/codex-rs/otel/src/events/session_telemetry.rs b/codex-rs/otel/src/events/session_telemetry.rs index 327416093a..c8520ea6c9 100644 --- a/codex-rs/otel/src/events/session_telemetry.rs +++ b/codex-rs/otel/src/events/session_telemetry.rs @@ -340,17 +340,43 @@ impl SessionTelemetry { Ok(response) => (Some(response.status().as_u16()), None), Err(error) => (error.status().map(|s| s.as_u16()), Some(error.to_string())), }; - self.record_api_request(attempt, status, error.as_deref(), duration); + self.record_api_request( + attempt, + status, + error.as_deref(), + duration, + false, + None, + false, + None, + None, + "unknown", + None, + None, + None, + None, + ); response } + #[allow(clippy::too_many_arguments)] pub fn record_api_request( &self, attempt: u64, status: Option, error: Option<&str>, duration: Duration, + auth_header_attached: bool, + auth_header_name: Option<&str>, + retry_after_unauthorized: bool, + recovery_mode: Option<&str>, + recovery_phase: Option<&str>, + endpoint: &str, + request_id: Option<&str>, + cf_ray: Option<&str>, + auth_error: Option<&str>, + auth_error_code: Option<&str>, ) { let success = status.is_some_and(|code| (200..=299).contains(&code)) && error.is_none(); let success_str = if success { "true" } else { "false" }; @@ -375,13 +401,76 @@ impl SessionTelemetry { http.response.status_code = status, error.message = error, attempt = attempt, + auth.header_attached = auth_header_attached, + auth.header_name = auth_header_name, + auth.retry_after_unauthorized = retry_after_unauthorized, + auth.recovery_mode = recovery_mode, + auth.recovery_phase = recovery_phase, + endpoint = endpoint, + auth.request_id = request_id, + auth.cf_ray = cf_ray, + auth.error = auth_error, + auth.error_code = auth_error_code, }, log: {}, trace: {}, ); } - pub fn record_websocket_request(&self, duration: Duration, error: Option<&str>) { + #[allow(clippy::too_many_arguments)] + pub fn record_websocket_connect( + &self, + duration: Duration, + status: Option, + error: Option<&str>, + auth_header_attached: bool, + auth_header_name: Option<&str>, + retry_after_unauthorized: bool, + recovery_mode: Option<&str>, + recovery_phase: Option<&str>, + endpoint: &str, + connection_reused: bool, + request_id: Option<&str>, + cf_ray: Option<&str>, + auth_error: Option<&str>, + auth_error_code: Option<&str>, + ) { + let success = error.is_none() + && status + .map(|code| (200..=299).contains(&code)) + .unwrap_or(true); + let success_str = if success { "true" } else { "false" }; + log_and_trace_event!( + self, + common: { + event.name = "codex.websocket_connect", + duration_ms = %duration.as_millis(), + http.response.status_code = status, + success = success_str, + error.message = error, + auth.header_attached = auth_header_attached, + auth.header_name = auth_header_name, + auth.retry_after_unauthorized = retry_after_unauthorized, + auth.recovery_mode = recovery_mode, + auth.recovery_phase = recovery_phase, + endpoint = endpoint, + auth.connection_reused = connection_reused, + auth.request_id = request_id, + auth.cf_ray = cf_ray, + auth.error = auth_error, + auth.error_code = auth_error_code, + }, + log: {}, + trace: {}, + ); + } + + pub fn record_websocket_request( + &self, + duration: Duration, + error: Option<&str>, + connection_reused: bool, + ) { let success_str = if error.is_none() { "true" } else { "false" }; self.counter( WEBSOCKET_REQUEST_COUNT_METRIC, @@ -400,6 +489,39 @@ impl SessionTelemetry { duration_ms = %duration.as_millis(), success = success_str, error.message = error, + auth.connection_reused = connection_reused, + }, + log: {}, + trace: {}, + ); + } + + #[allow(clippy::too_many_arguments)] + pub fn record_auth_recovery( + &self, + mode: &str, + step: &str, + outcome: &str, + request_id: Option<&str>, + cf_ray: Option<&str>, + auth_error: Option<&str>, + auth_error_code: Option<&str>, + recovery_reason: Option<&str>, + auth_state_changed: Option, + ) { + log_and_trace_event!( + self, + common: { + event.name = "codex.auth_recovery", + auth.mode = mode, + auth.step = step, + auth.outcome = outcome, + auth.request_id = request_id, + auth.cf_ray = cf_ray, + auth.error = auth_error, + auth.error_code = auth_error_code, + auth.recovery_reason = recovery_reason, + auth.state_changed = auth_state_changed, }, log: {}, trace: {}, diff --git a/codex-rs/otel/tests/suite/otel_export_routing_policy.rs b/codex-rs/otel/tests/suite/otel_export_routing_policy.rs index 317c6a691c..df5d876b4b 100644 --- a/codex-rs/otel/tests/suite/otel_export_routing_policy.rs +++ b/codex-rs/otel/tests/suite/otel_export_routing_policy.rs @@ -297,3 +297,462 @@ fn otel_export_routing_policy_routes_tool_result_log_and_trace_events() { assert!(!tool_trace_attrs.contains_key("mcp_server")); assert!(!tool_trace_attrs.contains_key("mcp_server_origin")); } + +#[test] +fn otel_export_routing_policy_routes_auth_recovery_log_and_trace_events() { + let log_exporter = InMemoryLogExporter::default(); + let logger_provider = SdkLoggerProvider::builder() + .with_simple_exporter(log_exporter.clone()) + .build(); + let span_exporter = InMemorySpanExporter::default(); + let tracer_provider = SdkTracerProvider::builder() + .with_simple_exporter(span_exporter.clone()) + .build(); + let tracer = tracer_provider.tracer("sink-split-test"); + + let subscriber = tracing_subscriber::registry() + .with( + opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge::new( + &logger_provider, + ) + .with_filter(filter_fn(OtelProvider::log_export_filter)), + ) + .with( + tracing_opentelemetry::layer() + .with_tracer(tracer) + .with_filter(filter_fn(OtelProvider::trace_export_filter)), + ); + + tracing::subscriber::with_default(subscriber, || { + tracing::callsite::rebuild_interest_cache(); + let manager = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.1", + "gpt-5.1", + Some("account-id".to_string()), + Some("engineer@example.com".to_string()), + Some(TelemetryAuthMode::Chatgpt), + "codex_exec".to_string(), + true, + "tty".to_string(), + SessionSource::Cli, + ); + let root_span = tracing::info_span!("root"); + let _root_guard = root_span.enter(); + manager.record_auth_recovery( + "managed", + "reload", + "recovery_succeeded", + Some("req-401"), + Some("ray-401"), + Some("missing_authorization_header"), + Some("token_expired"), + None, + Some(true), + ); + }); + + logger_provider.force_flush().expect("flush logs"); + tracer_provider.force_flush().expect("flush traces"); + + let logs = log_exporter.get_emitted_logs().expect("log export"); + let recovery_log = find_log_by_event_name(&logs, "codex.auth_recovery"); + let recovery_log_attrs = log_attributes(&recovery_log.record); + assert_eq!( + recovery_log_attrs.get("auth.mode").map(String::as_str), + Some("managed") + ); + assert_eq!( + recovery_log_attrs.get("auth.step").map(String::as_str), + Some("reload") + ); + assert_eq!( + recovery_log_attrs.get("auth.outcome").map(String::as_str), + Some("recovery_succeeded") + ); + assert_eq!( + recovery_log_attrs + .get("auth.request_id") + .map(String::as_str), + Some("req-401") + ); + assert_eq!( + recovery_log_attrs.get("auth.cf_ray").map(String::as_str), + Some("ray-401") + ); + assert_eq!( + recovery_log_attrs.get("auth.error").map(String::as_str), + Some("missing_authorization_header") + ); + assert_eq!( + recovery_log_attrs + .get("auth.error_code") + .map(String::as_str), + Some("token_expired") + ); + assert_eq!( + recovery_log_attrs + .get("auth.state_changed") + .map(String::as_str), + Some("true") + ); + + let spans = span_exporter.get_finished_spans().expect("span export"); + assert_eq!(spans.len(), 1); + let span_events = &spans[0].events.events; + assert_eq!(span_events.len(), 1); + + let recovery_trace_event = find_span_event_by_name_attr(span_events, "codex.auth_recovery"); + let recovery_trace_attrs = span_event_attributes(recovery_trace_event); + assert_eq!( + recovery_trace_attrs.get("auth.mode").map(String::as_str), + Some("managed") + ); + assert_eq!( + recovery_trace_attrs.get("auth.step").map(String::as_str), + Some("reload") + ); + assert_eq!( + recovery_trace_attrs.get("auth.outcome").map(String::as_str), + Some("recovery_succeeded") + ); + assert_eq!( + recovery_trace_attrs + .get("auth.request_id") + .map(String::as_str), + Some("req-401") + ); + assert_eq!( + recovery_trace_attrs.get("auth.cf_ray").map(String::as_str), + Some("ray-401") + ); + assert_eq!( + recovery_trace_attrs.get("auth.error").map(String::as_str), + Some("missing_authorization_header") + ); + assert_eq!( + recovery_trace_attrs + .get("auth.error_code") + .map(String::as_str), + Some("token_expired") + ); + assert_eq!( + recovery_trace_attrs + .get("auth.state_changed") + .map(String::as_str), + Some("true") + ); +} + +#[test] +fn otel_export_routing_policy_routes_api_request_auth_observability() { + let log_exporter = InMemoryLogExporter::default(); + let logger_provider = SdkLoggerProvider::builder() + .with_simple_exporter(log_exporter.clone()) + .build(); + let span_exporter = InMemorySpanExporter::default(); + let tracer_provider = SdkTracerProvider::builder() + .with_simple_exporter(span_exporter.clone()) + .build(); + let tracer = tracer_provider.tracer("sink-split-test"); + + let subscriber = tracing_subscriber::registry() + .with( + opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge::new( + &logger_provider, + ) + .with_filter(filter_fn(OtelProvider::log_export_filter)), + ) + .with( + tracing_opentelemetry::layer() + .with_tracer(tracer) + .with_filter(filter_fn(OtelProvider::trace_export_filter)), + ); + + tracing::subscriber::with_default(subscriber, || { + tracing::callsite::rebuild_interest_cache(); + let manager = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.1", + "gpt-5.1", + Some("account-id".to_string()), + Some("engineer@example.com".to_string()), + Some(TelemetryAuthMode::Chatgpt), + "codex_exec".to_string(), + true, + "tty".to_string(), + SessionSource::Cli, + ); + let root_span = tracing::info_span!("root"); + let _root_guard = root_span.enter(); + manager.record_api_request( + 1, + Some(401), + Some("http 401"), + std::time::Duration::from_millis(42), + true, + Some("authorization"), + true, + Some("managed"), + Some("refresh_token"), + "/responses", + Some("req-401"), + Some("ray-401"), + Some("missing_authorization_header"), + Some("token_expired"), + ); + }); + + logger_provider.force_flush().expect("flush logs"); + tracer_provider.force_flush().expect("flush traces"); + + let logs = log_exporter.get_emitted_logs().expect("log export"); + let request_log = find_log_by_event_name(&logs, "codex.api_request"); + let request_log_attrs = log_attributes(&request_log.record); + assert_eq!( + request_log_attrs + .get("auth.header_attached") + .map(String::as_str), + Some("true") + ); + assert_eq!( + request_log_attrs + .get("auth.header_name") + .map(String::as_str), + Some("authorization") + ); + assert_eq!( + request_log_attrs + .get("auth.retry_after_unauthorized") + .map(String::as_str), + Some("true") + ); + assert_eq!( + request_log_attrs + .get("auth.recovery_mode") + .map(String::as_str), + Some("managed") + ); + assert_eq!( + request_log_attrs + .get("auth.recovery_phase") + .map(String::as_str), + Some("refresh_token") + ); + assert_eq!( + request_log_attrs.get("endpoint").map(String::as_str), + Some("/responses") + ); + assert_eq!( + request_log_attrs.get("auth.error").map(String::as_str), + Some("missing_authorization_header") + ); + + let spans = span_exporter.get_finished_spans().expect("span export"); + let request_trace_event = + find_span_event_by_name_attr(&spans[0].events.events, "codex.api_request"); + let request_trace_attrs = span_event_attributes(request_trace_event); + assert_eq!( + request_trace_attrs + .get("auth.header_attached") + .map(String::as_str), + Some("true") + ); + assert_eq!( + request_trace_attrs + .get("auth.header_name") + .map(String::as_str), + Some("authorization") + ); + assert_eq!( + request_trace_attrs + .get("auth.retry_after_unauthorized") + .map(String::as_str), + Some("true") + ); + assert_eq!( + request_trace_attrs.get("endpoint").map(String::as_str), + Some("/responses") + ); +} + +#[test] +fn otel_export_routing_policy_routes_websocket_connect_auth_observability() { + let log_exporter = InMemoryLogExporter::default(); + let logger_provider = SdkLoggerProvider::builder() + .with_simple_exporter(log_exporter.clone()) + .build(); + let span_exporter = InMemorySpanExporter::default(); + let tracer_provider = SdkTracerProvider::builder() + .with_simple_exporter(span_exporter.clone()) + .build(); + let tracer = tracer_provider.tracer("sink-split-test"); + + let subscriber = tracing_subscriber::registry() + .with( + opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge::new( + &logger_provider, + ) + .with_filter(filter_fn(OtelProvider::log_export_filter)), + ) + .with( + tracing_opentelemetry::layer() + .with_tracer(tracer) + .with_filter(filter_fn(OtelProvider::trace_export_filter)), + ); + + tracing::subscriber::with_default(subscriber, || { + tracing::callsite::rebuild_interest_cache(); + let manager = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.1", + "gpt-5.1", + Some("account-id".to_string()), + Some("engineer@example.com".to_string()), + Some(TelemetryAuthMode::Chatgpt), + "codex_exec".to_string(), + true, + "tty".to_string(), + SessionSource::Cli, + ); + let root_span = tracing::info_span!("root"); + let _root_guard = root_span.enter(); + manager.record_websocket_connect( + std::time::Duration::from_millis(17), + Some(401), + Some("http 401"), + true, + Some("authorization"), + true, + Some("managed"), + Some("reload"), + "/responses", + false, + Some("req-ws-401"), + Some("ray-ws-401"), + Some("missing_authorization_header"), + Some("token_expired"), + ); + }); + + logger_provider.force_flush().expect("flush logs"); + tracer_provider.force_flush().expect("flush traces"); + + let logs = log_exporter.get_emitted_logs().expect("log export"); + let connect_log = find_log_by_event_name(&logs, "codex.websocket_connect"); + let connect_log_attrs = log_attributes(&connect_log.record); + assert_eq!( + connect_log_attrs + .get("auth.header_attached") + .map(String::as_str), + Some("true") + ); + assert_eq!( + connect_log_attrs + .get("auth.header_name") + .map(String::as_str), + Some("authorization") + ); + assert_eq!( + connect_log_attrs.get("auth.error").map(String::as_str), + Some("missing_authorization_header") + ); + assert_eq!( + connect_log_attrs.get("endpoint").map(String::as_str), + Some("/responses") + ); + assert_eq!( + connect_log_attrs + .get("auth.connection_reused") + .map(String::as_str), + Some("false") + ); + + let spans = span_exporter.get_finished_spans().expect("span export"); + let connect_trace_event = + find_span_event_by_name_attr(&spans[0].events.events, "codex.websocket_connect"); + let connect_trace_attrs = span_event_attributes(connect_trace_event); + assert_eq!( + connect_trace_attrs + .get("auth.recovery_phase") + .map(String::as_str), + Some("reload") + ); +} + +#[test] +fn otel_export_routing_policy_routes_websocket_request_transport_observability() { + let log_exporter = InMemoryLogExporter::default(); + let logger_provider = SdkLoggerProvider::builder() + .with_simple_exporter(log_exporter.clone()) + .build(); + let span_exporter = InMemorySpanExporter::default(); + let tracer_provider = SdkTracerProvider::builder() + .with_simple_exporter(span_exporter.clone()) + .build(); + let tracer = tracer_provider.tracer("sink-split-test"); + + let subscriber = tracing_subscriber::registry() + .with( + opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge::new( + &logger_provider, + ) + .with_filter(filter_fn(OtelProvider::log_export_filter)), + ) + .with( + tracing_opentelemetry::layer() + .with_tracer(tracer) + .with_filter(filter_fn(OtelProvider::trace_export_filter)), + ); + + tracing::subscriber::with_default(subscriber, || { + tracing::callsite::rebuild_interest_cache(); + let manager = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.1", + "gpt-5.1", + Some("account-id".to_string()), + Some("engineer@example.com".to_string()), + Some(TelemetryAuthMode::Chatgpt), + "codex_exec".to_string(), + true, + "tty".to_string(), + SessionSource::Cli, + ); + let root_span = tracing::info_span!("root"); + let _root_guard = root_span.enter(); + manager.record_websocket_request( + std::time::Duration::from_millis(23), + Some("stream error"), + true, + ); + }); + + logger_provider.force_flush().expect("flush logs"); + tracer_provider.force_flush().expect("flush traces"); + + let logs = log_exporter.get_emitted_logs().expect("log export"); + let request_log = find_log_by_event_name(&logs, "codex.websocket_request"); + let request_log_attrs = log_attributes(&request_log.record); + assert_eq!( + request_log_attrs + .get("auth.connection_reused") + .map(String::as_str), + Some("true") + ); + assert_eq!( + request_log_attrs.get("error.message").map(String::as_str), + Some("stream error") + ); + + let spans = span_exporter.get_finished_spans().expect("span export"); + let request_trace_event = + find_span_event_by_name_attr(&spans[0].events.events, "codex.websocket_request"); + let request_trace_attrs = span_event_attributes(request_trace_event); + assert_eq!( + request_trace_attrs + .get("auth.connection_reused") + .map(String::as_str), + Some("true") + ); +} diff --git a/codex-rs/otel/tests/suite/runtime_summary.rs b/codex-rs/otel/tests/suite/runtime_summary.rs index c2f252381f..778ed05783 100644 --- a/codex-rs/otel/tests/suite/runtime_summary.rs +++ b/codex-rs/otel/tests/suite/runtime_summary.rs @@ -47,8 +47,23 @@ fn runtime_metrics_summary_collects_tool_api_and_streaming_metrics() -> Result<( None, None, ); - manager.record_api_request(1, Some(200), None, Duration::from_millis(300)); - manager.record_websocket_request(Duration::from_millis(400), None); + manager.record_api_request( + 1, + Some(200), + None, + Duration::from_millis(300), + false, + None, + false, + None, + None, + "/responses", + None, + None, + None, + None, + ); + manager.record_websocket_request(Duration::from_millis(400), None, false); let sse_response: std::result::Result< Option>>, tokio::time::error::Elapsed, diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 6c2e7d336f..a485a9c16f 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -378,6 +378,7 @@ impl FileSystemSandboxPolicy { .filter(|entry| self.can_read_path_with_cwd(entry.path.as_path(), cwd)) .map(|entry| entry.path) .collect(), + /*normalize_effective_paths*/ true, ) } @@ -389,39 +390,96 @@ impl FileSystemSandboxPolicy { } let resolved_entries = self.resolved_entries_with_cwd(cwd); - let read_only_roots = dedup_absolute_paths( - resolved_entries - .iter() - .filter(|entry| !entry.access.can_write()) - .filter(|entry| !self.can_write_path_with_cwd(entry.path.as_path(), cwd)) - .map(|entry| entry.path.clone()) - .collect(), - ); + let writable_entries: Vec = resolved_entries + .iter() + .filter(|entry| entry.access.can_write()) + .filter(|entry| self.can_write_path_with_cwd(entry.path.as_path(), cwd)) + .map(|entry| entry.path.clone()) + .collect(); dedup_absolute_paths( - resolved_entries - .into_iter() - .filter(|entry| entry.access.can_write()) - .filter(|entry| self.can_write_path_with_cwd(entry.path.as_path(), cwd)) - .map(|entry| entry.path) - .collect(), + writable_entries.clone(), + /*normalize_effective_paths*/ true, ) .into_iter() .map(|root| { + // Filesystem-root policies stay in their effective canonical form + // so root-wide aliases do not create duplicate top-level masks. + // Example: keep `/var/...` normalized under `/` instead of + // materializing both `/var/...` and `/private/var/...`. + let preserve_raw_carveout_paths = root.as_path().parent().is_some(); + let raw_writable_roots: Vec<&AbsolutePathBuf> = writable_entries + .iter() + .filter(|path| normalize_effective_absolute_path((*path).clone()) == root) + .collect(); let mut read_only_subpaths = default_read_only_subpaths_for_writable_root(&root); // Narrower explicit non-write entries carve out broader writable roots. // More specific write entries still remain writable because they appear // as separate WritableRoot values and are checked independently. + // Preserve symlink path components that live under the writable root + // so downstream sandboxes can still mask the symlink inode itself. + // Example: if `/.codex -> /decoy`, bwrap must still see + // `/.codex`, not only the resolved `/decoy`. read_only_subpaths.extend( - read_only_roots + resolved_entries .iter() - .filter(|path| path.as_path() != root.as_path()) - .filter(|path| path.as_path().starts_with(root.as_path())) - .cloned(), + .filter(|entry| !entry.access.can_write()) + .filter(|entry| !self.can_write_path_with_cwd(entry.path.as_path(), cwd)) + .filter_map(|entry| { + let effective_path = normalize_effective_absolute_path(entry.path.clone()); + // Preserve the literal in-root path whenever the + // carveout itself lives under this writable root, even + // if following symlinks would resolve back to the root + // or escape outside it. Downstream sandboxes need that + // raw path so they can mask the symlink inode itself. + // Examples: + // - `/linked-private -> /decoy-private` + // - `/linked-private -> /tmp/outside-private` + // - `/alias-root -> ` + let raw_carveout_path = if preserve_raw_carveout_paths { + if entry.path == root { + None + } else if entry.path.as_path().starts_with(root.as_path()) { + Some(entry.path.clone()) + } else { + raw_writable_roots.iter().find_map(|raw_root| { + let suffix = entry + .path + .as_path() + .strip_prefix(raw_root.as_path()) + .ok()?; + if suffix.as_os_str().is_empty() { + return None; + } + root.join(suffix).ok() + }) + } + } else { + None + }; + + if let Some(raw_carveout_path) = raw_carveout_path { + return Some(raw_carveout_path); + } + + if effective_path == root + || !effective_path.as_path().starts_with(root.as_path()) + { + return None; + } + + Some(effective_path) + }), ); WritableRoot { root, - read_only_subpaths: dedup_absolute_paths(read_only_subpaths), + // Preserve literal in-root protected paths like `.git` and + // `.codex` so downstream sandboxes can still detect and mask + // the symlink itself instead of only its resolved target. + read_only_subpaths: dedup_absolute_paths( + read_only_subpaths, + /*normalize_effective_paths*/ false, + ), } }) .collect() @@ -448,6 +506,7 @@ impl FileSystemSandboxPolicy { .filter(|entry| root.as_ref() != Some(&entry.path)) .map(|entry| entry.path.clone()) .collect(), + /*normalize_effective_paths*/ true, ) } @@ -580,13 +639,19 @@ impl FileSystemSandboxPolicy { } else { ReadOnlyAccess::Restricted { include_platform_defaults, - readable_roots: dedup_absolute_paths(readable_roots), + readable_roots: dedup_absolute_paths( + readable_roots, + /*normalize_effective_paths*/ false, + ), } }; if workspace_root_writable { SandboxPolicy::WorkspaceWrite { - writable_roots: dedup_absolute_paths(writable_roots), + writable_roots: dedup_absolute_paths( + writable_roots, + /*normalize_effective_paths*/ false, + ), read_only_access, network_access: network_policy.is_enabled(), exclude_tmpdir_env_var: !tmpdir_writable, @@ -922,17 +987,43 @@ fn resolve_file_system_special_path( } } -fn dedup_absolute_paths(paths: Vec) -> Vec { +fn dedup_absolute_paths( + paths: Vec, + normalize_effective_paths: bool, +) -> Vec { let mut deduped = Vec::with_capacity(paths.len()); let mut seen = HashSet::new(); for path in paths { - if seen.insert(path.to_path_buf()) { - deduped.push(path); + let dedup_path = if normalize_effective_paths { + normalize_effective_absolute_path(path) + } else { + path + }; + if seen.insert(dedup_path.to_path_buf()) { + deduped.push(dedup_path); } } deduped } +fn normalize_effective_absolute_path(path: AbsolutePathBuf) -> AbsolutePathBuf { + let raw_path = path.to_path_buf(); + for ancestor in raw_path.ancestors() { + let Ok(canonical_ancestor) = ancestor.canonicalize() else { + continue; + }; + let Ok(suffix) = raw_path.strip_prefix(ancestor) else { + continue; + }; + if let Ok(normalized_path) = + AbsolutePathBuf::from_absolute_path(canonical_ancestor.join(suffix)) + { + return normalized_path; + } + } + path +} + fn default_read_only_subpaths_for_writable_root( writable_root: &AbsolutePathBuf, ) -> Vec { @@ -966,7 +1057,7 @@ fn default_read_only_subpaths_for_writable_root( } } - dedup_absolute_paths(subpaths) + dedup_absolute_paths(subpaths, /*normalize_effective_paths*/ false) } fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool { @@ -1038,8 +1129,19 @@ fn resolve_gitdir_from_file(dot_git: &AbsolutePathBuf) -> Option std::io::Result<()> { + std::os::unix::fs::symlink(original, link) + } + #[test] fn unknown_special_paths_are_ignored_by_legacy_bridge() -> std::io::Result<()> { let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { @@ -1067,6 +1169,333 @@ mod tests { Ok(()) } + #[cfg(unix)] + #[test] + fn effective_runtime_roots_canonicalize_symlinked_paths() { + let cwd = TempDir::new().expect("tempdir"); + let real_root = cwd.path().join("real"); + let link_root = cwd.path().join("link"); + let blocked = real_root.join("blocked"); + let codex_dir = real_root.join(".codex"); + + fs::create_dir_all(&blocked).expect("create blocked"); + fs::create_dir_all(&codex_dir).expect("create .codex"); + symlink_dir(&real_root, &link_root).expect("create symlinked root"); + + let link_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let link_blocked = link_root.join("blocked").expect("symlinked blocked path"); + let expected_root = AbsolutePathBuf::from_absolute_path( + real_root.canonicalize().expect("canonicalize real root"), + ) + .expect("absolute canonical root"); + let expected_blocked = AbsolutePathBuf::from_absolute_path( + blocked.canonicalize().expect("canonicalize blocked"), + ) + .expect("absolute canonical blocked"); + let expected_codex = AbsolutePathBuf::from_absolute_path( + codex_dir.canonicalize().expect("canonicalize .codex"), + ) + .expect("absolute canonical .codex"); + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_root }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_blocked }, + access: FileSystemAccessMode::None, + }, + ]); + + assert_eq!( + policy.get_unreadable_roots_with_cwd(cwd.path()), + vec![expected_blocked.clone()] + ); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!(writable_roots[0].root, expected_root); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_blocked) + ); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_codex) + ); + } + + #[cfg(unix)] + #[test] + fn writable_roots_preserve_symlinked_protected_subpaths() { + let cwd = TempDir::new().expect("tempdir"); + let root = cwd.path().join("root"); + let decoy = root.join("decoy-codex"); + let dot_codex = root.join(".codex"); + fs::create_dir_all(&decoy).expect("create decoy"); + symlink_dir(&decoy, &dot_codex).expect("create .codex symlink"); + + let root = AbsolutePathBuf::from_absolute_path(&root).expect("absolute root"); + let expected_dot_codex = AbsolutePathBuf::from_absolute_path( + root.as_path() + .canonicalize() + .expect("canonicalize root") + .join(".codex"), + ) + .expect("absolute .codex symlink"); + let unexpected_decoy = + AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy")) + .expect("absolute canonical decoy"); + + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { path: root }, + access: FileSystemAccessMode::Write, + }]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!( + writable_roots[0].read_only_subpaths, + vec![expected_dot_codex] + ); + assert!( + !writable_roots[0] + .read_only_subpaths + .contains(&unexpected_decoy) + ); + } + + #[cfg(unix)] + #[test] + fn writable_roots_preserve_explicit_symlinked_carveouts_under_symlinked_roots() { + let cwd = TempDir::new().expect("tempdir"); + let real_root = cwd.path().join("real"); + let link_root = cwd.path().join("link"); + let decoy = real_root.join("decoy-private"); + let linked_private = real_root.join("linked-private"); + fs::create_dir_all(&decoy).expect("create decoy"); + symlink_dir(&real_root, &link_root).expect("create symlinked root"); + symlink_dir(&decoy, &linked_private).expect("create linked-private symlink"); + + let link_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let link_private = link_root + .join("linked-private") + .expect("symlinked linked-private path"); + let expected_root = AbsolutePathBuf::from_absolute_path( + real_root.canonicalize().expect("canonicalize real root"), + ) + .expect("absolute canonical root"); + let expected_linked_private = expected_root + .join("linked-private") + .expect("expected linked-private path"); + let unexpected_decoy = + AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy")) + .expect("absolute canonical decoy"); + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_root }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_private }, + access: FileSystemAccessMode::None, + }, + ]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!(writable_roots[0].root, expected_root); + assert_eq!( + writable_roots[0].read_only_subpaths, + vec![expected_linked_private] + ); + assert!( + !writable_roots[0] + .read_only_subpaths + .contains(&unexpected_decoy) + ); + } + + #[cfg(unix)] + #[test] + fn writable_roots_preserve_explicit_symlinked_carveouts_that_escape_root() { + let cwd = TempDir::new().expect("tempdir"); + let real_root = cwd.path().join("real"); + let link_root = cwd.path().join("link"); + let decoy = cwd.path().join("outside-private"); + let linked_private = real_root.join("linked-private"); + fs::create_dir_all(&decoy).expect("create decoy"); + fs::create_dir_all(&real_root).expect("create real root"); + symlink_dir(&real_root, &link_root).expect("create symlinked root"); + symlink_dir(&decoy, &linked_private).expect("create linked-private symlink"); + + let link_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let link_private = link_root + .join("linked-private") + .expect("symlinked linked-private path"); + let expected_root = AbsolutePathBuf::from_absolute_path( + real_root.canonicalize().expect("canonicalize real root"), + ) + .expect("absolute canonical root"); + let expected_linked_private = expected_root + .join("linked-private") + .expect("expected linked-private path"); + let unexpected_decoy = + AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy")) + .expect("absolute canonical decoy"); + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_root }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_private }, + access: FileSystemAccessMode::None, + }, + ]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!(writable_roots[0].root, expected_root); + assert_eq!( + writable_roots[0].read_only_subpaths, + vec![expected_linked_private] + ); + assert!( + !writable_roots[0] + .read_only_subpaths + .contains(&unexpected_decoy) + ); + } + + #[cfg(unix)] + #[test] + fn writable_roots_preserve_explicit_symlinked_carveouts_that_alias_root() { + let cwd = TempDir::new().expect("tempdir"); + let root = cwd.path().join("root"); + let alias = root.join("alias-root"); + fs::create_dir_all(&root).expect("create root"); + symlink_dir(&root, &alias).expect("create alias symlink"); + + let root = AbsolutePathBuf::from_absolute_path(&root).expect("absolute root"); + let alias = root.join("alias-root").expect("alias root path"); + let expected_root = AbsolutePathBuf::from_absolute_path( + root.as_path().canonicalize().expect("canonicalize root"), + ) + .expect("absolute canonical root"); + let expected_alias = expected_root + .join("alias-root") + .expect("expected alias path"); + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: root }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: alias }, + access: FileSystemAccessMode::None, + }, + ]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!(writable_roots[0].root, expected_root); + assert_eq!(writable_roots[0].read_only_subpaths, vec![expected_alias]); + } + + #[cfg(unix)] + #[test] + fn tmpdir_special_path_canonicalizes_symlinked_tmpdir() { + if std::env::var_os(SYMLINKED_TMPDIR_TEST_ENV).is_none() { + let output = std::process::Command::new(std::env::current_exe().expect("test binary")) + .env(SYMLINKED_TMPDIR_TEST_ENV, "1") + .arg("--exact") + .arg("permissions::tests::tmpdir_special_path_canonicalizes_symlinked_tmpdir") + .output() + .expect("run tmpdir subprocess test"); + + assert!( + output.status.success(), + "tmpdir subprocess test failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let cwd = TempDir::new().expect("tempdir"); + let real_tmpdir = cwd.path().join("real-tmpdir"); + let link_tmpdir = cwd.path().join("link-tmpdir"); + let blocked = real_tmpdir.join("blocked"); + let codex_dir = real_tmpdir.join(".codex"); + + fs::create_dir_all(&blocked).expect("create blocked"); + fs::create_dir_all(&codex_dir).expect("create .codex"); + symlink_dir(&real_tmpdir, &link_tmpdir).expect("create symlinked tmpdir"); + + let link_blocked = + AbsolutePathBuf::from_absolute_path(link_tmpdir.join("blocked")).expect("link blocked"); + let expected_root = AbsolutePathBuf::from_absolute_path( + real_tmpdir + .canonicalize() + .expect("canonicalize real tmpdir"), + ) + .expect("absolute canonical tmpdir"); + let expected_blocked = AbsolutePathBuf::from_absolute_path( + blocked.canonicalize().expect("canonicalize blocked"), + ) + .expect("absolute canonical blocked"); + let expected_codex = AbsolutePathBuf::from_absolute_path( + codex_dir.canonicalize().expect("canonicalize .codex"), + ) + .expect("absolute canonical .codex"); + + unsafe { + std::env::set_var("TMPDIR", &link_tmpdir); + } + + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_blocked }, + access: FileSystemAccessMode::None, + }, + ]); + + assert_eq!( + policy.get_unreadable_roots_with_cwd(cwd.path()), + vec![expected_blocked.clone()] + ); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!(writable_roots[0].root, expected_root); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_blocked) + ); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_codex) + ); + } + #[test] fn resolve_access_with_cwd_uses_most_specific_entry() { let cwd = TempDir::new().expect("tempdir"); @@ -1183,6 +1612,13 @@ mod tests { let cwd = TempDir::new().expect("tempdir"); let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()).expect("resolve docs"); + let expected_docs = AbsolutePathBuf::from_absolute_path( + cwd.path() + .canonicalize() + .expect("canonicalize cwd") + .join("docs"), + ) + .expect("canonical docs"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { @@ -1200,7 +1636,10 @@ mod tests { policy.resolve_access_with_cwd(docs.as_path(), cwd.path()), FileSystemAccessMode::Read ); - assert_eq!(policy.get_readable_roots_with_cwd(cwd.path()), vec![docs]); + assert_eq!( + policy.get_readable_roots_with_cwd(cwd.path()), + vec![expected_docs] + ); assert!(policy.get_unreadable_roots_with_cwd(cwd.path()).is_empty()); } diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 4d6197a65c..8ac5242dc5 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -3681,9 +3681,9 @@ mod tests { #[test] fn restricted_file_system_policy_treats_root_with_carveouts_as_scoped_access() { let cwd = TempDir::new().expect("tempdir"); - let cwd_absolute = - AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute tempdir"); - let root = cwd_absolute + let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); + let root = AbsolutePathBuf::from_absolute_path(&canonical_cwd) + .expect("absolute canonical tempdir") .as_path() .ancestors() .last() @@ -3691,6 +3691,13 @@ mod tests { .expect("filesystem root"); let blocked = AbsolutePathBuf::resolve_path_against_base("blocked", cwd.path()) .expect("resolve blocked"); + let expected_blocked = AbsolutePathBuf::from_absolute_path( + cwd.path() + .canonicalize() + .expect("canonicalize cwd") + .join("blocked"), + ) + .expect("canonical blocked"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { @@ -3699,9 +3706,7 @@ mod tests { access: FileSystemAccessMode::Write, }, FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: blocked.clone(), - }, + path: FileSystemPath::Path { path: blocked }, access: FileSystemAccessMode::None, }, ]); @@ -3714,7 +3719,7 @@ mod tests { ); assert_eq!( policy.get_unreadable_roots_with_cwd(cwd.path()), - vec![blocked.clone()] + vec![expected_blocked.clone()] ); let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); @@ -3724,7 +3729,7 @@ mod tests { writable_roots[0] .read_only_subpaths .iter() - .any(|path| path.as_path() == blocked.as_path()) + .any(|path| path.as_path() == expected_blocked.as_path()) ); } @@ -3733,14 +3738,17 @@ mod tests { let cwd = TempDir::new().expect("tempdir"); std::fs::create_dir_all(cwd.path().join(".agents")).expect("create .agents"); std::fs::create_dir_all(cwd.path().join(".codex")).expect("create .codex"); + let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); let cwd_absolute = - AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute tempdir"); + AbsolutePathBuf::from_absolute_path(&canonical_cwd).expect("absolute tempdir"); let secret = AbsolutePathBuf::resolve_path_against_base("secret", cwd.path()) .expect("resolve unreadable path"); - let agents = AbsolutePathBuf::resolve_path_against_base(".agents", cwd.path()) - .expect("resolve .agents"); - let codex = AbsolutePathBuf::resolve_path_against_base(".codex", cwd.path()) - .expect("resolve .codex"); + let expected_secret = AbsolutePathBuf::from_absolute_path(canonical_cwd.join("secret")) + .expect("canonical secret"); + let expected_agents = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".agents")) + .expect("canonical .agents"); + let expected_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex")) + .expect("canonical .codex"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { @@ -3755,9 +3763,7 @@ mod tests { access: FileSystemAccessMode::Write, }, FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: secret.clone(), - }, + path: FileSystemPath::Path { path: secret }, access: FileSystemAccessMode::None, }, ]); @@ -3767,43 +3773,49 @@ mod tests { assert!(policy.include_platform_defaults()); assert_eq!( policy.get_readable_roots_with_cwd(cwd.path()), - vec![cwd_absolute] + vec![cwd_absolute.clone()] ); assert_eq!( policy.get_unreadable_roots_with_cwd(cwd.path()), - vec![secret.clone()] + vec![expected_secret.clone()] ); let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); assert_eq!(writable_roots.len(), 1); - assert_eq!(writable_roots[0].root.as_path(), cwd.path()); + assert_eq!(writable_roots[0].root, cwd_absolute); assert!( writable_roots[0] .read_only_subpaths .iter() - .any(|path| path.as_path() == secret.as_path()) + .any(|path| path.as_path() == expected_secret.as_path()) ); assert!( writable_roots[0] .read_only_subpaths .iter() - .any(|path| path.as_path() == agents.as_path()) + .any(|path| path.as_path() == expected_agents.as_path()) ); assert!( writable_roots[0] .read_only_subpaths .iter() - .any(|path| path.as_path() == codex.as_path()) + .any(|path| path.as_path() == expected_codex.as_path()) ); } #[test] fn restricted_file_system_policy_treats_read_entries_as_read_only_subpaths() { let cwd = TempDir::new().expect("tempdir"); + let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()).expect("resolve docs"); let docs_public = AbsolutePathBuf::resolve_path_against_base("docs/public", cwd.path()) .expect("resolve docs/public"); + let expected_docs = AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs")) + .expect("canonical docs"); + let expected_docs_public = + AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs/public")) + .expect("canonical docs/public"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { @@ -3812,13 +3824,11 @@ mod tests { access: FileSystemAccessMode::Write, }, FileSystemSandboxEntry { - path: FileSystemPath::Path { path: docs.clone() }, + path: FileSystemPath::Path { path: docs }, access: FileSystemAccessMode::Read, }, FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: docs_public.clone(), - }, + path: FileSystemPath::Path { path: docs_public }, access: FileSystemAccessMode::Write, }, ]); @@ -3827,8 +3837,8 @@ mod tests { assert_eq!( sorted_writable_roots(policy.get_writable_roots_with_cwd(cwd.path())), vec![ - (cwd.path().to_path_buf(), vec![docs.to_path_buf()]), - (docs_public.to_path_buf(), Vec::new()), + (canonical_cwd, vec![expected_docs.to_path_buf()]), + (expected_docs_public.to_path_buf(), Vec::new()), ] ); } @@ -3838,6 +3848,7 @@ mod tests { let cwd = TempDir::new().expect("tempdir"); let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()).expect("resolve docs"); + let canonical_cwd = cwd.path().canonicalize().expect("canonicalize cwd"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: ReadOnlyAccess::Restricted { @@ -3854,7 +3865,7 @@ mod tests { FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd.path()) .get_writable_roots_with_cwd(cwd.path()) ), - vec![(cwd.path().to_path_buf(), Vec::new())] + vec![(canonical_cwd, Vec::new())] ); } diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 97514ba20e..9b2c99ecc5 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -700,6 +700,7 @@ impl RmcpClient { &self, name: String, arguments: Option, + meta: Option, timeout: Option, ) -> Result { self.refresh_oauth_if_needed().await; @@ -712,8 +713,17 @@ impl RmcpClient { } None => None, }; + let meta = match meta { + Some(Value::Object(map)) => Some(rmcp::model::Meta(map)), + Some(other) => { + return Err(anyhow!( + "MCP tool request _meta must be a JSON object, got {other}" + )); + } + None => None, + }; let rmcp_params = CallToolRequestParams { - meta: None, + meta, name: name.into(), arguments, task: None, diff --git a/codex-rs/rmcp-client/tests/streamable_http_recovery.rs b/codex-rs/rmcp-client/tests/streamable_http_recovery.rs index 4710fdf78a..fb2fc96d20 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_recovery.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_recovery.rs @@ -105,6 +105,7 @@ async fn call_echo_tool(client: &RmcpClient, message: &str) -> anyhow::Result, retry_count: usize) -> String { + let _ = (base_url, retry_count); + String::new() +} +``` + +This is accepted: + +```rust +create_openai_url(/*base_url*/ None, /*retry_count*/ 3); +``` + +This is warned on by `argument_comment_mismatch`: + +```rust +create_openai_url(/*api_base*/ None, 3); +``` + +This is only warned on when `uncommented_anonymous_literal_argument` is enabled: + +```rust +create_openai_url(None, 3); +``` + +## Development + +Install the required tooling once: + +```bash +cargo install cargo-dylint dylint-link +rustup toolchain install nightly-2025-09-18 \ + --component llvm-tools-preview \ + --component rustc-dev \ + --component rust-src +``` + +Run the lint crate tests: + +```bash +cd tools/argument-comment-lint +cargo test +``` + +Run the lint against `codex-rs` from the repo root: + +```bash +./tools/argument-comment-lint/run.sh -p codex-core +just argument-comment-lint -p codex-core +``` + +If no package selection is provided, `run.sh` defaults to checking the +`codex-rs` workspace with `--workspace --no-deps`. + +Repo runs also promote `uncommented_anonymous_literal_argument` to an error by +default: + +```bash +./tools/argument-comment-lint/run.sh -p codex-core +``` + +The wrapper does that by setting `DYLINT_RUSTFLAGS`, and it leaves an explicit +existing setting alone. It also defaults `CARGO_INCREMENTAL=0` unless you have +already set it, because the current nightly Dylint flow can otherwise hit a +rustc incremental compilation ICE locally. To override that behavior for an ad +hoc run: + +```bash +DYLINT_RUSTFLAGS="-A uncommented_anonymous_literal_argument" \ +CARGO_INCREMENTAL=1 \ + ./tools/argument-comment-lint/run.sh -p codex-core +``` + +To expand target coverage for an ad hoc run: + +```bash +./tools/argument-comment-lint/run.sh -p codex-core -- --all-targets +``` diff --git a/tools/argument-comment-lint/run.sh b/tools/argument-comment-lint/run.sh new file mode 100755 index 0000000000..9452a6a85b --- /dev/null +++ b/tools/argument-comment-lint/run.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash + +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +lint_path="$repo_root/tools/argument-comment-lint" +manifest_path="$repo_root/codex-rs/Cargo.toml" +strict_lint="uncommented_anonymous_literal_argument" +noise_lint="unknown_lints" + +has_manifest_path=false +has_package_selection=false +has_no_deps=false +has_library_selection=false +expect_value="" + +for arg in "$@"; do + if [[ -n "$expect_value" ]]; then + case "$expect_value" in + manifest_path) + has_manifest_path=true + ;; + package_selection) + has_package_selection=true + ;; + library_selection) + has_library_selection=true + ;; + esac + expect_value="" + continue + fi + + case "$arg" in + --) + break + ;; + --manifest-path) + expect_value="manifest_path" + ;; + --manifest-path=*) + has_manifest_path=true + ;; + -p|--package) + expect_value="package_selection" + ;; + --package=*) + has_package_selection=true + ;; + --workspace) + has_package_selection=true + ;; + --no-deps) + has_no_deps=true + ;; + --lib|--lib-path) + expect_value="library_selection" + ;; + --lib=*|--lib-path=*) + has_library_selection=true + ;; + esac +done + +cmd=(cargo dylint --path "$lint_path") +if [[ "$has_library_selection" == false ]]; then + cmd+=(--all) +fi +if [[ "$has_manifest_path" == false ]]; then + cmd+=(--manifest-path "$manifest_path") +fi +if [[ "$has_package_selection" == false ]]; then + cmd+=(--workspace) +fi +if [[ "$has_no_deps" == false ]]; then + cmd+=(--no-deps) +fi +cmd+=("$@") + +if [[ "${DYLINT_RUSTFLAGS:-}" != *"$strict_lint"* ]]; then + export DYLINT_RUSTFLAGS="${DYLINT_RUSTFLAGS:+${DYLINT_RUSTFLAGS} }-D $strict_lint" +fi +if [[ "${DYLINT_RUSTFLAGS:-}" != *"$noise_lint"* ]]; then + export DYLINT_RUSTFLAGS="${DYLINT_RUSTFLAGS:+${DYLINT_RUSTFLAGS} }-A $noise_lint" +fi + +if [[ -z "${CARGO_INCREMENTAL:-}" ]]; then + export CARGO_INCREMENTAL=0 +fi + +exec "${cmd[@]}" diff --git a/tools/argument-comment-lint/rust-toolchain b/tools/argument-comment-lint/rust-toolchain new file mode 100644 index 0000000000..d159253fc0 --- /dev/null +++ b/tools/argument-comment-lint/rust-toolchain @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2025-09-18" +components = ["llvm-tools-preview", "rustc-dev", "rust-src"] diff --git a/tools/argument-comment-lint/src/comment_parser.rs b/tools/argument-comment-lint/src/comment_parser.rs new file mode 100644 index 0000000000..7ea66c195b --- /dev/null +++ b/tools/argument-comment-lint/src/comment_parser.rs @@ -0,0 +1,63 @@ +pub fn parse_argument_comment(text: &str) -> Option<&str> { + let trimmed = text.trim_end(); + let comment_start = trimmed.rfind("/*")?; + let comment = &trimmed[comment_start..]; + let name = comment.strip_prefix("/*")?.strip_suffix("*/")?; + is_identifier(name).then_some(name) +} + +pub fn parse_argument_comment_prefix(text: &str) -> Option<&str> { + let trimmed = text.trim_start(); + let comment = trimmed.strip_prefix("/*")?; + let (name, _) = comment.split_once("*/")?; + is_identifier(name).then_some(name) +} + +fn is_identifier(text: &str) -> bool { + let mut chars = text.chars(); + let Some(first) = chars.next() else { + return false; + }; + if !(first == '_' || first.is_ascii_alphabetic()) { + return false; + } + chars.all(|ch| ch == '_' || ch.is_ascii_alphanumeric()) +} + +#[cfg(test)] +mod tests { + use super::parse_argument_comment; + use super::parse_argument_comment_prefix; + + #[test] + fn parses_trailing_comment() { + assert_eq!(parse_argument_comment("(/*base_url*/ "), Some("base_url")); + assert_eq!( + parse_argument_comment(", /*timeout_ms*/ "), + Some("timeout_ms") + ); + assert_eq!( + parse_argument_comment(".method::(/*base_url*/ "), + Some("base_url") + ); + } + + #[test] + fn rejects_non_matching_shapes() { + assert_eq!(parse_argument_comment("(\n"), None); + assert_eq!(parse_argument_comment("(/* base_url*/ "), None); + assert_eq!(parse_argument_comment("(/*base_url */ "), None); + assert_eq!(parse_argument_comment("(/*base_url=*/ "), None); + assert_eq!(parse_argument_comment("(/*1base_url*/ "), None); + assert_eq!(parse_argument_comment_prefix("/*env=*/ None"), None); + } + + #[test] + fn parses_prefix_comment() { + assert_eq!(parse_argument_comment_prefix("/*env*/ None"), Some("env")); + assert_eq!( + parse_argument_comment_prefix("\n /*retry_count*/ 3"), + Some("retry_count") + ); + } +} diff --git a/tools/argument-comment-lint/src/lib.rs b/tools/argument-comment-lint/src/lib.rs new file mode 100644 index 0000000000..fcd3f1bcd0 --- /dev/null +++ b/tools/argument-comment-lint/src/lib.rs @@ -0,0 +1,273 @@ +#![feature(rustc_private)] + +mod comment_parser; + +extern crate rustc_ast; +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_lint; +extern crate rustc_middle; +extern crate rustc_session; +extern crate rustc_span; + +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::fn_def_id; +use clippy_utils::is_res_lang_ctor; +use clippy_utils::peel_blocks; +use clippy_utils::source::snippet; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::Expr; +use rustc_hir::ExprKind; +use rustc_hir::LangItem; +use rustc_hir::UnOp; +use rustc_hir::def::DefKind; +use rustc_lint::LateContext; +use rustc_lint::LateLintPass; +use rustc_span::BytePos; +use rustc_span::Span; + +use crate::comment_parser::parse_argument_comment; +use crate::comment_parser::parse_argument_comment_prefix; + +dylint_linting::dylint_library!(); + +#[unsafe(no_mangle)] +pub fn register_lints(_sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore) { + lint_store.register_lints(&[ + ARGUMENT_COMMENT_MISMATCH, + UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT, + ]); + lint_store.register_late_pass(|_| Box::new(ArgumentCommentLint)); +} + +rustc_session::declare_lint! { + /// ### What it does + /// + /// Checks `/*param*/` argument comments and verifies that the comment + /// matches the resolved callee parameter name. + /// + /// ### Why is this bad? + /// + /// A mismatched comment is worse than no comment because it actively + /// misleads the reader. + /// + /// ### Known problems + /// + /// This lint only runs when the callee resolves to a concrete function or + /// method with available parameter names. + /// + /// ### Example + /// + /// ```rust + /// fn create_openai_url(base_url: Option) -> String { + /// String::new() + /// } + /// + /// create_openai_url(/*api_base*/ None); + /// ``` + /// + /// Use instead: + /// + /// ```rust + /// fn create_openai_url(base_url: Option) -> String { + /// String::new() + /// } + /// + /// create_openai_url(/*base_url*/ None); + /// ``` + pub ARGUMENT_COMMENT_MISMATCH, + Warn, + "argument comment does not match the resolved parameter name" +} + +rustc_session::declare_lint! { + /// ### What it does + /// + /// Requires a `/*param*/` comment before anonymous literal-like + /// arguments such as `None`, booleans, and numeric literals. + /// + /// ### Why is this bad? + /// + /// Bare literal-like arguments make call sites harder to read because the + /// meaning of the value is hidden in the callee signature. + /// + /// ### Known problems + /// + /// This lint is opinionated, so it is `allow` by default. + /// + /// ### Example + /// + /// ```rust + /// fn create_openai_url(base_url: Option) -> String { + /// String::new() + /// } + /// + /// create_openai_url(None); + /// ``` + /// + /// Use instead: + /// + /// ```rust + /// fn create_openai_url(base_url: Option) -> String { + /// String::new() + /// } + /// + /// create_openai_url(/*base_url*/ None); + /// ``` + pub UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT, + Allow, + "anonymous literal-like argument is missing a `/*param*/` comment" +} + +#[derive(Default)] +pub struct ArgumentCommentLint; + +rustc_session::impl_lint_pass!( + ArgumentCommentLint => [ARGUMENT_COMMENT_MISMATCH, UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT] +); + +impl<'tcx> LateLintPass<'tcx> for ArgumentCommentLint { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if expr.span.from_expansion() { + return; + } + + match expr.kind { + ExprKind::Call(callee, args) => { + self.check_call(cx, expr, callee.span, args, 0); + } + ExprKind::MethodCall(_, receiver, args, _) => { + self.check_call(cx, expr, receiver.span, args, 1); + } + _ => {} + } + } +} + +impl ArgumentCommentLint { + fn check_call<'tcx>( + &self, + cx: &LateContext<'tcx>, + call: &'tcx Expr<'tcx>, + first_gap_anchor: Span, + args: &'tcx [Expr<'tcx>], + parameter_offset: usize, + ) { + let Some(def_id) = fn_def_id(cx, call) else { + return; + }; + if !def_id.is_local() && !is_workspace_crate_name(cx.tcx.crate_name(def_id.krate).as_str()) + { + return; + } + if !matches!(cx.tcx.def_kind(def_id), DefKind::Fn | DefKind::AssocFn) { + return; + } + + let parameter_names: Vec<_> = cx.tcx.fn_arg_idents(def_id).iter().copied().collect(); + for (index, arg) in args.iter().enumerate() { + if arg.span.from_expansion() { + continue; + } + + let Some(expected_name) = parameter_names.get(index + parameter_offset) else { + continue; + }; + let Some(expected_name) = expected_name else { + continue; + }; + let expected_name = expected_name.name.to_string(); + if !is_meaningful_parameter_name(&expected_name) { + continue; + } + + let boundary_span = if index == 0 { + first_gap_anchor + } else { + args[index - 1].span + }; + let gap_span = boundary_span.between(arg.span); + let gap_text = snippet(cx, gap_span, ""); + let arg_text = snippet(cx, arg.span, ".."); + let lookbehind_start = BytePos(arg.span.lo().0.saturating_sub(64)); + let lookbehind_text = + snippet(cx, arg.span.shrink_to_lo().with_lo(lookbehind_start), ""); + let argument_comment = parse_argument_comment(gap_text.as_ref()) + .or_else(|| parse_argument_comment(lookbehind_text.as_ref())) + .or_else(|| parse_argument_comment_prefix(arg_text.as_ref())); + + if let Some(actual_name) = argument_comment { + if actual_name != expected_name { + span_lint_and_help( + cx, + ARGUMENT_COMMENT_MISMATCH, + arg.span, + format!( + "argument comment `/*{actual_name}*/` does not match parameter `{expected_name}`" + ), + None, + format!("use `/*{expected_name}*/`"), + ); + } + continue; + } + + if !is_anonymous_literal_like(cx, arg) { + continue; + } + + span_lint_and_sugg( + cx, + UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT, + arg.span, + format!("anonymous literal-like argument for parameter `{expected_name}`"), + "prepend the parameter name comment", + format!("/*{expected_name}*/ {arg_text}"), + Applicability::MachineApplicable, + ); + } + } +} + +fn is_anonymous_literal_like(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let expr = peel_blocks(expr); + match expr.kind { + ExprKind::Lit(lit) => !matches!( + lit.node, + LitKind::Str(..) | LitKind::ByteStr(..) | LitKind::CStr(..) | LitKind::Char(..) + ), + ExprKind::Unary(UnOp::Neg, inner) => matches!(peel_blocks(inner).kind, ExprKind::Lit(_)), + ExprKind::Path(qpath) => { + is_res_lang_ctor(cx, cx.qpath_res(&qpath, expr.hir_id), LangItem::OptionNone) + } + _ => false, + } +} + +fn is_meaningful_parameter_name(name: &str) -> bool { + !name.is_empty() && !name.starts_with('_') +} + +fn is_workspace_crate_name(name: &str) -> bool { + name.starts_with("codex_") + || matches!( + name, + "app_test_support" | "core_test_support" | "mcp_test_support" + ) +} + +#[test] +fn ui() { + dylint_testing::ui_test(env!("CARGO_PKG_NAME"), "ui"); +} + +#[test] +fn workspace_crate_filter_accepts_first_party_names_only() { + assert!(is_workspace_crate_name("codex_core")); + assert!(is_workspace_crate_name("codex_tui")); + assert!(is_workspace_crate_name("core_test_support")); + assert!(!is_workspace_crate_name("std")); + assert!(!is_workspace_crate_name("tokio")); +} diff --git a/tools/argument-comment-lint/ui/allow_char_literals.rs b/tools/argument-comment-lint/ui/allow_char_literals.rs new file mode 100644 index 0000000000..85ef7d362f --- /dev/null +++ b/tools/argument-comment-lint/ui/allow_char_literals.rs @@ -0,0 +1,9 @@ +#![warn(uncommented_anonymous_literal_argument)] + +fn split_top_level(body: &str, delimiter: char) { + let _ = (body, delimiter); +} + +fn main() { + split_top_level("a|b|c", '|'); +} diff --git a/tools/argument-comment-lint/ui/allow_string_literals.rs b/tools/argument-comment-lint/ui/allow_string_literals.rs new file mode 100644 index 0000000000..9d61c4a761 --- /dev/null +++ b/tools/argument-comment-lint/ui/allow_string_literals.rs @@ -0,0 +1,9 @@ +#![warn(uncommented_anonymous_literal_argument)] + +fn describe(prefix: &str, suffix: &str) { + let _ = (prefix, suffix); +} + +fn main() { + describe("openai", r"https://api.openai.com/v1"); +} diff --git a/tools/argument-comment-lint/ui/comment_matches.rs b/tools/argument-comment-lint/ui/comment_matches.rs new file mode 100644 index 0000000000..f582fb958f --- /dev/null +++ b/tools/argument-comment-lint/ui/comment_matches.rs @@ -0,0 +1,12 @@ +#![warn(argument_comment_mismatch)] + +fn create_openai_url(base_url: Option, retry_count: usize) -> String { + let _ = (base_url, retry_count); + String::new() +} + +fn main() { + let base_url = Some(String::from("https://api.openai.com")); + create_openai_url(base_url, 3); + create_openai_url(/*base_url*/ None, 3); +} diff --git a/tools/argument-comment-lint/ui/comment_matches_multiline.rs b/tools/argument-comment-lint/ui/comment_matches_multiline.rs new file mode 100644 index 0000000000..45683841ff --- /dev/null +++ b/tools/argument-comment-lint/ui/comment_matches_multiline.rs @@ -0,0 +1,15 @@ +#![warn(argument_comment_mismatch)] +#![warn(uncommented_anonymous_literal_argument)] + +fn run_git_for_stdout(repo_root: &str, args: Vec<&str>, env: Option<&str>) -> String { + let _ = (repo_root, args, env); + String::new() +} + +fn main() { + let _ = run_git_for_stdout( + "/tmp/repo", + vec!["rev-parse", "HEAD"], + /*env*/ None, + ); +} diff --git a/tools/argument-comment-lint/ui/comment_mismatch.rs b/tools/argument-comment-lint/ui/comment_mismatch.rs new file mode 100644 index 0000000000..3eebdd9dc2 --- /dev/null +++ b/tools/argument-comment-lint/ui/comment_mismatch.rs @@ -0,0 +1,10 @@ +#![warn(argument_comment_mismatch)] + +fn create_openai_url(base_url: Option) -> String { + let _ = base_url; + String::new() +} + +fn main() { + let _ = create_openai_url(/*api_base*/ None); +} diff --git a/tools/argument-comment-lint/ui/comment_mismatch.stderr b/tools/argument-comment-lint/ui/comment_mismatch.stderr new file mode 100644 index 0000000000..6ede656029 --- /dev/null +++ b/tools/argument-comment-lint/ui/comment_mismatch.stderr @@ -0,0 +1,15 @@ +warning: argument comment `/*api_base*/` does not match parameter `base_url` + --> $DIR/comment_mismatch.rs:9:44 + | +LL | let _ = create_openai_url(/*api_base*/ None); + | ^^^^ + | + = help: use `/*base_url*/` +note: the lint level is defined here + --> $DIR/comment_mismatch.rs:1:9 + | +LL | #![warn(argument_comment_mismatch)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: 1 warning emitted + diff --git a/tools/argument-comment-lint/ui/ignore_external_methods.rs b/tools/argument-comment-lint/ui/ignore_external_methods.rs new file mode 100644 index 0000000000..44c72707d0 --- /dev/null +++ b/tools/argument-comment-lint/ui/ignore_external_methods.rs @@ -0,0 +1,9 @@ +#![warn(uncommented_anonymous_literal_argument)] + +fn main() { + let line = "{\"type\":\"response_item\"}"; + let _ = line.starts_with('{'); + let _ = line.find("type"); + let parts = ["type", "response_item"]; + let _ = parts.join("\n"); +} diff --git a/tools/argument-comment-lint/ui/uncommented_literal.rs b/tools/argument-comment-lint/ui/uncommented_literal.rs new file mode 100644 index 0000000000..a64804c780 --- /dev/null +++ b/tools/argument-comment-lint/ui/uncommented_literal.rs @@ -0,0 +1,18 @@ +#![warn(uncommented_anonymous_literal_argument)] + +struct Client; + +impl Client { + fn set_flag(&self, enabled: bool) {} +} + +fn create_openai_url(base_url: Option, retry_count: usize) -> String { + let _ = (base_url, retry_count); + String::new() +} + +fn main() { + let client = Client; + let _ = create_openai_url(None, 3); + client.set_flag(true); +} diff --git a/tools/argument-comment-lint/ui/uncommented_literal.stderr b/tools/argument-comment-lint/ui/uncommented_literal.stderr new file mode 100644 index 0000000000..a1060ef693 --- /dev/null +++ b/tools/argument-comment-lint/ui/uncommented_literal.stderr @@ -0,0 +1,26 @@ +warning: anonymous literal-like argument for parameter `base_url` + --> $DIR/uncommented_literal.rs:16:31 + | +LL | let _ = create_openai_url(None, 3); + | ^^^^ help: prepend the parameter name comment: `/*base_url*/ None` + | +note: the lint level is defined here + --> $DIR/uncommented_literal.rs:1:9 + | +LL | #![warn(uncommented_anonymous_literal_argument)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: anonymous literal-like argument for parameter `retry_count` + --> $DIR/uncommented_literal.rs:16:37 + | +LL | let _ = create_openai_url(None, 3); + | ^ help: prepend the parameter name comment: `/*retry_count*/ 3` + +warning: anonymous literal-like argument for parameter `enabled` + --> $DIR/uncommented_literal.rs:17:21 + | +LL | client.set_flag(true); + | ^^^^ help: prepend the parameter name comment: `/*enabled*/ true` + +warning: 3 warnings emitted +