diff --git a/codex-rs/core/src/function_tool.rs b/codex-rs/core/src/function_tool.rs index 5fd6639958..240e04361c 100644 --- a/codex-rs/core/src/function_tool.rs +++ b/codex-rs/core/src/function_tool.rs @@ -4,8 +4,6 @@ use thiserror::Error; pub enum FunctionCallError { #[error("{0}")] RespondToModel(String), - #[error("tool input rewritten by hook")] - UpdatedInput(serde_json::Value), #[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 d34f91a03a..8b6b28cfc3 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -48,8 +48,6 @@ pub(crate) enum PreToolUseHookResult { Blocked(String), } -pub(crate) const MAX_HOOK_INPUT_REWRITES: usize = 8; - pub(crate) enum PendingInputHookDisposition { Accepted(Box), Blocked { additional_contexts: Vec }, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 117eca7c5c..91e2e079ab 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -25,7 +25,6 @@ use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; -use crate::hook_runtime::MAX_HOOK_INPUT_REWRITES; use crate::hook_runtime::run_permission_request_hooks; use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files; use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; @@ -34,6 +33,7 @@ use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; +use crate::turn_metadata::McpTurnMetadataContext; use codex_analytics::AppInvocation; use codex_analytics::InvocationType; use codex_analytics::build_track_events_context; @@ -197,45 +197,18 @@ pub(crate) async fn handle_mcp_tool_call( ) .await; - let mut invocation = invocation; - let mut input_rewrites = 0; - loop { - let Some(decision) = maybe_request_mcp_tool_approval( - &sess, - turn_context, - &call_id, - &invocation, - &hook_tool_name, - metadata.as_ref(), - approval_mode, - ) - .await - else { - break; - }; - let tool_input = invocation - .arguments - .clone() - .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())); + if let Some(decision) = maybe_request_mcp_tool_approval( + &sess, + turn_context, + &call_id, + &invocation, + &hook_tool_name, + metadata.as_ref(), + approval_mode, + ) + .await + { let result = match decision { - McpToolApprovalDecision::AcceptWithUpdatedInput(updated_input) => { - input_rewrites += 1; - if input_rewrites > MAX_HOOK_INPUT_REWRITES { - notify_mcp_tool_call_skip( - sess.as_ref(), - turn_context.as_ref(), - &call_id, - invocation, - mcp_app_resource_uri.clone(), - "hook input rewrite limit exceeded".to_string(), - /*already_started*/ true, - ) - .await - } else { - invocation.arguments = Some(updated_input); - continue; - } - } McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember => { @@ -302,7 +275,8 @@ pub(crate) async fn handle_mcp_tool_call( return HandledMcpToolCall { result: CallToolResult::from_result(result), - tool_input, + tool_input: arguments_value + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), }; } @@ -841,7 +815,6 @@ async fn maybe_track_codex_app_used( #[derive(Debug, Clone, PartialEq, Eq)] enum McpToolApprovalDecision { Accept, - AcceptWithUpdatedInput(JsonValue), AcceptForSession, AcceptAndRemember, Decline { message: Option }, @@ -923,7 +896,13 @@ fn build_mcp_tool_call_request_meta( ) -> Option { let mut request_meta = serde_json::Map::new(); - if let Some(turn_metadata) = turn_context.turn_metadata_state.current_meta_value() { + if let Some(turn_metadata) = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(McpTurnMetadataContext { + model: turn_context.model_info.slug.as_str(), + reasoning_effort: turn_context.effective_reasoning_effort(), + }) + { request_meta.insert( crate::X_CODEX_TURN_METADATA_HEADER.to_string(), turn_metadata, @@ -1099,34 +1078,21 @@ async fn maybe_request_mcp_tool_approval( return Some(McpToolApprovalDecision::Accept); } - let permission_request_tool_input = invocation - .arguments - .clone() - .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())); match run_permission_request_hooks( sess, turn_context, call_id, PermissionRequestPayload { tool_name: HookToolName::new(hook_tool_name), - tool_input: permission_request_tool_input.clone(), + tool_input: invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), }, ) .await { - Some(PermissionRequestDecision::Allow { - updated_input: Some(updated_input), - }) => { - if updated_input == permission_request_tool_input { - return Some(McpToolApprovalDecision::Accept); - } - return Some(McpToolApprovalDecision::AcceptWithUpdatedInput( - updated_input, - )); - } - Some(PermissionRequestDecision::Allow { - updated_input: None, - }) => { + Some(PermissionRequestDecision::Allow) => { return Some(McpToolApprovalDecision::Accept); } Some(PermissionRequestDecision::Deny { message }) => { @@ -1880,7 +1846,6 @@ async fn apply_mcp_tool_approval_decision( } } McpToolApprovalDecision::Accept - | McpToolApprovalDecision::AcceptWithUpdatedInput(_) | McpToolApprovalDecision::Decline { .. } | McpToolApprovalDecision::Cancel | McpToolApprovalDecision::BlockedBySafetyMonitor(_) => {} diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 37d1c7f4f0..a6818579a6 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -4,6 +4,7 @@ use crate::session::tests::make_session_and_context; use crate::session::tests::make_session_and_context_with_rx; use crate::state::ActiveTurn; use crate::test_support::models_manager_with_provider; +use crate::turn_metadata::McpTurnMetadataContext; use codex_config::CONFIG_TOML_FILE; use codex_config::config_toml::ConfigToml; use codex_config::types::AppConfig; @@ -71,6 +72,13 @@ fn approval_metadata( } } +fn mcp_turn_metadata_context(turn_context: &TurnContext) -> McpTurnMetadataContext<'_> { + McpTurnMetadataContext { + model: turn_context.model_info.slug.as_str(), + reasoning_effort: turn_context.effective_reasoning_effort(), + } +} + fn write_sample_plugin_mcp(codex_home: &std::path::Path) { let plugin_root = codex_home.join("plugins/cache/test/sample/local"); std::fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create plugin manifest dir"); @@ -920,13 +928,10 @@ fn truncate_mcp_tool_result_for_event_bounds_large_error() { #[tokio::test] async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = serde_json::from_str::( - &turn_context - .turn_metadata_state - .current_header_value() - .expect("turn metadata header"), - ) - .expect("turn metadata json"); + let expected_turn_metadata = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) + .expect("turn metadata"); let meta = build_mcp_tool_call_request_meta( &turn_context, @@ -935,6 +940,25 @@ async fn mcp_tool_call_request_meta_includes_turn_metadata_for_custom_server() { /*metadata*/ None, ) .expect("custom servers should receive turn metadata"); + let turn_metadata = meta + .get(crate::X_CODEX_TURN_METADATA_HEADER) + .expect("turn metadata should be present"); + + assert_eq!( + turn_metadata + .get("model") + .and_then(serde_json::Value::as_str), + Some(turn_context.model_info.slug.as_str()) + ); + assert_eq!( + turn_metadata + .get("reasoning_effort") + .and_then(serde_json::Value::as_str), + turn_context + .effective_reasoning_effort() + .map(|effort| effort.to_string()) + .as_deref() + ); assert_eq!( meta, @@ -973,13 +997,10 @@ async fn mcp_tool_call_request_meta_includes_turn_started_at_unix_ms() { #[tokio::test] async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps_meta() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = serde_json::from_str::( - &turn_context - .turn_metadata_state - .current_header_value() - .expect("turn metadata header"), - ) - .expect("turn metadata json"); + let expected_turn_metadata = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) + .expect("turn metadata"); let metadata = McpToolApprovalMetadata { annotations: None, connector_id: Some("calendar".to_string()), @@ -1023,13 +1044,10 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps #[tokio::test] async fn codex_apps_tool_call_request_meta_includes_call_id_without_existing_codex_apps_meta() { let (_, turn_context) = make_session_and_context().await; - let expected_turn_metadata = serde_json::from_str::( - &turn_context - .turn_metadata_state - .current_header_value() - .expect("turn metadata header"), - ) - .expect("turn metadata json"); + let expected_turn_metadata = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) + .expect("turn metadata"); assert_eq!( build_mcp_tool_call_request_meta( @@ -2082,80 +2100,6 @@ async fn permission_request_hook_allows_mcp_tool_call() { ); } -#[tokio::test] -async fn permission_request_hook_can_update_mcp_tool_input() { - let (mut session, turn_context) = make_session_and_context().await; - install_mcp_permission_request_hook( - &mut session, - &turn_context, - "mcp__memory__.*", - &serde_json::json!({ - "hookSpecificOutput": { - "hookEventName": "PermissionRequest", - "decision": { - "behavior": "allow", - "updatedInput": { - "entities": [{ - "name": "Grace", - "entityType": "person" - }] - } - } - } - }), - ); - let session = Arc::new(session); - let turn_context = Arc::new(turn_context); - let invocation = McpInvocation { - server: "memory".to_string(), - tool: "create_entities".to_string(), - arguments: Some(serde_json::json!({ - "entities": [{ - "name": "Ada", - "entityType": "person" - }] - })), - }; - let metadata = McpToolApprovalMetadata { - annotations: Some(annotations( - Some(false), - Some(true), - /*open_world*/ None, - )), - connector_id: None, - connector_name: None, - connector_description: None, - tool_title: Some("Create entities".to_string()), - tool_description: None, - mcp_app_resource_uri: None, - codex_apps_meta: None, - openai_file_input_params: None, - }; - - let decision = maybe_request_mcp_tool_approval( - &session, - &turn_context, - "call-mcp-hook", - &invocation, - "mcp__memory__create_entities", - Some(&metadata), - AppToolApproval::Auto, - ) - .await; - - assert_eq!( - decision, - Some(McpToolApprovalDecision::AcceptWithUpdatedInput( - serde_json::json!({ - "entities": [{ - "name": "Grace", - "entityType": "person" - }] - }) - )) - ); -} - #[tokio::test] async fn permission_request_hook_uses_hook_tool_name_without_metadata() { let (mut session, turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/src/stream_events_utils.rs b/codex-rs/core/src/stream_events_utils.rs index 00dc66df20..fc608aed93 100644 --- a/codex-rs/core/src/stream_events_utils.rs +++ b/codex-rs/core/src/stream_events_utils.rs @@ -336,11 +336,6 @@ pub(crate) async fn handle_output_item_done( output.needs_follow_up = true; } - Err(FunctionCallError::UpdatedInput(_)) => { - return Err(CodexErr::Fatal( - "updated tool input escaped dispatch".to_string(), - )); - } // 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 a618c8d59a..f97e51fd8a 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -353,9 +353,6 @@ impl ToolEmitter { let result = Err(FunctionCallError::RespondToModel(normalized)); (event, result) } - Err(ToolError::UpdatedInput(updated_input)) => { - return Err(FunctionCallError::UpdatedInput(updated_input)); - } }; 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 96d5beaa97..2c19c6b6c7 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -372,9 +372,6 @@ impl ToolHandler for ExecCommandHandler { .await { Ok(response) => Ok(response), - Err(UnifiedExecError::UpdatedInput(updated_input)) => { - Err(FunctionCallError::UpdatedInput(updated_input)) - } Err(UnifiedExecError::SandboxDenied { output, .. }) => { let output_text = output.aggregated_output.text; let original_token_count = approx_token_count(&output_text); diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index c77dcad54b..14af2c9c5f 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -473,29 +473,7 @@ impl NetworkApprovalService { .await { match permission_request_decision { - 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); - return NetworkDecision::deny( - "updatedInput is not supported for network approval requests", - ); - } - PermissionRequestDecision::Allow { - updated_input: None, - } => { + PermissionRequestDecision::Allow => { pending .set_decision(PendingApprovalDecision::AllowOnce) .await; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index f438da301b..c618d778d6 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -398,7 +398,6 @@ impl ToolOrchestrator { if evaluate_permission_request_hooks && let Some(permission_request) = tool.permission_request_payload(req) { - let permission_request_for_noop_check = permission_request.clone(); match run_permission_request_hooks( approval_ctx.session, approval_ctx.turn, @@ -407,24 +406,7 @@ impl ToolOrchestrator { ) .await { - Some(PermissionRequestDecision::Allow { - updated_input: Some(updated_input), - }) => { - if !permission_request_for_noop_check.updated_input_is_noop(&updated_input) { - return Err(ToolError::UpdatedInput(updated_input)); - } - let decision = ReviewDecision::Approved; - otel.tool_decision( - &tool_ctx.tool_name, - &tool_ctx.call_id, - &decision, - ToolDecisionSource::Config, - ); - return Ok(decision); - } - Some(PermissionRequestDecision::Allow { - updated_input: None, - }) => { + Some(PermissionRequestDecision::Allow) => { let decision = ReviewDecision::Approved; otel.tool_decision( &tool_ctx.tool_name, diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 1735bf8daa..82ee5efd3a 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -5,7 +5,6 @@ use std::time::Instant; use crate::function_tool::FunctionCallError; use crate::goals::GoalRuntimeEvent; -use crate::hook_runtime::MAX_HOOK_INPUT_REWRITES; use crate::hook_runtime::record_additional_contexts; use crate::hook_runtime::run_post_tool_use_hooks; use crate::hook_runtime::run_pre_tool_use_hooks; @@ -229,23 +228,7 @@ where invocation: ToolInvocation, ) -> BoxFuture<'a, Result> { Box::pin(async move { - let mut invocation = invocation; - let mut rewrites = 0; - let output = loop { - match self.handle(invocation.clone()).await { - Err(FunctionCallError::UpdatedInput(updated_input)) => { - rewrites += 1; - if rewrites > MAX_HOOK_INPUT_REWRITES { - return Err(FunctionCallError::RespondToModel( - "hook input rewrite limit exceeded".to_string(), - )); - } - invocation = - ToolHandler::with_updated_hook_input(self, invocation, updated_input)?; - } - result => break result?, - } - }; + let output = self.handle(invocation.clone()).await?; let call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); let post_tool_use_payload = 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 b45c7cb7b1..fef8db5ca9 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -420,21 +420,7 @@ impl CoreShellActionProvider { ) .await { - Some(PermissionRequestDecision::Allow { - updated_input: Some(_), - }) => { - return PromptDecision { - decision: ReviewDecision::Denied, - guardian_review_id: None, - rejection_message: Some( - "updatedInput is not supported for intercepted exec approvals" - .to_string(), - ), - }; - } - Some(PermissionRequestDecision::Allow { - updated_input: None, - }) => { + Some(PermissionRequestDecision::Allow) => { return PromptDecision { decision: ReviewDecision::Approved, guardian_review_id: None, diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 2c139a027c..c17247beb4 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -154,14 +154,6 @@ 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. @@ -358,7 +350,6 @@ pub(crate) struct ToolCtx { #[derive(Debug)] pub(crate) enum ToolError { Rejected(String), - UpdatedInput(serde_json::Value), Codex(CodexErr), } diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index f2fb189137..19eb7a67d9 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -34,30 +34,6 @@ 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 { diff --git a/codex-rs/core/src/unified_exec/errors.rs b/codex-rs/core/src/unified_exec/errors.rs index 501503e9df..0576e7d7cf 100644 --- a/codex-rs/core/src/unified_exec/errors.rs +++ b/codex-rs/core/src/unified_exec/errors.rs @@ -1,5 +1,4 @@ use codex_protocol::exec_output::ExecToolCallOutput; -use serde_json::Value; use thiserror::Error; #[derive(Debug, Error)] @@ -24,8 +23,6 @@ pub(crate) enum UnifiedExecError { message: String, output: ExecToolCallOutput, }, - #[error("tool input rewritten by hook")] - UpdatedInput(Value), } impl UnifiedExecError { diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 9a8bb85c4f..4f85b1ddf7 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -221,9 +221,6 @@ async fn finish_deferred_network_approval_for_session( fn network_approval_error_message(err: ToolError) -> String { match err { ToolError::Rejected(message) => message, - ToolError::UpdatedInput(_) => { - "updatedInput is not supported for deferred network approvals".to_string() - } ToolError::Codex(err) => err.to_string(), } } @@ -1066,9 +1063,6 @@ impl UnifiedExecProcessManager { }; UnifiedExecError::sandbox_denied(message, output) } - ToolError::UpdatedInput(updated_input) => { - UnifiedExecError::UpdatedInput(updated_input) - } other => UnifiedExecError::create_process(format!("{other:?}")), }) } diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index ec16367ba4..1c92918c9d 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -496,56 +496,6 @@ elif mode == "exit_2": Ok(()) } -fn write_updating_permission_request_hook( - home: &Path, - matcher: &str, - updated_input: &Value, -) -> Result<()> { - let script_path = home.join("permission_request_hook.py"); - let log_path = home.join("permission_request_hook_log.jsonl"); - let updated_input_json = - serde_json::to_string(updated_input).context("serialize updated permission input")?; - let script = format!( - r#"import json -from pathlib import Path -import sys - -payload = json.load(sys.stdin) - -with Path(r"{log_path}").open("a", encoding="utf-8") as handle: - handle.write(json.dumps(payload) + "\n") - -print(json.dumps({{ - "hookSpecificOutput": {{ - "hookEventName": "PermissionRequest", - "decision": {{ - "behavior": "allow", - "updatedInput": {updated_input_json} - }} - }} -}})) -"#, - log_path = log_path.display(), - updated_input_json = updated_input_json, - ); - let hooks = serde_json::json!({ - "hooks": { - "PermissionRequest": [{ - "matcher": matcher, - "hooks": [{ - "type": "command", - "command": format!("python3 {}", script_path.display()), - "statusMessage": "rewriting permission request input", - }] - }] - } - }); - - fs::write(&script_path, script).context("write updating permission hook script")?; - fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; - Ok(()) -} - fn install_allow_permission_request_hook(home: &Path) -> Result<()> { write_permission_request_hook( home, @@ -1603,92 +1553,6 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } -#[tokio::test] -async fn permission_request_hook_rewrites_shell_command_before_execution() -> Result<()> { - skip_if_no_network!(Ok(())); - - let server = start_mock_server().await; - let call_id = "permissionrequest-shell-command-rewrite"; - let original_marker = std::env::temp_dir().join("permissionrequest-shell-command-original"); - let rewritten_marker = std::env::temp_dir().join("permissionrequest-shell-command-rewritten"); - let original_command = format!("rm -f {}", original_marker.display()); - let rewritten_command = format!("printf rewritten > {}", rewritten_marker.display()); - let args = serde_json::json!({ "command": original_command }); - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-1"), - core_test_support::responses::ev_function_call( - call_id, - "shell_command", - &serde_json::to_string(&args)?, - ), - ev_completed("resp-1"), - ]), - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "permission hook rewrote it"), - ev_completed("resp-2"), - ]), - ], - ) - .await; - - let updated_input = serde_json::json!({ "command": rewritten_command.clone() }); - let mut builder = test_codex() - .with_pre_build_hook(move |home| { - if let Err(error) = - write_updating_permission_request_hook(home, "^Bash$", &updated_input) - { - panic!("failed to write updating permission hook fixture: {error}"); - } - }) - .with_config(|config| { - trust_discovered_hooks(config); - }); - let test = builder.build(&server).await?; - - if original_marker.exists() { - fs::remove_file(&original_marker).context("remove stale original permission marker")?; - } - if rewritten_marker.exists() { - fs::remove_file(&rewritten_marker).context("remove stale rewritten permission marker")?; - } - fs::write(&original_marker, "seed").context("create original permission marker")?; - - test.submit_turn_with_approval_and_permission_profile( - "run the rewritten shell command after hook approval", - AskForApproval::OnRequest, - PermissionProfile::Disabled, - ) - .await?; - - let requests = responses.requests(); - assert_eq!(requests.len(), 2); - requests[1].function_call_output(call_id); - assert!( - original_marker.exists(), - "original command should not execute after permission rewrite" - ); - assert_eq!( - fs::read_to_string(&rewritten_marker).context("read rewritten permission marker")?, - "rewritten" - ); - - let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?; - assert_eq!(hook_inputs.len(), 1); - assert_permission_request_hook_input( - &hook_inputs[0], - "Bash", - &original_command, - /*description*/ None, - ); - assert!(hook_inputs[0].get("tool_use_id").is_none()); - - Ok(()) -} - #[tokio::test] async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1763,98 +1627,6 @@ async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result Ok(()) } -#[tokio::test] -async fn permission_request_hook_rewrites_apply_patch_before_execution() -> Result<()> { - skip_if_no_network!(Ok(())); - - let server = start_mock_server().await; - let call_id = "permissionrequest-apply-patch-rewrite"; - let original_file = "permission_request_apply_patch_original.txt"; - let rewritten_file = "permission_request_apply_patch_rewritten.txt"; - let original_path = format!("../{original_file}"); - let rewritten_path = format!("../{rewritten_file}"); - let original_patch = format!( - r#"*** Begin Patch -*** Add File: {original_path} -+original -*** End Patch"# - ); - let rewritten_patch = format!( - r#"*** Begin Patch -*** Add File: {rewritten_path} -+rewritten -*** End Patch"# - ); - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-1"), - ev_apply_patch_function_call(call_id, &original_patch), - ev_completed("resp-1"), - ]), - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "permission hook rewrote apply_patch"), - ev_completed("resp-2"), - ]), - ], - ) - .await; - - let updated_input = serde_json::json!({ "command": rewritten_patch.clone() }); - let mut builder = test_codex() - .with_pre_build_hook(move |home| { - if let Err(error) = - write_updating_permission_request_hook(home, "^apply_patch$", &updated_input) - { - panic!("failed to write updating permission hook fixture: {error}"); - } - }) - .with_config(|config| { - config.include_apply_patch_tool = true; - trust_discovered_hooks(config); - }); - let test = builder.build(&server).await?; - - test.submit_turn_with_approval_and_permission_profile( - "apply the rewritten patch after hook approval", - AskForApproval::OnRequest, - restrictive_workspace_write_profile(), - ) - .await?; - - let requests = responses.requests(); - assert_eq!(requests.len(), 2); - requests[1].function_call_output(call_id); - assert!( - !test.workspace_path(&original_path).exists(), - "original patch should not create its target file" - ); - assert_eq!( - fs::read_to_string(test.workspace_path(&rewritten_path)) - .context("read rewritten permission apply_patch file")?, - "rewritten\n" - ); - - let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?; - assert_eq!(hook_inputs.len(), 2); - assert_permission_request_hook_input( - &hook_inputs[0], - "apply_patch", - &original_patch, - /*description*/ None, - ); - assert_permission_request_hook_input( - &hook_inputs[1], - "apply_patch", - &rewritten_patch, - /*description*/ None, - ); - - Ok(()) -} - #[tokio::test] async fn permission_request_hook_sees_raw_exec_command_input() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json index 5b46925568..21d45382b0 100644 --- a/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json +++ b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json @@ -37,7 +37,7 @@ }, "updatedInput": { "default": null, - "description": "Replaces the entire tool input object when `behavior` is `allow`." + "description": "Reserved for a future input-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present." }, "updatedPermissions": { "default": null, diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 53e9b52ab3..9c3d442dac 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -23,12 +23,8 @@ pub(crate) struct PreToolUseOutput { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum PermissionRequestDecision { - Allow { - updated_input: Option, - }, - Deny { - message: String, - }, + Allow, + Deny { message: String }, } #[derive(Debug, Clone)] @@ -319,9 +315,7 @@ fn unsupported_permission_request_hook_specific_output( decision: Option<&PermissionRequestDecisionWire>, ) -> Option { let decision = decision?; - if decision.updated_input.is_some() - && !matches!(decision.behavior, PermissionRequestBehaviorWire::Allow) - { + if decision.updated_input.is_some() { Some("PermissionRequest hook returned unsupported updatedInput".to_string()) } else if decision.updated_permissions.is_some() { Some("PermissionRequest hook returned unsupported updatedPermissions".to_string()) @@ -336,9 +330,7 @@ fn permission_request_decision( decision: &PermissionRequestDecisionWire, ) -> PermissionRequestDecision { match decision.behavior { - PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow { - updated_input: decision.updated_input.clone(), - }, + PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow, PermissionRequestBehaviorWire::Deny => PermissionRequestDecision::Deny { message: decision .message @@ -445,7 +437,7 @@ mod tests { use super::parse_permission_request; #[test] - fn permission_request_accepts_allow_updated_input_field() { + fn permission_request_rejects_reserved_updated_input_field() { let parsed = parse_permission_request( &json!({ "continue": true, @@ -461,31 +453,6 @@ mod tests { ) .expect("permission request hook output should parse"); - assert_eq!( - parsed.decision, - Some(super::PermissionRequestDecision::Allow { - updated_input: Some(json!({})), - }) - ); - } - - #[test] - fn permission_request_rejects_deny_updated_input_field() { - let parsed = parse_permission_request( - &json!({ - "continue": true, - "hookSpecificOutput": { - "hookEventName": "PermissionRequest", - "decision": { - "behavior": "deny", - "updatedInput": {} - } - } - }) - .to_string(), - ) - .expect("permission request hook output should parse"); - assert_eq!( parsed.invalid_reason, Some("PermissionRequest hook returned unsupported updatedInput".to_string()) diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 133461a26f..2cebd0e002 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -47,7 +47,7 @@ pub struct PermissionRequestRequest { #[derive(Debug, Clone, PartialEq, Eq)] pub enum PermissionRequestDecision { - Allow { updated_input: Option }, + Allow, Deny { message: String }, } @@ -154,10 +154,8 @@ fn resolve_permission_request_decision<'a>( let mut resolved_allow = None; for decision in decisions { match decision { - PermissionRequestDecision::Allow { updated_input } => { - resolved_allow = Some(PermissionRequestDecision::Allow { - updated_input: updated_input.clone(), - }); + PermissionRequestDecision::Allow => { + resolved_allow = Some(PermissionRequestDecision::Allow); } PermissionRequestDecision::Deny { message } => { return Some(PermissionRequestDecision::Deny { @@ -221,8 +219,8 @@ fn parse_completed( }); } else if let Some(parsed_decision) = parsed.decision { match parsed_decision { - output_parser::PermissionRequestDecision::Allow { updated_input } => { - decision = Some(PermissionRequestDecision::Allow { updated_input }); + output_parser::PermissionRequestDecision::Allow => { + decision = Some(PermissionRequestDecision::Allow); } output_parser::PermissionRequestDecision::Deny { message } => { status = HookRunStatus::Blocked; @@ -296,9 +294,7 @@ mod tests { #[test] fn permission_request_deny_overrides_earlier_allow() { let decisions = [ - PermissionRequestDecision::Allow { - updated_input: None, - }, + PermissionRequestDecision::Allow, PermissionRequestDecision::Deny { message: "repo deny".to_string(), }, @@ -315,38 +311,13 @@ mod tests { #[test] fn permission_request_returns_allow_when_no_handler_denies() { let decisions = [ - PermissionRequestDecision::Allow { - updated_input: None, - }, - PermissionRequestDecision::Allow { - updated_input: None, - }, + PermissionRequestDecision::Allow, + PermissionRequestDecision::Allow, ]; assert_eq!( resolve_permission_request_decision(decisions.iter()), - Some(PermissionRequestDecision::Allow { - updated_input: None, - }) - ); - } - - #[test] - fn permission_request_keeps_highest_precedence_allow_update() { - let decisions = [ - PermissionRequestDecision::Allow { - updated_input: Some(serde_json::json!({ "command": "first" })), - }, - PermissionRequestDecision::Allow { - updated_input: Some(serde_json::json!({ "command": "second" })), - }, - ]; - - assert_eq!( - resolve_permission_request_decision(decisions.iter()), - Some(PermissionRequestDecision::Allow { - updated_input: Some(serde_json::json!({ "command": "second" })), - }) + Some(PermissionRequestDecision::Allow) ); } diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 7365529415..d08cce6ee2 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -138,7 +138,9 @@ pub(crate) struct PermissionRequestHookSpecificOutputWire { #[serde(deny_unknown_fields)] pub(crate) struct PermissionRequestDecisionWire { pub behavior: PermissionRequestBehaviorWire, - /// Replaces the entire tool input object when `behavior` is `allow`. + /// Reserved for a future input-rewrite capability. + /// + /// PermissionRequest hooks currently fail closed if this field is present. #[serde(default)] pub updated_input: Option, /// Reserved for a future permission-rewrite capability.