mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
Use optional default environment for disabled mode
Restore CODEX_EXEC_SERVER_URL=none semantics by making EnvironmentManager::default_environment return None when environment access is disabled. Remove the separate disabled-for-agent flag and derive tool availability from the optional default environment. Add an end-to-end tool exposure test for CODEX_EXEC_SERVER_URL=none. Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -5698,7 +5698,8 @@ impl CodexMessageProcessor {
|
||||
let environment = self
|
||||
.thread_manager
|
||||
.environment_manager()
|
||||
.default_environment();
|
||||
.default_environment()
|
||||
.unwrap_or_else(|| Arc::new(codex_exec_server::Environment::default()));
|
||||
// Status listing has no turn cwd. This fallback is used only
|
||||
// by executor-backed stdio MCPs whose config omits `cwd`.
|
||||
McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf())
|
||||
@@ -5855,7 +5856,8 @@ impl CodexMessageProcessor {
|
||||
let environment = self
|
||||
.thread_manager
|
||||
.environment_manager()
|
||||
.default_environment();
|
||||
.default_environment()
|
||||
.unwrap_or_else(|| Arc::new(codex_exec_server::Environment::default()));
|
||||
// Resource reads without a thread have no turn cwd. This fallback
|
||||
// is used only by executor-backed stdio MCPs whose config omits `cwd`.
|
||||
McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf())
|
||||
@@ -6449,8 +6451,7 @@ impl CodexMessageProcessor {
|
||||
.thread_manager
|
||||
.environment_manager()
|
||||
.default_environment()
|
||||
.get_filesystem();
|
||||
let fs = Some(fs);
|
||||
.map(|environment| environment.get_filesystem());
|
||||
let mut data = Vec::new();
|
||||
for cwd in cwds {
|
||||
let extra_roots = extra_roots_by_cwd
|
||||
|
||||
@@ -461,7 +461,9 @@ impl Codex {
|
||||
let (tx_event, rx_event) = async_channel::unbounded();
|
||||
|
||||
let environment = environment_manager.default_environment();
|
||||
let fs = Some(environment.get_filesystem());
|
||||
let fs = environment
|
||||
.as_ref()
|
||||
.map(|environment| environment.get_filesystem());
|
||||
let plugin_outcome = plugins_manager.plugins_for_config(&config).await;
|
||||
let effective_skill_roots = plugin_outcome.effective_skill_roots();
|
||||
let skills_input = skills_load_input_from_config(&config, effective_skill_roots);
|
||||
|
||||
@@ -641,8 +641,6 @@ impl Session {
|
||||
Arc::clone(&auth_manager),
|
||||
session_configuration.session_source.clone(),
|
||||
));
|
||||
let environment = environment_manager.default_environment();
|
||||
let allows_agent_environment_access = environment_manager.allows_agent_environment_access();
|
||||
let services = SessionServices {
|
||||
// Initialize the MCP connection manager with an uninitialized
|
||||
// instance. It will be replaced with one created via
|
||||
@@ -698,8 +696,6 @@ impl Session {
|
||||
config.js_repl_node_path.clone(),
|
||||
),
|
||||
environment_manager,
|
||||
environment: Some(environment),
|
||||
allows_agent_environment_access,
|
||||
};
|
||||
services
|
||||
.model_client
|
||||
|
||||
@@ -3245,8 +3245,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
config.js_repl_node_path.clone(),
|
||||
),
|
||||
environment_manager: Arc::new(codex_exec_server::EnvironmentManager::default()),
|
||||
environment: Some(Arc::clone(&environment)),
|
||||
allows_agent_environment_access: true,
|
||||
};
|
||||
let js_repl = Arc::new(JsReplHandle::with_node_path(
|
||||
config.js_repl_node_path.clone(),
|
||||
@@ -3280,7 +3278,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
model_info,
|
||||
&models_manager,
|
||||
/*network*/ None,
|
||||
/*allows_agent_environment_access*/ true,
|
||||
Some(environment),
|
||||
"turn_id".to_string(),
|
||||
Arc::clone(&js_repl),
|
||||
@@ -4346,8 +4343,6 @@ where
|
||||
config.js_repl_node_path.clone(),
|
||||
),
|
||||
environment_manager: Arc::new(codex_exec_server::EnvironmentManager::default()),
|
||||
environment: Some(Arc::clone(&environment)),
|
||||
allows_agent_environment_access: true,
|
||||
};
|
||||
let js_repl = Arc::new(JsReplHandle::with_node_path(
|
||||
config.js_repl_node_path.clone(),
|
||||
@@ -4381,7 +4376,6 @@ where
|
||||
model_info,
|
||||
&models_manager,
|
||||
/*network*/ None,
|
||||
/*allows_agent_environment_access*/ true,
|
||||
Some(environment),
|
||||
"turn_id".to_string(),
|
||||
Arc::clone(&js_repl),
|
||||
|
||||
@@ -345,7 +345,6 @@ impl Session {
|
||||
model_info: ModelInfo,
|
||||
models_manager: &ModelsManager,
|
||||
network: Option<NetworkProxy>,
|
||||
allows_agent_environment_access: bool,
|
||||
environment: Option<Arc<Environment>>,
|
||||
sub_id: String,
|
||||
js_repl: Arc<JsReplHandle>,
|
||||
@@ -382,7 +381,7 @@ impl Session {
|
||||
)
|
||||
.with_web_search_config(per_turn_config.web_search_config.clone())
|
||||
.with_allow_login_shell(per_turn_config.permissions.allow_login_shell)
|
||||
.with_has_environment(allows_agent_environment_access)
|
||||
.with_has_environment(environment.is_some())
|
||||
.with_spawn_agent_usage_hint(per_turn_config.multi_agent_v2.usage_hint_enabled)
|
||||
.with_spawn_agent_usage_hint_text(per_turn_config.multi_agent_v2.usage_hint_text.clone())
|
||||
.with_hide_spawn_agent_metadata(per_turn_config.multi_agent_v2.hide_spawn_agent_metadata)
|
||||
@@ -545,9 +544,8 @@ 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 fs = self
|
||||
.services
|
||||
.environment
|
||||
let environment = self.services.environment_manager.default_environment();
|
||||
let fs = environment
|
||||
.as_ref()
|
||||
.map(|environment| environment.get_filesystem());
|
||||
let skills_outcome = Arc::new(
|
||||
@@ -577,8 +575,7 @@ impl Session {
|
||||
)
|
||||
.then(|| started_proxy.proxy())
|
||||
}),
|
||||
self.services.allows_agent_environment_access,
|
||||
self.services.environment.clone(),
|
||||
environment,
|
||||
sub_id,
|
||||
Arc::clone(&self.js_repl),
|
||||
skills_outcome,
|
||||
|
||||
@@ -17,7 +17,6 @@ use crate::tools::network_approval::NetworkApprovalService;
|
||||
use crate::tools::sandboxing::ApprovalStore;
|
||||
use crate::unified_exec::UnifiedExecProcessManager;
|
||||
use codex_analytics::AnalyticsEventsClient;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_hooks::Hooks;
|
||||
use codex_login::AuthManager;
|
||||
@@ -68,6 +67,4 @@ pub(crate) struct SessionServices {
|
||||
/// Shared process-level environment registry. Sessions carry an `Arc` handle so they can pass
|
||||
/// the same manager through child-thread spawn paths without reconstructing it.
|
||||
pub(crate) environment_manager: Arc<EnvironmentManager>,
|
||||
pub(crate) environment: Option<Arc<Environment>>,
|
||||
pub(crate) allows_agent_environment_access: bool,
|
||||
}
|
||||
|
||||
@@ -923,8 +923,8 @@ impl ThreadManagerState {
|
||||
user_shell_override: Option<crate::shell::Shell>,
|
||||
) -> CodexResult<NewThread> {
|
||||
let environment = self.environment_manager.default_environment();
|
||||
let watch_registration = match environment.is_remote() {
|
||||
false => {
|
||||
let watch_registration = match environment.as_ref() {
|
||||
Some(environment) if !environment.is_remote() => {
|
||||
self.skills_watcher
|
||||
.register_config(
|
||||
&config,
|
||||
@@ -934,7 +934,7 @@ impl ThreadManagerState {
|
||||
)
|
||||
.await
|
||||
}
|
||||
true => crate::file_watcher::WatchRegistration::default(),
|
||||
Some(_) | None => crate::file_watcher::WatchRegistration::default(),
|
||||
};
|
||||
let CodexSpawnOk {
|
||||
codex, thread_id, ..
|
||||
|
||||
@@ -204,6 +204,7 @@ pub struct TestCodexBuilder {
|
||||
workspace_setups: Vec<Box<WorkspaceSetup>>,
|
||||
home: Option<Arc<TempDir>>,
|
||||
user_shell_override: Option<Shell>,
|
||||
exec_server_url: Option<String>,
|
||||
}
|
||||
|
||||
impl TestCodexBuilder {
|
||||
@@ -255,6 +256,11 @@ impl TestCodexBuilder {
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_exec_server_url(mut self, exec_server_url: impl Into<String>) -> Self {
|
||||
self.exec_server_url = Some(exec_server_url.into());
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_windows_cmd_shell(self) -> Self {
|
||||
if cfg!(windows) {
|
||||
self.with_user_shell(get_shell_by_model_provided_path(&PathBuf::from("cmd.exe")))
|
||||
@@ -350,9 +356,13 @@ impl TestCodexBuilder {
|
||||
let (config, fallback_cwd) = self
|
||||
.prepare_config(base_url, &home, test_env.cwd().clone())
|
||||
.await?;
|
||||
let exec_server_url = self
|
||||
.exec_server_url
|
||||
.clone()
|
||||
.or_else(|| test_env.exec_server_url().map(str::to_owned));
|
||||
let environment_manager = Arc::new(codex_exec_server::EnvironmentManager::new(
|
||||
codex_exec_server::EnvironmentManagerArgs {
|
||||
exec_server_url: test_env.exec_server_url().map(str::to_owned),
|
||||
exec_server_url,
|
||||
local_runtime_paths: None,
|
||||
},
|
||||
));
|
||||
@@ -888,6 +898,7 @@ pub fn test_codex() -> TestCodexBuilder {
|
||||
workspace_setups: vec![],
|
||||
home: None,
|
||||
user_shell_override: None,
|
||||
exec_server_url: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -75,6 +75,66 @@ fn ev_namespaced_function_call(
|
||||
})
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn exec_server_none_omits_environment_backed_tools() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let response_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_exec_server_url("none")
|
||||
.with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("unified exec should enable for test");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::JsRepl)
|
||||
.expect("js repl should enable for test");
|
||||
config.include_apply_patch_tool = true;
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn("which tools are available?").await?;
|
||||
|
||||
let tools = tool_names(&response_mock.single_request().body_json());
|
||||
assert!(
|
||||
tools.contains(&"update_plan".to_string()),
|
||||
"non-environment tool should remain available; got {tools:?}"
|
||||
);
|
||||
for environment_tool in [
|
||||
"exec_command",
|
||||
"write_stdin",
|
||||
"js_repl",
|
||||
"js_repl_reset",
|
||||
"apply_patch",
|
||||
"view_image",
|
||||
] {
|
||||
assert!(
|
||||
!tools.contains(&environment_tool.to_string()),
|
||||
"{environment_tool} should be omitted when CODEX_EXEC_SERVER_URL=none; got {tools:?}"
|
||||
);
|
||||
}
|
||||
assert!(
|
||||
test.thread_manager
|
||||
.environment_manager()
|
||||
.default_environment()
|
||||
.is_none()
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn custom_tool_unknown_returns_custom_output_error() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -16,25 +16,22 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL";
|
||||
/// Owns the execution/filesystem environments available to a session.
|
||||
///
|
||||
/// `EnvironmentManager` is the session-scoped registry for concrete
|
||||
/// environments. It always creates a local environment under [`LOCAL_ENVIRONMENT_ID`].
|
||||
/// When `CODEX_EXEC_SERVER_URL` is set to a websocket URL, it also creates a
|
||||
/// remote environment under [`REMOTE_ENVIRONMENT_ID`] and makes that the default
|
||||
/// environment. Otherwise the local environment is the default.
|
||||
/// environments. It creates a local environment under [`LOCAL_ENVIRONMENT_ID`]
|
||||
/// unless environment access is disabled. When `CODEX_EXEC_SERVER_URL` is set to
|
||||
/// a websocket URL, it also creates a remote environment under
|
||||
/// [`REMOTE_ENVIRONMENT_ID`] and makes that the default environment. Otherwise
|
||||
/// the local environment is the default.
|
||||
///
|
||||
/// Setting `CODEX_EXEC_SERVER_URL=none` does not remove the local environment:
|
||||
/// Codex internals may still use `default_environment()` or explicit
|
||||
/// `get_environment()` lookups.
|
||||
/// Instead it disables agent/tool access as reported by
|
||||
/// `allows_agent_environment_access()`, so shell/filesystem tools can be hidden
|
||||
/// from the model while internal local filesystem access still works.
|
||||
/// Setting `CODEX_EXEC_SERVER_URL=none` disables environment access by leaving
|
||||
/// the default environment unset. Callers use `default_environment().is_some()`
|
||||
/// as the signal for model-facing shell/filesystem tool availability.
|
||||
///
|
||||
/// Remote environments hold a lazy exec-server client handle. The websocket is
|
||||
/// not opened when the manager or environment is constructed; it connects on the
|
||||
/// first remote exec or filesystem operation.
|
||||
#[derive(Debug)]
|
||||
pub struct EnvironmentManager {
|
||||
default_environment: String,
|
||||
environment_disabled_for_agent: bool,
|
||||
default_environment: Option<String>,
|
||||
environments: HashMap<String, Arc<Environment>>,
|
||||
}
|
||||
|
||||
@@ -69,54 +66,50 @@ impl EnvironmentManager {
|
||||
exec_server_url,
|
||||
local_runtime_paths,
|
||||
} = args;
|
||||
let (exec_server_url, environment_disabled_for_agent) =
|
||||
normalize_exec_server_url(exec_server_url);
|
||||
let (exec_server_url, environment_disabled) = normalize_exec_server_url(exec_server_url);
|
||||
let mut environments = HashMap::new();
|
||||
environments.insert(
|
||||
LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
Arc::new(
|
||||
Environment::create_with_runtime_paths(
|
||||
/*exec_server_url*/ None,
|
||||
local_runtime_paths.clone(),
|
||||
)
|
||||
.expect("valid local environment"),
|
||||
),
|
||||
);
|
||||
|
||||
let default_environment = match exec_server_url {
|
||||
Some(exec_server_url) => {
|
||||
environments.insert(
|
||||
REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
Arc::new(
|
||||
Environment::create_with_runtime_paths(
|
||||
Some(exec_server_url),
|
||||
local_runtime_paths,
|
||||
)
|
||||
.expect("valid remote environment"),
|
||||
),
|
||||
);
|
||||
REMOTE_ENVIRONMENT_ID.to_string()
|
||||
let default_environment = if environment_disabled {
|
||||
None
|
||||
} else {
|
||||
environments.insert(
|
||||
LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
Arc::new(
|
||||
Environment::create_with_runtime_paths(
|
||||
/*exec_server_url*/ None,
|
||||
local_runtime_paths.clone(),
|
||||
)
|
||||
.expect("valid local environment"),
|
||||
),
|
||||
);
|
||||
match exec_server_url {
|
||||
Some(exec_server_url) => {
|
||||
environments.insert(
|
||||
REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
Arc::new(
|
||||
Environment::create_with_runtime_paths(
|
||||
Some(exec_server_url),
|
||||
local_runtime_paths,
|
||||
)
|
||||
.expect("valid remote environment"),
|
||||
),
|
||||
);
|
||||
Some(REMOTE_ENVIRONMENT_ID.to_string())
|
||||
}
|
||||
None => Some(LOCAL_ENVIRONMENT_ID.to_string()),
|
||||
}
|
||||
None => LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
};
|
||||
|
||||
Self {
|
||||
default_environment,
|
||||
environment_disabled_for_agent,
|
||||
environments,
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns true when model-facing tools may access an environment.
|
||||
pub fn allows_agent_environment_access(&self) -> bool {
|
||||
!self.environment_disabled_for_agent
|
||||
&& self.environments.contains_key(&self.default_environment)
|
||||
}
|
||||
|
||||
/// Returns the default environment instance.
|
||||
pub fn default_environment(&self) -> Arc<Environment> {
|
||||
self.get_environment(&self.default_environment)
|
||||
.expect("default environment exists")
|
||||
pub fn default_environment(&self) -> Option<Arc<Environment>> {
|
||||
self.default_environment
|
||||
.as_deref()
|
||||
.and_then(|environment_id| self.get_environment(environment_id))
|
||||
}
|
||||
|
||||
/// Returns a named environment instance.
|
||||
@@ -260,9 +253,8 @@ mod tests {
|
||||
local_runtime_paths: None,
|
||||
});
|
||||
|
||||
let environment = manager.default_environment();
|
||||
let environment = manager.default_environment().expect("default environment");
|
||||
assert!(!environment.is_remote());
|
||||
assert!(manager.allows_agent_environment_access());
|
||||
assert!(
|
||||
!manager
|
||||
.get_environment(LOCAL_ENVIRONMENT_ID)
|
||||
@@ -279,14 +271,8 @@ mod tests {
|
||||
local_runtime_paths: None,
|
||||
});
|
||||
|
||||
assert!(!manager.allows_agent_environment_access());
|
||||
assert!(!manager.default_environment().is_remote());
|
||||
assert!(
|
||||
!manager
|
||||
.get_environment(LOCAL_ENVIRONMENT_ID)
|
||||
.expect("local environment")
|
||||
.is_remote()
|
||||
);
|
||||
assert!(manager.default_environment().is_none());
|
||||
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
|
||||
assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none());
|
||||
}
|
||||
|
||||
@@ -297,9 +283,8 @@ mod tests {
|
||||
local_runtime_paths: None,
|
||||
});
|
||||
|
||||
let environment = manager.default_environment();
|
||||
let environment = manager.default_environment().expect("default environment");
|
||||
assert!(environment.is_remote());
|
||||
assert!(manager.allows_agent_environment_access());
|
||||
assert_eq!(environment.exec_server_url(), Some("ws://127.0.0.1:8765"));
|
||||
assert!(
|
||||
!manager
|
||||
@@ -320,8 +305,8 @@ mod tests {
|
||||
async fn environment_manager_default_environment_caches_environment() {
|
||||
let manager = EnvironmentManager::new(EnvironmentManagerArgs::default());
|
||||
|
||||
let first = manager.default_environment();
|
||||
let second = manager.default_environment();
|
||||
let first = manager.default_environment().expect("default environment");
|
||||
let second = manager.default_environment().expect("default environment");
|
||||
|
||||
assert!(Arc::ptr_eq(&first, &second));
|
||||
}
|
||||
@@ -338,43 +323,36 @@ mod tests {
|
||||
local_runtime_paths: Some(runtime_paths.clone()),
|
||||
});
|
||||
|
||||
let environment = manager.default_environment();
|
||||
let environment = manager.default_environment().expect("default environment");
|
||||
|
||||
assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths));
|
||||
let manager = EnvironmentManager::new(EnvironmentManagerArgs {
|
||||
exec_server_url: environment.exec_server_url().map(str::to_owned),
|
||||
local_runtime_paths: environment.local_runtime_paths().cloned(),
|
||||
});
|
||||
let environment = manager.default_environment();
|
||||
let environment = manager.default_environment().expect("default environment");
|
||||
assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn disabled_environment_manager_has_default_environment_but_no_tool_environment() {
|
||||
async fn disabled_environment_manager_has_no_default_environment() {
|
||||
let manager = EnvironmentManager::new(EnvironmentManagerArgs {
|
||||
exec_server_url: Some("none".to_string()),
|
||||
local_runtime_paths: None,
|
||||
});
|
||||
|
||||
assert!(!manager.default_environment().is_remote());
|
||||
assert!(!manager.allows_agent_environment_access());
|
||||
assert!(manager.default_environment().is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn environment_manager_allows_local_lookup_when_disabled() {
|
||||
async fn environment_manager_omits_environment_lookup_when_disabled() {
|
||||
let manager = EnvironmentManager::new(EnvironmentManagerArgs {
|
||||
exec_server_url: Some("none".to_string()),
|
||||
local_runtime_paths: None,
|
||||
});
|
||||
|
||||
assert!(!manager.default_environment().is_remote());
|
||||
assert!(!manager.allows_agent_environment_access());
|
||||
assert!(
|
||||
!manager
|
||||
.get_environment(LOCAL_ENVIRONMENT_ID)
|
||||
.expect("local environment")
|
||||
.is_remote()
|
||||
);
|
||||
assert!(manager.default_environment().is_none());
|
||||
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
|
||||
assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none());
|
||||
}
|
||||
|
||||
|
||||
@@ -627,7 +627,9 @@ fn config_cwd_for_app_server_target(
|
||||
app_server_target: &AppServerTarget,
|
||||
environment_manager: &EnvironmentManager,
|
||||
) -> std::io::Result<Option<AbsolutePathBuf>> {
|
||||
if environment_manager.default_environment().is_remote()
|
||||
if environment_manager
|
||||
.default_environment()
|
||||
.is_some_and(|environment| environment.is_remote())
|
||||
|| matches!(app_server_target, AppServerTarget::Remote { .. })
|
||||
{
|
||||
return Ok(None);
|
||||
|
||||
Reference in New Issue
Block a user