mirror of
https://github.com/openai/codex.git
synced 2026-05-23 20:44:50 +00:00
Merge subagent hooks base into stop stack
This commit is contained in:
@@ -13,7 +13,7 @@ use codex_hooks::PostToolUseRequest;
|
||||
use codex_hooks::PreToolUseOutcome;
|
||||
use codex_hooks::PreToolUseRequest;
|
||||
use codex_hooks::SessionStartOutcome;
|
||||
use codex_hooks::SessionStartTarget;
|
||||
use codex_hooks::StartHookTarget;
|
||||
use codex_hooks::StopHookTarget;
|
||||
use codex_hooks::StopOutcome;
|
||||
use codex_hooks::UserPromptSubmitOutcome;
|
||||
@@ -120,6 +120,9 @@ pub(crate) async fn run_pending_session_start_hooks(
|
||||
return false;
|
||||
};
|
||||
|
||||
// Pending session-start hooks are reused to dispatch thread-spawn subagent
|
||||
// starts. Other subagent sessions are internal/system work and do not run
|
||||
// start hooks.
|
||||
let target = match &turn_context.session_source {
|
||||
SessionSource::SubAgent(SubAgentSource::ThreadSpawn { agent_role, .. })
|
||||
if matches!(
|
||||
@@ -127,15 +130,17 @@ pub(crate) async fn run_pending_session_start_hooks(
|
||||
codex_hooks::SessionStartSource::Startup
|
||||
) =>
|
||||
{
|
||||
let metadata = subagent_hook_metadata(sess, agent_role);
|
||||
SessionStartTarget::SubagentStart {
|
||||
let agent_type = agent_role
|
||||
.clone()
|
||||
.unwrap_or_else(|| crate::agent::role::DEFAULT_ROLE_NAME.to_string());
|
||||
StartHookTarget::SubagentStart {
|
||||
turn_id: turn_context.sub_id.clone(),
|
||||
agent_id: metadata.agent_id,
|
||||
agent_type: metadata.agent_type,
|
||||
agent_id: sess.thread_id().to_string(),
|
||||
agent_type,
|
||||
}
|
||||
}
|
||||
SessionSource::SubAgent(_) => return false,
|
||||
_ => SessionStartTarget::SessionStart {
|
||||
_ => StartHookTarget::SessionStart {
|
||||
source: session_start_source,
|
||||
},
|
||||
};
|
||||
@@ -309,7 +314,9 @@ pub(crate) async fn run_turn_stop_hooks(
|
||||
parent_thread_id,
|
||||
..
|
||||
}) => {
|
||||
let metadata = subagent_hook_metadata(sess, agent_role);
|
||||
let agent_type = agent_role
|
||||
.clone()
|
||||
.unwrap_or_else(|| crate::agent::role::DEFAULT_ROLE_NAME.to_string());
|
||||
let agent_transcript_path = sess.hook_transcript_path().await;
|
||||
let parent_transcript_path = match sess
|
||||
.services
|
||||
@@ -333,8 +340,8 @@ pub(crate) async fn run_turn_stop_hooks(
|
||||
};
|
||||
(
|
||||
StopHookTarget::SubagentStop {
|
||||
agent_id: metadata.agent_id,
|
||||
agent_type: metadata.agent_type,
|
||||
agent_id: sess.thread_id().to_string(),
|
||||
agent_type,
|
||||
agent_transcript_path,
|
||||
},
|
||||
parent_transcript_path,
|
||||
@@ -688,23 +695,6 @@ fn hook_permission_mode(turn_context: &TurnContext) -> String {
|
||||
.to_string()
|
||||
}
|
||||
|
||||
struct SubagentHookMetadata {
|
||||
agent_id: String,
|
||||
agent_type: String,
|
||||
}
|
||||
|
||||
fn subagent_hook_metadata(
|
||||
sess: &Arc<Session>,
|
||||
agent_role: &Option<String>,
|
||||
) -> SubagentHookMetadata {
|
||||
SubagentHookMetadata {
|
||||
agent_id: sess.thread_id().to_string(),
|
||||
agent_type: agent_role
|
||||
.clone()
|
||||
.unwrap_or_else(|| crate::agent::role::DEFAULT_ROLE_NAME.to_string()),
|
||||
}
|
||||
}
|
||||
|
||||
fn compaction_trigger_label(value: CompactionTrigger) -> &'static str {
|
||||
match value {
|
||||
CompactionTrigger::Manual => "manual",
|
||||
|
||||
@@ -532,7 +532,7 @@ async fn preview_session_start_hooks(
|
||||
transcript_path: None,
|
||||
model: "gpt-5.2".to_string(),
|
||||
permission_mode: "default".to_string(),
|
||||
target: codex_hooks::SessionStartTarget::SessionStart {
|
||||
target: codex_hooks::StartHookTarget::SessionStart {
|
||||
source: codex_hooks::SessionStartSource::Startup,
|
||||
},
|
||||
}),
|
||||
@@ -1306,7 +1306,7 @@ async fn reload_user_config_layer_refreshes_hooks() -> anyhow::Result<()> {
|
||||
transcript_path: None,
|
||||
model: "gpt-5.2".to_string(),
|
||||
permission_mode: "default".to_string(),
|
||||
target: codex_hooks::SessionStartTarget::SessionStart {
|
||||
target: codex_hooks::StartHookTarget::SessionStart {
|
||||
source: codex_hooks::SessionStartSource::Startup,
|
||||
},
|
||||
};
|
||||
@@ -1413,7 +1413,7 @@ async fn refresh_runtime_config_refreshes_hooks() -> anyhow::Result<()> {
|
||||
transcript_path: None,
|
||||
model: "gpt-5.2".to_string(),
|
||||
permission_mode: "default".to_string(),
|
||||
target: codex_hooks::SessionStartTarget::SessionStart {
|
||||
target: codex_hooks::StartHookTarget::SessionStart {
|
||||
source: codex_hooks::SessionStartSource::Startup,
|
||||
},
|
||||
};
|
||||
|
||||
@@ -258,21 +258,21 @@ fn read_hook_log(home: &Path, filename: &str) -> Result<Vec<serde_json::Value>>
|
||||
.collect()
|
||||
}
|
||||
|
||||
async fn wait_for_hook_log_entries(
|
||||
async fn wait_for_hook_log(
|
||||
home: &Path,
|
||||
filename: &str,
|
||||
expected_len: usize,
|
||||
) -> Result<Vec<serde_json::Value>> {
|
||||
let deadline = Instant::now() + Duration::from_secs(2);
|
||||
loop {
|
||||
let entries = read_hook_log(home, filename)?;
|
||||
if entries.len() >= expected_len {
|
||||
return Ok(entries);
|
||||
let inputs = read_hook_log(home, filename)?;
|
||||
if inputs.len() >= expected_len {
|
||||
return Ok(inputs);
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
anyhow::bail!(
|
||||
"expected at least {expected_len} entries in {filename}, got {}",
|
||||
entries.len()
|
||||
inputs.len()
|
||||
);
|
||||
}
|
||||
sleep(Duration::from_millis(10)).await;
|
||||
@@ -517,7 +517,12 @@ async fn subagent_start_replaces_session_start_and_injects_context() -> Result<(
|
||||
let child_requests = wait_for_requests(&child_request_log).await?;
|
||||
assert_eq!(child_requests.len(), 1);
|
||||
|
||||
let start_inputs = read_hook_log(test.codex_home_path(), "subagent_start_hook_log.jsonl")?;
|
||||
let start_inputs = wait_for_hook_log(
|
||||
test.codex_home_path(),
|
||||
"subagent_start_hook_log.jsonl",
|
||||
/*expected_len*/ 1,
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(start_inputs.len(), 1);
|
||||
assert_eq!(start_inputs[0]["agent_type"].as_str(), Some("worker"));
|
||||
let spawned_id = wait_for_spawned_thread_id(&test).await?;
|
||||
@@ -526,8 +531,12 @@ async fn subagent_start_replaces_session_start_and_injects_context() -> Result<(
|
||||
Some(spawned_id.as_str())
|
||||
);
|
||||
|
||||
let session_start_inputs =
|
||||
read_hook_log(test.codex_home_path(), "session_start_hook_log.jsonl")?;
|
||||
let session_start_inputs = wait_for_hook_log(
|
||||
test.codex_home_path(),
|
||||
"session_start_hook_log.jsonl",
|
||||
/*expected_len*/ 1,
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(session_start_inputs.len(), 1);
|
||||
assert_eq!(session_start_inputs[0]["source"].as_str(), Some("startup"));
|
||||
assert_ne!(
|
||||
@@ -628,7 +637,7 @@ async fn subagent_stop_replaces_stop_and_skips_internal_subagents() -> Result<()
|
||||
let _ = wait_for_requests(&first_child_request).await?;
|
||||
let _ = wait_for_requests(&second_child_request).await?;
|
||||
|
||||
let subagent_stop_inputs = wait_for_hook_log_entries(
|
||||
let subagent_stop_inputs = wait_for_hook_log(
|
||||
test.codex_home_path(),
|
||||
"subagent_stop_hook_log.jsonl",
|
||||
/*expected_len*/ 2,
|
||||
|
||||
@@ -43,11 +43,11 @@ pub struct SessionStartRequest {
|
||||
pub transcript_path: Option<PathBuf>,
|
||||
pub model: String,
|
||||
pub permission_mode: String,
|
||||
pub target: SessionStartTarget,
|
||||
pub target: StartHookTarget,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub enum SessionStartTarget {
|
||||
pub enum StartHookTarget {
|
||||
SessionStart {
|
||||
source: SessionStartSource,
|
||||
},
|
||||
@@ -58,7 +58,7 @@ pub enum SessionStartTarget {
|
||||
},
|
||||
}
|
||||
|
||||
impl SessionStartTarget {
|
||||
impl StartHookTarget {
|
||||
fn event_name(&self) -> HookEventName {
|
||||
match self {
|
||||
Self::SessionStart { .. } => HookEventName::SessionStart,
|
||||
@@ -123,9 +123,9 @@ pub(crate) async fn run(
|
||||
};
|
||||
}
|
||||
|
||||
let (input_json, turn_id, parse_completed) = match request.target {
|
||||
SessionStartTarget::SessionStart { source } => {
|
||||
match serde_json::to_string(&SessionStartCommandInput::new(
|
||||
let (input_json, turn_id) = match request.target {
|
||||
StartHookTarget::SessionStart { source } => {
|
||||
let input_json = match serde_json::to_string(&SessionStartCommandInput::new(
|
||||
request.session_id.to_string(),
|
||||
request.transcript_path.clone(),
|
||||
request.cwd.display().to_string(),
|
||||
@@ -133,17 +133,7 @@ pub(crate) async fn run(
|
||||
request.permission_mode.clone(),
|
||||
source.as_str().to_string(),
|
||||
)) {
|
||||
Ok(input_json) => (
|
||||
input_json,
|
||||
turn_id,
|
||||
parse_completed
|
||||
as fn(
|
||||
&ConfiguredHandler,
|
||||
CommandRunResult,
|
||||
Option<String>,
|
||||
)
|
||||
-> dispatcher::ParsedHandler<SessionStartHandlerData>,
|
||||
),
|
||||
Ok(input_json) => input_json,
|
||||
Err(error) => {
|
||||
return serialization_failure_outcome(
|
||||
common::serialization_failure_hook_events(
|
||||
@@ -153,16 +143,17 @@ pub(crate) async fn run(
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
};
|
||||
(input_json, turn_id)
|
||||
}
|
||||
SessionStartTarget::SubagentStart {
|
||||
turn_id,
|
||||
StartHookTarget::SubagentStart {
|
||||
turn_id: subagent_turn_id,
|
||||
agent_id,
|
||||
agent_type,
|
||||
} => {
|
||||
let input = SubagentStartCommandInput {
|
||||
session_id: request.session_id.to_string(),
|
||||
turn_id: turn_id.clone(),
|
||||
turn_id: subagent_turn_id.clone(),
|
||||
transcript_path: NullableString::from_path(request.transcript_path.clone()),
|
||||
cwd: request.cwd.display().to_string(),
|
||||
hook_event_name: "SubagentStart".to_string(),
|
||||
@@ -171,28 +162,19 @@ pub(crate) async fn run(
|
||||
agent_id,
|
||||
agent_type,
|
||||
};
|
||||
match serde_json::to_string(&input) {
|
||||
Ok(input_json) => (
|
||||
input_json,
|
||||
Some(turn_id),
|
||||
parse_subagent_start_completed
|
||||
as fn(
|
||||
&ConfiguredHandler,
|
||||
CommandRunResult,
|
||||
Option<String>,
|
||||
)
|
||||
-> dispatcher::ParsedHandler<SessionStartHandlerData>,
|
||||
),
|
||||
let input_json = match serde_json::to_string(&input) {
|
||||
Ok(input_json) => input_json,
|
||||
Err(error) => {
|
||||
return serialization_failure_outcome(
|
||||
common::serialization_failure_hook_events(
|
||||
matched,
|
||||
Some(turn_id),
|
||||
Some(subagent_turn_id),
|
||||
format!("failed to serialize subagent start hook input: {error}"),
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
};
|
||||
(input_json, Some(subagent_turn_id))
|
||||
}
|
||||
};
|
||||
|
||||
@@ -224,55 +206,15 @@ pub(crate) async fn run(
|
||||
}
|
||||
}
|
||||
|
||||
/// Interprets completed `SessionStart` and `SubagentStart` hook runs.
|
||||
///
|
||||
/// The two events have different input payloads but share output handling:
|
||||
/// hook JSON can emit warnings/context or stop the session being started,
|
||||
/// invalid JSON-looking stdout fails, and plain stdout becomes model context.
|
||||
fn parse_completed(
|
||||
handler: &ConfiguredHandler,
|
||||
run_result: CommandRunResult,
|
||||
turn_id: Option<String>,
|
||||
) -> dispatcher::ParsedHandler<SessionStartHandlerData> {
|
||||
parse_start_completed(
|
||||
handler,
|
||||
run_result,
|
||||
turn_id,
|
||||
"session start",
|
||||
output_parser::parse_session_start,
|
||||
StartHookSemantics {
|
||||
allow_plain_text_context: true,
|
||||
allow_stop: true,
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
fn parse_subagent_start_completed(
|
||||
handler: &ConfiguredHandler,
|
||||
run_result: CommandRunResult,
|
||||
turn_id: Option<String>,
|
||||
) -> dispatcher::ParsedHandler<SessionStartHandlerData> {
|
||||
parse_start_completed(
|
||||
handler,
|
||||
run_result,
|
||||
turn_id,
|
||||
"subagent start",
|
||||
output_parser::parse_subagent_start,
|
||||
StartHookSemantics {
|
||||
allow_plain_text_context: false,
|
||||
allow_stop: false,
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
struct StartHookSemantics {
|
||||
allow_plain_text_context: bool,
|
||||
allow_stop: bool,
|
||||
}
|
||||
|
||||
fn parse_start_completed(
|
||||
handler: &ConfiguredHandler,
|
||||
run_result: CommandRunResult,
|
||||
turn_id: Option<String>,
|
||||
hook_label: &str,
|
||||
parse_output: fn(&str) -> Option<output_parser::SessionStartOutput>,
|
||||
semantics: StartHookSemantics,
|
||||
) -> dispatcher::ParsedHandler<SessionStartHandlerData> {
|
||||
let mut entries = Vec::new();
|
||||
let mut status = HookRunStatus::Completed;
|
||||
@@ -292,7 +234,17 @@ fn parse_start_completed(
|
||||
Some(0) => {
|
||||
let trimmed_stdout = run_result.stdout.trim();
|
||||
if trimmed_stdout.is_empty() {
|
||||
} else if let Some(parsed) = parse_output(&run_result.stdout) {
|
||||
} else if let Some(parsed) = match handler.event_name {
|
||||
HookEventName::SessionStart => {
|
||||
output_parser::parse_session_start(&run_result.stdout)
|
||||
}
|
||||
HookEventName::SubagentStart => {
|
||||
output_parser::parse_subagent_start(&run_result.stdout)
|
||||
}
|
||||
event_name => {
|
||||
panic!("expected start hook event, got {event_name:?}")
|
||||
}
|
||||
} {
|
||||
if let Some(system_message) = parsed.universal.system_message {
|
||||
entries.push(HookOutputEntry {
|
||||
kind: HookOutputEntryKind::Warning,
|
||||
@@ -307,7 +259,7 @@ fn parse_start_completed(
|
||||
);
|
||||
}
|
||||
let _ = parsed.universal.suppress_output;
|
||||
if semantics.allow_stop && !parsed.universal.continue_processing {
|
||||
if !parsed.universal.continue_processing {
|
||||
status = HookRunStatus::Stopped;
|
||||
should_stop = true;
|
||||
stop_reason = parsed.universal.stop_reason.clone();
|
||||
@@ -318,14 +270,22 @@ fn parse_start_completed(
|
||||
});
|
||||
}
|
||||
}
|
||||
// Preserve plain-text context support without treating malformed JSON as context.
|
||||
} else if !semantics.allow_plain_text_context
|
||||
|| output_parser::looks_like_json(&run_result.stdout)
|
||||
{
|
||||
} else if output_parser::looks_like_json(&run_result.stdout) {
|
||||
status = HookRunStatus::Failed;
|
||||
entries.push(HookOutputEntry {
|
||||
kind: HookOutputEntryKind::Error,
|
||||
text: format!("hook returned invalid {hook_label} JSON output"),
|
||||
text: match handler.event_name {
|
||||
HookEventName::SessionStart => {
|
||||
"hook returned invalid session start JSON output"
|
||||
}
|
||||
HookEventName::SubagentStart => {
|
||||
"hook returned invalid subagent start JSON output"
|
||||
}
|
||||
event_name => {
|
||||
panic!("expected start hook event, got {event_name:?}")
|
||||
}
|
||||
}
|
||||
.to_string(),
|
||||
});
|
||||
} else {
|
||||
let additional_context = trimmed_stdout.to_string();
|
||||
@@ -485,9 +445,77 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn subagent_start_plain_stdout_becomes_model_context() {
|
||||
let parsed = parse_completed(
|
||||
&handler_for(HookEventName::SubagentStart),
|
||||
run_result(Some(0), "hello from subagent hook\n", ""),
|
||||
/*turn_id*/ Some("turn-1".to_string()),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
parsed.data,
|
||||
SessionStartHandlerData {
|
||||
should_stop: false,
|
||||
stop_reason: None,
|
||||
additional_contexts_for_model: vec!["hello from subagent hook".to_string()],
|
||||
}
|
||||
);
|
||||
assert_eq!(parsed.completed.turn_id.as_deref(), Some("turn-1"));
|
||||
assert_eq!(parsed.completed.run.status, HookRunStatus::Completed);
|
||||
assert_eq!(
|
||||
parsed.completed.run.entries,
|
||||
vec![HookOutputEntry {
|
||||
kind: HookOutputEntryKind::Context,
|
||||
text: "hello from subagent hook".to_string(),
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn subagent_start_continue_false_stops_subagent() {
|
||||
let parsed = parse_completed(
|
||||
&handler_for(HookEventName::SubagentStart),
|
||||
run_result(
|
||||
Some(0),
|
||||
r#"{"continue":false,"stopReason":"skip child","hookSpecificOutput":{"hookEventName":"SubagentStart","additionalContext":"child context"}}"#,
|
||||
"",
|
||||
),
|
||||
/*turn_id*/ Some("turn-1".to_string()),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
parsed.data,
|
||||
SessionStartHandlerData {
|
||||
should_stop: true,
|
||||
stop_reason: Some("skip child".to_string()),
|
||||
additional_contexts_for_model: vec!["child context".to_string()],
|
||||
}
|
||||
);
|
||||
assert_eq!(parsed.completed.turn_id.as_deref(), Some("turn-1"));
|
||||
assert_eq!(parsed.completed.run.status, HookRunStatus::Stopped);
|
||||
assert_eq!(
|
||||
parsed.completed.run.entries,
|
||||
vec![
|
||||
HookOutputEntry {
|
||||
kind: HookOutputEntryKind::Context,
|
||||
text: "child context".to_string(),
|
||||
},
|
||||
HookOutputEntry {
|
||||
kind: HookOutputEntryKind::Stop,
|
||||
text: "skip child".to_string(),
|
||||
},
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
fn handler() -> ConfiguredHandler {
|
||||
handler_for(HookEventName::SessionStart)
|
||||
}
|
||||
|
||||
fn handler_for(event_name: HookEventName) -> ConfiguredHandler {
|
||||
ConfiguredHandler {
|
||||
event_name: HookEventName::SessionStart,
|
||||
event_name,
|
||||
matcher: None,
|
||||
command: "echo hook".to_string(),
|
||||
timeout_sec: 600,
|
||||
|
||||
@@ -58,7 +58,7 @@ 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;
|
||||
pub use events::session_start::SessionStartTarget;
|
||||
pub use events::session_start::StartHookTarget;
|
||||
pub use events::stop::StopHookTarget;
|
||||
pub use events::stop::StopOutcome;
|
||||
pub use events::stop::StopRequest;
|
||||
|
||||
Reference in New Issue
Block a user