From 8801b9a8a6c606508736d4e01dedabe52833d637 Mon Sep 17 00:00:00 2001 From: swastik-oai Date: Fri, 30 Oct 2026 19:18:01 +0000 Subject: [PATCH] Simplify recorded exec command review (#58418) ## What changed Inline validation and command preparation in `review_tool_call`, removing the `RecordedExecCall` helper. Use the tool invocation's call ID for approval actions and return `GuardianReviewResult` directly from the review future, including when no assessment is required. GitOrigin-RevId: aafd01f640d46dc542a649bc5b31b3cfba7bedb8 --- .../core/src/guardian/recorded_tool_call.rs | 128 ++++++++---------- 1 file changed, 54 insertions(+), 74 deletions(-) diff --git a/codex-rs/core/src/guardian/recorded_tool_call.rs b/codex-rs/core/src/guardian/recorded_tool_call.rs index ee3c921d06..f2a128d0ee 100644 --- a/codex-rs/core/src/guardian/recorded_tool_call.rs +++ b/codex-rs/core/src/guardian/recorded_tool_call.rs @@ -19,7 +19,6 @@ use super::review::ReviewInput; use crate::context_manager::ContextManager; use crate::guardian_review::GuardianReviewResult; use crate::session::session::Session; -use crate::session::step_context::StepContext; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::ToolEnvironmentArgs; @@ -27,7 +26,6 @@ use crate::tools::handlers::parse_arguments; use crate::tools::handlers::resolve_tool_environment; use crate::tools::handlers::unified_exec::preparation::ExecCommandLifetime; use crate::tools::handlers::unified_exec::preparation::ParsedExecCommand; -use crate::tools::handlers::unified_exec::preparation::PreparedExecCommand; use crate::tools::registry::PreToolUseError; use crate::tools::registry::apply_pre_tool_use; use crate::tools::sandboxing::ExecApprovalRequirement; @@ -38,7 +36,25 @@ pub(crate) async fn review_tool_call( tool_call: &ResponseItem, cancellation: &CancellationToken, ) -> Result { - let call = RecordedExecCall::from_item(tool_call)?; + match tool_call { + ResponseItem::FunctionCall { + call_id, + name, + namespace, + status, + encrypted_function_args, + .. + } if name == "exec_command" + && encrypted_function_args.as_ref().is_none_or(Vec::is_empty) + && ToolName::new(namespace.clone(), name).is_default_namespace() + && !call_id.trim().is_empty() + && matches!(status.as_deref(), None | Some("completed")) => {} + _ => { + return Err(CodexErr::InvalidRequest( + "expected a completed plaintext exec_command call with a nonempty call ID".into(), + )); + } + }; if history.raw_items().any(|item| { matches!(item, ResponseItem::FunctionCall { name, namespace, .. } if name == "request_permissions" @@ -99,15 +115,35 @@ pub(crate) async fn review_tool_call( "exec_command hook produced an unsupported payload".into(), )); }; - let effective_call = RecordedExecCall { - call_id: call.call_id, - arguments, - }; let step = &invocation.step_context; - let prepared = effective_call.prepare(session, step)?; + let args: ToolEnvironmentArgs = parse_arguments(arguments).map_err(tool_error)?; + let environment = + resolve_tool_environment(&step.environments, args.environment_id.as_deref()) + .map_err(tool_error)?; + let parsed = + ParsedExecCommand::parse(arguments, session, step, environment).map_err(tool_error)?; + let lifetime = if step.turn.config.features.enabled(Feature::UnifiedExec) { + ExecCommandLifetime::Interactive + } else { + ExecCommandLifetime::OneShot + }; + let prepared = parsed.prepare(lifetime).map_err(tool_error)?; + if codex_apply_patch::MaybeApplyPatch::intercepts_command( + &prepared.resolved.command, + &prepared.cwd, + ) { + return Err(CodexErr::InvalidRequest( + "the recorded exec_command would be handled by apply_patch; direct review only supports ordinary commands".into(), + )); + } let requirement = prepared.approval_requirement(session, step).await; let approval = match &requirement { - ExecApprovalRequirement::Skip { .. } => return Ok(None), + ExecApprovalRequirement::Skip { .. } => { + return Ok(GuardianReviewResult { + call_id: invocation.call_id, + assessment: None, + }); + } ExecApprovalRequirement::NeedsApproval { reason, .. } => reason.clone(), ExecApprovalRequirement::NeedsHumanApproval { .. } => { return Err(CodexErr::InvalidRequest( @@ -118,7 +154,7 @@ pub(crate) async fn review_tool_call( return Err(CodexErr::InvalidRequest(reason.clone())); } }; - let action = prepared.approval_action(call.call_id, &requirement); + let action = prepared.approval_action(&invocation.call_id, &requirement); let context = GuardianReviewContext::from(step); let request = from_approval_action(action, &context) .request @@ -141,71 +177,15 @@ pub(crate) async fn review_tool_call( GuardianReviewError::Timeout => CodexErr::RequestTimeout, other => CodexErr::Fatal(format!("Guardian assessment failed: {other:?}")), })?; - Ok(Some(assessment)) + Ok(GuardianReviewResult { + call_id: invocation.call_id, + assessment: Some(assessment), + }) }; - let assessment = tokio::select! { - _ = cancellation.cancelled() => return Err(CodexErr::TurnAborted), - _ = deadline.wait() => return Err(CodexErr::RequestTimeout), - result = review => result?, - }; - Ok(GuardianReviewResult { - call_id: call.call_id.to_owned(), - assessment, - }) -} - -struct RecordedExecCall<'a> { - call_id: &'a str, - arguments: &'a str, -} - -impl<'a> RecordedExecCall<'a> { - fn from_item(item: &'a ResponseItem) -> Result { - match item { - ResponseItem::FunctionCall { - call_id, - name, - namespace, - arguments, - status, - encrypted_function_args, - .. - } if name == "exec_command" - && encrypted_function_args.as_ref().is_none_or(Vec::is_empty) - && ToolName::new(namespace.clone(), name).is_default_namespace() - && !call_id.trim().is_empty() - && matches!(status.as_deref(), None | Some("completed")) => - { - Ok(Self { call_id, arguments }) - } - _ => Err(CodexErr::InvalidRequest( - "expected a completed plaintext exec_command call with a nonempty call ID".into(), - )), - } - } - - fn prepare(&self, session: &Arc, step: &StepContext) -> Result { - let args: ToolEnvironmentArgs = parse_arguments(self.arguments).map_err(tool_error)?; - let environment = - resolve_tool_environment(&step.environments, args.environment_id.as_deref()) - .map_err(tool_error)?; - let parsed = ParsedExecCommand::parse(self.arguments, session, step, environment) - .map_err(tool_error)?; - let lifetime = if step.turn.config.features.enabled(Feature::UnifiedExec) { - ExecCommandLifetime::Interactive - } else { - ExecCommandLifetime::OneShot - }; - let prepared_exec = parsed.prepare(lifetime).map_err(tool_error)?; - if codex_apply_patch::MaybeApplyPatch::intercepts_command( - &prepared_exec.resolved.command, - &prepared_exec.cwd, - ) { - return Err(CodexErr::InvalidRequest( - "the recorded exec_command would be handled by apply_patch; direct review only supports ordinary commands".into(), - )); - } - Ok(prepared_exec) + tokio::select! { + _ = cancellation.cancelled() => Err(CodexErr::TurnAborted), + _ = deadline.wait() => Err(CodexErr::RequestTimeout), + result = review => result, } }