From f7d4874a0e9d2a3c9ddcf816325f12cfb2a21110 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Tue, 14 Apr 2026 11:11:28 -0700 Subject: [PATCH] Refactor permission request updates --- codex-rs/core/src/hook_runtime.rs | 99 ++++++++------- codex-rs/core/src/tools/runtimes/shell.rs | 4 +- .../core/src/tools/runtimes/unified_exec.rs | 4 +- codex-rs/core/src/tools/sandboxing.rs | 26 ++-- codex-rs/core/src/tools/sandboxing_tests.rs | 27 ++-- codex-rs/core/tests/suite/hooks.rs | 9 +- ...rmission-request.command.input.schema.json | 90 +++++++++----- ...mission-request.command.output.schema.json | 115 +++++++++--------- codex-rs/hooks/src/engine/output_parser.rs | 73 +++++------ .../hooks/src/events/permission_request.rs | 113 ++++++----------- codex-rs/hooks/src/lib.rs | 10 +- codex-rs/hooks/src/schema.rs | 9 +- 12 files changed, 276 insertions(+), 303 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index d398ecf52b..0330dbd797 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -2,14 +2,12 @@ use std::future::Future; use std::path::Path; use std::sync::Arc; -use codex_hooks::PermissionDirectoryUpdate; use codex_hooks::PermissionRequestDecision; use codex_hooks::PermissionRequestOutcome; use codex_hooks::PermissionRequestRequest; -use codex_hooks::PermissionSuggestion; -use codex_hooks::PermissionSuggestionDestination; -use codex_hooks::PermissionSuggestionRule; -use codex_hooks::PermissionSuggestionType; +use codex_hooks::PermissionUpdate; +use codex_hooks::PermissionUpdateDestination; +use codex_hooks::PermissionUpdateRule; use codex_hooks::PostToolUseOutcome; use codex_hooks::PostToolUseRequest; use codex_hooks::PreToolUseOutcome; @@ -199,16 +197,9 @@ pub(crate) async fn run_permission_request_hooks( if let Some(PermissionRequestDecision::Allow { updated_permissions, - add_directories, }) = &decision { - apply_permission_updates_from_hook( - sess, - turn_context, - updated_permissions, - add_directories, - ) - .await; + apply_permission_updates_from_hook(sess, turn_context, updated_permissions).await; } decision @@ -217,28 +208,15 @@ pub(crate) async fn run_permission_request_hooks( async fn apply_permission_updates_from_hook( sess: &Arc, turn_context: &Arc, - updated_permissions: &[PermissionSuggestion], - add_directories: &[PermissionDirectoryUpdate], + updated_permissions: &[PermissionUpdate], ) { for permission in updated_permissions { if let Err(err) = apply_permission_update_from_hook(sess, turn_context, permission).await { - let message = - format!("PermissionRequest hook failed to apply updated permission: {err}"); - warn!("{message}"); - sess.send_event_raw(Event { - id: turn_context.sub_id.clone(), - msg: EventMsg::Warning(WarningEvent { message }), - }) - .await; - } - } - - for directory_update in add_directories { - if let Err(err) = - apply_directory_update_from_hook(sess, turn_context, directory_update).await - { - let message = - format!("PermissionRequest hook failed to apply updated writable roots: {err}"); + let update_kind = match permission { + PermissionUpdate::AddRules { .. } => "updated permission", + PermissionUpdate::AddDirectories { .. } => "updated writable roots", + }; + let message = format!("PermissionRequest hook failed to apply {update_kind}: {err}"); warn!("{message}"); sess.send_event_raw(Event { id: turn_context.sub_id.clone(), @@ -252,18 +230,20 @@ async fn apply_permission_updates_from_hook( async fn apply_permission_update_from_hook( sess: &Arc, turn_context: &Arc, - permission: &PermissionSuggestion, + permission: &PermissionUpdate, ) -> Result<(), String> { - match permission.suggestion_type { - PermissionSuggestionType::AddRules => { - for rule in &permission.rules { + match permission { + PermissionUpdate::AddRules { + rules, destination, .. + } => { + for rule in rules { match rule { - PermissionSuggestionRule::PrefixRule { command } => { + PermissionUpdateRule::PrefixRule { command } => { let amendment = ExecPolicyAmendment::new(command.clone()); apply_execpolicy_amendment_destination( sess, turn_context, - &permission.destination, + destination, &amendment, ) .await?; @@ -272,23 +252,26 @@ async fn apply_permission_update_from_hook( } Ok(()) } + PermissionUpdate::AddDirectories { .. } => { + apply_directory_update_from_hook(sess, turn_context, permission).await + } } } async fn apply_execpolicy_amendment_destination( sess: &Arc, turn_context: &Arc, - destination: &PermissionSuggestionDestination, + destination: &PermissionUpdateDestination, amendment: &ExecPolicyAmendment, ) -> Result<(), String> { match destination { - PermissionSuggestionDestination::Session => sess + PermissionUpdateDestination::Session => sess .services .exec_policy .add_amendment_to_current_policy(amendment) .await .map_err(|err| format!("failed to cache session prefix rule: {err}")), - PermissionSuggestionDestination::UserSettings => { + PermissionUpdateDestination::UserSettings => { let codex_home = sess.codex_home().await; sess.services .exec_policy @@ -296,7 +279,7 @@ async fn apply_execpolicy_amendment_destination( .await .map_err(|err| format!("failed to persist user prefix rule: {err}")) } - PermissionSuggestionDestination::ProjectSettings => { + PermissionUpdateDestination::ProjectSettings => { let config = turn_context.config.as_ref(); let project_codex_home = resolve_project_root(&config.config_layer_stack, &config.cwd) .await @@ -318,17 +301,26 @@ async fn apply_execpolicy_amendment_destination( async fn apply_directory_update_from_hook( sess: &Arc, turn_context: &Arc, - directory_update: &PermissionDirectoryUpdate, + permission: &PermissionUpdate, ) -> Result<(), String> { - let writable_roots = resolve_directory_update_paths(&turn_context.cwd, directory_update)?; + let writable_roots = resolve_directory_update_paths(&turn_context.cwd, permission)?; - match directory_update.destination { - PermissionSuggestionDestination::Session => {} - PermissionSuggestionDestination::UserSettings => { + match permission { + PermissionUpdate::AddDirectories { + destination: PermissionUpdateDestination::Session, + .. + } => {} + PermissionUpdate::AddDirectories { + destination: PermissionUpdateDestination::UserSettings, + .. + } => { let codex_home = sess.codex_home().await; append_writable_roots_to_config(codex_home.as_path(), &writable_roots).await?; } - PermissionSuggestionDestination::ProjectSettings => { + PermissionUpdate::AddDirectories { + destination: PermissionUpdateDestination::ProjectSettings, + .. + } => { let config = turn_context.config.as_ref(); let project_codex_home = resolve_project_root(&config.config_layer_stack, &config.cwd) .await @@ -340,6 +332,9 @@ async fn apply_directory_update_from_hook( .map_err(|err| format!("failed to create project config directory: {err}"))?; append_writable_roots_to_config(&project_codex_home, &writable_roots).await?; } + PermissionUpdate::AddRules { .. } => { + return Err("writable root update must use type:addDirectories".to_string()); + } } sess.record_granted_session_permissions(permission_profile_for_writable_roots(&writable_roots)) @@ -349,10 +344,12 @@ async fn apply_directory_update_from_hook( fn resolve_directory_update_paths( cwd: &AbsolutePathBuf, - directory_update: &PermissionDirectoryUpdate, + permission: &PermissionUpdate, ) -> Result, String> { - let writable_roots = directory_update - .directories + let PermissionUpdate::AddDirectories { directories, .. } = permission else { + return Err("writable root update must use type:addDirectories".to_string()); + }; + let writable_roots = directories .iter() .map(|directory| { let trimmed = directory.trim(); diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 00de58d3d7..768ae0f483 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -33,7 +33,7 @@ use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::approval_permission_suggestions; use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; -use codex_hooks::PermissionSuggestionDestination; +use codex_hooks::PermissionUpdateDestination; use codex_network_proxy::NetworkProxy; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::models::PermissionProfile; @@ -211,7 +211,7 @@ impl Approvable for ShellRuntime { req.exec_approval_requirement .proposed_execpolicy_amendment(), req.additional_permissions.as_ref(), - &[PermissionSuggestionDestination::UserSettings], + &[PermissionUpdateDestination::UserSettings], ); Some(PermissionRequestPayload { tool_name: "Bash".to_string(), diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 0b936fb40a..4fe7bfe5c0 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -34,7 +34,7 @@ use crate::unified_exec::NoopSpawnLifecycle; use crate::unified_exec::UnifiedExecError; use crate::unified_exec::UnifiedExecProcess; use crate::unified_exec::UnifiedExecProcessManager; -use codex_hooks::PermissionSuggestionDestination; +use codex_hooks::PermissionUpdateDestination; use codex_network_proxy::NetworkProxy; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; @@ -191,7 +191,7 @@ impl Approvable for UnifiedExecRuntime<'_> { req.exec_approval_requirement .proposed_execpolicy_amendment(), req.additional_permissions.as_ref(), - &[PermissionSuggestionDestination::UserSettings], + &[PermissionUpdateDestination::UserSettings], ); Some(PermissionRequestPayload { tool_name: "Bash".to_string(), diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 920c6514fd..dd84f4d08c 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -10,11 +10,10 @@ use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::state::SessionServices; use crate::tools::network_approval::NetworkApprovalSpec; -use codex_hooks::PermissionSuggestion; -use codex_hooks::PermissionSuggestionBehavior; -use codex_hooks::PermissionSuggestionDestination; -use codex_hooks::PermissionSuggestionRule; -use codex_hooks::PermissionSuggestionType; +use codex_hooks::PermissionUpdate; +use codex_hooks::PermissionUpdateBehavior; +use codex_hooks::PermissionUpdateDestination; +use codex_hooks::PermissionUpdateRule; use codex_network_proxy::NetworkProxy; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::approvals::NetworkApprovalContext; @@ -143,25 +142,24 @@ pub(crate) struct PermissionRequestPayload { pub tool_name: String, pub command: String, pub description: Option, - pub permission_suggestions: Vec, + pub permission_suggestions: Vec, } pub(crate) fn exec_policy_permission_suggestions( proposed_execpolicy_amendment: Option<&ExecPolicyAmendment>, - destinations: &[PermissionSuggestionDestination], -) -> Vec { + destinations: &[PermissionUpdateDestination], +) -> Vec { proposed_execpolicy_amendment .into_iter() .flat_map(|amendment| { destinations .iter() .cloned() - .map(move |destination| PermissionSuggestion { - suggestion_type: PermissionSuggestionType::AddRules, - rules: vec![PermissionSuggestionRule::PrefixRule { + .map(move |destination| PermissionUpdate::AddRules { + rules: vec![PermissionUpdateRule::PrefixRule { command: amendment.command.clone(), }], - behavior: PermissionSuggestionBehavior::Allow, + behavior: PermissionUpdateBehavior::Allow, destination, }) }) @@ -172,8 +170,8 @@ pub(crate) fn approval_permission_suggestions( network_approval_context: Option<&NetworkApprovalContext>, proposed_execpolicy_amendment: Option<&ExecPolicyAmendment>, additional_permissions: Option<&PermissionProfile>, - destinations: &[PermissionSuggestionDestination], -) -> Vec { + destinations: &[PermissionUpdateDestination], +) -> Vec { let available_decisions = ExecApprovalRequestEvent::default_available_decisions( network_approval_context, proposed_execpolicy_amendment, diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index 24094bd4c6..25e54d1a78 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -1,9 +1,9 @@ use super::*; use crate::sandboxing::SandboxPermissions; -use codex_hooks::PermissionSuggestion; -use codex_hooks::PermissionSuggestionDestination; -use codex_hooks::PermissionSuggestionRule; -use codex_hooks::PermissionSuggestionType; +use codex_hooks::PermissionUpdate; +use codex_hooks::PermissionUpdateBehavior; +use codex_hooks::PermissionUpdateDestination; +use codex_hooks::PermissionUpdateRule; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::approvals::NetworkApprovalContext; use codex_protocol::approvals::NetworkApprovalProtocol; @@ -129,22 +129,21 @@ fn command_approval_execpolicy_amendment_maps_to_user_settings_suggestion() { "node_modules".to_string(), ])), /*additional_permissions*/ None, - &[PermissionSuggestionDestination::UserSettings], + &[PermissionUpdateDestination::UserSettings], ); assert_eq!( suggestions, - vec![PermissionSuggestion { - suggestion_type: PermissionSuggestionType::AddRules, - rules: vec![PermissionSuggestionRule::PrefixRule { + vec![PermissionUpdate::AddRules { + rules: vec![PermissionUpdateRule::PrefixRule { command: vec![ "rm".to_string(), "-rf".to_string(), "node_modules".to_string(), ], }], - behavior: PermissionSuggestionBehavior::Allow, - destination: PermissionSuggestionDestination::UserSettings, + behavior: PermissionUpdateBehavior::Allow, + destination: PermissionUpdateDestination::UserSettings, }] ); } @@ -167,10 +166,10 @@ fn command_approval_with_additional_permissions_has_no_persistent_suggestions() write: None, }), }), - &[PermissionSuggestionDestination::UserSettings], + &[PermissionUpdateDestination::UserSettings], ); - assert_eq!(suggestions, Vec::::new()); + assert_eq!(suggestions, Vec::::new()); } #[test] @@ -185,8 +184,8 @@ fn network_approval_with_execpolicy_amendment_has_no_persistent_suggestions() { "https://example.com".to_string(), ])), /*additional_permissions*/ None, - &[PermissionSuggestionDestination::UserSettings], + &[PermissionUpdateDestination::UserSettings], ); - assert_eq!(suggestions, Vec::::new()); + assert_eq!(suggestions, Vec::::new()); } diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 14d51364cb..2f43074903 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -332,7 +332,8 @@ elif mode == "allow_add_directories_session": "hookEventName": "PermissionRequest", "decision": {{ "behavior": "allow", - "addDirectories": [{{ + "updatedPermissions": [{{ + "type": "addDirectories", "directories": [reason], "destination": "session" }}] @@ -345,7 +346,8 @@ elif mode == "allow_add_directories_project": "hookEventName": "PermissionRequest", "decision": {{ "behavior": "allow", - "addDirectories": [{{ + "updatedPermissions": [{{ + "type": "addDirectories", "directories": [reason], "destination": "projectSettings" }}] @@ -358,7 +360,8 @@ elif mode == "allow_add_directories_user": "hookEventName": "PermissionRequest", "decision": {{ "behavior": "allow", - "addDirectories": [{{ + "updatedPermissions": [{{ + "type": "addDirectories", "directories": [reason], "destination": "userSettings" }}] diff --git a/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json index 9e0bbce86f..42231c6ee4 100644 --- a/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json +++ b/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json @@ -23,33 +23,65 @@ ], "type": "object" }, - "PermissionSuggestion": { - "properties": { - "behavior": { - "$ref": "#/definitions/PermissionSuggestionBehavior" - }, - "destination": { - "$ref": "#/definitions/PermissionSuggestionDestination" - }, - "rules": { - "items": { - "$ref": "#/definitions/PermissionSuggestionRule" + "PermissionUpdate": { + "oneOf": [ + { + "properties": { + "behavior": { + "$ref": "#/definitions/PermissionUpdateBehavior" + }, + "destination": { + "$ref": "#/definitions/PermissionUpdateDestination" + }, + "rules": { + "items": { + "$ref": "#/definitions/PermissionUpdateRule" + }, + "type": "array" + }, + "type": { + "enum": [ + "addRules" + ], + "type": "string" + } }, - "type": "array" + "required": [ + "behavior", + "destination", + "rules", + "type" + ], + "type": "object" }, - "type": { - "$ref": "#/definitions/PermissionSuggestionType" + { + "properties": { + "destination": { + "$ref": "#/definitions/PermissionUpdateDestination" + }, + "directories": { + "items": { + "type": "string" + }, + "type": "array" + }, + "type": { + "enum": [ + "addDirectories" + ], + "type": "string" + } + }, + "required": [ + "destination", + "directories", + "type" + ], + "type": "object" } - }, - "required": [ - "behavior", - "destination", - "rules", - "type" - ], - "type": "object" + ] }, - "PermissionSuggestionBehavior": { + "PermissionUpdateBehavior": { "enum": [ "allow", "deny", @@ -57,7 +89,7 @@ ], "type": "string" }, - "PermissionSuggestionDestination": { + "PermissionUpdateDestination": { "enum": [ "session", "projectSettings", @@ -65,7 +97,7 @@ ], "type": "string" }, - "PermissionSuggestionRule": { + "PermissionUpdateRule": { "oneOf": [ { "properties": { @@ -89,12 +121,6 @@ "type": "object" } ] - }, - "PermissionSuggestionType": { - "enum": [ - "addRules" - ], - "type": "string" } }, "properties": { @@ -120,7 +146,7 @@ }, "permission_suggestions": { "items": { - "$ref": "#/definitions/PermissionSuggestion" + "$ref": "#/definitions/PermissionUpdate" }, "type": "array" }, 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 3156c4a229..fb2d81ce52 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 @@ -13,24 +13,6 @@ ], "type": "string" }, - "PermissionDirectoryUpdate": { - "properties": { - "destination": { - "$ref": "#/definitions/PermissionSuggestionDestination" - }, - "directories": { - "items": { - "type": "string" - }, - "type": "array" - } - }, - "required": [ - "destination", - "directories" - ], - "type": "object" - }, "PermissionRequestBehaviorWire": { "enum": [ "allow", @@ -41,13 +23,6 @@ "PermissionRequestDecisionWire": { "additionalProperties": false, "properties": { - "addDirectories": { - "default": null, - "items": { - "$ref": "#/definitions/PermissionDirectoryUpdate" - }, - "type": "array" - }, "behavior": { "$ref": "#/definitions/PermissionRequestBehaviorWire" }, @@ -67,7 +42,7 @@ "updatedPermissions": { "default": null, "items": { - "$ref": "#/definitions/PermissionSuggestion" + "$ref": "#/definitions/PermissionUpdate" }, "type": "array" } @@ -97,33 +72,65 @@ ], "type": "object" }, - "PermissionSuggestion": { - "properties": { - "behavior": { - "$ref": "#/definitions/PermissionSuggestionBehavior" - }, - "destination": { - "$ref": "#/definitions/PermissionSuggestionDestination" - }, - "rules": { - "items": { - "$ref": "#/definitions/PermissionSuggestionRule" + "PermissionUpdate": { + "oneOf": [ + { + "properties": { + "behavior": { + "$ref": "#/definitions/PermissionUpdateBehavior" + }, + "destination": { + "$ref": "#/definitions/PermissionUpdateDestination" + }, + "rules": { + "items": { + "$ref": "#/definitions/PermissionUpdateRule" + }, + "type": "array" + }, + "type": { + "enum": [ + "addRules" + ], + "type": "string" + } }, - "type": "array" + "required": [ + "behavior", + "destination", + "rules", + "type" + ], + "type": "object" }, - "type": { - "$ref": "#/definitions/PermissionSuggestionType" + { + "properties": { + "destination": { + "$ref": "#/definitions/PermissionUpdateDestination" + }, + "directories": { + "items": { + "type": "string" + }, + "type": "array" + }, + "type": { + "enum": [ + "addDirectories" + ], + "type": "string" + } + }, + "required": [ + "destination", + "directories", + "type" + ], + "type": "object" } - }, - "required": [ - "behavior", - "destination", - "rules", - "type" - ], - "type": "object" + ] }, - "PermissionSuggestionBehavior": { + "PermissionUpdateBehavior": { "enum": [ "allow", "deny", @@ -131,7 +138,7 @@ ], "type": "string" }, - "PermissionSuggestionDestination": { + "PermissionUpdateDestination": { "enum": [ "session", "projectSettings", @@ -139,7 +146,7 @@ ], "type": "string" }, - "PermissionSuggestionRule": { + "PermissionUpdateRule": { "oneOf": [ { "properties": { @@ -163,12 +170,6 @@ "type": "object" } ] - }, - "PermissionSuggestionType": { - "enum": [ - "addRules" - ], - "type": "string" } }, "properties": { diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 6f280d5697..bcad73da28 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -22,8 +22,7 @@ pub(crate) struct PreToolUseOutput { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum PermissionRequestDecision { Allow { - updated_permissions: Vec, - add_directories: Vec, + updated_permissions: Vec, }, Deny { message: String, @@ -306,12 +305,9 @@ fn unsupported_permission_request_hook_specific_output( if decision.updated_input.is_some() { Some("PermissionRequest hook returned unsupported updatedInput".to_string()) } else if matches!(decision.behavior, PermissionRequestBehaviorWire::Deny) - && (decision.updated_permissions.is_some() || decision.add_directories.is_some()) + && decision.updated_permissions.is_some() { - Some( - "PermissionRequest hook returned updatedPermissions/addDirectories for deny decision" - .to_string(), - ) + Some("PermissionRequest hook returned updatedPermissions for deny decision".to_string()) } else if decision.interrupt { Some("PermissionRequest hook returned unsupported interrupt:true".to_string()) } else { @@ -325,7 +321,6 @@ fn permission_request_decision( match decision.behavior { PermissionRequestBehaviorWire::Allow => PermissionRequestDecision::Allow { updated_permissions: decision.updated_permissions.clone().unwrap_or_default(), - add_directories: decision.add_directories.clone().unwrap_or_default(), }, PermissionRequestBehaviorWire::Deny => PermissionRequestDecision::Deny { message: decision @@ -488,19 +483,16 @@ mod tests { assert_eq!( parsed.decision, Some(PermissionRequestDecision::Allow { - updated_permissions: vec![crate::events::permission_request::PermissionSuggestion { - suggestion_type: - crate::events::permission_request::PermissionSuggestionType::AddRules, + updated_permissions: vec![crate::events::permission_request::PermissionUpdate::AddRules { rules: vec![ - crate::events::permission_request::PermissionSuggestionRule::PrefixRule { + crate::events::permission_request::PermissionUpdateRule::PrefixRule { command: vec!["rm".to_string(), "-f".to_string()], }, ], behavior: - crate::events::permission_request::PermissionSuggestionBehavior::Allow, - destination: crate::events::permission_request::PermissionSuggestionDestination::UserSettings, + crate::events::permission_request::PermissionUpdateBehavior::Allow, + destination: crate::events::permission_request::PermissionUpdateDestination::UserSettings, }], - add_directories: vec![], }) ); } @@ -510,15 +502,16 @@ mod tests { let parsed = parse_permission_request( &json!({ "continue": true, - "hookSpecificOutput": { - "hookEventName": "PermissionRequest", - "decision": { - "behavior": "allow", - "addDirectories": [{ - "directories": ["./logs", "/tmp/output"], - "destination": "session" - }] - } + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "allow", + "updatedPermissions": [{ + "type": "addDirectories", + "directories": ["./logs", "/tmp/output"], + "destination": "session" + }] + } } }) .to_string(), @@ -528,12 +521,11 @@ mod tests { assert_eq!( parsed.decision, Some(PermissionRequestDecision::Allow { - updated_permissions: vec![], - add_directories: vec![ - crate::events::permission_request::PermissionDirectoryUpdate { + updated_permissions: vec![ + crate::events::permission_request::PermissionUpdate::AddDirectories { directories: vec!["./logs".to_string(), "/tmp/output".to_string()], destination: - crate::events::permission_request::PermissionSuggestionDestination::Session, + crate::events::permission_request::PermissionUpdateDestination::Session, }, ], }) @@ -568,8 +560,7 @@ mod tests { assert_eq!( parsed.invalid_reason, Some( - "PermissionRequest hook returned updatedPermissions/addDirectories for deny decision" - .to_string() + "PermissionRequest hook returned updatedPermissions for deny decision".to_string() ) ); } @@ -579,15 +570,16 @@ mod tests { let parsed = parse_permission_request( &json!({ "continue": true, - "hookSpecificOutput": { - "hookEventName": "PermissionRequest", - "decision": { - "behavior": "deny", - "addDirectories": [{ - "directories": ["./logs"], - "destination": "session" - }] - } + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "deny", + "updatedPermissions": [{ + "type": "addDirectories", + "directories": ["./logs"], + "destination": "session" + }] + } } }) .to_string(), @@ -597,8 +589,7 @@ mod tests { assert_eq!( parsed.invalid_reason, Some( - "PermissionRequest hook returned updatedPermissions/addDirectories for deny decision" - .to_string() + "PermissionRequest hook returned updatedPermissions for deny decision".to_string() ) ); } diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index bc5f97bda7..96d4996879 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -36,38 +36,29 @@ use serde::Deserialize; use serde::Serialize; #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] -#[serde(rename_all = "camelCase")] -pub struct PermissionSuggestion { - #[serde(rename = "type")] - pub suggestion_type: PermissionSuggestionType, - pub rules: Vec, - pub behavior: PermissionSuggestionBehavior, - pub destination: PermissionSuggestionDestination, -} - -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] -#[serde(rename_all = "camelCase")] -pub struct PermissionDirectoryUpdate { - pub directories: Vec, - pub destination: PermissionSuggestionDestination, -} - -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] -#[serde(rename_all = "camelCase")] -pub enum PermissionSuggestionType { - AddRules, +#[serde(tag = "type", rename_all = "camelCase")] +pub enum PermissionUpdate { + AddRules { + rules: Vec, + behavior: PermissionUpdateBehavior, + destination: PermissionUpdateDestination, + }, + AddDirectories { + directories: Vec, + destination: PermissionUpdateDestination, + }, } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] #[serde(rename_all = "lowercase")] -pub enum PermissionSuggestionBehavior { +pub enum PermissionUpdateBehavior { Allow, Deny, Ask, } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] -pub enum PermissionSuggestionDestination { +pub enum PermissionUpdateDestination { #[serde(rename = "session")] Session, #[serde(rename = "projectSettings")] @@ -78,7 +69,7 @@ pub enum PermissionSuggestionDestination { #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] #[serde(tag = "type", rename_all = "camelCase")] -pub enum PermissionSuggestionRule { +pub enum PermissionUpdateRule { PrefixRule { command: Vec }, } @@ -94,14 +85,13 @@ pub struct PermissionRequestRequest { pub run_id_suffix: String, pub command: String, pub description: Option, - pub permission_suggestions: Vec, + pub permission_suggestions: Vec, } #[derive(Debug, Clone, PartialEq, Eq)] pub enum PermissionRequestDecision { Allow { - updated_permissions: Vec, - add_directories: Vec, + updated_permissions: Vec, }, Deny { message: String, @@ -222,12 +212,10 @@ fn resolve_permission_request_decision<'a>( ) -> Option { let mut saw_allow = false; let mut updated_permissions = Vec::new(); - let mut add_directories = Vec::new(); for decision in decisions { match decision { PermissionRequestDecision::Allow { updated_permissions: selected_permissions, - add_directories: selected_directories, } => { saw_allow = true; for permission in selected_permissions { @@ -235,11 +223,6 @@ fn resolve_permission_request_decision<'a>( updated_permissions.push(permission.clone()); } } - for directory_update in selected_directories { - if !add_directories.contains(directory_update) { - add_directories.push(directory_update.clone()); - } - } } PermissionRequestDecision::Deny { message } => { return Some(PermissionRequestDecision::Deny { @@ -250,17 +233,15 @@ fn resolve_permission_request_decision<'a>( } saw_allow.then_some(PermissionRequestDecision::Allow { updated_permissions, - add_directories, }) } fn invalid_permission_updates( decision: Option<&PermissionRequestDecision>, - offered_permissions: &[PermissionSuggestion], + offered_permissions: &[PermissionUpdate], ) -> Option { let PermissionRequestDecision::Allow { updated_permissions, - add_directories: _, } = decision? else { return None; @@ -334,11 +315,9 @@ fn parse_completed( match parsed_decision { output_parser::PermissionRequestDecision::Allow { updated_permissions, - add_directories, } => { decision = Some(PermissionRequestDecision::Allow { updated_permissions, - add_directories, }); } output_parser::PermissionRequestDecision::Deny { message } => { @@ -407,13 +386,11 @@ fn parse_completed( mod tests { use pretty_assertions::assert_eq; - use super::PermissionDirectoryUpdate; use super::PermissionRequestDecision; - use super::PermissionSuggestion; - use super::PermissionSuggestionBehavior; - use super::PermissionSuggestionDestination; - use super::PermissionSuggestionRule; - use super::PermissionSuggestionType; + use super::PermissionUpdate; + use super::PermissionUpdateBehavior; + use super::PermissionUpdateDestination; + use super::PermissionUpdateRule; use super::invalid_permission_updates; use super::resolve_permission_request_decision; @@ -422,7 +399,6 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![], - add_directories: vec![], }, PermissionRequestDecision::Deny { message: "repo deny".to_string(), @@ -442,11 +418,9 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![], - add_directories: vec![], }, PermissionRequestDecision::Allow { updated_permissions: vec![], - add_directories: vec![], }, ]; @@ -454,7 +428,6 @@ mod tests { resolve_permission_request_decision(decisions.iter()), Some(PermissionRequestDecision::Allow { updated_permissions: vec![], - add_directories: vec![], }) ); } @@ -468,22 +441,19 @@ mod tests { #[test] fn permission_request_accumulates_distinct_updated_permissions() { - let permission = PermissionSuggestion { - suggestion_type: PermissionSuggestionType::AddRules, - rules: vec![PermissionSuggestionRule::PrefixRule { + let permission = PermissionUpdate::AddRules { + rules: vec![PermissionUpdateRule::PrefixRule { command: vec!["rm".to_string(), "-f".to_string()], }], - behavior: PermissionSuggestionBehavior::Allow, - destination: PermissionSuggestionDestination::UserSettings, + behavior: PermissionUpdateBehavior::Allow, + destination: PermissionUpdateDestination::UserSettings, }; let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![permission.clone()], - add_directories: vec![], }, PermissionRequestDecision::Allow { updated_permissions: vec![permission.clone()], - add_directories: vec![], }, ]; @@ -491,57 +461,50 @@ mod tests { resolve_permission_request_decision(decisions.iter()), Some(PermissionRequestDecision::Allow { updated_permissions: vec![permission], - add_directories: vec![], }) ); } #[test] fn permission_request_accumulates_distinct_directory_updates() { - let directory_update = PermissionDirectoryUpdate { + let directory_update = PermissionUpdate::AddDirectories { directories: vec!["./logs".to_string(), "/tmp/output".to_string()], - destination: PermissionSuggestionDestination::Session, + destination: PermissionUpdateDestination::Session, }; let decisions = [ PermissionRequestDecision::Allow { - updated_permissions: vec![], - add_directories: vec![directory_update.clone()], + updated_permissions: vec![directory_update.clone()], }, PermissionRequestDecision::Allow { - updated_permissions: vec![], - add_directories: vec![directory_update.clone()], + updated_permissions: vec![directory_update.clone()], }, ]; assert_eq!( resolve_permission_request_decision(decisions.iter()), Some(PermissionRequestDecision::Allow { - updated_permissions: vec![], - add_directories: vec![directory_update], + updated_permissions: vec![directory_update], }) ); } #[test] fn permission_request_rejects_unoffered_updated_permissions() { - let offered = vec![PermissionSuggestion { - suggestion_type: PermissionSuggestionType::AddRules, - rules: vec![PermissionSuggestionRule::PrefixRule { + let offered = vec![PermissionUpdate::AddRules { + rules: vec![PermissionUpdateRule::PrefixRule { command: vec!["rm".to_string()], }], - behavior: PermissionSuggestionBehavior::Allow, - destination: PermissionSuggestionDestination::UserSettings, + behavior: PermissionUpdateBehavior::Allow, + destination: PermissionUpdateDestination::UserSettings, }]; let selected = PermissionRequestDecision::Allow { - updated_permissions: vec![PermissionSuggestion { - suggestion_type: PermissionSuggestionType::AddRules, - rules: vec![PermissionSuggestionRule::PrefixRule { + updated_permissions: vec![PermissionUpdate::AddRules { + rules: vec![PermissionUpdateRule::PrefixRule { command: vec!["curl".to_string()], }], - behavior: PermissionSuggestionBehavior::Allow, - destination: PermissionSuggestionDestination::UserSettings, + behavior: PermissionUpdateBehavior::Allow, + destination: PermissionUpdateDestination::UserSettings, }], - add_directories: vec![], }; assert_eq!( diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 73db6f8b85..30cbe2b97f 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -5,15 +5,13 @@ mod registry; mod schema; mod types; -pub use events::permission_request::PermissionDirectoryUpdate; pub use events::permission_request::PermissionRequestDecision; pub use events::permission_request::PermissionRequestOutcome; pub use events::permission_request::PermissionRequestRequest; -pub use events::permission_request::PermissionSuggestion; -pub use events::permission_request::PermissionSuggestionBehavior; -pub use events::permission_request::PermissionSuggestionDestination; -pub use events::permission_request::PermissionSuggestionRule; -pub use events::permission_request::PermissionSuggestionType; +pub use events::permission_request::PermissionUpdate; +pub use events::permission_request::PermissionUpdateBehavior; +pub use events::permission_request::PermissionUpdateDestination; +pub use events::permission_request::PermissionUpdateRule; pub use events::post_tool_use::PostToolUseOutcome; pub use events::post_tool_use::PostToolUseRequest; pub use events::pre_tool_use::PreToolUseOutcome; diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 7015c87fdf..7b27a14ac0 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -12,8 +12,7 @@ use serde_json::Value; use std::path::Path; use std::path::PathBuf; -use crate::events::permission_request::PermissionDirectoryUpdate; -use crate::events::permission_request::PermissionSuggestion; +use crate::events::permission_request::PermissionUpdate; const GENERATED_DIR: &str = "generated"; const POST_TOOL_USE_INPUT_FIXTURE: &str = "post-tool-use.command.input.schema.json"; @@ -147,9 +146,7 @@ pub(crate) struct PermissionRequestDecisionWire { #[serde(default)] pub updated_input: Option, #[serde(default)] - pub updated_permissions: Option>, - #[serde(default)] - pub add_directories: Option>, + pub updated_permissions: Option>, #[serde(default)] pub message: Option, /// Reserved for future short-circuiting semantics. @@ -266,7 +263,7 @@ pub(crate) struct PermissionRequestCommandInput { pub tool_name: String, pub tool_input: PermissionRequestToolInput, #[serde(skip_serializing_if = "Option::is_none")] - pub permission_suggestions: Option>, + pub permission_suggestions: Option>, } #[derive(Debug, Clone, Serialize, JsonSchema)]