mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] preserve explicit environment cwd (#27995)
## Why `TurnEnvironmentSelections::new` rewrote the primary environment's explicit `cwd` to the legacy fallback cwd. For a remote-first selection, this could replace the remote working directory with a local fallback path and made the legacy cwd overlay authoritative over environment-owned state. ## What changed - Preserve every explicit environment cwd when constructing turn environment selections. - Keep `cwd`-only app-server updates compatible by rebuilding the default environment selections at the requested cwd. - Cover both explicit primary cwd preservation and cwd-only updates reaching the model-visible execution environment. ## Testing - `just test -p codex-core session_update_settings_does_not_rewrite_sticky_environment_cwds` - `just test -p codex-core environment_settings_preserve_explicit_primary_cwd` - `just test -p codex-app-server thread_settings_update_cwd_retargets_default_environment`
This commit is contained in:
committed by
GitHub
Unverified
parent
495da45643
commit
bd4fe31c5a
@@ -435,8 +435,9 @@ impl TurnRequestProcessor {
|
||||
let additional_context = map_additional_context(params.additional_context);
|
||||
let turn_has_input = !mapped_items.is_empty();
|
||||
let cwd = resolve_request_cwd(params.cwd)?;
|
||||
let environments =
|
||||
Self::build_environment_override(thread.as_ref(), cwd, environment_selections).await;
|
||||
let environments = self
|
||||
.build_environment_override(thread.as_ref(), cwd, environment_selections)
|
||||
.await;
|
||||
let thread_settings = self
|
||||
.build_thread_settings_overrides(
|
||||
thread.as_ref(),
|
||||
@@ -509,28 +510,36 @@ impl TurnRequestProcessor {
|
||||
}
|
||||
|
||||
async fn build_environment_override(
|
||||
&self,
|
||||
thread: &CodexThread,
|
||||
cwd: Option<AbsolutePathBuf>,
|
||||
environment_selections: Option<Vec<TurnEnvironmentSelection>>,
|
||||
) -> Option<TurnEnvironmentSelections> {
|
||||
if cwd.is_none() && environment_selections.is_none() {
|
||||
return None;
|
||||
match (cwd, environment_selections) {
|
||||
(None, None) => None,
|
||||
(Some(cwd), None) => {
|
||||
let environment_selections =
|
||||
self.thread_manager.default_environment_selections(&cwd);
|
||||
Some(TurnEnvironmentSelections::new(cwd, environment_selections))
|
||||
}
|
||||
(cwd, Some(environment_selections)) => {
|
||||
let legacy_fallback_cwd = match cwd {
|
||||
Some(cwd) => cwd,
|
||||
None => {
|
||||
let snapshot = thread.config_snapshot().await;
|
||||
environment_selections
|
||||
.iter()
|
||||
.find(|selection| selection.environment_id == LOCAL_ENVIRONMENT_ID)
|
||||
.and_then(|selection| selection.cwd.to_abs_path().ok())
|
||||
.unwrap_or_else(|| snapshot.cwd().clone())
|
||||
}
|
||||
};
|
||||
Some(TurnEnvironmentSelections::new(
|
||||
legacy_fallback_cwd,
|
||||
environment_selections,
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
let snapshot = thread.config_snapshot().await;
|
||||
let environment_selections =
|
||||
environment_selections.unwrap_or_else(|| snapshot.environment_selections().to_vec());
|
||||
let legacy_fallback_cwd = cwd.unwrap_or_else(|| {
|
||||
environment_selections
|
||||
.iter()
|
||||
.find(|selection| selection.environment_id == LOCAL_ENVIRONMENT_ID)
|
||||
.and_then(|selection| selection.cwd.to_abs_path().ok())
|
||||
.unwrap_or_else(|| snapshot.cwd().clone())
|
||||
});
|
||||
Some(TurnEnvironmentSelections::new(
|
||||
legacy_fallback_cwd,
|
||||
environment_selections,
|
||||
))
|
||||
}
|
||||
|
||||
async fn build_thread_settings_overrides(
|
||||
@@ -694,12 +703,9 @@ impl TurnRequestProcessor {
|
||||
) -> Result<ThreadSettingsUpdateResponse, JSONRPCErrorError> {
|
||||
let (_, thread) = self.load_thread(¶ms.thread_id).await?;
|
||||
let cwd = resolve_request_cwd(params.cwd)?;
|
||||
let environments = Self::build_environment_override(
|
||||
thread.as_ref(),
|
||||
cwd,
|
||||
/*environment_selections*/ None,
|
||||
)
|
||||
.await;
|
||||
let environments = self
|
||||
.build_environment_override(thread.as_ref(), cwd, /*environment_selections*/ None)
|
||||
.await;
|
||||
let thread_settings = self
|
||||
.build_thread_settings_overrides(
|
||||
thread.as_ref(),
|
||||
|
||||
@@ -94,6 +94,59 @@ async fn thread_settings_update_emits_notification_and_updates_future_turns() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_settings_update_cwd_retargets_default_environment() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
let body = responses::sse(vec![
|
||||
responses::ev_response_created("resp-1"),
|
||||
responses::ev_assistant_message("msg-1", "done"),
|
||||
responses::ev_completed("resp-1"),
|
||||
]);
|
||||
let response_mock = responses::mount_sse_once(&server, body).await;
|
||||
let codex_home = TempDir::new()?;
|
||||
let workspace = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = TestAppServer::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??;
|
||||
let thread = start_thread(&mut mcp).await?.thread;
|
||||
|
||||
send_thread_settings_update(
|
||||
&mut mcp,
|
||||
ThreadSettingsUpdateParams {
|
||||
thread_id: thread.id.clone(),
|
||||
cwd: Some(workspace.path().to_path_buf()),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
let updated = read_thread_settings_updated(&mut mcp).await?;
|
||||
assert_eq!(updated.thread_settings.cwd.as_path(), workspace.path());
|
||||
|
||||
start_text_turn(&mut mcp, thread.id).await?;
|
||||
timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/completed"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
let environment_context = response_mock
|
||||
.single_request()
|
||||
.message_input_texts("user")
|
||||
.into_iter()
|
||||
.find(|text| text.starts_with("<environment_context>"))
|
||||
.context("environment context should be model visible")?;
|
||||
assert!(
|
||||
environment_context.contains(&format!(
|
||||
"<cwd>{}</cwd>",
|
||||
workspace.path().to_string_lossy()
|
||||
)),
|
||||
"default environment should use the updated cwd: {environment_context}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_settings_update_while_turn_is_active_emits_notification() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
|
||||
@@ -4615,6 +4615,7 @@ async fn session_update_settings_does_not_rewrite_sticky_environment_cwds() {
|
||||
.environment_selections()
|
||||
.to_vec()
|
||||
};
|
||||
let expected_environments = current_environments.clone();
|
||||
std::fs::create_dir_all(updated_cwd.as_path()).expect("create project dir");
|
||||
|
||||
session
|
||||
@@ -4628,21 +4629,28 @@ async fn session_update_settings_does_not_rewrite_sticky_environment_cwds() {
|
||||
.await
|
||||
.expect("cwd update should succeed");
|
||||
|
||||
let session_cwd = {
|
||||
let (session_cwd, stored_environments) = {
|
||||
let state = session.state.lock().await;
|
||||
state.session_configuration.cwd().clone()
|
||||
(
|
||||
state.session_configuration.cwd().clone(),
|
||||
state
|
||||
.session_configuration
|
||||
.environment_selections()
|
||||
.to_vec(),
|
||||
)
|
||||
};
|
||||
let config = session.get_config().await;
|
||||
let next_turn = session.new_default_turn().await;
|
||||
|
||||
assert_eq!(session_cwd, updated_cwd);
|
||||
assert_eq!(stored_environments, expected_environments);
|
||||
#[allow(deprecated)]
|
||||
let turn_cwd = turn_context.cwd.clone();
|
||||
#[allow(deprecated)]
|
||||
let next_turn_cwd = next_turn.cwd.clone();
|
||||
assert_eq!(config.cwd, turn_cwd);
|
||||
assert_eq!(next_turn_cwd, updated_cwd);
|
||||
assert_eq!(next_turn.config.cwd, updated_cwd);
|
||||
assert_eq!(next_turn_cwd, turn_cwd);
|
||||
assert_eq!(next_turn.config.cwd, turn_cwd);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -4678,7 +4686,7 @@ async fn relative_cwd_update_without_environments_resolves_under_session_cwd() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn cwd_update_rewrites_sticky_environment_cwd() {
|
||||
async fn environment_settings_preserve_explicit_primary_cwd() {
|
||||
let (session, _turn_context) = make_session_and_context().await;
|
||||
let (original_cwd, environment_cwd, environments) = {
|
||||
let mut state = session.state.lock().await;
|
||||
@@ -4706,9 +4714,8 @@ 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,
|
||||
PathUri::from_abs_path(&updated_cwd)
|
||||
PathUri::from_abs_path(&environment_cwd)
|
||||
);
|
||||
assert_ne!(environment_cwd, updated_cwd);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -106,7 +106,6 @@ pub const COLLABORATION_MODE_CLOSE_TAG: &str = "</collaboration_mode>";
|
||||
pub const REALTIME_CONVERSATION_OPEN_TAG: &str = "<realtime_conversation>";
|
||||
pub const REALTIME_CONVERSATION_CLOSE_TAG: &str = "</realtime_conversation>";
|
||||
pub const USER_MESSAGE_BEGIN: &str = "## My request for Codex:";
|
||||
const LOCAL_ENVIRONMENT_ID: &str = "local";
|
||||
|
||||
// TODO(anp): Replace `TurnEnvironmentSelection` with `PathUri` once path URIs carry environment
|
||||
// identifiers.
|
||||
@@ -127,22 +126,9 @@ impl TurnEnvironmentSelections {
|
||||
legacy_fallback_cwd: AbsolutePathBuf,
|
||||
environments: Vec<TurnEnvironmentSelection>,
|
||||
) -> Self {
|
||||
let mut settings = Self {
|
||||
Self {
|
||||
legacy_fallback_cwd,
|
||||
environments,
|
||||
};
|
||||
settings.sync_primary_environment_cwd();
|
||||
settings
|
||||
}
|
||||
|
||||
fn sync_primary_environment_cwd(&mut self) {
|
||||
let legacy_fallback_cwd = PathUri::from_abs_path(&self.legacy_fallback_cwd);
|
||||
// Keep remote environments' native cwd instead of replacing it with the local fallback.
|
||||
if let Some(turn_environment) = self.environments.first_mut()
|
||||
&& turn_environment.environment_id == LOCAL_ENVIRONMENT_ID
|
||||
&& turn_environment.cwd != legacy_fallback_cwd
|
||||
{
|
||||
turn_environment.cwd = legacy_fallback_cwd;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user