diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 812108a15c..548bb5be87 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -160,6 +160,7 @@ impl McpRequestProcessor { http_headers, env_http_headers, &resolved_scopes.scopes, + server.oauth_client_id(), server.oauth_resource.as_deref(), timeout_secs, config.mcp_oauth_callback_port, diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index ea1ff3ead4..c290d2091a 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -1346,6 +1346,7 @@ impl PluginRequestProcessor { let notification_name = name.clone(); tokio::spawn(async move { + let oauth_client_id = server.oauth_client_id(); let first_attempt = perform_oauth_login_silent( &name, &oauth_config.url, @@ -1353,6 +1354,7 @@ impl PluginRequestProcessor { oauth_config.http_headers.clone(), oauth_config.env_http_headers.clone(), &resolved_scopes.scopes, + oauth_client_id, server.oauth_resource.as_deref(), callback_port, callback_url.as_deref(), @@ -1368,6 +1370,7 @@ impl PluginRequestProcessor { oauth_config.http_headers, oauth_config.env_http_headers, &[], + oauth_client_id, server.oauth_resource.as_deref(), callback_port, callback_url.as_deref(), diff --git a/codex-rs/app-server/tests/suite/v2/model_list.rs b/codex-rs/app-server/tests/suite/v2/model_list.rs index e2039d333a..d22bdd2a18 100644 --- a/codex-rs/app-server/tests/suite/v2/model_list.rs +++ b/codex-rs/app-server/tests/suite/v2/model_list.rs @@ -1,8 +1,10 @@ use std::time::Duration; use anyhow::Result; +use app_test_support::ChatGptAuthFixture; use app_test_support::McpProcess; use app_test_support::to_response; +use app_test_support::write_chatgpt_auth; use app_test_support::write_models_cache; use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCResponse; @@ -13,10 +15,16 @@ use codex_app_server_protocol::ModelServiceTier; use codex_app_server_protocol::ModelUpgradeInfo; use codex_app_server_protocol::ReasoningEffortOption; use codex_app_server_protocol::RequestId; +use codex_config::types::AuthCredentialsStoreMode; +use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; +use codex_protocol::openai_models::ModelsResponse; +use core_test_support::responses::mount_models_once; use pretty_assertions::assert_eq; +use serde_json::json; use tempfile::TempDir; use tokio::time::timeout; +use wiremock::MockServer; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); const INVALID_REQUEST_ERROR_CODE: i64 = -32600; @@ -148,6 +156,101 @@ async fn list_models_includes_hidden_models() -> Result<()> { Ok(()) } +#[tokio::test] +async fn list_models_uses_chatgpt_remote_catalog_as_source_of_truth() -> Result<()> { + let server = MockServer::start().await; + let remote_model: ModelInfo = serde_json::from_value(json!({ + "slug": "chatgpt-remote-only", + "display_name": "ChatGPT Remote Only", + "description": "Remote-only model for app-server model/list coverage", + "default_reasoning_level": "medium", + "supported_reasoning_levels": [ + {"effort": "low", "description": "low"}, + {"effort": "medium", "description": "medium"} + ], + "shell_type": "shell_command", + "visibility": "list", + "minimal_client_version": [0, 1, 0], + "supported_in_api": true, + "priority": 0, + "upgrade": null, + "base_instructions": "base instructions", + "supports_reasoning_summaries": false, + "support_verbosity": false, + "default_verbosity": null, + "apply_patch_tool_type": null, + "truncation_policy": {"mode": "bytes", "limit": 10_000}, + "supports_parallel_tool_calls": false, + "supports_image_detail_original": false, + "context_window": 272_000, + "max_context_window": 272_000, + "experimental_supported_tools": [], + }))?; + let models_mock = mount_models_once( + &server, + ModelsResponse { + models: vec![remote_model.clone()], + }, + ) + .await; + + let codex_home = TempDir::new()?; + let server_uri = server.uri(); + std::fs::write( + codex_home.path().join("config.toml"), + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" +openai_base_url = "{server_uri}/v1" +"# + ), + )?; + write_chatgpt_auth( + codex_home.path(), + ChatGptAuthFixture::new("chatgpt-access-token").plan_type("pro"), + AuthCredentialsStoreMode::File, + )?; + + let mut mcp = McpProcess::new_with_env(codex_home.path(), &[("OPENAI_API_KEY", None)]).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_list_models_request(ModelListParams { + limit: Some(100), + cursor: None, + include_hidden: None, + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + + let ModelListResponse { + data: items, + next_cursor, + } = to_response::(response)?; + let mut expected_presets: Vec = vec![remote_model.into()]; + ModelPreset::mark_default_by_picker_visibility(&mut expected_presets); + let expected_items = expected_presets + .iter() + .map(model_from_preset) + .collect::>(); + + assert_eq!(items, expected_items); + assert!(next_cursor.is_none()); + assert_eq!( + models_mock.requests().len(), + 1, + "expected a single /models request" + ); + Ok(()) +} + #[tokio::test] async fn list_models_pagination_works() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/cli/src/marketplace_cmd.rs b/codex-rs/cli/src/marketplace_cmd.rs index b4132a0127..2ec948eacb 100644 --- a/codex-rs/cli/src/marketplace_cmd.rs +++ b/codex-rs/cli/src/marketplace_cmd.rs @@ -152,7 +152,7 @@ async fn run_list(overrides: Vec<(String, toml::Value)>) -> Result<()> { .context("failed to load configuration")?; let configured_marketplaces = config .config_layer_stack - .get_user_layer() + .get_active_user_layer() .and_then(|layer| layer.config.get("marketplaces")) .and_then(toml::Value::as_table); let Some(configured_marketplaces) = configured_marketplaces else { diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index af75999163..5c66a27d84 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -199,6 +199,7 @@ async fn perform_oauth_login_retry_without_scopes( http_headers: Option>, env_http_headers: Option>, resolved_scopes: &ResolvedMcpOAuthScopes, + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -210,6 +211,7 @@ async fn perform_oauth_login_retry_without_scopes( http_headers.clone(), env_http_headers.clone(), &resolved_scopes.scopes, + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -226,6 +228,7 @@ async fn perform_oauth_login_retry_without_scopes( http_headers, env_http_headers, &[], + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -309,6 +312,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }; @@ -338,6 +342,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re oauth_config.http_headers, oauth_config.env_http_headers, &resolved_scopes, + /*oauth_client_id*/ None, /*oauth_resource*/ None, config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), @@ -431,6 +436,7 @@ async fn run_login(config_overrides: &CliConfigOverrides, login_args: LoginArgs) http_headers, env_http_headers, &resolved_scopes, + server.oauth_client_id(), server.oauth_resource.as_deref(), config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), diff --git a/codex-rs/cli/src/plugin_cmd.rs b/codex-rs/cli/src/plugin_cmd.rs index dba06313e5..593fe644b2 100644 --- a/codex-rs/cli/src/plugin_cmd.rs +++ b/codex-rs/cli/src/plugin_cmd.rs @@ -349,7 +349,7 @@ fn configured_marketplace_snapshot_issues( load_errors: &[MarketplaceListError], marketplace_name: Option<&str>, ) -> Vec { - let Some(user_layer) = plugins_input.config_layer_stack.get_user_layer() else { + let Some(user_layer) = plugins_input.config_layer_stack.get_active_user_layer() else { return Vec::new(); }; let Some(configured_marketplaces) = user_layer diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 976bd4c768..50b1bf6c12 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -893,6 +893,7 @@ fn mcp_init_error_display_prompts_for_github_pat() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }), @@ -945,6 +946,7 @@ fn mcp_init_error_display_reports_generic_errors() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }), diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index b8fb9a080c..0d24718ba9 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -446,6 +446,7 @@ fn codex_apps_mcp_server_config(config: &McpConfig) -> McpServerConfig { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 0d0afc6c48..286c191b29 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -278,6 +278,7 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -302,6 +303,7 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 371c3ffba0..3b49db638f 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -96,6 +96,7 @@ pub use mcp_types::AppToolApproval; pub use mcp_types::McpServerConfig; pub use mcp_types::McpServerDisabledReason; pub use mcp_types::McpServerEnvVar; +pub use mcp_types::McpServerOAuthConfig; pub use mcp_types::McpServerToolConfig; pub use mcp_types::McpServerTransportConfig; pub use mcp_types::RawMcpServerConfig; diff --git a/codex-rs/config/src/mcp_edit.rs b/codex-rs/config/src/mcp_edit.rs index f5881a1257..aaad3444fd 100644 --- a/codex-rs/config/src/mcp_edit.rs +++ b/codex-rs/config/src/mcp_edit.rs @@ -212,6 +212,15 @@ fn serialize_mcp_server(config: &McpServerConfig) -> TomlItem { { entry["scopes"] = array_from_strings(scopes); } + if let Some(oauth) = &config.oauth + && let Some(client_id) = &oauth.client_id + && !client_id.is_empty() + { + let mut oauth_table = TomlTable::new(); + oauth_table.set_implicit(false); + oauth_table["client_id"] = value(client_id.clone()); + entry["oauth"] = TomlItem::Table(oauth_table); + } if let Some(resource) = &config.oauth_resource && !resource.is_empty() { diff --git a/codex-rs/config/src/mcp_edit_tests.rs b/codex-rs/config/src/mcp_edit_tests.rs index cfcd73c3e5..0215e7b808 100644 --- a/codex-rs/config/src/mcp_edit_tests.rs +++ b/codex-rs/config/src/mcp_edit_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::McpServerOAuthConfig; use crate::McpServerToolConfig; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -33,6 +34,7 @@ async fn replace_mcp_servers_serializes_per_tool_approval_overrides() -> anyhow: enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::from([ ( @@ -82,3 +84,63 @@ approval_mode = "approve" Ok(()) } + +#[tokio::test] +async fn replace_mcp_servers_serializes_oauth_client_id() -> anyhow::Result<()> { + let unique_suffix = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos(); + let codex_home = std::env::temp_dir().join(format!( + "codex-config-mcp-oauth-edit-test-{}-{unique_suffix}", + std::process::id() + )); + let servers = BTreeMap::from([( + "maas_outlook".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "https://example.com/mcp".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + experimental_environment: None, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }), + oauth_resource: None, + tools: HashMap::new(), + }, + )]); + + ConfigEditsBuilder::new(&codex_home) + .replace_mcp_servers(&servers) + .apply() + .await?; + + let config_path = codex_home.join(CONFIG_TOML_FILE); + let serialized = std::fs::read_to_string(&config_path)?; + assert_eq!( + serialized, + r#"[mcp_servers.maas_outlook] +url = "https://example.com/mcp" + +[mcp_servers.maas_outlook.oauth] +client_id = "eci-prd-pub-codex-123" +"# + ); + + let loaded = load_global_mcp_servers(&codex_home).await?; + assert_eq!(loaded, servers); + + std::fs::remove_dir_all(&codex_home)?; + + Ok(()) +} diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index d642d9fc57..8dfceec37e 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -114,6 +114,15 @@ impl AsRef for McpServerEnvVar { } } +/// OAuth client settings used when Codex launches an MCP OAuth flow. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] +#[schemars(deny_unknown_fields)] +pub struct McpServerOAuthConfig { + /// Explicit OAuth client identifier to present during authorization and token exchange. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub client_id: Option, +} + #[derive(Serialize, Debug, Clone, PartialEq)] pub struct McpServerConfig { #[serde(flatten)] @@ -167,6 +176,10 @@ pub struct McpServerConfig { #[serde(default, skip_serializing_if = "Option::is_none")] pub scopes: Option>, + /// Optional OAuth client settings for MCP login. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub oauth: Option, + /// Optional OAuth resource parameter to include during MCP login (RFC 8707). #[serde(default, skip_serializing_if = "Option::is_none")] pub oauth_resource: Option, @@ -176,6 +189,14 @@ pub struct McpServerConfig { pub tools: HashMap, } +impl McpServerConfig { + pub fn oauth_client_id(&self) -> Option<&str> { + self.oauth + .as_ref() + .and_then(|oauth| oauth.client_id.as_deref()) + } +} + /// Raw MCP config shape used for deserialization and supported-field JSON /// Schema generation. /// @@ -233,6 +254,8 @@ pub struct RawMcpServerConfig { #[serde(default)] pub scopes: Option>, #[serde(default)] + pub oauth: Option, + #[serde(default)] pub oauth_resource: Option, /// Legacy display-name field accepted for backward compatibility. #[serde(default, rename = "name")] @@ -267,6 +290,7 @@ impl TryFrom for McpServerConfig { enabled_tools, disabled_tools, scopes, + oauth, oauth_resource, _name: _, tools, @@ -297,6 +321,7 @@ impl TryFrom for McpServerConfig { throw_if_set("stdio", "bearer_token", bearer_token.as_ref())?; throw_if_set("stdio", "http_headers", http_headers.as_ref())?; throw_if_set("stdio", "env_http_headers", env_http_headers.as_ref())?; + throw_if_set("stdio", "oauth", oauth.as_ref())?; throw_if_set("stdio", "oauth_resource", oauth_resource.as_ref())?; let env_vars = env_vars.unwrap_or_default(); for env_var in &env_vars { @@ -338,6 +363,7 @@ impl TryFrom for McpServerConfig { enabled_tools, disabled_tools, scopes, + oauth, oauth_resource, tools: tools.unwrap_or_default(), }) diff --git a/codex-rs/config/src/mcp_types_tests.rs b/codex-rs/config/src/mcp_types_tests.rs index 50f705d0f8..f397115425 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -283,6 +283,26 @@ fn deserialize_streamable_http_server_config_with_oauth_resource() { ); } +#[test] +fn deserialize_streamable_http_server_config_with_oauth_client_id() { + let cfg: McpServerConfig = toml::from_str( + r#" + url = "https://example.com/mcp" + + [oauth] + client_id = "eci-prd-pub-codex-123" + "#, + ) + .expect("should deserialize http config with oauth client id"); + + assert_eq!( + cfg.oauth, + Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }) + ); +} + #[test] fn deserialize_server_config_with_tool_filters() { let cfg: McpServerConfig = toml::from_str( @@ -393,6 +413,7 @@ fn deserialize_ignores_unknown_server_fields() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } @@ -439,6 +460,19 @@ fn deserialize_rejects_headers_for_stdio() { ) .expect_err("should reject env_http_headers for stdio transport"); + let err = toml::from_str::( + r#" + command = "echo" + oauth = { client_id = "eci-prd-pub-codex-123" } + "#, + ) + .expect_err("should reject oauth for stdio transport"); + + assert!( + err.to_string().contains("oauth is not supported for stdio"), + "unexpected error: {err}" + ); + let err = toml::from_str::( r#" command = "echo" diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 74e3394f93..c7d0a5876b 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -7,6 +7,7 @@ pub use crate::mcp_types::AppToolApproval; pub use crate::mcp_types::McpServerConfig; pub use crate::mcp_types::McpServerDisabledReason; pub use crate::mcp_types::McpServerEnvVar; +pub use crate::mcp_types::McpServerOAuthConfig; pub use crate::mcp_types::McpServerToolConfig; pub use crate::mcp_types::McpServerTransportConfig; pub use crate::mcp_types::RawMcpServerConfig; diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index b26f1c21b2..fc4193aa67 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -1053,13 +1053,21 @@ fn normalize_plugin_mcp_server_value( } } - if let Some(JsonValue::Object(oauth)) = object.remove("oauth") - && oauth.contains_key("callbackPort") - { - warn!( - plugin = %plugin_root.display(), - "plugin MCP server OAuth callbackPort is ignored; Codex uses global MCP OAuth callback settings" - ); + if let Some(JsonValue::Object(mut oauth)) = object.remove("oauth") { + if oauth.remove("callbackPort").is_some() { + warn!( + plugin = %plugin_root.display(), + "plugin MCP server OAuth callbackPort is ignored; Codex uses global MCP OAuth callback settings" + ); + } + + if let Some(client_id) = oauth.remove("clientId") { + oauth.entry("client_id".to_string()).or_insert(client_id); + } + + if !oauth.is_empty() { + object.insert("oauth".to_string(), JsonValue::Object(oauth)); + } } if let Some(JsonValue::String(cwd)) = object.get("cwd") diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index e0b22c0040..449c87b186 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -22,6 +22,7 @@ use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; use codex_config::McpServerConfig; +use codex_config::McpServerOAuthConfig; use codex_config::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; use codex_login::CodexAuth; @@ -230,6 +231,9 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("client-id".to_string()), + }), oauth_resource: None, tools: HashMap::new(), }, @@ -694,6 +698,7 @@ async fn load_plugins_uses_manifest_configured_component_paths() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -805,6 +810,7 @@ async fn load_plugins_ignores_manifest_component_paths_without_dot_slash() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -966,6 +972,7 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }; diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index d8c64a0d13..9fc95349d2 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1200,6 +1200,17 @@ } ] }, + "McpServerOAuthConfig": { + "additionalProperties": false, + "description": "OAuth client settings used when Codex launches an MCP OAuth flow.", + "properties": { + "client_id": { + "description": "Explicit OAuth client identifier to present during authorization and token exchange.", + "type": "string" + } + }, + "type": "object" + }, "McpServerToolConfig": { "additionalProperties": false, "description": "Per-tool approval settings for a single MCP server tool.", @@ -2124,6 +2135,14 @@ "description": "Legacy display-name field accepted for backward compatibility.", "type": "string" }, + "oauth": { + "allOf": [ + { + "$ref": "#/definitions/McpServerOAuthConfig" + } + ], + "default": null + }, "oauth_resource": { "default": null, "type": "string" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 27a67da68d..ff28e50436 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -36,6 +36,7 @@ use codex_config::types::BundledSkillsConfig; use codex_config::types::FeedbackConfigToml; use codex_config::types::HistoryPersistence; use codex_config::types::McpServerEnvVar; +use codex_config::types::McpServerOAuthConfig; use codex_config::types::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; use codex_config::types::MemoriesConfig; @@ -124,6 +125,7 @@ fn stdio_mcp(command: &str) -> McpServerConfig { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } @@ -148,6 +150,7 @@ fn http_mcp(url: &str) -> McpServerConfig { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } @@ -4428,6 +4431,7 @@ async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4721,6 +4725,7 @@ async fn replace_mcp_servers_serializes_env_sorted() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4797,6 +4802,7 @@ async fn replace_mcp_servers_serializes_env_vars() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4858,6 +4864,7 @@ async fn replace_mcp_servers_serializes_sourced_env_vars() -> anyhow::Result<()> enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4909,6 +4916,7 @@ async fn replace_mcp_servers_serializes_cwd() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4963,6 +4971,7 @@ async fn replace_mcp_servers_streamable_http_serializes_bearer_token() -> anyhow enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5033,6 +5042,7 @@ async fn replace_mcp_servers_streamable_http_serializes_custom_headers() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5115,6 +5125,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5150,6 +5161,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5220,6 +5232,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers() enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5245,6 +5258,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers() enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5333,6 +5347,7 @@ async fn replace_mcp_servers_serializes_disabled_flag() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5383,6 +5398,7 @@ async fn replace_mcp_servers_serializes_required_flag() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5433,6 +5449,7 @@ async fn replace_mcp_servers_serializes_tool_filters() -> anyhow::Result<()> { enabled_tools: Some(vec!["allowed".to_string()]), disabled_tools: Some(vec!["blocked".to_string()]), scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5487,6 +5504,9 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }), oauth_resource: Some("https://resource.example.com".to_string()), tools: HashMap::new(), }, @@ -5500,6 +5520,8 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh let config_path = codex_home.path().join(CONFIG_TOML_FILE); let serialized = std::fs::read_to_string(&config_path)?; + assert!(serialized.contains("[mcp_servers.docs.oauth]")); + assert!(serialized.contains(r#"client_id = "eci-prd-pub-codex-123""#)); assert!(serialized.contains(r#"oauth_resource = "https://resource.example.com""#)); let loaded = load_global_mcp_servers(codex_home.path()).await?; @@ -5508,6 +5530,7 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh docs.oauth_resource.as_deref(), Some("https://resource.example.com") ); + assert_eq!(docs.oauth_client_id(), Some("eci-prd-pub-codex-123")); Ok(()) } diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 4e5b46e182..80bf4d9bf1 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -341,6 +341,15 @@ mod document_helpers { { entry["scopes"] = array_from_iter(scopes.iter().cloned()); } + if let Some(oauth) = &config.oauth + && let Some(client_id) = &oauth.client_id + && !client_id.is_empty() + { + let mut oauth_table = TomlTable::new(); + oauth_table.set_implicit(false); + oauth_table["client_id"] = value(client_id.clone()); + entry["oauth"] = TomlItem::Table(oauth_table); + } if let Some(resource) = &config.oauth_resource && !resource.is_empty() { diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index ec7342c779..751411afdf 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -1,5 +1,6 @@ use super::*; use codex_config::types::AppToolApproval; +use codex_config::types::McpServerOAuthConfig; use codex_config::types::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; use codex_config::types::SessionPickerViewMode; @@ -940,6 +941,7 @@ fn blocking_replace_mcp_servers_round_trips() { enabled_tools: Some(vec!["one".to_string(), "two".to_string()]), disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -969,6 +971,9 @@ fn blocking_replace_mcp_servers_round_trips() { enabled_tools: None, disabled_tools: Some(vec!["forbidden".to_string()]), scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }), oauth_resource: Some("https://resource.example.com".to_string()), tools: HashMap::new(), }, @@ -994,6 +999,9 @@ oauth_resource = \"https://resource.example.com\" [mcp_servers.http.http_headers] Z-Header = \"z\" +[mcp_servers.http.oauth] +client_id = \"eci-prd-pub-codex-123\" + [mcp_servers.stdio] command = \"cmd\" args = [\"--flag\"] @@ -1035,6 +1043,7 @@ fn blocking_replace_mcp_servers_serializes_tool_approval_overrides() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::from([( "search".to_string(), @@ -1099,6 +1108,7 @@ foo = { command = "cmd" } enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -1153,6 +1163,7 @@ foo = { command = "cmd" } # keep me enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -1206,6 +1217,7 @@ foo = { command = "cmd", args = ["--flag"] } # keep me enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -1260,6 +1272,7 @@ foo = { command = "cmd" } enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index 44764e0064..b8552596b3 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -151,6 +151,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( server_config.scopes.clone(), oauth_config.discovered_scopes.clone(), ); + let oauth_client_id = server_config.oauth_client_id(); let first_attempt = perform_oauth_login( &name, &oauth_config.url, @@ -158,6 +159,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( oauth_config.http_headers.clone(), oauth_config.env_http_headers.clone(), &resolved_scopes.scopes, + oauth_client_id, server_config.oauth_resource.as_deref(), config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), @@ -173,6 +175,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( oauth_config.http_headers, oauth_config.env_http_headers, &[], + oauth_client_id, server_config.oauth_resource.as_deref(), config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), @@ -369,6 +372,7 @@ fn mcp_dependency_to_server_config( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }); @@ -398,6 +402,7 @@ fn mcp_dependency_to_server_config( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }); diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index e7db3f4044..fd53e828a9 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -296,11 +296,14 @@ impl TestCodexBuilder { }; let base_url = format!("{}/v1", server.uri()); let test_env = TestEnv::local().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, /*resume_from*/ None, test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, + )) + .await } - pub async fn build_remote_aware( + pub async fn build_with_remote_env( &mut self, server: &wiremock::MockServer, ) -> anyhow::Result { @@ -310,8 +313,28 @@ impl TestCodexBuilder { }; let base_url = format!("{}/v1", server.uri()); let test_env = test_env().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, /*resume_from*/ None, test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, + )) + .await + } + + pub async fn build_with_remote_and_local_env( + &mut self, + server: &wiremock::MockServer, + ) -> anyhow::Result { + let home = match self.home.clone() { + Some(home) => home, + None => Arc::new(TempDir::new()?), + }; + let base_url = format!("{}/v1", server.uri()); + let test_env = test_env().await?; + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ true, + )) + .await } pub async fn build_with_streaming_server( @@ -329,6 +352,7 @@ impl TestCodexBuilder { home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, )) .await } @@ -350,8 +374,11 @@ impl TestCodexBuilder { config.realtime.version = RealtimeWsVersion::V1; })); let test_env = TestEnv::local().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, /*resume_from*/ None, test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, + )) + .await } pub async fn resume( @@ -362,8 +389,14 @@ impl TestCodexBuilder { ) -> anyhow::Result { let base_url = format!("{}/v1", server.uri()); let test_env = TestEnv::local().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, Some(rollout_path), test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, + home, + Some(rollout_path), + test_env, + /*include_local_environment*/ false, + )) + .await } async fn build_with_home_and_base_url( @@ -372,6 +405,7 @@ impl TestCodexBuilder { home: Arc, resume_from: Option, test_env: TestEnv, + include_local_environment: bool, ) -> anyhow::Result { let (config, fallback_cwd) = self .prepare_config(base_url, &home, test_env.cwd().clone()) @@ -391,13 +425,19 @@ impl TestCodexBuilder { std::env::current_exe()?, codex_linux_sandbox_exe, )?; - let environment_manager = Arc::new( + let environment_manager = Arc::new(if include_local_environment { + codex_exec_server::EnvironmentManager::create_for_tests_with_local( + exec_server_url, + local_runtime_paths, + ) + .await + } else { codex_exec_server::EnvironmentManager::create_for_tests( exec_server_url, local_runtime_paths, ) - .await, - ); + .await + }); let file_system = test_env.environment().get_filesystem(); let mut workspace_setups = vec![]; swap(&mut self.workspace_setups, &mut workspace_setups); @@ -788,9 +828,9 @@ impl TestCodexHarness { Ok(Self { server, test }) } - pub async fn with_remote_aware_builder(mut builder: TestCodexBuilder) -> Result { + pub async fn with_remote_env_builder(mut builder: TestCodexBuilder) -> Result { let server = start_mock_server().await; - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; Ok(Self { server, test }) } diff --git a/codex-rs/core/tests/suite/agents_md.rs b/codex-rs/core/tests/suite/agents_md.rs index 724b852396..276fa4bb51 100644 --- a/codex-rs/core/tests/suite/agents_md.rs +++ b/codex-rs/core/tests/suite/agents_md.rs @@ -16,7 +16,7 @@ async fn agents_instructions(mut builder: TestCodexBuilder) -> Result { ) .await; - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; test.submit_turn("hello").await?; let request = resp_mock.single_request(); diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index e42057215a..b692d32568 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -64,7 +64,7 @@ async fn apply_patch_harness_with( }); // Box harness construction so apply_patch_cli tests do not inline the // full test-thread startup path into each test future. - Box::pin(TestCodexHarness::with_remote_aware_builder(builder)).await + Box::pin(TestCodexHarness::with_remote_env_builder(builder)).await } async fn submit_without_wait(harness: &TestCodexHarness, prompt: &str) -> Result<()> { diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index 3bcb37e7b2..af63d092f8 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -244,6 +244,7 @@ async fn run_code_mode_turn_with_rmcp_config( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/hierarchical_agents.rs b/codex-rs/core/tests/suite/hierarchical_agents.rs index 212303ed0e..751807623b 100644 --- a/codex-rs/core/tests/suite/hierarchical_agents.rs +++ b/codex-rs/core/tests/suite/hierarchical_agents.rs @@ -32,7 +32,7 @@ async fn hierarchical_agents_appends_to_project_doc_in_user_instructions() { Ok::<(), anyhow::Error>(()) }); let test = builder - .build_remote_aware(&server) + .build_with_remote_env(&server) .await .expect("build test codex"); @@ -76,7 +76,7 @@ async fn hierarchical_agents_emits_when_no_project_doc() { .expect("test config should allow feature update"); }); let test = builder - .build_remote_aware(&server) + .build_with_remote_env(&server) .await .expect("build test codex"); diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 96ed732695..80290e31ba 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -193,6 +193,7 @@ fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode: enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index dc6d5cefc5..9c3414baeb 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -58,7 +58,7 @@ async fn unified_exec_test(server: &wiremock::MockServer) -> Result { "unified exec should enable for test: {result:?}", ); }); - builder.build_remote_aware(server).await + builder.build_with_remote_and_local_env(server).await } async fn submit_turn_with_approval_and_environments( @@ -77,7 +77,7 @@ async fn submit_turn_with_approval_and_environments( cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::OnRequest, approvals_reviewer: Some(ApprovalsReviewer::User), - sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + sandbox_policy: SandboxPolicy::new_read_only_policy(), permission_profile: None, model: test.session_configured.model.clone(), effort: None, @@ -350,7 +350,7 @@ async fn apply_patch_freeform_routes_to_selected_remote_environment() -> Result< let mut builder = test_codex().with_config(|config| { config.include_apply_patch_tool = true; }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_and_local_env(&server).await?; let local_cwd = TempDir::new()?; let file_name = "apply_patch_remote_freeform.txt"; let remote_cwd = PathBuf::from(format!( @@ -439,7 +439,7 @@ async fn apply_patch_approvals_are_remembered_per_environment() -> Result<()> { config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); config.approvals_reviewer = ApprovalsReviewer::User; }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_and_local_env(&server).await?; let local_cwd = TempDir::new()?; let remote_cwd = PathBuf::from(format!( "/tmp/codex-remote-apply-patch-approval-cwd-{}", diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 1a7062bdb1..7ead0364dc 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -332,6 +332,7 @@ fn insert_mcp_server( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -479,7 +480,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -602,7 +603,7 @@ async fn stdio_server_uses_configured_cwd_before_runtime_fallback() -> anyhow::R }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; let expected_cwd = expected_cwd @@ -655,7 +656,7 @@ async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() - }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; let expected_cwd = expected_cwd @@ -774,7 +775,7 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; wait_for_mcp_server(&fixture, server_name).await?; @@ -872,7 +873,7 @@ async fn stdio_mcp_parallel_tool_calls_default_false_runs_serially() -> anyhow:: }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -989,7 +990,7 @@ async fn stdio_mcp_parallel_tool_calls_opt_in_runs_concurrently() -> anyhow::Res }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -1070,7 +1071,7 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; wait_for_mcp_server(&fixture, server_name).await?; @@ -1202,7 +1203,7 @@ async fn stdio_image_responses_preserve_original_detail_metadata() -> anyhow::Re }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; wait_for_mcp_server(&fixture, server_name).await?; @@ -1337,7 +1338,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture @@ -1439,7 +1440,7 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -1557,7 +1558,7 @@ async fn stdio_server_propagates_explicit_local_env_var_source() -> anyhow::Resu }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture @@ -1649,7 +1650,7 @@ async fn remote_stdio_env_var_source_does_not_copy_local_env() -> anyhow::Result }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture @@ -1832,7 +1833,7 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; // Phase 4: submit the user turn that should trigger the MCP tool call. fixture @@ -2018,7 +2019,7 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; // Phase 5: wait for MCP startup before the turn is submitted, which keeps // failures tied to server startup/discovery. diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index b3cf2e3bca..bb4ca84b29 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -999,6 +999,7 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { enabled_tools: Some(vec!["echo".to_string(), "image".to_string()]), disabled_tools: Some(vec!["image".to_string()]), scopes: None, + oauth: None, oauth_resource: None, supports_parallel_tool_calls: false, tools: HashMap::new(), @@ -1127,6 +1128,7 @@ async fn tool_search_surfaced_mcp_tool_errors_are_returned_to_model() -> Result< enabled_tools: Some(vec!["echo".to_string()]), disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, supports_parallel_tool_calls: false, tools: HashMap::new(), @@ -1272,6 +1274,7 @@ async fn tool_search_uses_non_app_mcp_server_instructions_as_namespace_descripti enabled_tools: Some(vec!["echo".to_string()]), disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, supports_parallel_tool_calls: false, tools: HashMap::new(), diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 894110a6fb..6434f9c533 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -50,7 +50,7 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { let mut builder = test_codex().with_workspace_setup(move |cwd, fs| async move { write_repo_skill(cwd, fs, "demo", "demo skill", skill_body).await }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let skill_path = test .config diff --git a/codex-rs/core/tests/suite/sqlite_state.rs b/codex-rs/core/tests/suite/sqlite_state.rs index b2a2778047..0cc7d07b16 100644 --- a/codex-rs/core/tests/suite/sqlite_state.rs +++ b/codex-rs/core/tests/suite/sqlite_state.rs @@ -386,6 +386,7 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result< enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/truncation.rs b/codex-rs/core/tests/suite/truncation.rs index 0fd072a70f..447bca6731 100644 --- a/codex-rs/core/tests/suite/truncation.rs +++ b/codex-rs/core/tests/suite/truncation.rs @@ -398,6 +398,7 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()> enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -497,6 +498,7 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -779,6 +781,7 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 76ed9cdcbf..231d14f770 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -379,7 +379,7 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let cwd = test.config.cwd.to_path_buf(); let call_id = "uexec-begin-event"; @@ -438,7 +438,7 @@ async fn unified_exec_resolves_relative_workdir() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let workdir_rel = std::path::PathBuf::from("uexec_relative_workdir"); let workdir = create_workspace_directory(&test, &workdir_rel).await?; @@ -507,7 +507,7 @@ async fn unified_exec_respects_workdir_override() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let workdir = create_workspace_directory(&test, "uexec_workdir_test").await?; @@ -572,7 +572,7 @@ async fn unified_exec_emits_exec_command_end_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-end-event"; let args = json!({ @@ -645,7 +645,7 @@ async fn unified_exec_emits_output_delta_for_exec_command() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-delta-1"; let args = json!({ @@ -703,7 +703,7 @@ async fn unified_exec_full_lifecycle_with_background_end_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-full-lifecycle"; // This timing force the long-standing PTY @@ -910,7 +910,7 @@ allow_local_binding = true PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy_for_config), ); }); - let test = builder.build_remote_aware(server).await?; + let test = builder.build_with_remote_env(server).await?; assert!( test.config.permissions.network.is_some(), "expected managed network proxy config to be present" @@ -991,7 +991,7 @@ async fn unified_exec_emits_terminal_interaction_for_write_stdin() -> Result<()> .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let open_call_id = "uexec-open"; let open_args = json!({ @@ -1074,7 +1074,7 @@ async fn unified_exec_terminal_interaction_captures_delayed_output() -> Result<( .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let open_call_id = "uexec-delayed-open"; let open_args = json!({ @@ -1253,7 +1253,7 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let open_call_id = "uexec-open-session"; let open_args = json!({ @@ -1367,7 +1367,7 @@ async fn exec_command_reports_chunk_and_exit_metadata() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-metadata"; let args = serde_json::json!({ @@ -1462,7 +1462,7 @@ async fn exec_command_clamps_model_requested_max_output_tokens_to_policy() -> Re .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-clamped-max-output"; let args = serde_json::json!({ @@ -1524,7 +1524,7 @@ async fn write_stdin_clamps_model_requested_max_output_tokens_to_policy() -> Res .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let start_call_id = "uexec-stdin-clamp-start"; let start_args = serde_json::json!({ @@ -1611,7 +1611,7 @@ async fn unified_exec_defaults_to_pipe() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-default-pipe"; let args = serde_json::json!({ @@ -1680,7 +1680,7 @@ async fn unified_exec_can_enable_tty() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-tty-enabled"; let args = serde_json::json!({ @@ -1746,7 +1746,7 @@ async fn unified_exec_respects_early_exit_notifications() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-early-exit"; let args = serde_json::json!({ @@ -1829,7 +1829,7 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let start_call_id = "uexec-cat-start"; let send_call_id = "uexec-cat-send"; @@ -1983,7 +1983,7 @@ async fn unified_exec_emits_end_event_when_session_dies_via_stdin() -> Result<() .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let start_call_id = "uexec-end-on-exit-start"; let start_args = serde_json::json!({ @@ -2266,7 +2266,7 @@ async fn unified_exec_reuses_session_via_stdin() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let first_call_id = "uexec-start"; let first_args = serde_json::json!({ @@ -2365,7 +2365,7 @@ async fn unified_exec_streams_after_lagged_output() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let script = r#"python3 - <<'PY' import sys @@ -2485,7 +2485,7 @@ async fn unified_exec_timeout_and_followup_poll() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let first_call_id = "uexec-timeout"; let first_args = serde_json::json!({ @@ -2574,7 +2574,7 @@ async fn unified_exec_formats_large_output_summary() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let script = r#"python3 - <<'PY' import sys @@ -3002,7 +3002,7 @@ async fn unified_exec_runs_on_all_platforms() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec"; let args = serde_json::json!({ @@ -3066,7 +3066,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; const MAX_SESSIONS_FOR_TEST: i32 = 64; const FILLER_SESSIONS: i32 = MAX_SESSIONS_FOR_TEST - 1; diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index 4ff2062b40..5d5673b011 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -174,7 +174,7 @@ async fn assert_user_turn_local_image_resizes_to( let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -266,7 +266,7 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -562,7 +562,7 @@ async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<() let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_and_local_env(&server).await?; let local_cwd = TempDir::new()?; fs::write(local_cwd.path().join("remote.png"), b"not a remote image")?; let local_selection = TurnEnvironmentSelection { @@ -582,9 +582,7 @@ async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<() /*sandbox*/ None, ) .await?; - let png = BASE64_STANDARD.decode( - "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII=", - )?; + let png = png_bytes(/*width*/ 1, /*height*/ 1, [0, 255, 0, 255])?; test.fs() .write_file(&image_path, png, /*sandbox*/ None) .await?; @@ -664,7 +662,7 @@ async fn view_image_tool_can_preserve_original_resolution_when_requested_on_gpt5 let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -755,7 +753,7 @@ async fn view_image_tool_errors_clearly_for_unsupported_detail_values() -> anyho let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -833,7 +831,7 @@ async fn view_image_tool_treats_null_detail_as_omitted() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -923,7 +921,7 @@ async fn view_image_tool_resizes_when_model_lacks_original_detail_support() -> a let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.2"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1017,7 +1015,7 @@ async fn view_image_tool_does_not_force_original_resolution_with_capability_only let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1108,7 +1106,7 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1178,7 +1176,7 @@ async fn view_image_tool_errors_for_non_image_files() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1255,7 +1253,7 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, config, @@ -1385,7 +1383,7 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an .with_config(|config| { config.model = Some(model_slug.to_string()); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, .. } = &test; let rel_path = "assets/example.png"; @@ -1472,7 +1470,7 @@ async fn replaces_invalid_local_image_after_bad_request() -> anyhow::Result<()> let completion_mock = responses::mount_sse_once(&server, success_response).await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 7e4a3fb056..1d7d6e75f5 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -135,6 +135,20 @@ impl EnvironmentManager { } } + /// Builds a test-only manager that keeps the provider default while also + /// allowing tests to select the local environment explicitly. + pub async fn create_for_tests_with_local( + exec_server_url: Option, + local_runtime_paths: ExecServerRuntimePaths, + ) -> Self { + let mut snapshot = DefaultEnvironmentProvider::new(exec_server_url).snapshot_inner(); + snapshot.include_local = true; + match Self::from_provider_snapshot(snapshot, local_runtime_paths) { + Ok(manager) => manager, + Err(err) => panic!("test provider with local should create valid environments: {err}"), + } + } + /// Builds a manager from a provider-supplied startup snapshot. pub async fn from_provider

( provider: &P, diff --git a/codex-rs/exec/src/lib_tests.rs b/codex-rs/exec/src/lib_tests.rs index c12b483e89..321f4b8f9e 100644 --- a/codex-rs/exec/src/lib_tests.rs +++ b/codex-rs/exec/src/lib_tests.rs @@ -409,6 +409,7 @@ async fn thread_start_params_include_review_policy_when_review_policy_is_manual_ let codex_home = tempdir().expect("create temp codex home"); let cwd = tempdir().expect("create temp cwd"); let config = ConfigBuilder::default() + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { approvals_reviewer: Some(ApprovalsReviewer::User), diff --git a/codex-rs/models-manager/src/manager.rs b/codex-rs/models-manager/src/manager.rs index 517bb0abbb..af510c8d73 100644 --- a/codex-rs/models-manager/src/manager.rs +++ b/codex-rs/models-manager/src/manager.rs @@ -3,11 +3,13 @@ use crate::collaboration_mode_presets::builtin_collaboration_mode_presets; use crate::config::ModelsManagerConfig; use crate::model_info; use async_trait::async_trait; +use codex_app_server_protocol::AuthMode; use codex_login::AuthManager; use codex_protocol::config_types::CollaborationModeMask; use codex_protocol::error::Result as CoreResult; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; +use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; use std::fmt; use std::path::PathBuf; @@ -319,6 +321,23 @@ impl OpenAiModelsManager { /// Replace the cached remote models and rebuild the derived presets list. async fn apply_remote_models(&self, models: Vec) { + // Use the remote models list as the source of truth if it contains at least one + // non-hidden model and the user is using ChatGPT auth. + let should_use_remote_models_only = !models.is_empty() + && models + .iter() + .any(|model| model.visibility == ModelVisibility::List) + && self.auth_manager.as_ref().is_some_and(|auth_manager| { + matches!( + auth_manager.auth_mode(), + Some(AuthMode::Chatgpt | AuthMode::ChatgptAuthTokens) + ) + }); + if should_use_remote_models_only { + *self.remote_models.write().await = models; + return; + } + let mut existing_models = load_remote_models_from_file().unwrap_or_default(); for model in models { if let Some(existing_index) = existing_models diff --git a/codex-rs/models-manager/src/manager_tests.rs b/codex-rs/models-manager/src/manager_tests.rs index 9df9fb09c9..ede7cfe795 100644 --- a/codex-rs/models-manager/src/manager_tests.rs +++ b/codex-rs/models-manager/src/manager_tests.rs @@ -366,6 +366,160 @@ async fn refresh_available_models_sorts_by_priority() { assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch"); } +#[tokio::test] +async fn refresh_available_models_uses_remote_only_catalog_for_chatgpt_auth() { + let remote_models = vec![remote_model( + "chatgpt-visible-source-of-truth", + "ChatGPT Visible", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![remote_models.clone()]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint.clone()); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, remote_models); + assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch"); +} + +#[tokio::test] +async fn refresh_available_models_uses_cached_remote_only_catalog_for_chatgpt_auth() { + let remote_models = vec![remote_model( + "chatgpt-cached-source-of-truth", + "ChatGPT Cached", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let fetch_endpoint = TestModelsEndpoint::new(vec![remote_models.clone()]); + let fetch_manager = + openai_manager_for_tests(codex_home.path().to_path_buf(), fetch_endpoint.clone()); + + fetch_manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("initial refresh succeeds"); + + let cache_endpoint = TestModelsEndpoint::new(Vec::new()); + let cache_manager = + openai_manager_for_tests(codex_home.path().to_path_buf(), cache_endpoint.clone()); + + cache_manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("cached refresh succeeds"); + + assert_eq!(cache_manager.get_remote_models().await, remote_models); + assert_eq!( + cache_endpoint.fetch_count(), + 0, + "fresh cache should avoid a model fetch" + ); +} + +#[tokio::test] +async fn get_model_info_uses_fallback_for_bundled_models_when_chatgpt_remote_is_authoritative() { + let remote_models = vec![remote_model( + "chatgpt-authoritative-model-info", + "ChatGPT Model Info", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![remote_models]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint); + let bundled_slug = load_remote_models_from_file() + .expect("bundled models should parse") + .first() + .expect("bundled models should contain at least one model") + .slug + .clone(); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + let model_info = manager + .get_model_info(&bundled_slug, &ModelsManagerConfig::default()) + .await; + + assert_eq!(model_info.slug, bundled_slug); + assert!(model_info.used_fallback_model_metadata); +} + +#[tokio::test] +async fn refresh_available_models_preserves_bundled_catalog_for_empty_chatgpt_remote() { + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![Vec::new()]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint); + let expected = load_remote_models_from_file().expect("bundled models should parse"); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, expected); +} + +#[tokio::test] +async fn refresh_available_models_merges_hidden_only_chatgpt_remote_with_bundled_catalog() { + let hidden_remote = remote_model_with_visibility( + "chatgpt-hidden-only", + "ChatGPT Hidden", + /*priority*/ 0, + "hide", + ); + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![vec![hidden_remote.clone()]]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint); + let mut expected = load_remote_models_from_file().expect("bundled models should parse"); + expected.push(hidden_remote); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, expected); +} + +#[tokio::test] +async fn refresh_available_models_keeps_merging_for_api_auth() { + let remote_models = vec![remote_model( + "api-auth-visible-remote", + "API Auth Visible", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let endpoint = Arc::new(TestModelsEndpoint { + has_command_auth: true, + uses_codex_backend: false, + responses: Mutex::new(vec![remote_models.clone()].into()), + fetch_count: AtomicUsize::new(0), + }); + let manager = openai_manager_for_tests_with_auth( + codex_home.path().to_path_buf(), + endpoint.clone(), + Some(AuthManager::from_auth_for_testing(CodexAuth::from_api_key( + "test-api-key", + ))), + ); + let mut expected = load_remote_models_from_file().expect("bundled models should parse"); + expected.extend(remote_models); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, expected); + assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch"); +} + #[tokio::test] async fn refresh_available_models_uses_cache_when_fresh() { let remote_models = vec![remote_model("cached", "Cached", /*priority*/ 5)]; diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index f42786c652..eb6c7906a8 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -11,6 +11,9 @@ use base64::Engine; use base64::engine::general_purpose::URL_SAFE_NO_PAD; use reqwest::ClientBuilder; use reqwest::Url; +use rmcp::transport::AuthorizationManager; +use rmcp::transport::AuthorizationSession; +use rmcp::transport::auth::OAuthClientConfig; use rmcp::transport::auth::OAuthState; use sha2::Digest; use sha2::Sha256; @@ -81,6 +84,7 @@ pub async fn perform_oauth_login( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -92,6 +96,7 @@ pub async fn perform_oauth_login( http_headers, env_http_headers, scopes, + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -108,6 +113,7 @@ pub async fn perform_oauth_login_silent( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -119,6 +125,7 @@ pub async fn perform_oauth_login_silent( http_headers, env_http_headers, scopes, + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -135,6 +142,7 @@ async fn perform_oauth_login_with_browser_output( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -150,6 +158,7 @@ async fn perform_oauth_login_with_browser_output( store_mode, headers, scopes, + oauth_client_id, oauth_resource, /*launch_browser*/ true, callback_port, @@ -169,6 +178,7 @@ pub async fn perform_oauth_login_return_url( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, timeout_secs: Option, callback_port: Option, @@ -184,6 +194,7 @@ pub async fn perform_oauth_login_return_url( store_mode, headers, scopes, + oauth_client_id, oauth_resource, /*launch_browser*/ false, callback_port, @@ -436,6 +447,7 @@ impl OauthLoginFlow { store_mode: OAuthCredentialsStoreMode, headers: OauthHeaders, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, launch_browser: bool, callback_port: Option, @@ -471,11 +483,15 @@ impl OauthLoginFlow { let default_headers = build_default_headers(http_headers, env_http_headers)?; let http_client = apply_default_headers(ClientBuilder::new(), &default_headers).build()?; - let mut oauth_state = OAuthState::new(server_url, Some(http_client)).await?; let scope_refs: Vec<&str> = scopes.iter().map(String::as_str).collect(); - oauth_state - .start_authorization(&scope_refs, &redirect_uri, Some("Codex")) - .await?; + let oauth_state = start_authorization( + server_url, + http_client, + &scope_refs, + &redirect_uri, + oauth_client_id, + ) + .await?; let auth_url = append_query_param( &oauth_state.get_authorization_url().await?, "resource", @@ -585,6 +601,41 @@ impl OauthLoginFlow { } } +async fn start_authorization( + server_url: &str, + http_client: reqwest::Client, + scopes: &[&str], + redirect_uri: &str, + oauth_client_id: Option<&str>, +) -> Result { + let Some(oauth_client_id) = oauth_client_id.filter(|client_id| !client_id.trim().is_empty()) + else { + let mut oauth_state = OAuthState::new(server_url, Some(http_client)).await?; + oauth_state + .start_authorization(scopes, redirect_uri, Some("Codex")) + .await?; + return Ok(oauth_state); + }; + + let mut auth_manager = AuthorizationManager::new(server_url).await?; + auth_manager.with_client(http_client)?; + let metadata = auth_manager.discover_metadata().await?; + auth_manager.set_metadata(metadata); + auth_manager.configure_client(OAuthClientConfig { + client_id: oauth_client_id.to_string(), + client_secret: None, + scopes: scopes.iter().map(|scope| (*scope).to_string()).collect(), + redirect_uri: redirect_uri.to_string(), + })?; + let auth_url = auth_manager.get_authorization_url(scopes).await?; + + Ok(OAuthState::Session(AuthorizationSession { + auth_manager, + auth_url, + redirect_uri: redirect_uri.to_string(), + })) +} + fn append_query_param(url: &str, key: &str, value: Option<&str>) -> String { let Some(value) = value else { return url.to_string(); @@ -604,7 +655,13 @@ fn append_query_param(url: &str, key: &str, value: Option<&str>) -> String { #[cfg(test)] mod tests { + use axum::Json; + use axum::Router; + use axum::routing::get; use pretty_assertions::assert_eq; + use reqwest::Url; + use serde_json::json; + use tokio::net::TcpListener; use super::CallbackOutcome; use super::OAuthProviderError; @@ -613,6 +670,70 @@ mod tests { use super::callback_id_from_server_url; use super::callback_path_from_redirect_uri; use super::parse_oauth_callback; + use super::start_authorization; + + async fn spawn_oauth_metadata_server() -> String { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind metadata listener"); + let addr = listener.local_addr().expect("read metadata listener addr"); + let base_url = format!("http://{addr}"); + let metadata = json!({ + "authorization_endpoint": format!("{base_url}/oauth/authorize"), + "token_endpoint": format!("{base_url}/oauth/token"), + "scopes_supported": [""], + }); + let path_scoped_metadata = metadata.clone(); + let app = Router::new() + .route( + "/.well-known/oauth-authorization-server/mcp", + get(move || { + let metadata = path_scoped_metadata.clone(); + async move { Json(metadata) } + }), + ) + .route( + "/.well-known/oauth-authorization-server", + get(move || { + let metadata = metadata.clone(); + async move { Json(metadata) } + }), + ); + + tokio::spawn(async move { + axum::serve(listener, app) + .await + .expect("serve oauth metadata"); + }); + + base_url + } + + #[tokio::test] + async fn start_authorization_uses_configured_client_id() { + let base_url = spawn_oauth_metadata_server().await; + let oauth_state = start_authorization( + &format!("{base_url}/mcp"), + reqwest::Client::new(), + &[], + "http://127.0.0.1/callback", + Some("eci-prd-pub-codex-123"), + ) + .await + .expect("start oauth authorization"); + + let authorization_url = oauth_state + .get_authorization_url() + .await + .expect("read authorization url"); + let auth_url = Url::parse(&authorization_url).expect("authorization url should parse"); + let client_id = auth_url + .query_pairs() + .find(|(key, _)| key == "client_id") + .map(|(_, value)| value.into_owned()); + + assert_eq!(client_id.as_deref(), Some("eci-prd-pub-codex-123")); + } #[test] fn parse_oauth_callback_accepts_default_path() { diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index aece933ff1..22d9f842f1 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -47,6 +47,7 @@ use codex_app_server_protocol::FileSystemSandboxEntry; use codex_app_server_protocol::FileSystemSpecialPath; use codex_app_server_protocol::McpServerElicitationAction; use codex_app_server_protocol::NetworkApprovalContext; +use codex_app_server_protocol::NetworkApprovalProtocol; use codex_app_server_protocol::NetworkPolicyRuleAction; use codex_app_server_protocol::RequestId; use codex_features::Features; @@ -354,8 +355,25 @@ impl ApprovalOverlay { return; }; if request.thread_label().is_none() { + let subject = match request { + ApprovalRequest::Exec { + network_approval_context: Some(network_approval_context), + .. + } => history_cell::ApprovalDecisionSubject::NetworkAccess { + target: network_approval_target(network_approval_context, command), + }, + _ => { + if let Some(target) = network_approval_command_target(command) { + history_cell::ApprovalDecisionSubject::NetworkAccess { + target: target.to_string(), + } + } else { + history_cell::ApprovalDecisionSubject::Command(command.to_vec()) + } + } + }; let cell = history_cell::new_approval_decision_cell( - command.to_vec(), + subject, command_decision_to_review_decision(&decision), history_cell::ApprovalDecisionActor::User, ); @@ -623,6 +641,35 @@ fn approval_footer_hint( Line::from(spans) } +fn network_approval_target( + network_approval_context: &NetworkApprovalContext, + command: &[String], +) -> String { + if let Some(target) = network_approval_command_target(command) { + return target.to_string(); + } + + let scheme = match network_approval_context.protocol { + NetworkApprovalProtocol::Http => "http", + NetworkApprovalProtocol::Https => "https", + NetworkApprovalProtocol::Socks5Tcp => "socks5-tcp", + NetworkApprovalProtocol::Socks5Udp => "socks5-udp", + }; + format!("{scheme}://{}", network_approval_context.host) +} + +fn network_approval_command_target(command: &[String]) -> Option<&str> { + match command { + [program, target] if program == "network-access" && !target.is_empty() => { + Some(target.as_str()) + } + [command] => command + .strip_prefix("network-access ") + .filter(|target| !target.is_empty()), + _ => None, + } +} + fn build_header(request: &ApprovalRequest) -> Box { match request { ApprovalRequest::Exec { @@ -1102,6 +1149,21 @@ mod tests { .join("\n") } + fn render_history_cell_lines( + cell: &dyn crate::history_cell::HistoryCell, + width: u16, + ) -> Vec { + cell.display_lines(width) + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect() + } + fn normalize_snapshot_paths(rendered: String) -> String { [ (absolute_path("/tmp/readme.txt"), "/tmp/readme.txt"), @@ -2111,7 +2173,7 @@ mod tests { "git add tui/src/render/mod.rs tui/src/render/renderable.rs".into(), ]; let cell = history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), ReviewDecision::Approved, history_cell::ApprovalDecisionActor::User, ); @@ -2134,6 +2196,73 @@ mod tests { assert_eq!(rendered, expected); } + #[test] + fn exec_history_cell_does_not_render_blank_action_for_empty_command() { + let approved = history_cell::new_approval_decision_cell( + history_cell::ApprovalDecisionSubject::Command(Vec::new()), + ReviewDecision::Approved, + history_cell::ApprovalDecisionActor::User, + ); + assert_eq!( + render_history_cell_lines(approved.as_ref(), /*width*/ 80), + vec!["✔ You approved this request this time".to_string()] + ); + + let approved_for_session = history_cell::new_approval_decision_cell( + history_cell::ApprovalDecisionSubject::Command(Vec::new()), + ReviewDecision::ApprovedForSession, + history_cell::ApprovalDecisionActor::User, + ); + assert_eq!( + render_history_cell_lines(approved_for_session.as_ref(), /*width*/ 80), + vec!["✔ You approved this request every time this session".to_string()] + ); + } + + #[test] + fn network_access_command_history_uses_target_without_structured_context() { + let (tx_raw, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut view = make_overlay( + ApprovalRequest::Exec { + thread_id: ThreadId::new(), + thread_label: None, + id: "test".into(), + command: vec![ + "network-access".to_string(), + "https://example.com:8443".to_string(), + ], + reason: None, + available_decisions: vec![ + CommandExecutionApprovalDecision::Accept, + CommandExecutionApprovalDecision::Cancel, + ], + network_approval_context: None, + additional_permissions: None, + }, + tx, + Features::with_defaults(), + ); + + view.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + + let mut decision = None; + while let Ok(event) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + decision = Some(cell); + break; + } + } + let decision = decision.expect("expected decision cell in history"); + assert_eq!( + render_history_cell_lines(decision.as_ref(), /*width*/ 80), + vec![ + "✔ You approved codex network access to https://example.com:8443 this time" + .to_string(), + ] + ); + } + #[test] fn esc_cancels_mcp_elicitation() { let (tx_raw, mut rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs index 85c8fc6c03..d732177ce4 100644 --- a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -145,6 +145,129 @@ fn app_server_exec_approval_request_preserves_permissions_context() { ); } +#[tokio::test] +async fn network_exec_approval_history_describes_session_host_allowance() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let request = exec_approval_request_from_params( + AppServerCommandExecutionRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "item-1".to_string(), + started_at_ms: 0, + approval_id: Some("approval-1".to_string()), + reason: None, + network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { + host: "example.com".to_string(), + protocol: codex_app_server_protocol::NetworkApprovalProtocol::Https, + }), + command: Some("network-access https://example.com:8443".to_string()), + cwd: None, + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: Some(vec![ + codex_app_server_protocol::CommandExecutionApprovalDecision::AcceptForSession, + codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel, + ]), + }, + &test_path_buf("/tmp").abs(), + ); + + handle_exec_approval_request(&mut chat, "sub-network", request); + chat.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE)); + + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "network_exec_approval_history_session_host_allowance", + lines_to_single_string(&decision) + ); +} + +#[tokio::test] +async fn network_exec_approval_history_describes_one_time_host_allowance() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let request = exec_approval_request_from_params( + AppServerCommandExecutionRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "item-1".to_string(), + started_at_ms: 0, + approval_id: Some("approval-1".to_string()), + reason: None, + network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { + host: "example.com".to_string(), + protocol: codex_app_server_protocol::NetworkApprovalProtocol::Http, + }), + command: None, + cwd: None, + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: Some(vec![ + codex_app_server_protocol::CommandExecutionApprovalDecision::Accept, + codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel, + ]), + }, + &test_path_buf("/tmp").abs(), + ); + + handle_exec_approval_request(&mut chat, "sub-network", request); + chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "network_exec_approval_history_one_time_host_allowance", + lines_to_single_string(&decision) + ); +} + +#[tokio::test] +async fn network_exec_approval_history_describes_canceled_host_request() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let request = exec_approval_request_from_params( + AppServerCommandExecutionRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "item-1".to_string(), + started_at_ms: 0, + approval_id: Some("approval-1".to_string()), + reason: None, + network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { + host: "example.com".to_string(), + protocol: codex_app_server_protocol::NetworkApprovalProtocol::Socks5Tcp, + }), + command: Some("network-access socks5-tcp://example.com:1080".to_string()), + cwd: None, + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: Some(vec![ + codex_app_server_protocol::CommandExecutionApprovalDecision::Accept, + codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel, + ]), + }, + &test_path_buf("/tmp").abs(), + ); + + handle_exec_approval_request(&mut chat, "sub-network", request); + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "network_exec_approval_history_canceled_host_request", + lines_to_single_string(&decision) + ); +} + #[test] fn app_server_request_permissions_preserves_file_system_permissions() { let read_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/read-only"))) diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_canceled_host_request.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_canceled_host_request.snap new file mode 100644 index 0000000000..3d09cd7087 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_canceled_host_request.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests/approval_requests.rs +expression: lines_to_single_string(&decision) +--- +✗ You canceled the request for codex network access to + socks5-tcp://example.com:1080 diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_one_time_host_allowance.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_one_time_host_allowance.snap new file mode 100644 index 0000000000..4b56b187aa --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_one_time_host_allowance.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/chatwidget/tests/approval_requests.rs +expression: lines_to_single_string(&decision) +--- +✔ You approved codex network access to http://example.com this time diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_session_host_allowance.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_session_host_allowance.snap new file mode 100644 index 0000000000..e5197e9e71 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_session_host_allowance.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests/approval_requests.rs +expression: lines_to_single_string(&decision) +--- +✔ You approved codex network access to https://example.com:8443 every time this + session diff --git a/codex-rs/tui/src/chatwidget/tool_requests.rs b/codex-rs/tui/src/chatwidget/tool_requests.rs index 7ec567325b..de8fe6a704 100644 --- a/codex-rs/tui/src/chatwidget/tool_requests.rs +++ b/codex-rs/tui/src/chatwidget/tool_requests.rs @@ -149,7 +149,7 @@ impl ChatWidget { if ev.status == GuardianAssessmentStatus::Approved { let cell = if let Some(command) = guardian_command(&ev.action) { history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), crate::history_cell::ReviewDecision::Approved, history_cell::ApprovalDecisionActor::Guardian, ) @@ -169,7 +169,7 @@ impl ChatWidget { if ev.status == GuardianAssessmentStatus::TimedOut { let cell = if let Some(command) = guardian_command(&ev.action) { history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), crate::history_cell::ReviewDecision::TimedOut, history_cell::ApprovalDecisionActor::Guardian, ) @@ -213,7 +213,7 @@ impl ChatWidget { self.review.recent_auto_review_denials.push(ev.clone()); let cell = if let Some(command) = guardian_command(&ev.action) { history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), crate::history_cell::ReviewDecision::Denied, history_cell::ApprovalDecisionActor::Guardian, ) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 4521fe9a7b..7b97151edb 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1089,6 +1089,11 @@ fn exec_snippet(command: &[String]) -> String { truncate_exec_snippet(&full_cmd) } +fn non_empty_exec_snippet(command: &[String]) -> Option { + let snippet = exec_snippet(command); + (!snippet.is_empty()).then_some(snippet) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum ReviewDecision { Approved, @@ -1104,8 +1109,14 @@ pub(crate) enum ReviewDecision { Abort, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ApprovalDecisionSubject { + Command(Vec), + NetworkAccess { target: String }, +} + pub fn new_approval_decision_cell( - command: Vec, + subject: ApprovalDecisionSubject, decision: ReviewDecision, actor: ApprovalDecisionActor, ) -> Box { @@ -1113,19 +1124,37 @@ pub fn new_approval_decision_cell( use codex_protocol::approvals::NetworkPolicyRuleAction; let (symbol, summary): (Span<'static>, Vec>) = match decision { - Approved => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + Approved => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + actor.subject().into(), + "approved".bold(), + " codex to run ".into(), + Span::from(snippet).dim(), + " this time".bold(), + ] + } else { + vec![ + actor.subject().into(), + "approved".bold(), + " this request".into(), + " this time".bold(), + ] + }; + ("✔ ".green(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✔ ".green(), vec![ actor.subject().into(), "approved".bold(), - " codex to run ".into(), - snippet, + " codex network access to ".into(), + Span::from(target).dim(), " this time".bold(), ], - ) - } + ), + }, ApprovedExecpolicyAmendment { proposed_execpolicy_amendment, } => { @@ -1140,84 +1169,164 @@ pub fn new_approval_decision_cell( ], ) } - ApprovedForSession => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + ApprovedForSession => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + actor.subject().into(), + "approved".bold(), + " codex to run ".into(), + Span::from(snippet).dim(), + " every time this session".bold(), + ] + } else { + vec![ + actor.subject().into(), + "approved".bold(), + " this request".into(), + " every time this session".bold(), + ] + }; + ("✔ ".green(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✔ ".green(), vec![ actor.subject().into(), "approved".bold(), - " codex to run ".into(), - snippet, - " every time this session".bold(), - ], - ) - } - NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => ( - "✔ ".green(), - vec![ - actor.subject().into(), - "persisted".bold(), - " Codex network access to ".into(), - Span::from(network_policy_amendment.host).dim(), - ], - ), - NetworkPolicyRuleAction::Deny => ( - "✗ ".red(), - vec![ - actor.subject().into(), - "denied".bold(), " codex network access to ".into(), - Span::from(network_policy_amendment.host).dim(), - " and saved that rule".into(), + Span::from(target).dim(), + " every time this session".bold(), ], ), }, - Denied => { - let snippet = Span::from(exec_snippet(&command)).dim(); - let summary = match actor { - ApprovalDecisionActor::User => vec![ + NetworkPolicyAmendment { + network_policy_amendment, + } => { + let target = match subject { + ApprovalDecisionSubject::NetworkAccess { target } => target, + ApprovalDecisionSubject::Command(_) => network_policy_amendment.host, + }; + match network_policy_amendment.action { + NetworkPolicyRuleAction::Allow => ( + "✔ ".green(), + vec![ + actor.subject().into(), + "persisted".bold(), + " Codex network access to ".into(), + Span::from(target).dim(), + ], + ), + NetworkPolicyRuleAction::Deny => ( + "✗ ".red(), + vec![ + actor.subject().into(), + "denied".bold(), + " codex network access to ".into(), + Span::from(target).dim(), + " and saved that rule".into(), + ], + ), + } + } + Denied => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + let snippet = Span::from(snippet).dim(); + match actor { + ApprovalDecisionActor::User => vec![ + actor.subject().into(), + "did not approve".bold(), + " codex to run ".into(), + snippet, + ], + ApprovalDecisionActor::Guardian => vec![ + "Request ".into(), + "denied".bold(), + " for codex to run ".into(), + snippet, + ], + } + } else { + match actor { + ApprovalDecisionActor::User => vec![ + actor.subject().into(), + "did not approve".bold(), + " this request".into(), + ], + ApprovalDecisionActor::Guardian => { + vec!["Request ".into(), "denied".bold()] + } + } + }; + ("✗ ".red(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( + "✗ ".red(), + vec![ actor.subject().into(), "did not approve".bold(), - " codex to run ".into(), - snippet, + " codex network access to ".into(), + Span::from(target).dim(), ], - ApprovalDecisionActor::Guardian => vec![ - "Request ".into(), - "denied".bold(), - " for codex to run ".into(), - snippet, - ], - }; - ("✗ ".red(), summary) - } - TimedOut => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + ), + }, + TimedOut => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + "Review ".into(), + "timed out".bold(), + " before codex could run ".into(), + Span::from(snippet).dim(), + ] + } else { + vec![ + "Review ".into(), + "timed out".bold(), + " before this request could be approved".into(), + ] + }; + ("✗ ".red(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✗ ".red(), vec![ "Review ".into(), "timed out".bold(), - " before codex could run ".into(), - snippet, + " before codex could access ".into(), + Span::from(target).dim(), ], - ) - } - Abort => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + ), + }, + Abort => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + actor.subject().into(), + "canceled".bold(), + " the request to run ".into(), + Span::from(snippet).dim(), + ] + } else { + vec![ + actor.subject().into(), + "canceled".bold(), + " this request".into(), + ] + }; + ("✗ ".red(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✗ ".red(), vec![ actor.subject().into(), "canceled".bold(), - " the request to run ".into(), - snippet, + " the request for codex network access to ".into(), + Span::from(target).dim(), ], - ) - } + ), + }, }; Box::new(PrefixedWrappedHistoryCell::new( diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index be8629542e..c301700eaa 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -1133,7 +1133,7 @@ mod tests { cells.push(apply_begin_cell); let apply_end_cell: Arc = history_cell::new_approval_decision_cell( - vec!["ls".into()], + history_cell::ApprovalDecisionSubject::Command(vec!["ls".into()]), ReviewDecision::Approved, history_cell::ApprovalDecisionActor::User, )