From 4c9b4b684f2ad060790844b8c809714469520aac Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 7 Jan 2026 11:22:13 -0800 Subject: [PATCH 01/13] Fix app-server `write_models_cache` to treat models with less priority number as higher priority. (#8844) Rank models with p0 higher than p1. This shouldn't result in any behavioral changes. Just reordering. --- .../app-server/tests/common/models_cache.rs | 9 +- .../app-server/tests/suite/v2/model_list.rs | 86 +++++++++---------- 2 files changed, 47 insertions(+), 48 deletions(-) diff --git a/codex-rs/app-server/tests/common/models_cache.rs b/codex-rs/app-server/tests/common/models_cache.rs index f6ac1fad7e..31b614ce5d 100644 --- a/codex-rs/app-server/tests/common/models_cache.rs +++ b/codex-rs/app-server/tests/common/models_cache.rs @@ -40,7 +40,6 @@ fn preset_to_info(preset: &ModelPreset, priority: i32) -> ModelInfo { } } -// todo(aibrahim): fix the priorities to be the opposite here. /// Write a models_cache.json file to the codex home directory. /// This prevents ModelsManager from making network requests to refresh models. /// The cache will be treated as fresh (within TTL) and used instead of fetching from the network. @@ -51,14 +50,14 @@ pub fn write_models_cache(codex_home: &Path) -> std::io::Result<()> { .iter() .filter(|preset| preset.show_in_picker) .collect(); - // Convert presets to ModelInfo, assigning priorities (higher = earlier in list) - // Priority is used for sorting, so first model gets highest priority + // Convert presets to ModelInfo, assigning priorities (lower = earlier in list). + // Priority is used for sorting, so the first model gets the lowest priority. let models: Vec = presets .iter() .enumerate() .map(|(idx, preset)| { - // Higher priority = earlier in list, so reverse the index - let priority = (presets.len() - idx) as i32; + // Lower priority = earlier in list. + let priority = idx as i32; preset_to_info(preset, priority) }) .collect(); 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 4cdb306205..c98da19345 100644 --- a/codex-rs/app-server/tests/suite/v2/model_list.rs +++ b/codex-rs/app-server/tests/suite/v2/model_list.rs @@ -48,29 +48,23 @@ async fn list_models_returns_all_models_with_large_limit() -> Result<()> { let expected_models = vec![ Model { - id: "gpt-5.2".to_string(), - model: "gpt-5.2".to_string(), - display_name: "gpt-5.2".to_string(), - description: - "Latest frontier model with improvements across knowledge, reasoning and coding" - .to_string(), + id: "gpt-5.2-codex".to_string(), + model: "gpt-5.2-codex".to_string(), + display_name: "gpt-5.2-codex".to_string(), + description: "Latest frontier agentic coding model.".to_string(), supported_reasoning_efforts: vec![ ReasoningEffortOption { reasoning_effort: ReasoningEffort::Low, - description: "Balances speed with some reasoning; useful for straightforward \ - queries and short explanations" - .to_string(), + description: "Fast responses with lighter reasoning".to_string(), }, ReasoningEffortOption { reasoning_effort: ReasoningEffort::Medium, - description: "Provides a solid balance of reasoning depth and latency for \ - general-purpose tasks" + description: "Balances speed and reasoning depth for everyday tasks" .to_string(), }, ReasoningEffortOption { reasoning_effort: ReasoningEffort::High, - description: "Maximizes reasoning depth for complex or ambiguous problems" - .to_string(), + description: "Greater reasoning depth for complex problems".to_string(), }, ReasoningEffortOption { reasoning_effort: ReasoningEffort::XHigh, @@ -80,25 +74,6 @@ async fn list_models_returns_all_models_with_large_limit() -> Result<()> { default_reasoning_effort: ReasoningEffort::Medium, is_default: true, }, - Model { - id: "gpt-5.1-codex-mini".to_string(), - model: "gpt-5.1-codex-mini".to_string(), - display_name: "gpt-5.1-codex-mini".to_string(), - description: "Optimized for codex. Cheaper, faster, but less capable.".to_string(), - supported_reasoning_efforts: vec![ - ReasoningEffortOption { - reasoning_effort: ReasoningEffort::Medium, - description: "Dynamically adjusts reasoning based on the task".to_string(), - }, - ReasoningEffortOption { - reasoning_effort: ReasoningEffort::High, - description: "Maximizes reasoning depth for complex or ambiguous problems" - .to_string(), - }, - ], - default_reasoning_effort: ReasoningEffort::Medium, - is_default: false, - }, Model { id: "gpt-5.1-codex-max".to_string(), model: "gpt-5.1-codex-max".to_string(), @@ -127,23 +102,48 @@ async fn list_models_returns_all_models_with_large_limit() -> Result<()> { is_default: false, }, Model { - id: "gpt-5.2-codex".to_string(), - model: "gpt-5.2-codex".to_string(), - display_name: "gpt-5.2-codex".to_string(), - description: "Latest frontier agentic coding model.".to_string(), + id: "gpt-5.1-codex-mini".to_string(), + model: "gpt-5.1-codex-mini".to_string(), + display_name: "gpt-5.1-codex-mini".to_string(), + description: "Optimized for codex. Cheaper, faster, but less capable.".to_string(), + supported_reasoning_efforts: vec![ + ReasoningEffortOption { + reasoning_effort: ReasoningEffort::Medium, + description: "Dynamically adjusts reasoning based on the task".to_string(), + }, + ReasoningEffortOption { + reasoning_effort: ReasoningEffort::High, + description: "Maximizes reasoning depth for complex or ambiguous problems" + .to_string(), + }, + ], + default_reasoning_effort: ReasoningEffort::Medium, + is_default: false, + }, + Model { + id: "gpt-5.2".to_string(), + model: "gpt-5.2".to_string(), + display_name: "gpt-5.2".to_string(), + description: + "Latest frontier model with improvements across knowledge, reasoning and coding" + .to_string(), supported_reasoning_efforts: vec![ ReasoningEffortOption { reasoning_effort: ReasoningEffort::Low, - description: "Fast responses with lighter reasoning".to_string(), + description: "Balances speed with some reasoning; useful for straightforward \ + queries and short explanations" + .to_string(), }, ReasoningEffortOption { reasoning_effort: ReasoningEffort::Medium, - description: "Balances speed and reasoning depth for everyday tasks" + description: "Provides a solid balance of reasoning depth and latency for \ + general-purpose tasks" .to_string(), }, ReasoningEffortOption { reasoning_effort: ReasoningEffort::High, - description: "Greater reasoning depth for complex problems".to_string(), + description: "Maximizes reasoning depth for complex or ambiguous problems" + .to_string(), }, ReasoningEffortOption { reasoning_effort: ReasoningEffort::XHigh, @@ -187,7 +187,7 @@ async fn list_models_pagination_works() -> Result<()> { } = to_response::(first_response)?; assert_eq!(first_items.len(), 1); - assert_eq!(first_items[0].id, "gpt-5.2"); + assert_eq!(first_items[0].id, "gpt-5.2-codex"); let next_cursor = first_cursor.ok_or_else(|| anyhow!("cursor for second page"))?; let second_request = mcp @@ -209,7 +209,7 @@ async fn list_models_pagination_works() -> Result<()> { } = to_response::(second_response)?; assert_eq!(second_items.len(), 1); - assert_eq!(second_items[0].id, "gpt-5.1-codex-mini"); + assert_eq!(second_items[0].id, "gpt-5.1-codex-max"); let third_cursor = second_cursor.ok_or_else(|| anyhow!("cursor for third page"))?; let third_request = mcp @@ -231,7 +231,7 @@ async fn list_models_pagination_works() -> Result<()> { } = to_response::(third_response)?; assert_eq!(third_items.len(), 1); - assert_eq!(third_items[0].id, "gpt-5.1-codex-max"); + assert_eq!(third_items[0].id, "gpt-5.1-codex-mini"); let fourth_cursor = third_cursor.ok_or_else(|| anyhow!("cursor for fourth page"))?; let fourth_request = mcp @@ -253,7 +253,7 @@ async fn list_models_pagination_works() -> Result<()> { } = to_response::(fourth_response)?; assert_eq!(fourth_items.len(), 1); - assert_eq!(fourth_items[0].id, "gpt-5.2-codex"); + assert_eq!(fourth_items[0].id, "gpt-5.2"); assert!(fourth_cursor.is_none()); Ok(()) } From 1253d196412dbf834c4767c953c3560d870bcc5a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Wed, 7 Jan 2026 19:54:32 +0000 Subject: [PATCH 02/13] chore: drop useless feature flags (#8850) --- codex-rs/core/src/codex.rs | 100 ++++++++++----------------- codex-rs/core/src/config/mod.rs | 4 +- codex-rs/core/src/features.rs | 34 --------- codex-rs/core/src/features/legacy.rs | 7 -- codex-rs/core/src/tasks/review.rs | 3 +- codex-rs/core/src/tools/spec.rs | 14 +--- codex-rs/core/tests/suite/client.rs | 2 - codex-rs/core/tests/suite/skills.rs | 43 ++++-------- 8 files changed, 58 insertions(+), 149 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 98b34d51a2..3251ed118b 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -226,29 +226,22 @@ impl Codex { let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_event, rx_event) = async_channel::unbounded(); - let loaded_skills = if config.features.enabled(Feature::Skills) { - Some(skills_manager.skills_for_config(&config)) - } else { - None - }; + let loaded_skills = skills_manager.skills_for_config(&config); + // let loaded_skills = if config.features.enabled(Feature::Skills) { + // Some(skills_manager.skills_for_config(&config)) + // } else { + // None + // }; - if let Some(outcome) = &loaded_skills { - for err in &outcome.errors { - error!( - "failed to load skill {}: {}", - err.path.display(), - err.message - ); - } + for err in &loaded_skills.errors { + error!( + "failed to load skill {}: {}", + err.path.display(), + err.message + ); } - let user_instructions = get_user_instructions( - &config, - loaded_skills - .as_ref() - .map(|outcome| outcome.skills.as_slice()), - ) - .await; + let user_instructions = get_user_instructions(&config, Some(&loaded_skills.skills)).await; let exec_policy = ExecPolicyManager::load(&config.features, &config.config_layer_stack) .await @@ -1287,10 +1280,6 @@ impl Session { } pub(crate) async fn record_model_warning(&self, message: impl Into, ctx: &TurnContext) { - if !self.enabled(Feature::ModelWarnings) { - return; - } - let item = ResponseItem::Message { id: None, role: "user".to_string(), @@ -1747,7 +1736,7 @@ mod handlers { use crate::codex::spawn_review_thread; use crate::config::Config; - use crate::features::Feature; + use crate::mcp::auth::compute_auth_statuses; use crate::mcp::collect_mcp_snapshot_from_manager; use crate::review_prompts::resolve_review_request; @@ -2037,29 +2026,20 @@ mod handlers { } else { cwds }; - let skills = if sess.enabled(Feature::Skills) { - let skills_manager = &sess.services.skills_manager; - let mut entries = Vec::new(); - for cwd in cwds { - let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; - let errors = super::errors_to_info(&outcome.errors); - let skills = super::skills_to_info(&outcome.skills); - entries.push(SkillsListEntry { - cwd, - skills, - errors, - }); - } - entries - } else { - cwds.into_iter() - .map(|cwd| SkillsListEntry { - cwd, - skills: Vec::new(), - errors: Vec::new(), - }) - .collect() - }; + + let skills_manager = &sess.services.skills_manager; + let mut skills = Vec::new(); + for cwd in cwds { + let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; + let errors = super::errors_to_info(&outcome.errors); + let skills_metadata = super::skills_to_info(&outcome.skills); + skills.push(SkillsListEntry { + cwd, + skills: skills_metadata, + errors, + }); + } + let event = Event { id: sub_id, msg: EventMsg::ListSkillsResponse(ListSkillsResponseEvent { skills }), @@ -2212,8 +2192,7 @@ async fn spawn_review_thread( let mut review_features = sess.features.clone(); review_features .disable(crate::features::Feature::WebSearchRequest) - .disable(crate::features::Feature::WebSearchCached) - .disable(crate::features::Feature::ViewImageTool); + .disable(crate::features::Feature::WebSearchCached); let tools_config = ToolsConfig::new(&ToolsConfigParams { model_info: &review_model_info, features: &review_features, @@ -2345,16 +2324,12 @@ pub(crate) async fn run_task( }); sess.send_event(&turn_context, event).await; - let skills_outcome = if sess.enabled(Feature::Skills) { - Some( - sess.services - .skills_manager - .skills_for_cwd(&turn_context.cwd, false) - .await, - ) - } else { - None - }; + let skills_outcome = Some( + sess.services + .skills_manager + .skills_for_cwd(&turn_context.cwd, false) + .await, + ); let SkillInjections { items: skill_items, @@ -2519,7 +2494,7 @@ async fn run_turn( let prompt = Prompt { input, tools: router.specs(), - parallel_tool_calls: model_supports_parallel && sess.enabled(Feature::ParallelToolCalls), + parallel_tool_calls: model_supports_parallel, base_instructions_override: turn_context.base_instructions.clone(), output_schema: turn_context.final_output_json_schema.clone(), }; @@ -3656,8 +3631,7 @@ mod tests { #[tokio::test] async fn record_model_warning_appends_user_message() { let (mut session, turn_context) = make_session_and_context().await; - let mut features = Features::with_defaults(); - features.enable(Feature::ModelWarnings); + let features = Features::with_defaults(); session.features = features; session diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index e2c3bb252d..71b14973d8 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1883,7 +1883,7 @@ trust_level = "trusted" profiles.insert( "work".to_string(), ConfigProfile { - tools_view_image: Some(false), + tools_web_search: Some(false), ..Default::default() }, ); @@ -1899,7 +1899,7 @@ trust_level = "trusted" codex_home.path().to_path_buf(), )?; - assert!(!config.features.enabled(Feature::ViewImageTool)); + assert!(!config.features.enabled(Feature::WebSearchRequest)); Ok(()) } diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 2c1dcc9582..7ef74cfb7a 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -60,10 +60,6 @@ pub enum Feature { // Stable. /// Create a ghost commit at each turn. GhostCommit, - /// Include the view_image tool. - ViewImageTool, - /// Send warnings to the model to correct it on the tool usage. - ModelWarnings, /// Enable the default shell tool. ShellTool, @@ -87,14 +83,10 @@ pub enum Feature { RemoteCompaction, /// Refresh remote models and emit AppReady once the list is available. RemoteModels, - /// Allow model to call multiple tools in parallel (only for models supporting it). - ParallelToolCalls, /// Experimental shell snapshotting. ShellSnapshot, /// Experimental TUI v2 (viewport) implementation. Tui2, - /// Enable discovery and injection of skills. - Skills, /// Enforce UTF8 output in Powershell. PowershellUtf8, } @@ -231,7 +223,6 @@ impl Features { experimental_use_freeform_apply_patch: cfg.experimental_use_freeform_apply_patch, experimental_use_unified_exec_tool: cfg.experimental_use_unified_exec_tool, tools_web_search: cfg.tools.as_ref().and_then(|t| t.web_search), - tools_view_image: cfg.tools.as_ref().and_then(|t| t.view_image), ..Default::default() }; base_legacy.apply(&mut features); @@ -247,7 +238,6 @@ impl Features { experimental_use_unified_exec_tool: config_profile.experimental_use_unified_exec_tool, tools_web_search: config_profile.tools_web_search, - tools_view_image: config_profile.tools_view_image, }; profile_legacy.apply(&mut features); if let Some(profile_features) = config_profile.features.as_ref() { @@ -303,30 +293,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: false, }, - FeatureSpec { - id: Feature::ParallelToolCalls, - key: "parallel", - stage: Stage::Stable, - default_enabled: true, - }, - FeatureSpec { - id: Feature::ViewImageTool, - key: "view_image_tool", - stage: Stage::Stable, - default_enabled: true, - }, FeatureSpec { id: Feature::ShellTool, key: "shell_tool", stage: Stage::Stable, default_enabled: true, }, - FeatureSpec { - id: Feature::ModelWarnings, - key: "warnings", - stage: Stage::Stable, - default_enabled: true, - }, FeatureSpec { id: Feature::WebSearchRequest, key: "web_search_request", @@ -396,12 +368,6 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Experimental, default_enabled: false, }, - FeatureSpec { - id: Feature::Skills, - key: "skills", - stage: Stage::Experimental, - default_enabled: true, - }, FeatureSpec { id: Feature::PowershellUtf8, key: "powershell_utf8", diff --git a/codex-rs/core/src/features/legacy.rs b/codex-rs/core/src/features/legacy.rs index 09a982569f..ed508ffb5a 100644 --- a/codex-rs/core/src/features/legacy.rs +++ b/codex-rs/core/src/features/legacy.rs @@ -47,7 +47,6 @@ pub struct LegacyFeatureToggles { pub experimental_use_freeform_apply_patch: Option, pub experimental_use_unified_exec_tool: Option, pub tools_web_search: Option, - pub tools_view_image: Option, } impl LegacyFeatureToggles { @@ -76,12 +75,6 @@ impl LegacyFeatureToggles { self.tools_web_search, "tools.web_search", ); - set_if_some( - features, - Feature::ViewImageTool, - self.tools_view_image, - "tools.view_image", - ); } } diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index 4a2b587af4..c68fe1ce96 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -85,8 +85,7 @@ async fn start_review_conversation( // re-enable blocked tools (web search, view image). sub_agent_config .features - .disable(crate::features::Feature::WebSearchRequest) - .disable(crate::features::Feature::ViewImageTool); + .disable(crate::features::Feature::WebSearchRequest); // Set explicit review rubric for the sub-agent sub_agent_config.base_instructions = Some(crate::REVIEW_PROMPT.to_string()); diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index a92a624a7e..846025d58e 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -22,7 +22,6 @@ pub(crate) struct ToolsConfig { pub apply_patch_tool_type: Option, pub web_search_request: bool, pub web_search_cached: bool, - pub include_view_image_tool: bool, pub experimental_supported_tools: Vec, } @@ -40,7 +39,6 @@ impl ToolsConfig { let include_apply_patch_tool = features.enabled(Feature::ApplyPatchFreeform); let include_web_search_request = features.enabled(Feature::WebSearchRequest); let include_web_search_cached = features.enabled(Feature::WebSearchCached); - let include_view_image_tool = features.enabled(Feature::ViewImageTool); let shell_type = if !features.enabled(Feature::ShellTool) { ConfigShellToolType::Disabled @@ -72,7 +70,6 @@ impl ToolsConfig { apply_patch_tool_type, web_search_request: include_web_search_request, web_search_cached: include_web_search_cached, - include_view_image_tool, experimental_supported_tools: model_info.experimental_supported_tools.clone(), } } @@ -1107,10 +1104,8 @@ pub(crate) fn build_specs( }); } - if config.include_view_image_tool { - builder.push_spec_with_parallel_support(create_view_image_tool(), true); - builder.register_handler("view_image", view_image_handler); - } + builder.push_spec_with_parallel_support(create_view_image_tool(), true); + builder.register_handler("view_image", view_image_handler); if let Some(mcp_tools) = mcp_tools { let mut entries: Vec<(String, mcp_types::Tool)> = mcp_tools.into_iter().collect(); @@ -1236,7 +1231,6 @@ mod tests { let mut features = Features::with_defaults(); features.enable(Feature::UnifiedExec); features.enable(Feature::WebSearchRequest); - features.enable(Feature::ViewImageTool); let config = ToolsConfig::new(&ToolsConfigParams { model_info: &model_info, features: &features, @@ -1556,7 +1550,6 @@ mod tests { let config = test_config(); let model_info = ModelsManager::construct_model_info_offline("gpt-5-codex", &config); let mut features = Features::with_defaults(); - features.disable(Feature::ViewImageTool); features.enable(Feature::UnifiedExec); let tools_config = ToolsConfig::new(&ToolsConfigParams { model_info: &model_info, @@ -1575,8 +1568,7 @@ mod tests { fn test_test_model_info_includes_sync_tool() { let config = test_config(); let model_info = ModelsManager::construct_model_info_offline("test-gpt-5-codex", &config); - let mut features = Features::with_defaults(); - features.disable(Feature::ViewImageTool); + let features = Features::with_defaults(); let tools_config = ToolsConfig::new(&ToolsConfigParams { model_info: &model_info, features: &features, diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 0e5fe0d9bd..2c08083ba5 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -15,7 +15,6 @@ use codex_core::WireApi; use codex_core::auth::AuthCredentialsStoreMode; use codex_core::built_in_model_providers; use codex_core::error::CodexErr; -use codex_core::features::Feature; use codex_core::models_manager::manager::ModelsManager; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; @@ -683,7 +682,6 @@ async fn skills_append_to_instructions() { let mut config = load_default_config_for_test(&codex_home).await; config.model_provider = model_provider; config.cwd = codex_home.path().to_path_buf(); - config.features.enable(Feature::Skills); let thread_manager = ThreadManager::with_models_provider_and_home( CodexAuth::from_api_key("Test API Key"), diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index e64b5db3e7..8e9266ee86 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -2,7 +2,6 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Result; -use codex_core::features::Feature; use codex_core::protocol::AskForApproval; use codex_core::protocol::Op; use codex_core::protocol::SandboxPolicy; @@ -41,13 +40,9 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { let server = start_mock_server().await; let skill_body = "skill body"; - let mut builder = test_codex() - .with_config(|config| { - config.features.enable(Feature::Skills); - }) - .with_pre_build_hook(|home| { - write_skill(home, "demo", "demo skill", skill_body); - }); + let mut builder = test_codex().with_pre_build_hook(|home| { + write_skill(home, "demo", "demo skill", skill_body); + }); let test = builder.build(&server).await?; let skill_path = test.codex_home_path().join("skills/demo/SKILL.md"); @@ -111,15 +106,11 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let mut builder = test_codex() - .with_config(|config| { - config.features.enable(Feature::Skills); - }) - .with_pre_build_hook(|home| { - let skill_dir = home.join("skills").join("broken"); - fs::create_dir_all(&skill_dir).unwrap(); - fs::write(skill_dir.join("SKILL.md"), "not yaml").unwrap(); - }); + let mut builder = test_codex().with_pre_build_hook(|home| { + let skill_dir = home.join("skills").join("broken"); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write(skill_dir.join("SKILL.md"), "not yaml").unwrap(); + }); let test = builder.build(&server).await?; test.codex @@ -169,17 +160,13 @@ async fn list_skills_includes_system_cache_entries() -> Result<()> { const SYSTEM_SKILL_NAME: &str = "skill-creator"; let server = start_mock_server().await; - let mut builder = test_codex() - .with_config(|config| { - config.features.enable(Feature::Skills); - }) - .with_pre_build_hook(|home| { - let system_skill_path = system_skill_md_path(home, SYSTEM_SKILL_NAME); - assert!( - !system_skill_path.exists(), - "expected embedded system skills not yet installed, but {system_skill_path:?} exists" - ); - }); + let mut builder = test_codex().with_pre_build_hook(|home| { + let system_skill_path = system_skill_md_path(home, SYSTEM_SKILL_NAME); + assert!( + !system_skill_path.exists(), + "expected embedded system skills not yet installed, but {system_skill_path:?} exists" + ); + }); let test = builder.build(&server).await?; let system_skill_path = system_skill_md_path(test.codex_home_path(), SYSTEM_SKILL_NAME); From fe460e0f9a59214a50b55ebdeed6eebce4984293 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Wed, 7 Jan 2026 19:54:45 +0000 Subject: [PATCH 03/13] chore: drop some deprecated (#8848) --- codex-rs/core/src/thread_manager.rs | 48 ----------------------------- 1 file changed, 48 deletions(-) diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 742de1c530..d82b242fe5 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -169,53 +169,6 @@ impl ThreadManager { .await } - #[deprecated(note = "use get_thread")] - pub async fn get_conversation(&self, thread_id: ThreadId) -> CodexResult> { - self.get_thread(thread_id).await - } - - #[deprecated(note = "use start_thread")] - pub async fn new_conversation(&self, config: Config) -> CodexResult { - self.start_thread(config).await - } - - #[deprecated(note = "use resume_thread_from_rollout")] - pub async fn resume_conversation_from_rollout( - &self, - config: Config, - rollout_path: PathBuf, - auth_manager: Arc, - ) -> CodexResult { - self.resume_thread_from_rollout(config, rollout_path, auth_manager) - .await - } - - #[deprecated(note = "use resume_thread_with_history")] - pub async fn resume_conversation_with_history( - &self, - config: Config, - initial_history: InitialHistory, - auth_manager: Arc, - ) -> CodexResult { - self.resume_thread_with_history(config, initial_history, auth_manager) - .await - } - - #[deprecated(note = "use remove_thread")] - pub async fn remove_conversation(&self, thread_id: &ThreadId) -> Option> { - self.remove_thread(thread_id).await - } - - #[deprecated(note = "use fork_thread")] - pub async fn fork_conversation( - &self, - nth_user_message: usize, - config: Config, - path: PathBuf, - ) -> CodexResult { - self.fork_thread(nth_user_message, config, path).await - } - /// Removes the thread from the manager's internal map, though the thread is stored /// as `Arc`, it is possible that other references to it exist elsewhere. /// Returns the thread if the thread was found and removed. @@ -322,7 +275,6 @@ impl ThreadManagerState { )); self.threads.write().await.insert(thread_id, thread.clone()); - #[allow(deprecated)] Ok(NewThread { thread_id, thread, From e8421c761ca3c7f8b0187b3e775b8c22d9ed7781 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Wed, 7 Jan 2026 12:07:01 -0800 Subject: [PATCH 04/13] [chore] update app server doc with skills (#8853) --- codex-rs/app-server/README.md | 45 +++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index b8f7de90ab..88d96dda56 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -11,6 +11,8 @@ - [Initialization](#initialization) - [API Overview](#api-overview) - [Events](#events) +- [Approvals](#approvals) +- [Skills](#skills) - [Auth endpoints](#auth-endpoints) ## Protocol @@ -196,6 +198,25 @@ You can optionally specify config overrides on the new turn. If specified, these } } } ``` +### Example: Start a turn (invoke a skill) + +Invoke a skill by sending a text input that begins with `$`. + +```json +{ "method": "turn/start", "id": 33, "params": { + "threadId": "thr_123", + "input": [ + { "type": "text", "text": "$skill-creator Add a new skill for triaging flaky CI and include step-by-step usage." } + ] +} } +{ "id": 33, "result": { "turn": { + "id": "turn_457", + "status": "inProgress", + "items": [], + "error": null +} } } +``` + ### Example: Interrupt an active turn You can cancel a running Turn with `turn/interrupt`. @@ -405,6 +426,30 @@ Order of messages: UI guidance for IDEs: surface an approval dialog as soon as the request arrives. The turn will proceed after the server receives a response to the approval request. The terminal `item/completed` notification will be sent with the appropriate status. +## Skills + +Skills are invoked by sending a text input that starts with `$`. The rest of the text is passed to the skill as its input. + +Example: + +``` +$skill-creator Add a new skill for triaging flaky CI and include step-by-step usage. +``` + +Use `skills/list` to fetch the available skills (optionally scoped by `cwd` and/or with `forceReload`). + +```json +{ "method": "skills/list", "id": 25, "params": { + "cwd": "/Users/me/project", + "forceReload": false +} } +{ "id": 25, "result": { + "skills": [ + { "name": "skill-creator", "description": "Create or update a Codex skill" } + ] +} } +``` + ## Auth endpoints The JSON-RPC auth/account surface exposes request/response methods plus server-initiated notifications (no `id`). Use these to determine auth state, start or cancel logins, logout, and inspect ChatGPT rate limits. From 66450f04457faffbe97f6ae725bd95b589d26f39 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Wed, 7 Jan 2026 12:11:12 -0800 Subject: [PATCH 05/13] fix: implement 'Allow this session' for apply_patch approvals (#8451) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Summary** This PR makes “ApprovalDecision::AcceptForSession / don’t ask again this session” actually work for `apply_patch` approvals by caching approvals based on absolute file paths in codex-core, properly wiring it through app-server v2, and exposing the choice in both TUI and TUI2. - This brings `apply_patch` calls to be at feature-parity with general shell commands, which also have a "Yes, and don't ask again" option. - This also fixes VSCE's "Allow this session" button to actually work. While we're at it, also split the app-server v2 protocol's `ApprovalDecision` enum so execpolicy amendments are only available for command execution approvals. **Key changes** - Core: per-session patch approval allowlist keyed by absolute file paths - Handles multi-file patches and renames/moves by recording both source and destination paths for `Update { move_path: Some(...) }`. - Extend the `Approvable` trait and `ApplyPatchRuntime` to work with multiple keys, because an `apply_patch` tool call can modify multiple files. For a request to be auto-approved, we will need to check that all file paths have been approved previously. - App-server v2: honor AcceptForSession for file changes - File-change approval responses now map AcceptForSession to ReviewDecision::ApprovedForSession (no longer downgraded to plain Approved). - Replace `ApprovalDecision` with two enums: `CommandExecutionApprovalDecision` and `FileChangeApprovalDecision` - TUI / TUI2: expose “don’t ask again for these files this session” - Patch approval overlays now include a third option (“Yes, and don’t ask again for these files this session (s)”). - Snapshot updates for the approval modal. **Tests added/updated** - Core: - Integration test that proves ApprovedForSession on a patch skips the next patch prompt for the same file - App-server: - v2 integration test verifying FileChangeApprovalDecision::AcceptForSession works properly **User-visible behavior** - When the user approves a patch “for session”, future patches touching only those previously approved file(s) will no longer prompt gain during that session (both via app-server v2 and TUI/TUI2). **Manual testing** Tested both TUI and TUI2 - see screenshots below. TUI: image TUI2: image --- .../app-server-protocol/src/protocol/v2.rs | 27 ++- codex-rs/app-server-test-client/src/main.rs | 7 +- .../app-server/src/bespoke_event_handling.rs | 57 ++++-- .../app-server/tests/suite/v2/turn_start.rs | 193 +++++++++++++++++- codex-rs/core/src/apply_patch.rs | 56 ++--- codex-rs/core/src/safety.rs | 2 +- codex-rs/core/src/tools/events.rs | 7 +- .../core/src/tools/handlers/apply_patch.rs | 94 +++++++-- .../core/src/tools/runtimes/apply_patch.rs | 77 +++---- codex-rs/core/src/tools/runtimes/shell.rs | 10 +- .../core/src/tools/runtimes/unified_exec.rs | 10 +- codex-rs/core/src/tools/sandboxing.rs | 39 +++- codex-rs/core/tests/suite/approvals.rs | 117 +++++++++++ .../tui/src/bottom_pane/approval_overlay.rs | 6 + ...atwidget__tests__approval_modal_patch.snap | 5 +- .../tui2/src/bottom_pane/approval_overlay.rs | 6 + ...atwidget__tests__approval_modal_patch.snap | 5 +- ...atwidget__tests__approval_modal_patch.snap | 5 +- codex-rs/utils/absolute-path/src/lib.rs | 2 +- 19 files changed, 576 insertions(+), 149 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 6d0c009b42..ee06115806 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -487,14 +487,33 @@ pub struct ConfigEdit { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] -pub enum ApprovalDecision { +pub enum CommandExecutionApprovalDecision { + /// User approved the command. Accept, - /// Approve and remember the approval for the session. + /// User approved the command and future identical commands should run without prompting. AcceptForSession, + /// User approved the command, and wants to apply the proposed execpolicy amendment so future + /// matching commands can run without prompting. AcceptWithExecpolicyAmendment { execpolicy_amendment: ExecPolicyAmendment, }, + /// User denied the command. The agent will continue the turn. Decline, + /// User denied the command. The turn will also be immediately interrupted. + Cancel, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum FileChangeApprovalDecision { + /// User approved the file changes. + Accept, + /// User approved the file changes and future changes to the same files should run without prompting. + AcceptForSession, + /// User denied the file changes. The agent will continue the turn. + Decline, + /// User denied the file changes. The turn will also be immediately interrupted. Cancel, } @@ -1885,7 +1904,7 @@ pub struct CommandExecutionRequestApprovalParams { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct CommandExecutionRequestApprovalResponse { - pub decision: ApprovalDecision, + pub decision: CommandExecutionApprovalDecision, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -1905,7 +1924,7 @@ pub struct FileChangeRequestApprovalParams { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[ts(export_to = "v2/")] pub struct FileChangeRequestApprovalResponse { - pub decision: ApprovalDecision, + pub decision: FileChangeApprovalDecision, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server-test-client/src/main.rs b/codex-rs/app-server-test-client/src/main.rs index 389961cb0d..64577a5539 100644 --- a/codex-rs/app-server-test-client/src/main.rs +++ b/codex-rs/app-server-test-client/src/main.rs @@ -18,12 +18,13 @@ use clap::Parser; use clap::Subcommand; use codex_app_server_protocol::AddConversationListenerParams; use codex_app_server_protocol::AddConversationSubscriptionResponse; -use codex_app_server_protocol::ApprovalDecision; use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::ClientInfo; use codex_app_server_protocol::ClientRequest; +use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; +use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeRequestApprovalParams; use codex_app_server_protocol::FileChangeRequestApprovalResponse; use codex_app_server_protocol::GetAccountRateLimitsResponse; @@ -845,7 +846,7 @@ impl CodexClient { } let response = CommandExecutionRequestApprovalResponse { - decision: ApprovalDecision::Accept, + decision: CommandExecutionApprovalDecision::Accept, }; self.send_server_request_response(request_id, &response)?; println!("< approved commandExecution request for item {item_id}"); @@ -876,7 +877,7 @@ impl CodexClient { } let response = FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Accept, + decision: FileChangeApprovalDecision::Accept, }; self.send_server_request_response(request_id, &response)?; println!("< approved fileChange request for item {item_id}"); diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 8fb06ca2ed..d18a7878f4 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -13,9 +13,9 @@ use codex_app_server_protocol::AccountRateLimitsUpdatedNotification; use codex_app_server_protocol::AgentMessageDeltaNotification; use codex_app_server_protocol::ApplyPatchApprovalParams; use codex_app_server_protocol::ApplyPatchApprovalResponse; -use codex_app_server_protocol::ApprovalDecision; use codex_app_server_protocol::CodexErrorInfo as V2CodexErrorInfo; use codex_app_server_protocol::CommandAction as V2ParsedCommand; +use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionOutputDeltaNotification; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; @@ -26,6 +26,7 @@ use codex_app_server_protocol::ErrorNotification; use codex_app_server_protocol::ExecCommandApprovalParams; use codex_app_server_protocol::ExecCommandApprovalResponse; use codex_app_server_protocol::ExecPolicyAmendment as V2ExecPolicyAmendment; +use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeOutputDeltaNotification; use codex_app_server_protocol::FileChangeRequestApprovalParams; use codex_app_server_protocol::FileChangeRequestApprovalResponse; @@ -1193,6 +1194,21 @@ fn format_file_change_diff(change: &CoreFileChange) -> String { } } +fn map_file_change_approval_decision( + decision: FileChangeApprovalDecision, +) -> (ReviewDecision, Option) { + match decision { + FileChangeApprovalDecision::Accept => (ReviewDecision::Approved, None), + FileChangeApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None), + FileChangeApprovalDecision::Decline => { + (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)) + } + FileChangeApprovalDecision::Cancel => { + (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)) + } + } +} + #[allow(clippy::too_many_arguments)] async fn on_file_change_request_approval_response( event_turn_id: String, @@ -1211,23 +1227,12 @@ async fn on_file_change_request_approval_response( .unwrap_or_else(|err| { error!("failed to deserialize FileChangeRequestApprovalResponse: {err}"); FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: FileChangeApprovalDecision::Decline, } }); - let (decision, completion_status) = match response.decision { - ApprovalDecision::Accept - | ApprovalDecision::AcceptForSession - | ApprovalDecision::AcceptWithExecpolicyAmendment { .. } => { - (ReviewDecision::Approved, None) - } - ApprovalDecision::Decline => { - (ReviewDecision::Denied, Some(PatchApplyStatus::Declined)) - } - ApprovalDecision::Cancel => { - (ReviewDecision::Abort, Some(PatchApplyStatus::Declined)) - } - }; + let (decision, completion_status) = + map_file_change_approval_decision(response.decision); // Allow EventMsg::PatchApplyEnd to emit ItemCompleted for accepted patches. // Only short-circuit on declines/cancels/failures. (decision, completion_status) @@ -1281,16 +1286,18 @@ async fn on_command_execution_request_approval_response( .unwrap_or_else(|err| { error!("failed to deserialize CommandExecutionRequestApprovalResponse: {err}"); CommandExecutionRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: CommandExecutionApprovalDecision::Decline, } }); let decision = response.decision; let (decision, completion_status) = match decision { - ApprovalDecision::Accept => (ReviewDecision::Approved, None), - ApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None), - ApprovalDecision::AcceptWithExecpolicyAmendment { + CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None), + CommandExecutionApprovalDecision::AcceptForSession => { + (ReviewDecision::ApprovedForSession, None) + } + CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { execpolicy_amendment, } => ( ReviewDecision::ApprovedExecpolicyAmendment { @@ -1298,11 +1305,11 @@ async fn on_command_execution_request_approval_response( }, None, ), - ApprovalDecision::Decline => ( + CommandExecutionApprovalDecision::Decline => ( ReviewDecision::Denied, Some(CommandExecutionStatus::Declined), ), - ApprovalDecision::Cancel => ( + CommandExecutionApprovalDecision::Cancel => ( ReviewDecision::Abort, Some(CommandExecutionStatus::Declined), ), @@ -1442,6 +1449,14 @@ mod tests { Arc::new(Mutex::new(HashMap::new())) } + #[test] + fn file_change_accept_for_session_maps_to_approved_for_session() { + let (decision, completion_status) = + map_file_change_approval_decision(FileChangeApprovalDecision::AcceptForSession); + assert_eq!(decision, ReviewDecision::ApprovedForSession); + assert_eq!(completion_status, None); + } + #[tokio::test] async fn test_handle_error_records_message() -> Result<()> { let conversation_id = ThreadId::new(); diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 60fa656b19..ab450ea832 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -8,9 +8,10 @@ use app_test_support::create_mock_chat_completions_server_unchecked; use app_test_support::create_shell_command_sse_response; use app_test_support::format_with_current_shell_display; use app_test_support::to_response; -use codex_app_server_protocol::ApprovalDecision; +use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; +use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeOutputDeltaNotification; use codex_app_server_protocol::FileChangeRequestApprovalResponse; use codex_app_server_protocol::ItemCompletedNotification; @@ -426,7 +427,7 @@ async fn turn_start_exec_approval_decline_v2() -> Result<()> { mcp.send_response( request_id, serde_json::to_value(CommandExecutionRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: CommandExecutionApprovalDecision::Decline, })?, ) .await?; @@ -722,7 +723,7 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { mcp.send_response( request_id, serde_json::to_value(FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Accept, + decision: FileChangeApprovalDecision::Accept, })?, ) .await?; @@ -782,6 +783,190 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { Ok(()) } +#[tokio::test] +async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let tmp = TempDir::new()?; + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home)?; + let workspace = tmp.path().join("workspace"); + std::fs::create_dir(&workspace)?; + + let patch_1 = r#"*** Begin Patch +*** Add File: README.md ++new line +*** End Patch +"#; + let patch_2 = r#"*** Begin Patch +*** Update File: README.md +@@ +-new line ++updated line +*** End Patch +"#; + + let responses = vec![ + create_apply_patch_sse_response(patch_1, "patch-call-1")?, + create_final_assistant_message_sse_response("patch 1 applied")?, + create_apply_patch_sse_response(patch_2, "patch-call-2")?, + create_final_assistant_message_sse_response("patch 2 applied")?, + ]; + let server = create_mock_chat_completions_server(responses).await; + create_config_toml(&codex_home, &server.uri(), "untrusted")?; + + let mut mcp = McpProcess::new(&codex_home).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_req = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + cwd: Some(workspace.to_string_lossy().into_owned()), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + + // First turn: expect FileChangeRequestApproval, respond with AcceptForSession, and verify the file exists. + let turn_1_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "apply patch 1".into(), + }], + cwd: Some(workspace.clone()), + ..Default::default() + }) + .await?; + let turn_1_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_1_req)), + ) + .await??; + let TurnStartResponse { turn: turn_1 } = to_response::(turn_1_resp)?; + + let started_file_change_1 = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let started_notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(started_notif.params.clone().expect("item/started params"))?; + if let ThreadItem::FileChange { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::FileChange { id, status, .. } = started_file_change_1 else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call-1"); + assert_eq!(status, PatchApplyStatus::InProgress); + + let server_req = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_request_message(), + ) + .await??; + let ServerRequest::FileChangeRequestApproval { request_id, params } = server_req else { + panic!("expected FileChangeRequestApproval request") + }; + assert_eq!(params.item_id, "patch-call-1"); + assert_eq!(params.thread_id, thread.id); + assert_eq!(params.turn_id, turn_1.id); + + mcp.send_response( + request_id, + serde_json::to_value(FileChangeRequestApprovalResponse { + decision: FileChangeApprovalDecision::AcceptForSession, + })?, + ) + .await?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/fileChange/outputDelta"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/completed"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + let readme_path = workspace.join("README.md"); + assert_eq!(std::fs::read_to_string(&readme_path)?, "new line\n"); + + // Second turn: apply a patch to the same file. Approval should be skipped due to AcceptForSession. + let turn_2_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "apply patch 2".into(), + }], + cwd: Some(workspace.clone()), + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_2_req)), + ) + .await??; + + let started_file_change_2 = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let started_notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(started_notif.params.clone().expect("item/started params"))?; + if let ThreadItem::FileChange { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::FileChange { id, status, .. } = started_file_change_2 else { + unreachable!("loop ensures we break on file change items"); + }; + assert_eq!(id, "patch-call-2"); + assert_eq!(status, PatchApplyStatus::InProgress); + + // If the server incorrectly emits FileChangeRequestApproval, the helper below will error + // (it bails on unexpected JSONRPCMessage::Request), causing the test to fail. + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/fileChange/outputDelta"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/completed"), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + assert_eq!(std::fs::read_to_string(readme_path)?, "updated line\n"); + + Ok(()) +} + #[tokio::test] async fn turn_start_file_change_approval_decline_v2() -> Result<()> { skip_if_no_network!(Ok(())); @@ -888,7 +1073,7 @@ async fn turn_start_file_change_approval_decline_v2() -> Result<()> { mcp.send_response( request_id, serde_json::to_value(FileChangeRequestApprovalResponse { - decision: ApprovalDecision::Decline, + decision: FileChangeApprovalDecision::Decline, })?, ) .await?; diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 67433303e5..1a47ca60b7 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,10 +1,9 @@ -use crate::codex::Session; use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; use crate::protocol::FileChange; -use crate::protocol::ReviewDecision; use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; +use crate::tools::sandboxing::ExecApprovalRequirement; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use std::collections::HashMap; @@ -30,13 +29,12 @@ pub(crate) enum InternalApplyPatchInvocation { #[derive(Debug)] pub(crate) struct ApplyPatchExec { pub(crate) action: ApplyPatchAction, - pub(crate) user_explicitly_approved_this_action: bool, + pub(crate) auto_approved: bool, + pub(crate) exec_approval_requirement: ExecApprovalRequirement, } pub(crate) async fn apply_patch( - sess: &Session, turn_context: &TurnContext, - call_id: &str, action: ApplyPatchAction, ) -> InternalApplyPatchInvocation { match assess_patch_safety( @@ -50,40 +48,24 @@ pub(crate) async fn apply_patch( .. } => InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { action, - user_explicitly_approved_this_action: user_explicitly_approved, + auto_approved: !user_explicitly_approved, + exec_approval_requirement: ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, }), SafetyCheck::AskUser => { - // Compute a readable summary of path changes to include in the - // approval request so the user can make an informed decision. - // - // Note that it might be worth expanding this approval request to - // give the user the option to expand the set of writable roots so - // that similar patches can be auto-approved in the future during - // this session. - let rx_approve = sess - .request_patch_approval( - turn_context, - call_id.to_owned(), - convert_apply_patch_to_protocol(&action), - None, - None, - ) - .await; - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession => { - InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { - action, - user_explicitly_approved_this_action: true, - }) - } - ReviewDecision::Denied | ReviewDecision::Abort => { - InternalApplyPatchInvocation::Output(Err(FunctionCallError::RespondToModel( - "patch rejected by user".to_string(), - ))) - } - } + // Delegate the approval prompt (including cached approvals) to the + // tool runtime, consistent with how shell/unified_exec approvals + // are orchestrator-driven. + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + auto_approved: false, + exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + }) } SafetyCheck::Reject { reason } => InternalApplyPatchInvocation::Output(Err( FunctionCallError::RespondToModel(format!("patch rejected: {reason}")), diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index c3930b4f42..601a5a8b81 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -277,7 +277,7 @@ mod tests { }; assert_eq!( - assess_patch_safety(&add_inside, AskForApproval::OnRequest, &policy, &cwd,), + assess_patch_safety(&add_inside, AskForApproval::OnRequest, &policy, &cwd), SafetyCheck::AutoApprove { sandbox_type: SandboxType::None, user_explicitly_approved: false, diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index cdfc575cd9..02aa50d7b6 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -305,7 +305,12 @@ impl ToolEmitter { // Normalize common rejection messages for exec tools so tests and // users see a clear, consistent phrase. let normalized = if msg == "rejected by user" { - "exec command rejected by user".to_string() + match self { + Self::Shell { .. } | Self::UnifiedExec { .. } => { + "exec command rejected by user".to_string() + } + Self::ApplyPatch { .. } => "patch rejected by user".to_string(), + } } else { msg }; diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 14a481f4ea..adf76e6825 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -26,11 +26,38 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::spec::ApplyPatchToolArgs; use crate::tools::spec::JsonSchema; use async_trait::async_trait; +use codex_apply_patch::ApplyPatchAction; +use codex_apply_patch::ApplyPatchFileChange; +use codex_utils_absolute_path::AbsolutePathBuf; pub struct ApplyPatchHandler; const APPLY_PATCH_LARK_GRAMMAR: &str = include_str!("tool_apply_patch.lark"); +fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { + let mut keys = Vec::new(); + let cwd = action.cwd.as_path(); + + for (path, change) in action.changes() { + if let Some(key) = to_abs_path(cwd, path) { + keys.push(key); + } + + if let ApplyPatchFileChange::Update { move_path, .. } = change + && let Some(dest) = move_path + && let Some(key) = to_abs_path(cwd, dest) + { + keys.push(key); + } + } + + keys +} + +fn to_abs_path(cwd: &Path, path: &Path) -> Option { + AbsolutePathBuf::resolve_path_against_base(path, cwd).ok() +} + #[async_trait] impl ToolHandler for ApplyPatchHandler { fn kind(&self) -> ToolKind { @@ -81,9 +108,7 @@ impl ToolHandler for ApplyPatchHandler { let command = vec!["apply_patch".to_string(), patch_input.clone()]; match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { - match apply_patch::apply_patch(session.as_ref(), turn.as_ref(), &call_id, changes) - .await - { + match apply_patch::apply_patch(turn.as_ref(), changes).await { InternalApplyPatchInvocation::Output(item) => { let content = item?; Ok(ToolOutput::Function { @@ -93,10 +118,10 @@ impl ToolHandler for ApplyPatchHandler { }) } InternalApplyPatchInvocation::DelegateToExec(apply) => { - let emitter = ToolEmitter::apply_patch( - convert_apply_patch_to_protocol(&apply.action), - !apply.user_explicitly_approved_this_action, - ); + let changes = convert_apply_patch_to_protocol(&apply.action); + let file_paths = file_paths_for_action(&apply.action); + let emitter = + ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); let event_ctx = ToolEventCtx::new( session.as_ref(), turn.as_ref(), @@ -106,10 +131,11 @@ impl ToolHandler for ApplyPatchHandler { emitter.begin(event_ctx).await; let req = ApplyPatchRequest { - patch: apply.action.patch.clone(), - cwd: apply.action.cwd.clone(), + action: apply.action, + file_paths, + changes, + exec_approval_requirement: apply.exec_approval_requirement, timeout_ms: None, - user_explicitly_approved: apply.user_explicitly_approved_this_action, codex_exe: turn.codex_linux_sandbox_exe.clone(), }; @@ -178,7 +204,7 @@ pub(crate) async fn intercept_apply_patch( turn, ) .await; - match apply_patch::apply_patch(session, turn, call_id, changes).await { + match apply_patch::apply_patch(turn, changes).await { InternalApplyPatchInvocation::Output(item) => { let content = item?; Ok(Some(ToolOutput::Function { @@ -188,19 +214,19 @@ pub(crate) async fn intercept_apply_patch( })) } InternalApplyPatchInvocation::DelegateToExec(apply) => { - let emitter = ToolEmitter::apply_patch( - convert_apply_patch_to_protocol(&apply.action), - !apply.user_explicitly_approved_this_action, - ); + let changes = convert_apply_patch_to_protocol(&apply.action); + let approval_keys = file_paths_for_action(&apply.action); + let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); let event_ctx = ToolEventCtx::new(session, turn, call_id, tracker.as_ref().copied()); emitter.begin(event_ctx).await; let req = ApplyPatchRequest { - patch: apply.action.patch.clone(), - cwd: apply.action.cwd.clone(), + action: apply.action, + file_paths: approval_keys, + changes, + exec_approval_requirement: apply.exec_approval_requirement, timeout_ms, - user_explicitly_approved: apply.user_explicitly_approved_this_action, codex_exe: turn.codex_linux_sandbox_exe.clone(), }; @@ -342,3 +368,35 @@ It is important to remember: }, }) } + +#[cfg(test)] +mod tests { + use super::*; + use codex_apply_patch::MaybeApplyPatchVerified; + use pretty_assertions::assert_eq; + use tempfile::TempDir; + + #[test] + fn approval_keys_include_move_destination() { + let tmp = TempDir::new().expect("tmp"); + let cwd = tmp.path(); + std::fs::create_dir_all(cwd.join("old")).expect("create old dir"); + std::fs::create_dir_all(cwd.join("renamed/dir")).expect("create dest dir"); + std::fs::write(cwd.join("old/name.txt"), "old content\n").expect("write old file"); + let patch = r#"*** Begin Patch +*** Update File: old/name.txt +*** Move to: renamed/dir/name.txt +@@ +-old content ++new content +*** End Patch"#; + let argv = vec!["apply_patch".to_string(), patch.to_string()]; + let action = match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) { + MaybeApplyPatchVerified::Body(action) => action, + other => panic!("expected patch body, got: {other:?}"), + }; + + let keys = file_paths_for_action(&action); + assert_eq!(keys.len(), 2); + } +} diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 26d04f578c..7b9d0dccc5 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -11,6 +11,7 @@ use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; +use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::SandboxablePreference; @@ -18,30 +19,28 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::with_cached_approval; +use codex_apply_patch::ApplyPatchAction; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::ReviewDecision; +use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; use std::collections::HashMap; use std::path::PathBuf; -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct ApplyPatchRequest { - pub patch: String, - pub cwd: PathBuf, + pub action: ApplyPatchAction, + pub file_paths: Vec, + pub changes: std::collections::HashMap, + pub exec_approval_requirement: ExecApprovalRequirement, pub timeout_ms: Option, - pub user_explicitly_approved: bool, pub codex_exe: Option, } #[derive(Default)] pub struct ApplyPatchRuntime; -#[derive(serde::Serialize, Clone, Debug, Eq, PartialEq, Hash)] -pub(crate) struct ApprovalKey { - patch: String, - cwd: PathBuf, -} - impl ApplyPatchRuntime { pub fn new() -> Self { Self @@ -58,8 +57,8 @@ impl ApplyPatchRuntime { let program = exe.to_string_lossy().to_string(); Ok(CommandSpec { program, - args: vec![CODEX_APPLY_PATCH_ARG1.to_string(), req.patch.clone()], - cwd: req.cwd.clone(), + args: vec![CODEX_APPLY_PATCH_ARG1.to_string(), req.action.patch.clone()], + cwd: req.action.cwd.clone(), expiration: req.timeout_ms.into(), // Run apply_patch with a minimal environment for determinism and to avoid leaks. env: HashMap::new(), @@ -87,13 +86,10 @@ impl Sandboxable for ApplyPatchRuntime { } impl Approvable for ApplyPatchRuntime { - type ApprovalKey = ApprovalKey; + type ApprovalKey = AbsolutePathBuf; - fn approval_key(&self, req: &ApplyPatchRequest) -> Self::ApprovalKey { - ApprovalKey { - patch: req.patch.clone(), - cwd: req.cwd.clone(), - } + fn approval_keys(&self, req: &ApplyPatchRequest) -> Vec { + req.file_paths.clone() } fn start_approval_async<'a>( @@ -101,31 +97,25 @@ impl Approvable for ApplyPatchRuntime { req: &'a ApplyPatchRequest, ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision> { - let key = self.approval_key(req); let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); - let cwd = req.cwd.clone(); let retry_reason = ctx.retry_reason.clone(); - let user_explicitly_approved = req.user_explicitly_approved; + let approval_keys = self.approval_keys(req); + let changes = req.changes.clone(); Box::pin(async move { - with_cached_approval(&session.services, key, move || async move { - if let Some(reason) = retry_reason { - session - .request_command_approval( - turn, - call_id, - vec!["apply_patch".to_string()], - cwd, - Some(reason), - None, - ) - .await - } else if user_explicitly_approved { - ReviewDecision::ApprovedForSession - } else { - ReviewDecision::Approved - } + if let Some(reason) = retry_reason { + let rx_approve = session + .request_patch_approval(turn, call_id, changes.clone(), Some(reason), None) + .await; + return rx_approve.await.unwrap_or_default(); + } + + with_cached_approval(&session.services, approval_keys, || async move { + let rx_approve = session + .request_patch_approval(turn, call_id, changes, None, None) + .await; + rx_approve.await.unwrap_or_default() }) .await }) @@ -134,6 +124,17 @@ impl Approvable for ApplyPatchRuntime { fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { !matches!(policy, AskForApproval::Never) } + + // apply_patch approvals are decided upstream by assess_patch_safety. + // + // This override ensures the orchestrator runs the patch approval flow when required instead + // of falling back to the global exec approval policy. + fn exec_approval_requirement( + &self, + req: &ApplyPatchRequest, + ) -> Option { + Some(req.exec_approval_requirement.clone()) + } } impl ToolRuntime for ApplyPatchRuntime { diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 528efad740..49052bc06b 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -74,12 +74,12 @@ impl Sandboxable for ShellRuntime { impl Approvable for ShellRuntime { type ApprovalKey = ApprovalKey; - fn approval_key(&self, req: &ShellRequest) -> Self::ApprovalKey { - ApprovalKey { + fn approval_keys(&self, req: &ShellRequest) -> Vec { + vec![ApprovalKey { command: req.command.clone(), cwd: req.cwd.clone(), sandbox_permissions: req.sandbox_permissions, - } + }] } fn start_approval_async<'a>( @@ -87,7 +87,7 @@ impl Approvable for ShellRuntime { req: &'a ShellRequest, ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision> { - let key = self.approval_key(req); + let keys = self.approval_keys(req); let command = req.command.clone(); let cwd = req.cwd.clone(); let reason = ctx @@ -98,7 +98,7 @@ impl Approvable for ShellRuntime { let turn = ctx.turn; let call_id = ctx.call_id.to_string(); Box::pin(async move { - with_cached_approval(&session.services, key, move || async move { + with_cached_approval(&session.services, keys, move || async move { session .request_command_approval( turn, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index d6362eeb1b..47e9e5ca12 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -92,12 +92,12 @@ impl Sandboxable for UnifiedExecRuntime<'_> { impl Approvable for UnifiedExecRuntime<'_> { type ApprovalKey = UnifiedExecApprovalKey; - fn approval_key(&self, req: &UnifiedExecRequest) -> Self::ApprovalKey { - UnifiedExecApprovalKey { + fn approval_keys(&self, req: &UnifiedExecRequest) -> Vec { + vec![UnifiedExecApprovalKey { command: req.command.clone(), cwd: req.cwd.clone(), sandbox_permissions: req.sandbox_permissions, - } + }] } fn start_approval_async<'b>( @@ -105,7 +105,7 @@ impl Approvable for UnifiedExecRuntime<'_> { req: &'b UnifiedExecRequest, ctx: ApprovalCtx<'b>, ) -> BoxFuture<'b, ReviewDecision> { - let key = self.approval_key(req); + let keys = self.approval_keys(req); let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); @@ -116,7 +116,7 @@ impl Approvable for UnifiedExecRuntime<'_> { .clone() .or_else(|| req.justification.clone()); Box::pin(async move { - with_cached_approval(&session.services, key, || async move { + with_cached_approval(&session.services, keys, || async move { session .request_command_approval( turn, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 14dda62a8a..82af60e3d6 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -49,28 +49,44 @@ impl ApprovalStore { } } +/// Takes a vector of approval keys and returns a ReviewDecision. +/// There will be one key in most cases, but apply_patch can modify multiple files at once. +/// +/// - If all keys are already approved for session, we skip prompting. +/// - If the user approves for session, we store the decision for each key individually +/// so future requests touching any subset can also skip prompting. pub(crate) async fn with_cached_approval( services: &SessionServices, - key: K, + keys: Vec, fetch: F, ) -> ReviewDecision where - K: Serialize + Clone, + K: Serialize, F: FnOnce() -> Fut, Fut: Future, { - { + // To be defensive here, don't bother with checking the cache if keys are empty. + if keys.is_empty() { + return fetch().await; + } + + let already_approved = { let store = services.tool_approvals.lock().await; - if let Some(decision) = store.get(&key) { - return decision; - } + keys.iter() + .all(|key| matches!(store.get(key), Some(ReviewDecision::ApprovedForSession))) + }; + + if already_approved { + return ReviewDecision::ApprovedForSession; } let decision = fetch().await; if matches!(decision, ReviewDecision::ApprovedForSession) { let mut store = services.tool_approvals.lock().await; - store.put(key, ReviewDecision::ApprovedForSession); + for key in keys { + store.put(key, ReviewDecision::ApprovedForSession); + } } decision @@ -161,7 +177,14 @@ pub(crate) enum SandboxOverride { pub(crate) trait Approvable { type ApprovalKey: Hash + Eq + Clone + Debug + Serialize; - fn approval_key(&self, req: &Req) -> Self::ApprovalKey; + // In most cases (shell, unified_exec), a request will have a single approval key. + // + // However, apply_patch needs session "approve once, don't ask again" semantics that + // apply to multiple atomic targets (e.g., apply_patch approves per file path). Returning + // a list of keys lets the runtime treat the request as approved-for-session only if + // *all* keys are already approved, while still caching approvals per-key so future + // requests touching a subset can be auto-approved. + fn approval_keys(&self, req: &Req) -> Vec; /// Some tools may request to skip the sandbox on the first attempt /// (e.g., when the request explicitly asks for escalated permissions). diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 74e38534bd..ff8eca6f99 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1561,6 +1561,123 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { Ok(()) } +#[tokio::test(flavor = "current_thread")] +#[cfg(unix)] +async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }; + let sandbox_policy_for_config = sandbox_policy.clone(); + + let mut builder = test_codex() + .with_model("gpt-5.1-codex") + .with_config(move |config| { + config.approval_policy = Constrained::allow_any(approval_policy); + config.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config); + }); + let test = builder.build(&server).await?; + + let target = TargetPath::OutsideWorkspace("apply_patch_allow_session.txt"); + let (path, patch_path) = target.resolve_for_patch(&test); + let _ = fs::remove_file(&path); + + let patch_add = build_add_file_patch(&patch_path, "before"); + let patch_update = format!( + "*** Begin Patch\n*** Update File: {patch_path}\n@@\n-before\n+after\n*** End Patch\n" + ); + + let call_id_1 = "apply_patch_allow_session_1"; + let call_id_2 = "apply_patch_allow_session_2"; + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_apply_patch_function_call(call_id_1, &patch_add), + ev_completed("resp-1"), + ]), + ) + .await; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ) + .await; + + submit_turn( + &test, + "apply_patch allow session", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + let _ = expect_patch_approval(&test, call_id_1).await; + test.codex + .submit(Op::PatchApproval { + id: "0".into(), + decision: ReviewDecision::ApprovedForSession, + }) + .await?; + wait_for_completion(&test).await; + assert!(fs::read_to_string(&path)?.contains("before")); + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-3"), + ev_apply_patch_function_call(call_id_2, &patch_update), + ev_completed("resp-3"), + ]), + ) + .await; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-2", "done"), + ev_completed("resp-4"), + ]), + ) + .await; + + submit_turn( + &test, + "apply_patch allow session followup", + approval_policy, + sandbox_policy.clone(), + ) + .await?; + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TaskComplete(_) + ) + }) + .await; + match event { + EventMsg::TaskComplete(_) => {} + EventMsg::ApplyPatchApprovalRequest(event) => { + panic!("unexpected patch approval request: {:?}", event.call_id) + } + other => panic!("unexpected event: {other:?}"), + } + + assert!(fs::read_to_string(&path)?.contains("after")); + let _ = fs::remove_file(path); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] #[cfg(unix)] async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts() -> Result<()> { diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 0923d41b5f..0f0445fee8 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -498,6 +498,12 @@ fn patch_options() -> Vec { display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, + ApprovalOption { + label: "Yes, and don't ask again for these files".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], + }, ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), decision: ApprovalDecision::Review(ReviewDecision::Abort), diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index ed18675ac3..e394605dcc 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -2,6 +2,8 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend().vt100().screen().contents() --- + + Would you like to make the following edits? Reason: The model wants to apply changes @@ -12,6 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui2/src/bottom_pane/approval_overlay.rs b/codex-rs/tui2/src/bottom_pane/approval_overlay.rs index 0923d41b5f..0f0445fee8 100644 --- a/codex-rs/tui2/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui2/src/bottom_pane/approval_overlay.rs @@ -498,6 +498,12 @@ fn patch_options() -> Vec { display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, + ApprovalOption { + label: "Yes, and don't ask again for these files".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], + }, ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), decision: ApprovalDecision::Review(ReviewDecision::Abort), diff --git a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap index a5bfd136b7..c2ec167506 100644 --- a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui2__chatwidget__tests__approval_modal_patch.snap @@ -2,6 +2,8 @@ source: tui2/src/chatwidget/tests.rs expression: terminal.backend().vt100().screen().contents() --- + + Would you like to make the following edits? Reason: The model wants to apply changes @@ -12,6 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index ed18675ac3..e394605dcc 100644 --- a/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui2/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -2,6 +2,8 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend().vt100().screen().contents() --- + + Would you like to make the following edits? Reason: The model wants to apply changes @@ -12,6 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 2 +world › 1. Yes, proceed (y) - 2. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 2eb7d48758..7054103e30 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -17,7 +17,7 @@ use ts_rs::TS; /// using [AbsolutePathBufGuard::new]. If no base path is set, the /// deserialization will fail unless the path being deserialized is already /// absolute. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema, TS)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, JsonSchema, TS)] pub struct AbsolutePathBuf(PathBuf); impl AbsolutePathBuf { From 187924d7619fbbe8bc7b0c37710fe7d63dd80d19 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 7 Jan 2026 13:06:20 -0800 Subject: [PATCH 06/13] Override truncation policy at model info level (#8856) We used to override truncation policy by comparing model info vs config value in context manager. A better way to do it is to construct model info using the config value --- codex-rs/core/src/codex.rs | 10 +- codex-rs/core/src/config/mod.rs | 1 - .../core/src/models_manager/model_info.rs | 15 +++ codex-rs/core/src/truncate.rs | 24 +--- codex-rs/core/tests/suite/mod.rs | 1 + .../core/tests/suite/model_info_overrides.rs | 32 ++++++ codex-rs/core/tests/suite/remote_models.rs | 105 +++++++++++++++++- 7 files changed, 155 insertions(+), 33 deletions(-) create mode 100644 codex-rs/core/tests/suite/model_info_overrides.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3251ed118b..236a485c60 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -544,10 +544,7 @@ impl Session { final_output_json_schema: None, codex_linux_sandbox_exe: per_turn_config.codex_linux_sandbox_exe.clone(), tool_call_gate: Arc::new(ReadinessFlag::new()), - truncation_policy: TruncationPolicy::new( - per_turn_config.as_ref(), - model_info.truncation_policy.into(), - ), + truncation_policy: model_info.truncation_policy.into(), } } @@ -2244,10 +2241,7 @@ async fn spawn_review_thread( final_output_json_schema: None, codex_linux_sandbox_exe: parent_turn_context.codex_linux_sandbox_exe.clone(), tool_call_gate: Arc::new(ReadinessFlag::new()), - truncation_policy: TruncationPolicy::new( - &per_turn_config, - model_info.truncation_policy.into(), - ), + truncation_policy: model_info.truncation_policy.into(), }; // Seed the child task with the review prompt as the initial user message. diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 71b14973d8..6099e1c296 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -268,7 +268,6 @@ pub struct Config { /// Additional filenames to try when looking for project-level docs. pub project_doc_fallback_filenames: Vec, - // todo(aibrahim): this should be used in the override model info /// Token budget applied when storing tool/function outputs in the context manager. pub tool_output_token_limit: Option, diff --git a/codex-rs/core/src/models_manager/model_info.rs b/codex-rs/core/src/models_manager/model_info.rs index f907237f19..6e46b93586 100644 --- a/codex-rs/core/src/models_manager/model_info.rs +++ b/codex-rs/core/src/models_manager/model_info.rs @@ -5,9 +5,11 @@ use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::openai_models::ReasoningEffortPreset; +use codex_protocol::openai_models::TruncationMode; use codex_protocol::openai_models::TruncationPolicyConfig; use crate::config::Config; +use crate::truncate::approx_bytes_for_tokens; use tracing::warn; const BASE_INSTRUCTIONS: &str = include_str!("../../prompt.md"); @@ -71,6 +73,19 @@ pub(crate) fn with_config_overrides(mut model: ModelInfo, config: &Config) -> Mo if let Some(auto_compact_token_limit) = config.model_auto_compact_token_limit { model.auto_compact_token_limit = Some(auto_compact_token_limit); } + if let Some(token_limit) = config.tool_output_token_limit { + model.truncation_policy = match model.truncation_policy.mode { + TruncationMode::Bytes => { + let byte_limit = + i64::try_from(approx_bytes_for_tokens(token_limit)).unwrap_or(i64::MAX); + TruncationPolicyConfig::bytes(byte_limit) + } + TruncationMode::Tokens => { + let limit = i64::try_from(token_limit).unwrap_or(i64::MAX); + TruncationPolicyConfig::tokens(limit) + } + }; + } model } diff --git a/codex-rs/core/src/truncate.rs b/codex-rs/core/src/truncate.rs index 7cced9970a..8150b994d0 100644 --- a/codex-rs/core/src/truncate.rs +++ b/codex-rs/core/src/truncate.rs @@ -2,7 +2,6 @@ //! and suffix on UTF-8 boundaries, and helpers for line/token‑based truncation //! used across the core crate. -use crate::config::Config; use codex_protocol::models::FunctionCallOutputContentItem; use codex_protocol::openai_models::TruncationMode; use codex_protocol::openai_models::TruncationPolicyConfig; @@ -47,27 +46,6 @@ impl TruncationPolicy { } } - pub fn new(config: &Config, truncation_policy: TruncationPolicy) -> Self { - let config_token_limit = config.tool_output_token_limit; - - match truncation_policy { - TruncationPolicy::Bytes(family_bytes) => { - if let Some(token_limit) = config_token_limit { - Self::Bytes(approx_bytes_for_tokens(token_limit)) - } else { - Self::Bytes(family_bytes) - } - } - TruncationPolicy::Tokens(family_tokens) => { - if let Some(token_limit) = config_token_limit { - Self::Tokens(token_limit) - } else { - Self::Tokens(family_tokens) - } - } - } - } - /// Returns a token budget derived from this policy. /// /// - For `Tokens`, this is the explicit token limit. @@ -313,7 +291,7 @@ pub(crate) fn approx_token_count(text: &str) -> usize { len.saturating_add(APPROX_BYTES_PER_TOKEN.saturating_sub(1)) / APPROX_BYTES_PER_TOKEN } -fn approx_bytes_for_tokens(tokens: usize) -> usize { +pub(crate) fn approx_bytes_for_tokens(tokens: usize) -> usize { tokens.saturating_mul(APPROX_BYTES_PER_TOKEN) } diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index dde687a39d..7242641072 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -35,6 +35,7 @@ mod json_result; mod list_dir; mod list_models; mod live_cli; +mod model_info_overrides; mod model_overrides; mod model_tools; mod models_etag_responses; diff --git a/codex-rs/core/tests/suite/model_info_overrides.rs b/codex-rs/core/tests/suite/model_info_overrides.rs new file mode 100644 index 0000000000..9d9c0feef3 --- /dev/null +++ b/codex-rs/core/tests/suite/model_info_overrides.rs @@ -0,0 +1,32 @@ +use codex_core::models_manager::manager::ModelsManager; +use codex_protocol::openai_models::TruncationPolicyConfig; +use core_test_support::load_default_config_for_test; +use pretty_assertions::assert_eq; +use tempfile::TempDir; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn offline_model_info_without_tool_output_override() { + let codex_home = TempDir::new().expect("create temp dir"); + let config = load_default_config_for_test(&codex_home).await; + + let model_info = ModelsManager::construct_model_info_offline("gpt-5.1", &config); + + assert_eq!( + model_info.truncation_policy, + TruncationPolicyConfig::bytes(10_000) + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn offline_model_info_with_tool_output_override() { + let codex_home = TempDir::new().expect("create temp dir"); + let mut config = load_default_config_for_test(&codex_home).await; + config.tool_output_token_limit = Some(123); + + let model_info = ModelsManager::construct_model_info_offline("gpt-5.1-codex", &config); + + assert_eq!( + model_info.truncation_policy, + TruncationPolicyConfig::tokens(123) + ); +} diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index 507af97f25..71a3106bc3 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -186,6 +186,95 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_models_truncation_policy_without_override_preserves_remote() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = MockServer::builder() + .body_print_limit(BodyPrintLimit::Limited(80_000)) + .start() + .await; + + let slug = "codex-test-truncation-policy"; + let remote_model = test_remote_model_with_policy( + slug, + ModelVisibility::List, + 1, + TruncationPolicyConfig::bytes(12_000), + ); + mount_models_once( + &server, + ModelsResponse { + models: vec![remote_model], + }, + ) + .await; + + let harness = build_remote_models_harness(&server, |config| { + config.model = Some("gpt-5.1".to_string()); + }) + .await?; + + let models_manager = harness.thread_manager.get_models_manager(); + wait_for_model_available(&models_manager, slug, &harness.config).await; + + let model_info = models_manager + .construct_model_info(slug, &harness.config) + .await; + assert_eq!( + model_info.truncation_policy, + TruncationPolicyConfig::bytes(12_000) + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_models_truncation_policy_with_tool_output_override() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = MockServer::builder() + .body_print_limit(BodyPrintLimit::Limited(80_000)) + .start() + .await; + + let slug = "codex-test-truncation-override"; + let remote_model = test_remote_model_with_policy( + slug, + ModelVisibility::List, + 1, + TruncationPolicyConfig::bytes(10_000), + ); + mount_models_once( + &server, + ModelsResponse { + models: vec![remote_model], + }, + ) + .await; + + let harness = build_remote_models_harness(&server, |config| { + config.model = Some("gpt-5.1".to_string()); + config.tool_output_token_limit = Some(50); + }) + .await?; + + let models_manager = harness.thread_manager.get_models_manager(); + wait_for_model_available(&models_manager, slug, &harness.config).await; + + let model_info = models_manager + .construct_model_info(slug, &harness.config) + .await; + assert_eq!( + model_info.truncation_policy, + TruncationPolicyConfig::bytes(200) + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn remote_models_apply_remote_base_instructions() -> Result<()> { skip_if_no_network!(Ok(())); @@ -461,6 +550,20 @@ where } fn test_remote_model(slug: &str, visibility: ModelVisibility, priority: i32) -> ModelInfo { + test_remote_model_with_policy( + slug, + visibility, + priority, + TruncationPolicyConfig::bytes(10_000), + ) +} + +fn test_remote_model_with_policy( + slug: &str, + visibility: ModelVisibility, + priority: i32, + truncation_policy: TruncationPolicyConfig, +) -> ModelInfo { ModelInfo { slug: slug.to_string(), display_name: format!("{slug} display"), @@ -480,7 +583,7 @@ fn test_remote_model(slug: &str, visibility: ModelVisibility, priority: i32) -> support_verbosity: false, default_verbosity: None, apply_patch_tool_type: None, - truncation_policy: TruncationPolicyConfig::bytes(10_000), + truncation_policy, supports_parallel_tool_calls: false, context_window: Some(272_000), auto_compact_token_limit: None, From a9b5e8a136a1e2d4d7ff2aaafc74806f11394674 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 7 Jan 2026 13:15:46 -0800 Subject: [PATCH 07/13] Simplify error managment in `run_turn` (#8849) --- codex-rs/core/src/codex.rs | 72 +++++++++++++++++--------------------- codex-rs/core/src/error.rs | 37 ++++++++++++++++++++ 2 files changed, 69 insertions(+), 40 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 236a485c60..fb2a3b0587 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -2495,7 +2495,7 @@ async fn run_turn( let mut retries = 0; loop { - match try_run_turn( + let err = match try_run_turn( Arc::clone(&router), Arc::clone(&sess), Arc::clone(&turn_context), @@ -2505,17 +2505,10 @@ async fn run_turn( ) .await { - // todo(aibrahim): map special cases and ? on other errors Ok(output) => return Ok(output), - Err(CodexErr::TurnAborted) => { - return Err(CodexErr::TurnAborted); - } - Err(CodexErr::Interrupted) => return Err(CodexErr::Interrupted), - Err(CodexErr::EnvVar(var)) => return Err(CodexErr::EnvVar(var)), - Err(e @ CodexErr::Fatal(_)) => return Err(e), - Err(e @ CodexErr::ContextWindowExceeded) => { + Err(CodexErr::ContextWindowExceeded) => { sess.set_total_tokens_full(&turn_context).await; - return Err(e); + return Err(CodexErr::ContextWindowExceeded); } Err(CodexErr::UsageLimitReached(e)) => { let rate_limits = e.rate_limits.clone(); @@ -2524,39 +2517,38 @@ async fn run_turn( } return Err(CodexErr::UsageLimitReached(e)); } - Err(CodexErr::UsageNotIncluded) => return Err(CodexErr::UsageNotIncluded), - Err(e @ CodexErr::QuotaExceeded) => return Err(e), - Err(e @ CodexErr::InvalidImageRequest()) => return Err(e), - Err(e @ CodexErr::InvalidRequest(_)) => return Err(e), - Err(e @ CodexErr::RefreshTokenFailed(_)) => return Err(e), - Err(e) => { - // Use the configured provider-specific stream retry budget. - let max_retries = turn_context.client.get_provider().stream_max_retries(); - if retries < max_retries { - retries += 1; - let delay = match e { - CodexErr::Stream(_, Some(delay)) => delay, - _ => backoff(retries), - }; - warn!( - "stream disconnected - retrying turn ({retries}/{max_retries} in {delay:?})...", - ); + Err(err) => err, + }; - // Surface retry information to any UI/front‑end so the - // user understands what is happening instead of staring - // at a seemingly frozen screen. - sess.notify_stream_error( - &turn_context, - format!("Reconnecting... {retries}/{max_retries}"), - e, - ) - .await; + if !err.is_retryable() { + return Err(err); + } - tokio::time::sleep(delay).await; - } else { - return Err(e); + // Use the configured provider-specific stream retry budget. + let max_retries = turn_context.client.get_provider().stream_max_retries(); + if retries < max_retries { + retries += 1; + let delay = match &err { + CodexErr::Stream(_, requested_delay) => { + requested_delay.unwrap_or_else(|| backoff(retries)) } - } + _ => backoff(retries), + }; + warn!("stream disconnected - retrying turn ({retries}/{max_retries} in {delay:?})...",); + + // Surface retry information to any UI/front‑end so the + // user understands what is happening instead of staring + // at a seemingly frozen screen. + sess.notify_stream_error( + &turn_context, + format!("Reconnecting... {retries}/{max_retries}"), + err, + ) + .await; + + tokio::time::sleep(delay).await; + } else { + return Err(err); } } } diff --git a/codex-rs/core/src/error.rs b/codex-rs/core/src/error.rs index 4ae3e709fa..c2bce1a2e7 100644 --- a/codex-rs/core/src/error.rs +++ b/codex-rs/core/src/error.rs @@ -181,6 +181,43 @@ impl From for CodexErr { } } +impl CodexErr { + pub fn is_retryable(&self) -> bool { + match self { + CodexErr::TurnAborted + | CodexErr::Interrupted + | CodexErr::EnvVar(_) + | CodexErr::Fatal(_) + | CodexErr::UsageNotIncluded + | CodexErr::QuotaExceeded + | CodexErr::InvalidImageRequest() + | CodexErr::InvalidRequest(_) + | CodexErr::RefreshTokenFailed(_) + | CodexErr::UnsupportedOperation(_) + | CodexErr::Sandbox(_) + | CodexErr::LandlockSandboxExecutableNotProvided + | CodexErr::RetryLimit(_) + | CodexErr::ContextWindowExceeded + | CodexErr::ThreadNotFound(_) + | CodexErr::Spawn + | CodexErr::SessionConfiguredNotFirstEvent + | CodexErr::UsageLimitReached(_) => false, + CodexErr::Stream(..) + | CodexErr::Timeout + | CodexErr::UnexpectedStatus(_) + | CodexErr::ResponseStreamFailed(_) + | CodexErr::ConnectionFailed(_) + | CodexErr::InternalServerError + | CodexErr::InternalAgentDied + | CodexErr::Io(_) + | CodexErr::Json(_) + | CodexErr::TokioJoin(_) => true, + #[cfg(target_os = "linux")] + CodexErr::LandlockRuleset(_) | CodexErr::LandlockPathFd(_) => false, + } + } +} + #[derive(Debug)] pub struct ConnectionFailedError { pub source: reqwest::Error, From 21c6d40a44ed5c37496492fa8e0c57e4881103c2 Mon Sep 17 00:00:00 2001 From: Channing Conger Date: Wed, 7 Jan 2026 13:21:40 -0800 Subject: [PATCH 08/13] Add feature for optional request compression (#8767) Adds a new feature `enable_request_compression` that will compress using zstd requests to the codex-backend. Currently only enabled for codex-backend so only enabled for openai providers when using chatgpt::auth even when the feature is enabled Added a new info log line too for evaluating the compression ratio and overhead off compressing before requesting. You can enable with `RUST_LOG=$RUST_LOG,codex_client::transport=info` ``` 2026-01-06T00:09:48.272113Z INFO codex_client::transport: Compressed request body with zstd pre_compression_bytes=28914 post_compression_bytes=11485 compression_duration_ms=0 ``` --- codex-rs/Cargo.lock | 42 +++++++ codex-rs/Cargo.toml | 1 + codex-rs/codex-api/src/endpoint/chat.rs | 9 +- codex-rs/codex-api/src/endpoint/responses.rs | 22 +++- codex-rs/codex-api/src/endpoint/streaming.rs | 3 + codex-rs/codex-api/src/provider.rs | 2 + codex-rs/codex-api/src/requests/responses.rs | 20 ++- codex-rs/codex-api/tests/clients.rs | 13 +- codex-rs/codex-api/tests/sse_end_to_end.rs | 13 +- codex-rs/codex-client/Cargo.toml | 1 + codex-rs/codex-client/src/default_client.rs | 7 ++ codex-rs/codex-client/src/lib.rs | 1 + codex-rs/codex-client/src/request.rs | 14 +++ codex-rs/codex-client/src/transport.rs | 73 +++++++++-- codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/client.rs | 17 +++ codex-rs/core/src/features.rs | 8 ++ codex-rs/core/tests/suite/mod.rs | 1 + .../core/tests/suite/request_compression.rs | 116 ++++++++++++++++++ 19 files changed, 345 insertions(+), 19 deletions(-) create mode 100644 codex-rs/core/tests/suite/request_compression.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index bf68ff043f..c0cbcf1788 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -819,6 +819,8 @@ version = "1.2.30" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "deec109607ca693028562ed836a5f1c4b8bd77755c4e132fc5ce11b0b6211ae7" dependencies = [ + "jobserver", + "libc", "shlex", ] @@ -1197,6 +1199,7 @@ dependencies = [ "tracing", "tracing-opentelemetry", "tracing-subscriber", + "zstd", ] [[package]] @@ -1348,6 +1351,7 @@ dependencies = [ "which", "wildmatch", "wiremock", + "zstd", ] [[package]] @@ -3924,6 +3928,16 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8eaf4bc02d17cbdd7ff4c7438cafcdf7fb9a4613313ad11b4f8fefe7d3fa0130" +[[package]] +name = "jobserver" +version = "0.1.34" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9afb3de4395d6b3e67a780b6de64b51c978ecf11cb9a462c66be7d4ca9039d33" +dependencies = [ + "getrandom 0.3.3", + "libc", +] + [[package]] name = "js-sys" version = "0.3.77" @@ -8809,6 +8823,34 @@ dependencies = [ "syn 2.0.104", ] +[[package]] +name = "zstd" +version = "0.13.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e91ee311a569c327171651566e07972200e76fcfe2242a4fa446149a3881c08a" +dependencies = [ + "zstd-safe", +] + +[[package]] +name = "zstd-safe" +version = "7.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f49c4d5f0abb602a93fb8736af2a4f4dd9512e36f7f570d66e65ff867ed3b9d" +dependencies = [ + "zstd-sys", +] + +[[package]] +name = "zstd-sys" +version = "2.0.16+zstd.1.5.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91e19ebc2adc8f83e43039e79776e3fda8ca919132d68a1fed6a5faca2683748" +dependencies = [ + "cc", + "pkg-config", +] + [[package]] name = "zune-core" version = "0.4.12" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 2d0b5eec08..1dc043a4fc 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -218,6 +218,7 @@ tracing-subscriber = "0.3.22" tracing-test = "0.2.5" tree-sitter = "0.25.10" tree-sitter-bash = "0.25" +zstd = "0.13" tree-sitter-highlight = "0.25.10" ts-rs = "11" tui-scrollbar = "0.2.1" diff --git a/codex-rs/codex-api/src/endpoint/chat.rs b/codex-rs/codex-api/src/endpoint/chat.rs index b7fa0572f0..f747c57411 100644 --- a/codex-rs/codex-api/src/endpoint/chat.rs +++ b/codex-rs/codex-api/src/endpoint/chat.rs @@ -10,6 +10,7 @@ use crate::provider::WireApi; use crate::sse::chat::spawn_chat_stream; use crate::telemetry::SseTelemetry; use codex_client::HttpTransport; +use codex_client::RequestCompression; use codex_client::RequestTelemetry; use codex_protocol::models::ContentItem; use codex_protocol::models::ReasoningItemContent; @@ -80,7 +81,13 @@ impl ChatClient { extra_headers: HeaderMap, ) -> Result { self.streaming - .stream(self.path(), body, extra_headers, spawn_chat_stream) + .stream( + self.path(), + body, + extra_headers, + RequestCompression::None, + spawn_chat_stream, + ) .await } } diff --git a/codex-rs/codex-api/src/endpoint/responses.rs b/codex-rs/codex-api/src/endpoint/responses.rs index 476e8b8f13..57c7b0da03 100644 --- a/codex-rs/codex-api/src/endpoint/responses.rs +++ b/codex-rs/codex-api/src/endpoint/responses.rs @@ -9,9 +9,11 @@ use crate::provider::Provider; use crate::provider::WireApi; use crate::requests::ResponsesRequest; use crate::requests::ResponsesRequestBuilder; +use crate::requests::responses::Compression; use crate::sse::spawn_response_stream; use crate::telemetry::SseTelemetry; use codex_client::HttpTransport; +use codex_client::RequestCompression; use codex_client::RequestTelemetry; use codex_protocol::protocol::SessionSource; use http::HeaderMap; @@ -33,6 +35,7 @@ pub struct ResponsesOptions { pub conversation_id: Option, pub session_source: Option, pub extra_headers: HeaderMap, + pub compression: Compression, } impl ResponsesClient { @@ -56,7 +59,8 @@ impl ResponsesClient { &self, request: ResponsesRequest, ) -> Result { - self.stream(request.body, request.headers).await + self.stream(request.body, request.headers, request.compression) + .await } #[instrument(level = "trace", skip_all, err)] @@ -75,6 +79,7 @@ impl ResponsesClient { conversation_id, session_source, extra_headers, + compression, } = options; let request = ResponsesRequestBuilder::new(model, &prompt.instructions, &prompt.input) @@ -88,6 +93,7 @@ impl ResponsesClient { .session_source(session_source) .store_override(store_override) .extra_headers(extra_headers) + .compression(compression) .build(self.streaming.provider())?; self.stream_request(request).await @@ -104,9 +110,21 @@ impl ResponsesClient { &self, body: Value, extra_headers: HeaderMap, + compression: Compression, ) -> Result { + let compression = match compression { + Compression::None => RequestCompression::None, + Compression::Zstd => RequestCompression::Zstd, + }; + self.streaming - .stream(self.path(), body, extra_headers, spawn_response_stream) + .stream( + self.path(), + body, + extra_headers, + compression, + spawn_response_stream, + ) .await } } diff --git a/codex-rs/codex-api/src/endpoint/streaming.rs b/codex-rs/codex-api/src/endpoint/streaming.rs index 156d4084bc..de180845e8 100644 --- a/codex-rs/codex-api/src/endpoint/streaming.rs +++ b/codex-rs/codex-api/src/endpoint/streaming.rs @@ -6,6 +6,7 @@ use crate::provider::Provider; use crate::telemetry::SseTelemetry; use crate::telemetry::run_with_request_telemetry; use codex_client::HttpTransport; +use codex_client::RequestCompression; use codex_client::RequestTelemetry; use codex_client::StreamResponse; use http::HeaderMap; @@ -52,6 +53,7 @@ impl StreamingClient { path: &str, body: Value, extra_headers: HeaderMap, + compression: RequestCompression, spawner: fn(StreamResponse, Duration, Option>) -> ResponseStream, ) -> Result { let builder = || { @@ -62,6 +64,7 @@ impl StreamingClient { http::HeaderValue::from_static("text/event-stream"), ); req.body = Some(body.clone()); + req.compression = compression; add_auth_headers(&self.auth, req) }; diff --git a/codex-rs/codex-api/src/provider.rs b/codex-rs/codex-api/src/provider.rs index 8bd5fc9093..846a25bf5e 100644 --- a/codex-rs/codex-api/src/provider.rs +++ b/codex-rs/codex-api/src/provider.rs @@ -1,4 +1,5 @@ use codex_client::Request; +use codex_client::RequestCompression; use codex_client::RetryOn; use codex_client::RetryPolicy; use http::Method; @@ -87,6 +88,7 @@ impl Provider { url: self.url_for_path(path), headers: self.headers.clone(), body: None, + compression: RequestCompression::None, timeout: None, } } diff --git a/codex-rs/codex-api/src/requests/responses.rs b/codex-rs/codex-api/src/requests/responses.rs index 543b79bbe9..a18a147aba 100644 --- a/codex-rs/codex-api/src/requests/responses.rs +++ b/codex-rs/codex-api/src/requests/responses.rs @@ -11,10 +11,18 @@ use codex_protocol::protocol::SessionSource; use http::HeaderMap; use serde_json::Value; +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum Compression { + #[default] + None, + Zstd, +} + /// Assembled request body plus headers for a Responses stream request. pub struct ResponsesRequest { pub body: Value, pub headers: HeaderMap, + pub compression: Compression, } #[derive(Default)] @@ -32,6 +40,7 @@ pub struct ResponsesRequestBuilder<'a> { session_source: Option, store_override: Option, headers: HeaderMap, + compression: Compression, } impl<'a> ResponsesRequestBuilder<'a> { @@ -94,6 +103,11 @@ impl<'a> ResponsesRequestBuilder<'a> { self } + pub fn compression(mut self, compression: Compression) -> Self { + self.compression = compression; + self + } + pub fn build(self, provider: &Provider) -> Result { let model = self .model @@ -138,7 +152,11 @@ impl<'a> ResponsesRequestBuilder<'a> { insert_header(&mut headers, "x-openai-subagent", &subagent); } - Ok(ResponsesRequest { body, headers }) + Ok(ResponsesRequest { + body, + headers, + compression: self.compression, + }) } } diff --git a/codex-rs/codex-api/tests/clients.rs b/codex-rs/codex-api/tests/clients.rs index 3dafaf74fa..70af9fe829 100644 --- a/codex-rs/codex-api/tests/clients.rs +++ b/codex-rs/codex-api/tests/clients.rs @@ -11,6 +11,7 @@ use codex_api::Provider; use codex_api::ResponsesClient; use codex_api::ResponsesOptions; use codex_api::WireApi; +use codex_api::requests::responses::Compression; use codex_client::HttpTransport; use codex_client::Request; use codex_client::Response; @@ -229,7 +230,9 @@ async fn responses_client_uses_responses_path_for_responses_wire() -> Result<()> let client = ResponsesClient::new(transport, provider("openai", WireApi::Responses), NoAuth); let body = serde_json::json!({ "echo": true }); - let _stream = client.stream(body, HeaderMap::new()).await?; + let _stream = client + .stream(body, HeaderMap::new(), Compression::None) + .await?; let requests = state.take_stream_requests(); assert_path_ends_with(&requests, "/responses"); @@ -243,7 +246,9 @@ async fn responses_client_uses_chat_path_for_chat_wire() -> Result<()> { let client = ResponsesClient::new(transport, provider("openai", WireApi::Chat), NoAuth); let body = serde_json::json!({ "echo": true }); - let _stream = client.stream(body, HeaderMap::new()).await?; + let _stream = client + .stream(body, HeaderMap::new(), Compression::None) + .await?; let requests = state.take_stream_requests(); assert_path_ends_with(&requests, "/chat/completions"); @@ -258,7 +263,9 @@ async fn streaming_client_adds_auth_headers() -> Result<()> { let client = ResponsesClient::new(transport, provider("openai", WireApi::Responses), auth); let body = serde_json::json!({ "model": "gpt-test" }); - let _stream = client.stream(body, HeaderMap::new()).await?; + let _stream = client + .stream(body, HeaderMap::new(), Compression::None) + .await?; let requests = state.take_stream_requests(); assert_eq!(requests.len(), 1); diff --git a/codex-rs/codex-api/tests/sse_end_to_end.rs b/codex-rs/codex-api/tests/sse_end_to_end.rs index b91cf3a5d8..f324cc7480 100644 --- a/codex-rs/codex-api/tests/sse_end_to_end.rs +++ b/codex-rs/codex-api/tests/sse_end_to_end.rs @@ -9,6 +9,7 @@ use codex_api::Provider; use codex_api::ResponseEvent; use codex_api::ResponsesClient; use codex_api::WireApi; +use codex_api::requests::responses::Compression; use codex_client::HttpTransport; use codex_client::Request; use codex_client::Response; @@ -124,7 +125,11 @@ async fn responses_stream_parses_items_and_completed_end_to_end() -> Result<()> let client = ResponsesClient::new(transport, provider("openai", WireApi::Responses), NoAuth); let mut stream = client - .stream(serde_json::json!({"echo": true}), HeaderMap::new()) + .stream( + serde_json::json!({"echo": true}), + HeaderMap::new(), + Compression::None, + ) .await?; let mut events = Vec::new(); @@ -189,7 +194,11 @@ async fn responses_stream_aggregates_output_text_deltas() -> Result<()> { let client = ResponsesClient::new(transport, provider("openai", WireApi::Responses), NoAuth); let stream = client - .stream(serde_json::json!({"echo": true}), HeaderMap::new()) + .stream( + serde_json::json!({"echo": true}), + HeaderMap::new(), + Compression::None, + ) .await?; let mut stream = stream.aggregate(); diff --git a/codex-rs/codex-client/Cargo.toml b/codex-rs/codex-client/Cargo.toml index 2eeb456937..233bea4088 100644 --- a/codex-rs/codex-client/Cargo.toml +++ b/codex-rs/codex-client/Cargo.toml @@ -19,6 +19,7 @@ thiserror = { workspace = true } tokio = { workspace = true, features = ["macros", "rt", "time", "sync"] } tracing = { workspace = true } tracing-opentelemetry = { workspace = true } +zstd = { workspace = true } [lints] workspace = true diff --git a/codex-rs/codex-client/src/default_client.rs b/codex-rs/codex-client/src/default_client.rs index efb4d5aec4..781ded3614 100644 --- a/codex-rs/codex-client/src/default_client.rs +++ b/codex-rs/codex-client/src/default_client.rs @@ -104,6 +104,13 @@ impl CodexRequestBuilder { self.map(|builder| builder.json(value)) } + pub fn body(self, body: B) -> Self + where + B: Into, + { + self.map(|builder| builder.body(body)) + } + pub async fn send(self) -> Result { let headers = trace_headers(); diff --git a/codex-rs/codex-client/src/lib.rs b/codex-rs/codex-client/src/lib.rs index 66d1083c07..089d777c3a 100644 --- a/codex-rs/codex-client/src/lib.rs +++ b/codex-rs/codex-client/src/lib.rs @@ -11,6 +11,7 @@ pub use crate::default_client::CodexRequestBuilder; pub use crate::error::StreamError; pub use crate::error::TransportError; pub use crate::request::Request; +pub use crate::request::RequestCompression; pub use crate::request::Response; pub use crate::retry::RetryOn; pub use crate::retry::RetryPolicy; diff --git a/codex-rs/codex-client/src/request.rs b/codex-rs/codex-client/src/request.rs index f3d205de99..c2c9cf2b33 100644 --- a/codex-rs/codex-client/src/request.rs +++ b/codex-rs/codex-client/src/request.rs @@ -5,12 +5,20 @@ use serde::Serialize; use serde_json::Value; use std::time::Duration; +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum RequestCompression { + #[default] + None, + Zstd, +} + #[derive(Debug, Clone)] pub struct Request { pub method: Method, pub url: String, pub headers: HeaderMap, pub body: Option, + pub compression: RequestCompression, pub timeout: Option, } @@ -21,6 +29,7 @@ impl Request { url, headers: HeaderMap::new(), body: None, + compression: RequestCompression::None, timeout: None, } } @@ -29,6 +38,11 @@ impl Request { self.body = serde_json::to_value(body).ok(); self } + + pub fn with_compression(mut self, compression: RequestCompression) -> Self { + self.compression = compression; + self + } } #[derive(Debug, Clone)] diff --git a/codex-rs/codex-client/src/transport.rs b/codex-rs/codex-client/src/transport.rs index abe6e29ee5..50e9f8fab7 100644 --- a/codex-rs/codex-client/src/transport.rs +++ b/codex-rs/codex-client/src/transport.rs @@ -2,6 +2,7 @@ use crate::default_client::CodexHttpClient; use crate::default_client::CodexRequestBuilder; use crate::error::TransportError; use crate::request::Request; +use crate::request::RequestCompression; use crate::request::Response; use async_trait::async_trait; use bytes::Bytes; @@ -41,18 +42,70 @@ impl ReqwestTransport { } fn build(&self, req: Request) -> Result { - let mut builder = self - .client - .request( - Method::from_bytes(req.method.as_str().as_bytes()).unwrap_or(Method::GET), - &req.url, - ) - .headers(req.headers); - if let Some(timeout) = req.timeout { + let Request { + method, + url, + mut headers, + body, + compression, + timeout, + } = req; + + let mut builder = self.client.request( + Method::from_bytes(method.as_str().as_bytes()).unwrap_or(Method::GET), + &url, + ); + + if let Some(timeout) = timeout { builder = builder.timeout(timeout); } - if let Some(body) = req.body { - builder = builder.json(&body); + + if let Some(body) = body { + if compression != RequestCompression::None { + if headers.contains_key(http::header::CONTENT_ENCODING) { + return Err(TransportError::Build( + "request compression was requested but content-encoding is already set" + .to_string(), + )); + } + + let json = serde_json::to_vec(&body) + .map_err(|err| TransportError::Build(err.to_string()))?; + let pre_compression_bytes = json.len(); + let compression_start = std::time::Instant::now(); + let (compressed, content_encoding) = match compression { + RequestCompression::None => unreachable!("guarded by compression != None"), + RequestCompression::Zstd => ( + zstd::stream::encode_all(std::io::Cursor::new(json), 3) + .map_err(|err| TransportError::Build(err.to_string()))?, + http::HeaderValue::from_static("zstd"), + ), + }; + let post_compression_bytes = compressed.len(); + let compression_duration = compression_start.elapsed(); + + // Ensure the server knows to unpack the request body. + headers.insert(http::header::CONTENT_ENCODING, content_encoding); + if !headers.contains_key(http::header::CONTENT_TYPE) { + headers.insert( + http::header::CONTENT_TYPE, + http::HeaderValue::from_static("application/json"), + ); + } + + tracing::info!( + pre_compression_bytes, + post_compression_bytes, + compression_duration_ms = compression_duration.as_millis(), + "Compressed request body with zstd" + ); + + builder = builder.headers(headers).body(compressed); + } else { + builder = builder.headers(headers).json(&body); + } + } else { + builder = builder.headers(headers); } Ok(builder) } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 51fab19dec..5060092290 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -137,6 +137,7 @@ tracing-subscriber = { workspace = true } tracing-test = { workspace = true, features = ["no-env-filter"] } walkdir = { workspace = true } wiremock = { workspace = true } +zstd = { workspace = true } [package.metadata.cargo-shear] ignored = ["openssl-sys"] diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index c43c074c44..b1b9480e75 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -17,6 +17,7 @@ use codex_api::TransportError; use codex_api::common::Reasoning; use codex_api::create_text_param_for_request; use codex_api::error::ApiError; +use codex_api::requests::responses::Compression; use codex_app_server_protocol::AuthMode; use codex_otel::otel_manager::OtelManager; use codex_protocol::ThreadId; @@ -47,6 +48,7 @@ use crate::default_client::build_reqwest_client; use crate::error::CodexErr; use crate::error::Result; use crate::features::FEATURES; +use crate::features::Feature; use crate::flags::CODEX_RS_SSE_FIXTURE; use crate::model_provider_info::ModelProviderInfo; use crate::model_provider_info::WireApi; @@ -250,6 +252,20 @@ impl ModelClient { let api_auth = auth_provider_from_auth(auth.clone(), &self.provider).await?; let transport = ReqwestTransport::new(build_reqwest_client()); let (request_telemetry, sse_telemetry) = self.build_streaming_telemetry(); + let compression = if self + .config + .features + .enabled(Feature::EnableRequestCompression) + && auth + .as_ref() + .is_some_and(|auth| auth.mode == AuthMode::ChatGPT) + && self.provider.is_openai() + { + Compression::Zstd + } else { + Compression::None + }; + let client = ApiResponsesClient::new(transport, api_provider, api_auth) .with_telemetry(Some(request_telemetry), Some(sse_telemetry)); @@ -262,6 +278,7 @@ impl ModelClient { conversation_id: Some(conversation_id.clone()), session_source: Some(session_source.clone()), extra_headers: beta_feature_headers(&self.config), + compression, }; let stream_result = client diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 7ef74cfb7a..e1af096af5 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -89,6 +89,8 @@ pub enum Feature { Tui2, /// Enforce UTF8 output in Powershell. PowershellUtf8, + /// Compress request bodies (zstd) when sending streaming requests to codex-backend. + EnableRequestCompression, } impl Feature { @@ -374,6 +376,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Experimental, default_enabled: false, }, + FeatureSpec { + id: Feature::EnableRequestCompression, + key: "enable_request_compression", + stage: Stage::Experimental, + default_enabled: false, + }, FeatureSpec { id: Feature::Tui2, key: "tui2", diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 7242641072..effbc8a931 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -44,6 +44,7 @@ mod prompt_caching; mod quota_exceeded; mod read_file; mod remote_models; +mod request_compression; mod resume; mod resume_warning; mod review; diff --git a/codex-rs/core/tests/suite/request_compression.rs b/codex-rs/core/tests/suite/request_compression.rs new file mode 100644 index 0000000000..271f67e1af --- /dev/null +++ b/codex-rs/core/tests/suite/request_compression.rs @@ -0,0 +1,116 @@ +#![cfg(not(target_os = "windows"))] + +use codex_core::CodexAuth; +use codex_core::features::Feature; +use codex_core::protocol::EventMsg; +use codex_core::protocol::Op; +use codex_protocol::user_input::UserInput; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::get_responses_requests; +use core_test_support::responses::mount_sse_once; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn request_body_is_zstd_compressed_for_codex_backend_when_enabled() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + + let base_url = format!("{}/backend-api/codex/v1", server.uri()); + let mut builder = test_codex() + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(move |config| { + config.features.enable(Feature::EnableRequestCompression); + config.model_provider.base_url = Some(base_url); + }); + let codex = builder.build(&server).await?.codex; + + codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "compress me".into(), + }], + final_output_json_schema: None, + }) + .await?; + + // Wait until the task completes so the request definitely hit the server. + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = get_responses_requests(&server).await; + assert_eq!(requests.len(), 1); + + let request = &requests[0]; + let content_encoding = request + .headers + .get("content-encoding") + .and_then(|v| v.to_str().ok()); + assert_eq!(content_encoding, Some("zstd")); + + let decompressed = zstd::stream::decode_all(std::io::Cursor::new(request.body.clone()))?; + let json: serde_json::Value = serde_json::from_slice(&decompressed)?; + assert!( + json.get("input").is_some(), + "expected request body to decode as Responses API JSON" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn request_body_is_not_compressed_for_api_key_auth_even_when_enabled() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + + let base_url = format!("{}/backend-api/codex/v1", server.uri()); + let mut builder = test_codex().with_config(move |config| { + config.features.enable(Feature::EnableRequestCompression); + config.model_provider.base_url = Some(base_url); + }); + let codex = builder.build(&server).await?.codex; + + codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "do not compress".into(), + }], + final_output_json_schema: None, + }) + .await?; + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = get_responses_requests(&server).await; + assert_eq!(requests.len(), 1); + + let request = &requests[0]; + assert!( + request.headers.get("content-encoding").is_none(), + "did not expect request compression for API-key auth" + ); + + let json: serde_json::Value = serde_json::from_slice(&request.body)?; + assert!( + json.get("input").is_some(), + "expected request body to be plain Responses API JSON" + ); + + Ok(()) +} From 5f3f70203c856df1314ab5539ad61992dc4fbccf Mon Sep 17 00:00:00 2001 From: darlingm Date: Wed, 7 Jan 2026 16:24:02 -0500 Subject: [PATCH 09/13] Clarify YAML frontmatter formatting in skill-creator (#8610) Fixes #8609 # Summary Emphasize single-line name/description values and quoting when values could be interpreted as YAML syntax. # Testing Not run (skill-only change.) --- codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md b/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md index 7b44b52b22..60251f16a6 100644 --- a/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md +++ b/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md @@ -328,6 +328,8 @@ Write the YAML frontmatter with `name` and `description`: - Include all "when to use" information here - Not in the body. The body is only loaded after triggering, so "When to Use This Skill" sections in the body are not helpful to Codex. - Example description for a `docx` skill: "Comprehensive document creation, editing, and analysis with support for tracked changes, comments, formatting preservation, and text extraction. Use when Codex needs to work with professional documents (.docx files) for: (1) Creating new documents, (2) Modifying or editing content, (3) Working with tracked changes, (4) Adding comments, or any other document tasks" +Ensure the frontmatter is valid YAML. Keep `name` and `description` as single-line scalars. If either could be interpreted as YAML syntax, wrap it in quotes. + Do not include any other fields in YAML frontmatter. ##### Body From f07b8aa591f3afbcb09f2abbe58f008794b46255 Mon Sep 17 00:00:00 2001 From: gt-oai Date: Wed, 7 Jan 2026 21:24:18 +0000 Subject: [PATCH 10/13] Warn in /model if BASE_URL set (#8847) Screenshot 2026-01-07 at 18 37 59 It may not make sense to use the `/model` menu with a custom OPENAI_BASE_URL. But some model proxies may support it, so we shouldn't disable it completely. A warning is a reasonable compromise. --- codex-rs/tui/src/chatwidget.rs | 57 +++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 7 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 868319893c..537adda251 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -147,6 +147,7 @@ use strum::IntoEnumIterator; const USER_SHELL_COMMAND_HELP_TITLE: &str = "Prefix a command with ! to run it locally"; const USER_SHELL_COMMAND_HELP_HINT: &str = "Example: !ls"; +const DEFAULT_OPENAI_BASE_URL: &str = "https://api.openai.com/v1"; // Track information about an in-flight exec command. struct RunningCommand { command: Vec, @@ -2453,6 +2454,45 @@ impl ChatWidget { self.open_model_popup_with_presets(presets); } + fn model_menu_header(&self, title: &str, subtitle: &str) -> Box { + let title = title.to_string(); + let subtitle = subtitle.to_string(); + let mut header = ColumnRenderable::new(); + header.push(Line::from(title.bold())); + header.push(Line::from(subtitle.dim())); + if let Some(warning) = self.model_menu_warning_line() { + header.push(warning); + } + Box::new(header) + } + + fn model_menu_warning_line(&self) -> Option> { + let base_url = self.custom_openai_base_url()?; + let warning = format!( + "Warning: OPENAI_BASE_URL is set to {base_url}. Selecting models may not be supported or work properly." + ); + Some(Line::from(warning.red())) + } + + fn custom_openai_base_url(&self) -> Option { + if !self.config.model_provider.is_openai() { + return None; + } + + let base_url = self.config.model_provider.base_url.as_ref()?; + let trimmed = base_url.trim(); + if trimmed.is_empty() { + return None; + } + + let normalized = trimmed.trim_end_matches('/'); + if normalized == DEFAULT_OPENAI_BASE_URL { + return None; + } + + Some(trimmed.to_string()) + } + pub(crate) fn open_model_popup_with_presets(&mut self, presets: Vec) { let presets: Vec = presets .into_iter() @@ -2521,11 +2561,14 @@ impl ChatWidget { }); } + let header = self.model_menu_header( + "Select Model", + "Pick a quick auto mode or browse all models.", + ); self.bottom_pane.show_selection_view(SelectionViewParams { - title: Some("Select Model".to_string()), - subtitle: Some("Pick a quick auto mode or browse all models.".to_string()), footer_hint: Some(standard_popup_hint_line()), items, + header, ..Default::default() }); } @@ -2576,14 +2619,14 @@ impl ChatWidget { }); } + let header = self.model_menu_header( + "Select Model and Effort", + "Access legacy models by running codex -m or in your config.toml", + ); self.bottom_pane.show_selection_view(SelectionViewParams { - title: Some("Select Model and Effort".to_string()), - subtitle: Some( - "Access legacy models by running codex -m or in your config.toml" - .to_string(), - ), footer_hint: Some("Press enter to select reasoning effort, or esc to dismiss.".into()), items, + header, ..Default::default() }); } From 61e81af887fc77c4fca29c3d7fe3efe759ad7fa7 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Wed, 7 Jan 2026 13:34:48 -0800 Subject: [PATCH 11/13] Support symlink for skills discovery. (#8801) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills discovery now follows symlink entries for SkillScope::User ($CODEX_HOME/skills) and SkillScope::Admin (e.g. /etc/codex/skills). Added cycle protection: directories are canonicalized and tracked in a visited set to prevent infinite traversal from circular links. Added per-root traversal limits to avoid accidentally scanning huge trees: - max depth: 6 - max directories: 2000 (logs a warning if truncated) For now, symlink stat failures and traversal truncation are logged rather than surfaced as UI “invalid SKILL.md” warnings. --- codex-rs/core/src/skills/loader.rs | 347 ++++++++++++++++++++++++++++- 1 file changed, 339 insertions(+), 8 deletions(-) diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 35aeaec7f8..4efa801d97 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -6,7 +6,7 @@ use crate::skills::model::SkillMetadata; use crate::skills::system::system_cache_root_dir; use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::protocol::SkillScope; -use dunce::canonicalize as normalize_path; +use dunce::canonicalize as canonicalize_path; use serde::Deserialize; use std::collections::HashSet; use std::collections::VecDeque; @@ -36,6 +36,9 @@ const SKILLS_DIR_NAME: &str = "skills"; const MAX_NAME_LEN: usize = 64; const MAX_DESCRIPTION_LEN: usize = 1024; const MAX_SHORT_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; +// Traversal depth from the skills root. +const MAX_SCAN_DEPTH: usize = 6; +const MAX_SKILLS_DIRS_PER_ROOT: usize = 2000; #[derive(Debug)] enum SkillParseError { @@ -165,7 +168,7 @@ pub(crate) fn skill_roots_from_layer_stack( } fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut SkillLoadOutcome) { - let Ok(root) = normalize_path(root) else { + let Ok(root) = canonicalize_path(root) else { return; }; @@ -173,8 +176,38 @@ fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut Skil return; } - let mut queue: VecDeque = VecDeque::from([root]); - while let Some(dir) = queue.pop_front() { + fn enqueue_dir( + queue: &mut VecDeque<(PathBuf, usize)>, + visited_dirs: &mut HashSet, + truncated_by_dir_limit: &mut bool, + path: PathBuf, + depth: usize, + ) { + if depth > MAX_SCAN_DEPTH { + return; + } + if visited_dirs.len() >= MAX_SKILLS_DIRS_PER_ROOT { + *truncated_by_dir_limit = true; + return; + } + if visited_dirs.insert(path.clone()) { + queue.push_back((path, depth)); + } + } + + // Follow symlinks for user, admin, and repo skills. System skills are written by Codex itself. + let follow_symlinks = matches!( + scope, + SkillScope::Repo | SkillScope::User | SkillScope::Admin + ); + + let mut visited_dirs: HashSet = HashSet::new(); + visited_dirs.insert(root.clone()); + + let mut queue: VecDeque<(PathBuf, usize)> = VecDeque::from([(root.clone(), 0)]); + let mut truncated_by_dir_limit = false; + + while let Some((dir, depth)) = queue.pop_front() { let entries = match fs::read_dir(&dir) { Ok(entries) => entries, Err(e) => { @@ -199,11 +232,64 @@ fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut Skil }; if file_type.is_symlink() { + if !follow_symlinks { + continue; + } + + // Follow the symlink to determine what it points to. + let metadata = match fs::metadata(&path) { + Ok(metadata) => metadata, + Err(e) => { + error!( + "failed to stat skills entry {} (symlink): {e:#}", + path.display() + ); + continue; + } + }; + + if metadata.is_dir() { + let Ok(resolved_dir) = canonicalize_path(&path) else { + continue; + }; + enqueue_dir( + &mut queue, + &mut visited_dirs, + &mut truncated_by_dir_limit, + resolved_dir, + depth + 1, + ); + continue; + } + + if metadata.is_file() && file_name == SKILLS_FILENAME { + match parse_skill_file(&path, scope) { + Ok(skill) => outcome.skills.push(skill), + Err(err) => { + if scope != SkillScope::System { + outcome.errors.push(SkillError { + path, + message: err.to_string(), + }); + } + } + } + } + continue; } if file_type.is_dir() { - queue.push_back(path); + let Ok(resolved_dir) = canonicalize_path(&path) else { + continue; + }; + enqueue_dir( + &mut queue, + &mut visited_dirs, + &mut truncated_by_dir_limit, + resolved_dir, + depth + 1, + ); continue; } @@ -224,6 +310,14 @@ fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut Skil } } } + + if truncated_by_dir_limit { + tracing::warn!( + "skills scan truncated after {} directories (root: {})", + MAX_SKILLS_DIRS_PER_ROOT, + root.display() + ); + } } fn parse_skill_file(path: &Path, scope: SkillScope) -> Result { @@ -253,7 +347,7 @@ fn parse_skill_file(path: &Path, scope: SkillScope) -> Result PathBuf { - normalize_path(path).unwrap_or_else(|_| path.to_path_buf()) + canonicalize_path(path).unwrap_or_else(|_| path.to_path_buf()) } #[test] @@ -429,6 +523,243 @@ mod tests { path } + #[cfg(unix)] + fn symlink_dir(target: &Path, link: &Path) { + std::os::unix::fs::symlink(target, link).unwrap(); + } + + #[cfg(unix)] + fn symlink_file(target: &Path, link: &Path) { + std::os::unix::fs::symlink(target, link).unwrap(); + } + + #[tokio::test] + #[cfg(unix)] + async fn loads_skills_via_symlinked_subdir_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + let shared_skill_path = write_skill_at(shared.path(), "demo", "linked-skill", "from link"); + + fs::create_dir_all(codex_home.path().join("skills")).unwrap(); + symlink_dir(shared.path(), &codex_home.path().join("skills/shared")); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "linked-skill".to_string(), + description: "from link".to_string(), + short_description: None, + path: normalized(&shared_skill_path), + scope: SkillScope::User, + }] + ); + } + + #[tokio::test] + #[cfg(unix)] + async fn loads_skills_via_symlinked_skill_file_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + let shared_skill_path = + write_skill_at(shared.path(), "demo", "linked-file-skill", "from link"); + + let skill_dir = codex_home.path().join("skills/demo"); + fs::create_dir_all(&skill_dir).unwrap(); + symlink_file(&shared_skill_path, &skill_dir.join(SKILLS_FILENAME)); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "linked-file-skill".to_string(), + description: "from link".to_string(), + short_description: None, + path: normalized(&shared_skill_path), + scope: SkillScope::User, + }] + ); + } + + #[tokio::test] + #[cfg(unix)] + async fn does_not_loop_on_symlink_cycle_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + + // Create a cycle: + // $CODEX_HOME/skills/cycle/loop -> $CODEX_HOME/skills/cycle + let cycle_dir = codex_home.path().join("skills/cycle"); + fs::create_dir_all(&cycle_dir).unwrap(); + symlink_dir(&cycle_dir, &cycle_dir.join("loop")); + + let skill_path = write_skill_at(&cycle_dir, "demo", "cycle-skill", "still loads"); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "cycle-skill".to_string(), + description: "still loads".to_string(), + short_description: None, + path: normalized(&skill_path), + scope: SkillScope::User, + }] + ); + } + + #[test] + #[cfg(unix)] + fn loads_skills_via_symlinked_subdir_for_admin_scope() { + let admin_root = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + let shared_skill_path = + write_skill_at(shared.path(), "demo", "admin-linked-skill", "from link"); + fs::create_dir_all(admin_root.path()).unwrap(); + symlink_dir(shared.path(), &admin_root.path().join("shared")); + + let outcome = load_skills_from_roots([SkillRoot { + path: admin_root.path().to_path_buf(), + scope: SkillScope::Admin, + }]); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "admin-linked-skill".to_string(), + description: "from link".to_string(), + short_description: None, + path: normalized(&shared_skill_path), + scope: SkillScope::Admin, + }] + ); + } + + #[tokio::test] + #[cfg(unix)] + async fn loads_skills_via_symlinked_subdir_for_repo_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); + let shared = tempfile::tempdir().expect("tempdir"); + + let linked_skill_path = + write_skill_at(shared.path(), "demo", "repo-linked-skill", "from link"); + let repo_skills_root = repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME); + fs::create_dir_all(&repo_skills_root).unwrap(); + symlink_dir(shared.path(), &repo_skills_root.join("shared")); + + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; + let outcome = load_skills(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "repo-linked-skill".to_string(), + description: "from link".to_string(), + short_description: None, + path: normalized(&linked_skill_path), + scope: SkillScope::Repo, + }] + ); + } + + #[tokio::test] + #[cfg(unix)] + async fn system_scope_ignores_symlinked_subdir() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let shared = tempfile::tempdir().expect("tempdir"); + + write_skill_at(shared.path(), "demo", "system-linked-skill", "from link"); + + let system_root = codex_home.path().join("skills/.system"); + fs::create_dir_all(&system_root).unwrap(); + symlink_dir(shared.path(), &system_root.join("shared")); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); + } + + #[tokio::test] + async fn respects_max_scan_depth_for_user_scope() { + let codex_home = tempfile::tempdir().expect("tempdir"); + + let within_depth_path = write_skill( + &codex_home, + "d0/d1/d2/d3/d4/d5", + "within-depth-skill", + "loads", + ); + let _too_deep_path = write_skill( + &codex_home, + "d0/d1/d2/d3/d4/d5/d6", + "too-deep-skill", + "should not load", + ); + + let cfg = make_config(&codex_home).await; + let outcome = load_skills(&cfg); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "within-depth-skill".to_string(), + description: "loads".to_string(), + short_description: None, + path: normalized(&within_depth_path), + scope: SkillScope::User, + }] + ); + } + #[tokio::test] async fn loads_valid_skill() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -1029,7 +1360,7 @@ mod tests { outcome.errors ); let expected_path = - normalize_path(&nested_skill_path).unwrap_or_else(|_| nested_skill_path.clone()); + canonicalize_path(&nested_skill_path).unwrap_or_else(|_| nested_skill_path.clone()); assert_eq!( vec![SkillMetadata { name: "dupe-skill".to_string(), From efd0c21b9b6cf9909d5b7b20ca89a0b5799cc039 Mon Sep 17 00:00:00 2001 From: Shijie Rao Date: Wed, 7 Jan 2026 13:57:44 -0800 Subject: [PATCH 12/13] Feat: appServer.requirementList for requirement.toml (#8800) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Summary We are exposing requirements via `requirement/list` method from app-server so that we can conditionally disable the agent mode dropdown selection in VSCE and correctly setting the default value. ### Sample output #### `etc/codex/requirements.toml` Screenshot 2026-01-06 at 11 32 06 PM #### App server response Screenshot 2026-01-06 at 11 30 18 PM --- .../src/protocol/common.rs | 21 +++++ .../app-server-protocol/src/protocol/v2.rs | 16 ++++ codex-rs/app-server/README.md | 1 + .../app-server/src/codex_message_processor.rs | 3 + codex-rs/app-server/src/config_api.rs | 79 +++++++++++++++++++ codex-rs/app-server/src/message_processor.rs | 13 +++ codex-rs/core/src/config/service.rs | 17 ++++ .../src/config_loader/config_requirements.rs | 4 + codex-rs/core/src/config_loader/mod.rs | 7 +- codex-rs/core/src/config_loader/state.rs | 14 ++++ codex-rs/core/src/exec_policy.rs | 14 +++- codex-rs/core/src/skills/loader.rs | 7 +- 12 files changed, 191 insertions(+), 5 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index ed8207e24e..a841e29205 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -197,6 +197,11 @@ client_request_definitions! { response: v2::ConfigWriteResponse, }, + ConfigRequirementsRead => "configRequirements/read" { + params: #[ts(type = "undefined")] #[serde(skip_serializing_if = "Option::is_none")] Option<()>, + response: v2::ConfigRequirementsReadResponse, + }, + GetAccount => "account/read" { params: v2::GetAccountParams, response: v2::GetAccountResponse, @@ -711,6 +716,22 @@ mod tests { Ok(()) } + #[test] + fn serialize_config_requirements_read() -> Result<()> { + let request = ClientRequest::ConfigRequirementsRead { + request_id: RequestId::Integer(1), + params: None, + }; + assert_eq!( + json!({ + "method": "configRequirements/read", + "id": 1, + }), + serde_json::to_value(&request)?, + ); + Ok(()) + } + #[test] fn serialize_account_login_api_key() -> Result<()> { let request = ClientRequest::LoginAccount { diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index ee06115806..0b2e2c0a30 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -453,6 +453,22 @@ pub struct ConfigReadResponse { pub layers: Option>, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct ConfigRequirements { + pub allowed_approval_policies: Option>, + pub allowed_sandbox_modes: Option>, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct ConfigRequirementsReadResponse { + /// Null if no requirements are configured (e.g. no requirements.toml/MDM entries). + pub requirements: Option, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 88d96dda56..4457f4d272 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -88,6 +88,7 @@ Example (from OpenAI's official VSCode extension): - `config/read` — fetch the effective config on disk after resolving config layering. - `config/value/write` — write a single config key/value to the user's config.toml on disk. - `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk. +- `configRequirements/read` — fetch the loaded requirements allow-lists from `requirements.toml` and/or MDM (or `null` if none are configured). ### Example: Start or resume a thread diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 3b0e4a9db1..d17dc76b4b 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -510,6 +510,9 @@ impl CodexMessageProcessor { | ClientRequest::ConfigBatchWrite { .. } => { warn!("Config request reached CodexMessageProcessor unexpectedly"); } + ClientRequest::ConfigRequirementsRead { .. } => { + warn!("ConfigRequirementsRead request reached CodexMessageProcessor unexpectedly"); + } ClientRequest::GetAccountRateLimits { request_id, params: _, diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index b06de3f1e4..25434ce92b 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -3,13 +3,18 @@ use crate::error_code::INVALID_REQUEST_ERROR_CODE; use codex_app_server_protocol::ConfigBatchWriteParams; use codex_app_server_protocol::ConfigReadParams; use codex_app_server_protocol::ConfigReadResponse; +use codex_app_server_protocol::ConfigRequirements; +use codex_app_server_protocol::ConfigRequirementsReadResponse; use codex_app_server_protocol::ConfigValueWriteParams; use codex_app_server_protocol::ConfigWriteErrorCode; use codex_app_server_protocol::ConfigWriteResponse; use codex_app_server_protocol::JSONRPCErrorError; +use codex_app_server_protocol::SandboxMode; use codex_core::config::ConfigService; use codex_core::config::ConfigServiceError; +use codex_core::config_loader::ConfigRequirementsToml; use codex_core::config_loader::LoaderOverrides; +use codex_core::config_loader::SandboxModeRequirement as CoreSandboxModeRequirement; use serde_json::json; use std::path::PathBuf; use toml::Value as TomlValue; @@ -37,6 +42,19 @@ impl ConfigApi { self.service.read(params).await.map_err(map_error) } + pub(crate) async fn config_requirements_read( + &self, + ) -> Result { + let requirements = self + .service + .read_requirements() + .await + .map_err(map_error)? + .map(map_requirements_toml_to_api); + + Ok(ConfigRequirementsReadResponse { requirements }) + } + pub(crate) async fn write_value( &self, params: ConfigValueWriteParams, @@ -52,6 +70,32 @@ impl ConfigApi { } } +fn map_requirements_toml_to_api(requirements: ConfigRequirementsToml) -> ConfigRequirements { + ConfigRequirements { + allowed_approval_policies: requirements.allowed_approval_policies.map(|policies| { + policies + .into_iter() + .map(codex_app_server_protocol::AskForApproval::from) + .collect() + }), + allowed_sandbox_modes: requirements.allowed_sandbox_modes.map(|modes| { + modes + .into_iter() + .filter_map(map_sandbox_mode_requirement_to_api) + .collect() + }), + } +} + +fn map_sandbox_mode_requirement_to_api(mode: CoreSandboxModeRequirement) -> Option { + match mode { + CoreSandboxModeRequirement::ReadOnly => Some(SandboxMode::ReadOnly), + CoreSandboxModeRequirement::WorkspaceWrite => Some(SandboxMode::WorkspaceWrite), + CoreSandboxModeRequirement::DangerFullAccess => Some(SandboxMode::DangerFullAccess), + CoreSandboxModeRequirement::ExternalSandbox => None, + } +} + fn map_error(err: ConfigServiceError) -> JSONRPCErrorError { if let Some(code) = err.write_error_code() { return config_write_error(code, err.to_string()); @@ -73,3 +117,38 @@ fn config_write_error(code: ConfigWriteErrorCode, message: impl Into) -> })), } } + +#[cfg(test)] +mod tests { + use super::*; + use codex_protocol::protocol::AskForApproval as CoreAskForApproval; + use pretty_assertions::assert_eq; + + #[test] + fn map_requirements_toml_to_api_converts_core_enums() { + let requirements = ConfigRequirementsToml { + allowed_approval_policies: Some(vec![ + CoreAskForApproval::Never, + CoreAskForApproval::OnRequest, + ]), + allowed_sandbox_modes: Some(vec![ + CoreSandboxModeRequirement::ReadOnly, + CoreSandboxModeRequirement::ExternalSandbox, + ]), + }; + + let mapped = map_requirements_toml_to_api(requirements); + + assert_eq!( + mapped.allowed_approval_policies, + Some(vec![ + codex_app_server_protocol::AskForApproval::Never, + codex_app_server_protocol::AskForApproval::OnRequest, + ]) + ); + assert_eq!( + mapped.allowed_sandbox_modes, + Some(vec![SandboxMode::ReadOnly]), + ); + } +} diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 731342e3c5..60e938bb18 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -158,6 +158,12 @@ impl MessageProcessor { ClientRequest::ConfigBatchWrite { request_id, params } => { self.handle_config_batch_write(request_id, params).await; } + ClientRequest::ConfigRequirementsRead { + request_id, + params: _, + } => { + self.handle_config_requirements_read(request_id).await; + } other => { self.codex_message_processor.process_request(other).await; } @@ -210,4 +216,11 @@ impl MessageProcessor { Err(error) => self.outgoing.send_error(request_id, error).await, } } + + async fn handle_config_requirements_read(&self, request_id: RequestId) { + match self.config_api.config_requirements_read().await { + Ok(response) => self.outgoing.send_response(request_id, response).await, + Err(error) => self.outgoing.send_error(request_id, error).await, + } + } } diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index d65ed10a05..913c02df1d 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -4,6 +4,7 @@ use crate::config::edit::ConfigEdit; use crate::config::edit::ConfigEditsBuilder; use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigLayerStack; +use crate::config_loader::ConfigRequirementsToml; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::config_loader::merge_toml_values; @@ -157,6 +158,22 @@ impl ConfigService { }) } + pub async fn read_requirements( + &self, + ) -> Result, ConfigServiceError> { + let layers = self + .load_thread_agnostic_config() + .await + .map_err(|err| ConfigServiceError::io("failed to read configuration layers", err))?; + + let requirements = layers.requirements_toml().clone(); + if requirements.is_empty() { + Ok(None) + } else { + Ok(Some(requirements)) + } + } + pub async fn write_value( &self, params: ConfigValueWriteParams, diff --git a/codex-rs/core/src/config_loader/config_requirements.rs b/codex-rs/core/src/config_loader/config_requirements.rs index feb854df69..efbf9d61e5 100644 --- a/codex-rs/core/src/config_loader/config_requirements.rs +++ b/codex-rs/core/src/config_loader/config_requirements.rs @@ -58,6 +58,10 @@ impl From for SandboxModeRequirement { } impl ConfigRequirementsToml { + pub fn is_empty(&self) -> bool { + self.allowed_approval_policies.is_none() && self.allowed_sandbox_modes.is_none() + } + /// For every field in `other` that is `Some`, if the corresponding field in /// `self` is `None`, copy the value from `other` into `self`. pub fn merge_unset_fields(&mut self, mut other: ConfigRequirementsToml) { diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 2dbba678ac..bb995fda21 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -12,7 +12,6 @@ mod tests; use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigToml; -use crate::config_loader::config_requirements::ConfigRequirementsToml; use crate::config_loader::layer_io::LoadedConfigLayers; use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::config_types::SandboxMode; @@ -25,6 +24,8 @@ use std::path::Path; use toml::Value as TomlValue; pub use config_requirements::ConfigRequirements; +pub use config_requirements::ConfigRequirementsToml; +pub use config_requirements::SandboxModeRequirement; pub use merge::merge_toml_values; pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; @@ -201,7 +202,9 @@ pub async fn load_config_layers_state( )); } - ConfigLayerStack::new(layers, config_requirements_toml.try_into()?) + let requirements_toml = config_requirements_toml.clone(); + let requirements = config_requirements_toml.try_into()?; + ConfigLayerStack::new(layers, requirements, requirements_toml) } /// Attempts to load a config.toml file from `config_toml`. diff --git a/codex-rs/core/src/config_loader/state.rs b/codex-rs/core/src/config_loader/state.rs index 0ef14403d6..2b01a22644 100644 --- a/codex-rs/core/src/config_loader/state.rs +++ b/codex-rs/core/src/config_loader/state.rs @@ -1,4 +1,5 @@ use crate::config_loader::ConfigRequirements; +use crate::config_loader::ConfigRequirementsToml; use super::fingerprint::record_origins; use super::fingerprint::version_for_toml; @@ -86,18 +87,25 @@ pub struct ConfigLayerStack { /// Constraints that must be enforced when deriving a [Config] from the /// layers. requirements: ConfigRequirements, + + /// Raw requirements data as loaded from requirements.toml/MDM/legacy + /// sources. This preserves the original allow-lists so they can be + /// surfaced via APIs. + requirements_toml: ConfigRequirementsToml, } impl ConfigLayerStack { pub fn new( layers: Vec, requirements: ConfigRequirements, + requirements_toml: ConfigRequirementsToml, ) -> std::io::Result { let user_layer_index = verify_layer_ordering(&layers)?; Ok(Self { layers, user_layer_index, requirements, + requirements_toml, }) } @@ -111,6 +119,10 @@ impl ConfigLayerStack { &self.requirements } + pub fn requirements_toml(&self) -> &ConfigRequirementsToml { + &self.requirements_toml + } + /// Creates a new [ConfigLayerStack] using the specified values to inject a /// "user layer" into the stack. If such a layer already exists, it is /// replaced; otherwise, it is inserted into the stack at the appropriate @@ -131,6 +143,7 @@ impl ConfigLayerStack { layers, user_layer_index: self.user_layer_index, requirements: self.requirements.clone(), + requirements_toml: self.requirements_toml.clone(), } } None => { @@ -151,6 +164,7 @@ impl ConfigLayerStack { layers, user_layer_index: Some(user_layer_index), requirements: self.requirements.clone(), + requirements_toml: self.requirements_toml.clone(), } } } diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 4c8ad06872..d8880a9b3c 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -421,6 +421,7 @@ mod tests { use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigRequirements; + use crate::config_loader::ConfigRequirementsToml; use crate::features::Feature; use crate::features::Features; use codex_app_server_protocol::ConfigLayerSource; @@ -441,7 +442,12 @@ mod tests { ConfigLayerSource::Project { dot_codex_folder }, TomlValue::Table(Default::default()), ); - ConfigLayerStack::new(vec![layer], ConfigRequirements::default()).expect("ConfigLayerStack") + ConfigLayerStack::new( + vec![layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("ConfigLayerStack") } #[tokio::test] @@ -573,7 +579,11 @@ mod tests { TomlValue::Table(Default::default()), ), ]; - let config_stack = ConfigLayerStack::new(layers, ConfigRequirements::default())?; + let config_stack = ConfigLayerStack::new( + layers, + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + )?; let policy = load_exec_policy(&config_stack).await?; diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 4efa801d97..a7461dcf97 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -410,6 +410,7 @@ mod tests { use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigRequirements; + use crate::config_loader::ConfigRequirementsToml; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; @@ -471,7 +472,11 @@ mod tests { TomlValue::Table(toml::map::Map::new()), ), ]; - let stack = ConfigLayerStack::new(layers, ConfigRequirements::default())?; + let stack = ConfigLayerStack::new( + layers, + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + )?; let got = skill_roots_from_layer_stack(&stack) .into_iter() From 1becaea2752fa8cf63ea010eb403eee994f1b770 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 7 Jan 2026 13:58:30 -0800 Subject: [PATCH 13/13] fix: update resource path resolution logic so it works with Bazel --- codex-rs/exec-server/tests/common/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/codex-rs/exec-server/tests/common/lib.rs b/codex-rs/exec-server/tests/common/lib.rs index a5a6a3cac5..d5e3af208a 100644 --- a/codex-rs/exec-server/tests/common/lib.rs +++ b/codex-rs/exec-server/tests/common/lib.rs @@ -34,10 +34,11 @@ where { let mcp_executable = codex_utils_cargo_bin::cargo_bin("codex-exec-mcp-server")?; let execve_wrapper = codex_utils_cargo_bin::cargo_bin("codex-execve-wrapper")?; - // `bash` requires a special lookup when running under Buck because it is a + + // `bash` requires a special lookup when running under Bazel because it is a // _resource_ rather than a binary target. - let bash = if let Some(root) = codex_utils_cargo_bin::buck_project_root()? { - root.join("codex-rs/exec-server/tests/suite/bash") + let bash = if let Some(bazel_runfiles_dir) = std::env::var_os("RUNFILES_DIR") { + PathBuf::from(bazel_runfiles_dir).join("_main/codex-rs/exec-server/tests/suite/bash") } else { Path::new(env!("CARGO_MANIFEST_DIR")) .join("..")