From bcccc6c208ec91e882de9b0a75f0ea88dc8a35dd Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 30 Apr 2026 16:51:14 -0700 Subject: [PATCH] address hook rewrite review feedback --- codex-rs/core/src/tools/events.rs | 6 +----- codex-rs/core/src/tools/network_approval.rs | 10 +++++++++ codex-rs/core/src/tools/orchestrator.rs | 4 ++-- codex-rs/core/src/tools/sandboxing.rs | 8 +++++++ codex-rs/core/src/tools/sandboxing_tests.rs | 24 +++++++++++++++++++++ 5 files changed, 45 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index fabc9a8141..175cb41af1 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -354,11 +354,7 @@ impl ToolEmitter { (event, result) } Err(ToolError::UpdatedInput(updated_input)) => { - let event = ToolEventStage::Failure(ToolEventFailure::Message( - "tool input rewritten by hook".to_string(), - )); - let result = Err(FunctionCallError::UpdatedInput(updated_input)); - (event, result) + return Err(FunctionCallError::UpdatedInput(updated_input)); } }; self.emit(ctx, event).await; diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 76d66f140b..c77dcad54b 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -476,6 +476,16 @@ impl NetworkApprovalService { PermissionRequestDecision::Allow { updated_input: Some(_), } => { + if let Some(owner_call) = owner_call.as_ref() { + self.record_call_outcome( + &owner_call.registration_id, + NetworkApprovalOutcome::DeniedByPolicy( + "updatedInput is not supported for network approval requests" + .to_string(), + ), + ) + .await; + } pending.set_decision(PendingApprovalDecision::Deny).await; let mut pending_approvals = self.pending_host_approvals.lock().await; pending_approvals.remove(&key); diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 05e0a530d9..89c378104f 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -397,7 +397,7 @@ impl ToolOrchestrator { if evaluate_permission_request_hooks && let Some(permission_request) = tool.permission_request_payload(req) { - let permission_request_tool_input = permission_request.tool_input.clone(); + let permission_request_for_noop_check = permission_request.clone(); match run_permission_request_hooks( approval_ctx.session, approval_ctx.turn, @@ -409,7 +409,7 @@ impl ToolOrchestrator { Some(PermissionRequestDecision::Allow { updated_input: Some(updated_input), }) => { - if updated_input != permission_request_tool_input { + if !permission_request_for_noop_check.updated_input_is_noop(&updated_input) { return Err(ToolError::UpdatedInput(updated_input)); } let decision = ReviewDecision::Approved; diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index f6581368cf..08a4e8026c 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -154,6 +154,14 @@ impl PermissionRequestPayload { tool_input: serde_json::Value::Object(tool_input), } } + + pub(crate) fn updated_input_is_noop(&self, updated_input: &serde_json::Value) -> bool { + if self.tool_name.name() == "Bash" { + return self.tool_input.get("command") == updated_input.get("command"); + } + + self.tool_input == *updated_input + } } // Specifies what tool orchestrator should do with a given tool call. diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 19eb7a67d9..f2fb189137 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -34,6 +34,30 @@ fn bash_permission_request_payload_includes_description_when_present() { ); } +#[test] +fn bash_updated_input_noop_ignores_description() { + let payload = PermissionRequestPayload::bash( + "echo hi".to_string(), + Some("network-access example.com".to_string()), + ); + + assert!(payload.updated_input_is_noop(&json!({ "command": "echo hi" }))); +} + +#[test] +fn non_bash_updated_input_noop_requires_full_equality() { + let payload = PermissionRequestPayload { + tool_name: HookToolName::apply_patch(), + tool_input: json!({ "command": "patch" }), + }; + + assert!(payload.updated_input_is_noop(&json!({ "command": "patch" }))); + assert!(!payload.updated_input_is_noop(&json!({ + "command": "patch", + "description": "ignored" + }))); +} + #[test] fn external_sandbox_skips_exec_approval_on_request() { let sandbox_policy = SandboxPolicy::ExternalSandbox {