From 4e5f10f1b6a1f691c327a44214ca72659b80a62b Mon Sep 17 00:00:00 2001 From: Rasmus Rygaard Date: Tue, 14 Apr 2026 17:23:28 -0700 Subject: [PATCH] Use ConfigStore for local config loading --- codex-rs/Cargo.lock | 1 + codex-rs/config-store/Cargo.toml | 1 + codex-rs/config-store/src/lib.rs | 2 + codex-rs/config-store/src/local.rs | 125 ++++++++++++++ codex-rs/config/src/diagnostics.rs | 12 ++ codex-rs/config/src/lib.rs | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/config_loader/mod.rs | 203 +++++++++++++++-------- codex-rs/core/src/config_loader/tests.rs | 118 +++++++++++++ 9 files changed, 399 insertions(+), 65 deletions(-) create mode 100644 codex-rs/config-store/src/local.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index bde19a6e05..466c3e94bb 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1922,6 +1922,7 @@ dependencies = [ "codex-async-utils", "codex-code-mode", "codex-config", + "codex-config-store", "codex-connectors", "codex-core-skills", "codex-exec-server", diff --git a/codex-rs/config-store/Cargo.toml b/codex-rs/config-store/Cargo.toml index bdc4ef7b3f..90fd26abf6 100644 --- a/codex-rs/config-store/Cargo.toml +++ b/codex-rs/config-store/Cargo.toml @@ -16,6 +16,7 @@ async-trait = { workspace = true } codex-utils-absolute-path = { workspace = true } serde = { workspace = true, features = ["derive"] } thiserror = { workspace = true } +tokio = { workspace = true, features = ["fs"] } toml = { workspace = true } [dev-dependencies] diff --git a/codex-rs/config-store/src/lib.rs b/codex-rs/config-store/src/lib.rs index 7d02ead69c..6b995702a4 100644 --- a/codex-rs/config-store/src/lib.rs +++ b/codex-rs/config-store/src/lib.rs @@ -8,11 +8,13 @@ //! wire-friendly: prefer primitive fields over Rust-specific error or filesystem types. mod error; +mod local; mod store; mod types; pub use error::ConfigStoreError; pub use error::ConfigStoreResult; +pub use local::LocalConfigStore; pub use store::ConfigDocumentStore; pub use types::ConfigDocumentErrorSpan; pub use types::ConfigDocumentRead; diff --git a/codex-rs/config-store/src/local.rs b/codex-rs/config-store/src/local.rs new file mode 100644 index 0000000000..76af834d6f --- /dev/null +++ b/codex-rs/config-store/src/local.rs @@ -0,0 +1,125 @@ +use async_trait::async_trait; + +use crate::ConfigDocumentErrorSpan; +use crate::ConfigDocumentRead; +use crate::ConfigDocumentStore; +use crate::ConfigStoreResult; +use crate::ReadConfigDocumentParams; + +/// Filesystem-backed implementation of [ConfigDocumentStore]. +/// +/// This implementation reads the requested path from the local filesystem and parses it as TOML. +/// It does not apply config-layer ordering, project trust, relative-path resolution, or fallback +/// behavior for missing documents. +#[derive(Debug, Default, Clone)] +pub struct LocalConfigStore; + +#[async_trait] +impl ConfigDocumentStore for LocalConfigStore { + async fn read_config_document( + &self, + params: ReadConfigDocumentParams, + ) -> ConfigStoreResult { + match tokio::fs::read_to_string(params.path.as_path()).await { + Ok(raw_toml) => match toml::from_str(&raw_toml) { + Ok(value) => Ok(ConfigDocumentRead::Present { value }), + Err(error) => Ok(ConfigDocumentRead::ParseError { + raw_toml, + message: error.message().to_string(), + span: error.span().map(ConfigDocumentErrorSpan::from), + }), + }, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + Ok(ConfigDocumentRead::Missing) + } + Err(err) => Ok(ConfigDocumentRead::ReadError { + kind: read_error_kind_name(err.kind()).to_string(), + message: err.to_string(), + }), + } + } +} + +fn read_error_kind_name(kind: std::io::ErrorKind) -> &'static str { + match kind { + std::io::ErrorKind::PermissionDenied => "permission_denied", + std::io::ErrorKind::InvalidData => "invalid_data", + std::io::ErrorKind::InvalidInput => "invalid_input", + std::io::ErrorKind::TimedOut => "timed_out", + std::io::ErrorKind::Interrupted => "interrupted", + std::io::ErrorKind::Unsupported => "unsupported", + _ => "other", + } +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + use toml::Value as TomlValue; + + use super::*; + use codex_utils_absolute_path::AbsolutePathBuf; + + fn read_params(path: AbsolutePathBuf) -> ReadConfigDocumentParams { + ReadConfigDocumentParams { path } + } + + #[tokio::test] + async fn reads_present_config_document() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let path = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("config.toml"))?; + tokio::fs::write(path.as_path(), "model = \"gpt-5\"\n").await?; + + let got = LocalConfigStore + .read_config_document(read_params(path)) + .await?; + + let expected = ConfigDocumentRead::Present { + value: TomlValue::Table(toml::map::Map::from_iter([( + "model".to_string(), + TomlValue::String("gpt-5".to_string()), + )])), + }; + assert_eq!(got, expected); + Ok(()) + } + + #[tokio::test] + async fn reports_missing_config_document() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let path = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("config.toml"))?; + + let got = LocalConfigStore + .read_config_document(read_params(path)) + .await?; + + assert_eq!(got, ConfigDocumentRead::Missing); + Ok(()) + } + + #[tokio::test] + async fn reports_parse_error_with_raw_toml() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let path = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("config.toml"))?; + let raw_toml = "model = ["; + tokio::fs::write(path.as_path(), raw_toml).await?; + + let got = LocalConfigStore + .read_config_document(read_params(path)) + .await?; + + match got { + ConfigDocumentRead::ParseError { + raw_toml: got_raw, + message, + span, + } => { + assert_eq!(got_raw, raw_toml); + assert!(!message.is_empty()); + assert!(span.is_some()); + } + other => panic!("expected parse error, got {other:?}"), + } + Ok(()) + } +} diff --git a/codex-rs/config/src/diagnostics.rs b/codex-rs/config/src/diagnostics.rs index 899114d6d7..2c9d795fdc 100644 --- a/codex-rs/config/src/diagnostics.rs +++ b/codex-rs/config/src/diagnostics.rs @@ -106,6 +106,18 @@ pub fn config_error_from_toml( ConfigError::new(path.as_ref().to_path_buf(), range, err.message()) } +pub fn config_error_from_parse_message( + path: impl AsRef, + contents: &str, + message: impl Into, + span: Option>, +) -> ConfigError { + let range = span + .map(|span| text_range_from_span(contents, span)) + .unwrap_or_else(default_range); + ConfigError::new(path.as_ref().to_path_buf(), range, message) +} + pub fn config_error_from_typed_toml( path: impl AsRef, contents: &str, diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index ffed561984..98add186ba 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -51,6 +51,7 @@ pub use diagnostics::ConfigError; pub use diagnostics::ConfigLoadError; pub use diagnostics::TextPosition; pub use diagnostics::TextRange; +pub use diagnostics::config_error_from_parse_message; pub use diagnostics::config_error_from_toml; pub use diagnostics::config_error_from_typed_toml; pub use diagnostics::first_layer_config_error; diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index c91f07f705..ce3432208e 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -33,6 +33,7 @@ codex-async-utils = { workspace = true } codex-code-mode = { workspace = true } codex-connectors = { workspace = true } codex-config = { workspace = true } +codex-config-store = { workspace = true } codex-core-skills = { workspace = true } codex-exec-server = { workspace = true } codex-features = { workspace = true } diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 36fc956bf4..8bbe66eb89 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -11,6 +11,11 @@ use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigRequirementsWithSources; use codex_config::config_toml::ConfigToml; use codex_config::config_toml::ProjectConfig; +use codex_config_store::ConfigDocumentRead; +use codex_config_store::ConfigDocumentStore; +use codex_config_store::ConfigStoreError; +use codex_config_store::LocalConfigStore; +use codex_config_store::ReadConfigDocumentParams; use codex_git_utils::resolve_root_git_project_for_trust; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::SandboxMode; @@ -57,6 +62,8 @@ pub use codex_config::TextPosition; pub use codex_config::TextRange; pub use codex_config::WebSearchModeRequirement; pub(crate) use codex_config::build_cli_overrides_layer; +pub(crate) use codex_config::config_error_from_parse_message; +#[cfg(test)] pub(crate) use codex_config::config_error_from_toml; pub use codex_config::default_project_root_markers; pub use codex_config::format_config_error; @@ -123,6 +130,28 @@ pub async fn load_config_layers_state( cli_overrides: &[(String, TomlValue)], overrides: LoaderOverrides, cloud_requirements: CloudRequirementsLoader, +) -> io::Result { + let config_store = LocalConfigStore; + load_config_layers_state_with_store( + codex_home, + cwd, + cli_overrides, + overrides, + cloud_requirements, + &config_store, + ) + .await +} + +/// Equivalent to [load_config_layers_state], using the supplied store for path-addressed +/// config.toml reads. +pub async fn load_config_layers_state_with_store( + codex_home: &Path, + cwd: Option, + cli_overrides: &[(String, TomlValue)], + overrides: LoaderOverrides, + cloud_requirements: CloudRequirementsLoader, + config_store: &(impl ConfigDocumentStore + ?Sized), ) -> io::Result { let mut config_requirements_toml = ConfigRequirementsWithSources::default(); @@ -172,23 +201,26 @@ pub async fn load_config_layers_state( // Include an entry for the "system" config folder, loading its config.toml, // if it exists. let system_config_toml_file = system_config_toml_file()?; - let system_layer = - load_config_toml_for_required_layer(&system_config_toml_file, |config_toml| { + let system_layer = load_config_toml_for_required_layer( + config_store, + &system_config_toml_file, + |config_toml| { ConfigLayerEntry::new( ConfigLayerSource::System { file: system_config_toml_file.clone(), }, config_toml, ) - }) - .await?; + }, + ) + .await?; layers.push(system_layer); // Add a layer for $CODEX_HOME/config.toml if it exists. Note if the file // exists, but is malformed, then this error should be propagated to the // user. let user_file = AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home); - let user_layer = load_config_toml_for_required_layer(&user_file, |config_toml| { + let user_layer = load_config_toml_for_required_layer(config_store, &user_file, |config_toml| { ConfigLayerEntry::new( ConfigLayerSource::User { file: user_file.clone(), @@ -247,6 +279,7 @@ pub async fn load_config_layers_state( } }; let project_layers = load_project_layers( + config_store, &cwd, &project_trust_context.project_root, &project_trust_context, @@ -320,17 +353,14 @@ pub async fn load_config_layers_state( /// - If there is an error reading the file or parsing the TOML, returns an /// error. async fn load_config_toml_for_required_layer( - config_toml: impl AsRef, + config_store: &(impl ConfigDocumentStore + ?Sized), + config_toml: &AbsolutePathBuf, 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 { - Ok(contents) => { - let config: TomlValue = toml::from_str(&contents).map_err(|err| { - let config_error = config_error_from_toml(toml_file, &contents, err.clone()); - io_error_from_config_error(io::ErrorKind::InvalidData, config_error, Some(err)) - })?; - let config_parent = toml_file.parent().ok_or_else(|| { + let toml_file = config_toml.as_path(); + let toml_value = match read_config_document(config_store, config_toml).await? { + ConfigDocumentRead::Present { value } => { + let config_parent = config_toml.parent().ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidData, format!( @@ -339,23 +369,71 @@ async fn load_config_toml_for_required_layer( ), ) })?; - resolve_relative_paths_in_config_toml(config, config_parent) + resolve_relative_paths_in_config_toml(value, config_parent.as_path()) } - 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()), - )) - } + ConfigDocumentRead::Missing => Ok(TomlValue::Table(toml::map::Map::new())), + ConfigDocumentRead::ParseError { + raw_toml, + message, + span, + } => { + let config_error = config_error_from_parse_message( + toml_file, + &raw_toml, + message, + span.map(Into::into), + ); + Err(io_error_from_config_error( + io::ErrorKind::InvalidData, + config_error, + /*source*/ None, + )) } + ConfigDocumentRead::ReadError { kind, message } => Err(io::Error::new( + io_error_kind_from_config_document_read_kind(&kind), + format!( + "Failed to read config file {}: {message}", + toml_file.display() + ), + )), }?; Ok(create_entry(toml_value)) } +async fn read_config_document( + config_store: &(impl ConfigDocumentStore + ?Sized), + path: &AbsolutePathBuf, +) -> io::Result { + config_store + .read_config_document(ReadConfigDocumentParams { path: path.clone() }) + .await + .map_err(config_store_error_to_io) +} + +fn config_store_error_to_io(error: ConfigStoreError) -> io::Error { + match error { + ConfigStoreError::InvalidRequest { message } => { + io::Error::new(io::ErrorKind::InvalidInput, message) + } + ConfigStoreError::ReadFailed { message } | ConfigStoreError::Internal { message } => { + io::Error::other(message) + } + } +} + +fn io_error_kind_from_config_document_read_kind(kind: &str) -> io::ErrorKind { + match kind { + "permission_denied" => io::ErrorKind::PermissionDenied, + "invalid_data" => io::ErrorKind::InvalidData, + "invalid_input" => io::ErrorKind::InvalidInput, + "timed_out" => io::ErrorKind::TimedOut, + "interrupted" => io::ErrorKind::Interrupted, + "unsupported" => io::ErrorKind::Unsupported, + _ => io::ErrorKind::Other, + } +} + /// If available, apply requirements from the platform system /// `requirements.toml` location to `config_requirements_toml` by filling in /// any unset fields. @@ -766,6 +844,7 @@ async fn find_project_root( /// starting from folders closest to `project_root` (which is the lowest /// precedence) to those closest to `cwd` (which is the highest precedence). async fn load_project_layers( + config_store: &(impl ConfigDocumentStore + ?Sized), cwd: &AbsolutePathBuf, project_root: &AbsolutePathBuf, trust_context: &ProjectTrustContext, @@ -810,32 +889,9 @@ async fn load_project_layers( continue; } let config_file = dot_codex_abs.join(CONFIG_TOML_FILE); - match tokio::fs::read_to_string(&config_file).await { - Ok(contents) => { - let config: TomlValue = match toml::from_str(&contents) { - Ok(config) => config, - Err(e) => { - if decision.is_trusted() { - let config_file_display = config_file.as_path().display(); - return Err(io::Error::new( - io::ErrorKind::InvalidData, - format!( - "Error parsing project config file {config_file_display}: {e}" - ), - )); - } - layers.push(project_layer_entry( - trust_context, - &dot_codex_abs, - &layer_dir, - TomlValue::Table(toml::map::Map::new()), - /*config_toml_exists*/ true, - )); - continue; - } - }; - let config = - resolve_relative_paths_in_config_toml(config, dot_codex_abs.as_path())?; + match read_config_document(config_store, &config_file).await? { + ConfigDocumentRead::Present { value } => { + let config = resolve_relative_paths_in_config_toml(value, dot_codex_abs.as_path())?; let entry = project_layer_entry( trust_context, &dot_codex_abs, @@ -845,25 +901,42 @@ async fn load_project_layers( ); layers.push(entry); } - Err(err) => { - if err.kind() == io::ErrorKind::NotFound { - // If there is no config.toml file, record an empty entry - // for this project layer, as this may still have subfolders - // that are significant in the overall ConfigLayerStack. - layers.push(project_layer_entry( - trust_context, - &dot_codex_abs, - &layer_dir, - TomlValue::Table(toml::map::Map::new()), - /*config_toml_exists*/ false, - )); - } else { + ConfigDocumentRead::ParseError { message, .. } => { + if decision.is_trusted() { let config_file_display = config_file.as_path().display(); return Err(io::Error::new( - err.kind(), - format!("Failed to read project config file {config_file_display}: {err}"), + io::ErrorKind::InvalidData, + format!( + "Error parsing project config file {config_file_display}: {message}" + ), )); } + layers.push(project_layer_entry( + trust_context, + &dot_codex_abs, + &layer_dir, + TomlValue::Table(toml::map::Map::new()), + /*config_toml_exists*/ true, + )); + } + ConfigDocumentRead::Missing => { + // If there is no config.toml file, record an empty entry + // for this project layer, as this may still have subfolders + // that are significant in the overall ConfigLayerStack. + layers.push(project_layer_entry( + trust_context, + &dot_codex_abs, + &layer_dir, + TomlValue::Table(toml::map::Map::new()), + /*config_toml_exists*/ false, + )); + } + ConfigDocumentRead::ReadError { kind, message } => { + let config_file_display = config_file.as_path().display(); + return Err(io::Error::new( + io_error_kind_from_config_document_read_kind(&kind), + format!("Failed to read project config file {config_file_display}: {message}"), + )); } } } diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index ed69682e86..f1f1e2e142 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -11,11 +11,17 @@ use crate::config_loader::ConfigRequirements; use crate::config_loader::ConfigRequirementsToml; use crate::config_loader::ConfigRequirementsWithSources; use crate::config_loader::RequirementSource; +use crate::config_loader::load_config_layers_state_with_store; use crate::config_loader::load_requirements_toml; use crate::config_loader::version_for_toml; +use async_trait::async_trait; use codex_config::CONFIG_TOML_FILE; use codex_config::config_toml::ConfigToml; use codex_config::config_toml::ProjectConfig; +use codex_config_store::ConfigDocumentRead; +use codex_config_store::ConfigDocumentStore; +use codex_config_store::ConfigStoreResult; +use codex_config_store::ReadConfigDocumentParams; use codex_protocol::config_types::TrustLevel; use codex_protocol::config_types::WebSearchMode; use codex_protocol::protocol::AskForApproval; @@ -36,6 +42,25 @@ fn config_error_from_io(err: &std::io::Error) -> &super::ConfigError { .expect("expected ConfigLoadError") } +#[derive(Default)] +struct MemoryConfigStore { + documents: HashMap, +} + +#[async_trait] +impl ConfigDocumentStore for MemoryConfigStore { + async fn read_config_document( + &self, + params: ReadConfigDocumentParams, + ) -> ConfigStoreResult { + Ok(self + .documents + .get(¶ms.path) + .cloned() + .unwrap_or(ConfigDocumentRead::Missing)) + } +} + async fn make_config_for_test( codex_home: &Path, project_path: &Path, @@ -108,6 +133,99 @@ async fn returns_config_error_for_invalid_user_config_toml() { assert_eq!(config_error, &expected_config_error); } +#[tokio::test] +async fn loads_user_config_from_config_store() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let cwd = tempdir()?; + let user_file = AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home.path()); + let user_config = toml::from_str::(r#"model = "store-user-model""#)?; + let store = MemoryConfigStore { + documents: HashMap::from([( + user_file.clone(), + ConfigDocumentRead::Present { value: user_config }, + )]), + }; + + let state = load_config_layers_state_with_store( + codex_home.path(), + Some(AbsolutePathBuf::try_from(cwd.path())?), + &[] as &[(String, TomlValue)], + LoaderOverrides::without_managed_config_for_tests(), + CloudRequirementsLoader::default(), + &store, + ) + .await?; + + let expected_user_layer = ConfigLayerEntry::new( + super::ConfigLayerSource::User { file: user_file }, + toml::from_str::(r#"model = "store-user-model""#)?, + ); + assert_eq!(state.get_user_layer(), Some(&expected_user_layer)); + Ok(()) +} + +#[tokio::test] +async fn loads_project_config_from_config_store() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let project = tempdir()?; + let dot_codex = project.path().join(".codex"); + tokio::fs::create_dir(&dot_codex).await?; + + let user_file = AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home.path()); + let project_root = AbsolutePathBuf::try_from(project.path())?; + let project_config_file = + AbsolutePathBuf::from_absolute_path(dot_codex.join(CONFIG_TOML_FILE))?; + let user_config = toml::Value::try_from(ConfigToml { + projects: Some(HashMap::from([( + super::project_trust_key(project.path()), + ProjectConfig { + trust_level: Some(TrustLevel::Trusted), + }, + )])), + ..Default::default() + })?; + let project_config = toml::from_str::(r#"model = "store-project-model""#)?; + let store = MemoryConfigStore { + documents: HashMap::from([ + ( + user_file, + ConfigDocumentRead::Present { value: user_config }, + ), + ( + project_config_file.clone(), + ConfigDocumentRead::Present { + value: project_config.clone(), + }, + ), + ]), + }; + + let state = load_config_layers_state_with_store( + codex_home.path(), + Some(project_root.clone()), + &[] as &[(String, TomlValue)], + LoaderOverrides::without_managed_config_for_tests(), + CloudRequirementsLoader::default(), + &store, + ) + .await?; + + let project_layer = ConfigLayerEntry::new( + super::ConfigLayerSource::Project { + dot_codex_folder: project_config_file.parent().expect("project config parent"), + }, + project_config, + ); + assert_eq!( + state + .layers_high_to_low() + .into_iter() + .find(|layer| matches!(layer.name, super::ConfigLayerSource::Project { .. })), + Some(&project_layer) + ); + Ok(()) +} + #[tokio::test] async fn returns_config_error_for_invalid_managed_config_toml() { let tmp = tempdir().expect("tempdir");