diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 6acae02578..7be05d060d 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -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, - agent_role: &Option, -) -> 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", diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 4f441ea08f..eaeaf77295 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -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, }, }; diff --git a/codex-rs/core/tests/suite/subagent_notifications.rs b/codex-rs/core/tests/suite/subagent_notifications.rs index 95d6b0e62d..c2d80ce2cf 100644 --- a/codex-rs/core/tests/suite/subagent_notifications.rs +++ b/codex-rs/core/tests/suite/subagent_notifications.rs @@ -258,21 +258,21 @@ fn read_hook_log(home: &Path, filename: &str) -> Result> .collect() } -async fn wait_for_hook_log_entries( +async fn wait_for_hook_log( home: &Path, filename: &str, expected_len: usize, ) -> Result> { 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, diff --git a/codex-rs/hooks/src/events/session_start.rs b/codex-rs/hooks/src/events/session_start.rs index fdf550b690..76eb508083 100644 --- a/codex-rs/hooks/src/events/session_start.rs +++ b/codex-rs/hooks/src/events/session_start.rs @@ -43,11 +43,11 @@ pub struct SessionStartRequest { pub transcript_path: Option, 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, - ) - -> dispatcher::ParsedHandler, - ), + 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, - ) - -> dispatcher::ParsedHandler, - ), + 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, -) -> dispatcher::ParsedHandler { - 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, -) -> dispatcher::ParsedHandler { - 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, - hook_label: &str, - parse_output: fn(&str) -> Option, - semantics: StartHookSemantics, ) -> dispatcher::ParsedHandler { 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, diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 2bd0cc2d6e..70ee3b4540 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -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;