chore: cleanup Config instantiation codepaths

[bolinfest]

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:

codex/codex-rs/core/src/config/mod.rs

Lines 314 to 324 in 774bd9e

	let root_value = load_resolved_config(
	&codex_home,
	cli_overrides,
	crate::config_loader::LoaderOverrides::default(),
	)
	.await?;
	let cfg = deserialize_config_toml_with_base(root_value, &codex_home).map_err(|e| {
	tracing::error!("Failed to deserialize overridden config: {e}");
	e
	})?;

codex/codex-rs/core/src/config/mod.rs

Lines 334 to 344 in 774bd9e

	let root_value = load_resolved_config(
	codex_home,
	cli_overrides,
	crate::config_loader::LoaderOverrides::default(),
	)
	.await?;
	let cfg = deserialize_config_toml_with_base(root_value, codex_home).map_err(|e| {
	tracing::error!("Failed to deserialize overridden config: {e}");
	e
	})?;

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:

codex/codex-rs/core/src/config/mod.rs

Lines 308 to 311 in 774bd9e

	pub async fn load_with_cli_overrides(
	cli_overrides: Vec<(String, TomlValue)>,
	overrides: ConfigOverrides,
	) -> std::io::Result<Self> {

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.

[aibrahim-oai]

load_config_as_toml_with_cli_overrides is returning ConfigToml while load_with_cli_overrides returns Config. How are they substituting each other?

[aibrahim-oai]

Also what is [ConfigRequirements]

[bolinfest]

Sorry, some of this came from my in-flight PR that I have stacked on top and then I split it out to create a smaller PR.

ConfigRequirements is a new abstraction we are introducing to ensure enterprise-level restrictions are enforced when creating a Config.

Admittedly, one is not a substitute for the other, but the higher-level goal is that callers should generally never work with ConfigToml directly, only a fully-instantiated Config, because using a raw ConfigToml runs the risk that you are using a value where ConfigRequirements have not been enforced.

[aibrahim-oai]

This comment always confused me. It's saying exclusive for tests but it's used in normal code and doesn't have cfg test around it.

[bolinfest]

I know...the problem is that it needs to access things that are private to this file, but we also need to make it available to integration tests. Adding #[cfg(test)] would make it so it is not available to integration tests, so we're kinda stuck.

[aibrahim-oai]

some places in the code is using #[cfg(any(test, feature = "test-support"))] not sure if this is possible here. we probably have core tests that are using it. It's still being used in non tests load_with_cli_overrides and load_with_cli_overrides_and_harness_overrides. Is this expected?