Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions codex-rs/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion codex-rs/core/src/unified_exec/process_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ 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"),
Expand Down Expand Up @@ -156,7 +157,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: request.cwd.to_path_buf(),
cwd: PathUri::from_abs_path(&request.cwd),
Comment thread
anp-oai marked this conversation as resolved.
env_policy,
env,
tty,
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/core/src/unified_exec/process_manager_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ fn env_overlay_for_exec_server_keeps_runtime_changes_only() {
}

#[test]
fn exec_server_params_use_env_policy_overlay_contract() {
fn exec_server_params_use_path_uri_and_env_policy_overlay_contract() {
let cwd: codex_utils_absolute_path::AbsolutePathBuf = std::env::current_dir()
.expect("current dir")
.try_into()
Expand Down Expand Up @@ -115,6 +115,7 @@ fn exec_server_params_use_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!(params.env_policy.is_some());
assert_eq!(
params.env,
Expand Down
70 changes: 12 additions & 58 deletions codex-rs/core/tests/remote_env_windows/remote_env_windows_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ use codex_features::Feature;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ExecCommandSource;
use codex_protocol::protocol::ExecCommandStatus;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::TurnEnvironmentSelection;
use codex_protocol::protocol::TurnEnvironmentSelections;
Expand All @@ -23,7 +21,6 @@ use core_test_support::responses::start_mock_server;
use core_test_support::test_codex::test_codex;
use core_test_support::test_codex::turn_permission_fields;
use core_test_support::wait_for_event;
use pretty_assertions::assert_eq;
use serde_json::json;
use tokio::io::AsyncBufReadExt;
use tokio::io::BufReader;
Expand All @@ -33,7 +30,7 @@ const CALL_ID: &str = "wine-cmd-smoke";
const COMMAND: &str = "echo WINE_BAZEL_OK&&cd";

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn windows_exec_server_records_host_shell_mismatch() -> Result<()> {
async fn windows_exec_server_rejects_non_native_cwd_uri() -> Result<()> {
let executable = codex_utils_cargo_bin::cargo_bin("wine-windows-exec-server")?;
let mut exec_server = WineTestCommand::new(executable)
.env("CODEX_HOME", r"C:\codex-home")
Expand Down Expand Up @@ -124,79 +121,36 @@ async fn windows_exec_server_records_host_shell_mismatch() -> Result<()> {
})
.await?;

let mut begin = None;
let mut end = None;
let mut saw_exec_event = false;
loop {
match wait_for_event(&test.codex, |_| true).await {
EventMsg::ExecCommandBegin(event) if event.call_id == CALL_ID => {
begin = Some(event)
saw_exec_event = true
}
EventMsg::ExecCommandEnd(event) if event.call_id == CALL_ID => {
end = Some(event)
saw_exec_event = true
}
EventMsg::TurnComplete(_) => break,
_ => {}
}
}

let begin = begin.context("exec_command should emit a begin event")?;
let expected_commands = [
vec![
"/bin/bash".to_string(),
"-c".to_string(),
COMMAND.to_string(),
],
vec!["/bin/sh".to_string(), "-c".to_string(), COMMAND.to_string()],
];
// This intentionally records the current cross-OS failure mode: the Linux
// orchestrator resolves its own shell before sending the command to the
// Windows exec-server, where that Unix shell cannot start.
assert!(
expected_commands.contains(&begin.command),
"unexpected command: {:?}",
begin.command,
);
assert_eq!(
(begin.cwd.clone(), begin.source),
(
test.config.cwd.clone(),
ExecCommandSource::UnifiedExecStartup,
),
);

let end = end.context("exec_command should emit an end event")?;
assert_eq!(
(
end.command,
end.cwd,
end.source,
end.stdout,
end.stderr,
end.aggregated_output,
end.exit_code,
end.status,
),
(
begin.command,
test.config.cwd.clone(),
ExecCommandSource::UnifiedExecStartup,
String::new(),
String::new(),
String::new(),
-1,
ExecCommandStatus::Failed,
),
!saw_exec_event,
"a non-native cwd should be rejected before process lifecycle events",
);

let request = response_mock
.last_request()
.context("model should receive the failed command output")?;
.context("model should receive the rejected command output")?;
let (output, success) = request
.function_call_output_content_and_success(CALL_ID)
.context("failed command output should be present")?;
let output = output.context("failed command output should contain text")?;
.context("rejected command output should be present")?;
let output = output.context("rejected command output should contain text")?;
assert!(
output.contains("Process exited with code -1"),
output.contains("exec-server rejected request (-32602)")
&& output.contains("cwd URI")
&& output.contains("is not valid on this exec-server host"),
"unexpected command output: {output:?}",
);
assert_ne!(success, Some(true));
Expand Down
6 changes: 3 additions & 3 deletions codex-rs/exec-server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ Request params:
{
"processId": "proc-1",
"argv": ["bash", "-lc", "printf 'hello\\n'"],
"cwd": "/absolute/working/directory",
"cwd": "file:///absolute/working/directory",
"env": {
"PATH": "/usr/bin:/bin"
},
Expand All @@ -171,7 +171,7 @@ Field definitions:

- `processId`: caller-chosen stable id for this process within the connection.
- `argv`: command vector. It must be non-empty.
- `cwd`: absolute working directory used for the child process.
- `cwd`: `file:` URI for the child process working directory.
- `env`: environment variables passed to the child process.
- `tty`: when `true`, spawn a PTY-backed interactive process.
- `pipeStdin`: when `true`, keep non-PTY stdin writable via `process/write`.
Expand Down Expand Up @@ -409,7 +409,7 @@ Initialize:
Start a process:

```json
{"id":2,"method":"process/start","params":{"processId":"proc-1","argv":["bash","-lc","printf 'ready\\n'; while IFS= read -r line; do printf 'echo:%s\\n' \"$line\"; done"],"cwd":"/tmp","env":{"PATH":"/usr/bin:/bin"},"tty":true,"pipeStdin":false,"arg0":null}}
{"id":2,"method":"process/start","params":{"processId":"proc-1","argv":["bash","-lc","printf 'ready\\n'; while IFS= read -r line; do printf 'echo:%s\\n' \"$line\"; done"],"cwd":"file:///tmp","env":{"PATH":"/usr/bin:/bin"},"tty":true,"pipeStdin":false,"arg0":null}}
{"id":2,"result":{"processId":"proc-1"}}
{"method":"process/output","params":{"processId":"proc-1","seq":1,"stream":"stdout","chunk":"cmVhZHkK"}}
```
Expand Down
4 changes: 3 additions & 1 deletion codex-rs/exec-server/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ mod tests {
use crate::ProcessId;
use crate::environment_provider::EnvironmentDefault;
use crate::environment_provider::EnvironmentProviderSnapshot;
use codex_utils_path_uri::PathUri;
use pretty_assertions::assert_eq;

fn test_runtime_paths() -> ExecServerRuntimePaths {
Expand Down Expand Up @@ -862,7 +863,8 @@ mod tests {
.start(crate::ExecParams {
process_id: ProcessId::from("default-env-proc"),
argv: vec!["true".to_string()],
cwd: std::env::current_dir().expect("read current dir"),
cwd: PathUri::from_path(std::env::current_dir().expect("read current dir"))
.expect("cwd URI"),
env_policy: None,
env: Default::default(),
tty: false,
Expand Down
39 changes: 35 additions & 4 deletions codex-rs/exec-server/src/local_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,12 @@ 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| {
invalid_params(format!(
"cwd URI `{}` is not valid on this exec-server host: {err}",
params.cwd
))
})?;

{
let mut process_map = self.inner.processes.lock().await;
Expand All @@ -172,7 +178,7 @@ impl LocalProcess {
codex_utils_pty::spawn_pty_process(
program,
args,
params.cwd.as_path(),
native_cwd.as_path(),
&env,
&params.arg0,
TerminalSize::default(),
Expand All @@ -182,7 +188,7 @@ impl LocalProcess {
codex_utils_pty::spawn_pipe_process(
program,
args,
params.cwd.as_path(),
native_cwd.as_path(),
&env,
&params.arg0,
)
Expand All @@ -191,7 +197,7 @@ impl LocalProcess {
codex_utils_pty::spawn_pipe_process_no_stdin(
program,
args,
params.cwd.as_path(),
native_cwd.as_path(),
&env,
&params.arg0,
)
Expand Down Expand Up @@ -783,6 +789,7 @@ fn notification_sender(inner: &Inner) -> Option<RpcNotificationSender> {
mod tests {
use super::*;
use codex_protocol::config_types::ShellEnvironmentPolicyInherit;
use codex_utils_path_uri::PathUri;
use codex_utils_pty::ProcessDriver;
use pretty_assertions::assert_eq;
use tokio::sync::oneshot;
Expand All @@ -792,7 +799,7 @@ mod tests {
ExecParams {
process_id: ProcessId::from("env-test"),
argv: vec!["true".to_string()],
cwd: std::path::PathBuf::from("/tmp"),
cwd: PathUri::from_path(std::env::current_dir().expect("cwd")).expect("cwd URI"),
env_policy: None,
env,
tty: false,
Expand All @@ -801,6 +808,30 @@ mod tests {
}
}

#[tokio::test]
async fn start_process_rejects_non_native_cwd_before_launch() {
#[cfg(unix)]
let uri = "file://server/share/checkout";
#[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 mut params = test_exec_params(HashMap::new());
params.cwd = cwd;

let result = LocalProcess::default().start_process(params).await;
let Err(error) = result else {
panic!("non-native cwd should be rejected");
};

assert_eq!(error, expected);
}

#[test]
fn child_env_defaults_to_exact_env() {
let params = test_exec_params(HashMap::from([("ONLY_THIS".to_string(), "1".to_string())]));
Expand Down
9 changes: 6 additions & 3 deletions codex-rs/exec-server/src/protocol.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::collections::HashMap;
use std::path::PathBuf;

use crate::FileSystemSandboxContext;
use base64::engine::general_purpose::STANDARD as BASE64_STANDARD;
Expand Down Expand Up @@ -77,7 +76,8 @@ pub struct EnvironmentInfo {
pub struct ShellInfo {
/// Stable shell name, for example `zsh`, `bash`, `powershell`, `sh`, or `cmd`.
pub name: String,
/// Path the exec server would use for that shell.
/// Target-native shell executable path or command name. Fallbacks such as `cmd.exe` need not
/// be absolute, so this is not a [`PathUri`].
pub path: String,
}

Expand All @@ -88,14 +88,17 @@ pub struct ExecParams {
/// This is a protocol key, not an OS pid.
pub process_id: ProcessId,
pub argv: Vec<String>,
pub cwd: PathBuf,
/// Working directory URI, interpreted using the exec-server host's path rules at launch time.
pub cwd: PathUri,
#[serde(default)]
pub env_policy: Option<ExecEnvPolicy>,
pub env: HashMap<String, String>,
pub tty: bool,
/// Keep non-tty stdin writable through `process/write`.
#[serde(default)]
pub pipe_stdin: bool,
/// Optional process-visible argv0 override. Values such as `codex-linux-sandbox` are command
/// names rather than paths, so this is not a [`PathUri`].
pub arg0: Option<String>,
}

Expand Down
3 changes: 2 additions & 1 deletion codex-rs/exec-server/src/server/handler/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::collections::HashMap;
use std::sync::Arc;
use std::time::Duration;

use codex_utils_path_uri::PathUri;
use pretty_assertions::assert_eq;
use tokio::sync::mpsc;
use uuid::Uuid;
Expand All @@ -26,7 +27,7 @@ fn exec_params_with_argv(process_id: &str, argv: Vec<String>) -> ExecParams {
ExecParams {
process_id: ProcessId::from(process_id),
argv,
cwd: std::env::current_dir().expect("cwd"),
cwd: PathUri::from_path(std::env::current_dir().expect("cwd")).expect("cwd URI"),
env_policy: None,
env: inherited_path_env(),
tty: false,
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/exec-server/src/server/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ mod tests {
use codex_app_server_protocol::JSONRPCRequest;
use codex_app_server_protocol::JSONRPCResponse;
use codex_app_server_protocol::RequestId;
use codex_utils_path_uri::PathUri;
use serde::Serialize;
use serde::de::DeserializeOwned;
use tokio::io::AsyncBufReadExt;
Expand Down Expand Up @@ -396,7 +397,7 @@ mod tests {
ExecParams {
process_id,
argv: sleep_then_print_argv(),
cwd: std::env::current_dir().expect("cwd"),
cwd: PathUri::from_path(std::env::current_dir().expect("cwd")).expect("cwd URI"),
env_policy: None,
env,
tty: false,
Expand Down
Loading
Loading