diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index e4b747e82..d94daa0e8 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2109,6 +2109,7 @@ dependencies = [ "codex-shell-command", "codex-utils-absolute-path", "codex-utils-cargo-bin", + "codex-utils-path-uri", "inventory", "pretty_assertions", "rmcp", @@ -4050,6 +4051,7 @@ dependencies = [ "codex-utils-home-dir", "codex-utils-oss", "codex-utils-path", + "codex-utils-path-uri", "codex-utils-plugins", "codex-utils-sandbox-summary", "codex-utils-sleep-inhibitor", diff --git a/codex-rs/app-server-protocol/Cargo.toml b/codex-rs/app-server-protocol/Cargo.toml index 0749b07e0..dc69e2d4b 100644 --- a/codex-rs/app-server-protocol/Cargo.toml +++ b/codex-rs/app-server-protocol/Cargo.toml @@ -19,6 +19,7 @@ codex-experimental-api-macros = { workspace = true } codex-protocol = { workspace = true } codex-shell-command = { workspace = true } codex-utils-absolute-path = { workspace = true } +codex-utils-path-uri = { workspace = true } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 922db80f2..c59141103 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -27,7 +27,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -37,7 +37,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -84,6 +84,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "CommandAction": { "oneOf": [ { @@ -286,7 +289,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json index f2ab78334..4d7eff3eb 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalParams.json @@ -27,7 +27,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -37,7 +37,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -58,6 +58,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "FileSystemAccessMode": { "enum": [ "read", @@ -71,7 +74,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json index 5cce2cdc5..e0bfd161f 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json @@ -1,10 +1,6 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { - "AbsolutePathBuf": { - "description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.", - "type": "string" - }, "AdditionalFileSystemPermissions": { "properties": { "entries": { @@ -27,7 +23,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -37,7 +33,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -58,6 +54,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "FileSystemAccessMode": { "enum": [ "read", @@ -71,7 +70,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 2c87ad5cd..2a4967604 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -107,7 +107,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -117,7 +117,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -164,6 +164,9 @@ "AgentPath": { "type": "string" }, + "ApiPathString": { + "type": "string" + }, "AppBranding": { "description": "EXPERIMENTAL - app metadata returned by app-list APIs.", "properties": { @@ -1318,7 +1321,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 5ab8d7c63..34ccaa47e 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -27,7 +27,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -37,7 +37,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -84,6 +84,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "ApplyPatchApprovalParams": { "properties": { "callId": { @@ -640,7 +643,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ 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 46c3f0720..ab08c4211 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 @@ -6020,7 +6020,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/v2/AbsolutePathBuf" + "$ref": "#/definitions/v2/ApiPathString" }, "type": [ "array", @@ -6030,7 +6030,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/v2/AbsolutePathBuf" + "$ref": "#/definitions/v2/ApiPathString" }, "type": [ "array", @@ -6135,6 +6135,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "AppBranding": { "description": "EXPERIMENTAL - app metadata returned by app-list APIs.", "properties": { @@ -9372,7 +9375,7 @@ { "properties": { "path": { - "$ref": "#/definitions/v2/AbsolutePathBuf" + "$ref": "#/definitions/v2/ApiPathString" }, "type": { "enum": [ 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 e95d4c98a..5b0bca923 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 @@ -265,7 +265,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -275,7 +275,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -380,6 +380,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "AppBranding": { "description": "EXPERIMENTAL - app metadata returned by app-list APIs.", "properties": { @@ -5685,7 +5688,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ 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 a366c99a4..717c4c987 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json @@ -27,7 +27,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -37,7 +37,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -58,6 +58,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "AutoReviewDecisionSource": { "description": "[UNSTABLE] Source that produced a terminal approval auto-review decision.", "enum": [ @@ -78,7 +81,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ 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 bc081c7be..28fec1275 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json @@ -27,7 +27,7 @@ "read": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -37,7 +37,7 @@ "write": { "description": "This will be removed in favor of `entries`.", "items": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": [ "array", @@ -58,6 +58,9 @@ }, "type": "object" }, + "ApiPathString": { + "type": "string" + }, "FileSystemAccessMode": { "enum": [ "read", @@ -71,7 +74,7 @@ { "properties": { "path": { - "$ref": "#/definitions/AbsolutePathBuf" + "$ref": "#/definitions/ApiPathString" }, "type": { "enum": [ diff --git a/codex-rs/app-server-protocol/schema/typescript/ApiPathString.ts b/codex-rs/app-server-protocol/schema/typescript/ApiPathString.ts new file mode 100644 index 000000000..ba5a3a961 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/ApiPathString.ts @@ -0,0 +1,26 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * A UTF-8 path for preserving raw path compatibility at the app-server API + * boundary while Codex migrates to [`PathUri`]. + * + * Supports storing arbitrary strings read from the API and converting to and + * from [`PathUri`] using an explicitly selected native path convention. + * + * When converting from [`PathUri`], "native" refers to the supplied + * [`PathConvention`], which may be foreign to the operating system running + * this process. The inner string is private so path-producing code must convert + * from [`AbsolutePathBuf`] or use [`Self::from_path_uri`] instead of bypassing + * the intended conversion boundary. Non-UTF-8 paths are converted to UTF-8 + * lossily because this API value is serialized as a JSON string. + * + * Deserialization accepts any UTF-8 string without interpreting or validating + * it. That unrestricted construction path is intentionally available only to + * serde: Codex-internal code cannot construct this type directly from a raw + * `String` and is instead encouraged to convert through [`PathUri`] or + * [`AbsolutePathBuf`]. Relative path text remains valid until an operation + * such as [`Self::to_path_uri`] requires an absolute path. + */ +export type ApiPathString = string; diff --git a/codex-rs/app-server-protocol/schema/typescript/index.ts b/codex-rs/app-server-protocol/schema/typescript/index.ts index c9ad437e5..66fea07dc 100644 --- a/codex-rs/app-server-protocol/schema/typescript/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/index.ts @@ -3,6 +3,7 @@ export type { AbsolutePathBuf } from "./AbsolutePathBuf"; export type { AgentMessageInputContent } from "./AgentMessageInputContent"; export type { AgentPath } from "./AgentPath"; +export type { ApiPathString } from "./ApiPathString"; export type { ApplyPatchApprovalParams } from "./ApplyPatchApprovalParams"; export type { ApplyPatchApprovalResponse } from "./ApplyPatchApprovalResponse"; export type { AuthMode } from "./AuthMode"; 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 e29263b95..68b1fb660 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts @@ -1,15 +1,15 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -import type { AbsolutePathBuf } from "../AbsolutePathBuf"; +import type { ApiPathString } from "../ApiPathString"; import type { FileSystemSandboxEntry } from "./FileSystemSandboxEntry"; export type AdditionalFileSystemPermissions = { /** * This will be removed in favor of `entries`. */ -read: Array | null, +read: Array | null, /** * This will be removed in favor of `entries`. */ -write: Array | null, globScanMaxDepth?: number, entries?: Array, }; +write: Array | null, globScanMaxDepth?: number, entries?: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemPath.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemPath.ts index 2efc7eab3..0733c0204 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemPath.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FileSystemPath.ts @@ -1,7 +1,7 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -import type { AbsolutePathBuf } from "../AbsolutePathBuf"; +import type { ApiPathString } from "../ApiPathString"; import type { FileSystemSpecialPath } from "./FileSystemSpecialPath"; -export type FileSystemPath = { "type": "path", path: AbsolutePathBuf, } | { "type": "glob_pattern", pattern: string, } | { "type": "special", value: FileSystemSpecialPath, }; +export type FileSystemPath = { "type": "path", path: ApiPathString, } | { "type": "glob_pattern", pattern: string, } | { "type": "special", value: FileSystemSpecialPath, }; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 64ed11c64..9339e0916 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -3504,7 +3504,7 @@ mod tests { additional_permissions: Some(v2::AdditionalPermissionProfile { network: None, file_system: Some(v2::AdditionalFileSystemPermissions { - read: Some(vec![absolute_path("/tmp/allowed")]), + read: Some(vec![absolute_path("/tmp/allowed").into()]), write: None, glob_scan_max_depth: None, entries: None, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index f556890a9..655a9dea2 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -37,6 +37,7 @@ use serde::Serialize; use serde_json::Value as JsonValue; use serde_with::serde_as; use std::collections::HashMap; +use std::io; use std::path::PathBuf; use ts_rs::TS; @@ -687,9 +688,11 @@ impl From for GuardianApprovalReviewAction { } } -impl From for CoreGuardianAssessmentAction { - fn from(value: GuardianApprovalReviewAction) -> Self { - match value { +impl TryFrom for CoreGuardianAssessmentAction { + type Error = io::Error; + + fn try_from(value: GuardianApprovalReviewAction) -> Result { + Ok(match value { GuardianApprovalReviewAction::Command { source, command, @@ -742,9 +745,9 @@ impl From for CoreGuardianAssessmentAction { permissions, } => Self::RequestPermissions { reason, - permissions: permissions.into(), + permissions: permissions.try_into()?, }, - } + }) } } 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 45fc5c7e1..cc93e3a6c 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/permissions.rs @@ -16,9 +16,12 @@ 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::ApiPathString; +use codex_utils_path_uri::PathConvention; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; +use std::io; use std::num::NonZeroUsize; use std::path::PathBuf; use ts_rs::TS; @@ -54,9 +57,9 @@ impl From for NetworkApprovalContext { #[ts(export_to = "v2/")] pub struct AdditionalFileSystemPermissions { /// This will be removed in favor of `entries`. - pub read: Option>, + pub read: Option>, /// This will be removed in favor of `entries`. - pub write: Option>, + pub write: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] pub glob_scan_max_depth: Option, @@ -65,27 +68,32 @@ pub struct AdditionalFileSystemPermissions { pub entries: Option>, } -impl From for AdditionalFileSystemPermissions { - fn from(value: CoreFileSystemPermissions) -> Self { +// TODO(anp): Remove this conversion once core permission paths use PathUri. +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() }, + path: FileSystemPath::Path { + path: ApiPathString::from_abs_path(path), + }, access: FileSystemAccessMode::Read, })); } if let Some(paths) = write.as_ref() { entries.extend(paths.iter().map(|path| FileSystemSandboxEntry { - path: FileSystemPath::Path { path: path.clone() }, + path: FileSystemPath::Path { + path: ApiPathString::from_abs_path(path), + }, access: FileSystemAccessMode::Write, })); } Self { - read, - write, + read: read.map(|paths| paths.iter().map(ApiPathString::from_abs_path).collect()), + write: write.map(|paths| paths.iter().map(ApiPathString::from_abs_path).collect()), glob_scan_max_depth: None, entries: Some(entries), } @@ -106,21 +114,50 @@ impl From for AdditionalFileSystemPermissions { } } -impl From for CoreFileSystemPermissions { - fn from(value: AdditionalFileSystemPermissions) -> Self { +// TODO(anp): Remove this conversion once core permission paths use PathUri. +impl TryFrom for CoreFileSystemPermissions { + type Error = io::Error; + + fn try_from(value: AdditionalFileSystemPermissions) -> Result { let mut permissions = if let Some(entries) = value.entries { Self { entries: entries .into_iter() - .map(CoreFileSystemSandboxEntry::from) - .collect(), + .map(CoreFileSystemSandboxEntry::::try_from) + .collect::>()?, glob_scan_max_depth: None, } } else { - CoreFileSystemPermissions::from_read_write_roots(value.read, value.write) + let read = value + .read + .map(|paths| { + paths + .into_iter() + .map(|path| { + path.to_path_uri(PathConvention::native()) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))? + .to_abs_path() + }) + .collect::>>() + }) + .transpose()?; + let write = value + .write + .map(|paths| { + paths + .into_iter() + .map(|path| { + path.to_path_uri(PathConvention::native()) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))? + .to_abs_path() + }) + .collect::>>() + }) + .transpose()?; + CoreFileSystemPermissions::from_read_write_roots(read, write) }; permissions.glob_scan_max_depth = value.glob_scan_max_depth; - permissions + Ok(permissions) } } @@ -156,6 +193,7 @@ 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 { Self { @@ -165,12 +203,17 @@ impl From for RequestPermissionProfile { } } -impl From for CoreRequestPermissionProfile { - fn from(value: RequestPermissionProfile) -> Self { - Self { +impl TryFrom for CoreRequestPermissionProfile { + type Error = io::Error; + + fn try_from(value: RequestPermissionProfile) -> Result { + Ok(Self { network: value.network.map(CoreNetworkPermissions::from), - file_system: value.file_system.map(CoreFileSystemPermissions::from), - } + file_system: value + .file_system + .map(CoreFileSystemPermissions::::try_from) + .transpose()?, + }) } } @@ -231,16 +274,20 @@ impl From for CoreFileSystemSpecialPath { #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type")] #[ts(export_to = "v2/")] +// TODO(anp): Rename this type to distinguish it from the generic protocol FileSystemPath. pub enum FileSystemPath { - Path { path: AbsolutePathBuf }, + Path { path: ApiPathString }, GlobPattern { pattern: String }, Special { value: FileSystemSpecialPath }, } -impl From for FileSystemPath { - fn from(value: CoreFileSystemPath) -> Self { +// TODO(anp): Remove this conversion once core permission paths use PathUri. +impl From> for FileSystemPath { + fn from(value: CoreFileSystemPath) -> Self { match value { - CoreFileSystemPath::Path { path } => Self::Path { path }, + CoreFileSystemPath::Path { path } => Self::Path { + path: ApiPathString::from_abs_path(&path), + }, CoreFileSystemPath::GlobPattern { pattern } => Self::GlobPattern { pattern }, CoreFileSystemPath::Special { value } => Self::Special { value: value.into(), @@ -249,15 +296,23 @@ impl From for FileSystemPath { } } -impl From for CoreFileSystemPath { - fn from(value: FileSystemPath) -> Self { - match value { - FileSystemPath::Path { path } => Self::Path { path }, +// TODO(anp): Remove this conversion once core permission paths use PathUri. +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()?, + }, FileSystemPath::GlobPattern { pattern } => Self::GlobPattern { pattern }, FileSystemPath::Special { value } => Self::Special { value: value.into(), }, - } + }) } } @@ -269,8 +324,9 @@ pub struct FileSystemSandboxEntry { pub access: FileSystemAccessMode, } -impl From for FileSystemSandboxEntry { - fn from(value: CoreFileSystemSandboxEntry) -> Self { +// TODO(anp): Remove this conversion once core permission paths use PathUri. +impl From> for FileSystemSandboxEntry { + fn from(value: CoreFileSystemSandboxEntry) -> Self { Self { path: value.path.into(), access: value.access.into(), @@ -278,12 +334,14 @@ impl From for FileSystemSandboxEntry { } } -impl From for CoreFileSystemSandboxEntry { - fn from(value: FileSystemSandboxEntry) -> Self { - Self { - path: value.path.into(), +impl TryFrom for CoreFileSystemSandboxEntry { + type Error = io::Error; + + fn try_from(value: FileSystemSandboxEntry) -> Result { + Ok(Self { + path: value.path.try_into()?, access: value.access.to_core(), - } + }) } } @@ -375,6 +433,7 @@ 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 { Self { @@ -384,12 +443,17 @@ impl From for AdditionalPermissionProfile { } } -impl From for CoreAdditionalPermissionProfile { - fn from(value: AdditionalPermissionProfile) -> Self { - Self { +impl TryFrom for CoreAdditionalPermissionProfile { + type Error = io::Error; + + fn try_from(value: AdditionalPermissionProfile) -> Result { + Ok(Self { network: value.network.map(CoreNetworkPermissions::from), - file_system: value.file_system.map(CoreFileSystemPermissions::from), - } + file_system: value + .file_system + .map(CoreFileSystemPermissions::::try_from) + .transpose()?, + }) } } @@ -405,12 +469,17 @@ pub struct GrantedPermissionProfile { pub file_system: Option, } -impl From for CoreAdditionalPermissionProfile { - fn from(value: GrantedPermissionProfile) -> Self { - Self { +impl TryFrom for CoreAdditionalPermissionProfile { + type Error = io::Error; + + fn try_from(value: GrantedPermissionProfile) -> Result { + Ok(Self { network: value.network.map(CoreNetworkPermissions::from), - file_system: value.file_system.map(CoreFileSystemPermissions::from), - } + file_system: value + .file_system + .map(CoreFileSystemPermissions::::try_from) + .transpose()?, + }) } } diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index 7b2b45c7e..2567e386c 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -35,6 +35,7 @@ use codex_protocol::user_input::UserInput as CoreUserInput; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::test_path_buf; +use codex_utils_path_uri::ApiPathString; use pretty_assertions::assert_eq; use serde_json::Value as JsonValue; use serde_json::json; @@ -379,8 +380,8 @@ fn external_agent_config_import_params_accept_legacy_plugin_details() { } #[test] -fn command_execution_request_approval_rejects_relative_additional_permission_paths() { - let err = serde_json::from_value::(json!({ +fn command_execution_request_approval_localization_rejects_relative_additional_permission_paths() { + let params = serde_json::from_value::(json!({ "threadId": "thr_123", "turnId": "turn_123", "itemId": "call_123", @@ -401,12 +402,14 @@ fn command_execution_request_approval_rejects_relative_additional_permission_pat "proposedNetworkPolicyAmendments": null, "availableDecisions": null })) - .expect_err("relative additional permission paths should fail"); - assert!( - err.to_string() - .contains("AbsolutePathBuf deserialized without a base path"), - "unexpected error: {err}" - ); + .expect("API paths should deserialize before localization"); + let additional_permissions = params + .additional_permissions + .expect("additional permissions should be present"); + + let err = CoreAdditionalPermissionProfile::try_from(additional_permissions) + .expect_err("relative additional permission paths should fail localization"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); } #[test] @@ -451,12 +454,12 @@ fn permissions_request_approval_uses_request_permission_profile() { }), file_system: Some(AdditionalFileSystemPermissions { read: Some(vec![ - AbsolutePathBuf::try_from(PathBuf::from(read_only_path)) - .expect("path must be absolute"), + serde_json::from_value(json!(read_only_path)) + .expect("API path string should deserialize") ]), write: Some(vec![ - AbsolutePathBuf::try_from(PathBuf::from(read_write_path)) - .expect("path must be absolute"), + serde_json::from_value(json!(read_write_path)) + .expect("API path string should deserialize") ]), glob_scan_max_depth: None, entries: None, @@ -465,7 +468,8 @@ fn permissions_request_approval_uses_request_permission_profile() { ); assert_eq!( - CoreRequestPermissionProfile::from(params.permissions), + CoreRequestPermissionProfile::try_from(params.permissions) + .expect("API paths should convert to native paths"), CoreRequestPermissionProfile { network: Some(CoreNetworkPermissions { enabled: Some(true), @@ -559,7 +563,8 @@ fn additional_file_system_permissions_preserves_canonical_entries() { } ); assert_eq!( - CoreFileSystemPermissions::from(permissions), + CoreFileSystemPermissions::try_from(permissions) + .expect("API paths should convert to native paths"), core_permissions ); } @@ -574,23 +579,25 @@ fn additional_file_system_permissions_populates_entries_for_legacy_roots() { ); let permissions = AdditionalFileSystemPermissions::from(core_permissions.clone()); + let read_only_api_path = ApiPathString::from_abs_path(&read_only_path); + let read_write_api_path = ApiPathString::from_abs_path(&read_write_path); assert_eq!( permissions, AdditionalFileSystemPermissions { - read: Some(vec![read_only_path.clone()]), - write: Some(vec![read_write_path.clone()]), + read: Some(vec![read_only_api_path.clone()]), + write: Some(vec![read_write_api_path.clone()]), glob_scan_max_depth: None, entries: Some(vec![ FileSystemSandboxEntry { path: FileSystemPath::Path { - path: read_only_path, + path: read_only_api_path, }, access: FileSystemAccessMode::Read, }, FileSystemSandboxEntry { path: FileSystemPath::Path { - path: read_write_path, + path: read_write_api_path, }, access: FileSystemAccessMode::Write, }, @@ -598,7 +605,8 @@ fn additional_file_system_permissions_populates_entries_for_legacy_roots() { } ); assert_eq!( - CoreFileSystemPermissions::from(permissions), + CoreFileSystemPermissions::try_from(permissions) + .expect("API paths should convert to native paths"), core_permissions ); } @@ -667,12 +675,12 @@ fn permissions_request_approval_response_uses_granted_permission_profile_without }), file_system: Some(AdditionalFileSystemPermissions { read: Some(vec![ - AbsolutePathBuf::try_from(PathBuf::from(read_only_path)) - .expect("path must be absolute"), + serde_json::from_value(json!(read_only_path)) + .expect("API path string should deserialize") ]), write: Some(vec![ - AbsolutePathBuf::try_from(PathBuf::from(read_write_path)) - .expect("path must be absolute"), + serde_json::from_value(json!(read_write_path)) + .expect("API path string should deserialize") ]), glob_scan_max_depth: None, entries: None, @@ -681,7 +689,8 @@ fn permissions_request_approval_response_uses_granted_permission_profile_without ); assert_eq!( - CoreAdditionalPermissionProfile::from(response.permissions), + CoreAdditionalPermissionProfile::try_from(response.permissions) + .expect("API paths should convert to native paths"), CoreAdditionalPermissionProfile { network: Some(CoreNetworkPermissions { enabled: Some(true), diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index d1099d3b1..868cef81f 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -786,6 +786,8 @@ pub(crate) async fn apply_bespoke_event_handling( .await; let pending_response = PendingRequestPermissionsResponse { call_id: request.call_id, + conversation_id, + turn_id: request.turn_id, requested_permissions, request_cwd, pending_request_id, @@ -948,15 +950,14 @@ pub(crate) async fn apply_bespoke_event_handling( codex_error_info: ev.codex_error_info.map(V2CodexErrorInfo::from), additional_details: None, }; - handle_error(conversation_id, turn_error.clone(), &thread_state).await; - outgoing - .send_server_notification(ServerNotification::Error(ErrorNotification { - error: turn_error.clone(), - will_retry: false, - thread_id: conversation_id.to_string(), - turn_id: event_turn_id.clone(), - })) - .await; + handle_error_notification( + conversation_id, + &event_turn_id, + turn_error, + &outgoing, + &thread_state, + ) + .await; } EventMsg::StreamError(ev) => { // We don't need to update the turn summary store for stream errors as they are intermediate error states for retries, @@ -1641,6 +1642,24 @@ async fn handle_error( state.turn_summary.last_error = Some(error); } +async fn handle_error_notification( + conversation_id: ThreadId, + event_turn_id: &str, + error: TurnError, + outgoing: &ThreadScopedOutgoingMessageSender, + thread_state: &Arc>, +) { + handle_error(conversation_id, error.clone(), thread_state).await; + outgoing + .send_server_notification(ServerNotification::Error(ErrorNotification { + error, + will_retry: false, + thread_id: conversation_id.to_string(), + turn_id: event_turn_id.to_string(), + })) + .await; +} + async fn on_request_user_input_response( event_turn_id: String, pending_request_id: RequestId, @@ -1796,6 +1815,8 @@ async fn on_request_permissions_response( ) { let PendingRequestPermissionsResponse { call_id, + conversation_id, + turn_id, requested_permissions, request_cwd, pending_request_id, @@ -1806,12 +1827,34 @@ async fn on_request_permissions_response( let response = receiver.await; resolve_server_request_on_thread_listener(&thread_state, pending_request_id.clone()).await; drop(request_permissions_guard); - let Some(response) = request_permissions_response_from_client_result( + let response = match request_permissions_response_from_client_result( requested_permissions, response, request_cwd.as_path(), - ) else { - return; + ) { + 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}"); + handle_error_notification( + conversation_id, + &turn_id, + TurnError { + message, + codex_error_info: None, + additional_details: None, + }, + &outgoing, + &thread_state, + ) + .await; + if let Err(err) = conversation.submit(Op::Interrupt).await { + error!("failed to interrupt turn after invalid permission paths: {err}"); + } + return; + } }; outgoing.track_effective_permissions_approval_response(pending_request_id, response.clone()); @@ -1828,6 +1871,8 @@ async fn on_request_permissions_response( struct PendingRequestPermissionsResponse { call_id: String, + conversation_id: ThreadId, + turn_id: String, requested_permissions: CoreRequestPermissionProfile, request_cwd: AbsolutePathBuf, pending_request_id: RequestId, @@ -1840,25 +1885,25 @@ fn request_permissions_response_from_client_result( requested_permissions: CoreRequestPermissionProfile, response: std::result::Result, cwd: &std::path::Path, -) -> Option { +) -> std::io::Result> { let value = match response { Ok(Ok(value)) => value, - Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return None, + Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return Ok(None), Ok(Err(err)) => { error!("request failed with client error: {err:?}"); - return Some(CoreRequestPermissionsResponse { + return Ok(Some(CoreRequestPermissionsResponse { permissions: Default::default(), scope: CorePermissionGrantScope::Turn, strict_auto_review: false, - }); + })); } Err(err) => { error!("request failed: {err:?}"); - return Some(CoreRequestPermissionsResponse { + return Ok(Some(CoreRequestPermissionsResponse { permissions: Default::default(), scope: CorePermissionGrantScope::Turn, strict_auto_review: false, - }); + })); } }; @@ -1879,23 +1924,23 @@ fn request_permissions_response_from_client_result( ) { error!("strict auto review is only supported for turn-scoped permission grants"); - return Some(CoreRequestPermissionsResponse { + return Ok(Some(CoreRequestPermissionsResponse { permissions: Default::default(), scope: CorePermissionGrantScope::Turn, strict_auto_review: false, - }); + })); } - let granted_permissions: CoreAdditionalPermissionProfile = response.permissions.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() }; - Some(CoreRequestPermissionsResponse { + Ok(Some(CoreRequestPermissionsResponse { permissions, scope: response.scope.to_core(), strict_auto_review, - }) + })) } const REVIEW_FALLBACK_MESSAGE: &str = "Reviewer failed to output a response."; @@ -2899,7 +2944,8 @@ mod tests { CoreRequestPermissionProfile::default(), Ok(Err(error)), std::env::current_dir().expect("current dir").as_path(), - ); + ) + .expect("paths should localize"); assert_eq!(response, None); } @@ -2994,6 +3040,7 @@ mod tests { }))), cwd.as_path(), ) + .expect("paths should localize") .expect("response should be accepted"); assert_eq!( @@ -3017,6 +3064,7 @@ mod tests { }))), std::env::current_dir().expect("current dir").as_path(), ) + .expect("paths should localize") .expect("response should be accepted"); assert_eq!( @@ -3044,6 +3092,7 @@ mod tests { }))), std::env::current_dir().expect("current dir").as_path(), ) + .expect("paths should localize") .expect("response should be accepted"); assert_eq!( @@ -3075,6 +3124,7 @@ mod tests { }))), std::env::current_dir().expect("current dir").as_path(), ) + .expect("paths should localize") .expect("response should be accepted"); assert_eq!(response.scope, CorePermissionGrantScope::Turn); @@ -3110,6 +3160,7 @@ mod tests { }))), cwd.as_path(), ) + .expect("paths should localize") .expect("response should be accepted"); assert_eq!( @@ -3156,6 +3207,7 @@ mod tests { }))), request_cwd.as_path(), ) + .expect("paths should localize") .expect("response should be accepted"); assert_eq!( @@ -3197,6 +3249,7 @@ mod tests { }))), cwd.as_path(), ) + .expect("paths should localize") .expect("response should be accepted"); assert_eq!( diff --git a/codex-rs/app-server/src/transport_tests.rs b/codex-rs/app-server/src/transport_tests.rs index 57a6c4c45..4b9b387aa 100644 --- a/codex-rs/app-server/src/transport_tests.rs +++ b/codex-rs/app-server/src/transport_tests.rs @@ -260,7 +260,7 @@ async fn command_execution_request_approval_strips_additional_permissions_withou network: None, file_system: Some( codex_app_server_protocol::AdditionalFileSystemPermissions { - read: Some(vec![absolute_path("/tmp/allowed")]), + read: Some(vec![absolute_path("/tmp/allowed").into()]), write: None, glob_scan_max_depth: None, entries: None, @@ -325,7 +325,7 @@ async fn command_execution_request_approval_keeps_additional_permissions_with_ca network: None, file_system: Some( codex_app_server_protocol::AdditionalFileSystemPermissions { - read: Some(vec![absolute_path("/tmp/allowed")]), + read: Some(vec![absolute_path("/tmp/allowed").into()]), write: None, glob_scan_max_depth: None, entries: None, diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index fb094ced6..b000cba12 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -151,6 +151,7 @@ codex-cli = { workspace = true } codex-mcp = { workspace = true } core_test_support = { workspace = true } codex-utils-cargo-bin = { workspace = true } +codex-utils-path-uri = { workspace = true } assert_matches = { workspace = true } chrono = { workspace = true, features = ["serde"] } insta = { workspace = true } diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 479512e56..e4ab34777 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -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, }, diff --git a/codex-rs/tui/src/app/side.rs b/codex-rs/tui/src/app/side.rs index 61fed2815..852017521 100644 --- a/codex-rs/tui/src/app/side.rs +++ b/codex-rs/tui/src/app/side.rs @@ -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, diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 41a72a47c..c3c83e6d4 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -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!( diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 1783f0ec6..c83920556 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -210,9 +210,9 @@ impl App { &self, thread_id: ThreadId, request: &ServerRequest, - ) -> Option { + ) -> std::io::Result> { 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 }; diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs index 922e0ffd5..2208bcabb 100644 --- a/codex-rs/tui/src/app_server_approval_conversions.rs +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -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, + }]), }), } ); diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index f318dd210..b321aeb15 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -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)), }) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index caed3297b..80be78555 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -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 { + 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 { diff --git a/codex-rs/tui/src/chatwidget/protocol_requests.rs b/codex-rs/tui/src/chatwidget/protocol_requests.rs index 5b53f44fd..2f33c9809 100644 --- a/codex-rs/tui/src/chatwidget/protocol_requests.rs +++ b/codex-rs/tui/src/chatwidget/protocol_requests.rs @@ -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, }); } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 1dcb848d5..a34d39384 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -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; diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs index 18fc15564..69d75ddc6 100644 --- a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -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, diff --git a/codex-rs/utils/path-uri/src/api_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs index a80fbdae9..030477776 100644 --- a/codex-rs/utils/path-uri/src/api_path_string.rs +++ b/codex-rs/utils/path-uri/src/api_path_string.rs @@ -16,9 +16,9 @@ use ts_rs::TS; /// /// When converting from [`PathUri`], "native" refers to the supplied /// [`PathConvention`], which may be foreign to the operating system running -/// this process. The inner string is private so path-producing code must use -/// [`Self::from_abs_path`] or [`Self::from_path_uri`] instead of bypassing the -/// intended conversion boundary. Non-UTF-8 paths are converted to UTF-8 +/// this process. The inner string is private so path-producing code must convert +/// from [`AbsolutePathBuf`] or use [`Self::from_path_uri`] instead of bypassing +/// the intended conversion boundary. Non-UTF-8 paths are converted to UTF-8 /// lossily because this API value is serialized as a JSON string. /// /// Deserialization accepts any UTF-8 string without interpreting or validating @@ -102,6 +102,12 @@ impl ApiPathString { } } +impl From for ApiPathString { + fn from(path: AbsolutePathBuf) -> Self { + Self::from_abs_path(&path) + } +} + fn parse_posix_path(path: &str) -> Option { let path = path.strip_prefix('/')?; if path.contains('\0') { diff --git a/codex-rs/utils/path-uri/src/api_path_string_tests.rs b/codex-rs/utils/path-uri/src/api_path_string_tests.rs index 8b849f178..3fbdb3dec 100644 --- a/codex-rs/utils/path-uri/src/api_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/api_path_string_tests.rs @@ -444,7 +444,7 @@ fn renders_an_absolute_path_using_the_host_convention() { .expect("native path should be absolute"); assert_eq!( - ApiPathString::from_abs_path(&path), + ApiPathString::from(path), ApiPathString(native_path.to_string()) ); }