From 8bc667b07be22c005769e814bc529e30cff1ea27 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 23 Apr 2026 00:21:59 -0700 Subject: [PATCH] app-server: include filesystem entries in permission requests (#19086) ## Why `item/permissions/requestApproval` sends a requested permission profile to app-server clients. The core profile already stores filesystem permissions as `entries`, but the v2 compatibility conversion used the legacy `read`/`write` projection whenever possible and left `entries` unset. That made the request ambiguous for clients that consume the canonical v2 shape: `permissions.fileSystem.entries` was missing even though filesystem access was being requested. A client that rendered or echoed grants from `entries` could treat the request as having no filesystem permission entries, then return an empty or incomplete grant. The app-server intersects responses with the original request, so omitted filesystem permissions are denied. ## What Changed - Populate `AdditionalFileSystemPermissions.entries` when converting legacy read/write roots for request permission payloads, while preserving `read` and `write` for compatibility. - Mark `read` and `write` as transitional schema fields in the generated app-server schema. - Add regression coverage for the v2 conversion, the app-server `item/permissions/requestApproval` round trip, and TUI app-server approval conversion expectations. - Refresh generated JSON and TypeScript schema fixtures. ## Verification - `just fmt` - `cargo test -p codex-app-server-protocol` - `cargo test -p codex-app-server request_permissions_round_trip` - `cargo test -p codex-tui converts_request_permissions_into_granted_permissions` - `cargo test -p codex-tui resolves_permissions_and_user_input_through_app_server_request_id` --- ...CommandExecutionRequestApprovalParams.json | 2 + .../PermissionsRequestApprovalParams.json | 2 + .../PermissionsRequestApprovalResponse.json | 2 + .../schema/json/ServerNotification.json | 2 + .../schema/json/ServerRequest.json | 2 + .../codex_app_server_protocol.schemas.json | 2 + .../codex_app_server_protocol.v2.schemas.json | 2 + ...anApprovalReviewCompletedNotification.json | 2 + ...dianApprovalReviewStartedNotification.json | 2 + .../v2/AdditionalFileSystemPermissions.ts | 10 +++- .../app-server-protocol/src/protocol/v2.rs | 58 ++++++++++++++++++- .../tests/suite/v2/request_permissions.rs | 24 +++++++- codex-rs/tui/src/app/app_server_requests.rs | 15 ++++- .../src/app_server_approval_conversions.rs | 15 ++++- 14 files changed, 134 insertions(+), 6 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 3968da2a3..78e75d7c4 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -25,6 +25,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -34,6 +35,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json index c2e26c553..ef268908f 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json @@ -25,6 +25,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -34,6 +35,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json index e066108e6..f49165296 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json @@ -25,6 +25,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -34,6 +35,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 38acf1c90..59b3f5b45 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -84,6 +84,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -93,6 +94,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 016dbe341..84bf52473 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -25,6 +25,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -34,6 +35,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 516e33aa7..3eaf25b9b 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -5298,6 +5298,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/v2/AbsolutePathBuf" }, @@ -5307,6 +5308,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/v2/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 67a4cb186..b1f993515 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -148,6 +148,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -157,6 +158,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json index 27716e59b..e4a278330 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -25,6 +25,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -34,6 +35,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json index c039f2191..b4ad6af44 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -25,6 +25,7 @@ ] }, "read": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, @@ -34,6 +35,7 @@ ] }, "write": { + "description": "This will be removed in favor of `entries`.", "items": { "$ref": "#/definitions/AbsolutePathBuf" }, diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts index 2be8f7f0e..e29263b95 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts @@ -4,4 +4,12 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; import type { FileSystemSandboxEntry } from "./FileSystemSandboxEntry"; -export type AdditionalFileSystemPermissions = { read: Array | null, write: Array | null, globScanMaxDepth?: number, entries?: Array, }; +export type AdditionalFileSystemPermissions = { +/** + * This will be removed in favor of `entries`. + */ +read: Array | null, +/** + * This will be removed in favor of `entries`. + */ +write: Array | null, globScanMaxDepth?: number, entries?: Array, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 655ad2d0a..937678793 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1273,7 +1273,9 @@ impl From for NetworkApprovalContext { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct AdditionalFileSystemPermissions { + /// This will be removed in favor of `entries`. pub read: Option>, + /// This will be removed in favor of `entries`. pub write: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] @@ -1286,11 +1288,26 @@ pub struct AdditionalFileSystemPermissions { 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), + ); + if let Some(paths) = read.as_ref() { + entries.extend(paths.iter().map(|path| FileSystemSandboxEntry { + path: FileSystemPath::Path { path: path.clone() }, + access: FileSystemAccessMode::Read, + })); + } + if let Some(paths) = write.as_ref() { + entries.extend(paths.iter().map(|path| FileSystemSandboxEntry { + path: FileSystemPath::Path { path: path.clone() }, + access: FileSystemAccessMode::Write, + })); + } Self { read, write, glob_scan_max_depth: None, - entries: None, + entries: Some(entries), } } else { Self { @@ -7764,6 +7781,45 @@ mod tests { ); } + #[test] + fn additional_file_system_permissions_populates_entries_for_legacy_roots() { + let read_only_path = absolute_path("read-only"); + let read_write_path = absolute_path("read-write"); + let core_permissions = CoreFileSystemPermissions::from_read_write_roots( + Some(vec![read_only_path.clone()]), + Some(vec![read_write_path.clone()]), + ); + + let permissions = AdditionalFileSystemPermissions::from(core_permissions.clone()); + + assert_eq!( + permissions, + AdditionalFileSystemPermissions { + read: Some(vec![read_only_path.clone()]), + write: Some(vec![read_write_path.clone()]), + glob_scan_max_depth: None, + entries: Some(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: read_only_path, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: read_write_path, + }, + access: FileSystemAccessMode::Write, + }, + ]), + } + ); + assert_eq!( + CoreFileSystemPermissions::from(permissions), + core_permissions + ); + } + #[test] fn additional_file_system_permissions_rejects_zero_glob_scan_depth() { serde_json::from_value::(json!({ diff --git a/codex-rs/app-server/tests/suite/v2/request_permissions.rs b/codex-rs/app-server/tests/suite/v2/request_permissions.rs index 4a4eb9340..7acdc1b12 100644 --- a/codex-rs/app-server/tests/suite/v2/request_permissions.rs +++ b/codex-rs/app-server/tests/suite/v2/request_permissions.rs @@ -78,12 +78,32 @@ async fn request_permissions_round_trip() -> Result<()> { assert_eq!(params.item_id, "call1"); assert!(params.cwd.as_path().is_absolute()); assert_eq!(params.reason, Some("Select a workspace root".to_string())); - let requested_writes = params + let requested_file_system = params .permissions .file_system - .and_then(|file_system| file_system.write) + .expect("request should include file system permissions"); + let requested_writes = requested_file_system + .write + .clone() .expect("request should include write permissions"); assert_eq!(requested_writes.len(), 2); + assert_eq!( + requested_file_system.entries, + Some(vec![ + codex_app_server_protocol::FileSystemSandboxEntry { + path: codex_app_server_protocol::FileSystemPath::Path { + path: requested_writes[0].clone(), + }, + access: codex_app_server_protocol::FileSystemAccessMode::Write, + }, + codex_app_server_protocol::FileSystemSandboxEntry { + path: codex_app_server_protocol::FileSystemPath::Path { + path: requested_writes[1].clone(), + }, + access: codex_app_server_protocol::FileSystemAccessMode::Write, + }, + ]) + ); let resolved_request_id = request_id.clone(); mcp.send_response( diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 3149dd9f5..5c2c69385 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -562,7 +562,20 @@ mod tests { read: Some(vec![absolute_path(read_path)]), write: Some(vec![absolute_path(write_path)]), glob_scan_max_depth: None, - entries: None, + entries: Some(vec![ + codex_app_server_protocol::FileSystemSandboxEntry { + path: codex_app_server_protocol::FileSystemPath::Path { + path: absolute_path(read_path), + }, + access: codex_app_server_protocol::FileSystemAccessMode::Read, + }, + codex_app_server_protocol::FileSystemSandboxEntry { + path: codex_app_server_protocol::FileSystemPath::Path { + path: absolute_path(write_path), + }, + access: codex_app_server_protocol::FileSystemAccessMode::Write, + }, + ]), }), }, scope: PermissionGrantScope::Session, diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs index a26f9b2a9..a0d86db7d 100644 --- a/codex-rs/tui/src/app_server_approval_conversions.rs +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -93,7 +93,20 @@ mod tests { read: Some(vec![absolute_path("/tmp/read-only")]), write: Some(vec![absolute_path("/tmp/write")]), glob_scan_max_depth: None, - entries: None, + entries: Some(vec![ + codex_app_server_protocol::FileSystemSandboxEntry { + path: codex_app_server_protocol::FileSystemPath::Path { + path: absolute_path("/tmp/read-only"), + }, + access: codex_app_server_protocol::FileSystemAccessMode::Read, + }, + codex_app_server_protocol::FileSystemSandboxEntry { + path: codex_app_server_protocol::FileSystemPath::Path { + path: absolute_path("/tmp/write"), + }, + access: codex_app_server_protocol::FileSystemAccessMode::Write, + }, + ]), }), } );