Merge branch 'dev/winston/mitm-child-env-snapshots' into dev/winston/mitm-command-ca-bundles-restack-20260618

This commit is contained in:
Winston Howes
2026-06-18 13:22:35 -07:00
49 changed files with 1811 additions and 421 deletions
@@ -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, };
+37 -13
View File
@@ -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, &registered_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<PathBuf, String>) -> 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<String, HashSet<String>> = HashMap::new();
@@ -293,21 +299,21 @@ pub(crate) fn filter_experimental_ts_tree(tree: &mut BTreeMap<PathBuf, String>)
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, &registered_fields);
filter_experimental_fields_in_definitions(bundle, &registered_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<String> {
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
@@ -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 {
@@ -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,
}
@@ -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::*;
+4
View File
@@ -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.<opaque>" }`, 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.<opaque>" }`, 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`.
+146
View File
@@ -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<OutgoingMessageSender>,
thread_state_manager: ThreadStateManager,
) -> Arc<dyn TimeProvider> {
Arc::new(AppServerTimeProvider {
outgoing: Arc::downgrade(&outgoing),
thread_state_manager,
})
}
struct AppServerTimeProvider {
outgoing: Weak<OutgoingMessageSender>,
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<OutgoingMessageSender>,
thread_state_manager: ThreadStateManager,
thread_id: ThreadId,
) -> Result<DateTime<Utc>> {
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<ConnectionId> {
// 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"
);
}
}
+1
View File
@@ -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;
+5 -2
View File
@@ -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();
@@ -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();
+17
View File
@@ -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<ConnectionId> {
let state = self.state.lock().await;
state
@@ -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
"#
),
)
}
@@ -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;
+125 -81
View File
@@ -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<String> {
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<String> {
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<ApplyPatchShell> {
classify_shell_name(shell).and_then(|name| match name.as_str() {
fn classify_shell(shell: &str, flag: &str, convention: PathConvention) -> Option<ApplyPatchShell> {
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<ApplyPatchShell> {
})
}
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 <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 <path> &&`) 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<ApplyPatchAction, ApplyPatchError> {
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<String>, 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<String>,
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,
)
+137 -116
View File
@@ -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\", \"<patch>\"]"
@@ -109,7 +111,7 @@ pub enum ApplyPatchFileChange {
},
Update {
unified_diff: String,
move_path: Option<PathBuf>,
move_path: Option<PathUri>,
/// 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<PathBuf, ApplyPatchFileChange>,
changes: HashMap<PathUri, ApplyPatchFileChange>,
/// 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<PathBuf, ApplyPatchFileChange> {
pub fn changes(&self) -> &HashMap<PathUri, ApplyPatchFileChange> {
&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<String> {
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<u8>,
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<AppliedPatch, ApplyPatchError> {
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<Vec<(usize, usize, Vec<String>)>, ApplyPatchError> {
let mut replacements: Vec<(usize, usize, Vec<String>)> = 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<ApplyPatchFileUpdate, ApplyPatchError> {
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(),
+12 -11
View File
@@ -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<PathUri, PathUriParseError> {
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));
}
}
@@ -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,
+1
View File
@@ -122,6 +122,7 @@ pub fn arg0_dispatch() -> Option<Arg0PathEntryGuard> {
Ok(runtime) => runtime,
Err(_) => std::process::exit(1),
};
let cwd = cwd.into();
match runtime.block_on(codex_apply_patch::apply_patch(
&patch_arg,
&cwd,
+5 -6
View File
@@ -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::<ToolSuggestDiscoverablePlugin>::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,
+172 -5
View File
@@ -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]
+1
View File
@@ -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";
+45 -11
View File
@@ -768,6 +768,29 @@ fn apply_plugin_mcp_server_policy(config: &mut McpServerConfig, policy: &PluginM
}
}
pub(crate) struct PluginSkillInventory {
skills: Vec<SkillMetadata>,
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<SkillMetadata>,
@@ -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<AbsolutePathBuf>,
) -> 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<Product>,
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<Product>,
) -> 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::<Vec<_>>();
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(
+34 -7
View File
@@ -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<LoadedPluginsCache>,
loaded_plugins_load_semaphore: Semaphore,
tool_suggest_metadata_cache: ToolSuggestMetadataCache,
remote_installed_plugins_cache: RwLock<Option<Vec<RemoteInstalledPlugin>>>,
remote_installed_plugins_cache_refresh_state: RwLock<RemoteInstalledPluginsCacheRefreshState>,
global_remote_catalog_cache_refresh_state: RwLock<GlobalRemoteCatalogCacheRefreshState>,
@@ -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<PluginCapabilitySummary, MarketplaceError> {
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,
+100
View File
@@ -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();
+1 -1
View File
@@ -66,7 +66,7 @@ pub struct MarketplacePlugin {
pub keywords: Vec<String>,
}
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum MarketplacePluginSource {
Local {
path: AbsolutePathBuf,
@@ -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<Arc<ToolSuggestMetadataFragment>, 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<ToolSuggestMetadataCacheState>,
load_semaphore: Semaphore,
}
#[derive(Default)]
struct ToolSuggestMetadataCacheState {
generation: u64,
entries: HashMap<PluginArtifactIdentity, ToolSuggestMetadataEntry>,
}
#[derive(Clone, PartialEq, Eq, Hash)]
struct PluginArtifactIdentity {
plugin_id: String,
source: MarketplacePluginSource,
}
pub(crate) struct ToolSuggestMetadataFragment {
config_name: String,
display_name: String,
description: Option<String>,
mcp_server_names: Vec<String>,
app_declarations: Vec<AppDeclaration>,
skill_inventory: Option<PluginSkillInventory>,
}
impl ToolSuggestMetadataFragment {
pub(crate) fn project(
&self,
skill_config_rules: &SkillConfigRules,
auth_mode: Option<AuthMode>,
) -> PluginCapabilitySummary {
let mut app_declarations = self.app_declarations.clone();
let mut mcp_servers = self
.mcp_server_names
.iter()
.cloned()
.map(|name| (name, ()))
.collect::<HashMap<_, _>>();
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::<Vec<_>>();
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<Product>,
) -> Result<Arc<ToolSuggestMetadataFragment>, 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<ToolSuggestMetadataEntry> {
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<Product>,
) -> 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::<Vec<_>>();
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),
}))
}
+5 -14
View File
@@ -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
+5 -5
View File
@@ -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()
})
+26 -11
View File
@@ -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<PathBuf> {
@@ -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() {
+47 -24
View File
@@ -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,
+4 -4
View File
@@ -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(
+49 -39
View File
@@ -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<AbsolutePathBuf> {
fn file_paths_for_action(action: &ApplyPatchAction) -> Vec<PathUri> {
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<AbsolutePathBuf> {
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<AbsolutePathBuf>,
cwd: &PathUri,
) -> std::io::Result<(
Vec<PathUri>,
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::<Result<Vec<_>, _>>()?;
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<PathUri>,
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<Session>,
@@ -540,9 +551,7 @@ pub(crate) async fn intercept_apply_patch(
call_id: &str,
tool_name: &str,
) -> Result<Option<FunctionToolOutput>, 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
{
@@ -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,
+3 -1
View File
@@ -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<FunctionToolOutput, Func
}
// Intercept apply_patch if present.
let apply_patch_cwd = PathUri::from_abs_path(&exec_params.cwd);
if let Some(output) = intercept_apply_patch(
&exec_params.command,
&exec_params.cwd,
&apply_patch_cwd,
fs.as_ref(),
turn_environment.clone(),
session.clone(),
@@ -287,20 +287,18 @@ impl ExecCommandHandler {
}
};
// TODO(anp) intercept apply_patch properly when cwd is a foreign path
if let Some(native_cwd) = native_cwd.as_ref()
&& let Some(output) = intercept_apply_patch(
&command,
native_cwd,
fs.as_ref(),
turn_environment.clone(),
context.session.clone(),
context.turn.clone(),
Some(&tracker),
&context.call_id,
"exec_command",
)
.await?
if let Some(output) = intercept_apply_patch(
&command,
&cwd,
fs.as_ref(),
turn_environment.clone(),
context.session.clone(),
context.turn.clone(),
Some(&tracker),
&context.call_id,
"exec_command",
)
.await?
{
manager.release_process_id(process_id).await;
return Ok(boxed_tool_output(ExecCommandToolOutput {
@@ -32,7 +32,6 @@ use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxType;
use codex_sandboxing::SandboxablePreference;
use codex_sandboxing::policy_transforms::effective_permission_profile;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use futures::future::BoxFuture;
use std::path::PathBuf;
@@ -41,14 +40,14 @@ use std::time::Instant;
#[derive(Clone, Debug, Eq, PartialEq, Hash, serde::Serialize)]
pub(crate) struct ApplyPatchApprovalKey {
environment_id: String,
path: AbsolutePathBuf,
path: PathUri,
}
#[derive(Debug)]
pub struct ApplyPatchRequest {
pub turn_environment: TurnEnvironment,
pub action: ApplyPatchAction,
pub file_paths: Vec<AbsolutePathBuf>,
pub file_paths: Vec<PathUri>,
pub changes: std::collections::HashMap<PathBuf, FileChange>,
pub exec_approval_requirement: ExecApprovalRequirement,
pub additional_permissions: Option<AdditionalPermissionProfile>,
@@ -81,10 +80,15 @@ impl ApplyPatchRuntime {
) -> std::io::Result<GuardianApprovalRequest> {
// 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::<std::io::Result<Vec<_>>>()?;
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<ApplyPatchRequest, ApplyPatchRuntimeOutput> 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(),
@@ -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,
+2 -2
View File
@@ -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,
@@ -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
+9
View File
@@ -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,
@@ -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()
};
@@ -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]
@@ -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.
@@ -24,6 +24,9 @@ pub(super) fn server_request_thread_id(request: &ServerRequest) -> Option<Thread
ServerRequest::DynamicToolCall { params, .. } => {
ThreadId::from_string(&params.thread_id).ok()
}
ServerRequest::CurrentTimeRead { params, .. } => {
ThreadId::from_string(&params.thread_id).ok()
}
ServerRequest::ChatgptAuthTokensRefresh { .. }
| ServerRequest::AttestationGenerate { .. }
| ServerRequest::ApplyPatchApproval { .. }
@@ -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,
}
+1
View File
@@ -97,6 +97,7 @@ impl SideParentStatus {
| ServerRequest::ExecCommandApproval { .. } => Some(SideParentStatus::NeedsApproval),
ServerRequest::DynamicToolCall { .. }
| ServerRequest::AttestationGenerate { .. }
| ServerRequest::CurrentTimeRead { .. }
| ServerRequest::ChatgptAuthTokensRefresh { .. } => None,
}
}
@@ -46,6 +46,7 @@ impl ChatWidget {
}
ServerRequest::DynamicToolCall { .. }
| ServerRequest::AttestationGenerate { .. }
| ServerRequest::CurrentTimeRead { .. }
| ServerRequest::ChatgptAuthTokensRefresh { .. }
| ServerRequest::ApplyPatchApproval { .. }
| ServerRequest::ExecCommandApproval { .. } => {
+30 -8
View File
@@ -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<Self> {
@@ -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<AbsolutePathBuf> {
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<Item = &str> {
path.split(move |character| match self {
Self::Posix => character == '/',
Self::Windows => matches!(character, '/' | '\\'),
})
}
}
impl fmt::Display for PathConvention {
+58 -12
View File
@@ -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::<Vec<_>>(), 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 [