diff --git a/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs b/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs index d1d7c680f6..6cedaeeabe 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs @@ -7,17 +7,19 @@ use codex_protocol::approvals::NetworkPolicyRuleAction as CoreNetworkPolicyRuleA use codex_protocol::models::ActivePermissionProfile as CoreActivePermissionProfile; use codex_protocol::models::AdditionalPermissionProfile as CoreAdditionalPermissionProfile; use codex_protocol::models::FileSystemPermissions as CoreFileSystemPermissions; +use codex_protocol::models::ManagedFileSystemPermissions as CoreManagedFileSystemPermissions; use codex_protocol::models::NetworkPermissions as CoreNetworkPermissions; +use codex_protocol::models::PermissionProfile as CorePermissionProfile; use codex_protocol::permissions::FileSystemAccessMode as CoreFileSystemAccessMode; use codex_protocol::permissions::FileSystemPath as CoreFileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry as CoreFileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSpecialPath as CoreFileSystemSpecialPath; +use codex_protocol::permissions::NetworkSandboxPolicy as CoreNetworkSandboxPolicy; use codex_protocol::protocol::NetworkAccess as CoreNetworkAccess; use codex_protocol::request_permissions::PermissionGrantScope as CorePermissionGrantScope; use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; -use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::LegacyAppPathString; -use codex_utils_path_uri::PathConvention; +use codex_utils_path_uri::PathUri; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; @@ -68,9 +70,8 @@ pub struct AdditionalFileSystemPermissions { pub entries: Option>, } -// TODO(anp): Remove this conversion once core permission paths use PathUri. -impl From> for AdditionalFileSystemPermissions { - fn from(value: CoreFileSystemPermissions) -> Self { +impl From> for AdditionalFileSystemPermissions { + fn from(value: CoreFileSystemPermissions) -> Self { if let Some((read, write)) = value.legacy_read_write_roots() { let mut entries = Vec::with_capacity( read.as_ref().map_or(0, Vec::len) + write.as_ref().map_or(0, Vec::len), @@ -78,7 +79,7 @@ impl From> for AdditionalFileSystemPe if let Some(paths) = read.as_ref() { entries.extend(paths.iter().map(|path| FileSystemSandboxEntry { path: FileSystemPath::Path { - path: LegacyAppPathString::from_abs_path(path), + path: path.clone().into(), }, access: FileSystemAccessMode::Read, })); @@ -86,7 +87,7 @@ impl From> for AdditionalFileSystemPe if let Some(paths) = write.as_ref() { entries.extend(paths.iter().map(|path| FileSystemSandboxEntry { path: FileSystemPath::Path { - path: LegacyAppPathString::from_abs_path(path), + path: path.clone().into(), }, access: FileSystemAccessMode::Write, })); @@ -95,13 +96,15 @@ impl From> for AdditionalFileSystemPe read: read.map(|paths| { paths .iter() - .map(LegacyAppPathString::from_abs_path) + .cloned() + .map(LegacyAppPathString::from) .collect() }), write: write.map(|paths| { paths .iter() - .map(LegacyAppPathString::from_abs_path) + .cloned() + .map(LegacyAppPathString::from) .collect() }), glob_scan_max_depth: None, @@ -124,8 +127,7 @@ impl From> for AdditionalFileSystemPe } } -// TODO(anp): Remove this conversion once core permission paths use PathUri. -impl TryFrom for CoreFileSystemPermissions { +impl TryFrom for CoreFileSystemPermissions { type Error = io::Error; fn try_from(value: AdditionalFileSystemPermissions) -> Result { @@ -133,7 +135,7 @@ impl TryFrom for CoreFileSystemPermissions::try_from) + .map(CoreFileSystemSandboxEntry::::try_from) .collect::>()?, glob_scan_max_depth: None, } @@ -144,9 +146,8 @@ impl TryFrom for CoreFileSystemPermissions>>() }) @@ -157,9 +158,8 @@ impl TryFrom for CoreFileSystemPermissions>>() }) @@ -194,7 +194,7 @@ impl From for CoreNetworkPermissions { } } -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[serde(deny_unknown_fields)] #[ts(export_to = "v2/")] @@ -203,9 +203,8 @@ pub struct RequestPermissionProfile { pub file_system: Option, } -// TODO(anp): Remove this conversion once core permission paths use PathUri. -impl From for RequestPermissionProfile { - fn from(value: CoreRequestPermissionProfile) -> Self { +impl From> for RequestPermissionProfile { + fn from(value: CoreRequestPermissionProfile) -> Self { Self { network: value.network.map(AdditionalNetworkPermissions::from), file_system: value.file_system.map(AdditionalFileSystemPermissions::from), @@ -213,7 +212,7 @@ impl From for RequestPermissionProfile { } } -impl TryFrom for CoreRequestPermissionProfile { +impl TryFrom for CoreRequestPermissionProfile { type Error = io::Error; fn try_from(value: RequestPermissionProfile) -> Result { @@ -221,7 +220,7 @@ impl TryFrom for CoreRequestPermissionProfile { network: value.network.map(CoreNetworkPermissions::from), file_system: value .file_system - .map(CoreFileSystemPermissions::::try_from) + .map(CoreFileSystemPermissions::::try_from) .transpose()?, }) } @@ -307,13 +306,10 @@ pub enum FileSystemPath { Special { value: FileSystemSpecialPath }, } -// TODO(anp): Remove this conversion once core permission paths use PathUri. -impl From> for FileSystemPath { - fn from(value: CoreFileSystemPath) -> Self { +impl From> for FileSystemPath { + fn from(value: CoreFileSystemPath) -> Self { match value { - CoreFileSystemPath::Path { path } => Self::Path { - path: LegacyAppPathString::from_abs_path(&path), - }, + CoreFileSystemPath::Path { path } => Self::Path { path: path.into() }, CoreFileSystemPath::GlobPattern { pattern } => Self::GlobPattern { pattern }, CoreFileSystemPath::Special { value } => Self::Special { value: value.into(), @@ -322,17 +318,14 @@ impl From> for FileSystemPath { } } -// TODO(anp): Remove this conversion once core permission paths use PathUri. -impl TryFrom for CoreFileSystemPath { +impl TryFrom for CoreFileSystemPath { type Error = io::Error; fn try_from(value: FileSystemPath) -> Result { Ok(match value { FileSystemPath::Path { path } => Self::Path { - path: path - .to_path_uri(PathConvention::native()) - .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))? - .to_abs_path()?, + path: PathUri::try_from(path) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))?, }, FileSystemPath::GlobPattern { pattern } => Self::GlobPattern { pattern }, FileSystemPath::Special { value } => Self::Special { @@ -350,9 +343,8 @@ pub struct FileSystemSandboxEntry { pub access: FileSystemAccessMode, } -// TODO(anp): Remove this conversion once core permission paths use PathUri. -impl From> for FileSystemSandboxEntry { - fn from(value: CoreFileSystemSandboxEntry) -> Self { +impl From> for FileSystemSandboxEntry { + fn from(value: CoreFileSystemSandboxEntry) -> Self { Self { path: value.path.into(), access: value.access.into(), @@ -360,7 +352,7 @@ impl From> for FileSystemSandboxEntr } } -impl TryFrom for CoreFileSystemSandboxEntry { +impl TryFrom for CoreFileSystemSandboxEntry { type Error = io::Error; fn try_from(value: FileSystemSandboxEntry) -> Result { @@ -383,7 +375,7 @@ pub struct PermissionProfileListParams { pub limit: Option, /// Optional working directory to resolve project config layers. #[ts(optional = nullable)] - pub cwd: Option, + pub cwd: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] @@ -461,9 +453,8 @@ pub struct AdditionalPermissionProfile { pub file_system: Option, } -// TODO(anp): Remove this conversion once core permission paths use PathUri. -impl From for AdditionalPermissionProfile { - fn from(value: CoreAdditionalPermissionProfile) -> Self { +impl From> for AdditionalPermissionProfile { + fn from(value: CoreAdditionalPermissionProfile) -> Self { Self { network: value.network.map(AdditionalNetworkPermissions::from), file_system: value.file_system.map(AdditionalFileSystemPermissions::from), @@ -471,7 +462,7 @@ impl From for AdditionalPermissionProfile { } } -impl TryFrom for CoreAdditionalPermissionProfile { +impl TryFrom for CoreAdditionalPermissionProfile { type Error = io::Error; fn try_from(value: AdditionalPermissionProfile) -> Result { @@ -479,7 +470,7 @@ impl TryFrom for CoreAdditionalPermissionProfile { network: value.network.map(CoreNetworkPermissions::from), file_system: value .file_system - .map(CoreFileSystemPermissions::::try_from) + .map(CoreFileSystemPermissions::::try_from) .transpose()?, }) } @@ -497,7 +488,7 @@ pub struct GrantedPermissionProfile { pub file_system: Option, } -impl TryFrom for CoreAdditionalPermissionProfile { +impl TryFrom for CoreAdditionalPermissionProfile { type Error = io::Error; fn try_from(value: GrantedPermissionProfile) -> Result { @@ -505,12 +496,18 @@ impl TryFrom for CoreAdditionalPermissionProfile { network: value.network.map(CoreNetworkPermissions::from), file_system: value .file_system - .map(CoreFileSystemPermissions::::try_from) + .map(CoreFileSystemPermissions::::try_from) .transpose()?, }) } } +impl GrantedPermissionProfile { + pub fn is_empty(&self) -> bool { + self.network.is_none() && self.file_system.is_none() + } +} + #[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -542,7 +539,7 @@ pub enum SandboxPolicy { #[ts(rename_all = "camelCase")] WorkspaceWrite { #[serde(default)] - writable_roots: Vec, + writable_roots: Vec, #[serde(default)] network_access: bool, #[serde(default)] @@ -571,7 +568,7 @@ enum SandboxPolicyDeserialize { #[serde(rename_all = "camelCase")] WorkspaceWrite { #[serde(default)] - writable_roots: Vec, + writable_roots: Vec, #[serde(default)] read_only_access: Option, #[serde(default)] @@ -635,8 +632,103 @@ impl<'de> Deserialize<'de> for SandboxPolicy { } impl SandboxPolicy { - pub fn to_core(&self) -> codex_protocol::protocol::SandboxPolicy { - match self { + pub fn try_to_path_uri_permission_profile(&self) -> io::Result> { + let network = match self { + SandboxPolicy::DangerFullAccess => CoreNetworkSandboxPolicy::Enabled, + SandboxPolicy::ReadOnly { network_access } + | SandboxPolicy::WorkspaceWrite { network_access, .. } => { + if *network_access { + CoreNetworkSandboxPolicy::Enabled + } else { + CoreNetworkSandboxPolicy::Restricted + } + } + SandboxPolicy::ExternalSandbox { network_access } => match network_access { + NetworkAccess::Restricted => CoreNetworkSandboxPolicy::Restricted, + NetworkAccess::Enabled => CoreNetworkSandboxPolicy::Enabled, + }, + }; + + Ok(match self { + SandboxPolicy::DangerFullAccess => CorePermissionProfile::Disabled, + SandboxPolicy::ExternalSandbox { .. } => CorePermissionProfile::External { network }, + SandboxPolicy::ReadOnly { .. } => CorePermissionProfile::Managed { + file_system: CoreManagedFileSystemPermissions::Restricted { + entries: vec![CoreFileSystemSandboxEntry { + path: CoreFileSystemPath::Special { + value: CoreFileSystemSpecialPath::Root, + }, + access: CoreFileSystemAccessMode::Read, + }], + glob_scan_max_depth: None, + }, + network, + }, + SandboxPolicy::WorkspaceWrite { + writable_roots, + exclude_tmpdir_env_var, + exclude_slash_tmp, + .. + } => { + let mut entries = vec![ + CoreFileSystemSandboxEntry { + path: CoreFileSystemPath::Special { + value: CoreFileSystemSpecialPath::Root, + }, + access: CoreFileSystemAccessMode::Read, + }, + CoreFileSystemSandboxEntry { + path: CoreFileSystemPath::Special { + value: CoreFileSystemSpecialPath::ProjectRoots { subpath: None }, + }, + access: CoreFileSystemAccessMode::Write, + }, + ]; + if !exclude_slash_tmp { + entries.push(CoreFileSystemSandboxEntry { + path: CoreFileSystemPath::Special { + value: CoreFileSystemSpecialPath::SlashTmp, + }, + access: CoreFileSystemAccessMode::Write, + }); + } + if !exclude_tmpdir_env_var { + entries.push(CoreFileSystemSandboxEntry { + path: CoreFileSystemPath::Special { + value: CoreFileSystemSpecialPath::Tmpdir, + }, + access: CoreFileSystemAccessMode::Write, + }); + } + entries.extend( + writable_roots + .iter() + .cloned() + .map(|root| { + Ok(CoreFileSystemSandboxEntry { + path: CoreFileSystemPath::Path { + path: PathUri::try_from(root).map_err(|err| { + io::Error::new(io::ErrorKind::InvalidInput, err) + })?, + }, + access: CoreFileSystemAccessMode::Write, + }) + }) + .collect::>>()?, + ); + CorePermissionProfile::Managed { + file_system: CoreManagedFileSystemPermissions::Restricted { + entries, + glob_scan_max_depth: None, + }, + network, + } + } + }) + } + + pub fn try_to_core(&self) -> io::Result { + Ok(match self { SandboxPolicy::DangerFullAccess => { codex_protocol::protocol::SandboxPolicy::DangerFullAccess } @@ -659,12 +751,26 @@ impl SandboxPolicy { exclude_tmpdir_env_var, exclude_slash_tmp, } => codex_protocol::protocol::SandboxPolicy::WorkspaceWrite { - writable_roots: writable_roots.clone(), + writable_roots: writable_roots + .iter() + .cloned() + .map(|root| { + if let Ok(root) = root.to_host_abs_path() { + return Ok(root); + } + let root_uri = PathUri::try_from(root) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))?; + Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("sandbox writable root path URI {root_uri} is not local"), + )) + }) + .collect::>()?, network_access: *network_access, exclude_tmpdir_env_var: *exclude_tmpdir_env_var, exclude_slash_tmp: *exclude_slash_tmp, }, - } + }) } } @@ -691,7 +797,10 @@ impl From for SandboxPolicy { exclude_tmpdir_env_var, exclude_slash_tmp, } => SandboxPolicy::WorkspaceWrite { - writable_roots, + writable_roots: writable_roots + .iter() + .map(LegacyAppPathString::from_abs_path) + .collect(), network_access, exclude_tmpdir_env_var, exclude_slash_tmp, @@ -765,7 +874,7 @@ pub struct PermissionsRequestApprovalParams { /// Unix timestamp (in milliseconds) when this approval request started. #[ts(type = "number")] pub started_at_ms: i64, - pub cwd: AbsolutePathBuf, + pub cwd: LegacyAppPathString, pub reason: Option, pub permissions: RequestPermissionProfile, } diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index db5e505eb4..a1d3b48862 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -109,10 +109,11 @@ use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequest use codex_protocol::request_permissions::RequestPermissionsResponse as CoreRequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputAnswer as CoreRequestUserInputAnswer; use codex_protocol::request_user_input::RequestUserInputResponse as CoreRequestUserInputResponse; -use codex_sandboxing::policy_transforms::intersect_permission_profiles; +use codex_sandboxing::policy_transforms::intersect_permission_profiles_for_uri; use codex_shell_command::parse_command::shlex_join; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathUri; use std::collections::HashMap; use std::sync::Arc; use std::time::SystemTime; @@ -539,7 +540,10 @@ pub(crate) async fn apply_bespoke_event_handling( item_id: item_id.clone(), started_at_ms: event.started_at_ms, reason: event.reason.clone(), - grant_root: event.grant_root.clone(), + grant_root: event + .grant_root + .as_deref() + .map(LegacyAppPathString::from_path), }; let (pending_request_id, rx) = outgoing .send_request(ServerRequestPayload::FileChangeRequestApproval(params)) @@ -790,7 +794,7 @@ pub(crate) async fn apply_bespoke_event_handling( let requested_permissions = request.permissions.clone(); let request_cwd = match request.cwd.clone() { Some(cwd) => cwd, - None => conversation.config_snapshot().await.cwd().clone(), + None => PathUri::from_abs_path(conversation.config_snapshot().await.cwd()), }; let params = PermissionsRequestApprovalParams { thread_id: conversation_id.to_string(), @@ -798,7 +802,7 @@ pub(crate) async fn apply_bespoke_event_handling( item_id: request.call_id.clone(), environment_id: request.environment_id.clone(), started_at_ms: request.started_at_ms, - cwd: request_cwd.clone(), + cwd: request_cwd.clone().into(), reason: request.reason, permissions: request.permissions.into(), }; @@ -1783,14 +1787,12 @@ async fn on_request_permissions_response( let response = match request_permissions_response_from_client_result( requested_permissions, response, - request_cwd.as_path(), + &request_cwd, ) { Ok(Some(response)) => response, Ok(None) => return, - // TODO(anp): Remove this native-path localization error path once core permission paths - // remain PathUri after crossing the app-server boundary. Err(err) => { - let message = format!("failed to localize granted filesystem paths: {err}"); + let message = format!("failed to parse granted filesystem paths: {err}"); handle_error_notification( conversation_id, &turn_id, @@ -1826,8 +1828,8 @@ struct PendingRequestPermissionsResponse { call_id: String, conversation_id: ThreadId, turn_id: String, - requested_permissions: CoreRequestPermissionProfile, - request_cwd: AbsolutePathBuf, + requested_permissions: CoreRequestPermissionProfile, + request_cwd: PathUri, pending_request_id: RequestId, outgoing: ThreadScopedOutgoingMessageSender, receiver: oneshot::Receiver, @@ -1835,10 +1837,10 @@ struct PendingRequestPermissionsResponse { } fn request_permissions_response_from_client_result( - requested_permissions: CoreRequestPermissionProfile, + requested_permissions: CoreRequestPermissionProfile, response: std::result::Result, - cwd: &std::path::Path, -) -> std::io::Result> { + cwd: &PathUri, +) -> std::io::Result>> { let value = match response { Ok(Ok(value)) => value, Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return Ok(None), @@ -1883,11 +1885,17 @@ fn request_permissions_response_from_client_result( strict_auto_review: false, })); } - let granted_permissions: CoreAdditionalPermissionProfile = response.permissions.try_into()?; + let granted_permissions: CoreAdditionalPermissionProfile = + response.permissions.try_into()?; let permissions = if granted_permissions.is_empty() { CoreRequestPermissionProfile::default() } else { - intersect_permission_profiles(requested_permissions.into(), granted_permissions, cwd).into() + intersect_permission_profiles_for_uri( + requested_permissions.into(), + granted_permissions, + cwd, + ) + .into() }; Ok(Some(CoreRequestPermissionsResponse { permissions, @@ -2148,6 +2156,14 @@ mod tests { Arc::new(Mutex::new(ThreadState::default())) } + fn current_dir_uri() -> PathUri { + let cwd = AbsolutePathBuf::from_absolute_path( + std::env::current_dir().expect("current dir should be available"), + ) + .expect("current dir should be absolute"); + PathUri::from_abs_path(&cwd) + } + const TEST_TURN_COMPLETED_AT: i64 = 1_716_000_456; const TEST_TURN_DURATION_MS: i64 = 1_234; @@ -2350,7 +2366,7 @@ mod tests { let action = codex_protocol::protocol::GuardianAssessmentAction::Command { source: codex_protocol::protocol::GuardianCommandSource::Shell, command: "rm -rf /tmp/example.sqlite".to_string(), - cwd: test_path_buf("/tmp").abs(), + cwd: test_path_buf("/tmp").abs().into(), }; let notification = guardian_auto_approval_review_notification( &conversation_id, @@ -2396,7 +2412,7 @@ mod tests { let action = codex_protocol::protocol::GuardianAssessmentAction::Command { source: codex_protocol::protocol::GuardianCommandSource::Shell, command: "rm -rf /tmp/example.sqlite".to_string(), - cwd: test_path_buf("/tmp").abs(), + cwd: test_path_buf("/tmp").abs().into(), }; let notification = guardian_auto_approval_review_notification( &conversation_id, @@ -2882,7 +2898,7 @@ mod tests { let response = request_permissions_response_from_client_result( CoreRequestPermissionProfile::default(), Ok(Err(error)), - std::env::current_dir().expect("current dir").as_path(), + ¤t_dir_uri(), ) .expect("paths should localize"); @@ -2906,16 +2922,18 @@ mod tests { } else { "/tmp/ignored" }; - let absolute_path = |path: &str| { - AbsolutePathBuf::try_from(std::path::PathBuf::from(path)).expect("absolute path") + let path_uri = |path: &str| { + PathUri::from_abs_path( + &AbsolutePathBuf::try_from(std::path::PathBuf::from(path)).expect("absolute path"), + ) }; let requested_permissions = CoreRequestPermissionProfile { network: Some(CoreNetworkPermissions { enabled: Some(true), }), file_system: Some(CoreFileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path(input_path)]), - Some(vec![absolute_path(output_path)]), + Some(vec![path_uri(input_path)]), + Some(vec![path_uri(output_path)]), )), }; let cases = vec![ @@ -2945,7 +2963,7 @@ mod tests { CoreRequestPermissionProfile { file_system: Some(CoreFileSystemPermissions::from_read_write_roots( /*read*/ None, - Some(vec![absolute_path(output_path)]), + Some(vec![path_uri(output_path)]), )), ..CoreRequestPermissionProfile::default() }, @@ -2962,22 +2980,22 @@ mod tests { }), CoreRequestPermissionProfile { file_system: Some(CoreFileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path(input_path)]), - Some(vec![absolute_path(output_path)]), + Some(vec![path_uri(input_path)]), + Some(vec![path_uri(output_path)]), )), ..CoreRequestPermissionProfile::default() }, ), ]; - let cwd = std::env::current_dir().expect("current dir"); + let cwd = current_dir_uri(); for (granted_permissions, expected_permissions) in cases { let response = request_permissions_response_from_client_result( requested_permissions.clone(), Ok(Ok(serde_json::json!({ "permissions": granted_permissions, }))), - cwd.as_path(), + &cwd, ) .expect("paths should localize") .expect("response should be accepted"); @@ -3001,7 +3019,7 @@ mod tests { "scope": "session", "permissions": {}, }))), - std::env::current_dir().expect("current dir").as_path(), + ¤t_dir_uri(), ) .expect("paths should localize") .expect("response should be accepted"); @@ -3029,7 +3047,7 @@ mod tests { }, }, }))), - std::env::current_dir().expect("current dir").as_path(), + ¤t_dir_uri(), ) .expect("paths should localize") .expect("response should be accepted"); @@ -3061,7 +3079,7 @@ mod tests { }, }, }))), - std::env::current_dir().expect("current dir").as_path(), + ¤t_dir_uri(), ) .expect("paths should localize") .expect("response should be accepted"); @@ -3075,18 +3093,21 @@ mod tests { let temp_dir = TempDir::new().expect("temp dir"); let cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute cwd"); let child = cwd.join("child"); - let requested_permissions = CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions { - entries: vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::project_roots(/*subpath*/ None), - }, - access: FileSystemAccessMode::Write, - }], - glob_scan_max_depth: None, - }), - ..Default::default() - }; + let cwd_uri = PathUri::from_abs_path(&cwd); + let child_uri = PathUri::from_abs_path(&child); + let requested_permissions: CoreRequestPermissionProfile = + CoreRequestPermissionProfile { + file_system: Some(CoreFileSystemPermissions { + entries: vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + access: FileSystemAccessMode::Write, + }], + glob_scan_max_depth: None, + }), + ..Default::default() + }; let response = request_permissions_response_from_client_result( requested_permissions, @@ -3097,7 +3118,7 @@ mod tests { }, }, }))), - cwd.as_path(), + &cwd_uri, ) .expect("paths should localize") .expect("response should be accepted"); @@ -3107,7 +3128,7 @@ mod tests { CoreRequestPermissionProfile { file_system: Some(CoreFileSystemPermissions::from_read_write_roots( /*read*/ None, - Some(vec![child]), + Some(vec![child_uri]), )), ..Default::default() } @@ -3122,18 +3143,20 @@ mod tests { let later_cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("later-cwd")) .expect("absolute later cwd"); let later_child = later_cwd.join("child"); - let requested_permissions = CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions { - entries: vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::project_roots(/*subpath*/ None), - }, - access: FileSystemAccessMode::Write, - }], - glob_scan_max_depth: None, - }), - ..Default::default() - }; + let request_cwd_uri = PathUri::from_abs_path(&request_cwd); + let requested_permissions: CoreRequestPermissionProfile = + CoreRequestPermissionProfile { + file_system: Some(CoreFileSystemPermissions { + entries: vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + access: FileSystemAccessMode::Write, + }], + glob_scan_max_depth: None, + }), + ..Default::default() + }; let response = request_permissions_response_from_client_result( requested_permissions, @@ -3144,7 +3167,7 @@ mod tests { }, }, }))), - request_cwd.as_path(), + &request_cwd_uri, ) .expect("paths should localize") .expect("response should be accepted"); @@ -3160,10 +3183,12 @@ mod tests { let temp_dir = TempDir::new().expect("temp dir"); let cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute cwd"); let child = cwd.join("child"); + let cwd_uri = PathUri::from_abs_path(&cwd); + let child_uri = PathUri::from_abs_path(&child); let requested_permissions = CoreRequestPermissionProfile { file_system: Some(CoreFileSystemPermissions::from_read_write_roots( /*read*/ None, - Some(vec![child]), + Some(vec![child_uri]), )), ..Default::default() }; @@ -3186,7 +3211,7 @@ mod tests { }, }, }))), - cwd.as_path(), + &cwd_uri, ) .expect("paths should localize") .expect("response should be accepted"); diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index bd9a0f7c20..ece64c63df 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -1,12 +1,10 @@ -use std::path::Path; - use codex_analytics::GuardianReviewedAction; use codex_protocol::approvals::GuardianAssessmentAction; use codex_protocol::approvals::GuardianCommandSource; use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::request_permissions::RequestPermissionProfile; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use serde::Serialize; use serde_json::Value; @@ -18,17 +16,17 @@ pub(crate) enum GuardianApprovalRequest { Shell { id: String, command: Vec, - cwd: AbsolutePathBuf, + cwd: PathUri, sandbox_permissions: crate::sandboxing::SandboxPermissions, - additional_permissions: Option, + additional_permissions: Option>, justification: Option, }, ExecCommand { id: String, command: Vec, - cwd: AbsolutePathBuf, + cwd: PathUri, sandbox_permissions: crate::sandboxing::SandboxPermissions, - additional_permissions: Option, + additional_permissions: Option>, justification: Option, tty: bool, }, @@ -38,13 +36,13 @@ pub(crate) enum GuardianApprovalRequest { source: GuardianCommandSource, program: String, argv: Vec, - cwd: AbsolutePathBuf, - additional_permissions: Option, + cwd: PathUri, + additional_permissions: Option>, }, ApplyPatch { id: String, - cwd: AbsolutePathBuf, - files: Vec, + cwd: PathUri, + files: Vec, patch: String, }, NetworkAccess { @@ -73,7 +71,7 @@ pub(crate) enum GuardianApprovalRequest { id: String, turn_id: String, reason: Option, - permissions: RequestPermissionProfile, + permissions: RequestPermissionProfile, }, } @@ -83,10 +81,14 @@ pub(crate) struct GuardianNetworkAccessTrigger { pub(crate) call_id: String, pub(crate) tool_name: String, pub(crate) command: Vec, - pub(crate) cwd: AbsolutePathBuf, + #[serde(serialize_with = "serialize_path_uri_as_native_path")] + pub(crate) cwd: PathUri, pub(crate) sandbox_permissions: crate::sandboxing::SandboxPermissions, - #[serde(skip_serializing_if = "Option::is_none")] - pub(crate) additional_permissions: Option, + #[serde( + skip_serializing_if = "Option::is_none", + serialize_with = "serialize_optional_additional_permissions_as_native_paths" + )] + pub(crate) additional_permissions: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub(crate) justification: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -107,10 +109,10 @@ pub(crate) struct GuardianMcpAnnotations { struct CommandApprovalAction<'a> { tool: &'a str, command: &'a [String], - cwd: &'a Path, + cwd: String, sandbox_permissions: crate::sandboxing::SandboxPermissions, #[serde(skip_serializing_if = "Option::is_none")] - additional_permissions: Option<&'a AdditionalPermissionProfile>, + additional_permissions: Option>, #[serde(skip_serializing_if = "Option::is_none")] justification: Option<&'a String>, #[serde(skip_serializing_if = "Option::is_none")] @@ -123,9 +125,9 @@ struct ExecveApprovalAction<'a> { tool: &'a str, program: &'a str, argv: &'a [String], - cwd: &'a Path, + cwd: String, #[serde(skip_serializing_if = "Option::is_none")] - additional_permissions: Option<&'a AdditionalPermissionProfile>, + additional_permissions: Option>, } #[derive(Serialize)] @@ -169,28 +171,50 @@ struct RequestPermissionsApprovalAction<'a> { turn_id: &'a str, #[serde(skip_serializing_if = "Option::is_none")] reason: Option<&'a String>, - permissions: &'a RequestPermissionProfile, + permissions: RequestPermissionProfile, } fn serialize_guardian_action(value: impl Serialize) -> serde_json::Result { serde_json::to_value(value) } +fn serialize_path_uri_as_native_path(path: &PathUri, serializer: S) -> Result +where + S: serde::Serializer, +{ + serializer.serialize_str(&path.inferred_native_path_string()) +} + +fn serialize_optional_additional_permissions_as_native_paths( + permissions: &Option>, + serializer: S, +) -> Result +where + S: serde::Serializer, +{ + permissions + .clone() + .map(|permissions| permissions.map_paths(|path| path.inferred_native_path_string())) + .serialize(serializer) +} + fn serialize_command_guardian_action( tool: &'static str, command: &[String], - cwd: &Path, + cwd: &PathUri, sandbox_permissions: crate::sandboxing::SandboxPermissions, - additional_permissions: Option<&AdditionalPermissionProfile>, + additional_permissions: Option<&AdditionalPermissionProfile>, justification: Option<&String>, tty: Option, ) -> serde_json::Result { serialize_guardian_action(CommandApprovalAction { tool, command, - cwd, + cwd: cwd.inferred_native_path_string(), sandbox_permissions, - additional_permissions, + additional_permissions: additional_permissions + .cloned() + .map(|permissions| permissions.map_paths(|path| path.inferred_native_path_string())), justification, tty, }) @@ -199,7 +223,7 @@ fn serialize_command_guardian_action( fn command_assessment_action( source: GuardianCommandSource, command: &[String], - cwd: &AbsolutePathBuf, + cwd: &PathUri, ) -> GuardianAssessmentAction { GuardianAssessmentAction::Command { source, @@ -308,8 +332,10 @@ pub(crate) fn guardian_approval_request_to_json( tool: guardian_command_source_tool_name(*source), program, argv, - cwd, - additional_permissions: additional_permissions.as_ref(), + cwd: cwd.inferred_native_path_string(), + additional_permissions: additional_permissions.clone().map(|permissions| { + permissions.map_paths(|path| path.inferred_native_path_string()) + }), }), GuardianApprovalRequest::ApplyPatch { id: _, @@ -318,8 +344,11 @@ pub(crate) fn guardian_approval_request_to_json( patch, } => Ok(serde_json::json!({ "tool": "apply_patch", - "cwd": cwd, - "files": files, + "cwd": cwd.inferred_native_path_string(), + "files": files + .iter() + .map(PathUri::inferred_native_path_string) + .collect::>(), "patch": patch, })), GuardianApprovalRequest::NetworkAccess { @@ -372,7 +401,9 @@ pub(crate) fn guardian_approval_request_to_json( tool: "request_permissions", turn_id, reason: reason.as_ref(), - permissions, + permissions: permissions + .clone() + .map_paths(|path| path.inferred_native_path_string()), }), } } @@ -455,7 +486,9 @@ pub(crate) fn guardian_reviewed_action( .. } => GuardianReviewedAction::Shell { sandbox_permissions: *sandbox_permissions, - additional_permissions: additional_permissions.clone(), + additional_permissions: additional_permissions.clone().map(|permissions| { + permissions.map_paths(|path| path.inferred_native_path_string()) + }), }, GuardianApprovalRequest::ExecCommand { sandbox_permissions, @@ -464,7 +497,9 @@ pub(crate) fn guardian_reviewed_action( .. } => GuardianReviewedAction::UnifiedExec { sandbox_permissions: *sandbox_permissions, - additional_permissions: additional_permissions.clone(), + additional_permissions: additional_permissions.clone().map(|permissions| { + permissions.map_paths(|path| path.inferred_native_path_string()) + }), tty: *tty, }, #[cfg(unix)] @@ -476,7 +511,9 @@ pub(crate) fn guardian_reviewed_action( } => GuardianReviewedAction::Execve { source: *source, program: program.clone(), - additional_permissions: additional_permissions.clone(), + additional_permissions: additional_permissions.clone().map(|permissions| { + permissions.map_paths(|path| path.inferred_native_path_string()) + }), }, GuardianApprovalRequest::ApplyPatch { .. } => GuardianReviewedAction::ApplyPatch {}, GuardianApprovalRequest::NetworkAccess { protocol, port, .. } => { diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 02c8a67473..b01567f668 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3,7 +3,6 @@ use std::collections::BTreeMap; use std::collections::HashMap; use std::collections::HashSet; use std::fmt::Debug; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicU64; @@ -142,7 +141,7 @@ use codex_rollout::state_db; use codex_rollout_trace::AgentResultTracePayload; use codex_rollout_trace::ThreadStartedTraceMetadata; use codex_rollout_trace::ThreadTraceContext; -use codex_sandboxing::policy_transforms::intersect_permission_profiles; +use codex_sandboxing::policy_transforms::intersect_permission_profiles_for_uri; use codex_shell_command::parse_command::parse_command; use codex_terminal_detection::user_agent; use codex_thread_store::CreateThreadParams; @@ -2151,11 +2150,11 @@ impl Session { approval_id: Option, environment_id: Option, command: Vec, - cwd: AbsolutePathBuf, + cwd: PathUri, reason: Option, network_approval_context: Option, proposed_execpolicy_amendment: Option, - additional_permissions: Option, + additional_permissions: Option>, available_decisions: Option>, ) -> ReviewDecision { // command-level approvals use `call_id`. @@ -2267,10 +2266,10 @@ impl Session { self: &Arc, turn_context: &Arc, call_id: String, - args: RequestPermissionsArgs, + args: RequestPermissionsArgs, environment: TurnEnvironmentSelection, cancellation_token: CancellationToken, - ) -> Option { + ) -> Option> { match turn_context.as_ref().approval_policy.value() { AskForApproval::Never => { return Some(RequestPermissionsResponse { @@ -2294,18 +2293,6 @@ impl Session { } let requested_permissions = args.permissions; - // TODO(anp): Migrate request_permissions to support paths from foreign environments. - let Ok(native_environment_cwd) = environment.cwd.to_abs_path() else { - warn!( - cwd = %environment.cwd, - "request_permissions requires a cwd native to the Codex host" - ); - return Some(RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - }); - }; if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) { let originating_turn_state = { @@ -2373,7 +2360,7 @@ impl Session { let response = Self::normalize_request_permissions_response( requested_permissions, response, - native_environment_cwd.as_path(), + &environment.cwd, ); self.record_granted_request_permissions_for_turn( &response, @@ -2413,7 +2400,7 @@ impl Session { started_at_ms: now_unix_timestamp_ms(), reason: args.reason, permissions: requested_permissions, - cwd: Some(native_environment_cwd), + cwd: Some(environment.cwd), }); self.send_event(turn_context.as_ref(), event).await; tokio::select! { @@ -2434,10 +2421,10 @@ impl Session { self: &Arc, turn_context: &Arc, call_id: String, - args: RequestPermissionsArgs, - cwd: AbsolutePathBuf, + args: RequestPermissionsArgs, + cwd: PathUri, cancellation_token: CancellationToken, - ) -> Option { + ) -> Option> { let turn_environment = match args.environment_id.as_deref() { Some(environment_id) => turn_context .environments @@ -2454,7 +2441,7 @@ impl Session { }); }; let mut environment = turn_environment.selection(); - environment.cwd = PathUri::from_abs_path(&cwd); + environment.cwd = cwd; self.request_permissions_for_environment( turn_context, call_id, @@ -2541,7 +2528,7 @@ impl Session { pub async fn notify_request_permissions_response( &self, call_id: &str, - response: RequestPermissionsResponse, + response: RequestPermissionsResponse, ) { let (entry, originating_turn_state) = { let mut active = self.active_turn.lock().await; @@ -2557,26 +2544,11 @@ impl Session { }; match entry { Some(entry) => { - // TODO(anp): Migrate request_permissions to support paths from foreign environments. - let response = match entry.environment.cwd.to_abs_path() { - Ok(native_environment_cwd) => Self::normalize_request_permissions_response( - entry.requested_permissions, - response, - native_environment_cwd.as_path(), - ), - Err(err) => { - warn!( - cwd = %entry.environment.cwd, - %err, - "request_permissions requires a cwd native to the Codex host" - ); - RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - } - } - }; + let response = Self::normalize_request_permissions_response( + entry.requested_permissions, + response, + &entry.environment.cwd, + ); self.record_granted_request_permissions_for_turn( &response, &entry.environment.environment_id, @@ -2592,10 +2564,10 @@ impl Session { } fn normalize_request_permissions_response( - requested_permissions: RequestPermissionProfile, - response: RequestPermissionsResponse, - cwd: &Path, - ) -> RequestPermissionsResponse { + requested_permissions: RequestPermissionProfile, + response: RequestPermissionsResponse, + cwd: &PathUri, + ) -> RequestPermissionsResponse { if response.strict_auto_review && matches!(response.scope, PermissionGrantScope::Session) { return RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), @@ -2609,7 +2581,7 @@ impl Session { } RequestPermissionsResponse { - permissions: intersect_permission_profiles( + permissions: intersect_permission_profiles_for_uri( requested_permissions.into(), response.permissions.into(), cwd, @@ -2622,7 +2594,7 @@ impl Session { async fn record_granted_request_permissions_for_turn( &self, - response: &RequestPermissionsResponse, + response: &RequestPermissionsResponse, environment_id: &str, originating_turn_state: Option<&Arc>>, ) { @@ -2633,7 +2605,7 @@ impl Session { PermissionGrantScope::Turn => { if let Some(turn_state) = originating_turn_state { let mut ts = turn_state.lock().await; - let permissions: AdditionalPermissionProfile = + let permissions: AdditionalPermissionProfile = response.permissions.clone().into(); ts.record_granted_permissions(environment_id, permissions); if response.strict_auto_review { @@ -2658,7 +2630,7 @@ impl Session { pub(crate) async fn granted_turn_permissions( &self, environment_id: &str, - ) -> Option { + ) -> Option> { let active = self.active_turn.lock().await; let active = active.as_ref()?; let ts = active.turn_state.lock().await; @@ -2681,7 +2653,7 @@ impl Session { pub(crate) async fn granted_session_permissions( &self, environment_id: &str, - ) -> Option { + ) -> Option> { let state = self.state.lock().await; state.granted_permissions(environment_id) } diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index d1e31b7994..61d4cf5429 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -15,6 +15,7 @@ use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputResponse; use codex_rmcp_client::ElicitationResponse; use codex_sandboxing::policy_transforms::merge_permission_profiles; +use codex_utils_path_uri::PathUri; use rmcp::model::RequestId; use tokio::sync::oneshot; @@ -92,7 +93,7 @@ pub(crate) struct TurnState { pending_dynamic_tools: HashMap>, pub(crate) pending_input: TurnInputQueue, mailbox_delivery_phase: MailboxDeliveryPhase, - granted_permissions_by_environment_id: HashMap, + granted_permissions_by_environment_id: HashMap>, strict_auto_review_enabled: bool, pub(crate) tool_calls: u64, pub(crate) has_memory_citation: bool, @@ -100,8 +101,8 @@ pub(crate) struct TurnState { } pub(crate) struct PendingRequestPermissions { - pub(crate) tx_response: oneshot::Sender, - pub(crate) requested_permissions: RequestPermissionProfile, + pub(crate) tx_response: oneshot::Sender>, + pub(crate) requested_permissions: RequestPermissionProfile, pub(crate) environment: TurnEnvironmentSelection, } @@ -209,7 +210,7 @@ impl TurnState { pub(crate) fn record_granted_permissions( &mut self, environment_id: &str, - permissions: AdditionalPermissionProfile, + permissions: AdditionalPermissionProfile, ) { let granted_permissions = merge_permission_profiles( self.granted_permissions_by_environment_id @@ -225,7 +226,7 @@ impl TurnState { pub(crate) fn granted_permissions( &self, environment_id: &str, - ) -> Option { + ) -> Option> { self.granted_permissions_by_environment_id .get(environment_id) .cloned() diff --git a/codex-rs/core/src/tools/handlers/request_permissions.rs b/codex-rs/core/src/tools/handlers/request_permissions.rs index 9fcb1c642c..3b38306233 100644 --- a/codex-rs/core/src/tools/handlers/request_permissions.rs +++ b/codex-rs/core/src/tools/handlers/request_permissions.rs @@ -1,5 +1,6 @@ use codex_protocol::request_permissions::RequestPermissionsArgs; -use codex_sandboxing::policy_transforms::normalize_additional_permissions; +use codex_sandboxing::policy_transforms::normalize_additional_permissions_for_uri; +use codex_sandboxing::policy_transforms::resolve_additional_permission_paths; use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; @@ -7,7 +8,6 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::handlers::parse_arguments; -use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_tool_environment; use crate::tools::handlers::shell_spec::create_request_permissions_tool; use crate::tools::handlers::shell_spec::request_permissions_tool_description; @@ -15,16 +15,9 @@ use crate::tools::registry::CoreToolRuntime; use crate::tools::registry::ToolExecutor; use codex_tools::ToolName; use codex_tools::ToolSpec; -use serde::Deserialize; pub struct RequestPermissionsHandler; -#[derive(Deserialize)] -struct RequestPermissionsEnvironmentArgs { - #[serde(default, rename = "environment_id", alias = "environmentId")] - environment_id: Option, -} - impl ToolExecutor for RequestPermissionsHandler { fn tool_name(&self) -> ToolName { ToolName::plain("request_permissions") @@ -63,34 +56,29 @@ impl RequestPermissionsHandler { } }; - let environment_args: RequestPermissionsEnvironmentArgs = parse_arguments(&arguments)?; - let Some(turn_environment) = resolve_tool_environment( - &step_context.environments, - environment_args.environment_id.as_deref(), - )? + let args: RequestPermissionsArgs = parse_arguments(&arguments)?; + let Some(turn_environment) = + resolve_tool_environment(&step_context.environments, args.environment_id.as_deref())? else { return Err(FunctionCallError::RespondToModel( "request_permissions requires a primary environment".to_string(), )); }; - // TODO(anp): Migrate request_permissions parsing and permission profiles to PathUri so - // environment-native foreign paths do not require host conversion. - let native_cwd = turn_environment.cwd().to_abs_path().map_err(|err| { - FunctionCallError::RespondToModel(format!( - "request_permissions cwd `{}` is not native to the Codex host: {err}", - turn_environment.cwd() - )) - })?; - let mut args: RequestPermissionsArgs = - parse_arguments_with_base_path(&arguments, &native_cwd)?; - args.permissions = normalize_additional_permissions(args.permissions.into()) - .map(codex_protocol::request_permissions::RequestPermissionProfile::from) - .map_err(FunctionCallError::RespondToModel)?; - if args.permissions.is_empty() { + let permissions = + resolve_additional_permission_paths(args.permissions.into(), turn_environment.cwd()) + .and_then(normalize_additional_permissions_for_uri) + .map(codex_protocol::request_permissions::RequestPermissionProfile::from) + .map_err(FunctionCallError::RespondToModel)?; + if permissions.is_empty() { return Err(FunctionCallError::RespondToModel( "request_permissions requires at least one permission".to_string(), )); } + let args = RequestPermissionsArgs { + environment_id: args.environment_id, + reason: args.reason, + permissions, + }; let response = session .request_permissions_for_environment( @@ -107,6 +95,7 @@ impl RequestPermissionsHandler { ) })?; + let response = response.map_paths(|path| path.inferred_native_path_string()); let content = serde_json::to_string(&response).map_err(|err| { FunctionCallError::Fatal(format!( "failed to serialize request_permissions response: {err}" diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index afa8e2194d..aa64365e52 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -20,6 +20,7 @@ use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; @@ -259,7 +260,7 @@ async fn wait_for_exec_approval_or_completion( async fn expect_request_permissions_event( test: &TestCodex, expected_call_id: &str, -) -> RequestPermissionProfile { +) -> RequestPermissionProfile { let event = wait_for_event(&test.codex, |event| { matches!( event, @@ -297,11 +298,15 @@ fn requested_directory_write_permissions(path: &Path) -> RequestPermissionProfil } } -fn normalized_directory_write_permissions(path: &Path) -> Result { +fn normalized_directory_write_permissions( + path: &Path, +) -> Result> { Ok(RequestPermissionProfile { file_system: Some(FileSystemPermissions::from_read_write_roots( Some(vec![]), - Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]), + Some(vec![PathUri::from_abs_path(&AbsolutePathBuf::try_from( + path.canonicalize()?, + )?)]), )), ..RequestPermissionProfile::default() }) @@ -372,7 +377,7 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res let approval = expect_exec_approval(&test, command).await; assert_eq!( approval.additional_permissions, - Some(requested_permissions.clone()) + Some(requested_permissions.clone().into()) ); test.codex .submit(Op::ExecApproval { @@ -594,7 +599,7 @@ async fn relative_additional_permissions_resolve_against_tool_workdir( let approval = expect_exec_approval(&test, command).await; assert_eq!( approval.additional_permissions, - Some(expected_permissions.clone()) + Some(expected_permissions.clone().into()) ); test.codex .submit(Op::ExecApproval { @@ -875,9 +880,9 @@ async fn workspace_write_with_additional_permissions_can_write_outside_cwd() -> let normalized_requested_permissions = RequestPermissionProfile { file_system: Some(FileSystemPermissions::from_read_write_roots( Some(vec![]), - Some(vec![AbsolutePathBuf::try_from( + Some(vec![PathUri::from_abs_path(&AbsolutePathBuf::try_from( outside_dir.path().canonicalize()?, - )?]), + )?)]), )), ..RequestPermissionProfile::default() }; @@ -1011,7 +1016,7 @@ async fn with_additional_permissions_denied_approval_blocks_execution() -> Resul let approval = expect_exec_approval(&test, &command).await; assert_eq!( approval.additional_permissions, - Some(normalized_requested_permissions) + Some(normalized_requested_permissions.into()) ); test.codex .submit(Op::ExecApproval { @@ -1085,9 +1090,9 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul let normalized_requested_permissions = RequestPermissionProfile { file_system: Some(FileSystemPermissions::from_read_write_roots( Some(vec![]), - Some(vec![AbsolutePathBuf::try_from( + Some(vec![PathUri::from_abs_path(&AbsolutePathBuf::try_from( outside_dir.path().canonicalize()?, - )?]), + )?)]), )), ..Default::default() }; @@ -1569,8 +1574,12 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() file_system: Some(FileSystemPermissions::from_read_write_roots( Some(vec![]), Some(vec![ - AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?, - AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?, + PathUri::from_abs_path(&AbsolutePathBuf::try_from( + first_dir.path().canonicalize()?, + )?), + PathUri::from_abs_path(&AbsolutePathBuf::try_from( + second_dir.path().canonicalize()?, + )?), ]), )), ..RequestPermissionProfile::default() @@ -1581,8 +1590,12 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() file_system: Some(FileSystemPermissions::from_read_write_roots( Some(vec![]), Some(vec![ - AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?, - AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?, + PathUri::from_abs_path(&AbsolutePathBuf::try_from( + first_dir.path().canonicalize()?, + )?), + PathUri::from_abs_path(&AbsolutePathBuf::try_from( + second_dir.path().canonicalize()?, + )?), ]), )), ..Default::default() @@ -1655,7 +1668,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() assert!(approval_reads.as_ref().is_none_or(Vec::is_empty)); let mut approval_writes = approval_writes.unwrap_or_default(); - approval_writes.sort_by_key(|path| path.display().to_string()); + approval_writes.sort_by_key(ToString::to_string); let (_, expected_writes) = merged_permissions .file_system @@ -1663,7 +1676,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() .legacy_read_write_roots() .expect("expected legacy-compatible permissions"); let mut expected_writes = expected_writes.unwrap_or_default(); - expected_writes.sort_by_key(|path| path.display().to_string()); + expected_writes.sort_by_key(ToString::to_string); assert_eq!(approval_writes, expected_writes); test.codex diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 5cf4f27f18..6a8fc8e49e 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -1,11 +1,17 @@ use crate::mcp::RequestId; use crate::models::AdditionalPermissionProfile; +use crate::models::FileSystemPermissions; use crate::models::PermissionProfile; use crate::parse_command::ParsedCommand; +use crate::permissions::FileSystemPath; +use crate::permissions::FileSystemSandboxEntry; use crate::protocol::FileChange; use crate::protocol::ReviewDecision; use crate::request_permissions::RequestPermissionProfile; -use codex_utils_absolute_path::AbsolutePathBuf; +use crate::request_permissions::deserialize_request_permission_profile_from_native_paths; +use crate::request_permissions::serialize_request_permission_profile_as_native_paths; +use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathUri; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; @@ -138,17 +144,33 @@ pub enum GuardianAssessmentAction { Command { source: GuardianCommandSource, command: String, - cwd: AbsolutePathBuf, + #[serde( + serialize_with = "serialize_path_uri_as_native_path", + deserialize_with = "deserialize_path_uri_from_native_path" + )] + cwd: PathUri, }, Execve { source: GuardianCommandSource, program: String, argv: Vec, - cwd: AbsolutePathBuf, + #[serde( + serialize_with = "serialize_path_uri_as_native_path", + deserialize_with = "deserialize_path_uri_from_native_path" + )] + cwd: PathUri, }, ApplyPatch { - cwd: AbsolutePathBuf, - files: Vec, + #[serde( + serialize_with = "serialize_path_uri_as_native_path", + deserialize_with = "deserialize_path_uri_from_native_path" + )] + cwd: PathUri, + #[serde( + serialize_with = "serialize_path_uris_as_native_paths", + deserialize_with = "deserialize_path_uris_from_native_paths" + )] + files: Vec, }, NetworkAccess { target: String, @@ -165,10 +187,119 @@ pub enum GuardianAssessmentAction { }, RequestPermissions { reason: Option, - permissions: RequestPermissionProfile, + #[serde( + serialize_with = "serialize_request_permission_profile_as_native_paths", + deserialize_with = "deserialize_request_permission_profile_from_native_paths" + )] + permissions: RequestPermissionProfile, }, } +fn serialize_path_uri_as_native_path(path: &PathUri, serializer: S) -> Result +where + S: serde::Serializer, +{ + serializer.serialize_str(&path.inferred_native_path_string()) +} + +fn deserialize_path_uri_from_native_path<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let path = LegacyAppPathString::deserialize(deserializer)?; + PathUri::try_from(path).map_err(serde::de::Error::custom) +} + +fn serialize_path_uris_as_native_paths( + paths: &[PathUri], + serializer: S, +) -> Result +where + S: serde::Serializer, +{ + paths + .iter() + .map(PathUri::inferred_native_path_string) + .collect::>() + .serialize(serializer) +} + +fn deserialize_path_uris_from_native_paths<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + Vec::::deserialize(deserializer)? + .into_iter() + .map(|path| PathUri::try_from(path).map_err(serde::de::Error::custom)) + .collect() +} + +fn serialize_optional_additional_permissions_as_native_paths( + permissions: &Option>, + serializer: S, +) -> Result +where + S: serde::Serializer, +{ + permissions + .clone() + .map(|permissions| permissions.map_paths(|path| path.inferred_native_path_string())) + .serialize(serializer) +} + +fn deserialize_optional_additional_permissions_from_native_paths<'de, D>( + deserializer: D, +) -> Result>, D::Error> +where + D: serde::Deserializer<'de>, +{ + Option::>::deserialize(deserializer)? + .map(|permissions| { + try_map_additional_permission_paths(permissions, |path| { + PathUri::try_from(path).map_err(serde::de::Error::custom) + }) + }) + .transpose() +} + +fn try_map_additional_permission_paths( + permissions: AdditionalPermissionProfile, + mut map: impl FnMut(InputPath) -> Result, +) -> Result, Error> { + let file_system = permissions + .file_system + .map(|file_system| { + let entries = file_system + .entries + .into_iter() + .map(|entry| { + let path = match entry.path { + FileSystemPath::Path { path } => FileSystemPath::Path { path: map(path)? }, + FileSystemPath::GlobPattern { pattern } => { + FileSystemPath::GlobPattern { pattern } + } + FileSystemPath::Special { value } => FileSystemPath::Special { value }, + }; + Ok(FileSystemSandboxEntry { + path, + access: entry.access, + }) + }) + .collect::, Error>>()?; + Ok(FileSystemPermissions { + entries, + glob_scan_max_depth: file_system.glob_scan_max_depth, + }) + }) + .transpose()?; + Ok(AdditionalPermissionProfile { + network: permissions.network, + file_system, + }) +} + #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] pub struct NetworkPolicyAmendment { pub host: String, @@ -244,7 +375,11 @@ pub struct ExecApprovalRequestEvent { /// The command to be executed. pub command: Vec, /// The command's working directory. - pub cwd: AbsolutePathBuf, + #[serde( + serialize_with = "serialize_path_uri_as_native_path", + deserialize_with = "deserialize_path_uri_from_native_path" + )] + pub cwd: PathUri, /// Optional human-readable reason for the approval (e.g. retry without sandbox). #[serde(skip_serializing_if = "Option::is_none")] pub reason: Option, @@ -261,9 +396,14 @@ pub struct ExecApprovalRequestEvent { #[ts(optional)] pub proposed_network_policy_amendments: Option>, /// Optional additional filesystem permissions requested for this command. - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde( + default, + skip_serializing_if = "Option::is_none", + serialize_with = "serialize_optional_additional_permissions_as_native_paths", + deserialize_with = "deserialize_optional_additional_permissions_from_native_paths" + )] #[ts(optional)] - pub additional_permissions: Option, + pub additional_permissions: Option>, /// Ordered list of decisions the client may present for this prompt. /// /// When absent, clients should derive the legacy default set from the @@ -299,7 +439,7 @@ impl ExecApprovalRequestEvent { network_approval_context: Option<&NetworkApprovalContext>, proposed_execpolicy_amendment: Option<&ExecPolicyAmendment>, proposed_network_policy_amendments: Option<&[NetworkPolicyAmendment]>, - additional_permissions: Option<&AdditionalPermissionProfile>, + additional_permissions: Option<&AdditionalPermissionProfile>, ) -> Vec { if network_approval_context.is_some() { let mut decisions = vec![ReviewDecision::Approved, ReviewDecision::ApprovedForSession]; @@ -432,7 +572,7 @@ mod tests { GuardianAssessmentAction::Command { source: GuardianCommandSource::Shell, command: "rm -rf /tmp/guardian".to_string(), - cwd: test_path_buf("/tmp").abs(), + cwd: test_path_buf("/tmp").abs().into(), } ); } @@ -465,7 +605,7 @@ mod tests { "-f".to_string(), "/tmp/file.sqlite".to_string(), ], - cwd: test_path_buf("/tmp").abs(), + cwd: test_path_buf("/tmp").abs().into(), } ); } diff --git a/codex-rs/protocol/src/request_permissions.rs b/codex-rs/protocol/src/request_permissions.rs index bf80d6d647..827aa7aea2 100644 --- a/codex-rs/protocol/src/request_permissions.rs +++ b/codex-rs/protocol/src/request_permissions.rs @@ -1,7 +1,11 @@ use crate::models::AdditionalPermissionProfile; use crate::models::FileSystemPermissions; use crate::models::NetworkPermissions; +use crate::permissions::FileSystemPath; +use crate::permissions::FileSystemSandboxEntry; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathUri; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; @@ -139,8 +143,110 @@ pub struct RequestPermissionsEvent { pub started_at_ms: i64, #[serde(skip_serializing_if = "Option::is_none")] pub reason: Option, - pub permissions: RequestPermissionProfile, - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde( + serialize_with = "serialize_request_permission_profile_as_native_paths", + deserialize_with = "deserialize_request_permission_profile_from_native_paths" + )] + pub permissions: RequestPermissionProfile, + #[serde( + default, + skip_serializing_if = "Option::is_none", + serialize_with = "serialize_optional_path_uri_as_native_path", + deserialize_with = "deserialize_optional_path_uri_from_native_path" + )] #[ts(optional)] - pub cwd: Option, + pub cwd: Option, +} + +pub(crate) fn serialize_request_permission_profile_as_native_paths( + permissions: &RequestPermissionProfile, + serializer: S, +) -> Result +where + S: serde::Serializer, +{ + permissions + .clone() + .map_paths(|path| path.inferred_native_path_string()) + .serialize(serializer) +} + +pub(crate) fn deserialize_request_permission_profile_from_native_paths<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + let permissions = RequestPermissionProfile::::deserialize(deserializer)?; + try_map_request_permission_profile_paths(permissions, path_uri_from_legacy_app_path) + .map_err(serde::de::Error::custom) +} + +fn serialize_optional_path_uri_as_native_path( + path: &Option, + serializer: S, +) -> Result +where + S: serde::Serializer, +{ + path.as_ref() + .map(PathUri::inferred_native_path_string) + .serialize(serializer) +} + +fn deserialize_optional_path_uri_from_native_path<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + Option::::deserialize(deserializer)? + .map(path_uri_from_legacy_app_path) + .transpose() + .map_err(serde::de::Error::custom) +} + +fn path_uri_from_legacy_app_path( + path: LegacyAppPathString, +) -> Result { + if let Ok(path_uri) = PathUri::parse(path.as_str()) { + return Ok(path_uri); + } + path.try_into() +} + +fn try_map_request_permission_profile_paths( + permissions: RequestPermissionProfile, + mut map: impl FnMut(InputPath) -> Result, +) -> Result, Error> { + let file_system = permissions + .file_system + .map(|file_system| { + let entries = file_system + .entries + .into_iter() + .map(|entry| { + let path = match entry.path { + FileSystemPath::Path { path } => FileSystemPath::Path { path: map(path)? }, + FileSystemPath::GlobPattern { pattern } => { + FileSystemPath::GlobPattern { pattern } + } + FileSystemPath::Special { value } => FileSystemPath::Special { value }, + }; + Ok(FileSystemSandboxEntry { + path, + access: entry.access, + }) + }) + .collect::, Error>>()?; + Ok(FileSystemPermissions { + entries, + glob_scan_max_depth: file_system.glob_scan_max_depth, + }) + }) + .transpose()?; + Ok(RequestPermissionProfile { + network: permissions.network, + file_system, + }) } diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 0b72897685..3f0d45b3df 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -3,16 +3,13 @@ use std::collections::VecDeque; use super::App; use crate::app_command::AppCommand; -use crate::app_server_approval_conversions::granted_permission_profile_from_request; use crate::app_server_session::AppServerSession; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::FileChangeRequestApprovalResponse; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::McpServerElicitationRequestResponse; -use codex_app_server_protocol::PermissionsRequestApprovalResponse; use codex_app_server_protocol::RequestId as AppServerRequestId; use codex_app_server_protocol::ServerRequest; -use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; impl App { pub(super) async fn reject_app_server_request( @@ -104,18 +101,6 @@ impl PendingAppServerRequests { None } ServerRequest::PermissionsRequestApproval { request_id, params } => { - // TODO(anp): Remove this duplicate validation once core permission paths remain - // PathUri after crossing the app-server boundary. Native permission paths do not - // yet have an ingress validation step, so validate them here before recording the - // request as pending. Discovering an invalid path later in a UI delivery path - // would leave the app-server RPC waiting without a clean rejection path. - if let Err(err) = CoreRequestPermissionProfile::try_from(params.permissions.clone()) - { - return Some(UnsupportedAppServerRequest { - request_id: request_id.clone(), - message: format!("failed to localize requested filesystem paths: {err}"), - }); - } self.permissions_approvals .insert(params.item_id.clone(), request_id.clone()); None @@ -223,14 +208,7 @@ impl PendingAppServerRequests { .map(|request_id| { Ok::(AppServerRequestResolution { request_id, - result: serde_json::to_value(PermissionsRequestApprovalResponse { - permissions: granted_permission_profile_from_request( - response.permissions.clone(), - ), - scope: response.scope.into(), - strict_auto_review: response.strict_auto_review.then_some(true), - }) - .map_err(|err| { + result: serde_json::to_value(response).map_err(|err| { format!("failed to serialize permissions approval response: {err}") })?, }) @@ -417,7 +395,6 @@ struct McpRequestKey { mod tests { use super::PendingAppServerRequests; use super::ResolvedAppServerRequest; - use super::UnsupportedAppServerRequest; use crate::app_command::AppCommand as Op; use codex_app_server_protocol::AdditionalFileSystemPermissions; use codex_app_server_protocol::AdditionalNetworkPermissions; @@ -438,9 +415,6 @@ mod tests { use codex_app_server_protocol::ToolRequestUserInputAnswer; use codex_app_server_protocol::ToolRequestUserInputParams; use codex_app_server_protocol::ToolRequestUserInputResponse; - use codex_protocol::models::FileSystemPermissions; - use codex_protocol::models::NetworkPermissions; - use codex_protocol::request_permissions::RequestPermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::json; @@ -488,9 +462,8 @@ mod tests { } #[test] - fn rejects_permissions_with_paths_that_cannot_be_localized() { + fn preserves_permissions_with_relative_target_paths() { let mut pending = PendingAppServerRequests::default(); - let request_id = AppServerRequestId::Integer(7); let permissions = codex_app_server_protocol::RequestPermissionProfile { network: None, file_system: Some(AdditionalFileSystemPermissions { @@ -503,36 +476,28 @@ mod tests { entries: None, }), }; - let localization_error = - RequestPermissionProfile::try_from(permissions.clone()).expect_err("relative path"); let cwd = AbsolutePathBuf::try_from(PathBuf::from(if cfg!(windows) { r"C:\tmp" } else { "/tmp" })) .expect("path must be absolute"); + let request = ServerRequest::PermissionsRequestApproval { + request_id: AppServerRequestId::Integer(7), + params: PermissionsRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "perm-1".to_string(), + environment_id: None, + started_at_ms: 0, + cwd: cwd.into(), + reason: None, + permissions, + }, + }; - assert_eq!( - pending.note_server_request(&ServerRequest::PermissionsRequestApproval { - request_id: request_id.clone(), - params: PermissionsRequestApprovalParams { - thread_id: "thread-1".to_string(), - turn_id: "turn-1".to_string(), - item_id: "perm-1".to_string(), - environment_id: None, - started_at_ms: 0, - cwd, - reason: None, - permissions, - }, - }), - Some(UnsupportedAppServerRequest { - request_id, - message: format!( - "failed to localize requested filesystem paths: {localization_error}" - ), - }) - ); + assert_eq!(pending.note_server_request(&request), None); + assert!(pending.contains_server_request(&request)); } #[test] @@ -561,7 +526,7 @@ mod tests { item_id: "perm-1".to_string(), environment_id: None, started_at_ms: 0, - cwd: absolute_path(if cfg!(windows) { r"C:\tmp" } else { "/tmp" }), + cwd: absolute_path(if cfg!(windows) { r"C:\tmp" } else { "/tmp" }).into(), reason: None, permissions: serde_json::from_value(json!({ "network": { "enabled": null } @@ -588,18 +553,20 @@ mod tests { let permissions = pending .take_resolution(&Op::RequestPermissionsResponse { id: "perm-1".to_string(), - response: codex_protocol::request_permissions::RequestPermissionsResponse { - permissions: RequestPermissionProfile { - network: Some(NetworkPermissions { + response: PermissionsRequestApprovalResponse { + permissions: codex_app_server_protocol::GrantedPermissionProfile { + network: Some(AdditionalNetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path(read_path)]), - Some(vec![absolute_path(write_path)]), - )), + file_system: Some(AdditionalFileSystemPermissions { + read: Some(vec![absolute_path(read_path).into()]), + write: Some(vec![absolute_path(write_path).into()]), + glob_scan_max_depth: None, + entries: None, + }), }, - scope: codex_protocol::request_permissions::PermissionGrantScope::Session, - strict_auto_review: false, + scope: PermissionGrantScope::Session, + strict_auto_review: None, }, }) .expect("permissions response should serialize") @@ -617,20 +584,7 @@ mod tests { read: Some(vec![absolute_path(read_path).into()]), write: Some(vec![absolute_path(write_path).into()]), glob_scan_max_depth: None, - entries: Some(vec![ - codex_app_server_protocol::FileSystemSandboxEntry { - path: codex_app_server_protocol::FileSystemPath::Path { - path: absolute_path(read_path).into(), - }, - access: codex_app_server_protocol::FileSystemAccessMode::Read, - }, - codex_app_server_protocol::FileSystemSandboxEntry { - path: codex_app_server_protocol::FileSystemPath::Path { - path: absolute_path(write_path).into(), - }, - access: codex_app_server_protocol::FileSystemAccessMode::Write, - }, - ]), + entries: None, }), }, scope: PermissionGrantScope::Session, diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index d8d63575bb..de8fdf0289 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -26,6 +26,7 @@ use crate::history_cell::UserHistoryCell; use crate::history_cell::new_session_info; use crate::multi_agents::AgentPickerThreadEntry; use crate::multi_agents::SubAgentActivityDisplay; +use crate::session_state::LocalWorkspaceState; use assert_matches::assert_matches; use crate::app_command::AppCommand as Op; @@ -91,11 +92,8 @@ use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::ServiceTier; use codex_protocol::config_types::Settings; use codex_protocol::models::ActivePermissionProfile; -use codex_protocol::models::FileSystemPermissions; -use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS; -use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::user_input::TextElement; use codex_utils_absolute_path::AbsolutePathBuf; use crossterm::event::KeyModifiers; @@ -2241,7 +2239,7 @@ async fn inactive_thread_approval_bubbles_into_active_view() -> Result<()> { /*capacity*/ 1, ThreadSessionState { approval_policy: AskForApproval::OnRequest, - permission_profile: PermissionProfile::workspace_write(), + permission_profile: PermissionProfile::workspace_write().into(), rollout_path: Some(test_path_buf("/tmp/agent-rollout.jsonl")), ..test_thread_session(agent_thread_id, test_path_buf("/tmp/agent")) }, @@ -2400,7 +2398,7 @@ async fn side_defers_subagent_approval_overlay_until_side_exits() -> Result<()> /*capacity*/ 4, ThreadSessionState { approval_policy: AskForApproval::OnRequest, - permission_profile: PermissionProfile::workspace_write(), + permission_profile: PermissionProfile::workspace_write().into(), rollout_path: Some(test_path_buf("/tmp/agent-rollout.jsonl")), ..test_thread_session(agent_thread_id, test_path_buf("/tmp/agent")) }, @@ -2643,7 +2641,7 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions( item_id: "call-approval".to_string(), environment_id: Some("remote".to_string()), started_at_ms: 0, - cwd: test_absolute_path("/tmp"), + cwd: test_absolute_path("/tmp").into(), reason: Some("Need access to .git".to_string()), permissions: codex_app_server_protocol::RequestPermissionProfile { network: Some(AdditionalNetworkPermissions { @@ -2674,14 +2672,16 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions( assert_eq!(environment_id.as_deref(), Some("remote")); assert_eq!( permissions, - RequestPermissionProfile { - network: Some(NetworkPermissions { + codex_app_server_protocol::RequestPermissionProfile { + network: Some(AdditionalNetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![test_absolute_path("/tmp/read-only")]), - Some(vec![test_absolute_path("/tmp/write")]), - )), + file_system: Some(AdditionalFileSystemPermissions { + read: Some(vec![test_absolute_path("/tmp/read-only").into()]), + write: Some(vec![test_absolute_path("/tmp/write").into()]), + glob_scan_max_depth: None, + entries: None, + }), } ); } @@ -2784,7 +2784,7 @@ async fn inactive_thread_approval_badge_clears_after_turn_completion_notificatio /*capacity*/ 4, ThreadSessionState { approval_policy: AskForApproval::OnRequest, - permission_profile: PermissionProfile::workspace_write(), + permission_profile: PermissionProfile::workspace_write().into(), rollout_path: Some(test_path_buf("/tmp/agent-rollout.jsonl")), ..test_thread_session(agent_thread_id, test_path_buf("/tmp/agent")) }, @@ -2839,8 +2839,8 @@ async fn inactive_thread_started_notification_initializes_replay_session() -> Re let shared_root = test_path_buf("/tmp/shared").abs(); let primary_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, - permission_profile: PermissionProfile::workspace_write(), - runtime_workspace_roots: vec![primary_cwd.clone(), shared_root.clone()], + permission_profile: PermissionProfile::workspace_write().into(), + runtime_workspace_roots: vec![primary_cwd.clone().into(), shared_root.clone().into()], ..test_thread_session(main_thread_id, primary_cwd.to_path_buf()) }; @@ -2886,8 +2886,10 @@ async fn inactive_thread_started_notification_initializes_replay_session() -> Re updated_at: 2, recency_at: Some(2), status: codex_app_server_protocol::ThreadStatus::Idle, - path: Some(rollout_path.clone()), - cwd: test_path_buf("/tmp/agent").abs(), + path: Some(codex_app_server_protocol::LegacyAppPathString::from_path( + &rollout_path, + )), + cwd: test_path_buf("/tmp/agent").abs().into(), cli_version: "0.0.0".to_string(), source: codex_app_server_protocol::SessionSource::Unknown, thread_source: None, @@ -2916,10 +2918,10 @@ async fn inactive_thread_started_notification_initializes_replay_session() -> Re assert_eq!(session.model, "gpt-agent"); assert_eq!(session.model_provider_id, "agent-provider"); assert_eq!(session.approval_policy, primary_session.approval_policy); - assert_eq!(session.cwd.as_path(), test_path_buf("/tmp/agent").as_path()); + assert_eq!(session.cwd, test_path_buf("/tmp/agent").abs().into()); assert_eq!( session.runtime_workspace_roots, - vec![test_path_buf("/tmp/agent").abs(), shared_root] + vec![test_path_buf("/tmp/agent").abs().into(), shared_root.into()] ); assert_eq!(session.rollout_path, Some(rollout_path)); assert_eq!( @@ -2947,8 +2949,8 @@ async fn inactive_thread_started_notification_preserves_primary_model_when_path_ let primary_cwd = test_path_buf("/tmp/main").abs(); let primary_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, - permission_profile: PermissionProfile::workspace_write(), - runtime_workspace_roots: vec![primary_cwd.clone()], + permission_profile: PermissionProfile::workspace_write().into(), + runtime_workspace_roots: vec![primary_cwd.clone().into()], ..test_thread_session(main_thread_id, primary_cwd.to_path_buf()) }; @@ -2982,7 +2984,7 @@ async fn inactive_thread_started_notification_preserves_primary_model_when_path_ recency_at: Some(2), status: codex_app_server_protocol::ThreadStatus::Idle, path: None, - cwd: test_path_buf("/tmp/agent").abs(), + cwd: test_path_buf("/tmp/agent").abs().into(), cli_version: "0.0.0".to_string(), source: codex_app_server_protocol::SessionSource::Unknown, thread_source: None, @@ -3023,8 +3025,8 @@ async fn thread_read_session_state_does_not_reuse_primary_permission_profile() { let primary_cwd = test_path_buf("/tmp/main").abs(); let primary_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, - permission_profile: PermissionProfile::workspace_write(), - runtime_workspace_roots: vec![primary_cwd.clone()], + permission_profile: PermissionProfile::workspace_write().into(), + runtime_workspace_roots: vec![primary_cwd.clone().into()], ..test_thread_session(main_thread_id, primary_cwd.to_path_buf()) }; app.primary_session_configured = Some(primary_session); @@ -3044,7 +3046,7 @@ async fn thread_read_session_state_does_not_reuse_primary_permission_profile() { recency_at: Some(2), status: codex_app_server_protocol::ThreadStatus::Idle, path: None, - cwd: test_path_buf("/tmp/read").abs(), + cwd: test_path_buf("/tmp/read").abs().into(), cli_version: "0.0.0".to_string(), source: codex_app_server_protocol::SessionSource::Unknown, thread_source: None, @@ -3060,10 +3062,10 @@ async fn thread_read_session_state_does_not_reuse_primary_permission_profile() { .await; assert_eq!(session.thread_id, read_thread_id); - assert_eq!(session.cwd.as_path(), test_path_buf("/tmp/read").as_path()); + assert_eq!(session.cwd, test_path_buf("/tmp/read").abs().into()); assert_eq!( session.runtime_workspace_roots, - vec![test_path_buf("/tmp/read").abs()] + vec![test_path_buf("/tmp/read").abs().into()] ); let expected_permission_profile = app .chat_widget @@ -3072,7 +3074,8 @@ async fn thread_read_session_state_does_not_reuse_primary_permission_profile() { .permission_profile() .clone(); assert_eq!( - session.permission_profile, expected_permission_profile, + session.permission_profile, + expected_permission_profile.into(), "thread/read does not return fresh server permissions; the fallback profile must use the \ active widget permissions rather than reusing the cached primary session profile" ); @@ -3930,9 +3933,9 @@ async fn render_clear_ui_header_after_long_transcript_for_snapshot() -> String { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, - permission_profile: PermissionProfile::read_only(), + permission_profile: PermissionProfile::read_only().into(), active_permission_profile: None, - cwd: test_path_buf("/tmp/project").abs(), + cwd: test_path_buf("/tmp/project").abs().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), reasoning_effort: Some(ReasoningEffortConfig::High), @@ -3940,6 +3943,7 @@ async fn render_clear_ui_header_after_long_transcript_for_snapshot() -> String { personality: None, message_history: None, network_proxy: None, + local_workspace: None, rollout_path: Some(PathBuf::new()), }; Arc::new(new_session_info( @@ -4395,6 +4399,7 @@ async fn replace_goal_confirmation_snapshot() { } fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState { + let cwd = cwd.abs(); ThreadSessionState { thread_id, forked_from_id: None, @@ -4405,9 +4410,9 @@ fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, - permission_profile: PermissionProfile::read_only(), + permission_profile: PermissionProfile::read_only().into(), active_permission_profile: None, - cwd: cwd.abs(), + cwd: cwd.clone().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -4415,6 +4420,10 @@ fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState personality: None, message_history: None, network_proxy: None, + local_workspace: Some(LocalWorkspaceState { + cwd, + runtime_workspace_roots: Vec::new(), + }), rollout_path: Some(PathBuf::new()), } } @@ -5019,9 +5028,9 @@ async fn backtrack_selection_with_duplicate_history_targets_unique_turn() { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, - permission_profile: PermissionProfile::read_only(), + permission_profile: PermissionProfile::read_only().into(), active_permission_profile: None, - cwd: test_path_buf("/home/user/project").abs(), + cwd: test_path_buf("/home/user/project").abs().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -5029,6 +5038,7 @@ async fn backtrack_selection_with_duplicate_history_targets_unique_turn() { personality: None, message_history: None, network_proxy: None, + local_workspace: None, rollout_path: Some(PathBuf::new()), }; Arc::new(new_session_info( @@ -5085,9 +5095,9 @@ async fn backtrack_selection_with_duplicate_history_targets_unique_turn() { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, - permission_profile: PermissionProfile::read_only(), + permission_profile: PermissionProfile::read_only().into(), active_permission_profile: None, - cwd: test_path_buf("/home/user/project").abs(), + cwd: test_path_buf("/home/user/project").abs().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -5095,6 +5105,7 @@ async fn backtrack_selection_with_duplicate_history_targets_unique_turn() { personality: None, message_history: None, network_proxy: None, + local_workspace: None, rollout_path: Some(PathBuf::new()), }); @@ -5235,9 +5246,9 @@ async fn backtrack_resubmit_preserves_data_image_urls_in_user_turn() { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, - permission_profile: PermissionProfile::read_only(), + permission_profile: PermissionProfile::read_only().into(), active_permission_profile: None, - cwd: test_path_buf("/home/user/project").abs(), + cwd: test_path_buf("/home/user/project").abs().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -5245,6 +5256,7 @@ async fn backtrack_resubmit_preserves_data_image_urls_in_user_turn() { personality: None, message_history: None, network_proxy: None, + local_workspace: None, rollout_path: Some(PathBuf::new()), }); @@ -5478,7 +5490,7 @@ async fn refreshed_snapshot_session_persists_resumed_turns() { }], )]; let resumed_session = ThreadSessionState { - cwd: test_path_buf("/tmp/refreshed").abs(), + cwd: test_path_buf("/tmp/refreshed").abs().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), ..initial_session.clone() @@ -5662,7 +5674,7 @@ async fn thread_rollback_response_discards_queued_active_thread_events() { recency_at: Some(0), status: codex_app_server_protocol::ThreadStatus::Idle, path: None, - cwd: test_path_buf("/tmp/project").abs(), + cwd: test_path_buf("/tmp/project").abs().into(), cli_version: "0.0.0".to_string(), source: SessionSource::Cli, thread_source: None, @@ -5699,9 +5711,9 @@ async fn new_session_requests_shutdown_for_previous_conversation() { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, - permission_profile: PermissionProfile::read_only(), + permission_profile: PermissionProfile::read_only().into(), active_permission_profile: None, - cwd: test_path_buf("/home/user/project").abs(), + cwd: test_path_buf("/home/user/project").abs().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -5709,6 +5721,7 @@ async fn new_session_requests_shutdown_for_previous_conversation() { personality: None, message_history: None, network_proxy: None, + local_workspace: None, rollout_path: Some(PathBuf::new()), }; @@ -6035,7 +6048,7 @@ async fn inactive_thread_settings_notification_updates_cached_collaboration_mode let notification = ThreadSettingsUpdatedNotification { thread_id: inactive_thread_id.to_string(), thread_settings: ThreadSettings { - cwd: test_absolute_path("/tmp/thread-settings"), + cwd: test_absolute_path("/tmp/thread-settings").into(), approval_policy: AskForApproval::OnRequest, approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::AutoReview, sandbox_policy: codex_app_server_protocol::SandboxPolicy::ReadOnly { @@ -6113,9 +6126,9 @@ async fn clear_only_ui_reset_preserves_chat_session_state() { service_tier: None, approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, - permission_profile: PermissionProfile::read_only(), + permission_profile: PermissionProfile::read_only().into(), active_permission_profile: None, - cwd: test_path_buf("/tmp/project").abs(), + cwd: test_path_buf("/tmp/project").abs().into(), runtime_workspace_roots: Vec::new(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -6123,6 +6136,7 @@ async fn clear_only_ui_reset_preserves_chat_session_state() { personality: None, message_history: None, network_proxy: None, + local_workspace: None, rollout_path: Some(PathBuf::new()), }); app.chat_widget @@ -6162,7 +6176,9 @@ async fn clear_only_ui_reset_preserves_chat_session_state() { async fn clear_only_ui_reset_allows_active_skill_warning_to_render_again() { let mut app = make_test_app().await; let error = SkillErrorInfo { - path: test_path_buf("/tmp/project/.codex/skills/abc/SKILL.md"), + path: codex_utils_path_uri::LegacyAppPathString::from_path(&test_path_buf( + "/tmp/project/.codex/skills/abc/SKILL.md", + )), message: "invalid description".to_string(), }; diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index b149fd53f3..6ed952a0d1 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -5,7 +5,9 @@ //! when the visible thread changes. use super::*; +use crate::app_server_session::local_rollout_path; use crate::session_resume::read_session_model; +use codex_utils_path_uri::PathUri; #[derive(Clone, Copy)] pub(super) enum ThreadRollbackOrigin { @@ -196,6 +198,17 @@ impl App { } pub(super) async fn thread_cwd(&self, thread_id: ThreadId) -> Option { + let channel = self.thread_event_channels.get(&thread_id)?; + let store = channel.store.lock().await; + store + .session + .as_ref()? + .local_workspace + .as_ref() + .map(|workspace| workspace.cwd.clone()) + } + + async fn thread_target_cwd(&self, thread_id: ThreadId) -> Option { let channel = self.thread_event_channels.get(&thread_id)?; let store = channel.store.lock().await; store.session.as_ref().map(|session| session.cwd.clone()) @@ -315,26 +328,16 @@ impl App { } } } - ServerRequest::PermissionsRequestApproval { params, .. } => { - // TODO(anp): Remove this native-path localization error path once core permission - // paths remain PathUri after crossing the app-server boundary. - let permissions = params.permissions.clone().try_into().map_err(|err| { - std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!("failed to localize requested filesystem paths: {err}"), - ) - })?; - Some(ThreadInteractiveRequest::Approval( - ApprovalRequest::Permissions { - thread_id, - thread_label, - call_id: params.item_id.clone(), - environment_id: params.environment_id.clone(), - reason: params.reason.clone(), - permissions, - }, - )) - } + ServerRequest::PermissionsRequestApproval { params, .. } => Some( + ThreadInteractiveRequest::Approval(ApprovalRequest::Permissions { + thread_id, + thread_label, + call_id: params.item_id.clone(), + environment_id: params.environment_id.clone(), + reason: params.reason.clone(), + permissions: params.permissions.clone(), + }), + ), _ => None, }) } @@ -639,6 +642,10 @@ impl App { } } if should_start_turn { + let target_cwd = self.thread_target_cwd(thread_id).await.unwrap_or_else(|| { + PathUri::from_host_native_path(cwd) + .unwrap_or_else(|_| PathUri::from_abs_path(&self.config.cwd)) + }); let config = self.chat_widget.config_ref(); let approvals_reviewer = approvals_reviewer.unwrap_or(config.approvals_reviewer); @@ -653,7 +660,8 @@ impl App { .turn_start( thread_id, items.to_vec(), - cwd.clone(), + target_cwd, + config.cwd.as_path(), *approval_policy, approvals_reviewer, permissions_override, @@ -680,7 +688,12 @@ impl App { self.handle_skills_list_result( app_server .skills_list(codex_app_server_protocol::SkillsListParams { - cwds: cwds.clone(), + cwds: cwds + .iter() + .map(|cwd| { + codex_utils_path_uri::LegacyAppPathString::from_path(cwd) + }) + .collect(), force_reload: *force_reload, }) .await, @@ -992,9 +1005,18 @@ impl App { session.thread_id = thread_id; session.thread_name = notification.thread.name.clone(); session.model_provider_id = notification.thread.model_provider.clone(); - session - .set_cwd_retargeting_implicit_runtime_workspace_root(notification.thread.cwd.clone()); - let rollout_path = notification.thread.path.clone(); + if let Some(cwd) = notification.thread.cwd.to_inferred_path_uri() { + session.set_cwd_retargeting_implicit_runtime_workspace_root(cwd); + } else { + tracing::warn!( + cwd = %notification.thread.cwd, + "ignoring non-absolute cwd in thread/started notification" + ); + } + let rollout_path = local_rollout_path( + notification.thread.path.as_ref(), + self.app_server_target.thread_params_mode(), + ); if let Some(model) = read_session_model(self.state_db.as_deref(), thread_id, rollout_path.as_deref()).await { diff --git a/codex-rs/tui/src/app_command.rs b/codex-rs/tui/src/app_command.rs index 17587231a5..1a934b7a92 100644 --- a/codex-rs/tui/src/app_command.rs +++ b/codex-rs/tui/src/app_command.rs @@ -4,6 +4,7 @@ use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::McpServerElicitationAction; +use codex_app_server_protocol::PermissionsRequestApprovalResponse; use codex_app_server_protocol::RequestId as AppServerRequestId; use codex_app_server_protocol::ReviewTarget; use codex_app_server_protocol::ToolRequestUserInputResponse; @@ -17,7 +18,6 @@ use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::ActivePermissionProfile; use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig; -use codex_protocol::request_permissions::RequestPermissionsResponse; use serde::Serialize; use serde_json::Value; @@ -81,7 +81,7 @@ pub(crate) enum AppCommand { }, RequestPermissionsResponse { id: String, - response: RequestPermissionsResponse, + response: PermissionsRequestApprovalResponse, }, ReloadUserConfig, ListSkills { @@ -230,7 +230,7 @@ impl AppCommand { pub(crate) fn request_permissions_response( id: String, - response: RequestPermissionsResponse, + response: PermissionsRequestApprovalResponse, ) -> Self { Self::RequestPermissionsResponse { id, response } } diff --git a/codex-rs/tui/src/app_event_sender.rs b/codex-rs/tui/src/app_event_sender.rs index 1d5ac55d95..2a0d51befa 100644 --- a/codex-rs/tui/src/app_event_sender.rs +++ b/codex-rs/tui/src/app_event_sender.rs @@ -9,11 +9,11 @@ use crate::app_command::AppCommand; use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::McpServerElicitationAction; +use codex_app_server_protocol::PermissionsRequestApprovalResponse; use codex_app_server_protocol::RequestId as AppServerRequestId; use codex_app_server_protocol::ReviewTarget; use codex_app_server_protocol::ToolRequestUserInputResponse; use codex_protocol::ThreadId; -use codex_protocol::request_permissions::RequestPermissionsResponse; use tokio::sync::mpsc::UnboundedSender; use crate::app_event::AppEvent; @@ -93,7 +93,7 @@ impl AppEventSender { &self, thread_id: ThreadId, id: String, - response: RequestPermissionsResponse, + response: PermissionsRequestApprovalResponse, ) { self.send(AppEvent::SubmitThreadOp { thread_id, diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs index 2208bcabbf..7014ac1df6 100644 --- a/codex-rs/tui/src/app_server_approval_conversions.rs +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -6,22 +6,19 @@ //! submission. use crate::diff_model::FileChange; -use codex_app_server_protocol::AdditionalNetworkPermissions; use codex_app_server_protocol::FileUpdateChange; use codex_app_server_protocol::GrantedPermissionProfile; use codex_app_server_protocol::PatchChangeKind; -use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; +use codex_app_server_protocol::RequestPermissionProfile; use std::collections::HashMap; use std::path::PathBuf; pub(crate) fn granted_permission_profile_from_request( - value: CoreRequestPermissionProfile, + value: RequestPermissionProfile, ) -> GrantedPermissionProfile { GrantedPermissionProfile { - network: value.network.map(|network| AdditionalNetworkPermissions { - enabled: network.enabled, - }), - file_system: value.file_system.map(Into::into), + network: value.network, + file_system: value.file_system, } } @@ -41,7 +38,7 @@ pub(crate) fn file_update_changes_to_display( }, PatchChangeKind::Update { move_path } => FileChange::Update { unified_diff: change.diff, - move_path, + move_path: move_path.map(PathBuf::from), }, }; (path, file_change) @@ -64,7 +61,6 @@ mod tests { use codex_app_server_protocol::GrantedPermissionProfile; use codex_app_server_protocol::PatchChangeKind; use codex_app_server_protocol::RequestPermissionProfile; - use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -104,9 +100,6 @@ mod tests { entries: None, }), }; - let request = CoreRequestPermissionProfile::try_from(request) - .expect("API paths should convert to native paths"); - assert_eq!( granted_permission_profile_from_request(request), GrantedPermissionProfile { @@ -117,20 +110,7 @@ mod tests { read: Some(vec![absolute_path("/tmp/read-only").into()]), write: Some(vec![absolute_path("/tmp/write").into()]), glob_scan_max_depth: None, - entries: Some(vec![ - FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: absolute_path("/tmp/read-only").into(), - }, - access: FileSystemAccessMode::Read, - }, - FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: absolute_path("/tmp/write").into(), - }, - access: FileSystemAccessMode::Write, - }, - ]), + entries: None, }), } ); @@ -152,9 +132,6 @@ mod tests { }]), }), }; - let request = CoreRequestPermissionProfile::try_from(request) - .expect("API paths should convert to native paths"); - assert_eq!( granted_permission_profile_from_request(request), GrantedPermissionProfile { diff --git a/codex-rs/tui/src/approval_events.rs b/codex-rs/tui/src/approval_events.rs index ba183225b8..6020909c4e 100644 --- a/codex-rs/tui/src/approval_events.rs +++ b/codex-rs/tui/src/approval_events.rs @@ -12,10 +12,11 @@ use crate::diff_model::FileChange; use codex_app_server_protocol::AdditionalPermissionProfile; use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::ExecPolicyAmendment; +use codex_app_server_protocol::LegacyAppPathString; use codex_app_server_protocol::NetworkApprovalContext; use codex_app_server_protocol::NetworkPolicyAmendment; use codex_app_server_protocol::NetworkPolicyRuleAction; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_app_server_protocol::RequestPermissionProfile; use serde::Deserialize; use serde::Serialize; @@ -29,7 +30,7 @@ pub(crate) struct ExecApprovalRequestEvent { #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) environment_id: Option, pub(crate) command: Vec, - pub(crate) cwd: AbsolutePathBuf, + pub(crate) cwd: LegacyAppPathString, pub(crate) reason: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) proposed_execpolicy_amendment: Option, @@ -117,5 +118,20 @@ pub(crate) struct ApplyPatchApprovalRequestEvent { #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) reason: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) grant_root: Option, + pub(crate) grant_root: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub(crate) struct RequestPermissionsEvent { + pub(crate) call_id: String, + #[serde(default)] + pub(crate) turn_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) environment_id: Option, + pub(crate) started_at_ms: i64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) reason: Option, + pub(crate) permissions: RequestPermissionProfile, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) cwd: Option, } diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 672c19d439..ff9cecd19a 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -49,13 +49,13 @@ use codex_app_server_protocol::McpServerElicitationAction; use codex_app_server_protocol::NetworkApprovalContext; use codex_app_server_protocol::NetworkApprovalProtocol; use codex_app_server_protocol::NetworkPolicyRuleAction; +use codex_app_server_protocol::PermissionGrantScope; +use codex_app_server_protocol::PermissionsRequestApprovalResponse; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::RequestPermissionProfile; use codex_features::Features; use codex_protocol::ThreadId; -use codex_protocol::request_permissions::PermissionGrantScope; -use codex_protocol::request_permissions::RequestPermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::LegacyAppPathString; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; @@ -399,7 +399,11 @@ impl ApprovalOverlay { let granted_permissions = match decision { PermissionsDecision::GrantForTurn | PermissionsDecision::GrantForTurnWithStrictAutoReview - | PermissionsDecision::GrantForSession => permissions.clone(), + | PermissionsDecision::GrantForSession => { + crate::app_server_approval_conversions::granted_permission_profile_from_request( + permissions.clone(), + ) + } PermissionsDecision::Deny => Default::default(), }; let scope = if matches!(decision, PermissionsDecision::GrantForSession) { @@ -429,10 +433,10 @@ impl ApprovalOverlay { self.app_event_tx.request_permissions_response( thread_id, call_id.to_string(), - codex_protocol::request_permissions::RequestPermissionsResponse { + PermissionsRequestApprovalResponse { permissions: granted_permissions, scope, - strict_auto_review, + strict_auto_review: strict_auto_review.then_some(true), }, ); } @@ -1029,9 +1033,12 @@ fn special_path_label(value: &FileSystemSpecialPath) -> String { } } -fn path_label(base: &str, subpath: &Option) -> String { +fn path_label( + base: &str, + subpath: &Option, +) -> String { match subpath { - Some(subpath) => format!("{base}/{subpath}"), + Some(subpath) => format!("{base}/{}", subpath.render_for_ui()), None => base.to_string(), } } @@ -1250,13 +1257,19 @@ mod tests { environment_id: None, reason: Some("need workspace access".to_string()), permissions: RequestPermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path("/tmp/readme.txt")]), - Some(vec![absolute_path("/tmp/out.txt")]), - )), + network: Some( + NetworkPermissions { + enabled: Some(true), + } + .into(), + ), + file_system: Some( + FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path("/tmp/readme.txt").into()]), + Some(vec![absolute_path("/tmp/out.txt").into()]), + ) + .into(), + ), }, } } @@ -1787,8 +1800,8 @@ mod tests { network: None, file_system: Some( FileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path("/tmp/readme.txt")]), - Some(vec![absolute_path("/tmp/out.txt")]), + Some(vec![absolute_path("/tmp/readme.txt").into()]), + Some(vec![absolute_path("/tmp/out.txt").into()]), ) .into(), ), @@ -1872,9 +1885,11 @@ mod tests { entries: Some(vec![FileSystemSandboxEntry { path: FileSystemPath::Special { value: FileSystemSpecialPath::ProjectRoots { - subpath: Some(LegacyAppPathString::from_path(std::path::Path::new( - ".git", - ))), + subpath: Some( + codex_app_server_protocol::LegacyAppPathString::from_path( + std::path::Path::new(".git"), + ), + ), }, }, access: FileSystemAccessMode::Read, @@ -1941,7 +1956,7 @@ mod tests { { assert!(response.permissions.is_empty()); assert_eq!(response.scope, PermissionGrantScope::Turn); - assert!(!response.strict_auto_review); + assert_eq!(response.strict_auto_review, None); saw_op = true; break; } @@ -1968,7 +1983,7 @@ mod tests { } = ev { assert_eq!(response.scope, PermissionGrantScope::Turn); - assert!(response.strict_auto_review); + assert_eq!(response.strict_auto_review, Some(true)); saw_op = true; break; } @@ -2001,8 +2016,8 @@ mod tests { }), file_system: Some( FileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path("/tmp/readme.txt")]), - Some(vec![absolute_path("/tmp/out.txt")]), + Some(vec![absolute_path("/tmp/readme.txt").into()]), + Some(vec![absolute_path("/tmp/out.txt").into()]), ) .into(), ), @@ -2058,8 +2073,8 @@ mod tests { }), file_system: Some( FileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path("/tmp/readme.txt")]), - Some(vec![absolute_path("/tmp/out.txt")]), + Some(vec![absolute_path("/tmp/readme.txt").into()]), + Some(vec![absolute_path("/tmp/out.txt").into()]), ) .into(), ), diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index fa745c652e..d6ca773e1f 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -47,6 +47,7 @@ use crate::app_event::HistoryLookupResponse; use crate::app_server_approval_conversions::file_update_changes_to_display; use crate::approval_events::ApplyPatchApprovalRequestEvent; use crate::approval_events::ExecApprovalRequestEvent; +use crate::approval_events::RequestPermissionsEvent; use crate::bottom_pane::StatusLineItem; use crate::bottom_pane::StatusLineSetupView; use crate::bottom_pane::StatusSurfacePreviewData; @@ -62,6 +63,7 @@ use crate::mention_codec::encode_history_mentions; use crate::model_catalog::ModelCatalog; use crate::multi_agents; use crate::multi_agents::AgentMetadata; +use crate::session_state::LocalWorkspaceState; use crate::session_state::SessionNetworkProxyRuntime; use crate::session_state::ThreadSessionState; use crate::status::RateLimitWindowDisplay; @@ -153,7 +155,6 @@ use codex_protocol::items::AgentMessageItem; use codex_protocol::models::MessagePhase; use codex_protocol::plan_tool::PlanItemArg as UpdatePlanItemArg; use codex_protocol::plan_tool::StepStatus as UpdatePlanItemStatus; -use codex_protocol::request_permissions::RequestPermissionsEvent; use codex_protocol::user_input::ByteRange; use codex_protocol::user_input::TextElement; use codex_terminal_detection::Multiplexer; @@ -162,6 +163,7 @@ use codex_terminal_detection::TerminalName; use codex_terminal_detection::terminal_info; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_cli::resume_hint; +use codex_utils_path_uri::LegacyAppPathString; use codex_utils_path_uri::PathUri; use codex_utils_plugins::mention_syntax::PLUGIN_TEXT_MENTION_SIGIL; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; @@ -583,7 +585,7 @@ pub(crate) struct ChatWidget { pending_collab_spawn_requests: HashMap, suppressed_exec_calls: HashSet, skills_all: Vec, - skills_initial_state: Option>, + skills_initial_state: Option>, last_unified_wait: Option, unified_exec_wait_streak: Option, turn_lifecycle: TurnLifecycleState, @@ -689,6 +691,8 @@ pub(crate) struct ChatWidget { current_rollout_path: Option, // Current working directory (if known) current_cwd: Option, + // Host-local workspace state for embedded sessions. Remote target paths never enter Config. + local_workspace: Option, // App-server-backed command runner for status-line workspace metadata lookups. workspace_command_runner: Option, // Instruction source files loaded for the current session, supplied by app-server. @@ -855,12 +859,9 @@ fn exec_approval_request_from_params( params: CommandExecutionRequestApprovalParams, fallback_cwd: &AbsolutePathBuf, ) -> ExecApprovalRequestEvent { - // TODO(anp): Keep this as PathUri once `tui::approval_events::ExecApprovalRequestEvent` and - // approval rendering support foreign paths. let cwd = params .cwd - .and_then(|cwd| cwd.to_inferred_abs_path()) - .unwrap_or_else(|| fallback_cwd.clone()); + .unwrap_or_else(|| LegacyAppPathString::from_abs_path(fallback_cwd)); ExecApprovalRequestEvent { call_id: params.item_id, command: params @@ -895,16 +896,16 @@ fn patch_approval_request_from_params( fn request_permissions_from_params( params: codex_app_server_protocol::PermissionsRequestApprovalParams, -) -> std::io::Result { - Ok(RequestPermissionsEvent { +) -> RequestPermissionsEvent { + RequestPermissionsEvent { turn_id: params.turn_id, call_id: params.item_id, environment_id: params.environment_id, started_at_ms: params.started_at_ms, reason: params.reason, - permissions: params.permissions.try_into()?, + permissions: params.permissions, cwd: Some(params.cwd), - }) + } } fn token_usage_info_from_app_server(token_usage: ThreadTokenUsage) -> TokenUsageInfo { @@ -1291,7 +1292,7 @@ impl ChatWidget { UserInput::Skill { name, path } => Some(MentionBinding { sigil: TOOL_MENTION_SIGIL, mention: name.clone(), - path: path.to_string_lossy().into_owned(), + path: path.as_str().to_string(), }), UserInput::Mention { name, path } => { let plugin_id = path.strip_prefix("plugin://"); diff --git a/codex-rs/tui/src/chatwidget/interrupts.rs b/codex-rs/tui/src/chatwidget/interrupts.rs index 7b4c23e273..cdcd30f8bb 100644 --- a/codex-rs/tui/src/chatwidget/interrupts.rs +++ b/codex-rs/tui/src/chatwidget/interrupts.rs @@ -5,11 +5,11 @@ use std::collections::VecDeque; use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::approval_events::ApplyPatchApprovalRequestEvent; use crate::approval_events::ExecApprovalRequestEvent; +use crate::approval_events::RequestPermissionsEvent; use codex_app_server_protocol::McpServerElicitationRequestParams; use codex_app_server_protocol::RequestId as AppServerRequestId; use codex_app_server_protocol::ThreadItem; use codex_app_server_protocol::ToolRequestUserInputParams; -use codex_protocol::request_permissions::RequestPermissionsEvent; use super::ChatWidget; @@ -163,7 +163,7 @@ mod tests { turn_id: "turn".to_string(), environment_id: None, command: vec!["true".to_string()], - cwd: AbsolutePathBuf::current_dir().expect("current dir"), + cwd: AbsolutePathBuf::current_dir().expect("current dir").into(), reason: None, network_approval_context: None, proposed_execpolicy_amendment: None, diff --git a/codex-rs/tui/src/chatwidget/protocol_requests.rs b/codex-rs/tui/src/chatwidget/protocol_requests.rs index 5729c073fd..b1846ce3c9 100644 --- a/codex-rs/tui/src/chatwidget/protocol_requests.rs +++ b/codex-rs/tui/src/chatwidget/protocol_requests.rs @@ -30,16 +30,7 @@ impl ChatWidget { self.on_elicitation_request(request_id, params); } ServerRequest::PermissionsRequestApproval { params, .. } => { - // TODO(anp): Remove this native-path localization error path once core permission - // paths remain PathUri after crossing the app-server boundary. - match request_permissions_from_params(params) { - Ok(event) => self.on_request_permissions(event), - Err(err) => { - self.add_error_message(format!( - "failed to localize requested filesystem paths: {err}" - )); - } - } + self.on_request_permissions(request_permissions_from_params(params)); } ServerRequest::ToolRequestUserInput { params, .. } => { self.on_request_user_input(params);