chore: cleanup Config instantiation codepaths (#8226)
This PR does various types of cleanup before I can proceed with more ambitious changes to config loading. First, I noticed duplicated code across these two methods:774bd9e432/codex-rs/core/src/config/mod.rs (L314-L324)774bd9e432/codex-rs/core/src/config/mod.rs (L334-L344)This has now been consolidated in `load_config_as_toml_with_cli_overrides()`. Further, I noticed that `Config::load_with_cli_overrides()` took two similar arguments:774bd9e432/codex-rs/core/src/config/mod.rs (L308-L311)The difference between `cli_overrides` and `overrides` was not immediately obvious to me. At first glance, it appears that one should be able to be expressed in terms of the other, but it turns out that some fields of `ConfigOverrides` (such as `cwd` and `codex_linux_sandbox_exe`) are, by design, not configurable via a `.toml` file or a command-line `--config` flag. That said, I discovered that many callers of `Config::load_with_cli_overrides()` were passing `ConfigOverrides::default()` for `overrides`, so I created two separate methods: - `Config::load_with_cli_overrides(cli_overrides: Vec<(String, TomlValue)>)` - `Config::load_with_cli_overrides_and_harness_overrides(cli_overrides: Vec<(String, TomlValue)>, harness_overrides: ConfigOverrides)` The latter has a long name, as it is _not_ what should be used in the common case, so the extra typing is designed to draw attention to this fact. I tried to update the existing callsites to use the shorter name, where possible. Further, in the cases where `ConfigOverrides` is used, usually only a limited subset of fields are actually set, so I updated the declarations to leverage `..Default::default()` where possible.
This commit is contained in:
parent
774bd9e432
commit
a8797019a1
14 changed files with 60 additions and 62 deletions
|
|
@ -282,7 +282,7 @@ impl CodexMessageProcessor {
|
|||
}
|
||||
|
||||
async fn load_latest_config(&self) -> Result<Config, JSONRPCErrorError> {
|
||||
Config::load_with_cli_overrides(self.cli_overrides.clone(), ConfigOverrides::default())
|
||||
Config::load_with_cli_overrides(self.cli_overrides.clone())
|
||||
.await
|
||||
.map_err(|err| JSONRPCErrorError {
|
||||
code: INTERNAL_ERROR_CODE,
|
||||
|
|
@ -3348,7 +3348,7 @@ fn errors_to_info(
|
|||
|
||||
async fn derive_config_from_params(
|
||||
overrides: ConfigOverrides,
|
||||
cli_overrides: Option<std::collections::HashMap<String, serde_json::Value>>,
|
||||
cli_overrides: Option<HashMap<String, serde_json::Value>>,
|
||||
) -> std::io::Result<Config> {
|
||||
let cli_overrides = cli_overrides
|
||||
.unwrap_or_default()
|
||||
|
|
@ -3356,7 +3356,7 @@ async fn derive_config_from_params(
|
|||
.map(|(k, v)| (k, json_to_toml(v)))
|
||||
.collect();
|
||||
|
||||
Config::load_with_cli_overrides(cli_overrides, overrides).await
|
||||
Config::load_with_cli_overrides_and_harness_overrides(cli_overrides, overrides).await
|
||||
}
|
||||
|
||||
async fn read_summary_from_rollout(
|
||||
|
|
|
|||
|
|
@ -2,7 +2,6 @@
|
|||
|
||||
use codex_common::CliConfigOverrides;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use std::io::ErrorKind;
|
||||
use std::io::Result as IoResult;
|
||||
use std::path::PathBuf;
|
||||
|
|
@ -81,12 +80,11 @@ pub async fn run_main(
|
|||
format!("error parsing -c overrides: {e}"),
|
||||
)
|
||||
})?;
|
||||
let config =
|
||||
Config::load_with_cli_overrides(cli_kv_overrides.clone(), ConfigOverrides::default())
|
||||
.await
|
||||
.map_err(|e| {
|
||||
std::io::Error::new(ErrorKind::InvalidData, format!("error loading config: {e}"))
|
||||
})?;
|
||||
let config = Config::load_with_cli_overrides(cli_kv_overrides.clone())
|
||||
.await
|
||||
.map_err(|e| {
|
||||
std::io::Error::new(ErrorKind::InvalidData, format!("error loading config: {e}"))
|
||||
})?;
|
||||
|
||||
let feedback = CodexFeedback::new();
|
||||
|
||||
|
|
|
|||
|
|
@ -3,7 +3,6 @@ use std::path::PathBuf;
|
|||
use clap::Parser;
|
||||
use codex_common::CliConfigOverrides;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
|
||||
use crate::chatgpt_token::init_chatgpt_token_from_auth;
|
||||
use crate::get_task::GetTaskResponse;
|
||||
|
|
@ -28,7 +27,6 @@ pub async fn run_apply_command(
|
|||
.config_overrides
|
||||
.parse_overrides()
|
||||
.map_err(anyhow::Error::msg)?,
|
||||
ConfigOverrides::default(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
|
|
|||
|
|
@ -109,7 +109,7 @@ async fn run_command_under_sandbox(
|
|||
log_denials: bool,
|
||||
) -> anyhow::Result<()> {
|
||||
let sandbox_mode = create_sandbox_mode(full_auto);
|
||||
let config = Config::load_with_cli_overrides(
|
||||
let config = Config::load_with_cli_overrides_and_harness_overrides(
|
||||
config_overrides
|
||||
.parse_overrides()
|
||||
.map_err(anyhow::Error::msg)?,
|
||||
|
|
|
|||
|
|
@ -6,7 +6,6 @@ use codex_core::auth::CLIENT_ID;
|
|||
use codex_core::auth::login_with_api_key;
|
||||
use codex_core::auth::logout;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_login::ServerOptions;
|
||||
use codex_login::run_device_code_login;
|
||||
use codex_login::run_login_server;
|
||||
|
|
@ -210,8 +209,7 @@ async fn load_config_or_exit(cli_config_overrides: CliConfigOverrides) -> Config
|
|||
}
|
||||
};
|
||||
|
||||
let config_overrides = ConfigOverrides::default();
|
||||
match Config::load_with_cli_overrides(cli_overrides, config_overrides).await {
|
||||
match Config::load_with_cli_overrides(cli_overrides).await {
|
||||
Ok(config) => config,
|
||||
Err(e) => {
|
||||
eprintln!("Error loading configuration: {e}");
|
||||
|
|
|
|||
|
|
@ -631,7 +631,11 @@ async fn cli_main(codex_linux_sandbox_exe: Option<PathBuf>) -> anyhow::Result<()
|
|||
..Default::default()
|
||||
};
|
||||
|
||||
let config = Config::load_with_cli_overrides(cli_kv_overrides, overrides).await?;
|
||||
let config = Config::load_with_cli_overrides_and_harness_overrides(
|
||||
cli_kv_overrides,
|
||||
overrides,
|
||||
)
|
||||
.await?;
|
||||
for def in codex_core::features::FEATURES.iter() {
|
||||
let name = def.key;
|
||||
let stage = stage_str(def.stage);
|
||||
|
|
|
|||
|
|
@ -8,7 +8,6 @@ use clap::ArgGroup;
|
|||
use codex_common::CliConfigOverrides;
|
||||
use codex_common::format_env_display::format_env_display;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_core::config::edit::ConfigEditsBuilder;
|
||||
use codex_core::config::find_codex_home;
|
||||
use codex_core::config::load_global_mcp_servers;
|
||||
|
|
@ -200,7 +199,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re
|
|||
let overrides = config_overrides
|
||||
.parse_overrides()
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
let config = Config::load_with_cli_overrides(overrides, ConfigOverrides::default())
|
||||
let config = Config::load_with_cli_overrides(overrides)
|
||||
.await
|
||||
.context("failed to load configuration")?;
|
||||
|
||||
|
|
@ -349,7 +348,7 @@ async fn run_login(config_overrides: &CliConfigOverrides, login_args: LoginArgs)
|
|||
let overrides = config_overrides
|
||||
.parse_overrides()
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
let config = Config::load_with_cli_overrides(overrides, ConfigOverrides::default())
|
||||
let config = Config::load_with_cli_overrides(overrides)
|
||||
.await
|
||||
.context("failed to load configuration")?;
|
||||
|
||||
|
|
@ -392,7 +391,7 @@ async fn run_logout(config_overrides: &CliConfigOverrides, logout_args: LogoutAr
|
|||
let overrides = config_overrides
|
||||
.parse_overrides()
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
let config = Config::load_with_cli_overrides(overrides, ConfigOverrides::default())
|
||||
let config = Config::load_with_cli_overrides(overrides)
|
||||
.await
|
||||
.context("failed to load configuration")?;
|
||||
|
||||
|
|
@ -421,7 +420,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) ->
|
|||
let overrides = config_overrides
|
||||
.parse_overrides()
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
let config = Config::load_with_cli_overrides(overrides, ConfigOverrides::default())
|
||||
let config = Config::load_with_cli_overrides(overrides)
|
||||
.await
|
||||
.context("failed to load configuration")?;
|
||||
|
||||
|
|
@ -678,7 +677,7 @@ async fn run_get(config_overrides: &CliConfigOverrides, get_args: GetArgs) -> Re
|
|||
let overrides = config_overrides
|
||||
.parse_overrides()
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
let config = Config::load_with_cli_overrides(overrides, ConfigOverrides::default())
|
||||
let config = Config::load_with_cli_overrides(overrides)
|
||||
.await
|
||||
.context("failed to load configuration")?;
|
||||
|
||||
|
|
|
|||
|
|
@ -5,7 +5,6 @@ use chrono::Utc;
|
|||
use reqwest::header::HeaderMap;
|
||||
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_login::AuthManager;
|
||||
|
||||
pub fn set_user_agent_suffix(suffix: &str) {
|
||||
|
|
@ -62,9 +61,7 @@ pub fn extract_chatgpt_account_id(token: &str) -> Option<String> {
|
|||
|
||||
pub async fn load_auth_manager() -> Option<AuthManager> {
|
||||
// TODO: pass in cli overrides once cloud tasks properly support them.
|
||||
let config = Config::load_with_cli_overrides(Vec::new(), ConfigOverrides::default())
|
||||
.await
|
||||
.ok()?;
|
||||
let config = Config::load_with_cli_overrides(Vec::new()).await.ok()?;
|
||||
Some(AuthManager::new(
|
||||
config.codex_home,
|
||||
false,
|
||||
|
|
|
|||
|
|
@ -305,28 +305,40 @@ pub struct Config {
|
|||
}
|
||||
|
||||
impl Config {
|
||||
/// This is the preferred way to create an instance of [Config].
|
||||
pub async fn load_with_cli_overrides(
|
||||
cli_overrides: Vec<(String, TomlValue)>,
|
||||
overrides: ConfigOverrides,
|
||||
) -> std::io::Result<Self> {
|
||||
let codex_home = find_codex_home()?;
|
||||
|
||||
let root_value = load_resolved_config(
|
||||
&codex_home,
|
||||
cli_overrides,
|
||||
crate::config_loader::LoaderOverrides::default(),
|
||||
let config_toml =
|
||||
load_config_as_toml_with_cli_overrides(&codex_home, cli_overrides).await?;
|
||||
Self::load_from_base_config_with_overrides(
|
||||
config_toml,
|
||||
ConfigOverrides::default(),
|
||||
codex_home,
|
||||
)
|
||||
.await?;
|
||||
}
|
||||
|
||||
let cfg = deserialize_config_toml_with_base(root_value, &codex_home).map_err(|e| {
|
||||
tracing::error!("Failed to deserialize overridden config: {e}");
|
||||
e
|
||||
})?;
|
||||
|
||||
Self::load_from_base_config_with_overrides(cfg, overrides, codex_home)
|
||||
/// This is a secondary way of creating [Config], which is appropriate when
|
||||
/// the harness is meant to be used with a specific configuration that
|
||||
/// ignores user settings. For example, the `codex exec` subcommand is
|
||||
/// designed to use [AskForApproval::Never] exclusively.
|
||||
///
|
||||
/// Further, [ConfigOverrides] contains some options that are not supported
|
||||
/// in [ConfigToml], such as `cwd` and `codex_linux_sandbox_exe`.
|
||||
pub async fn load_with_cli_overrides_and_harness_overrides(
|
||||
cli_overrides: Vec<(String, TomlValue)>,
|
||||
harness_overrides: ConfigOverrides,
|
||||
) -> std::io::Result<Self> {
|
||||
let codex_home = find_codex_home()?;
|
||||
let config_toml =
|
||||
load_config_as_toml_with_cli_overrides(&codex_home, cli_overrides).await?;
|
||||
Self::load_from_base_config_with_overrides(config_toml, harness_overrides, codex_home)
|
||||
}
|
||||
}
|
||||
|
||||
/// DEPRECATED: Use [Config::load_with_cli_overrides()] instead because this
|
||||
/// codepath is not guaranteed to honor [ConfigRequirements].
|
||||
pub async fn load_config_as_toml_with_cli_overrides(
|
||||
codex_home: &Path,
|
||||
cli_overrides: Vec<(String, TomlValue)>,
|
||||
|
|
@ -927,8 +939,8 @@ pub fn resolve_oss_provider(
|
|||
}
|
||||
|
||||
impl Config {
|
||||
/// Meant to be used exclusively for tests: `load_with_overrides()` should
|
||||
/// be used in all other cases.
|
||||
/// Meant to be used exclusively for tests:
|
||||
/// [Config::load_with_cli_overrides()] should be used in all other cases.
|
||||
pub fn load_from_base_config_with_overrides(
|
||||
cfg: ConfigToml,
|
||||
overrides: ConfigOverrides,
|
||||
|
|
|
|||
|
|
@ -202,7 +202,8 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
|||
additional_writable_roots: add_dir,
|
||||
};
|
||||
|
||||
let config = Config::load_with_cli_overrides(cli_kv_overrides, overrides).await?;
|
||||
let config =
|
||||
Config::load_with_cli_overrides_and_harness_overrides(cli_kv_overrides, overrides).await?;
|
||||
|
||||
if let Err(err) = enforce_login_restrictions(&config).await {
|
||||
eprintln!("{err}");
|
||||
|
|
|
|||
|
|
@ -1,5 +1,7 @@
|
|||
//! Configuration object accepted by the `codex` MCP tool-call.
|
||||
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_core::protocol::AskForApproval;
|
||||
use codex_protocol::config_types::SandboxMode;
|
||||
use codex_utils_json_to_toml::json_to_toml;
|
||||
|
|
@ -139,7 +141,7 @@ impl CodexToolCallParam {
|
|||
pub async fn into_config(
|
||||
self,
|
||||
codex_linux_sandbox_exe: Option<PathBuf>,
|
||||
) -> std::io::Result<(String, codex_core::config::Config)> {
|
||||
) -> std::io::Result<(String, Config)> {
|
||||
let Self {
|
||||
prompt,
|
||||
model,
|
||||
|
|
@ -154,22 +156,17 @@ impl CodexToolCallParam {
|
|||
} = self;
|
||||
|
||||
// Build the `ConfigOverrides` recognized by codex-core.
|
||||
let overrides = codex_core::config::ConfigOverrides {
|
||||
let overrides = ConfigOverrides {
|
||||
model,
|
||||
review_model: None,
|
||||
config_profile: profile,
|
||||
cwd: cwd.map(PathBuf::from),
|
||||
approval_policy: approval_policy.map(Into::into),
|
||||
sandbox_mode: sandbox.map(Into::into),
|
||||
model_provider: None,
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
compact_prompt,
|
||||
include_apply_patch_tool: None,
|
||||
show_raw_agent_reasoning: None,
|
||||
tools_web_search_request: None,
|
||||
additional_writable_roots: Vec::new(),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let cli_overrides = cli_overrides
|
||||
|
|
@ -179,7 +176,7 @@ impl CodexToolCallParam {
|
|||
.collect();
|
||||
|
||||
let cfg =
|
||||
codex_core::config::Config::load_with_cli_overrides(cli_overrides, overrides).await?;
|
||||
Config::load_with_cli_overrides_and_harness_overrides(cli_overrides, overrides).await?;
|
||||
|
||||
Ok((prompt, cfg))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,7 +7,6 @@ use std::path::PathBuf;
|
|||
|
||||
use codex_common::CliConfigOverrides;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
|
||||
use mcp_types::JSONRPCMessage;
|
||||
use tokio::io::AsyncBufReadExt;
|
||||
|
|
@ -90,7 +89,7 @@ pub async fn run_main(
|
|||
format!("error parsing -c overrides: {e}"),
|
||||
)
|
||||
})?;
|
||||
let config = Config::load_with_cli_overrides(cli_kv_overrides, ConfigOverrides::default())
|
||||
let config = Config::load_with_cli_overrides(cli_kv_overrides)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
std::io::Error::new(ErrorKind::InvalidData, format!("error loading config: {e}"))
|
||||
|
|
|
|||
|
|
@ -205,20 +205,15 @@ pub async fn run_main(
|
|||
|
||||
let overrides = ConfigOverrides {
|
||||
model,
|
||||
review_model: None,
|
||||
approval_policy,
|
||||
sandbox_mode,
|
||||
cwd,
|
||||
model_provider: model_provider_override.clone(),
|
||||
config_profile: cli.config_profile.clone(),
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
include_apply_patch_tool: None,
|
||||
show_raw_agent_reasoning: cli.oss.then_some(true),
|
||||
tools_web_search_request: None,
|
||||
additional_writable_roots: additional_dirs,
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let config = load_config_or_exit(cli_kv_overrides.clone(), overrides.clone()).await;
|
||||
|
|
@ -552,7 +547,7 @@ async fn load_config_or_exit(
|
|||
overrides: ConfigOverrides,
|
||||
) -> Config {
|
||||
#[allow(clippy::print_stderr)]
|
||||
match Config::load_with_cli_overrides(cli_kv_overrides, overrides).await {
|
||||
match Config::load_with_cli_overrides_and_harness_overrides(cli_kv_overrides, overrides).await {
|
||||
Ok(config) => config,
|
||||
Err(err) => {
|
||||
eprintln!("Error loading configuration: {err}");
|
||||
|
|
|
|||
|
|
@ -573,7 +573,7 @@ async fn load_config_or_exit(
|
|||
overrides: ConfigOverrides,
|
||||
) -> Config {
|
||||
#[allow(clippy::print_stderr)]
|
||||
match Config::load_with_cli_overrides(cli_kv_overrides, overrides).await {
|
||||
match Config::load_with_cli_overrides_and_harness_overrides(cli_kv_overrides, overrides).await {
|
||||
Ok(config) => config,
|
||||
Err(err) => {
|
||||
eprintln!("Error loading configuration: {err}");
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue