mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
core: keep remote exec on reported shell (#28983)
## Why We need to avoid resolving shells on the app-server's host for remote environments. We might make it possible to do fancier shell resolution from remote envs but for now just require the model to produce a shell that matches the environment's default. This gets my e2e demo working for shell commands after #28854 moved shell resolution to PathUri and caused remote envs to hit the fallback shell when the shell wasn't available on the host. ## What Remote `exec_command` calls now accept only the environment's reported default shell name or exact path, and execute with that reported path. Other explicit shells return a concise error. A Wine-backed integration test covers explicit PowerShell execution in the Windows cwd.
This commit is contained in:
committed by
GitHub
Unverified
parent
195c936fa2
commit
346d2c163f
@@ -1,3 +1,4 @@
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
||||
use crate::function_tool::FunctionCallError;
|
||||
@@ -31,6 +32,7 @@ use codex_otel::TOOL_CALL_UNIFIED_EXEC_METRIC;
|
||||
use codex_sandboxing::SandboxManager;
|
||||
use codex_sandboxing::SandboxType;
|
||||
use codex_sandboxing::SandboxablePreference;
|
||||
use codex_shell_command::shell_detect::detect_shell_type;
|
||||
use codex_tools::ToolName;
|
||||
use codex_tools::ToolSpec;
|
||||
use codex_utils_output_truncation::approx_token_count;
|
||||
@@ -172,7 +174,7 @@ impl ExecCommandHandler {
|
||||
)));
|
||||
}
|
||||
};
|
||||
let args: ExecCommandArgs = match native_cwd.as_ref() {
|
||||
let mut args: ExecCommandArgs = match native_cwd.as_ref() {
|
||||
Some(native_cwd) => {
|
||||
// The base path only resolves paths nested in the permissions config types.
|
||||
parse_arguments_with_base_path(&arguments, native_cwd)?
|
||||
@@ -195,7 +197,6 @@ impl ExecCommandHandler {
|
||||
)
|
||||
.await;
|
||||
}
|
||||
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
|
||||
@@ -205,6 +206,26 @@ impl ExecCommandHandler {
|
||||
.clone()
|
||||
.map(Arc::new)
|
||||
.unwrap_or_else(|| session.user_shell());
|
||||
// TODO(anp): Resolve requested shells in remote environments instead of restricting
|
||||
// commands to the reported default shell.
|
||||
if environment.is_remote()
|
||||
&& let Some(requested_shell) = args.shell.take()
|
||||
{
|
||||
let Some(remote_shell) = turn_environment.shell.as_ref() else {
|
||||
return Err(FunctionCallError::RespondToModel(format!(
|
||||
"environment `{}` does not report a shell",
|
||||
turn_environment.environment_id
|
||||
)));
|
||||
};
|
||||
if detect_shell_type(Path::new(&requested_shell)) != Some(remote_shell.shell_type) {
|
||||
return Err(FunctionCallError::RespondToModel(format!(
|
||||
"environment `{}` only supports `{}`",
|
||||
turn_environment.environment_id,
|
||||
remote_shell.name()
|
||||
)));
|
||||
}
|
||||
}
|
||||
let process_id = manager.allocate_process_id().await;
|
||||
let resolved_command = get_command(
|
||||
&args,
|
||||
shell,
|
||||
|
||||
@@ -228,6 +228,77 @@ async fn remote_test_env_exposes_target_shell_to_model() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn explicit_remote_shell_runs_in_remote_cwd() -> Result<()> {
|
||||
const CALL_ID: &str = "remote-explicit-shell";
|
||||
|
||||
let (shell, command) = match core_test_support::test_environment() {
|
||||
TestEnvironment::Docker { .. } => (
|
||||
"bash",
|
||||
r#"case "$PWD" in /tmp/codex-core-test-cwd-*) ;; *) echo "unexpected cwd: $PWD" >&2; exit 1 ;; esac"#,
|
||||
),
|
||||
TestEnvironment::WineExec => (
|
||||
"powershell",
|
||||
r#"$cwd = (Get-Location).Path; if ($cwd -notlike 'C:\codex-core-test-cwd-*') { Write-Error "unexpected cwd: $cwd"; exit 1 }"#,
|
||||
),
|
||||
TestEnvironment::Local => return Ok(()),
|
||||
};
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let arguments = serde_json::to_string(&json!({
|
||||
"cmd": command,
|
||||
"shell": shell,
|
||||
"login": false,
|
||||
"yield_time_ms": 10_000,
|
||||
}))?;
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build_with_remote_env(&server).await?;
|
||||
let response_mock = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_function_call(CALL_ID, "exec_command", &arguments),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
test.submit_turn_with_environments(
|
||||
"run the remote shell in the remote cwd",
|
||||
Some(vec![TurnEnvironmentSelection {
|
||||
environment_id: REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
cwd: PathUri::from_abs_path(&test.config.cwd),
|
||||
}]),
|
||||
)
|
||||
.await?;
|
||||
let request = response_mock
|
||||
.last_request()
|
||||
.context("model should receive the command output")?;
|
||||
let (output, success) = request
|
||||
.function_call_output_content_and_success(CALL_ID)
|
||||
.context("remote shell tool result should be present")?;
|
||||
assert_ne!(success, Some(false));
|
||||
assert!(
|
||||
output.is_some_and(|output| output.contains("Process exited with code 0")),
|
||||
"remote shell command should exit successfully",
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn absolute_path(path: PathBuf) -> AbsolutePathBuf {
|
||||
AbsolutePathBuf::try_from(path).expect("path should be absolute")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user