mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Use local environment for user shell commands (#28163)
## Why User shell commands still read the legacy turn cwd and session shell even though execution context is now owned by selected turn environments. App-server also defines `thread/shellCommand` as a local-host escape hatch, so it must use an available local environment even when a remote environment is primary. ## What changed - Add `ResolvedTurnEnvironments::local()` to find the selected local environment. - Resolve the user shell command cwd and shell from that local `TurnEnvironment`. - Emit the standard `shell is unavailable in this session` error when no selected local environment or resolved local shell is available. - Add an integration test covering `/shell` without a local environment. ## Test plan - `just test -p codex-core user_shell_command_without_local_environment_emits_error`
This commit is contained in:
committed by
GitHub
Unverified
parent
e752f7b4ae
commit
1e015884c5
@@ -194,6 +194,12 @@ impl TurnEnvironmentSnapshot {
|
||||
self.turn_environments.first()
|
||||
}
|
||||
|
||||
pub(crate) fn local(&self) -> Option<&TurnEnvironment> {
|
||||
self.turn_environments
|
||||
.iter()
|
||||
.find(|environment| !environment.environment.is_remote())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn primary_environment(&self) -> Option<Arc<codex_exec_server::Environment>> {
|
||||
self.primary()
|
||||
|
||||
@@ -36,6 +36,7 @@ use crate::exec_policy::ExecPolicyManager;
|
||||
use crate::image_preparation::prepare_response_items;
|
||||
use crate::parse_turn_item;
|
||||
use crate::realtime_conversation::RealtimeConversationManager;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::session_prefix::format_subagent_notification_message;
|
||||
use crate::skills::SkillRenderSideEffects;
|
||||
use crate::skills_load_input_from_config;
|
||||
@@ -1504,10 +1505,11 @@ impl Session {
|
||||
self.emit_config_changed_contributors(previous_config.as_ref(), new_config.as_ref());
|
||||
self.services.skills_manager.clear_cache();
|
||||
self.services.plugins_manager.clear_cache();
|
||||
let environments = self.services.turn_environments.snapshot().await;
|
||||
let hooks = build_hooks_for_config(
|
||||
config.as_ref(),
|
||||
self.services.plugins_manager.as_ref(),
|
||||
self.services.user_shell.as_ref(),
|
||||
environments.single_local_environment(),
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -3528,11 +3530,17 @@ pub(crate) fn emit_subagent_session_started(
|
||||
async fn build_hooks_for_config(
|
||||
config: &Config,
|
||||
plugins_manager: &PluginsManager,
|
||||
user_shell: &crate::shell::Shell,
|
||||
environment: Option<&TurnEnvironment>,
|
||||
) -> Hooks {
|
||||
let mut hook_shell_argv = user_shell.derive_exec_args("", /*use_login_shell*/ false);
|
||||
let hook_shell_program = hook_shell_argv.remove(0);
|
||||
let _ = hook_shell_argv.pop();
|
||||
let (hook_shell_program, hook_shell_argv) = environment
|
||||
.and_then(|environment| environment.shell.as_ref())
|
||||
.map(|shell| {
|
||||
let mut argv = shell.derive_exec_args("", /*use_login_shell*/ false);
|
||||
let program = argv.remove(0);
|
||||
let _ = argv.pop();
|
||||
(Some(program), argv)
|
||||
})
|
||||
.unwrap_or_default();
|
||||
let plugins_input = config.plugins_config_input();
|
||||
let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await;
|
||||
let plugin_hook_sources = plugin_outcome.effective_plugin_hook_sources();
|
||||
@@ -3544,7 +3552,7 @@ async fn build_hooks_for_config(
|
||||
config_layer_stack: Some(config.config_layer_stack.clone()),
|
||||
plugin_hook_sources,
|
||||
plugin_hook_load_warnings,
|
||||
shell_program: Some(hook_shell_program),
|
||||
shell_program: hook_shell_program,
|
||||
shell_args: hook_shell_argv,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -439,7 +439,7 @@ async fn warm_plugins_and_skills_for_session_init(
|
||||
config: Arc<Config>,
|
||||
plugins_manager: Arc<PluginsManager>,
|
||||
skills_manager: Arc<SkillsManager>,
|
||||
turn_environments: TurnEnvironmentSnapshot,
|
||||
turn_environments: &TurnEnvironmentSnapshot,
|
||||
) -> Vec<SkillError> {
|
||||
let fs = turn_environments.primary_filesystem();
|
||||
let plugins_input = config.plugins_config_input();
|
||||
@@ -829,7 +829,7 @@ impl Session {
|
||||
Arc::clone(&config),
|
||||
Arc::clone(&plugins_manager),
|
||||
Arc::clone(&skills_manager),
|
||||
resolved_environments,
|
||||
&resolved_environments,
|
||||
)
|
||||
.instrument(info_span!(
|
||||
"session_init.plugin_skill_warmup",
|
||||
@@ -913,8 +913,12 @@ impl Session {
|
||||
(None, None)
|
||||
};
|
||||
|
||||
let hooks =
|
||||
build_hooks_for_config(&config, plugins_manager.as_ref(), &default_shell).await;
|
||||
let hooks = build_hooks_for_config(
|
||||
&config,
|
||||
plugins_manager.as_ref(),
|
||||
resolved_environments.single_local_environment(),
|
||||
)
|
||||
.await;
|
||||
for warning in hooks.startup_warnings() {
|
||||
post_session_configured_events.push(Event {
|
||||
id: INITIAL_SUBMIT_ID.to_owned(),
|
||||
|
||||
@@ -29,6 +29,7 @@ use crate::turn_timing::now_unix_timestamp_ms;
|
||||
use crate::user_shell_command::user_shell_command_record_item;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
use codex_protocol::exec_output::StreamOutput;
|
||||
use codex_protocol::protocol::ErrorEvent;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::ExecCommandBeginEvent;
|
||||
use codex_protocol::protocol::ExecCommandEndEvent;
|
||||
@@ -124,18 +125,26 @@ pub(crate) async fn execute_user_shell_command(
|
||||
session.send_event(turn_context.as_ref(), event).await;
|
||||
}
|
||||
|
||||
// Execute the user's script under their default shell when known; this
|
||||
let Some((turn_environment, environment_shell)) = turn_context
|
||||
.environments
|
||||
.local()
|
||||
.and_then(|environment| environment.shell.as_ref().map(|shell| (environment, shell)))
|
||||
else {
|
||||
send_user_shell_error(
|
||||
&session,
|
||||
turn_context.as_ref(),
|
||||
"shell is unavailable in this session",
|
||||
)
|
||||
.await;
|
||||
return;
|
||||
};
|
||||
|
||||
// Execute the user's script under the environment's shell; this
|
||||
// allows commands that use shell features (pipes, &&, redirects, etc.).
|
||||
// We do not source rc files or otherwise reformat the script.
|
||||
let use_login_shell = true;
|
||||
let session_shell = session.user_shell();
|
||||
let environment = turn_context.environments.single_local_environment();
|
||||
let shell = environment
|
||||
.and_then(|environment| environment.shell.as_ref())
|
||||
.unwrap_or(session_shell.as_ref());
|
||||
let shell_snapshot_location =
|
||||
environment.and_then(|environment| environment.shell_snapshot(environment.cwd()));
|
||||
let display_command = shell.derive_exec_args(&command, use_login_shell);
|
||||
let display_command = environment_shell.derive_exec_args(&command, use_login_shell);
|
||||
let shell_snapshot_location = turn_environment.shell_snapshot(turn_environment.cwd());
|
||||
let mut exec_env_map = create_env(
|
||||
&turn_context.shell_environment_policy,
|
||||
Some(session.thread_id),
|
||||
@@ -145,7 +154,7 @@ pub(crate) async fn execute_user_shell_command(
|
||||
}
|
||||
let exec_command = prepare_user_shell_exec_command(
|
||||
&display_command,
|
||||
shell,
|
||||
environment_shell,
|
||||
shell_snapshot_location.as_ref(),
|
||||
&turn_context.shell_environment_policy.r#set,
|
||||
&mut exec_env_map,
|
||||
@@ -153,10 +162,7 @@ pub(crate) async fn execute_user_shell_command(
|
||||
|
||||
let call_id = Uuid::new_v4().to_string();
|
||||
let raw_command = command;
|
||||
#[allow(deprecated)]
|
||||
let cwd = environment
|
||||
.map(|environment| environment.cwd().clone())
|
||||
.unwrap_or_else(|| turn_context.cwd.clone());
|
||||
let cwd = turn_environment.cwd().clone();
|
||||
|
||||
let parsed_cmd = parse_command(&display_command);
|
||||
session
|
||||
@@ -341,9 +347,21 @@ pub(crate) async fn execute_user_shell_command(
|
||||
}
|
||||
}
|
||||
|
||||
async fn send_user_shell_error(session: &Session, turn_context: &TurnContext, message: &str) {
|
||||
session
|
||||
.send_event(
|
||||
turn_context,
|
||||
EventMsg::Error(ErrorEvent {
|
||||
message: message.to_string(),
|
||||
codex_error_info: None,
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
fn prepare_user_shell_exec_command(
|
||||
display_command: &[String],
|
||||
session_shell: &Shell,
|
||||
shell: &Shell,
|
||||
shell_snapshot: Option<&AbsolutePathBuf>,
|
||||
shell_environment_set: &HashMap<String, String>,
|
||||
exec_env_map: &mut HashMap<String, String>,
|
||||
@@ -352,7 +370,7 @@ fn prepare_user_shell_exec_command(
|
||||
{
|
||||
prepare_user_shell_exec_command_with_path_prepend(
|
||||
display_command,
|
||||
session_shell,
|
||||
shell,
|
||||
shell_snapshot,
|
||||
shell_environment_set,
|
||||
exec_env_map,
|
||||
@@ -364,7 +382,7 @@ fn prepare_user_shell_exec_command(
|
||||
{
|
||||
maybe_wrap_shell_lc_with_snapshot(
|
||||
display_command,
|
||||
session_shell,
|
||||
shell,
|
||||
shell_snapshot,
|
||||
shell_environment_set,
|
||||
exec_env_map,
|
||||
@@ -384,7 +402,7 @@ fn prepare_user_shell_exec_command(
|
||||
#[cfg(unix)]
|
||||
fn prepare_user_shell_exec_command_with_path_prepend(
|
||||
display_command: &[String],
|
||||
session_shell: &Shell,
|
||||
shell: &Shell,
|
||||
shell_snapshot: Option<&AbsolutePathBuf>,
|
||||
shell_environment_set: &HashMap<String, String>,
|
||||
exec_env_map: &mut HashMap<String, String>,
|
||||
@@ -395,7 +413,7 @@ fn prepare_user_shell_exec_command_with_path_prepend(
|
||||
prepend_runtime_path(exec_env_map, &mut runtime_path_prepends);
|
||||
maybe_wrap_shell_lc_with_snapshot(
|
||||
display_command,
|
||||
session_shell,
|
||||
shell,
|
||||
shell_snapshot,
|
||||
&explicit_env_overrides,
|
||||
exec_env_map,
|
||||
|
||||
@@ -322,7 +322,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
attempt.sandbox,
|
||||
attempt.windows_sandbox_level,
|
||||
);
|
||||
let command = if matches!(session_shell.shell_type, ShellType::PowerShell) {
|
||||
let command = if matches!(req.shell_type, ShellType::PowerShell) {
|
||||
prefix_powershell_script_with_utf8(&command)
|
||||
} else {
|
||||
command
|
||||
|
||||
@@ -21,6 +21,7 @@ use core_test_support::responses::mount_sse_once;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::submit_thread_settings;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::test_codex::turn_permission_fields;
|
||||
@@ -99,6 +100,40 @@ async fn user_shell_cmd_ls_and_cat_in_temp_dir() {
|
||||
assert_eq!(stdout, contents);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn user_shell_command_without_local_environment_emits_error() -> anyhow::Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let mut builder = test_codex();
|
||||
let test = builder.build(&server).await?;
|
||||
submit_thread_settings(
|
||||
&test.codex,
|
||||
codex_protocol::protocol::ThreadSettingsOverrides {
|
||||
environments: Some(codex_protocol::protocol::TurnEnvironmentSelections::new(
|
||||
test.config.cwd.clone(),
|
||||
vec![],
|
||||
)),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
|
||||
test.codex
|
||||
.submit(Op::RunUserShellCommand {
|
||||
command: "echo shell".to_string(),
|
||||
})
|
||||
.await?;
|
||||
|
||||
let EventMsg::Error(error) =
|
||||
wait_for_event(&test.codex, |event| matches!(event, EventMsg::Error(_))).await
|
||||
else {
|
||||
unreachable!()
|
||||
};
|
||||
assert_eq!(error.message, "shell is unavailable in this session");
|
||||
assert_eq!(error.codex_error_info, None);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn user_shell_cmd_can_be_interrupted() {
|
||||
// Set up isolated config and conversation.
|
||||
|
||||
Reference in New Issue
Block a user