mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add turn-scoped environment selections (#18416)
## Summary - add experimental turn/start.environments params for per-turn environment id + cwd selections - pass selections through core protocol ops and resolve them with EnvironmentManager before TurnContext creation - treat omitted selections as default behavior, empty selections as no environment, and non-empty selections as first environment/cwd as the turn primary ## Testing - ran `just fmt` - ran `just write-app-server-schema` - not run: unit tests for this stacked PR --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
6368f506b7
commit
1d4cc494c9
@@ -125,7 +125,7 @@ pub(super) async fn user_input_or_turn_inner(
|
||||
op: Op,
|
||||
mirror_user_text_to_realtime: Option<()>,
|
||||
) {
|
||||
let (items, updates, responsesapi_client_metadata) = match op {
|
||||
let (items, updates, responsesapi_client_metadata, environments) = match op {
|
||||
Op::UserTurn {
|
||||
cwd,
|
||||
approval_policy,
|
||||
@@ -139,6 +139,7 @@ pub(super) async fn user_input_or_turn_inner(
|
||||
items,
|
||||
collaboration_mode,
|
||||
personality,
|
||||
environments,
|
||||
} => {
|
||||
let collaboration_mode = collaboration_mode.or_else(|| {
|
||||
Some(CollaborationMode {
|
||||
@@ -167,10 +168,12 @@ pub(super) async fn user_input_or_turn_inner(
|
||||
app_server_client_version: None,
|
||||
},
|
||||
None,
|
||||
environments,
|
||||
)
|
||||
}
|
||||
Op::UserInput {
|
||||
items,
|
||||
environments,
|
||||
final_output_json_schema,
|
||||
responsesapi_client_metadata,
|
||||
} => (
|
||||
@@ -180,11 +183,15 @@ pub(super) async fn user_input_or_turn_inner(
|
||||
..Default::default()
|
||||
},
|
||||
responsesapi_client_metadata,
|
||||
environments,
|
||||
),
|
||||
_ => unreachable!(),
|
||||
};
|
||||
|
||||
let Ok(current_context) = sess.new_turn_with_sub_id(sub_id.clone(), updates).await else {
|
||||
let Ok(current_context) = sess
|
||||
.new_turn_with_sub_id(sub_id.clone(), updates, environments)
|
||||
.await
|
||||
else {
|
||||
// new_turn_with_sub_id already emits the error event.
|
||||
return;
|
||||
};
|
||||
|
||||
@@ -1027,6 +1027,7 @@ impl Session {
|
||||
self,
|
||||
self.next_internal_sub_id(),
|
||||
Op::UserInput {
|
||||
environments: None,
|
||||
items: vec![UserInput::Text {
|
||||
text,
|
||||
text_elements: Vec::new(),
|
||||
|
||||
@@ -110,6 +110,7 @@ pub(super) async fn spawn_review_thread(
|
||||
reasoning_summary,
|
||||
session_source,
|
||||
environment: parent_turn_context.environment.clone(),
|
||||
environments: parent_turn_context.environments.clone(),
|
||||
tools_config,
|
||||
features: parent_turn_context.features.clone(),
|
||||
ghost_snapshot: parent_turn_context.ghost_snapshot.clone(),
|
||||
|
||||
@@ -779,6 +779,7 @@ async fn new_turn_refreshes_managed_network_proxy_for_sandbox_change() -> anyhow
|
||||
sandbox_policy: Some(SandboxPolicy::DangerFullAccess),
|
||||
..Default::default()
|
||||
},
|
||||
/*environment_selections*/ None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1495,6 +1496,7 @@ async fn fork_startup_context_then_first_turn_diff_snapshot() -> anyhow::Result<
|
||||
initial
|
||||
.codex
|
||||
.submit(Op::UserInput {
|
||||
environments: None,
|
||||
items: vec![UserInput::Text {
|
||||
text: "fork seed".into(),
|
||||
text_elements: Vec::new(),
|
||||
@@ -1555,6 +1557,7 @@ async fn fork_startup_context_then_first_turn_diff_snapshot() -> anyhow::Result<
|
||||
forked
|
||||
.thread
|
||||
.submit(Op::UserInput {
|
||||
environments: None,
|
||||
items: vec![UserInput::Text {
|
||||
text: "after fork".into(),
|
||||
text_elements: Vec::new(),
|
||||
@@ -3021,7 +3024,8 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
inherited_shell_snapshot: None,
|
||||
user_shell_override: None,
|
||||
};
|
||||
let per_turn_config = Session::build_per_turn_config(&session_configuration);
|
||||
let per_turn_config =
|
||||
Session::build_per_turn_config(&session_configuration, session_configuration.cwd.clone());
|
||||
let model_info = ModelsManager::construct_model_info_offline_for_tests(
|
||||
session_configuration.collaboration_mode.model(),
|
||||
&per_turn_config.to_models_manager_config(),
|
||||
@@ -3137,6 +3141,8 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
&models_manager,
|
||||
/*network*/ None,
|
||||
Some(environment),
|
||||
/*environments*/ None,
|
||||
session_configuration.cwd.clone(),
|
||||
"turn_id".to_string(),
|
||||
Arc::clone(&js_repl),
|
||||
skills_outcome,
|
||||
@@ -3707,6 +3713,7 @@ fn op_kind_distinguishes_turn_ops() {
|
||||
);
|
||||
assert_eq!(
|
||||
Op::UserInput {
|
||||
environments: None,
|
||||
items: vec![],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
@@ -3725,6 +3732,7 @@ async fn user_turn_updates_approvals_reviewer() {
|
||||
&session,
|
||||
"sub-1".to_string(),
|
||||
Op::UserTurn {
|
||||
environments: None,
|
||||
items: vec![UserInput::Text {
|
||||
text: "hello".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
@@ -3751,6 +3759,133 @@ async fn user_turn_updates_approvals_reviewer() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn turn_environment_selection_sets_primary_environment() {
|
||||
let (session, _turn_context, _rx) = make_session_and_context_with_rx().await;
|
||||
let selected_cwd =
|
||||
AbsolutePathBuf::try_from(session.get_config().await.cwd.as_path().join("selected"))
|
||||
.expect("absolute path");
|
||||
|
||||
let turn_context = session
|
||||
.new_turn_with_sub_id(
|
||||
"sub-1".to_string(),
|
||||
SessionSettingsUpdate::default(),
|
||||
Some(vec![codex_protocol::protocol::TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: selected_cwd.clone(),
|
||||
}]),
|
||||
)
|
||||
.await
|
||||
.expect("turn should start");
|
||||
|
||||
let turn_environments = turn_context
|
||||
.environments
|
||||
.as_ref()
|
||||
.expect("turn environments should be recorded");
|
||||
assert_eq!(turn_environments.len(), 1);
|
||||
assert_eq!(turn_environments[0].environment_id, "local");
|
||||
assert!(std::sync::Arc::ptr_eq(
|
||||
turn_context
|
||||
.environment
|
||||
.as_ref()
|
||||
.expect("primary environment should be set"),
|
||||
&turn_environments[0].environment
|
||||
));
|
||||
assert_eq!(turn_context.cwd.as_path(), selected_cwd.as_path());
|
||||
assert_eq!(turn_context.config.cwd.as_path(), selected_cwd.as_path());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn multiple_turn_environment_selections_use_first_as_primary_environment() {
|
||||
let (session, _turn_context, _rx) = make_session_and_context_with_rx().await;
|
||||
let session_cwd = session.get_config().await.cwd.clone();
|
||||
let first_cwd =
|
||||
AbsolutePathBuf::try_from(session_cwd.as_path().join("first")).expect("absolute path");
|
||||
let second_cwd =
|
||||
AbsolutePathBuf::try_from(session_cwd.as_path().join("second")).expect("absolute path");
|
||||
|
||||
let turn_context = session
|
||||
.new_turn_with_sub_id(
|
||||
"sub-1".to_string(),
|
||||
SessionSettingsUpdate::default(),
|
||||
Some(vec![
|
||||
codex_protocol::protocol::TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: first_cwd.clone(),
|
||||
},
|
||||
codex_protocol::protocol::TurnEnvironmentSelection {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: second_cwd.clone(),
|
||||
},
|
||||
]),
|
||||
)
|
||||
.await
|
||||
.expect("turn should start");
|
||||
|
||||
let turn_environments = turn_context
|
||||
.environments
|
||||
.as_ref()
|
||||
.expect("turn environments should be recorded");
|
||||
assert_eq!(turn_environments.len(), 2);
|
||||
assert_eq!(turn_environments[0].cwd, first_cwd);
|
||||
assert_eq!(turn_environments[1].cwd, second_cwd);
|
||||
assert!(std::sync::Arc::ptr_eq(
|
||||
turn_context
|
||||
.environment
|
||||
.as_ref()
|
||||
.expect("primary environment should be set"),
|
||||
&turn_environments[0].environment
|
||||
));
|
||||
assert_eq!(turn_context.cwd, first_cwd);
|
||||
assert_eq!(turn_context.config.cwd, first_cwd);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn empty_turn_environment_selection_clears_primary_environment() {
|
||||
let (session, _turn_context, _rx) = make_session_and_context_with_rx().await;
|
||||
|
||||
let turn_context = session
|
||||
.new_turn_with_sub_id(
|
||||
"sub-1".to_string(),
|
||||
SessionSettingsUpdate::default(),
|
||||
Some(vec![]),
|
||||
)
|
||||
.await
|
||||
.expect("turn should start");
|
||||
|
||||
assert!(turn_context.environment.is_none());
|
||||
assert_eq!(turn_context.cwd, session.get_config().await.cwd);
|
||||
assert_eq!(turn_context.config.cwd, session.get_config().await.cwd);
|
||||
assert_eq!(
|
||||
turn_context
|
||||
.environments
|
||||
.as_ref()
|
||||
.expect("turn environments should be recorded")
|
||||
.len(),
|
||||
0
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unknown_turn_environment_selection_returns_error() {
|
||||
let (session, _turn_context, _rx) = make_session_and_context_with_rx().await;
|
||||
|
||||
let err = session
|
||||
.new_turn_with_sub_id(
|
||||
"sub-1".to_string(),
|
||||
SessionSettingsUpdate::default(),
|
||||
Some(vec![codex_protocol::protocol::TurnEnvironmentSelection {
|
||||
environment_id: "missing".to_string(),
|
||||
cwd: session.get_config().await.cwd.clone(),
|
||||
}]),
|
||||
)
|
||||
.await
|
||||
.expect_err("unknown environment should fail");
|
||||
|
||||
assert!(matches!(err, CodexErr::InvalidRequest(_)));
|
||||
assert!(err.to_string().contains("missing"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn spawn_task_turn_span_inherits_dispatch_trace_context() {
|
||||
struct TraceCaptureTask {
|
||||
@@ -4107,7 +4242,8 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
|
||||
inherited_shell_snapshot: None,
|
||||
user_shell_override: None,
|
||||
};
|
||||
let per_turn_config = Session::build_per_turn_config(&session_configuration);
|
||||
let per_turn_config =
|
||||
Session::build_per_turn_config(&session_configuration, session_configuration.cwd.clone());
|
||||
let model_info = ModelsManager::construct_model_info_offline_for_tests(
|
||||
session_configuration.collaboration_mode.model(),
|
||||
&per_turn_config.to_models_manager_config(),
|
||||
@@ -4223,6 +4359,8 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
|
||||
&models_manager,
|
||||
/*network*/ None,
|
||||
Some(environment),
|
||||
/*environments*/ None,
|
||||
session_configuration.cwd.clone(),
|
||||
"turn_id".to_string(),
|
||||
Arc::clone(&js_repl),
|
||||
skills_outcome,
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use super::*;
|
||||
use codex_model_provider::SharedModelProvider;
|
||||
use codex_model_provider::create_model_provider;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
|
||||
pub(super) fn image_generation_tool_auth_allowed(auth_manager: Option<&AuthManager>) -> bool {
|
||||
matches!(
|
||||
@@ -24,6 +25,14 @@ impl TurnSkillsContext {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug)]
|
||||
pub(crate) struct TurnEnvironment {
|
||||
#[allow(dead_code)]
|
||||
pub(crate) environment_id: String,
|
||||
pub(crate) environment: Arc<Environment>,
|
||||
pub(crate) cwd: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
/// The context needed for a single turn of the thread.
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct TurnContext {
|
||||
@@ -39,6 +48,7 @@ pub(crate) struct TurnContext {
|
||||
pub(crate) reasoning_summary: ReasoningSummaryConfig,
|
||||
pub(crate) session_source: SessionSource,
|
||||
pub(crate) environment: Option<Arc<Environment>>,
|
||||
pub(crate) environments: Option<Vec<TurnEnvironment>>,
|
||||
/// The session's absolute working directory. All relative paths provided
|
||||
/// by the model as well as sandbox policies are resolved against this path
|
||||
/// instead of `std::env::current_dir()`.
|
||||
@@ -168,6 +178,7 @@ impl TurnContext {
|
||||
reasoning_summary: self.reasoning_summary,
|
||||
session_source: self.session_source.clone(),
|
||||
environment: self.environment.clone(),
|
||||
environments: self.environments.clone(),
|
||||
cwd: self.cwd.clone(),
|
||||
current_date: self.current_date.clone(),
|
||||
timezone: self.timezone.clone(),
|
||||
@@ -300,11 +311,14 @@ fn local_time_context() -> (String, String) {
|
||||
|
||||
impl Session {
|
||||
/// Don't expand the number of mutated arguments on config. We are in the process of getting rid of it.
|
||||
pub(crate) fn build_per_turn_config(session_configuration: &SessionConfiguration) -> Config {
|
||||
pub(crate) fn build_per_turn_config(
|
||||
session_configuration: &SessionConfiguration,
|
||||
cwd: AbsolutePathBuf,
|
||||
) -> Config {
|
||||
// todo(aibrahim): store this state somewhere else so we don't need to mut config
|
||||
let config = session_configuration.original_config_do_not_use.clone();
|
||||
let mut per_turn_config = (*config).clone();
|
||||
per_turn_config.cwd = session_configuration.cwd.clone();
|
||||
per_turn_config.cwd = cwd;
|
||||
per_turn_config.model_reasoning_effort =
|
||||
session_configuration.collaboration_mode.reasoning_effort();
|
||||
per_turn_config.model_reasoning_summary = session_configuration.model_reasoning_summary;
|
||||
@@ -346,6 +360,8 @@ impl Session {
|
||||
models_manager: &ModelsManager,
|
||||
network: Option<NetworkProxy>,
|
||||
environment: Option<Arc<Environment>>,
|
||||
environments: Option<Vec<TurnEnvironment>>,
|
||||
cwd: AbsolutePathBuf,
|
||||
sub_id: String,
|
||||
js_repl: Arc<JsReplHandle>,
|
||||
skills_outcome: Arc<SkillLoadOutcome>,
|
||||
@@ -389,8 +405,6 @@ impl Session {
|
||||
&per_turn_config.agent_roles,
|
||||
));
|
||||
|
||||
let cwd = session_configuration.cwd.clone();
|
||||
|
||||
let per_turn_config = Arc::new(per_turn_config);
|
||||
let turn_metadata_state = Arc::new(TurnMetadataState::new(
|
||||
conversation_id.to_string(),
|
||||
@@ -414,6 +428,7 @@ impl Session {
|
||||
reasoning_summary,
|
||||
session_source,
|
||||
environment,
|
||||
environments,
|
||||
cwd,
|
||||
current_date: Some(current_date),
|
||||
timezone: Some(timezone),
|
||||
@@ -450,7 +465,22 @@ impl Session {
|
||||
&self,
|
||||
sub_id: String,
|
||||
updates: SessionSettingsUpdate,
|
||||
) -> ConstraintResult<Arc<TurnContext>> {
|
||||
environment_selections: Option<Vec<TurnEnvironmentSelection>>,
|
||||
) -> CodexResult<Arc<TurnContext>> {
|
||||
let turn_environments = match self.resolve_turn_environments(environment_selections) {
|
||||
Ok(turn_environments) => turn_environments,
|
||||
Err(err) => {
|
||||
self.send_event_raw(Event {
|
||||
id: sub_id.clone(),
|
||||
msg: EventMsg::Error(ErrorEvent {
|
||||
message: err.to_string(),
|
||||
codex_error_info: Some(CodexErrorInfo::BadRequest),
|
||||
}),
|
||||
})
|
||||
.await;
|
||||
return Err(err);
|
||||
}
|
||||
};
|
||||
let update_result = {
|
||||
let mut state = self.state.lock().await;
|
||||
match state.session_configuration.clone().apply(&updates) {
|
||||
@@ -482,15 +512,16 @@ impl Session {
|
||||
) = match update_result {
|
||||
Ok(update) => update,
|
||||
Err(err) => {
|
||||
let message = err.to_string();
|
||||
self.send_event_raw(Event {
|
||||
id: sub_id.clone(),
|
||||
msg: EventMsg::Error(ErrorEvent {
|
||||
message: err.to_string(),
|
||||
message: message.clone(),
|
||||
codex_error_info: Some(CodexErrorInfo::BadRequest),
|
||||
}),
|
||||
})
|
||||
.await;
|
||||
return Err(err);
|
||||
return Err(CodexErr::InvalidRequest(message));
|
||||
}
|
||||
};
|
||||
|
||||
@@ -511,17 +542,63 @@ impl Session {
|
||||
sub_id,
|
||||
session_configuration,
|
||||
updates.final_output_json_schema,
|
||||
turn_environments,
|
||||
)
|
||||
.await)
|
||||
}
|
||||
|
||||
fn resolve_turn_environments(
|
||||
&self,
|
||||
environment_selections: Option<Vec<TurnEnvironmentSelection>>,
|
||||
) -> CodexResult<Option<Vec<TurnEnvironment>>> {
|
||||
let Some(environment_selections) = environment_selections else {
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let mut turn_environments = Vec::with_capacity(environment_selections.len());
|
||||
for environment_selection in environment_selections {
|
||||
let environment = self
|
||||
.services
|
||||
.environment_manager
|
||||
.get_environment(&environment_selection.environment_id)
|
||||
.ok_or_else(|| {
|
||||
CodexErr::InvalidRequest(format!(
|
||||
"unknown turn environment id `{}`",
|
||||
environment_selection.environment_id
|
||||
))
|
||||
})?;
|
||||
let cwd = environment_selection.cwd;
|
||||
turn_environments.push(TurnEnvironment {
|
||||
environment_id: environment_selection.environment_id,
|
||||
environment,
|
||||
cwd,
|
||||
});
|
||||
}
|
||||
|
||||
Ok(Some(turn_environments))
|
||||
}
|
||||
|
||||
async fn new_turn_from_configuration(
|
||||
&self,
|
||||
sub_id: String,
|
||||
session_configuration: SessionConfiguration,
|
||||
final_output_json_schema: Option<Option<Value>>,
|
||||
turn_environments: Option<Vec<TurnEnvironment>>,
|
||||
) -> Arc<TurnContext> {
|
||||
let per_turn_config = Self::build_per_turn_config(&session_configuration);
|
||||
// `None` means use the thread's default environment. `Some([])` is an
|
||||
// explicit no-environment turn, so do not fall back in that case.
|
||||
let primary_turn_environment = turn_environments
|
||||
.as_ref()
|
||||
.and_then(|turn_environments| turn_environments.first());
|
||||
let environment = match primary_turn_environment {
|
||||
Some(turn_environment) => Some(Arc::clone(&turn_environment.environment)),
|
||||
None if turn_environments.is_some() => None,
|
||||
None => self.services.environment_manager.default_environment(),
|
||||
};
|
||||
let cwd = primary_turn_environment
|
||||
.map(|turn_environment| turn_environment.cwd.clone())
|
||||
.unwrap_or_else(|| session_configuration.cwd.clone());
|
||||
let per_turn_config = Self::build_per_turn_config(&session_configuration, cwd.clone());
|
||||
{
|
||||
let mcp_connection_manager = self.services.mcp_connection_manager.read().await;
|
||||
mcp_connection_manager.set_approval_policy(&session_configuration.approval_policy);
|
||||
@@ -544,7 +621,6 @@ impl Session {
|
||||
.await;
|
||||
let effective_skill_roots = plugin_outcome.effective_skill_roots();
|
||||
let skills_input = skills_load_input_from_config(&per_turn_config, effective_skill_roots);
|
||||
let environment = self.services.environment_manager.default_environment();
|
||||
let fs = environment
|
||||
.as_ref()
|
||||
.map(|environment| environment.get_filesystem());
|
||||
@@ -576,6 +652,8 @@ impl Session {
|
||||
.then(|| started_proxy.proxy())
|
||||
}),
|
||||
environment,
|
||||
turn_environments,
|
||||
cwd,
|
||||
sub_id,
|
||||
Arc::clone(&self.js_repl),
|
||||
skills_outcome,
|
||||
@@ -619,6 +697,7 @@ impl Session {
|
||||
sub_id,
|
||||
session_configuration,
|
||||
/*final_output_json_schema*/ None,
|
||||
/*turn_environments*/ None,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user