From 4557ab2d2330204f8c3972f34d94e2f36e142d2e Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 14 Apr 2026 16:44:21 -0700 Subject: [PATCH] Support continue/stopReason universal fields --- codex-rs/core/src/codex.rs | 60 ++++++- codex-rs/core/src/function_tool.rs | 2 + codex-rs/core/src/hook_runtime.rs | 12 +- codex-rs/core/src/stream_events_utils.rs | 8 +- codex-rs/core/src/tools/events.rs | 8 + .../core/src/tools/handlers/unified_exec.rs | 16 +- codex-rs/core/src/tools/network_approval.rs | 21 ++- codex-rs/core/src/tools/orchestrator.rs | 9 +- codex-rs/core/src/tools/parallel.rs | 16 +- .../tools/runtimes/shell/unix_escalation.rs | 41 ++++- codex-rs/core/src/tools/sandboxing.rs | 1 + codex-rs/core/src/unified_exec/errors.rs | 6 + .../core/src/unified_exec/process_manager.rs | 7 +- codex-rs/core/tests/suite/hooks.rs | 170 ++++++++++++++++++ codex-rs/hooks/src/engine/output_parser.rs | 32 +++- .../hooks/src/events/permission_request.rs | 99 +++++++++- 16 files changed, 464 insertions(+), 44 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f16f76dad1..2003549768 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -315,6 +315,7 @@ use crate::tools::js_repl::resolve_compatible_node; use crate::tools::network_approval::NetworkApprovalService; use crate::tools::network_approval::build_blocked_request_observer; use crate::tools::network_approval::build_network_policy_decider; +use crate::tools::parallel::ToolCallCompletion; use crate::tools::parallel::ToolCallRuntime; use crate::tools::router::ToolRouterParams; use crate::tools::sandboxing::ApprovalStore; @@ -6439,10 +6440,12 @@ pub(crate) async fn run_turn( let SamplingRequestResult { needs_follow_up: model_needs_follow_up, last_agent_message: sampling_request_last_agent_message, + stop_requested, } = sampling_request_output; can_drain_pending_input = true; let has_pending_input = sess.has_pending_input().await; - let needs_follow_up = model_needs_follow_up || has_pending_input; + let needs_follow_up = + !stop_requested && (model_needs_follow_up || has_pending_input); let total_usage_tokens = sess.get_total_token_usage().await; let token_limit_reached = total_usage_tokens >= auto_compact_limit; @@ -6456,6 +6459,7 @@ pub(crate) async fn run_turn( auto_compact_limit, token_limit_reached, model_needs_follow_up, + stop_requested, has_pending_input, needs_follow_up, "post sampling token usage" @@ -7221,6 +7225,7 @@ pub(crate) async fn built_tools( struct SamplingRequestResult { needs_follow_up: bool, last_agent_message: Option, + stop_requested: bool, } /// Ephemeral per-response state for streaming a single proposed plan. @@ -7743,22 +7748,26 @@ async fn handle_assistant_item_done_in_plan_mode( } async fn drain_in_flight( - in_flight: &mut FuturesOrdered>>, + in_flight: &mut FuturesOrdered>>, sess: Arc, turn_context: Arc, -) -> CodexResult<()> { +) -> CodexResult { while let Some(res) = in_flight.next().await { match res { - Ok(response_input) => { + Ok(ToolCallCompletion::Response(response_input)) => { sess.record_conversation_items(&turn_context, &[response_input.into()]) .await; } + Ok(ToolCallCompletion::StopTurn { reason }) => { + debug!(?reason, "tool requested turn stop"); + return Ok(true); + } Err(err) => { error_or_panic(format!("in-flight tool future failed during drain: {err}")); } } } - Ok(()) + Ok(false) } #[allow(clippy::too_many_arguments)] @@ -7801,7 +7810,7 @@ async fn try_run_sampling_request( .instrument(trace_span!("stream_request")) .or_cancel(&cancellation_token) .await??; - let mut in_flight: FuturesOrdered>> = + let mut in_flight: FuturesOrdered>> = FuturesOrdered::new(); let mut needs_follow_up = false; let mut last_agent_message: Option = None; @@ -7884,6 +7893,25 @@ async fn try_run_sampling_request( cancellation_token: cancellation_token.child_token(), }; + let preempt_for_mailbox_mail = match &item { + ResponseItem::Message { role, phase, .. } => { + role == "assistant" && matches!(phase, Some(MessagePhase::Commentary)) + } + ResponseItem::Reasoning { .. } => true, + ResponseItem::LocalShellCall { .. } + | ResponseItem::FunctionCall { .. } + | ResponseItem::ToolSearchCall { .. } + | ResponseItem::FunctionCallOutput { .. } + | ResponseItem::CustomToolCall { .. } + | ResponseItem::CustomToolCallOutput { .. } + | ResponseItem::ToolSearchOutput { .. } + | ResponseItem::WebSearchCall { .. } + | ResponseItem::ImageGenerationCall { .. } + | ResponseItem::GhostSnapshot { .. } + | ResponseItem::Compaction { .. } + | ResponseItem::Other => false, + }; + let output_result = match handle_output_item_done(&mut ctx, item, previously_active_item) .instrument(handle_responses) @@ -7899,6 +7927,14 @@ async fn try_run_sampling_request( last_agent_message = Some(agent_message); } needs_follow_up |= output_result.needs_follow_up; + // todo: remove before stabilizing multi-agent v2 + if preempt_for_mailbox_mail && sess.mailbox_rx.lock().await.has_pending() { + break Ok(SamplingRequestResult { + needs_follow_up: true, + last_agent_message, + stop_requested: false, + }); + } } ResponseEvent::OutputItemAdded(item) => { if let Some(turn_item) = handle_non_tool_response_item( @@ -7997,6 +8033,7 @@ async fn try_run_sampling_request( break Ok(SamplingRequestResult { needs_follow_up, last_agent_message, + stop_requested: false, }); } ResponseEvent::OutputTextDelta(delta) => { @@ -8087,7 +8124,8 @@ async fn try_run_sampling_request( ) .await; - drain_in_flight(&mut in_flight, sess.clone(), turn_context.clone()).await?; + let stop_requested = + drain_in_flight(&mut in_flight, sess.clone(), turn_context.clone()).await?; if cancellation_token.is_cancelled() { return Err(CodexErr::TurnAborted); @@ -8104,7 +8142,13 @@ async fn try_run_sampling_request( } } - outcome + outcome.map(|mut outcome| { + if stop_requested { + outcome.needs_follow_up = false; + outcome.stop_requested = true; + } + outcome + }) } pub(super) fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) -> Option { diff --git a/codex-rs/core/src/function_tool.rs b/codex-rs/core/src/function_tool.rs index 240e04361c..c6d3406b78 100644 --- a/codex-rs/core/src/function_tool.rs +++ b/codex-rs/core/src/function_tool.rs @@ -4,6 +4,8 @@ use thiserror::Error; pub enum FunctionCallError { #[error("{0}")] RespondToModel(String), + #[error("stop turn")] + StopTurn(Option), #[error("LocalShellCall without call_id or id")] MissingLocalShellCallId, #[error("Fatal error: {0}")] diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 0f8c6586ec..1d6f2f6aa1 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -1,7 +1,6 @@ use std::future::Future; use std::sync::Arc; -use codex_hooks::PermissionRequestDecision; use codex_hooks::PermissionRequestOutcome; use codex_hooks::PermissionRequestRequest; use codex_hooks::PostToolUseOutcome; @@ -157,7 +156,7 @@ pub(crate) async fn run_permission_request_hooks( turn_context: &Arc, run_id_suffix: &str, payload: PermissionRequestPayload, -) -> Option { +) -> PermissionRequestOutcome { let request = PermissionRequestRequest { session_id: sess.conversation_id, turn_id: turn_context.sub_id.clone(), @@ -173,13 +172,10 @@ pub(crate) async fn run_permission_request_hooks( let preview_runs = sess.hooks().preview_permission_request(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; - let PermissionRequestOutcome { - hook_events, - decision, - } = sess.hooks().run_permission_request(request).await; - emit_hook_completed_events(sess, turn_context, hook_events).await; + let outcome = sess.hooks().run_permission_request(request).await; + emit_hook_completed_events(sess, turn_context, outcome.hook_events.clone()).await; - decision + outcome } pub(crate) async fn run_post_tool_use_hooks( diff --git a/codex-rs/core/src/stream_events_utils.rs b/codex-rs/core/src/stream_events_utils.rs index d491bb6adb..40ea6d9c69 100644 --- a/codex-rs/core/src/stream_events_utils.rs +++ b/codex-rs/core/src/stream_events_utils.rs @@ -14,6 +14,7 @@ use crate::function_tool::FunctionCallError; use crate::memories::citations::get_thread_id_from_citations; use crate::memories::citations::parse_memory_citation; use crate::parse_turn_item; +use crate::tools::parallel::ToolCallCompletion; use crate::tools::parallel::ToolCallRuntime; use crate::tools::router::ToolRouter; use codex_protocol::error::CodexErr; @@ -184,7 +185,7 @@ async fn record_stage1_output_usage_for_completed_item( /// queuing any tool execution futures. This records items immediately so /// history and rollout stay in sync even if the turn is later cancelled. pub(crate) type InFlightFuture<'f> = - Pin> + Send + 'f>>; + Pin> + Send + 'f>>; #[derive(Default)] pub(crate) struct OutputItemResult { @@ -320,6 +321,11 @@ pub(crate) async fn handle_output_item_done( output.needs_follow_up = true; } + Err(FunctionCallError::StopTurn(_)) => { + record_completed_response_item(ctx.sess.as_ref(), ctx.turn_context.as_ref(), &item) + .await; + output.needs_follow_up = true; + } // A fatal error occurred; surface it back into history. Err(FunctionCallError::Fatal(message)) => { return Err(CodexErr::Fatal(message)); diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 9a6712efcb..21d163a742 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -353,6 +353,14 @@ impl ToolEmitter { let result = Err(FunctionCallError::RespondToModel(normalized)); (event, result) } + Err(ToolError::StopTurn(reason)) => { + let message = reason + .clone() + .unwrap_or_else(|| "PermissionRequest hook stopped execution".to_string()); + let event = ToolEventStage::Failure(ToolEventFailure::Rejected(message)); + let result = Err(FunctionCallError::StopTurn(reason)); + (event, result) + } }; self.emit(ctx, event).await; result diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 32214b9817..23a0b2d12f 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -309,7 +309,7 @@ impl ToolHandler for UnifiedExecHandler { } emit_unified_exec_tty_metric(&turn.session_telemetry, tty); - manager + match manager .exec_command( ExecCommandRequest { command, @@ -331,11 +331,17 @@ impl ToolHandler for UnifiedExecHandler { &context, ) .await - .map_err(|err| { - FunctionCallError::RespondToModel(format!( + { + Ok(output) => output, + Err(crate::unified_exec::UnifiedExecError::StopTurn(reason)) => { + return Err(FunctionCallError::StopTurn(reason)); + } + Err(err) => { + return Err(FunctionCallError::RespondToModel(format!( "exec_command failed for `{command_for_display}`: {err:?}" - )) - })? + ))); + } + } } "write_stdin" => { let args: WriteStdinArgs = parse_arguments(&arguments)?; diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 8fc53804cf..f0e07a2171 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -118,6 +118,7 @@ enum PendingApprovalDecision { enum NetworkApprovalOutcome { DeniedByUser, DeniedByPolicy(String), + StopTurn(Option), } /// Whether an allowlist miss may be reviewed instead of hard-denied. @@ -381,7 +382,7 @@ impl NetworkApprovalService { let command = owner_call .as_ref() .map_or_else(|| prompt_command.join(" "), |call| call.command.clone()); - if let Some(permission_request_decision) = run_permission_request_hooks( + let permission_request_outcome = run_permission_request_hooks( &session, &turn_context, &guardian_approval_id, @@ -391,8 +392,21 @@ impl NetworkApprovalService { description: Some(format!("network-access {target}")), }, ) - .await - { + .await; + if permission_request_outcome.should_stop { + if let Some(owner_call) = owner_call.as_ref() { + self.record_call_outcome( + &owner_call.registration_id, + NetworkApprovalOutcome::StopTurn(permission_request_outcome.stop_reason), + ) + .await; + } + pending.set_decision(PendingApprovalDecision::Deny).await; + let mut pending_approvals = self.pending_host_approvals.lock().await; + pending_approvals.remove(&key); + return NetworkDecision::deny(REASON_NOT_ALLOWED); + } + if let Some(permission_request_decision) = permission_request_outcome.decision { match permission_request_decision { PermissionRequestDecision::Allow => { pending @@ -668,6 +682,7 @@ pub(crate) async fn finish_immediate_network_approval( Err(ToolError::Rejected("rejected by user".to_string())) } Some(NetworkApprovalOutcome::DeniedByPolicy(message)) => Err(ToolError::Rejected(message)), + Some(NetworkApprovalOutcome::StopTurn(reason)) => Err(ToolError::StopTurn(reason)), None => Ok(()), } } diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 7d3be77719..bdac9e2f56 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -369,14 +369,17 @@ impl ToolOrchestrator { T: ToolRuntime, { if let Some(permission_request) = tool.permission_request_payload(req) { - match run_permission_request_hooks( + let permission_request_outcome = run_permission_request_hooks( approval_ctx.session, approval_ctx.turn, permission_request_run_id, permission_request, ) - .await - { + .await; + if permission_request_outcome.should_stop { + return Err(ToolError::StopTurn(permission_request_outcome.stop_reason)); + } + match permission_request_outcome.decision { Some(PermissionRequestDecision::Allow) => { telemetry.otel.tool_decision( telemetry.tool_name, diff --git a/codex-rs/core/src/tools/parallel.rs b/codex-rs/core/src/tools/parallel.rs index a1965db5cc..af215775d3 100644 --- a/codex-rs/core/src/tools/parallel.rs +++ b/codex-rs/core/src/tools/parallel.rs @@ -32,6 +32,11 @@ pub(crate) struct ToolCallRuntime { parallel_execution: Arc>, } +pub(crate) enum ToolCallCompletion { + Response(ResponseInputItem), + StopTurn { reason: Option }, +} + impl ToolCallRuntime { pub(crate) fn new( router: Arc, @@ -57,15 +62,20 @@ impl ToolCallRuntime { self, call: ToolCall, cancellation_token: CancellationToken, - ) -> impl std::future::Future> { + ) -> impl std::future::Future> { let error_call = call.clone(); let future = self.handle_tool_call_with_source(call, ToolCallSource::Direct, cancellation_token); async move { match future.await { - Ok(response) => Ok(response.into_response()), + Ok(response) => Ok(ToolCallCompletion::Response(response.into_response())), + Err(FunctionCallError::StopTurn(reason)) => { + Ok(ToolCallCompletion::StopTurn { reason }) + } Err(FunctionCallError::Fatal(message)) => Err(CodexErr::Fatal(message)), - Err(other) => Ok(Self::failure_response(error_call, other)), + Err(other) => Ok(ToolCallCompletion::Response(Self::failure_response( + error_call, other, + ))), } } .in_current_span() diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 18813cfc89..09a32d6cfb 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -216,7 +216,7 @@ pub(super) async fn try_run_zsh_fork( let exec_result = escalate_server .exec(exec_params, cancel_token, Arc::new(command_executor)) .await - .map_err(|err| ToolError::Rejected(err.to_string()))?; + .map_err(map_escalation_error)?; map_exec_result(attempt.sandbox, exec_result).map(Some) } @@ -325,6 +325,13 @@ struct PromptDecision { decision: ReviewDecision, guardian_review_id: Option, rejection_message: Option, + stop_turn: Option, +} + +#[derive(Debug, thiserror::Error)] +#[error("stop turn")] +struct StopTurnRequested { + reason: Option, } fn execve_prompt_is_rejected_by_policy( @@ -406,19 +413,28 @@ impl CoreShellActionProvider { description: None, }; let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); - match run_permission_request_hooks( + let permission_request_outcome = run_permission_request_hooks( &session, &turn, &effective_approval_id, permission_request, ) - .await - { + .await; + if permission_request_outcome.should_stop { + return PromptDecision { + decision: ReviewDecision::Denied, + guardian_review_id: None, + rejection_message: None, + stop_turn: permission_request_outcome.stop_reason, + }; + } + match permission_request_outcome.decision { Some(PermissionRequestDecision::Allow) => { return PromptDecision { decision: ReviewDecision::Approved, guardian_review_id: None, rejection_message: None, + stop_turn: None, }; } Some(PermissionRequestDecision::Deny { message }) => { @@ -426,6 +442,7 @@ impl CoreShellActionProvider { decision: ReviewDecision::Denied, guardian_review_id: None, rejection_message: Some(message), + stop_turn: None, }; } None => {} @@ -452,6 +469,7 @@ impl CoreShellActionProvider { decision, guardian_review_id, rejection_message: None, + stop_turn: None, }; } @@ -474,6 +492,7 @@ impl CoreShellActionProvider { decision, guardian_review_id: None, rejection_message: None, + stop_turn: None, } }) .await) @@ -504,6 +523,12 @@ impl CoreShellActionProvider { let prompt_decision = self .prompt(program, argv, workdir, &self.stopwatch, prompt_permissions) .await?; + if let Some(reason) = prompt_decision.stop_turn { + return Err(StopTurnRequested { + reason: Some(reason), + } + .into()); + } match prompt_decision.decision { ReviewDecision::Approved | ReviewDecision::ApprovedForSession @@ -566,6 +591,14 @@ impl CoreShellActionProvider { } } +fn map_escalation_error(err: anyhow::Error) -> ToolError { + if let Some(stop_turn) = err.downcast_ref::() { + ToolError::StopTurn(stop_turn.reason.clone()) + } else { + ToolError::Rejected(err.to_string()) + } +} + // Shell-wrapper parsing is weaker than direct exec interception because it can // only see the script text, not the final resolved executable path. Keep it // disabled by default so path-sensitive rules rely on the later authoritative diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index d85298734d..eeb6d8afb9 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -321,6 +321,7 @@ pub(crate) struct ToolCtx { #[derive(Debug)] pub(crate) enum ToolError { Rejected(String), + StopTurn(Option), Codex(CodexErr), } diff --git a/codex-rs/core/src/unified_exec/errors.rs b/codex-rs/core/src/unified_exec/errors.rs index 0576e7d7cf..aa7d36a9d1 100644 --- a/codex-rs/core/src/unified_exec/errors.rs +++ b/codex-rs/core/src/unified_exec/errors.rs @@ -3,6 +3,8 @@ use thiserror::Error; #[derive(Debug, Error)] pub(crate) enum UnifiedExecError { + #[error("stop turn")] + StopTurn(Option), #[error("Failed to create unified exec process: {message}")] CreateProcess { message: String }, #[error("Unified exec process failed: {message}")] @@ -26,6 +28,10 @@ pub(crate) enum UnifiedExecError { } impl UnifiedExecError { + pub(crate) fn stop_turn(reason: Option) -> Self { + Self::StopTurn(reason) + } + pub(crate) fn create_process(message: String) -> Self { Self::CreateProcess { message } } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index d959ae0d91..8a9ab8922c 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -778,7 +778,12 @@ impl UnifiedExecProcessManager { ) .await .map(|result| (result.output, result.deferred_network_approval)) - .map_err(|e| UnifiedExecError::create_process(format!("{e:?}"))) + .map_err(|err| match err { + crate::tools::sandboxing::ToolError::StopTurn(reason) => { + UnifiedExecError::stop_turn(reason) + } + other => UnifiedExecError::create_process(format!("{other:?}")), + }) } pub(super) async fn collect_output_until_deadline( diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index ee63ca77b8..6674e73160 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -294,6 +294,11 @@ elif mode == "deny": }} }} }})) +elif mode == "continue_false": + print(json.dumps({{ + "continue": False, + "stopReason": reason + }})) elif mode == "exit_2": sys.stderr.write(reason + "\n") raise SystemExit(2) @@ -1221,6 +1226,81 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_continue_false_stops_turn_without_tool_output() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "permissionrequest-continue-false"; + let marker = std::env::temp_dir().join("permissionrequest-continue-false-marker"); + let command = format!("rm -f {}", marker.display()); + let stop_reason = "stop after permission request"; + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + ) + .await; + + let test = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = write_permission_request_hook( + home, + Some(PERMISSION_REQUEST_HOOK_MATCHER), + "continue_false", + stop_reason, + ) { + panic!("failed to write permission request hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }) + .build(&server) + .await?; + + fs::write(&marker, "seed").context("create permission request continue-false marker")?; + + test.submit_turn_with_policies( + "run the shell command after permission request hook", + AskForApproval::OnRequest, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + assert_eq!(responses.requests().len(), 1); + assert!( + marker.exists(), + "continue:false should stop before the tool executes" + ); + assert_single_permission_request_hook_input( + test.codex_home_path(), + &command, + /*description*/ None, + )?; + assert!( + timeout( + Duration::from_secs(2), + wait_for_event(&test.codex, |event| matches!( + event, + EventMsg::ExecApprovalRequest(_) + )) + ) + .await + .is_err(), + "expected the permission request hook to stop before approval UI appears" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn permission_request_hook_sees_raw_exec_command_input() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1302,6 +1382,96 @@ async fn permission_request_hook_sees_raw_exec_command_input() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_continue_false_stops_exec_command_turn() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "permissionrequest-exec-command-continue-false"; + let marker = std::env::temp_dir().join("permissionrequest-exec-command-continue-false-marker"); + let command = format!("rm -f {}", marker.display()); + let stop_reason = "stop after permission request"; + let justification = "keep the temporary marker in place"; + let args = serde_json::json!({ + "cmd": command, + "login": true, + "sandbox_permissions": "require_escalated", + "justification": justification, + }); + let responses = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + call_id, + "exec_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-1"), + ]), + ) + .await; + + let test = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = write_permission_request_hook( + home, + Some(PERMISSION_REQUEST_HOOK_MATCHER), + "continue_false", + stop_reason, + ) { + panic!("failed to write permission request hook test fixture: {error}"); + } + }) + .with_config(|config| { + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }) + .build(&server) + .await?; + + fs::write(&marker, "seed").context("create exec command continue-false marker")?; + + test.submit_turn_with_policies( + "run the exec command after permission request hook", + AskForApproval::OnRequest, + codex_protocol::protocol::SandboxPolicy::new_read_only_policy(), + ) + .await?; + + assert_eq!(responses.requests().len(), 1); + assert!( + marker.exists(), + "continue:false should stop before exec_command executes" + ); + assert_single_permission_request_hook_input( + test.codex_home_path(), + &command, + Some(justification), + )?; + assert!( + timeout( + Duration::from_secs(2), + wait_for_event(&test.codex, |event| matches!( + event, + EventMsg::ExecApprovalRequest(_) + )) + ) + .await + .is_err(), + "expected the permission request hook to stop before approval UI appears" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn permission_request_hook_allows_network_approval_without_prompt() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 0a3a994e19..d034e4ee19 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -275,11 +275,7 @@ fn unsupported_pre_tool_use_universal(universal: &UniversalOutput) -> Option Option { - if !universal.continue_processing { - Some("PermissionRequest hook returned unsupported continue:false".to_string()) - } else if universal.stop_reason.is_some() { - Some("PermissionRequest hook returned unsupported stopReason".to_string()) - } else if universal.suppress_output { + if universal.suppress_output { Some("PermissionRequest hook returned unsupported suppressOutput".to_string()) } else { None @@ -491,4 +487,30 @@ mod tests { Some("PermissionRequest hook returned unsupported interrupt:true".to_string()) ); } + + #[test] + fn permission_request_accepts_continue_false_and_stop_reason() { + let parsed = parse_permission_request( + &json!({ + "continue": false, + "stopReason": "stop now", + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "allow" + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + + assert_eq!(parsed.invalid_reason, None); + assert!(!parsed.universal.continue_processing); + assert_eq!(parsed.universal.stop_reason.as_deref(), Some("stop now")); + assert_eq!( + parsed.decision, + Some(super::PermissionRequestDecision::Allow) + ); + } } diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index c6b58b86f1..51e52dd7f4 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -54,11 +54,15 @@ pub enum PermissionRequestDecision { #[derive(Debug)] pub struct PermissionRequestOutcome { pub hook_events: Vec, + pub should_stop: bool, + pub stop_reason: Option, pub decision: Option, } #[derive(Debug, Default, PartialEq, Eq)] struct PermissionRequestHandlerData { + should_stop: bool, + stop_reason: Option, decision: Option, } @@ -94,6 +98,8 @@ pub(crate) async fn run( if matched.is_empty() { return PermissionRequestOutcome { hook_events: Vec::new(), + should_stop: false, + stop_reason: None, decision: None, }; } @@ -109,6 +115,8 @@ pub(crate) async fn run( ); return PermissionRequestOutcome { hook_events, + should_stop: false, + stop_reason: None, decision: None, }; } @@ -124,6 +132,10 @@ pub(crate) async fn run( ) .await; + let should_stop = results.iter().any(|result| result.data.should_stop); + let stop_reason = results + .iter() + .find_map(|result| result.data.stop_reason.clone()); // Preserve the most specific matching allow, but treat any deny as final so // broader policy layers cannot accidentally overrule a more specific block. let decision = resolve_permission_request_decision( @@ -139,7 +151,9 @@ pub(crate) async fn run( common::hook_completed_for_tool_use(result.completed, &request.run_id_suffix) }) .collect(), - decision, + should_stop, + stop_reason, + decision: (!should_stop).then_some(decision).flatten(), } } @@ -189,6 +203,8 @@ fn parse_completed( ) -> dispatcher::ParsedHandler { let mut entries = Vec::new(); let mut status = HookRunStatus::Completed; + let mut should_stop = false; + let mut stop_reason = None; let mut decision = None; match run_result.error.as_deref() { @@ -212,7 +228,18 @@ fn parse_completed( text: system_message, }); } - if let Some(invalid_reason) = parsed.invalid_reason { + if !parsed.universal.continue_processing { + status = HookRunStatus::Stopped; + should_stop = true; + stop_reason = parsed.universal.stop_reason.clone(); + let stop_text = parsed.universal.stop_reason.unwrap_or_else(|| { + "PermissionRequest hook stopped execution".to_string() + }); + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Stop, + text: stop_text, + }); + } else if let Some(invalid_reason) = parsed.invalid_reason { status = HookRunStatus::Failed; entries.push(HookOutputEntry { kind: HookOutputEntryKind::Error, @@ -281,16 +308,28 @@ fn parse_completed( dispatcher::ParsedHandler { completed, - data: PermissionRequestHandlerData { decision }, + data: PermissionRequestHandlerData { + should_stop, + stop_reason, + decision, + }, } } #[cfg(test)] mod tests { + use codex_protocol::protocol::HookEventName; + use codex_protocol::protocol::HookOutputEntry; + use codex_protocol::protocol::HookOutputEntryKind; + use codex_protocol::protocol::HookRunStatus; use pretty_assertions::assert_eq; use super::PermissionRequestDecision; + use super::PermissionRequestHandlerData; + use super::parse_completed; use super::resolve_permission_request_decision; + use crate::engine::ConfiguredHandler; + use crate::engine::command_runner::CommandRunResult; #[test] fn permission_request_deny_overrides_earlier_allow() { @@ -328,4 +367,58 @@ mod tests { assert_eq!(resolve_permission_request_decision(decisions.iter()), None); } + + #[test] + fn continue_false_stops_permission_request_flow() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"continue":false,"stopReason":"stop now","hookSpecificOutput":{"hookEventName":"PermissionRequest","decision":{"behavior":"allow"}}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PermissionRequestHandlerData { + should_stop: true, + stop_reason: Some("stop now".to_string()), + decision: None, + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Stopped); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Stop, + text: "stop now".to_string(), + }] + ); + } + + fn handler() -> ConfiguredHandler { + ConfiguredHandler { + event_name: HookEventName::PermissionRequest, + matcher: None, + command: "python3 hook.py".to_string(), + timeout_sec: 30, + status_message: None, + source_path: std::path::PathBuf::from("/tmp/hooks.json"), + display_order: 0, + } + } + + fn run_result(exit_code: Option, stdout: &str, stderr: &str) -> CommandRunResult { + CommandRunResult { + started_at: 0, + completed_at: 1, + duration_ms: 1, + exit_code, + stdout: stdout.to_string(), + stderr: stderr.to_string(), + error: None, + } + } }