mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Avoid hard-coded environment context shell (#21390)
## Summary - make resolved turn environment shell metadata optional instead of hard-coding bash - render environment context shells from explicit environment metadata when present, falling back to the existing session shell - update environment context tests for inherited PowerShell-style fallback and explicit per-environment shell override ## Testing - Not run (not requested; formatted with `just fmt`). Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
f9063045e1
commit
63a27ad6c6
@@ -1,4 +1,6 @@
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::shell::Shell;
|
||||
use codex_protocol::protocol::TurnContextItem;
|
||||
use codex_protocol::protocol::TurnContextNetworkItem;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -30,15 +32,16 @@ impl EnvironmentContextEnvironment {
|
||||
}
|
||||
}
|
||||
|
||||
fn from_turn_environments(
|
||||
environments: &[crate::session::turn_context::TurnEnvironment],
|
||||
) -> Vec<Self> {
|
||||
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.clone(),
|
||||
shell: environment
|
||||
.shell
|
||||
.clone()
|
||||
.unwrap_or_else(|| shell.name().to_string()),
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
@@ -174,10 +177,11 @@ impl EnvironmentContext {
|
||||
)
|
||||
}
|
||||
|
||||
pub(crate) fn from_turn_context(turn_context: &TurnContext) -> Self {
|
||||
pub(crate) fn from_turn_context(turn_context: &TurnContext, shell: &Shell) -> Self {
|
||||
Self::new(
|
||||
EnvironmentContextEnvironment::from_turn_environments(
|
||||
&turn_context.environments.turn_environments,
|
||||
shell,
|
||||
),
|
||||
turn_context.current_date.clone(),
|
||||
turn_context.timezone.clone(),
|
||||
|
||||
@@ -259,3 +259,46 @@ fn serialize_environment_context_with_multiple_selected_environments() {
|
||||
|
||||
assert_eq!(context.render(), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn serialize_environment_context_prefers_environment_shell_when_present() {
|
||||
let local_cwd = test_path_buf("/repo/local");
|
||||
let remote_cwd = test_path_buf("/repo/remote");
|
||||
let context = EnvironmentContext::new(
|
||||
vec![
|
||||
EnvironmentContextEnvironment {
|
||||
id: "local".to_string(),
|
||||
cwd: local_cwd.abs(),
|
||||
shell: "powershell".to_string(),
|
||||
},
|
||||
EnvironmentContextEnvironment {
|
||||
id: "remote".to_string(),
|
||||
cwd: remote_cwd.abs(),
|
||||
shell: "cmd".to_string(),
|
||||
},
|
||||
],
|
||||
/*current_date*/ None,
|
||||
/*timezone*/ None,
|
||||
/*network*/ None,
|
||||
/*subagents*/ None,
|
||||
);
|
||||
|
||||
let expected = format!(
|
||||
r#"<environment_context>
|
||||
<environments>
|
||||
<environment id="local">
|
||||
<cwd>{}</cwd>
|
||||
<shell>powershell</shell>
|
||||
</environment>
|
||||
<environment id="remote">
|
||||
<cwd>{}</cwd>
|
||||
<shell>cmd</shell>
|
||||
</environment>
|
||||
</environments>
|
||||
</environment_context>"#,
|
||||
local_cwd.display(),
|
||||
remote_cwd.display()
|
||||
);
|
||||
|
||||
assert_eq!(context.render(), expected);
|
||||
}
|
||||
|
||||
@@ -29,7 +29,7 @@ fn build_environment_update_item(
|
||||
|
||||
let prev = previous?;
|
||||
let prev_context = EnvironmentContext::from_turn_context_item(prev, shell.name().to_string());
|
||||
let next_context = EnvironmentContext::from_turn_context(next);
|
||||
let next_context = EnvironmentContext::from_turn_context(next, shell);
|
||||
if prev_context.equals_except_shell(&next_context) {
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -75,9 +75,7 @@ pub(crate) fn resolve_environment_selections(
|
||||
environment_id,
|
||||
environment,
|
||||
cwd: selected_environment.cwd.clone(),
|
||||
// TODO(starr): Resolve shell metadata per environment instead of
|
||||
// hardcoding bash.
|
||||
shell: "bash".to_string(),
|
||||
shell: None,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -176,5 +174,6 @@ mod tests {
|
||||
.environment_id,
|
||||
"local"
|
||||
);
|
||||
assert_eq!(resolved.primary().expect("primary environment").shell, None);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2703,13 +2703,14 @@ impl Session {
|
||||
);
|
||||
}
|
||||
if turn_context.config.include_environment_context {
|
||||
let shell = self.user_shell();
|
||||
let subagents = self
|
||||
.services
|
||||
.agent_control
|
||||
.format_environment_context_subagents(self.conversation_id)
|
||||
.await;
|
||||
contextual_user_sections.push(
|
||||
crate::context::EnvironmentContext::from_turn_context(turn_context)
|
||||
crate::context::EnvironmentContext::from_turn_context(turn_context, shell.as_ref())
|
||||
.with_subagents(subagents)
|
||||
.render(),
|
||||
);
|
||||
|
||||
@@ -3057,7 +3057,7 @@ fn turn_environments_for_tests(
|
||||
environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
environment: Arc::clone(environment),
|
||||
cwd: cwd.clone(),
|
||||
shell: "bash".to_string(),
|
||||
shell: None,
|
||||
}]
|
||||
}
|
||||
|
||||
@@ -4803,7 +4803,7 @@ async fn primary_environment_uses_first_turn_environment() {
|
||||
environment_id: "second".to_string(),
|
||||
environment: Arc::clone(&first_environment.environment),
|
||||
cwd: second_cwd.clone(),
|
||||
shell: "bash".to_string(),
|
||||
shell: None,
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
@@ -5749,6 +5749,47 @@ async fn build_settings_update_items_emits_environment_item_for_network_changes(
|
||||
assert!(environment_update.contains("<denied>blocked.example.com</denied>"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn environment_context_uses_session_shell_when_environment_shell_is_absent() {
|
||||
let (mut session, mut turn_context) = make_session_and_context().await;
|
||||
session.services.user_shell = Arc::new(crate::shell::Shell {
|
||||
shell_type: crate::shell::ShellType::PowerShell,
|
||||
shell_path: PathBuf::from("powershell"),
|
||||
shell_snapshot: crate::shell::empty_shell_snapshot_receiver(),
|
||||
});
|
||||
for environment in &mut turn_context.environments.turn_environments {
|
||||
environment.shell = None;
|
||||
}
|
||||
|
||||
let session_shell = session.user_shell();
|
||||
let environment_context = crate::context::EnvironmentContext::from_turn_context(
|
||||
&turn_context,
|
||||
session_shell.as_ref(),
|
||||
)
|
||||
.render();
|
||||
assert!(
|
||||
environment_context.contains("<shell>powershell</shell>"),
|
||||
"{environment_context}"
|
||||
);
|
||||
|
||||
let primary_environment = turn_context
|
||||
.environments
|
||||
.turn_environments
|
||||
.first_mut()
|
||||
.expect("primary environment");
|
||||
primary_environment.shell = Some("cmd".to_string());
|
||||
|
||||
let environment_context = crate::context::EnvironmentContext::from_turn_context(
|
||||
&turn_context,
|
||||
session_shell.as_ref(),
|
||||
)
|
||||
.render();
|
||||
assert!(
|
||||
environment_context.contains("<shell>cmd</shell>"),
|
||||
"{environment_context}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_settings_update_items_emits_environment_item_for_time_changes() {
|
||||
let (session, previous_context) = make_session_and_context().await;
|
||||
|
||||
@@ -39,7 +39,7 @@ pub(crate) struct TurnEnvironment {
|
||||
pub(crate) environment_id: String,
|
||||
pub(crate) environment: Arc<Environment>,
|
||||
pub(crate) cwd: AbsolutePathBuf,
|
||||
pub(crate) shell: String,
|
||||
pub(crate) shell: Option<String>,
|
||||
}
|
||||
|
||||
impl TurnEnvironment {
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
#![allow(clippy::unwrap_used)]
|
||||
|
||||
use codex_apply_patch::APPLY_PATCH_TOOL_INSTRUCTIONS;
|
||||
use codex_core::shell::default_user_shell;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::config_types::CollaborationMode;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
@@ -54,7 +55,7 @@ fn assert_default_env_context(text: &str, cwd: &str) {
|
||||
"expected cwd in environment context: {text}"
|
||||
);
|
||||
assert!(
|
||||
text.contains("<shell>bash</shell>"),
|
||||
text.contains(&format!("<shell>{}</shell>", default_user_shell().name())),
|
||||
"expected shell in environment context: {text}"
|
||||
);
|
||||
assert!(
|
||||
|
||||
Reference in New Issue
Block a user