diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index c4be53cf0d..d833a1b26f 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -3149,7 +3149,7 @@ impl Session { pub async fn notify_request_permissions_response( &self, call_id: &str, - response: RequestPermissionsResponse, + mut response: RequestPermissionsResponse, ) { let mut granted_for_session = None; let mut granted_always_allow = None; @@ -3192,6 +3192,7 @@ impl Session { .await { error!("failed to persist granted permissions: {err}"); + response.scope = PermissionGrantScope::Session; } else { self.reload_user_config_layer().await; } diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index cb80199d81..b26ab6e93c 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2613,7 +2613,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req } #[tokio::test] -async fn notify_request_permissions_response_persists_always_allow_permissions() { +async fn notify_request_permissions_response_falls_back_to_session_when_persist_fails() { let (session, _turn_context) = make_session_and_context().await; let active_turn = ActiveTurn::default(); let (tx, rx) = oneshot::channel(); @@ -2654,7 +2654,7 @@ async fn notify_request_permissions_response_persists_always_allow_permissions() ); assert_eq!( rx.await.expect("response should be delivered").scope, - PermissionGrantScope::AlwaysAllow + PermissionGrantScope::Session ); assert!(!session.codex_home().await.join(CONFIG_TOML_FILE).exists()); diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 7695cac662..944e634ce3 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -6220,9 +6220,121 @@ sandbox_mode = "workspace-write" assert_eq!( err.to_string(), - "cannot persist granted permissions without `default_permissions`" + "cannot persist granted permissions without user-configured `default_permissions`" ); assert!(!codex_home.path().join(CONFIG_TOML_FILE).exists()); Ok(()) } + +#[tokio::test] +async fn persist_granted_permission_profile_does_not_copy_inherited_profiles() -> anyhow::Result<()> +{ + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + std::fs::write(cwd.path().join(".git"), "gitdir: nowhere")?; + std::fs::create_dir_all(cwd.path().join(".codex"))?; + + let inherited_read = AbsolutePathBuf::try_from(cwd.path().join("repo-only"))?; + std::fs::write( + cwd.path().join(".codex").join(CONFIG_TOML_FILE), + format!( + r#" +sandbox_mode = "workspace-write" +default_permissions = "workspace" + +[permissions.workspace.filesystem] +read = ["{}"] + +[permissions.workspace.network] +enabled = true +"#, + inherited_read.display() + ), + )?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#" +sandbox_mode = "workspace-write" +default_permissions = "workspace" + +[permissions.workspace] +"#, + )?; + + let cwd = AbsolutePathBuf::try_from(cwd.path().to_path_buf())?; + let config_layer_stack = load_config_layers_state( + codex_home.path(), + Some(cwd.clone()), + &[], + LoaderOverrides::default(), + CloudRequirementsLoader::default(), + ) + .await?; + let config_toml = deserialize_config_toml_with_base( + config_layer_stack.effective_config(), + codex_home.path(), + )?; + let config = Config::load_config_with_layer_stack( + config_toml, + ConfigOverrides { + cwd: Some(cwd.to_path_buf()), + ..Default::default() + }, + codex_home.path().to_path_buf(), + config_layer_stack, + )?; + + let granted_write = AbsolutePathBuf::try_from(cwd.as_path().join("logs"))?; + let profile_name = persist_granted_permission_profile( + codex_home.path(), + &config, + &PermissionProfile { + file_system: Some(FileSystemPermissions { + read: None, + write: Some(vec![granted_write.clone()]), + }), + network: None, + macos: None, + }, + ) + .await?; + + assert_eq!(profile_name, "workspace"); + + let persisted = std::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE))?; + let persisted: ConfigToml = toml::from_str(&persisted)?; + assert_eq!(persisted.default_permissions.as_deref(), Some("workspace")); + assert_eq!( + persisted + .permissions + .as_ref() + .and_then(|permissions| permissions.entries.get("workspace")), + Some(&PermissionProfileToml { + filesystem: Some(FilesystemPermissionsToml { + entries: BTreeMap::from([( + granted_write.display().to_string(), + FilesystemPermissionToml::Access(FileSystemAccessMode::Write), + )]), + }), + network: None, + macos: None, + }) + ); + assert!( + !persisted + .permissions + .as_ref() + .and_then(|permissions| permissions.entries.get("workspace")) + .is_some_and( + |profile| profile + .filesystem + .as_ref() + .is_some_and(|filesystem| filesystem + .entries + .contains_key(&inherited_read.display().to_string())) + ) + ); + + Ok(()) +} diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 53a29f9306..137a63f174 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2877,12 +2877,12 @@ impl Config { } } -pub(crate) fn default_permissions_profile_name(config: &Config) -> Option { +pub(crate) fn user_default_permissions_profile_name(config: &Config) -> Option { config .config_layer_stack - .effective_config() - .try_into::() - .ok() + .get_user_layer() + .map(|layer| layer.config.clone()) + .and_then(|config| config.try_into::().ok()) .and_then(|selection| selection.default_permissions) } @@ -2892,14 +2892,16 @@ pub(crate) async fn persist_granted_permission_profile( granted_permissions: &PermissionProfile, ) -> anyhow::Result { let granted_profile = permission_profile_toml_from_runtime_permissions(granted_permissions); - let Some(profile_name) = default_permissions_profile_name(config) else { - anyhow::bail!("cannot persist granted permissions without `default_permissions`"); + let Some(profile_name) = user_default_permissions_profile_name(config) else { + anyhow::bail!( + "cannot persist granted permissions without user-configured `default_permissions`" + ); }; let existing = config .config_layer_stack - .effective_config() - .try_into::() - .ok() + .get_user_layer() + .map(|layer| layer.config.clone()) + .and_then(|config| config.try_into::().ok()) .and_then(|config_toml| config_toml.permissions) .and_then(|permissions| permissions.entries.get(&profile_name).cloned()); let profile = merge_permission_profile_toml(existing.as_ref(), &granted_profile); diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 7a08c7d1d3..6c4a32679f 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -713,14 +713,7 @@ fn exec_options( display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], }), - ReviewDecision::ApprovedForAlways => { - additional_permissions.as_ref().map(|_| ApprovalOption { - label: "Yes, and always allow these permissions".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::ApprovedForAlways), - display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], - }) - } + ReviewDecision::ApprovedForAlways => None, ReviewDecision::NetworkPolicyAmendment { network_policy_amendment, } => { @@ -1289,6 +1282,37 @@ mod tests { ); } + #[test] + fn additional_permissions_exec_options_hide_persistent_approval() { + let additional_permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(vec![absolute_path("/tmp/readme.txt")]), + write: Some(vec![absolute_path("/tmp/out.txt")]), + }), + ..Default::default() + }; + let options = exec_options( + &[ + ReviewDecision::Approved, + ReviewDecision::ApprovedForSession, + ReviewDecision::ApprovedForAlways, + ReviewDecision::Abort, + ], + None, + Some(&additional_permissions), + ); + + let labels: Vec = options.into_iter().map(|option| option.label).collect(); + assert_eq!( + labels, + vec![ + "Yes, proceed".to_string(), + "Yes, and allow these permissions for this session".to_string(), + "No, and tell Codex what to do differently".to_string(), + ] + ); + } + #[test] fn permissions_options_use_expected_labels() { let labels: Vec = permissions_options() diff --git a/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs b/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs index 004375ee3e..1501cb0df4 100644 --- a/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs @@ -699,14 +699,7 @@ fn exec_options( display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], }), - ReviewDecision::ApprovedForAlways => { - additional_permissions.as_ref().map(|_| ApprovalOption { - label: "Yes, and always allow these permissions".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::ApprovedForAlways), - display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], - }) - } + ReviewDecision::ApprovedForAlways => None, ReviewDecision::NetworkPolicyAmendment { network_policy_amendment, } => { @@ -1275,6 +1268,37 @@ mod tests { ); } + #[test] + fn additional_permissions_exec_options_hide_persistent_approval() { + let additional_permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(vec![absolute_path("/tmp/readme.txt")]), + write: Some(vec![absolute_path("/tmp/out.txt")]), + }), + ..Default::default() + }; + let options = exec_options( + &[ + ReviewDecision::Approved, + ReviewDecision::ApprovedForSession, + ReviewDecision::ApprovedForAlways, + ReviewDecision::Abort, + ], + None, + Some(&additional_permissions), + ); + + let labels: Vec = options.into_iter().map(|option| option.label).collect(); + assert_eq!( + labels, + vec![ + "Yes, proceed".to_string(), + "Yes, and allow these permissions for this session".to_string(), + "No, and tell Codex what to do differently".to_string(), + ] + ); + } + #[test] fn permissions_options_use_expected_labels() { let labels: Vec = permissions_options()