codex: address PR review feedback (#28146)

This commit is contained in:
Adam Perry
2026-06-16 16:15:39 +00:00
parent 13c7367a46
commit efb3ceb085
46 changed files with 491 additions and 423 deletions
+3
View File
@@ -250,6 +250,9 @@ impl CommandExecManager {
let (program, args) = command
.split_first()
.ok_or_else(|| invalid_request("command must not be empty"))?;
let cwd = cwd
.to_abs_path()
.map_err(|err| invalid_request(format!("invalid command cwd: {err}")))?;
{
let mut sessions = self.sessions.lock().await;
if sessions.contains_key(&process_key) {
+25 -7
View File
@@ -548,14 +548,32 @@ async fn resolve_turn_environment_selections(
let Some(environments) = environments else {
return Ok(None);
};
let mut selections = Vec::with_capacity(environments.len());
for environment in environments {
let environment_id = environment.environment_id;
let convention = environment
.cwd
.infer_absolute_path_convention()
.ok_or_else(|| {
invalid_request(format!(
"invalid cwd for environment `{environment_id}`: path `{}` does not use absolute POSIX or Windows path syntax",
environment.cwd
))
})?;
let cwd = environment.cwd.to_path_uri(convention).map_err(|err| {
invalid_request(format!(
"invalid cwd for environment `{environment_id}`: {err}"
))
})?;
selections.push(TurnEnvironmentSelection {
environment_id,
cwd,
});
}
thread_manager
.resolve_native_environment_selections(
environments
.into_iter()
.map(|environment| (environment.environment_id, environment.cwd)),
)
.map(Some)
.map_err(environment_selection_error)
.validate_environment_selections(&selections)
.map_err(environment_selection_error)?;
Ok(Some(selections))
}
fn resolve_runtime_workspace_roots(workspace_roots: Vec<AbsolutePathBuf>) -> Vec<AbsolutePathBuf> {
@@ -33,7 +33,6 @@ use codex_login::REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR;
use codex_protocol::config_types::SERVICE_TIER_DEFAULT_REQUEST_VALUE;
use codex_protocol::config_types::TrustLevel;
use codex_protocol::openai_models::ReasoningEffort;
use codex_utils_path_uri::ApiPathString;
use pretty_assertions::assert_eq;
use serde_json::Value;
use serde_json::json;
@@ -294,7 +293,10 @@ async fn thread_start_rejects_unknown_environment_as_invalid_request() -> Result
.send_thread_start_request(ThreadStartParams {
environments: Some(vec![TurnEnvironmentParams {
environment_id: "missing".to_string(),
cwd: ApiPathString::from_abs_path(&codex_home.path().to_path_buf().try_into()?),
cwd: codex_utils_absolute_path::AbsolutePathBuf::try_from(
codex_home.path().to_path_buf(),
)?
.into(),
}]),
..Default::default()
})
@@ -76,7 +76,7 @@ use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS;
use codex_protocol::models::ImageDetail;
use codex_protocol::openai_models::ReasoningEffort;
use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS;
use codex_utils_path_uri::ApiPathString;
use codex_utils_absolute_path::AbsolutePathBuf;
use core_test_support::responses;
use core_test_support::skip_if_no_network;
use pretty_assertions::assert_eq;
@@ -1327,7 +1327,10 @@ async fn turn_start_rejects_unknown_environment_before_starting_turn() -> Result
}],
environments: Some(vec![TurnEnvironmentParams {
environment_id: "missing".to_string(),
cwd: ApiPathString::from_abs_path(&codex_home.path().to_path_buf().try_into()?),
cwd: codex_utils_absolute_path::AbsolutePathBuf::try_from(
codex_home.path().to_path_buf(),
)?
.into(),
}]),
..Default::default()
})
@@ -2745,7 +2748,7 @@ fn environment_params(
.map(|id| {
Ok(TurnEnvironmentParams {
environment_id: (*id).to_string(),
cwd: ApiPathString::from_abs_path(&cwd.to_path_buf().try_into()?),
cwd: AbsolutePathBuf::try_from(cwd.to_path_buf())?.into(),
})
})
.collect()
+6 -1
View File
@@ -53,11 +53,16 @@ pub(crate) async fn load_project_instructions(
let mut loaded = LoadedAgentsMd::from_user_instructions(user_instructions);
for turn_environment in &environments.turn_environments {
let filesystem = turn_environment.environment.get_filesystem();
// TODO(anp): Migrate AGENTS.md discovery to PathUri so instructions can be loaded from
// environment-native foreign working directories.
let Ok(cwd) = turn_environment.cwd().to_abs_path() else {
continue;
};
match read_agents_md(
config,
filesystem.as_ref(),
&turn_environment.environment_id,
turn_environment.cwd(),
&cwd,
)
.await
{
-1
View File
@@ -256,7 +256,6 @@ fn resolved_local_environments<const N: usize>(
PathUri::from_abs_path(&cwd),
/*shell*/ None,
)
.expect("turn environment")
})
.collect(),
}
@@ -44,14 +44,18 @@ impl EnvironmentContextEnvironment {
fn from_turn_environments(environments: &[TurnEnvironment], shell: &Shell) -> Vec<Self> {
environments
.iter()
.map(|environment| Self {
id: environment.environment_id.clone(),
cwd: environment.cwd().clone(),
shell: environment
.shell
.as_ref()
.map(|shell| shell.name().to_string())
.unwrap_or_else(|| shell.name().to_string()),
.filter_map(|environment| {
// TODO(anp): Migrate EnvironmentContextEnvironment to PathUri so foreign
// environments remain visible in model context.
Some(Self {
id: environment.environment_id.clone(),
cwd: environment.cwd().to_abs_path().ok()?,
shell: environment
.shell
.as_ref()
.map(|shell| shell.name().to_string())
.unwrap_or_else(|| shell.name().to_string()),
})
})
.collect()
}
+14 -22
View File
@@ -97,7 +97,7 @@ impl ThreadEnvironments {
}
let turn_environment = match current.turn_environments.iter().find(|environment| {
environment.environment_id == selected_environment.environment_id
&& environment.cwd_uri() == &selected_environment.cwd
&& environment.cwd() == &selected_environment.cwd
}) {
Some(environment) => environment.clone(),
None => match Self::resolve_selection(
@@ -159,13 +159,7 @@ impl ThreadEnvironments {
environment,
selected_environment.cwd.clone(),
shell,
)
.map_err(|err| {
CodexErr::InvalidRequest(format!(
"turn environment cwd `{}` is not valid on this host: {err}",
selected_environment.cwd
))
})?;
);
let task = shell_snapshot
.clone()
.build(turn_environment.clone())
@@ -227,8 +221,10 @@ impl TurnEnvironmentSnapshot {
(!environment.environment.is_remote()).then_some(environment)
}
pub(crate) fn single_local_environment_cwd(&self) -> Option<&AbsolutePathBuf> {
self.single_local_environment().map(TurnEnvironment::cwd)
pub(crate) fn single_local_environment_cwd(&self) -> Option<AbsolutePathBuf> {
// TODO(anp): Migrate local-environment consumers to PathUri so this compatibility
// conversion can be removed.
self.single_local_environment()?.cwd().to_abs_path().ok()
}
}
@@ -571,15 +567,12 @@ url = "ws://127.0.0.1:8765"
.expect("remote environment"),
);
let remote = TurnEnvironmentSnapshot {
turn_environments: vec![
TurnEnvironment::new(
REMOTE_ENVIRONMENT_ID.to_string(),
remote_environment.clone(),
cwd_uri.clone(),
/*shell*/ None,
)
.expect("remote turn environment"),
],
turn_environments: vec![TurnEnvironment::new(
REMOTE_ENVIRONMENT_ID.to_string(),
remote_environment.clone(),
cwd_uri.clone(),
/*shell*/ None,
)],
};
let multiple = TurnEnvironmentSnapshot {
turn_environments: vec![
@@ -589,12 +582,11 @@ url = "ws://127.0.0.1:8765"
remote_environment,
cwd_uri,
/*shell*/ None,
)
.expect("remote turn environment"),
),
],
};
assert_eq!(local.single_local_environment_cwd(), Some(&cwd));
assert_eq!(local.single_local_environment_cwd(), Some(cwd));
assert_eq!(remote.single_local_environment_cwd(), None);
assert_eq!(multiple.single_local_environment_cwd(), None);
}
+1
View File
@@ -461,6 +461,7 @@ pub(crate) async fn execute_exec_request(
windows_sandbox_filesystem_overrides,
arg0,
} = exec_request;
let cwd = cwd.to_abs_path()?;
let params = ExecParams {
command,
+3 -1
View File
@@ -750,11 +750,13 @@ async fn run_review_on_session(
.unwrap_or_default();
let guardian_permission_profile = PermissionProfile::read_only();
let parent_turn_environments = params.parent_turn.environments.to_selections();
// TODO(anp): Migrate guardian review thread settings to a PathUri fallback cwd so foreign
// parent environments do not fall back to the host-native config cwd.
let parent_turn_legacy_fallback_cwd = params
.parent_turn
.environments
.primary()
.map(|environment| environment.cwd().clone())
.and_then(|environment| environment.cwd().to_abs_path().ok())
.unwrap_or_else(|| params.parent_turn.config.cwd.clone());
let submit_result = run_before_review_deadline(
+4 -2
View File
@@ -25,6 +25,7 @@ use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_sandboxing::SandboxExecRequest;
use codex_sandboxing::SandboxType;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use std::collections::HashMap;
#[derive(Debug)]
@@ -42,7 +43,7 @@ pub(crate) struct ExecServerEnvConfig {
#[derive(Debug)]
pub struct ExecRequest {
pub command: Vec<String>,
pub cwd: AbsolutePathBuf,
pub cwd: PathUri,
pub env: HashMap<String, String>,
pub(crate) exec_server_env_config: Option<ExecServerEnvConfig>,
pub network: Option<NetworkProxy>,
@@ -77,6 +78,7 @@ impl ExecRequest {
arg0: Option<String>,
) -> Self {
let windows_sandbox_policy_cwd = cwd.clone();
let cwd = PathUri::from_abs_path(&cwd);
let (file_system_sandbox_policy, network_sandbox_policy) =
permission_profile.to_runtime_permissions();
Self {
@@ -135,7 +137,7 @@ impl ExecRequest {
}
Self {
command,
cwd,
cwd: PathUri::from_abs_path(&cwd),
env,
exec_server_env_config: None,
network,
+11 -10
View File
@@ -318,17 +318,18 @@ impl Session {
)
.await;
let environment_manager = self.services.turn_environments.environment_manager();
let mcp_runtime_context = match turn_context.environments.primary() {
Some(turn_environment) => McpRuntimeContext::new(
Arc::clone(&environment_manager),
turn_environment.cwd().to_path_buf(),
),
None => McpRuntimeContext::new(
environment_manager,
// TODO(anp): Migrate MCP runtime cwd plumbing to PathUri so foreign environment cwd
// values can be used without falling back to the legacy host cwd.
let cwd = turn_context
.environments
.primary()
.and_then(|turn_environment| turn_environment.cwd().to_abs_path().ok())
.map(|cwd| cwd.to_path_buf())
.unwrap_or_else(|| {
#[allow(deprecated)]
turn_context.cwd.to_path_buf(),
),
};
turn_context.cwd.to_path_buf()
});
let mcp_runtime_context = McpRuntimeContext::new(environment_manager, cwd);
let mcp_startup_cancellation_token = {
let mut guard = self.services.mcp_startup_cancellation_token.lock().await;
guard.cancel();
+4 -1
View File
@@ -1119,9 +1119,12 @@ impl Session {
};
let mcp_runtime_context = {
let turn_environments = sess.services.turn_environments.snapshot().await;
// TODO(anp): Migrate MCP runtime cwd plumbing to PathUri so foreign environment
// cwd values can be used without falling back to the session host cwd.
let cwd = turn_environments
.primary()
.map(|turn_environment| turn_environment.cwd().to_path_buf())
.and_then(|turn_environment| turn_environment.cwd().to_abs_path().ok())
.map(|cwd| cwd.to_path_buf())
.unwrap_or_else(|| session_configuration.cwd().to_path_buf());
McpRuntimeContext::new(
sess.services.turn_environments.environment_manager(),
+16 -13
View File
@@ -979,8 +979,11 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an
&'a mut self,
_req: &'a (),
_ctx: crate::tools::sandboxing::ApprovalCtx<'a>,
) -> futures::future::BoxFuture<'a, ReviewDecision> {
Box::pin(async { ReviewDecision::Approved })
) -> futures::future::BoxFuture<
'a,
Result<ReviewDecision, crate::tools::sandboxing::ToolError>,
> {
Box::pin(async { Ok(ReviewDecision::Approved) })
}
}
@@ -5697,10 +5700,9 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir
turn_context_mut.environments.turn_environments[0] = TurnEnvironment::new(
"remote".to_string(),
current_environment.environment,
codex_utils_path_uri::PathUri::from_abs_path(&environment_cwd),
PathUri::from_abs_path(&environment_cwd),
current_environment.shell,
)
.expect("remote turn environment");
);
let call_id = "call-1".to_string();
let handler = RequestPermissionsHandler;
@@ -6317,15 +6319,16 @@ async fn primary_environment_uses_first_turn_environment() {
let first_environment = turn_context.environments.turn_environments[0].clone();
#[allow(deprecated)]
let second_cwd = turn_context.cwd.join("second");
turn_context.environments.turn_environments.push(
TurnEnvironment::new(
let second_cwd_uri = codex_utils_path_uri::PathUri::from_abs_path(&second_cwd);
turn_context
.environments
.turn_environments
.push(TurnEnvironment::new(
"second".to_string(),
Arc::clone(&first_environment.environment),
codex_utils_path_uri::PathUri::from_abs_path(&second_cwd),
second_cwd_uri.clone(),
/*shell*/ None,
)
.expect("second turn environment"),
);
));
assert_eq!(
turn_context
@@ -6343,12 +6346,12 @@ async fn primary_environment_uses_first_turn_environment() {
.find(|environment| environment.environment_id == "second")
.expect("second environment")
.cwd(),
&second_cwd
&second_cwd_uri
);
assert_eq!(turn_context.environments.turn_environments.len(), 2);
assert_eq!(
turn_context.environments.turn_environments[1].cwd(),
&second_cwd
&second_cwd_uri
);
}
+18 -11
View File
@@ -414,13 +414,16 @@ pub(crate) async fn run_turn(
async fn turn_diff_display_roots(turn_context: &TurnContext) -> Vec<(String, PathBuf)> {
let mut display_roots = Vec::new();
for turn_environment in &turn_context.environments.turn_environments {
let root = get_git_repo_root_with_fs(
turn_environment.environment.get_filesystem().as_ref(),
turn_environment.cwd(),
)
.await
.unwrap_or_else(|| turn_environment.cwd().clone())
.into_path_buf();
// TODO(anp): Migrate git-root discovery and diff display roots to PathUri so foreign
// environment roots can participate without host-native conversion.
let Ok(cwd) = turn_environment.cwd().to_abs_path() else {
continue;
};
let root =
get_git_repo_root_with_fs(turn_environment.environment.get_filesystem().as_ref(), &cwd)
.await
.unwrap_or(cwd)
.into_path_buf();
display_roots.push((turn_environment.environment_id.clone(), root));
}
display_roots
@@ -634,10 +637,14 @@ async fn build_extension_turn_input_items(
.turn_environments
.iter()
.enumerate()
.map(|(index, environment)| TurnInputEnvironment {
environment_id: environment.environment_id.clone(),
cwd: environment.cwd().as_path().to_path_buf(),
is_primary: index == 0,
.filter_map(|(index, environment)| {
// TODO(anp): Migrate extension turn-input environments to PathUri so foreign cwd
// values are not omitted from extension context.
Some(TurnInputEnvironment {
environment_id: environment.environment_id.clone(),
cwd: environment.cwd().to_abs_path().ok()?.into_path_buf(),
is_primary: index == 0,
})
})
.collect::<Vec<_>>();
+12 -24
View File
@@ -3,7 +3,6 @@ use crate::SkillLoadOutcome;
use crate::agents_md::LoadedAgentsMd;
use crate::config::GhostSnapshotConfig;
use crate::environment_selection::TurnEnvironmentSnapshot;
use crate::path_utils;
use crate::shell_snapshot::ShellSnapshotFile;
use codex_core_skills::HostLoadedSkills;
use codex_file_system::FileSystemSandboxContext;
@@ -49,13 +48,7 @@ pub(crate) type ShellSnapshotTask = Shared<BoxFuture<'static, Option<Arc<ShellSn
pub(crate) struct TurnEnvironment {
pub(crate) environment_id: String,
pub(crate) environment: Arc<Environment>,
// Keep both representations together while cwd consumers migrate to URI semantics. Keeping
// them synchronized means neither representation can be exposed through a mutable reference;
// updates must rebuild the validated pair through `TurnEnvironment::new`. Once
// `TurnEnvironment::cwd` itself becomes a `PathUri`, convert only at native filesystem and
// process-launch boundaries and remove this paired migration state.
cwd: AbsolutePathBuf,
cwd_uri: PathUri,
cwd: PathUri,
pub(crate) shell: Option<shell::Shell>,
pub(crate) shell_snapshot: ShellSnapshotTask,
}
@@ -64,22 +57,20 @@ impl TurnEnvironment {
pub(crate) fn new(
environment_id: String,
environment: Arc<Environment>,
cwd_uri: PathUri,
cwd: PathUri,
shell: Option<shell::Shell>,
) -> std::io::Result<Self> {
let cwd = cwd_uri.to_abs_path()?;
Ok(Self {
) -> Self {
Self {
environment_id,
environment,
cwd,
cwd_uri,
shell,
shell_snapshot: futures::future::ready(None).boxed().shared(),
})
}
}
pub(crate) fn shell_snapshot(&self, cwd: &AbsolutePathBuf) -> Option<AbsolutePathBuf> {
if !path_utils::paths_match_after_normalization(self.cwd.as_path(), cwd.as_path()) {
pub(crate) fn shell_snapshot(&self, cwd: &PathUri) -> Option<AbsolutePathBuf> {
if &self.cwd != cwd {
return None;
}
self.shell_snapshot
@@ -88,18 +79,14 @@ impl TurnEnvironment {
.map(ShellSnapshotFile::path)
}
pub(crate) fn cwd(&self) -> &AbsolutePathBuf {
pub(crate) fn cwd(&self) -> &PathUri {
&self.cwd
}
pub(crate) fn cwd_uri(&self) -> &PathUri {
&self.cwd_uri
}
pub(crate) fn selection(&self) -> TurnEnvironmentSelection {
TurnEnvironmentSelection {
environment_id: self.environment_id.clone(),
cwd: self.cwd_uri.clone(),
cwd: self.cwd.clone(),
}
}
}
@@ -110,7 +97,6 @@ impl std::fmt::Debug for TurnEnvironment {
.field("environment_id", &self.environment_id)
.field("environment", &self.environment)
.field("cwd", &self.cwd)
.field("cwd_uri", &self.cwd_uri)
.field("shell", &self.shell)
.finish_non_exhaustive()
}
@@ -755,9 +741,11 @@ impl Session {
) -> Arc<TurnContext> {
let turn_environments = self.services.turn_environments.snapshot().await;
let primary_turn_environment = turn_environments.primary().cloned();
// TODO(anp): Migrate per-turn config and legacy TurnContext cwd consumers to PathUri so
// a foreign primary environment does not fall back to the session's host cwd.
let cwd = primary_turn_environment
.as_ref()
.map(|turn_environment| turn_environment.cwd().clone())
.and_then(|turn_environment| turn_environment.cwd().to_abs_path().ok())
.unwrap_or_else(|| session_configuration.cwd().clone());
let per_turn_config = Self::build_per_turn_config(&session_configuration, cwd.clone());
{
+3 -1
View File
@@ -76,7 +76,9 @@ impl ShellSnapshot {
}
let shell = environment.shell.clone()?;
let cwd = environment.cwd().clone();
// TODO(anp): Migrate shell snapshot creation to accept PathUri and defer native
// conversion to the spawned shell process.
let cwd = environment.cwd().to_abs_path().ok()?;
Self::build_for_cwd(Arc::clone(config), cwd, shell).await
}
+12 -2
View File
@@ -162,7 +162,17 @@ pub(crate) async fn execute_user_shell_command(
let call_id = Uuid::new_v4().to_string();
let raw_command = command;
let cwd = turn_environment.cwd().clone();
// TODO(anp): Migrate user-shell events and execution plumbing to PathUri so this local-only
// feature does not need to project the selected environment cwd onto the Codex host.
let Ok(cwd) = turn_environment.cwd().to_abs_path() else {
send_user_shell_error(
&session,
turn_context.as_ref(),
"shell working directory is not native to the Codex host",
)
.await;
return;
};
let parsed_cmd = parse_command(&display_command);
session
@@ -185,7 +195,7 @@ pub(crate) async fn execute_user_shell_command(
let permission_profile = PermissionProfile::Disabled;
let exec_env = ExecRequest {
command: exec_command.clone(),
cwd: cwd.clone(),
cwd: turn_environment.cwd().clone(),
env: exec_env_map,
exec_server_env_config: None,
// `/shell` is the explicit full-access escape hatch, so it must not
-28
View File
@@ -66,7 +66,6 @@ use codex_thread_store::ThreadStore;
use codex_thread_store::ThreadStoreError;
use codex_thread_store::UpdateThreadMetadataParams;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::ApiPathString;
use futures::StreamExt;
use futures::stream::FuturesUnordered;
use std::collections::HashMap;
@@ -473,33 +472,6 @@ impl ThreadManager {
Ok(())
}
/// Resolves environment-native cwd strings without applying the Codex host's path rules.
pub fn resolve_native_environment_selections(
&self,
environments: impl IntoIterator<Item = (String, ApiPathString)>,
) -> CodexResult<Vec<TurnEnvironmentSelection>> {
let environments = environments.into_iter();
let mut selections = Vec::with_capacity(environments.size_hint().0);
for (environment_id, cwd) in environments {
let convention = cwd.infer_absolute_path_convention().ok_or_else(|| {
CodexErr::InvalidRequest(format!(
"invalid cwd for environment `{environment_id}`: path `{cwd}` does not use absolute POSIX or Windows path syntax"
))
})?;
let cwd = cwd.to_path_uri(convention).map_err(|err| {
CodexErr::InvalidRequest(format!(
"invalid cwd for environment `{environment_id}`: {err}"
))
})?;
selections.push(TurnEnvironmentSelection {
environment_id,
cwd,
});
}
self.validate_environment_selections(&selections)?;
Ok(selections)
}
pub fn get_models_manager(&self) -> SharedModelsManager {
self.state.models_manager.clone()
}
+4 -125
View File
@@ -23,7 +23,6 @@ use codex_protocol::protocol::SessionSource;
use codex_protocol::protocol::ThreadSource;
use codex_protocol::protocol::TurnStartedEvent;
use codex_protocol::protocol::UserMessageEvent;
use codex_utils_path_uri::ApiPathString;
use codex_utils_path_uri::PathUri;
use core_test_support::PathBufExt;
use core_test_support::PathExt;
@@ -299,126 +298,6 @@ async fn shutdown_all_threads_bounded_submits_shutdown_to_every_thread() {
assert!(manager.list_thread_ids().await.is_empty());
}
#[tokio::test]
async fn start_thread_rejects_explicit_local_environment_when_default_provider_is_disabled() {
let temp_dir = tempdir().expect("tempdir");
let mut config = test_config().await;
config.codex_home = temp_dir.path().join("codex-home").abs();
config.cwd = config.codex_home.abs();
std::fs::create_dir_all(&config.codex_home).expect("create codex home");
let runtime_paths = codex_exec_server::ExecServerRuntimePaths::new(
std::env::current_exe().expect("current exe path"),
/*codex_linux_sandbox_exe*/ None,
)
.expect("runtime paths");
let environment_manager = Arc::new(
codex_exec_server::EnvironmentManager::create_for_tests(
Some("none".to_string()),
Some(runtime_paths),
)
.await,
);
let manager = ThreadManager::with_models_provider_and_home_for_tests(
CodexAuth::from_api_key("dummy"),
config.model_provider.clone(),
config.codex_home.to_path_buf(),
environment_manager,
);
let result = manager
.start_thread_with_options(StartThreadOptions {
config: config.clone(),
initial_history: InitialHistory::New,
session_source: None,
thread_source: None,
dynamic_tools: Vec::new(),
metrics_service_name: None,
parent_trace: None,
environments: vec![TurnEnvironmentSelection {
environment_id: "local".to_string(),
cwd: PathUri::from_abs_path(&config.cwd),
}],
thread_extension_init: Default::default(),
})
.await;
let err = match result {
Ok(_) => panic!("explicit local environment should not resolve when provider is disabled"),
Err(err) => err,
};
assert_eq!(err.to_string(), "unknown turn environment id `local`");
assert!(manager.list_thread_ids().await.is_empty());
}
#[tokio::test]
async fn native_environment_selections_preserve_selected_path_syntax() {
let temp_dir = tempdir().expect("tempdir");
let mut config = test_config().await;
config.codex_home = temp_dir.path().join("codex-home").abs();
config.cwd = temp_dir.path().join("workspace").abs();
std::fs::create_dir_all(&config.codex_home).expect("create codex home");
std::fs::create_dir_all(&config.cwd).expect("create workspace");
let manager = ThreadManager::with_models_provider_and_home_for_tests(
CodexAuth::from_api_key("dummy"),
config.model_provider.clone(),
config.codex_home.to_path_buf(),
Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()),
);
let selections = manager
.resolve_native_environment_selections([(
"local".to_string(),
ApiPathString::from_abs_path(&config.cwd),
)])
.expect("resolve local selection");
assert_eq!(
selections,
vec![TurnEnvironmentSelection {
environment_id: "local".to_string(),
cwd: PathUri::from_abs_path(&config.cwd),
}]
);
}
#[tokio::test]
async fn native_environment_selections_reject_duplicate_ids() {
let temp_dir = tempdir().expect("tempdir");
let mut config = test_config().await;
config.codex_home = temp_dir.path().join("codex-home").abs();
config.cwd = temp_dir.path().join("workspace").abs();
std::fs::create_dir_all(&config.codex_home).expect("create codex home");
let manager = ThreadManager::with_models_provider_and_home_for_tests(
CodexAuth::from_api_key("dummy"),
config.model_provider.clone(),
config.codex_home.to_path_buf(),
Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()),
);
let native_cwd = ApiPathString::from_abs_path(&config.cwd);
let error = manager
.resolve_native_environment_selections([
("local".to_string(), native_cwd.clone()),
("local".to_string(), native_cwd),
])
.expect_err("duplicate ids must fail");
assert_eq!(error.to_string(), "duplicate turn environment id `local`");
let error = manager
.resolve_native_environment_selections([(
"local".to_string(),
serde_json::from_value(serde_json::json!("relative"))
.expect("API path should deserialize"),
)])
.expect_err("relative cwd must fail at the environment boundary");
assert_eq!(
error.to_string(),
"invalid cwd for environment `local`: path `relative` does not use absolute POSIX or Windows path syntax"
);
}
#[tokio::test]
async fn start_thread_keeps_internal_threads_hidden_from_normal_lookups() {
let temp_dir = tempdir().expect("tempdir");
@@ -727,11 +606,11 @@ async fn resume_and_fork_do_not_restore_thread_environments_from_rollout() {
assert_eq!(resumed_turn.environments.turn_environments.len(), 1);
assert_eq!(
resumed_turn.environments.turn_environments[0].cwd(),
&default_cwd
&PathUri::from_abs_path(&default_cwd)
);
assert_ne!(
resumed_turn.environments.turn_environments[0].cwd(),
&selected_cwd
&PathUri::from_abs_path(&selected_cwd)
);
let forked = manager
@@ -754,11 +633,11 @@ async fn resume_and_fork_do_not_restore_thread_environments_from_rollout() {
assert_eq!(forked_turn.environments.turn_environments.len(), 1);
assert_eq!(
forked_turn.environments.turn_environments[0].cwd(),
&default_cwd
&PathUri::from_abs_path(&default_cwd)
);
assert_ne!(
forked_turn.environments.turn_environments[0].cwd(),
&selected_cwd
&PathUri::from_abs_path(&selected_cwd)
);
}
@@ -299,7 +299,7 @@ pub async fn handle(
Ok(FunctionToolOutput::from_text(content, Some(true)))
}
fn single_local_environment_cwd(turn: &TurnContext) -> Result<&AbsolutePathBuf, FunctionCallError> {
fn single_local_environment_cwd(turn: &TurnContext) -> Result<AbsolutePathBuf, FunctionCallError> {
let [turn_environment] = turn.environments.turn_environments.as_slice() else {
return Err(FunctionCallError::RespondToModel(
"spawn_agents_on_csv requires exactly one local environment".to_string(),
@@ -312,5 +312,12 @@ fn single_local_environment_cwd(turn: &TurnContext) -> Result<&AbsolutePathBuf,
));
}
Ok(turn_environment.cwd())
// TODO(anp): Migrate spawn_agents_on_csv filesystem access to PathUri before enabling it for
// remote environments.
turn_environment.cwd().to_abs_path().map_err(|err| {
FunctionCallError::RespondToModel(format!(
"spawn_agents_on_csv cwd `{}` is not native to the Codex host: {err}",
turn_environment.cwd()
))
})
}
@@ -359,11 +359,18 @@ impl ApplyPatchHandler {
"apply_patch is unavailable in this session".to_string(),
));
};
let cwd = turn_environment.cwd().clone();
// 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_uri(),
turn_environment.cwd(),
);
match codex_apply_patch::verify_apply_patch_args(args, &cwd, fs.as_ref(), Some(&sandbox))
.await
@@ -114,10 +114,15 @@ async fn to_extension_call(invocation: &ToolInvocation) -> ExtensionToolCall {
ConversationHistory::new(invocation.session.clone_history().await.into_raw_items());
let mut environments = Vec::with_capacity(invocation.turn.environments.turn_environments.len());
for environment in &invocation.turn.environments.turn_environments {
// TODO(anp): Migrate extension ToolEnvironment and granted-permission lookup to PathUri
// so extensions can receive foreign environment cwd values.
let Ok(native_cwd) = environment.cwd().to_abs_path() else {
continue;
};
let additional_permissions = apply_granted_turn_permissions(
invocation.session.as_ref(),
&environment.environment_id,
environment.cwd().as_path(),
native_cwd.as_path(),
SandboxPermissions::UseDefault,
/*additional_permissions*/ None,
)
@@ -125,10 +130,10 @@ async fn to_extension_call(invocation: &ToolInvocation) -> ExtensionToolCall {
.additional_permissions;
let file_system_sandbox_context = invocation
.turn
.file_system_sandbox_context(additional_permissions, environment.cwd_uri());
.file_system_sandbox_context(additional_permissions, environment.cwd());
environments.push(ToolEnvironment {
environment_id: environment.environment_id.clone(),
cwd: environment.cwd().clone(),
cwd: native_cwd,
file_system: environment.environment.get_filesystem(),
file_system_sandbox_context,
});
@@ -315,7 +320,7 @@ mod tests {
.environments
.turn_environments
.iter()
.map(|environment| Some(environment.cwd_uri().clone()))
.map(|environment| Some(environment.cwd().clone()))
.collect::<Vec<_>>();
let history_item = ResponseItem::Message {
id: None,
@@ -70,8 +70,16 @@ impl RequestPermissionsHandler {
"request_permissions requires a primary environment".to_string(),
));
};
// TODO(anp): Migrate request_permissions parsing and permission profiles to PathUri so
// environment-native foreign paths do not require host conversion.
let native_cwd = turn_environment.cwd().to_abs_path().map_err(|err| {
FunctionCallError::RespondToModel(format!(
"request_permissions cwd `{}` is not native to the Codex host: {err}",
turn_environment.cwd()
))
})?;
let mut args: RequestPermissionsArgs =
parse_arguments_with_base_path(&arguments, turn_environment.cwd())?;
parse_arguments_with_base_path(&arguments, &native_cwd)?;
args.permissions = normalize_additional_permissions(args.permissions.into())
.map(codex_protocol::request_permissions::RequestPermissionProfile::from)
.map_err(FunctionCallError::RespondToModel)?;
@@ -10,6 +10,7 @@ use crate::tools::registry::PostToolUsePayload;
use codex_exec_server::Environment;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_tools::UnifiedExecShellMode;
use codex_utils_path_uri::ApiPathString;
use serde::Deserialize;
use std::path::PathBuf;
use std::sync::Arc;
@@ -54,7 +55,7 @@ struct ExecCommandEnvironmentArgs {
// Keep this raw until after environment selection; relative paths must be
// resolved against the selected environment cwd, not the process cwd.
#[serde(default)]
workdir: Option<String>,
workdir: Option<ApiPathString>,
}
fn default_exec_yield_time_ms() -> u64 {
@@ -11,7 +11,6 @@ use crate::tools::handlers::apply_patch::intercept_apply_patch;
use crate::tools::handlers::implicit_granted_permissions;
use crate::tools::handlers::normalize_and_validate_additional_permissions;
use crate::tools::handlers::parse_arguments;
use crate::tools::handlers::parse_arguments_with_base_path;
use crate::tools::handlers::resolve_tool_environment;
use crate::tools::handlers::rewrite_function_string_argument;
use crate::tools::handlers::updated_hook_command;
@@ -131,21 +130,26 @@ impl ExecCommandHandler {
};
let cwd = environment_args
.workdir
.as_deref()
.as_ref()
.filter(|workdir| !workdir.is_empty())
.map_or_else(
|| turn_environment.cwd().clone(),
|workdir| turn_environment.cwd().join(workdir),
);
.and_then(|workdir| turn_environment.cwd().join(workdir.as_str()).ok())
.unwrap_or_else(|| turn_environment.cwd().clone());
// TODO(anp): Migrate command approvals, granted permissions, and tool hooks to PathUri so
// foreign command working directories do not require a host-native projection here.
let native_cwd = cwd.to_abs_path().map_err(|err| {
FunctionCallError::RespondToModel(format!(
"command workdir `{cwd}` is not native to the Codex host: {err}"
))
})?;
let environment = Arc::clone(&turn_environment.environment);
let fs = environment.get_filesystem();
let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?;
let args: ExecCommandArgs = parse_arguments(&arguments)?;
let hook_command = args.cmd.clone();
maybe_emit_implicit_skill_invocation(
session.as_ref(),
context.turn.as_ref(),
&hook_command,
&cwd,
&native_cwd,
)
.await;
let process_id = manager.allocate_process_id().await;
@@ -186,7 +190,7 @@ impl ExecCommandHandler {
let effective_additional_permissions = apply_granted_turn_permissions(
context.session.as_ref(),
&turn_environment.environment_id,
cwd.as_path(),
native_cwd.as_path(),
sandbox_permissions,
additional_permissions,
)
@@ -226,7 +230,7 @@ impl ExecCommandHandler {
effective_additional_permissions.sandbox_permissions,
effective_additional_permissions.additional_permissions,
effective_additional_permissions.permissions_preapproved,
&cwd,
native_cwd.as_path(),
)
},
|permissions| Ok(Some(permissions)),
@@ -240,7 +244,7 @@ impl ExecCommandHandler {
if let Some(output) = intercept_apply_patch(
&command,
&cwd,
&native_cwd,
fs.as_ref(),
turn_environment.clone(),
context.session.clone(),
@@ -277,7 +281,14 @@ impl ExecCommandHandler {
yield_time_ms,
max_output_tokens,
cwd,
sandbox_cwd: turn_environment.cwd().clone(),
// TODO(anp): Migrate the sandbox orchestration traits to PathUri so the
// selected environment root can remain foreign until sandbox enforcement.
sandbox_cwd: turn_environment.cwd().to_abs_path().map_err(|err| {
FunctionCallError::RespondToModel(format!(
"environment cwd `{}` is not native to the Codex host: {err}",
turn_environment.cwd()
))
})?,
turn_environment: turn_environment.clone(),
shell_mode,
network: context.turn.network.clone(),
+35 -18
View File
@@ -11,6 +11,7 @@ use codex_protocol::openai_models::InputModality;
use codex_utils_image::PromptImageMode;
use codex_utils_image::data_url_from_bytes;
use codex_utils_image::load_for_prompt_bytes;
use codex_utils_path_uri::ApiPathString;
use serde::Deserialize;
use crate::function_tool::FunctionCallError;
@@ -27,7 +28,6 @@ use crate::tools::registry::CoreToolRuntime;
use crate::tools::registry::ToolExecutor;
use codex_tools::ToolName;
use codex_tools::ToolSpec;
use codex_utils_path_uri::PathUri;
pub struct ViewImageHandler {
options: ViewImageToolOptions,
@@ -55,7 +55,7 @@ const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str =
#[derive(Deserialize)]
struct ViewImageArgs {
path: String,
path: ApiPathString,
#[serde(default)]
environment_id: Option<String>,
detail: Option<String>,
@@ -143,29 +143,40 @@ impl ViewImageHandler {
"view_image is unavailable in this session".to_string(),
));
};
let cwd = turn_environment.cwd().clone();
let abs_path = cwd.join(path);
// Image paths may be absolute using the selected environment's native syntax or relative
// to its cwd. Resolve either form to a PathUri without parsing it as a Codex-host path.
let path_uri = match path.infer_absolute_path_convention() {
Some(convention) => path.to_path_uri(convention).map_err(|err| err.to_string()),
None => turn_environment
.cwd()
.join(path.as_str())
.map_err(|err| err.to_string()),
}
.map_err(|err| {
FunctionCallError::RespondToModel(format!("invalid image path `{path}`: {err}"))
})?;
// Preserve the existing native error spelling for local paths while still rendering a
// meaningful URI when the selected environment path is foreign to the Codex host.
let display_path = path_uri
.to_abs_path()
.map_or_else(|_| path_uri.to_string(), |path| path.display().to_string());
let sandbox = turn.file_system_sandbox_context(
/*additional_permissions*/ None,
turn_environment.cwd_uri(),
turn_environment.cwd(),
);
let fs = turn_environment.environment.get_filesystem();
let path_uri = PathUri::from_abs_path(&abs_path);
let metadata = fs
.get_metadata(&path_uri, Some(&sandbox))
.await
.map_err(|error| {
FunctionCallError::RespondToModel(format!(
"unable to locate image at `{}`: {error}",
abs_path.display()
"unable to locate image at `{display_path}`: {error}"
))
})?;
if !metadata.is_file {
return Err(FunctionCallError::RespondToModel(format!(
"image path `{}` is not a file",
abs_path.display()
"image path `{display_path}` is not a file"
)));
}
let file_bytes = fs
@@ -173,11 +184,17 @@ impl ViewImageHandler {
.await
.map_err(|error| {
FunctionCallError::RespondToModel(format!(
"unable to read image at `{}`: {error}",
abs_path.display()
"unable to read image at `{display_path}`: {error}"
))
})?;
let event_path = abs_path.clone();
// TODO(anp): Migrate image-view events and image decoding hints to PathUri so foreign
// environment paths do not require a host-native projection after remote file reads.
let native_path = path_uri.to_abs_path().map_err(|err| {
FunctionCallError::RespondToModel(format!(
"image path `{path_uri}` is not native to the Codex host: {err}"
))
})?;
let event_path = native_path.clone();
let can_request_original_detail = can_request_original_image_detail(&turn.model_info);
let use_original_detail =
@@ -197,11 +214,11 @@ impl ViewImageHandler {
} else {
PromptImageMode::ResizeToFit
};
load_for_prompt_bytes(abs_path.as_path(), file_bytes, image_mode)
load_for_prompt_bytes(native_path.as_path(), file_bytes, image_mode)
.map_err(|error| {
FunctionCallError::RespondToModel(format!(
"unable to process image at `{}`: {error}",
abs_path.display()
native_path.display()
))
})?
.into_data_url()
@@ -272,6 +289,7 @@ mod tests {
use crate::turn_diff_tracker::TurnDiffTracker;
use codex_protocol::models::PermissionProfile;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use core_test_support::TempDirExt;
use pretty_assertions::assert_eq;
use serde_json::json;
@@ -290,8 +308,7 @@ mod tests {
current.environment,
PathUri::from_abs_path(&cwd),
current.shell,
)
.expect("turn environment");
);
}
#[test]
+1 -1
View File
@@ -537,7 +537,7 @@ impl ToolOrchestrator {
} else {
ToolDecisionSource::User
};
let decision = tool.start_approval_async(req, approval_ctx).await;
let decision = tool.start_approval_async(req, approval_ctx).await?;
let tool_name = flat_tool_name(&tool_ctx.tool_name);
otel.tool_decision(
tool_name.as_ref(),
@@ -133,7 +133,7 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
&'a mut self,
req: &'a ApplyPatchRequest,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision> {
) -> BoxFuture<'a, Result<ReviewDecision, ToolError>> {
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
@@ -144,11 +144,12 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
Box::pin(async move {
if let Some(review_id) = guardian_review_id {
let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id);
return review_approval_request(session, turn, review_id, action, retry_reason)
.await;
return Ok(
review_approval_request(session, turn, review_id, action, retry_reason).await,
);
}
if req.permissions_preapproved && retry_reason.is_none() {
return ReviewDecision::Approved;
return Ok(ReviewDecision::Approved);
}
if let Some(reason) = retry_reason {
let rx_approve = session
@@ -160,10 +161,10 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
/*grant_root*/ None,
)
.await;
return rx_approve.await.unwrap_or_default();
return Ok(rx_approve.await.unwrap_or_default());
}
with_cached_approval(
Ok(with_cached_approval(
&session.services,
"apply_patch",
approval_keys,
@@ -176,7 +177,7 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
rx_approve.await.unwrap_or_default()
},
)
.await
.await)
})
}
@@ -22,7 +22,6 @@ fn test_turn_environment(environment_id: &str) -> crate::session::turn_context::
PathUri::from_abs_path(&std::env::temp_dir().abs()),
/*shell*/ None,
)
.expect("turn environment")
}
#[test]
+4 -5
View File
@@ -33,22 +33,21 @@ pub(crate) mod apply_patch;
pub(crate) mod shell;
pub(crate) mod unified_exec;
/// Shared helper to construct sandbox transform inputs from a tokenized command line and native
/// working directory. Validates that at least a program is present.
/// Shared helper to construct sandbox transform inputs from a tokenized command line and working
/// directory. Validates that at least a program is present.
pub(crate) fn build_sandbox_command(
command: &[String],
cwd: &AbsolutePathBuf,
cwd: &PathUri,
env: &HashMap<String, String>,
additional_permissions: Option<AdditionalPermissionProfile>,
) -> Result<SandboxCommand, ToolError> {
let (program, args) = command
.split_first()
.ok_or_else(|| ToolError::Rejected("command args are empty".to_string()))?;
let cwd = PathUri::from_abs_path(cwd);
Ok(SandboxCommand {
program: program.clone().into(),
args: args.to_vec(),
cwd,
cwd: cwd.clone(),
env: env.clone(),
additional_permissions,
})
@@ -91,7 +91,7 @@ async fn explicit_escalation_prepares_exec_without_managed_network() -> anyhow::
let command = vec!["/bin/echo".to_string(), "ok".to_string()];
let command = build_sandbox_command(
&command,
&command_cwd,
&PathUri::from_abs_path(&command_cwd),
&exec_env_for_sandbox_permissions(&env, SandboxPermissions::RequireEscalated),
/*additional_permissions*/ None,
)
@@ -129,7 +129,7 @@ async fn explicit_escalation_prepares_exec_without_managed_network() -> anyhow::
)
.expect("prepare exec request");
assert_eq!(exec_request.cwd, command_cwd);
assert_eq!(exec_request.cwd, PathUri::from_abs_path(&command_cwd));
assert_eq!(
exec_request.windows_sandbox_policy_cwd,
native_sandbox_policy_cwd
+35 -26
View File
@@ -47,6 +47,7 @@ use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxablePreference;
use codex_shell_command::powershell::prefix_powershell_script_with_utf8;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use futures::future::BoxFuture;
use std::collections::HashMap;
use tokio_util::sync::CancellationToken;
@@ -138,7 +139,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
&'a mut self,
req: &'a ShellRequest,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision> {
) -> BoxFuture<'a, Result<ReviewDecision, ToolError>> {
let keys = self.approval_keys(req);
let command = req.command.clone();
let cwd = req.cwd.clone();
@@ -150,7 +151,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
let guardian_review_id = ctx.guardian_review_id.clone();
Box::pin(async move {
if let Some(review_id) = guardian_review_id {
return review_approval_request(
return Ok(review_approval_request(
session,
turn,
review_id,
@@ -164,28 +165,30 @@ impl Approvable<ShellRequest> for ShellRuntime {
},
retry_reason,
)
.await;
.await);
}
with_cached_approval(&session.services, "shell", keys, move || async move {
let available_decisions = None;
session
.request_command_approval(
turn,
call_id,
/*approval_id*/ None,
command,
cwd,
reason,
ctx.network_approval_context.clone(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
req.additional_permissions.clone(),
available_decisions,
)
.await
})
.await
Ok(
with_cached_approval(&session.services, "shell", keys, move || async move {
let available_decisions = None;
session
.request_command_approval(
turn,
call_id,
/*approval_id*/ None,
command,
cwd,
reason,
ctx.network_approval_context.clone(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
req.additional_permissions.clone(),
available_decisions,
)
.await
})
.await,
)
})
}
@@ -247,7 +250,9 @@ impl ToolRuntime<ShellRequest, ExecToolCallOutput> for ShellRuntime {
.shell
.as_ref()
.unwrap_or(session_shell.as_ref());
let shell_snapshot_location = req.turn_environment.shell_snapshot(&req.cwd);
let shell_snapshot_location = req
.turn_environment
.shell_snapshot(&PathUri::from_abs_path(&req.cwd));
let (file_system_sandbox_policy, _) = attempt.permissions.to_runtime_permissions();
let sandbox_permissions = sandbox_permissions_preserving_denied_reads(
req.sandbox_permissions,
@@ -305,8 +310,12 @@ impl ToolRuntime<ShellRequest, ExecToolCallOutput> for ShellRuntime {
}
}
let command =
build_sandbox_command(&command, &req.cwd, &env, req.additional_permissions.clone())?;
let command = build_sandbox_command(
&command,
&PathUri::from_abs_path(&req.cwd),
&env,
req.additional_permissions.clone(),
)?;
let mut expiration: crate::exec::ExecExpiration = req.timeout_ms.into();
expiration = expiration.with_cancellation(req.cancellation_token.clone());
if let Some(cancellation) = attempt.network_denial_cancellation_token.clone() {
@@ -132,8 +132,12 @@ pub(super) async fn try_run_zsh_fork(
};
let mut env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions);
prepend_zsh_fork_bin_to_path(&mut env, shell_zsh_path);
let command =
build_sandbox_command(command, &req.cwd, &env, req.additional_permissions.clone())?;
let command = build_sandbox_command(
command,
&PathUri::from_abs_path(&req.cwd),
&env,
req.additional_permissions.clone(),
)?;
let options = ExecOptions {
expiration: req.timeout_ms.into(),
capture_policy: ExecCapturePolicy::ShellTool,
@@ -172,6 +176,11 @@ pub(super) async fn try_run_zsh_fork(
let exec_policy = Arc::new(RwLock::new(
ctx.session.services.exec_policy.current().as_ref().clone(),
));
let sandbox_cwd = sandbox_cwd.to_abs_path().map_err(|err| {
ToolError::Rejected(format!(
"sandbox cwd `{sandbox_cwd}` is not native to the Codex host: {err}"
))
})?;
let command_executor = CoreShellCommandExecutor {
command,
cwd: sandbox_cwd,
@@ -273,9 +282,15 @@ pub(crate) async fn prepare_unified_exec_zsh_fork(
let exec_policy = Arc::new(RwLock::new(
ctx.session.services.exec_policy.current().as_ref().clone(),
));
let native_cwd = exec_request.cwd.to_abs_path().map_err(|err| {
ToolError::Rejected(format!(
"zsh-fork cwd `{}` is not native to the Codex host: {err}",
exec_request.cwd
))
})?;
let command_executor = CoreShellCommandExecutor {
command: exec_request.command.clone(),
cwd: exec_request.cwd.clone(),
cwd: native_cwd,
permission_profile: exec_request.permission_profile.clone(),
file_system_sandbox_policy: exec_request.file_system_sandbox_policy.clone(),
network_sandbox_policy: exec_request.network_sandbox_policy,
@@ -852,7 +867,7 @@ impl CoreShellCommandExecutor {
let result = crate::sandboxing::execute_exec_request_with_after_spawn(
crate::sandboxing::ExecRequest {
command: self.command.clone(),
cwd: self.cwd.clone(),
cwd: PathUri::from_abs_path(&self.cwd),
env: exec_env,
exec_server_env_config: None,
network: self.network.clone(),
@@ -1003,9 +1018,15 @@ impl CoreShellCommandExecutor {
network.apply_to_env(&mut exec_request.env);
}
let cwd = exec_request.cwd.to_abs_path().map_err(|err| {
anyhow::anyhow!(
"escalated cwd `{}` is not native to the Codex host: {err}",
exec_request.cwd
)
})?;
Ok(PreparedExec {
command: exec_request.command,
cwd: exec_request.cwd.to_path_buf(),
cwd: cwd.to_path_buf(),
env: exec_request.env,
arg0: exec_request.arg0,
})
@@ -51,6 +51,7 @@ use codex_sandboxing::SandboxablePreference;
use codex_shell_command::powershell::prefix_powershell_script_with_utf8;
use codex_tools::UnifiedExecShellMode;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use futures::future::BoxFuture;
use std::collections::HashMap;
use tokio_util::sync::CancellationToken;
@@ -63,7 +64,7 @@ pub struct UnifiedExecRequest {
pub shell_type: ShellType,
pub hook_command: String,
pub process_id: i32,
pub cwd: AbsolutePathBuf,
pub cwd: PathUri,
pub sandbox_cwd: AbsolutePathBuf,
pub turn_environment: TurnEnvironment,
pub env: HashMap<String, String>,
@@ -84,7 +85,7 @@ pub struct UnifiedExecRequest {
#[derive(serde::Serialize, Clone, Debug, Eq, PartialEq, Hash)]
pub struct UnifiedExecApprovalKey {
pub command: Vec<String>,
pub cwd: AbsolutePathBuf,
pub cwd: PathUri,
pub tty: bool,
pub sandbox_permissions: SandboxPermissions,
pub additional_permissions: Option<AdditionalPermissionProfile>,
@@ -147,19 +148,26 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
&'b mut self,
req: &'b UnifiedExecRequest,
ctx: ApprovalCtx<'b>,
) -> BoxFuture<'b, ReviewDecision> {
) -> BoxFuture<'b, Result<ReviewDecision, ToolError>> {
let keys = self.approval_keys(req);
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
let command = req.command.clone();
let cwd = req.cwd.clone();
let retry_reason = ctx.retry_reason.clone();
let reason = retry_reason.clone().or_else(|| req.justification.clone());
let guardian_review_id = ctx.guardian_review_id.clone();
Box::pin(async move {
// TODO(anp): Migrate command approval payloads to PathUri so approvals can represent
// working directories native to a remote environment.
let cwd = req.cwd.to_abs_path().map_err(|err| {
ToolError::Rejected(format!(
"command cwd `{}` is not native to the Codex host: {err}",
req.cwd
))
})?;
if let Some(review_id) = guardian_review_id {
return review_approval_request(
return Ok(review_approval_request(
session,
turn,
review_id,
@@ -174,28 +182,30 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
},
retry_reason,
)
.await;
.await);
}
with_cached_approval(&session.services, "unified_exec", keys, || async move {
let available_decisions = None;
session
.request_command_approval(
turn,
call_id,
/*approval_id*/ None,
command,
cwd.clone(),
reason,
ctx.network_approval_context.clone(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
req.additional_permissions.clone(),
available_decisions,
)
.await
})
.await
Ok(
with_cached_approval(&session.services, "unified_exec", keys, || async move {
let available_decisions = None;
session
.request_command_approval(
turn,
call_id,
/*approval_id*/ None,
command,
cwd.clone(),
reason,
ctx.network_approval_context.clone(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
req.additional_permissions.clone(),
available_decisions,
)
.await
})
.await,
)
})
}
@@ -238,6 +248,9 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
);
let network =
managed_network_for_sandbox_permissions(req.network.as_ref(), sandbox_permissions)?;
// TODO(anp): Migrate guardian network approval triggers to PathUri so remote working
// directories do not suppress managed-network approval context.
let cwd = req.cwd.to_abs_path().ok()?;
Some(NetworkApprovalSpec {
network: Some(network.clone()),
mode: NetworkApprovalMode::Deferred,
@@ -245,7 +258,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
call_id: ctx.call_id.clone(),
tool_name: flat_tool_name(&ctx.tool_name).into_owned(),
command: req.command.clone(),
cwd: req.cwd.clone(),
cwd,
sandbox_permissions: req.sandbox_permissions,
additional_permissions: req.additional_permissions.clone(),
justification: req.justification.clone(),
@@ -398,6 +411,9 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
.env_for(command, options, managed_network)
.map_err(ToolError::Codex)?;
exec_env.exec_server_env_config = req.exec_server_env_config.clone();
if req.turn_environment.environment.is_remote() {
exec_env.cwd = req.cwd.clone();
}
self.manager
.open_session_with_exec_env(
req.process_id,
@@ -435,7 +451,7 @@ mod tests {
TurnEnvironment::new(
LOCAL_ENVIRONMENT_ID.to_string(),
Arc::new(Environment::default_for_tests()),
cwd,
PathUri::from_abs_path(&cwd),
/*shell*/ None,
)
}
@@ -477,7 +493,7 @@ mod tests {
shell_type: ShellType::Sh,
hook_command: "pwd".to_string(),
process_id: 1000,
cwd,
cwd: PathUri::from_abs_path(&cwd),
sandbox_cwd: sandbox_cwd.clone(),
turn_environment: test_turn_environment(sandbox_cwd.clone()),
env: HashMap::new(),
@@ -576,7 +592,7 @@ mod tests {
shell_type: ShellType::Zsh,
hook_command: "echo hi".to_string(),
process_id: 1000,
cwd: cwd.clone(),
cwd: PathUri::from_abs_path(&cwd),
sandbox_cwd: cwd.clone(),
turn_environment: test_turn_environment(cwd),
env: HashMap::new(),
+1 -1
View File
@@ -366,7 +366,7 @@ pub(crate) trait Approvable<Req> {
&'a mut self,
req: &'a Req,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision>;
) -> BoxFuture<'a, Result<ReviewDecision, ToolError>>;
}
pub(crate) trait Sandboxable {
+2 -3
View File
@@ -597,10 +597,9 @@ async fn zsh_fork_unified_exec_keeps_shell_parameter_when_remote_environment_ava
))
.expect("remote test environment"),
),
codex_utils_path_uri::PathUri::from_abs_path(&remote_cwd),
remote_cwd,
/*shell*/ None,
)
.expect("remote turn environment"),
),
);
})
.await;
+2 -1
View File
@@ -32,6 +32,7 @@ use codex_protocol::models::AdditionalPermissionProfile;
use codex_tools::UnifiedExecShellMode;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_output_truncation::TruncationPolicy;
use codex_utils_path_uri::PathUri;
use rand::Rng;
use rand::rng;
use tokio::sync::Mutex;
@@ -96,7 +97,7 @@ pub(crate) struct ExecCommandRequest {
pub process_id: i32,
pub yield_time_ms: u64,
pub max_output_tokens: Option<usize>,
pub cwd: AbsolutePathBuf,
pub cwd: PathUri,
pub sandbox_cwd: AbsolutePathBuf,
pub turn_environment: TurnEnvironment,
pub shell_mode: UnifiedExecShellMode,
@@ -57,7 +57,6 @@ use codex_protocol::protocol::ExecCommandSource;
use codex_tools::ToolName;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_output_truncation::approx_token_count;
use codex_utils_path_uri::PathUri;
const UNIFIED_EXEC_ENV: [(&str, &str); 10] = [
("NO_COLOR", "1"),
@@ -157,7 +156,7 @@ fn exec_server_params_for_request(
codex_exec_server::ExecParams {
process_id: exec_server_process_id(process_id).into(),
argv: request.command.clone(),
cwd: PathUri::from_abs_path(&request.cwd),
cwd: request.cwd.clone(),
env_policy,
env,
tty,
@@ -385,10 +384,15 @@ impl UnifiedExecProcessManager {
request: ExecCommandRequest,
context: &UnifiedExecContext,
) -> Result<ExecCommandToolOutput, UnifiedExecError> {
let cwd = request.cwd.clone();
let process = self
.open_session_with_sandbox(&request, cwd.clone(), context)
.await;
// TODO(anp): Migrate unified-exec events and retained process metadata to PathUri so
// foreign working directories do not need a host-native projection here.
let event_cwd = request.cwd.to_abs_path().map_err(|err| {
UnifiedExecError::create_process(format!(
"command cwd `{}` is not native to the Codex host: {err}",
request.cwd
))
})?;
let process = self.open_session_with_sandbox(&request, context).await;
let (process, mut deferred_network_approval) = match process {
Ok((process, deferred_network_approval)) => {
@@ -416,7 +420,7 @@ impl UnifiedExecProcessManager {
);
let emitter = ToolEmitter::unified_exec(
&request.command,
cwd.clone(),
event_cwd.clone(),
ExecCommandSource::UnifiedExecStartup,
Some(request.process_id.to_string()),
);
@@ -434,7 +438,7 @@ impl UnifiedExecProcessManager {
context,
&request.command,
request.hook_command.clone(),
cwd.clone(),
event_cwd.clone(),
start,
request.process_id,
request.tty,
@@ -493,7 +497,7 @@ impl UnifiedExecProcessManager {
process_started_alive,
context,
&request,
cwd.clone(),
event_cwd.clone(),
Arc::clone(&transcript),
text.clone(),
message.clone(),
@@ -513,7 +517,7 @@ impl UnifiedExecProcessManager {
process_started_alive,
context,
&request,
cwd.clone(),
event_cwd.clone(),
Arc::clone(&transcript),
text.clone(),
message.clone(),
@@ -565,7 +569,7 @@ impl UnifiedExecProcessManager {
process_started_alive,
context,
&request,
cwd.clone(),
event_cwd.clone(),
Arc::clone(&transcript),
text.clone(),
message.clone(),
@@ -582,7 +586,7 @@ impl UnifiedExecProcessManager {
Arc::clone(&context.turn),
context.call_id.clone(),
request.command.clone(),
cwd.clone(),
event_cwd.clone(),
Some(process_id.to_string()),
Arc::clone(&transcript),
text.clone(),
@@ -899,6 +903,12 @@ impl UnifiedExecProcessManager {
#[cfg(target_os = "windows")]
if request.sandbox == codex_sandboxing::SandboxType::WindowsRestrictedToken {
let native_cwd = request.cwd.to_abs_path().map_err(|err| {
UnifiedExecError::create_process(format!(
"failed to convert command cwd `{}` for Windows sandbox execution: {err}",
request.cwd
))
})?;
let codex_home = crate::config::find_codex_home().map_err(|err| {
UnifiedExecError::create_process(format!(
"windows sandbox: failed to resolve codex_home: {err}"
@@ -933,7 +943,7 @@ impl UnifiedExecProcessManager {
request.windows_sandbox_workspace_roots.as_slice(),
codex_home.as_ref(),
request.command.clone(),
request.cwd.as_path(),
native_cwd.as_path(),
request.env.clone(),
request.network.is_some(),
None,
@@ -955,7 +965,7 @@ impl UnifiedExecProcessManager {
request.windows_sandbox_workspace_roots.as_slice(),
codex_home.as_ref(),
request.command.clone(),
request.cwd.as_path(),
native_cwd.as_path(),
request.env.clone(),
None,
&additional_deny_read_paths,
@@ -995,11 +1005,17 @@ impl UnifiedExecProcessManager {
.command
.split_first()
.ok_or(UnifiedExecError::MissingCommandLine)?;
let native_cwd = request.cwd.to_abs_path().map_err(|err| {
UnifiedExecError::create_process(format!(
"failed to convert command cwd `{}` for local execution: {err}",
request.cwd
))
})?;
let spawn_result = if tty {
codex_utils_pty::pty::spawn_process_with_inherited_fds(
program,
args,
request.cwd.as_path(),
native_cwd.as_path(),
&request.env,
&request.arg0,
codex_utils_pty::TerminalSize::default(),
@@ -1010,7 +1026,7 @@ impl UnifiedExecProcessManager {
codex_utils_pty::pipe::spawn_process_no_stdin_with_inherited_fds(
program,
args,
request.cwd.as_path(),
native_cwd.as_path(),
&request.env,
&request.arg0,
&inherited_fds,
@@ -1026,7 +1042,6 @@ impl UnifiedExecProcessManager {
pub(super) async fn open_session_with_sandbox(
&self,
request: &ExecCommandRequest,
cwd: AbsolutePathBuf,
context: &UnifiedExecContext,
) -> Result<(UnifiedExecProcess, Option<DeferredNetworkApproval>), UnifiedExecError> {
let local_policy_env = create_env(
@@ -1067,7 +1082,7 @@ impl UnifiedExecProcessManager {
shell_type: request.shell_type,
hook_command: request.hook_command.clone(),
process_id: request.process_id,
cwd,
cwd: request.cwd.clone(),
sandbox_cwd: request.sandbox_cwd.clone(),
turn_environment: request.turn_environment.clone(),
env,
@@ -1,5 +1,6 @@
use super::*;
use crate::unified_exec::clamp_yield_time;
use codex_utils_path_uri::PathUri;
use pretty_assertions::assert_eq;
use tokio::time::Duration;
use tokio::time::Instant;
@@ -72,13 +73,14 @@ fn exec_server_params_use_path_uri_and_env_policy_overlay_contract() {
.expect("current dir")
.try_into()
.expect("absolute path");
let command_cwd = PathUri::parse("file://server/share/workspace").expect("valid UNC path URI");
let file_system_sandbox_policy =
codex_protocol::permissions::FileSystemSandboxPolicy::unrestricted();
let network_sandbox_policy = codex_protocol::permissions::NetworkSandboxPolicy::Restricted;
let permission_profile = codex_protocol::models::PermissionProfile::Disabled;
let request = ExecRequest {
command: vec!["bash".to_string(), "-lc".to_string(), "true".to_string()],
cwd: cwd.clone(),
cwd: command_cwd.clone(),
env: HashMap::from([
("HOME".to_string(), "/client-home".to_string()),
("PATH".to_string(), "/sandbox-path".to_string()),
@@ -116,7 +118,7 @@ fn exec_server_params_use_path_uri_and_env_policy_overlay_contract() {
exec_server_params_for_request(/*process_id*/ 123, &request, /*tty*/ true);
assert_eq!(params.process_id.as_str(), "123");
assert_eq!(params.cwd, PathUri::from_abs_path(&request.cwd));
assert_eq!(params.cwd, command_cwd);
assert!(params.env_policy.is_some());
assert_eq!(
params.env,
@@ -200,7 +202,7 @@ async fn failed_initial_end_for_unstored_process_uses_fallback_output() {
yield_time_ms: 1000,
max_output_tokens: None,
#[allow(deprecated)]
cwd: turn.cwd.clone(),
cwd: PathUri::from_abs_path(&turn.cwd),
#[allow(deprecated)]
sandbox_cwd: turn.cwd.clone(),
turn_environment: turn
@@ -339,11 +339,11 @@ async fn app_server_starts_thread_with_windows_environment_native_cwd() -> Resul
.received_requests()
.await
.context("failed to fetch received requests")?;
let model_request_includes_remote_instructions = requests.iter().any(|request| {
request
.body_json::<serde_json::Value>()
.is_ok_and(|body| body.to_string().contains(AGENTS_INSTRUCTIONS))
});
let initial_request = requests.first().context("missing initial model request")?;
let model_request_includes_remote_instructions = initial_request
.body_json::<serde_json::Value>()?
.to_string()
.contains(AGENTS_INSTRUCTIONS);
// TODO(anp): Load remote workspace instructions into the model context.
assert!(!model_request_includes_remote_instructions);
+5 -9
View File
@@ -156,10 +156,11 @@ impl LocalProcess {
.argv
.split_first()
.ok_or_else(|| invalid_params("argv must not be empty".to_string()))?;
let native_cwd = params.cwd.to_abs_path().map_err(|err| {
let native_cwd = params.cwd.to_abs_path().map_err(|_| {
invalid_params(format!(
"cwd URI `{}` is not valid on this exec-server host: {err}",
params.cwd
"'{}' is invalid on '{}'",
params.cwd,
std::env::consts::OS
))
})?;
@@ -815,12 +816,7 @@ mod tests {
#[cfg(windows)]
let uri = "file:///usr/local/checkout";
let cwd = PathUri::parse(uri).expect("non-native cwd URI");
let source = cwd
.to_abs_path()
.expect_err("cwd should not be native to this host");
let expected = invalid_params(format!(
"cwd URI `{cwd}` is not valid on this exec-server host: {source}"
));
let expected = invalid_params(format!("'{cwd}' is invalid on '{}'", std::env::consts::OS));
let mut params = test_exec_params(HashMap::new());
params.cwd = cwd;
+1 -1
View File
@@ -60,7 +60,7 @@ async fn spawn_command_under_sandbox(
child.arg0(arg0);
}
child.args(args);
child.current_dir(exec_request.cwd);
child.current_dir(exec_request.cwd.to_abs_path()?.as_path());
child.env_clear();
child.envs(exec_request.env);
@@ -97,6 +97,10 @@ impl ApiPathString {
&self.0
}
pub fn is_empty(&self) -> bool {
self.0.is_empty()
}
pub fn into_string(self) -> String {
self.0
}
+31 -3
View File
@@ -198,7 +198,11 @@ impl PathUri {
Some(Self(url))
}
/// Lexically joins a relative URI path onto this URI.
/// Lexically joins a relative path onto this URI.
///
/// `/` separates components for every base URI. When the base URI is
/// inferred to use the Windows path convention, `\` is also treated as a
/// separator. A leading separator is rejected as an absolute path.
///
/// Empty and `.` segments are ignored, while `..` removes one segment
/// without escaping the URI root. Literal `%`, `?`, and `#` characters are
@@ -207,7 +211,18 @@ impl PathUri {
/// Opaque fallback URIs created by [`Self::from_abs_path`] reject non-empty
/// joins.
pub fn join(&self, path: &str) -> Result<Self, PathUriParseError> {
if path.starts_with('/') {
let uses_windows_path_convention =
self.infer_path_convention() == Some(PathConvention::Windows);
let starts_with_root_separator = path.chars().next().is_some_and(|character| {
if uses_windows_path_convention {
matches!(character, '/' | '\\')
} else {
character == '/'
}
});
let has_windows_drive_prefix = uses_windows_path_convention
&& matches!(path.as_bytes(), [drive, b':', ..] if drive.is_ascii_alphabetic());
if starts_with_root_separator || has_windows_drive_prefix {
return Err(PathUriParseError::JoinPathMustBeRelative(path.to_string()));
}
if path.contains('\0') {
@@ -230,7 +245,14 @@ impl PathUri {
unreachable!("validated file URLs support hierarchical path segments");
};
segments.pop_if_empty();
for component in path.split('/') {
let is_separator = |character| {
if uses_windows_path_convention {
matches!(character, '/' | '\\')
} else {
character == '/'
}
};
for component in path.split(is_separator) {
match component {
"" | "." => {}
".." => {
@@ -378,6 +400,12 @@ impl FromStr for PathUri {
}
}
impl From<AbsolutePathBuf> for PathUri {
fn from(path: AbsolutePathBuf) -> Self {
Self::from_abs_path(&path)
}
}
impl fmt::Display for PathUri {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.0.fmt(f)
+26
View File
@@ -481,11 +481,26 @@ fn join_normalizes_relative_uri_segments() {
),
("file:///", "../../etc", "file:///etc"),
("file:///C:/Users", "../Windows", "file:///C:/Windows"),
(
"file:///C:/Users/Alice",
r"..\Bob\source",
"file:///C:/Users/Bob/source",
),
(
"file://server/share/src",
"../tests",
"file://server/share/tests",
),
(
"file://server/share/src",
r"..\tests",
"file://server/share/tests",
),
(
"file:///workspace",
r"src\main.rs",
"file:///workspace/src%5Cmain.rs",
),
(
"file:///workspace",
"a?b#c%d",
@@ -502,6 +517,7 @@ fn join_normalizes_relative_uri_segments() {
#[test]
fn join_rejects_absolute_and_null_paths() {
let base = PathUri::parse("file:///workspace").expect("valid base URI");
let windows_base = PathUri::parse("file:///C:/workspace").expect("valid base URI");
assert!(matches!(
base.join("/src"),
@@ -513,6 +529,16 @@ fn join_rejects_absolute_and_null_paths() {
path: "src\0file".to_string(),
})
);
assert!(matches!(
windows_base.join(r"\src"),
Err(PathUriParseError::JoinPathMustBeRelative(path)) if path == r"\src"
));
for path in [r"C:\src", "C:src"] {
assert!(matches!(
windows_base.join(path),
Err(PathUriParseError::JoinPathMustBeRelative(rejected)) if rejected == path
));
}
}
#[test]