From bc4b55c09c0c4beccc1cac4825f21856272edac6 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 4 Jun 2026 12:26:45 -0700 Subject: [PATCH] Simplify agent hook sessions --- codex-rs/core/src/codex_delegate.rs | 15 +----- codex-rs/core/src/hook_agent.rs | 10 ---- codex-rs/core/src/hook_agent_tests.rs | 13 ----- codex-rs/core/src/session/mod.rs | 56 +++++++--------------- codex-rs/core/src/session/review.rs | 1 - codex-rs/core/src/session/session.rs | 2 - codex-rs/core/src/session/tests.rs | 8 ---- codex-rs/core/src/session/turn.rs | 24 ---------- codex-rs/core/src/session/turn_context.rs | 3 -- codex-rs/core/src/session/turn_tests.rs | 18 ------- codex-rs/hooks/src/engine/discovery.rs | 30 ++++++++++++ codex-rs/hooks/src/engine/prompt_runner.rs | 2 +- 12 files changed, 50 insertions(+), 132 deletions(-) diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index b8b54f4be5..678a6e5d10 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -79,17 +79,6 @@ pub(crate) async fn run_codex_thread_interactive( let (tx_ops, rx_ops) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let conversation_history = initial_history.unwrap_or(InitialHistory::New); let forked_from_thread_id = conversation_history.forked_from_id(); - let is_agent_hook = crate::hook_agent::is_agent_hook_subagent_source(&subagent_source); - let extensions = if is_agent_hook { - codex_extension_api::empty_extension_registry() - } else { - Arc::clone(&parent_session.services.extensions) - }; - let environment_selections = if is_agent_hook { - Default::default() - } else { - parent_ctx.environments.clone() - }; let CodexSpawnOk { codex, .. } = Box::pin(Codex::spawn(CodexSpawnArgs { config, installation_id: parent_session.installation_id.clone(), @@ -99,7 +88,7 @@ pub(crate) async fn run_codex_thread_interactive( skills_manager: Arc::clone(&parent_session.services.skills_manager), plugins_manager: Arc::clone(&parent_session.services.plugins_manager), mcp_manager: Arc::clone(&parent_session.services.mcp_manager), - extensions, + extensions: Arc::clone(&parent_session.services.extensions), conversation_history, session_source: SessionSource::SubAgent(subagent_source.clone()), forked_from_thread_id, @@ -113,7 +102,7 @@ pub(crate) async fn run_codex_thread_interactive( inherited_exec_policy: Some(Arc::clone(&parent_session.services.exec_policy)), parent_rollout_thread_trace: codex_rollout_trace::ThreadTraceContext::disabled(), parent_trace: None, - environment_selections, + environment_selections: parent_ctx.environments.clone(), analytics_events_client: Some(parent_session.services.analytics_events_client.clone()), thread_store: Arc::clone(&parent_session.services.thread_store), attestation_provider: parent_session.services.attestation_provider.clone(), diff --git a/codex-rs/core/src/hook_agent.rs b/codex-rs/core/src/hook_agent.rs index 2ae10a000f..ca0d876671 100644 --- a/codex-rs/core/src/hook_agent.rs +++ b/codex-rs/core/src/hook_agent.rs @@ -8,7 +8,6 @@ use codex_hooks::AgentHookRunner; use codex_protocol::config_types::WebSearchMode; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use codex_protocol::user_input::UserInput; use tokio_util::sync::CancellationToken; @@ -19,7 +18,6 @@ use crate::config::Constrained; use crate::session::session::Session; use crate::session::turn_context::TurnContext; -pub(crate) const AGENT_HOOK_MAX_MODEL_REQUESTS: usize = 50; const AGENT_HOOK_SOURCE_NAME: &str = "agent_hook"; const AGENT_HOOK_BASE_INSTRUCTIONS: &str = r#"You evaluate a Codex agent hook. @@ -32,14 +30,6 @@ or Use ok:false only when the hook criteria fail. Do not answer the user's task. Do not include Markdown or extra text."#; -pub(crate) fn is_agent_hook_source(session_source: &SessionSource) -> bool { - matches!(session_source, SessionSource::SubAgent(source) if is_agent_hook_subagent_source(source)) -} - -pub(crate) fn is_agent_hook_subagent_source(source: &SubAgentSource) -> bool { - matches!(source, SubAgentSource::Other(name) if name == AGENT_HOOK_SOURCE_NAME) -} - pub(crate) fn build_agent_hook_runner( parent_session: Arc, parent_turn: Arc, diff --git a/codex-rs/core/src/hook_agent_tests.rs b/codex-rs/core/src/hook_agent_tests.rs index 841d438f88..45aede1c6a 100644 --- a/codex-rs/core/src/hook_agent_tests.rs +++ b/codex-rs/core/src/hook_agent_tests.rs @@ -1,23 +1,10 @@ use codex_features::Feature; use codex_protocol::config_types::WebSearchMode; use codex_protocol::protocol::AskForApproval; -use codex_protocol::protocol::SessionSource; -use codex_protocol::protocol::SubAgentSource; use pretty_assertions::assert_eq; use super::*; -#[test] -fn recognizes_only_agent_hook_subagent_source() { - assert!(is_agent_hook_source(&SessionSource::SubAgent( - SubAgentSource::Other("agent_hook".to_string()) - ))); - assert!(!is_agent_hook_source(&SessionSource::SubAgent( - SubAgentSource::Other("other".to_string()) - ))); - assert!(!is_agent_hook_source(&SessionSource::Cli)); -} - #[tokio::test] async fn agent_hook_config_is_isolated_and_inherits_active_permissions() { let (_, turn) = crate::session::tests::make_session_and_context().await; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index aa79f8380c..0d4506d178 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -505,20 +505,15 @@ impl Codex { } = args; let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_event, rx_event) = async_channel::unbounded(); - let is_agent_hook = crate::hook_agent::is_agent_hook_source(&session_source); let primary_environment = environment_selections.primary_environment(); let mut user_instruction_warnings = Vec::new(); - let user_instructions = if is_agent_hook { - None - } else { - AgentsMdManager::new(&config) - .user_instructions( - primary_environment.as_deref(), - &mut user_instruction_warnings, - ) - .await - }; + let user_instructions = AgentsMdManager::new(&config) + .user_instructions( + primary_environment.as_deref(), + &mut user_instruction_warnings, + ) + .await; config.startup_warnings.extend(user_instruction_warnings); let exec_policy = if crate::guardian::is_guardian_reviewer_source(&session_source) { @@ -596,8 +591,6 @@ impl Codex { config.features.enabled(Feature::FastMode), &model_info, ); - let max_model_requests_per_turn = - is_agent_hook.then_some(crate::hook_agent::AGENT_HOOK_MAX_MODEL_REQUESTS); let session_configuration = SessionConfiguration { provider: config.model_provider.clone(), collaboration_mode, @@ -628,7 +621,6 @@ impl Codex { dynamic_tools, inherited_shell_snapshot, user_shell_override, - max_model_requests_per_turn, }; // Generate a unique ID for the lifetime of this Codex session. @@ -1491,7 +1483,7 @@ impl Session { // layers such as request/session overrides that were present when this session // was created. let notify_config_contributors = !self.services.extensions.config_contributors().is_empty(); - let (previous_config, new_config, config, service_tier, session_source) = { + let (previous_config, new_config, config, service_tier) = { let mut state = self.state.lock().await; let previous_config = notify_config_contributors .then(|| Self::build_effective_session_config(&state.session_configuration)); @@ -1506,14 +1498,7 @@ impl Session { state.session_configuration.original_config_do_not_use = Arc::clone(&config); let new_config = notify_config_contributors .then(|| Self::build_effective_session_config(&state.session_configuration)); - let session_source = state.session_configuration.session_source.clone(); - ( - previous_config, - new_config, - config, - service_tier, - session_source, - ) + (previous_config, new_config, config, service_tier) }; self.emit_config_changed_contributors(previous_config.as_ref(), new_config.as_ref()); self.services.skills_manager.clear_cache(); @@ -1530,7 +1515,6 @@ impl Session { self.services.plugins_manager.as_ref(), self.services.user_shell.as_ref(), Some(prompt_hook_runner), - &session_source, ) .await; @@ -2879,17 +2863,15 @@ impl Session { developer_sections.push(skills_instructions.render()); } } - if !crate::hook_agent::is_agent_hook_source(&turn_context.session_source) { - let loaded_plugins = self - .services - .plugins_manager - .plugins_for_config(&turn_context.config.plugins_config_input()) - .await; - if let Some(plugin_instructions) = - AvailablePluginsInstructions::from_plugins(loaded_plugins.capability_summaries()) - { - developer_sections.push(plugin_instructions.render()); - } + let loaded_plugins = self + .services + .plugins_manager + .plugins_for_config(&turn_context.config.plugins_config_input()) + .await; + if let Some(plugin_instructions) = + AvailablePluginsInstructions::from_plugins(loaded_plugins.capability_summaries()) + { + developer_sections.push(plugin_instructions.render()); } let context_contributors = self.services.extensions.context_contributors().to_vec(); for contributor in context_contributors { @@ -3411,11 +3393,7 @@ async fn build_hooks_for_config( plugins_manager: &PluginsManager, user_shell: &crate::shell::Shell, prompt_hook_runner: Option, - session_source: &SessionSource, ) -> Hooks { - if crate::hook_agent::is_agent_hook_source(session_source) { - return Hooks::default(); - } let mut hook_shell_argv = user_shell.derive_exec_args("", /*use_login_shell*/ false); let hook_shell_program = hook_shell_argv.remove(0); let _ = hook_shell_argv.pop(); diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 21c72cac48..fe78e033ec 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -138,7 +138,6 @@ pub(super) async fn spawn_review_thread( #[allow(deprecated)] cwd: parent_turn_context.cwd.clone(), final_output_json_schema: None, - max_model_requests: parent_turn_context.max_model_requests, codex_self_exe: parent_turn_context.codex_self_exe.clone(), codex_linux_sandbox_exe: parent_turn_context.codex_linux_sandbox_exe.clone(), dynamic_tools: parent_turn_context.dynamic_tools.clone(), diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 4595380dd0..b3f0559c59 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -107,7 +107,6 @@ pub(crate) struct SessionConfiguration { pub(super) dynamic_tools: Vec, pub(super) inherited_shell_snapshot: Option>, pub(super) user_shell_override: Option, - pub(super) max_model_requests_per_turn: Option, } impl SessionConfiguration { @@ -989,7 +988,6 @@ impl Session { plugins_manager.as_ref(), &default_shell, Some(prompt_hook_runner), - &session_configuration.session_source, ) .await; for warning in hooks.startup_warnings() { diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 0926cff519..a3e5558971 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -3187,7 +3187,6 @@ async fn set_rate_limits_retains_previous_credits() { dynamic_tools: Vec::new(), inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, }; let mut state = SessionState::new(session_configuration); @@ -3296,7 +3295,6 @@ async fn set_rate_limits_updates_plan_type_when_present() { dynamic_tools: Vec::new(), inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, }; let mut state = SessionState::new(session_configuration); @@ -3829,7 +3827,6 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati dynamic_tools: Vec::new(), inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, } } @@ -4574,7 +4571,6 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { dynamic_tools: Vec::new(), inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, }; let (tx_event, _rx_event) = async_channel::unbounded(); @@ -4686,7 +4682,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { dynamic_tools: Vec::new(), inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, }; let per_turn_config = Session::build_per_turn_config(&session_configuration, session_configuration.cwd.clone()); @@ -4925,7 +4920,6 @@ async fn make_session_with_config_and_rx( dynamic_tools: Vec::new(), inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, }; let (tx_event, rx_event) = async_channel::unbounded(); @@ -5031,7 +5025,6 @@ async fn make_session_with_history_source_and_agent_control_and_rx( dynamic_tools: Vec::new(), inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, }; let (tx_event, rx_event) = async_channel::unbounded(); @@ -6779,7 +6772,6 @@ where dynamic_tools, inherited_shell_snapshot: None, user_shell_override: None, - max_model_requests_per_turn: None, }; let per_turn_config = Session::build_per_turn_config(&session_configuration, session_configuration.cwd.clone()); diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 7e8452d6af..8e20580291 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -193,7 +193,6 @@ pub(crate) async fn run_turn( let mut last_agent_message: Option = None; let mut stop_hook_active = false; - let mut model_request_count = 0usize; // Although from the perspective of codex.rs, TurnDiffTracker has the lifecycle of a Task which contains // many turns, from the perspective of the user, it is a single turn. #[allow(deprecated)] @@ -244,20 +243,6 @@ pub(crate) async fn run_turn( let turn_metadata_header = turn_context .turn_metadata_state .current_header_value_for_model_request(&window_id); - if let Some(message) = - model_request_limit_error(turn_context.max_model_requests, model_request_count) - { - sess.send_event( - &turn_context, - EventMsg::Error(ErrorEvent { - message, - codex_error_info: None, - }), - ) - .await; - break; - } - model_request_count = model_request_count.saturating_add(1); match run_sampling_request( Arc::clone(&sess), @@ -2249,15 +2234,6 @@ async fn try_run_sampling_request( outcome } -fn model_request_limit_error( - max_model_requests: Option, - model_request_count: usize, -) -> Option { - let limit = max_model_requests?; - (model_request_count >= limit) - .then(|| format!("agent hook exceeded its {limit} model request limit")) -} - pub(crate) fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) -> Option { for item in responses.iter().rev() { if let Some(message) = last_assistant_message_from_item(item, /*plan_mode*/ false) { diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 303d20de06..d600368137 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -93,7 +93,6 @@ pub struct TurnContext { pub features: ManagedFeatures, pub(crate) ghost_snapshot: GhostSnapshotConfig, pub(crate) final_output_json_schema: Option, - pub(crate) max_model_requests: Option, pub(crate) codex_self_exe: Option, pub(crate) codex_linux_sandbox_exe: Option, pub(crate) truncation_policy: TruncationPolicy, @@ -269,7 +268,6 @@ impl TurnContext { features, ghost_snapshot: self.ghost_snapshot.clone(), final_output_json_schema: self.final_output_json_schema.clone(), - max_model_requests: self.max_model_requests, codex_self_exe: self.codex_self_exe.clone(), codex_linux_sandbox_exe: self.codex_linux_sandbox_exe.clone(), truncation_policy, @@ -570,7 +568,6 @@ impl Session { features: per_turn_config.features.clone(), ghost_snapshot: per_turn_config.ghost_snapshot.clone(), final_output_json_schema: None, - max_model_requests: session_configuration.max_model_requests_per_turn, codex_self_exe: per_turn_config.codex_self_exe.clone(), codex_linux_sandbox_exe: per_turn_config.codex_linux_sandbox_exe.clone(), truncation_policy: model_info.truncation_policy.into(), diff --git a/codex-rs/core/src/session/turn_tests.rs b/codex-rs/core/src/session/turn_tests.rs index 24ea871627..81a43ff5a9 100644 --- a/codex-rs/core/src/session/turn_tests.rs +++ b/codex-rs/core/src/session/turn_tests.rs @@ -35,24 +35,6 @@ fn assistant_output_text(text: &str) -> ResponseItem { } } -#[test] -fn model_request_limit_allows_exactly_the_configured_number_of_requests() { - let limit = crate::hook_agent::AGENT_HOOK_MAX_MODEL_REQUESTS; - assert_eq!( - model_request_limit_error( - /*max_model_requests*/ None, /*model_request_count*/ 100, - ), - None - ); - assert_eq!(model_request_limit_error(Some(limit), limit - 1), None); - assert_eq!( - model_request_limit_error(Some(limit), limit), - Some(format!( - "agent hook exceeded its {limit} model request limit" - )) - ); -} - #[tokio::test] async fn plan_mode_uses_contributed_turn_item_for_last_agent_message() { let (mut session, turn_context) = crate::session::tests::make_session_and_context().await; diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 6b60287c07..bc179f6863 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -1251,6 +1251,36 @@ mod tests { ); } + #[test] + fn unsupported_agent_hook_is_skipped_during_discovery() { + let mut handlers = Vec::new(); + let mut hook_entries = Vec::new(); + let mut warnings = Vec::new(); + let mut display_order = 0; + let source_path = source_path(); + let hook_states = std::collections::HashMap::new(); + + append_matcher_groups( + &mut handlers, + &mut hook_entries, + &mut warnings, + &mut display_order, + &hook_handler_source(&source_path, &hook_states), + HookEventName::SessionStart, + vec![agent_group(/*matcher*/ None)], + ); + + assert_eq!(handlers, Vec::::new()); + assert_eq!(hook_entries.len(), 0); + assert_eq!( + warnings, + vec![format!( + "skipping agent hook in {}: agent hooks are not supported for SessionStart", + source_path.display() + )] + ); + } + #[test] fn toml_hook_discovery_ignores_malformed_state_entries() { let layer = ConfigLayerEntry::new( diff --git a/codex-rs/hooks/src/engine/prompt_runner.rs b/codex-rs/hooks/src/engine/prompt_runner.rs index 6a8e2a1ac8..65f1b82c5a 100644 --- a/codex-rs/hooks/src/engine/prompt_runner.rs +++ b/codex-rs/hooks/src/engine/prompt_runner.rs @@ -72,7 +72,7 @@ pub(crate) fn model_hook_behavior(event_name: HookEventName) -> ModelHookBehavio // continueOnBlock feeds the reason back to the model, otherwise it stops // the current turn. HookEventName::PostToolUse => ModelHookBehavior::FeedbackOrStop, - // Claude does not support prompt hooks for these lifecycle events. + // Claude does not support model-backed hooks for these lifecycle events. // Keeping them explicit makes new events choose semantics deliberately. HookEventName::SessionStart | HookEventName::SubagentStart