From 18f1a08bc9c6e39331d9cf34ee240ea0124173cb Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 23 Mar 2026 21:09:00 +0000 Subject: [PATCH 1/8] feat: new op type for sub-agents communication (#15556) Add `InterAgentCommunication` for v2 agent communication --- codex-rs/core/src/agent/control.rs | 10 +-- .../core/src/agent/inter_agent_instruction.rs | 74 ----------------- codex-rs/core/src/agent/mod.rs | 1 - codex-rs/core/src/codex.rs | 28 +++++++ codex-rs/core/src/codex_thread.rs | 33 +------- codex-rs/core/src/context_manager/history.rs | 4 +- codex-rs/core/src/tasks/mod.rs | 1 + codex-rs/core/src/thread_manager.rs | 14 ---- .../src/tools/handlers/multi_agents_tests.rs | 23 +++++ .../handlers/multi_agents_v2/send_input.rs | 15 +--- codex-rs/protocol/src/protocol.rs | 83 +++++++++++++++++++ 11 files changed, 145 insertions(+), 141 deletions(-) delete mode 100644 codex-rs/core/src/agent/inter_agent_instruction.rs diff --git a/codex-rs/core/src/agent/control.rs b/codex-rs/core/src/agent/control.rs index 5bc5a0711c..3bdcc2efd9 100644 --- a/codex-rs/core/src/agent/control.rs +++ b/codex-rs/core/src/agent/control.rs @@ -1,6 +1,4 @@ use crate::agent::AgentStatus; -use crate::agent::inter_agent_instruction::InterAgentDelivery; -use crate::agent::inter_agent_instruction::InterAgentInstruction; use crate::agent::registry::AgentMetadata; use crate::agent::registry::AgentRegistry; use crate::agent::role::DEFAULT_ROLE_NAME; @@ -23,6 +21,7 @@ use codex_protocol::ThreadId; use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::InitialHistory; +use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::Op; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::SessionSource; @@ -488,18 +487,17 @@ impl AgentControl { .await } - pub(crate) async fn deliver_inter_agent_instruction( + pub(crate) async fn send_inter_agent_communication( &self, agent_id: ThreadId, - instruction: InterAgentInstruction, - delivery: InterAgentDelivery, + communication: InterAgentCommunication, ) -> CodexResult { let state = self.upgrade()?; self.handle_thread_request_result( agent_id, &state, state - .deliver_inter_agent_instruction(agent_id, instruction, delivery) + .send_op(agent_id, Op::InterAgentCommunication { communication }) .await, ) .await diff --git a/codex-rs/core/src/agent/inter_agent_instruction.rs b/codex-rs/core/src/agent/inter_agent_instruction.rs deleted file mode 100644 index 0eff40beb4..0000000000 --- a/codex-rs/core/src/agent/inter_agent_instruction.rs +++ /dev/null @@ -1,74 +0,0 @@ -use codex_protocol::AgentPath; -use codex_protocol::models::ContentItem; -use codex_protocol::models::ResponseItem; - -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub(crate) enum InterAgentDelivery { - CurrentTurn, - NextTurn, -} - -#[derive(Clone, Debug, Eq, PartialEq)] -pub(crate) struct InterAgentInstruction { - author: AgentPath, - recipient: AgentPath, - other_recipients: Vec, - content: String, -} - -impl InterAgentInstruction { - pub(crate) fn new( - author: AgentPath, - recipient: AgentPath, - other_recipients: Vec, - content: String, - ) -> Self { - Self { - author, - recipient, - other_recipients, - content, - } - } - - pub(crate) fn to_response_item(&self) -> ResponseItem { - ResponseItem::Message { - id: None, - role: "assistant".to_string(), - content: vec![ContentItem::OutputText { - text: self.as_text(), - }], - end_turn: None, - phase: None, - } - } - - pub(crate) fn is_message_content(content: &[ContentItem]) -> bool { - content.iter().any(|content_item| match content_item { - ContentItem::InputText { text } | ContentItem::OutputText { text } => { - Self::is_instruction_text(text) - } - _ => false, - }) - } - - fn as_text(&self) -> String { - let other_recipients = self - .other_recipients - .iter() - .map(std::string::ToString::to_string) - .collect::>() - .join(", "); - format!( - "author: {}\nrecipient: {}\nother_recipients: [{other_recipients}]\nContent: {}", - self.author, self.recipient, self.content - ) - } - - fn is_instruction_text(text: &str) -> bool { - text.starts_with("author: ") - && text.contains("\nrecipient: ") - && text.contains("\nother_recipients: [") - && text.contains("]\nContent: ") - } -} diff --git a/codex-rs/core/src/agent/mod.rs b/codex-rs/core/src/agent/mod.rs index 3f14e5c590..350962dc08 100644 --- a/codex-rs/core/src/agent/mod.rs +++ b/codex-rs/core/src/agent/mod.rs @@ -1,6 +1,5 @@ pub(crate) mod agent_resolver; pub(crate) mod control; -pub(crate) mod inter_agent_instruction; mod registry; pub(crate) mod role; pub(crate) mod status; diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index b040883c25..9b81025526 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4306,6 +4306,10 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv handlers::user_input_or_turn(&sess, sub.id.clone(), sub.op).await; false } + Op::InterAgentCommunication { communication } => { + handlers::inter_agent_communication(&sess, sub.id.clone(), communication).await; + false + } Op::ExecApproval { id: approval_id, turn_id, @@ -4485,6 +4489,7 @@ mod handlers { use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; + use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::ListCustomPromptsResponseEvent; use codex_protocol::protocol::ListSkillsResponseEvent; use codex_protocol::protocol::McpServerRefreshConfig; @@ -4627,6 +4632,29 @@ mod handlers { } } + pub async fn inter_agent_communication( + sess: &Arc, + sub_id: String, + communication: InterAgentCommunication, + ) { + let pending_item = communication.to_response_input_item(); + if sess + .inject_response_items(vec![pending_item.clone()]) + .await + .is_ok() + { + return; + } + + let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; + sess.maybe_emit_unknown_model_warning_for_turn(turn_context.as_ref()) + .await; + sess.queue_response_items_for_next_turn(vec![pending_item]) + .await; + sess.spawn_task(turn_context, Vec::new(), crate::tasks::RegularTask::new()) + .await; + } + pub async fn run_user_shell_command(sess: &Arc, sub_id: String, command: String) { if let Some((turn_context, cancellation_token)) = sess.active_turn_context_and_cancellation_token().await diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index ff36de6c15..9fa7069118 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -1,6 +1,4 @@ use crate::agent::AgentStatus; -use crate::agent::inter_agent_instruction::InterAgentDelivery; -use crate::agent::inter_agent_instruction::InterAgentInstruction; use crate::codex::Codex; use crate::codex::SteerInputError; use crate::config::ConstraintResult; @@ -158,6 +156,7 @@ impl CodexThread { /// If the thread already has an active turn, the message is queued as pending input for that /// turn. Otherwise it is queued at session scope and a regular turn is started so the agent /// can consume that pending input through the normal turn pipeline. + #[cfg(test)] pub(crate) async fn append_message(&self, message: ResponseItem) -> CodexResult { let submission_id = uuid::Uuid::new_v4().to_string(); let pending_item = pending_message_input_item(&message)?; @@ -180,36 +179,6 @@ impl CodexThread { Ok(submission_id) } - pub(crate) async fn deliver_inter_agent_instruction( - &self, - instruction: InterAgentInstruction, - delivery: InterAgentDelivery, - ) -> CodexResult { - let message = instruction.to_response_item(); - match delivery { - InterAgentDelivery::CurrentTurn => self.append_message(message).await, - InterAgentDelivery::NextTurn => self.queue_message_for_next_turn(message).await, - } - } - - /// Queue a prebuilt message so the next turn records it before any submitted user input. - pub(crate) async fn queue_message_for_next_turn( - &self, - message: ResponseItem, - ) -> CodexResult { - let submission_id = uuid::Uuid::new_v4().to_string(); - let pending_item = pending_message_input_item(&message)?; - self.codex - .session - .queue_response_items_for_next_turn(vec![pending_item]) - .await; - self.codex - .session - .ensure_task_for_queued_response_items() - .await; - Ok(submission_id) - } - pub fn rollout_path(&self) -> Option { self.rollout_path.clone() } diff --git a/codex-rs/core/src/context_manager/history.rs b/codex-rs/core/src/context_manager/history.rs index 6ed0370481..57807e5f85 100644 --- a/codex-rs/core/src/context_manager/history.rs +++ b/codex-rs/core/src/context_manager/history.rs @@ -1,4 +1,3 @@ -use crate::agent::inter_agent_instruction::InterAgentInstruction; use crate::codex::TurnContext; use crate::context_manager::normalize; use crate::event_mapping::is_contextual_user_message_content; @@ -18,6 +17,7 @@ use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ImageDetail; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::InputModality; +use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::TokenUsage; use codex_protocol::protocol::TokenUsageInfo; use codex_protocol::protocol::TurnContextItem; @@ -638,7 +638,7 @@ pub(crate) fn is_user_turn_boundary(item: &ResponseItem) -> bool { } fn is_inter_agent_instruction_content(content: &[ContentItem]) -> bool { - InterAgentInstruction::is_message_content(content) + InterAgentCommunication::is_message_content(content) } fn user_message_positions(items: &[ResponseItem]) -> Vec { diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index b2f110486c..d9c4954cc8 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -405,6 +405,7 @@ impl Session { turn.add_task(task); *active = Some(turn); } + async fn take_active_turn(&self) -> Option { let mut active = self.active_turn.lock().await; active.take() diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index d93bf37187..185a32a70f 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -3,8 +3,6 @@ use crate::CodexAuth; use crate::ModelProviderInfo; use crate::OPENAI_PROVIDER_ID; use crate::agent::AgentControl; -use crate::agent::inter_agent_instruction::InterAgentDelivery; -use crate::agent::inter_agent_instruction::InterAgentInstruction; use crate::codex::Codex; use crate::codex::CodexSpawnArgs; use crate::codex::CodexSpawnOk; @@ -621,18 +619,6 @@ impl ThreadManagerState { thread.append_message(message).await } - pub(crate) async fn deliver_inter_agent_instruction( - &self, - thread_id: ThreadId, - instruction: InterAgentInstruction, - delivery: InterAgentDelivery, - ) -> CodexResult { - let thread = self.get_thread(thread_id).await?; - thread - .deliver_inter_agent_instruction(instruction, delivery) - .await - } - /// Remove a thread from the manager by ID, returning it when present. pub(crate) async fn remove_thread(&self, thread_id: &ThreadId) -> Option> { self.threads.write().await.remove(thread_id) diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index f60137f7fe..7b8471f03e 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -24,6 +24,7 @@ use crate::tools::handlers::multi_agents_v2::SpawnAgentHandler as SpawnAgentHand use crate::tools::handlers::multi_agents_v2::WaitAgentHandler as WaitAgentHandlerV2; use crate::turn_diff_tracker::TurnDiffTracker; use codex_features::Feature; +use codex_protocol::AgentPath; use codex_protocol::ThreadId; use codex_protocol::models::BaseInstructions; use codex_protocol::models::ContentItem; @@ -373,6 +374,18 @@ async fn multi_agent_v2_spawn_returns_path_and_send_input_accepts_relative_path( .await .expect("send_input should accept v2 path"); + assert!(manager.captured_ops().iter().any(|(id, op)| { + *id == child_thread_id + && matches!( + op, + Op::InterAgentCommunication { communication } + if communication.author == AgentPath::root() + && communication.recipient.as_str() == "/root/test_process" + && communication.other_recipients.is_empty() + && communication.content == "continue" + ) + })); + let child_thread = manager .get_thread(child_thread_id) .await @@ -618,6 +631,16 @@ async fn multi_agent_v2_send_input_interrupts_busy_child_without_losing_message( .filter_map(|(id, op)| (*id == agent_id).then_some(op)) .collect(); assert!(ops_for_agent.iter().any(|op| matches!(op, Op::Interrupt))); + assert!(ops_for_agent.iter().any(|op| { + matches!( + op, + Op::InterAgentCommunication { communication } + if communication.author == AgentPath::root() + && communication.recipient.as_str() == "/root/worker" + && communication.other_recipients.is_empty() + && communication.content == "continue" + ) + })); assert!(!ops_for_agent.iter().any(|op| matches!( op, Op::UserInput { items, .. } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/send_input.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/send_input.rs index 8c29a7aec8..c17e12cca8 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/send_input.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/send_input.rs @@ -1,6 +1,5 @@ use super::*; -use crate::agent::inter_agent_instruction::InterAgentDelivery; -use crate::agent::inter_agent_instruction::InterAgentInstruction; +use codex_protocol::protocol::InterAgentCommunication; pub(crate) struct Handler; @@ -66,7 +65,7 @@ impl ToolHandler for Handler { "target agent is missing an agent_path".to_string(), ) })?; - let instruction = InterAgentInstruction::new( + let communication = InterAgentCommunication::new( turn.session_source .get_agent_path() .unwrap_or_else(AgentPath::root), @@ -77,15 +76,7 @@ impl ToolHandler for Handler { session .services .agent_control - .deliver_inter_agent_instruction( - receiver_thread_id, - instruction, - if args.interrupt { - InterAgentDelivery::NextTurn - } else { - InterAgentDelivery::CurrentTurn - }, - ) + .send_inter_agent_communication(receiver_thread_id, communication) .await } else { session diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index c88c4ecee8..c8af844f7b 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -38,6 +38,7 @@ use crate::message_history::HistoryEntry; use crate::models::BaseInstructions; use crate::models::ContentItem; use crate::models::MessagePhase; +use crate::models::ResponseInputItem; use crate::models::ResponseItem; use crate::models::WebSearchAction; use crate::num_format::format_with_separators; @@ -293,6 +294,12 @@ pub enum Op { personality: Option, }, + /// Inter-agent communication that should be recorded as assistant history + /// while still using the normal thread submission lifecycle. + InterAgentCommunication { + communication: InterAgentCommunication, + }, + /// Override parts of the persistent turn context for subsequent turns. /// /// All fields are optional; when omitted, the existing value is preserved. @@ -499,6 +506,81 @@ pub enum Op { ListModels, } +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, JsonSchema, TS)] +pub struct InterAgentCommunication { + pub author: AgentPath, + pub recipient: AgentPath, + #[serde(default)] + pub other_recipients: Vec, + pub content: String, +} + +impl InterAgentCommunication { + pub fn new( + author: AgentPath, + recipient: AgentPath, + other_recipients: Vec, + content: String, + ) -> Self { + Self { + author, + recipient, + other_recipients, + content, + } + } + + pub fn to_response_item(&self) -> ResponseItem { + ResponseItem::Message { + id: None, + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: self.as_text(), + }], + end_turn: None, + phase: None, + } + } + + pub fn to_response_input_item(&self) -> ResponseInputItem { + ResponseInputItem::Message { + role: "assistant".to_string(), + content: vec![ContentItem::OutputText { + text: self.as_text(), + }], + } + } + + pub fn is_message_content(content: &[ContentItem]) -> bool { + content.iter().any(|content_item| match content_item { + ContentItem::InputText { text } | ContentItem::OutputText { text } => { + Self::is_instruction_text(text) + } + _ => false, + }) + } + + fn as_text(&self) -> String { + let other_recipients = self + .other_recipients + .iter() + .map(std::string::ToString::to_string) + .collect::>() + .join(", "); + format!( + "author: {}\nrecipient: {}\nother_recipients: [{other_recipients}]\nContent: {}", + self.author, self.recipient, self.content + ) + } + + fn is_instruction_text(text: &str) -> bool { + text.starts_with("author: ") + && text.contains("\nrecipient: ") + && text.contains("\nother_recipients: [") + && text.contains("]\nContent: ") + } +} + impl Op { pub fn kind(&self) -> &'static str { match self { @@ -510,6 +592,7 @@ impl Op { Self::RealtimeConversationClose => "realtime_conversation_close", Self::UserInput { .. } => "user_input", Self::UserTurn { .. } => "user_turn", + Self::InterAgentCommunication { .. } => "inter_agent_communication", Self::OverrideTurnContext { .. } => "override_turn_context", Self::ExecApproval { .. } => "exec_approval", Self::PatchApproval { .. } => "patch_approval", From 73bbb07ba8302932a5462811bc68da0ef66ce50a Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Mon, 23 Mar 2026 14:32:59 -0700 Subject: [PATCH 2/8] [hooks] add non-streaming (non-stdin style) shell-only PreToolUse support (#15211) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - add `PreToolUse` hook for bash-like tool execution only at first - block shell execution before dispatch with deny-only hook behavior - introduces common.rs matcher framework for matching when hooks are run example run: ``` › run three parallel echo commands, and the second one should echo "[block-pre-tool-use]" as a test • Running the three echo commands in parallel now and I’ll report the output directly. • Running PreToolUse hook: name for demo pre tool use hook • Running PreToolUse hook: name for demo pre tool use hook • Running PreToolUse hook: name for demo pre tool use hook PreToolUse hook (completed) warning: wizard-tower PreToolUse demo inspected Bash: echo "first parallel echo" PreToolUse hook (blocked) warning: wizard-tower PreToolUse demo blocked a Bash command on purpose. feedback: PreToolUse demo blocked the command. Remove [block-pre-tool-use] to continue. PreToolUse hook (completed) warning: wizard-tower PreToolUse demo inspected Bash: echo "third parallel echo" • Ran echo "first parallel echo" └ first parallel echo • Ran echo "third parallel echo" └ third parallel echo • Three little waves went out in parallel. 1. printed first parallel echo 2. was blocked before execution because it contained the exact test string [block-pre-tool-use] 3. printed third parallel echo There was also an unrelated macOS defaults warning around the successful commands, but the echoes themselves worked fine. If you want, I can rerun the second one with a slightly modified string so it passes cleanly. ``` --- .../schema/json/ServerNotification.json | 1 + .../codex_app_server_protocol.schemas.json | 1 + .../codex_app_server_protocol.v2.schemas.json | 1 + .../json/v2/HookCompletedNotification.json | 1 + .../json/v2/HookStartedNotification.json | 1 + .../schema/typescript/v2/HookEventName.ts | 2 +- .../app-server-protocol/src/protocol/v2.rs | 2 +- codex-rs/core/src/hook_runtime.rs | 32 ++ codex-rs/core/src/tools/registry.rs | 47 ++ codex-rs/core/src/tools/registry_tests.rs | 62 +++ codex-rs/core/tests/suite/hooks.rs | 426 ++++++++++++++++ codex-rs/core/tests/suite/otel.rs | 3 +- .../src/event_processor_with_human_output.rs | 1 + .../pre-tool-use.command.input.schema.json | 80 +++ .../pre-tool-use.command.output.schema.json | 101 ++++ .../session-start.command.output.schema.json | 1 + ...r-prompt-submit.command.output.schema.json | 1 + codex-rs/hooks/src/engine/config.rs | 2 + codex-rs/hooks/src/engine/discovery.rs | 103 +++- codex-rs/hooks/src/engine/dispatcher.rs | 59 ++- codex-rs/hooks/src/engine/mod.rs | 11 + codex-rs/hooks/src/engine/output_parser.rs | 152 ++++++ codex-rs/hooks/src/engine/schema_loader.rs | 12 + codex-rs/hooks/src/events/common.rs | 111 ++++ codex-rs/hooks/src/events/mod.rs | 3 +- codex-rs/hooks/src/events/pre_tool_use.rs | 479 ++++++++++++++++++ codex-rs/hooks/src/lib.rs | 2 + codex-rs/hooks/src/registry.rs | 13 + codex-rs/hooks/src/schema.rs | 112 +++- codex-rs/protocol/src/protocol.rs | 1 + codex-rs/tui/src/chatwidget.rs | 1 + ...tool_use_hook_events_render_snapshot.snap} | 5 +- ...on_start_hook_events_render_snapshot.snap} | 1 - codex-rs/tui/src/chatwidget/tests.rs | 46 +- codex-rs/tui_app_server/src/chatwidget.rs | 1 + ..._tool_use_hook_events_render_snapshot.snap | 9 + ...on_start_hook_events_render_snapshot.snap} | 0 .../tui_app_server/src/chatwidget/tests.rs | 46 +- 38 files changed, 1877 insertions(+), 55 deletions(-) create mode 100644 codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json create mode 100644 codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json create mode 100644 codex-rs/hooks/src/events/pre_tool_use.rs rename codex-rs/{tui_app_server/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_events_render_snapshot.snap => tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap} (59%) rename codex-rs/tui/src/chatwidget/snapshots/{codex_tui__chatwidget__tests__hook_events_render_snapshot.snap => codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap} (91%) create mode 100644 codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap rename codex-rs/tui_app_server/src/chatwidget/snapshots/{codex_tui_app_server__chatwidget__tests__hook_events_render_snapshot.snap => codex_tui_app_server__chatwidget__tests__session_start_hook_events_render_snapshot.snap} (100%) diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 7d192f0b01..b576581cf9 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1180,6 +1180,7 @@ }, "HookEventName": { "enum": [ + "preToolUse", "sessionStart", "userPromptSubmit", "stop" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 6b731a9b50..a644549325 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -7994,6 +7994,7 @@ }, "HookEventName": { "enum": [ + "preToolUse", "sessionStart", "userPromptSubmit", "stop" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 0d69834f15..71e6034529 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -4698,6 +4698,7 @@ }, "HookEventName": { "enum": [ + "preToolUse", "sessionStart", "userPromptSubmit", "stop" diff --git a/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json index 84fea949c8..881c343601 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json @@ -3,6 +3,7 @@ "definitions": { "HookEventName": { "enum": [ + "preToolUse", "sessionStart", "userPromptSubmit", "stop" diff --git a/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json index 7b55420da2..18fdb5008d 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json @@ -3,6 +3,7 @@ "definitions": { "HookEventName": { "enum": [ + "preToolUse", "sessionStart", "userPromptSubmit", "stop" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HookEventName.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HookEventName.ts index a531b78dcf..b75ee3930a 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/HookEventName.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/HookEventName.ts @@ -2,4 +2,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -export type HookEventName = "sessionStart" | "userPromptSubmit" | "stop"; +export type HookEventName = "preToolUse" | "sessionStart" | "userPromptSubmit" | "stop"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index e270d8ad61..3e30f81323 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -377,7 +377,7 @@ v2_enum_from_core!( v2_enum_from_core!( pub enum HookEventName from CoreHookEventName { - SessionStart, UserPromptSubmit, Stop + PreToolUse, SessionStart, UserPromptSubmit, Stop } ); diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 26b49facc3..7e6ecaca10 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -1,6 +1,8 @@ use std::future::Future; use std::sync::Arc; +use codex_hooks::PreToolUseOutcome; +use codex_hooks::PreToolUseRequest; use codex_hooks::SessionStartOutcome; use codex_hooks::UserPromptSubmitOutcome; use codex_hooks::UserPromptSubmitRequest; @@ -109,6 +111,36 @@ pub(crate) async fn run_pending_session_start_hooks( .await } +pub(crate) async fn run_pre_tool_use_hooks( + sess: &Arc, + turn_context: &Arc, + tool_use_id: String, + command: String, +) -> Option { + let request = PreToolUseRequest { + session_id: sess.conversation_id, + turn_id: turn_context.sub_id.clone(), + cwd: turn_context.cwd.clone(), + transcript_path: sess.hook_transcript_path().await, + model: turn_context.model_info.slug.clone(), + permission_mode: hook_permission_mode(turn_context), + tool_name: "Bash".to_string(), + tool_use_id, + command, + }; + let preview_runs = sess.hooks().preview_pre_tool_use(&request); + emit_hook_started_events(sess, turn_context, preview_runs).await; + + let PreToolUseOutcome { + hook_events, + should_block, + block_reason, + } = sess.hooks().run_pre_tool_use(request).await; + emit_hook_completed_events(sess, turn_context, hook_events).await; + + if should_block { block_reason } else { None } +} + pub(crate) async fn run_user_prompt_submit_hooks( sess: &Arc, turn_context: &Arc, diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index bcee62a044..37b7d015b3 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -5,6 +5,7 @@ use std::time::Instant; use crate::client_common::tools::ToolSpec; use crate::function_tool::FunctionCallError; +use crate::hook_runtime::run_pre_tool_use_hooks; use crate::memories::usage::emit_metric_for_tool_read; use crate::protocol::SandboxPolicy; use crate::sandbox_tags::sandbox_tag; @@ -20,7 +21,10 @@ use codex_hooks::HookToolInput; use codex_hooks::HookToolInputLocalShell; use codex_hooks::HookToolKind; use codex_protocol::models::ResponseInputItem; +use codex_protocol::models::ShellCommandToolCallParams; +use codex_protocol::models::ShellToolCallParams; use codex_utils_readiness::Readiness; +use serde::Deserialize; use tracing::warn; #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] @@ -243,6 +247,20 @@ impl ToolRegistry { return Err(FunctionCallError::Fatal(message)); } + if let Some(command) = pre_tool_use_command(tool_name.as_ref(), &invocation.payload) + && let Some(reason) = run_pre_tool_use_hooks( + &invocation.session, + &invocation.turn, + invocation.call_id.clone(), + command.clone(), + ) + .await + { + return Err(FunctionCallError::RespondToModel(format!( + "Bash command blocked by hook: {reason}. Command: {command}" + ))); + } + let is_mutating = handler.is_mutating(&invocation).await; let response_cell = tokio::sync::Mutex::new(None); let invocation_for_tool = invocation.clone(); @@ -413,6 +431,35 @@ fn sandbox_policy_tag(policy: &SandboxPolicy) -> &'static str { } } +#[derive(Deserialize)] +struct PreToolUseExecCommandArgs { + cmd: String, +} + +fn pre_tool_use_command(tool_name: &str, payload: &ToolPayload) -> Option { + match (tool_name, payload) { + ("shell" | "container.exec", ToolPayload::Function { arguments }) => { + serde_json::from_str::(arguments) + .ok() + .map(|params| codex_shell_command::parse_command::shlex_join(¶ms.command)) + } + ("local_shell", ToolPayload::LocalShell { params }) => Some( + codex_shell_command::parse_command::shlex_join(¶ms.command), + ), + ("shell_command", ToolPayload::Function { arguments }) => { + serde_json::from_str::(arguments) + .ok() + .map(|params| params.command) + } + ("exec_command", ToolPayload::Function { arguments }) => { + serde_json::from_str::(arguments) + .ok() + .map(|params| params.cmd) + } + _ => None, + } +} + // Hooks use a separate wire-facing input type so hook payload JSON stays stable // and decoupled from core's internal tool runtime representation. impl From<&ToolPayload> for HookToolInput { diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index 5d9e98df35..46e7d0c3f6 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -1,6 +1,8 @@ use super::*; use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolPayload; use async_trait::async_trait; +use codex_protocol::models::ShellToolCallParams; use pretty_assertions::assert_eq; struct TestHandler; @@ -48,3 +50,63 @@ fn handler_looks_up_namespaced_aliases_explicitly() { .is_some_and(|handler| Arc::ptr_eq(handler, &namespaced_handler)) ); } + +#[test] +fn pre_tool_use_command_uses_raw_shell_command_input() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ "command": "printf shell command" }).to_string(), + }; + + assert_eq!( + pre_tool_use_command("shell_command", &payload), + Some("printf shell command".to_string()) + ); +} + +#[test] +fn pre_tool_use_command_shell_joins_vector_input() { + let payload = ToolPayload::LocalShell { + params: ShellToolCallParams { + command: vec![ + "bash".to_string(), + "-lc".to_string(), + "printf hi".to_string(), + ], + workdir: None, + timeout_ms: None, + sandbox_permissions: None, + prefix_rule: None, + additional_permissions: None, + justification: None, + }, + }; + + assert_eq!( + pre_tool_use_command("local_shell", &payload), + Some("bash -lc 'printf hi'".to_string()) + ); +} + +#[test] +fn pre_tool_use_command_uses_raw_exec_command_input() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ "cmd": "printf exec command" }).to_string(), + }; + + assert_eq!( + pre_tool_use_command("exec_command", &payload), + Some("printf exec command".to_string()) + ); +} + +#[test] +fn pre_tool_use_command_skips_non_shell_tools() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ + "plan": [{ "step": "watch the tide", "status": "pending" }] + }) + .to_string(), + }; + + assert_eq!(pre_tool_use_command("update_plan", &payload), None); +} diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index c7042dd251..c7de5dcb26 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -174,6 +174,69 @@ if payload.get("prompt") == {blocked_prompt_json}: Ok(()) } +fn write_pre_tool_use_hook( + home: &Path, + matcher: Option<&str>, + mode: &str, + reason: &str, +) -> Result<()> { + let script_path = home.join("pre_tool_use_hook.py"); + let log_path = home.join("pre_tool_use_hook_log.jsonl"); + let mode_json = serde_json::to_string(mode).context("serialize pre tool use mode")?; + let reason_json = serde_json::to_string(reason).context("serialize pre tool use reason")?; + let script = format!( + r#"import json +from pathlib import Path +import sys + +log_path = Path(r"{log_path}") +mode = {mode_json} +reason = {reason_json} + +payload = json.load(sys.stdin) + +with log_path.open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +if mode == "json_deny": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason + }} + }})) +elif mode == "exit_2": + sys.stderr.write(reason + "\n") + raise SystemExit(2) +"#, + log_path = log_path.display(), + mode_json = mode_json, + reason_json = reason_json, + ); + + let mut group = serde_json::json!({ + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running pre tool use hook", + }] + }); + if let Some(matcher) = matcher { + group["matcher"] = Value::String(matcher.to_string()); + } + + let hooks = serde_json::json!({ + "hooks": { + "PreToolUse": [group] + } + }); + + fs::write(&script_path, script).context("write pre tool use hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn write_session_start_hook_recording_transcript(home: &Path) -> Result<()> { let script_path = home.join("session_start_hook.py"); let log_path = home.join("session_start_hook_log.jsonl"); @@ -253,6 +316,15 @@ fn read_stop_hook_inputs(home: &Path) -> Result> { .collect() } +fn read_pre_tool_use_hook_inputs(home: &Path) -> Result> { + fs::read_to_string(home.join("pre_tool_use_hook_log.jsonl")) + .context("read pre tool use hook log")? + .lines() + .filter(|line| !line.trim().is_empty()) + .map(|line| serde_json::from_str(line).context("parse pre tool use hook log line")) + .collect() +} + fn read_session_start_hook_inputs(home: &Path) -> Result> { fs::read_to_string(home.join("session_start_hook_log.jsonl")) .context("read session start hook log")? @@ -849,3 +921,357 @@ async fn blocked_queued_prompt_does_not_strand_earlier_accepted_prompt() -> Resu server.shutdown().await; Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-shell-command"; + let marker = std::env::temp_dir().join("pretooluse-shell-command-marker"); + let command = format!("printf blocked > {}", marker.display()); + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + call_id, + "shell_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "hook blocked it"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = + write_pre_tool_use_hook(home, Some("^Bash$"), "json_deny", "blocked by pre hook") + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + if marker.exists() { + fs::remove_file(&marker).context("remove leftover pre tool use marker")?; + } + + test.submit_turn_with_policy( + "run the blocked shell command", + codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ) + .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("shell command output string"); + assert!( + output.contains("Bash command blocked by hook: blocked by pre hook"), + "blocked tool output should surface the hook reason", + ); + assert!( + output.contains(&format!("Command: {command}")), + "blocked tool output should surface the blocked command", + ); + assert!( + !marker.exists(), + "blocked command should not create marker file" + ); + + 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"], "Bash"); + assert_eq!(hook_inputs[0]["tool_use_id"], call_id); + assert_eq!(hook_inputs[0]["tool_input"]["command"], command); + let transcript_path = hook_inputs[0]["transcript_path"] + .as_str() + .expect("pre tool use hook transcript_path"); + assert!( + !transcript_path.is_empty(), + "pre tool use hook should receive a non-empty transcript_path", + ); + assert!( + Path::new(transcript_path).exists(), + "pre tool use hook transcript_path should be materialized on disk", + ); + assert!( + hook_inputs[0]["turn_id"] + .as_str() + .is_some_and(|turn_id| !turn_id.is_empty()) + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_blocks_local_shell_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-local-shell"; + let marker = std::env::temp_dir().join("pretooluse-local-shell-marker"); + let command = vec![ + "/bin/sh".to_string(), + "-c".to_string(), + format!("printf blocked > {}", marker.display()), + ]; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_local_shell_call( + call_id, + "completed", + command.iter().map(String::as_str).collect(), + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "local shell blocked"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = + write_pre_tool_use_hook(home, Some("^Bash$"), "json_deny", "blocked local shell") + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + if marker.exists() { + fs::remove_file(&marker).context("remove leftover local shell marker")?; + } + + test.submit_turn("run the blocked local shell command") + .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("local shell output string"); + assert!( + output.contains("Bash command blocked by hook: blocked local shell"), + "blocked local shell output should surface the hook reason", + ); + assert!( + output.contains(&format!( + "Command: {}", + codex_shell_command::parse_command::shlex_join(&command) + )), + "blocked local shell output should surface the blocked command", + ); + assert!( + !marker.exists(), + "blocked local shell command should not execute" + ); + + 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"]["command"], + codex_shell_command::parse_command::shlex_join(&command), + ); + assert!( + hook_inputs[0]["turn_id"] + .as_str() + .is_some_and(|turn_id| !turn_id.is_empty()) + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_blocks_exec_command_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-exec-command"; + let marker = std::env::temp_dir().join("pretooluse-exec-command-marker"); + let command = format!("printf blocked > {}", marker.display()); + let args = serde_json::json!({ "cmd": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + call_id, + "exec_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "exec command blocked"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = + write_pre_tool_use_hook(home, Some("^Bash$"), "exit_2", "blocked exec command") + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + if marker.exists() { + fs::remove_file(&marker).context("remove leftover exec marker")?; + } + + test.submit_turn("run the blocked exec command").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("exec command output string"); + assert!( + output.contains("Bash command blocked by hook: blocked exec command"), + "blocked exec command output should surface the hook reason", + ); + assert!( + output.contains(&format!("Command: {command}")), + "blocked exec command output should surface the blocked command", + ); + assert!(!marker.exists(), "blocked exec command should not execute"); + + 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_use_id"], call_id); + assert_eq!(hook_inputs[0]["tool_input"]["command"], command); + assert!( + hook_inputs[0]["turn_id"] + .as_str() + .is_some_and(|turn_id| !turn_id.is_empty()) + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_does_not_fire_for_non_shell_tools() -> 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 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_pre_tool_use_hook(home, None, "json_deny", "should not fire") + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + 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 blocked by PreToolUse", + ); + + let hook_log_path = test.codex_home_path().join("pre_tool_use_hook_log.jsonl"); + assert!( + !hook_log_path.exists(), + "non-shell tools should not trigger pre tool use hooks", + ); + + Ok(()) +} diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index ecf6283664..96df3fd2f1 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -973,6 +973,7 @@ async fn handle_response_item_records_tool_result_for_local_shell_call() { .features .disable(Feature::GhostCommit) .expect("test config should allow feature update"); + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); }) .build(&server) .await @@ -989,7 +990,7 @@ async fn handle_response_item_records_tool_result_for_local_shell_call() { .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::TokenCount(_))).await; + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; logs_assert(|lines: &[&str]| { let line = lines diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 0e49166b81..2b935fce95 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -988,6 +988,7 @@ impl EventProcessorWithHumanOutput { fn hook_event_name(event_name: HookEventName) -> &'static str { match event_name { + HookEventName::PreToolUse => "PreToolUse", HookEventName::SessionStart => "SessionStart", HookEventName::UserPromptSubmit => "UserPromptSubmit", HookEventName::Stop => "Stop", diff --git a/codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json b/codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json new file mode 100644 index 0000000000..86dfac165c --- /dev/null +++ b/codex-rs/hooks/schema/generated/pre-tool-use.command.input.schema.json @@ -0,0 +1,80 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "additionalProperties": false, + "definitions": { + "NullableString": { + "type": [ + "string", + "null" + ] + }, + "PreToolUseToolInput": { + "additionalProperties": false, + "properties": { + "command": { + "type": "string" + } + }, + "required": [ + "command" + ], + "type": "object" + } + }, + "properties": { + "cwd": { + "type": "string" + }, + "hook_event_name": { + "const": "PreToolUse", + "type": "string" + }, + "model": { + "type": "string" + }, + "permission_mode": { + "enum": [ + "default", + "acceptEdits", + "plan", + "dontAsk", + "bypassPermissions" + ], + "type": "string" + }, + "session_id": { + "type": "string" + }, + "tool_input": { + "$ref": "#/definitions/PreToolUseToolInput" + }, + "tool_name": { + "const": "Bash", + "type": "string" + }, + "tool_use_id": { + "type": "string" + }, + "transcript_path": { + "$ref": "#/definitions/NullableString" + }, + "turn_id": { + "description": "Codex extension: expose the active turn id to internal turn-scoped hooks.", + "type": "string" + } + }, + "required": [ + "cwd", + "hook_event_name", + "model", + "permission_mode", + "session_id", + "tool_input", + "tool_name", + "tool_use_id", + "transcript_path", + "turn_id" + ], + "title": "pre-tool-use.command.input", + "type": "object" +} \ No newline at end of file diff --git a/codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json b/codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json new file mode 100644 index 0000000000..0992983fe4 --- /dev/null +++ b/codex-rs/hooks/schema/generated/pre-tool-use.command.output.schema.json @@ -0,0 +1,101 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "additionalProperties": false, + "definitions": { + "HookEventNameWire": { + "enum": [ + "PreToolUse", + "SessionStart", + "UserPromptSubmit", + "Stop" + ], + "type": "string" + }, + "PreToolUseDecisionWire": { + "enum": [ + "approve", + "block" + ], + "type": "string" + }, + "PreToolUseHookSpecificOutputWire": { + "additionalProperties": false, + "properties": { + "additionalContext": { + "default": null, + "type": "string" + }, + "hookEventName": { + "$ref": "#/definitions/HookEventNameWire" + }, + "permissionDecision": { + "allOf": [ + { + "$ref": "#/definitions/PreToolUsePermissionDecisionWire" + } + ], + "default": null + }, + "permissionDecisionReason": { + "default": null, + "type": "string" + }, + "updatedInput": { + "default": null + } + }, + "required": [ + "hookEventName" + ], + "type": "object" + }, + "PreToolUsePermissionDecisionWire": { + "enum": [ + "allow", + "deny", + "ask" + ], + "type": "string" + } + }, + "properties": { + "continue": { + "default": true, + "type": "boolean" + }, + "decision": { + "allOf": [ + { + "$ref": "#/definitions/PreToolUseDecisionWire" + } + ], + "default": null + }, + "hookSpecificOutput": { + "allOf": [ + { + "$ref": "#/definitions/PreToolUseHookSpecificOutputWire" + } + ], + "default": null + }, + "reason": { + "default": null, + "type": "string" + }, + "stopReason": { + "default": null, + "type": "string" + }, + "suppressOutput": { + "default": false, + "type": "boolean" + }, + "systemMessage": { + "default": null, + "type": "string" + } + }, + "title": "pre-tool-use.command.output", + "type": "object" +} \ No newline at end of file diff --git a/codex-rs/hooks/schema/generated/session-start.command.output.schema.json b/codex-rs/hooks/schema/generated/session-start.command.output.schema.json index 292777ff67..f44928983d 100644 --- a/codex-rs/hooks/schema/generated/session-start.command.output.schema.json +++ b/codex-rs/hooks/schema/generated/session-start.command.output.schema.json @@ -4,6 +4,7 @@ "definitions": { "HookEventNameWire": { "enum": [ + "PreToolUse", "SessionStart", "UserPromptSubmit", "Stop" diff --git a/codex-rs/hooks/schema/generated/user-prompt-submit.command.output.schema.json b/codex-rs/hooks/schema/generated/user-prompt-submit.command.output.schema.json index c6935aa6da..27878752c1 100644 --- a/codex-rs/hooks/schema/generated/user-prompt-submit.command.output.schema.json +++ b/codex-rs/hooks/schema/generated/user-prompt-submit.command.output.schema.json @@ -10,6 +10,7 @@ }, "HookEventNameWire": { "enum": [ + "PreToolUse", "SessionStart", "UserPromptSubmit", "Stop" diff --git a/codex-rs/hooks/src/engine/config.rs b/codex-rs/hooks/src/engine/config.rs index 0d9357e392..1a2d962bc8 100644 --- a/codex-rs/hooks/src/engine/config.rs +++ b/codex-rs/hooks/src/engine/config.rs @@ -8,6 +8,8 @@ pub(crate) struct HooksFile { #[derive(Debug, Default, Deserialize)] pub(crate) struct HookEvents { + #[serde(rename = "PreToolUse", default)] + pub pre_tool_use: Vec, #[serde(rename = "SessionStart", default)] pub session_start: Vec, #[serde(rename = "UserPromptSubmit", default)] diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index db0f38c645..55dfca63cd 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -3,11 +3,12 @@ use std::path::Path; use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; -use regex::Regex; use super::ConfiguredHandler; use super::config::HookHandlerConfig; use super::config::HooksFile; +use crate::events::common::matcher_pattern_for_event; +use crate::events::common::validate_matcher_pattern; pub(crate) struct DiscoveryResult { pub handlers: Vec, @@ -69,6 +70,21 @@ pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) - } }; + for group in parsed.hooks.pre_tool_use { + append_group_handlers( + &mut handlers, + &mut warnings, + &mut display_order, + source_path.as_path(), + codex_protocol::protocol::HookEventName::PreToolUse, + matcher_pattern_for_event( + codex_protocol::protocol::HookEventName::PreToolUse, + group.matcher.as_deref(), + ), + group.hooks, + ); + } + for group in parsed.hooks.session_start { append_group_handlers( &mut handlers, @@ -76,7 +92,7 @@ pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) - &mut display_order, source_path.as_path(), codex_protocol::protocol::HookEventName::SessionStart, - effective_matcher( + matcher_pattern_for_event( codex_protocol::protocol::HookEventName::SessionStart, group.matcher.as_deref(), ), @@ -91,7 +107,7 @@ pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) - &mut display_order, source_path.as_path(), codex_protocol::protocol::HookEventName::UserPromptSubmit, - effective_matcher( + matcher_pattern_for_event( codex_protocol::protocol::HookEventName::UserPromptSubmit, group.matcher.as_deref(), ), @@ -106,7 +122,7 @@ pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) - &mut display_order, source_path.as_path(), codex_protocol::protocol::HookEventName::Stop, - effective_matcher( + matcher_pattern_for_event( codex_protocol::protocol::HookEventName::Stop, group.matcher.as_deref(), ), @@ -118,17 +134,6 @@ pub(crate) fn discover_handlers(config_layer_stack: Option<&ConfigLayerStack>) - DiscoveryResult { handlers, warnings } } -fn effective_matcher( - event_name: codex_protocol::protocol::HookEventName, - matcher: Option<&str>, -) -> Option<&str> { - match event_name { - codex_protocol::protocol::HookEventName::SessionStart => matcher, - codex_protocol::protocol::HookEventName::UserPromptSubmit - | codex_protocol::protocol::HookEventName::Stop => None, - } -} - fn append_group_handlers( handlers: &mut Vec, warnings: &mut Vec, @@ -139,7 +144,7 @@ fn append_group_handlers( group_handlers: Vec, ) { if let Some(matcher) = matcher - && let Err(err) = Regex::new(matcher) + && let Err(err) = validate_matcher_pattern(matcher) { warnings.push(format!( "invalid matcher {matcher:?} in {}: {err}", @@ -205,7 +210,7 @@ mod tests { use super::ConfiguredHandler; use super::HookHandlerConfig; use super::append_group_handlers; - use super::effective_matcher; + use crate::events::common::matcher_pattern_for_event; #[test] fn user_prompt_submit_ignores_invalid_matcher_during_discovery() { @@ -219,7 +224,7 @@ mod tests { &mut display_order, Path::new("/tmp/hooks.json"), HookEventName::UserPromptSubmit, - effective_matcher(HookEventName::UserPromptSubmit, Some("[")), + matcher_pattern_for_event(HookEventName::UserPromptSubmit, Some("[")), vec![HookHandlerConfig::Command { command: "echo hello".to_string(), timeout_sec: None, @@ -242,4 +247,66 @@ mod tests { }] ); } + + #[test] + fn pre_tool_use_keeps_valid_matcher_during_discovery() { + let mut handlers = Vec::new(); + let mut warnings = Vec::new(); + let mut display_order = 0; + + append_group_handlers( + &mut handlers, + &mut warnings, + &mut display_order, + Path::new("/tmp/hooks.json"), + HookEventName::PreToolUse, + matcher_pattern_for_event(HookEventName::PreToolUse, Some("^Bash$")), + vec![HookHandlerConfig::Command { + command: "echo hello".to_string(), + timeout_sec: None, + r#async: false, + status_message: None, + }], + ); + + assert_eq!(warnings, Vec::::new()); + assert_eq!( + handlers, + vec![ConfiguredHandler { + event_name: HookEventName::PreToolUse, + matcher: Some("^Bash$".to_string()), + command: "echo hello".to_string(), + timeout_sec: 600, + status_message: None, + source_path: PathBuf::from("/tmp/hooks.json"), + display_order: 0, + }] + ); + } + + #[test] + fn pre_tool_use_treats_star_matcher_as_match_all() { + let mut handlers = Vec::new(); + let mut warnings = Vec::new(); + let mut display_order = 0; + + append_group_handlers( + &mut handlers, + &mut warnings, + &mut display_order, + Path::new("/tmp/hooks.json"), + HookEventName::PreToolUse, + matcher_pattern_for_event(HookEventName::PreToolUse, Some("*")), + vec![HookHandlerConfig::Command { + command: "echo hello".to_string(), + timeout_sec: None, + r#async: false, + status_message: None, + }], + ); + + assert_eq!(warnings, Vec::::new()); + assert_eq!(handlers.len(), 1); + assert_eq!(handlers[0].matcher.as_deref(), Some("*")); + } } diff --git a/codex-rs/hooks/src/engine/dispatcher.rs b/codex-rs/hooks/src/engine/dispatcher.rs index e316d9af98..0b29e12fa0 100644 --- a/codex-rs/hooks/src/engine/dispatcher.rs +++ b/codex-rs/hooks/src/engine/dispatcher.rs @@ -14,6 +14,7 @@ use super::CommandShell; use super::ConfiguredHandler; use super::command_runner::CommandRunResult; use super::command_runner::run_command; +use crate::events::common::matches_matcher; #[derive(Debug)] pub(crate) struct ParsedHandler { @@ -30,13 +31,9 @@ pub(crate) fn select_handlers( .iter() .filter(|handler| handler.event_name == event_name) .filter(|handler| match event_name { - HookEventName::SessionStart => match (&handler.matcher, matcher_input) { - (Some(matcher), Some(input)) => regex::Regex::new(matcher) - .map(|regex| regex.is_match(input)) - .unwrap_or(false), - (None, _) => true, - _ => false, - }, + HookEventName::PreToolUse | HookEventName::SessionStart => { + matches_matcher(handler.matcher.as_deref(), matcher_input) + } HookEventName::UserPromptSubmit | HookEventName::Stop => true, }) .cloned() @@ -109,7 +106,9 @@ pub(crate) fn completed_summary( fn scope_for_event(event_name: HookEventName) -> HookScope { match event_name { HookEventName::SessionStart => HookScope::Thread, - HookEventName::UserPromptSubmit | HookEventName::Stop => HookScope::Turn, + HookEventName::PreToolUse | HookEventName::UserPromptSubmit | HookEventName::Stop => { + HookScope::Turn + } } } @@ -172,6 +171,50 @@ mod tests { assert_eq!(selected[1].display_order, 1); } + #[test] + fn pre_tool_use_matches_tool_name() { + let handlers = vec![ + make_handler(HookEventName::PreToolUse, Some("^Bash$"), "echo same", 0), + make_handler(HookEventName::PreToolUse, Some("^Edit$"), "echo same", 1), + ]; + + let selected = select_handlers(&handlers, HookEventName::PreToolUse, Some("Bash")); + + assert_eq!(selected.len(), 1); + assert_eq!(selected[0].display_order, 0); + } + + #[test] + fn pre_tool_use_star_matcher_matches_all_tools() { + let handlers = vec![ + make_handler(HookEventName::PreToolUse, Some("*"), "echo same", 0), + make_handler(HookEventName::PreToolUse, Some("^Edit$"), "echo same", 1), + ]; + + let selected = select_handlers(&handlers, HookEventName::PreToolUse, Some("Bash")); + + assert_eq!(selected.len(), 1); + assert_eq!(selected[0].display_order, 0); + } + + #[test] + fn pre_tool_use_regex_alternation_matches_each_tool_name() { + let handlers = vec![make_handler( + HookEventName::PreToolUse, + Some("Edit|Write"), + "echo same", + 0, + )]; + + let selected_edit = select_handlers(&handlers, HookEventName::PreToolUse, Some("Edit")); + let selected_write = select_handlers(&handlers, HookEventName::PreToolUse, Some("Write")); + let selected_bash = select_handlers(&handlers, HookEventName::PreToolUse, Some("Bash")); + + assert_eq!(selected_edit.len(), 1); + assert_eq!(selected_write.len(), 1); + assert_eq!(selected_bash.len(), 0); + } + #[test] fn user_prompt_submit_ignores_matcher() { let handlers = vec![ diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index 24ff72990e..f54403b74b 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -10,6 +10,8 @@ use std::path::PathBuf; use codex_config::ConfigLayerStack; use codex_protocol::protocol::HookRunSummary; +use crate::events::pre_tool_use::PreToolUseOutcome; +use crate::events::pre_tool_use::PreToolUseRequest; use crate::events::session_start::SessionStartOutcome; use crate::events::session_start::SessionStartRequest; use crate::events::stop::StopOutcome; @@ -46,6 +48,7 @@ impl ConfiguredHandler { fn event_name_label(&self) -> &'static str { match self.event_name { + codex_protocol::protocol::HookEventName::PreToolUse => "pre-tool-use", codex_protocol::protocol::HookEventName::SessionStart => "session-start", codex_protocol::protocol::HookEventName::UserPromptSubmit => "user-prompt-submit", codex_protocol::protocol::HookEventName::Stop => "stop", @@ -105,6 +108,10 @@ impl ClaudeHooksEngine { crate::events::session_start::preview(&self.handlers, request) } + pub(crate) fn preview_pre_tool_use(&self, request: &PreToolUseRequest) -> Vec { + crate::events::pre_tool_use::preview(&self.handlers, request) + } + pub(crate) async fn run_session_start( &self, request: SessionStartRequest, @@ -113,6 +120,10 @@ impl ClaudeHooksEngine { crate::events::session_start::run(&self.handlers, &self.shell, request, turn_id).await } + pub(crate) async fn run_pre_tool_use(&self, request: PreToolUseRequest) -> PreToolUseOutcome { + crate::events::pre_tool_use::run(&self.handlers, &self.shell, request).await + } + pub(crate) fn preview_user_prompt_submit( &self, request: &UserPromptSubmitRequest, diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index d72ae07155..3fc0e7a0ba 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -12,6 +12,13 @@ pub(crate) struct SessionStartOutput { pub additional_context: Option, } +#[derive(Debug, Clone)] +pub(crate) struct PreToolUseOutput { + pub universal: UniversalOutput, + pub block_reason: Option, + pub invalid_reason: Option, +} + #[derive(Debug, Clone)] pub(crate) struct UserPromptSubmitOutput { pub universal: UniversalOutput, @@ -31,6 +38,9 @@ pub(crate) struct StopOutput { use crate::schema::BlockDecisionWire; use crate::schema::HookUniversalOutputWire; +use crate::schema::PreToolUseCommandOutputWire; +use crate::schema::PreToolUseDecisionWire; +use crate::schema::PreToolUsePermissionDecisionWire; use crate::schema::SessionStartCommandOutputWire; use crate::schema::StopCommandOutputWire; use crate::schema::UserPromptSubmitCommandOutputWire; @@ -46,6 +56,54 @@ pub(crate) fn parse_session_start(stdout: &str) -> Option { }) } +pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { + let PreToolUseCommandOutputWire { + universal: universal_wire, + decision, + reason, + hook_specific_output, + } = parse_json(stdout)?; + let universal = UniversalOutput::from(universal_wire); + let hook_specific_output = hook_specific_output.as_ref(); + let use_hook_specific_decision = hook_specific_output.is_some_and(|output| { + output.permission_decision.is_some() + || output.permission_decision_reason.is_some() + || output.updated_input.is_some() + || output.additional_context.is_some() + }); + let invalid_reason = unsupported_pre_tool_use_universal(&universal).or_else(|| { + if use_hook_specific_decision { + hook_specific_output.and_then(unsupported_pre_tool_use_hook_specific_output) + } else { + unsupported_pre_tool_use_legacy_decision(decision.as_ref(), reason.as_deref()) + } + }); + let block_reason = if invalid_reason.is_none() { + if use_hook_specific_decision { + hook_specific_output.and_then(|output| match output.permission_decision { + Some(PreToolUsePermissionDecisionWire::Deny) => output + .permission_decision_reason + .as_deref() + .and_then(trimmed_reason), + _ => None, + }) + } else { + match decision.as_ref() { + Some(PreToolUseDecisionWire::Block) => reason.as_deref().and_then(trimmed_reason), + Some(PreToolUseDecisionWire::Approve) | None => None, + } + } + } else { + None + }; + + Some(PreToolUseOutput { + universal, + block_reason, + invalid_reason, + }) +} + pub(crate) fn parse_user_prompt_submit(stdout: &str) -> Option { let wire: UserPromptSubmitCommandOutputWire = parse_json(stdout)?; let should_block = matches!(wire.decision, Some(BlockDecisionWire::Block)); @@ -119,3 +177,97 @@ where fn invalid_block_message(event_name: &str) -> String { format!("{event_name} hook returned decision:block without a non-empty reason") } + +fn unsupported_pre_tool_use_universal(universal: &UniversalOutput) -> Option { + if !universal.continue_processing { + Some("PreToolUse hook returned unsupported continue:false".to_string()) + } else if universal.stop_reason.is_some() { + Some("PreToolUse hook returned unsupported stopReason".to_string()) + } else if universal.suppress_output { + Some("PreToolUse hook returned unsupported suppressOutput".to_string()) + } else { + None + } +} + +fn unsupported_pre_tool_use_hook_specific_output( + output: &crate::schema::PreToolUseHookSpecificOutputWire, +) -> Option { + if output.updated_input.is_some() { + Some("PreToolUse hook returned unsupported updatedInput".to_string()) + } else if output + .additional_context + .as_deref() + .and_then(trimmed_reason) + .is_some() + { + Some("PreToolUse hook returned unsupported additionalContext".to_string()) + } else { + match output.permission_decision { + Some(PreToolUsePermissionDecisionWire::Allow) => { + Some("PreToolUse hook returned unsupported permissionDecision:allow".to_string()) + } + Some(PreToolUsePermissionDecisionWire::Ask) => { + Some("PreToolUse hook returned unsupported permissionDecision:ask".to_string()) + } + Some(PreToolUsePermissionDecisionWire::Deny) => { + if output + .permission_decision_reason + .as_deref() + .and_then(trimmed_reason) + .is_none() + { + Some(invalid_pre_tool_use_reason_message()) + } else { + None + } + } + None => { + if output.permission_decision_reason.is_some() { + Some("PreToolUse hook returned permissionDecisionReason without permissionDecision".to_string()) + } else { + None + } + } + } + } +} + +fn unsupported_pre_tool_use_legacy_decision( + decision: Option<&PreToolUseDecisionWire>, + reason: Option<&str>, +) -> Option { + match decision { + Some(PreToolUseDecisionWire::Approve) => { + Some("PreToolUse hook returned unsupported decision:approve".to_string()) + } + Some(PreToolUseDecisionWire::Block) => { + if reason.and_then(trimmed_reason).is_none() { + Some(invalid_block_message("PreToolUse")) + } else { + None + } + } + None => { + if reason.is_some() { + Some("PreToolUse hook returned reason without decision".to_string()) + } else { + None + } + } + } +} + +fn invalid_pre_tool_use_reason_message() -> String { + "PreToolUse hook returned permissionDecision:deny without a non-empty permissionDecisionReason" + .to_string() +} + +fn trimmed_reason(reason: &str) -> Option { + let trimmed = reason.trim(); + if trimmed.is_empty() { + None + } else { + Some(trimmed.to_string()) + } +} diff --git a/codex-rs/hooks/src/engine/schema_loader.rs b/codex-rs/hooks/src/engine/schema_loader.rs index 2ad54e5062..1a51e59527 100644 --- a/codex-rs/hooks/src/engine/schema_loader.rs +++ b/codex-rs/hooks/src/engine/schema_loader.rs @@ -4,6 +4,8 @@ use serde_json::Value; #[allow(dead_code)] pub(crate) struct GeneratedHookSchemas { + pub pre_tool_use_command_input: Value, + pub pre_tool_use_command_output: Value, pub session_start_command_input: Value, pub session_start_command_output: Value, pub user_prompt_submit_command_input: Value, @@ -15,6 +17,14 @@ pub(crate) struct GeneratedHookSchemas { pub(crate) fn generated_hook_schemas() -> &'static GeneratedHookSchemas { static SCHEMAS: OnceLock = OnceLock::new(); SCHEMAS.get_or_init(|| GeneratedHookSchemas { + pre_tool_use_command_input: parse_json_schema( + "pre-tool-use.command.input", + include_str!("../../schema/generated/pre-tool-use.command.input.schema.json"), + ), + pre_tool_use_command_output: parse_json_schema( + "pre-tool-use.command.output", + include_str!("../../schema/generated/pre-tool-use.command.output.schema.json"), + ), session_start_command_input: parse_json_schema( "session-start.command.input", include_str!("../../schema/generated/session-start.command.input.schema.json"), @@ -56,6 +66,8 @@ mod tests { fn loads_generated_hook_schemas() { let schemas = generated_hook_schemas(); + assert_eq!(schemas.pre_tool_use_command_input["type"], "object"); + assert_eq!(schemas.pre_tool_use_command_output["type"], "object"); assert_eq!(schemas.session_start_command_input["type"], "object"); assert_eq!(schemas.session_start_command_output["type"], "object"); assert_eq!(schemas.user_prompt_submit_command_input["type"], "object"); diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index b6358e068a..b4c274bbd3 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -1,4 +1,5 @@ use codex_protocol::protocol::HookCompletedEvent; +use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookOutputEntry; use codex_protocol::protocol::HookOutputEntryKind; use codex_protocol::protocol::HookRunStatus; @@ -67,3 +68,113 @@ pub(crate) fn serialization_failure_hook_events( }) .collect() } + +pub(crate) fn matcher_pattern_for_event( + event_name: HookEventName, + matcher: Option<&str>, +) -> Option<&str> { + match event_name { + HookEventName::PreToolUse | HookEventName::SessionStart => matcher, + HookEventName::UserPromptSubmit | HookEventName::Stop => None, + } +} + +pub(crate) fn validate_matcher_pattern(matcher: &str) -> Result<(), regex::Error> { + if is_match_all_matcher(matcher) { + return Ok(()); + } + regex::Regex::new(matcher).map(|_| ()) +} + +pub(crate) fn matches_matcher(matcher: Option<&str>, input: Option<&str>) -> bool { + match matcher { + None => true, + Some(matcher) if is_match_all_matcher(matcher) => true, + Some(matcher) => input + .and_then(|input| { + regex::Regex::new(matcher) + .ok() + .map(|regex| regex.is_match(input)) + }) + .unwrap_or(false), + } +} + +fn is_match_all_matcher(matcher: &str) -> bool { + matcher.is_empty() || matcher == "*" +} + +#[cfg(test)] +mod tests { + use codex_protocol::protocol::HookEventName; + use pretty_assertions::assert_eq; + + use super::matcher_pattern_for_event; + use super::matches_matcher; + use super::validate_matcher_pattern; + + #[test] + fn matcher_omitted_matches_all_occurrences() { + assert!(matches_matcher(None, Some("Bash"))); + assert!(matches_matcher(None, Some("Write"))); + } + + #[test] + fn matcher_star_matches_all_occurrences() { + assert!(matches_matcher(Some("*"), Some("Bash"))); + assert!(matches_matcher(Some("*"), Some("Edit"))); + assert_eq!(validate_matcher_pattern("*"), Ok(())); + } + + #[test] + fn matcher_empty_string_matches_all_occurrences() { + assert!(matches_matcher(Some(""), Some("Bash"))); + assert!(matches_matcher(Some(""), Some("SessionStart"))); + assert_eq!(validate_matcher_pattern(""), Ok(())); + } + + #[test] + fn matcher_uses_regex_matching() { + assert!(matches_matcher(Some("Edit|Write"), Some("Edit"))); + assert!(matches_matcher(Some("Edit|Write"), Some("Write"))); + assert!(!matches_matcher(Some("Edit|Write"), Some("Bash"))); + assert_eq!(validate_matcher_pattern("Edit|Write"), Ok(())); + } + + #[test] + fn matcher_supports_anchored_regexes() { + assert!(matches_matcher(Some("^Bash$"), Some("Bash"))); + assert!(!matches_matcher(Some("^Bash$"), Some("BashOutput"))); + assert_eq!(validate_matcher_pattern("^Bash$"), Ok(())); + } + + #[test] + fn invalid_regex_is_rejected() { + assert!(validate_matcher_pattern("[").is_err()); + assert!(!matches_matcher(Some("["), Some("Bash"))); + } + + #[test] + fn unsupported_events_ignore_matchers() { + assert_eq!( + matcher_pattern_for_event(HookEventName::UserPromptSubmit, Some("^hello")), + None + ); + assert_eq!( + matcher_pattern_for_event(HookEventName::Stop, Some("^done$")), + None + ); + } + + #[test] + fn supported_events_keep_matchers() { + assert_eq!( + matcher_pattern_for_event(HookEventName::PreToolUse, Some("Bash")), + Some("Bash") + ); + assert_eq!( + matcher_pattern_for_event(HookEventName::SessionStart, Some("startup|resume")), + Some("startup|resume") + ); + } +} diff --git a/codex-rs/hooks/src/events/mod.rs b/codex-rs/hooks/src/events/mod.rs index 3bb54699af..603395eb5a 100644 --- a/codex-rs/hooks/src/events/mod.rs +++ b/codex-rs/hooks/src/events/mod.rs @@ -1,4 +1,5 @@ -mod common; +pub(crate) mod common; +pub mod pre_tool_use; pub mod session_start; pub mod stop; pub mod user_prompt_submit; diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs new file mode 100644 index 0000000000..8366bb632c --- /dev/null +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -0,0 +1,479 @@ +use std::path::PathBuf; + +use codex_protocol::ThreadId; +use codex_protocol::protocol::HookCompletedEvent; +use codex_protocol::protocol::HookEventName; +use codex_protocol::protocol::HookOutputEntry; +use codex_protocol::protocol::HookOutputEntryKind; +use codex_protocol::protocol::HookRunStatus; +use codex_protocol::protocol::HookRunSummary; + +use super::common; +use crate::engine::CommandShell; +use crate::engine::ConfiguredHandler; +use crate::engine::command_runner::CommandRunResult; +use crate::engine::dispatcher; +use crate::engine::output_parser; +use crate::schema::PreToolUseCommandInput; + +#[derive(Debug, Clone)] +pub struct PreToolUseRequest { + pub session_id: ThreadId, + pub turn_id: String, + pub cwd: PathBuf, + pub transcript_path: Option, + pub model: String, + pub permission_mode: String, + pub tool_name: String, + pub tool_use_id: String, + pub command: String, +} + +#[derive(Debug)] +pub struct PreToolUseOutcome { + pub hook_events: Vec, + pub should_block: bool, + pub block_reason: Option, +} + +#[derive(Debug, Default, PartialEq, Eq)] +struct PreToolUseHandlerData { + should_block: bool, + block_reason: Option, +} + +pub(crate) fn preview( + handlers: &[ConfiguredHandler], + request: &PreToolUseRequest, +) -> Vec { + dispatcher::select_handlers( + handlers, + HookEventName::PreToolUse, + Some(&request.tool_name), + ) + .into_iter() + .map(|handler| dispatcher::running_summary(&handler)) + .collect() +} + +pub(crate) async fn run( + handlers: &[ConfiguredHandler], + shell: &CommandShell, + request: PreToolUseRequest, +) -> PreToolUseOutcome { + let matched = dispatcher::select_handlers( + handlers, + HookEventName::PreToolUse, + Some(&request.tool_name), + ); + if matched.is_empty() { + return PreToolUseOutcome { + hook_events: Vec::new(), + should_block: false, + block_reason: None, + }; + } + + let input_json = match serde_json::to_string(&PreToolUseCommandInput { + session_id: request.session_id.to_string(), + turn_id: request.turn_id.clone(), + transcript_path: crate::schema::NullableString::from_path(request.transcript_path.clone()), + cwd: request.cwd.display().to_string(), + hook_event_name: "PreToolUse".to_string(), + model: request.model.clone(), + permission_mode: request.permission_mode.clone(), + tool_name: "Bash".to_string(), + tool_input: crate::schema::PreToolUseToolInput { + command: request.command.clone(), + }, + tool_use_id: request.tool_use_id.clone(), + }) { + Ok(input_json) => input_json, + Err(error) => { + return serialization_failure_outcome(common::serialization_failure_hook_events( + matched, + Some(request.turn_id), + format!("failed to serialize pre tool use hook input: {error}"), + )); + } + }; + + let results = dispatcher::execute_handlers( + shell, + matched, + input_json, + request.cwd.as_path(), + Some(request.turn_id), + parse_completed, + ) + .await; + + let should_block = results.iter().any(|result| result.data.should_block); + let block_reason = results + .iter() + .find_map(|result| result.data.block_reason.clone()); + + PreToolUseOutcome { + hook_events: results.into_iter().map(|result| result.completed).collect(), + should_block, + block_reason, + } +} + +fn parse_completed( + handler: &ConfiguredHandler, + run_result: CommandRunResult, + turn_id: Option, +) -> dispatcher::ParsedHandler { + let mut entries = Vec::new(); + let mut status = HookRunStatus::Completed; + let mut should_block = false; + let mut block_reason = None; + + match run_result.error.as_deref() { + Some(error) => { + status = HookRunStatus::Failed; + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: error.to_string(), + }); + } + None => match run_result.exit_code { + Some(0) => { + let trimmed_stdout = run_result.stdout.trim(); + if trimmed_stdout.is_empty() { + } else if let Some(parsed) = output_parser::parse_pre_tool_use(&run_result.stdout) { + if let Some(system_message) = parsed.universal.system_message { + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Warning, + text: system_message, + }); + } + if let Some(invalid_reason) = parsed.invalid_reason { + status = HookRunStatus::Failed; + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: invalid_reason, + }); + } else if let Some(reason) = parsed.block_reason { + status = HookRunStatus::Blocked; + should_block = true; + block_reason = Some(reason.clone()); + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: reason, + }); + } + } else if trimmed_stdout.starts_with('{') || trimmed_stdout.starts_with('[') { + status = HookRunStatus::Failed; + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "hook returned invalid pre-tool-use JSON output".to_string(), + }); + } + } + Some(2) => { + if let Some(reason) = common::trimmed_non_empty(&run_result.stderr) { + status = HookRunStatus::Blocked; + should_block = true; + block_reason = Some(reason.clone()); + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: reason, + }); + } else { + status = HookRunStatus::Failed; + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "PreToolUse hook exited with code 2 but did not write a blocking reason to stderr".to_string(), + }); + } + } + Some(exit_code) => { + status = HookRunStatus::Failed; + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: format!("hook exited with code {exit_code}"), + }); + } + None => { + status = HookRunStatus::Failed; + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "hook exited without a status code".to_string(), + }); + } + }, + } + + let completed = HookCompletedEvent { + turn_id, + run: dispatcher::completed_summary(handler, &run_result, status, entries), + }; + + dispatcher::ParsedHandler { + completed, + data: PreToolUseHandlerData { + should_block, + block_reason, + }, + } +} + +fn serialization_failure_outcome(hook_events: Vec) -> PreToolUseOutcome { + PreToolUseOutcome { + hook_events, + should_block: false, + block_reason: None, + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use codex_protocol::protocol::HookEventName; + use codex_protocol::protocol::HookOutputEntry; + use codex_protocol::protocol::HookOutputEntryKind; + use codex_protocol::protocol::HookRunStatus; + use pretty_assertions::assert_eq; + + use super::PreToolUseHandlerData; + use super::parse_completed; + use crate::engine::ConfiguredHandler; + use crate::engine::command_runner::CommandRunResult; + + #[test] + fn permission_decision_deny_blocks_processing() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":"do not run that"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: true, + block_reason: Some("do not run that".to_string()), + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: "do not run that".to_string(), + }] + ); + } + + #[test] + fn deprecated_block_decision_blocks_processing() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"decision":"block","reason":"do not run that"}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: true, + block_reason: Some("do not run that".to_string()), + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: "do not run that".to_string(), + }] + ); + } + + #[test] + fn unsupported_permission_decision_fails_open() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"please confirm"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "PreToolUse hook returned unsupported permissionDecision:ask".to_string(), + }] + ); + } + + #[test] + fn deprecated_approve_decision_fails_open() { + let parsed = parse_completed( + &handler(), + run_result(Some(0), r#"{"decision":"approve"}"#, ""), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "PreToolUse hook returned unsupported decision:approve".to_string(), + }] + ); + } + + #[test] + fn unsupported_additional_context_fails_open() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":"do not run that","additionalContext":"nope"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "PreToolUse hook returned unsupported additionalContext".to_string(), + }] + ); + } + + #[test] + fn plain_stdout_is_ignored() { + let parsed = parse_completed( + &handler(), + run_result(Some(0), "hook ran successfully\n", ""), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); + assert_eq!(parsed.completed.run.entries, vec![]); + } + + #[test] + fn invalid_json_like_stdout_fails_instead_of_becoming_noop() { + let parsed = parse_completed( + &handler(), + run_result(Some(0), "{\"decision\":\n", ""), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: false, + block_reason: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Error, + text: "hook returned invalid pre-tool-use JSON output".to_string(), + }] + ); + } + + #[test] + fn exit_code_two_blocks_processing() { + let parsed = parse_completed( + &handler(), + run_result(Some(2), "", "blocked by policy\n"), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: true, + block_reason: Some("blocked by policy".to_string()), + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: "blocked by policy".to_string(), + }] + ); + } + + fn handler() -> ConfiguredHandler { + ConfiguredHandler { + event_name: HookEventName::PreToolUse, + matcher: Some("^Bash$".to_string()), + command: "echo hook".to_string(), + timeout_sec: 5, + status_message: None, + source_path: PathBuf::from("/tmp/hooks.json"), + display_order: 0, + } + } + + fn run_result(exit_code: Option, stdout: &str, stderr: &str) -> CommandRunResult { + CommandRunResult { + started_at: 1, + completed_at: 2, + duration_ms: 1, + exit_code, + stdout: stdout.to_string(), + stderr: stderr.to_string(), + error: None, + } + } +} diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 768a24c5e3..86b39b0b7d 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -5,6 +5,8 @@ mod registry; mod schema; mod types; +pub use events::pre_tool_use::PreToolUseOutcome; +pub use events::pre_tool_use::PreToolUseRequest; pub use events::session_start::SessionStartOutcome; pub use events::session_start::SessionStartRequest; pub use events::session_start::SessionStartSource; diff --git a/codex-rs/hooks/src/registry.rs b/codex-rs/hooks/src/registry.rs index 3b63bda8c3..440af648dd 100644 --- a/codex-rs/hooks/src/registry.rs +++ b/codex-rs/hooks/src/registry.rs @@ -3,6 +3,8 @@ use tokio::process::Command; use crate::engine::ClaudeHooksEngine; use crate::engine::CommandShell; +use crate::events::pre_tool_use::PreToolUseOutcome; +use crate::events::pre_tool_use::PreToolUseRequest; use crate::events::session_start::SessionStartOutcome; use crate::events::session_start::SessionStartRequest; use crate::events::stop::StopOutcome; @@ -92,6 +94,13 @@ impl Hooks { self.engine.preview_session_start(request) } + pub fn preview_pre_tool_use( + &self, + request: &PreToolUseRequest, + ) -> Vec { + self.engine.preview_pre_tool_use(request) + } + pub async fn run_session_start( &self, request: SessionStartRequest, @@ -100,6 +109,10 @@ impl Hooks { self.engine.run_session_start(request, turn_id).await } + pub async fn run_pre_tool_use(&self, request: PreToolUseRequest) -> PreToolUseOutcome { + self.engine.run_pre_tool_use(request).await + } + pub fn preview_user_prompt_submit( &self, request: &UserPromptSubmitRequest, diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 067658541a..277500984c 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -13,6 +13,8 @@ use std::path::Path; use std::path::PathBuf; const GENERATED_DIR: &str = "generated"; +const PRE_TOOL_USE_INPUT_FIXTURE: &str = "pre-tool-use.command.input.schema.json"; +const PRE_TOOL_USE_OUTPUT_FIXTURE: &str = "pre-tool-use.command.output.schema.json"; const SESSION_START_INPUT_FIXTURE: &str = "session-start.command.input.schema.json"; const SESSION_START_OUTPUT_FIXTURE: &str = "session-start.command.output.schema.json"; const USER_PROMPT_SUBMIT_INPUT_FIXTURE: &str = "user-prompt-submit.command.input.schema.json"; @@ -63,6 +65,8 @@ pub(crate) struct HookUniversalOutputWire { #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] pub(crate) enum HookEventNameWire { + #[serde(rename = "PreToolUse")] + PreToolUse, #[serde(rename = "SessionStart")] SessionStart, #[serde(rename = "UserPromptSubmit")] @@ -71,6 +75,81 @@ pub(crate) enum HookEventNameWire { Stop, } +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "camelCase")] +#[serde(deny_unknown_fields)] +#[schemars(rename = "pre-tool-use.command.output")] +pub(crate) struct PreToolUseCommandOutputWire { + #[serde(flatten)] + pub universal: HookUniversalOutputWire, + #[serde(default)] + pub decision: Option, + #[serde(default)] + pub reason: Option, + #[serde(default)] + pub hook_specific_output: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "camelCase")] +#[serde(deny_unknown_fields)] +pub(crate) struct PreToolUseHookSpecificOutputWire { + pub hook_event_name: HookEventNameWire, + #[serde(default)] + pub permission_decision: Option, + #[serde(default)] + pub permission_decision_reason: Option, + #[serde(default)] + pub updated_input: Option, + #[serde(default)] + pub additional_context: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] +pub(crate) enum PreToolUsePermissionDecisionWire { + #[serde(rename = "allow")] + Allow, + #[serde(rename = "deny")] + Deny, + #[serde(rename = "ask")] + Ask, +} + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] +pub(crate) enum PreToolUseDecisionWire { + #[serde(rename = "approve")] + Approve, + #[serde(rename = "block")] + Block, +} + +#[derive(Debug, Clone, Serialize, JsonSchema)] +#[serde(rename_all = "camelCase")] +#[serde(deny_unknown_fields)] +pub(crate) struct PreToolUseToolInput { + pub command: String, +} + +#[derive(Debug, Clone, Serialize, JsonSchema)] +#[serde(deny_unknown_fields)] +#[schemars(rename = "pre-tool-use.command.input")] +pub(crate) struct PreToolUseCommandInput { + pub session_id: String, + /// Codex extension: expose the active turn id to internal turn-scoped hooks. + pub turn_id: String, + pub transcript_path: NullableString, + pub cwd: String, + #[schemars(schema_with = "pre_tool_use_hook_event_name_schema")] + pub hook_event_name: String, + pub model: String, + #[schemars(schema_with = "permission_mode_schema")] + pub permission_mode: String, + #[schemars(schema_with = "pre_tool_use_tool_name_schema")] + pub tool_name: String, + pub tool_input: PreToolUseToolInput, + pub tool_use_id: String, +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "camelCase")] #[serde(deny_unknown_fields)] @@ -212,6 +291,14 @@ pub fn write_schema_fixtures(schema_root: &Path) -> anyhow::Result<()> { let generated_dir = schema_root.join(GENERATED_DIR); ensure_empty_dir(&generated_dir)?; + write_schema( + &generated_dir.join(PRE_TOOL_USE_INPUT_FIXTURE), + schema_json::()?, + )?; + write_schema( + &generated_dir.join(PRE_TOOL_USE_OUTPUT_FIXTURE), + schema_json::()?, + )?; write_schema( &generated_dir.join(SESSION_START_INPUT_FIXTURE), schema_json::()?, @@ -295,6 +382,14 @@ fn session_start_hook_event_name_schema(_gen: &mut SchemaGenerator) -> Schema { string_const_schema("SessionStart") } +fn pre_tool_use_hook_event_name_schema(_gen: &mut SchemaGenerator) -> Schema { + string_const_schema("PreToolUse") +} + +fn pre_tool_use_tool_name_schema(_gen: &mut SchemaGenerator) -> Schema { + string_const_schema("Bash") +} + fn user_prompt_submit_hook_event_name_schema(_gen: &mut SchemaGenerator) -> Schema { string_const_schema("UserPromptSubmit") } @@ -346,6 +441,9 @@ fn default_continue() -> bool { #[cfg(test)] mod tests { + use super::PRE_TOOL_USE_INPUT_FIXTURE; + use super::PRE_TOOL_USE_OUTPUT_FIXTURE; + use super::PreToolUseCommandInput; use super::SESSION_START_INPUT_FIXTURE; use super::SESSION_START_OUTPUT_FIXTURE; use super::STOP_INPUT_FIXTURE; @@ -362,6 +460,12 @@ mod tests { fn expected_fixture(name: &str) -> &'static str { match name { + PRE_TOOL_USE_INPUT_FIXTURE => { + include_str!("../schema/generated/pre-tool-use.command.input.schema.json") + } + PRE_TOOL_USE_OUTPUT_FIXTURE => { + include_str!("../schema/generated/pre-tool-use.command.output.schema.json") + } SESSION_START_INPUT_FIXTURE => { include_str!("../schema/generated/session-start.command.input.schema.json") } @@ -395,6 +499,8 @@ mod tests { write_schema_fixtures(&schema_root).expect("write generated hook schemas"); for fixture in [ + PRE_TOOL_USE_INPUT_FIXTURE, + PRE_TOOL_USE_OUTPUT_FIXTURE, SESSION_START_INPUT_FIXTURE, SESSION_START_OUTPUT_FIXTURE, USER_PROMPT_SUBMIT_INPUT_FIXTURE, @@ -414,6 +520,10 @@ mod tests { fn turn_scoped_hook_inputs_include_codex_turn_id_extension() { // Codex intentionally diverges from Claude's public hook docs here so // internal hook consumers can key off the active turn. + let pre_tool_use: Value = serde_json::from_slice( + &schema_json::().expect("serialize pre tool use input schema"), + ) + .expect("parse pre tool use input schema"); let user_prompt_submit: Value = serde_json::from_slice( &schema_json::() .expect("serialize user prompt submit input schema"), @@ -424,7 +534,7 @@ mod tests { ) .expect("parse stop input schema"); - for schema in [&user_prompt_submit, &stop] { + for schema in [&pre_tool_use, &user_prompt_submit, &stop] { assert_eq!(schema["properties"]["turn_id"]["type"], "string"); assert!( schema["required"] diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index c8af844f7b..09b5948262 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1425,6 +1425,7 @@ pub enum EventMsg { #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "snake_case")] pub enum HookEventName { + PreToolUse, SessionStart, UserPromptSubmit, Stop, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index b6ccf71189..a553240170 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -9603,6 +9603,7 @@ fn extract_first_bold(s: &str) -> Option { fn hook_event_label(event_name: codex_protocol::protocol::HookEventName) -> &'static str { match event_name { + codex_protocol::protocol::HookEventName::PreToolUse => "PreToolUse", codex_protocol::protocol::HookEventName::SessionStart => "SessionStart", codex_protocol::protocol::HookEventName::UserPromptSubmit => "UserPromptSubmit", codex_protocol::protocol::HookEventName::Stop => "Stop", diff --git a/codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_events_render_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap similarity index 59% rename from codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_events_render_snapshot.snap rename to codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap index 27474ef6d7..11fa8ab8e3 100644 --- a/codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_events_render_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap @@ -1,10 +1,9 @@ --- source: tui/src/chatwidget/tests.rs -assertion_line: 8586 expression: combined --- -• Running SessionStart hook: warming the shell +• Running PreToolUse hook: warming the shell -SessionStart hook (completed) +PreToolUse hook (completed) warning: Heads up from the hook hook context: Remember the startup checklist. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_events_render_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap similarity index 91% rename from codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_events_render_snapshot.snap rename to codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap index 27474ef6d7..30dfa78e5b 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__hook_events_render_snapshot.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__session_start_hook_events_render_snapshot.snap @@ -1,6 +1,5 @@ --- source: tui/src/chatwidget/tests.rs -assertion_line: 8586 expression: combined --- • Running SessionStart hook: warming the shell diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 06eed8c8cc..2300421966 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -12052,7 +12052,33 @@ async fn deltas_then_same_final_message_are_rendered_snapshot() { } #[tokio::test] -async fn hook_events_render_snapshot() { +async fn pre_tool_use_hook_events_render_snapshot() { + assert_hook_events_snapshot( + codex_protocol::protocol::HookEventName::PreToolUse, + "pre-tool-use:0:/tmp/hooks.json", + "warming the shell", + "pre_tool_use_hook_events_render_snapshot", + ) + .await; +} + +#[tokio::test] +async fn session_start_hook_events_render_snapshot() { + assert_hook_events_snapshot( + codex_protocol::protocol::HookEventName::SessionStart, + "session-start:0:/tmp/hooks.json", + "warming the shell", + "session_start_hook_events_render_snapshot", + ) + .await; +} + +async fn assert_hook_events_snapshot( + event_name: codex_protocol::protocol::HookEventName, + run_id: &str, + status_message: &str, + snapshot_name: &str, +) { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; chat.handle_codex_event(Event { @@ -12060,15 +12086,15 @@ async fn hook_events_render_snapshot() { msg: EventMsg::HookStarted(codex_protocol::protocol::HookStartedEvent { turn_id: None, run: codex_protocol::protocol::HookRunSummary { - id: "session-start:0:/tmp/hooks.json".to_string(), - event_name: codex_protocol::protocol::HookEventName::SessionStart, + id: run_id.to_string(), + event_name, handler_type: codex_protocol::protocol::HookHandlerType::Command, execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, - scope: codex_protocol::protocol::HookScope::Thread, + scope: codex_protocol::protocol::HookScope::Turn, source_path: PathBuf::from("/tmp/hooks.json"), display_order: 0, status: codex_protocol::protocol::HookRunStatus::Running, - status_message: Some("warming the shell".to_string()), + status_message: Some(status_message.to_string()), started_at: 1, completed_at: None, duration_ms: None, @@ -12082,15 +12108,15 @@ async fn hook_events_render_snapshot() { msg: EventMsg::HookCompleted(codex_protocol::protocol::HookCompletedEvent { turn_id: None, run: codex_protocol::protocol::HookRunSummary { - id: "session-start:0:/tmp/hooks.json".to_string(), - event_name: codex_protocol::protocol::HookEventName::SessionStart, + id: run_id.to_string(), + event_name, handler_type: codex_protocol::protocol::HookHandlerType::Command, execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, - scope: codex_protocol::protocol::HookScope::Thread, + scope: codex_protocol::protocol::HookScope::Turn, source_path: PathBuf::from("/tmp/hooks.json"), display_order: 0, status: codex_protocol::protocol::HookRunStatus::Completed, - status_message: Some("warming the shell".to_string()), + status_message: Some(status_message.to_string()), started_at: 1, completed_at: Some(11), duration_ms: Some(10), @@ -12113,7 +12139,7 @@ async fn hook_events_render_snapshot() { .iter() .map(|lines| lines_to_single_string(lines)) .collect::(); - assert_snapshot!("hook_events_render_snapshot", combined); + assert_snapshot!(snapshot_name, combined); } // Combined visual snapshot using vt100 for history + direct buffer overlay for UI. diff --git a/codex-rs/tui_app_server/src/chatwidget.rs b/codex-rs/tui_app_server/src/chatwidget.rs index 4796401cc9..9363e3e356 100644 --- a/codex-rs/tui_app_server/src/chatwidget.rs +++ b/codex-rs/tui_app_server/src/chatwidget.rs @@ -10774,6 +10774,7 @@ fn extract_first_bold(s: &str) -> Option { fn hook_event_label(event_name: codex_protocol::protocol::HookEventName) -> &'static str { match event_name { + codex_protocol::protocol::HookEventName::PreToolUse => "PreToolUse", codex_protocol::protocol::HookEventName::SessionStart => "SessionStart", codex_protocol::protocol::HookEventName::UserPromptSubmit => "UserPromptSubmit", codex_protocol::protocol::HookEventName::Stop => "Stop", diff --git a/codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap b/codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap new file mode 100644 index 0000000000..f9a87c5682 --- /dev/null +++ b/codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__pre_tool_use_hook_events_render_snapshot.snap @@ -0,0 +1,9 @@ +--- +source: tui_app_server/src/chatwidget/tests.rs +expression: combined +--- +• Running PreToolUse hook: warming the shell + +PreToolUse hook (completed) + warning: Heads up from the hook + hook context: Remember the startup checklist. diff --git a/codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__hook_events_render_snapshot.snap b/codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__session_start_hook_events_render_snapshot.snap similarity index 100% rename from codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__hook_events_render_snapshot.snap rename to codex-rs/tui_app_server/src/chatwidget/snapshots/codex_tui_app_server__chatwidget__tests__session_start_hook_events_render_snapshot.snap diff --git a/codex-rs/tui_app_server/src/chatwidget/tests.rs b/codex-rs/tui_app_server/src/chatwidget/tests.rs index df229f4c85..611bf13adc 100644 --- a/codex-rs/tui_app_server/src/chatwidget/tests.rs +++ b/codex-rs/tui_app_server/src/chatwidget/tests.rs @@ -12465,7 +12465,33 @@ async fn deltas_then_same_final_message_are_rendered_snapshot() { } #[tokio::test] -async fn hook_events_render_snapshot() { +async fn pre_tool_use_hook_events_render_snapshot() { + assert_hook_events_snapshot( + codex_protocol::protocol::HookEventName::PreToolUse, + "pre-tool-use:0:/tmp/hooks.json", + "warming the shell", + "pre_tool_use_hook_events_render_snapshot", + ) + .await; +} + +#[tokio::test] +async fn session_start_hook_events_render_snapshot() { + assert_hook_events_snapshot( + codex_protocol::protocol::HookEventName::SessionStart, + "session-start:0:/tmp/hooks.json", + "warming the shell", + "session_start_hook_events_render_snapshot", + ) + .await; +} + +async fn assert_hook_events_snapshot( + event_name: codex_protocol::protocol::HookEventName, + run_id: &str, + status_message: &str, + snapshot_name: &str, +) { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; chat.handle_codex_event(Event { @@ -12473,15 +12499,15 @@ async fn hook_events_render_snapshot() { msg: EventMsg::HookStarted(codex_protocol::protocol::HookStartedEvent { turn_id: None, run: codex_protocol::protocol::HookRunSummary { - id: "session-start:0:/tmp/hooks.json".to_string(), - event_name: codex_protocol::protocol::HookEventName::SessionStart, + id: run_id.to_string(), + event_name, handler_type: codex_protocol::protocol::HookHandlerType::Command, execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, - scope: codex_protocol::protocol::HookScope::Thread, + scope: codex_protocol::protocol::HookScope::Turn, source_path: PathBuf::from("/tmp/hooks.json"), display_order: 0, status: codex_protocol::protocol::HookRunStatus::Running, - status_message: Some("warming the shell".to_string()), + status_message: Some(status_message.to_string()), started_at: 1, completed_at: None, duration_ms: None, @@ -12495,15 +12521,15 @@ async fn hook_events_render_snapshot() { msg: EventMsg::HookCompleted(codex_protocol::protocol::HookCompletedEvent { turn_id: None, run: codex_protocol::protocol::HookRunSummary { - id: "session-start:0:/tmp/hooks.json".to_string(), - event_name: codex_protocol::protocol::HookEventName::SessionStart, + id: run_id.to_string(), + event_name, handler_type: codex_protocol::protocol::HookHandlerType::Command, execution_mode: codex_protocol::protocol::HookExecutionMode::Sync, - scope: codex_protocol::protocol::HookScope::Thread, + scope: codex_protocol::protocol::HookScope::Turn, source_path: PathBuf::from("/tmp/hooks.json"), display_order: 0, status: codex_protocol::protocol::HookRunStatus::Completed, - status_message: Some("warming the shell".to_string()), + status_message: Some(status_message.to_string()), started_at: 1, completed_at: Some(11), duration_ms: Some(10), @@ -12526,7 +12552,7 @@ async fn hook_events_render_snapshot() { .iter() .map(|lines| lines_to_single_string(lines)) .collect::(); - assert_snapshot!("hook_events_render_snapshot", combined); + assert_snapshot!(snapshot_name, combined); } // Combined visual snapshot using vt100 for history + direct buffer overlay for UI. From 191fd9fd16e8f4ea43adb438122fb13f1b2ed674 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 23 Mar 2026 22:09:55 +0000 Subject: [PATCH 3/8] feat: use serde to differenciate inter agent communication (#15560) Use `serde` to encode the inter agent communication to an assistant message and use the decode to see if this is such a message Note: this assume serde on small pattern is fast enough --- .../src/codex/rollout_reconstruction_tests.rs | 16 +++++-- .../core/src/context_manager/history_tests.rs | 27 ++++++++--- .../src/tools/handlers/multi_agents_tests.rs | 26 +++++++--- codex-rs/protocol/src/protocol.rs | 48 ++++--------------- 4 files changed, 62 insertions(+), 55 deletions(-) diff --git a/codex-rs/core/src/codex/rollout_reconstruction_tests.rs b/codex-rs/core/src/codex/rollout_reconstruction_tests.rs index 09b4f45613..96423754db 100644 --- a/codex-rs/core/src/codex/rollout_reconstruction_tests.rs +++ b/codex-rs/core/src/codex/rollout_reconstruction_tests.rs @@ -3,9 +3,11 @@ use super::*; use crate::protocol::CompactedItem; use crate::protocol::InitialHistory; use crate::protocol::ResumedHistory; +use codex_protocol::AgentPath; use codex_protocol::ThreadId; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; +use codex_protocol::protocol::InterAgentCommunication; use pretty_assertions::assert_eq; use std::path::PathBuf; @@ -38,7 +40,15 @@ fn inter_agent_assistant_message(text: &str) -> ResponseItem { id: None, role: "assistant".to_string(), content: vec![ContentItem::OutputText { - text: text.to_string(), + text: serde_json::to_string(&InterAgentCommunication::new( + AgentPath::root(), + AgentPath::root() + .join("worker") + .expect("worker path should be valid"), + Vec::new(), + text.to_string(), + )) + .expect("inter-agent communication should serialize"), }], end_turn: None, phase: None, @@ -455,9 +465,7 @@ async fn reconstruct_history_rollback_counts_inter_agent_assistant_turns() { turn_id: Some(assistant_turn_id.clone()), ..first_context_item.clone() }; - let assistant_instruction = inter_agent_assistant_message( - "author: /root\nrecipient: /root/worker\nother_recipients: []\nContent: continue", - ); + let assistant_instruction = inter_agent_assistant_message("continue"); let assistant_reply = assistant_message("worker reply"); let rollout_items = vec![ diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index 4deb76ed6d..1a4dc0ed8e 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -4,6 +4,7 @@ use crate::truncate::TruncationPolicy; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use codex_git::GhostCommit; +use codex_protocol::AgentPath; use codex_protocol::models::BaseInstructions; use codex_protocol::models::ContentItem; use codex_protocol::models::FunctionCallOutputBody; @@ -17,6 +18,7 @@ use codex_protocol::models::ReasoningItemContent; use codex_protocol::models::ReasoningItemReasoningSummary; use codex_protocol::openai_models::InputModality; use codex_protocol::openai_models::default_input_modalities; +use codex_protocol::protocol::InterAgentCommunication; use image::ImageBuffer; use image::ImageFormat; use image::Rgba; @@ -39,11 +41,17 @@ fn assistant_msg(text: &str) -> ResponseItem { } fn inter_agent_assistant_msg(text: &str) -> ResponseItem { + let communication = InterAgentCommunication::new( + AgentPath::root(), + AgentPath::root().join("worker").unwrap(), + Vec::new(), + text.to_string(), + ); ResponseItem::Message { id: None, role: "assistant".to_string(), content: vec![ContentItem::OutputText { - text: text.to_string(), + text: serde_json::to_string(&communication).unwrap(), }], end_turn: None, phase: None, @@ -239,9 +247,7 @@ fn items_after_last_model_generated_tokens_are_zero_without_model_generated_item #[test] fn inter_agent_assistant_messages_are_turn_boundaries() { - let item = inter_agent_assistant_msg( - "author: /root\nrecipient: /root/worker\nother_recipients: []\nContent: continue", - ); + let item = inter_agent_assistant_msg("continue"); assert!(is_user_turn_boundary(&item)); } @@ -250,9 +256,7 @@ fn inter_agent_assistant_messages_are_turn_boundaries() { fn drop_last_n_user_turns_treats_inter_agent_assistant_messages_as_instruction_turns() { let first_turn = user_input_text_msg("first"); let first_reply = assistant_msg("done"); - let inter_agent_turn = inter_agent_assistant_msg( - "author: /root\nrecipient: /root/worker\nother_recipients: []\nContent: continue", - ); + let inter_agent_turn = inter_agent_assistant_msg("continue"); let inter_agent_reply = assistant_msg("worker reply"); let mut history = create_history_with_items(vec![ first_turn.clone(), @@ -266,6 +270,15 @@ fn drop_last_n_user_turns_treats_inter_agent_assistant_messages_as_instruction_t assert_eq!(history.raw_items(), &vec![first_turn, first_reply]); } +#[test] +fn legacy_inter_agent_assistant_messages_are_not_turn_boundaries() { + let item = assistant_msg( + "author: /root\nrecipient: /root/worker\nother_recipients: []\nContent: continue", + ); + + assert!(!is_user_turn_boundary(&item)); +} + #[test] fn total_token_usage_includes_all_items_after_last_model_generated_item() { let mut history = create_history_with_items(vec![assistant_msg("already counted by API")]); diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index 7b8471f03e..f74ebf0577 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -32,6 +32,7 @@ use codex_protocol::models::FunctionCallOutputBody; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::InitialHistory; +use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::RolloutItem; use codex_protocol::user_input::UserInput; use pretty_assertions::assert_eq; @@ -79,6 +80,16 @@ fn thread_manager() -> ThreadManager { ) } +fn inter_agent_message_text(recipient: &str, content: &str) -> String { + serde_json::to_string(&InterAgentCommunication::new( + AgentPath::root(), + AgentPath::try_from(recipient).expect("recipient path should be valid"), + Vec::new(), + content.to_string(), + )) + .expect("inter-agent communication should serialize") +} + #[derive(Clone, Copy)] struct NeverEndingTask; @@ -390,6 +401,7 @@ async fn multi_agent_v2_spawn_returns_path_and_send_input_accepts_relative_path( .get_thread(child_thread_id) .await .expect("child thread should exist"); + let expected_message = inter_agent_message_text("/root/test_process", "continue"); timeout(Duration::from_secs(2), async { loop { let history_items = child_thread @@ -407,8 +419,7 @@ async fn multi_agent_v2_spawn_returns_path_and_send_input_accepts_relative_path( && content.iter().any(|content_item| matches!( content_item, ContentItem::OutputText { text } - if text - == "author: /root\nrecipient: /root/test_process\nother_recipients: []\nContent: continue" + if text == &expected_message )) ) }); @@ -508,6 +519,10 @@ async fn multi_agent_v2_send_input_accepts_structured_items() { .find(|(id, op)| *id == agent_id && *op == expected); assert_eq!(captured, Some((agent_id, expected))); + let expected_message = inter_agent_message_text( + "/root/worker", + "[mention:$drive](app://google_drive)\nread the folder", + ); timeout(Duration::from_secs(2), async { loop { let history_items = thread @@ -525,8 +540,7 @@ async fn multi_agent_v2_send_input_accepts_structured_items() { && content.iter().any(|content_item| matches!( content_item, ContentItem::OutputText { text } - if text - == "author: /root\nrecipient: /root/worker\nother_recipients: []\nContent: [mention:$drive](app://google_drive)\nread the folder" + if text == &expected_message )) ) }); @@ -650,6 +664,7 @@ async fn multi_agent_v2_send_input_interrupts_busy_child_without_losing_message( )) ))); + let expected_message = inter_agent_message_text("/root/worker", "continue"); timeout(Duration::from_secs(5), async { loop { let history_items = thread @@ -667,8 +682,7 @@ async fn multi_agent_v2_send_input_interrupts_busy_child_without_losing_message( && content.iter().any(|content_item| matches!( content_item, ContentItem::OutputText { text } - if text - == "author: /root\nrecipient: /root/worker\nother_recipients: []\nContent: continue" + if text == &expected_message )) ) }); diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 09b5948262..e6b1ae79dc 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -530,54 +530,26 @@ impl InterAgentCommunication { } } - pub fn to_response_item(&self) -> ResponseItem { - ResponseItem::Message { - id: None, - role: "assistant".to_string(), - content: vec![ContentItem::OutputText { - text: self.as_text(), - }], - end_turn: None, - phase: None, - } - } - pub fn to_response_input_item(&self) -> ResponseInputItem { ResponseInputItem::Message { role: "assistant".to_string(), content: vec![ContentItem::OutputText { - text: self.as_text(), + text: serde_json::to_string(self).unwrap_or_default(), }], } } pub fn is_message_content(content: &[ContentItem]) -> bool { - content.iter().any(|content_item| match content_item { - ContentItem::InputText { text } | ContentItem::OutputText { text } => { - Self::is_instruction_text(text) + Self::from_message_content(content).is_some() + } + + fn from_message_content(content: &[ContentItem]) -> Option { + match content { + [ContentItem::InputText { text }] | [ContentItem::OutputText { text }] => { + serde_json::from_str(text).ok() } - _ => false, - }) - } - - fn as_text(&self) -> String { - let other_recipients = self - .other_recipients - .iter() - .map(std::string::ToString::to_string) - .collect::>() - .join(", "); - format!( - "author: {}\nrecipient: {}\nother_recipients: [{other_recipients}]\nContent: {}", - self.author, self.recipient, self.content - ) - } - - fn is_instruction_text(text: &str) -> bool { - text.starts_with("author: ") - && text.contains("\nrecipient: ") - && text.contains("\nother_recipients: [") - && text.contains("]\nContent: ") + _ => None, + } } } From 67c1c7c054dc494044e091acf17b3f666d5b7658 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Mon, 23 Mar 2026 15:19:01 -0700 Subject: [PATCH 4/8] chore(core) Add approvals reviewer to UserTurn (#15426) ## Summary Adds support for approvals_reviewer to `Op::UserTurn` so we can migrate `[CodexMessageProcessor::turn_start]` to use Op::UserTurn ## Testing - [x] Adds quick test for the new field Co-authored-by: Codex --- codex-rs/core/src/codex.rs | 3 +- codex-rs/core/src/codex_tests.rs | 35 +++++++++++++++++++ codex-rs/core/src/guardian/review_session.rs | 1 + codex-rs/core/tests/common/test_codex.rs | 1 + codex-rs/core/tests/suite/apply_patch_cli.rs | 7 ++++ codex-rs/core/tests/suite/approvals.rs | 1 + codex-rs/core/tests/suite/client.rs | 2 ++ codex-rs/core/tests/suite/code_mode.rs | 1 + .../tests/suite/collaboration_instructions.rs | 2 ++ codex-rs/core/tests/suite/compact.rs | 7 ++++ codex-rs/core/tests/suite/exec_policy.rs | 2 ++ codex-rs/core/tests/suite/image_rollout.rs | 2 ++ codex-rs/core/tests/suite/items.rs | 5 +++ codex-rs/core/tests/suite/json_result.rs | 1 + codex-rs/core/tests/suite/live_reload.rs | 1 + codex-rs/core/tests/suite/model_switching.rs | 14 ++++++++ .../core/tests/suite/model_visible_layout.rs | 5 +++ codex-rs/core/tests/suite/models_cache_ttl.rs | 1 + .../core/tests/suite/models_etag_responses.rs | 1 + codex-rs/core/tests/suite/personality.rs | 13 +++++++ codex-rs/core/tests/suite/prompt_caching.rs | 5 +++ codex-rs/core/tests/suite/remote_models.rs | 4 +++ .../core/tests/suite/request_permissions.rs | 1 + .../tests/suite/request_permissions_tool.rs | 1 + .../core/tests/suite/request_user_input.rs | 2 ++ codex-rs/core/tests/suite/rmcp_client.rs | 6 ++++ .../tests/suite/safety_check_downgrade.rs | 4 +++ codex-rs/core/tests/suite/shell_snapshot.rs | 4 +++ codex-rs/core/tests/suite/skill_approval.rs | 1 + codex-rs/core/tests/suite/skills.rs | 1 + codex-rs/core/tests/suite/sqlite_state.rs | 1 + codex-rs/core/tests/suite/tool_harness.rs | 5 +++ codex-rs/core/tests/suite/tool_parallelism.rs | 2 ++ codex-rs/core/tests/suite/truncation.rs | 1 + codex-rs/core/tests/suite/unified_exec.rs | 26 ++++++++++++++ codex-rs/core/tests/suite/user_shell_cmd.rs | 1 + codex-rs/core/tests/suite/view_image.rs | 14 ++++++++ .../core/tests/suite/websocket_fallback.rs | 1 + codex-rs/docs/protocol_v1.md | 2 +- codex-rs/protocol/src/protocol.rs | 5 +++ codex-rs/tui/src/chatwidget.rs | 1 + codex-rs/tui_app_server/src/app.rs | 4 ++- codex-rs/tui_app_server/src/app_command.rs | 4 +++ 43 files changed, 198 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 9b81025526..5e428362f2 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4550,6 +4550,7 @@ mod handlers { Op::UserTurn { cwd, approval_policy, + approvals_reviewer, sandbox_policy, model, effort, @@ -4575,7 +4576,7 @@ mod handlers { SessionSettingsUpdate { cwd: Some(cwd), approval_policy: Some(approval_policy), - approvals_reviewer: None, + approvals_reviewer, sandbox_policy: Some(sandbox_policy), windows_sandbox_level: None, collaboration_mode, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 24470051f2..5d6769e0ea 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -3013,6 +3013,41 @@ fn op_kind_distinguishes_turn_ops() { ); } +#[tokio::test] +async fn user_turn_updates_approvals_reviewer() { + let (session, turn_context, _rx) = make_session_and_context_with_rx().await; + let config = session.get_config().await; + + handlers::user_input_or_turn( + &session, + "sub-1".to_string(), + Op::UserTurn { + items: vec![UserInput::Text { + text: "hello".to_string(), + text_elements: Vec::new(), + }], + cwd: config.cwd.clone(), + approval_policy: config.permissions.approval_policy.value(), + approvals_reviewer: Some(crate::config::types::ApprovalsReviewer::GuardianSubagent), + sandbox_policy: config.permissions.sandbox_policy.get().clone(), + model: turn_context.model_info.slug.clone(), + effort: config.model_reasoning_effort, + summary: config.model_reasoning_summary, + service_tier: None, + final_output_json_schema: None, + collaboration_mode: None, + personality: config.personality, + }, + ) + .await; + + let state = session.state.lock().await; + assert_eq!( + state.session_configuration.approvals_reviewer, + crate::config::types::ApprovalsReviewer::GuardianSubagent + ); +} + #[tokio::test] async fn spawn_task_turn_span_inherits_dispatch_trace_context() { struct TraceCaptureTask { diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 729e172387..50bf2ed843 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -514,6 +514,7 @@ async fn run_review_on_session( items: params.prompt_items.clone(), cwd: params.parent_turn.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: params.model.clone(), effort: params.reasoning_effort, diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index 6df93bcd85..c61631e5bf 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -723,6 +723,7 @@ impl TestCodex { final_output_json_schema: None, cwd: self.config.cwd.clone(), approval_policy, + approvals_reviewer: None, sandbox_policy, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index b113fc465c..d0cefcb616 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -310,6 +310,7 @@ async fn apply_patch_cli_move_without_content_change_has_no_turn_diff( final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, @@ -919,6 +920,7 @@ async fn apply_patch_shell_command_heredoc_with_cd_emits_turn_diff() -> Result<( final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, @@ -1000,6 +1002,7 @@ async fn apply_patch_shell_command_failure_propagates_error_and_skips_diff() -> final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, @@ -1151,6 +1154,7 @@ async fn apply_patch_emits_turn_diff_event_with_unified_diff( final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, @@ -1215,6 +1219,7 @@ async fn apply_patch_turn_diff_for_rename_with_content_change( final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, @@ -1287,6 +1292,7 @@ async fn apply_patch_aggregates_diff_across_multiple_tool_calls() -> Result<()> final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, @@ -1359,6 +1365,7 @@ async fn apply_patch_aggregates_diff_preserves_success_after_failure() -> Result final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index f77697d017..6ec05bedc2 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -558,6 +558,7 @@ async fn submit_turn( final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy, + approvals_reviewer: None, sandbox_policy, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 3ea30c5967..5da71f556b 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1310,6 +1310,7 @@ async fn user_turn_collaboration_mode_overrides_model_and_effort() -> anyhow::Re }], cwd: config.cwd.clone(), approval_policy: config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: config.permissions.sandbox_policy.get().clone(), model: session_configured.model.clone(), effort: Some(ReasoningEffort::Low), @@ -1427,6 +1428,7 @@ async fn user_turn_explicit_reasoning_summary_overrides_model_catalog_default() }], cwd: config.cwd.clone(), approval_policy: config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: config.permissions.sandbox_policy.get().clone(), model: session_configured.model, effort: None, diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index b9e4f05b36..fa8229ecc3 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -2319,6 +2319,7 @@ text( final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: test.session_configured.model.clone(), effort: None, diff --git a/codex-rs/core/tests/suite/collaboration_instructions.rs b/codex-rs/core/tests/suite/collaboration_instructions.rs index 81d0678cad..1517d9968c 100644 --- a/codex-rs/core/tests/suite/collaboration_instructions.rs +++ b/codex-rs/core/tests/suite/collaboration_instructions.rs @@ -178,6 +178,7 @@ async fn collaboration_instructions_added_on_user_turn() -> Result<()> { }], cwd: test.config.cwd.clone(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: test.config.permissions.sandbox_policy.get().clone(), model: test.session_configured.model.clone(), effort: None, @@ -293,6 +294,7 @@ async fn user_turn_overrides_collaboration_instructions_after_override() -> Resu }], cwd: test.config.cwd.clone(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: test.config.permissions.sandbox_policy.get().clone(), model: test.session_configured.model.clone(), effort: None, diff --git a/codex-rs/core/tests/suite/compact.rs b/codex-rs/core/tests/suite/compact.rs index 2f4365a959..a54d740111 100644 --- a/codex-rs/core/tests/suite/compact.rs +++ b/codex-rs/core/tests/suite/compact.rs @@ -1658,6 +1658,7 @@ async fn auto_compact_runs_after_resume_when_token_usage_is_over_limit() { final_output_json_schema: None, cwd: resumed.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: resumed.session_configured.model.clone(), effort: None, @@ -1748,6 +1749,7 @@ async fn pre_sampling_compact_runs_on_switch_to_smaller_context_model() { final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: previous_model.to_string(), effort: None, @@ -1772,6 +1774,7 @@ async fn pre_sampling_compact_runs_on_switch_to_smaller_context_model() { final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: next_model.to_string(), effort: None, @@ -1882,6 +1885,7 @@ async fn pre_sampling_compact_runs_after_resume_and_switch_to_smaller_model() { final_output_json_schema: None, cwd: initial.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: previous_model.to_string(), effort: None, @@ -1930,6 +1934,7 @@ async fn pre_sampling_compact_runs_after_resume_and_switch_to_smaller_model() { final_output_json_schema: None, cwd: resumed.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: next_model.to_string(), effort: None, @@ -3132,6 +3137,7 @@ async fn snapshot_request_shape_pre_turn_compaction_strips_incoming_model_switch final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: previous_model.to_string(), effort: None, @@ -3156,6 +3162,7 @@ async fn snapshot_request_shape_pre_turn_compaction_strips_incoming_model_switch final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: next_model.to_string(), effort: None, diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 18be468020..fb055c970f 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -51,6 +51,7 @@ async fn submit_user_turn( final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy, + approvals_reviewer: None, sandbox_policy, model: session_model, effort: None, @@ -131,6 +132,7 @@ async fn execpolicy_blocks_shell_invocation() -> Result<()> { final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/image_rollout.rs b/codex-rs/core/tests/suite/image_rollout.rs index 07b51c7629..1526fe11a2 100644 --- a/codex-rs/core/tests/suite/image_rollout.rs +++ b/codex-rs/core/tests/suite/image_rollout.rs @@ -122,6 +122,7 @@ async fn copy_paste_local_image_persists_rollout_request_shape() -> anyhow::Resu final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -205,6 +206,7 @@ async fn drag_drop_image_persists_rollout_request_shape() -> anyhow::Result<()> final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/items.rs b/codex-rs/core/tests/suite/items.rs index d860545553..f949cd4b97 100644 --- a/codex-rs/core/tests/suite/items.rs +++ b/codex-rs/core/tests/suite/items.rs @@ -522,6 +522,7 @@ async fn plan_mode_emits_plan_item_from_proposed_plan_block() -> anyhow::Result< final_output_json_schema: None, cwd: std::env::current_dir()?, approval_policy: codex_protocol::protocol::AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, model: session_configured.model.clone(), effort: None, @@ -598,6 +599,7 @@ async fn plan_mode_strips_plan_from_agent_messages() -> anyhow::Result<()> { final_output_json_schema: None, cwd: std::env::current_dir()?, approval_policy: codex_protocol::protocol::AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, model: session_configured.model.clone(), effort: None, @@ -706,6 +708,7 @@ async fn plan_mode_streaming_citations_are_stripped_across_added_deltas_and_done final_output_json_schema: None, cwd: std::env::current_dir()?, approval_policy: codex_protocol::protocol::AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, model: session_configured.model.clone(), effort: None, @@ -892,6 +895,7 @@ async fn plan_mode_streaming_proposed_plan_tag_split_across_added_and_delta_is_p final_output_json_schema: None, cwd: std::env::current_dir()?, approval_policy: codex_protocol::protocol::AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, model: session_configured.model.clone(), effort: None, @@ -1005,6 +1009,7 @@ async fn plan_mode_handles_missing_plan_close_tag() -> anyhow::Result<()> { final_output_json_schema: None, cwd: std::env::current_dir()?, approval_policy: codex_protocol::protocol::AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, model: session_configured.model.clone(), effort: None, diff --git a/codex-rs/core/tests/suite/json_result.rs b/codex-rs/core/tests/suite/json_result.rs index f7dfd02daf..3b6f3e3f9c 100644 --- a/codex-rs/core/tests/suite/json_result.rs +++ b/codex-rs/core/tests/suite/json_result.rs @@ -80,6 +80,7 @@ async fn codex_returns_json_result(model: String) -> anyhow::Result<()> { final_output_json_schema: Some(serde_json::from_str(SCHEMA)?), cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, diff --git a/codex-rs/core/tests/suite/live_reload.rs b/codex-rs/core/tests/suite/live_reload.rs index 2192bdffd9..663cf47488 100644 --- a/codex-rs/core/tests/suite/live_reload.rs +++ b/codex-rs/core/tests/suite/live_reload.rs @@ -61,6 +61,7 @@ async fn submit_skill_turn(test: &TestCodex, skill_path: PathBuf, prompt: &str) final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/model_switching.rs b/codex-rs/core/tests/suite/model_switching.rs index 9902f0ee6c..a7c5ceb0d2 100644 --- a/codex-rs/core/tests/suite/model_switching.rs +++ b/codex-rs/core/tests/suite/model_switching.rs @@ -126,6 +126,7 @@ async fn model_change_appends_model_instructions_developer_message() -> Result<( final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -162,6 +163,7 @@ async fn model_change_appends_model_instructions_developer_message() -> Result<( final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: next_model.to_string(), effort: test.config.model_reasoning_effort, @@ -221,6 +223,7 @@ async fn model_and_personality_change_only_appends_model_instructions() -> Resul final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -257,6 +260,7 @@ async fn model_and_personality_change_only_appends_model_instructions() -> Resul final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: next_model.to_string(), effort: test.config.model_reasoning_effort, @@ -398,6 +402,7 @@ async fn model_change_from_image_to_text_strips_prior_image_content() -> Result< final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: image_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -418,6 +423,7 @@ async fn model_change_from_image_to_text_strips_prior_image_content() -> Result< final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: text_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -525,6 +531,7 @@ async fn generated_image_is_replayed_for_image_capable_models() -> Result<()> { final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: image_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -545,6 +552,7 @@ async fn generated_image_is_replayed_for_image_capable_models() -> Result<()> { final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: image_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -655,6 +663,7 @@ async fn model_change_from_generated_image_to_text_preserves_prior_generated_ima final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: image_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -675,6 +684,7 @@ async fn model_change_from_generated_image_to_text_preserves_prior_generated_ima final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: text_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -787,6 +797,7 @@ async fn thread_rollback_after_generated_image_drops_entire_image_turn_history() final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: image_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -815,6 +826,7 @@ async fn thread_rollback_after_generated_image_drops_entire_image_turn_history() final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: image_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -969,6 +981,7 @@ async fn model_switch_to_smaller_model_updates_token_context_window() -> Result< final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: large_model_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -1027,6 +1040,7 @@ async fn model_switch_to_smaller_model_updates_token_context_window() -> Result< final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: smaller_model_slug.to_string(), effort: test.config.model_reasoning_effort, diff --git a/codex-rs/core/tests/suite/model_visible_layout.rs b/codex-rs/core/tests/suite/model_visible_layout.rs index a10fa7c262..49f635432f 100644 --- a/codex-rs/core/tests/suite/model_visible_layout.rs +++ b/codex-rs/core/tests/suite/model_visible_layout.rs @@ -121,6 +121,7 @@ async fn snapshot_model_visible_layout_turn_overrides() -> Result<()> { final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -144,6 +145,7 @@ async fn snapshot_model_visible_layout_turn_overrides() -> Result<()> { final_output_json_schema: None, cwd: preturn_context_diff_cwd, approval_policy: AskForApproval::OnRequest, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -222,6 +224,7 @@ async fn snapshot_model_visible_layout_cwd_change_does_not_refresh_agents() -> R final_output_json_schema: None, cwd: cwd_one.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -245,6 +248,7 @@ async fn snapshot_model_visible_layout_cwd_change_does_not_refresh_agents() -> R final_output_json_schema: None, cwd: cwd_two, approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -354,6 +358,7 @@ async fn snapshot_model_visible_layout_resume_with_personality_change() -> Resul final_output_json_schema: None, cwd: resume_override_cwd, approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: resumed.session_configured.model.clone(), effort: resumed.config.model_reasoning_effort, diff --git a/codex-rs/core/tests/suite/models_cache_ttl.rs b/codex-rs/core/tests/suite/models_cache_ttl.rs index 7cb7573347..3d5904bd7a 100644 --- a/codex-rs/core/tests/suite/models_cache_ttl.rs +++ b/codex-rs/core/tests/suite/models_cache_ttl.rs @@ -96,6 +96,7 @@ async fn renews_cache_ttl_on_matching_models_etag() -> Result<()> { final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: codex_protocol::protocol::AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: test.session_configured.model.clone(), effort: None, diff --git a/codex-rs/core/tests/suite/models_etag_responses.rs b/codex-rs/core/tests/suite/models_etag_responses.rs index a479ebb80c..272c820caf 100644 --- a/codex-rs/core/tests/suite/models_etag_responses.rs +++ b/codex-rs/core/tests/suite/models_etag_responses.rs @@ -102,6 +102,7 @@ async fn refresh_models_on_models_etag_mismatch_and_avoid_duplicate_models_fetch final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/personality.rs b/codex-rs/core/tests/suite/personality.rs index 9a495e7af4..ed4da07168 100644 --- a/codex-rs/core/tests/suite/personality.rs +++ b/codex-rs/core/tests/suite/personality.rs @@ -103,6 +103,7 @@ async fn user_turn_personality_none_does_not_add_update_message() -> anyhow::Res final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -153,6 +154,7 @@ async fn config_personality_some_sets_instructions_template() -> anyhow::Result< final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -210,6 +212,7 @@ async fn config_personality_none_sends_no_personality() -> anyhow::Result<()> { final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -273,6 +276,7 @@ async fn default_personality_is_pragmatic_without_config_toml() -> anyhow::Resul final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -324,6 +328,7 @@ async fn user_turn_personality_some_adds_update_message() -> anyhow::Result<()> final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -361,6 +366,7 @@ async fn user_turn_personality_some_adds_update_message() -> anyhow::Result<()> final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -427,6 +433,7 @@ async fn user_turn_personality_same_value_does_not_add_update_message() -> anyho final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -464,6 +471,7 @@ async fn user_turn_personality_same_value_does_not_add_update_message() -> anyho final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -543,6 +551,7 @@ async fn user_turn_personality_skips_if_feature_disabled() -> anyhow::Result<()> final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -580,6 +589,7 @@ async fn user_turn_personality_skips_if_feature_disabled() -> anyhow::Result<()> final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: test.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: test.config.model_reasoning_effort, @@ -696,6 +706,7 @@ async fn remote_model_friendly_personality_instructions_with_feature() -> anyhow final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: remote_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -814,6 +825,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() - final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: remote_slug.to_string(), effort: test.config.model_reasoning_effort, @@ -851,6 +863,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() - final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: remote_slug.to_string(), effort: test.config.model_reasoning_effort, diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index 14caaf8f0b..9be83e6bb0 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -703,6 +703,7 @@ async fn per_turn_overrides_keep_cached_prefix_and_key_constant() -> anyhow::Res }], cwd: new_cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: new_policy.clone(), model: "o3".to_string(), effort: Some(ReasoningEffort::High), @@ -815,6 +816,7 @@ async fn send_user_turn_with_no_changes_does_not_send_environment_context() -> a }], cwd: default_cwd.clone(), approval_policy: default_approval_policy, + approvals_reviewer: None, sandbox_policy: default_sandbox_policy.clone(), model: default_model.clone(), effort: default_effort, @@ -835,6 +837,7 @@ async fn send_user_turn_with_no_changes_does_not_send_environment_context() -> a }], cwd: default_cwd.clone(), approval_policy: default_approval_policy, + approvals_reviewer: None, sandbox_policy: default_sandbox_policy.clone(), model: default_model.clone(), effort: default_effort, @@ -939,6 +942,7 @@ async fn send_user_turn_with_changes_sends_environment_context() -> anyhow::Resu }], cwd: default_cwd.clone(), approval_policy: default_approval_policy, + approvals_reviewer: None, sandbox_policy: default_sandbox_policy.clone(), model: default_model, effort: default_effort, @@ -959,6 +963,7 @@ async fn send_user_turn_with_changes_sends_environment_context() -> anyhow::Resu }], cwd: default_cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: "o3".to_string(), effort: Some(ReasoningEffort::High), diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index 860d83fe9a..83d911533d 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -173,6 +173,7 @@ async fn remote_models_long_model_slug_is_sent_with_high_reasoning() -> Result<( final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: config.permissions.sandbox_policy.get().clone(), model: requested_model.to_string(), effort: None, @@ -231,6 +232,7 @@ async fn namespaced_model_slug_uses_catalog_metadata_without_fallback_warning() final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: config.permissions.sandbox_policy.get().clone(), model: requested_model.to_string(), effort: None, @@ -394,6 +396,7 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: REMOTE_MODEL_SLUG.to_string(), effort: None, @@ -612,6 +615,7 @@ async fn remote_models_apply_remote_base_instructions() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: model.to_string(), effort: None, diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index b1aaac65b4..7c16599bf0 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -193,6 +193,7 @@ async fn submit_turn( final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy, + approvals_reviewer: None, sandbox_policy, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/request_permissions_tool.rs b/codex-rs/core/tests/suite/request_permissions_tool.rs index a01d6e0ab7..14506f4a41 100644 --- a/codex-rs/core/tests/suite/request_permissions_tool.rs +++ b/codex-rs/core/tests/suite/request_permissions_tool.rs @@ -145,6 +145,7 @@ async fn submit_turn( final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy, + approvals_reviewer: None, sandbox_policy, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/request_user_input.rs b/codex-rs/core/tests/suite/request_user_input.rs index 1bf759d270..8e30b37c21 100644 --- a/codex-rs/core/tests/suite/request_user_input.rs +++ b/codex-rs/core/tests/suite/request_user_input.rs @@ -138,6 +138,7 @@ async fn request_user_input_round_trip_for_mode(mode: ModeKind) -> anyhow::Resul final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -255,6 +256,7 @@ where final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 772674f79a..6cbf9521ba 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -129,6 +129,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, effort: None, @@ -298,6 +299,7 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> { final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, effort: None, @@ -501,6 +503,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: text_only_model_slug.to_string(), effort: None, @@ -615,6 +618,7 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> { final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, effort: None, @@ -776,6 +780,7 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> { final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, effort: None, @@ -1022,6 +1027,7 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/safety_check_downgrade.rs b/codex-rs/core/tests/suite/safety_check_downgrade.rs index eabbdc9d03..51a88ef16a 100644 --- a/codex-rs/core/tests/suite/safety_check_downgrade.rs +++ b/codex-rs/core/tests/suite/safety_check_downgrade.rs @@ -45,6 +45,7 @@ async fn openai_model_header_mismatch_emits_warning_event_and_warning_item() -> final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: REQUESTED_MODEL.to_string(), effort: test.config.model_reasoning_effort, @@ -143,6 +144,7 @@ async fn response_model_field_mismatch_emits_warning_when_header_matches_request final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: REQUESTED_MODEL.to_string(), effort: test.config.model_reasoning_effort, @@ -228,6 +230,7 @@ async fn openai_model_header_mismatch_only_emits_one_warning_per_turn() -> Resul final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: REQUESTED_MODEL.to_string(), effort: test.config.model_reasoning_effort, @@ -277,6 +280,7 @@ async fn openai_model_header_casing_only_mismatch_does_not_warn() -> Result<()> final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: REQUESTED_MODEL.to_string(), effort: test.config.model_reasoning_effort, diff --git a/codex-rs/core/tests/suite/shell_snapshot.rs b/codex-rs/core/tests/suite/shell_snapshot.rs index 68228a412e..55bb0ac284 100644 --- a/codex-rs/core/tests/suite/shell_snapshot.rs +++ b/codex-rs/core/tests/suite/shell_snapshot.rs @@ -164,6 +164,7 @@ async fn run_snapshot_command_with_options( final_output_json_schema: None, cwd, approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -254,6 +255,7 @@ async fn run_shell_command_snapshot_with_options( final_output_json_schema: None, cwd, approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -324,6 +326,7 @@ async fn run_tool_turn_on_harness( final_output_json_schema: None, cwd, approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -555,6 +558,7 @@ async fn shell_command_snapshot_still_intercepts_apply_patch() -> Result<()> { final_output_json_schema: None, cwd: cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model, effort: None, diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index b5fda12ae0..5a50e09cee 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -65,6 +65,7 @@ async fn submit_turn_with_policies( final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy, + approvals_reviewer: None, sandbox_policy, model: test.session_configured.model.clone(), effort: None, diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 801e0dd6bb..388618b56a 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -74,6 +74,7 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/sqlite_state.rs b/codex-rs/core/tests/suite/sqlite_state.rs index 248ada02cc..2df92dbf95 100644 --- a/codex-rs/core/tests/suite/sqlite_state.rs +++ b/codex-rs/core/tests/suite/sqlite_state.rs @@ -395,6 +395,7 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result< final_output_json_schema: None, cwd: test.cwd_path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: test.session_configured.model.clone(), effort: None, diff --git a/codex-rs/core/tests/suite/tool_harness.rs b/codex-rs/core/tests/suite/tool_harness.rs index bb1da9e8b1..9594195a5b 100644 --- a/codex-rs/core/tests/suite/tool_harness.rs +++ b/codex-rs/core/tests/suite/tool_harness.rs @@ -85,6 +85,7 @@ async fn shell_tool_executes_command_and_streams_output() -> anyhow::Result<()> final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -155,6 +156,7 @@ async fn update_plan_tool_emits_plan_update_event() -> anyhow::Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -235,6 +237,7 @@ async fn update_plan_tool_rejects_malformed_payload() -> anyhow::Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -330,6 +333,7 @@ async fn apply_patch_tool_executes_and_emits_patch_events() -> anyhow::Result<() final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -433,6 +437,7 @@ async fn apply_patch_reports_parse_diagnostics() -> anyhow::Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/tool_parallelism.rs b/codex-rs/core/tests/suite/tool_parallelism.rs index a177a6ee37..faff0b2e09 100644 --- a/codex-rs/core/tests/suite/tool_parallelism.rs +++ b/codex-rs/core/tests/suite/tool_parallelism.rs @@ -42,6 +42,7 @@ async fn run_turn(test: &TestCodex, prompt: &str) -> anyhow::Result<()> { final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -358,6 +359,7 @@ async fn shell_tools_start_before_response_completed_when_stream_delayed() -> an final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/truncation.rs b/codex-rs/core/tests/suite/truncation.rs index e7b25799ef..f10567de24 100644 --- a/codex-rs/core/tests/suite/truncation.rs +++ b/codex-rs/core/tests/suite/truncation.rs @@ -486,6 +486,7 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> { final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 7252d9a6b6..c07fd20e5e 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -192,6 +192,7 @@ async fn unified_exec_intercepts_apply_patch_exec_command() -> Result<()> { final_output_json_schema: None, cwd, approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -325,6 +326,7 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -407,6 +409,7 @@ async fn unified_exec_resolves_relative_workdir() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -492,6 +495,7 @@ async fn unified_exec_respects_workdir_override() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -589,6 +593,7 @@ async fn unified_exec_emits_exec_command_end_event() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -668,6 +673,7 @@ async fn unified_exec_emits_output_delta_for_exec_command() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -748,6 +754,7 @@ async fn unified_exec_full_lifecycle_with_background_end_event() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -882,6 +889,7 @@ async fn unified_exec_emits_terminal_interaction_for_write_stdin() -> Result<()> final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1023,6 +1031,7 @@ async fn unified_exec_terminal_interaction_captures_delayed_output() -> Result<( final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1187,6 +1196,7 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1289,6 +1299,7 @@ async fn exec_command_reports_chunk_and_exit_metadata() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1411,6 +1422,7 @@ async fn unified_exec_defaults_to_pipe() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1505,6 +1517,7 @@ async fn unified_exec_can_enable_tty() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1590,6 +1603,7 @@ async fn unified_exec_respects_early_exit_notifications() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1725,6 +1739,7 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1897,6 +1912,7 @@ async fn unified_exec_emits_end_event_when_session_dies_via_stdin() -> Result<() final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1978,6 +1994,7 @@ async fn unified_exec_keeps_long_running_session_after_turn_end() -> Result<()> final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -2070,6 +2087,7 @@ async fn unified_exec_interrupt_preserves_long_running_session() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -2178,6 +2196,7 @@ async fn unified_exec_reuses_session_via_stdin() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -2317,6 +2336,7 @@ PY final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -2435,6 +2455,7 @@ async fn unified_exec_timeout_and_followup_poll() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -2535,6 +2556,7 @@ PY final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -2634,6 +2656,7 @@ async fn unified_exec_runs_under_sandbox() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, // Important! sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, @@ -2743,6 +2766,7 @@ async fn unified_exec_python_prompt_under_seatbelt() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::new_read_only_policy(), model: session_model, effort: None, @@ -2842,6 +2866,7 @@ async fn unified_exec_runs_on_all_platforms() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -2981,6 +3006,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> { final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/user_shell_cmd.rs b/codex-rs/core/tests/suite/user_shell_cmd.rs index eb593c6fe3..801ff76274 100644 --- a/codex-rs/core/tests/suite/user_shell_cmd.rs +++ b/codex-rs/core/tests/suite/user_shell_cmd.rs @@ -176,6 +176,7 @@ async fn user_shell_command_does_not_replace_active_turn() -> anyhow::Result<()> final_output_json_schema: None, cwd: fixture.cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: fixture.session_configured.model.clone(), effort: None, diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index efc2e53324..732d6b29eb 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -170,6 +170,7 @@ async fn user_turn_with_local_image_attaches_image() -> anyhow::Result<()> { final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -278,6 +279,7 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { final_output_json_schema: None, cwd: cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -418,6 +420,7 @@ async fn view_image_tool_can_preserve_original_resolution_when_requested_on_gpt5 final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -516,6 +519,7 @@ async fn view_image_tool_errors_clearly_for_unsupported_detail_values() -> anyho final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -607,6 +611,7 @@ async fn view_image_tool_treats_null_detail_as_omitted() -> anyhow::Result<()> { final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -706,6 +711,7 @@ async fn view_image_tool_resizes_when_model_lacks_original_detail_support() -> a final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -816,6 +822,7 @@ async fn view_image_tool_does_not_force_original_resolution_with_capability_feat final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -917,6 +924,7 @@ await codex.emitImage(out); final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1036,6 +1044,7 @@ console.log(out.type); final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1128,6 +1137,7 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> { final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1203,6 +1213,7 @@ async fn view_image_tool_errors_for_non_image_files() -> anyhow::Result<()> { final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1283,6 +1294,7 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, @@ -1405,6 +1417,7 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: model_slug.to_string(), effort: None, @@ -1479,6 +1492,7 @@ async fn replaces_invalid_local_image_after_bad_request() -> anyhow::Result<()> final_output_json_schema: None, cwd: config.cwd.clone(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_model, effort: None, diff --git a/codex-rs/core/tests/suite/websocket_fallback.rs b/codex-rs/core/tests/suite/websocket_fallback.rs index 0090093c77..e7f33df148 100644 --- a/codex-rs/core/tests/suite/websocket_fallback.rs +++ b/codex-rs/core/tests/suite/websocket_fallback.rs @@ -156,6 +156,7 @@ async fn websocket_fallback_hides_first_websocket_retry_stream_error() -> Result final_output_json_schema: None, cwd: cwd.path().to_path_buf(), approval_policy: AskForApproval::Never, + approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, model: session_configured.model.clone(), effort: None, diff --git a/codex-rs/docs/protocol_v1.md b/codex-rs/docs/protocol_v1.md index 4d4e5c147c..9f238b40ee 100644 --- a/codex-rs/docs/protocol_v1.md +++ b/codex-rs/docs/protocol_v1.md @@ -65,7 +65,7 @@ Since only 1 `Task` can be run at a time, for parallel tasks it is recommended t For complete documentation of the `Op` and `EventMsg` variants, refer to [protocol.rs](../protocol/src/protocol.rs). Some example payload types: - `Op` - - `Op::UserTurn` – Any input from the user to kick off a `Turn` + - `Op::UserTurn` – Any input from the user to kick off a `Turn`, including full per-turn context such as cwd, model, sandbox, approval policy, and optional `approvals_reviewer` - `Op::UserInput` – Legacy form of user input - `Op::Interrupt` – Interrupts a running turn - `Op::ExecApproval` – Approve or deny code execution diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index e6b1ae79dc..b5c96f4d05 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -255,6 +255,11 @@ pub enum Op { /// Policy to use for command approval. approval_policy: AskForApproval, + /// Reviewer to use for approval requests raised during this turn. + /// + /// When omitted, the session keeps the current setting + approvals_reviewer: Option, + /// Policy to use for tool calls such as `local_shell`. sandbox_policy: SandboxPolicy, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index a553240170..ade9235b40 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -5260,6 +5260,7 @@ impl ChatWidget { items, cwd: self.config.cwd.clone(), approval_policy: self.config.permissions.approval_policy.value(), + approvals_reviewer: None, sandbox_policy: self.config.permissions.sandbox_policy.get().clone(), model: effective_mode.model().to_string(), effort: effective_mode.reasoning_effort(), diff --git a/codex-rs/tui_app_server/src/app.rs b/codex-rs/tui_app_server/src/app.rs index 6de3a2a34a..bb36d83949 100644 --- a/codex-rs/tui_app_server/src/app.rs +++ b/codex-rs/tui_app_server/src/app.rs @@ -2010,6 +2010,7 @@ impl App { items, cwd, approval_policy, + approvals_reviewer, sandbox_policy, model, effort, @@ -2042,7 +2043,8 @@ impl App { items.to_vec(), cwd.clone(), approval_policy, - self.chat_widget.config_ref().approvals_reviewer, + approvals_reviewer + .unwrap_or(self.chat_widget.config_ref().approvals_reviewer), sandbox_policy.clone(), model.to_string(), effort, diff --git a/codex-rs/tui_app_server/src/app_command.rs b/codex-rs/tui_app_server/src/app_command.rs index ed89ad86fb..e01a250274 100644 --- a/codex-rs/tui_app_server/src/app_command.rs +++ b/codex-rs/tui_app_server/src/app_command.rs @@ -42,6 +42,7 @@ pub(crate) enum AppCommandView<'a> { items: &'a [UserInput], cwd: &'a PathBuf, approval_policy: AskForApproval, + approvals_reviewer: &'a Option, sandbox_policy: &'a SandboxPolicy, model: &'a str, effort: Option, @@ -159,6 +160,7 @@ impl AppCommand { items, cwd, approval_policy, + approvals_reviewer: None, sandbox_policy, model, effort, @@ -303,6 +305,7 @@ impl AppCommand { items, cwd, approval_policy, + approvals_reviewer, sandbox_policy, model, effort, @@ -315,6 +318,7 @@ impl AppCommand { items, cwd, approval_policy: *approval_policy, + approvals_reviewer, sandbox_policy, model, effort: *effort, From 0f34b14b4193ae39fc2c39bad38645b646d448ca Mon Sep 17 00:00:00 2001 From: Charley Cunningham Date: Mon, 23 Mar 2026 15:36:23 -0700 Subject: [PATCH 5/8] [codex] Add rollback context duplication snapshot (#15562) ## What changed - adds a targeted snapshot test for rollback with contextual diffs in `codex_tests.rs` - snapshots the exact model-visible request input before the rolled-back turn and on the follow-up request after rollback - shows the duplicate developer and environment context pair appearing again before the follow-up user message ## Why Rollback currently rewinds the reference context baseline without rewinding the live session overrides. On the next turn, the same contextual diff is emitted again and duplicated in the request sent to the model. ## Impact - makes the regression visible in a canonical snapshot test - keeps the snapshot on the shared `context_snapshot` path without adding new formatting helpers - gives a direct repro for future fixes to rollback/context reconstruction --------- Co-authored-by: Codex --- .../core/tests/suite/compact_resume_fork.rs | 140 ++++++++++++++++++ ...lowup_turn_duplicates_context_updates.snap | 25 ++++ 2 files changed, 165 insertions(+) create mode 100644 codex-rs/core/tests/suite/snapshots/all__suite__compact_resume_fork__rollback_followup_turn_duplicates_context_updates.snap diff --git a/codex-rs/core/tests/suite/compact_resume_fork.rs b/codex-rs/core/tests/suite/compact_resume_fork.rs index fafbced0b7..0b7852af0e 100644 --- a/codex-rs/core/tests/suite/compact_resume_fork.rs +++ b/codex-rs/core/tests/suite/compact_resume_fork.rs @@ -16,6 +16,9 @@ use codex_core::ThreadManager; use codex_core::compact::SUMMARIZATION_PROMPT; use codex_core::config::Config; use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; +use codex_protocol::config_types::CollaborationMode; +use codex_protocol::config_types::ModeKind; +use codex_protocol::config_types::Settings; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::WarningEvent; @@ -502,6 +505,143 @@ async fn snapshot_rollback_past_compaction_replays_append_only_history() -> Resu Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +/// Scenario: rolling back a turn that introduced persistent pre-turn context +/// diffs currently duplicates those context updates on the next request. +async fn snapshot_rollback_followup_turn_duplicates_context_updates() -> Result<()> { + if network_disabled() { + println!("Skipping test because network is disabled in this sandbox"); + return Ok(()); + } + + const MODEL: &str = "gpt-5.1-codex"; + const TURN_ONE_USER: &str = "turn 1 user"; + const TURN_TWO_USER: &str = "turn 2 user"; + const FOLLOWUP_USER: &str = "follow-up user"; + const ROLLED_BACK_DEV_INSTRUCTIONS: &str = "ROLLED_BACK_DEV_INSTRUCTIONS"; + const PRETURN_CONTEXT_DIFF_CWD: &str = "PRETURN_CONTEXT_DIFF_CWD"; + + let server = MockServer::start().await; + let request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_assistant_message("m1", "turn 1 assistant"), + ev_completed("r1"), + ]), + sse(vec![ + ev_assistant_message("m2", "turn 2 assistant"), + ev_completed("r2"), + ]), + sse(vec![ev_completed("r3")]), + ], + ) + .await; + + let (_home, config, _manager, conversation) = + start_test_conversation(&server, Some(MODEL)).await; + + user_turn(&conversation, TURN_ONE_USER).await; + + let override_cwd = config.cwd.join(PRETURN_CONTEXT_DIFF_CWD); + std::fs::create_dir_all(&override_cwd)?; + conversation + .submit(Op::OverrideTurnContext { + cwd: Some(override_cwd), + approval_policy: None, + approvals_reviewer: None, + sandbox_policy: None, + windows_sandbox_level: None, + model: None, + effort: None, + summary: None, + service_tier: None, + collaboration_mode: Some(CollaborationMode { + mode: ModeKind::Default, + settings: Settings { + model: MODEL.to_string(), + reasoning_effort: None, + developer_instructions: Some(ROLLED_BACK_DEV_INSTRUCTIONS.to_string()), + }, + }), + personality: None, + }) + .await?; + + user_turn(&conversation, TURN_TWO_USER).await; + + conversation + .submit(Op::ThreadRollback { num_turns: 1 }) + .await?; + let rollback_event = wait_for_event(&conversation, |ev| { + matches!(ev, EventMsg::ThreadRolledBack(_)) + }) + .await; + let EventMsg::ThreadRolledBack(rollback_event) = rollback_event else { + panic!("expected thread rolled back event"); + }; + assert_eq!(rollback_event.num_turns, 1); + + user_turn(&conversation, FOLLOWUP_USER).await; + + let requests = request_log.requests(); + assert_eq!(requests.len(), 3); + + assert_eq!( + requests[1] + .message_input_texts("developer") + .iter() + .filter(|text| text.contains(ROLLED_BACK_DEV_INSTRUCTIONS)) + .count(), + 1 + ); + assert_eq!( + requests[1] + .message_input_texts("user") + .iter() + .filter(|text| text.contains(PRETURN_CONTEXT_DIFF_CWD)) + .count(), + 1 + ); + assert_eq!( + requests[2] + .message_input_texts("developer") + .iter() + .filter(|text| text.contains(ROLLED_BACK_DEV_INSTRUCTIONS)) + .count(), + 2 + ); + + let after_rollback_user_texts = requests[2].message_input_texts("user"); + assert_eq!( + after_rollback_user_texts + .iter() + .filter(|text| text.contains(PRETURN_CONTEXT_DIFF_CWD)) + .count(), + 2 + ); + assert_eq!( + after_rollback_user_texts.last().map(String::as_str), + Some(FOLLOWUP_USER) + ); + + insta::assert_snapshot!( + "rollback_followup_turn_duplicates_context_updates", + context_snapshot::format_labeled_requests_snapshot( + "rollback currently duplicates pre-turn override context updates on the follow-up request", + &[ + ("rolled-back turn request", &requests[1]), + ("follow-up request after rollback", &requests[2]), + ], + &ContextSnapshotOptions::default() + .strip_capability_instructions() + .render_mode(ContextSnapshotRenderMode::KindWithTextPrefix { max_chars: 96 }), + ) + ); + + Ok(()) +} + fn normalize_line_endings(value: &mut Value) { match value { Value::String(text) => { diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__compact_resume_fork__rollback_followup_turn_duplicates_context_updates.snap b/codex-rs/core/tests/suite/snapshots/all__suite__compact_resume_fork__rollback_followup_turn_duplicates_context_updates.snap new file mode 100644 index 0000000000..676421c373 --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__compact_resume_fork__rollback_followup_turn_duplicates_context_updates.snap @@ -0,0 +1,25 @@ +--- +source: core/tests/suite/compact_resume_fork.rs +expression: "context_snapshot::format_labeled_requests_snapshot(\"rollback currently duplicates pre-turn override context updates on the follow-up request\",\n&[(\"rolled-back turn request\", &requests[1]),\n(\"follow-up request after rollback\", &requests[2]),],\n&ContextSnapshotOptions::default().strip_capability_instructions().render_mode(ContextSnapshotRenderMode::KindWithTextPrefix\n{ max_chars: 96 }),)" +--- +Scenario: rollback currently duplicates pre-turn override context updates on the follow-up request + +## rolled-back turn request +00:message/developer: +01:message/user:> +02:message/user:turn 1 user +03:message/assistant:turn 1 assistant +04:message/developer:ROLLED_BACK_DEV_INSTRUCTIONS +05:message/user: +06:message/user:turn 2 user + +## follow-up request after rollback +00:message/developer: +01:message/user:> +02:message/user:turn 1 user +03:message/assistant:turn 1 assistant +04:message/developer:ROLLED_BACK_DEV_INSTRUCTIONS +05:message/user: +06:message/developer:ROLLED_BACK_DEV_INSTRUCTIONS +07:message/user: +08:message/user:follow-up user From 4605c653085ac1ea3a4c48e4d1727022bd942f68 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 23 Mar 2026 22:56:17 +0000 Subject: [PATCH 6/8] feat: custom watcher for multi-agent v2 (#15570) Custom watcher that sends an InterAgentCommunication on end of turn --- codex-rs/core/src/agent/control.rs | 36 +++ codex-rs/core/src/agent/control_tests.rs | 243 ++++++++++++++++++ .../src/codex/rollout_reconstruction_tests.rs | 16 +- .../src/tools/handlers/multi_agents_tests.rs | 74 +++--- 4 files changed, 327 insertions(+), 42 deletions(-) diff --git a/codex-rs/core/src/agent/control.rs b/codex-rs/core/src/agent/control.rs index 3bdcc2efd9..282e29c48a 100644 --- a/codex-rs/core/src/agent/control.rs +++ b/codex-rs/core/src/agent/control.rs @@ -266,6 +266,7 @@ impl AgentControl { new_thread.thread_id, notification_source, child_reference, + agent_metadata.agent_path.clone(), ); Ok(LiveAgent { @@ -437,6 +438,7 @@ impl AgentControl { resumed_thread.thread_id, Some(notification_source.clone()), child_reference, + agent_metadata.agent_path.clone(), ); self.persist_thread_spawn_edge_for_source( resumed_thread.thread.as_ref(), @@ -687,6 +689,7 @@ impl AgentControl { child_thread_id: ThreadId, session_source: Option, child_reference: String, + child_agent_path: Option, ) { let Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { parent_thread_id, .. @@ -717,6 +720,39 @@ impl AgentControl { let Ok(state) = control.upgrade() else { return; }; + let child_thread = state.get_thread(child_thread_id).await.ok(); + if let Some(child_agent_path) = child_agent_path + && child_thread + .as_ref() + .map(|thread| thread.enabled(Feature::MultiAgentV2)) + .unwrap_or(true) + { + let AgentStatus::Completed(Some(content)) = &status else { + return; + }; + let Some((parent_path, _)) = child_agent_path.as_str().rsplit_once('/') else { + return; + }; + let Ok(parent_agent_path) = AgentPath::try_from(parent_path) else { + return; + }; + let Some(parent_thread_id) = control.state.agent_id_for_path(&parent_agent_path) + else { + return; + }; + let _ = control + .send_inter_agent_communication( + parent_thread_id, + InterAgentCommunication::new( + child_agent_path, + parent_agent_path, + Vec::new(), + content.clone(), + ), + ) + .await; + return; + } let Ok(parent_thread) = state.get_thread(parent_thread_id).await else { return; }; diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index 677bf5a2f3..2cf7398f7d 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -11,11 +11,13 @@ use crate::contextual_user_message::SUBAGENT_NOTIFICATION_OPEN_TAG; use assert_matches::assert_matches; use chrono::Utc; use codex_features::Feature; +use codex_protocol::AgentPath; use codex_protocol::config_types::ModeKind; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::TurnAbortReason; @@ -125,6 +127,29 @@ fn history_contains_text(history_items: &[ResponseItem], needle: &str) -> bool { }) } +fn history_contains_assistant_inter_agent_communication( + history_items: &[ResponseItem], + expected: &InterAgentCommunication, +) -> bool { + history_items.iter().any(|item| { + let ResponseItem::Message { role, content, .. } = item else { + return false; + }; + if role != "assistant" { + return false; + } + content.iter().any(|content_item| match content_item { + ContentItem::OutputText { text } => { + serde_json::from_str::(text) + .ok() + .as_ref() + == Some(expected) + } + ContentItem::InputText { .. } | ContentItem::InputImage { .. } => false, + }) + }) +} + async fn wait_for_subagent_notification(parent_thread: &Arc) -> bool { let wait = async { loop { @@ -937,6 +962,223 @@ async fn spawn_child_completion_notifies_parent_history() { assert_eq!(wait_for_subagent_notification(&parent_thread).await, true); } +#[tokio::test] +async fn multi_agent_v2_completion_sends_inter_agent_message_to_direct_parent() { + let harness = AgentControlHarness::new().await; + let (root_thread_id, _) = harness.start_thread().await; + let mut config = harness.config.clone(); + let _ = config.features.enable(Feature::MultiAgentV2); + let worker_path = AgentPath::root().join("worker_a").expect("worker path"); + let worker_thread_id = harness + .control + .spawn_agent( + config.clone(), + text_input("hello worker"), + Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id: root_thread_id, + depth: 1, + agent_path: Some(worker_path.clone()), + agent_nickname: None, + agent_role: Some("explorer".to_string()), + })), + ) + .await + .expect("worker spawn should succeed"); + let tester_path = worker_path.join("tester").expect("tester path"); + let tester_thread_id = harness + .control + .spawn_agent( + config, + text_input("hello tester"), + Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id: worker_thread_id, + depth: 2, + agent_path: Some(tester_path.clone()), + agent_nickname: None, + agent_role: Some("explorer".to_string()), + })), + ) + .await + .expect("tester spawn should succeed"); + + let tester_thread = harness + .manager + .get_thread(tester_thread_id) + .await + .expect("tester thread should exist"); + let tester_turn = tester_thread.codex.session.new_default_turn().await; + tester_thread + .codex + .session + .send_event( + tester_turn.as_ref(), + EventMsg::TurnComplete(TurnCompleteEvent { + turn_id: tester_turn.sub_id.clone(), + last_agent_message: Some("done".to_string()), + }), + ) + .await; + + timeout(Duration::from_secs(2), async { + loop { + let delivered = harness + .manager + .captured_ops() + .into_iter() + .any(|(thread_id, op)| { + thread_id == worker_thread_id + && matches!( + op, + Op::InterAgentCommunication { communication } + if communication.author == tester_path + && communication.recipient == worker_path + && communication.other_recipients.is_empty() + && communication.content == "done" + ) + }); + if delivered { + break; + } + sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("completion watcher should send inter-agent communication"); + + let worker_thread = harness + .manager + .get_thread(worker_thread_id) + .await + .expect("worker thread should exist"); + let expected_message = InterAgentCommunication::new( + tester_path.clone(), + worker_path.clone(), + Vec::new(), + "done".to_string(), + ); + timeout(Duration::from_secs(2), async { + loop { + let history_items = worker_thread + .codex + .session + .clone_history() + .await + .raw_items() + .to_vec(); + if history_contains_assistant_inter_agent_communication( + &history_items, + &expected_message, + ) && !has_subagent_notification(&history_items) + { + break; + } + sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("worker should record assistant inter-agent message"); +} + +#[tokio::test] +async fn multi_agent_v2_completion_ignores_dead_direct_parent() { + let harness = AgentControlHarness::new().await; + let (root_thread_id, root_thread) = harness.start_thread().await; + let mut config = harness.config.clone(); + let _ = config.features.enable(Feature::MultiAgentV2); + let worker_path = AgentPath::root().join("worker_a").expect("worker path"); + let worker_thread_id = harness + .control + .spawn_agent( + config.clone(), + text_input("hello worker"), + Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id: root_thread_id, + depth: 1, + agent_path: Some(worker_path.clone()), + agent_nickname: None, + agent_role: Some("explorer".to_string()), + })), + ) + .await + .expect("worker spawn should succeed"); + let tester_path = worker_path.join("tester").expect("tester path"); + let tester_thread_id = harness + .control + .spawn_agent( + config, + text_input("hello tester"), + Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id: worker_thread_id, + depth: 2, + agent_path: Some(tester_path.clone()), + agent_nickname: None, + agent_role: Some("explorer".to_string()), + })), + ) + .await + .expect("tester spawn should succeed"); + harness + .control + .shutdown_live_agent(worker_thread_id) + .await + .expect("worker shutdown should succeed"); + + let tester_thread = harness + .manager + .get_thread(tester_thread_id) + .await + .expect("tester thread should exist"); + let tester_turn = tester_thread.codex.session.new_default_turn().await; + tester_thread + .codex + .session + .send_event( + tester_turn.as_ref(), + EventMsg::TurnComplete(TurnCompleteEvent { + turn_id: tester_turn.sub_id.clone(), + last_agent_message: Some("done".to_string()), + }), + ) + .await; + + sleep(Duration::from_millis(100)).await; + + assert!( + !harness + .manager + .captured_ops() + .into_iter() + .any(|(thread_id, op)| { + thread_id == worker_thread_id + && matches!( + op, + Op::InterAgentCommunication { communication } + if communication.author == tester_path + && communication.recipient == worker_path + && communication.content == "done" + ) + }) + ); + + let root_history_items = root_thread + .codex + .session + .clone_history() + .await + .raw_items() + .to_vec(); + assert!(!history_contains_assistant_inter_agent_communication( + &root_history_items, + &InterAgentCommunication::new( + tester_path, + AgentPath::root(), + Vec::new(), + "done".to_string(), + ) + )); + assert!(!has_subagent_notification(&root_history_items)); +} + #[tokio::test] async fn completion_watcher_notifies_parent_when_child_is_missing() { let harness = AgentControlHarness::new().await; @@ -953,6 +1195,7 @@ async fn completion_watcher_notifies_parent_when_child_is_missing() { agent_role: Some("explorer".to_string()), })), child_thread_id.to_string(), + None, ); assert_eq!(wait_for_subagent_notification(&parent_thread).await, true); diff --git a/codex-rs/core/src/codex/rollout_reconstruction_tests.rs b/codex-rs/core/src/codex/rollout_reconstruction_tests.rs index 96423754db..33fb5f9af7 100644 --- a/codex-rs/core/src/codex/rollout_reconstruction_tests.rs +++ b/codex-rs/core/src/codex/rollout_reconstruction_tests.rs @@ -36,19 +36,17 @@ fn assistant_message(text: &str) -> ResponseItem { } fn inter_agent_assistant_message(text: &str) -> ResponseItem { + let communication = InterAgentCommunication::new( + AgentPath::root(), + AgentPath::root().join("worker").unwrap(), + Vec::new(), + text.to_string(), + ); ResponseItem::Message { id: None, role: "assistant".to_string(), content: vec![ContentItem::OutputText { - text: serde_json::to_string(&InterAgentCommunication::new( - AgentPath::root(), - AgentPath::root() - .join("worker") - .expect("worker path should be valid"), - Vec::new(), - text.to_string(), - )) - .expect("inter-agent communication should serialize"), + text: serde_json::to_string(&communication).unwrap(), }], end_turn: None, phase: None, diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index f74ebf0577..b780986d98 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -80,14 +80,31 @@ fn thread_manager() -> ThreadManager { ) } +fn history_contains_inter_agent_communication( + history_items: &[ResponseItem], + expected: &InterAgentCommunication, +) -> bool { + history_items.iter().any(|item| { + let ResponseItem::Message { role, content, .. } = item else { + return false; + }; + if role != "assistant" { + return false; + } + content.iter().any(|content_item| match content_item { + ContentItem::OutputText { text } => { + serde_json::from_str::(text) + .ok() + .as_ref() + == Some(expected) + } + ContentItem::InputText { .. } | ContentItem::InputImage { .. } => false, + }) + }) +} + fn inter_agent_message_text(recipient: &str, content: &str) -> String { - serde_json::to_string(&InterAgentCommunication::new( - AgentPath::root(), - AgentPath::try_from(recipient).expect("recipient path should be valid"), - Vec::new(), - content.to_string(), - )) - .expect("inter-agent communication should serialize") + format!("author: /root\nrecipient: {recipient}\nother_recipients: []\nContent: {content}") } #[derive(Clone, Copy)] @@ -401,7 +418,12 @@ async fn multi_agent_v2_spawn_returns_path_and_send_input_accepts_relative_path( .get_thread(child_thread_id) .await .expect("child thread should exist"); - let expected_message = inter_agent_message_text("/root/test_process", "continue"); + let expected_communication = InterAgentCommunication::new( + AgentPath::root(), + AgentPath::try_from("/root/test_process").expect("agent path"), + Vec::new(), + "continue".to_string(), + ); timeout(Duration::from_secs(2), async { loop { let history_items = child_thread @@ -411,18 +433,8 @@ async fn multi_agent_v2_spawn_returns_path_and_send_input_accepts_relative_path( .await .raw_items() .to_vec(); - let recorded = history_items.iter().any(|item| { - matches!( - item, - ResponseItem::Message { role, content, .. } - if role == "assistant" - && content.iter().any(|content_item| matches!( - content_item, - ContentItem::OutputText { text } - if text == &expected_message - )) - ) - }); + let recorded = + history_contains_inter_agent_communication(&history_items, &expected_communication); let saw_user_message = history_items.iter().any(|item| { matches!( item, @@ -664,7 +676,6 @@ async fn multi_agent_v2_send_input_interrupts_busy_child_without_losing_message( )) ))); - let expected_message = inter_agent_message_text("/root/worker", "continue"); timeout(Duration::from_secs(5), async { loop { let history_items = thread @@ -674,18 +685,15 @@ async fn multi_agent_v2_send_input_interrupts_busy_child_without_losing_message( .await .raw_items() .to_vec(); - let saw_envelope = history_items.iter().any(|item| { - matches!( - item, - ResponseItem::Message { role, content, .. } - if role == "assistant" - && content.iter().any(|content_item| matches!( - content_item, - ContentItem::OutputText { text } - if text == &expected_message - )) - ) - }); + let saw_envelope = history_contains_inter_agent_communication( + &history_items, + &InterAgentCommunication::new( + AgentPath::root(), + AgentPath::try_from("/root/worker").expect("agent path"), + Vec::new(), + "continue".to_string(), + ), + ); let saw_user_message = history_items.iter().any(|item| { matches!( item, From 0b5ba25b467869c35891c9c2b38178a5d13011bd Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 23 Mar 2026 22:57:54 +0000 Subject: [PATCH 7/8] feat: custom watcher for multi-agent v2 (#15575) From 527244910fb851cea6147334dbc08f8fbce4cb9d Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 23 Mar 2026 23:27:55 +0000 Subject: [PATCH 8/8] feat: custom watcher for multi-agent v2 (#15576) The new wait tool just returns `Wait timed out.` or `Wait completed.`. The actual content is done through the notification watcher --- .../src/tools/handlers/multi_agents_tests.rs | 71 ++++++++++++++++--- .../tools/handlers/multi_agents_v2/wait.rs | 38 +++++----- codex-rs/core/src/tools/spec.rs | 65 +++++++++++------ codex-rs/core/src/tools/spec_tests.rs | 6 +- 4 files changed, 123 insertions(+), 57 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index b780986d98..873b55d660 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -9,12 +9,14 @@ use crate::config::types::ShellEnvironmentPolicy; use crate::function_tool::FunctionCallError; use crate::protocol::AgentStatus; use crate::protocol::AskForApproval; +use crate::protocol::EventMsg; use crate::protocol::FileSystemSandboxPolicy; use crate::protocol::NetworkSandboxPolicy; use crate::protocol::Op; use crate::protocol::SandboxPolicy; use crate::protocol::SessionSource; use crate::protocol::SubAgentSource; +use crate::protocol::TurnCompleteEvent; use crate::state::TaskKind; use crate::tasks::SessionTask; use crate::tasks::SessionTaskContext; @@ -1414,7 +1416,7 @@ async fn multi_agent_v2_wait_agent_accepts_targets_argument() { assert_eq!( result, crate::tools::handlers::multi_agents_v2::wait::WaitAgentResult { - status: HashMap::from([(target, AgentStatus::NotFound)]), + message: "Wait completed.".to_string(), timed_out: false, } ); @@ -1582,12 +1584,7 @@ async fn wait_agent_returns_final_status_without_timeout() { } #[tokio::test] -async fn multi_agent_v2_wait_agent_returns_statuses_keyed_by_path() { - #[derive(Debug, Deserialize)] - struct SpawnAgentResult { - task_name: String, - } - +async fn multi_agent_v2_wait_agent_returns_summary_for_named_targets() { let (mut session, mut turn) = make_session_and_context().await; let manager = thread_manager(); let root = manager @@ -1617,9 +1614,7 @@ async fn multi_agent_v2_wait_agent_returns_statuses_keyed_by_path() { )) .await .expect("spawn_agent should succeed"); - let (content, _) = expect_text_output(spawn_output); - let spawn_result: SpawnAgentResult = - serde_json::from_str(&content).expect("spawn result should parse"); + let _ = expect_text_output(spawn_output); let agent_id = session .services @@ -1667,13 +1662,67 @@ async fn multi_agent_v2_wait_agent_returns_statuses_keyed_by_path() { assert_eq!( result, crate::tools::handlers::multi_agents_v2::wait::WaitAgentResult { - status: HashMap::from([(spawn_result.task_name, AgentStatus::Shutdown)]), + message: "Wait completed.".to_string(), timed_out: false, } ); assert_eq!(success, None); } +#[tokio::test] +async fn multi_agent_v2_wait_agent_does_not_return_completed_content() { + let (mut session, mut turn) = make_session_and_context().await; + let manager = thread_manager(); + session.services.agent_control = manager.agent_control(); + let mut config = (*turn.config).clone(); + config + .features + .enable(Feature::MultiAgentV2) + .expect("test config should allow feature update"); + turn.config = Arc::new(config.clone()); + + let thread = manager.start_thread(config).await.expect("start thread"); + let agent_id = thread.thread_id; + let child_turn = thread.thread.codex.session.new_default_turn().await; + thread + .thread + .codex + .session + .send_event( + child_turn.as_ref(), + EventMsg::TurnComplete(TurnCompleteEvent { + turn_id: child_turn.sub_id.clone(), + last_agent_message: Some("sensitive child output".to_string()), + }), + ) + .await; + + let output = WaitAgentHandlerV2 + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "wait_agent", + function_payload(json!({ + "targets": [agent_id.to_string()], + "timeout_ms": 1000 + })), + )) + .await + .expect("wait_agent should succeed"); + let (content, success) = expect_text_output(output); + let result: crate::tools::handlers::multi_agents_v2::wait::WaitAgentResult = + serde_json::from_str(&content).expect("wait_agent result should be json"); + assert_eq!( + result, + crate::tools::handlers::multi_agents_v2::wait::WaitAgentResult { + message: "Wait completed.".to_string(), + timed_out: false, + } + ); + assert!(!content.contains("sensitive child output")); + assert_eq!(success, None); +} + #[tokio::test] async fn close_agent_submits_shutdown_and_returns_previous_status() { let (mut session, turn) = make_session_and_context().await; diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs index e4bc7d3f4a..1b1dc90e42 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs @@ -35,21 +35,12 @@ impl ToolHandler for Handler { let args: WaitArgs = parse_arguments(&arguments)?; let receiver_thread_ids = resolve_agent_targets(&session, &turn, args.targets).await?; let mut receiver_agents = Vec::with_capacity(receiver_thread_ids.len()); - let mut target_by_thread_id = HashMap::with_capacity(receiver_thread_ids.len()); for receiver_thread_id in &receiver_thread_ids { let agent_metadata = session .services .agent_control .get_agent_metadata(*receiver_thread_id) .unwrap_or_default(); - target_by_thread_id.insert( - *receiver_thread_id, - agent_metadata - .agent_path - .as_ref() - .map(ToString::to_string) - .unwrap_or_else(|| receiver_thread_id.to_string()), - ); receiver_agents.push(CollabAgentRef { thread_id: *receiver_thread_id, agent_nickname: agent_metadata.agent_nickname, @@ -152,18 +143,7 @@ impl ToolHandler for Handler { let timed_out = statuses.is_empty(); let statuses_by_id = statuses.clone().into_iter().collect::>(); let agent_statuses = build_wait_agent_statuses(&statuses_by_id, &receiver_agents); - let result = WaitAgentResult { - status: statuses - .into_iter() - .filter_map(|(thread_id, status)| { - target_by_thread_id - .get(&thread_id) - .cloned() - .map(|target| (target, status)) - }) - .collect(), - timed_out, - }; + let result = WaitAgentResult::from_timed_out(timed_out); session .send_event( @@ -191,10 +171,24 @@ struct WaitArgs { #[derive(Debug, Deserialize, Serialize, PartialEq, Eq)] pub(crate) struct WaitAgentResult { - pub(crate) status: HashMap, + pub(crate) message: String, pub(crate) timed_out: bool, } +impl WaitAgentResult { + fn from_timed_out(timed_out: bool) -> Self { + let message = if timed_out { + "Wait timed out." + } else { + "Wait completed." + }; + Self { + message: message.to_string(), + timed_out, + } + } +} + impl ToolOutput for WaitAgentResult { fn log_preview(&self) -> String { tool_output_json_text(self, "wait_agent") diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index d8419e1f58..b115e572e6 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -178,23 +178,41 @@ fn resume_agent_output_schema() -> JsonValue { }) } -fn wait_output_schema() -> JsonValue { - json!({ - "type": "object", - "properties": { - "status": { - "type": "object", - "description": "Final statuses keyed by canonical task name when available, otherwise by agent id.", - "additionalProperties": agent_status_output_schema() +fn wait_output_schema(multi_agent_v2: bool) -> JsonValue { + if multi_agent_v2 { + json!({ + "type": "object", + "properties": { + "message": { + "type": "string", + "description": "Brief wait summary without the agent's final content." + }, + "timed_out": { + "type": "boolean", + "description": "Whether the wait call returned due to timeout before any agent reached a final status." + } }, - "timed_out": { - "type": "boolean", - "description": "Whether the wait call returned due to timeout before any agent reached a final status." - } - }, - "required": ["status", "timed_out"], - "additionalProperties": false - }) + "required": ["message", "timed_out"], + "additionalProperties": false + }) + } else { + json!({ + "type": "object", + "properties": { + "status": { + "type": "object", + "description": "Final statuses keyed by canonical task name when available, otherwise by agent id.", + "additionalProperties": agent_status_output_schema() + }, + "timed_out": { + "type": "boolean", + "description": "Whether the wait call returned due to timeout before any agent reached a final status." + } + }, + "required": ["status", "timed_out"], + "additionalProperties": false + }) + } } fn close_agent_output_schema() -> JsonValue { @@ -1422,7 +1440,7 @@ fn create_resume_agent_tool() -> ToolSpec { }) } -fn create_wait_agent_tool() -> ToolSpec { +fn create_wait_agent_tool(multi_agent_v2: bool) -> ToolSpec { let mut properties = BTreeMap::new(); properties.insert( "targets".to_string(), @@ -1445,8 +1463,13 @@ fn create_wait_agent_tool() -> ToolSpec { ToolSpec::Function(ResponsesApiTool { name: "wait_agent".to_string(), - description: "Wait for agents to reach a final status. Completed statuses may include the agent's final message. Returns empty status when timed out. Once the agent reaches a final status, a notification message will be received containing the same completed status." - .to_string(), + description: if multi_agent_v2 { + "Wait for agents to reach a final status. Returns a brief wait summary instead of the agent's final content. Returns a timeout summary when no agent reaches a final status before the deadline." + .to_string() + } else { + "Wait for agents to reach a final status. Completed statuses may include the agent's final message. Returns empty status when timed out. Once the agent reaches a final status, a notification message will be received containing the same completed status." + .to_string() + }, strict: false, defer_loading: None, parameters: JsonSchema::Object { @@ -1454,7 +1477,7 @@ fn create_wait_agent_tool() -> ToolSpec { required: Some(vec!["targets".to_string()]), additional_properties: Some(false.into()), }, - output_schema: Some(wait_output_schema()), + output_schema: Some(wait_output_schema(multi_agent_v2)), }) } @@ -3006,7 +3029,7 @@ pub(crate) fn build_specs_with_discoverable_tools( } push_tool_spec( &mut builder, - create_wait_agent_tool(), + create_wait_agent_tool(config.multi_agent_v2), /*supports_parallel_tool_calls*/ false, config.code_mode_enabled, ); diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index 1cb6bb1664..b33748dd35 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -469,7 +469,7 @@ fn test_full_toolset_specs_for_gpt5_codex_unified_exec_web_search() { create_view_image_tool(config.can_request_original_image_detail), create_spawn_agent_tool(&config), create_send_input_tool(), - create_wait_agent_tool(), + create_wait_agent_tool(config.multi_agent_v2), create_close_agent_tool(), ] { expected.insert(tool_name(&spec).to_string(), spec); @@ -607,8 +607,8 @@ fn test_build_specs_multi_agent_v2_uses_task_names_and_hides_resume() { .as_ref() .expect("wait_agent should define output schema"); assert_eq!( - output_schema["properties"]["status"]["description"], - json!("Final statuses keyed by canonical task name when available, otherwise by agent id.") + output_schema["properties"]["message"]["description"], + json!("Brief wait summary without the agent's final content.") ); assert_lacks_tool_name(&tools, "resume_agent"); }