mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fixed config merging issue with profiles (#9509)
This PR fixes a small issue with chained (layered) config.toml file merging. The old logic didn't properly handle profiles. In particular, if a lower-layer config overrides a profile defined in a higher-layer config, the override did not take effect. This prevents users from having project-specific profile overrides and contradicts the (soon-to-be) documented behavior of config merging. The change adds a unit test for this case. It also exposes a function from the config crate that is needed by the app server code paths to implement support for layered configs.
This commit is contained in:
committed by
GitHub
Unverified
parent
0b3c802a54
commit
79c5bf9835
@@ -399,6 +399,7 @@ pub struct ConfigBuilder {
|
||||
cli_overrides: Option<Vec<(String, TomlValue)>>,
|
||||
harness_overrides: Option<ConfigOverrides>,
|
||||
loader_overrides: Option<LoaderOverrides>,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
}
|
||||
|
||||
impl ConfigBuilder {
|
||||
@@ -422,21 +423,29 @@ impl ConfigBuilder {
|
||||
self
|
||||
}
|
||||
|
||||
pub fn fallback_cwd(mut self, fallback_cwd: Option<PathBuf>) -> Self {
|
||||
self.fallback_cwd = fallback_cwd;
|
||||
self
|
||||
}
|
||||
|
||||
pub async fn build(self) -> std::io::Result<Config> {
|
||||
let Self {
|
||||
codex_home,
|
||||
cli_overrides,
|
||||
harness_overrides,
|
||||
loader_overrides,
|
||||
fallback_cwd,
|
||||
} = self;
|
||||
let codex_home = codex_home.map_or_else(find_codex_home, std::io::Result::Ok)?;
|
||||
let cli_overrides = cli_overrides.unwrap_or_default();
|
||||
let harness_overrides = harness_overrides.unwrap_or_default();
|
||||
let mut harness_overrides = harness_overrides.unwrap_or_default();
|
||||
let loader_overrides = loader_overrides.unwrap_or_default();
|
||||
let cwd = match harness_overrides.cwd.as_deref() {
|
||||
let cwd_override = harness_overrides.cwd.as_deref().or(fallback_cwd.as_deref());
|
||||
let cwd = match cwd_override {
|
||||
Some(path) => AbsolutePathBuf::try_from(path)?,
|
||||
None => AbsolutePathBuf::current_dir()?,
|
||||
};
|
||||
harness_overrides.cwd = Some(cwd.to_path_buf());
|
||||
let config_layer_stack =
|
||||
load_config_layers_state(&codex_home, Some(cwd), &cli_overrides, loader_overrides)
|
||||
.await?;
|
||||
@@ -2301,6 +2310,52 @@ trust_level = "trusted"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn project_profile_overrides_user_profile() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let workspace = TempDir::new()?;
|
||||
let workspace_key = workspace.path().to_string_lossy().replace('\\', "\\\\");
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
format!(
|
||||
r#"
|
||||
profile = "global"
|
||||
|
||||
[profiles.global]
|
||||
model = "gpt-global"
|
||||
|
||||
[profiles.project]
|
||||
model = "gpt-project"
|
||||
|
||||
[projects."{workspace_key}"]
|
||||
trust_level = "trusted"
|
||||
"#,
|
||||
),
|
||||
)?;
|
||||
let project_config_dir = workspace.path().join(".codex");
|
||||
std::fs::create_dir_all(&project_config_dir)?;
|
||||
std::fs::write(
|
||||
project_config_dir.join(CONFIG_TOML_FILE),
|
||||
r#"
|
||||
profile = "project"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.harness_overrides(ConfigOverrides {
|
||||
cwd: Some(workspace.path().to_path_buf()),
|
||||
..Default::default()
|
||||
})
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(config.active_profile.as_deref(), Some("project"));
|
||||
assert_eq!(config.model.as_deref(), Some("gpt-project"));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn profile_sandbox_mode_overrides_base() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -104,6 +104,7 @@ pub use rollout::list::ThreadSortKey;
|
||||
pub use rollout::list::ThreadsPage;
|
||||
pub use rollout::list::parse_cursor;
|
||||
pub use rollout::list::read_head_for_summary;
|
||||
pub use rollout::list::read_session_meta_line;
|
||||
mod function_tool;
|
||||
mod state;
|
||||
mod tasks;
|
||||
|
||||
@@ -20,6 +20,7 @@ use crate::protocol::EventMsg;
|
||||
use codex_file_search as file_search;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::RolloutLine;
|
||||
use codex_protocol::protocol::SessionMetaLine;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
|
||||
/// Returned page of thread (thread) summaries.
|
||||
@@ -744,6 +745,24 @@ pub async fn read_head_for_summary(path: &Path) -> io::Result<Vec<serde_json::Va
|
||||
Ok(summary.head)
|
||||
}
|
||||
|
||||
/// Read the SessionMetaLine from the head of a rollout file for reuse by
|
||||
/// callers that need the session metadata (e.g. to derive a cwd for config).
|
||||
pub async fn read_session_meta_line(path: &Path) -> io::Result<SessionMetaLine> {
|
||||
let head = read_head_for_summary(path).await?;
|
||||
let Some(first) = head.first() else {
|
||||
return Err(io::Error::other(format!(
|
||||
"rollout at {} is empty",
|
||||
path.display()
|
||||
)));
|
||||
};
|
||||
serde_json::from_value::<SessionMetaLine>(first.clone()).map_err(|_| {
|
||||
io::Error::other(format!(
|
||||
"rollout at {} does not start with session metadata",
|
||||
path.display()
|
||||
))
|
||||
})
|
||||
}
|
||||
|
||||
async fn file_modified_time(path: &Path) -> io::Result<Option<OffsetDateTime>> {
|
||||
let meta = tokio::fs::metadata(path).await?;
|
||||
let modified = meta.modified().ok();
|
||||
|
||||
+51
-17
@@ -18,6 +18,7 @@ use codex_core::RolloutRecorder;
|
||||
use codex_core::ThreadSortKey;
|
||||
use codex_core::auth::enforce_login_restrictions;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_core::config::find_codex_home;
|
||||
use codex_core::config::load_config_as_toml_with_cli_overrides;
|
||||
@@ -25,6 +26,7 @@ use codex_core::config::resolve_oss_provider;
|
||||
use codex_core::find_thread_path_by_id_str;
|
||||
use codex_core::get_platform_sandbox;
|
||||
use codex_core::protocol::AskForApproval;
|
||||
use codex_core::read_session_meta_line;
|
||||
use codex_core::terminal::Multiplexer;
|
||||
use codex_protocol::config_types::AltScreenMode;
|
||||
use codex_protocol::config_types::SandboxMode;
|
||||
@@ -246,7 +248,6 @@ pub async fn run_main(
|
||||
std::process::exit(1);
|
||||
}
|
||||
|
||||
let active_profile = config.active_profile.clone();
|
||||
let log_dir = codex_core::config::log_dir(&config)?;
|
||||
std::fs::create_dir_all(&log_dir)?;
|
||||
// Open (or create) your log file, appending to it.
|
||||
@@ -336,16 +337,9 @@ pub async fn run_main(
|
||||
.with(otel_tracing_layer)
|
||||
.try_init();
|
||||
|
||||
run_ratatui_app(
|
||||
cli,
|
||||
config,
|
||||
overrides,
|
||||
cli_kv_overrides,
|
||||
active_profile,
|
||||
feedback,
|
||||
)
|
||||
.await
|
||||
.map_err(|err| std::io::Error::other(err.to_string()))
|
||||
run_ratatui_app(cli, config, overrides, cli_kv_overrides, feedback)
|
||||
.await
|
||||
.map_err(|err| std::io::Error::other(err.to_string()))
|
||||
}
|
||||
|
||||
async fn run_ratatui_app(
|
||||
@@ -353,7 +347,6 @@ async fn run_ratatui_app(
|
||||
initial_config: Config,
|
||||
overrides: ConfigOverrides,
|
||||
cli_kv_overrides: Vec<(String, toml::Value)>,
|
||||
active_profile: Option<String>,
|
||||
feedback: codex_feedback::CodexFeedback,
|
||||
) -> color_eyre::Result<AppExitInfo> {
|
||||
color_eyre::install()?;
|
||||
@@ -404,15 +397,15 @@ async fn run_ratatui_app(
|
||||
initial_config.cli_auth_credentials_store_mode,
|
||||
);
|
||||
let login_status = get_login_status(&initial_config);
|
||||
let should_show_trust_screen = should_show_trust_screen(&initial_config);
|
||||
let should_show_trust_screen_flag = should_show_trust_screen(&initial_config);
|
||||
let should_show_onboarding =
|
||||
should_show_onboarding(login_status, &initial_config, should_show_trust_screen);
|
||||
should_show_onboarding(login_status, &initial_config, should_show_trust_screen_flag);
|
||||
|
||||
let config = if should_show_onboarding {
|
||||
let onboarding_result = run_onboarding_app(
|
||||
OnboardingScreenArgs {
|
||||
show_login_screen: should_show_login_screen(login_status, &initial_config),
|
||||
show_trust_screen: should_show_trust_screen,
|
||||
show_trust_screen: should_show_trust_screen_flag,
|
||||
login_status,
|
||||
auth_manager: auth_manager.clone(),
|
||||
config: initial_config.clone(),
|
||||
@@ -437,7 +430,7 @@ async fn run_ratatui_app(
|
||||
.map(|d| d == TrustDirectorySelection::Trust)
|
||||
.unwrap_or(false)
|
||||
{
|
||||
load_config_or_exit(cli_kv_overrides, overrides).await
|
||||
load_config_or_exit(cli_kv_overrides.clone(), overrides.clone()).await
|
||||
} else {
|
||||
initial_config
|
||||
}
|
||||
@@ -570,6 +563,33 @@ async fn run_ratatui_app(
|
||||
resume_picker::SessionSelection::StartFresh
|
||||
};
|
||||
|
||||
let config = match &session_selection {
|
||||
resume_picker::SessionSelection::Resume(path)
|
||||
| resume_picker::SessionSelection::Fork(path) => {
|
||||
let history_cwd = match read_session_meta_line(path).await {
|
||||
Ok(meta_line) => Some(meta_line.meta.cwd),
|
||||
Err(err) => {
|
||||
let rollout_path = path.display().to_string();
|
||||
tracing::warn!(
|
||||
%rollout_path,
|
||||
%err,
|
||||
"Failed to read session metadata from rollout"
|
||||
);
|
||||
None
|
||||
}
|
||||
};
|
||||
load_config_or_exit_with_fallback_cwd(
|
||||
cli_kv_overrides.clone(),
|
||||
overrides.clone(),
|
||||
history_cwd,
|
||||
)
|
||||
.await
|
||||
}
|
||||
_ => config,
|
||||
};
|
||||
let active_profile = config.active_profile.clone();
|
||||
let should_show_trust_screen = should_show_trust_screen(&config);
|
||||
|
||||
let Cli {
|
||||
prompt,
|
||||
images,
|
||||
@@ -671,9 +691,23 @@ fn get_login_status(config: &Config) -> LoginStatus {
|
||||
async fn load_config_or_exit(
|
||||
cli_kv_overrides: Vec<(String, toml::Value)>,
|
||||
overrides: ConfigOverrides,
|
||||
) -> Config {
|
||||
load_config_or_exit_with_fallback_cwd(cli_kv_overrides, overrides, None).await
|
||||
}
|
||||
|
||||
async fn load_config_or_exit_with_fallback_cwd(
|
||||
cli_kv_overrides: Vec<(String, toml::Value)>,
|
||||
overrides: ConfigOverrides,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> Config {
|
||||
#[allow(clippy::print_stderr)]
|
||||
match Config::load_with_cli_overrides_and_harness_overrides(cli_kv_overrides, overrides).await {
|
||||
match ConfigBuilder::default()
|
||||
.cli_overrides(cli_kv_overrides)
|
||||
.harness_overrides(overrides)
|
||||
.fallback_cwd(fallback_cwd)
|
||||
.build()
|
||||
.await
|
||||
{
|
||||
Ok(config) => config,
|
||||
Err(err) => {
|
||||
eprintln!("Error loading configuration: {err}");
|
||||
|
||||
+51
-17
@@ -18,6 +18,7 @@ use codex_core::RolloutRecorder;
|
||||
use codex_core::ThreadSortKey;
|
||||
use codex_core::auth::enforce_login_restrictions;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_core::config::find_codex_home;
|
||||
use codex_core::config::load_config_as_toml_with_cli_overrides;
|
||||
@@ -25,6 +26,7 @@ use codex_core::config::resolve_oss_provider;
|
||||
use codex_core::find_thread_path_by_id_str;
|
||||
use codex_core::get_platform_sandbox;
|
||||
use codex_core::protocol::AskForApproval;
|
||||
use codex_core::read_session_meta_line;
|
||||
use codex_core::terminal::Multiplexer;
|
||||
use codex_protocol::config_types::AltScreenMode;
|
||||
use codex_protocol::config_types::SandboxMode;
|
||||
@@ -262,7 +264,6 @@ pub async fn run_main(
|
||||
std::process::exit(1);
|
||||
}
|
||||
|
||||
let active_profile = config.active_profile.clone();
|
||||
let log_dir = codex_core::config::log_dir(&config)?;
|
||||
std::fs::create_dir_all(&log_dir)?;
|
||||
// Open (or create) your log file, appending to it.
|
||||
@@ -355,16 +356,9 @@ pub async fn run_main(
|
||||
let terminal_info = codex_core::terminal::terminal_info();
|
||||
tracing::info!(terminal = ?terminal_info, "Detected terminal info");
|
||||
|
||||
run_ratatui_app(
|
||||
cli,
|
||||
config,
|
||||
overrides,
|
||||
cli_kv_overrides,
|
||||
active_profile,
|
||||
feedback,
|
||||
)
|
||||
.await
|
||||
.map_err(|err| std::io::Error::other(err.to_string()))
|
||||
run_ratatui_app(cli, config, overrides, cli_kv_overrides, feedback)
|
||||
.await
|
||||
.map_err(|err| std::io::Error::other(err.to_string()))
|
||||
}
|
||||
|
||||
async fn run_ratatui_app(
|
||||
@@ -372,7 +366,6 @@ async fn run_ratatui_app(
|
||||
initial_config: Config,
|
||||
overrides: ConfigOverrides,
|
||||
cli_kv_overrides: Vec<(String, toml::Value)>,
|
||||
active_profile: Option<String>,
|
||||
feedback: codex_feedback::CodexFeedback,
|
||||
) -> color_eyre::Result<AppExitInfo> {
|
||||
color_eyre::install()?;
|
||||
@@ -424,15 +417,15 @@ async fn run_ratatui_app(
|
||||
initial_config.cli_auth_credentials_store_mode,
|
||||
);
|
||||
let login_status = get_login_status(&initial_config);
|
||||
let should_show_trust_screen = should_show_trust_screen(&initial_config);
|
||||
let should_show_trust_screen_flag = should_show_trust_screen(&initial_config);
|
||||
let should_show_onboarding =
|
||||
should_show_onboarding(login_status, &initial_config, should_show_trust_screen);
|
||||
should_show_onboarding(login_status, &initial_config, should_show_trust_screen_flag);
|
||||
|
||||
let config = if should_show_onboarding {
|
||||
let onboarding_result = run_onboarding_app(
|
||||
OnboardingScreenArgs {
|
||||
show_login_screen: should_show_login_screen(login_status, &initial_config),
|
||||
show_trust_screen: should_show_trust_screen,
|
||||
show_trust_screen: should_show_trust_screen_flag,
|
||||
login_status,
|
||||
auth_manager: auth_manager.clone(),
|
||||
config: initial_config.clone(),
|
||||
@@ -458,7 +451,7 @@ async fn run_ratatui_app(
|
||||
.map(|d| d == TrustDirectorySelection::Trust)
|
||||
.unwrap_or(false)
|
||||
{
|
||||
load_config_or_exit(cli_kv_overrides, overrides).await
|
||||
load_config_or_exit(cli_kv_overrides.clone(), overrides.clone()).await
|
||||
} else {
|
||||
initial_config
|
||||
}
|
||||
@@ -594,6 +587,33 @@ async fn run_ratatui_app(
|
||||
resume_picker::SessionSelection::StartFresh
|
||||
};
|
||||
|
||||
let config = match &session_selection {
|
||||
resume_picker::SessionSelection::Resume(path)
|
||||
| resume_picker::SessionSelection::Fork(path) => {
|
||||
let history_cwd = match read_session_meta_line(path).await {
|
||||
Ok(meta_line) => Some(meta_line.meta.cwd),
|
||||
Err(err) => {
|
||||
let rollout_path = path.display().to_string();
|
||||
tracing::warn!(
|
||||
%rollout_path,
|
||||
%err,
|
||||
"Failed to read session metadata from rollout"
|
||||
);
|
||||
None
|
||||
}
|
||||
};
|
||||
load_config_or_exit_with_fallback_cwd(
|
||||
cli_kv_overrides.clone(),
|
||||
overrides.clone(),
|
||||
history_cwd,
|
||||
)
|
||||
.await
|
||||
}
|
||||
_ => config,
|
||||
};
|
||||
let active_profile = config.active_profile.clone();
|
||||
let should_show_trust_screen = should_show_trust_screen(&config);
|
||||
|
||||
let Cli {
|
||||
prompt,
|
||||
images,
|
||||
@@ -699,9 +719,23 @@ fn get_login_status(config: &Config) -> LoginStatus {
|
||||
async fn load_config_or_exit(
|
||||
cli_kv_overrides: Vec<(String, toml::Value)>,
|
||||
overrides: ConfigOverrides,
|
||||
) -> Config {
|
||||
load_config_or_exit_with_fallback_cwd(cli_kv_overrides, overrides, None).await
|
||||
}
|
||||
|
||||
async fn load_config_or_exit_with_fallback_cwd(
|
||||
cli_kv_overrides: Vec<(String, toml::Value)>,
|
||||
overrides: ConfigOverrides,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> Config {
|
||||
#[allow(clippy::print_stderr)]
|
||||
match Config::load_with_cli_overrides_and_harness_overrides(cli_kv_overrides, overrides).await {
|
||||
match ConfigBuilder::default()
|
||||
.cli_overrides(cli_kv_overrides)
|
||||
.harness_overrides(overrides)
|
||||
.fallback_cwd(fallback_cwd)
|
||||
.build()
|
||||
.await
|
||||
{
|
||||
Ok(config) => config,
|
||||
Err(err) => {
|
||||
eprintln!("Error loading configuration: {err}");
|
||||
|
||||
Reference in New Issue
Block a user