From 2219e3df2e711ebe9f2ea7837f39b8c08aca55d3 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Mon, 13 Apr 2026 22:20:33 -0700 Subject: [PATCH] Support addDirectories in PermissionRequest hooks Add hook schema and runtime support for decision.addDirectories, persist writable roots by destination, and reuse granted roots in later shell approvals. Co-authored-by: Codex --- codex-rs/core/src/codex.rs | 5 + codex-rs/core/src/config/edit.rs | 57 +++++++ codex-rs/core/src/config/edit_tests.rs | 49 ++++++ codex-rs/core/src/hook_runtime.rs | 122 +++++++++++++- codex-rs/core/src/tools/handlers/shell.rs | 12 +- .../core/src/unified_exec/process_manager.rs | 12 +- codex-rs/core/tests/suite/hooks.rs | 158 ++++++++++++++++++ ...mission-request.command.output.schema.json | 25 +++ codex-rs/hooks/src/engine/output_parser.rs | 77 ++++++++- .../hooks/src/events/permission_request.rs | 54 ++++++ codex-rs/hooks/src/lib.rs | 1 + codex-rs/hooks/src/schema.rs | 3 + 12 files changed, 569 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f44a79528f..4c839da51f 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -3490,6 +3490,11 @@ impl Session { state.granted_permissions() } + pub(crate) async fn record_granted_session_permissions(&self, permissions: PermissionProfile) { + let mut state = self.state.lock().await; + state.record_granted_permissions(permissions); + } + pub async fn notify_dynamic_tool_response(&self, call_id: &str, response: DynamicToolResponse) { let entry = { let mut active = self.active_turn.lock().await; diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 0f1189b22d..d9a50d46b3 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -51,6 +51,8 @@ pub enum ConfigEdit { SetSkillConfig { path: PathBuf, enabled: bool }, /// Set or clear a skill config entry under `[[skills.config]]` by name. SetSkillConfigByName { name: String, enabled: bool }, + /// Append normalized writable roots under `[sandbox_workspace_write]`. + AppendSandboxWorkspaceWriteRoots { roots: Vec }, /// Set trust_level under `[projects.""]`, /// migrating inline tables to explicit tables. SetProjectTrustLevel { path: PathBuf, level: TrustLevel }, @@ -425,6 +427,9 @@ impl ConfigDocument { ConfigEdit::SetSkillConfigByName { name, enabled } => { Ok(self.set_skill_config(SkillConfigSelector::Name(name.clone()), *enabled)) } + ConfigEdit::AppendSandboxWorkspaceWriteRoots { roots } => { + Ok(self.append_sandbox_workspace_write_roots(roots)) + } ConfigEdit::SetPath { segments, value } => Ok(self.insert(segments, value.clone())), ConfigEdit::ClearPath { segments } => Ok(self.clear_owned(segments)), ConfigEdit::SetProjectTrustLevel { path, level } => { @@ -625,6 +630,58 @@ impl ConfigDocument { mutated } + fn append_sandbox_workspace_write_roots(&mut self, roots: &[PathBuf]) -> bool { + if roots.is_empty() { + return false; + } + + let Some(table) = self.descend( + &["sandbox_workspace_write".to_string()], + TraversalMode::Create, + ) else { + return false; + }; + + let mut created_writable_roots = false; + let writable_roots = table.entry("writable_roots").or_insert_with(|| { + created_writable_roots = true; + let array = roots + .iter() + .map(|root| root.to_string_lossy().to_string()) + .collect::(); + TomlItem::Value(array.into()) + }); + + let Some(array) = writable_roots + .as_value_mut() + .and_then(toml_edit::Value::as_array_mut) + else { + let replacement = roots + .iter() + .map(|root| root.to_string_lossy().to_string()) + .collect::(); + let mut replacement = TomlItem::Value(replacement.into()); + Self::preserve_decor(writable_roots, &mut replacement); + *writable_roots = replacement; + return true; + }; + + let mut mutated = created_writable_roots; + for root in roots { + let root = root.to_string_lossy().to_string(); + if !array + .iter() + .filter_map(toml_edit::Value::as_str) + .any(|entry| entry == root) + { + array.push(root); + mutated = true; + } + } + + mutated + } + fn scoped_segments(&self, scope: Scope, segments: &[&str]) -> Vec { let resolved: Vec = segments .iter() diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index af1251b34f..1f79a6db61 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -340,6 +340,55 @@ model_reasoning_effort = "high" assert_eq!(contents, expected); } +#[test] +fn append_sandbox_workspace_write_roots_preserves_existing_keys() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + let original = r#"[sandbox_workspace_write] +network_access = false +writable_roots = ["/tmp/existing"] +"#; + std::fs::write(codex_home.join(CONFIG_TOML_FILE), original).expect("seed config"); + + apply_blocking( + codex_home, + /*profile*/ None, + &[ConfigEdit::AppendSandboxWorkspaceWriteRoots { + roots: vec![ + PathBuf::from("/tmp/existing"), + PathBuf::from("/tmp/new-root"), + ], + }], + ) + .expect("persist"); + + let contents = std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config"); + let expected = r#"[sandbox_workspace_write] +network_access = false +writable_roots = ["/tmp/existing", "/tmp/new-root"]"#; + assert_eq!(contents.trim_end(), expected); +} + +#[test] +fn append_sandbox_workspace_write_roots_creates_config_file() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + + apply_blocking( + codex_home, + /*profile*/ None, + &[ConfigEdit::AppendSandboxWorkspaceWriteRoots { + roots: vec![PathBuf::from("/tmp/new-root")], + }], + ) + .expect("persist"); + + let contents = std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config"); + let expected = r#"[sandbox_workspace_write] +writable_roots = ["/tmp/new-root"]"#; + assert_eq!(contents.trim_end(), expected); +} + #[test] fn blocking_set_model_scopes_to_active_profile() { let tmp = tempdir().expect("tmpdir"); diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 0a788e6969..d398ecf52b 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -1,6 +1,8 @@ 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; @@ -18,6 +20,8 @@ use codex_hooks::UserPromptSubmitRequest; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::items::TurnItem; use codex_protocol::models::DeveloperInstructions; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::AskForApproval; @@ -33,9 +37,13 @@ use tracing::warn; use crate::codex::Session; use crate::codex::TurnContext; +use crate::config::edit::ConfigEdit; +use crate::config::edit::ConfigEditsBuilder; use crate::config_loader::resolve_project_root; use crate::event_mapping::parse_turn_item; use crate::tools::sandboxing::PermissionRequestPayload; +use codex_sandboxing::policy_transforms::normalize_additional_permissions; +use codex_utils_absolute_path::AbsolutePathBuf; pub(crate) struct HookRuntimeOutcome { pub should_stop: bool, @@ -191,9 +199,16 @@ 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).await; + apply_permission_updates_from_hook( + sess, + turn_context, + updated_permissions, + add_directories, + ) + .await; } decision @@ -203,6 +218,7 @@ async fn apply_permission_updates_from_hook( sess: &Arc, turn_context: &Arc, updated_permissions: &[PermissionSuggestion], + add_directories: &[PermissionDirectoryUpdate], ) { for permission in updated_permissions { if let Err(err) = apply_permission_update_from_hook(sess, turn_context, permission).await { @@ -216,6 +232,21 @@ async fn apply_permission_updates_from_hook( .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}"); + warn!("{message}"); + sess.send_event_raw(Event { + id: turn_context.sub_id.clone(), + msg: EventMsg::Warning(WarningEvent { message }), + }) + .await; + } + } } async fn apply_permission_update_from_hook( @@ -284,6 +315,95 @@ async fn apply_execpolicy_amendment_destination( } } +async fn apply_directory_update_from_hook( + sess: &Arc, + turn_context: &Arc, + directory_update: &PermissionDirectoryUpdate, +) -> Result<(), String> { + let writable_roots = resolve_directory_update_paths(&turn_context.cwd, directory_update)?; + + match directory_update.destination { + PermissionSuggestionDestination::Session => {} + PermissionSuggestionDestination::UserSettings => { + let codex_home = sess.codex_home().await; + append_writable_roots_to_config(codex_home.as_path(), &writable_roots).await?; + } + PermissionSuggestionDestination::ProjectSettings => { + let config = turn_context.config.as_ref(); + let project_codex_home = resolve_project_root(&config.config_layer_stack, &config.cwd) + .await + .map_err(|err| format!("failed to resolve project root for writable roots: {err}"))? + .join(".codex") + .to_path_buf(); + tokio::fs::create_dir_all(&project_codex_home) + .await + .map_err(|err| format!("failed to create project config directory: {err}"))?; + append_writable_roots_to_config(&project_codex_home, &writable_roots).await?; + } + } + + sess.record_granted_session_permissions(permission_profile_for_writable_roots(&writable_roots)) + .await; + Ok(()) +} + +fn resolve_directory_update_paths( + cwd: &AbsolutePathBuf, + directory_update: &PermissionDirectoryUpdate, +) -> Result, String> { + let writable_roots = directory_update + .directories + .iter() + .map(|directory| { + let trimmed = directory.trim(); + if trimmed.is_empty() { + return Err("writable root update contained an empty directory".to_string()); + } + + let resolved = if Path::new(trimmed).is_absolute() { + Path::new(trimmed).to_path_buf() + } else { + cwd.join(trimmed).to_path_buf() + }; + + AbsolutePathBuf::from_absolute_path(resolved.as_path()) + .map_err(|err| format!("invalid writable root `{trimmed}`: {err}")) + }) + .collect::, _>>()?; + + normalize_additional_permissions(permission_profile_for_writable_roots(&writable_roots)) + .map_err(|err| format!("failed to normalize writable roots: {err}"))? + .file_system + .and_then(|file_system| file_system.write) + .filter(|write_paths| !write_paths.is_empty()) + .ok_or_else(|| "writable root update did not include any directories".to_string()) +} + +fn permission_profile_for_writable_roots(writable_roots: &[AbsolutePathBuf]) -> PermissionProfile { + PermissionProfile { + file_system: Some(FileSystemPermissions { + read: None, + write: Some(writable_roots.to_vec()), + }), + ..Default::default() + } +} + +async fn append_writable_roots_to_config( + codex_home: &Path, + writable_roots: &[AbsolutePathBuf], +) -> Result<(), String> { + ConfigEditsBuilder::new(codex_home) + .with_edits([ConfigEdit::AppendSandboxWorkspaceWriteRoots { + roots: writable_roots + .iter() + .map(codex_utils_absolute_path::AbsolutePathBuf::to_path_buf) + .collect(), + }]) + .apply() + .await + .map_err(|err| format!("failed to persist writable roots: {err}")) +} pub(crate) async fn run_post_tool_use_hooks( sess: &Arc, turn_context: &Arc, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 3465f32583..2ba68a554c 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -37,6 +37,7 @@ use crate::tools::sandboxing::ToolCtx; use codex_features::Feature; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::ExecCommandSource; +use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ShellCommandBackendConfig; @@ -501,6 +502,15 @@ impl ShellHandler { ); emitter.begin(event_ctx).await; + let exec_approval_file_system_sandbox_policy = + if effective_additional_permissions.permissions_preapproved { + effective_file_system_sandbox_policy( + &turn.file_system_sandbox_policy, + normalized_additional_permissions.as_ref(), + ) + } else { + turn.file_system_sandbox_policy.clone() + }; let exec_approval_requirement = session .services .exec_policy @@ -508,7 +518,7 @@ impl ShellHandler { command: &exec_params.command, approval_policy: turn.approval_policy.value(), sandbox_policy: turn.sandbox_policy.get(), - file_system_sandbox_policy: &turn.file_system_sandbox_policy, + file_system_sandbox_policy: &exec_approval_file_system_sandbox_policy, sandbox_permissions: if effective_additional_permissions.permissions_preapproved { codex_protocol::models::SandboxPermissions::UseDefault } else { diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index b3705ca07e..9034ff24db 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -48,6 +48,7 @@ use crate::unified_exec::process::OutputHandles; use crate::unified_exec::process::SpawnLifecycleHandle; use crate::unified_exec::process::UnifiedExecProcess; use codex_protocol::protocol::ExecCommandSource; +use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::approx_token_count; @@ -658,6 +659,15 @@ impl UnifiedExecProcessManager { self, context.turn.tools_config.unified_exec_shell_mode.clone(), ); + let exec_approval_file_system_sandbox_policy = if request.additional_permissions_preapproved + { + effective_file_system_sandbox_policy( + &context.turn.file_system_sandbox_policy, + request.additional_permissions.as_ref(), + ) + } else { + context.turn.file_system_sandbox_policy.clone() + }; let exec_approval_requirement = context .session .services @@ -666,7 +676,7 @@ impl UnifiedExecProcessManager { command: &request.command, approval_policy: context.turn.approval_policy.value(), sandbox_policy: context.turn.sandbox_policy.get(), - file_system_sandbox_policy: &context.turn.file_system_sandbox_policy, + file_system_sandbox_policy: &exec_approval_file_system_sandbox_policy, sandbox_permissions: if request.additional_permissions_preapproved { crate::sandboxing::SandboxPermissions::UseDefault } else { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index fa0c63bca6..14d51364cb 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -326,6 +326,45 @@ elif mode == "allow_selected_user": }} }} }})) +elif mode == "allow_add_directories_session": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PermissionRequest", + "decision": {{ + "behavior": "allow", + "addDirectories": [{{ + "directories": [reason], + "destination": "session" + }}] + }} + }} + }})) +elif mode == "allow_add_directories_project": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PermissionRequest", + "decision": {{ + "behavior": "allow", + "addDirectories": [{{ + "directories": [reason], + "destination": "projectSettings" + }}] + }} + }} + }})) +elif mode == "allow_add_directories_user": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PermissionRequest", + "decision": {{ + "behavior": "allow", + "addDirectories": [{{ + "directories": [reason], + "destination": "userSettings" + }}] + }} + }} + }})) elif mode == "allow_unoffered_permission": print(json.dumps({{ "hookSpecificOutput": {{ @@ -1530,6 +1569,125 @@ async fn permission_request_hook_persists_selected_user_exec_rule() -> Result<() Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_persists_user_writable_root_and_reuses_it() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id_first = "permissionrequest-user-dir-first"; + let call_id_second = "permissionrequest-user-dir-second"; + let requested_dir = std::env::temp_dir().join("permissionrequest-user-dir"); + fs::create_dir_all(&requested_dir)?; + let first_file = requested_dir.join("first.txt"); + let second_file = requested_dir.join("second.txt"); + let first_command = format!("printf first > {}", first_file.display()); + let second_command = format!("printf second > {}", second_file.display()); + let first_args = serde_json::json!({ + "command": first_command, + "sandbox_permissions": "require_escalated", + }); + let second_args = serde_json::json!({ + "command": second_command, + }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + call_id_first, + "shell_command", + &serde_json::to_string(&first_args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "persisted writable root"), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_function_call( + call_id_second, + "shell_command", + &serde_json::to_string(&second_args)?, + ), + ev_completed("resp-3"), + ]), + sse(vec![ + ev_response_created("resp-4"), + ev_assistant_message("msg-2", "reused writable root"), + ev_completed("resp-4"), + ]), + ], + ) + .await; + + let requested_dir_string = requested_dir.display().to_string(); + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + if let Err(error) = write_permission_request_hook( + home, + Some(PERMISSION_REQUEST_HOOK_MATCHER), + "allow_add_directories_user", + &requested_dir_string, + ) { + 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"); + }); + let test = builder.build(&server).await?; + + let _ = fs::remove_file(&first_file); + let _ = fs::remove_file(&second_file); + + test.submit_turn_with_policies( + "run the first shell command after hook approval", + AskForApproval::OnRequest, + SandboxPolicy::new_read_only_policy(), + ) + .await?; + assert_eq!(fs::read_to_string(&first_file)?, "first"); + + test.submit_turn_with_policies( + "run the second shell command with the persisted writable root", + AskForApproval::OnRequest, + SandboxPolicy::new_read_only_policy(), + ) + .await?; + assert_eq!(fs::read_to_string(&second_file)?, "second"); + + let requests = responses.requests(); + assert_eq!(requests.len(), 4); + requests[1].function_call_output(call_id_first); + requests[3].function_call_output(call_id_second); + + let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?; + assert_eq!( + hook_inputs.len(), + 1, + "persisted writable root should bypass the second permission hook" + ); + + let config_path = test.codex_home_path().join("config.toml"); + let config_contents = fs::read_to_string(&config_path) + .with_context(|| format!("read {}", config_path.display()))?; + assert!( + config_contents.contains("writable_roots") + && config_contents.contains(&requested_dir.display().to_string()), + "expected {config_path:?} to contain writable root for {}, got:\n{config_contents}", + requested_dir.display(), + ); + + 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/schema/generated/permission-request.command.output.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json index 8e8319caa7..3156c4a229 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,6 +13,24 @@ ], "type": "string" }, + "PermissionDirectoryUpdate": { + "properties": { + "destination": { + "$ref": "#/definitions/PermissionSuggestionDestination" + }, + "directories": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "destination", + "directories" + ], + "type": "object" + }, "PermissionRequestBehaviorWire": { "enum": [ "allow", @@ -23,6 +41,13 @@ "PermissionRequestDecisionWire": { "additionalProperties": false, "properties": { + "addDirectories": { + "default": null, + "items": { + "$ref": "#/definitions/PermissionDirectoryUpdate" + }, + "type": "array" + }, "behavior": { "$ref": "#/definitions/PermissionRequestBehaviorWire" }, diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index ec537c63cd..6f280d5697 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -23,6 +23,7 @@ pub(crate) struct PreToolUseOutput { pub(crate) enum PermissionRequestDecision { Allow { updated_permissions: Vec, + add_directories: Vec, }, Deny { message: String, @@ -305,9 +306,12 @@ 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.updated_permissions.is_some() || decision.add_directories.is_some()) { - Some("PermissionRequest hook returned updatedPermissions for deny decision".to_string()) + Some( + "PermissionRequest hook returned updatedPermissions/addDirectories for deny decision" + .to_string(), + ) } else if decision.interrupt { Some("PermissionRequest hook returned unsupported interrupt:true".to_string()) } else { @@ -321,6 +325,7 @@ 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 @@ -495,6 +500,42 @@ mod tests { crate::events::permission_request::PermissionSuggestionBehavior::Allow, destination: crate::events::permission_request::PermissionSuggestionDestination::UserSettings, }], + add_directories: vec![], + }) + ); + } + + #[test] + fn permission_request_accepts_add_directories_for_allow_decision() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "allow", + "addDirectories": [{ + "directories": ["./logs", "/tmp/output"], + "destination": "session" + }] + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + + assert_eq!( + parsed.decision, + Some(PermissionRequestDecision::Allow { + updated_permissions: vec![], + add_directories: vec![ + crate::events::permission_request::PermissionDirectoryUpdate { + directories: vec!["./logs".to_string(), "/tmp/output".to_string()], + destination: + crate::events::permission_request::PermissionSuggestionDestination::Session, + }, + ], }) ); } @@ -527,7 +568,37 @@ mod tests { assert_eq!( parsed.invalid_reason, Some( - "PermissionRequest hook returned updatedPermissions for deny decision".to_string() + "PermissionRequest hook returned updatedPermissions/addDirectories for deny decision" + .to_string() + ) + ); + } + + #[test] + fn permission_request_rejects_add_directories_for_deny_decision() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "deny", + "addDirectories": [{ + "directories": ["./logs"], + "destination": "session" + }] + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + + assert_eq!( + parsed.invalid_reason, + Some( + "PermissionRequest hook returned updatedPermissions/addDirectories 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 388580d559..bc5f97bda7 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -45,6 +45,13 @@ pub struct PermissionSuggestion { 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 { @@ -94,6 +101,7 @@ pub struct PermissionRequestRequest { pub enum PermissionRequestDecision { Allow { updated_permissions: Vec, + add_directories: Vec, }, Deny { message: String, @@ -214,10 +222,12 @@ 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 { @@ -225,6 +235,11 @@ 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 { @@ -235,6 +250,7 @@ fn resolve_permission_request_decision<'a>( } saw_allow.then_some(PermissionRequestDecision::Allow { updated_permissions, + add_directories, }) } @@ -244,6 +260,7 @@ fn invalid_permission_updates( ) -> Option { let PermissionRequestDecision::Allow { updated_permissions, + add_directories: _, } = decision? else { return None; @@ -317,9 +334,11 @@ 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 } => { @@ -388,6 +407,7 @@ fn parse_completed( mod tests { use pretty_assertions::assert_eq; + use super::PermissionDirectoryUpdate; use super::PermissionRequestDecision; use super::PermissionSuggestion; use super::PermissionSuggestionBehavior; @@ -402,6 +422,7 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![], + add_directories: vec![], }, PermissionRequestDecision::Deny { message: "repo deny".to_string(), @@ -421,9 +442,11 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![], + add_directories: vec![], }, PermissionRequestDecision::Allow { updated_permissions: vec![], + add_directories: vec![], }, ]; @@ -431,6 +454,7 @@ mod tests { resolve_permission_request_decision(decisions.iter()), Some(PermissionRequestDecision::Allow { updated_permissions: vec![], + add_directories: vec![], }) ); } @@ -455,9 +479,11 @@ mod tests { let decisions = [ PermissionRequestDecision::Allow { updated_permissions: vec![permission.clone()], + add_directories: vec![], }, PermissionRequestDecision::Allow { updated_permissions: vec![permission.clone()], + add_directories: vec![], }, ]; @@ -465,6 +491,33 @@ 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 { + directories: vec!["./logs".to_string(), "/tmp/output".to_string()], + destination: PermissionSuggestionDestination::Session, + }; + let decisions = [ + PermissionRequestDecision::Allow { + updated_permissions: vec![], + add_directories: vec![directory_update.clone()], + }, + PermissionRequestDecision::Allow { + updated_permissions: vec![], + add_directories: vec![directory_update.clone()], + }, + ]; + + assert_eq!( + resolve_permission_request_decision(decisions.iter()), + Some(PermissionRequestDecision::Allow { + updated_permissions: vec![], + add_directories: vec![directory_update], }) ); } @@ -488,6 +541,7 @@ mod tests { behavior: PermissionSuggestionBehavior::Allow, destination: PermissionSuggestionDestination::UserSettings, }], + add_directories: vec![], }; assert_eq!( diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 75b87d2566..73db6f8b85 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -5,6 +5,7 @@ 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; diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 5b57316663..7015c87fdf 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -12,6 +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; const GENERATED_DIR: &str = "generated"; @@ -148,6 +149,8 @@ pub(crate) struct PermissionRequestDecisionWire { #[serde(default)] pub updated_permissions: Option>, #[serde(default)] + pub add_directories: Option>, + #[serde(default)] pub message: Option, /// Reserved for future short-circuiting semantics. ///