diff --git a/codex-rs/app-server-protocol/schema/typescript/ServerRequest.ts b/codex-rs/app-server-protocol/schema/typescript/ServerRequest.ts index 80e9ffc116..89a5440056 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ServerRequest.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ServerRequest.ts @@ -16,4 +16,4 @@ import type { ToolRequestUserInputParams } from "./v2/ToolRequestUserInputParams /** * Request initiated from the server and sent to the client. */ -export type ServerRequest = { "method": "item/commandExecution/requestApproval", id: RequestId, params: CommandExecutionRequestApprovalParams, } | { "method": "item/fileChange/requestApproval", id: RequestId, params: FileChangeRequestApprovalParams, } | { "method": "item/tool/requestUserInput", id: RequestId, params: ToolRequestUserInputParams, } | { "method": "mcpServer/elicitation/request", id: RequestId, params: McpServerElicitationRequestParams, } | { "method": "item/permissions/requestApproval", id: RequestId, params: PermissionsRequestApprovalParams, } | { "method": "item/tool/call", id: RequestId, params: DynamicToolCallParams, } | { "method": "account/chatgptAuthTokens/refresh", id: RequestId, params: ChatgptAuthTokensRefreshParams, } | { "method": "attestation/generate", id: RequestId, params: AttestationGenerateParams, } | { "method": "applyPatchApproval", id: RequestId, params: ApplyPatchApprovalParams, } | { "method": "execCommandApproval", id: RequestId, params: ExecCommandApprovalParams, }; +export type ServerRequest ={ "method": "item/commandExecution/requestApproval", id: RequestId, params: CommandExecutionRequestApprovalParams, } | { "method": "item/fileChange/requestApproval", id: RequestId, params: FileChangeRequestApprovalParams, } | { "method": "item/tool/requestUserInput", id: RequestId, params: ToolRequestUserInputParams, } | { "method": "mcpServer/elicitation/request", id: RequestId, params: McpServerElicitationRequestParams, } | { "method": "item/permissions/requestApproval", id: RequestId, params: PermissionsRequestApprovalParams, } | { "method": "item/tool/call", id: RequestId, params: DynamicToolCallParams, } | { "method": "account/chatgptAuthTokens/refresh", id: RequestId, params: ChatgptAuthTokensRefreshParams, } | { "method": "attestation/generate", id: RequestId, params: AttestationGenerateParams, } | { "method": "applyPatchApproval", id: RequestId, params: ApplyPatchApprovalParams, } | { "method": "execCommandApproval", id: RequestId, params: ExecCommandApprovalParams, }; diff --git a/codex-rs/app-server-protocol/src/export.rs b/codex-rs/app-server-protocol/src/export.rs index 5e6c2ad015..91eca690f1 100644 --- a/codex-rs/app-server-protocol/src/export.rs +++ b/codex-rs/app-server-protocol/src/export.rs @@ -14,6 +14,9 @@ use crate::export_server_responses; use crate::protocol::common::EXPERIMENTAL_CLIENT_METHOD_PARAM_TYPES; use crate::protocol::common::EXPERIMENTAL_CLIENT_METHOD_RESPONSE_TYPES; use crate::protocol::common::EXPERIMENTAL_CLIENT_METHODS; +use crate::protocol::common::EXPERIMENTAL_SERVER_METHOD_PARAM_TYPES; +use crate::protocol::common::EXPERIMENTAL_SERVER_METHOD_RESPONSE_TYPES; +use crate::protocol::common::EXPERIMENTAL_SERVER_METHODS; use anyhow::Context; use anyhow::Result; use anyhow::anyhow; @@ -249,10 +252,10 @@ fn filter_experimental_ts(out_dir: &Path) -> Result<()> { let registered_fields = experimental_fields(); let experimental_method_types = experimental_method_types(); // Most generated TS files are filtered by schema processing, but - // `ClientRequest.ts` and any type with `#[experimental(...)]` fields need - // direct post-processing because they encode method/field information in - // file-local unions/interfaces. - filter_client_request_ts(out_dir, EXPERIMENTAL_CLIENT_METHODS)?; + // Request unions and types with `#[experimental(...)]` fields need direct + // post-processing because they encode method/field information locally. + filter_request_ts(out_dir, "ClientRequest.ts", EXPERIMENTAL_CLIENT_METHODS)?; + filter_request_ts(out_dir, "ServerRequest.ts", EXPERIMENTAL_SERVER_METHODS)?; filter_experimental_type_fields_ts(out_dir, ®istered_fields)?; remove_generated_type_files(out_dir, &experimental_method_types, "ts")?; Ok(()) @@ -261,10 +264,13 @@ fn filter_experimental_ts(out_dir: &Path) -> Result<()> { pub(crate) fn filter_experimental_ts_tree(tree: &mut BTreeMap) -> Result<()> { let registered_fields = experimental_fields(); let experimental_method_types = experimental_method_types(); - if let Some(content) = tree.get_mut(Path::new("ClientRequest.ts")) { - let filtered = - filter_client_request_ts_contents(std::mem::take(content), EXPERIMENTAL_CLIENT_METHODS); - *content = filtered; + for (file_name, experimental_methods) in [ + ("ClientRequest.ts", EXPERIMENTAL_CLIENT_METHODS), + ("ServerRequest.ts", EXPERIMENTAL_SERVER_METHODS), + ] { + if let Some(content) = tree.get_mut(Path::new(file_name)) { + *content = filter_request_ts_contents(std::mem::take(content), experimental_methods); + } } let mut fields_by_type_name: HashMap> = HashMap::new(); @@ -293,21 +299,21 @@ pub(crate) fn filter_experimental_ts_tree(tree: &mut BTreeMap) Ok(()) } -/// Removes union arms from `ClientRequest.ts` for methods marked experimental. -fn filter_client_request_ts(out_dir: &Path, experimental_methods: &[&str]) -> Result<()> { - let path = out_dir.join("ClientRequest.ts"); +/// Removes union arms from a generated request type for methods marked experimental. +fn filter_request_ts(out_dir: &Path, file_name: &str, experimental_methods: &[&str]) -> Result<()> { + let path = out_dir.join(file_name); if !path.exists() { return Ok(()); } let mut content = fs::read_to_string(&path).with_context(|| format!("Failed to read {}", path.display()))?; - content = filter_client_request_ts_contents(content, experimental_methods); + content = filter_request_ts_contents(content, experimental_methods); fs::write(&path, content).with_context(|| format!("Failed to write {}", path.display()))?; Ok(()) } -fn filter_client_request_ts_contents(mut content: String, experimental_methods: &[&str]) -> String { +fn filter_request_ts_contents(mut content: String, experimental_methods: &[&str]) -> String { let Some((prefix, body, suffix)) = split_type_alias(&content) else { return content; }; @@ -404,6 +410,7 @@ fn filter_experimental_schema(bundle: &mut Value) -> Result<()> { filter_experimental_fields_in_root(bundle, ®istered_fields); filter_experimental_fields_in_definitions(bundle, ®istered_fields); prune_experimental_methods(bundle, EXPERIMENTAL_CLIENT_METHODS); + prune_experimental_methods(bundle, EXPERIMENTAL_SERVER_METHODS); remove_experimental_method_type_definitions(bundle); Ok(()) } @@ -560,6 +567,8 @@ fn experimental_method_types() -> HashSet { collect_experimental_type_names(EXPERIMENTAL_CLIENT_METHOD_PARAM_TYPES, &mut type_names); collect_experimental_type_names(EXPERIMENTAL_CLIENT_METHOD_RESPONSE_TYPES, &mut type_names); collect_experimental_type_names(EXPERIMENTAL_CLIENT_METHOD_DEPENDENCY_TYPES, &mut type_names); + collect_experimental_type_names(EXPERIMENTAL_SERVER_METHOD_PARAM_TYPES, &mut type_names); + collect_experimental_type_names(EXPERIMENTAL_SERVER_METHOD_RESPONSE_TYPES, &mut type_names); type_names } @@ -2118,6 +2127,13 @@ mod tests { client_request_ts.contains("MockExperimentalMethodParams"), false ); + let server_request_ts = std::str::from_utf8( + fixture_tree + .get(Path::new("ServerRequest.ts")) + .ok_or_else(|| anyhow::anyhow!("missing ServerRequest.ts fixture"))?, + )?; + assert_eq!(server_request_ts.contains("currentTime/read"), false); + assert_eq!(server_request_ts.contains("CurrentTimeReadParams"), false); let typescript_index = std::str::from_utf8( fixture_tree .get(Path::new("index.ts")) @@ -2138,6 +2154,14 @@ mod tests { fixture_tree.contains_key(Path::new("v2/MockExperimentalMethodResponse.ts")), false ); + assert_eq!( + fixture_tree.contains_key(Path::new("v2/CurrentTimeReadParams.ts")), + false + ); + assert_eq!( + fixture_tree.contains_key(Path::new("v2/CurrentTimeReadResponse.ts")), + false + ); assert_eq!( fixture_tree.contains_key(Path::new("v2/RemoteControlClient.ts")), false diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index c1fac138a8..e5a882b3b5 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -1184,7 +1184,8 @@ client_request_definitions! { macro_rules! server_request_definitions { ( $( - $(#[$variant_meta:meta])* + $(#[experimental($reason:expr)])? + $(#[doc = $variant_doc:literal])* $variant:ident $(=> $wire:literal)? { params: $params:ty, response: $response:ty, @@ -1197,7 +1198,7 @@ macro_rules! server_request_definitions { #[serde(tag = "method", rename_all = "camelCase")] pub enum ServerRequest { $( - $(#[$variant_meta])* + $(#[doc = $variant_doc])* $(#[serde(rename = $wire)] #[ts(rename = $wire)])? $variant { #[serde(rename = "id")] @@ -1237,7 +1238,7 @@ macro_rules! server_request_definitions { #[serde(tag = "method", rename_all = "camelCase")] pub enum ServerResponse { $( - $(#[$variant_meta])* + $(#[doc = $variant_doc])* $(#[serde(rename = $wire)])? $variant { #[serde(rename = "id")] @@ -1281,6 +1282,22 @@ macro_rules! server_request_definitions { } } + pub(crate) const EXPERIMENTAL_SERVER_METHODS: &[&str] = &[ + $( + experimental_method_entry!($(#[experimental($reason)])? $(=> $wire)?), + )* + ]; + pub(crate) const EXPERIMENTAL_SERVER_METHOD_PARAM_TYPES: &[&str] = &[ + $( + experimental_type_entry!($(#[experimental($reason)])? $params), + )* + ]; + pub(crate) const EXPERIMENTAL_SERVER_METHOD_RESPONSE_TYPES: &[&str] = &[ + $( + experimental_type_entry!($(#[experimental($reason)])? $response), + )* + ]; + pub fn export_server_responses( out_dir: &::std::path::Path, ) -> ::std::result::Result<(), ::ts_rs::ExportError> { @@ -1470,6 +1487,13 @@ server_request_definitions! { response: v2::AttestationGenerateResponse, }, + #[experimental("currentTime/read")] + /// Read the current time from an external clock owned by the client. + CurrentTimeRead => "currentTime/read" { + params: v2::CurrentTimeReadParams, + response: v2::CurrentTimeReadResponse, + }, + /// DEPRECATED APIs below /// Request to approve a patch. /// This request is used for Turns started via the legacy APIs (i.e. SendUserTurn, SendUserMessage). @@ -2372,6 +2396,32 @@ mod tests { Ok(()) } + #[test] + fn serialize_current_time_read_request() -> Result<()> { + let params = v2::CurrentTimeReadParams { + thread_id: "thread-123".to_string(), + }; + let request = ServerRequest::CurrentTimeRead { + request_id: RequestId::Integer(10), + params: params.clone(), + }; + assert_eq!( + json!({ + "method": "currentTime/read", + "id": 10, + "params": { + "threadId": "thread-123" + } + }), + serde_json::to_value(&request)?, + ); + + let payload = ServerRequestPayload::CurrentTimeRead(params); + assert_eq!(request.id(), &RequestId::Integer(10)); + assert_eq!(payload.request_with_id(RequestId::Integer(10)), request); + Ok(()) + } + #[test] fn serialize_server_response() -> Result<()> { let response = ServerResponse::CommandExecutionRequestApproval { diff --git a/codex-rs/app-server-protocol/src/protocol/v2/current_time.rs b/codex-rs/app-server-protocol/src/protocol/v2/current_time.rs new file mode 100644 index 0000000000..9fd23f7981 --- /dev/null +++ b/codex-rs/app-server-protocol/src/protocol/v2/current_time.rs @@ -0,0 +1,20 @@ +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; +use ts_rs::TS; + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct CurrentTimeReadParams { + pub thread_id: String, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct CurrentTimeReadResponse { + /// Current time as whole Unix seconds. + #[ts(type = "number")] + pub current_time_at: i64, +} diff --git a/codex-rs/app-server-protocol/src/protocol/v2/mod.rs b/codex-rs/app-server-protocol/src/protocol/v2/mod.rs index b5fa9fdc65..1097b03811 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/mod.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/mod.rs @@ -6,6 +6,7 @@ mod attestation; mod collaboration_mode; mod command_exec; mod config; +mod current_time; mod environment; mod experimental_feature; mod feedback; @@ -32,6 +33,7 @@ pub use attestation::*; pub use collaboration_mode::*; pub use command_exec::*; pub use config::*; +pub use current_time::*; pub use environment::*; pub use experimental_feature::*; pub use feedback::*; diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 183c7c6007..b9c7899fc8 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1469,6 +1469,10 @@ When the client responds to `item/tool/requestUserInput`, the server emits `serv Desktop hosts that provide upstream attestation should set `capabilities.requestAttestation` during `initialize` and handle the server-initiated `attestation/generate` request. App-server issues it just in time before ChatGPT Codex requests that forward `x-oai-attestation`; the client responds with `{ "token": "v1." }`, where `token` is an opaque client-owned value. When app-server receives a client response, it forwards a consistent outer envelope such as `{ "v": 1, "s": 0, "t": "v1." }`, where `t` contains the client token unchanged. If app-server attempts attestation but fails within its own boundary, it sends the same envelope shape with an app-server status code and without `t` (`1 = timeout`, `2 = request failed`, `3 = request canceled`, `4 = malformed response`). If no initialized client opted into attestation, app-server omits `x-oai-attestation` for that upstream request. +### Current time + +When `[features.current_time_reminder]` is enabled with `clock_source = "external"`, app-server sends the client subscribed to the thread an experimental `currentTime/read` request with `{ "threadId": "thr_123" }` when a time reminder is due. The client responds with `{ "currentTimeAt": 1781717655 }`, where `currentTimeAt` is an integer Unix timestamp in seconds. A failed, canceled, timed-out, or malformed response stops the turn before the model request is sent. + ### MCP server elicitations MCP servers can interrupt a turn and ask the client for structured input via `mcpServer/elicitation/request`. diff --git a/codex-rs/app-server/src/current_time.rs b/codex-rs/app-server/src/current_time.rs new file mode 100644 index 0000000000..9b76d337a2 --- /dev/null +++ b/codex-rs/app-server/src/current_time.rs @@ -0,0 +1,146 @@ +use std::sync::Arc; +use std::sync::Weak; + +use anyhow::Context; +use anyhow::Result; +use anyhow::anyhow; +use anyhow::bail; +use chrono::DateTime; +use chrono::Utc; +use codex_app_server_protocol::CurrentTimeReadParams; +use codex_app_server_protocol::CurrentTimeReadResponse; +use codex_app_server_protocol::ServerRequestPayload; +use codex_core::TimeFuture; +use codex_core::TimeProvider; +use codex_protocol::ThreadId; +use tokio::time::Duration; +use tokio::time::Instant; +use tokio::time::timeout_at; + +use crate::outgoing_message::ConnectionId; +use crate::outgoing_message::OutgoingMessageSender; +use crate::thread_state::ThreadStateManager; + +const CURRENT_TIME_REQUEST_TIMEOUT: Duration = Duration::from_secs(5); + +pub(crate) fn app_server_time_provider( + outgoing: Arc, + thread_state_manager: ThreadStateManager, +) -> Arc { + Arc::new(AppServerTimeProvider { + outgoing: Arc::downgrade(&outgoing), + thread_state_manager, + }) +} + +struct AppServerTimeProvider { + outgoing: Weak, + thread_state_manager: ThreadStateManager, +} + +impl TimeProvider for AppServerTimeProvider { + fn current_time(&self, thread_id: ThreadId) -> TimeFuture<'_> { + let outgoing = self.outgoing.clone(); + let thread_state_manager = self.thread_state_manager.clone(); + Box::pin(async move { + let outgoing = outgoing + .upgrade() + .context("app-server current-time provider is unavailable")?; + request_current_time(outgoing, thread_state_manager, thread_id).await + }) + } +} + +async fn request_current_time( + outgoing: Arc, + thread_state_manager: ThreadStateManager, + thread_id: ThreadId, +) -> Result> { + let deadline = Instant::now() + CURRENT_TIME_REQUEST_TIMEOUT; + timeout_at( + deadline, + thread_state_manager.wait_for_thread_subscriber(thread_id), + ) + .await + .map_err(|_| { + anyhow!( + "timed out waiting for a client to subscribe to the thread after {}s", + CURRENT_TIME_REQUEST_TIMEOUT.as_secs() + ) + })?; + let connection_ids = thread_state_manager + .subscribed_connection_ids(thread_id) + .await; + let connection_id = require_single_current_time_connection(&connection_ids)?; + let connection_ids = [connection_id]; + let (request_id, rx) = outgoing + .send_request_to_connections( + Some(&connection_ids), + ServerRequestPayload::CurrentTimeRead(CurrentTimeReadParams { + thread_id: thread_id.to_string(), + }), + /*thread_id*/ None, + ) + .await; + + let result = match timeout_at(deadline, rx).await { + Ok(Ok(Ok(result))) => result, + Ok(Ok(Err(err))) => { + bail!( + "current-time request failed: code={} message={}", + err.code, + err.message + ); + } + Ok(Err(err)) => bail!("current-time request was canceled: {err}"), + Err(_) => { + let _canceled = outgoing.cancel_request(&request_id).await; + bail!( + "current-time request timed out after {}s", + CURRENT_TIME_REQUEST_TIMEOUT.as_secs() + ); + } + }; + let response: CurrentTimeReadResponse = + serde_json::from_value(result).context("invalid current-time response")?; + + DateTime::from_timestamp(response.current_time_at, 0) + .ok_or_else(|| anyhow!("current-time response is outside the supported range")) +} + +fn require_single_current_time_connection(connection_ids: &[ConnectionId]) -> Result { + // External clocks are not interchangeable, so do not choose one silently. + match connection_ids { + [connection_id] => Ok(*connection_id), + _ => bail!( + "expected exactly one client subscribed to the thread, found {}", + connection_ids.len() + ), + } +} + +#[cfg(test)] +mod tests { + use super::require_single_current_time_connection; + use crate::outgoing_message::ConnectionId; + + #[test] + fn current_time_connection_must_be_unambiguous() { + assert_eq!( + require_single_current_time_connection(&[ConnectionId(7)]).unwrap(), + ConnectionId(7) + ); + assert_eq!( + require_single_current_time_connection(&[]) + .unwrap_err() + .to_string(), + "expected exactly one client subscribed to the thread, found 0" + ); + assert_eq!( + require_single_current_time_connection(&[ConnectionId(7), ConnectionId(8)]) + .unwrap_err() + .to_string(), + "expected exactly one client subscribed to the thread, found 2" + ); + } +} diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index cac2db54eb..f4cd83d732 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -91,6 +91,7 @@ mod config_manager; mod config_manager_service; mod connection_cleanup; mod connection_rpc_gate; +mod current_time; mod dynamic_tools; mod error_code; mod extensions; diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 0f9f6d3321..ad29e05010 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -7,6 +7,7 @@ use std::sync::atomic::AtomicBool; use crate::attestation::app_server_attestation_provider; use crate::config_manager::ConfigManager; use crate::connection_rpc_gate::ConnectionRpcGate; +use crate::current_time::app_server_time_provider; use crate::error_code::invalid_request; use crate::extensions::ThreadExtensionDependencies; use crate::extensions::app_server_extension_event_sink; @@ -280,7 +281,6 @@ impl ConnectionSessionState { .get() .is_some_and(|session| session.supports_openai_form_elicitation) } - pub(crate) fn initialize(&self, session: InitializedConnectionSessionState) -> Result<(), ()> { self.initialized.set(session).map_err(|_| ()) } @@ -379,7 +379,10 @@ impl MessageProcessor { outgoing.clone(), thread_state_manager.clone(), )), - /*external_time_provider*/ None, + Some(app_server_time_provider( + outgoing.clone(), + thread_state_manager.clone(), + )), ) }); let models_manager = thread_manager.get_models_manager(); diff --git a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs index 1f333f1eb1..3e336cc2a5 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs @@ -1381,6 +1381,31 @@ mod thread_processor_behavior_tests { Ok(()) } + #[tokio::test] + async fn wait_for_thread_subscriber_unblocks_after_connection_attaches() -> Result<()> { + let manager = ThreadStateManager::new(); + let thread_id = ThreadId::from_string("ba62fd70-2ec2-4b1b-9d94-355694332dd2")?; + let connection = ConnectionId(1); + manager + .connection_initialized(connection, ConnectionCapabilities::default()) + .await; + + let wait_for_subscriber = manager.wait_for_thread_subscriber(thread_id); + let attach_connection = async { + tokio::task::yield_now().await; + manager + .try_add_connection_to_thread(thread_id, connection) + .await + }; + let ((), attached) = tokio::time::timeout(Duration::from_secs(1), async { + tokio::join!(wait_for_subscriber, attach_connection) + }) + .await?; + + assert!(attached); + Ok(()) + } + #[tokio::test] async fn closed_connection_cannot_be_reintroduced_by_auto_subscribe() -> Result<()> { let manager = ThreadStateManager::new(); diff --git a/codex-rs/app-server/src/thread_state.rs b/codex-rs/app-server/src/thread_state.rs index 6d2b48a4c8..890d4da5eb 100644 --- a/codex-rs/app-server/src/thread_state.rs +++ b/codex-rs/app-server/src/thread_state.rs @@ -329,6 +329,23 @@ impl ThreadStateManager { .min_by_key(|connection_id| connection_id.0) } + pub(crate) async fn wait_for_thread_subscriber(&self, thread_id: ThreadId) { + let mut has_connections = { + let mut state = self.state.lock().await; + state + .threads + .entry(thread_id) + .or_default() + .has_connections_watcher + .subscribe() + }; + while !*has_connections.borrow_and_update() { + if has_connections.changed().await.is_err() { + break; + } + } + } + pub(crate) async fn subscribed_connection_ids(&self, thread_id: ThreadId) -> Vec { let state = self.state.lock().await; state diff --git a/codex-rs/app-server/tests/suite/v2/current_time.rs b/codex-rs/app-server/tests/suite/v2/current_time.rs new file mode 100644 index 0000000000..74e734ae39 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/current_time.rs @@ -0,0 +1,130 @@ +use std::path::Path; + +use anyhow::Result; +use app_test_support::TestAppServer; +use app_test_support::create_final_assistant_message_sse_response; +use app_test_support::to_response; +use codex_app_server_protocol::CurrentTimeReadResponse; +use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ServerRequest; +use codex_app_server_protocol::ThreadStartParams; +use codex_app_server_protocol::ThreadStartResponse; +use codex_app_server_protocol::TurnStartParams; +use codex_app_server_protocol::TurnStartResponse; +use codex_app_server_protocol::UserInput; +use core_test_support::responses; +use core_test_support::skip_if_no_network; +use pretty_assertions::assert_eq; +use tempfile::TempDir; +use tokio::time::Duration; +use tokio::time::timeout; + +#[cfg(windows)] +const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(25); +#[cfg(not(windows))] +const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10); +const CURRENT_TIME_AT: i64 = 1_781_717_655; +const CURRENT_TIME_REMINDER: &str = "It is 2026-06-17 17:34:15 UTC."; + +#[tokio::test] +async fn current_time_read_round_trip_adds_reminder_to_model_input() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let response_mock = responses::mount_sse_once( + &server, + create_final_assistant_message_sse_response("Done")?, + ) + .await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut app_server = TestAppServer::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, app_server.initialize()).await??; + + let thread_request_id = app_server + .send_thread_start_request(ThreadStartParams::default()) + .await?; + let thread_response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_response_message(RequestId::Integer(thread_request_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response(thread_response)?; + + let turn_request_id = app_server + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![UserInput::Text { + text: "What time is it?".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let turn_response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_response_message(RequestId::Integer(turn_request_id)), + ) + .await??; + let _: TurnStartResponse = to_response(turn_response)?; + + let server_request = timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_request_message(), + ) + .await??; + let ServerRequest::CurrentTimeRead { request_id, params } = server_request else { + panic!("expected CurrentTimeRead request, got: {server_request:?}"); + }; + assert_eq!(params.thread_id, thread.id); + app_server + .send_response( + request_id, + serde_json::to_value(CurrentTimeReadResponse { + current_time_at: CURRENT_TIME_AT, + })?, + ) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + assert!( + response_mock + .single_request() + .message_input_texts("developer") + .iter() + .any(|text| text == CURRENT_TIME_REMINDER) + ); + Ok(()) +} + +fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { + std::fs::write( + codex_home.join("config.toml"), + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" +model_provider = "mock_provider" + +[features.current_time_reminder] +enabled = true +reminder_interval_model_requests = 1 +clock_source = "external" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index 9d07da0660..2745e8ee00 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -11,6 +11,7 @@ mod config_rpc; mod connection_handling_websocket; #[cfg(unix)] mod connection_handling_websocket_unix; +mod current_time; mod dynamic_tools; #[cfg(not(target_os = "windows"))] mod executor_mcp; diff --git a/codex-rs/apply-patch/src/invocation.rs b/codex-rs/apply-patch/src/invocation.rs index 0e0261c3b5..de9c735c59 100644 --- a/codex-rs/apply-patch/src/invocation.rs +++ b/codex-rs/apply-patch/src/invocation.rs @@ -1,9 +1,7 @@ use std::collections::HashMap; -use std::path::Path; use std::sync::LazyLock; use codex_exec_server::ExecutorFileSystem; -use codex_utils_absolute_path::AbsolutePathBuf; use tree_sitter::Parser; use tree_sitter::Query; use tree_sitter::QueryCursor; @@ -21,6 +19,7 @@ use crate::parser::Hunk; use crate::parser::ParseError; use crate::parser::parse_patch; use crate::unified_diff_from_chunks; +use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; use std::str::Utf8Error; use tree_sitter::LanguageError; @@ -51,15 +50,17 @@ pub enum ExtractHeredocError { FailedToFindHeredocBody, } -fn classify_shell_name(shell: &str) -> Option { - std::path::Path::new(shell) - .file_stem() - .and_then(|name| name.to_str()) - .map(str::to_ascii_lowercase) +fn classify_shell_name(shell: &str, convention: PathConvention) -> Option { + let basename = convention.path_segments(shell).next_back()?; + let stem = basename + .rsplit_once('.') + .and_then(|(stem, _extension)| (!stem.is_empty()).then_some(stem)) + .unwrap_or(basename); + Some(stem.to_ascii_lowercase()) } -fn classify_shell(shell: &str, flag: &str) -> Option { - classify_shell_name(shell).and_then(|name| match name.as_str() { +fn classify_shell(shell: &str, flag: &str, convention: PathConvention) -> Option { + classify_shell_name(shell, convention).and_then(|name| match name.as_str() { "bash" | "zsh" | "sh" if matches!(flag, "-lc" | "-c") => Some(ApplyPatchShell::Unix), "pwsh" | "powershell" if flag.eq_ignore_ascii_case("-command") => { Some(ApplyPatchShell::PowerShell) @@ -69,20 +70,24 @@ fn classify_shell(shell: &str, flag: &str) -> Option { }) } -fn can_skip_flag(shell: &str, flag: &str) -> bool { - classify_shell_name(shell).is_some_and(|name| { +fn can_skip_flag(shell: &str, flag: &str, convention: PathConvention) -> bool { + classify_shell_name(shell, convention).is_some_and(|name| { matches!(name.as_str(), "pwsh" | "powershell") && flag.eq_ignore_ascii_case("-noprofile") }) } -fn parse_shell_script(argv: &[String]) -> Option<(ApplyPatchShell, &str)> { +fn parse_shell_script<'a>(argv: &'a [String], cwd: &PathUri) -> Option<(ApplyPatchShell, &'a str)> { + let convention = cwd.infer_path_convention()?; match argv { - [shell, flag, script] => classify_shell(shell, flag).map(|shell_type| { + [shell, flag, script] => classify_shell(shell, flag, convention).map(|shell_type| { let script = script.as_str(); (shell_type, script) }), - [shell, skip_flag, flag, script] if can_skip_flag(shell, skip_flag) => { - classify_shell(shell, flag).map(|shell_type| { + [shell, skip_flag, flag, script] => { + if !can_skip_flag(shell, skip_flag, convention) { + return None; + } + classify_shell(shell, flag, convention).map(|shell_type| { let script = script.as_str(); (shell_type, script) }) @@ -103,7 +108,8 @@ fn extract_apply_patch_from_shell( } // TODO: make private once we remove tests in lib.rs -pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { +/// `cwd` supplies the path convention used to interpret the shell executable in `argv`. +pub fn maybe_parse_apply_patch(argv: &[String], cwd: &PathUri) -> MaybeApplyPatch { match argv { // Direct invocation: apply_patch [cmd, body] if APPLY_PATCH_COMMANDS.contains(&cmd.as_str()) => match parse_patch(body) { @@ -111,7 +117,7 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { Err(e) => MaybeApplyPatch::PatchParseError(e), }, // Shell heredoc form: (optional `cd &&`) apply_patch <<'EOF' ... - _ => match parse_shell_script(argv) { + _ => match parse_shell_script(argv, cwd) { Some((shell, script)) => match extract_apply_patch_from_shell(shell, script) { Ok((body, workdir)) => match parse_patch(&body) { Ok(mut source) => { @@ -130,11 +136,11 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { } } -/// cwd must be an absolute path so that we can resolve relative paths in the -/// patch. +/// `cwd` must identify an absolute environment-native path so relative patch paths can be +/// resolved without projecting them onto the app-server or exec-server host. pub async fn maybe_parse_apply_patch_verified( argv: &[String], - cwd: &AbsolutePathBuf, + cwd: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, ) -> MaybeApplyPatchVerified { @@ -145,13 +151,13 @@ pub async fn maybe_parse_apply_patch_verified( { return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation); } - if let Some((_, script)) = parse_shell_script(argv) + if let Some((_, script)) = parse_shell_script(argv, cwd) && parse_patch(script).is_ok() { return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation); } - match maybe_parse_apply_patch(argv) { + match maybe_parse_apply_patch(argv, cwd) { MaybeApplyPatch::Body(args) => verify_apply_patch_args(args, cwd, fs, sandbox).await, MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e), MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()), @@ -161,10 +167,22 @@ pub async fn maybe_parse_apply_patch_verified( pub async fn verify_apply_patch_args( args: ApplyPatchArgs, - cwd: &AbsolutePathBuf, + cwd: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, ) -> MaybeApplyPatchVerified { + match try_verify_apply_patch_args(args, cwd, fs, sandbox).await { + Ok(action) => MaybeApplyPatchVerified::Body(action), + Err(err) => MaybeApplyPatchVerified::CorrectnessError(err), + } +} + +async fn try_verify_apply_patch_args( + args: ApplyPatchArgs, + cwd: &PathUri, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, +) -> Result { let ApplyPatchArgs { patch, hunks, @@ -173,35 +191,24 @@ pub async fn verify_apply_patch_args( } = args; let effective_cwd = workdir .as_ref() - .map(|dir| cwd.join(Path::new(dir))) + .map(|dir| cwd.join(dir)) + .transpose()? .unwrap_or_else(|| cwd.clone()); let mut changes = HashMap::new(); for hunk in hunks { - let path = hunk.resolve_path(&effective_cwd); + let path = hunk.resolve_path(&effective_cwd)?; match hunk { Hunk::AddFile { contents, .. } => { - changes.insert( - path.into_path_buf(), - ApplyPatchFileChange::Add { content: contents }, - ); + changes.insert(path, ApplyPatchFileChange::Add { content: contents }); } Hunk::DeleteFile { .. } => { - let path_uri = PathUri::from_abs_path(&path); - let content = match fs.read_file_text(&path_uri, sandbox).await { - Ok(content) => content, - Err(e) => { - return MaybeApplyPatchVerified::CorrectnessError( - ApplyPatchError::IoError(IoError { - context: format!("Failed to read {}", path.display()), - source: e, - }), - ); - } - }; - changes.insert( - path.into_path_buf(), - ApplyPatchFileChange::Delete { content }, - ); + let content = fs.read_file_text(&path, sandbox).await.map_err(|source| { + ApplyPatchError::IoError(IoError { + context: format!("Failed to read {}", path.inferred_native_path_string()), + source, + }) + })?; + changes.insert(path, ApplyPatchFileChange::Delete { content }); } Hunk::UpdateFile { move_path, chunks, .. @@ -210,27 +217,24 @@ pub async fn verify_apply_patch_args( unified_diff, content: contents, .. - } = match unified_diff_from_chunks(&path, &chunks, fs, sandbox).await { - Ok(diff) => diff, - Err(e) => { - return MaybeApplyPatchVerified::CorrectnessError(e); - } - }; + } = unified_diff_from_chunks(&path, &chunks, fs, sandbox).await?; changes.insert( - path.into_path_buf(), + path, ApplyPatchFileChange::Update { unified_diff, - move_path: move_path.map(|p| effective_cwd.join(p).into_path_buf()), + move_path: move_path + .map(|path| effective_cwd.join(&path.to_string_lossy())) + .transpose()?, new_content: contents, }, ); } } } - MaybeApplyPatchVerified::Body(ApplyPatchAction { + Ok(ApplyPatchAction { changes, patch, - cwd: effective_cwd.into(), + cwd: effective_cwd, }) } @@ -392,7 +396,6 @@ mod tests { use crate::unified_diff_from_chunks; use assert_matches::assert_matches; use codex_exec_server::LOCAL_FS; - use codex_utils_absolute_path::test_support::PathExt; use pretty_assertions::assert_eq; use std::fs; use std::path::PathBuf; @@ -448,8 +451,22 @@ mod tests { }] } + #[track_caller] fn assert_match_args(args: Vec, expected_workdir: Option<&str>) { - match maybe_parse_apply_patch(&args) { + assert_match_args_with_cwd( + args, + &PathUri::parse("file:///workspace").expect("valid POSIX test cwd"), + expected_workdir, + ); + } + + #[track_caller] + fn assert_match_args_with_cwd( + args: Vec, + cwd: &PathUri, + expected_workdir: Option<&str>, + ) { + match maybe_parse_apply_patch(&args, cwd) { MaybeApplyPatch::Body(ApplyPatchArgs { hunks, workdir, .. }) => { assert_eq!(workdir.as_deref(), expected_workdir); assert_eq!(hunks, expected_single_add()); @@ -458,6 +475,7 @@ mod tests { } } + #[track_caller] fn assert_match(script: &str, expected_workdir: Option<&str>) { let args = args_bash(script); assert_match_args(args, expected_workdir); @@ -466,7 +484,10 @@ mod tests { fn assert_not_match(script: &str) { let args = args_bash(script); assert_matches!( - maybe_parse_apply_patch(&args), + maybe_parse_apply_patch( + &args, + &PathUri::parse("file:///workspace").expect("valid POSIX test cwd"), + ), MaybeApplyPatch::NotApplyPatch ); } @@ -479,7 +500,7 @@ mod tests { assert_matches!( maybe_parse_apply_patch_verified( &args, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), LOCAL_FS.as_ref(), /*sandbox*/ None, ) @@ -496,7 +517,7 @@ mod tests { assert_matches!( maybe_parse_apply_patch_verified( &args, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), LOCAL_FS.as_ref(), /*sandbox*/ None, ) @@ -516,7 +537,10 @@ mod tests { "#, ]); - match maybe_parse_apply_patch(&args) { + match maybe_parse_apply_patch( + &args, + &PathUri::parse("file:///workspace").expect("valid POSIX test cwd"), + ) { MaybeApplyPatch::Body(ApplyPatchArgs { hunks, .. }) => { assert_eq!( hunks, @@ -541,7 +565,10 @@ mod tests { "#, ]); - match maybe_parse_apply_patch(&args) { + match maybe_parse_apply_patch( + &args, + &PathUri::parse("file:///workspace").expect("valid POSIX test cwd"), + ) { MaybeApplyPatch::Body(ApplyPatchArgs { hunks, .. }) => { assert_eq!( hunks, @@ -580,7 +607,10 @@ mod tests { PATCH"#, ]); - match maybe_parse_apply_patch(&args) { + match maybe_parse_apply_patch( + &args, + &PathUri::parse("file:///workspace").expect("valid POSIX test cwd"), + ) { MaybeApplyPatch::Body(ApplyPatchArgs { hunks, workdir, .. }) => { assert_eq!(workdir, None); assert_eq!( @@ -614,6 +644,21 @@ PATCH"#, assert_match_args(args_pwsh(&script), /*expected_workdir*/ None); } + #[tokio::test] + async fn test_apply_patch_interception_uses_cwd_convention_for_windows_pwsh_path() { + let script = heredoc_script(""); + assert_match_args_with_cwd( + strs_to_strings(&[ + r"C:\Program Files\PowerShell\7\pwsh.exe", + "-NoProfile", + "-Command", + &script, + ]), + &PathUri::parse("file:///C:/windows").expect("valid Windows test cwd"), + /*expected_workdir*/ None, + ); + } + #[tokio::test] async fn test_cmd_heredoc_with_cd() { let script = heredoc_script("cd foo && "); @@ -707,9 +752,9 @@ PATCH"#, _ => panic!("Expected a single UpdateFile hunk"), }; - let path_abs = path.as_path().abs(); + let path_uri = PathUri::from_path(&path).expect("absolute test path"); let diff = - unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) + unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) .await .unwrap(); let expected_diff = r#"@@ -2,2 +2,2 @@ @@ -747,9 +792,9 @@ PATCH"#, _ => panic!("Expected a single UpdateFile hunk"), }; - let path_abs = path.as_path().abs(); + let path_uri = PathUri::from_path(&path).expect("absolute test path"); let diff = - unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) + unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) .await .unwrap(); let expected_diff = r#"@@ -3 +3,2 @@ @@ -787,7 +832,7 @@ PATCH"#, let result = maybe_parse_apply_patch_verified( &argv, - &AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), + &PathUri::from_path(session_dir.path()).expect("absolute test path"), LOCAL_FS.as_ref(), /*sandbox*/ None, ) @@ -799,7 +844,8 @@ PATCH"#, result, MaybeApplyPatchVerified::Body(ApplyPatchAction { changes: HashMap::from([( - session_dir.path().join(relative_path), + PathUri::from_path(session_dir.path().join(relative_path)) + .expect("absolute test path"), ApplyPatchFileChange::Update { unified_diff: r#"@@ -1 +1 @@ -session directory content @@ -811,9 +857,7 @@ PATCH"#, }, )]), patch: argv[1].clone(), - cwd: AbsolutePathBuf::from_absolute_path(session_dir.path()) - .unwrap() - .into(), + cwd: PathUri::from_path(session_dir.path()).expect("absolute test path"), }) ); } @@ -843,7 +887,7 @@ PATCH"#, let result = maybe_parse_apply_patch_verified( &argv, - &AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), + &PathUri::from_path(session_dir.path()).expect("absolute test path"), LOCAL_FS.as_ref(), /*sandbox*/ None, ) @@ -858,18 +902,18 @@ PATCH"#, worktree_dir.as_path() ); - let source_path = worktree_dir.join(source_name); + let source_path = + PathUri::from_path(worktree_dir.join(source_name)).expect("absolute test path"); let change = action .changes() - .get(source_path.as_path()) + .get(&source_path) .expect("source file change present"); match change { ApplyPatchFileChange::Update { move_path, .. } => { - assert_eq!( - move_path.as_deref(), - Some(worktree_dir.join(dest_name).as_path()) - ); + let expected_move_path = + PathUri::from_path(worktree_dir.join(dest_name)).expect("absolute test path"); + assert_eq!(move_path.as_ref(), Some(&expected_move_path)); } other => panic!("expected update change, got {other:?}"), } @@ -879,7 +923,7 @@ PATCH"#, async fn test_unreadable_destinations_still_verify() { let session_dir = tempdir().unwrap(); fs::write(session_dir.path().join("binary.dat"), [0xff, 0xfe, 0xfd]).unwrap(); - let cwd = AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(); + let cwd = PathUri::from_path(session_dir.path()).expect("absolute test path"); let add_argv = vec![ "apply_patch".to_string(), "*** Begin Patch\n*** Add File: binary.dat\n+text\n*** End Patch".to_string(), @@ -922,7 +966,7 @@ PATCH"#, let result = maybe_parse_apply_patch_verified( &argv, - &AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), + &PathUri::from_path(session_dir.path()).expect("absolute test path"), LOCAL_FS.as_ref(), /*sandbox*/ None, ) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index ff11c43242..f27610d6cc 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -6,7 +6,6 @@ mod streaming_parser; use std::collections::HashMap; use std::io; -use std::path::Path; use std::path::PathBuf; use anyhow::Context; @@ -15,8 +14,8 @@ use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::RemoveOptions; -use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use codex_utils_path_uri::PathUriParseError; pub use parser::Hunk; pub use parser::ParseError; use parser::ParseError::*; @@ -50,6 +49,9 @@ pub enum ApplyPatchError { /// Error that occurs while computing replacements when applying patch chunks #[error("{0}")] ComputeReplacements(String), + /// A patch path could not be resolved as a path URI. + #[error(transparent)] + PathUri(#[from] PathUriParseError), /// A raw patch body was provided without an explicit `apply_patch` invocation. #[error( "patch detected without explicit call to apply_patch. Rerun as [\"apply_patch\", \"\"]" @@ -109,7 +111,7 @@ pub enum ApplyPatchFileChange { }, Update { unified_diff: String, - move_path: Option, + move_path: Option, /// new_content that will result after the unified_diff is applied. new_content: String, }, @@ -134,7 +136,7 @@ pub enum MaybeApplyPatchVerified { /// construction, all paths should be absolute paths. #[derive(Debug, PartialEq)] pub struct ApplyPatchAction { - changes: HashMap, + changes: HashMap, /// The raw patch argument that can be used to apply the patch. i.e., if the /// original arg was parsed in "lenient" mode with a @@ -151,18 +153,15 @@ impl ApplyPatchAction { } /// Returns the changes that would be made by applying the patch. - pub fn changes(&self) -> &HashMap { + pub fn changes(&self) -> &HashMap { &self.changes } /// Should be used exclusively for testing. (Not worth the overhead of /// creating a feature flag for this.) - pub fn new_add_for_test(path: &AbsolutePathBuf, content: String) -> Self { + pub fn new_add_for_test(path: &PathUri, content: String) -> Self { #[expect(clippy::expect_used)] - let filename = path - .file_name() - .expect("path should not be empty") - .to_string_lossy(); + let filename = path.basename().expect("path should not be empty"); let patch = format!( r#"*** Begin Patch *** Update File: {filename} @@ -170,11 +169,11 @@ impl ApplyPatchAction { + {content} *** End Patch"#, ); - let changes = HashMap::from([(path.to_path_buf(), ApplyPatchFileChange::Add { content })]); + let changes = HashMap::from([(path.clone(), ApplyPatchFileChange::Add { content })]); #[expect(clippy::expect_used)] Self { changes, - cwd: path.parent().expect("path should have parent").into(), + cwd: path.parent().expect("path should have parent"), patch, } } @@ -276,7 +275,7 @@ impl ApplyPatchFailure { /// Applies the patch and prints the result to stdout/stderr. pub async fn apply_patch( patch: &str, - cwd: &AbsolutePathBuf, + cwd: &PathUri, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, fs: &dyn ExecutorFileSystem, @@ -315,7 +314,7 @@ pub async fn apply_patch( /// Applies hunks and continues to update stdout/stderr pub async fn apply_hunks( hunks: &[Hunk], - cwd: &AbsolutePathBuf, + cwd: &PathUri, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, fs: &dyn ExecutorFileSystem, @@ -361,7 +360,7 @@ pub struct AffectedPaths { /// Returns an error if the patch could not be applied. async fn apply_hunks_to_files( hunks: &[Hunk], - cwd: &AbsolutePathBuf, + cwd: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, delta: &mut AppliedPatchDelta, @@ -388,26 +387,26 @@ async fn apply_hunks_to_files( }; } + // TODO(anp): Carry PathUri through committed patch deltas and the turn diff tracker. for hunk in hunks { let affected_path = hunk.path().to_path_buf(); - let path_abs = hunk.resolve_path(cwd); - let path_uri = PathUri::from_abs_path(&path_abs); + let path_uri = hunk.resolve_path(cwd)?; match hunk { Hunk::AddFile { contents, .. } => { let overwritten_content = - read_optional_file_text_for_delta(&path_abs, fs, sandbox, &mut delta.exact) + read_optional_file_text_for_delta(&path_uri, fs, sandbox, &mut delta.exact) .await; try_write!( write_file_with_missing_parent_retry( fs, - &path_abs, + &path_uri, contents.clone().into_bytes(), sandbox, ) .await ); delta.changes.push(AppliedPatchChange { - path: path_abs.into_path_buf(), + path: path_uri.to_path_buf(), change: AppliedPatchFileChange::Add { content: contents.clone(), overwritten_content, @@ -416,14 +415,19 @@ async fn apply_hunks_to_files( added.push(affected_path); } Hunk::DeleteFile { .. } => { - note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta.exact).await; + note_existing_path_delta_support(&path_uri, fs, sandbox, &mut delta.exact).await; let deleted_content = fs.read_file_text(&path_uri, sandbox).await.ok(); if deleted_content.is_none() { delta.exact = false; } - ensure_not_directory(&path_abs, fs, sandbox) + ensure_not_directory(&path_uri, fs, sandbox) .await - .with_context(|| format!("Failed to delete file {}", path_abs.display()))?; + .with_context(|| { + format!( + "Failed to delete file {}", + path_uri.inferred_native_path_string() + ) + })?; if let Err(error) = fs .remove( &path_uri, @@ -434,10 +438,15 @@ async fn apply_hunks_to_files( sandbox, ) .await - .with_context(|| format!("Failed to delete file {}", path_abs.display())) + .with_context(|| { + format!( + "Failed to delete file {}", + path_uri.inferred_native_path_string() + ) + }) { delta.exact &= remove_failure_was_side_effect_free( - &path_abs, + &path_uri, deleted_content.as_deref(), fs, sandbox, @@ -447,7 +456,7 @@ async fn apply_hunks_to_files( } if let Some(content) = deleted_content { delta.changes.push(AppliedPatchChange { - path: path_abs.into_path_buf(), + path: path_uri.to_path_buf(), change: AppliedPatchFileChange::Delete { content }, }); } @@ -456,20 +465,20 @@ async fn apply_hunks_to_files( Hunk::UpdateFile { move_path, chunks, .. } => { - note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta.exact).await; + note_existing_path_delta_support(&path_uri, fs, sandbox, &mut delta.exact).await; let AppliedPatch { original_contents, new_contents, - } = derive_new_contents_from_chunks(&path_abs, chunks, fs, sandbox).await?; + } = derive_new_contents_from_chunks(&path_uri, chunks, fs, sandbox).await?; if let Some(dest) = move_path { - let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd); + let dest_uri = cwd.join(&dest.to_string_lossy())?; let overwritten_move_content = - read_optional_file_text_for_delta(&dest_abs, fs, sandbox, &mut delta.exact) + read_optional_file_text_for_delta(&dest_uri, fs, sandbox, &mut delta.exact) .await; try_write!( write_file_with_missing_parent_retry( fs, - &dest_abs, + &dest_uri, new_contents.clone().into_bytes(), sandbox, ) @@ -477,16 +486,19 @@ async fn apply_hunks_to_files( ); let dest_write_change_index = delta.changes.len(); delta.changes.push(AppliedPatchChange { - path: dest_abs.to_path_buf(), + path: dest_uri.to_path_buf(), change: AppliedPatchFileChange::Add { content: new_contents.clone(), overwritten_content: overwritten_move_content.clone(), }, }); - ensure_not_directory(&path_abs, fs, sandbox) + ensure_not_directory(&path_uri, fs, sandbox) .await .with_context(|| { - format!("Failed to remove original {}", path_abs.display()) + format!( + "Failed to remove original {}", + path_uri.inferred_native_path_string() + ) })?; if let Err(error) = fs .remove( @@ -499,11 +511,14 @@ async fn apply_hunks_to_files( ) .await .with_context(|| { - format!("Failed to remove original {}", path_abs.display()) + format!( + "Failed to remove original {}", + path_uri.inferred_native_path_string() + ) }) { delta.exact &= remove_failure_was_side_effect_free( - &path_abs, + &path_uri, Some(&original_contents), fs, sandbox, @@ -512,9 +527,9 @@ async fn apply_hunks_to_files( return Err(error); } delta.changes[dest_write_change_index] = AppliedPatchChange { - path: path_abs.into_path_buf(), + path: path_uri.to_path_buf(), change: AppliedPatchFileChange::Update { - move_path: Some(dest_abs.into_path_buf()), + move_path: Some(dest_uri.to_path_buf()), old_content: original_contents, overwritten_move_content, new_content: new_contents, @@ -527,11 +542,11 @@ async fn apply_hunks_to_files( .await .with_context(|| format!( "Failed to write file {}", - path_abs.display() + path_uri.inferred_native_path_string() )) ); delta.changes.push(AppliedPatchChange { - path: path_abs.into_path_buf(), + path: path_uri.to_path_buf(), change: AppliedPatchFileChange::Update { move_path: None, old_content: original_contents, @@ -552,12 +567,11 @@ async fn apply_hunks_to_files( } async fn ensure_not_directory( - path: &AbsolutePathBuf, + path: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, ) -> io::Result<()> { - let path_uri = PathUri::from_abs_path(path); - let metadata = fs.get_metadata(&path_uri, sandbox).await?; + let metadata = fs.get_metadata(path, sandbox).await?; if metadata.is_directory { return Err(io::Error::new( io::ErrorKind::InvalidInput, @@ -568,15 +582,14 @@ async fn ensure_not_directory( } async fn remove_failure_was_side_effect_free( - path: &AbsolutePathBuf, + path: &PathUri, expected_content: Option<&str>, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, ) -> bool { - let path_uri = PathUri::from_abs_path(path); match expected_content { Some(expected_content) => fs - .read_file_text(&path_uri, sandbox) + .read_file_text(path, sandbox) .await .is_ok_and(|content| content == expected_content), None => false, @@ -584,14 +597,13 @@ async fn remove_failure_was_side_effect_free( } async fn read_optional_file_text_for_delta( - path: &AbsolutePathBuf, + path: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, exact: &mut bool, ) -> Option { note_existing_path_delta_support(path, fs, sandbox, exact).await; - let path_uri = PathUri::from_abs_path(path); - match fs.read_file_text(&path_uri, sandbox).await { + match fs.read_file_text(path, sandbox).await { Ok(content) => Some(content), Err(source) if source.kind() == io::ErrorKind::NotFound => None, Err(_) => { @@ -602,13 +614,12 @@ async fn read_optional_file_text_for_delta( } async fn note_existing_path_delta_support( - path: &AbsolutePathBuf, + path: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, exact: &mut bool, ) { - let path_uri = PathUri::from_abs_path(path); - match fs.get_metadata(&path_uri, sandbox).await { + match fs.get_metadata(path, sandbox).await { Ok(metadata) if metadata.is_file && !metadata.is_symlink => {} Ok(_) => *exact = false, Err(source) if source.kind() == io::ErrorKind::NotFound => {} @@ -618,37 +629,39 @@ async fn note_existing_path_delta_support( async fn write_file_with_missing_parent_retry( fs: &dyn ExecutorFileSystem, - path_abs: &AbsolutePathBuf, + path: &PathUri, contents: Vec, sandbox: Option<&FileSystemSandboxContext>, ) -> anyhow::Result<()> { - let path_uri = PathUri::from_abs_path(path_abs); - match fs.write_file(&path_uri, contents.clone(), sandbox).await { + match fs.write_file(path, contents.clone(), sandbox).await { Ok(()) => Ok(()), Err(err) if err.kind() == io::ErrorKind::NotFound => { - if let Some(parent_abs) = path_abs.parent() { - let parent_uri = PathUri::from_abs_path(&parent_abs); - fs.create_directory( - &parent_uri, - CreateDirectoryOptions { recursive: true }, - sandbox, - ) + if let Some(parent) = path.parent() { + fs.create_directory(&parent, CreateDirectoryOptions { recursive: true }, sandbox) + .await + .with_context(|| { + format!( + "Failed to create parent directories for {}", + path.inferred_native_path_string() + ) + })?; + } + fs.write_file(path, contents, sandbox) .await .with_context(|| { format!( - "Failed to create parent directories for {}", - path_abs.display() + "Failed to write file {}", + path.inferred_native_path_string() ) })?; - } - fs.write_file(&path_uri, contents, sandbox) - .await - .with_context(|| format!("Failed to write file {}", path_abs.display()))?; Ok(()) } - Err(err) => { - Err(err).with_context(|| format!("Failed to write file {}", path_abs.display())) - } + Err(err) => Err(err).with_context(|| { + format!( + "Failed to write file {}", + path.inferred_native_path_string() + ) + }), } } @@ -660,15 +673,17 @@ struct AppliedPatch { /// Return *only* the new file contents (joined into a single `String`) after /// applying the chunks to the file at `path`. async fn derive_new_contents_from_chunks( - path_abs: &AbsolutePathBuf, + path: &PathUri, chunks: &[UpdateFileChunk], fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, ) -> std::result::Result { - let path_uri = PathUri::from_abs_path(path_abs); - let original_contents = fs.read_file_text(&path_uri, sandbox).await.map_err(|err| { + let original_contents = fs.read_file_text(path, sandbox).await.map_err(|err| { ApplyPatchError::IoError(IoError { - context: format!("Failed to read file to update {}", path_abs.display()), + context: format!( + "Failed to read file to update {}", + path.inferred_native_path_string() + ), source: err, }) })?; @@ -681,7 +696,8 @@ async fn derive_new_contents_from_chunks( original_lines.pop(); } - let replacements = compute_replacements(&original_lines, path_abs.as_path(), chunks)?; + let path_text = path.inferred_native_path_string(); + let replacements = compute_replacements(&original_lines, &path_text, chunks)?; let new_lines = apply_replacements(original_lines, &replacements); let mut new_lines = new_lines; if !new_lines.last().is_some_and(String::is_empty) { @@ -699,7 +715,7 @@ async fn derive_new_contents_from_chunks( /// `(start_index, old_len, new_lines)`. fn compute_replacements( original_lines: &[String], - path: &Path, + path: &str, chunks: &[UpdateFileChunk], ) -> std::result::Result)>, ApplyPatchError> { let mut replacements: Vec<(usize, usize, Vec)> = Vec::new(); @@ -718,9 +734,7 @@ fn compute_replacements( line_index = idx + 1; } else { return Err(ApplyPatchError::ComputeReplacements(format!( - "Failed to find context '{}' in {}", - ctx_line, - path.display() + "Failed to find context '{ctx_line}' in {path}" ))); } } @@ -776,7 +790,7 @@ fn compute_replacements( } else { return Err(ApplyPatchError::ComputeReplacements(format!( "Failed to find expected lines in {}:\n{}", - path.display(), + path, chunk.old_lines.join("\n"), ))); } @@ -824,16 +838,16 @@ pub struct ApplyPatchFileUpdate { } pub async fn unified_diff_from_chunks( - path_abs: &AbsolutePathBuf, + path: &PathUri, chunks: &[UpdateFileChunk], fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, ) -> std::result::Result { - unified_diff_from_chunks_with_context(path_abs, chunks, /*context*/ 1, fs, sandbox).await + unified_diff_from_chunks_with_context(path, chunks, /*context*/ 1, fs, sandbox).await } pub async fn unified_diff_from_chunks_with_context( - path_abs: &AbsolutePathBuf, + path: &PathUri, chunks: &[UpdateFileChunk], context: usize, fs: &dyn ExecutorFileSystem, @@ -842,7 +856,7 @@ pub async fn unified_diff_from_chunks_with_context( let AppliedPatch { original_contents, new_contents, - } = derive_new_contents_from_chunks(path_abs, chunks, fs, sandbox).await?; + } = derive_new_contents_from_chunks(path, chunks, fs, sandbox).await?; let text_diff = TextDiff::from_lines(&original_contents, &new_contents); let unified_diff = text_diff.unified_diff().context_radius(context).to_string(); Ok(ApplyPatchFileUpdate { @@ -875,7 +889,6 @@ pub fn print_summary( mod tests { use super::*; use codex_exec_server::LOCAL_FS; - use codex_utils_absolute_path::test_support::PathExt; use pretty_assertions::assert_eq; use std::fs; use std::string::ToString; @@ -900,7 +913,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -924,7 +937,7 @@ mod tests { #[tokio::test] async fn test_apply_patch_hunks_accept_relative_and_absolute_paths() { let dir = tempdir().unwrap(); - let cwd = dir.path().abs(); + let cwd = PathUri::from_path(dir.path()).expect("absolute test path"); let relative_add = dir.path().join("relative-add.txt"); let absolute_add = dir.path().join("absolute-add.txt"); let relative_delete = dir.path().join("relative-delete.txt"); @@ -1003,7 +1016,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1039,7 +1052,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1079,7 +1092,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1123,7 +1136,7 @@ mod tests { let mut stderr = Vec::new(); let failure = apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1183,7 +1196,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1241,7 +1254,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1285,7 +1298,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1328,7 +1341,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1378,9 +1391,9 @@ mod tests { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; - let path_abs = path.as_path().abs(); + let path_uri = PathUri::from_path(&path).expect("absolute test path"); let diff = unified_diff_from_chunks( - &path_abs, + &path_uri, update_file_chunks, LOCAL_FS.as_ref(), /*sandbox*/ None, @@ -1426,11 +1439,15 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let path_abs = path.as_path().abs(); - let diff = - unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) - .await - .unwrap(); + let resolved_path = PathUri::from_path(&path).expect("absolute test path"); + let diff = unified_diff_from_chunks( + &resolved_path, + chunks, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await + .unwrap(); let expected_diff = r#"@@ -1,2 +1,2 @@ -foo +FOO @@ -1468,11 +1485,15 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let path_abs = path.as_path().abs(); - let diff = - unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) - .await - .unwrap(); + let resolved_path = PathUri::from_path(&path).expect("absolute test path"); + let diff = unified_diff_from_chunks( + &resolved_path, + chunks, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await + .unwrap(); let expected_diff = r#"@@ -2,2 +2,2 @@ bar -baz @@ -1508,9 +1529,9 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let path_abs = path.as_path().abs(); + let path_uri = PathUri::from_path(&path).expect("absolute test path"); let diff = - unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) + unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) .await .unwrap(); let expected_diff = r#"@@ -3 +3,2 @@ @@ -1559,9 +1580,9 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let path_abs = path.as_path().abs(); + let path_uri = PathUri::from_path(&path).expect("absolute test path"); let diff = - unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) + unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) .await .unwrap(); @@ -1589,7 +1610,7 @@ mod tests { let mut stderr = Vec::new(); apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1627,7 +1648,7 @@ g let mut stderr = Vec::new(); let result = apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), @@ -1646,7 +1667,7 @@ g let dir = tempdir().unwrap(); let path = dir.path().join("binary.dat"); fs::write(dir.path().join("source.txt"), "before\n").unwrap(); - let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(); + let cwd = PathUri::from_path(dir.path()).expect("absolute test path"); for patch in [ wrap_patch("*** Add File: binary.dat\n+text"), @@ -1684,7 +1705,7 @@ g let mut stderr = Vec::new(); let delta = apply_patch( &patch, - &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &PathUri::from_path(dir.path()).expect("absolute test path"), &mut stdout, &mut stderr, LOCAL_FS.as_ref(), diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index 5854f2616f..04e0053a3a 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -25,9 +25,10 @@ //! leading/trailing whitespace around patch markers. use crate::ApplyPatchArgs; use crate::streaming_parser::StreamingPatchParser; -use codex_utils_absolute_path::AbsolutePathBuf; #[cfg(test)] use codex_utils_absolute_path::test_support::PathBufExt; +use codex_utils_path_uri::PathUri; +use codex_utils_path_uri::PathUriParseError; use std::path::Path; use std::path::PathBuf; @@ -81,12 +82,12 @@ pub enum Hunk { } impl Hunk { - pub fn resolve_path(&self, cwd: &AbsolutePathBuf) -> AbsolutePathBuf { + pub fn resolve_path(&self, cwd: &PathUri) -> Result { let path = match self { Hunk::UpdateFile { path, .. } => path, Hunk::AddFile { .. } | Hunk::DeleteFile { .. } => self.path(), }; - AbsolutePathBuf::resolve_path_against_base(path, cwd) + cwd.join(&path.to_string_lossy()) } /// Returns the path affected by this hunk, using the move destination for rename hunks. @@ -479,7 +480,7 @@ fn test_parse_patch_accepts_relative_and_absolute_hunk_paths() { #[test] fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() { let cwd_dir = tempfile::tempdir().unwrap(); - let cwd = cwd_dir.path().to_path_buf().abs(); + let cwd = PathUri::from_path(cwd_dir.path()).unwrap(); let absolute_dir = tempfile::tempdir().unwrap(); let absolute_add = absolute_dir.path().join("absolute-add.py").abs(); let absolute_delete = absolute_dir.path().join("absolute-delete.py").abs(); @@ -491,13 +492,13 @@ fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() { path: PathBuf::from("relative-add.py"), contents: String::new(), }, - cwd.join("relative-add.py"), + cwd.join("relative-add.py").unwrap(), ), ( DeleteFile { path: PathBuf::from("relative-delete.py"), }, - cwd.join("relative-delete.py"), + cwd.join("relative-delete.py").unwrap(), ), ( UpdateFile { @@ -505,20 +506,20 @@ fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() { move_path: None, chunks: Vec::new(), }, - cwd.join("relative-update.py"), + cwd.join("relative-update.py").unwrap(), ), ( AddFile { path: absolute_add.to_path_buf(), contents: String::new(), }, - absolute_add, + PathUri::from_abs_path(&absolute_add), ), ( DeleteFile { path: absolute_delete.to_path_buf(), }, - absolute_delete, + PathUri::from_abs_path(&absolute_delete), ), ( UpdateFile { @@ -526,10 +527,10 @@ fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() { move_path: None, chunks: Vec::new(), }, - absolute_update, + PathUri::from_abs_path(&absolute_update), ), ] { - assert_eq!(hunk.resolve_path(&cwd), expected_path); + assert_eq!(hunk.resolve_path(&cwd), Ok(expected_path)); } } diff --git a/codex-rs/apply-patch/src/standalone_executable.rs b/codex-rs/apply-patch/src/standalone_executable.rs index 45ca0d0619..384b2ecee2 100644 --- a/codex-rs/apply-patch/src/standalone_executable.rs +++ b/codex-rs/apply-patch/src/standalone_executable.rs @@ -65,6 +65,8 @@ pub fn run_main() -> i32 { return 1; } }; + // TODO(anp): Discover the standalone executable cwd as PathUri directly. + let cwd = codex_utils_path_uri::PathUri::from_abs_path(&cwd); match runtime.block_on(crate::apply_patch( &patch_arg, &cwd, diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index 2102eaf875..1c28f81d8c 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -122,6 +122,7 @@ pub fn arg0_dispatch() -> Option { Ok(runtime) => runtime, Err(_) => std::process::exit(1), }; + let cwd = cwd.into(); match runtime.block_on(codex_apply_patch::apply_patch( &patch_arg, &cwd, diff --git a/codex-rs/core-plugins/src/discoverable.rs b/codex-rs/core-plugins/src/discoverable.rs index 0379819c47..eb1df6ff52 100644 --- a/codex-rs/core-plugins/src/discoverable.rs +++ b/codex-rs/core-plugins/src/discoverable.rs @@ -1,8 +1,8 @@ use anyhow::Context; use codex_app_server_protocol::PluginAvailability; use codex_app_server_protocol::PluginInstallPolicy; +use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_login::CodexAuth; -use codex_plugin::PluginCapabilitySummary; use codex_plugin::PluginId; use std::collections::HashSet; use tracing::warn; @@ -93,6 +93,7 @@ impl PluginsManager { } else { None }; + let skill_config_rules = skill_config_rules_from_stack(&input.plugins.config_layer_stack); let mut discoverable_plugins = Vec::::new(); for marketplace in marketplaces { @@ -110,17 +111,15 @@ impl PluginsManager { } let plugin_id = plugin.id.clone(); - match self - .read_plugin_detail_for_marketplace_plugin( - &input.plugins, + .tool_suggest_metadata_for_marketplace_plugin( &marketplace_name, - plugin, + &plugin, + &skill_config_rules, ) .await { Ok(plugin) => { - let plugin: PluginCapabilitySummary = plugin.into(); discoverable_plugins.push(ToolSuggestDiscoverablePlugin { id: plugin.config_name, remote_plugin_id: None, diff --git a/codex-rs/core-plugins/src/discoverable_tests.rs b/codex-rs/core-plugins/src/discoverable_tests.rs index 8465d2431e..d472c76344 100644 --- a/codex-rs/core-plugins/src/discoverable_tests.rs +++ b/codex-rs/core-plugins/src/discoverable_tests.rs @@ -208,7 +208,7 @@ async fn includes_openai_curated_when_remote_enabled_without_auth() { } #[tokio::test] -async fn deduplicates_configured_marketplace_plugin() { +async fn deduplicates_and_reprojects_cached_configured_marketplace_plugin() { let codex_home = tempdir().expect("tempdir should succeed"); let plugin_name = "sample"; let marketplace_name = OPENAI_BUNDLED_MARKETPLACE_NAME; @@ -229,6 +229,12 @@ async fn deduplicates_configured_marketplace_plugin() { ), ); write_curated_plugin(&marketplace_root, plugin_name); + write_plugin_app( + &marketplace_root, + plugin_name, + "sample-docs", + "connector_sample", + ); write_file( &codex_home.path().join(CONFIG_TOML_FILE), &format!( @@ -241,18 +247,179 @@ source = "/tmp/{marketplace_name}" "# ), ); - let plugins = load_plugins_config(codex_home.path(), codex_home.path()).await; let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); - let discoverable_plugins = list_discoverable_plugins( + assert!(plugins_manager.set_auth_mode(Some(AuthMode::Chatgpt))); + let chatgpt_projection = list_discoverable_plugins( + &plugins_manager, + discovery_input(plugins.clone(), &[plugin_id.as_str()], &[], &[]), + /*auth*/ None, + ) + .await; + let expected = ToolSuggestDiscoverablePlugin { + id: plugin_id.clone(), + remote_plugin_id: None, + name: "sample".to_string(), + description: Some( + "Plugin that includes skills, MCP servers, and app connectors".to_string(), + ), + has_skills: true, + mcp_server_names: Vec::new(), + app_connector_ids: vec!["connector_sample".to_string()], + }; + assert_eq!(chatgpt_projection, vec![expected.clone()]); + + assert!(plugins_manager.set_auth_mode(Some(AuthMode::ApiKey))); + let api_key_projection = list_discoverable_plugins( &plugins_manager, discovery_input(plugins, &[plugin_id.as_str()], &[], &[]), /*auth*/ None, ) .await; + assert_eq!( + api_key_projection, + vec![ToolSuggestDiscoverablePlugin { + mcp_server_names: vec!["sample-docs".to_string()], + app_connector_ids: Vec::new(), + ..expected + }] + ); +} - assert_eq!(discoverable_plugins.len(), 1); - assert_eq!(discoverable_plugins[0].id, plugin_id); +#[tokio::test] +async fn reprojects_cached_skill_availability_for_current_config() { + let codex_home = tempdir().expect("tempdir should succeed"); + let curated_root = curated_plugins_repo_path(codex_home.path()); + write_openai_curated_marketplace(&curated_root, &["slack"]); + + let plugins = load_plugins_config(codex_home.path(), codex_home.path()).await; + let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); + let expected = ToolSuggestDiscoverablePlugin { + id: "slack@openai-curated".to_string(), + remote_plugin_id: None, + name: "slack".to_string(), + description: Some( + "Plugin that includes skills, MCP servers, and app connectors".to_string(), + ), + has_skills: true, + mcp_server_names: vec!["sample-docs".to_string()], + app_connector_ids: vec!["connector_calendar".to_string()], + }; + let initial = list_discoverable_plugins( + &plugins_manager, + discovery_input(plugins, &[], &[], &[]), + /*auth*/ None, + ) + .await; + assert_eq!(initial, vec![expected.clone()]); + + write_file( + &codex_home.path().join(CONFIG_TOML_FILE), + r#"[[skills.config]] +name = "slack:sample" +enabled = false +"#, + ); + let plugins = load_plugins_config(codex_home.path(), codex_home.path()).await; + let after_skill_disabled = list_discoverable_plugins( + &plugins_manager, + discovery_input(plugins, &[], &[], &[]), + /*auth*/ None, + ) + .await; + assert_eq!( + after_skill_disabled, + vec![ToolSuggestDiscoverablePlugin { + has_skills: false, + ..expected + }] + ); +} + +#[tokio::test] +async fn does_not_advertise_skills_when_skill_loading_fails() { + let codex_home = tempdir().expect("tempdir should succeed"); + let curated_root = curated_plugins_repo_path(codex_home.path()); + write_openai_curated_marketplace(&curated_root, &["slack"]); + write_file( + &curated_root.join("plugins/slack/skills/SKILL.md"), + "---\nname: bad", + ); + + let plugins = load_plugins_config(codex_home.path(), codex_home.path()).await; + let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); + let discoverable_plugins = list_discoverable_plugins( + &plugins_manager, + discovery_input(plugins, &[], &[], &[]), + /*auth*/ None, + ) + .await; + + assert_eq!( + discoverable_plugins, + vec![ToolSuggestDiscoverablePlugin { + id: "slack@openai-curated".to_string(), + remote_plugin_id: None, + name: "slack".to_string(), + description: Some( + "Plugin that includes skills, MCP servers, and app connectors".to_string(), + ), + has_skills: false, + mcp_server_names: vec!["sample-docs".to_string()], + app_connector_ids: vec!["connector_calendar".to_string()], + }] + ); +} + +#[tokio::test] +async fn clear_cache_invalidates_cached_tool_suggest_metadata() { + let codex_home = tempdir().expect("tempdir should succeed"); + let curated_root = curated_plugins_repo_path(codex_home.path()); + write_openai_curated_marketplace(&curated_root, &["slack"]); + let plugin_manifest = curated_root.join("plugins/slack/.codex-plugin/plugin.json"); + write_file( + &plugin_manifest, + r#"{ + "name": "slack", + "description": "Before reload" +}"#, + ); + + let plugins = load_plugins_config(codex_home.path(), codex_home.path()).await; + let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); + let input = discovery_input(plugins, &[], &[], &[]); + let expected_cached = vec![ToolSuggestDiscoverablePlugin { + id: "slack@openai-curated".to_string(), + remote_plugin_id: None, + name: "slack".to_string(), + description: Some("Before reload".to_string()), + has_skills: true, + mcp_server_names: vec!["sample-docs".to_string()], + app_connector_ids: vec!["connector_calendar".to_string()], + }]; + let initial = list_discoverable_plugins(&plugins_manager, input.clone(), /*auth*/ None).await; + assert_eq!(initial, expected_cached); + + write_file( + &plugin_manifest, + r#"{ + "name": "slack", + "description": "After reload" +}"#, + ); + let before_reload = + list_discoverable_plugins(&plugins_manager, input.clone(), /*auth*/ None).await; + assert_eq!(before_reload, expected_cached); + + plugins_manager.clear_cache(); + let after_reload = list_discoverable_plugins(&plugins_manager, input, /*auth*/ None).await; + assert_eq!( + after_reload, + vec![ToolSuggestDiscoverablePlugin { + description: Some("After reload".to_string()), + ..expected_cached[0].clone() + }] + ); } #[tokio::test] diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 356c0f8bdc..08e77606fc 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -18,6 +18,7 @@ pub mod store; #[cfg(test)] mod test_support; pub mod toggles; +mod tool_suggest_metadata; pub const OPENAI_CURATED_MARKETPLACE_NAME: &str = "openai-curated"; pub const OPENAI_API_CURATED_MARKETPLACE_NAME: &str = "openai-api-curated"; diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 029be960b6..43b561b3be 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -768,6 +768,29 @@ fn apply_plugin_mcp_server_policy(config: &mut McpServerConfig, policy: &PluginM } } +pub(crate) struct PluginSkillInventory { + skills: Vec, + had_errors: bool, +} + +impl PluginSkillInventory { + pub(crate) fn has_enabled_skills(&self, skill_config_rules: &SkillConfigRules) -> bool { + contains_enabled_skill( + &self.skills, + &resolve_disabled_skill_paths(&self.skills, skill_config_rules), + ) + } + + fn resolve(self, skill_config_rules: &SkillConfigRules) -> ResolvedPluginSkills { + let disabled_skill_paths = resolve_disabled_skill_paths(&self.skills, skill_config_rules); + ResolvedPluginSkills { + skills: self.skills, + disabled_skill_paths, + had_errors: self.had_errors, + } + } +} + #[derive(Debug, Clone)] pub struct ResolvedPluginSkills { pub skills: Vec, @@ -777,14 +800,19 @@ pub struct ResolvedPluginSkills { impl ResolvedPluginSkills { pub fn has_enabled_skills(&self) -> bool { - self.had_errors - || self - .skills - .iter() - .any(|skill| !self.disabled_skill_paths.contains(&skill.path_to_skills_md)) + self.had_errors || contains_enabled_skill(&self.skills, &self.disabled_skill_paths) } } +fn contains_enabled_skill( + skills: &[SkillMetadata], + disabled_skill_paths: &HashSet, +) -> bool { + skills + .iter() + .any(|skill| !disabled_skill_paths.contains(&skill.path_to_skills_md)) +} + pub async fn load_plugin_skills( plugin_root: &AbsolutePathBuf, plugin_id: &PluginId, @@ -792,6 +820,17 @@ pub async fn load_plugin_skills( restriction_product: Option, skill_config_rules: &SkillConfigRules, ) -> ResolvedPluginSkills { + load_plugin_skill_inventory(plugin_root, plugin_id, manifest, restriction_product) + .await + .resolve(skill_config_rules) +} + +pub(crate) async fn load_plugin_skill_inventory( + plugin_root: &AbsolutePathBuf, + plugin_id: &PluginId, + manifest: &PluginManifest, + restriction_product: Option, +) -> PluginSkillInventory { let roots = plugin_skill_roots(plugin_root, &manifest.paths) .into_iter() .map(|path| SkillRoot { @@ -810,13 +849,8 @@ pub async fn load_plugin_skills( .into_iter() .filter(|skill| skill.matches_product_restriction_for_product(restriction_product)) .collect::>(); - let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules); - ResolvedPluginSkills { - skills, - disabled_skill_paths, - had_errors, - } + PluginSkillInventory { skills, had_errors } } fn plugin_skill_roots( diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index b1bb508eeb..c31040ebf1 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -51,6 +51,7 @@ use crate::startup_sync::sync_openai_plugins_repo; use crate::store::PluginInstallResult as StorePluginInstallResult; use crate::store::PluginStore; use crate::store::PluginStoreError; +use crate::tool_suggest_metadata::ToolSuggestMetadataCache; use codex_analytics::AnalyticsEventsClient; use codex_app_server_protocol::AuthMode; use codex_config::ConfigLayerStack; @@ -354,6 +355,7 @@ pub struct PluginsManager { // Keep the cache auth-independent so auth changes only need to resolve capabilities again. loaded_plugins_cache: RwLock, loaded_plugins_load_semaphore: Semaphore, + tool_suggest_metadata_cache: ToolSuggestMetadataCache, remote_installed_plugins_cache: RwLock>>, remote_installed_plugins_cache_refresh_state: RwLock, global_remote_catalog_cache_refresh_state: RwLock, @@ -410,6 +412,7 @@ impl PluginsManager { non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()), loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()), loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1), + tool_suggest_metadata_cache: ToolSuggestMetadataCache::new(), remote_installed_plugins_cache: RwLock::new(None), remote_installed_plugins_cache_refresh_state: RwLock::new( RemoteInstalledPluginsCacheRefreshState::default(), @@ -551,6 +554,7 @@ impl PluginsManager { } fn clear_loaded_plugins_cache(&self) { + self.tool_suggest_metadata_cache.clear(); let mut cache = match self.loaded_plugins_cache.write() { Ok(cache) => cache, Err(err) => err.into_inner(), @@ -559,6 +563,17 @@ impl PluginsManager { cache.entry = None; } + fn clear_caches_after_marketplace_source_refresh( + &self, + installed_plugin_cache_refreshed: bool, + ) { + if installed_plugin_cache_refreshed { + self.clear_cache(); + } else { + self.tool_suggest_metadata_cache.clear(); + } + } + /// Load plugins for a config layer stack without touching the plugins cache. pub async fn plugins_for_layer_stack( &self, @@ -1444,6 +1459,19 @@ impl PluginsManager { )) } + pub(crate) async fn tool_suggest_metadata_for_marketplace_plugin( + &self, + marketplace_name: &str, + plugin: &ConfiguredMarketplacePlugin, + skill_config_rules: &SkillConfigRules, + ) -> Result { + let fragment = self + .tool_suggest_metadata_cache + .metadata_for_plugin(marketplace_name, plugin, self.restriction_product) + .await?; + Ok(fragment.project(skill_config_rules, self.auth_mode())) + } + pub async fn read_plugin_for_config( &self, config: &PluginsConfigInput, @@ -1795,9 +1823,7 @@ impl PluginsManager { &outcome.upgraded_roots, ) { Ok(cache_refreshed) => { - if cache_refreshed { - self.clear_cache(); - } + self.clear_caches_after_marketplace_source_refresh(cache_refreshed); } Err(err) => { self.clear_cache(); @@ -1977,9 +2003,8 @@ impl PluginsManager { &configured_curated_plugin_ids, ) { Ok(cache_refreshed) => { - if cache_refreshed { - manager.clear_cache(); - } + manager + .clear_caches_after_marketplace_source_refresh(cache_refreshed); } Err(err) => { manager.clear_cache(); @@ -2220,7 +2245,9 @@ impl PluginsManager { } } -fn remote_plugin_install_required_description(source: &MarketplacePluginSource) -> String { +pub(crate) fn remote_plugin_install_required_description( + source: &MarketplacePluginSource, +) -> String { let source_description = match source { MarketplacePluginSource::Git { url, diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index f5073f68b0..1aa11c6120 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -3,6 +3,8 @@ use crate::LoadedPlugin; use crate::OPENAI_API_CURATED_MARKETPLACE_NAME; use crate::OPENAI_CURATED_MARKETPLACE_NAME; use crate::PluginLoadOutcome; +use crate::ToolSuggestDiscoverablePlugin; +use crate::ToolSuggestPluginDiscoveryInput; use crate::installed_marketplaces::marketplace_install_root; use crate::loader::load_plugin_skills; use crate::loader::load_plugins_from_layer_stack; @@ -18,6 +20,7 @@ use crate::startup_sync::curated_plugins_repo_path; use crate::test_support::TEST_CURATED_PLUGIN_CACHE_VERSION; use crate::test_support::TEST_CURATED_PLUGIN_SHA; use crate::test_support::load_plugins_config as load_plugins_config_input; +use crate::test_support::write_curated_plugin; use crate::test_support::write_curated_plugin_sha_with as write_curated_plugin_sha; use crate::test_support::write_file; use crate::test_support::write_openai_api_curated_marketplace; @@ -3497,6 +3500,103 @@ source = "/tmp/debug" ); } +#[tokio::test] +async fn configured_marketplace_upgrade_invalidates_cached_tool_suggest_metadata() { + let tmp = tempfile::tempdir().unwrap(); + let remote_repo = tmp.path().join("remote-marketplace"); + let remote_repo_url = url::Url::from_directory_path(&remote_repo) + .unwrap() + .to_string(); + write_file( + &remote_repo.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "sample", + "source": { + "source": "local", + "path": "./plugins/sample" + } + } + ] +}"#, + ); + write_curated_plugin(&remote_repo, "sample"); + write_file( + &remote_repo.join("plugins/sample/.codex-plugin/plugin.json"), + r#"{"name":"sample","description":"Before upgrade"}"#, + ); + init_git_repo(&remote_repo); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + &format!( + r#"[features] +plugins = true + +[marketplaces.debug] +source_type = "git" +source = "{remote_repo_url}" +"# + ), + ); + + let manager = PluginsManager::new(tmp.path().to_path_buf()); + let config = load_config(tmp.path(), tmp.path()).await; + let initial_upgrade = manager + .upgrade_configured_marketplaces_for_config(&config, /*marketplace_name*/ None) + .expect("initial marketplace install should succeed"); + assert_eq!(initial_upgrade.errors, Vec::new()); + assert_eq!(initial_upgrade.upgraded_roots.len(), 1); + + let config = load_config(tmp.path(), tmp.path()).await; + let input = ToolSuggestPluginDiscoveryInput { + plugins: config.clone(), + configured_plugin_ids: HashSet::from(["sample@debug".to_string()]), + disabled_plugin_ids: HashSet::new(), + loaded_plugin_app_connector_ids: HashSet::new(), + }; + let expected = ToolSuggestDiscoverablePlugin { + id: "sample@debug".to_string(), + remote_plugin_id: None, + name: "sample".to_string(), + description: Some("Before upgrade".to_string()), + has_skills: true, + mcp_server_names: vec!["sample-docs".to_string()], + app_connector_ids: vec!["connector_calendar".to_string()], + }; + assert_eq!( + manager + .list_tool_suggest_discoverable_plugins(&input, /*auth*/ None) + .await + .expect("initial tool-suggest metadata should load"), + vec![expected.clone()] + ); + + write_file( + &remote_repo.join("plugins/sample/.codex-plugin/plugin.json"), + r#"{"name":"sample","description":"After upgrade"}"#, + ); + run_git(&remote_repo, &["add", "."]); + run_git(&remote_repo, &["commit", "-m", "update plugin"]); + let upgrade = manager + .upgrade_configured_marketplaces_for_config(&config, /*marketplace_name*/ None) + .expect("marketplace upgrade should succeed"); + assert_eq!(upgrade.errors, Vec::new()); + assert_eq!(upgrade.upgraded_roots.len(), 1); + + assert_eq!( + manager + .list_tool_suggest_discoverable_plugins(&input, /*auth*/ None) + .await + .expect("refreshed tool-suggest metadata should load"), + vec![ToolSuggestDiscoverablePlugin { + description: Some("After upgrade".to_string()), + ..expected + }] + ); +} + #[tokio::test] async fn list_marketplaces_uses_config_when_known_registry_is_malformed() { let tmp = tempfile::tempdir().unwrap(); diff --git a/codex-rs/core-plugins/src/marketplace.rs b/codex-rs/core-plugins/src/marketplace.rs index ce4fcd2ffc..82cbcf1415 100644 --- a/codex-rs/core-plugins/src/marketplace.rs +++ b/codex-rs/core-plugins/src/marketplace.rs @@ -66,7 +66,7 @@ pub struct MarketplacePlugin { pub keywords: Vec, } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum MarketplacePluginSource { Local { path: AbsolutePathBuf, diff --git a/codex-rs/core-plugins/src/tool_suggest_metadata.rs b/codex-rs/core-plugins/src/tool_suggest_metadata.rs new file mode 100644 index 0000000000..2a667405e8 --- /dev/null +++ b/codex-rs/core-plugins/src/tool_suggest_metadata.rs @@ -0,0 +1,232 @@ +use std::collections::HashMap; +use std::sync::Arc; +use std::sync::RwLock; + +use codex_app_server_protocol::AuthMode; +use codex_core_skills::config_rules::SkillConfigRules; +use codex_plugin::AppDeclaration; +use codex_plugin::PluginCapabilitySummary; +use codex_plugin::PluginId; +use codex_plugin::PluginIdError; +use codex_plugin::app_connector_ids_from_declarations; +use codex_plugin::prompt_safe_plugin_description; +use codex_protocol::protocol::Product; +use tokio::sync::Semaphore; + +use crate::app_mcp_routing::apply_app_mcp_routing_policy; +use crate::loader::PluginSkillInventory; +use crate::loader::load_plugin_apps; +use crate::loader::load_plugin_mcp_servers; +use crate::loader::load_plugin_skill_inventory; +use crate::manager::ConfiguredMarketplacePlugin; +use crate::manager::remote_plugin_install_required_description; +use crate::manifest::load_plugin_manifest; +use crate::marketplace::MarketplaceError; +use crate::marketplace::MarketplacePluginSource; + +const MAX_TOOL_SUGGEST_METADATA_CACHE_ENTRIES: usize = 1024; + +type ToolSuggestMetadataEntry = Result, String>; + +/// Source-derived plugin metadata cached for tool suggestions. +/// +/// `PluginsManager` clears these entries alongside its loaded-plugin cache. Current skill config +/// and auth routing are projected after each lookup and are not part of this cache. +pub(crate) struct ToolSuggestMetadataCache { + state: RwLock, + load_semaphore: Semaphore, +} + +#[derive(Default)] +struct ToolSuggestMetadataCacheState { + generation: u64, + entries: HashMap, +} + +#[derive(Clone, PartialEq, Eq, Hash)] +struct PluginArtifactIdentity { + plugin_id: String, + source: MarketplacePluginSource, +} + +pub(crate) struct ToolSuggestMetadataFragment { + config_name: String, + display_name: String, + description: Option, + mcp_server_names: Vec, + app_declarations: Vec, + skill_inventory: Option, +} + +impl ToolSuggestMetadataFragment { + pub(crate) fn project( + &self, + skill_config_rules: &SkillConfigRules, + auth_mode: Option, + ) -> PluginCapabilitySummary { + let mut app_declarations = self.app_declarations.clone(); + let mut mcp_servers = self + .mcp_server_names + .iter() + .cloned() + .map(|name| (name, ())) + .collect::>(); + if auth_mode.is_some() { + apply_app_mcp_routing_policy( + &mut app_declarations, + &mut mcp_servers, + auth_mode, + /*plugin_active*/ true, + ); + } + let mut mcp_server_names = mcp_servers.into_keys().collect::>(); + mcp_server_names.sort_unstable(); + + PluginCapabilitySummary { + config_name: self.config_name.clone(), + display_name: self.display_name.clone(), + description: self.description.clone(), + has_skills: self + .skill_inventory + .as_ref() + .is_some_and(|inventory| inventory.has_enabled_skills(skill_config_rules)), + mcp_server_names, + app_connector_ids: app_connector_ids_from_declarations(&app_declarations), + } + } +} + +impl ToolSuggestMetadataCache { + pub(crate) fn new() -> Self { + Self { + state: RwLock::new(ToolSuggestMetadataCacheState::default()), + load_semaphore: Semaphore::new(/*permits*/ 1), + } + } + + pub(crate) fn clear(&self) { + let mut state = match self.state.write() { + Ok(state) => state, + Err(err) => err.into_inner(), + }; + state.generation = state.generation.wrapping_add(1); + state.entries.clear(); + } + + pub(crate) async fn metadata_for_plugin( + &self, + marketplace_name: &str, + plugin: &ConfiguredMarketplacePlugin, + restriction_product: Option, + ) -> Result, MarketplaceError> { + let artifact = PluginArtifactIdentity { + plugin_id: plugin.id.clone(), + source: plugin.source.clone(), + }; + loop { + if let Some(entry) = self.cached_entry(&artifact) { + return entry.map_err(MarketplaceError::InvalidPlugin); + } + + let _load_permit = self.load_semaphore.acquire().await.map_err(|_| { + MarketplaceError::InvalidPlugin( + "tool-suggest metadata cache loader closed".to_string(), + ) + })?; + if let Some(entry) = self.cached_entry(&artifact) { + return entry.map_err(MarketplaceError::InvalidPlugin); + } + + let generation = self.generation(); + let entry = load_plugin_metadata(marketplace_name, plugin, restriction_product).await; + if self.cache_entry_if_current(generation, artifact.clone(), entry.clone()) { + return entry.map_err(MarketplaceError::InvalidPlugin); + } + } + } + + fn cached_entry(&self, artifact: &PluginArtifactIdentity) -> Option { + match self.state.read() { + Ok(state) => state.entries.get(artifact).cloned(), + Err(err) => err.into_inner().entries.get(artifact).cloned(), + } + } + + fn generation(&self) -> u64 { + match self.state.read() { + Ok(state) => state.generation, + Err(err) => err.into_inner().generation, + } + } + + fn cache_entry_if_current( + &self, + generation: u64, + artifact: PluginArtifactIdentity, + entry: ToolSuggestMetadataEntry, + ) -> bool { + let mut state = match self.state.write() { + Ok(state) => state, + Err(err) => err.into_inner(), + }; + if state.generation != generation { + return false; + } + if state.entries.len() >= MAX_TOOL_SUGGEST_METADATA_CACHE_ENTRIES + && !state.entries.contains_key(&artifact) + { + state.entries.clear(); + } + state.entries.insert(artifact, entry); + true + } +} + +async fn load_plugin_metadata( + marketplace_name: &str, + plugin: &ConfiguredMarketplacePlugin, + restriction_product: Option, +) -> ToolSuggestMetadataEntry { + let plugin_id = PluginId::new(plugin.name.clone(), marketplace_name.to_string()).map_err( + |err| match err { + PluginIdError::Invalid(message) => message, + }, + )?; + + let MarketplacePluginSource::Local { path: plugin_root } = &plugin.source else { + return Ok(Arc::new(ToolSuggestMetadataFragment { + config_name: plugin.id.clone(), + display_name: plugin.name.clone(), + description: prompt_safe_plugin_description(Some( + &remote_plugin_install_required_description(&plugin.source), + )), + mcp_server_names: Vec::new(), + app_declarations: Vec::new(), + skill_inventory: None, + })); + }; + if !plugin_root.as_path().is_dir() { + return Err("path does not exist or is not a directory".to_string()); + } + let manifest = load_plugin_manifest(plugin_root.as_path()) + .ok_or_else(|| "missing or invalid plugin.json".to_string())?; + let skill_inventory = + load_plugin_skill_inventory(plugin_root, &plugin_id, &manifest, restriction_product).await; + let mut mcp_server_names = + load_plugin_mcp_servers(plugin_root.as_path(), /*auth_mode*/ None) + .await + .into_keys() + .collect::>(); + mcp_server_names.sort_unstable(); + mcp_server_names.dedup(); + let app_declarations = load_plugin_apps(plugin_root.as_path()).await; + + Ok(Arc::new(ToolSuggestMetadataFragment { + config_name: plugin.id.clone(), + display_name: plugin.name.clone(), + description: prompt_safe_plugin_description(manifest.description.as_deref()), + mcp_server_names, + app_declarations, + skill_inventory: Some(skill_inventory), + })) +} diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 208db44d9c..5e9a3f6e5a 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -7,6 +7,7 @@ use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::FileSystemSandboxPolicy; +use codex_utils_path_uri::PathUri; use std::collections::HashMap; use std::path::PathBuf; @@ -35,24 +36,12 @@ pub(crate) async fn apply_patch( file_system_sandbox_policy: &FileSystemSandboxPolicy, action: ApplyPatchAction, ) -> InternalApplyPatchInvocation { - // TODO(anp): Migrate patch safety checks to PathUri. - let cwd = match action.cwd.to_abs_path() { - Ok(cwd) => cwd, - Err(err) => { - return InternalApplyPatchInvocation::Output(Err(FunctionCallError::RespondToModel( - format!( - "patch cwd `{}` is not native to the Codex host: {err}", - action.cwd - ), - ))); - } - }; match assess_patch_safety( &action, turn_context.approval_policy.value(), &turn_context.permission_profile(), file_system_sandbox_policy, - &cwd, + &action.cwd, turn_context.windows_sandbox_level, ) { SafetyCheck::AutoApprove { @@ -103,9 +92,11 @@ pub(crate) fn convert_apply_patch_to_protocol( new_content: _new_content, } => FileChange::Update { unified_diff: unified_diff.clone(), - move_path: move_path.clone(), + move_path: move_path.as_ref().map(PathUri::to_path_buf), }, }; + // TODO(anp): Carry PathUri through patch protocol events once app-server and rollout + // compatibility no longer require path-flavored strings. result.insert(path.to_path_buf(), protocol_change); } result diff --git a/codex-rs/core/src/apply_patch_tests.rs b/codex-rs/core/src/apply_patch_tests.rs index c0190c3708..dfc48f8a63 100644 --- a/codex-rs/core/src/apply_patch_tests.rs +++ b/codex-rs/core/src/apply_patch_tests.rs @@ -1,5 +1,5 @@ use super::*; -use core_test_support::PathBufExt; +use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -7,14 +7,14 @@ use tempfile::tempdir; #[test] fn convert_apply_patch_maps_add_variant() { let tmp = tempdir().expect("tmp"); - let p = tmp.path().join("a.txt").abs(); - // Create an action with a single Add change - let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string()); + let path = tmp.path().join("a.txt"); + let path_uri = PathUri::from_path(&path).expect("absolute test path"); + let action = ApplyPatchAction::new_add_for_test(&path_uri, "hello".to_string()); let got = convert_apply_patch_to_protocol(&action); assert_eq!( - got.get(p.as_path()), + got.get(path.as_path()), Some(&FileChange::Add { content: "hello".to_string() }) diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index dbae1c0c68..916379705b 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -2,7 +2,6 @@ use std::path::Component; use std::path::Path; use std::path::PathBuf; -use crate::util::resolve_path; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use codex_protocol::config_types::WindowsSandboxLevel; @@ -11,7 +10,7 @@ use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_sandboxing::SandboxType; use codex_sandboxing::get_platform_sandbox; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; const PATCH_REJECTED_OUTSIDE_PROJECT_REASON: &str = "writing outside of the project; rejected by user approval settings"; @@ -35,7 +34,7 @@ pub fn assess_patch_safety( policy: AskForApproval, permission_profile: &PermissionProfile, file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, + cwd: &PathUri, windows_sandbox_level: WindowsSandboxLevel, ) -> SafetyCheck { if action.is_empty() { @@ -118,14 +117,17 @@ pub fn assess_patch_safety( fn patch_rejection_reason( permission_profile: &PermissionProfile, file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, + cwd: &PathUri, ) -> &'static str { + let has_no_writable_roots = cwd.to_abs_path().is_ok_and(|cwd| { + file_system_sandbox_policy + .get_writable_roots_with_cwd(cwd.as_path()) + .is_empty() + }); match permission_profile { PermissionProfile::Managed { .. } if !file_system_sandbox_policy.has_full_disk_write_access() - && file_system_sandbox_policy - .get_writable_roots_with_cwd(cwd.as_path()) - .is_empty() => + && has_no_writable_roots => { PATCH_REJECTED_READ_ONLY_REASON } @@ -138,8 +140,17 @@ fn patch_rejection_reason( fn is_write_patch_constrained_to_writable_paths( action: &ApplyPatchAction, file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, + cwd: &PathUri, ) -> bool { + // A full-disk policy permits every patch target, so no per-path writable-root check can + // further constrain the result. + if file_system_sandbox_policy.has_full_disk_write_access() { + return true; + } + // TODO(anp): Make filesystem sandbox policies operate on PathUri. + let Ok(native_cwd) = cwd.to_abs_path() else { + return false; + }; // Normalize a path by removing `.` and resolving `..` without touching the // filesystem (works even if the file does not exist). fn normalize(path: &Path) -> Option { @@ -159,14 +170,18 @@ fn is_write_patch_constrained_to_writable_paths( // Determine whether `path` is inside **any** writable root. Both `path` // and roots are converted to absolute, normalized forms before the // prefix check. - let is_path_writable = |p: &Path| { - let abs = resolve_path(cwd, &p.to_path_buf()); + let is_path_writable = |path: &PathUri| { + // TODO(anp): Make sandbox policy path checks accept PathUri without host projection. + let Ok(path) = path.to_abs_path() else { + return false; + }; + let abs = path.into_path_buf(); let abs = match normalize(&abs) { Some(v) => v, None => return false, }; - file_system_sandbox_policy.can_write_path_with_cwd(&abs, cwd) + file_system_sandbox_policy.can_write_path_with_cwd(&abs, &native_cwd) }; for (path, change) in action.changes() { diff --git a/codex-rs/core/src/safety_tests.rs b/codex-rs/core/src/safety_tests.rs index ca15d0fe42..6f7efe893b 100644 --- a/codex-rs/core/src/safety_tests.rs +++ b/codex-rs/core/src/safety_tests.rs @@ -7,6 +7,7 @@ use codex_protocol::protocol::FileSystemSandboxEntry; use codex_protocol::protocol::FileSystemSpecialPath; use codex_protocol::protocol::GranularApprovalConfig; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use core_test_support::PathExt; use pretty_assertions::assert_eq; use tempfile::TempDir; @@ -17,11 +18,13 @@ fn test_writable_roots_constraint() { // the real current working directory. let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let parent = cwd.parent().unwrap(); // Helper to build a single‑entry patch that adds a file at `p`. - let make_add_change = - |p: AbsolutePathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string()); + let make_add_change = |p: AbsolutePathBuf| { + ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&p), "".to_string()) + }; let add_inside = make_add_change(cwd.join("inner.txt")); let add_outside = make_add_change(parent.join("outside.txt")); @@ -37,13 +40,13 @@ fn test_writable_roots_constraint() { assert!(is_write_patch_constrained_to_writable_paths( &add_inside, &workspace_only_file_system_policy, - &cwd, + &cwd_uri, )); assert!(!is_write_patch_constrained_to_writable_paths( &add_outside, &workspace_only_file_system_policy, - &cwd, + &cwd_uri, )); // With the parent dir explicitly added as a writable root, the @@ -56,7 +59,7 @@ fn test_writable_roots_constraint() { assert!(is_write_patch_constrained_to_writable_paths( &add_outside, &file_system_policy_with_parent, - &cwd, + &cwd_uri, )); } @@ -64,8 +67,12 @@ fn test_writable_roots_constraint() { fn external_sandbox_auto_approves_in_on_request() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let add_inside_path = cwd.join("inner.txt"); - let add_inside = ApplyPatchAction::new_add_for_test(&add_inside_path, "".to_string()); + let add_inside = ApplyPatchAction::new_add_for_test( + &PathUri::from_abs_path(&add_inside_path), + "".to_string(), + ); let permission_profile = PermissionProfile::External { network: NetworkSandboxPolicy::Enabled, @@ -78,7 +85,7 @@ fn external_sandbox_auto_approves_in_on_request() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled ), SafetyCheck::AutoApprove { @@ -92,9 +99,11 @@ fn external_sandbox_auto_approves_in_on_request() { fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let parent = cwd.parent().unwrap(); let outside_path = parent.join("outside.txt"); - let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string()); + let add_outside = + ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&outside_path), "".to_string()); let permission_profile = PermissionProfile::workspace_write_with( &[], NetworkSandboxPolicy::Restricted, @@ -109,7 +118,7 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled, ), SafetyCheck::AskUser, @@ -126,7 +135,7 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { }), &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled, ), SafetyCheck::AskUser, @@ -137,9 +146,11 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { fn granular_sandbox_approval_false_rejects_out_of_root_patch() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let parent = cwd.parent().unwrap(); let outside_path = parent.join("outside.txt"); - let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string()); + let add_outside = + ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&outside_path), "".to_string()); let permission_profile = PermissionProfile::workspace_write_with( &[], NetworkSandboxPolicy::Restricted, @@ -160,7 +171,7 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() { }), &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled, ), SafetyCheck::Reject { @@ -173,15 +184,17 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() { fn read_only_policy_rejects_patch_with_read_only_reason() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let inside_path = cwd.join("inside.txt"); - let action = ApplyPatchAction::new_add_for_test(&inside_path, "".to_string()); + let action = + ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&inside_path), "".to_string()); let permission_profile = PermissionProfile::read_only(); let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd, + &cwd_uri, )); assert_eq!( assess_patch_safety( @@ -189,7 +202,7 @@ fn read_only_policy_rejects_patch_with_read_only_reason() { AskForApproval::Never, &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled, ), SafetyCheck::Reject { @@ -201,9 +214,13 @@ fn read_only_policy_rejects_patch_with_read_only_reason() { fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let blocked_path = cwd.join("blocked.txt"); let blocked_absolute = blocked_path; - let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string()); + let action = ApplyPatchAction::new_add_for_test( + &PathUri::from_abs_path(&blocked_absolute), + "".to_string(), + ); let permission_profile = PermissionProfile::External { network: NetworkSandboxPolicy::Restricted, }; @@ -225,7 +242,7 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd, + &cwd_uri, )); assert_eq!( assess_patch_safety( @@ -233,7 +250,7 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled, ), SafetyCheck::AskUser, @@ -244,10 +261,14 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let blocked_path = cwd.join("docs").join("blocked.txt"); let blocked_absolute = blocked_path; let docs_absolute = AbsolutePathBuf::resolve_path_against_base("docs", &cwd); - let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string()); + let action = ApplyPatchAction::new_add_for_test( + &PathUri::from_abs_path(&blocked_absolute), + "".to_string(), + ); let permission_profile = PermissionProfile::External { network: NetworkSandboxPolicy::Restricted, }; @@ -269,7 +290,7 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd, + &cwd_uri, )); assert_eq!( assess_patch_safety( @@ -277,7 +298,7 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled, ), SafetyCheck::AskUser, @@ -288,8 +309,10 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { fn missing_project_dot_codex_config_requires_approval() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().abs(); + let cwd_uri = PathUri::from_abs_path(&cwd); let config_path = cwd.join(".codex").join("config.toml"); - let action = ApplyPatchAction::new_add_for_test(&config_path, "".to_string()); + let action = + ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&config_path), "".to_string()); let permission_profile = PermissionProfile::workspace_write_with( &[], NetworkSandboxPolicy::Restricted, @@ -309,7 +332,7 @@ fn missing_project_dot_codex_config_requires_approval() { assert!(!is_write_patch_constrained_to_writable_paths( &action, &file_system_sandbox_policy, - &cwd, + &cwd_uri, )); assert_eq!( assess_patch_safety( @@ -317,7 +340,7 @@ fn missing_project_dot_codex_config_requires_approval() { AskForApproval::OnRequest, &permission_profile, &file_system_sandbox_policy, - &cwd, + &cwd_uri, WindowsSandboxLevel::Disabled, ), SafetyCheck::AskUser, diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 8034dfd627..34c41d835d 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -632,7 +632,7 @@ mod tests { use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::items::TurnItem; use codex_protocol::protocol::PatchApplyStatus; - use codex_utils_absolute_path::AbsolutePathBuf; + use codex_utils_path_uri::PathUri; use std::sync::Arc; use tempfile::tempdir; use tokio::sync::Mutex; @@ -645,7 +645,7 @@ mod tests { make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await; let tracker = Arc::new(Mutex::new(TurnDiffTracker::new())); let dir = tempdir().expect("tempdir"); - let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).expect("absolute cwd"); + let cwd = PathUri::from_path(dir.path()).expect("absolute cwd"); let mut stdout = Vec::new(); let mut stderr = Vec::new(); let delta = codex_apply_patch::apply_patch( @@ -729,7 +729,7 @@ mod tests { make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await; let tracker = Arc::new(Mutex::new(TurnDiffTracker::new())); let dir = tempdir().expect("tempdir"); - let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).expect("absolute cwd"); + let cwd = PathUri::from_path(dir.path()).expect("absolute cwd"); for patch in [ "*** Begin Patch\n*** Add File: a.txt\n+one\n*** End Patch", @@ -782,7 +782,7 @@ mod tests { make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await; let tracker = Arc::new(Mutex::new(TurnDiffTracker::new())); let dir = tempdir().expect("tempdir"); - let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).expect("absolute cwd"); + let cwd = PathUri::from_path(dir.path()).expect("absolute cwd"); let mut stdout = Vec::new(); let mut stderr = Vec::new(); let delta = codex_apply_patch::apply_patch( diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 9a00fac94d..5bb2b013c3 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -203,33 +203,21 @@ fn format_update_chunks_for_progress(chunks: &[codex_apply_patch::UpdateFileChun unified_diff } -fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { +fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { let mut keys = Vec::new(); - // TODO(anp): Migrate permission path accounting to PathUri. - let Ok(cwd) = action.cwd.to_abs_path() else { - return keys; - }; - for (path, change) in action.changes() { - if let Some(key) = to_abs_path(&cwd, path) { - keys.push(key); - } + keys.push(path.clone()); if let ApplyPatchFileChange::Update { move_path, .. } = change && let Some(dest) = move_path - && let Some(key) = to_abs_path(&cwd, dest) { - keys.push(key); + keys.push(dest.clone()); } } keys } -fn to_abs_path(cwd: &AbsolutePathBuf, path: &Path) -> Option { - Some(AbsolutePathBuf::resolve_path_against_base(path, cwd)) -} - fn write_permissions_for_paths( file_paths: &[AbsolutePathBuf], file_system_sandbox_policy: &codex_protocol::permissions::FileSystemSandboxPolicy, @@ -275,13 +263,14 @@ async fn effective_patch_permissions( turn: &TurnContext, environment_id: &str, action: &ApplyPatchAction, - cwd: &AbsolutePathBuf, -) -> ( - Vec, + cwd: &PathUri, +) -> std::io::Result<( + Vec, crate::tools::handlers::EffectiveAdditionalPermissions, codex_protocol::permissions::FileSystemSandboxPolicy, -) { +)> { let file_paths = file_paths_for_action(action); + let native_cwd = cwd.to_abs_path()?; let granted_permissions = merge_permission_profiles( session .granted_session_permissions(environment_id) @@ -297,19 +286,43 @@ async fn effective_patch_permissions( &base_file_system_sandbox_policy, granted_permissions.as_ref(), ); + let native_file_paths = file_paths + .iter() + .map(PathUri::to_abs_path) + .collect::, _>>()?; let effective_additional_permissions = apply_granted_turn_permissions( session, environment_id, - cwd.as_path(), + native_cwd.as_path(), crate::sandboxing::SandboxPermissions::UseDefault, - write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, cwd), + write_permissions_for_paths(&native_file_paths, &file_system_sandbox_policy, &native_cwd), ) .await; - ( + Ok(( file_paths, effective_additional_permissions, file_system_sandbox_policy, + )) +} + +fn patch_permissions_without_path_matching( + action: &ApplyPatchAction, +) -> ( + Vec, + crate::tools::handlers::EffectiveAdditionalPermissions, + codex_protocol::permissions::FileSystemSandboxPolicy, +) { + // TODO(anp): Make permission matching operate on PathUri. Until then, foreign paths skip + // permission matching; a managed turn still fails closed at the platform sandbox boundary. + ( + file_paths_for_action(action), + crate::tools::handlers::EffectiveAdditionalPermissions { + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + permissions_preapproved: false, + }, + codex_protocol::permissions::FileSystemSandboxPolicy::unrestricted(), ) } @@ -366,21 +379,18 @@ impl ApplyPatchHandler { "apply_patch is unavailable in this session".to_string(), )); }; - // TODO(anp): Migrate apply-patch verification and permission accounting to PathUri so - // patches can target environment-native foreign paths without host projection. - let cwd = turn_environment.cwd().to_abs_path().map_err(|err| { - FunctionCallError::RespondToModel(format!( - "apply_patch cwd `{}` is not native to the Codex host: {err}", - turn_environment.cwd() - )) - })?; let fs = turn_environment.environment.get_filesystem(); let sandbox = turn.file_system_sandbox_context( /*additional_permissions*/ None, turn_environment.cwd(), ); - match codex_apply_patch::verify_apply_patch_args(args, &cwd, fs.as_ref(), Some(&sandbox)) - .await + match codex_apply_patch::verify_apply_patch_args( + args, + turn_environment.cwd(), + fs.as_ref(), + Some(&sandbox), + ) + .await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = @@ -389,9 +399,10 @@ impl ApplyPatchHandler { turn.as_ref(), &turn_environment.environment_id, &changes, - &cwd, + turn_environment.cwd(), ) - .await; + .await + .unwrap_or_else(|_| patch_permissions_without_path_matching(&changes)); match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) .await { @@ -531,7 +542,7 @@ impl CoreToolRuntime for ApplyPatchHandler { #[allow(clippy::too_many_arguments)] pub(crate) async fn intercept_apply_patch( command: &[String], - cwd: &AbsolutePathBuf, + cwd: &PathUri, fs: &dyn ExecutorFileSystem, turn_environment: TurnEnvironment, session: Arc, @@ -540,9 +551,7 @@ pub(crate) async fn intercept_apply_patch( call_id: &str, tool_name: &str, ) -> Result, FunctionCallError> { - let sandbox_cwd = PathUri::from_abs_path(cwd); - let sandbox = - turn.file_system_sandbox_context(/*additional_permissions*/ None, &sandbox_cwd); + let sandbox = turn.file_system_sandbox_context(/*additional_permissions*/ None, cwd); match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd, fs, Some(&sandbox)) .await { @@ -555,7 +564,8 @@ pub(crate) async fn intercept_apply_patch( &changes, cwd, ) - .await; + .await + .unwrap_or_else(|_| patch_permissions_without_path_matching(&changes)); match apply_patch::apply_patch(turn.as_ref(), &file_system_sandbox_policy, changes) .await { diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index e9a118641a..f8d5f9710e 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -225,6 +225,8 @@ async fn approval_keys_include_move_destination() { +new content *** End Patch"#; let argv = vec!["apply_patch".to_string(), patch.to_string()]; + // TODO(anp): Keep apply_patch handler test cwd values as PathUri. + let cwd = PathUri::from_abs_path(&cwd); let action = match codex_apply_patch::maybe_parse_apply_patch_verified( &argv, &cwd, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 3e1f160e9c..131bcdba95 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -26,6 +26,7 @@ use crate::tools::sandboxing::ToolCtx; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::ExecCommandSource; use codex_tools::ToolName; +use codex_utils_path_uri::PathUri; mod shell_command; @@ -139,9 +140,10 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result, + pub file_paths: Vec, pub changes: std::collections::HashMap, pub exec_approval_requirement: ExecApprovalRequirement, pub additional_permissions: Option, @@ -81,10 +80,15 @@ impl ApplyPatchRuntime { ) -> std::io::Result { // TODO(anp): Remove this conversion once the guardian API supports PathUri. let cwd = req.action.cwd.to_abs_path()?; + let files = req + .file_paths + .iter() + .map(PathUri::to_abs_path) + .collect::>>()?; Ok(GuardianApprovalRequest::ApplyPatch { id: call_id.to_string(), cwd, - files: req.file_paths.clone(), + files, patch: req.action.patch.clone(), }) } @@ -245,15 +249,9 @@ impl ToolRuntime for ApplyPatchRunti let sandbox = Self::file_system_sandbox_context_for_attempt(req, attempt); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - // TODO(anp): Teach apply_patch to operate on PathUri directly. - let cwd = req - .action - .cwd - .to_abs_path() - .map_err(|err| ToolError::Rejected(err.to_string()))?; let result = codex_apply_patch::apply_patch( &req.action.patch, - &cwd, + &req.action.cwd, &mut stdout, &mut stderr, fs.as_ref(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index 0dc1ed9a67..054f2c7c15 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -53,13 +53,14 @@ async fn guardian_review_request_includes_patch_context() { let path = std::env::temp_dir() .join("guardian-apply-patch-test.txt") .abs(); - let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); + let action = + ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&path), "hello".to_string()); let expected_cwd = action.cwd.to_abs_path().expect("native patch cwd"); let expected_patch = action.patch.clone(); let request = ApplyPatchRequest { turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID), action, - file_paths: vec![path.clone()], + file_paths: vec![PathUri::from_abs_path(&path)], changes: HashMap::from([( path.to_path_buf(), FileChange::Add { @@ -82,7 +83,7 @@ async fn guardian_review_request_includes_patch_context() { GuardianApprovalRequest::ApplyPatch { id: "call-1".to_string(), cwd: expected_cwd, - files: request.file_paths, + files: vec![path], patch: expected_patch, } ); @@ -94,12 +95,13 @@ async fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() { let path = std::env::temp_dir() .join("apply-patch-permission-request-payload.txt") .abs(); - let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); + let action = + ApplyPatchAction::new_add_for_test(&PathUri::from_abs_path(&path), "hello".to_string()); let expected_patch = action.patch.clone(); let req = ApplyPatchRequest { turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID), action, - file_paths: vec![path], + file_paths: vec![PathUri::from_abs_path(&path)], changes: HashMap::new(), exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { reason: None, @@ -130,10 +132,11 @@ async fn approval_keys_include_environment_id() { let path = std::env::temp_dir() .join("apply-patch-approval-key.txt") .abs(); + let path_uri = PathUri::from_abs_path(&path); let req = ApplyPatchRequest { turn_environment: test_turn_environment("remote"), - action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()), - file_paths: vec![path.clone()], + action: ApplyPatchAction::new_add_for_test(&path_uri, "hello".to_string()), + file_paths: vec![path_uri.clone()], changes: HashMap::new(), exec_approval_requirement: ExecApprovalRequirement::Skip { bypass_sandbox: false, @@ -150,7 +153,7 @@ async fn approval_keys_include_environment_id() { serde_json::json!([ { "environment_id": "remote", - "path": path, + "path": path_uri, } ]) ); @@ -164,8 +167,11 @@ async fn sandbox_cwd_uses_patch_action_cwd() { .abs(); let req = ApplyPatchRequest { turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID), - action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()), - file_paths: vec![path.clone()], + action: ApplyPatchAction::new_add_for_test( + &PathUri::from_abs_path(&path), + "hello".to_string(), + ), + file_paths: vec![PathUri::from_abs_path(&path)], changes: HashMap::new(), exec_approval_requirement: ExecApprovalRequirement::Skip { bypass_sandbox: false, @@ -192,8 +198,11 @@ async fn file_system_sandbox_context_uses_active_attempt() { }; let req = ApplyPatchRequest { turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID), - action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()), - file_paths: vec![path.clone()], + action: ApplyPatchAction::new_add_for_test( + &PathUri::from_abs_path(&path), + "hello".to_string(), + ), + file_paths: vec![PathUri::from_abs_path(&path)], changes: HashMap::new(), exec_approval_requirement: ExecApprovalRequirement::Skip { bypass_sandbox: false, @@ -259,8 +268,11 @@ async fn no_sandbox_attempt_has_no_file_system_context() { .abs(); let req = ApplyPatchRequest { turn_environment: test_turn_environment(codex_exec_server::LOCAL_ENVIRONMENT_ID), - action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()), - file_paths: vec![path.clone()], + action: ApplyPatchAction::new_add_for_test( + &PathUri::from_abs_path(&path), + "hello".to_string(), + ), + file_paths: vec![PathUri::from_abs_path(&path)], changes: HashMap::new(), exec_approval_requirement: ExecApprovalRequirement::Skip { bypass_sandbox: false, diff --git a/codex-rs/core/src/turn_diff_tracker_tests.rs b/codex-rs/core/src/turn_diff_tracker_tests.rs index a7be3d2b0c..9c58de913b 100644 --- a/codex-rs/core/src/turn_diff_tracker_tests.rs +++ b/codex-rs/core/src/turn_diff_tracker_tests.rs @@ -4,7 +4,7 @@ use codex_apply_patch::MaybeApplyPatchVerified; use codex_exec_server::LOCAL_FS; use codex_git_utils::ApplyGitRequest; use codex_git_utils::apply_git_patch; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; @@ -18,7 +18,7 @@ fn git_blob_sha1_hex(data: &str) -> String { } async fn apply_verified_patch(root: &Path, patch: &str) -> AppliedPatchDelta { - let cwd = AbsolutePathBuf::from_absolute_path(root).expect("absolute tempdir path"); + let cwd = PathUri::from_path(root).expect("absolute tempdir path"); let argv = vec!["apply_patch".to_string(), patch.to_string()]; match codex_apply_patch::maybe_parse_apply_patch_verified( &argv, diff --git a/codex-rs/core/tests/remote_env_windows/remote_env_windows_test.rs b/codex-rs/core/tests/remote_env_windows/remote_env_windows_test.rs index ae3c9406a9..cff8ded739 100644 --- a/codex-rs/core/tests/remote_env_windows/remote_env_windows_test.rs +++ b/codex-rs/core/tests/remote_env_windows/remote_env_windows_test.rs @@ -50,7 +50,11 @@ const APP_SERVER_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_s #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> { const CALL_ID: &str = "wine-cmd-smoke"; + const PATCH_CALL_ID: &str = "wine-apply-patch"; + const VERIFY_CALL_ID: &str = "wine-verify-patch"; + const PATCH_FILE: &str = "codex-apply-patch-smoke.txt"; const COMMAND: &str = r#"if ((Get-Location).Path -ne 'C:\windows') { exit 1 }"#; + const VERIFY_COMMAND: &str = r#"$path = Join-Path (Get-Location) 'codex-apply-patch-smoke.txt'; if (-not (Test-Path $path)) { exit 1 }; if ([IO.File]::ReadAllText($path) -ne "patched through unified exec`n") { exit 2 }; Remove-Item $path; Write-Output 'PATCH_VERIFIED'"#; WineExecServer .scope(|exec_server_url| async move { @@ -63,6 +67,22 @@ async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> { "workdir": r"C:\windows", "yield_time_ms": 10_000, }))?; + let patch = format!( + "*** Begin Patch\n*** Add File: {PATCH_FILE}\n+patched through unified exec\n*** End Patch" + ); + let patch_arguments = serde_json::to_string(&json!({ + "cmd": format!("apply_patch <<'EOF'\n{patch}\nEOF\n"), + "login": false, + // Resolve this relative workdir using the selected Windows environment cwd. + "workdir": r"apply-patch-smoke\nested", + "yield_time_ms": 10_000, + }))?; + let verify_arguments = serde_json::to_string(&json!({ + "cmd": VERIFY_COMMAND, + "login": false, + "workdir": r"apply-patch-smoke\nested", + "yield_time_ms": 10_000, + }))?; let response_mock = mount_sse_sequence( &server, vec![ @@ -73,9 +93,19 @@ async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> { ]), sse(vec![ ev_response_created("resp-2"), - ev_assistant_message("msg-1", "done"), + ev_function_call(PATCH_CALL_ID, "exec_command", &patch_arguments), ev_completed("resp-2"), ]), + sse(vec![ + ev_response_created("resp-3"), + ev_function_call(VERIFY_CALL_ID, "exec_command", &verify_arguments), + ev_completed("resp-3"), + ]), + sse(vec![ + ev_response_created("resp-4"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-4"), + ]), ], ) .await; @@ -97,7 +127,7 @@ async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> { test.config.cwd.clone(), vec![TurnEnvironmentSelection { environment_id: REMOTE_ENVIRONMENT_ID.to_string(), - cwd: PathUri::parse("file:///C:/windows")?, + cwd: PathUri::parse("file:///C:/codex-home")?, }], ); @@ -130,6 +160,7 @@ async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> { let mut begin = None; let mut end = None; + let mut patch_end = None; let mut turn_complete = false; loop { match wait_for_event(&test.codex, |_| true).await { @@ -139,6 +170,9 @@ async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> { EventMsg::ExecCommandEnd(event) if event.call_id == CALL_ID => { end = Some(event) } + EventMsg::PatchApplyEnd(event) if event.call_id == PATCH_CALL_ID => { + patch_end = Some(event) + } EventMsg::TurnComplete(_) => turn_complete = true, _ => {} } @@ -162,14 +196,48 @@ async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> { assert_eq!((&begin.cwd, &end.cwd), (&expected_cwd, &expected_cwd)); assert_eq!((end.exit_code, end.status), (0, ExecCommandStatus::Completed)); + let patch_end = patch_end.context("intercepted apply_patch should emit an end event")?; + assert!( + patch_end.success, + "intercepted apply_patch failed: stdout={:?} stderr={:?}", + patch_end.stdout, patch_end.stderr + ); + assert!( + patch_end + .changes + .contains_key(&std::path::PathBuf::from(format!( + r"C:\codex-home\apply-patch-smoke\nested\{PATCH_FILE}" + ))), + "apply_patch should retain the Windows cwd: {:?}", + patch_end.changes + ); let request = response_mock .last_request() .context("model should receive the command output")?; + let (verify_output, verify_success) = request + .function_call_output_content_and_success(VERIFY_CALL_ID) + .context("verification output should be present")?; + anyhow::ensure!( + verify_success != Some(false), + "verification command failed: {verify_output:?}" + ); + anyhow::ensure!( + verify_output + .as_deref() + .is_some_and(|output| output.contains("PATCH_VERIFIED")), + "verification command did not confirm the patched file: {verify_output:?}" + ); + let (_output, success) = request .function_call_output_content_and_success(CALL_ID) .context("command output should be present")?; assert_ne!(success, Some(false)); - + let (patch_output, patch_success) = request + .function_call_output_content_and_success(PATCH_CALL_ID) + .context("apply_patch output should be present")?; + let patch_output = patch_output.context("apply_patch output should contain text")?; + assert!(patch_output.contains(PATCH_FILE)); + assert_ne!(patch_success, Some(false)); Ok(()) }) .await diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index a1f4b95b50..9745fa878e 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -1719,6 +1719,15 @@ async fn handle_server_request( ) .await } + ServerRequest::CurrentTimeRead { request_id, .. } => { + reject_server_request( + client, + request_id, + &method, + "external current time is not supported in exec mode".to_string(), + ) + .await + } ServerRequest::ApplyPatchApproval { request_id, params } => { reject_server_request( client, diff --git a/codex-rs/prompts/src/permissions_instructions.rs b/codex-rs/prompts/src/permissions_instructions.rs index f360a987db..a4a043f24f 100644 --- a/codex-rs/prompts/src/permissions_instructions.rs +++ b/codex-rs/prompts/src/permissions_instructions.rs @@ -22,6 +22,8 @@ const APPROVAL_POLICY_ON_FAILURE: &str = include_str!("../templates/permissions/approval_policy/on_failure.md"); const APPROVAL_POLICY_ON_REQUEST_RULE: &str = include_str!("../templates/permissions/approval_policy/on_request.md"); +const APPROVAL_POLICY_ON_REQUEST_AUTO_REVIEW: &str = + include_str!("../templates/permissions/approval_policy/on_request_auto_review.md"); const APPROVAL_POLICY_ON_REQUEST_RULE_REQUEST_PERMISSION: &str = include_str!("../templates/permissions/approval_policy/on_request_rule_request_permission.md"); const AUTO_REVIEW_APPROVAL_SUFFIX: &str = "`approvals_reviewer` is `auto_review`: Sandbox escalations with require_escalated will be reviewed for compliance with the policy. If a rejection happens, you should proceed only with a materially safer alternative, or inform the user of the risk and send a final message to ask for approval."; @@ -208,6 +210,8 @@ fn approval_text( let on_request_instructions = || { let on_request_rule = if exec_permission_approvals_enabled { APPROVAL_POLICY_ON_REQUEST_RULE_REQUEST_PERMISSION.to_string() + } else if approvals_reviewer == ApprovalsReviewer::AutoReview { + APPROVAL_POLICY_ON_REQUEST_AUTO_REVIEW.to_string() } else { APPROVAL_POLICY_ON_REQUEST_RULE.to_string() }; diff --git a/codex-rs/prompts/src/permissions_instructions_tests.rs b/codex-rs/prompts/src/permissions_instructions_tests.rs index 580e9781ce..8564a7cf1e 100644 --- a/codex-rs/prompts/src/permissions_instructions_tests.rs +++ b/codex-rs/prompts/src/permissions_instructions_tests.rs @@ -257,17 +257,98 @@ fn on_request_includes_tool_guidance_alongside_inline_permission_guidance_when_b #[test] fn auto_review_approvals_append_auto_review_specific_guidance() { + let mut exec_policy = Policy::empty(); + exec_policy + .add_prefix_rule(&["git".to_string(), "pull".to_string()], Decision::Allow) + .expect("add rule"); let text = approval_text( AskForApproval::OnRequest, ApprovalsReviewer::AutoReview, - &Policy::empty(), + &exec_policy, /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, ); assert!(text.contains("`approvals_reviewer` is `auto_review`")); - assert!(!text.contains("`approvals_reviewer` is `guardian_subagent`")); assert!(text.contains("materially safer alternative")); + assert!(!text.contains("`approvals_reviewer` is `guardian_subagent`")); + assert!(text.contains( + "When the sandbox is likely to block a command needed for the task, request escalation up front" + )); + assert!(text.contains( + "When unsure, prefer requesting escalation unnecessarily over failing to request it when needed" + )); + assert!( + text.contains( + "Request escalation for commands that need write access outside writable roots" + ) + ); + assert!(text.contains( + "Request escalation for git operations that may write lock files, such as updating the index or refs" + )); + assert!(!text.contains("Request escalation for GUI commands")); + assert!(!text.contains("Use escalation when it is the direct or most reliable way")); + assert!(!text.contains("Do not spend extra turns running likely-to-fail sandbox probes first")); + assert!(text.contains("Do not include a `prefix_rule` parameter.")); + assert!(text.contains( + "Request escalation for commands that may need network access, including HTTP calls, package registries, internal services, data-service APIs, remote queries, data fetches, or live probes" + )); + assert!(text.contains( + "Request escalation for commands that may need remote authentication, cluster, cloud, or database access" + )); + assert!(text.contains( + "Request escalation for commands that may need process, cache, or other environment access outside the sandbox" + )); + assert!( + text.contains( + "including DNS, connection, authentication, retry, or service endpoint errors" + ) + ); + assert!(text.contains( + "Request escalation before potentially destructive actions, such as `rm` or `git reset`" + )); + assert!(text.contains( + "If a command may be hanging on sandbox-blocked access, stop after a short timeout and rerun with `require_escalated`" + )); + assert!(!text.contains("Be judicious with escalating")); + assert!(text.contains("Approved command prefixes")); + assert!(text.contains(r#"["git", "pull"]"#)); + assert!(!text.contains("prefix_rule guidance")); +} + +#[test] +fn normal_on_request_keeps_user_approval_wording() { + let text = approval_text( + AskForApproval::OnRequest, + ApprovalsReviewer::User, + &Policy::empty(), + /*exec_permission_approvals_enabled*/ false, + /*request_permissions_tool_enabled*/ false, + ); + + assert!(text.contains("approved by the user")); + assert!(text.contains("Be judicious with escalating")); + assert!(!text.contains( + "When the sandbox is likely to block a command needed for the task, request escalation up front" + )); +} + +#[test] +fn auto_review_on_request_with_inline_permission_requests_keeps_suffix() { + let text = approval_text( + AskForApproval::OnRequest, + ApprovalsReviewer::AutoReview, + &Policy::empty(), + /*exec_permission_approvals_enabled*/ true, + /*request_permissions_tool_enabled*/ false, + ); + + assert!(text.contains("with_additional_permissions")); + assert!(text.contains("`approvals_reviewer` is `auto_review`")); + assert!(text.contains("materially safer alternative")); + assert!(!text.contains( + "When the sandbox is likely to block a command needed for the task, request escalation up front" + )); } #[test] diff --git a/codex-rs/prompts/templates/permissions/approval_policy/on_request_auto_review.md b/codex-rs/prompts/templates/permissions/approval_policy/on_request_auto_review.md new file mode 100644 index 0000000000..82bb24e963 --- /dev/null +++ b/codex-rs/prompts/templates/permissions/approval_policy/on_request_auto_review.md @@ -0,0 +1,45 @@ +# Escalation Requests + +Commands are run outside the sandbox after approval. The command string is split into independent command segments at shell control operators, including but not limited to: + +- Pipes: | +- Logical operators: &&, || +- Command separators: ; +- Subshell boundaries: (...), $() + +Each resulting segment is evaluated independently for sandbox restrictions and approval requirements. + +Example: + +git pull | tee output.txt + +This is treated as two command segments: + +["git", "pull"] + +["tee", "output.txt"] + +Commands that use more advanced shell features like redirection (>, >>, <), substitutions ($(...), ...), environment variables (FOO=bar), or wildcard patterns (*, ?) require care because each independent command segment is evaluated separately. + +## How to request escalation + +IMPORTANT: To request approval to execute a command that will require escalated privileges: + +- Provide the `sandbox_permissions` parameter with the value `"require_escalated"` +- Include a concise `justification` parameter that explains why escalated privileges are needed. +- Do not include a `prefix_rule` parameter. + +## When to request escalation + +While commands are running inside the sandbox, here are some scenarios that justify escalation: + +- When the sandbox is likely to block a command needed for the task, request escalation up front. +- Request escalation for commands that need write access outside writable roots, such as tests that write to `/var`. +- Request escalation for git operations that may write lock files, such as updating the index or refs. +- Request escalation for commands that may need network access, including HTTP calls, package registries, internal services, data-service APIs, remote queries, data fetches, or live probes. +- Request escalation for commands that may need remote authentication, cluster, cloud, or database access. +- Request escalation for commands that may need process, cache, or other environment access outside the sandbox. +- If a sandboxed attempt fails with sandboxing or likely network symptoms, including DNS, connection, authentication, retry, or service endpoint errors, rerun with `sandbox_permissions` set to `"require_escalated"` and include `justification`. +- If a command may be hanging on sandbox-blocked access, stop after a short timeout and rerun with `require_escalated`. +- Request escalation before potentially destructive actions, such as `rm` or `git reset`, that the user did not explicitly ask for. +- When unsure, prefer requesting escalation unnecessarily over failing to request it when needed. diff --git a/codex-rs/tui/src/app/app_server_event_targets.rs b/codex-rs/tui/src/app/app_server_event_targets.rs index 90f688f7ed..e29d0e9d5d 100644 --- a/codex-rs/tui/src/app/app_server_event_targets.rs +++ b/codex-rs/tui/src/app/app_server_event_targets.rs @@ -24,6 +24,9 @@ pub(super) fn server_request_thread_id(request: &ServerRequest) -> Option { ThreadId::from_string(¶ms.thread_id).ok() } + ServerRequest::CurrentTimeRead { params, .. } => { + ThreadId::from_string(¶ms.thread_id).ok() + } ServerRequest::ChatgptAuthTokensRefresh { .. } | ServerRequest::AttestationGenerate { .. } | ServerRequest::ApplyPatchApproval { .. } diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index d61e84dcec..0b72897685 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -153,6 +153,12 @@ impl PendingAppServerRequests { message: "Attestation generation is not available in TUI.".to_string(), }) } + ServerRequest::CurrentTimeRead { request_id, .. } => { + Some(UnsupportedAppServerRequest { + request_id: request_id.clone(), + message: "External current time is not available in TUI.".to_string(), + }) + } ServerRequest::ApplyPatchApproval { request_id, .. } => { Some(UnsupportedAppServerRequest { request_id: request_id.clone(), @@ -352,6 +358,7 @@ impl PendingAppServerRequests { ServerRequest::DynamicToolCall { .. } | ServerRequest::ChatgptAuthTokensRefresh { .. } | ServerRequest::AttestationGenerate { .. } + | ServerRequest::CurrentTimeRead { .. } | ServerRequest::ApplyPatchApproval { .. } | ServerRequest::ExecCommandApproval { .. } => true, } diff --git a/codex-rs/tui/src/app/side.rs b/codex-rs/tui/src/app/side.rs index 167fc5c7cb..0b69153fa5 100644 --- a/codex-rs/tui/src/app/side.rs +++ b/codex-rs/tui/src/app/side.rs @@ -97,6 +97,7 @@ impl SideParentStatus { | ServerRequest::ExecCommandApproval { .. } => Some(SideParentStatus::NeedsApproval), ServerRequest::DynamicToolCall { .. } | ServerRequest::AttestationGenerate { .. } + | ServerRequest::CurrentTimeRead { .. } | ServerRequest::ChatgptAuthTokensRefresh { .. } => None, } } diff --git a/codex-rs/tui/src/chatwidget/protocol_requests.rs b/codex-rs/tui/src/chatwidget/protocol_requests.rs index 2f33c9809a..5729c073fd 100644 --- a/codex-rs/tui/src/chatwidget/protocol_requests.rs +++ b/codex-rs/tui/src/chatwidget/protocol_requests.rs @@ -46,6 +46,7 @@ impl ChatWidget { } ServerRequest::DynamicToolCall { .. } | ServerRequest::AttestationGenerate { .. } + | ServerRequest::CurrentTimeRead { .. } | ServerRequest::ChatgptAuthTokensRefresh { .. } | ServerRequest::ApplyPatchApproval { .. } | ServerRequest::ExecCommandApproval { .. } => { diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index 5e8b5b2cbc..6bc80b5561 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -12,6 +12,7 @@ use serde::Serializer; use std::fmt; use std::io; use std::path::Path; +use std::path::PathBuf; use std::str::FromStr; use thiserror::Error; use ts_rs::TS; @@ -96,6 +97,7 @@ impl PathUri { Self::from_opaque_path_bytes(&path_bytes) } + /// Parses an absolute native path using the specified path convention. pub(crate) fn from_absolute_native_path( path: &str, convention: PathConvention, @@ -202,6 +204,11 @@ impl PathUri { .map(decode_uri_path) } + /// Renders this URI as a path-flavored string using its inferred convention. + pub fn to_path_buf(&self) -> PathBuf { + PathBuf::from(self.inferred_native_path_string()) + } + /// Returns the parent URI, or `None` for the URI root or an opaque fallback /// URI created by [`Self::from_abs_path`]. pub fn parent(&self) -> Option { @@ -310,15 +317,19 @@ impl PathUri { /// Converts this file URI to a path using the current host's path rules. /// - /// Conversion should succeed when the URI was created from an - /// [`AbsolutePathBuf`] on the current host, including fallback URIs created - /// by [`Self::from_abs_path`]. It may fail when the URI came from a different - /// operating system and its `file:` URI form cannot be represented using - /// the current host's path rules, such as a UNC authority on POSIX or a - /// POSIX root on Windows. Because a `file:` URI does not record its source - /// operating system, callers should only use this method when the URI is - /// known to identify a path on the current host. + /// The URI's inferred path convention must match the current host. Conversion should succeed + /// when the URI was created from an [`AbsolutePathBuf`] on the current host, including fallback + /// URIs created by [`Self::from_abs_path`]. Foreign conventions are rejected rather than being + /// projected onto a syntactically valid but unrelated host path. pub fn to_abs_path(&self) -> io::Result { + if self.infer_path_convention() != Some(PathConvention::native()) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + PathUriParseError::InvalidFileUriPath { + path: self.to_string(), + }, + )); + } if let Some(path_bytes) = decode_bad_path_uri(&self.0) { #[cfg(unix)] let decoded_path = { @@ -688,6 +699,17 @@ impl PathConvention { pub const fn native() -> Self { Self::Posix } + + /// Splits absolute or relative native path text into lexical segments. + /// + /// This does not validate the path or require it to be absolute. POSIX paths split on `/`, + /// while Windows paths split on both `\\` and `/`. Empty segments are retained. + pub fn path_segments(self, path: &str) -> impl DoubleEndedIterator { + path.split(move |character| match self { + Self::Posix => character == '/', + Self::Windows => matches!(character, '/' | '\\'), + }) + } } impl fmt::Display for PathConvention { diff --git a/codex-rs/utils/path-uri/src/tests.rs b/codex-rs/utils/path-uri/src/tests.rs index 4ccb9e9c81..328142dc6d 100644 --- a/codex-rs/utils/path-uri/src/tests.rs +++ b/codex-rs/utils/path-uri/src/tests.rs @@ -34,21 +34,24 @@ fn file_uri_round_trips_an_absolute_path() { #[test] fn non_native_uri_io_conversion_is_invalid_input() { #[cfg(unix)] - let uri = PathUri::parse("file://server/share/file.txt").expect("valid file URI"); + let uris = ["file://server/share/file.txt", "file:///C:/workspace"]; #[cfg(windows)] - let uri = PathUri::parse("file:///usr/local/file.txt").expect("valid file URI"); + let uris = ["file:///usr/local/file.txt"]; - let error = uri - .to_abs_path() - .expect_err("URI should not be host-native"); + for uri in uris { + let uri = PathUri::parse(uri).expect("valid file URI"); + let error = uri + .to_abs_path() + .expect_err("URI should not be host-native"); - assert_eq!( - (error.kind(), error.to_string()), - ( - io::ErrorKind::InvalidInput, - format!("'{uri}' is invalid on '{}'", std::env::consts::OS), - ) - ); + assert_eq!( + (error.kind(), error.to_string()), + ( + io::ErrorKind::InvalidInput, + format!("'{uri}' is invalid on '{}'", std::env::consts::OS), + ) + ); + } } #[test] @@ -90,6 +93,35 @@ fn infers_path_conventions_from_uri_shape() { } } +#[test] +fn path_convention_splits_absolute_relative_and_bare_path_text() { + for (convention, path, expected) in [ + ( + PathConvention::Posix, + "/usr/local/bin/bash", + vec!["", "usr", "local", "bin", "bash"], + ), + ( + PathConvention::Posix, + r"tools\pwsh.exe", + vec![r"tools\pwsh.exe"], + ), + ( + PathConvention::Windows, + r"C:\Program Files\PowerShell\7\pwsh.exe", + vec!["C:", "Program Files", "PowerShell", "7", "pwsh.exe"], + ), + ( + PathConvention::Windows, + "tools/pwsh.exe", + vec!["tools", "pwsh.exe"], + ), + (PathConvention::Windows, "cmd.exe", vec!["cmd.exe"]), + ] { + assert_eq!(convention.path_segments(path).collect::>(), expected); + } +} + #[test] fn drive_shaped_posix_uri_is_intentionally_inferred_as_windows() { let path = PathUri::parse("file:///C:/actually/a/posix/path").expect("valid path URI"); @@ -475,6 +507,20 @@ fn basename_uses_decoded_uri_segments() { } } +#[test] +fn path_buf_uses_the_inferred_native_spelling() { + let windows = PathUri::parse("file:///C:/Program%20Files/pwsh.exe").expect("Windows URI"); + let posix = PathUri::parse("file:///usr/local/bin/bash").expect("POSIX URI"); + + assert_eq!( + (windows.to_path_buf(), posix.to_path_buf()), + ( + PathBuf::from(r"C:\Program Files\pwsh.exe"), + PathBuf::from("/usr/local/bin/bash"), + ) + ); +} + #[test] fn parent_uses_uri_hierarchy_and_preserves_authority() { for (input, expected) in [