mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] exec-server honors remote environment cwd and shell (#28122)
## Why Next slice needed to make progress on the `remote_env_windows` test is to support passing a Windows cwd for the remote environment and using that environment's native shell. This lets the test run a real Windows process instead of only recording an early path or shell mismatch. ## What - change `TurnEnvironmentSelection.cwd` from `AbsolutePathBuf` to `PathUri` - convert local cwd values to URIs when constructing selections - preserve a remote primary cwd instead of replacing it with the local legacy fallback - prefer the selected environment's discovered shell for unified exec, falling back to the session shell when unavailable - convert back to a host-native absolute path at current native-only consumer boundaries - reject or deny unsupported foreign cwd values at the existing request-permissions boundary, with TODOs for its future migration - extend the hermetic Wine test to execute Windows PowerShell in `C:\windows` and verify successful process completion - record the current app-server rejection against the same Wine-backed remote Windows fixture when its cwd is supplied as a native Windows path
This commit is contained in:
@@ -7,6 +7,7 @@ use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::error::Result as CodexResult;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::shell::Shell;
|
||||
@@ -20,7 +21,7 @@ pub(crate) fn default_thread_environment_selections(
|
||||
.into_iter()
|
||||
.map(|environment_id| TurnEnvironmentSelection {
|
||||
environment_id,
|
||||
cwd: cwd.clone(),
|
||||
cwd: PathUri::from_abs_path(cwd),
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
@@ -99,7 +100,12 @@ pub(crate) async fn resolve_environment_selections(
|
||||
turn_environments.push(TurnEnvironment::new(
|
||||
environment_id,
|
||||
environment,
|
||||
selected_environment.cwd.clone(),
|
||||
selected_environment.cwd.to_abs_path().map_err(|err| {
|
||||
CodexErr::InvalidRequest(format!(
|
||||
"turn environment cwd `{}` is not valid on this host: {err}",
|
||||
selected_environment.cwd
|
||||
))
|
||||
})?,
|
||||
shell,
|
||||
));
|
||||
}
|
||||
@@ -114,6 +120,7 @@ mod tests {
|
||||
use codex_exec_server::REMOTE_ENVIRONMENT_ID;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::*;
|
||||
@@ -129,6 +136,7 @@ mod tests {
|
||||
#[tokio::test]
|
||||
async fn default_thread_environment_selections_use_manager_default_id() {
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let cwd_uri = PathUri::from_abs_path(&cwd);
|
||||
let manager = EnvironmentManager::create_for_tests(
|
||||
Some("ws://127.0.0.1:8765".to_string()),
|
||||
Some(test_runtime_paths()),
|
||||
@@ -139,7 +147,7 @@ mod tests {
|
||||
default_thread_environment_selections(&manager, &cwd),
|
||||
vec![TurnEnvironmentSelection {
|
||||
environment_id: REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
cwd,
|
||||
cwd: cwd_uri,
|
||||
}]
|
||||
);
|
||||
}
|
||||
@@ -157,6 +165,7 @@ url = "ws://127.0.0.1:8765"
|
||||
)
|
||||
.expect("write environments.toml");
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let cwd_uri = PathUri::from_abs_path(&cwd);
|
||||
let manager =
|
||||
EnvironmentManager::from_codex_home(temp_dir.path(), Some(test_runtime_paths()))
|
||||
.await
|
||||
@@ -167,11 +176,11 @@ url = "ws://127.0.0.1:8765"
|
||||
vec![
|
||||
TurnEnvironmentSelection {
|
||||
environment_id: LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
cwd: cwd.clone(),
|
||||
cwd: cwd_uri.clone(),
|
||||
},
|
||||
TurnEnvironmentSelection {
|
||||
environment_id: REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
cwd,
|
||||
cwd: cwd_uri,
|
||||
},
|
||||
]
|
||||
);
|
||||
@@ -191,6 +200,7 @@ url = "ws://127.0.0.1:8765"
|
||||
#[tokio::test]
|
||||
async fn resolve_environment_selections_rejects_duplicate_ids() {
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let cwd_uri = PathUri::from_abs_path(&cwd);
|
||||
let manager = EnvironmentManager::default_for_tests();
|
||||
|
||||
let err = resolve_environment_selections(
|
||||
@@ -198,11 +208,11 @@ url = "ws://127.0.0.1:8765"
|
||||
&[
|
||||
TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: cwd.clone(),
|
||||
cwd: cwd_uri.clone(),
|
||||
},
|
||||
TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: cwd.join("other"),
|
||||
cwd: cwd_uri.join("other").expect("other cwd URI"),
|
||||
},
|
||||
],
|
||||
)
|
||||
@@ -216,13 +226,14 @@ url = "ws://127.0.0.1:8765"
|
||||
async fn resolved_environment_selections_use_first_selection_as_primary() {
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let selected_cwd = cwd.join("selected");
|
||||
let selected_cwd_uri = PathUri::from_abs_path(&selected_cwd);
|
||||
let manager = EnvironmentManager::default_for_tests();
|
||||
|
||||
let resolved = resolve_environment_selections(
|
||||
&manager,
|
||||
&[TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: selected_cwd,
|
||||
cwd: selected_cwd_uri,
|
||||
}],
|
||||
)
|
||||
.await
|
||||
@@ -255,12 +266,13 @@ url = "ws://127.0.0.1:8765"
|
||||
#[tokio::test]
|
||||
async fn single_local_environment_cwd_requires_exactly_one_local_environment() {
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let cwd_uri = PathUri::from_abs_path(&cwd);
|
||||
let local_manager = EnvironmentManager::default_for_tests();
|
||||
let local = resolve_environment_selections(
|
||||
&local_manager,
|
||||
&[TurnEnvironmentSelection {
|
||||
environment_id: LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
cwd: cwd.clone(),
|
||||
cwd: cwd_uri,
|
||||
}],
|
||||
)
|
||||
.await
|
||||
|
||||
@@ -149,6 +149,7 @@ use codex_thread_store::ResumeThreadParams;
|
||||
use codex_thread_store::ThreadPersistenceMetadata;
|
||||
use codex_thread_store::ThreadStore;
|
||||
use codex_utils_output_truncation::TruncationPolicy;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use futures::future::BoxFuture;
|
||||
use futures::future::Shared;
|
||||
use futures::prelude::*;
|
||||
@@ -2206,6 +2207,18 @@ impl Session {
|
||||
}
|
||||
|
||||
let requested_permissions = args.permissions;
|
||||
// TODO(anp): Migrate request_permissions to support paths from foreign environments.
|
||||
let Ok(native_environment_cwd) = environment.cwd.to_abs_path() else {
|
||||
warn!(
|
||||
cwd = %environment.cwd,
|
||||
"request_permissions requires a cwd native to the Codex host"
|
||||
);
|
||||
return Some(RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
};
|
||||
|
||||
if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) {
|
||||
let originating_turn_state = {
|
||||
@@ -2273,7 +2286,7 @@ impl Session {
|
||||
let response = Self::normalize_request_permissions_response(
|
||||
requested_permissions,
|
||||
response,
|
||||
environment.cwd.as_path(),
|
||||
native_environment_cwd.as_path(),
|
||||
);
|
||||
self.record_granted_request_permissions_for_turn(
|
||||
&response,
|
||||
@@ -2313,7 +2326,7 @@ impl Session {
|
||||
started_at_ms: now_unix_timestamp_ms(),
|
||||
reason: args.reason,
|
||||
permissions: requested_permissions,
|
||||
cwd: Some(environment.cwd),
|
||||
cwd: Some(native_environment_cwd),
|
||||
});
|
||||
self.send_event(turn_context.as_ref(), event).await;
|
||||
tokio::select! {
|
||||
@@ -2354,7 +2367,7 @@ impl Session {
|
||||
});
|
||||
};
|
||||
let mut environment = turn_environment.selection();
|
||||
environment.cwd = cwd;
|
||||
environment.cwd = PathUri::from_abs_path(&cwd);
|
||||
self.request_permissions_for_environment(
|
||||
turn_context,
|
||||
call_id,
|
||||
@@ -2457,11 +2470,26 @@ impl Session {
|
||||
};
|
||||
match entry {
|
||||
Some(entry) => {
|
||||
let response = Self::normalize_request_permissions_response(
|
||||
entry.requested_permissions,
|
||||
response,
|
||||
entry.environment.cwd.as_path(),
|
||||
);
|
||||
// TODO(anp): Migrate request_permissions to support paths from foreign environments.
|
||||
let response = match entry.environment.cwd.to_abs_path() {
|
||||
Ok(native_environment_cwd) => Self::normalize_request_permissions_response(
|
||||
entry.requested_permissions,
|
||||
response,
|
||||
native_environment_cwd.as_path(),
|
||||
),
|
||||
Err(err) => {
|
||||
warn!(
|
||||
cwd = %entry.environment.cwd,
|
||||
%err,
|
||||
"request_permissions requires a cwd native to the Codex host"
|
||||
);
|
||||
RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
}
|
||||
}
|
||||
};
|
||||
self.record_granted_request_permissions_for_turn(
|
||||
&response,
|
||||
&entry.environment.environment_id,
|
||||
|
||||
@@ -58,6 +58,7 @@ use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelections;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionProfile;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use tracing::Span;
|
||||
|
||||
use crate::rollout::recorder::RolloutRecorder;
|
||||
@@ -4705,7 +4706,7 @@ async fn cwd_update_rewrites_sticky_environment_cwd() {
|
||||
assert_eq!(state.session_configuration.cwd(), &updated_cwd);
|
||||
assert_eq!(
|
||||
state.session_configuration.environment_selections()[0].cwd,
|
||||
updated_cwd
|
||||
PathUri::from_abs_path(&updated_cwd)
|
||||
);
|
||||
assert_ne!(environment_cwd, updated_cwd);
|
||||
}
|
||||
|
||||
@@ -79,7 +79,7 @@ impl TurnEnvironment {
|
||||
pub(crate) fn selection(&self) -> TurnEnvironmentSelection {
|
||||
TurnEnvironmentSelection {
|
||||
environment_id: self.environment_id.clone(),
|
||||
cwd: self.cwd.clone(),
|
||||
cwd: self.cwd_uri.clone(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -23,6 +23,7 @@ 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::PathUri;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::PathExt;
|
||||
use core_test_support::responses::mount_models_once;
|
||||
@@ -331,7 +332,7 @@ async fn start_thread_rejects_explicit_local_environment_when_default_provider_i
|
||||
parent_trace: None,
|
||||
environments: vec![TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: config.cwd.clone(),
|
||||
cwd: PathUri::from_abs_path(&config.cwd),
|
||||
}],
|
||||
thread_extension_init: Default::default(),
|
||||
})
|
||||
@@ -594,7 +595,7 @@ async fn resume_and_fork_do_not_restore_thread_environments_from_rollout() {
|
||||
std::fs::create_dir_all(&selected_cwd).expect("create selected cwd");
|
||||
let environments = vec![TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: selected_cwd.clone(),
|
||||
cwd: PathUri::from_abs_path(&selected_cwd),
|
||||
}];
|
||||
let default_cwd = config.cwd.clone();
|
||||
let mut source_config = config.clone();
|
||||
|
||||
@@ -151,9 +151,16 @@ impl ExecCommandHandler {
|
||||
let process_id = manager.allocate_process_id().await;
|
||||
let shell_mode =
|
||||
shell_mode_for_environment(&turn.unified_exec_shell_mode, environment.as_ref());
|
||||
// Remote environments may use a different OS and must build commands with their native
|
||||
// shell; fall back to the session shell when the environment did not report one.
|
||||
let shell = turn_environment
|
||||
.shell
|
||||
.clone()
|
||||
.map(Arc::new)
|
||||
.unwrap_or_else(|| session.user_shell());
|
||||
let resolved_command = get_command(
|
||||
&args,
|
||||
session.user_shell(),
|
||||
shell,
|
||||
&shell_mode,
|
||||
turn.config.permissions.allow_login_shell,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user