agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

Use ApiPathString in app-server filesystem permission paths (#28367)

Browse Source 
## Why

Clients running an app-server on one OS and an exec-server on another OS
need to be able to pass sandbox config to app-server that refers to
resources on the executor's foreign OS.

## What

`AbsolutePathBuf` can't represent these paths and we don't want users to
be exposed to `PathUri` yet, so this moves the public app-server API to
be expressed in terms of `ApiPathString`.

Stacked on #28165.

- change app-server v2 filesystem permission paths, including legacy
read/write roots, to `ApiPathString`
- localize API paths through `PathUri` when converting into the current
native core permission types
- make path-bearing permission conversions fallible and surface
localization failures instead of silently treating malformed grants as
ordinary denials
- propagate conversion failures through app-server and TUI approval
handling
- regenerate the app-server JSON and TypeScript schemas
- leave migration TODOs on native-path conversions so they can be
removed once core permission paths use `PathUri`
This commit is contained in:
Adam Perry @ OpenAI
2026-06-15 19:25:54 -07:00
committed by GitHub
Unverified
parent d959664420
commit ecfe174d5f
34 changed files with 546 additions and 233 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/app/app_server_requests.rs
+66 -4
View File
@@ -12,6 +12,7 @@ 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(
@@ -103,6 +104,18 @@ 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
@@ -397,6 +410,7 @@ 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;
@@ -465,6 +479,54 @@ mod tests {
assert_eq!(resolution.result, json!({ "decision": "accept" }));
}
#[test]
fn rejects_permissions_with_paths_that_cannot_be_localized() {
let mut pending = PendingAppServerRequests::default();
let request_id = AppServerRequestId::Integer(7);
let permissions = codex_app_server_protocol::RequestPermissionProfile {
network: None,
file_system: Some(AdditionalFileSystemPermissions {
read: Some(vec![
serde_json::from_value(json!("relative/path"))
.expect("relative API path should deserialize"),
]),
write: None,
glob_scan_max_depth: None,
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");
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}"
),
})
);
}
#[test]
fn resolves_permissions_and_user_input_through_app_server_request_id() {
let mut pending = PendingAppServerRequests::default();
@@ -544,19 +606,19 @@ mod tests {
enabled: Some(true),
}),
file_system: Some(AdditionalFileSystemPermissions {
read: Some(vec![absolute_path(read_path)]),
write: Some(vec![absolute_path(write_path)]),
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),
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),
path: absolute_path(write_path).into(),
},
access: codex_app_server_protocol::FileSystemAccessMode::Write,
},
codex-rs/tui/src/app/side.rs
+1 -1
View File
@@ -529,7 +529,7 @@ impl App {
{
if self.discard_side_thread(app_server, side_thread_id).await {
self.surface_pending_inactive_thread_interactive_requests()
.await;
.await?;
} else if active_thread_id_before_switch == Some(side_thread_id) {
self.keep_side_thread_visible_after_cleanup_failure(
tui,
codex-rs/tui/src/app/tests.rs
+13 -7
View File
@@ -2433,7 +2433,7 @@ async fn side_defers_subagent_approval_overlay_until_side_exits() -> Result<()>
app.side_threads.remove(&side_thread_id);
app.active_thread_id = Some(main_thread_id);
app.surface_pending_inactive_thread_interactive_requests()
.await;
.await?;
assert_eq!(app.chat_widget.has_active_view(), true);
@@ -2462,8 +2462,8 @@ async fn inactive_thread_exec_approval_preserves_context() {
enabled: Some(true),
}),
file_system: Some(AdditionalFileSystemPermissions {
read: Some(vec![test_absolute_path("/tmp/read-only")]),
write: Some(vec![test_absolute_path("/tmp/write")]),
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,
}),
@@ -2481,6 +2481,7 @@ async fn inactive_thread_exec_approval_preserves_context() {
})) = app
.interactive_request_for_thread_request(thread_id, &request)
.await
.expect("valid localized paths")
else {
panic!("expected exec approval request");
};
@@ -2499,8 +2500,8 @@ async fn inactive_thread_exec_approval_preserves_context() {
enabled: Some(true),
}),
file_system: Some(AdditionalFileSystemPermissions {
read: Some(vec![test_absolute_path("/tmp/read-only")]),
write: Some(vec![test_absolute_path("/tmp/write")]),
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,
}),
@@ -2542,6 +2543,7 @@ async fn inactive_thread_exec_approval_splits_shell_wrapped_command() {
let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Exec { command, .. })) = app
.interactive_request_for_thread_request(thread_id, &request)
.await
.expect("valid localized paths")
else {
panic!("expected exec approval request");
};
@@ -2595,6 +2597,7 @@ async fn inactive_thread_file_change_approval_recovers_buffered_changes() {
let request = app
.interactive_request_for_thread_request(thread_id, &request)
.await
.expect("valid localized paths")
.expect("expected file change approval request");
let ThreadInteractiveRequest::Approval(ApprovalRequest::ApplyPatch {
@@ -2646,8 +2649,8 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions(
enabled: Some(true),
}),
file_system: Some(AdditionalFileSystemPermissions {
read: Some(vec![test_absolute_path("/tmp/read-only")]),
write: Some(vec![test_absolute_path("/tmp/write")]),
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,
}),
@@ -2662,6 +2665,7 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions(
})) = app
.interactive_request_for_thread_request(thread_id, &request)
.await
.expect("valid localized paths")
else {
panic!("expected permissions approval request");
};
@@ -2703,6 +2707,7 @@ async fn inactive_thread_url_elicitation_routes_to_app_link() {
let Some(ThreadInteractiveRequest::AppLink(params)) = app
.interactive_request_for_thread_request(thread_id, &request)
.await
.expect("valid localized paths")
else {
panic!("expected app link request");
};
@@ -2742,6 +2747,7 @@ async fn inactive_thread_invalid_url_elicitation_is_declined() {
assert!(
app.interactive_request_for_thread_request(thread_id, &request)
.await
.expect("valid localized paths")
.is_none()
);
assert_matches!(
codex-rs/tui/src/app/thread_routing.rs
+30 -17
View File
@@ -210,9 +210,9 @@ impl App {
&self,
thread_id: ThreadId,
request: &ServerRequest,
) -> Option<ThreadInteractiveRequest> {
) -> std::io::Result<Option<ThreadInteractiveRequest>> {
let thread_label = Some(self.thread_label(thread_id));
match request {
Ok(match request {
ServerRequest::CommandExecutionRequestApproval { params, .. } => {
let network_approval_context = params.network_approval_context.clone();
let additional_permissions = params.additional_permissions.clone();
@@ -305,18 +305,28 @@ impl App {
}
}
}
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().into(),
}),
),
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,
},
))
}
_ => None,
}
})
}
pub(super) fn push_thread_interactive_request(&mut self, request: ThreadInteractiveRequest) {
@@ -376,20 +386,23 @@ impl App {
requests
}
pub(super) async fn surface_pending_inactive_thread_interactive_requests(&mut self) {
pub(super) async fn surface_pending_inactive_thread_interactive_requests(
&mut self,
) -> Result<()> {
if self.active_side_parent_thread_id().is_some() {
return;
return Ok(());
}
let requests = self.pending_inactive_thread_requests().await;
for (thread_id, request) in requests {
if let Some(request) = self
.interactive_request_for_thread_request(thread_id, &request)
.await
.await?
{
self.push_thread_interactive_request(request);
}
}
Ok(())
}
pub(super) async fn submit_active_thread_op(
@@ -991,7 +1004,7 @@ impl App {
) -> Result<()> {
let inactive_interactive_request = if self.active_thread_id != Some(thread_id) {
self.interactive_request_for_thread_request(thread_id, &request)
.await
.await?
} else {
None
};
codex-rs/tui/src/app_server_approval_conversions.rs
+61 -49
View File
@@ -54,8 +54,16 @@ mod tests {
use super::file_update_changes_to_display;
use super::granted_permission_profile_from_request;
use crate::diff_model::FileChange;
use codex_app_server_protocol::AdditionalFileSystemPermissions;
use codex_app_server_protocol::AdditionalNetworkPermissions;
use codex_app_server_protocol::FileSystemAccessMode;
use codex_app_server_protocol::FileSystemPath;
use codex_app_server_protocol::FileSystemSandboxEntry;
use codex_app_server_protocol::FileSystemSpecialPath;
use codex_app_server_protocol::FileUpdateChange;
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;
@@ -85,40 +93,42 @@ mod tests {
#[test]
fn converts_request_permissions_into_granted_permissions() {
let request = RequestPermissionProfile {
network: Some(AdditionalNetworkPermissions {
enabled: Some(true),
}),
file_system: Some(AdditionalFileSystemPermissions {
read: Some(vec![absolute_path("/tmp/read-only").into()]),
write: Some(vec![absolute_path("/tmp/write").into()]),
glob_scan_max_depth: None,
entries: None,
}),
};
let request = CoreRequestPermissionProfile::try_from(request)
.expect("API paths should convert to native paths");
assert_eq!(
granted_permission_profile_from_request(CoreRequestPermissionProfile::from(
codex_app_server_protocol::RequestPermissionProfile {
network: Some(codex_app_server_protocol::AdditionalNetworkPermissions {
enabled: Some(true),
}),
file_system: Some(codex_app_server_protocol::AdditionalFileSystemPermissions {
read: Some(vec![absolute_path("/tmp/read-only")]),
write: Some(vec![absolute_path("/tmp/write")]),
glob_scan_max_depth: None,
entries: None,
}),
}
)),
codex_app_server_protocol::GrantedPermissionProfile {
network: Some(codex_app_server_protocol::AdditionalNetworkPermissions {
granted_permission_profile_from_request(request),
GrantedPermissionProfile {
network: Some(AdditionalNetworkPermissions {
enabled: Some(true),
}),
file_system: Some(codex_app_server_protocol::AdditionalFileSystemPermissions {
read: Some(vec![absolute_path("/tmp/read-only")]),
write: Some(vec![absolute_path("/tmp/write")]),
file_system: Some(AdditionalFileSystemPermissions {
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![
codex_app_server_protocol::FileSystemSandboxEntry {
path: codex_app_server_protocol::FileSystemPath::Path {
path: absolute_path("/tmp/read-only"),
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: absolute_path("/tmp/read-only").into(),
},
access: codex_app_server_protocol::FileSystemAccessMode::Read,
access: FileSystemAccessMode::Read,
},
codex_app_server_protocol::FileSystemSandboxEntry {
path: codex_app_server_protocol::FileSystemPath::Path {
path: absolute_path("/tmp/write"),
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: absolute_path("/tmp/write").into(),
},
access: codex_app_server_protocol::FileSystemAccessMode::Write,
access: FileSystemAccessMode::Write,
},
]),
}),
@@ -128,35 +138,37 @@ mod tests {
#[test]
fn converts_request_permissions_into_canonical_granted_permissions() {
let request = RequestPermissionProfile {
network: None,
file_system: Some(AdditionalFileSystemPermissions {
read: None,
write: None,
glob_scan_max_depth: None,
entries: Some(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
access: FileSystemAccessMode::Write,
}]),
}),
};
let request = CoreRequestPermissionProfile::try_from(request)
.expect("API paths should convert to native paths");
assert_eq!(
granted_permission_profile_from_request(CoreRequestPermissionProfile::from(
codex_app_server_protocol::RequestPermissionProfile {
network: None,
file_system: Some(codex_app_server_protocol::AdditionalFileSystemPermissions {
read: None,
write: None,
glob_scan_max_depth: None,
entries: Some(vec![codex_app_server_protocol::FileSystemSandboxEntry {
path: codex_app_server_protocol::FileSystemPath::Special {
value: codex_app_server_protocol::FileSystemSpecialPath::Root,
},
access: codex_app_server_protocol::FileSystemAccessMode::Write,
}]),
}),
}
)),
codex_app_server_protocol::GrantedPermissionProfile {
granted_permission_profile_from_request(request),
GrantedPermissionProfile {
network: None,
file_system: Some(codex_app_server_protocol::AdditionalFileSystemPermissions {
file_system: Some(AdditionalFileSystemPermissions {
read: None,
write: None,
glob_scan_max_depth: None,
entries: Some(vec![codex_app_server_protocol::FileSystemSandboxEntry {
path: codex_app_server_protocol::FileSystemPath::Special {
value: codex_app_server_protocol::FileSystemSpecialPath::Root,
entries: Some(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
access: codex_app_server_protocol::FileSystemAccessMode::Write,
},]),
access: FileSystemAccessMode::Write,
}]),
}),
}
);
codex-rs/tui/src/bottom_pane/approval_overlay.rs
+1 -1
View File
@@ -1000,7 +1000,7 @@ fn format_file_system_entry_paths<'a>(
) -> String {
entries
.map(|entry| match &entry.path {
FileSystemPath::Path { path } => format!("`{}`", path.display()),
FileSystemPath::Path { path } => format!("`{path}`"),
FileSystemPath::GlobPattern { pattern } => format!("glob `{pattern}`"),
FileSystemPath::Special { value } => format!("`{}`", special_path_label(value)),
})
codex-rs/tui/src/chatwidget.rs
+4 -4
View File
@@ -865,16 +865,16 @@ fn patch_approval_request_from_params(
fn request_permissions_from_params(
params: codex_app_server_protocol::PermissionsRequestApprovalParams,
) -> RequestPermissionsEvent {
RequestPermissionsEvent {
) -> std::io::Result<RequestPermissionsEvent> {
Ok(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.into(),
permissions: params.permissions.try_into()?,
cwd: Some(params.cwd),
}
})
}
fn token_usage_info_from_app_server(token_usage: ThreadTokenUsage) -> TokenUsageInfo {
codex-rs/tui/src/chatwidget/protocol_requests.rs
+22 -2
View File
@@ -30,7 +30,16 @@ impl ChatWidget {
self.on_elicitation_request(request_id, params);
}
ServerRequest::PermissionsRequestApproval { params, .. } => {
self.on_request_permissions(request_permissions_from_params(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}"
));
}
}
}
ServerRequest::ToolRequestUserInput { params, .. } => {
self.on_request_user_input(params);
@@ -62,6 +71,17 @@ impl ChatWidget {
completion: Option<(i64, codex_app_server_protocol::AutoReviewDecisionSource)>,
action: GuardianApprovalReviewAction,
) {
// TODO(anp): Remove this native-path localization error path once core permission paths
// remain PathUri after crossing the app-server boundary.
let action = match action.try_into() {
Ok(action) => action,
Err(err) => {
self.add_error_message(format!(
"failed to localize guardian filesystem paths: {err}"
));
return;
}
};
let (completed_at_ms, decision_source) = match completion {
Some((completed_at_ms, decision_source)) => {
(Some(completed_at_ms), Some(decision_source))
@@ -128,7 +148,7 @@ impl ChatWidget {
GuardianAssessmentDecisionSource::Agent
}
}),
action: action.into(),
action,
});
}
codex-rs/tui/src/chatwidget/tests.rs
+1
View File
@@ -164,6 +164,7 @@ pub(super) use codex_terminal_detection::TerminalInfo;
pub(super) use codex_terminal_detection::TerminalName;
pub(super) use codex_utils_absolute_path::AbsolutePathBuf;
pub(super) use codex_utils_approval_presets::builtin_approval_presets;
pub(super) use codex_utils_path_uri::ApiPathString;
pub(super) use crossterm::event::KeyCode;
pub(super) use crossterm::event::KeyEvent;
pub(super) use crossterm::event::KeyModifiers;
codex-rs/tui/src/chatwidget/tests/approval_requests.rs
+12 -7
View File
@@ -89,6 +89,8 @@ fn app_server_exec_approval_request_preserves_permissions_context() {
.expect("absolute read path");
let write_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/write")))
.expect("absolute write path");
let read_api_path = ApiPathString::from_abs_path(&read_path);
let write_api_path = ApiPathString::from_abs_path(&write_path);
let request = exec_approval_request_from_params(
AppServerCommandExecutionRequestApprovalParams {
thread_id: "thread-1".to_string(),
@@ -109,8 +111,8 @@ fn app_server_exec_approval_request_preserves_permissions_context() {
enabled: Some(true),
}),
file_system: Some(AppServerAdditionalFileSystemPermissions {
read: Some(vec![read_path.clone()]),
write: Some(vec![write_path.clone()]),
read: Some(vec![read_api_path.clone()]),
write: Some(vec![write_api_path.clone()]),
glob_scan_max_depth: None,
entries: None,
}),
@@ -136,8 +138,8 @@ fn app_server_exec_approval_request_preserves_permissions_context() {
enabled: Some(true),
}),
file_system: Some(AppServerAdditionalFileSystemPermissions {
read: Some(vec![read_path]),
write: Some(vec![write_path]),
read: Some(vec![read_api_path]),
write: Some(vec![write_api_path]),
glob_scan_max_depth: None,
entries: None,
}),
@@ -274,6 +276,8 @@ fn app_server_request_permissions_preserves_file_system_permissions() {
.expect("absolute read path");
let write_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/write")))
.expect("absolute write path");
let read_api_path = ApiPathString::from_abs_path(&read_path);
let write_api_path = ApiPathString::from_abs_path(&write_path);
let cwd =
AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp"))).expect("absolute cwd");
@@ -290,13 +294,14 @@ fn app_server_request_permissions_preserves_file_system_permissions() {
enabled: Some(true),
}),
file_system: Some(AppServerAdditionalFileSystemPermissions {
read: Some(vec![read_path.clone()]),
write: Some(vec![write_path.clone()]),
read: Some(vec![read_api_path]),
write: Some(vec![write_api_path]),
glob_scan_max_depth: None,
entries: None,
}),
},
});
})
.expect("API paths should convert to native paths");
assert_eq!(
request.permissions,