diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index ca588e59d4..6280dd0cc4 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -21,6 +21,7 @@ use codex_core::ExecPolicyError; use codex_core::check_execpolicy_for_warnings; use codex_core::config_loader::ConfigLoadError; use codex_core::config_loader::TextRange as CoreTextRange; +use codex_core::config_loader::format_expansion_warnings; use codex_feedback::CodexFeedback; use tokio::io::AsyncBufReadExt; use tokio::io::AsyncWriteExt; @@ -163,6 +164,20 @@ fn project_config_warning(config: &Config) -> Option }) } +fn expansion_warning(config: &Config) -> Option { + let warnings = config.config_layer_stack.expansion_warnings(); + if warnings.is_empty() { + return None; + } + + Some(ConfigWarningNotification { + summary: format_expansion_warnings(&warnings), + details: None, + path: None, + range: None, + }) +} + pub async fn run_main( codex_linux_sandbox_exe: Option, cli_config_overrides: CliConfigOverrides, @@ -241,6 +256,9 @@ pub async fn run_main( if let Some(warning) = project_config_warning(&config) { config_warnings.push(warning); } + if let Some(warning) = expansion_warning(&config) { + config_warnings.push(warning); + } let feedback = CodexFeedback::new(); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 4e49efabda..4e0d3e1960 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -19,10 +19,13 @@ use crate::config::types::Tui; use crate::config::types::UriBasedFileOpener; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigRequirements; +use crate::config_loader::EnvProvider; use crate::config_loader::LoaderOverrides; use crate::config_loader::McpServerIdentity; use crate::config_loader::McpServerRequirement; +use crate::config_loader::RealEnv; use crate::config_loader::Sourced; +use crate::config_loader::expand_key_for_matching_with_env; use crate::config_loader::load_config_layers_state; use crate::features::Feature; use crate::features::FeatureOverrides; @@ -59,7 +62,6 @@ use dirs::home_dir; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; -use similar::DiffableStr; use std::collections::BTreeMap; use std::collections::HashMap; use std::io::ErrorKind; @@ -646,6 +648,15 @@ pub(crate) fn set_project_trust_level_inner( doc: &mut DocumentMut, project_path: &Path, trust_level: TrustLevel, +) -> anyhow::Result<()> { + set_project_trust_level_inner_with_env(doc, project_path, trust_level, &RealEnv) +} + +fn set_project_trust_level_inner_with_env( + doc: &mut DocumentMut, + project_path: &Path, + trust_level: TrustLevel, + env: &impl EnvProvider, ) -> anyhow::Result<()> { // Ensure we render a human-friendly structure: // @@ -657,7 +668,8 @@ pub(crate) fn set_project_trust_level_inner( // // [projects] // "/path/to/project" = { trust_level = "trusted" } - let project_key = project_path.to_string_lossy().to_string(); + let target_key = project_path.to_string_lossy().to_string(); + let normalized_target_key = normalize_path_for_matching(&target_key); // Ensure top-level `projects` exists as a non-inline, explicit table. If it // exists but was previously represented as a non-table (e.g., inline), @@ -690,6 +702,25 @@ pub(crate) fn set_project_trust_level_inner( )); }; + let mut matched_symbolic_key: Option = None; + let mut matched_non_symbolic_key: Option = None; + for existing_key in projects_tbl.iter().map(|(key, _)| key.to_string()) { + let expanded = expand_key_for_matching_with_env(&existing_key, env); + if normalize_path_for_matching(&expanded) != normalized_target_key { + continue; + } + if is_symbolic_project_key(&existing_key) { + if matched_symbolic_key.is_none() { + matched_symbolic_key = Some(existing_key); + } + } else if matched_non_symbolic_key.is_none() { + matched_non_symbolic_key = Some(existing_key); + } + } + let project_key = matched_symbolic_key + .or(matched_non_symbolic_key) + .unwrap_or_else(|| target_key.clone()); + // Ensure the per-project entry is its own explicit table. If it exists but // is not a table (e.g., an inline table), replace it with an explicit table. let needs_proj_table = !projects_tbl.contains_key(project_key.as_str()) @@ -708,9 +739,96 @@ pub(crate) fn set_project_trust_level_inner( }; proj_tbl.set_implicit(false); proj_tbl["trust_level"] = toml_edit::value(trust_level.to_string()); + + let symbolic_match_exists = projects_tbl.iter().any(|(key, _)| { + if key == target_key.as_str() || !is_symbolic_project_key(key) { + return false; + } + let expanded = expand_key_for_matching_with_env(key, env); + normalize_path_for_matching(&expanded) == normalized_target_key + }); + let should_remove_absolute = project_key != target_key + && symbolic_match_exists + && projects_tbl + .get(target_key.as_str()) + .is_some_and(is_trust_level_only_table); + if should_remove_absolute { + projects_tbl.remove(target_key.as_str()); + } Ok(()) } +fn find_matching_project_config( + projects: &HashMap, + normalized_target_key: &str, + env: &impl EnvProvider, +) -> Option { + let mut best: Option<(String, ProjectConfig)> = None; + for (key, project_config) in projects { + let expanded_key = expand_key_for_matching_with_env(key, env); + let normalized_key = normalize_path_for_matching(&expanded_key); + if normalized_key != normalized_target_key { + continue; + } + let candidate = (key.clone(), project_config.clone()); + if best.as_ref().is_none_or(|(best_key, best_config)| { + is_better_project_match( + best_key.as_str(), + best_config, + candidate.0.as_str(), + &candidate.1, + ) + }) { + best = Some(candidate); + } + } + best.map(|(_, config)| config) +} + +fn is_better_project_match( + existing_key: &str, + existing_config: &ProjectConfig, + candidate_key: &str, + candidate_config: &ProjectConfig, +) -> bool { + let existing_rank = project_trust_rank(existing_config.trust_level); + let candidate_rank = project_trust_rank(candidate_config.trust_level); + if existing_rank != candidate_rank { + return candidate_rank > existing_rank; + } + let existing_symbolic = is_symbolic_project_key(existing_key); + let candidate_symbolic = is_symbolic_project_key(candidate_key); + if existing_symbolic != candidate_symbolic { + return candidate_symbolic; + } + candidate_key < existing_key +} + +fn project_trust_rank(level: Option) -> u8 { + match level { + Some(TrustLevel::Untrusted) => 2, + Some(TrustLevel::Trusted) => 1, + None => 0, + } +} + +fn is_symbolic_project_key(key: &str) -> bool { + key.starts_with('~') || key.contains('$') +} + +fn is_trust_level_only_table(item: &toml_edit::Item) -> bool { + item.as_table() + .is_some_and(|table| table.len() == 1 && table.contains_key("trust_level")) +} + +fn normalize_path_for_matching(path: &str) -> String { + let path_buf = PathBuf::from(path); + std::fs::canonicalize(&path_buf) + .unwrap_or(path_buf) + .to_string_lossy() + .to_string() +} + /// Patch `CODEX_HOME/config.toml` project state to set trust level. /// Use with caution. pub fn set_project_trust_level( @@ -1111,20 +1229,38 @@ impl ConfigToml { /// Resolves the cwd to an existing project, or returns None if ConfigToml /// does not contain a project corresponding to cwd or a git repo for cwd pub fn get_active_project(&self, resolved_cwd: &Path) -> Option { - let projects = self.projects.clone().unwrap_or_default(); + self.get_active_project_with_env(resolved_cwd, &RealEnv) + } - if let Some(project_config) = projects.get(&resolved_cwd.to_string_lossy().to_string()) { - return Some(project_config.clone()); + fn get_active_project_with_env( + &self, + resolved_cwd: &Path, + env: &impl EnvProvider, + ) -> Option { + let projects = self.projects.clone().unwrap_or_default(); + if projects.is_empty() { + return None; + } + + let resolved_cwd_key = resolved_cwd.to_string_lossy().to_string(); + let normalized_cwd_key = normalize_path_for_matching(&resolved_cwd_key); + if let Some(project_config) = + find_matching_project_config(&projects, &normalized_cwd_key, env) + { + return Some(project_config); } // If cwd lives inside a git repo/worktree, check whether the root git project // (the primary repository working directory) is trusted. This lets // worktrees inherit trust from the main project. - if let Some(repo_root) = resolve_root_git_project_for_trust(resolved_cwd) - && let Some(project_config_for_root) = - projects.get(&repo_root.to_string_lossy().to_string_lossy().to_string()) - { - return Some(project_config_for_root.clone()); + if let Some(repo_root) = resolve_root_git_project_for_trust(resolved_cwd) { + let repo_root_key = repo_root.to_string_lossy().to_string(); + let normalized_repo_root_key = normalize_path_for_matching(&repo_root_key); + if let Some(project_config_for_root) = + find_matching_project_config(&projects, &normalized_repo_root_key, env) + { + return Some(project_config_for_root); + } } None @@ -1757,6 +1893,7 @@ mod tests { use crate::config::types::HistoryPersistence; use crate::config::types::McpServerTransportConfig; use crate::config::types::Notifications; + use crate::config_loader::FakeEnv; use crate::config_loader::RequirementSource; use crate::features::Feature; @@ -4157,6 +4294,191 @@ trust_level = "trusted" Ok(()) } + #[test] + fn test_set_project_trust_level_updates_symbolic_tilde_key() -> anyhow::Result<()> { + let mut doc = r#"[projects."~"] +trust_level = "untrusted" +"# + .parse::()?; + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let project_dir = Path::new("/Users/tester"); + + set_project_trust_level_inner_with_env(&mut doc, project_dir, TrustLevel::Trusted, &env)?; + + let expected = r#"[projects."~"] +trust_level = "trusted" +"#; + assert_eq!(doc.to_string(), expected); + Ok(()) + } + + #[test] + fn test_set_project_trust_level_updates_symbolic_home_key() -> anyhow::Result<()> { + let mut doc = r#"[projects."$HOME"] +trust_level = "untrusted" +"# + .parse::()?; + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let project_dir = Path::new("/Users/tester"); + + set_project_trust_level_inner_with_env(&mut doc, project_dir, TrustLevel::Trusted, &env)?; + + let expected = r#"[projects."$HOME"] +trust_level = "trusted" +"#; + assert_eq!(doc.to_string(), expected); + Ok(()) + } + + #[test] + fn test_get_active_project_matches_symbolic_tilde_key() { + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let cfg = ConfigToml { + projects: Some(HashMap::from([( + "~".to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Trusted), + }, + )])), + ..Default::default() + }; + + let active = cfg.get_active_project_with_env(Path::new("/Users/tester"), &env); + assert_eq!( + active, + Some(ProjectConfig { + trust_level: Some(TrustLevel::Trusted), + }) + ); + } + + #[test] + fn test_get_active_project_matches_symbolic_home_key() { + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let cfg = ConfigToml { + projects: Some(HashMap::from([( + "$HOME".to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Untrusted), + }, + )])), + ..Default::default() + }; + + let active = cfg.get_active_project_with_env(Path::new("/Users/tester"), &env); + assert_eq!( + active, + Some(ProjectConfig { + trust_level: Some(TrustLevel::Untrusted), + }) + ); + } + + #[test] + fn test_set_project_trust_level_removes_absolute_duplicate_when_symbolic_exists() + -> anyhow::Result<()> { + let mut doc = r#"[projects."~"] +trust_level = "trusted" + +[projects."/Users/tester"] +trust_level = "trusted" +"# + .parse::()?; + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let project_dir = Path::new("/Users/tester"); + + set_project_trust_level_inner_with_env(&mut doc, project_dir, TrustLevel::Trusted, &env)?; + + let actual: toml::Value = toml::from_str(&doc.to_string())?; + let expected: toml::Value = toml::from_str( + r#"[projects."~"] +trust_level = "trusted" +"#, + )?; + assert_eq!(actual, expected); + Ok(()) + } + + #[test] + fn test_set_project_trust_level_prefers_symbolic_key_when_absolute_appears_first() + -> anyhow::Result<()> { + let mut doc = r#"[projects."/Users/tester"] +trust_level = "trusted" + +[projects."~"] +trust_level = "untrusted" +"# + .parse::()?; + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let project_dir = Path::new("/Users/tester"); + + set_project_trust_level_inner_with_env(&mut doc, project_dir, TrustLevel::Trusted, &env)?; + + let actual: toml::Value = toml::from_str(&doc.to_string())?; + let expected: toml::Value = toml::from_str( + r#"[projects."~"] +trust_level = "trusted" +"#, + )?; + assert_eq!(actual, expected); + Ok(()) + } + + #[test] + fn test_set_project_trust_level_keeps_absolute_entry_when_it_has_extra_fields() + -> anyhow::Result<()> { + let mut doc = r#"[projects."/Users/tester"] +trust_level = "trusted" +note = "keep me" + +[projects."~"] +trust_level = "untrusted" +"# + .parse::()?; + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let project_dir = Path::new("/Users/tester"); + + set_project_trust_level_inner_with_env(&mut doc, project_dir, TrustLevel::Trusted, &env)?; + + let actual: toml::Value = toml::from_str(&doc.to_string())?; + let expected: toml::Value = toml::from_str( + r#"[projects."/Users/tester"] +trust_level = "trusted" +note = "keep me" + +[projects."~"] +trust_level = "trusted" +"#, + )?; + assert_eq!(actual, expected); + Ok(()) + } + + #[test] + fn test_set_project_trust_level_prefers_symbolic_home_key_when_absolute_appears_first() + -> anyhow::Result<()> { + let mut doc = r#"[projects."/Users/tester"] +trust_level = "trusted" + +[projects."$HOME"] +trust_level = "untrusted" +"# + .parse::()?; + let env = FakeEnv::new([("HOME", "/Users/tester")]); + let project_dir = Path::new("/Users/tester"); + + set_project_trust_level_inner_with_env(&mut doc, project_dir, TrustLevel::Trusted, &env)?; + + let actual: toml::Value = toml::from_str(&doc.to_string())?; + let expected: toml::Value = toml::from_str( + r#"[projects."$HOME"] +trust_level = "trusted" +"#, + )?; + assert_eq!(actual, expected); + Ok(()) + } + #[test] fn test_set_project_trusted_migrates_top_level_inline_projects_preserving_entries() -> anyhow::Result<()> { diff --git a/codex-rs/core/src/config_loader/expansion.rs b/codex-rs/core/src/config_loader/expansion.rs new file mode 100644 index 0000000000..420c1fa1a9 --- /dev/null +++ b/codex-rs/core/src/config_loader/expansion.rs @@ -0,0 +1,358 @@ +use std::collections::BTreeSet; +use std::collections::HashMap; + +use dirs::home_dir; +use serde_json::json; +use toml::Value as TomlValue; + +pub const KEY_COLLISION_SENTINEL: &str = "KEY_COLLISION"; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ConfigExpansionWarning { + pub var: String, + pub path: String, +} + +impl ConfigExpansionWarning { + pub fn new(var: impl Into, path: impl Into) -> Self { + Self { + var: var.into(), + path: path.into(), + } + } +} + +pub(crate) trait EnvProvider { + fn get(&self, key: &str) -> Option; +} + +#[derive(Debug, Default, Clone, Copy)] +pub(crate) struct RealEnv; + +impl EnvProvider for RealEnv { + fn get(&self, key: &str) -> Option { + std::env::var(key).ok() + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum PathSegment { + Key(String), + Index(usize), +} + +pub(crate) struct ExpansionResult { + pub value: TomlValue, + pub warnings: Vec, +} + +pub(crate) fn expand_config_toml(value: TomlValue) -> ExpansionResult { + expand_config_toml_with_env(value, &RealEnv) +} + +pub(crate) fn expand_key_for_matching_with_env(key: &str, env: &impl EnvProvider) -> String { + if key == "~" { + let home_var = if cfg!(windows) { "USERPROFILE" } else { "HOME" }; + if let Some(home) = env.get(home_var) { + return home; + } + if let Some(home) = home_dir() { + return home.to_string_lossy().to_string(); + } + return key.to_string(); + } + let mut warnings = Vec::new(); + expand_string(key, "", env, &mut warnings) +} + +pub(crate) fn expand_config_toml_with_env( + value: TomlValue, + env: &impl EnvProvider, +) -> ExpansionResult { + let mut warnings = Vec::new(); + let mut path = Vec::new(); + let value = expand_value(value, &mut path, env, &mut warnings); + ExpansionResult { value, warnings } +} + +fn expand_value( + value: TomlValue, + path: &mut Vec, + env: &impl EnvProvider, + warnings: &mut Vec, +) -> TomlValue { + match value { + TomlValue::String(value) => { + let path_display = format_path(path); + TomlValue::String(expand_string(&value, &path_display, env, warnings)) + } + TomlValue::Array(values) => { + let mut expanded = Vec::with_capacity(values.len()); + for (index, value) in values.into_iter().enumerate() { + path.push(PathSegment::Index(index)); + expanded.push(expand_value(value, path, env, warnings)); + path.pop(); + } + TomlValue::Array(expanded) + } + TomlValue::Table(values) => { + let mut expanded = toml::map::Map::new(); + let mut original_key_by_expanded_key: HashMap = HashMap::new(); + for (key, value) in values { + path.push(PathSegment::Key(key.clone())); + let parent_path_display = format_path(&path[..path.len().saturating_sub(1)]); + let key_path_display = format_path(path); + let expanded_key = expand_string(&key, &key_path_display, env, warnings); + let expanded_value = expand_value(value, path, env, warnings); + path.pop(); + if let Some(previous_original_key) = + original_key_by_expanded_key.get(&expanded_key).cloned() + { + warnings.push(new_key_collision_warning( + parent_path_display, + expanded_key, + previous_original_key, + key, + )); + continue; + } + original_key_by_expanded_key.insert(expanded_key.clone(), key); + expanded.insert(expanded_key, expanded_value); + } + TomlValue::Table(expanded) + } + value => value, + } +} + +fn new_key_collision_warning( + parent_path: String, + expanded_key: String, + original_key_a: String, + original_key_b: String, +) -> ConfigExpansionWarning { + // Encode collision details into the existing warning shape to avoid a breaking API change. + let path = json!({ + "path": parent_path, + "expanded_key": expanded_key, + "original_keys": [original_key_a, original_key_b], + }) + .to_string(); + ConfigExpansionWarning::new(KEY_COLLISION_SENTINEL, path) +} + +fn expand_string( + input: &str, + path: &str, + env: &impl EnvProvider, + warnings: &mut Vec, +) -> String { + let mut missing_vars = BTreeSet::new(); + let input = expand_tilde_prefix(input, env, &mut missing_vars); + let mut output = String::with_capacity(input.len()); + let mut iter = input.chars().peekable(); + + while let Some(ch) = iter.next() { + if ch != '$' { + output.push(ch); + continue; + } + + let Some(&next) = iter.peek() else { + output.push('$'); + continue; + }; + + if next == '$' { + output.push('$'); + iter.next(); + continue; + } + + if next == '{' { + iter.next(); + let mut name = String::new(); + let mut closed = false; + while let Some(&ch) = iter.peek() { + iter.next(); + if ch == '}' { + closed = true; + break; + } + name.push(ch); + } + + if !closed || !is_valid_env_name(&name) { + output.push_str("${"); + output.push_str(&name); + if closed { + output.push('}'); + } + continue; + } + + if let Some(value) = env.get(&name) { + output.push_str(&value); + } else { + missing_vars.insert(name.clone()); + output.push_str("${"); + output.push_str(&name); + output.push('}'); + } + continue; + } + + if !is_valid_env_start(next) { + output.push('$'); + continue; + } + + let mut name = String::new(); + name.push(next); + iter.next(); + while let Some(&ch) = iter.peek() { + if is_valid_env_continue(ch) { + name.push(ch); + iter.next(); + } else { + break; + } + } + + if let Some(value) = env.get(&name) { + output.push_str(&value); + } else { + missing_vars.insert(name.clone()); + output.push('$'); + output.push_str(&name); + } + } + + for var in missing_vars { + warnings.push(ConfigExpansionWarning::new(var, path.to_string())); + } + + output +} + +fn expand_tilde_prefix( + input: &str, + env: &impl EnvProvider, + missing_vars: &mut BTreeSet, +) -> String { + if !input.starts_with("~/") && !input.starts_with("~\\") { + return input.to_string(); + } + + let home_var = if cfg!(windows) { "USERPROFILE" } else { "HOME" }; + match env.get(home_var) { + Some(home) => { + let mut expanded = String::with_capacity(home.len() + input.len().saturating_sub(1)); + expanded.push_str(&home); + expanded.push_str(&input[1..]); + expanded + } + None => { + missing_vars.insert(home_var.to_string()); + input.to_string() + } + } +} + +fn is_valid_env_start(ch: char) -> bool { + ch == '_' || ch.is_ascii_alphabetic() +} + +fn is_valid_env_continue(ch: char) -> bool { + ch == '_' || ch.is_ascii_alphanumeric() +} + +fn is_valid_env_name(name: &str) -> bool { + let mut chars = name.chars(); + let Some(first) = chars.next() else { + return false; + }; + if !is_valid_env_start(first) { + return false; + } + chars.all(is_valid_env_continue) +} + +fn format_path(segments: &[PathSegment]) -> String { + if segments.is_empty() { + return "".to_string(); + } + + let mut output = String::new(); + for segment in segments { + match segment { + PathSegment::Key(key) => { + if is_simple_key(key) { + if !output.is_empty() { + output.push('.'); + } + output.push_str(key); + } else { + output.push_str("[\""); + output.push_str(&escape_key(key)); + output.push_str("\"]"); + } + } + PathSegment::Index(index) => { + output.push('['); + output.push_str(&index.to_string()); + output.push(']'); + } + } + } + output +} + +fn is_simple_key(key: &str) -> bool { + let mut chars = key.chars(); + let Some(first) = chars.next() else { + return false; + }; + if !(first.is_ascii_alphabetic() || first == '_') { + return false; + } + chars.all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '-') +} + +fn escape_key(key: &str) -> String { + let mut escaped = String::with_capacity(key.len()); + for ch in key.chars() { + match ch { + '\\' => escaped.push_str("\\\\"), + '"' => escaped.push_str("\\\""), + _ => escaped.push(ch), + } + } + escaped +} + +#[cfg(test)] +pub(crate) struct FakeEnv { + vars: std::collections::HashMap, +} + +#[cfg(test)] +impl FakeEnv { + pub(crate) fn new( + vars: impl IntoIterator, impl Into)>, + ) -> Self { + Self { + vars: vars + .into_iter() + .map(|(k, v)| (k.into(), v.into())) + .collect(), + } + } +} + +#[cfg(test)] +impl EnvProvider for FakeEnv { + fn get(&self, key: &str) -> Option { + self.vars.get(key).cloned() + } +} diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index ae56c384e4..f2fc27e35f 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -1,5 +1,6 @@ mod config_requirements; mod diagnostics; +mod expansion; mod fingerprint; mod layer_io; #[cfg(target_os = "macos")] @@ -24,8 +25,10 @@ use codex_protocol::protocol::AskForApproval; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::Deserialize; +use std::collections::HashMap; use std::io; use std::path::Path; +use std::path::PathBuf; use toml::Value as TomlValue; pub use config_requirements::ConfigRequirements; @@ -45,8 +48,19 @@ pub(crate) use diagnostics::first_layer_config_error_from_entries; pub use diagnostics::format_config_error; pub use diagnostics::format_config_error_with_source; pub(crate) use diagnostics::io_error_from_config_error; +pub use expansion::ConfigExpansionWarning; +pub(crate) use expansion::EnvProvider; +#[cfg(test)] +pub(crate) use expansion::FakeEnv; +use expansion::KEY_COLLISION_SENTINEL; +pub(crate) use expansion::RealEnv; +pub(crate) use expansion::expand_config_toml; +#[cfg(test)] +pub(crate) use expansion::expand_config_toml_with_env; +pub(crate) use expansion::expand_key_for_matching_with_env; pub use merge::merge_toml_values; pub(crate) use overrides::build_cli_overrides_layer; +pub use state::ConfigExpansionWarningInfo; pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; pub use state::ConfigLayerStackOrdering; @@ -62,6 +76,78 @@ pub const SYSTEM_CONFIG_TOML_FILE_UNIX: &str = "/etc/codex/config.toml"; const DEFAULT_PROJECT_ROOT_MARKERS: &[&str] = &[".git"]; +pub fn config_layer_source_display(source: &ConfigLayerSource) -> String { + match source { + ConfigLayerSource::System { file } => file.as_path().display().to_string(), + ConfigLayerSource::User { file } => file.as_path().display().to_string(), + ConfigLayerSource::Project { dot_codex_folder } => dot_codex_folder + .as_path() + .join(CONFIG_TOML_FILE) + .display() + .to_string(), + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file } => { + file.as_path().display().to_string() + } + ConfigLayerSource::Mdm { domain, key } => format!("MDM {domain}:{key}"), + ConfigLayerSource::LegacyManagedConfigTomlFromMdm => "MDM managed_config.toml".to_string(), + ConfigLayerSource::SessionFlags => "session flags".to_string(), + } +} + +pub fn format_expansion_warnings(warnings: &[ConfigExpansionWarningInfo]) -> String { + let mut message = + "Some config values could not expand environment variables and were left unchanged.\n" + .to_string(); + for (index, warning) in warnings.iter().enumerate() { + let display_index = index + 1; + let source = config_layer_source_display(&warning.source); + if warning.warning.var == KEY_COLLISION_SENTINEL { + message.push_str(&format_collision_warning( + display_index, + source.as_str(), + warning.warning.path.as_str(), + )); + continue; + } + let path = warning.warning.path.as_str(); + let var = warning.warning.var.as_str(); + message.push_str(&format!( + " {display_index}. {source}: {path} references ${var} which is not set\n" + )); + } + message +} + +fn format_collision_warning(display_index: usize, source: &str, path: &str) -> String { + let parsed = serde_json::from_str::(path); + if let Ok(value) = parsed { + let parent_path = value + .get("path") + .and_then(serde_json::Value::as_str) + .unwrap_or(""); + let expanded_key = value + .get("expanded_key") + .and_then(serde_json::Value::as_str) + .unwrap_or(""); + let original_keys = value + .get("original_keys") + .and_then(serde_json::Value::as_array) + .map(|keys| { + keys.iter() + .filter_map(serde_json::Value::as_str) + .map(|key| format!("\"{key}\"")) + .collect::>() + .join(" and ") + }) + .filter(|keys| !keys.is_empty()) + .unwrap_or_else(|| "\"\"".to_string()); + return format!( + " {display_index}. {source}: {parent_path} has duplicate keys after expansion: {original_keys} both expand to \"{expanded_key}\" (kept first)\n" + ); + } + format!(" {display_index}. {source}: duplicate keys after expansion (details unavailable)\n") +} + /// To build up the set of admin-enforced constraints, we build up from multiple /// configuration layers in the following order, but a constraint defined in an /// earlier layer cannot be overridden by a later layer: @@ -96,6 +182,16 @@ pub async fn load_config_layers_state( cwd: Option, cli_overrides: &[(String, TomlValue)], overrides: LoaderOverrides, +) -> io::Result { + load_config_layers_state_with_env(codex_home, cwd, cli_overrides, overrides, &RealEnv).await +} + +async fn load_config_layers_state_with_env( + codex_home: &Path, + cwd: Option, + cli_overrides: &[(String, TomlValue)], + overrides: LoaderOverrides, + env: &impl EnvProvider, ) -> io::Result { let mut config_requirements_toml = ConfigRequirementsWithSources::default(); @@ -201,6 +297,7 @@ pub async fn load_config_layers_state( &project_root_markers, codex_home, &user_file, + env, ) .await { @@ -231,10 +328,13 @@ pub async fn load_config_layers_state( // Add a layer for runtime overrides from the CLI or UI, if any exist. if let Some(cli_overrides_layer) = cli_overrides_layer { - layers.push(ConfigLayerEntry::new( + let expanded = expand_config_toml(cli_overrides_layer); + let entry = ConfigLayerEntry::new_with_warnings( ConfigLayerSource::SessionFlags, - cli_overrides_layer, - )); + expanded.value, + expanded.warnings, + ); + layers.push(entry); } // Make a best-effort to support the legacy `managed_config.toml` as a @@ -256,18 +356,23 @@ pub async fn load_config_layers_state( ), ) })?; - let managed_config = - resolve_relative_paths_in_config_toml(config.managed_config, managed_parent)?; - layers.push(ConfigLayerEntry::new( + let expanded = expand_config_toml(config.managed_config); + let managed_config = resolve_relative_paths_in_config_toml(expanded.value, managed_parent)?; + let entry = ConfigLayerEntry::new_with_warnings( ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: config.file }, managed_config, - )); + expanded.warnings, + ); + layers.push(entry); } if let Some(config) = managed_config_from_mdm { - layers.push(ConfigLayerEntry::new( + let expanded = expand_config_toml(config); + let entry = ConfigLayerEntry::new_with_warnings( ConfigLayerSource::LegacyManagedConfigTomlFromMdm, - config, - )); + expanded.value, + expanded.warnings, + ); + layers.push(entry); } ConfigLayerStack::new( @@ -277,6 +382,17 @@ pub async fn load_config_layers_state( ) } +#[cfg(test)] +pub(crate) async fn load_config_layers_state_with_env_for_tests( + codex_home: &Path, + cwd: Option, + cli_overrides: &[(String, TomlValue)], + overrides: LoaderOverrides, + env: &impl EnvProvider, +) -> io::Result { + load_config_layers_state_with_env(codex_home, cwd, cli_overrides, overrides, env).await +} + /// Attempts to load a config.toml file from `config_toml`. /// - If the file exists and is valid TOML, passes the parsed `toml::Value` to /// `create_entry` and returns the resulting layer entry. @@ -289,7 +405,7 @@ async fn load_config_toml_for_required_layer( create_entry: impl FnOnce(TomlValue) -> ConfigLayerEntry, ) -> io::Result { let toml_file = config_toml.as_ref(); - let toml_value = match tokio::fs::read_to_string(toml_file).await { + let (toml_value, warnings) = match tokio::fs::read_to_string(toml_file).await { Ok(contents) => { let config: TomlValue = toml::from_str(&contents).map_err(|err| { let config_error = config_error_from_toml(toml_file, &contents, err.clone()); @@ -304,21 +420,24 @@ async fn load_config_toml_for_required_layer( ), ) })?; - resolve_relative_paths_in_config_toml(config, config_parent) + let expanded = expand_config_toml(config); + let resolved = resolve_relative_paths_in_config_toml(expanded.value, config_parent)?; + (resolved, expanded.warnings) + } + Err(e) if e.kind() == io::ErrorKind::NotFound => { + (TomlValue::Table(toml::map::Map::new()), Vec::new()) } Err(e) => { - if e.kind() == io::ErrorKind::NotFound { - Ok(TomlValue::Table(toml::map::Map::new())) - } else { - Err(io::Error::new( - e.kind(), - format!("Failed to read config file {}: {e}", toml_file.display()), - )) - } + return Err(io::Error::new( + e.kind(), + format!("Failed to read config file {}: {e}", toml_file.display()), + )); } - }?; + }; - Ok(create_entry(toml_value)) + let mut entry = create_entry(toml_value); + entry.expansion_warnings = warnings; + Ok(entry) } /// If available, apply requirements from `/etc/codex/requirements.toml` to @@ -451,11 +570,57 @@ fn default_project_root_markers() -> Vec { struct ProjectTrustContext { project_root: AbsolutePathBuf, project_root_key: String, + project_root_display_key: String, repo_root_key: Option, - projects_trust: std::collections::HashMap, + repo_root_display_key: Option, + projects_trust: HashMap, + trust_key_by_normalized: HashMap, user_config_file: AbsolutePathBuf, } +#[derive(Clone)] +struct TrustEntry { + trust_level: TrustLevel, + original_key: String, + is_symbolic: bool, +} + +fn is_symbolic_project_key(key: &str) -> bool { + key.starts_with('~') || key.contains('$') +} + +fn normalize_path_for_matching(path: &str) -> String { + let path_buf = PathBuf::from(path); + std::fs::canonicalize(&path_buf) + .unwrap_or(path_buf) + .to_string_lossy() + .to_string() +} + +fn expand_and_normalize_project_key(key: &str, env: &impl EnvProvider) -> String { + let expanded = expand_key_for_matching_with_env(key, env); + normalize_path_for_matching(&expanded) +} + +fn trust_rank(level: TrustLevel) -> u8 { + match level { + TrustLevel::Untrusted => 2, + TrustLevel::Trusted => 1, + } +} + +fn should_replace_trust_entry(existing: &TrustEntry, candidate: &TrustEntry) -> bool { + let existing_rank = trust_rank(existing.trust_level); + let candidate_rank = trust_rank(candidate.trust_level); + if existing_rank != candidate_rank { + return candidate_rank > existing_rank; + } + if existing.is_symbolic != candidate.is_symbolic { + return candidate.is_symbolic; + } + candidate.original_key < existing.original_key +} + struct ProjectTrustDecision { trust_level: Option, trust_key: String, @@ -469,36 +634,53 @@ impl ProjectTrustDecision { impl ProjectTrustContext { fn decision_for_dir(&self, dir: &AbsolutePathBuf) -> ProjectTrustDecision { - let dir_key = dir.as_path().to_string_lossy().to_string(); + let dir_display_key = dir.as_path().to_string_lossy().to_string(); + let dir_key = normalize_path_for_matching(&dir_display_key); if let Some(trust_level) = self.projects_trust.get(&dir_key).copied() { + let trust_key = self + .trust_key_by_normalized + .get(&dir_key) + .cloned() + .unwrap_or(dir_display_key); return ProjectTrustDecision { trust_level: Some(trust_level), - trust_key: dir_key, + trust_key, }; } if let Some(trust_level) = self.projects_trust.get(&self.project_root_key).copied() { + let trust_key = self + .trust_key_by_normalized + .get(&self.project_root_key) + .cloned() + .unwrap_or_else(|| self.project_root_display_key.clone()); return ProjectTrustDecision { trust_level: Some(trust_level), - trust_key: self.project_root_key.clone(), + trust_key, }; } if let Some(repo_root_key) = self.repo_root_key.as_ref() && let Some(trust_level) = self.projects_trust.get(repo_root_key).copied() { + let trust_key = self + .trust_key_by_normalized + .get(repo_root_key) + .cloned() + .or_else(|| self.repo_root_display_key.clone()) + .unwrap_or_else(|| repo_root_key.clone()); return ProjectTrustDecision { trust_level: Some(trust_level), - trust_key: repo_root_key.clone(), + trust_key, }; } ProjectTrustDecision { trust_level: None, trust_key: self - .repo_root_key + .repo_root_display_key .clone() - .unwrap_or_else(|| self.project_root_key.clone()), + .unwrap_or_else(|| self.project_root_display_key.clone()), } } @@ -545,28 +727,59 @@ async fn project_trust_context( project_root_markers: &[String], config_base_dir: &Path, user_config_file: &AbsolutePathBuf, + env: &impl EnvProvider, ) -> io::Result { let config_toml = deserialize_config_toml_with_base(merged_config.clone(), config_base_dir)?; let project_root = find_project_root(cwd, project_root_markers).await?; let projects = config_toml.projects.unwrap_or_default(); - let project_root_key = project_root.as_path().to_string_lossy().to_string(); + let project_root_display_key = project_root.as_path().to_string_lossy().to_string(); + let project_root_key = normalize_path_for_matching(&project_root_display_key); let repo_root = resolve_root_git_project_for_trust(cwd.as_path()); - let repo_root_key = repo_root + let repo_root_display_key = repo_root .as_ref() .map(|root| root.to_string_lossy().to_string()); + let repo_root_key = repo_root_display_key + .as_deref() + .map(normalize_path_for_matching); - let projects_trust = projects - .into_iter() - .filter_map(|(key, project)| project.trust_level.map(|trust_level| (key, trust_level))) - .collect(); + let mut trust_entries: HashMap = HashMap::new(); + for (key, project) in projects { + let Some(trust_level) = project.trust_level else { + continue; + }; + let normalized_key = expand_and_normalize_project_key(&key, env); + let candidate = TrustEntry { + trust_level, + original_key: key.clone(), + is_symbolic: is_symbolic_project_key(&key), + }; + trust_entries + .entry(normalized_key) + .and_modify(|existing| { + if should_replace_trust_entry(existing, &candidate) { + *existing = candidate.clone(); + } + }) + .or_insert(candidate); + } + + let mut projects_trust = HashMap::new(); + let mut trust_key_by_normalized = HashMap::new(); + for (normalized_key, entry) in trust_entries { + projects_trust.insert(normalized_key.clone(), entry.trust_level); + trust_key_by_normalized.insert(normalized_key, entry.original_key); + } Ok(ProjectTrustContext { project_root, project_root_key, + project_root_display_key, repo_root_key, + repo_root_display_key, projects_trust, + trust_key_by_normalized, user_config_file: user_config_file.clone(), }) } @@ -715,10 +928,12 @@ async fn load_project_layers( continue; } }; + let expanded = expand_config_toml(config); let config = - resolve_relative_paths_in_config_toml(config, dot_codex_abs.as_path())?; - let entry = + resolve_relative_paths_in_config_toml(expanded.value, dot_codex_abs.as_path())?; + let mut entry = project_layer_entry(trust_context, &dot_codex_abs, &layer_dir, config, true); + entry.expansion_warnings = expanded.warnings; layers.push(entry); } Err(err) => { diff --git a/codex-rs/core/src/config_loader/state.rs b/codex-rs/core/src/config_loader/state.rs index 847b19d7e5..4460e72915 100644 --- a/codex-rs/core/src/config_loader/state.rs +++ b/codex-rs/core/src/config_loader/state.rs @@ -1,5 +1,6 @@ -use crate::config_loader::ConfigRequirements; -use crate::config_loader::ConfigRequirementsToml; +use super::ConfigExpansionWarning; +use super::ConfigRequirements; +use super::ConfigRequirementsToml; use super::fingerprint::record_origins; use super::fingerprint::version_for_toml; @@ -29,23 +30,42 @@ pub struct ConfigLayerEntry { pub config: TomlValue, pub version: String, pub disabled_reason: Option, + pub expansion_warnings: Vec, } impl ConfigLayerEntry { pub fn new(name: ConfigLayerSource, config: TomlValue) -> Self { - let version = version_for_toml(&config); - Self { - name, - config, - version, - disabled_reason: None, - } + Self::new_with_warnings(name, config, Vec::new()) } pub fn new_disabled( name: ConfigLayerSource, config: TomlValue, disabled_reason: impl Into, + ) -> Self { + Self::new_disabled_with_warnings(name, config, disabled_reason, Vec::new()) + } + + pub fn new_with_warnings( + name: ConfigLayerSource, + config: TomlValue, + expansion_warnings: Vec, + ) -> Self { + let version = version_for_toml(&config); + Self { + name, + config, + version, + disabled_reason: None, + expansion_warnings, + } + } + + pub fn new_disabled_with_warnings( + name: ConfigLayerSource, + config: TomlValue, + disabled_reason: impl Into, + expansion_warnings: Vec, ) -> Self { let version = version_for_toml(&config); Self { @@ -53,6 +73,7 @@ impl ConfigLayerEntry { config, version, disabled_reason: Some(disabled_reason.into()), + expansion_warnings, } } @@ -90,6 +111,12 @@ impl ConfigLayerEntry { } } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ConfigExpansionWarningInfo { + pub source: ConfigLayerSource, + pub warning: ConfigExpansionWarning, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ConfigLayerStackOrdering { LowestPrecedenceFirst, @@ -144,6 +171,22 @@ impl ConfigLayerStack { &self.requirements_toml } + pub fn expansion_warnings(&self) -> Vec { + let mut warnings = Vec::new(); + for layer in &self.layers { + if layer.expansion_warnings.is_empty() { + continue; + } + for warning in &layer.expansion_warnings { + warnings.push(ConfigExpansionWarningInfo { + source: layer.name.clone(), + warning: warning.clone(), + }); + } + } + warnings + } + /// Creates a new [ConfigLayerStack] using the specified values to inject a /// "user layer" into the stack. If such a layer already exists, it is /// replaced; otherwise, it is inserted into the stack at the appropriate diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 03b0706c84..5740350ab4 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -1,11 +1,18 @@ use super::LoaderOverrides; +use super::expand_config_toml_with_env; +use super::expansion::FakeEnv; +use super::expansion::KEY_COLLISION_SENTINEL; +use super::format_expansion_warnings; use super::load_config_layers_state; +use super::load_config_layers_state_with_env_for_tests; use crate::config::CONFIG_TOML_FILE; use crate::config::ConfigBuilder; use crate::config::ConfigOverrides; use crate::config::ConfigToml; use crate::config::ProjectConfig; +use crate::config_loader::ConfigExpansionWarningInfo; use crate::config_loader::ConfigLayerEntry; +use crate::config_loader::ConfigLayerSource; use crate::config_loader::ConfigLoadError; use crate::config_loader::ConfigRequirements; use crate::config_loader::config_requirements::ConfigRequirementsWithSources; @@ -233,6 +240,7 @@ async fn returns_empty_when_all_layers_missing() { config: TomlValue::Table(toml::map::Map::new()), version: version_for_toml(&TomlValue::Table(toml::map::Map::new())), disabled_reason: None, + expansion_warnings: Vec::new(), }, user_layer, ); @@ -388,6 +396,196 @@ allowed_sandbox_modes = ["read-only"] Ok(()) } +#[test] +fn expands_env_vars_in_keys_and_values() { + let config: TomlValue = toml::from_str( + r#"[projects."$PROJECTS/gpt5test"] +trust_level = "trusted" +notes = "path=$PROJECTS/gpt5test model=${MODEL} $$MODEL" +"#, + ) + .expect("parse toml"); + + let env = FakeEnv::new([("PROJECTS", "/Users/tester/projects"), ("MODEL", "gpt-5")]); + let result = expand_config_toml_with_env(config, &env); + assert_eq!(result.warnings, Vec::new()); + + let expected: TomlValue = toml::from_str( + r#"[projects."/Users/tester/projects/gpt5test"] +trust_level = "trusted" +notes = "path=/Users/tester/projects/gpt5test model=gpt-5 $MODEL" +"#, + ) + .expect("parse toml"); + assert_eq!(result.value, expected); +} + +#[test] +fn expands_tilde_prefix_only_at_start() { + let config: TomlValue = toml::from_str( + r#" +path = "~/skills/skill.md" +no_expand = "docs/~user/skill.md" +"#, + ) + .expect("parse toml"); + + let env = if cfg!(windows) { + FakeEnv::new([("USERPROFILE", "C:\\Users\\tester")]) + } else { + FakeEnv::new([("HOME", "/Users/tester")]) + }; + let result = expand_config_toml_with_env(config, &env); + assert_eq!(result.warnings, Vec::new()); + + let expected_path = if cfg!(windows) { + "C:\\Users\\tester\\skills\\skill.md" + } else { + "/Users/tester/skills/skill.md" + }; + let expected: TomlValue = toml::from_str(&format!( + r#" +path = "{expected_path}" +no_expand = "docs/~user/skill.md" +"# + )) + .expect("parse toml"); + assert_eq!(result.value, expected); +} + +#[test] +fn warns_when_env_var_missing() { + let config: TomlValue = toml::from_str( + r#" +path = "$MISSING/bin" +[projects."$PROJECTS/demo"] +trust_level = "trusted" +"#, + ) + .expect("parse toml"); + + let env = FakeEnv::new([("PROJECTS", "/Users/tester/projects")]); + let result = expand_config_toml_with_env(config, &env); + + assert_eq!( + result.warnings, + vec![super::ConfigExpansionWarning::new("MISSING", "path")] + ); + + let expected: TomlValue = toml::from_str( + r#" +path = "$MISSING/bin" +[projects."/Users/tester/projects/demo"] +trust_level = "trusted" +"#, + ) + .expect("parse toml"); + assert_eq!(result.value, expected); +} + +#[test] +fn warns_when_env_var_missing_in_key() { + let config: TomlValue = toml::from_str( + r#"[projects."$PROJECTS/demo"] +trust_level = "trusted" +"#, + ) + .expect("parse toml"); + + let env = FakeEnv::new(Vec::<(String, String)>::new()); + let result = expand_config_toml_with_env(config, &env); + + assert_eq!( + result.warnings, + vec![super::ConfigExpansionWarning::new( + "PROJECTS", + "projects[\"$PROJECTS/demo\"]" + )] + ); +} + +#[test] +fn warns_when_home_missing_for_tilde() { + let config: TomlValue = toml::from_str( + r#" +path = "~/skills/skill.md" +"#, + ) + .expect("parse toml"); + + let env = FakeEnv::new(Vec::<(String, String)>::new()); + let result = expand_config_toml_with_env(config, &env); + + let expected_var = if cfg!(windows) { "USERPROFILE" } else { "HOME" }; + assert_eq!( + result.warnings, + vec![super::ConfigExpansionWarning::new(expected_var, "path")] + ); + + let expected: TomlValue = toml::from_str( + r#" +path = "~/skills/skill.md" +"#, + ) + .expect("parse toml"); + assert_eq!(result.value, expected); +} + +#[test] +fn warns_on_duplicate_keys_after_expansion_and_keeps_first() { + let config: TomlValue = toml::from_str( + r#" +[projects."$ROOT/demo"] +trust_level = "trusted" +[projects."${ROOT}/demo"] +trust_level = "untrusted" +"#, + ) + .expect("parse toml"); + + let env = FakeEnv::new([("ROOT", "/tmp/root")]); + let result = expand_config_toml_with_env(config, &env); + + assert_eq!(result.warnings.len(), 1); + assert_eq!(result.warnings[0].var, KEY_COLLISION_SENTINEL); + + let expected: TomlValue = toml::from_str( + r#" +[projects."/tmp/root/demo"] +trust_level = "trusted" +"#, + ) + .expect("parse toml"); + assert_eq!(result.value, expected); +} + +#[test] +fn formats_duplicate_key_collision_warnings() { + let config: TomlValue = toml::from_str( + r#" +[projects."$ROOT/demo"] +trust_level = "trusted" +[projects."${ROOT}/demo"] +trust_level = "untrusted" +"#, + ) + .expect("parse toml"); + + let env = FakeEnv::new([("ROOT", "/tmp/root")]); + let result = expand_config_toml_with_env(config, &env); + assert_eq!(result.warnings.len(), 1); + + let warnings = vec![ConfigExpansionWarningInfo { + source: ConfigLayerSource::SessionFlags, + warning: result.warnings[0].clone(), + }]; + let formatted = format_expansion_warnings(&warnings); + assert!(formatted.contains("duplicate keys after expansion")); + assert!(formatted.contains("\"$ROOT/demo\" and \"${ROOT}/demo\"")); + assert!(formatted.contains("\"/tmp/root/demo\"")); + assert!(formatted.contains("session flags: projects")); +} + #[cfg(target_os = "macos")] #[tokio::test] async fn managed_preferences_requirements_take_precedence() -> anyhow::Result<()> { @@ -648,6 +846,7 @@ async fn project_layer_is_added_when_dot_codex_exists_without_config_toml() -> s config: TomlValue::Table(toml::map::Map::new()), version: version_for_toml(&TomlValue::Table(toml::map::Map::new())), disabled_reason: None, + expansion_warnings: Vec::new(), }], project_layers ); @@ -755,6 +954,59 @@ async fn project_layers_disabled_when_untrusted_or_unknown() -> std::io::Result< Ok(()) } +#[tokio::test] +async fn project_layers_enabled_when_trusted_via_symbolic_tilde_key() -> std::io::Result<()> { + let tmp = tempdir()?; + + let home_dir = tmp.path().join("home"); + let project_root = home_dir.clone(); + let nested = project_root.join("child"); + tokio::fs::create_dir_all(project_root.join(".git")).await?; + tokio::fs::create_dir_all(project_root.join(".codex")).await?; + tokio::fs::create_dir_all(&nested).await?; + tokio::fs::write( + project_root.join(".codex").join(CONFIG_TOML_FILE), + "foo = \"project\"\n", + ) + .await?; + + let codex_home = tmp.path().join("codex_home"); + tokio::fs::create_dir_all(&codex_home).await?; + tokio::fs::write( + codex_home.join(CONFIG_TOML_FILE), + "[projects.\"~\"]\ntrust_level = \"trusted\"\n", + ) + .await?; + + let cwd = AbsolutePathBuf::from_absolute_path(&nested)?; + let env = FakeEnv::new([("HOME", home_dir.to_string_lossy().to_string())]); + let layers = load_config_layers_state_with_env_for_tests( + &codex_home, + Some(cwd), + &[] as &[(String, TomlValue)], + LoaderOverrides::default(), + &env, + ) + .await?; + + let project_layers: Vec<_> = layers + .get_layers( + super::ConfigLayerStackOrdering::HighestPrecedenceFirst, + true, + ) + .into_iter() + .filter(|layer| matches!(layer.name, super::ConfigLayerSource::Project { .. })) + .collect(); + assert_eq!(project_layers.len(), 1); + assert_eq!(project_layers[0].disabled_reason, None); + assert_eq!( + layers.effective_config().get("foo"), + Some(&TomlValue::String("project".to_string())) + ); + + Ok(()) +} + #[tokio::test] async fn invalid_project_config_ignored_when_untrusted_or_unknown() -> std::io::Result<()> { let tmp = tempdir()?; diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 441041aa68..456da0e9bc 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -30,6 +30,7 @@ use codex_core::config::load_config_as_toml_with_cli_overrides; use codex_core::config::resolve_oss_provider; use codex_core::config_loader::ConfigLoadError; use codex_core::config_loader::format_config_error_with_source; +use codex_core::config_loader::format_expansion_warnings; use codex_core::git_info::get_git_repo_root; use codex_core::models_manager::manager::RefreshStrategy; use codex_core::protocol::AskForApproval; @@ -236,6 +237,14 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any let config = Config::load_with_cli_overrides_and_harness_overrides(cli_kv_overrides, overrides).await?; + #[allow(clippy::print_stderr)] + { + let warnings = config.config_layer_stack.expansion_warnings(); + if !warnings.is_empty() { + eprintln!("⚠ {}", format_expansion_warnings(&warnings)); + } + } + if let Err(err) = enforce_login_restrictions(&config) { eprintln!("{err}"); std::process::exit(1); diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 4e41a2a0d6..2cd59edb6f 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -42,6 +42,7 @@ use codex_core::config::ConfigOverrides; use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; use codex_core::config_loader::ConfigLayerStackOrdering; +use codex_core::config_loader::format_expansion_warnings; use codex_core::features::Feature; use codex_core::models_manager::manager::RefreshStrategy; use codex_core::models_manager::model_presets::HIDE_GPT_5_1_CODEX_MAX_MIGRATION_PROMPT_CONFIG; @@ -229,6 +230,17 @@ fn emit_project_config_warnings(app_event_tx: &AppEventSender, config: &Config) ))); } +fn emit_expansion_warnings(app_event_tx: &AppEventSender, config: &Config) { + let warnings = config.config_layer_stack.expansion_warnings(); + if warnings.is_empty() { + return; + } + + app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( + history_cell::new_warning_event(format_expansion_warnings(&warnings)), + ))); +} + #[derive(Debug, Clone, PartialEq, Eq)] struct SessionSummary { usage_line: String, @@ -922,6 +934,7 @@ impl App { let (app_event_tx, mut app_event_rx) = unbounded_channel(); let app_event_tx = AppEventSender::new(app_event_tx); emit_deprecation_notice(&app_event_tx, ollama_chat_support_notice); + emit_expansion_warnings(&app_event_tx, &config); emit_project_config_warnings(&app_event_tx, &config); let harness_overrides = diff --git a/docs/config.md b/docs/config.md index 9d53538cae..9e311d7c8c 100644 --- a/docs/config.md +++ b/docs/config.md @@ -28,6 +28,26 @@ Codex can run a notification hook when the agent finishes a turn. See the config The generated JSON Schema for `config.toml` lives at `codex-rs/core/config.schema.json`. +## Variable expansion + +Codex expands environment variables in all `config.toml` string values and table keys. + +- `$VAR` and `${VAR}` expand to environment variables. +- `$$` escapes to a literal `$`. +- `~` expands only when the string starts with `~/` or `~\\`. + +If a variable is unset (including `HOME`/`USERPROFILE` for `~`), the value is left unchanged +and Codex emits a warning after loading the config. + +When Codex persists project trust entries, it prefers updating an existing symbolic project key +(for example, `~` or `$HOME`) if it expands to the same directory, instead of adding a duplicate +absolute path entry. + +When Codex checks whether a project is trusted, it also expands symbolic project keys (including +the bare `~` and `$VAR` forms) before matching against the current working directory. If both a +symbolic key and its absolute expansion exist for the same directory, Codex prefers the symbolic +entry and may remove the absolute entry when it only contains `trust_level`. + ## Notices Codex stores "do not show again" flags for some UI prompts under the `[notice]` table. diff --git a/expansion.md b/expansion.md new file mode 100644 index 0000000000..2aece932e7 --- /dev/null +++ b/expansion.md @@ -0,0 +1,180 @@ +# Env + Tilde Expansion for config.toml + +## Conversation Summary +- We inspected the codebase and found `~/.codex/config.toml` is loaded at startup via the config loader stack. + - Loader and layer stack: `codex-rs/core/src/config_loader/mod.rs` (`load_config_layers_state`). + - Config building: `codex-rs/core/src/config/mod.rs` (e.g., `load_config_as_toml_with_cli_overrides`). + - Entry points that load config early: `codex-rs/exec/src/lib.rs`, `codex-rs/app-server/src/lib.rs`, CLI/TUI via config builder. +- User wants *universal* variable expansion at ingestion time (all strings + keys), not limited to `projects` paths. +- If a variable is unset, they want a *user-visible warning* after load, similar to other config warnings (MCP-like). The warning should not hard-fail config loading. +- We decided to add a **separate, strict `~` expansion feature** (not just shell-like behavior). Rules below. + +## Goals +- Expand `$VAR` and `${VAR}` *everywhere* in config TOML string values and table keys. +- Add strict `~` expansion rule *as a separate feature*. +- If expansion fails (unset env var), leave the string/key unexpanded and **emit a warning**. +- Surface warnings in all frontends: + - App server: `ConfigWarningNotification` at startup. + - TUI: history warning event on startup. + - CLI/exec: stderr warnings. + +## Non-Goals +- No `%VAR%` expansion (Windows style). +- No `~user` or mid-string `foo/~` expansion. +- No change to runtime overrides format or CLI flags. + +## Proposed Expansion Rules +### Env vars +- Supported syntax: `$VAR` and `${VAR}`. +- Expansion occurs in **all string values and all table keys**. +- `$$` escapes to a literal `$`. +- If env var is unset: + - Leave the token unexpanded. + - Record a warning including variable name and config path/key. +- If key expansion produces a duplicate key within the same table: + - Do not silently overwrite. + - Keep the first entry (first-wins). + - Emit a warning describing the collision. + - Note: “first” is based on TOML map iteration order (typically lexicographic by key), not necessarily file order. + +### Tilde +- Only expand when the string **starts** with `~/` or `~\`. +- No expansion for `~user`, `foo/~`, or `bar~baz`. +- Source: + - Unix/macOS: `$HOME` + - Windows: `$USERPROFILE` +- If required env var is unset: leave unexpanded + warning. + +## Where to Implement +### Primary integration point +- `codex-rs/core/src/config_loader/mod.rs` during config layer loading, **before merge** and **before `ConfigToml` deserialization**. +- Introduce a pass that walks `toml::Value` and rewrites: + - string values + - table keys +- Keep expansion *per-layer* so warnings can be attributed to a layer and shown with source info later. + +### Warning plumbing +- Extend config loader data structures to carry expansion warnings. + - Candidate: add `warnings: Vec` (new type) to `ConfigLayerEntry`. + - Or add warnings at the `ConfigLayerStack` level (aggregate list with source path). +- App server already builds warnings at startup in `codex-rs/app-server/src/lib.rs` (see `config_warnings` and `ConfigWarningNotification`). Add expansion warnings here. +- TUI already emits warnings in `codex-rs/tui/src/app.rs` (see `emit_project_config_warnings`). Add expansion warnings here. +- CLI/exec: print warnings to stderr after config load (similar to other config warnings). + +### Non-breaking warning model for key collisions +- `ConfigExpansionWarning` is publicly re-exported from `codex_core::config_loader`. +- Changing its public fields would be a breaking API change for downstream consumers. +- To avoid a breaking change while still surfacing collisions: + - Keep `ConfigExpansionWarning { var, path }` as-is. + - Use a sentinel value in `var` for collisions (e.g., `KEY_COLLISION`). + - Encode collision details into `path` in a structured string format that the formatter understands. + - Centralize user-facing rendering in `format_expansion_warnings(...)` so callers do not need to interpret sentinel values. + +## Suggested Warning Text +- Summary: `Config variable expansion failed; some values were left unchanged.` +- Details: list like: + - `1. $PROJECTS in [projects."$PROJECTS/foo"] is unset` + - `2. $HOME in "~/something" is unset` +- Include file path and (if available) TOML location. If no range info, include layer source + key path. + +Additional collision example: +- `3. /path/to/config.toml: projects has duplicate key after expansion: "$ROOT/a" and "${ROOT}/a" both expand to "/abs/a" (kept first)` + +## Tests to Add +- `core/src/config_loader/tests.rs` or new test module in `config_loader`: + - Expands `$VAR` in string value. + - Expands `$VAR` in table key (e.g., `[projects."$PROJECTS/foo"]`). + - Expands `${VAR}`. + - Escapes `$$`. + - Strict `~` expansion only at start (`~/x` and `~\x`), not mid-string. + - Unset env var emits warning and leaves token unchanged. + - Duplicate key after expansion emits a collision warning and does not overwrite silently. +- If any warnings are surfaced to app server/tui, add lightweight tests around warning aggregation (or ensure existing tests still pass). + +## Feature Checklist +- Env var expansion for all TOML string values and table keys (`$VAR`, `${VAR}`). +- `$$` escape to literal `$`. +- Strict tilde expansion only at string start (`~/` or `~\\`). +- Unset env var produces warning and leaves token unchanged. +- Warning aggregation per config layer, surfaced consistently in app-server, TUI, and CLI/exec. +- Project trust key behavior: + - Trust writes match existing `[projects]` keys by expanded+normalized path. + - If both symbolic and absolute keys match, trust writes prefer updating the symbolic key. + - If a matching symbolic key exists and an absolute duplicate table contains only `trust_level`, the absolute duplicate is removed. + - Absolute duplicate entries with additional fields are preserved. + +## Test Ideas (Concrete) +- Unit tests for expansion parsing: + - `$FOO` and `${FOO}` expand in strings. + - `$$FOO` preserves literal `$FOO`. + - Mixed text like `path=$FOO/sub` expands. + - Unset `$MISSING` leaves token and emits warning. +- TOML structure tests: + - Table key expansion: `[projects."$PROJECTS/foo"]` expands to `[projects."/abs/foo"]`. + - Nested tables and arrays with strings expand correctly. +- Tilde tests: + - `~/x` and `~\\x` expand using HOME/USERPROFILE. + - `~user/x`, `foo/~` do not expand and produce no warning. + - `~/x` when HOME/USERPROFILE missing emits warning and remains `~/x`. +- Warning plumbing: + - App server collects expansion warnings into `ConfigWarningNotification`. + - TUI inserts warning history cell on startup when expansion warnings exist. + - CLI/exec emits stderr warning for expansion failures. + - Collision warnings render clearly via `format_expansion_warnings(...)`. + +## Docs +- Update `docs/config.md` (and any other relevant docs) to describe: + - `$VAR`/`${VAR}` expansion + - `$$` escaping + - strict `~` expansion rules + - warning behavior for unset vars + +## Implementation Steps (Detailed) +1. **Add expansion utility** + - New helper module in `core/src/config_loader/` (e.g., `expand.rs`) with: + - `expand_toml(value: TomlValue) -> (TomlValue, Vec)` + - Walk table keys and values recursively. + - Use a small parser to handle `$VAR`, `${VAR}`, and `$$`. + - Apply strict `~` expansion only at string start. + - Track warnings with a path (e.g., `projects.$PROJECTS/foo` or TOML key path) and env var name. + - Collision handling (non-breaking): + - During table expansion, track expanded keys. + - On duplicate expanded key: + - keep the first value (first-wins), + - emit a `ConfigExpansionWarning` with a sentinel `var` (e.g., `KEY_COLLISION`), + - encode the expanded key and both original keys into `path` so the formatter can render a human-readable message. + - Caveat: because TOML maps are typically iterated in sorted-key order, “first-wins” is deterministic but may not match file order. + +2. **Integrate into loader** + - In `load_config_layers_state` (or immediately after loading each layer): + - Expand the layer’s `TomlValue`. + - Store any warnings on the `ConfigLayerEntry` (or collect into stack). + +3. **Surface warnings** + - App server: add expansion warnings into `config_warnings` list in `codex-rs/app-server/src/lib.rs`. + - TUI: add a new `emit_*` function to append history warnings. + - CLI/exec: print to stderr after config load. + - Suggested insertion point: immediately after + `Config::load_with_cli_overrides_and_harness_overrides(...)` in + `codex-rs/exec/src/lib.rs`. + - Use `format_expansion_warnings(...)` and print the full multi-line message to stderr. + +4. **Tests** + - Add unit tests for expansion logic and warning emission. + - Add collision tests that verify: + - no silent overwrites, + - collision warning presence, + - collision warning rendering via `format_expansion_warnings(...)`. + - If feasible, add an exec-level stderr capture test for expansion warnings. + +5. **Docs + fmt/tests** + - Update docs. + - Run `just fmt` in `codex-rs`. + - Run project-specific tests (`cargo test -p codex-core` or relevant crate), then get user approval before `cargo test --all-features` if required. + +## Notes/Constraints +- Follow repo rule: do not modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` or `CODEX_SANDBOX_ENV_VAR`. +- Use clippy style preferences (inline format args, no wildcard matches when avoidable, etc.). +- Known trade-off (documented convention): + - Using a sentinel in `ConfigExpansionWarning.var` for collisions is API-compatible but may surprise future code that assumes `var` is always an environment variable name. + - Mitigation: prefer `format_expansion_warnings(...)` for user-facing output rather than inspecting `warning.var` directly.