Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce
ConfigBuilderas an alternative to our existingConfigconstructors.I noticed that the existing constructors,
Config::load_with_cli_overrides()andConfig::load_with_cli_overrides_and_harness_overrides(), did not takecodex_homeas a parameter, which can be a problem.Historically, when Codex was purely a CLI, we wanted to be extra sure that the creation of
codex_homewas always done viafind_codex_home(), so we did not exposecodex_homeas a parameter when creatingConfigin business logic. But in integration tests,codex_homenearly always needs to be configured (as a temp directory), which is why callers would have to go throughConfig::load_from_base_config_with_overrides()instead.Now that the Codex harness also functions as an app server, which could conceivably load multiple threads where
codex_homeis parameterized differently in each one, I think it makes sense to make this configurable. Going to a builder pattern makes it more flexible to ensure an arbitrary permutation of options can be set when constructing aConfigwhile using the appropriate defaults for the options that aren't set explicitly.Ultimately, I think this should make it possible for us to make
Config::load_from_base_config_with_overrides()private because all integration tests should be able to leverageConfigBuilderinstead. Though there could be edge cases, so I'll pursue that migration after we get through the current config overhaul.Stack created with Sapling. Best reviewed with ReviewStack.