diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 2f2da1345..633899b68 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1402,6 +1402,7 @@ dependencies = [ "seccompiler", "serde", "serde_json", + "serde_path_to_error", "serde_yaml", "serial_test", "sha1", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 9a58bcbfa..b39cfd5e1 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -188,6 +188,7 @@ seccompiler = "0.5.0" sentry = "0.46.0" serde = "1" serde_json = "1" +serde_path_to_error = "0.1.20" serde_with = "3.16" serde_yaml = "0.9" serial_test = "3.2.0" diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index ee65a9ac7..acf08be99 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -2503,6 +2503,24 @@ pub struct DeprecationNoticeNotification { pub details: Option, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct TextPosition { + /// 1-based line number. + pub line: usize, + /// 1-based column number (in Unicode scalar values). + pub column: usize, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct TextRange { + pub start: TextPosition, + pub end: TextPosition, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -2511,6 +2529,14 @@ pub struct ConfigWarningNotification { pub summary: String, /// Optional extra guidance or error details. pub details: Option, + /// Optional path to the config file that triggered the warning. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub path: Option, + /// Optional range for the error location inside the config file. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub range: Option, } #[cfg(test)] diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 022ae9a9c..fde208106 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -15,7 +15,12 @@ use crate::outgoing_message::OutgoingMessageSender; use codex_app_server_protocol::ConfigLayerSource; use codex_app_server_protocol::ConfigWarningNotification; use codex_app_server_protocol::JSONRPCMessage; +use codex_app_server_protocol::TextPosition as AppTextPosition; +use codex_app_server_protocol::TextRange as AppTextRange; +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_feedback::CodexFeedback; use tokio::io::AsyncBufReadExt; use tokio::io::AsyncWriteExt; @@ -46,6 +51,69 @@ mod outgoing_message; /// plenty for an interactive CLI. const CHANNEL_CAPACITY: usize = 128; +fn config_warning_from_error( + summary: impl Into, + err: &std::io::Error, +) -> ConfigWarningNotification { + let (path, range) = match config_error_location(err) { + Some((path, range)) => (Some(path), Some(range)), + None => (None, None), + }; + ConfigWarningNotification { + summary: summary.into(), + details: Some(err.to_string()), + path, + range, + } +} + +fn config_error_location(err: &std::io::Error) -> Option<(String, AppTextRange)> { + err.get_ref() + .and_then(|err| err.downcast_ref::()) + .map(|err| { + let config_error = err.config_error(); + ( + config_error.path.to_string_lossy().to_string(), + app_text_range(&config_error.range), + ) + }) +} + +fn exec_policy_warning_location(err: &ExecPolicyError) -> (Option, Option) { + match err { + ExecPolicyError::ParsePolicy { path, source } => { + if let Some(location) = source.location() { + let range = AppTextRange { + start: AppTextPosition { + line: location.range.start.line, + column: location.range.start.column, + }, + end: AppTextPosition { + line: location.range.end.line, + column: location.range.end.column, + }, + }; + return (Some(location.path), Some(range)); + } + (Some(path.clone()), None) + } + _ => (None, None), + } +} + +fn app_text_range(range: &CoreTextRange) -> AppTextRange { + AppTextRange { + start: AppTextPosition { + line: range.start.line, + column: range.start.column, + }, + end: AppTextPosition { + line: range.end.line, + column: range.end.column, + }, + } +} + fn project_config_warning(config: &Config) -> Option { let mut disabled_folders = Vec::new(); @@ -53,12 +121,11 @@ fn project_config_warning(config: &Config) -> Option .config_layer_stack .get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true) { - if !matches!(layer.name, ConfigLayerSource::Project { .. }) { + if !matches!(layer.name, ConfigLayerSource::Project { .. }) + || layer.disabled_reason.is_none() + { continue; } - if layer.disabled_reason.is_none() { - continue; - }; if let ConfigLayerSource::Project { dot_codex_folder } = &layer.name { disabled_folders.push(( dot_codex_folder.as_path().display().to_string(), @@ -85,6 +152,8 @@ fn project_config_warning(config: &Config) -> Option Some(ConfigWarningNotification { summary: message, details: None, + path: None, + range: None, }) } @@ -139,10 +208,7 @@ pub async fn run_main( { Ok(config) => config, Err(err) => { - let message = ConfigWarningNotification { - summary: "Invalid configuration; using defaults.".to_string(), - details: Some(err.to_string()), - }; + let message = config_warning_from_error("Invalid configuration; using defaults.", &err); config_warnings.push(message); Config::load_default_with_cli_overrides(cli_kv_overrides.clone()).map_err(|e| { std::io::Error::new( @@ -156,9 +222,12 @@ pub async fn run_main( if let Ok(Some(err)) = check_execpolicy_for_warnings(&config.features, &config.config_layer_stack).await { + let (path, range) = exec_policy_warning_location(&err); let message = ConfigWarningNotification { summary: "Error parsing rules; custom rules not applied.".to_string(), details: Some(err.to_string()), + path, + range, }; config_warnings.push(message); } diff --git a/codex-rs/app-server/src/outgoing_message.rs b/codex-rs/app-server/src/outgoing_message.rs index cf720ef83..7b4a599b0 100644 --- a/codex-rs/app-server/src/outgoing_message.rs +++ b/codex-rs/app-server/src/outgoing_message.rs @@ -286,6 +286,8 @@ mod tests { let notification = ServerNotification::ConfigWarning(ConfigWarningNotification { summary: "Config error: using defaults".to_string(), details: Some("error loading config: bad config".to_string()), + path: None, + range: None, }); let jsonrpc_notification = OutgoingMessage::AppServerNotification(notification); diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 8ee2848a9..c151356e3 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -64,6 +64,7 @@ reqwest = { workspace = true, features = ["json", "stream"] } schemars = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +serde_path_to_error = { workspace = true } serde_yaml = { workspace = true } sha1 = { workspace = true } sha2 = { workspace = true } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index ada42663b..fae370ec0 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -410,9 +410,21 @@ impl ConfigBuilder { // relative paths to absolute paths based on the parent folder of the // respective config file, so we should be safe to deserialize without // AbsolutePathBufGuard here. - let config_toml: ConfigToml = merged_toml - .try_into() - .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + let config_toml: ConfigToml = match merged_toml.try_into() { + Ok(config_toml) => config_toml, + Err(err) => { + if let Some(config_error) = + crate::config_loader::first_layer_config_error(&config_layer_stack).await + { + return Err(crate::config_loader::io_error_from_config_error( + std::io::ErrorKind::InvalidData, + config_error, + Some(err), + )); + } + return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, err)); + } + }; Config::load_config_with_layer_stack( config_toml, harness_overrides, diff --git a/codex-rs/core/src/config_loader/diagnostics.rs b/codex-rs/core/src/config_loader/diagnostics.rs new file mode 100644 index 000000000..64f9c8388 --- /dev/null +++ b/codex-rs/core/src/config_loader/diagnostics.rs @@ -0,0 +1,388 @@ +//! Helpers for mapping config parse/validation failures to file locations and +//! rendering them in a user-friendly way. + +use crate::config::CONFIG_TOML_FILE; +use crate::config::ConfigToml; +use codex_app_server_protocol::ConfigLayerSource; +use codex_utils_absolute_path::AbsolutePathBufGuard; +use serde_path_to_error::Path as SerdePath; +use serde_path_to_error::Segment as SerdeSegment; +use std::fmt; +use std::fmt::Write; +use std::io; +use std::path::Path; +use std::path::PathBuf; +use toml_edit::Document; +use toml_edit::Item; +use toml_edit::Table; +use toml_edit::Value; + +use super::ConfigLayerEntry; +use super::ConfigLayerStack; +use super::ConfigLayerStackOrdering; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TextPosition { + pub line: usize, + pub column: usize, +} + +/// Text range in 1-based line/column coordinates. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TextRange { + pub start: TextPosition, + pub end: TextPosition, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ConfigError { + pub path: PathBuf, + pub range: TextRange, + pub message: String, +} + +impl ConfigError { + pub fn new(path: PathBuf, range: TextRange, message: impl Into) -> Self { + Self { + path, + range, + message: message.into(), + } + } +} + +#[derive(Debug)] +pub struct ConfigLoadError { + error: ConfigError, + source: Option, +} + +impl ConfigLoadError { + pub fn new(error: ConfigError, source: Option) -> Self { + Self { error, source } + } + + pub fn config_error(&self) -> &ConfigError { + &self.error + } +} + +impl fmt::Display for ConfigLoadError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{}:{}:{}: {}", + self.error.path.display(), + self.error.range.start.line, + self.error.range.start.column, + self.error.message + ) + } +} + +impl std::error::Error for ConfigLoadError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.source + .as_ref() + .map(|err| err as &dyn std::error::Error) + } +} + +pub(crate) fn io_error_from_config_error( + kind: io::ErrorKind, + error: ConfigError, + source: Option, +) -> io::Error { + io::Error::new(kind, ConfigLoadError::new(error, source)) +} + +pub(crate) fn config_error_from_toml( + path: impl AsRef, + contents: &str, + err: toml::de::Error, +) -> ConfigError { + let range = err + .span() + .map(|span| text_range_from_span(contents, span)) + .unwrap_or_else(default_range); + ConfigError::new(path.as_ref().to_path_buf(), range, err.message()) +} + +pub(crate) fn config_error_from_config_toml( + path: impl AsRef, + contents: &str, +) -> Option { + let deserializer = match toml::de::Deserializer::parse(contents) { + Ok(deserializer) => deserializer, + Err(err) => return Some(config_error_from_toml(path, contents, err)), + }; + + let result: Result = serde_path_to_error::deserialize(deserializer); + match result { + Ok(_) => None, + Err(err) => { + let path_hint = err.path().clone(); + let toml_err: toml::de::Error = err.into_inner(); + let range = span_for_config_path(contents, &path_hint) + .or_else(|| toml_err.span()) + .map(|span| text_range_from_span(contents, span)) + .unwrap_or_else(default_range); + Some(ConfigError::new( + path.as_ref().to_path_buf(), + range, + toml_err.message(), + )) + } + } +} + +pub(crate) async fn first_layer_config_error(layers: &ConfigLayerStack) -> Option { + // When the merged config fails schema validation, we surface the first concrete + // per-file error to point users at a specific file and range rather than an + // opaque merged-layer failure. + first_layer_config_error_for_entries( + layers.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, false), + ) + .await +} + +pub(crate) async fn first_layer_config_error_from_entries( + layers: &[ConfigLayerEntry], +) -> Option { + first_layer_config_error_for_entries(layers.iter()).await +} + +async fn first_layer_config_error_for_entries<'a, I>(layers: I) -> Option +where + I: IntoIterator, +{ + for layer in layers { + let Some(path) = config_path_for_layer(layer) else { + continue; + }; + let contents = match tokio::fs::read_to_string(&path).await { + Ok(contents) => contents, + Err(err) if err.kind() == io::ErrorKind::NotFound => continue, + Err(err) => { + tracing::debug!("Failed to read config file {}: {err}", path.display()); + continue; + } + }; + + let Some(parent) = path.parent() else { + tracing::debug!("Config file {} has no parent directory", path.display()); + continue; + }; + let _guard = AbsolutePathBufGuard::new(parent); + if let Some(error) = config_error_from_config_toml(&path, &contents) { + return Some(error); + } + } + + None +} + +fn config_path_for_layer(layer: &ConfigLayerEntry) -> Option { + match &layer.name { + ConfigLayerSource::System { file } => Some(file.to_path_buf()), + ConfigLayerSource::User { file } => Some(file.to_path_buf()), + ConfigLayerSource::Project { dot_codex_folder } => { + Some(dot_codex_folder.as_path().join(CONFIG_TOML_FILE)) + } + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file } => Some(file.to_path_buf()), + ConfigLayerSource::Mdm { .. } + | ConfigLayerSource::SessionFlags + | ConfigLayerSource::LegacyManagedConfigTomlFromMdm => None, + } +} + +fn text_range_from_span(contents: &str, span: std::ops::Range) -> TextRange { + let start = position_for_offset(contents, span.start); + let end_index = if span.end > span.start { + span.end - 1 + } else { + span.end + }; + let end = position_for_offset(contents, end_index); + TextRange { start, end } +} + +pub fn format_config_error(error: &ConfigError, contents: &str) -> String { + let mut output = String::new(); + let start = error.range.start; + let _ = writeln!( + output, + "{}:{}:{}: {}", + error.path.display(), + start.line, + start.column, + error.message + ); + + let line_index = start.line.saturating_sub(1); + let line = match contents.lines().nth(line_index) { + Some(line) => line.trim_end_matches('\r'), + None => return output.trim_end().to_string(), + }; + + let line_number = start.line; + let gutter = line_number.to_string().len(); + let _ = writeln!(output, "{:width$} |", "", width = gutter); + let _ = writeln!(output, "{line_number:>gutter$} | {line}"); + + let highlight_len = if error.range.end.line == error.range.start.line + && error.range.end.column >= error.range.start.column + { + error.range.end.column - error.range.start.column + 1 + } else { + 1 + }; + let spaces = " ".repeat(start.column.saturating_sub(1)); + let carets = "^".repeat(highlight_len.max(1)); + let _ = writeln!(output, "{:width$} | {spaces}{carets}", "", width = gutter); + output.trim_end().to_string() +} + +pub fn format_config_error_with_source(error: &ConfigError) -> String { + match std::fs::read_to_string(&error.path) { + Ok(contents) => format_config_error(error, &contents), + Err(_) => format_config_error(error, ""), + } +} + +fn position_for_offset(contents: &str, index: usize) -> TextPosition { + let bytes = contents.as_bytes(); + if bytes.is_empty() { + return TextPosition { line: 1, column: 1 }; + } + + let safe_index = index.min(bytes.len().saturating_sub(1)); + let column_offset = index.saturating_sub(safe_index); + let index = safe_index; + + let line_start = bytes[..index] + .iter() + .rposition(|byte| *byte == b'\n') + .map(|pos| pos + 1) + .unwrap_or(0); + let line = bytes[..line_start] + .iter() + .filter(|byte| **byte == b'\n') + .count(); + + let column = std::str::from_utf8(&bytes[line_start..=index]) + .map(|slice| slice.chars().count().saturating_sub(1)) + .unwrap_or_else(|_| index - line_start); + let column = column + column_offset; + + TextPosition { + line: line + 1, + column: column + 1, + } +} + +fn default_range() -> TextRange { + let position = TextPosition { line: 1, column: 1 }; + TextRange { + start: position, + end: position, + } +} + +enum TomlNode<'a> { + Item(&'a Item), + Table(&'a Table), + Value(&'a Value), +} + +fn span_for_path(contents: &str, path: &SerdePath) -> Option> { + let doc = contents.parse::>().ok()?; + let node = node_for_path(doc.as_item(), path)?; + match node { + TomlNode::Item(item) => item.span(), + TomlNode::Table(table) => table.span(), + TomlNode::Value(value) => value.span(), + } +} + +fn span_for_config_path(contents: &str, path: &SerdePath) -> Option> { + if is_features_table_path(path) + && let Some(span) = span_for_features_value(contents) + { + return Some(span); + } + span_for_path(contents, path) +} + +fn is_features_table_path(path: &SerdePath) -> bool { + let mut segments = path.iter(); + matches!(segments.next(), Some(SerdeSegment::Map { key }) if key == "features") + && segments.next().is_none() +} + +fn span_for_features_value(contents: &str) -> Option> { + let doc = contents.parse::>().ok()?; + let root = doc.as_item().as_table_like()?; + let features_item = root.get("features")?; + let features_table = features_item.as_table_like()?; + for (_, item) in features_table.iter() { + match item { + Item::Value(Value::Boolean(_)) => continue, + Item::Value(value) => return value.span(), + Item::Table(table) => return table.span(), + Item::ArrayOfTables(array) => return array.span(), + Item::None => continue, + } + } + None +} + +fn node_for_path<'a>(item: &'a Item, path: &SerdePath) -> Option> { + let segments: Vec<_> = path.iter().cloned().collect(); + let mut node = TomlNode::Item(item); + let mut index = 0; + while index < segments.len() { + match &segments[index] { + SerdeSegment::Map { key } | SerdeSegment::Enum { variant: key } => { + if let Some(next) = map_child(&node, key) { + node = next; + index += 1; + continue; + } + + if index + 1 < segments.len() { + index += 1; + continue; + } + return None; + } + SerdeSegment::Seq { index: seq_index } => { + node = seq_child(&node, *seq_index)?; + index += 1; + } + SerdeSegment::Unknown => return None, + } + } + Some(node) +} + +fn map_child<'a>(node: &TomlNode<'a>, key: &str) -> Option> { + match node { + TomlNode::Item(item) => { + let table = item.as_table_like()?; + table.get(key).map(TomlNode::Item) + } + TomlNode::Table(table) => table.get(key).map(TomlNode::Item), + TomlNode::Value(Value::InlineTable(table)) => table.get(key).map(TomlNode::Value), + _ => None, + } +} + +fn seq_child<'a>(node: &TomlNode<'a>, index: usize) -> Option> { + match node { + TomlNode::Item(Item::Value(Value::Array(array))) => array.get(index).map(TomlNode::Value), + TomlNode::Item(Item::ArrayOfTables(array)) => array.get(index).map(TomlNode::Table), + TomlNode::Value(Value::Array(array)) => array.get(index).map(TomlNode::Value), + _ => None, + } +} diff --git a/codex-rs/core/src/config_loader/layer_io.rs b/codex-rs/core/src/config_loader/layer_io.rs index 0ece69b47..f6f7d2e37 100644 --- a/codex-rs/core/src/config_loader/layer_io.rs +++ b/codex-rs/core/src/config_loader/layer_io.rs @@ -1,4 +1,6 @@ use super::LoaderOverrides; +use super::diagnostics::config_error_from_toml; +use super::diagnostics::io_error_from_config_error; #[cfg(target_os = "macos")] use super::macos::load_managed_admin_config_layer; use codex_utils_absolute_path::AbsolutePathBuf; @@ -75,7 +77,12 @@ pub(super) async fn read_config_from_path( Ok(value) => Ok(Some(value)), Err(err) => { tracing::error!("Failed to parse {}: {err}", path.as_ref().display()); - Err(io::Error::new(io::ErrorKind::InvalidData, err)) + let config_error = config_error_from_toml(path.as_ref(), &contents, err.clone()); + Err(io_error_from_config_error( + io::ErrorKind::InvalidData, + config_error, + Some(err), + )) } }, Err(err) if err.kind() == io::ErrorKind::NotFound => { diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 8f2365f67..6d85a7936 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -1,4 +1,5 @@ mod config_requirements; +mod diagnostics; mod fingerprint; mod layer_io; #[cfg(target_os = "macos")] @@ -34,6 +35,16 @@ pub use config_requirements::McpServerRequirement; pub use config_requirements::RequirementSource; pub use config_requirements::SandboxModeRequirement; pub use config_requirements::Sourced; +pub use diagnostics::ConfigError; +pub use diagnostics::ConfigLoadError; +pub use diagnostics::TextPosition; +pub use diagnostics::TextRange; +pub(crate) use diagnostics::config_error_from_toml; +pub(crate) use diagnostics::first_layer_config_error; +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 merge::merge_toml_values; pub(crate) use overrides::build_cli_overrides_layer; pub use state::ConfigLayerEntry; @@ -171,16 +182,44 @@ pub async fn load_config_layers_state( merge_toml_values(&mut merged_so_far, cli_overrides_layer); } - let project_root_markers = project_root_markers_from_config(&merged_so_far)? - .unwrap_or_else(default_project_root_markers); - let project_trust_context = project_trust_context( + let project_root_markers = match project_root_markers_from_config(&merged_so_far) { + Ok(markers) => markers.unwrap_or_else(default_project_root_markers), + Err(err) => { + if let Some(config_error) = first_layer_config_error_from_entries(&layers).await { + return Err(io_error_from_config_error( + io::ErrorKind::InvalidData, + config_error, + None, + )); + } + return Err(err); + } + }; + let project_trust_context = match project_trust_context( &merged_so_far, &cwd, &project_root_markers, codex_home, &user_file, ) - .await?; + .await + { + Ok(context) => context, + Err(err) => { + let source = err + .get_ref() + .and_then(|err| err.downcast_ref::()) + .cloned(); + if let Some(config_error) = first_layer_config_error_from_entries(&layers).await { + return Err(io_error_from_config_error( + io::ErrorKind::InvalidData, + config_error, + source, + )); + } + return Err(err); + } + }; let project_layers = load_project_layers( &cwd, &project_trust_context.project_root, @@ -252,11 +291,9 @@ async fn load_config_toml_for_required_layer( 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(|e| { - io::Error::new( - io::ErrorKind::InvalidData, - format!("Error parsing config file {}: {e}", toml_file.display()), - ) + 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(|| { io::Error::new( diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 9167f9f87..03b0706c8 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -6,6 +6,7 @@ use crate::config::ConfigOverrides; use crate::config::ConfigToml; use crate::config::ProjectConfig; use crate::config_loader::ConfigLayerEntry; +use crate::config_loader::ConfigLoadError; use crate::config_loader::ConfigRequirements; use crate::config_loader::config_requirements::ConfigRequirementsWithSources; use crate::config_loader::fingerprint::version_for_toml; @@ -21,6 +22,13 @@ use std::path::Path; use tempfile::tempdir; use toml::Value as TomlValue; +fn config_error_from_io(err: &std::io::Error) -> &super::ConfigError { + err.get_ref() + .and_then(|err| err.downcast_ref::()) + .map(ConfigLoadError::config_error) + .expect("expected ConfigLoadError") +} + async fn make_config_for_test( codex_home: &Path, project_path: &Path, @@ -44,6 +52,98 @@ async fn make_config_for_test( .await } +#[tokio::test] +async fn returns_config_error_for_invalid_user_config_toml() { + let tmp = tempdir().expect("tempdir"); + let contents = "model = \"gpt-4\"\ninvalid = ["; + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, contents).expect("write config"); + + let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); + let err = load_config_layers_state( + tmp.path(), + Some(cwd), + &[] as &[(String, TomlValue)], + LoaderOverrides::default(), + ) + .await + .expect_err("expected error"); + + let config_error = config_error_from_io(&err); + let expected_toml_error = toml::from_str::(contents).expect_err("parse error"); + let expected_config_error = + super::config_error_from_toml(&config_path, contents, expected_toml_error); + assert_eq!(config_error, &expected_config_error); +} + +#[tokio::test] +async fn returns_config_error_for_invalid_managed_config_toml() { + let tmp = tempdir().expect("tempdir"); + let managed_path = tmp.path().join("managed_config.toml"); + let contents = "model = \"gpt-4\"\ninvalid = ["; + std::fs::write(&managed_path, contents).expect("write managed config"); + + let overrides = LoaderOverrides { + managed_config_path: Some(managed_path.clone()), + ..Default::default() + }; + + let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); + let err = load_config_layers_state( + tmp.path(), + Some(cwd), + &[] as &[(String, TomlValue)], + overrides, + ) + .await + .expect_err("expected error"); + + let config_error = config_error_from_io(&err); + let expected_toml_error = toml::from_str::(contents).expect_err("parse error"); + let expected_config_error = + super::config_error_from_toml(&managed_path, contents, expected_toml_error); + assert_eq!(config_error, &expected_config_error); +} + +#[tokio::test] +async fn returns_config_error_for_schema_error_in_user_config() { + let tmp = tempdir().expect("tempdir"); + let contents = "model_context_window = \"not_a_number\""; + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, contents).expect("write config"); + + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .build() + .await + .expect_err("expected error"); + + let config_error = config_error_from_io(&err); + let _guard = codex_utils_absolute_path::AbsolutePathBufGuard::new(tmp.path()); + let expected_config_error = + super::diagnostics::config_error_from_config_toml(&config_path, contents) + .expect("schema error"); + assert_eq!(config_error, &expected_config_error); +} + +#[test] +fn schema_error_points_to_feature_value() { + let tmp = tempdir().expect("tempdir"); + let contents = "[features]\ncollaboration_modes = \"true\""; + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, contents).expect("write config"); + + let _guard = codex_utils_absolute_path::AbsolutePathBufGuard::new(tmp.path()); + let error = super::diagnostics::config_error_from_config_toml(&config_path, contents) + .expect("schema error"); + + let value_line = contents.lines().nth(1).expect("value line"); + let value_column = value_line.find("\"true\"").expect("value") + 1; + assert_eq!(error.range.start.line, 2); + assert_eq!(error.range.start.column, value_column); +} + #[tokio::test] async fn merges_managed_config_layer_on_top() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 533b328e2..767989128 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -28,6 +28,8 @@ use codex_core::config::ConfigOverrides; use codex_core::config::find_codex_home; 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::git_info::get_git_repo_root; use codex_core::models_manager::manager::RefreshStrategy; use codex_core::protocol::AskForApproval; @@ -162,7 +164,18 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any { Ok(config_toml) => config_toml, Err(err) => { - eprintln!("Error loading config.toml: {err}"); + let config_error = err + .get_ref() + .and_then(|err| err.downcast_ref::()) + .map(ConfigLoadError::config_error); + if let Some(config_error) = config_error { + eprintln!( + "Error loading config.toml:\n{}", + format_config_error_with_source(config_error) + ); + } else { + eprintln!("Error loading config.toml: {err}"); + } std::process::exit(1); } } diff --git a/codex-rs/execpolicy/src/error.rs b/codex-rs/execpolicy/src/error.rs index 9664e71a5..25f922721 100644 --- a/codex-rs/execpolicy/src/error.rs +++ b/codex-rs/execpolicy/src/error.rs @@ -3,6 +3,24 @@ use thiserror::Error; pub type Result = std::result::Result; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TextPosition { + pub line: usize, + pub column: usize, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TextRange { + pub start: TextPosition, + pub end: TextPosition, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ErrorLocation { + pub path: String, + pub range: TextRange, +} + #[derive(Debug, Error)] pub enum Error { #[error("invalid decision: {0}")] @@ -26,3 +44,27 @@ pub enum Error { #[error("starlark error: {0}")] Starlark(StarlarkError), } + +impl Error { + pub fn location(&self) -> Option { + match self { + Error::Starlark(err) => err.span().map(|span| { + let resolved = span.resolve_span(); + ErrorLocation { + path: span.filename().to_string(), + range: TextRange { + start: TextPosition { + line: resolved.begin.line + 1, + column: resolved.begin.column + 1, + }, + end: TextPosition { + line: resolved.end.line + 1, + column: resolved.end.column + 1, + }, + }, + } + }), + _ => None, + } + } +} diff --git a/codex-rs/execpolicy/src/lib.rs b/codex-rs/execpolicy/src/lib.rs index 31062f1cb..a9453f28d 100644 --- a/codex-rs/execpolicy/src/lib.rs +++ b/codex-rs/execpolicy/src/lib.rs @@ -10,7 +10,10 @@ pub use amend::AmendError; pub use amend::blocking_append_allow_prefix_rule; pub use decision::Decision; pub use error::Error; +pub use error::ErrorLocation; pub use error::Result; +pub use error::TextPosition; +pub use error::TextRange; pub use execpolicycheck::ExecPolicyCheckCommand; pub use parser::PolicyParser; pub use policy::Evaluation; diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index cac1b12e0..e3ee2ed4e 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -23,6 +23,8 @@ use codex_core::config::ConfigOverrides; use codex_core::config::find_codex_home; 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::find_thread_path_by_id_str; use codex_core::get_platform_sandbox; use codex_core::protocol::AskForApproval; @@ -177,7 +179,18 @@ pub async fn run_main( { Ok(config_toml) => config_toml, Err(err) => { - eprintln!("Error loading config.toml: {err}"); + let config_error = err + .get_ref() + .and_then(|err| err.downcast_ref::()) + .map(ConfigLoadError::config_error); + if let Some(config_error) = config_error { + eprintln!( + "Error loading config.toml:\n{}", + format_config_error_with_source(config_error) + ); + } else { + eprintln!("Error loading config.toml: {err}"); + } std::process::exit(1); } };