diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 139e527e92..38a1b295a9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3943,6 +3943,7 @@ dependencies = [ "codex-utils-path-uri", "codex-utils-plugins", "pretty_assertions", + "serde_json", "thiserror 2.0.18", ] diff --git a/codex-rs/core-plugins/src/executor_hooks.rs b/codex-rs/core-plugins/src/executor_hooks.rs index 3b3bd770da..c5aaed3e77 100644 --- a/codex-rs/core-plugins/src/executor_hooks.rs +++ b/codex-rs/core-plugins/src/executor_hooks.rs @@ -1,124 +1,192 @@ +//! Executor-discovered plugin hook admission and trusted MCP routing. + +use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; use codex_config::HookHandlerConfig; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; +use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; +use codex_mcp::ToolInfo; use codex_plugin::ExecutorPluginHookSource; use codex_plugin::PluginId; use codex_plugin::manifest::PluginManifestHooks; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::protocol::HookEventName; +use serde_json::Map; +use serde_json::Value; use crate::manifest::parse_plugin_manifest_uri; struct AllowlistedExecutorPluginHook { plugin_id: &'static str, event: HookEventName, - server: &'static str, - tool: &'static str, + target: ExecutorPluginHookTarget, +} + +enum ExecutorPluginHookTarget { + Executor { + server: &'static str, + tool: &'static str, + }, + App { + connector_id: &'static str, + tool: &'static str, + }, } // Executor plugin manifests are unsigned, so temporarily hardcode the expected -// bundled plugin identities, events, and MCP targets until plugin signing lands. +// plugin identities, events, and MCP targets until plugin signing lands. const ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS: &[AllowlistedExecutorPluginHook] = &[ AllowlistedExecutorPluginHook { plugin_id: "browser@openai-bundled", event: HookEventName::Stop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "browser@openai-bundled", event: HookEventName::Interrupt, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "browser@openai-bundled", event: HookEventName::SubagentStop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome@openai-bundled", event: HookEventName::Stop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome@openai-bundled", event: HookEventName::Interrupt, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome@openai-bundled", event: HookEventName::SubagentStop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome-dev@openai-bundled", event: HookEventName::Stop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome-dev@openai-bundled", event: HookEventName::Interrupt, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome-dev@openai-bundled", event: HookEventName::SubagentStop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome-internal@openai-bundled", event: HookEventName::Stop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome-internal@openai-bundled", event: HookEventName::Interrupt, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "chrome-internal@openai-bundled", event: HookEventName::SubagentStop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "computer-use@openai-bundled", event: HookEventName::Stop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "computer-use@openai-bundled", event: HookEventName::Interrupt, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, }, AllowlistedExecutorPluginHook { plugin_id: "computer-use@openai-bundled", event: HookEventName::SubagentStop, - server: "node_repl", - tool: "turn_ended", + target: ExecutorPluginHookTarget::Executor { + server: "node_repl", + tool: "turn_ended", + }, + }, + AllowlistedExecutorPluginHook { + plugin_id: "browser@openai-curated-remote", + event: HookEventName::Stop, + target: ExecutorPluginHookTarget::App { + connector_id: "connector_openai_browser", + tool: "browser.turn_ended", + }, + }, + AllowlistedExecutorPluginHook { + plugin_id: "browser@openai-curated-remote", + event: HookEventName::SubagentStop, + target: ExecutorPluginHookTarget::App { + connector_id: "connector_openai_browser", + tool: "browser.turn_ended", + }, }, ]; /// Returns accepted inline hook sources from executor-discovered plugin manifests. +/// Each source carries its trusted MCP routing metadata. `lookup_enabled_tool` must use a +/// consistent catalog/config snapshot, include app-only tools, and exclude tools disabled by +/// app policy. /// /// Executor scoped hooks are best-effort because executor capabilities can become available /// after earlier lifecycle events have passed. /// /// Note: Executor manifests are not signed yet, so temporarily we only admit the known cleanup -/// hook from the bundled Browser, Chrome, Chrome Dev, Chrome Internal, and Computer Use plugins. -pub fn executor_plugin_hook_sources( +/// hooks from the bundled plugins above and the remote Browser plugin. +pub fn executor_plugin_hook_sources<'a>( snapshot: &ExecutorCapabilityDiscoverySnapshot, + lookup_enabled_tool: impl Fn(&str, &str) -> Option<&'a ToolInfo>, ) -> Vec { let mut sources = Vec::new(); @@ -158,6 +226,8 @@ pub fn executor_plugin_hook_sources( sources.push(ExecutorPluginHookSource { plugin_id: plugin_id.clone(), environment_id: environment_id.clone(), + mcp_environment_id: None, + mcp_metadata: None, plugin_root: plugin_root.clone(), manifest_path: plugin.manifest.path.clone(), source_relative_path: format!("{manifest_relative_path}#hooks[{hook_index}]"), @@ -167,10 +237,17 @@ pub fn executor_plugin_hook_sources( } // FIXME: Remove this temporary filter once executor plugin hooks can be trusted. - sources.into_iter().filter_map(allowlisted_source).collect() + sources + .into_iter() + .filter_map(|source| allowlisted_source(source, &lookup_enabled_tool)) + .filter_map(|source| resolve_mcp_routing(source, &lookup_enabled_tool)) + .collect() } -fn allowlisted_source(mut source: ExecutorPluginHookSource) -> Option { +fn allowlisted_source<'a>( + mut source: ExecutorPluginHookSource, + lookup_enabled_tool: &impl Fn(&str, &str) -> Option<&'a ToolInfo>, +) -> Option { let plugin_id = source.plugin_id.as_key(); for (event, groups) in source.hooks.matcher_groups_mut() { groups.retain_mut(|group| { @@ -178,15 +255,39 @@ fn allowlisted_source(mut source: ExecutorPluginHookSource) -> Option server == expected_server && tool == expected_tool, + ExecutorPluginHookTarget::App { + connector_id, + tool: expected_tool, + } => { + // Raw Apps tool names can collide; admission must also match the listed connector. + server == CODEX_APPS_MCP_SERVER_NAME + && tool == expected_tool + && input.is_empty() + && lookup_enabled_tool(server, tool).is_some_and(|tool_info| { + tool_info.connector_id.as_deref() == Some(connector_id) + }) + } + } }); !group.hooks.is_empty() }); @@ -194,6 +295,41 @@ fn allowlisted_source(mut source: ExecutorPluginHookSource) -> Option( + mut source: ExecutorPluginHookSource, + lookup_enabled_tool: &impl Fn(&str, &str) -> Option<&'a ToolInfo>, +) -> Option { + let HookHandlerConfig::McpTool { server, tool, .. } = source + .hooks + .matcher_groups_mut() + .into_iter() + .find_map(|(_, groups)| groups.first())? + .hooks + .first()? + else { + return None; + }; + if server != CODEX_APPS_MCP_SERVER_NAME { + return Some(source); + } + + let tool_info = lookup_enabled_tool(server, tool)?; + let routing_metadata = tool_info + .tool + .meta + .as_ref()? + .get(MCP_TOOL_CODEX_APPS_META_KEY)? + .as_object()?; + routing_metadata.get("resource_uri")?.as_str()?; + source.mcp_environment_id = Some(DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string()); + source.mcp_metadata = Some(Map::from_iter([( + MCP_TOOL_CODEX_APPS_META_KEY.to_string(), + Value::Object(routing_metadata.clone()), + )])); + Some(source) +} + #[cfg(test)] #[path = "executor_hooks_tests.rs"] mod tests; diff --git a/codex-rs/core-plugins/src/executor_hooks_tests.rs b/codex-rs/core-plugins/src/executor_hooks_tests.rs index 55c836b48f..5c31d10c9f 100644 --- a/codex-rs/core-plugins/src/executor_hooks_tests.rs +++ b/codex-rs/core-plugins/src/executor_hooks_tests.rs @@ -80,6 +80,8 @@ fn expected_source(index: usize) -> ExecutorPluginHookSource { ExecutorPluginHookSource { plugin_id: PluginId::parse("computer-use@openai-bundled").expect("plugin id"), environment_id: "executor-a".to_string(), + mcp_environment_id: None, + mcp_metadata: None, plugin_root: PathUri::parse("file:///plugins/computer-use").expect("plugin root"), manifest_path: PathUri::parse("file:///plugins/computer-use/.codex-plugin/plugin.json") .expect("manifest path"), @@ -128,7 +130,7 @@ fn discovers_allowlisted_executor_plugin_hook_sources() { manifest, ); - let sources = executor_plugin_hook_sources(&snapshot); + let sources = executor_plugin_hook_sources(&snapshot, |_, _| None); let mut interrupt_source = expected_source(/*index*/ 1); interrupt_source.hooks.interrupt = std::mem::take(&mut interrupt_source.hooks.stop); let mut stop_and_interrupt_source = expected_source(/*index*/ 2); @@ -175,7 +177,10 @@ fn filters_mixed_handlers_without_rewriting_allowed_groups() { manifest, ); - assert_eq!(executor_plugin_hook_sources(&snapshot), vec![expected]); + assert_eq!( + executor_plugin_hook_sources(&snapshot, |_, _| None), + vec![expected] + ); } #[test] @@ -204,7 +209,139 @@ fn preserves_allowlisted_executor_plugin_hook_options() { }; expected.hooks.interrupt = expected.hooks.stop.clone(); - assert_eq!(executor_plugin_hook_sources(&snapshot), vec![expected]); + assert_eq!( + executor_plugin_hook_sources(&snapshot, |_, _| None), + vec![expected] + ); +} + +#[test] +fn resolves_apps_hook_metadata_from_the_registered_connector() { + let mut manifest = cleanup_hook_manifest(); + manifest["name"] = json!("browser"); + let handler = &mut manifest["hooks"]["hooks"]["Stop"][0]["hooks"][0]; + handler["server"] = json!("codex_apps"); + handler["tool"] = json!("browser.turn_ended"); + handler["input"] = json!({}); + let mut expected = expected_source(/*index*/ 0); + expected.plugin_id = PluginId::parse("browser@openai-curated-remote").expect("plugin id"); + expected.mcp_environment_id = Some(DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string()); + let routing = json!({ + "resource_uri": "/connector_openai_browser/browser-link/turn_ended", + "contains_mcp_source": true, + }); + expected.mcp_metadata = Some(Map::from_iter([( + MCP_TOOL_CODEX_APPS_META_KEY.to_string(), + routing.clone(), + )])); + + for (name, event, connector_id, input, routing_metadata, admitted) in [ + ( + "registered app", + "Stop", + Some("connector_openai_browser"), + json!({}), + routing.clone(), + true, + ), + ( + "colliding tool name", + "Stop", + Some("connector_other_browser"), + json!({}), + routing.clone(), + false, + ), + ( + "unavailable tool", + "Stop", + None, + json!({}), + routing.clone(), + false, + ), + ( + "registered subagent app", + "SubagentStop", + Some("connector_openai_browser"), + json!({}), + routing.clone(), + true, + ), + ( + "colliding subagent tool name", + "SubagentStop", + Some("connector_other_browser"), + json!({}), + routing.clone(), + false, + ), + ( + "manifest arguments", + "Stop", + Some("connector_openai_browser"), + json!({ "untrusted": "manifest-provided input" }), + routing, + false, + ), + ( + "missing routing metadata", + "Stop", + Some("connector_openai_browser"), + json!({}), + json!(null), + false, + ), + ( + "invalid resource URI", + "Stop", + Some("connector_openai_browser"), + json!({}), + json!({ "resource_uri": 42 }), + false, + ), + ] { + let mut manifest = manifest.clone(); + manifest["hooks"]["hooks"]["Stop"][0]["hooks"][0]["input"] = input; + let events = manifest["hooks"]["hooks"] + .as_object_mut() + .expect("hook events"); + let groups = events.remove("Stop").expect("stop groups"); + events.insert(event.to_string(), groups); + let mut expected = expected.clone(); + expected.hooks = serde_json::from_value(manifest["hooks"]["hooks"].clone()).expect("hooks"); + let snapshot = snapshot_for_manifest( + "browser@openai-curated-remote", + "executor-a", + "file:///plugins/computer-use/.codex-plugin/plugin.json", + manifest, + ); + let tool_info = connector_id.map(|connector_id| { + serde_json::from_value::(json!({ + "server_name": "codex_apps", + "tool_name": "turn_ended", + "tool_namespace": "browser", + "connector_id": connector_id, + "tool": { + "name": "browser.turn_ended", + "inputSchema": { "type": "object" }, + "_meta": { "_codex_apps": routing_metadata }, + }, + })) + .expect("listed tool") + }); + let actual = executor_plugin_hook_sources(&snapshot, |server, tool| { + tool_info + .as_ref() + .filter(|info| info.server_name == server && info.tool.name == tool) + }); + let expected = if admitted { + vec![expected.clone()] + } else { + Vec::new() + }; + assert_eq!(actual, expected, "{name}"); + } } #[test] @@ -269,7 +406,7 @@ fn ignores_unallowlisted_executor_plugin_hooks() { ); assert_eq!( - executor_plugin_hook_sources(&snapshot), + executor_plugin_hook_sources(&snapshot, |_, _| None), Vec::::new(), "{name}" ); @@ -289,7 +426,7 @@ fn ignores_file_backed_executor_plugin_hooks() { ); assert_eq!( - executor_plugin_hook_sources(&file_backed), + executor_plugin_hook_sources(&file_backed, |_, _| None), Vec::::new() ); } diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index a8e8308797..e8202d38f5 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -5,6 +5,8 @@ use std::time::Duration; use codex_analytics::CompactionTrigger; use codex_analytics::HookRunFact; use codex_analytics::build_track_events_context; +use codex_connectors::AppToolPolicyEvaluator; +use codex_connectors::AppToolPolicyInput; use codex_core_plugins::executor_plugin_hook_sources; use codex_hooks::InterruptRequest; use codex_hooks::PermissionRequestDecision; @@ -23,8 +25,10 @@ use codex_hooks::UserPromptSubmitOutcome; use codex_hooks::UserPromptSubmitRequest; use codex_hooks::hook_execution_mode_label; use codex_hooks::hook_handler_type_label; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_otel::HOOK_RUN_DURATION_METRIC; use codex_otel::HOOK_RUN_METRIC; +use codex_plugin::ExecutorPluginHookSource; use codex_protocol::items::FunctionCallOutputItem; use codex_protocol::items::TurnItem; use codex_protocol::items::UserMessageItem; @@ -311,6 +315,39 @@ pub(crate) async fn run_post_tool_use_hooks( outcome } +fn executor_hook_sources_for_step(step_context: &StepContext) -> Vec { + step_context + .executor_capability_discovery + .as_deref() + .map(|snapshot| { + let app_tool_policy = + AppToolPolicyEvaluator::new(&step_context.mcp.config().config_layer_stack); + executor_plugin_hook_sources(snapshot, |server, tool| { + step_context + .mcp + .tool_info(server, tool) + .filter(|tool_info| { + if server != CODEX_APPS_MCP_SERVER_NAME { + return true; + } + let annotations = tool_info.tool.annotations.as_ref(); + app_tool_policy + .policy(AppToolPolicyInput { + connector_id: tool_info.connector_id.as_deref(), + tool_name: &tool_info.tool.name, + tool_title: tool_info.tool.title.as_deref(), + destructive_hint: annotations + .and_then(|annotations| annotations.destructive_hint), + open_world_hint: annotations + .and_then(|annotations| annotations.open_world_hint), + }) + .enabled + }) + }) + }) + .unwrap_or_default() +} + fn build_request_metadata( step_context: Option<&StepContext>, turn_context: &TurnContext, @@ -404,11 +441,7 @@ pub(crate) async fn run_turn_stop_hooks( last_assistant_message, target, }; - let executor_hook_sources = step_context - .executor_capability_discovery - .as_deref() - .map(executor_plugin_hook_sources) - .unwrap_or_default(); + let executor_hook_sources = executor_hook_sources_for_step(step_context); let hooks = sess.hooks().with_executor_hooks(executor_hook_sources); emit_hook_started_events(sess, turn_context, hooks.preview_stop(&request)).await; @@ -461,9 +494,8 @@ pub(crate) async fn run_turn_interrupt_hooks( // The active turn has already been detached. Reuse only its last executing step's discovery. let last_known_step_context = turn_state.lock().await.last_known_step_context.clone(); let executor_hook_sources = last_known_step_context - .as_ref() - .and_then(|step_context| step_context.executor_capability_discovery.as_deref()) - .map(executor_plugin_hook_sources) + .as_deref() + .map(executor_hook_sources_for_step) .unwrap_or_default(); let has_executor_hooks = !executor_hook_sources.is_empty(); let hooks = sess.hooks().with_executor_hooks(executor_hook_sources); diff --git a/codex-rs/core/tests/suite/hooks_executor.rs b/codex-rs/core/tests/suite/hooks_executor.rs index b8419e18cc..11e728f717 100644 --- a/codex-rs/core/tests/suite/hooks_executor.rs +++ b/codex-rs/core/tests/suite/hooks_executor.rs @@ -1,7 +1,9 @@ use anyhow::Context; use anyhow::Result; use codex_config::McpServerConfig; +use codex_config::test_support::CloudConfigBundleFixture; use codex_core::EnvironmentConfig; +use codex_core::StartThreadOptions; use codex_core::TurnInputRequest; use codex_core::windows_sandbox::WindowsSandboxLevelExt; use codex_exec_server::CreateDirectoryOptions; @@ -14,10 +16,17 @@ use codex_protocol::models::PermissionProfileSnapshot; use codex_protocol::protocol::EnvironmentConfigState; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; +use codex_protocol::protocol::SessionSource; +use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::protocol::ThreadSource; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::protocol::TurnEnvironmentSelections; use codex_protocol::user_input::UserInput; +use codex_utils_path_uri::PathUri; +use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::apps_enabled_builder; +use core_test_support::apps_test_server::recorded_apps_tool_calls; use core_test_support::responses::ResponseMock; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -30,6 +39,7 @@ use core_test_support::skip_if_no_network; use core_test_support::skip_if_remote; use core_test_support::submit_thread_settings; use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::TestCodexBuilder; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use core_test_support::wait_for_mcp_server; @@ -38,12 +48,14 @@ use serde_json::Value; use serde_json::json; use std::sync::Arc; use std::time::Duration; +use test_case::test_case; use tokio::net::TcpListener; use tokio::sync::Notify; use wiremock::Mock; use wiremock::MockServer; use wiremock::Request; use wiremock::ResponseTemplate; +use wiremock::matchers::body_partial_json; use wiremock::matchers::method; use wiremock::matchers::path; @@ -350,6 +362,194 @@ async fn executor_stop_hook_rejects_mismatched_environment() -> Result<()> { Ok(()) } +#[test_case("Stop", "", "", 1; "enabled")] +#[test_case("SubagentStop", "", "", 1; "subagent_enabled")] +#[test_case( + "Stop", + "[apps.connector_openai_browser]\nenabled = false\n", + "", + 0; + "user_disabled" +)] +#[test_case( + "Stop", + "[apps.connector_openai_browser]\nenabled = true\n", + "[apps.connector_openai_browser]\nenabled = false\n", + 0; + "managed_disabled" +)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn executor_browser_and_computer_use_stop_hooks_use_separate_mcp_routes( + hook_event: &'static str, + user_config: &'static str, + requirements: &'static str, + expected_browser_calls: usize, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let apps_server = start_mock_server().await; + let apps = AppsTestServer::mount(&apps_server).await?; + let routing = json!({ + "resource_uri": "/connector_openai_browser/browser-link/turn_ended", + "contains_mcp_source": true, + }); + let listed_routing = routing.clone(); + Mock::given(method("POST")) + .and(path("/api/codex/ps/mcp")) + .and(body_partial_json(json!({ "method": "tools/list" }))) + .respond_with(move |request: &Request| { + let request: Value = serde_json::from_slice(&request.body).expect("valid Apps request"); + ResponseTemplate::new(200).set_body_json(json!({ + "jsonrpc": "2.0", + "id": request["id"], + "result": { "tools": [{ + "name": "browser.turn_ended", + "inputSchema": { "type": "object" }, + "_meta": { + "connector_id": "connector_openai_browser", + "connector_name": "Browser", + "ui": { "visibility": ["app"] }, + "_codex_apps": listed_routing, + }, + }] }, + })) + }) + .with_priority(1) + .mount(&apps_server) + .await; + + let plugins = [ + ( + "browser@openai-curated-remote", + json!({ + "name": "browser", + "hooks": { "hooks": { + "Stop": [{ "hooks": [{ + "type": "mcp_tool", + "server": "codex_apps", + "tool": "browser.turn_ended", + "input": {}, + }] }], + "SubagentStop": [{ "hooks": [{ + "type": "mcp_tool", + "server": "codex_apps", + "tool": "browser.turn_ended", + "input": {}, + }] }], + } }, + }), + ), + ("computer-use@openai-bundled", computer_use_hook_manifest()), + ]; + let mut fixture = executor_plugin_hook_fixture( + apps_enabled_builder(apps.chatgpt_base_url) + .with_pre_build_hook(move |home| { + std::fs::write(home.join("config.toml"), user_config) + .expect("write Browser app configuration"); + }) + .with_cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement(requirements), + ), + &plugins, + vec![completed_turn_response("browser-turn")], + ) + .await?; + if hook_event == "SubagentStop" { + let child = fixture + .test + .thread_manager + .start_thread(StartThreadOptions { + session_source: Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id: fixture.test.session_configured.thread_id, + depth: 1, + agent_path: None, + agent_nickname: None, + agent_role: None, + })), + thread_source: Some(ThreadSource::Subagent), + environments: Some(fixture.test.codex.environment_selections().await), + ..StartThreadOptions::new(fixture.test.config.clone()) + }) + .await?; + fixture.test.codex = child.thread; + fixture.test.session_configured = child.session_configured; + } + fixture.attach().await?; + if hook_event == "SubagentStop" { + // Cached subagent MCP servers start on first use; cleanup must not start them. + fixture + .test + .codex + .call_mcp_tool( + "node_repl", + "js", + Some(json!({ "code": "1 + 1" })), + /*meta*/ None, + ) + .await?; + } + fixture.test.submit_text_turn("finish browsing").await?; + fixture.wait_for_hook_call().await?; + let node_calls = fixture.calls().await?; + let expected_node_tools = if hook_event == "SubagentStop" { + vec!["js", "turn_ended"] + } else { + vec!["turn_ended"] + }; + assert_eq!( + node_calls + .iter() + .map(|call| call["params"]["name"].as_str().expect("Node tool name")) + .collect::>(), + expected_node_tools + ); + let node_params = &node_calls.last().context("Node cleanup call")?["params"]; + assert_eq!(node_params["arguments"]["hook_event_name"], hook_event); + assert!(node_params["_meta"].get("_codex_apps").is_none()); + + // Executor cleanup runs in the background, so observe a no-call window after Node runs. + let browser_timeout = if expected_browser_calls == 0 { + Duration::from_secs(1) + } else { + Duration::from_secs(10) + }; + let calls = tokio::time::timeout(browser_timeout, async { + loop { + let calls = recorded_apps_tool_calls(&apps_server).await; + if !calls.is_empty() { + break calls; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .unwrap_or_default(); + assert_eq!(calls.len(), expected_browser_calls); + if calls.is_empty() { + return Ok(()); + } + let browser_params = &calls[0]["params"]; + let response_body = fixture.responses.single_request().body_json(); + let thread_id = fixture.test.session_configured.thread_id.to_string(); + assert_eq!(browser_params["name"], "browser.turn_ended"); + assert_eq!(browser_params["arguments"], json!({})); + assert_eq!(browser_params["_meta"]["_codex_apps"], routing); + let turn_metadata = &browser_params["_meta"]["x-codex-turn-metadata"]; + assert_eq!( + node_params["_meta"]["x-codex-turn-metadata"], + *turn_metadata + ); + assert_eq!(turn_metadata["thread_id"], thread_id); + if hook_event == "SubagentStop" { + assert_eq!(turn_metadata["thread_source"], "subagent"); + } + assert_eq!( + turn_metadata["turn_id"], + response_body["client_metadata"]["turn_id"] + ); + Ok(()) +} + fn completed_turn_response(id: &str) -> ResponseTemplate { sse_response(sse(vec![ ev_response_created(id), @@ -368,6 +568,58 @@ async fn executor_stop_hook_fixture() -> Result { } async fn executor_hook_fixture(responses: Vec) -> Result { + executor_plugin_hook_fixture( + test_codex(), + &[("computer-use@openai-bundled", computer_use_hook_manifest())], + responses, + ) + .await +} + +fn computer_use_hook_manifest() -> Value { + json!({ + "name": "computer-use", + // Separate entries must all survive registration in the same environment. + "hooks": [ + { "hooks": { "Interrupt": [{ "hooks": [{ + "type": "mcp_tool", + "server": "node_repl", + "tool": "turn_ended", + "input": { + "hook_event_name": "${hook_event_name}", + "session_id": "${session_id}", + "turn_id": "${turn_id}", + }, + }] }] } }, + { "hooks": { "Stop": [{ "hooks": [{ + "type": "mcp_tool", + "server": "node_repl", + "tool": "turn_ended", + "input": { + "hook_event_name": "${hook_event_name}", + "session_id": "${session_id}", + "turn_id": "${turn_id}", + }, + }] }] } }, + { "hooks": { "SubagentStop": [{ "hooks": [{ + "type": "mcp_tool", + "server": "node_repl", + "tool": "turn_ended", + "input": { + "hook_event_name": "${hook_event_name}", + "session_id": "${session_id}", + "turn_id": "${turn_id}", + }, + }] }] } }, + ], + }) +} + +async fn executor_plugin_hook_fixture( + builder: TestCodexBuilder, + plugins: &[(&'static str, Value)], + responses: Vec, +) -> Result { let server = start_mock_server().await; let hook_called = Arc::new(Notify::new()); let hook_called_for_server = Arc::clone(&hook_called); @@ -383,12 +635,14 @@ async fn executor_hook_fixture(responses: Vec) -> Result return ResponseTemplate::new(202), - Some("tools/list") => json!({ "tools": [{ - "name": "turn_ended", - "inputSchema": { "type": "object" }, - }] }), + Some("tools/list") => json!({ "tools": [ + { "name": "js", "inputSchema": { "type": "object" } }, + { "name": "turn_ended", "inputSchema": { "type": "object" } }, + ] }), Some("tools/call") => { - hook_called_for_server.notify_one(); + if request["params"]["name"] == "turn_ended" { + hook_called_for_server.notify_one(); + } json!({ "content": [{ "type": "text", "text": "ok" }] }) } method => panic!("unexpected Node REPL request: {method:?}"), @@ -403,7 +657,7 @@ async fn executor_hook_fixture(responses: Vec) -> Result) -> Result) -> Result, + plugin_roots: Vec<(&'static str, PathUri)>, } impl ExecutorHookFixture { @@ -525,13 +761,17 @@ impl ExecutorHookFixture { exec_policy: None, mcp_policy: None, network_policy: None, - selected_capability_roots: vec![SelectedCapabilityRoot { - id: "computer-use@openai-bundled".to_string(), - location: CapabilityRootLocation::Environment { - environment_id: selection.environment_id.clone(), - path: self.test.workspace_path_uri("computer-use")?, - }, - }], + selected_capability_roots: self + .plugin_roots + .iter() + .map(|(plugin_id, plugin_root)| SelectedCapabilityRoot { + id: plugin_id.to_string(), + location: CapabilityRootLocation::Environment { + environment_id: selection.environment_id.clone(), + path: plugin_root.clone(), + }, + }) + .collect(), }, ) .await?; diff --git a/codex-rs/hooks/src/engine/mcp_runner.rs b/codex-rs/hooks/src/engine/mcp_runner.rs index 63befa6bc6..7a8da53e4d 100644 --- a/codex-rs/hooks/src/engine/mcp_runner.rs +++ b/codex-rs/hooks/src/engine/mcp_runner.rs @@ -39,17 +39,34 @@ pub(crate) async fn run_mcp_tool( let hook_event: Value = serde_json::from_str(hook_event_json).context("failed to parse hook event input")?; let input = expand_mcp_argument_template(argument_template, &hook_event)?; + let (environment_id, mut call_metadata) = match &handler.source_path { + HandlerSourcePath::Local(_) => (None, None), + HandlerSourcePath::ExecutorScoped { + environment_id, + mcp_environment_id, + mcp_metadata, + .. + } => ( + Some( + mcp_environment_id + .as_ref() + .unwrap_or(environment_id) + .clone(), + ), + mcp_metadata.as_deref().cloned(), + ), + }; + if let Some(metadata) = metadata { + call_metadata + .get_or_insert_with(Map::new) + .extend(metadata.clone()); + } executor .execute(HookMcpCall { server: server.to_string(), tool: tool.to_string(), - environment_id: match &handler.source_path { - HandlerSourcePath::Local(_) => None, - HandlerSourcePath::ExecutorScoped { environment_id, .. } => { - Some(environment_id.clone()) - } - }, - metadata: metadata.cloned(), + environment_id, + metadata: call_metadata, input, timeout: Duration::from_secs(handler.timeout_sec), }) diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index a287e83c12..67bb23644c 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -40,6 +40,8 @@ use codex_protocol::protocol::HookSource; use codex_protocol::protocol::HookTrustStatus; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use serde_json::Map; +use serde_json::Value; use std::collections::HashMap; use std::collections::HashSet; use std::sync::Arc; @@ -70,8 +72,7 @@ pub(crate) struct ConfiguredHandler { pub(crate) enum HandlerSourcePath { Local(AbsolutePathBuf), /// Executor-scoped handlers are currently excluded from user-visible hook reporting - /// (events, summary, telemetry) because the only supported 1p hook is - /// coupled to executor-scoped execution. Their handlers are always executed async. + /// (events, summary, telemetry). Their handlers are always executed async. /// /// TODO: With CCA, all hooks will be executor-scoped, so user visibility /// (participation in lifecycle events and summaries) and execution behavior @@ -79,6 +80,8 @@ pub(crate) enum HandlerSourcePath { ExecutorScoped { plugin_id: PluginId, environment_id: String, + mcp_environment_id: Option, + mcp_metadata: Option>>, manifest_path: PathUri, source_relative_path: String, }, @@ -266,30 +269,14 @@ impl ClaudeHooksEngine { ) }); - // All events share the first environment, even if it has no handler for an event. - let Some(selected_environment_id) = executor_hooks - .first() - .map(|source| source.environment_id.clone()) - else { - return; - }; - let environment_count = executor_hooks + let mut display_order = self + .handlers .iter() - .map(|source| &source.environment_id) - .collect::>() - .len(); - // FIXME: Remove this restriction once executor hooks support multiple environments. - if environment_count > 1 { - tracing::warn!( - executor_environment_count = environment_count, - selected_environment_id = %selected_environment_id, - "multiple executor environments found; only executing hooks in the first" - ); - } + .map(|handler| handler.display_order) + .max() + .map_or(0, |display_order| display_order.saturating_add(1)); + let mut seen_targets = HashSet::new(); for source in executor_hooks { - if source.environment_id != selected_environment_id { - continue; - } for (event_name, groups) in source.hooks.into_matcher_groups() { let Some(handler) = groups.into_iter().flat_map(|group| group.hooks).next() else { continue; @@ -305,12 +292,21 @@ impl ClaudeHooksEngine { unreachable!("allowlisted executor handler must be an MCP tool"); }; - let display_order = self - .handlers - .iter() - .map(|handler| handler.display_order) - .max() - .map_or(0, |display_order| display_order.saturating_add(1)); + // Bundled plugins can share a cleanup target; run it once per event + // and MCP environment. + let target = ( + std::mem::discriminant(&event_name), + source + .mcp_environment_id + .as_ref() + .unwrap_or(&source.environment_id) + .clone(), + server.clone(), + tool.clone(), + ); + if !seen_targets.insert(target) { + continue; + } self.handlers.push(ConfiguredHandler { event_name, matcher: None, @@ -320,6 +316,8 @@ impl ClaudeHooksEngine { source_path: HandlerSourcePath::ExecutorScoped { plugin_id: source.plugin_id.clone(), environment_id: source.environment_id.clone(), + mcp_environment_id: source.mcp_environment_id.clone(), + mcp_metadata: source.mcp_metadata.clone().map(Box::new), manifest_path: source.manifest_path.clone(), source_relative_path: source.source_relative_path.clone(), }, @@ -331,6 +329,7 @@ impl ClaudeHooksEngine { input, }, }); + display_order = display_order.saturating_add(1); } } } diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index 3aba38c56d..9bf691d155 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -2000,6 +2000,8 @@ fn executor_stop_hook_fixture() -> ( let source = ExecutorPluginHookSource { plugin_id: PluginId::parse("computer-use@openai-bundled").expect("valid plugin ID"), environment_id: "executor-a".to_string(), + mcp_environment_id: None, + mcp_metadata: None, plugin_root: "file:///plugins/computer-use" .parse() .expect("valid plugin root URI"), @@ -2036,6 +2038,8 @@ fn executor_stop_hook_fixture() -> ( source_path: HandlerSourcePath::ExecutorScoped { plugin_id: PluginId::parse("computer-use@openai-bundled").expect("valid plugin ID"), environment_id: "executor-a".to_string(), + mcp_environment_id: None, + mcp_metadata: None, manifest_path: "file:///plugins/computer-use/.codex-plugin/plugin.json" .parse() .expect("valid plugin manifest URI"), @@ -2202,19 +2206,22 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { } #[test] -fn executor_stop_hooks_register_only_the_first_environment_and_handler() { - let (mut engine, _, _, _, mut first_source) = executor_stop_hook_fixture(); - let expected_handlers = engine.handlers.clone(); - let first_group = &mut first_source.hooks.stop[0]; - let mut second_handler = first_group.hooks[0].clone(); - let HookHandlerConfig::McpTool { tool, .. } = &mut second_handler else { - panic!("executor Stop handler should be an MCP tool"); - }; - *tool = "second_turn_ended".to_string(); - first_group.hooks.push(second_handler); +fn executor_stop_hooks_register_each_target_environment_once() { + let (mut engine, _, _, _, first_source) = executor_stop_hook_fixture(); + let mut expected_handlers = engine.handlers.clone(); + let mut duplicate_target = first_source.clone(); + duplicate_target.plugin_id = PluginId::parse("chrome@openai-bundled").expect("valid plugin ID"); let mut second_source = first_source.clone(); second_source.environment_id = "executor-b".to_string(); - engine.set_executor_hooks(vec![first_source, second_source]); + let mut second_handler = expected_handlers[0].clone(); + second_handler.display_order = 1; + let HandlerSourcePath::ExecutorScoped { environment_id, .. } = &mut second_handler.source_path + else { + panic!("executor Stop handler should have an executor source"); + }; + *environment_id = second_source.environment_id.clone(); + expected_handlers.push(second_handler); + engine.set_executor_hooks(vec![first_source, duplicate_target, second_source]); assert_eq!(engine.handlers, expected_handlers); } @@ -2223,6 +2230,7 @@ fn executor_stop_hooks_register_only_the_first_environment_and_handler() { async fn executor_interrupt_hooks_register_and_run() { let (mut engine, calls, stop_request, expected_stop_call, mut source) = executor_stop_hook_fixture(); + source.hooks.subagent_stop = source.hooks.stop.clone(); source.hooks.interrupt = source.hooks.stop.clone(); engine.set_executor_hooks(vec![source]); @@ -2232,7 +2240,11 @@ async fn executor_interrupt_hooks_register_and_run() { .iter() .map(|handler| handler.event_name) .collect::>(), - vec![HookEventName::Stop, HookEventName::Interrupt] + vec![ + HookEventName::SubagentStop, + HookEventName::Stop, + HookEventName::Interrupt, + ] ); assert_eq!(engine.preview_interrupt(), Vec::new()); @@ -2257,20 +2269,42 @@ async fn executor_interrupt_hooks_register_and_run() { } #[test] -fn executor_hooks_use_one_environment_for_all_events() { +fn executor_hooks_register_events_from_each_environment() { let (mut engine, _, _, _, mut first_source) = executor_stop_hook_fixture(); first_source.hooks.interrupt = std::mem::take(&mut first_source.hooks.stop); engine.set_executor_hooks(vec![first_source.clone()]); - let expected_handlers = engine.handlers.clone(); + let mut expected_handlers = engine.handlers.clone(); let mut second_source = first_source.clone(); second_source.environment_id = "executor-b".to_string(); second_source.hooks.stop = std::mem::take(&mut second_source.hooks.interrupt); + let mut second_handler = expected_handlers[0].clone(); + second_handler.event_name = HookEventName::Stop; + second_handler.display_order = 1; + let HandlerSourcePath::ExecutorScoped { environment_id, .. } = &mut second_handler.source_path + else { + panic!("executor Stop handler should have an executor source"); + }; + *environment_id = second_source.environment_id.clone(); + expected_handlers.push(second_handler); engine.set_executor_hooks(vec![first_source, second_source]); assert_eq!(engine.handlers, expected_handlers); } +#[tokio::test] +async fn executor_stop_hooks_use_an_admitted_mcp_environment() { + let (mut engine, calls, request, mut expected_call, mut source) = executor_stop_hook_fixture(); + source.mcp_environment_id = Some("local".to_string()); + engine.set_executor_hooks(vec![source]); + + engine.run_stop(request).await; + wait_for_mcp_calls(&calls, /*count*/ 1).await; + + expected_call.environment_id = Some("local".to_string()); + assert_eq!(*calls.lock().expect("lock MCP calls"), vec![expected_call]); +} + #[tokio::test] async fn memory_consolidation_stop_preserves_policy_and_executor_cleanup() { for (source, runs_policy) in [ diff --git a/codex-rs/plugin/Cargo.toml b/codex-rs/plugin/Cargo.toml index 3e55f55c33..89e2bc2fc9 100644 --- a/codex-rs/plugin/Cargo.toml +++ b/codex-rs/plugin/Cargo.toml @@ -18,6 +18,7 @@ codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-plugins = { workspace = true } +serde_json = { workspace = true } thiserror = { workspace = true } [dev-dependencies] diff --git a/codex-rs/plugin/src/lib.rs b/codex-rs/plugin/src/lib.rs index 947d8ea3b7..1775205523 100644 --- a/codex-rs/plugin/src/lib.rs +++ b/codex-rs/plugin/src/lib.rs @@ -23,6 +23,8 @@ pub use provider::PluginResourceLocator; pub use provider::ResolvedPlugin; pub use provider::ResolvedPluginError; pub use provider::ResolvedPluginLocation; +use serde_json::Map; +use serde_json::Value; #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct AppConnectorId(pub String); @@ -68,11 +70,15 @@ pub struct PluginHookSource { pub hooks: HookEventsToml, } -/// Inline plugin hooks whose paths and MCP target belong to an executor environment. +/// Inline plugin hooks discovered in an executor environment. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ExecutorPluginHookSource { pub plugin_id: PluginId, pub environment_id: String, + /// An admitted MCP target can run outside the plugin's source environment. + pub mcp_environment_id: Option, + /// Trusted MCP routing metadata for this cleanup target. + pub mcp_metadata: Option>, pub plugin_root: PathUri, pub manifest_path: PathUri, pub source_relative_path: String,