mirror of
https://github.com/openai/codex.git
synced 2026-11-09 20:50:33 +00:00
Run executor hooks for interrupted turns (#41432)
## Why Executor-scoped cleanup hooks need to observe an interrupted turn as well as a normally completed one. ## What changed - Allow bundled executor plugins to register the allowlisted `Interrupt` hook alongside `Stop`, while preserving multiple allowed hook entries from the selected executor environment. - Run interrupt hooks with the interrupted turn's last step discovery and turn metadata. Turns without a step context do not reuse discovery from an earlier turn. - Keep executor-scoped interrupt hooks asynchronous and omit public hook lifecycle events for them, matching executor `Stop` hook behavior. ## Testing Added coverage for hook filtering and registration, interrupt execution and metadata propagation, environment selection, and turns without a step context. GitOrigin-RevId: f0c536b3382a37cf3bf693c4573d21f6f66ace4e
This commit is contained in:
committed by
copyberry
parent
0d22692962
commit
c2abf869d5
@@ -1,6 +1,4 @@
|
||||
use codex_config::HookEventsToml;
|
||||
use codex_config::HookHandlerConfig;
|
||||
use codex_config::MatcherGroup;
|
||||
use codex_exec_server::ExecutorCapabilityDiscoverySnapshot;
|
||||
use codex_plugin::ExecutorPluginHookSource;
|
||||
use codex_plugin::PluginId;
|
||||
@@ -18,7 +16,7 @@ struct AllowlistedExecutorPluginHook {
|
||||
}
|
||||
|
||||
// Executor plugin manifests are unsigned, so temporarily hardcode the expected
|
||||
// bundled plugin identities and cleanup MCP target until plugin signing lands.
|
||||
// bundled plugin identities, events, and MCP targets until plugin signing lands.
|
||||
const ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS: &[AllowlistedExecutorPluginHook] = &[
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "browser@openai-bundled",
|
||||
@@ -26,30 +24,60 @@ const ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS: &[AllowlistedExecutorPluginHook] = &[
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "browser@openai-bundled",
|
||||
event: HookEventName::Interrupt,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "chrome@openai-bundled",
|
||||
event: HookEventName::Stop,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "chrome@openai-bundled",
|
||||
event: HookEventName::Interrupt,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "chrome-dev@openai-bundled",
|
||||
event: HookEventName::Stop,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "chrome-dev@openai-bundled",
|
||||
event: HookEventName::Interrupt,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "chrome-internal@openai-bundled",
|
||||
event: HookEventName::Stop,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "chrome-internal@openai-bundled",
|
||||
event: HookEventName::Interrupt,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "computer-use@openai-bundled",
|
||||
event: HookEventName::Stop,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
AllowlistedExecutorPluginHook {
|
||||
plugin_id: "computer-use@openai-bundled",
|
||||
event: HookEventName::Interrupt,
|
||||
server: "node_repl",
|
||||
tool: "turn_ended",
|
||||
},
|
||||
];
|
||||
|
||||
/// Returns accepted inline hook sources from executor-discovered plugin manifests.
|
||||
@@ -113,31 +141,27 @@ pub fn executor_plugin_hook_sources(
|
||||
}
|
||||
|
||||
fn allowlisted_source(mut source: ExecutorPluginHookSource) -> Option<ExecutorPluginHookSource> {
|
||||
let allowlisted_hook = ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS.iter().find(|hook| {
|
||||
hook.plugin_id == source.plugin_id.as_key() && hook.event == HookEventName::Stop
|
||||
})?;
|
||||
let handler = source
|
||||
.hooks
|
||||
.stop
|
||||
.into_iter()
|
||||
.filter(|group| group.matcher.is_none())
|
||||
.flat_map(|group| group.hooks)
|
||||
.find(|handler| {
|
||||
matches!(
|
||||
handler,
|
||||
HookHandlerConfig::McpTool { server, tool, .. }
|
||||
if server == allowlisted_hook.server && tool == allowlisted_hook.tool
|
||||
)
|
||||
})?;
|
||||
|
||||
source.hooks = HookEventsToml {
|
||||
stop: vec![MatcherGroup {
|
||||
matcher: None,
|
||||
hooks: vec![handler],
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
Some(source)
|
||||
let plugin_id = source.plugin_id.as_key();
|
||||
for (event, groups) in source.hooks.matcher_groups_mut() {
|
||||
groups.retain_mut(|group| {
|
||||
if group.matcher.is_some() {
|
||||
return false;
|
||||
}
|
||||
group.hooks.retain(|handler| {
|
||||
let HookHandlerConfig::McpTool { server, tool, .. } = handler else {
|
||||
return false;
|
||||
};
|
||||
ALLOWLISTED_EXECUTOR_PLUGIN_HOOKS.iter().any(|hook| {
|
||||
hook.plugin_id == plugin_id
|
||||
&& hook.event == event
|
||||
&& hook.server == server
|
||||
&& hook.tool == tool
|
||||
})
|
||||
});
|
||||
!group.hooks.is_empty()
|
||||
});
|
||||
}
|
||||
(!source.hooks.is_empty()).then_some(source)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_config::HookEventsToml;
|
||||
use codex_config::MatcherGroup;
|
||||
use codex_exec_server::CapabilityRootDiscovery;
|
||||
use codex_exec_server::CapabilityTextFile;
|
||||
use codex_exec_server::DiscoveredPluginFiles;
|
||||
@@ -105,24 +107,20 @@ fn expected_source(index: usize) -> ExecutorPluginHookSource {
|
||||
|
||||
#[test]
|
||||
fn discovers_allowlisted_executor_plugin_hook_sources() {
|
||||
let mut manifest = cleanup_hook_manifest();
|
||||
manifest["hooks"]["hooks"]["Stop"][0]["hooks"]
|
||||
.as_array_mut()
|
||||
.expect("stop hooks")
|
||||
.push(json!({
|
||||
"type": "command",
|
||||
"command": "echo ignored"
|
||||
}));
|
||||
manifest["hooks"]["hooks"]["UserPromptSubmit"] = json!([{
|
||||
"hooks": [{
|
||||
"type": "mcp_tool",
|
||||
"server": "other",
|
||||
"tool": "ignored"
|
||||
}]
|
||||
}]);
|
||||
let second_hook_file = cleanup_hook_manifest()["hooks"].clone();
|
||||
let first_hook_file = manifest["hooks"].take();
|
||||
manifest["hooks"] = json!([first_hook_file, second_hook_file]);
|
||||
let cleanup_hooks = cleanup_hook_manifest()["hooks"]["hooks"]["Stop"].clone();
|
||||
let manifest = json!({
|
||||
"name": "computer-use",
|
||||
"hooks": [
|
||||
{ "hooks": { "Stop": cleanup_hooks } },
|
||||
{ "hooks": { "Interrupt": cleanup_hooks } },
|
||||
{ "hooks": {
|
||||
"Stop": cleanup_hooks,
|
||||
"Interrupt": cleanup_hooks,
|
||||
"UserPromptSubmit": cleanup_hooks
|
||||
} },
|
||||
{ "hooks": { "UserPromptSubmit": cleanup_hooks } }
|
||||
]
|
||||
});
|
||||
let snapshot = snapshot_for_manifest(
|
||||
"computer-use@openai-bundled",
|
||||
"executor-a",
|
||||
@@ -131,13 +129,55 @@ fn discovers_allowlisted_executor_plugin_hook_sources() {
|
||||
);
|
||||
|
||||
let sources = executor_plugin_hook_sources(&snapshot);
|
||||
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);
|
||||
stop_and_interrupt_source.hooks.interrupt = stop_and_interrupt_source.hooks.stop.clone();
|
||||
|
||||
assert_eq!(
|
||||
sources,
|
||||
vec![expected_source(/*index*/ 0), expected_source(/*index*/ 1)]
|
||||
vec![
|
||||
expected_source(/*index*/ 0),
|
||||
interrupt_source,
|
||||
stop_and_interrupt_source,
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn filters_mixed_handlers_without_rewriting_allowed_groups() {
|
||||
let mut expected = expected_source(/*index*/ 0);
|
||||
let mut second_handler = expected.hooks.stop[0].hooks[0].clone();
|
||||
let HookHandlerConfig::McpTool { input, .. } = &mut second_handler else {
|
||||
panic!("expected an MCP tool hook");
|
||||
};
|
||||
input.insert("order".to_string(), json!(2));
|
||||
expected.hooks.stop[0].hooks.push(second_handler.clone());
|
||||
expected.hooks.stop.push(MatcherGroup {
|
||||
matcher: None,
|
||||
hooks: vec![second_handler],
|
||||
});
|
||||
|
||||
let mut manifest = cleanup_hook_manifest();
|
||||
manifest["hooks"]["hooks"]["Stop"] =
|
||||
serde_json::to_value(&expected.hooks.stop).expect("serialize stop hooks");
|
||||
manifest["hooks"]["hooks"]["Stop"][0]["hooks"]
|
||||
.as_array_mut()
|
||||
.expect("stop handlers")
|
||||
.insert(
|
||||
/*index*/ 0,
|
||||
json!({ "type": "mcp_tool", "server": "node_repl", "tool": "other" }),
|
||||
);
|
||||
let snapshot = snapshot_for_manifest(
|
||||
"computer-use@openai-bundled",
|
||||
"executor-a",
|
||||
"file:///plugins/computer-use/.codex-plugin/plugin.json",
|
||||
manifest,
|
||||
);
|
||||
|
||||
assert_eq!(executor_plugin_hook_sources(&snapshot), vec![expected]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn preserves_allowlisted_executor_plugin_hook_options() {
|
||||
let mut manifest = cleanup_hook_manifest();
|
||||
@@ -145,6 +185,7 @@ fn preserves_allowlisted_executor_plugin_hook_options() {
|
||||
handler["input"] = json!({ "untrusted": "manifest-provided input" });
|
||||
handler["timeout"] = json!(30);
|
||||
handler["statusMessage"] = json!("Cleaning up Computer Use");
|
||||
manifest["hooks"]["hooks"]["Interrupt"] = manifest["hooks"]["hooks"]["Stop"].clone();
|
||||
let snapshot = snapshot_for_manifest(
|
||||
"computer-use@openai-bundled",
|
||||
"executor-a",
|
||||
@@ -161,6 +202,7 @@ fn preserves_allowlisted_executor_plugin_hook_options() {
|
||||
timeout_sec: Some(30),
|
||||
status_message: Some("Cleaning up Computer Use".to_string()),
|
||||
};
|
||||
expected.hooks.interrupt = expected.hooks.stop.clone();
|
||||
|
||||
assert_eq!(executor_plugin_hook_sources(&snapshot), vec![expected]);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user