mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Keep global app-server config independent of the launch directory (#51872)
## Why Global requests can inherit project settings from the app-server's launch directory. This can prevent marketplace removal or expose project-only feature settings, and config reloads can fail if that directory is deleted. ## What changed - Load configuration without project context when no working directory is supplied, using `CODEX_HOME` to materialize `Config.cwd`. - Use global configuration for marketplace mutations, marketplace upgrade reloads, and MCP requests without thread context. - Pass the launch directory explicitly during startup to preserve project-aware startup configuration. ## Testing Update regression tests to verify that marketplace removal ignores launch-project configuration and leaves the project config intact. Extend feature-list coverage to verify that global requests ignore project features, thread requests retain them, and global requests work after the launch directory is deleted on Unix. GitOrigin-RevId: b77063a7ea0b09fd0a79cfc4040db9f1d02606b3
This commit is contained in:
@@ -192,11 +192,14 @@ impl ConfigManager {
|
||||
&self,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> std::io::Result<Config> {
|
||||
let Some(fallback_cwd) = fallback_cwd else {
|
||||
return self.load_non_project_config().await;
|
||||
};
|
||||
self.load_with_cli_overrides(
|
||||
&self.current_cli_overrides(),
|
||||
/*request_overrides*/ None,
|
||||
ConfigOverrides::default(),
|
||||
fallback_cwd,
|
||||
Some(fallback_cwd),
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -204,9 +207,23 @@ impl ConfigManager {
|
||||
/// Loads system, user, and runtime settings without discovering a project
|
||||
/// from the app-server process's working directory.
|
||||
pub(crate) async fn load_non_project_config(&self) -> std::io::Result<Config> {
|
||||
let mut manager = self.clone();
|
||||
manager.loader_overrides.ignore_project_config = true;
|
||||
manager.load_latest_config(/*fallback_cwd*/ None).await
|
||||
let policy_load = self.refresh_application_network_policy().await?;
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(self.codex_home.clone())
|
||||
.cli_overrides(self.current_cli_overrides())
|
||||
.loader_overrides(self.loader_overrides.clone())
|
||||
.strict_config(self.strict_config)
|
||||
.fallback_cwd(Some(self.codex_home.clone()))
|
||||
.cloud_config_bundle(policy_load.cloud_config.clone())
|
||||
.thread_config_loader(Arc::clone(&self.thread_config_loader))
|
||||
.without_project_context()
|
||||
.build()
|
||||
.await?;
|
||||
self.check_application_policy_load(&policy_load)?;
|
||||
self.apply_network_policy(&mut config);
|
||||
self.apply_runtime_feature_enablement(&mut config);
|
||||
self.apply_arg0_paths(&mut config);
|
||||
Ok(config)
|
||||
}
|
||||
|
||||
pub(crate) async fn load_latest_config_with_session_layers(
|
||||
|
||||
@@ -529,6 +529,7 @@ pub async fn run_main_with_transport_options(
|
||||
)
|
||||
})?;
|
||||
let codex_home = find_codex_home()?;
|
||||
let startup_cwd = std::env::current_dir().ok();
|
||||
let local_runtime_paths = ExecServerRuntimeOptions::from_optional_paths(
|
||||
arg0_paths.codex_self_exe.clone(),
|
||||
arg0_paths.codex_linux_sandbox_exe.clone(),
|
||||
@@ -544,7 +545,7 @@ pub async fn run_main_with_transport_options(
|
||||
Arc::new(NoopThreadConfigLoader),
|
||||
);
|
||||
let bootstrap_config = config_manager
|
||||
.load_startup_config(/*fallback_cwd*/ None)
|
||||
.load_startup_config(startup_cwd.clone())
|
||||
.await?;
|
||||
let bootstrap_auth =
|
||||
AuthManager::shared_from_config(&bootstrap_config, /*enable_codex_api_key_env*/ false)
|
||||
@@ -557,10 +558,7 @@ pub async fn run_main_with_transport_options(
|
||||
);
|
||||
let mut config_warnings = Vec::new();
|
||||
let mut plugin_startup_config = PluginStartupConfig::Current;
|
||||
let config = match config_manager
|
||||
.load_latest_config(/*fallback_cwd*/ None)
|
||||
.await
|
||||
{
|
||||
let config = match config_manager.load_latest_config(startup_cwd).await {
|
||||
Ok(config) => config,
|
||||
Err(err) if is_unsupported_untrusted_approval_policy_error(&err) => {
|
||||
return Err(err);
|
||||
|
||||
@@ -465,11 +465,8 @@ impl MessageProcessor {
|
||||
rpc_transport,
|
||||
Arc::clone(&user_verification),
|
||||
);
|
||||
let marketplace_processor = MarketplaceRequestProcessor::new(
|
||||
Arc::clone(&config),
|
||||
config_manager.clone(),
|
||||
Arc::clone(&thread_manager),
|
||||
);
|
||||
let marketplace_processor =
|
||||
MarketplaceRequestProcessor::new(config_manager.clone(), Arc::clone(&thread_manager));
|
||||
let mcp_processor = McpRequestProcessor::new(
|
||||
auth_manager.clone(),
|
||||
Arc::clone(&thread_manager),
|
||||
|
||||
@@ -17,6 +17,16 @@ pub(crate) fn for_cwd(manager: ConfigManager, cwd: AbsolutePathBuf) -> ConfigLay
|
||||
Arc::new(move || runtime.block_on(manager.load_config_layers_for_cwd(cwd.clone())))
|
||||
}
|
||||
|
||||
/// Reloads config without deriving project context from a process cwd.
|
||||
pub(crate) fn global(manager: ConfigManager) -> ConfigLayerReload {
|
||||
let runtime = tokio::runtime::Handle::current();
|
||||
Arc::new(move || {
|
||||
runtime
|
||||
.block_on(manager.load_non_project_config())
|
||||
.map(|config| config.config_layer_stack)
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn defaults(manager: ConfigManager) -> ConfigLayerReload {
|
||||
let runtime = tokio::runtime::Handle::current();
|
||||
Arc::new(move || {
|
||||
|
||||
@@ -3,19 +3,13 @@ use crate::plugin_config_reload;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct MarketplaceRequestProcessor {
|
||||
config: Arc<Config>,
|
||||
config_manager: ConfigManager,
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
}
|
||||
|
||||
impl MarketplaceRequestProcessor {
|
||||
pub(crate) fn new(
|
||||
config: Arc<Config>,
|
||||
config_manager: ConfigManager,
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
) -> Self {
|
||||
pub(crate) fn new(config_manager: ConfigManager, thread_manager: Arc<ThreadManager>) -> Self {
|
||||
Self {
|
||||
config,
|
||||
config_manager,
|
||||
thread_manager,
|
||||
}
|
||||
@@ -52,7 +46,7 @@ impl MarketplaceRequestProcessor {
|
||||
&self,
|
||||
params: MarketplaceRemoveParams,
|
||||
) -> Result<MarketplaceRemoveResponse, JSONRPCErrorError> {
|
||||
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
|
||||
let config = self.load_global_config().await?;
|
||||
remove_marketplace(
|
||||
config.codex_home.to_path_buf(),
|
||||
config.config_layer_stack,
|
||||
@@ -75,12 +69,11 @@ impl MarketplaceRequestProcessor {
|
||||
&self,
|
||||
params: MarketplaceUpgradeParams,
|
||||
) -> Result<MarketplaceUpgradeResponse, JSONRPCErrorError> {
|
||||
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
|
||||
let config = self.load_global_config().await?;
|
||||
let plugins_manager = self.thread_manager.plugins_manager();
|
||||
let MarketplaceUpgradeParams { marketplace_name } = params;
|
||||
let plugins_input = config.plugins_config_input();
|
||||
let reload_config =
|
||||
plugin_config_reload::for_cwd(self.config_manager.clone(), config.cwd.clone());
|
||||
let reload_config = plugin_config_reload::global(self.config_manager.clone());
|
||||
|
||||
let outcome = tokio::task::spawn_blocking(move || {
|
||||
plugins_manager.upgrade_configured_marketplaces_for_config(
|
||||
@@ -118,9 +111,9 @@ impl MarketplaceRequestProcessor {
|
||||
&self,
|
||||
params: MarketplaceAddParams,
|
||||
) -> Result<MarketplaceAddResponse, JSONRPCErrorError> {
|
||||
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
|
||||
let config = self.load_global_config().await?;
|
||||
add_marketplace_to_codex_home(
|
||||
self.config.codex_home.to_path_buf(),
|
||||
config.codex_home.to_path_buf(),
|
||||
config.config_layer_stack.requirements().clone(),
|
||||
MarketplaceAddRequest {
|
||||
source: params.source,
|
||||
@@ -140,12 +133,9 @@ impl MarketplaceRequestProcessor {
|
||||
})
|
||||
}
|
||||
|
||||
async fn load_latest_config(
|
||||
&self,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> Result<Config, JSONRPCErrorError> {
|
||||
async fn load_global_config(&self) -> Result<Config, JSONRPCErrorError> {
|
||||
self.config_manager
|
||||
.load_latest_config(fallback_cwd)
|
||||
.load_non_project_config()
|
||||
.await
|
||||
.map_err(|err| internal_error(format!("failed to reload config: {err}")))
|
||||
}
|
||||
|
||||
@@ -106,12 +106,9 @@ impl McpRequestProcessor {
|
||||
Ok(McpServerRefreshResponse {})
|
||||
}
|
||||
|
||||
async fn load_latest_config(
|
||||
&self,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> Result<Config, JSONRPCErrorError> {
|
||||
async fn load_global_config(&self) -> Result<Config, JSONRPCErrorError> {
|
||||
self.config_manager
|
||||
.load_latest_config(fallback_cwd)
|
||||
.load_non_project_config()
|
||||
.await
|
||||
.map_err(|err| internal_error(format!("failed to reload config: {err}")))
|
||||
}
|
||||
@@ -152,7 +149,7 @@ impl McpRequestProcessor {
|
||||
((*config).clone(), runtime_context)
|
||||
}
|
||||
None => {
|
||||
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
|
||||
let config = self.load_global_config().await?;
|
||||
let mcp_config = self
|
||||
.thread_manager
|
||||
.mcp_manager()
|
||||
@@ -421,7 +418,7 @@ impl McpRequestProcessor {
|
||||
.await
|
||||
.map_err(|err| internal_error(format!("failed to reload config: {err}")))?
|
||||
}
|
||||
None => self.load_latest_config(/*fallback_cwd*/ None).await?,
|
||||
None => self.load_global_config().await?,
|
||||
};
|
||||
let mcp_manager = self.thread_manager.mcp_manager();
|
||||
let auth = self.auth_manager.auth().await;
|
||||
@@ -622,7 +619,7 @@ impl McpRequestProcessor {
|
||||
return Err(invalid_request("originCallId requires threadId"));
|
||||
}
|
||||
|
||||
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
|
||||
let config = self.load_global_config().await?;
|
||||
let mcp_manager = self.thread_manager.mcp_manager();
|
||||
let mcp_config = mcp_manager.runtime_config(&config).await;
|
||||
let codex_apps_tools_cache = mcp_manager.codex_apps_tools_cache();
|
||||
|
||||
@@ -145,12 +145,16 @@ async fn experimental_feature_list_resolves_thread_project_config() -> Result<()
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
let workspace = TempDir::new()?;
|
||||
let codex_home_key = codex_home.path().to_string_lossy().replace('\\', "\\\\");
|
||||
let workspace_key = workspace.path().to_string_lossy().replace('\\', "\\\\");
|
||||
MockResponsesConfig::new(&server.uri())
|
||||
.with_extra_config(&format!(
|
||||
"[projects.\"{workspace_key}\"]\ntrust_level = \"trusted\""
|
||||
"[projects.\"{codex_home_key}\"]\ntrust_level = \"trusted\"\n\
|
||||
[projects.\"{workspace_key}\"]\ntrust_level = \"trusted\""
|
||||
))
|
||||
.write(codex_home.path())?;
|
||||
#[cfg(unix)]
|
||||
let config_toml = std::fs::read(codex_home.path().join("config.toml"))?;
|
||||
let project_config_dir = workspace.path().join(".codex");
|
||||
std::fs::create_dir_all(&project_config_dir)?;
|
||||
std::fs::write(
|
||||
@@ -159,6 +163,12 @@ async fn experimental_feature_list_resolves_thread_project_config() -> Result<()
|
||||
memories = true
|
||||
"#,
|
||||
)?;
|
||||
let launch_project_config_dir = codex_home.path().join(".codex");
|
||||
std::fs::create_dir(&launch_project_config_dir)?;
|
||||
std::fs::copy(
|
||||
project_config_dir.join("config.toml"),
|
||||
launch_project_config_dir.join("config.toml"),
|
||||
)?;
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
@@ -166,6 +176,30 @@ memories = true
|
||||
.build_initialized_with_timeout(DEFAULT_TIMEOUT)
|
||||
.await?;
|
||||
|
||||
#[cfg(unix)]
|
||||
{
|
||||
let deleted_launch_dir = workspace.path().join("deleted-launch");
|
||||
std::fs::rename(codex_home.path(), &deleted_launch_dir)?;
|
||||
std::fs::create_dir(codex_home.path())?;
|
||||
std::fs::write(codex_home.path().join("config.toml"), config_toml)?;
|
||||
std::fs::create_dir(&launch_project_config_dir)?;
|
||||
std::fs::copy(
|
||||
project_config_dir.join("config.toml"),
|
||||
launch_project_config_dir.join("config.toml"),
|
||||
)?;
|
||||
std::fs::remove_dir_all(deleted_launch_dir)?;
|
||||
}
|
||||
let request_id = mcp
|
||||
.send_experimental_feature_list_request(ExperimentalFeatureListParams::default())
|
||||
.await?;
|
||||
let global = read_response::<ExperimentalFeatureListResponse>(&mut mcp, request_id).await?;
|
||||
let global_memories = global
|
||||
.data
|
||||
.iter()
|
||||
.find(|feature| feature.name == "memories")
|
||||
.expect("memories feature should be present");
|
||||
assert!(!global_memories.enabled);
|
||||
|
||||
let thread_start_id = mcp
|
||||
.send_thread_start_request_with_auto_env(ThreadStartParams {
|
||||
cwd: Some(workspace.path().display().to_string()),
|
||||
|
||||
@@ -123,15 +123,11 @@ async fn marketplace_remove_rejects_unknown_marketplace() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false; "project only")]
|
||||
#[test_case(true; "project and user")]
|
||||
#[tokio::test]
|
||||
async fn marketplace_remove_preserves_project_marketplace(user_entry: bool) -> Result<()> {
|
||||
async fn marketplace_remove_ignores_startup_project_config() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
if user_entry {
|
||||
record_user_marketplace(codex_home.path(), "debug", &configured_marketplace_update())?;
|
||||
}
|
||||
// TestAppServer starts in CODEX_HOME, so make it a trusted project as well.
|
||||
// TestAppServer starts in CODEX_HOME, so put project config there to verify
|
||||
// that the global marketplace RPC does not use it.
|
||||
std::fs::create_dir_all(codex_home.path().join(".git"))?;
|
||||
std::fs::create_dir_all(codex_home.path().join(".codex"))?;
|
||||
let project_config_path = codex_home.path().join(".codex/config.toml");
|
||||
@@ -141,35 +137,26 @@ async fn marketplace_remove_preserves_project_marketplace(user_entry: bool) -> R
|
||||
write_installed_marketplace(codex_home.path(), "debug")?;
|
||||
let snapshot_path =
|
||||
marketplace_install_root(codex_home.path()).join("debug/.agents/plugins/marketplace.json");
|
||||
let user_config_path = codex_home.path().join("config.toml");
|
||||
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.build_initialized()
|
||||
.await?;
|
||||
let user_config = std::fs::read_to_string(&user_config_path)?;
|
||||
let request_id = mcp
|
||||
.send_marketplace_remove_request(MarketplaceRemoveParams {
|
||||
marketplace_name: "debug".to_string(),
|
||||
let response: MarketplaceRemoveResponse = mcp
|
||||
.request(|request_id| ClientRequest::MarketplaceRemove {
|
||||
request_id,
|
||||
params: MarketplaceRemoveParams {
|
||||
marketplace_name: "debug".to_string(),
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
let err = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(request_id)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
assert_eq!(err.error.code, -32600);
|
||||
assert!(
|
||||
err.error
|
||||
.message
|
||||
.starts_with("marketplace `debug` is configured in project (")
|
||||
);
|
||||
assert_eq!(std::fs::read_to_string(user_config_path)?, user_config);
|
||||
assert_eq!(response.marketplace_name, "debug");
|
||||
assert!(response.installed_root.is_some());
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(project_config_path)?,
|
||||
project_config
|
||||
);
|
||||
assert_eq!(std::fs::read_to_string(snapshot_path)?, "{}");
|
||||
assert!(!snapshot_path.exists());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1457,6 +1457,7 @@ pub struct ConfigBuilder {
|
||||
cloud_config_bundle: CloudConfigBundleLoader,
|
||||
thread_config_loader: Option<Arc<dyn ThreadConfigLoader>>,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
without_project_context: bool,
|
||||
}
|
||||
|
||||
impl ConfigBuilder {
|
||||
@@ -1503,6 +1504,12 @@ impl ConfigBuilder {
|
||||
self
|
||||
}
|
||||
|
||||
/// Materializes `Config.cwd` without using it as config-layer context.
|
||||
pub fn without_project_context(mut self) -> Self {
|
||||
self.without_project_context = true;
|
||||
self
|
||||
}
|
||||
|
||||
pub async fn build(self) -> std::io::Result<Config> {
|
||||
// Keep the large config-loading future off small runtime thread stacks.
|
||||
Box::pin(self.build_inner()).await
|
||||
@@ -1518,6 +1525,7 @@ impl ConfigBuilder {
|
||||
cloud_config_bundle,
|
||||
thread_config_loader,
|
||||
fallback_cwd,
|
||||
without_project_context,
|
||||
} = self;
|
||||
let codex_home = match codex_home {
|
||||
Some(codex_home) => AbsolutePathBuf::from_absolute_path(codex_home)?,
|
||||
@@ -1535,7 +1543,7 @@ impl ConfigBuilder {
|
||||
let config_layer_stack = load_config_layers_state(
|
||||
LOCAL_FS.as_ref(),
|
||||
&codex_home,
|
||||
Some(cwd),
|
||||
(!without_project_context).then_some(cwd),
|
||||
&cli_overrides,
|
||||
ConfigLoadOptions {
|
||||
loader_overrides,
|
||||
|
||||
Reference in New Issue
Block a user