From fbd4efa9ed6b9fe13dacd56247cc714903df72b7 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 22 May 2026 15:21:08 -0700 Subject: [PATCH 1/9] [codex] Use TurnInput for session task input (#24151) ## Why The idea here is to erase the difference between initial and followup inputs to a turn. Followup inputs are already represented as TurnInput. Eventual goal is not to have explicit on task input at all and pull everything from input Q. ## What Changed - Changes `SessionTask::run` and the erased `AnySessionTask::run` path to accept `Vec`. - Wraps user-submitted spawn input as `TurnInput::UserInput` at the session task start boundary. - Updates `run_turn` to record initial `TurnInput` using the same hook and recording path used for pending input. - Keeps review-specific conversion local to `ReviewTask`, where the sub-Codex one-shot API still expects `Vec`. - Moves the synthetic compact prompt into `CompactTask` and starts compact tasks with empty task input. ## Validation - `cargo check -p codex-core` - `just test -p codex-core -E 'test(task_finish_emits_turn_item_lifecycle_for_leftover_pending_user_input) | test(queued_response_items_for_next_turn_move_into_next_active_turn) | test(steered_input_reopens_mailbox_delivery_for_current_turn)'` --- codex-rs/core/src/session/handlers.rs | 12 +-- codex-rs/core/src/session/tests.rs | 6 +- codex-rs/core/src/session/turn.rs | 104 +++++++++++++------------- codex-rs/core/src/tasks/compact.rs | 8 +- codex-rs/core/src/tasks/mod.rs | 13 +++- codex-rs/core/src/tasks/regular.rs | 4 +- codex-rs/core/src/tasks/review.rs | 13 +++- codex-rs/core/src/tasks/user_shell.rs | 4 +- 8 files changed, 89 insertions(+), 75 deletions(-) diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index 3d57bd7074..b1e36b0347 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -460,16 +460,8 @@ pub async fn reload_user_config(sess: &Arc) { pub async fn compact(sess: &Arc, sub_id: String) { let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; - sess.spawn_task( - Arc::clone(&turn_context), - vec![UserInput::Text { - text: turn_context.compact_prompt().to_string(), - // Compaction prompt is synthesized; no UI element ranges to preserve. - text_elements: Vec::new(), - }], - CompactTask, - ) - .await; + sess.spawn_task(Arc::clone(&turn_context), Vec::new(), CompactTask) + .await; } pub async fn thread_rollback(sess: &Arc, sub_id: String, num_turns: u32) { diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index df7559a7d9..30d26191d7 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -5738,7 +5738,7 @@ async fn spawn_task_turn_span_inherits_dispatch_trace_context() { self: Arc, _session: Arc, _ctx: Arc, - _input: Vec, + _input: Vec, _cancellation_token: CancellationToken, ) -> Option { let mut trace = self @@ -7814,7 +7814,7 @@ impl SessionTask for NeverEndingTask { self: Arc, _session: Arc, _ctx: Arc, - _input: Vec, + _input: Vec, cancellation_token: CancellationToken, ) -> Option { if self.listen_to_cancellation_token { @@ -7843,7 +7843,7 @@ impl SessionTask for GuardianDeniedApprovalTask { self: Arc, session: Arc, ctx: Arc, - _input: Vec, + _input: Vec, cancellation_token: CancellationToken, ) -> Option { let session = session.clone_session(); diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 26ba845460..f81b2b1f74 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -114,8 +114,8 @@ use tracing::trace; use tracing::trace_span; use tracing::warn; -/// Takes a user message as input and runs a loop where, at each sampling request, the model -/// replies with either: +/// Takes initial turn input and runs a loop where, at each sampling request, +/// the model replies with either: /// /// - requested function calls /// - an assistant message @@ -132,7 +132,7 @@ pub(crate) async fn run_turn( sess: Arc, turn_context: Arc, turn_extension_data: Arc, - input: Vec, + input: Vec, prewarmed_client_session: Option, cancellation_token: CancellationToken, ) -> Option { @@ -172,26 +172,9 @@ pub(crate) async fn run_turn( if run_pending_session_start_hooks(&sess, &turn_context).await { return None; } - if !input.is_empty() { - let initial_turn_input = TurnInput::UserInput(input.clone()); - let user_prompt_submit_outcome = - inspect_pending_input(&sess, &turn_context, &initial_turn_input).await; - if user_prompt_submit_outcome.should_stop { - record_additional_contexts( - &sess, - &turn_context, - user_prompt_submit_outcome.additional_contexts, - ) - .await; - return None; - } - record_pending_input( - &sess, - &turn_context, - initial_turn_input, - user_prompt_submit_outcome.additional_contexts, - ) - .await; + let mut can_drain_pending_input = input.is_empty(); + if run_hooks_and_record_inputs(&sess, &turn_context, &input).await { + return None; } sess.merge_connector_selection(explicitly_enabled_connectors.clone()) @@ -232,9 +215,8 @@ pub(crate) async fn run_turn( // one instance across retries within this turn. // Pending input is drained into history before building the next model request. // However, we defer that drain until after sampling in two cases: - // 1. At the start of a turn, so the fresh user prompt in `input` gets sampled first. + // 1. At the start of a turn, so the fresh turn input in `input` gets sampled first. // 2. After auto-compact, when model/tool continuation needs to resume before any steer. - let mut can_drain_pending_input = input.is_empty(); loop { // Note that pending_input would be something like a message the user @@ -246,27 +228,7 @@ pub(crate) async fn run_turn( Vec::new() }; - let mut blocked_pending_input = false; - let mut accepted_pending_input = false; - for pending_input_item in pending_input { - let hook_outcome = - inspect_pending_input(&sess, &turn_context, &pending_input_item).await; - if hook_outcome.should_stop { - blocked_pending_input = true; - record_additional_contexts(&sess, &turn_context, hook_outcome.additional_contexts) - .await; - } else { - accepted_pending_input = true; - record_pending_input( - &sess, - &turn_context, - pending_input_item, - hook_outcome.additional_contexts, - ) - .await; - } - } - if blocked_pending_input && !accepted_pending_input { + if run_hooks_and_record_inputs(&sess, &turn_context, &pending_input).await { break; } @@ -450,6 +412,32 @@ pub(crate) async fn run_turn( last_agent_message } +async fn run_hooks_and_record_inputs( + sess: &Arc, + turn_context: &Arc, + input: &[TurnInput], +) -> bool { + let mut blocked_input = false; + let mut accepted_input = false; + for input_item in input { + let hook_outcome = inspect_pending_input(sess, turn_context, input_item).await; + if hook_outcome.should_stop { + blocked_input = true; + record_additional_contexts(sess, turn_context, hook_outcome.additional_contexts).await; + } else { + accepted_input = true; + record_pending_input( + sess, + turn_context, + input_item.clone(), + hook_outcome.additional_contexts, + ) + .await; + } + } + blocked_input && !accepted_input +} + #[expect( clippy::await_holding_invalid_type, reason = "MCP tool listing borrows the read guard across cancellation-aware await" @@ -457,9 +445,18 @@ pub(crate) async fn run_turn( async fn build_skills_and_plugins( sess: &Arc, turn_context: &TurnContext, - input: &[UserInput], + input: &[TurnInput], cancellation_token: &CancellationToken, ) -> Option<(Vec, HashSet)> { + let user_input = input + .iter() + .filter_map(|item| match item { + TurnInput::UserInput(content) => Some(content.as_slice()), + TurnInput::ResponseInputItem(_) => None, + }) + .flatten() + .cloned() + .collect::>(); let tracking = build_track_events_context( turn_context.model_info.slug.clone(), sess.conversation_id.to_string(), @@ -473,7 +470,7 @@ async fn build_skills_and_plugins( // Structured plugin:// mentions are resolved from the current session's // enabled plugins, then converted into turn-scoped guidance below. let mentioned_plugins = - collect_explicit_plugin_mentions(input, loaded_plugins.capability_summaries()); + collect_explicit_plugin_mentions(&user_input, loaded_plugins.capability_summaries()); let mcp_tools = if turn_context.apps_enabled() || !mentioned_plugins.is_empty() { // Plugin mentions need raw MCP/app inventory even when app tools // are normally hidden so we can describe the plugin's currently @@ -511,7 +508,7 @@ async fn build_skills_and_plugins( let skill_name_counts_lower = build_skill_name_counts(&skills_outcome.skills, &skills_outcome.disabled_paths).1; let mentioned_skills = collect_explicit_skill_mentions( - input, + &user_input, &skills_outcome.skills, &skills_outcome.disabled_paths, &connector_slug_counts, @@ -553,7 +550,7 @@ async fn build_skills_and_plugins( ); let plugin_items = build_plugin_injections(&mentioned_plugins, &mcp_tools, &available_connectors); - let mut explicitly_enabled_connectors = collect_explicit_app_ids(input); + let mut explicitly_enabled_connectors = collect_explicit_app_ids(&user_input); explicitly_enabled_connectors.extend(skill_connector_ids); let connector_names_by_id = available_connectors .iter() @@ -589,7 +586,7 @@ async fn build_skills_and_plugins( async fn track_turn_resolved_config_analytics( sess: &Session, turn_context: &TurnContext, - input: &[UserInput], + input: &[TurnInput], ) { let thread_config = { let state = sess.state.lock().await; @@ -606,6 +603,11 @@ async fn track_turn_resolved_config_analytics( thread_id: sess.conversation_id.to_string(), num_input_images: input .iter() + .filter_map(|item| match item { + TurnInput::UserInput(content) => Some(content.as_slice()), + TurnInput::ResponseInputItem(_) => None, + }) + .flatten() .filter(|item| { matches!(item, UserInput::Image { .. } | UserInput::LocalImage { .. }) }) diff --git a/codex-rs/core/src/tasks/compact.rs b/codex-rs/core/src/tasks/compact.rs index dddf46391e..77914633a2 100644 --- a/codex-rs/core/src/tasks/compact.rs +++ b/codex-rs/core/src/tasks/compact.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use super::SessionTask; use super::SessionTaskContext; +use crate::session::TurnInput; use crate::session::turn_context::TurnContext; use crate::state::TaskKind; use codex_protocol::user_input::UserInput; @@ -23,7 +24,7 @@ impl SessionTask for CompactTask { self: Arc, session: Arc, ctx: Arc, - input: Vec, + _input: Vec, _cancellation_token: CancellationToken, ) -> Option { let session = session.clone_session(); @@ -47,6 +48,11 @@ impl SessionTask for CompactTask { /*inc*/ 1, &[("type", "local")], ); + let input = vec![UserInput::Text { + text: ctx.compact_prompt().to_string(), + // Compaction prompt is synthesized; no UI element ranges to preserve. + text_elements: Vec::new(), + }]; crate::compact::run_compact_task(session.clone(), ctx, input).await }; None diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index 9cb6b5f0f2..4d6db8acf2 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -214,7 +214,7 @@ pub(crate) trait SessionTask: Send + Sync + 'static { self: Arc, session: Arc, ctx: Arc, - input: Vec, + input: Vec, cancellation_token: CancellationToken, ) -> impl std::future::Future> + Send; @@ -245,7 +245,7 @@ pub(crate) trait AnySessionTask: Send + Sync + 'static { self: Arc, session: Arc, ctx: Arc, - input: Vec, + input: Vec, cancellation_token: CancellationToken, ) -> BoxFuture<'static, Option>; @@ -276,7 +276,7 @@ where self: Arc, session: Arc, ctx: Arc, - input: Vec, + input: Vec, cancellation_token: CancellationToken, ) -> BoxFuture<'static, Option> { Box::pin(SessionTask::run( @@ -380,6 +380,11 @@ impl Session { )); let ctx = Arc::clone(&turn_context); let task_for_run = Arc::clone(&task); + let task_input = if input.is_empty() { + Vec::new() + } else { + vec![TurnInput::UserInput(input)] + }; let task_cancellation_token = cancellation_token.child_token(); // Task-owned turn spans keep a core-owned span open for the // full task lifecycle after the submission dispatch span ends. @@ -405,7 +410,7 @@ impl Session { .run( Arc::clone(&session_ctx), ctx, - input, + task_input, task_cancellation_token.child_token(), ) .await; diff --git a/codex-rs/core/src/tasks/regular.rs b/codex-rs/core/src/tasks/regular.rs index 531d5d7da7..6c6a0e5b09 100644 --- a/codex-rs/core/src/tasks/regular.rs +++ b/codex-rs/core/src/tasks/regular.rs @@ -2,13 +2,13 @@ use std::sync::Arc; use tokio_util::sync::CancellationToken; +use crate::session::TurnInput; use crate::session::turn::run_turn; use crate::session::turn_context::TurnContext; use crate::session_startup_prewarm::SessionStartupPrewarmResolution; use crate::state::TaskKind; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::TurnStartedEvent; -use codex_protocol::user_input::UserInput; use tracing::Instrument; use tracing::trace_span; @@ -41,7 +41,7 @@ impl SessionTask for RegularTask { self: Arc, session: Arc, ctx: Arc, - input: Vec, + input: Vec, cancellation_token: CancellationToken, ) -> Option { let sess = session.clone_session(); diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index f89c7d062f..ff7f38a7ea 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -20,6 +20,7 @@ use crate::codex_delegate::run_codex_thread_one_shot; use crate::config::Constrained; use crate::review_format::format_review_findings_block; use crate::review_format::render_review_output_text; +use crate::session::TurnInput; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::state::TaskKind; @@ -59,7 +60,7 @@ impl SessionTask for ReviewTask { self: Arc, session: Arc, ctx: Arc, - input: Vec, + input: Vec, cancellation_token: CancellationToken, ) -> Option { session.session.services.session_telemetry.counter( @@ -68,11 +69,19 @@ impl SessionTask for ReviewTask { &[], ); + let mut user_input = Vec::new(); + for item in input { + match item { + TurnInput::UserInput(mut content) => user_input.append(&mut content), + TurnInput::ResponseInputItem(_) => {} + } + } + // Start sub-codex conversation and get the receiver for events. let output = match start_review_conversation( session.clone(), ctx.clone(), - input, + user_input, cancellation_token.clone(), ) .await diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index 2ce4056957..396aecbeea 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -7,7 +7,6 @@ use codex_network_proxy::PROXY_ACTIVE_ENV_KEY; use codex_network_proxy::PROXY_ENV_KEYS; #[cfg(target_os = "macos")] use codex_network_proxy::PROXY_GIT_SSH_COMMAND_ENV_KEY; -use codex_protocol::user_input::UserInput; use tokio_util::sync::CancellationToken; use tracing::error; use uuid::Uuid; @@ -17,6 +16,7 @@ use crate::exec::StdoutStream; use crate::exec::execute_exec_request; use crate::exec_env::create_env; use crate::sandboxing::ExecRequest; +use crate::session::TurnInput; use crate::session::turn_context::TurnContext; use crate::state::TaskKind; use crate::tools::format_exec_output_str; @@ -77,7 +77,7 @@ impl SessionTask for UserShellCommandTask { self: Arc, session: Arc, turn_context: Arc, - _input: Vec, + _input: Vec, cancellation_token: CancellationToken, ) -> Option { execute_user_shell_command( From 7924743c38e9a28795c1533ab5143dceea5ca566 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Fri, 22 May 2026 15:38:40 -0700 Subject: [PATCH 2/9] [codex] Add image re-encoding benchmarks (#23935) ## Summary - add Divan benchmarks for prompt image re-encoding paths - wire the image benchmark smoke test into Rust CI workflows ## Why Image prompt handling includes re-encoding work that benefits from repeatable benchmark coverage so changes can be measured in CI and locally. This already helped identify a potential regression from changing compiler flags. ## Impact Developers can run and compare the new image re-encoding benchmarks, and CI exercises the benchmark target via the Rust benchmark smoke test. --- .github/workflows/rust-ci-full.yml | 5 + .github/workflows/rust-ci.yml | 5 + MODULE.bazel.lock | 3 + codex-rs/Cargo.lock | 34 +++- codex-rs/Cargo.toml | 1 + codex-rs/utils/image/Cargo.toml | 5 + codex-rs/utils/image/benches/prompt_images.rs | 178 ++++++++++++++++++ justfile | 9 + 8 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 codex-rs/utils/image/benches/prompt_images.rs diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index b65c2f5d8f..c4a9329c00 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -28,8 +28,13 @@ jobs: - uses: dtolnay/rust-toolchain@a0b273b48ed29de4470960879e8381ff45632f26 # 1.93.0 with: components: rustfmt + - uses: taiki-e/install-action@44c6d64aa62cd779e873306675c7a58e86d6d532 # v2.62.49 + with: + tool: just - name: cargo fmt run: cargo fmt -- --config imports_granularity=Item --check + - name: Rust benchmark smoke test + run: just bench-smoke cargo_shear: name: cargo shear diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index 75c5c33601..029b6f2c7e 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -70,8 +70,13 @@ jobs: - uses: dtolnay/rust-toolchain@a0b273b48ed29de4470960879e8381ff45632f26 # 1.93.0 with: components: rustfmt + - uses: taiki-e/install-action@44c6d64aa62cd779e873306675c7a58e86d6d532 # v2.62.49 + with: + tool: just - name: cargo fmt run: cargo fmt -- --config imports_granularity=Item --check + - name: Rust benchmark smoke test + run: just bench-smoke cargo_shear: name: cargo shear diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 1392b5a568..ad56196dd9 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -768,6 +768,7 @@ "compact_str_0.8.1": "{\"dependencies\":[{\"default_features\":false,\"name\":\"arbitrary\",\"optional\":true,\"req\":\"^1\"},{\"name\":\"borsh\",\"optional\":true,\"req\":\"^1\"},{\"name\":\"bytes\",\"optional\":true,\"req\":\"^1\"},{\"default_features\":false,\"features\":[\"alloc\"],\"name\":\"castaway\",\"req\":\"^0.2.3\"},{\"name\":\"cfg-if\",\"req\":\"^1\"},{\"kind\":\"dev\",\"name\":\"cfg-if\",\"req\":\"^1\"},{\"default_features\":false,\"name\":\"diesel\",\"optional\":true,\"req\":\"^2\"},{\"name\":\"itoa\",\"req\":\"^1\"},{\"default_features\":false,\"name\":\"markup\",\"optional\":true,\"req\":\"^0.13\"},{\"default_features\":false,\"features\":[\"std\"],\"name\":\"proptest\",\"optional\":true,\"req\":\"^1\"},{\"default_features\":false,\"features\":[\"std\"],\"kind\":\"dev\",\"name\":\"proptest\",\"req\":\"^1\"},{\"default_features\":false,\"name\":\"quickcheck\",\"optional\":true,\"req\":\"^1\"},{\"default_features\":false,\"kind\":\"dev\",\"name\":\"quickcheck\",\"req\":\"^1\"},{\"kind\":\"dev\",\"name\":\"quickcheck_macros\",\"req\":\"^1\"},{\"kind\":\"dev\",\"name\":\"rayon\",\"req\":\"^1\"},{\"default_features\":false,\"features\":[\"size_32\"],\"name\":\"rkyv\",\"optional\":true,\"req\":\"^0.7\"},{\"default_features\":false,\"features\":[\"alloc\",\"size_32\"],\"kind\":\"dev\",\"name\":\"rkyv\",\"req\":\"^0.7\"},{\"name\":\"rustversion\",\"req\":\"^1\"},{\"name\":\"ryu\",\"req\":\"^1\"},{\"default_features\":false,\"features\":[\"derive\",\"alloc\"],\"name\":\"serde\",\"optional\":true,\"req\":\"^1\"},{\"features\":[\"derive\"],\"kind\":\"dev\",\"name\":\"serde\",\"req\":\"^1\"},{\"kind\":\"dev\",\"name\":\"serde_json\",\"req\":\"^1\"},{\"features\":[\"union\"],\"name\":\"smallvec\",\"optional\":true,\"req\":\"^1\"},{\"default_features\":false,\"name\":\"sqlx\",\"optional\":true,\"req\":\"^0.7\"},{\"name\":\"static_assertions\",\"req\":\"^1\"},{\"kind\":\"dev\",\"name\":\"test-case\",\"req\":\"^3\"},{\"kind\":\"dev\",\"name\":\"test-strategy\",\"req\":\"^0.3\"}],\"features\":{\"arbitrary\":[\"dep:arbitrary\"],\"borsh\":[\"dep:borsh\"],\"bytes\":[\"dep:bytes\"],\"default\":[\"std\"],\"diesel\":[\"dep:diesel\"],\"markup\":[\"dep:markup\"],\"proptest\":[\"dep:proptest\"],\"quickcheck\":[\"dep:quickcheck\"],\"rkyv\":[\"dep:rkyv\"],\"serde\":[\"dep:serde\"],\"smallvec\":[\"dep:smallvec\"],\"sqlx\":[\"dep:sqlx\",\"std\"],\"sqlx-mysql\":[\"sqlx\",\"sqlx/mysql\"],\"sqlx-postgres\":[\"sqlx\",\"sqlx/postgres\"],\"sqlx-sqlite\":[\"sqlx\",\"sqlx/sqlite\"],\"std\":[]}}", "compiletest_rs_0.11.2": "{\"dependencies\":[{\"name\":\"diff\",\"req\":\"^0.1.10\"},{\"name\":\"filetime\",\"req\":\"^0.2\"},{\"name\":\"getopts\",\"req\":\"^0.2\"},{\"name\":\"lazy_static\",\"req\":\"^1.4\"},{\"name\":\"libc\",\"req\":\"^0.2\",\"target\":\"cfg(unix)\"},{\"name\":\"log\",\"req\":\"^0.4\"},{\"name\":\"miow\",\"req\":\"^0.6\",\"target\":\"cfg(windows)\"},{\"name\":\"regex\",\"req\":\"^1.0\"},{\"name\":\"rustfix\",\"req\":\"^0.8\"},{\"name\":\"serde\",\"req\":\"^1.0\"},{\"name\":\"serde_derive\",\"req\":\"^1.0\"},{\"name\":\"serde_json\",\"req\":\"^1.0\"},{\"name\":\"tempfile\",\"optional\":true,\"req\":\"^3.0\"},{\"name\":\"tester\",\"req\":\"^0.9\"},{\"features\":[\"Win32\"],\"name\":\"windows-sys\",\"req\":\"^0.59\",\"target\":\"cfg(windows)\"}],\"features\":{\"rustc\":[],\"stable\":[],\"tmp\":[\"tempfile\"]}}", "concurrent-queue_2.5.0": "{\"dependencies\":[{\"default_features\":false,\"features\":[\"cargo_bench_support\"],\"kind\":\"dev\",\"name\":\"criterion\",\"req\":\"^0.5\"},{\"default_features\":false,\"name\":\"crossbeam-utils\",\"req\":\"^0.8.11\"},{\"kind\":\"dev\",\"name\":\"easy-parallel\",\"req\":\"^3.1.0\"},{\"kind\":\"dev\",\"name\":\"fastrand\",\"req\":\"^2.0.0\"},{\"name\":\"loom\",\"optional\":true,\"req\":\"^0.7\",\"target\":\"cfg(loom)\"},{\"default_features\":false,\"name\":\"portable-atomic\",\"optional\":true,\"req\":\"^1\"},{\"kind\":\"dev\",\"name\":\"wasm-bindgen-test\",\"req\":\"^0.3\",\"target\":\"cfg(target_family = \\\"wasm\\\")\"}],\"features\":{\"default\":[\"std\"],\"std\":[]}}", + "condtype_1.3.0": "{\"dependencies\":[{\"kind\":\"dev\",\"name\":\"cfg-if\",\"req\":\"^1\"},{\"kind\":\"dev\",\"name\":\"libc\",\"req\":\"^0.2.141\"}],\"features\":{}}", "console_0.15.11": "{\"dependencies\":[{\"name\":\"encode_unicode\",\"req\":\"^1\",\"target\":\"cfg(windows)\"},{\"name\":\"libc\",\"req\":\"^0.2.99\"},{\"name\":\"once_cell\",\"req\":\"^1.8\"},{\"default_features\":false,\"features\":[\"std\",\"bit-set\",\"break-dead-code\"],\"kind\":\"dev\",\"name\":\"proptest\",\"req\":\"^1.0.0\"},{\"kind\":\"dev\",\"name\":\"regex\",\"req\":\"^1.4.2\"},{\"name\":\"unicode-width\",\"optional\":true,\"req\":\"^0.2\"},{\"features\":[\"Win32_Foundation\",\"Win32_System_Console\",\"Win32_Storage_FileSystem\",\"Win32_UI_Input_KeyboardAndMouse\"],\"name\":\"windows-sys\",\"req\":\"^0.59\",\"target\":\"cfg(windows)\"}],\"features\":{\"ansi-parsing\":[],\"default\":[\"unicode-width\",\"ansi-parsing\"],\"windows-console-colors\":[\"ansi-parsing\"]}}", "const-hex_1.17.0": "{\"dependencies\":[{\"name\":\"cfg-if\",\"req\":\"^1\"},{\"name\":\"cpufeatures\",\"req\":\"^0.2\",\"target\":\"cfg(any(target_arch = \\\"x86\\\", target_arch = \\\"x86_64\\\"))\"},{\"kind\":\"dev\",\"name\":\"divan\",\"package\":\"codspeed-divan-compat\",\"req\":\"^3\"},{\"default_features\":false,\"features\":[\"alloc\"],\"kind\":\"dev\",\"name\":\"faster-hex\",\"req\":\"^0.10.0\"},{\"default_features\":false,\"features\":[\"alloc\"],\"kind\":\"dev\",\"name\":\"hex\",\"req\":\"~0.4.2\"},{\"default_features\":false,\"name\":\"proptest\",\"optional\":true,\"req\":\"^1.4\"},{\"kind\":\"dev\",\"name\":\"rustc-hex\",\"req\":\"^2.1\"},{\"default_features\":false,\"features\":[\"derive\"],\"kind\":\"dev\",\"name\":\"serde\",\"req\":\"^1.0\"},{\"default_features\":false,\"name\":\"serde_core\",\"optional\":true,\"req\":\"^1.0\"},{\"default_features\":false,\"features\":[\"alloc\"],\"kind\":\"dev\",\"name\":\"serde_json\",\"req\":\"^1.0\"}],\"features\":{\"__fuzzing\":[\"dep:proptest\",\"std\"],\"alloc\":[\"serde_core?/alloc\",\"proptest?/alloc\"],\"core-error\":[],\"default\":[\"std\"],\"force-generic\":[],\"hex\":[],\"nightly\":[],\"portable-simd\":[],\"serde\":[\"dep:serde_core\"],\"std\":[\"serde_core?/std\",\"proptest?/std\",\"alloc\"]}}", "const-oid_0.9.6": "{\"dependencies\":[{\"features\":[\"derive\"],\"name\":\"arbitrary\",\"optional\":true,\"req\":\"^1.2\"},{\"kind\":\"dev\",\"name\":\"hex-literal\",\"req\":\"^0.3\"}],\"features\":{\"db\":[],\"std\":[]}}", @@ -859,6 +860,8 @@ "dispatch2_0.3.0": "{\"dependencies\":[{\"default_features\":false,\"features\":[\"std\"],\"name\":\"bitflags\",\"req\":\"^2.5.0\"},{\"default_features\":false,\"features\":[\"alloc\"],\"name\":\"block2\",\"optional\":true,\"req\":\">=0.6.1, <0.8.0\"},{\"default_features\":false,\"name\":\"libc\",\"optional\":true,\"req\":\"^0.2.80\"},{\"default_features\":false,\"features\":[\"std\"],\"name\":\"objc2\",\"optional\":true,\"req\":\">=0.6.1, <0.8.0\"},{\"kind\":\"dev\",\"name\":\"static_assertions\",\"req\":\"^1.1.0\"}],\"features\":{\"alloc\":[],\"block2\":[\"dep:block2\"],\"default\":[\"std\",\"block2\",\"libc\",\"objc2\"],\"libc\":[\"dep:libc\"],\"objc2\":[\"dep:objc2\"],\"std\":[\"alloc\"]}}", "display_container_0.9.0": "{\"dependencies\":[{\"name\":\"either\",\"req\":\"^1.8\"},{\"name\":\"indenter\",\"req\":\"^0.3.3\"}],\"features\":{}}", "displaydoc_0.2.5": "{\"dependencies\":[{\"default_features\":false,\"kind\":\"dev\",\"name\":\"libc\",\"req\":\"^0.2\"},{\"kind\":\"dev\",\"name\":\"pretty_assertions\",\"req\":\"^0.6.1\"},{\"name\":\"proc-macro2\",\"req\":\"^1.0\"},{\"name\":\"quote\",\"req\":\"^1.0\"},{\"kind\":\"dev\",\"name\":\"rustversion\",\"req\":\"^1.0.0\"},{\"kind\":\"dev\",\"name\":\"static_assertions\",\"req\":\"^1.1\"},{\"name\":\"syn\",\"req\":\"^2.0\"},{\"kind\":\"dev\",\"name\":\"thiserror\",\"req\":\"^1.0.24\"},{\"kind\":\"dev\",\"name\":\"trybuild\",\"req\":\"^1.0\"}],\"features\":{\"default\":[\"std\"],\"std\":[]}}", + "divan-macros_0.1.21": "{\"dependencies\":[{\"name\":\"proc-macro2\",\"req\":\"^1\"},{\"default_features\":false,\"name\":\"quote\",\"req\":\"^1\"},{\"default_features\":false,\"features\":[\"full\",\"clone-impls\",\"parsing\",\"printing\",\"proc-macro\"],\"name\":\"syn\",\"req\":\"^2.0.18\"}],\"features\":{}}", + "divan_0.1.21": "{\"dependencies\":[{\"name\":\"cfg-if\",\"req\":\"^1\"},{\"default_features\":false,\"features\":[\"std\",\"env\"],\"name\":\"clap\",\"req\":\"^4\"},{\"name\":\"condtype\",\"req\":\"^1.3\"},{\"name\":\"divan-macros\",\"req\":\"=0.1.21\"},{\"name\":\"libc\",\"req\":\"^0.2.148\",\"target\":\"cfg(unix)\"},{\"kind\":\"dev\",\"name\":\"mimalloc\",\"req\":\"^0.1\"},{\"default_features\":false,\"features\":[\"std\",\"string\"],\"name\":\"regex\",\"package\":\"regex-lite\",\"req\":\"^0.1\"}],\"features\":{\"default\":[\"wrap_help\"],\"dyn_thread_local\":[],\"help\":[\"clap/help\"],\"internal_benches\":[],\"wrap_help\":[\"help\",\"clap/wrap_help\"]}}", "dns-lookup_3.0.1": "{\"dependencies\":[{\"name\":\"cfg-if\",\"req\":\"^1.0\"},{\"name\":\"libc\",\"req\":\"^0.2\",\"target\":\"cfg(unix)\"},{\"name\":\"socket2\",\"req\":\"^0.6.0\"},{\"features\":[\"Win32_Networking_WinSock\",\"Win32_Foundation\"],\"name\":\"windows-sys\",\"req\":\"^0.60\",\"target\":\"cfg(windows)\"}],\"features\":{}}", "document-features_0.2.12": "{\"dependencies\":[{\"name\":\"litrs\",\"req\":\"^1.0.0\"}],\"features\":{\"default\":[],\"self-test\":[]}}", "dotenvy_0.15.7": "{\"dependencies\":[{\"name\":\"clap\",\"optional\":true,\"req\":\"^3.2\"},{\"kind\":\"dev\",\"name\":\"once_cell\",\"req\":\"^1.16.0\"},{\"kind\":\"dev\",\"name\":\"tempfile\",\"req\":\"^3.3.0\"}],\"features\":{\"cli\":[\"clap\"]}}", diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 38ce92e46b..779785704d 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3981,6 +3981,7 @@ version = "0.0.0" dependencies = [ "base64 0.22.1", "codex-utils-cache", + "divan", "image", "mime_guess", "thiserror 2.0.18", @@ -4223,6 +4224,12 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "condtype" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "baf0a07a401f374238ab8e2f11a104d2851bf9ce711ec69804834de8af45c7af" + [[package]] name = "console" version = "0.15.11" @@ -5219,6 +5226,31 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "divan" +version = "0.1.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a405457ec78b8fe08b0e32b4a3570ab5dff6dd16eb9e76a5ee0a9d9cbd898933" +dependencies = [ + "cfg-if", + "clap", + "condtype", + "divan-macros", + "libc", + "regex-lite", +] + +[[package]] +name = "divan-macros" +version = "0.1.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9556bc800956545d6420a640173e5ba7dfa82f38d3ea5a167eb555bc69ac3323" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.114", +] + [[package]] name = "dns-lookup" version = "3.0.1" @@ -5492,7 +5524,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index df169504f0..95c065c74b 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -275,6 +275,7 @@ deno_core_icudata = "0.77.0" derive_more = "2" diffy = "0.4.2" dirs = "6" +divan = "0.1.21" dns-lookup = "3.0.1" dotenvy = "0.15.7" dunce = "1.0.4" diff --git a/codex-rs/utils/image/Cargo.toml b/codex-rs/utils/image/Cargo.toml index 5ac187caaa..7ba28f4996 100644 --- a/codex-rs/utils/image/Cargo.toml +++ b/codex-rs/utils/image/Cargo.toml @@ -16,7 +16,12 @@ thiserror = { workspace = true } tokio = { workspace = true, features = ["fs", "rt", "rt-multi-thread", "macros"] } [dev-dependencies] +divan = { workspace = true } image = { workspace = true, features = ["jpeg", "png", "gif", "webp"] } [lib] doctest = false + +[[bench]] +name = "prompt_images" +harness = false diff --git a/codex-rs/utils/image/benches/prompt_images.rs b/codex-rs/utils/image/benches/prompt_images.rs new file mode 100644 index 0000000000..5d2bcb61cb --- /dev/null +++ b/codex-rs/utils/image/benches/prompt_images.rs @@ -0,0 +1,178 @@ +use std::io::Cursor; +use std::path::Path; + +use codex_utils_image::PromptImageMode; +use codex_utils_image::load_for_prompt_bytes; +use divan::Bencher; +use image::DynamicImage; +use image::ImageFormat; +use image::Rgb; +use image::RgbImage; +use image::Rgba; +use image::RgbaImage; + +const CACHE_MISS_VARIANT_COUNT: usize = 48; + +const SMALL_SCREENSHOT: ImageSize = ImageSize { + width: 1_536, + height: 864, +}; +const LARGE_SCREENSHOT: ImageSize = ImageSize { + width: 2_560, + height: 1_440, +}; +const LARGE_PHOTO: ImageSize = ImageSize { + width: 3_264, + height: 2_448, +}; + +#[derive(Clone, Copy)] +struct ImageSize { + width: u32, + height: u32, +} + +fn main() { + divan::main(); +} + +#[divan::bench] +fn small_png_screenshot_fresh_attachment(bencher: Bencher) { + bench_fresh_attachment( + bencher, + "small-screenshot.png", + cache_miss_variants(screenshot_png(SMALL_SCREENSHOT)), + ); +} + +#[divan::bench] +fn large_png_screenshot_fresh_attachment(bencher: Bencher) { + bench_fresh_attachment( + bencher, + "large-screenshot.png", + cache_miss_variants(screenshot_png(LARGE_SCREENSHOT)), + ); +} + +#[divan::bench] +fn large_jpeg_photo_fresh_attachment(bencher: Bencher) { + bench_fresh_attachment( + bencher, + "large-photo.jpg", + cache_miss_variants(photo_jpeg(LARGE_PHOTO)), + ); +} + +#[divan::bench] +fn small_png_screenshot_repeated_attachment(bencher: Bencher) { + bench_repeated_attachment( + bencher, + "small-screenshot.png", + screenshot_png(SMALL_SCREENSHOT), + ); +} + +fn bench_fresh_attachment(bencher: Bencher, path: &'static str, images: Vec>) { + let mut image_index = 0; + + bencher + // Divan excludes `with_inputs` from the measured benchmark timing. + .with_inputs(move || { + let image = images[image_index].clone(); + image_index = (image_index + 1) % images.len(); + image + }) + .bench_local_values(move |image| prepare_prompt_data_url(path, image)); +} + +fn bench_repeated_attachment(bencher: Bencher, path: &'static str, image: Vec) { + let _ = prepare_prompt_data_url(path, image.clone()); + + bencher + // Divan excludes the per-iteration input clone from measured timing. + .with_inputs(move || image.clone()) + .bench_local_values(move |image| prepare_prompt_data_url(path, image)); +} + +fn prepare_prompt_data_url(path: &str, image: Vec) -> String { + #[allow(clippy::expect_used)] + load_for_prompt_bytes(Path::new(path), image, PromptImageMode::ResizeToFit) + .expect("benchmark fixture should load") + .into_data_url() +} + +fn cache_miss_variants(image: Vec) -> Vec> { + // The loader caches by content digest. Suffixes keep this workload on the miss path. + (0..CACHE_MISS_VARIANT_COUNT) + .map(|variant| { + let mut image = image.clone(); + image.extend_from_slice(&variant.to_le_bytes()); + image + }) + .collect() +} + +/// Encodes a synthetic UI screenshot fixture for prompt image benchmarks. +fn screenshot_png(size: ImageSize) -> Vec { + let image = RgbaImage::from_fn(size.width, size.height, |x, y| { + let toolbar = y < 52; + let sidebar = x < 240; + let panel_border = x % 320 < 2 || y % 216 < 2; + let text_row = x > 270 && y > 88 && x % 19 < 13 && y % 31 < 3; + + if toolbar { + Rgba([33, 40, 52, 255]) + } else if sidebar { + let selection = y / 68 % 5 == 2; + if selection { + Rgba([65, 106, 171, 255]) + } else { + Rgba([44, 54, 67, 255]) + } + } else if panel_border { + Rgba([198, 205, 216, 255]) + } else if text_row { + Rgba([72, 82, 96, 255]) + } else { + let panel = ((x / 320) + (y / 216) * 3) % 4; + match panel { + 0 => Rgba([246, 248, 252, 255]), + 1 => Rgba([234, 241, 250, 255]), + 2 => Rgba([240, 247, 236, 255]), + _ => Rgba([250, 240, 235, 255]), + } + } + }); + + encode_fixture(DynamicImage::ImageRgba8(image), ImageFormat::Png) +} + +/// Encodes a synthetic textured photo fixture for prompt image benchmarks. +fn photo_jpeg(size: ImageSize) -> Vec { + let image = RgbImage::from_fn(size.width, size.height, |x, y| { + let x_gradient = x * 255 / size.width; + let y_gradient = y * 255 / size.height; + let texture = ((x.wrapping_mul(17) ^ y.wrapping_mul(31) ^ (x / 7) ^ (y / 11)) & 0xff) as u8; + + Rgb([ + blend_channel(x_gradient, texture, 3), + blend_channel((x_gradient + y_gradient) / 2, texture, 5), + blend_channel(255 - y_gradient, texture, 4), + ]) + }); + + encode_fixture(DynamicImage::ImageRgb8(image), ImageFormat::Jpeg) +} + +fn blend_channel(gradient: u32, texture: u8, divisor: u32) -> u8 { + ((gradient + u32::from(texture) / divisor) % 256) as u8 +} + +fn encode_fixture(image: DynamicImage, format: ImageFormat) -> Vec { + let mut encoded = Cursor::new(Vec::new()); + #[allow(clippy::expect_used)] + image + .write_to(&mut encoded, format) + .expect("benchmark fixture should encode"); + encoded.into_inner() +} diff --git a/justfile b/justfile index 907cd71f6d..d5e9fc3a36 100644 --- a/justfile +++ b/justfile @@ -53,6 +53,15 @@ install: # there should be no need to add `--all-features`. test *args: RUST_MIN_STACK={{ rust_min_stack }} cargo nextest run --no-fail-fast "$@" + just bench-smoke + +# Run explicit workspace benchmark targets. +bench *args: + cargo bench --workspace --bench '*' "$@" + +# Run benchmark targets once to ensure they start successfully. +bench-smoke: + just bench -- --test # Build and run Codex from source using Bazel. # Note we have to use the combination of `[no-cd]` and `--run_under="cd $PWD &&"` From 10ac2781eb7d83d6900686138242dfde12451453 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Fri, 22 May 2026 16:31:33 -0700 Subject: [PATCH 3/9] chore: add JSON schema policy fixture coverage (#24152) ## Why Before changing the Codex Bridge JSON schema policy, add integration coverage around real connector-like MCP tool schemas. The existing unit tests cover individual sanitizer behaviors, but they do not make it easy to see whether full fixture schemas keep model-visible guidance, prune only unreachable definitions, drop unsupported JSON Schema fields, and stay within the Responses API schema budget. ## What Changed - Added `tools/tests/json_schema_policy_fixtures.rs`, which converts MCP tool fixtures through `mcp_tool_to_responses_api_tool` and validates the resulting Responses tool parameters. - Added connector-style fixtures for Slack, Google Calendar, Google Drive, Notion, and Microsoft Outlook Email under `tools/tests/fixtures/json_schema_policy/`. - Added fixture assertions for preserved guidance, pruned definitions, expected field drops after `JsonSchema` conversion, marker count baselines, and dangling local `$ref` prevention. - Added a real oversized golden Notion `create_page` input schema fixture to exercise the compaction path that strips descriptions, drops root `$defs`, rewrites local refs, and fits the compacted schema under the budget. --- codex-rs/Cargo.lock | 1 + codex-rs/tools/Cargo.toml | 1 + .../json_schema_policy/google_calendar.json | 85 ++ .../json_schema_policy/google_drive.json | 65 + .../microsoft_outlook_email.json | 90 ++ .../fixtures/json_schema_policy/notion.json | 72 ++ ...sized_notion_create_page_input_schema.json | 1124 +++++++++++++++++ .../fixtures/json_schema_policy/slack.json | 75 ++ .../tests/json_schema_policy_fixtures.rs | 224 ++++ 9 files changed, 1737 insertions(+) create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/notion.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/slack.json create mode 100644 codex-rs/tools/tests/json_schema_policy_fixtures.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 779785704d..b843fcae6b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3770,6 +3770,7 @@ dependencies = [ "codex-features", "codex-protocol", "codex-utils-absolute-path", + "codex-utils-cargo-bin", "codex-utils-output-truncation", "codex-utils-pty", "codex-utils-string", diff --git a/codex-rs/tools/Cargo.toml b/codex-rs/tools/Cargo.toml index 7d02f1bf36..7cc0e34845 100644 --- a/codex-rs/tools/Cargo.toml +++ b/codex-rs/tools/Cargo.toml @@ -31,6 +31,7 @@ tracing = { workspace = true } urlencoding = { workspace = true } [dev-dependencies] +codex-utils-cargo-bin = { workspace = true } pretty_assertions = { workspace = true } [lib] diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json b/codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json new file mode 100644 index 0000000000..9673ecf596 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json @@ -0,0 +1,85 @@ +{ + "source": "standard/google_calendar/tools", + "tools": [ + { + "name": "google_calendar_create_event", + "description": "Create a calendar event.", + "input_schema": { + "type": "object", + "properties": { + "event": { + "$ref": "#/definitions/Event" + }, + "notify_attendees": { + "type": "boolean", + "description": "Whether attendees should receive notifications." + } + }, + "required": [ + "event" + ], + "definitions": { + "Event": { + "type": "object", + "description": "Calendar event payload.", + "properties": { + "title": { + "type": "string", + "description": "Event title." + }, + "start": { + "anyOf": [ + { + "$ref": "#/definitions/DateTime" + }, + { + "type": "null" + } + ] + } + } + }, + "DateTime": { + "type": "object", + "description": "Calendar date-time.", + "properties": { + "dateTime": { + "type": "string", + "format": "date-time", + "description": "RFC3339 date-time." + }, + "timeZone": { + "type": "string", + "enum": [ + "UTC", + "America/Los_Angeles" + ], + "description": "IANA time zone." + } + } + }, + "UnusedCalendarResource": { + "type": "string", + "description": "Unreachable calendar resource." + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/event/$ref", + "value": "#/definitions/Event" + }, + { + "pointer": "/definitions/DateTime/properties/timeZone/enum/1", + "value": "America/Los_Angeles" + } + ], + "expected_pruned": [ + "/definitions/UnusedCalendarResource" + ], + "expected_dropped_fields": [ + "/definitions/DateTime/properties/dateTime/format" + ] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json b/codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json new file mode 100644 index 0000000000..bf37296cc0 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json @@ -0,0 +1,65 @@ +{ + "source": "standard/google_drive/tools", + "tools": [ + { + "name": "google_drive_copy_file", + "description": "Copy a Google Drive file.", + "input_schema": { + "type": "object", + "properties": { + "file": { + "description": "File selector.", + "oneOf": [ + { + "type": "string", + "description": "A Drive file ID." + }, + { + "type": "object", + "properties": { + "shared_drive_id": { + "type": "string", + "description": "Shared drive identifier." + } + } + } + ] + }, + "metadata": { + "description": "Optional copy metadata.", + "allOf": [ + { + "type": "object", + "description": "Base metadata.", + "properties": { + "source": { + "type": "string", + "description": "Metadata source." + } + } + } + ] + }, + "title": { + "type": "string", + "description": "Copied file title." + } + }, + "required": [ + "file" + ] + }, + "expected_preserved": [ + { + "pointer": "/properties/title/description", + "value": "Copied file title." + } + ], + "expected_pruned": [], + "expected_dropped_fields": [ + "/properties/file/oneOf", + "/properties/metadata/allOf" + ] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json b/codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json new file mode 100644 index 0000000000..bf0aff4118 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json @@ -0,0 +1,90 @@ +{ + "source": "golden/microsoft_outlook_email/tools", + "tools": [ + { + "name": "microsoft_outlook_email_send", + "description": "Send an Outlook email.", + "input_schema": { + "type": "object", + "properties": { + "message_id": { + "$ref": "#/$defs/Message/properties/id" + }, + "attachment": { + "type": "array", + "items": { + "$ref": "#/$defs/Attachment" + } + } + }, + "$defs": { + "Message": { + "type": "object", + "description": "Outlook message.", + "properties": { + "id": { + "type": "string", + "description": "Message identifier." + }, + "sender": { + "$ref": "#/$defs/EmailAddress" + }, + "importance": { + "type": "string", + "enum": [ + "low", + "normal", + "high" + ], + "description": "Message importance." + } + } + }, + "Attachment": { + "type": "object", + "description": "Email attachment.", + "properties": { + "name": { + "type": "string", + "description": "Attachment file name." + } + } + }, + "EmailAddress": { + "type": "object", + "description": "Email address object.", + "properties": { + "address": { + "type": "string", + "description": "SMTP address." + } + } + }, + "GiantUnusedPayload": { + "type": "object", + "description": "Representative unreachable Outlook payload.", + "properties": { + "opaque": { + "type": "string" + } + } + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/message_id/$ref", + "value": "#/$defs/Message/properties/id" + }, + { + "pointer": "/$defs/Message/properties/importance/enum/2", + "value": "high" + } + ], + "expected_pruned": [ + "/$defs/GiantUnusedPayload" + ], + "expected_dropped_fields": [] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/notion.json b/codex-rs/tools/tests/fixtures/json_schema_policy/notion.json new file mode 100644 index 0000000000..49a362d49e --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/notion.json @@ -0,0 +1,72 @@ +{ + "source": "golden/notion/tools", + "tools": [ + { + "name": "notion_fetch_page", + "description": "Fetch a Notion page.", + "input_schema": { + "type": "object", + "properties": { + "page": { + "$ref": "#/$defs/Page%20Ref" + }, + "filter": { + "type": "object", + "additionalProperties": { + "$ref": "#/$defs/Filter" + } + }, + "content_mode": { + "description": "How much content to return.", + "anyOf": [ + { + "type": "string", + "enum": [ + "summary", + "full" + ] + }, + { + "type": "null" + } + ] + } + }, + "$defs": { + "Page Ref": { + "type": "string", + "description": "Notion page ID or URL." + }, + "Filter": { + "type": "object", + "description": "Filter object.", + "properties": { + "created_by": { + "type": "string", + "description": "Creator user ID." + } + } + }, + "UnusedDatabase": { + "type": "object", + "description": "Unreachable Notion database schema." + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/page/$ref", + "value": "#/$defs/Page%20Ref" + }, + { + "pointer": "/properties/content_mode/anyOf/0/enum/0", + "value": "summary" + } + ], + "expected_pruned": [ + "/$defs/UnusedDatabase" + ], + "expected_dropped_fields": [] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json b/codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json new file mode 100644 index 0000000000..b4957a1830 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json @@ -0,0 +1,1124 @@ +{ + "source": "golden/notion/tools", + "tools": [ + { + "name": "create_page", + "description": "Create a Notion page.", + "input_schema": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "create_page.input", + "type": "object", + "description": "Input schema for create_page.", + "$defs": { + "rich_text": { + "type": "object", + "description": "Notion rich text object. This schema captures the stable high-value fields and permits provider extensions.", + "properties": { + "type": { + "type": "string", + "description": "Rich text discriminator such as `text`, `mention`, or `equation`." + }, + "plain_text": { + "type": "string", + "description": "Flattened plain-text representation." + }, + "href": { + "type": [ + "string", + "null" + ], + "description": "Optional hyperlink target when present." + }, + "annotations": { + "type": "object", + "description": "Text annotations applied to this run.", + "properties": { + "bold": { + "type": "boolean" + }, + "italic": { + "type": "boolean" + }, + "strikethrough": { + "type": "boolean" + }, + "underline": { + "type": "boolean" + }, + "code": { + "type": "boolean" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + }, + "text": { + "type": "object", + "description": "Text payload for `type=text` entries.", + "properties": { + "content": { + "type": "string" + }, + "link": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "required": [ + "url" + ], + "additionalProperties": false + } + ] + } + }, + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "icon": { + "type": "object", + "description": "Notion icon union. Keep provider variants explicit and allow future variants.", + "properties": { + "type": { + "type": "string", + "description": "Icon discriminator such as `emoji`, `external`, `file_upload`, `custom_emoji`, or `icon`." + }, + "emoji": { + "type": "string", + "description": "Emoji character for `type=emoji`." + }, + "name": { + "type": "string", + "description": "Named native icon or custom emoji name when applicable." + }, + "color": { + "type": "string", + "description": "Optional icon color for native icon variants." + }, + "external": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "additionalProperties": true + }, + "file_upload": { + "type": "object", + "properties": { + "id": { + "type": "string" + } + }, + "additionalProperties": true + }, + "custom_emoji": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "url": { + "type": "string", + "format": "uri" + } + }, + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "file_ref": { + "type": "object", + "description": "Notion file union used by cover and attachments.", + "properties": { + "type": { + "type": "string", + "description": "File discriminator such as `external`, `file`, or `file_upload`." + }, + "name": { + "type": "string" + }, + "external": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "required": [ + "url" + ], + "additionalProperties": true + }, + "file": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + }, + "expiry_time": { + "type": "string", + "format": "date-time" + } + }, + "additionalProperties": true + }, + "file_upload": { + "type": "object", + "properties": { + "id": { + "type": "string" + } + }, + "required": [ + "id" + ], + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "parent": { + "type": "object", + "description": "Notion parent reference. The exact allowed variants depend on the endpoint.", + "properties": { + "type": { + "type": "string", + "description": "Parent discriminator such as `page_id`, `data_source_id`, `database_id`, `block_id`, or `workspace`." + }, + "page_id": { + "type": "string" + }, + "data_source_id": { + "type": "string" + }, + "database_id": { + "type": "string" + }, + "block_id": { + "type": "string" + }, + "workspace": { + "type": "boolean" + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "date_value": { + "type": "object", + "properties": { + "start": { + "type": "string", + "description": "ISO 8601 date or datetime string." + }, + "end": { + "type": [ + "string", + "null" + ] + }, + "time_zone": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "start" + ], + "additionalProperties": false + }, + "user": { + "type": "object", + "description": "Notion user or bot object.", + "properties": { + "object": { + "type": "string", + "const": "user" + }, + "id": { + "type": "string" + }, + "name": { + "type": [ + "string", + "null" + ] + }, + "avatar_url": { + "type": [ + "string", + "null" + ], + "format": "uri" + }, + "type": { + "type": "string", + "enum": [ + "person", + "bot" + ] + }, + "person": { + "type": "object", + "properties": { + "email": { + "type": [ + "string", + "null" + ], + "format": "email" + } + }, + "additionalProperties": true + }, + "bot": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "object", + "id", + "type" + ], + "additionalProperties": true + }, + "property_value": { + "type": "object", + "description": "Provider-native page property value object. This schema captures common property families and leaves room for future Notion property variants.", + "properties": { + "type": { + "type": "string" + }, + "title": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "number": { + "type": [ + "number", + "null" + ] + }, + "checkbox": { + "type": "boolean" + }, + "url": { + "type": [ + "string", + "null" + ], + "format": "uri" + }, + "email": { + "type": [ + "string", + "null" + ], + "format": "email" + }, + "phone_number": { + "type": [ + "string", + "null" + ] + }, + "date": { + "anyOf": [ + { + "$ref": "#/$defs/date_value" + }, + { + "type": "null" + } + ] + }, + "select": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + } + ] + }, + "status": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + } + ] + }, + "multi_select": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + } + }, + "people": { + "type": "array", + "items": { + "$ref": "#/$defs/user" + } + }, + "relation": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string" + } + }, + "required": [ + "id" + ], + "additionalProperties": false + } + }, + "files": { + "type": "array", + "items": { + "$ref": "#/$defs/file_ref" + } + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": "string", + "format": "date-time" + }, + "formula": { + "type": "object", + "additionalProperties": true + }, + "rollup": { + "type": "object", + "additionalProperties": true + }, + "verification": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "page": { + "type": "object", + "description": "Notion page object, modeled as a practical schema-authoring subset.", + "properties": { + "object": { + "type": "string", + "const": "page" + }, + "id": { + "type": "string" + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": "string", + "format": "date-time" + }, + "created_by": { + "$ref": "#/$defs/user" + }, + "last_edited_by": { + "$ref": "#/$defs/user" + }, + "cover": { + "anyOf": [ + { + "$ref": "#/$defs/file_ref" + }, + { + "type": "null" + } + ] + }, + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "archived": { + "type": "boolean" + }, + "in_trash": { + "type": "boolean" + }, + "is_locked": { + "type": "boolean" + }, + "url": { + "type": "string", + "format": "uri" + }, + "properties": { + "type": "object", + "description": "Page property map keyed by property name.", + "additionalProperties": { + "$ref": "#/$defs/property_value" + } + } + }, + "required": [ + "object", + "id", + "parent", + "properties" + ], + "additionalProperties": true + }, + "text_block_payload": { + "type": "object", + "properties": { + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "color": { + "type": "string" + }, + "children": { + "type": "array", + "items": { + "$ref": "#/$defs/block" + } + } + }, + "additionalProperties": true + }, + "block": { + "type": "object", + "description": "Notion block object. The schema preserves the top-level discriminator and important typed payload families while allowing future provider expansion.", + "properties": { + "object": { + "type": "string", + "const": "block" + }, + "id": { + "type": "string" + }, + "type": { + "type": "string" + }, + "has_children": { + "type": "boolean" + }, + "archived": { + "type": "boolean" + }, + "in_trash": { + "type": "boolean" + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": "string", + "format": "date-time" + }, + "created_by": { + "$ref": "#/$defs/user" + }, + "last_edited_by": { + "$ref": "#/$defs/user" + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "paragraph": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_1": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_2": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_3": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_4": { + "$ref": "#/$defs/text_block_payload" + }, + "bulleted_list_item": { + "$ref": "#/$defs/text_block_payload" + }, + "numbered_list_item": { + "$ref": "#/$defs/text_block_payload" + }, + "toggle": { + "$ref": "#/$defs/text_block_payload" + }, + "to_do": { + "allOf": [ + { + "$ref": "#/$defs/text_block_payload" + }, + { + "type": "object", + "properties": { + "checked": { + "type": "boolean" + } + }, + "additionalProperties": true + } + ] + }, + "callout": { + "allOf": [ + { + "$ref": "#/$defs/text_block_payload" + }, + { + "type": "object", + "properties": { + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + } + }, + "additionalProperties": true + } + ] + }, + "code": { + "type": "object", + "properties": { + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "caption": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "language": { + "type": "string" + } + }, + "additionalProperties": true + }, + "image": { + "type": "object", + "properties": { + "caption": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "type": { + "type": "string" + }, + "file": { + "$ref": "#/$defs/file_ref" + }, + "external": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "additionalProperties": true + } + }, + "additionalProperties": true + }, + "tab": { + "type": "object", + "additionalProperties": true + }, + "meeting_notes": { + "type": "object", + "additionalProperties": true + }, + "unsupported": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "object", + "id", + "type" + ], + "additionalProperties": true + }, + "property_schema": { + "type": "object", + "description": "Data source property schema entry.", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "description": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + } + ] + }, + "type": { + "type": "string" + }, + "number": { + "type": "object", + "additionalProperties": true + }, + "select": { + "type": "object", + "additionalProperties": true + }, + "multi_select": { + "type": "object", + "additionalProperties": true + }, + "status": { + "type": "object", + "additionalProperties": true + }, + "date": { + "type": "object", + "additionalProperties": true + }, + "people": { + "type": "object", + "additionalProperties": true + }, + "files": { + "type": "object", + "additionalProperties": true + }, + "checkbox": { + "type": "object", + "additionalProperties": true + }, + "url": { + "type": "object", + "additionalProperties": true + }, + "email": { + "type": "object", + "additionalProperties": true + }, + "phone_number": { + "type": "object", + "additionalProperties": true + }, + "formula": { + "type": "object", + "additionalProperties": true + }, + "relation": { + "type": "object", + "additionalProperties": true + }, + "rollup": { + "type": "object", + "additionalProperties": true + }, + "created_time": { + "type": "object", + "additionalProperties": true + }, + "last_edited_time": { + "type": "object", + "additionalProperties": true + }, + "created_by": { + "type": "object", + "additionalProperties": true + }, + "last_edited_by": { + "type": "object", + "additionalProperties": true + }, + "verification": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "data_source": { + "type": "object", + "description": "Notion data source object representing a table under a database.", + "properties": { + "object": { + "type": "string", + "const": "data_source" + }, + "id": { + "type": "string" + }, + "name": { + "type": [ + "string", + "null" + ] + }, + "title": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "url": { + "type": "string", + "format": "uri" + }, + "properties": { + "type": "object", + "description": "Schema map keyed by property name or id.", + "additionalProperties": { + "$ref": "#/$defs/property_schema" + } + }, + "in_trash": { + "type": "boolean" + } + }, + "required": [ + "object", + "id", + "parent" + ], + "additionalProperties": true + }, + "database": { + "type": "object", + "description": "Notion database container object.", + "properties": { + "object": { + "type": "string", + "const": "database" + }, + "id": { + "type": "string" + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "title": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "description": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + }, + "cover": { + "anyOf": [ + { + "$ref": "#/$defs/file_ref" + }, + { + "type": "null" + } + ] + }, + "is_inline": { + "type": "boolean" + }, + "is_locked": { + "type": "boolean" + }, + "in_trash": { + "type": "boolean" + }, + "url": { + "type": "string", + "format": "uri" + }, + "data_sources": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "id" + ], + "additionalProperties": true + } + } + }, + "required": [ + "object", + "id", + "parent" + ], + "additionalProperties": true + }, + "comment": { + "type": "object", + "description": "Notion comment object.", + "properties": { + "object": { + "type": "string", + "const": "comment" + }, + "id": { + "type": "string" + }, + "discussion_id": { + "type": "string" + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": [ + "string", + "null" + ], + "format": "date-time" + }, + "created_by": { + "$ref": "#/$defs/user" + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "markdown": { + "type": "string" + }, + "attachments": { + "type": "array", + "items": { + "$ref": "#/$defs/file_ref" + } + } + }, + "required": [ + "object", + "id", + "discussion_id", + "created_by" + ], + "additionalProperties": true + } + }, + "properties": { + "parent": { + "$ref": "#/$defs/parent", + "description": "Request body field. Parent reference controlling whether the page is standalone or a row under a data source." + }, + "properties": { + "type": "object", + "description": "Request body field. Page property map keyed by property name. Values should follow the target data source schema when the parent is a data source.", + "additionalProperties": { + "$ref": "#/$defs/property_value" + } + }, + "children": { + "type": "array", + "description": "Request body field. Initial child blocks. Prefer this structured provider-native content form over markdown when exact block structure matters.", + "items": { + "$ref": "#/$defs/block" + } + }, + "icon": { + "$ref": "#/$defs/icon", + "description": "Request body field. Optional page icon." + }, + "cover": { + "$ref": "#/$defs/file_ref", + "description": "Request body field. Optional page cover image/file reference." + }, + "template": { + "type": "object", + "description": "Request body field. Optional template application settings. Template application may be asynchronous.", + "additionalProperties": true + }, + "markdown": { + "type": "string", + "description": "Request body field. Optional provider-supported markdown body. Included because Notion supports it, but block children remain the preferred structural primitive in this package." + } + }, + "additionalProperties": false, + "required": [ + "parent", + "properties" + ], + "not": { + "required": [ + "children", + "markdown" + ] + } + } + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/slack.json b/codex-rs/tools/tests/fixtures/json_schema_policy/slack.json new file mode 100644 index 0000000000..7461da73dd --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/slack.json @@ -0,0 +1,75 @@ +{ + "source": "standard/slack/tools", + "tools": [ + { + "name": "slack_schedule_message", + "description": "Schedule a Slack message.", + "input_schema": { + "type": "object", + "properties": { + "channel_id": { + "type": "string", + "description": "Slack channel ID." + }, + "message": { + "type": "string", + "description": "Message text." + }, + "post_at": { + "type": "integer", + "description": "Unix timestamp for delivery." + }, + "thread_ts": { + "description": "Optional parent thread timestamp.", + "anyOf": [ + { + "$ref": "#/$defs/SlackTs" + }, + { + "type": "null" + } + ] + } + }, + "required": [ + "channel_id", + "message", + "post_at" + ], + "additionalProperties": false, + "$defs": { + "SlackTs": { + "type": "string", + "description": "Slack timestamp string.", + "pattern": "^[0-9]+[.][0-9]+$" + }, + "UnusedPayload": { + "type": "object", + "description": "Large unreachable Slack payload.", + "properties": { + "debug": { + "type": "string" + } + } + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/thread_ts/anyOf/0/$ref", + "value": "#/$defs/SlackTs" + }, + { + "pointer": "/$defs/SlackTs/description", + "value": "Slack timestamp string." + } + ], + "expected_pruned": [ + "/$defs/UnusedPayload" + ], + "expected_dropped_fields": [ + "/$defs/SlackTs/pattern" + ] + } + ] +} diff --git a/codex-rs/tools/tests/json_schema_policy_fixtures.rs b/codex-rs/tools/tests/json_schema_policy_fixtures.rs new file mode 100644 index 0000000000..1e244ade1e --- /dev/null +++ b/codex-rs/tools/tests/json_schema_policy_fixtures.rs @@ -0,0 +1,224 @@ +use codex_tools::ToolName; +use codex_tools::mcp_tool_to_responses_api_tool; +use pretty_assertions::assert_eq; +use serde::Deserialize; +use serde::de::DeserializeOwned; +use serde_json::Value; +use serde_json::json; +use std::fs; +use std::sync::Arc; + +const FIXTURE_PATHS: [&str; 5] = [ + "tests/fixtures/json_schema_policy/slack.json", + "tests/fixtures/json_schema_policy/google_calendar.json", + "tests/fixtures/json_schema_policy/google_drive.json", + "tests/fixtures/json_schema_policy/notion.json", + "tests/fixtures/json_schema_policy/microsoft_outlook_email.json", +]; +const OVERSIZED_NOTION_CREATE_PAGE_SCHEMA_PATH: &str = + "tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json"; + +#[derive(Debug, Deserialize)] +struct FixtureFile { + source: String, + tools: Vec, +} + +#[derive(Debug, Deserialize)] +struct FixtureTool { + name: String, + description: String, + input_schema: Value, + #[serde(default)] + expected_preserved: Vec, + #[serde(default)] + expected_pruned: Vec, + #[serde(default)] + expected_dropped_fields: Vec, +} + +#[derive(Debug, Deserialize)] +struct ExpectedValue { + pointer: String, + value: Value, +} + +#[test] +fn json_schema_policy_fixtures_convert_to_responses_tools() { + for fixture in FIXTURE_PATHS.into_iter().map(load_fixture::) { + for fixture_tool in &fixture.tools { + let responses_tool = convert_fixture_tool(&fixture, fixture_tool); + let parameters = serde_json::to_value(&responses_tool.parameters) + .expect("responses parameters should serialize"); + + let expected_fields = [ + ( + "preserve the tool name", + json!(fixture_tool.name), + json!(responses_tool.name), + ), + ( + "preserve the tool description", + json!(fixture_tool.description), + json!(responses_tool.description), + ), + ( + "remain a strict:false tool", + json!(false), + json!(responses_tool.strict), + ), + ( + "produce object-shaped parameters", + json!("object"), + parameters.get("type").cloned().unwrap_or(Value::Null), + ), + ]; + + for (message, expected, actual) in expected_fields { + assert_eq!(actual, expected, "{} should {message}", fixture_tool.name); + } + assert!( + parameters.get("properties").is_some_and(Value::is_object), + "{} should produce a parameters.properties object", + fixture_tool.name + ); + + for expected in &fixture_tool.expected_preserved { + assert_eq!( + parameters.pointer(&expected.pointer), + Some(&expected.value), + "{} should preserve {}", + fixture_tool.name, + expected.pointer + ); + } + + for pointer in &fixture_tool.expected_pruned { + assert!( + parameters.pointer(pointer).is_none(), + "{} should prune unreachable definition {pointer}", + fixture_tool.name + ); + } + + for pointer in &fixture_tool.expected_dropped_fields { + assert!( + fixture_tool.input_schema.pointer(pointer).is_some(), + "{} fixture should contain expected dropped field {pointer}", + fixture_tool.name + ); + assert!( + parameters.pointer(pointer).is_none(), + "{} should drop field {pointer} after JsonSchema conversion", + fixture_tool.name + ); + } + } + } +} + +#[test] +fn json_schema_policy_oversized_golden_schema_triggers_compaction() { + let fixture: FixtureFile = load_fixture(OVERSIZED_NOTION_CREATE_PAGE_SCHEMA_PATH); + let fixture_tool = fixture + .tools + .first() + .expect("oversized fixture should contain a tool"); + let input_bytes = compact_json_len(&fixture_tool.input_schema); + + let responses_tool = convert_fixture_tool(&fixture, fixture_tool); + let parameters = + serde_json::to_value(&responses_tool.parameters).expect("responses parameters serialize"); + let output_bytes = compact_json_len(¶meters); + + assert!( + output_bytes < input_bytes, + "compaction should reduce schema size from {input_bytes} bytes" + ); + + let absent_pointers = [ + ("/description", "drop root description"), + ("/properties/parent/description", "drop nested descriptions"), + ( + "/$defs", + "drop root definitions after stripping descriptions is insufficient", + ), + ]; + for (pointer, message) in absent_pointers { + assert!( + parameters.pointer(pointer).is_none(), + "oversized schema should {message}" + ); + } + + let expected_values = [ + ( + "/properties/parent", + json!({}), + "rewrite local refs before dropping root definitions", + ), + ( + "/properties/children/items", + json!({}), + "rewrite nested local refs before dropping root definitions", + ), + ( + "/properties/markdown/type", + json!("string"), + "retain top-level argument shape", + ), + ( + "/properties/properties/type", + json!("object"), + "retain object argument shape", + ), + ]; + for (pointer, expected, message) in expected_values { + assert_eq!( + parameters.pointer(pointer), + Some(&expected), + "oversized schema should {message}" + ); + } +} + +fn load_fixture(path: &str) -> T { + let path = codex_utils_cargo_bin::find_resource!(path) + .unwrap_or_else(|err| panic!("resolve fixture {path}: {err}")); + let fixture = fs::read_to_string(&path) + .unwrap_or_else(|err| panic!("read fixture {}: {err}", path.display())); + serde_json::from_str(&fixture) + .unwrap_or_else(|err| panic!("parse fixture {}: {err}", path.display())) +} + +fn convert_fixture_tool( + fixture: &FixtureFile, + fixture_tool: &FixtureTool, +) -> codex_tools::ResponsesApiTool { + let name = &fixture_tool.name; + let input_schema = fixture_tool + .input_schema + .as_object() + .unwrap_or_else(|| panic!("{name} input_schema should be an object")) + .clone(); + let tool = rmcp::model::Tool { + name: name.to_string().into(), + title: None, + description: Some(fixture_tool.description.clone().into()), + input_schema: Arc::new(input_schema), + output_schema: None, + annotations: None, + execution: None, + icons: None, + meta: None, + }; + + mcp_tool_to_responses_api_tool(&ToolName::namespaced(&fixture.source, name), &tool) + .unwrap_or_else(|err| panic!("convert {name} from {}: {err}", fixture.source)) +} + +fn compact_json_len(value: &Value) -> usize { + serde_json::to_vec(value) + .unwrap_or_else(|err| panic!("serialize compact JSON: {err}")) + .len() +} From 6ad3a8350902f5825fe4172ef06cb5b506d10f3d Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 22 May 2026 16:46:25 -0700 Subject: [PATCH 4/9] [codex] Remove external client session reset plumbing (#24157) ## Why The turn loop no longer needs to decide when a `ModelClientSession` should reset its websocket state after compaction. That reset behavior belongs inside the model client, where the websocket cache and retry state are owned. The repo guidance now calls this out explicitly so future changes let the incremental request logic decide whether the previous request can be reused. ## What Changed - Removed the `reset_client_session` return value from pre-sampling and auto-compact helpers in `core/src/session/turn.rs`. - Changed compaction helpers to return `CodexResult<()>` so callers only handle success or failure. - Made `ModelClientSession::reset_websocket_session` private to `core/src/client.rs`, leaving it callable only from model-client internals. - Added `AGENTS.md` guidance not to call `reset_client_session` unnecessarily. ## Validation - `just test -p codex-core session::turn` --- AGENTS.md | 1 + codex-rs/core/src/client.rs | 2 +- codex-rs/core/src/session/turn.rs | 101 +++++++++++------------------- 3 files changed, 40 insertions(+), 64 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9906d3039a..4714b1b8aa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,6 +30,7 @@ In the codex-rs folder where the rust code lives: - Prefer private modules and explicitly exported public crate API. - If you change `ConfigToml` or nested config types, run `just write-config-schema` to update `codex-rs/core/config.schema.json`. - When working with MCP tool calls, prefer using `codex-rs/codex-mcp/src/mcp_connection_manager.rs` to handle mutation of tools and tool calls. Aim to minimize the footprint of changes and leverage existing abstractions rather than plumbing code through multiple levels of function calls. +- Do not call `reset_client_session` unnecessarily; let the incremental check logic decide whether to reuse the previous request. - If you change Rust dependencies (`Cargo.toml` or `Cargo.lock`), run `just bazel-lock-update` from the repo root to refresh `MODULE.bazel.lock`, and include that lockfile update in the same change. - After dependency changes, run `just bazel-lock-check` from the repo root so lockfile drift is caught diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 07a1c2adf5..d84b32a8b4 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -938,7 +938,7 @@ impl Drop for ModelClientSession { } impl ModelClientSession { - pub(crate) fn reset_websocket_session(&mut self) { + fn reset_websocket_session(&mut self) { self.websocket_session.connection = None; self.websocket_session.last_request = None; self.websocket_session.last_response_rx = None; diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index f81b2b1f74..5a2ed71dda 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -142,25 +142,18 @@ pub(crate) async fn run_turn( // new user message are recorded. Estimate pending incoming items (context // diffs/full reinjection + user input) and trigger compaction preemptively // when they would push the thread over the compaction threshold. - let pre_sampling_compact = - match run_pre_sampling_compact(&sess, &turn_context, &mut client_session).await { - Ok(pre_sampling_compact) => pre_sampling_compact, - Err(err) => { - if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded - && let Err(err) = sess - .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { - turn_context: turn_context.as_ref(), - }) - .await - { - warn!("failed to usage-limit active goal after usage-limit error: {err}"); - } - error!("Failed to run pre-sampling compact"); - return None; - } - }; - if pre_sampling_compact.reset_client_session { - client_session.reset_websocket_session(); + if let Err(err) = run_pre_sampling_compact(&sess, &turn_context, &mut client_session).await { + if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded + && let Err(err) = sess + .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { + turn_context: turn_context.as_ref(), + }) + .await + { + warn!("failed to usage-limit active goal after usage-limit error: {err}"); + } + error!("Failed to run pre-sampling compact"); + return None; } sess.record_context_updates_and_set_reference_context_item(turn_context.as_ref()) @@ -287,7 +280,7 @@ pub(crate) async fn run_turn( // as long as compaction works well in getting us way below the token limit, we shouldn't worry about being in an infinite loop. if token_limit_reached && needs_follow_up { - let reset_client_session = match run_auto_compact( + if let Err(err) = run_auto_compact( &sess, &turn_context, &mut client_session, @@ -297,24 +290,18 @@ pub(crate) async fn run_turn( ) .await { - Ok(reset_client_session) => reset_client_session, - Err(err) => { - if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded - && let Err(err) = sess - .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { - turn_context: turn_context.as_ref(), - }) - .await - { - warn!( - "failed to usage-limit active goal after usage-limit error: {err}" - ); - } - return None; + if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded + && let Err(err) = sess + .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { + turn_context: turn_context.as_ref(), + }) + .await + { + warn!( + "failed to usage-limit active goal after usage-limit error: {err}" + ); } - }; - if reset_client_session { - client_session.reset_websocket_session(); + return None; } can_drain_pending_input = !model_needs_follow_up; continue; @@ -636,10 +623,6 @@ async fn track_turn_resolved_config_analytics( }); } -struct PreSamplingCompactResult { - reset_client_session: bool, -} - #[derive(Debug)] struct AutoCompactTokenStatus { // Full active context usage, independent of the configured auto-compact scope. @@ -710,14 +693,12 @@ async fn run_pre_sampling_compact( sess: &Arc, turn_context: &Arc, client_session: &mut ModelClientSession, -) -> CodexResult { - let mut pre_sampling_compacted = - maybe_run_previous_model_inline_compact(sess, turn_context, client_session).await?; - let mut reset_client_session = pre_sampling_compacted; +) -> CodexResult<()> { + maybe_run_previous_model_inline_compact(sess, turn_context, client_session).await?; let token_status = auto_compact_token_status(sess.as_ref(), turn_context.as_ref()).await; // Compact if the configured auto-compaction budget or usable context window is exhausted. if token_status.token_limit_reached { - reset_client_session |= run_auto_compact( + run_auto_compact( sess, turn_context, client_session, @@ -726,26 +707,21 @@ async fn run_pre_sampling_compact( CompactionPhase::PreTurn, ) .await?; - pre_sampling_compacted = true; } - Ok(PreSamplingCompactResult { - reset_client_session: pre_sampling_compacted && reset_client_session, - }) + Ok(()) } /// Runs pre-sampling compaction against the previous model when switching to a smaller /// context-window model. /// -/// Returns `Ok(true)` when compaction ran successfully, `Ok(false)` when compaction was skipped -/// because the model/context-window preconditions were not met, and `Err(_)` only when compaction -/// was attempted and failed. +/// Returns `Err(_)` only when compaction was attempted and failed. async fn maybe_run_previous_model_inline_compact( sess: &Arc, turn_context: &Arc, client_session: &mut ModelClientSession, -) -> CodexResult { +) -> CodexResult<()> { let Some(previous_turn_settings) = sess.previous_turn_settings().await else { - return Ok(false); + return Ok(()); }; let previous_model_turn_context = Arc::new( turn_context @@ -754,10 +730,10 @@ async fn maybe_run_previous_model_inline_compact( ); let Some(old_context_window) = previous_model_turn_context.model_context_window() else { - return Ok(false); + return Ok(()); }; let Some(new_context_window) = turn_context.model_context_window() else { - return Ok(false); + return Ok(()); }; let active_context_tokens = sess.get_total_token_usage().await; let previous_model_limit_reached = match turn_context @@ -778,7 +754,7 @@ async fn maybe_run_previous_model_inline_compact( && previous_model_turn_context.model_info.slug != turn_context.model_info.slug && old_context_window > new_context_window; if should_run { - let _ = run_auto_compact( + run_auto_compact( sess, &previous_model_turn_context, client_session, @@ -787,9 +763,8 @@ async fn maybe_run_previous_model_inline_compact( CompactionPhase::PreTurn, ) .await?; - return Ok(true); } - Ok(false) + Ok(()) } async fn run_auto_compact( @@ -799,7 +774,7 @@ async fn run_auto_compact( initial_context_injection: InitialContextInjection, reason: CompactionReason, phase: CompactionPhase, -) -> CodexResult { +) -> CodexResult<()> { if should_use_remote_compact_task(turn_context.provider.info()) { if turn_context.features.enabled(Feature::RemoteCompactionV2) { run_inline_remote_auto_compact_task_v2( @@ -811,7 +786,7 @@ async fn run_auto_compact( phase, ) .await?; - return Ok(false); + return Ok(()); } run_inline_remote_auto_compact_task( Arc::clone(sess), @@ -831,7 +806,7 @@ async fn run_auto_compact( ) .await?; } - Ok(true) + Ok(()) } pub(super) fn collect_explicit_app_ids_from_skill_items( From 4bcabbfbec668de26bdb2b1c39f4748c419962ff Mon Sep 17 00:00:00 2001 From: dhruvgupta-oai Date: Fri, 22 May 2026 19:58:49 -0400 Subject: [PATCH 5/9] Display workspace usage limit error copy from response header (#24114) ## Why `openai/openai#947613` adds `X-Codex-Rate-Limit-Reached-Type` for Codex workspace credit-depletion and spend-cap responses. The CLI currently reads the adjacent promo header but otherwise renders generic usage-limit copy, so those responses do not explain the workspace-specific action the user needs to take. Backend dependency: https://github.com/openai/openai/pull/947613 ## What Changed - Parse `X-Codex-Rate-Limit-Reached-Type` in the usage-limit error handling path alongside `x-codex-promo-message`. - Keep the header value parsing with the shared `RateLimitReachedType` enum. - Carry the parsed type on `UsageLimitReachedError` and render client-owned copy for the four workspace owner/member credit and spend-cap values. - Preserve existing promo and plan-based text for absent, generic, or unknown header values. - Keep the existing TUI workspace-owner nudge state path unchanged; the response header only selects the displayed error string. - Add focused display coverage for all specific type values and the generic fallback case. ## Test Plan - Added `usage_limit_reached_error_formats_rate_limit_reached_types` coverage. - Not run manually, per request; CI runs validation on the pushed commit. --- codex-rs/codex-api/src/api_bridge.rs | 4 ++ codex-rs/codex-api/src/api_bridge_tests.rs | 31 +++++++++++++ codex-rs/codex-api/src/rate_limits.rs | 8 ++++ codex-rs/protocol/src/error.rs | 34 ++++++++++++++ codex-rs/protocol/src/error_tests.rs | 54 ++++++++++++++++++++++ codex-rs/protocol/src/protocol.rs | 15 ++++++ 6 files changed, 146 insertions(+) diff --git a/codex-rs/codex-api/src/api_bridge.rs b/codex-rs/codex-api/src/api_bridge.rs index 401dfab3a9..1c34d8bbf2 100644 --- a/codex-rs/codex-api/src/api_bridge.rs +++ b/codex-rs/codex-api/src/api_bridge.rs @@ -2,6 +2,7 @@ use crate::TransportError; use crate::error::ApiError; use crate::rate_limits::parse_promo_message; use crate::rate_limits::parse_rate_limit_for_limit; +use crate::rate_limits::parse_rate_limit_reached_type; use base64::Engine; use chrono::DateTime; use chrono::Utc; @@ -85,6 +86,8 @@ pub fn map_api_error(err: ApiError) -> CodexErr { parse_rate_limit_for_limit(map, limit_id.as_deref()) }); let promo_message = headers.as_ref().and_then(parse_promo_message); + let rate_limit_reached_type = + headers.as_ref().and_then(parse_rate_limit_reached_type); let resets_at = err .error .resets_at @@ -94,6 +97,7 @@ pub fn map_api_error(err: ApiError) -> CodexErr { resets_at, rate_limits: rate_limits.map(Box::new), promo_message, + rate_limit_reached_type, }); } else if err.error.error_type.as_deref() == Some("usage_not_included") { return CodexErr::UsageNotIncluded; diff --git a/codex-rs/codex-api/src/api_bridge_tests.rs b/codex-rs/codex-api/src/api_bridge_tests.rs index af7e34a649..101e5566fe 100644 --- a/codex-rs/codex-api/src/api_bridge_tests.rs +++ b/codex-rs/codex-api/src/api_bridge_tests.rs @@ -194,6 +194,37 @@ fn map_api_error_does_not_fallback_limit_name_to_limit_id() { ); } +#[test] +fn map_api_error_ignores_unparseable_rate_limit_reached_type_headers() { + let values = [ + http::HeaderValue::from_static("future_rate_limit_reached_type"), + http::HeaderValue::from_bytes(&[0xff]).expect("valid opaque header value"), + ]; + + for value in values { + let mut headers = HeaderMap::new(); + headers.insert("x-codex-rate-limit-reached-type", value); + let body = serde_json::json!({ + "error": { + "type": "usage_limit_reached", + "plan_type": "pro", + } + }) + .to_string(); + let err = map_api_error(ApiError::Transport(TransportError::Http { + status: http::StatusCode::TOO_MANY_REQUESTS, + url: Some("http://example.com/v1/responses".to_string()), + headers: Some(headers), + body: Some(body), + })); + + let CodexErr::UsageLimitReached(usage_limit) = err else { + panic!("expected CodexErr::UsageLimitReached, got {err:?}"); + }; + assert_eq!(usage_limit.rate_limit_reached_type, None); + } +} + #[test] fn map_api_error_extracts_identity_auth_details_from_headers() { let mut headers = HeaderMap::new(); diff --git a/codex-rs/codex-api/src/rate_limits.rs b/codex-rs/codex-api/src/rate_limits.rs index 979500cdab..a2ad876671 100644 --- a/codex-rs/codex-api/src/rate_limits.rs +++ b/codex-rs/codex-api/src/rate_limits.rs @@ -1,5 +1,6 @@ use codex_protocol::account::PlanType; use codex_protocol::protocol::CreditsSnapshot; +use codex_protocol::protocol::RateLimitReachedType; use codex_protocol::protocol::RateLimitSnapshot; use codex_protocol::protocol::RateLimitWindow; use http::HeaderMap; @@ -178,6 +179,13 @@ pub fn parse_promo_message(headers: &HeaderMap) -> Option { .map(std::string::ToString::to_string) } +pub(crate) fn parse_rate_limit_reached_type(headers: &HeaderMap) -> Option { + parse_header_str(headers, "x-codex-rate-limit-reached-type")? + .trim() + .parse() + .ok() +} + fn parse_rate_limit_window( headers: &HeaderMap, used_percent_header: &str, diff --git a/codex-rs/protocol/src/error.rs b/codex-rs/protocol/src/error.rs index ef9c86cadf..d7e953af0b 100644 --- a/codex-rs/protocol/src/error.rs +++ b/codex-rs/protocol/src/error.rs @@ -7,6 +7,7 @@ use crate::exec_output::ExecToolCallOutput; use crate::network_policy::NetworkPolicyDecisionPayload; use crate::protocol::CodexErrorInfo; use crate::protocol::ErrorEvent; +use crate::protocol::RateLimitReachedType; use crate::protocol::RateLimitSnapshot; use crate::protocol::TruncationPolicy; use chrono::DateTime; @@ -451,6 +452,7 @@ pub struct UsageLimitReachedError { pub resets_at: Option>, pub rate_limits: Option>, pub promo_message: Option, + pub rate_limit_reached_type: Option, } impl std::fmt::Display for UsageLimitReachedError { @@ -470,6 +472,38 @@ impl std::fmt::Display for UsageLimitReachedError { ); } + if let Some(rate_limit_reached_type) = self.rate_limit_reached_type { + match rate_limit_reached_type { + RateLimitReachedType::WorkspaceOwnerCreditsDepleted => { + return write!( + f, + "Your workspace is out of credits. Add credits to continue." + ); + } + RateLimitReachedType::WorkspaceMemberCreditsDepleted => { + return write!( + f, + "Your workspace is out of credits. Ask your workspace owner to refill in order to continue." + ); + } + RateLimitReachedType::WorkspaceOwnerUsageLimitReached => { + return write!( + f, + "You hit your spend cap set in your workspace. Increase your spend cap to continue." + ); + } + RateLimitReachedType::WorkspaceMemberUsageLimitReached => { + return write!( + f, + "You hit your spend cap set by the owner of your workspace. Ask an owner to increase your spend cap to continue." + ); + } + RateLimitReachedType::RateLimitReached => { + // Generic limits intentionally use the existing promo or plan copy below. + } + } + } + if let Some(promo_message) = &self.promo_message { return write!( f, diff --git a/codex-rs/protocol/src/error_tests.rs b/codex-rs/protocol/src/error_tests.rs index aef7478607..11bd26133b 100644 --- a/codex-rs/protocol/src/error_tests.rs +++ b/codex-rs/protocol/src/error_tests.rs @@ -56,6 +56,7 @@ fn usage_limit_reached_error_formats_plus_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -63,6 +64,44 @@ fn usage_limit_reached_error_formats_plus_plan() { ); } +#[test] +fn usage_limit_reached_error_formats_rate_limit_reached_types() { + let cases = [ + ( + RateLimitReachedType::RateLimitReached, + "You've hit your usage limit. Upgrade to Pro (https://chatgpt.com/explore/pro), visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again later.", + ), + ( + RateLimitReachedType::WorkspaceOwnerCreditsDepleted, + "Your workspace is out of credits. Add credits to continue.", + ), + ( + RateLimitReachedType::WorkspaceMemberCreditsDepleted, + "Your workspace is out of credits. Ask your workspace owner to refill in order to continue.", + ), + ( + RateLimitReachedType::WorkspaceOwnerUsageLimitReached, + "You hit your spend cap set in your workspace. Increase your spend cap to continue.", + ), + ( + RateLimitReachedType::WorkspaceMemberUsageLimitReached, + "You hit your spend cap set by the owner of your workspace. Ask an owner to increase your spend cap to continue.", + ), + ]; + + for (rate_limit_reached_type, expected) in cases { + let err = UsageLimitReachedError { + plan_type: Some(PlanType::Known(KnownPlan::Plus)), + resets_at: None, + rate_limits: Some(Box::new(rate_limit_snapshot())), + promo_message: None, + rate_limit_reached_type: Some(rate_limit_reached_type), + }; + + assert_eq!(err.to_string(), expected); + } +} + #[test] fn server_overloaded_maps_to_protocol() { let err = CodexErr::ServerOverloaded; @@ -177,6 +216,7 @@ fn usage_limit_reached_error_formats_free_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -191,6 +231,7 @@ fn usage_limit_reached_error_formats_go_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -205,6 +246,7 @@ fn usage_limit_reached_error_formats_default_when_none() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -223,6 +265,7 @@ fn usage_limit_reached_error_formats_team_plan() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. To get more access now, send a request to your admin or try again at {expected_time}." @@ -238,6 +281,7 @@ fn usage_limit_reached_error_formats_business_plan_without_reset() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -252,6 +296,7 @@ fn usage_limit_reached_error_formats_self_serve_business_usage_based_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -266,6 +311,7 @@ fn usage_limit_reached_error_formats_enterprise_cbp_usage_based_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -280,6 +326,7 @@ fn usage_limit_reached_error_formats_default_for_other_plans() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -298,6 +345,7 @@ fn usage_limit_reached_error_formats_pro_plan_with_reset() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at {expected_time}." @@ -324,6 +372,7 @@ fn usage_limit_reached_error_hides_upsell_for_non_codex_limit_name() { "Visit https://chatgpt.com/codex/settings/usage to purchase more credits" .to_string(), ), + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit for codex_other. Switch to another model now, or try again at {expected_time}." @@ -343,6 +392,7 @@ fn usage_limit_reached_includes_minutes_when_available() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!("You've hit your usage limit. Try again at {expected_time}."); assert_eq!(err.to_string(), expected); @@ -482,6 +532,7 @@ fn usage_limit_reached_includes_hours_and_minutes() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. Upgrade to Pro (https://chatgpt.com/explore/pro), visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at {expected_time}." @@ -502,6 +553,7 @@ fn usage_limit_reached_includes_days_hours_minutes() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!("You've hit your usage limit. Try again at {expected_time}."); assert_eq!(err.to_string(), expected); @@ -519,6 +571,7 @@ fn usage_limit_reached_less_than_minute() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!("You've hit your usage limit. Try again at {expected_time}."); assert_eq!(err.to_string(), expected); @@ -538,6 +591,7 @@ fn usage_limit_reached_with_promo_message() { promo_message: Some( "To continue using Codex, start a free trial of today".to_string(), ), + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. To continue using Codex, start a free trial of today, or try again at {expected_time}." diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 7d955f1d1f..d3b8a9e820 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -2000,6 +2000,21 @@ pub enum RateLimitReachedType { WorkspaceMemberUsageLimitReached, } +impl FromStr for RateLimitReachedType { + type Err = String; + + fn from_str(value: &str) -> Result { + match value { + "rate_limit_reached" => Ok(Self::RateLimitReached), + "workspace_owner_credits_depleted" => Ok(Self::WorkspaceOwnerCreditsDepleted), + "workspace_member_credits_depleted" => Ok(Self::WorkspaceMemberCreditsDepleted), + "workspace_owner_usage_limit_reached" => Ok(Self::WorkspaceOwnerUsageLimitReached), + "workspace_member_usage_limit_reached" => Ok(Self::WorkspaceMemberUsageLimitReached), + other => Err(format!("unknown rate limit reached type: {other}")), + } + } +} + #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, TS)] pub struct RateLimitWindow { /// Percentage (0-100) of the window that has been consumed. From ed47f1ab1eba595df81e7ce62568b83328d9c42a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 22 May 2026 17:08:59 -0700 Subject: [PATCH 6/9] release: build macOS x64 zsh artifact (#24165) ## Why The zsh release workflow currently publishes macOS arm64 and Linux zsh fork artifacts, but no macOS x64 artifact. The Codex package builder therefore cannot include codex-resources/zsh/bin/zsh for x86_64-apple-darwin packages. ## What Changed - Added an x86_64-apple-darwin row to the macOS zsh release matrix. - Runs that row on macos-15-large, the Intel macOS runner appropriate for the native zsh build. - Added the matching macos-x86_64 platform to the zsh DotSlash publish config so the generated release manifest can reference the new tarball. --- .github/dotslash-zsh-config.json | 5 +++++ .github/workflows/rust-release-zsh.yml | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/.github/dotslash-zsh-config.json b/.github/dotslash-zsh-config.json index db2c416401..37285f19e2 100644 --- a/.github/dotslash-zsh-config.json +++ b/.github/dotslash-zsh-config.json @@ -7,6 +7,11 @@ "format": "tar.gz", "path": "codex-zsh/bin/zsh" }, + "macos-x86_64": { + "name": "codex-zsh-x86_64-apple-darwin.tar.gz", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh" + }, "linux-x86_64": { "name": "codex-zsh-x86_64-unknown-linux-musl.tar.gz", "format": "tar.gz", diff --git a/.github/workflows/rust-release-zsh.yml b/.github/workflows/rust-release-zsh.yml index 492b8dc5e7..b55d2e714b 100644 --- a/.github/workflows/rust-release-zsh.yml +++ b/.github/workflows/rust-release-zsh.yml @@ -69,6 +69,10 @@ jobs: fail-fast: false matrix: include: + - runner: macos-15-large + target: x86_64-apple-darwin + variant: macos-15 + archive_name: codex-zsh-x86_64-apple-darwin.tar.gz - runner: macos-15-xlarge target: aarch64-apple-darwin variant: macos-15 From 03e6c5f60048b81f443f3573b1578320c2189fbc Mon Sep 17 00:00:00 2001 From: Anton Panasenko Date: Fri, 22 May 2026 17:38:22 -0700 Subject: [PATCH 7/9] fix(remote-control): cap reconnect backoff (#24164) ## Why Remote-control websocket reconnects currently use the shared exponential backoff helper without a local ceiling, so a long failure streak can stretch retries out indefinitely and leave the runtime behavior hard to inspect from logs. ## What Changed Cap the remote-control reconnect delay at 30 seconds, then reset the reconnect attempt counter once that capped delay is emitted so the next failure starts from the initial jittered delay again. The reconnect failure log now records the attempt number, chosen delay, and whether the cap triggered a reset, with a separate info log when the backoff counter is reset after the cap. ## Verification `just test -p codex-app-server-transport` Related issue: N/A --- .../src/transport/remote_control/websocket.rs | 52 +++++++++++++++++-- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs b/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs index 74ef9c2301..1e696c2e46 100644 --- a/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs +++ b/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs @@ -64,6 +64,8 @@ const REMOTE_CONTROL_WEBSOCKET_PONG_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(60); const REMOTE_CONTROL_ACCOUNT_ID_RETRY_INTERVAL: std::time::Duration = std::time::Duration::from_secs(1); +const REMOTE_CONTROL_RECONNECT_BACKOFF_CAP: std::time::Duration = + std::time::Duration::from_secs(30); struct BoundedOutboundBuffer { buffer_by_stream: HashMap<(ClientId, StreamId), VecDeque>, @@ -514,12 +516,23 @@ impl RemoteControlWebsocket { } else { self.status_publisher .publish_status(RemoteControlConnectionStatus::Errored); + let reconnect_attempt = self.reconnect_attempt.saturating_add(1); + let (reconnect_delay, reconnect_backoff_reset) = + next_reconnect_delay(&mut self.reconnect_attempt); warn!( - "failed to connect to app-server remote control websocket: {}, err: {}", - remote_control_target.websocket_url, err + websocket_url = %remote_control_target.websocket_url, + error = %err, + reconnect_attempt, + reconnect_delay = ?reconnect_delay, + reconnect_backoff_reset, + "failed to connect to app-server remote control websocket" ); - let reconnect_delay = backoff(self.reconnect_attempt); - self.reconnect_attempt += 1; + if reconnect_backoff_reset { + info!( + reconnect_backoff_cap = ?REMOTE_CONTROL_RECONNECT_BACKOFF_CAP, + "reset app-server remote control websocket reconnect backoff after cap" + ); + } reconnect_delay }; tokio::select! { @@ -1036,6 +1049,17 @@ pub(crate) async fn load_remote_control_auth( }) } +fn next_reconnect_delay(reconnect_attempt: &mut u64) -> (std::time::Duration, bool) { + let reconnect_delay = backoff(*reconnect_attempt).min(REMOTE_CONTROL_RECONNECT_BACKOFF_CAP); + let reconnect_backoff_reset = reconnect_delay == REMOTE_CONTROL_RECONNECT_BACKOFF_CAP; + *reconnect_attempt = if reconnect_backoff_reset { + 0 + } else { + (*reconnect_attempt).saturating_add(1) + }; + (reconnect_delay, reconnect_backoff_reset) +} + pub(super) async fn connect_remote_control_websocket( remote_control_target: &RemoteControlTarget, state_db: Option<&StateRuntime>, @@ -1326,6 +1350,26 @@ mod tests { const TEST_HTTP_ACCEPT_TIMEOUT: Duration = Duration::from_secs(5); const TEST_INSTALLATION_ID: &str = "11111111-1111-4111-8111-111111111111"; + #[test] + fn next_reconnect_delay_resets_after_cap() { + let mut reconnect_attempt = 9; + + let (reconnect_delay, reconnect_backoff_reset) = + next_reconnect_delay(&mut reconnect_attempt); + + assert_eq!(reconnect_delay, REMOTE_CONTROL_RECONNECT_BACKOFF_CAP); + assert!(reconnect_backoff_reset); + assert_eq!(reconnect_attempt, 0); + + let (reconnect_delay, reconnect_backoff_reset) = + next_reconnect_delay(&mut reconnect_attempt); + + assert!(reconnect_delay >= Duration::from_millis(180)); + assert!(reconnect_delay <= Duration::from_millis(220)); + assert!(!reconnect_backoff_reset); + assert_eq!(reconnect_attempt, 1); + } + fn remote_control_status_channel() -> ( RemoteControlStatusPublisher, watch::Receiver, From c7bcb90f9b705f556ed0fb1bde13d836cbf71924 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 22 May 2026 17:54:07 -0700 Subject: [PATCH 8/9] package: include zsh fork in Codex package (#23756) ## Why The package layout gives Codex a stable place for runtime helpers that should travel with the entrypoint. `shell_zsh_fork` still required users to configure `zsh_path` manually, even though we already publish prebuilt zsh fork artifacts. This PR builds on #24129 and uses the shared DotSlash artifact fetcher to include the zsh fork in Codex packages when a matching target artifact exists. Packaged Codex builds can then discover the bundled fork automatically; the user/profile `zsh_path` override is removed so the feature uses the package-managed artifact instead of a legacy path knob. ## What Changed - Added `scripts/codex_package/codex-zsh`, a checked-in DotSlash manifest for the current macOS arm64 and Linux zsh fork artifacts. - Taught `scripts/build_codex_package.py` to fetch the matching zsh fork artifact and install it at `codex-resources/zsh/bin/zsh` when available for the selected target. - Added package layout validation for the optional bundled zsh resource. - Added `InstallContext::bundled_zsh_path()` and `InstallContext::bundled_zsh_bin_dir()` for package-layout resource discovery. - Threaded the packaged zsh path through config loading as the runtime `zsh_path` for packaged installs, and removed the config/profile/CLI override path. - Kept the packaged default zsh override typed as `AbsolutePathBuf` until the existing runtime `Config::zsh_path` boundary. - Updated app-server zsh-fork integration tests to spawn `codex-app-server` from a temporary package layout with `codex-resources/zsh/bin/zsh`, matching the new packaged discovery path instead of setting `zsh_path` in config. - Switched package executable copying from metadata-preserving `copy2()` to `copyfile()` plus explicit executable bits, which avoids macOS file-flag failures when local smoke tests use system binaries as inputs. ## Testing To verify that the `zsh` executable from the Codex package is picked up correctly, first I ran: ```shell ./scripts/build_codex_package.py ``` which created: ``` /private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/ ``` so then I ran: ``` /private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/bin/codex exec --enable shell_zsh_fork 'run `echo $0`' ``` which reported the following, as expected: ``` /private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/codex-resources/zsh/bin/zsh ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/23756). * #23768 * __->__ #23756 --- codex-rs/Cargo.lock | 1 + .../app-server/tests/common/mcp_process.rs | 23 ++++++ .../tests/suite/v2/turn_start_zsh_fork.rs | 77 +++++++++++++++---- codex-rs/config/src/config_toml.rs | 3 - codex-rs/config/src/profile_toml.rs | 2 - codex-rs/core/Cargo.toml | 1 + codex-rs/core/config.schema.json | 16 ---- codex-rs/core/src/config/config_tests.rs | 19 +++++ codex-rs/core/src/config/mod.rs | 16 +++- codex-rs/core/src/config/permissions_tests.rs | 2 +- codex-rs/core/src/session/session.rs | 4 +- codex-rs/core/src/session/tests.rs | 4 +- codex-rs/exec/src/lib.rs | 2 +- codex-rs/install-context/src/lib.rs | 35 +++++++++ scripts/codex_package/README.md | 7 ++ scripts/codex_package/cli.py | 4 +- scripts/codex_package/codex-zsh | 43 +++++++++++ scripts/codex_package/layout.py | 17 +++- scripts/codex_package/targets.py | 1 + scripts/codex_package/zsh.py | 22 ++++++ 20 files changed, 250 insertions(+), 49 deletions(-) create mode 100755 scripts/codex_package/codex-zsh create mode 100644 scripts/codex_package/zsh.py diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b843fcae6b..a56343628f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2507,6 +2507,7 @@ dependencies = [ "codex-feedback", "codex-git-utils", "codex-hooks", + "codex-install-context", "codex-login", "codex-mcp", "codex-memories-read", diff --git a/codex-rs/app-server/tests/common/mcp_process.rs b/codex-rs/app-server/tests/common/mcp_process.rs index 993ffa35f3..2d2c26b5a6 100644 --- a/codex-rs/app-server/tests/common/mcp_process.rs +++ b/codex-rs/app-server/tests/common/mcp_process.rs @@ -173,6 +173,20 @@ impl McpProcess { .await } + pub async fn new_with_program_and_env( + codex_home: &Path, + program: &Path, + env_overrides: &[(&str, Option<&str>)], + ) -> anyhow::Result { + Self::new_with_program_env_and_args( + codex_home, + program, + env_overrides, + &[DISABLE_PLUGIN_STARTUP_TASKS_ARG], + ) + .await + } + async fn new_with_env_and_args( codex_home: &Path, env_overrides: &[(&str, Option<&str>)], @@ -180,6 +194,15 @@ impl McpProcess { ) -> anyhow::Result { let program = codex_utils_cargo_bin::cargo_bin("codex-app-server") .context("should find binary for codex-app-server")?; + Self::new_with_program_env_and_args(codex_home, &program, env_overrides, args).await + } + + async fn new_with_program_env_and_args( + codex_home: &Path, + program: &Path, + env_overrides: &[(&str, Option<&str>)], + args: &[&str], + ) -> anyhow::Result { let mut cmd = Command::new(program); cmd.stdin(Stdio::piped()); diff --git a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs index 6fe93e36af..7b7a6c2563 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs @@ -37,6 +37,7 @@ use core_test_support::skip_if_no_network; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::path::Path; +use std::path::PathBuf; use tempfile::TempDir; use tokio::time::timeout; @@ -94,10 +95,9 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -162,7 +162,7 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { }; assert_eq!(id, "call-zsh-fork"); assert_eq!(status, CommandExecutionStatus::InProgress); - assert!(command.starts_with(&zsh_path.display().to_string())); + assert!(command.starts_with(&command_packaged_zsh_path(&codex_home).display().to_string())); assert!(command.contains("/bin/sh -c")); assert!(command.contains("sleep 0.01")); assert!(command.contains(&release_marker.display().to_string())); @@ -213,10 +213,9 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -346,10 +345,9 @@ async fn turn_start_shell_zsh_fork_exec_approval_cancel_v2() -> Result<()> { (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -505,10 +503,9 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -603,7 +600,8 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() ); approved_subcommand_strings.push(approval_command.to_string()); } - let is_parent_approval = approval_command.contains(&zsh_path.display().to_string()) + let is_parent_approval = approval_command + .contains(&command_packaged_zsh_path(&codex_home).display().to_string()) && (approval_command.contains(&shell_command) || (has_first_file && has_second_file) || approval_command.contains(&parent_shell_hint)); @@ -738,9 +736,58 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() Ok(()) } -async fn create_zsh_test_mcp_process(codex_home: &Path, zdotdir: &Path) -> Result { +async fn create_zsh_test_mcp_process( + codex_home: &Path, + zdotdir: &Path, + zsh_path: &Path, +) -> Result { + let app_server = create_test_package_app_server(codex_home, zsh_path)?; let zdotdir = zdotdir.to_string_lossy().into_owned(); - McpProcess::new_with_env(codex_home, &[("ZDOTDIR", Some(zdotdir.as_str()))]).await + McpProcess::new_with_program_and_env( + codex_home, + &app_server, + &[("ZDOTDIR", Some(zdotdir.as_str()))], + ) + .await +} + +fn create_test_package_app_server(codex_home: &Path, zsh_path: &Path) -> Result { + let package_dir = codex_home.join("test-package"); + let bin_dir = package_dir.join("bin"); + let package_zsh_path = packaged_zsh_path(codex_home); + let Some(zsh_bin_dir) = package_zsh_path.parent() else { + anyhow::bail!("packaged zsh path should have parent"); + }; + std::fs::create_dir_all(&bin_dir)?; + std::fs::create_dir_all(zsh_bin_dir)?; + std::fs::write(package_dir.join("codex-package.json"), "{}")?; + + let app_server = bin_dir.join("codex-app-server"); + copy_with_permissions( + &codex_utils_cargo_bin::cargo_bin("codex-app-server")?, + &app_server, + )?; + copy_with_permissions(zsh_path, &package_zsh_path)?; + Ok(app_server) +} + +fn packaged_zsh_path(codex_home: &Path) -> PathBuf { + codex_home + .join("test-package") + .join("codex-resources") + .join("zsh") + .join("bin") + .join("zsh") +} + +fn command_packaged_zsh_path(codex_home: &Path) -> PathBuf { + let path = packaged_zsh_path(codex_home); + std::fs::canonicalize(&path).unwrap_or(path) +} + +fn copy_with_permissions(source: &Path, destination: &Path) -> std::io::Result<()> { + std::fs::copy(source, destination)?; + std::fs::set_permissions(destination, std::fs::metadata(source)?.permissions()) } fn create_config_toml( @@ -748,7 +795,6 @@ fn create_config_toml( server_uri: &str, approval_policy: &str, feature_flags: &BTreeMap, - zsh_path: &Path, ) -> std::io::Result<()> { let mut features = BTreeMap::from([(Feature::RemoteModels, false)]); for (feature, enabled) in feature_flags { @@ -774,7 +820,6 @@ fn create_config_toml( model = "mock-model" approval_policy = "{approval_policy}" sandbox_mode = "read-only" -zsh_path = "{zsh_path}" model_provider = "mock_provider" @@ -787,9 +832,7 @@ base_url = "{server_uri}/v1" wire_api = "responses" request_max_retries = 0 stream_max_retries = 0 -"#, - approval_policy = approval_policy, - zsh_path = zsh_path.display() +"# ), ) } diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index b0e2a0d45c..34ae6a41b3 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -291,9 +291,6 @@ pub struct ConfigToml { #[schemars(skip)] pub js_repl_node_module_dirs: Option>, - /// Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution. - pub zsh_path: Option, - /// Profile to use from the `profiles` map. pub profile: Option, diff --git a/codex-rs/config/src/profile_toml.rs b/codex-rs/config/src/profile_toml.rs index e7cddd3d67..7d13c02a41 100644 --- a/codex-rs/config/src/profile_toml.rs +++ b/codex-rs/config/src/profile_toml.rs @@ -48,8 +48,6 @@ pub struct ConfigProfile { /// Deprecated: ignored. #[schemars(skip)] pub js_repl_node_module_dirs: Option>, - /// Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution. - pub zsh_path: Option, pub experimental_compact_prompt_file: Option, pub include_permissions_instructions: Option, pub include_apps_instructions: Option, diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 8472918dca..a51c3db7ca 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -47,6 +47,7 @@ codex-shell-command = { workspace = true } codex-execpolicy = { workspace = true } codex-git-utils = { workspace = true } codex-hooks = { workspace = true } +codex-install-context = { workspace = true } codex-network-proxy = { workspace = true } codex-otel = { workspace = true } codex-plugin = { workspace = true } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 7d95eec379..8b653609de 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -711,14 +711,6 @@ } ], "default": null - }, - "zsh_path": { - "allOf": [ - { - "$ref": "#/definitions/AbsolutePathBuf" - } - ], - "description": "Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution." } }, "type": "object" @@ -4986,14 +4978,6 @@ ], "default": null, "description": "Windows-specific configuration." - }, - "zsh_path": { - "allOf": [ - { - "$ref": "#/definitions/AbsolutePathBuf" - } - ], - "description": "Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution." } }, "title": "ConfigToml", diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 09f17e201d..58b6dcc0da 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -4470,6 +4470,25 @@ async fn add_dir_override_extends_workspace_writable_roots() -> std::io::Result< Ok(()) } +#[tokio::test] +async fn default_zsh_path_sets_runtime_zsh_path() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let default_zsh_path = codex_home.path().join("packaged-zsh"); + + let config = Config::load_from_base_config_with_overrides( + ConfigToml::default(), + ConfigOverrides { + default_zsh_path: Some(default_zsh_path.abs()), + ..Default::default() + }, + codex_home.abs(), + ) + .await?; + assert_eq!(config.zsh_path, Some(default_zsh_path)); + + Ok(()) +} + #[tokio::test] async fn sqlite_home_defaults_to_codex_home_for_workspace_write() -> std::io::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 7044b9c676..261850c8a0 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -67,6 +67,7 @@ use codex_features::FeaturesToml; use codex_features::MultiAgentV2ConfigToml; use codex_features::NetworkProxyConfigToml; use codex_git_utils::resolve_root_git_project_for_trust; +use codex_install_context::InstallContext; use codex_login::AuthManagerConfig; use codex_mcp::McpConfig; use codex_memories_read::memory_root; @@ -1392,11 +1393,18 @@ impl Config { .effective_config() .try_into() .map_err(|err| std::io::Error::new(std::io::ErrorKind::InvalidData, err))?; + let default_zsh_path = refreshed_config + .zsh_path + .clone() + .map(AbsolutePathBuf::try_from) + .transpose()?; + Self::load_config_with_layer_stack( LOCAL_FS.as_ref(), cfg, ConfigOverrides { cwd: Some(self.cwd.to_path_buf()), + default_zsh_path, ..Default::default() }, refreshed_config.codex_home.clone(), @@ -2128,7 +2136,7 @@ pub struct ConfigOverrides { pub codex_self_exe: Option, pub codex_linux_sandbox_exe: Option, pub main_execve_wrapper_exe: Option, - pub zsh_path: Option, + pub default_zsh_path: Option, pub base_instructions: Option, pub developer_instructions: Option, pub personality: Option, @@ -2480,7 +2488,7 @@ impl Config { codex_self_exe, codex_linux_sandbox_exe, main_execve_wrapper_exe, - zsh_path: zsh_path_override, + default_zsh_path, base_instructions, developer_instructions, personality, @@ -3199,7 +3207,9 @@ impl Config { ) .await?; let compact_prompt = compact_prompt.or(file_compact_prompt); - let zsh_path = zsh_path_override.or(cfg.zsh_path.map(Into::into)); + let zsh_path = default_zsh_path + .or_else(|| InstallContext::current().bundled_zsh_path()) + .map(AbsolutePathBuf::into_path_buf); let review_model = override_review_model.or(cfg.review_model); diff --git a/codex-rs/core/src/config/permissions_tests.rs b/codex-rs/core/src/config/permissions_tests.rs index 09aa7606aa..8f9d6732e4 100644 --- a/codex-rs/core/src/config/permissions_tests.rs +++ b/codex-rs/core/src/config/permissions_tests.rs @@ -82,7 +82,7 @@ async fn restricted_read_implicitly_allows_helper_executables() -> std::io::Resu }, ConfigOverrides { cwd: Some(cwd.clone()), - zsh_path: Some(zsh_path.clone()), + default_zsh_path: Some(AbsolutePathBuf::try_from(zsh_path.clone())?), main_execve_wrapper_exe: Some(execve_wrapper), ..Default::default() }, diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 6ef8aad91e..e430202ae1 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -826,13 +826,13 @@ impl Session { } else if use_zsh_fork_shell { let zsh_path = config.zsh_path.as_ref().ok_or_else(|| { anyhow::anyhow!( - "zsh fork feature enabled, but `zsh_path` is not configured; set `zsh_path` in config.toml" + "zsh fork feature enabled, but no packaged zsh fork is available for this install" ) })?; let zsh_path = zsh_path.to_path_buf(); shell::get_shell(shell::ShellType::Zsh, Some(&zsh_path)).ok_or_else(|| { anyhow::anyhow!( - "zsh fork feature enabled, but zsh_path `{}` is not usable; set `zsh_path` to a valid zsh executable", + "zsh fork feature enabled, but packaged zsh fork `{}` is not usable", zsh_path.display() ) })? diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 30d26191d7..ac62a3482c 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -4319,7 +4319,7 @@ async fn absolute_cwd_update_with_turn_environment_is_allowed() { } #[tokio::test] -async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() { +async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { let codex_home = tempfile::tempdir().expect("create temp dir"); let mut config = build_test_config(codex_home.path()).await; config @@ -4420,7 +4420,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() { Err(err) => err, }; let msg = format!("{err:#}"); - assert!(msg.contains("zsh fork feature enabled, but `zsh_path` is not configured")); + assert!(msg.contains("zsh fork feature enabled, but no packaged zsh fork is available")); } // todo: use online model info diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 52590b56cc..9061227b6b 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -416,7 +416,7 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result codex_self_exe: arg0_paths.codex_self_exe.clone(), codex_linux_sandbox_exe: arg0_paths.codex_linux_sandbox_exe.clone(), main_execve_wrapper_exe: arg0_paths.main_execve_wrapper_exe.clone(), - zsh_path: None, + default_zsh_path: None, base_instructions: None, developer_instructions: None, personality: None, diff --git a/codex-rs/install-context/src/lib.rs b/codex-rs/install-context/src/lib.rs index ec5c5217ca..d92a80cbef 100644 --- a/codex-rs/install-context/src/lib.rs +++ b/codex-rs/install-context/src/lib.rs @@ -11,6 +11,7 @@ const PATH_DIRNAME: &str = "codex-path"; const RELEASES_DIRNAME: &str = "releases"; const RESOURCES_DIRNAME: &str = "codex-resources"; const STANDALONE_PACKAGES_DIRNAME: &str = "standalone"; +const ZSH_DIRNAME: &str = "zsh"; static INSTALL_CONTEXT: OnceLock = OnceLock::new(); #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -166,6 +167,18 @@ impl InstallContext { None } + + pub fn bundled_zsh_path(&self) -> Option { + if cfg!(windows) { + None + } else { + self.bundled_resource(zsh_resource_path()) + } + } + + pub fn bundled_zsh_bin_dir(&self) -> Option { + self.bundled_zsh_path()?.parent() + } } impl CodexPackageLayout { @@ -260,6 +273,10 @@ fn default_rg_command() -> PathBuf { } } +fn zsh_resource_path() -> PathBuf { + PathBuf::from(ZSH_DIRNAME).join(BIN_DIRNAME).join("zsh") +} + #[cfg(test)] mod tests { use super::*; @@ -345,6 +362,11 @@ mod tests { fs::write(&exe_path, "")?; fs::write(resources_dir.join(TEST_RESOURCE_NAME), "")?; fs::write(path_dir.join(default_rg_command()), "")?; + if !cfg!(windows) { + let zsh_path = resources_dir.join(zsh_resource_path()); + fs::create_dir_all(zsh_path.parent().expect("zsh path should have parent"))?; + fs::write(&zsh_path, "")?; + } let canonical_package_dir = AbsolutePathBuf::from_absolute_path(package_dir.path().canonicalize()?)?; let canonical_bin_dir = AbsolutePathBuf::from_absolute_path(bin_dir.canonicalize()?)?; @@ -382,6 +404,19 @@ mod tests { context.bundled_resource(TEST_RESOURCE_NAME), Some(canonical_resources_dir.join(TEST_RESOURCE_NAME)) ); + if cfg!(windows) { + assert_eq!(context.bundled_zsh_path(), None); + assert_eq!(context.bundled_zsh_bin_dir(), None); + } else { + assert_eq!( + context.bundled_zsh_path(), + Some(canonical_resources_dir.join(zsh_resource_path())) + ); + assert_eq!( + context.bundled_zsh_bin_dir(), + Some(canonical_resources_dir.join(ZSH_DIRNAME).join(BIN_DIRNAME)) + ); + } Ok(()) } diff --git a/scripts/codex_package/README.md b/scripts/codex_package/README.md index af070e2c44..323a3ce5ba 100644 --- a/scripts/codex_package/README.md +++ b/scripts/codex_package/README.md @@ -13,6 +13,7 @@ The builder creates a canonical Codex package directory: │ └── [.exe] ├── codex-resources │ ├── bwrap # Linux only +│ ├── zsh/bin/zsh # supported Unix targets only │ ├── codex-command-runner.exe # Windows only │ └── codex-windows-sandbox-setup.exe # Windows only └── codex-path @@ -67,3 +68,9 @@ DotSlash manifest at `scripts/codex_package/rg`. Downloaded archives are cached under `$TMPDIR/codex-package/-rg` and are reused only after the recorded size and SHA-256 digest have been verified. Pass `--rg-bin` to use a local ripgrep executable instead. + +The patched zsh fork used by `shell_zsh_fork` is fetched from the DotSlash +manifest at `scripts/codex_package/codex-zsh` when the selected target has a +matching prebuilt artifact. Downloaded archives are cached under +`$TMPDIR/codex-package/-zsh` and installed at +`codex-resources/zsh/bin/zsh`. diff --git a/scripts/codex_package/cli.py b/scripts/codex_package/cli.py index 9c0b16258d..0152002669 100644 --- a/scripts/codex_package/cli.py +++ b/scripts/codex_package/cli.py @@ -15,6 +15,7 @@ from .targets import TARGET_SPECS from .targets import PackageInputs from .targets import default_target from .targets import resolve_input_path +from .zsh import resolve_zsh_bin from .version import read_workspace_version @@ -161,13 +162,14 @@ def main() -> int: inputs = PackageInputs( entrypoint_bin=source_outputs.entrypoint_bin, rg_bin=resolve_rg_bin(spec, args.rg_bin), + zsh_bin=resolve_zsh_bin(spec), bwrap_bin=source_outputs.bwrap_bin, codex_command_runner_bin=source_outputs.codex_command_runner_bin, codex_windows_sandbox_setup_bin=source_outputs.codex_windows_sandbox_setup_bin, ) prepare_package_dir(package_dir, force=args.force) build_package_dir(package_dir, version, variant, spec, inputs) - validate_package_dir(package_dir, variant, spec) + validate_package_dir(package_dir, variant, spec, include_zsh=inputs.zsh_bin is not None) for archive_output in args.archive_output: archive_path = archive_output.resolve() diff --git a/scripts/codex_package/codex-zsh b/scripts/codex_package/codex-zsh new file mode 100755 index 0000000000..8876474d81 --- /dev/null +++ b/scripts/codex_package/codex-zsh @@ -0,0 +1,43 @@ +#!/usr/bin/env dotslash + +{ + "name": "codex-zsh", + "platforms": { + "macos-aarch64": { + "size": 358776, + "hash": "sha256", + "digest": "c6dbb063a0135b947ab1cacc655b2b750874699472f412ec7daba97543a90c3c", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh", + "providers": [ + { + "url": "https://github.com/openai/codex/releases/download/rust-v0.132.0/codex-zsh-aarch64-apple-darwin.tar.gz" + } + ] + }, + "linux-x86_64": { + "size": 433413, + "hash": "sha256", + "digest": "5f42d9fc8e9c8c399a727512002906006ae9de966ea7b3d87ca36b47efc59938", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh", + "providers": [ + { + "url": "https://github.com/openai/codex/releases/download/rust-v0.132.0/codex-zsh-x86_64-unknown-linux-musl.tar.gz" + } + ] + }, + "linux-aarch64": { + "size": 411653, + "hash": "sha256", + "digest": "6c6e32c297425db02b4dbffb10925895875d14647fc3eb2f18767be97dc6a945", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh", + "providers": [ + { + "url": "https://github.com/openai/codex/releases/download/rust-v0.132.0/codex-zsh-aarch64-unknown-linux-musl.tar.gz" + } + ] + } + } +} diff --git a/scripts/codex_package/layout.py b/scripts/codex_package/layout.py index 6d8982a631..c763eb2604 100644 --- a/scripts/codex_package/layout.py +++ b/scripts/codex_package/layout.py @@ -8,6 +8,7 @@ from pathlib import Path from .targets import PackageInputs from .targets import PackageVariant from .targets import TargetSpec +from .zsh import ZSH_RESOURCE_PATH LAYOUT_VERSION = 1 @@ -50,6 +51,13 @@ def build_package_dir( ) copy_executable(inputs.rg_bin, path_dir / spec.rg_name, is_windows=spec.is_windows) + if inputs.zsh_bin is not None: + copy_executable( + inputs.zsh_bin, + resources_dir / ZSH_RESOURCE_PATH, + is_windows=False, + ) + if inputs.bwrap_bin is not None: copy_executable(inputs.bwrap_bin, resources_dir / "bwrap", is_windows=False) @@ -83,6 +91,8 @@ def validate_package_dir( package_dir: Path, variant: PackageVariant, spec: TargetSpec, + *, + include_zsh: bool, ) -> None: required_dirs = [ Path("bin"), @@ -122,6 +132,11 @@ def validate_package_dir( ] executable_files = list(required_files) + if include_zsh: + zsh_path = Path("codex-resources") / ZSH_RESOURCE_PATH + required_files.append(zsh_path) + executable_files.append(zsh_path) + if spec.is_linux: required_files.append(Path("codex-resources") / "bwrap") executable_files.append(Path("codex-resources") / "bwrap") @@ -148,7 +163,7 @@ def validate_package_dir( def copy_executable(src: Path, dest: Path, *, is_windows: bool) -> None: dest.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(src, dest) + shutil.copyfile(src, dest) if not is_windows: mode = dest.stat().st_mode dest.chmod(mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) diff --git a/scripts/codex_package/targets.py b/scripts/codex_package/targets.py index 4af0d4a00d..8307a3e630 100644 --- a/scripts/codex_package/targets.py +++ b/scripts/codex_package/targets.py @@ -40,6 +40,7 @@ class PackageVariant: class PackageInputs: entrypoint_bin: Path rg_bin: Path + zsh_bin: Path | None bwrap_bin: Path | None codex_command_runner_bin: Path | None codex_windows_sandbox_setup_bin: Path | None diff --git a/scripts/codex_package/zsh.py b/scripts/codex_package/zsh.py new file mode 100644 index 0000000000..f4ee7e19dd --- /dev/null +++ b/scripts/codex_package/zsh.py @@ -0,0 +1,22 @@ +"""Fetch the patched zsh fork used by shell_zsh_fork.""" + +from pathlib import Path + +from .dotslash import fetch_dotslash_executable +from .targets import REPO_ROOT +from .targets import TargetSpec + + +ZSH_MANIFEST = REPO_ROOT / "scripts" / "codex_package" / "codex-zsh" +ZSH_RESOURCE_PATH = Path("zsh") / "bin" / "zsh" + + +def resolve_zsh_bin(spec: TargetSpec) -> Path | None: + return fetch_dotslash_executable( + spec, + manifest_path=ZSH_MANIFEST, + artifact_label="codex-zsh", + cache_key=f"{spec.target}-zsh", + dest_name="zsh", + missing_ok=True, + ) From 5c20513a1b3d15898429abd92b3676b76795a892 Mon Sep 17 00:00:00 2001 From: Abhinav Date: Fri, 22 May 2026 17:56:58 -0700 Subject: [PATCH 9/9] Default function tools into tool hooks (#23757) # Why `PreToolUse`, `PostToolUse`, and `updatedInput` coverage for local function tools currently depends on each handler remembering to wire up the hook contract itself. That makes coverage easy to miss as new function tools are added, even though most of them share the same basic shape: a model-facing function call with JSON arguments. # What This makes `CoreToolRuntime` provide the default hook contract for ordinary local function tools: - build generic `PreToolUse` and `PostToolUse` payloads from the function tool name and arguments - apply `updatedInput` rewrites back into function-tool arguments through the same default path - let tool outputs override the post-hook input or response when they have a more stable hook-facing contract The exceptions stay explicit: - hosted tools remain outside the generic local function path - code-mode `wait` and `write_stdin` opt out for now - `PostToolUse` feedback replaces only the model-visible response, so code mode keeps its typed tool result With the generic path in place, the MCP and extension-tool adapters no longer need their own duplicate pre/post hook plumbing. The new coverage exercises the registry default plus end-to-end local function behavior for pre-hook blocking, `updatedInput` rewriting, and post-hook context. --- .../core/src/tools/code_mode/wait_handler.rs | 23 ++- .../src/tools/handlers/extension_tools.rs | 45 +--- codex-rs/core/src/tools/handlers/mcp.rs | 73 +------ .../handlers/unified_exec/write_stdin.rs | 10 + codex-rs/core/src/tools/hook_names.rs | 12 ++ codex-rs/core/src/tools/registry.rs | 126 ++++++++++-- codex-rs/core/src/tools/registry_tests.rs | 192 ++++++++++++++++++ codex-rs/core/tests/suite/hooks.rs | 180 ++++++++-------- 8 files changed, 437 insertions(+), 224 deletions(-) diff --git a/codex-rs/core/src/tools/code_mode/wait_handler.rs b/codex-rs/core/src/tools/code_mode/wait_handler.rs index fccf516b14..d0c0453df6 100644 --- a/codex-rs/core/src/tools/code_mode/wait_handler.rs +++ b/codex-rs/core/src/tools/code_mode/wait_handler.rs @@ -2,9 +2,12 @@ use serde::Deserialize; use crate::function_tool::FunctionCallError; use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::registry::CoreToolRuntime; +use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; use codex_tools::ToolName; use codex_tools::ToolSpec; @@ -110,4 +113,22 @@ impl ToolExecutor for CodeModeWaitHandler { } } -impl CoreToolRuntime for CodeModeWaitHandler {} +impl CoreToolRuntime for CodeModeWaitHandler { + fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { + // Code-mode `wait` is runtime control for an existing code cell, not a + // standalone user action. Tool calls made from code mode still flow + // through normal dispatch, but hooks should not block or rewrite the + // wait loop itself. + None + } + + fn post_tool_use_payload( + &self, + _invocation: &ToolInvocation, + _result: &dyn ToolOutput, + ) -> Option { + // The wait result feeds code-mode control flow, so do not let + // PostToolUse replace it with model-facing hook feedback. + None + } +} diff --git a/codex-rs/core/src/tools/handlers/extension_tools.rs b/codex-rs/core/src/tools/handlers/extension_tools.rs index 470b32beae..8c9f55c460 100644 --- a/codex-rs/core/src/tools/handlers/extension_tools.rs +++ b/codex-rs/core/src/tools/handlers/extension_tools.rs @@ -4,17 +4,12 @@ use codex_tools::ConversationHistory; use codex_tools::ToolCall as ExtensionToolCall; use codex_tools::ToolName; use codex_tools::ToolSpec; -use serde_json::Value; use crate::function_tool::FunctionCallError; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; -use crate::tools::flat_tool_name; -use crate::tools::hook_names::HookToolName; use crate::tools::registry::CoreToolRuntime; -use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; pub(crate) struct ExtensionToolAdapter(Arc>); @@ -23,13 +18,6 @@ impl ExtensionToolAdapter { pub(crate) fn new(executor: Arc>) -> Self { Self(executor) } - - fn arguments_from_payload<'a>(&self, payload: &'a ToolPayload) -> Option<&'a str> { - let ToolPayload::Function { arguments } = payload else { - return None; - }; - Some(arguments) - } } #[async_trait::async_trait] @@ -60,30 +48,7 @@ impl ToolExecutor for ExtensionToolAdapter { impl CoreToolRuntime for ExtensionToolAdapter { fn matches_kind(&self, payload: &ToolPayload) -> bool { - self.arguments_from_payload(payload).is_some() - } - - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - let arguments = self.arguments_from_payload(&invocation.payload)?; - Some(PreToolUsePayload { - tool_name: HookToolName::new(flat_tool_name(&self.tool_name()).into_owned()), - tool_input: extension_tool_hook_input(arguments), - }) - } - - fn post_tool_use_payload( - &self, - invocation: &ToolInvocation, - result: &dyn ToolOutput, - ) -> Option { - let arguments = self.arguments_from_payload(&invocation.payload)?; - Some(PostToolUsePayload { - tool_name: HookToolName::new(flat_tool_name(&self.tool_name()).into_owned()), - tool_use_id: invocation.call_id.clone(), - tool_input: extension_tool_hook_input(arguments), - tool_response: result - .post_tool_use_response(&invocation.call_id, &invocation.payload)?, - }) + matches!(payload, ToolPayload::Function { .. }) } } @@ -100,14 +65,6 @@ async fn to_extension_call(invocation: &ToolInvocation) -> ExtensionToolCall { } } -fn extension_tool_hook_input(arguments: &str) -> Value { - if arguments.trim().is_empty() { - return Value::Object(serde_json::Map::new()); - } - - serde_json::from_str(arguments).unwrap_or_else(|_| Value::String(arguments.to_string())) -} - #[cfg(test)] mod tests { use std::sync::Arc; diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 5f6261323d..0d5c158272 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -9,10 +9,7 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::flat_tool_name; -use crate::tools::hook_names::HookToolName; use crate::tools::registry::CoreToolRuntime; -use crate::tools::registry::PostToolUsePayload; -use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; use crate::tools::registry::ToolTelemetryTags; use crate::tools::tool_search_entry::ToolSearchInfo; @@ -23,8 +20,6 @@ use codex_tools::ToolName; use codex_tools::ToolSearchSourceInfo; use codex_tools::ToolSpec; use codex_tools::mcp_tool_to_responses_api_tool; -use serde_json::Map; -use serde_json::Value; pub struct McpHandler { tool_info: ToolInfo, @@ -143,58 +138,6 @@ impl CoreToolRuntime for McpHandler { tags }) } - - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - let ToolPayload::Function { arguments } = &invocation.payload else { - return None; - }; - - Some(PreToolUsePayload { - tool_name: HookToolName::new(self.tool_name().to_string()), - tool_input: mcp_hook_tool_input(arguments), - }) - } - - fn with_updated_hook_input( - &self, - mut invocation: ToolInvocation, - updated_input: Value, - ) -> Result { - invocation.payload = match invocation.payload { - ToolPayload::Function { .. } => ToolPayload::Function { - arguments: serde_json::to_string(&updated_input).map_err(|err| { - FunctionCallError::RespondToModel(format!( - "failed to serialize rewritten MCP arguments: {err}" - )) - })?, - }, - payload => { - return Err(FunctionCallError::RespondToModel(format!( - "tool {} does not support hook input rewriting for payload {payload:?}", - self.tool_name() - ))); - } - }; - Ok(invocation) - } - fn post_tool_use_payload( - &self, - invocation: &ToolInvocation, - result: &dyn crate::tools::context::ToolOutput, - ) -> Option { - let ToolPayload::Function { .. } = &invocation.payload else { - return None; - }; - - let tool_response = - result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; - Some(PostToolUsePayload { - tool_name: HookToolName::new(self.tool_name().to_string()), - tool_use_id: invocation.call_id.clone(), - tool_input: result.post_tool_use_input(&invocation.payload)?, - tool_response, - }) - } } fn create_tool_spec(tool_info: &ToolInfo) -> Result { @@ -223,14 +166,6 @@ fn create_tool_spec(tool_info: &ToolInfo) -> Result })) } -fn mcp_hook_tool_input(raw_arguments: &str) -> Value { - if raw_arguments.trim().is_empty() { - return Value::Object(Map::new()); - } - - serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string())) -} - fn build_mcp_search_text(info: &ToolInfo) -> String { let tool_name = info.canonical_tool_name(); let mut schema_properties = info @@ -288,6 +223,9 @@ mod tests { use super::*; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolCallSource; + use crate::tools::hook_names::HookToolName; + use crate::tools::registry::PostToolUsePayload; + use crate::tools::registry::PreToolUsePayload; use crate::turn_diff_tracker::TurnDiffTracker; use pretty_assertions::assert_eq; use serde_json::json; @@ -447,11 +385,6 @@ mod tests { ); } - #[test] - fn mcp_hook_tool_input_defaults_empty_args_to_object() { - assert_eq!(mcp_hook_tool_input(" "), json!({})); - } - #[test] fn mcp_read_only_hint_supports_parallel_calls_without_server_opt_in() { let mut read_only_info = tool_info("foo", "mcp__foo__", "read"); diff --git a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs index dfa4240ebc..a639ea0065 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs @@ -5,6 +5,7 @@ use crate::tools::context::boxed_tool_output; use crate::tools::handlers::parse_arguments; use crate::tools::registry::CoreToolRuntime; use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; use crate::unified_exec::WriteStdinRequest; use codex_protocol::protocol::EventMsg; @@ -101,11 +102,20 @@ impl CoreToolRuntime for WriteStdinHandler { matches!(payload, ToolPayload::Function { .. }) } + fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { + // `write_stdin` is transport for an existing exec session. Empty writes + // are background polls, and non-empty writes continue a command that + // already ran PreToolUse as Bash, so do not emit a second pre hook here. + None + } + fn post_tool_use_payload( &self, invocation: &ToolInvocation, result: &dyn crate::tools::context::ToolOutput, ) -> Option { + // A `write_stdin` poll can observe final completion for the original + // `exec_command`; emit that command's matching Bash PostToolUse. post_unified_exec_tool_use_payload(invocation, result) } } diff --git a/codex-rs/core/src/tools/hook_names.rs b/codex-rs/core/src/tools/hook_names.rs index 9d3b6c2409..92ebe8aa56 100644 --- a/codex-rs/core/src/tools/hook_names.rs +++ b/codex-rs/core/src/tools/hook_names.rs @@ -38,6 +38,18 @@ impl HookToolName { } } + /// Returns the hook identity for spawning sub-agents. + /// + /// The serialized name remains `spawn_agent`, while `Agent` is accepted as + /// a matcher alias for compatibility with hook configurations that describe + /// sub-agent creation using Claude Code-style names. + pub(crate) fn spawn_agent() -> Self { + Self { + name: "spawn_agent".to_string(), + matcher_aliases: vec!["Agent".to_string()], + } + } + /// Returns the hook identity historically used for shell-like tools. pub(crate) fn bash() -> Self { Self::new("Bash") diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 411ce14359..a08d5c7834 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -19,6 +19,7 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::flat_tool_name; +use crate::tools::handlers::multi_agents_spec::MULTI_AGENT_V1_NAMESPACE; use crate::tools::hook_names::HookToolName; use crate::tools::lifecycle::notify_tool_finish; use crate::tools::lifecycle::notify_tool_start; @@ -26,6 +27,7 @@ use crate::tools::tool_dispatch_trace::ToolDispatchTrace; use crate::tools::tool_search_entry::ToolSearchInfo; use crate::util::error_or_panic; use codex_extension_api::ToolCallOutcome; +use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ResponseInputItem; use codex_protocol::protocol::EventMsg; use codex_tools::ToolName; @@ -64,14 +66,47 @@ pub(crate) trait CoreToolRuntime: ToolExecutor { fn post_tool_use_payload( &self, - _invocation: &ToolInvocation, - _result: &dyn ToolOutput, + invocation: &ToolInvocation, + result: &dyn ToolOutput, ) -> Option { - None + let ToolPayload::Function { arguments } = &invocation.payload else { + return None; + }; + + Some(PostToolUsePayload { + tool_name: function_hook_tool_name(invocation), + tool_use_id: result.post_tool_use_id(&invocation.call_id), + tool_input: result + .post_tool_use_input(&invocation.payload) + .unwrap_or_else(|| function_hook_tool_input(arguments)), + tool_response: result + .post_tool_use_response(&invocation.call_id, &invocation.payload) + .or_else(|| { + // Most function tools can expose their model-facing output + // as the hook response. Outputs with a more stable hook + // contract should override post_tool_use_response above. + let ResponseInputItem::FunctionCallOutput { + output: FunctionCallOutputPayload { body, .. }, + .. + } = result.to_response_item(&invocation.call_id, &invocation.payload) + else { + return None; + }; + + serde_json::to_value(body).ok() + })?, + }) } - fn pre_tool_use_payload(&self, _invocation: &ToolInvocation) -> Option { - None + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + let ToolPayload::Function { arguments } = &invocation.payload else { + return None; + }; + + Some(PreToolUsePayload { + tool_name: function_hook_tool_name(invocation), + tool_input: function_hook_tool_input(arguments), + }) } /// Rebuilds a tool invocation from hook-facing `tool_input`. @@ -80,12 +115,25 @@ pub(crate) trait CoreToolRuntime: ToolExecutor { /// hook contract they expose from `pre_tool_use_payload`. fn with_updated_hook_input( &self, - _invocation: ToolInvocation, - _updated_input: Value, + invocation: ToolInvocation, + updated_input: Value, ) -> Result { - Err(FunctionCallError::RespondToModel( - "tool does not support hook input rewriting".to_string(), - )) + let ToolPayload::Function { .. } = &invocation.payload else { + return Err(FunctionCallError::RespondToModel( + "hook input rewrite received unsupported function tool payload".to_string(), + )); + }; + + let arguments = serde_json::to_string(&updated_input).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten {} arguments: {err}", + flat_tool_name(&invocation.tool_name) + )) + })?; + Ok(ToolInvocation { + payload: ToolPayload::Function { arguments }, + ..invocation + }) } /// Creates an optional consumer for streamed tool argument diffs. @@ -133,6 +181,29 @@ impl AnyToolResult { } } +struct PostToolUseFeedbackOutput { + original: Box, + model_visible: FunctionToolOutput, +} + +impl ToolOutput for PostToolUseFeedbackOutput { + fn log_preview(&self) -> String { + self.original.log_preview() + } + + fn success_for_logging(&self) -> bool { + self.original.success_for_logging() + } + + fn to_response_item(&self, call_id: &str, payload: &ToolPayload) -> ResponseInputItem { + self.model_visible.to_response_item(call_id, payload) + } + + fn code_mode_result(&self, payload: &ToolPayload) -> Value { + self.original.code_mode_result(payload) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct PreToolUsePayload { /// Hook-facing tool name model. @@ -538,11 +609,15 @@ impl ToolRegistry { }; if let Some(replacement_text) = replacement_text { let mut guard = response_cell.lock().await; - if let Some(result) = guard.as_mut() { - result.result = Box::new(FunctionToolOutput::from_text( - replacement_text, - /*success*/ None, - )); + if let Some(mut result) = guard.take() { + result.result = Box::new(PostToolUseFeedbackOutput { + original: result.result, + model_visible: FunctionToolOutput::from_text( + replacement_text, + /*success*/ None, + ), + }); + *guard = Some(result); } } } @@ -618,6 +693,27 @@ async fn handle_any_tool( }) } +fn function_hook_tool_name(invocation: &ToolInvocation) -> HookToolName { + if invocation.tool_name.name == "spawn_agent" + && matches!( + invocation.tool_name.namespace.as_deref(), + None | Some(MULTI_AGENT_V1_NAMESPACE) + ) + { + return HookToolName::spawn_agent(); + } + + HookToolName::new(flat_tool_name(&invocation.tool_name).into_owned()) +} + +fn function_hook_tool_input(arguments: &str) -> Value { + if arguments.trim().is_empty() { + return Value::Object(serde_json::Map::new()); + } + + serde_json::from_str(arguments).unwrap_or_else(|_| Value::String(arguments.to_string())) +} + fn unsupported_tool_call_message(payload: &ToolPayload, tool_name: &ToolName) -> String { match payload { ToolPayload::Custom { .. } => format!("unsupported custom tool call: {tool_name}"), diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index 8517730d32..f2566251c5 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -172,6 +172,198 @@ fn handler_looks_up_namespaced_aliases_explicitly() { ); } +#[tokio::test] +async fn function_tools_expose_default_hook_payloads_and_rewrites() -> anyhow::Result<()> { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let tool_name = codex_tools::ToolName::namespaced("functions.", "echo"); + let handler = TestHandler { + tool_name: tool_name.clone(), + }; + let invocation = ToolInvocation { + payload: ToolPayload::Function { + arguments: serde_json::json!({ "message": "hello" }).to_string(), + }, + ..test_invocation(Arc::new(session), Arc::new(turn), "call-1", tool_name) + }; + let output = + crate::tools::context::FunctionToolOutput::from_text("echoed".to_string(), Some(true)); + + assert_eq!( + handler.pre_tool_use_payload(&invocation), + Some(PreToolUsePayload { + tool_name: HookToolName::new("functions.echo"), + tool_input: serde_json::json!({ "message": "hello" }), + }) + ); + assert_eq!( + handler.post_tool_use_payload(&invocation, &output), + Some(PostToolUsePayload { + tool_name: HookToolName::new("functions.echo"), + tool_use_id: "call-1".to_string(), + tool_input: serde_json::json!({ "message": "hello" }), + tool_response: serde_json::json!("echoed"), + }) + ); + + let invocation = handler + .with_updated_hook_input(invocation, serde_json::json!({ "message": "rewritten" }))?; + let ToolPayload::Function { arguments } = invocation.payload else { + panic!("generic rewritten function payload should remain function-shaped"); + }; + assert_eq!( + serde_json::from_str::(&arguments)?, + serde_json::json!({ "message": "rewritten" }) + ); + + Ok(()) +} + +#[tokio::test] +async fn function_hook_input_defaults_empty_arguments_to_object() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let tool_name = codex_tools::ToolName::plain("echo"); + let handler = TestHandler { + tool_name: tool_name.clone(), + }; + let invocation = ToolInvocation { + payload: ToolPayload::Function { + arguments: " ".to_string(), + }, + ..test_invocation(Arc::new(session), Arc::new(turn), "call-1", tool_name) + }; + + assert_eq!( + handler.pre_tool_use_payload(&invocation), + Some(PreToolUsePayload { + tool_name: HookToolName::new("echo"), + tool_input: serde_json::json!({}), + }) + ); +} + +#[tokio::test] +async fn spawn_agent_function_tools_use_agent_matcher_alias() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let session = Arc::new(session); + let turn = Arc::new(turn); + + let hook_payloads = [ + codex_tools::ToolName::plain("spawn_agent"), + codex_tools::ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "spawn_agent"), + ] + .into_iter() + .map(|tool_name| { + let handler = TestHandler { + tool_name: tool_name.clone(), + }; + let invocation = ToolInvocation { + payload: ToolPayload::Function { + arguments: serde_json::json!({ "message": "inspect this repo" }).to_string(), + }, + ..test_invocation(Arc::clone(&session), Arc::clone(&turn), "call-1", tool_name) + }; + handler.pre_tool_use_payload(&invocation) + }) + .collect::>(); + + assert_eq!( + hook_payloads, + vec![ + Some(PreToolUsePayload { + tool_name: HookToolName::spawn_agent(), + tool_input: serde_json::json!({ "message": "inspect this repo" }), + }), + Some(PreToolUsePayload { + tool_name: HookToolName::spawn_agent(), + tool_input: serde_json::json!({ "message": "inspect this repo" }), + }), + ] + ); +} + +#[tokio::test] +async fn code_mode_wait_does_not_expose_default_hook_payloads() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + let output = crate::tools::context::FunctionToolOutput::from_text("ok".to_string(), Some(true)); + + let wait = crate::tools::handlers::CodeModeWaitHandler; + let wait_invocation = test_invocation( + Arc::new(session), + Arc::new(turn), + "wait-call", + wait.tool_name(), + ); + assert_eq!(wait.pre_tool_use_payload(&wait_invocation), None); + assert_eq!(wait.post_tool_use_payload(&wait_invocation, &output), None); +} + +#[tokio::test] +async fn write_stdin_does_not_expose_default_pre_tool_use_payload() { + let (session, turn) = crate::session::tests::make_session_and_context().await; + + let write_stdin = crate::tools::handlers::WriteStdinHandler; + let invocation = test_invocation( + Arc::new(session), + Arc::new(turn), + "write-stdin-call", + write_stdin.tool_name(), + ); + + assert_eq!(write_stdin.pre_tool_use_payload(&invocation), None); +} + +#[test] +fn post_tool_use_feedback_output_keeps_code_mode_result_typed() { + let result = AnyToolResult { + call_id: "call-1".to_string(), + payload: ToolPayload::Function { + arguments: "{}".to_string(), + }, + result: Box::new(PostToolUseFeedbackOutput { + original: Box::new(codex_tools::JsonToolOutput::new( + serde_json::json!({ "typed": true }), + )), + model_visible: crate::tools::context::FunctionToolOutput::from_text( + "hook feedback".to_string(), + /*success*/ None, + ), + }), + post_tool_use_payload: None, + }; + + assert_eq!( + result.into_response(), + ResponseInputItem::FunctionCallOutput { + call_id: "call-1".to_string(), + output: codex_protocol::models::FunctionCallOutputPayload::from_text( + "hook feedback".to_string() + ), + } + ); + + let result = AnyToolResult { + call_id: "call-1".to_string(), + payload: ToolPayload::Function { + arguments: "{}".to_string(), + }, + result: Box::new(PostToolUseFeedbackOutput { + original: Box::new(codex_tools::JsonToolOutput::new( + serde_json::json!({ "typed": true }), + )), + model_visible: crate::tools::context::FunctionToolOutput::from_text( + "hook feedback".to_string(), + /*success*/ None, + ), + }), + post_tool_use_payload: None, + }; + + assert_eq!( + result.code_mode_result(), + serde_json::json!({ "typed": true }) + ); +} + #[tokio::test] async fn dispatch_notifies_tool_lifecycle_contributors() -> anyhow::Result<()> { let (mut session, turn) = crate::session::tests::make_session_and_context().await; diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index ee1826ceb9..3791746551 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -3463,42 +3463,35 @@ async fn pre_tool_use_blocks_apply_patch_with_write_alias() -> Result<()> { } #[tokio::test] -async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> { +async fn pre_tool_use_blocks_local_function_tool_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let call_id = "pretooluse-update-plan"; - let args = serde_json::json!({ - "plan": [{ - "step": "watch the tide", - "status": "pending", - }] - }); + let call_id = "pretooluse-local-function-tool"; + let args = serde_json::json!({}); let responses = mount_sse_sequence( &server, vec![ sse(vec![ ev_response_created("resp-1"), - core_test_support::responses::ev_function_call( - call_id, - "update_plan", - &serde_json::to_string(&args)?, - ), + ev_function_call(call_id, "test_sync_tool", &serde_json::to_string(&args)?), ev_completed("resp-1"), ]), sse(vec![ ev_response_created("resp-2"), - ev_assistant_message("msg-1", "plan updated"), + ev_assistant_message("msg-1", "local function hook blocked it"), ev_completed("resp-2"), ]), ], ) .await; + let reason = "blocked local function pre hook"; let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") .with_pre_build_hook(|home| { if let Err(error) = - write_pre_tool_use_hook(home, /*matcher*/ None, "json_deny", "should not fire") + write_pre_tool_use_hook(home, Some("^test_sync_tool$"), "json_deny", reason) { panic!("failed to write pre tool use hook test fixture: {error}"); } @@ -3506,7 +3499,8 @@ async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> { .with_config(trust_discovered_hooks); let test = builder.build(&server).await?; - test.submit_turn("update the plan").await?; + test.submit_turn("call the local function tool with the pre hook") + .await?; let requests = responses.requests(); assert_eq!(requests.len(), 2); @@ -3514,17 +3508,85 @@ async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> { let output = output_item .get("output") .and_then(Value::as_str) - .expect("update plan output string"); + .expect("blocked local function tool output string"); assert!( - !output.contains("should not fire"), - "non-shell tool output should not be blocked by PreToolUse", + output.contains(&format!( + "Tool call blocked by PreToolUse hook: {reason}. Tool: test_sync_tool" + )), + "blocked local function output should surface the hook reason and tool name", ); - let hook_log_path = test.codex_home_path().join("pre_tool_use_hook_log.jsonl"); - assert!( - !hook_log_path.exists(), - "plan tool should not trigger pre tool use hooks", - ); + let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["hook_event_name"], "PreToolUse"); + assert_eq!(hook_inputs[0]["tool_name"], "test_sync_tool"); + assert_eq!(hook_inputs[0]["tool_use_id"], call_id); + assert_eq!(hook_inputs[0]["tool_input"], args); + + Ok(()) +} + +#[tokio::test] +async fn pre_tool_use_rewrites_local_function_tool_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-local-function-tool-rewrite"; + let original_args = serde_json::json!({ + "barrier": { + "id": "pretooluse-local-function-invalid-barrier", + "participants": 0, + } + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + call_id, + "test_sync_tool", + &serde_json::to_string(&original_args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "local function hook rewrote it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let updated_input = serde_json::json!({}); + let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") + .with_pre_build_hook(move |home| { + if let Err(error) = + write_updating_pre_tool_use_hook(home, "^test_sync_tool$", &updated_input) + { + panic!("failed to write updating pre tool use hook test fixture: {error}"); + } + }) + .with_config(trust_discovered_hooks); + let test = builder.build(&server).await?; + + test.submit_turn("call the local function tool with the pre hook rewrite") + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("rewritten local function tool output string"); + assert_eq!(output, "ok"); + + let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["tool_input"], original_args); Ok(()) } @@ -4133,73 +4195,3 @@ async fn post_tool_use_records_apply_patch_context_with_edit_alias() -> Result<( Ok(()) } - -#[tokio::test] -async fn post_tool_use_does_not_fire_for_plan_tool() -> Result<()> { - skip_if_no_network!(Ok(())); - - let server = start_mock_server().await; - let call_id = "posttooluse-update-plan"; - let args = serde_json::json!({ - "plan": [{ - "step": "watch the tide", - "status": "pending", - }] - }); - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-1"), - core_test_support::responses::ev_function_call( - call_id, - "update_plan", - &serde_json::to_string(&args)?, - ), - ev_completed("resp-1"), - ]), - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "plan updated"), - ev_completed("resp-2"), - ]), - ], - ) - .await; - - let mut builder = test_codex() - .with_pre_build_hook(|home| { - if let Err(error) = write_post_tool_use_hook( - home, - /*matcher*/ None, - "decision_block", - "should not fire", - ) { - panic!("failed to write post tool use hook test fixture: {error}"); - } - }) - .with_config(trust_discovered_hooks); - let test = builder.build(&server).await?; - - test.submit_turn("update the plan").await?; - - let requests = responses.requests(); - assert_eq!(requests.len(), 2); - let output_item = requests[1].function_call_output(call_id); - let output = output_item - .get("output") - .and_then(Value::as_str) - .expect("update plan output string"); - assert!( - !output.contains("should not fire"), - "non-shell tool output should not be affected by PostToolUse", - ); - - let hook_log_path = test.codex_home_path().join("post_tool_use_hook_log.jsonl"); - assert!( - !hook_log_path.exists(), - "plan tool should not trigger post tool use hooks", - ); - - Ok(()) -}