-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Improve handling of config and rules errors for app server clients #9182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
993f97b
96f9092
9286e7e
c0b73c0
1be99e7
d431a62
9837b87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| #![deny(clippy::print_stdout, clippy::print_stderr)] | ||
|
|
||
| use codex_common::CliConfigOverrides; | ||
| use codex_core::config::Config; | ||
| use codex_core::config::ConfigBuilder; | ||
| use codex_core::config_loader::LoaderOverrides; | ||
| use std::io::ErrorKind; | ||
|
|
@@ -10,7 +11,9 @@ use std::path::PathBuf; | |
| use crate::message_processor::MessageProcessor; | ||
| use crate::outgoing_message::OutgoingMessage; | ||
| use crate::outgoing_message::OutgoingMessageSender; | ||
| use codex_app_server_protocol::ConfigWarningNotification; | ||
| use codex_app_server_protocol::JSONRPCMessage; | ||
| use codex_core::check_execpolicy_for_warnings; | ||
| use codex_feedback::CodexFeedback; | ||
| use tokio::io::AsyncBufReadExt; | ||
| use tokio::io::AsyncWriteExt; | ||
|
|
@@ -82,14 +85,38 @@ pub async fn run_main( | |
| ) | ||
| })?; | ||
| let loader_overrides_for_config_api = loader_overrides.clone(); | ||
| let config = ConfigBuilder::default() | ||
| let mut config_warnings = Vec::new(); | ||
| let config = match ConfigBuilder::default() | ||
| .cli_overrides(cli_kv_overrides.clone()) | ||
| .loader_overrides(loader_overrides) | ||
| .build() | ||
| .await | ||
| .map_err(|e| { | ||
| std::io::Error::new(ErrorKind::InvalidData, format!("error loading config: {e}")) | ||
| })?; | ||
| { | ||
| Ok(config) => config, | ||
| Err(err) => { | ||
| let message = ConfigWarningNotification { | ||
| summary: "Invalid configuration; using defaults.".to_string(), | ||
| details: Some(err.to_string()), | ||
| }; | ||
| config_warnings.push(message); | ||
| Config::load_default_with_cli_overrides(cli_kv_overrides.clone()).map_err(|e| { | ||
| std::io::Error::new( | ||
| ErrorKind::InvalidData, | ||
| format!("error loading default config after config error: {e}"), | ||
| ) | ||
| })? | ||
| } | ||
| }; | ||
|
|
||
| if let Ok(Some(err)) = | ||
| check_execpolicy_for_warnings(&config.features, &config.config_layer_stack).await | ||
| { | ||
| let message = ConfigWarningNotification { | ||
| summary: "Error parsing rules; custom rules not applied.".to_string(), | ||
| details: Some(err.to_string()), | ||
| }; | ||
| config_warnings.push(message); | ||
| } | ||
|
|
||
| let feedback = CodexFeedback::new(); | ||
|
|
||
|
|
@@ -127,6 +154,12 @@ pub async fn run_main( | |
| .with(otel_logger_layer) | ||
| .with(otel_tracing_layer) | ||
| .try_init(); | ||
| for warning in &config_warnings { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If warnings is non-empty, should we start up or should we just exit? Admittedly, this would be more palatable if we were able to help the user find the errant
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should definitely start up. The current code just exits, and that leaves the user with a broken experience in the VSCE (just a spinner with no indication of a failure). |
||
| match &warning.details { | ||
| Some(details) => error!("{} {}", warning.summary, details), | ||
| None => error!("{}", warning.summary), | ||
| } | ||
| } | ||
|
|
||
| // Task: process incoming messages. | ||
| let processor_handle = tokio::spawn({ | ||
|
|
@@ -140,6 +173,7 @@ pub async fn run_main( | |
| cli_overrides, | ||
| loader_overrides, | ||
| feedback.clone(), | ||
| config_warnings, | ||
| ); | ||
| async move { | ||
| while let Some(msg) = incoming_rx.recv().await { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,19 +46,19 @@ fn is_policy_match(rule_match: &RuleMatch) -> bool { | |
|
|
||
| #[derive(Debug, Error)] | ||
| pub enum ExecPolicyError { | ||
| #[error("failed to read execpolicy files from {dir}: {source}")] | ||
| #[error("failed to read rules files from {dir}: {source}")] | ||
| ReadDir { | ||
| dir: PathBuf, | ||
| source: std::io::Error, | ||
| }, | ||
|
|
||
| #[error("failed to read execpolicy file {path}: {source}")] | ||
| #[error("failed to read rules file {path}: {source}")] | ||
| ReadFile { | ||
| path: PathBuf, | ||
| source: std::io::Error, | ||
| }, | ||
|
|
||
| #[error("failed to parse execpolicy file {path}: {source}")] | ||
| #[error("failed to parse rules file {path}: {source}")] | ||
| ParsePolicy { | ||
| path: String, | ||
| source: codex_execpolicy::Error, | ||
|
|
@@ -67,19 +67,19 @@ pub enum ExecPolicyError { | |
|
|
||
| #[derive(Debug, Error)] | ||
| pub enum ExecPolicyUpdateError { | ||
| #[error("failed to update execpolicy file {path}: {source}")] | ||
| #[error("failed to update rules file {path}: {source}")] | ||
| AppendRule { path: PathBuf, source: AmendError }, | ||
|
|
||
| #[error("failed to join blocking execpolicy update task: {source}")] | ||
| #[error("failed to join blocking rules update task: {source}")] | ||
| JoinBlockingTask { source: tokio::task::JoinError }, | ||
|
|
||
| #[error("failed to update in-memory execpolicy: {source}")] | ||
| #[error("failed to update in-memory rules: {source}")] | ||
| AddRule { | ||
| #[from] | ||
| source: ExecPolicyRuleError, | ||
| }, | ||
|
|
||
| #[error("cannot append execpolicy rule because execpolicy feature is disabled")] | ||
| #[error("cannot append rule because rules feature is disabled")] | ||
| FeatureDisabled, | ||
| } | ||
|
|
||
|
|
@@ -98,7 +98,11 @@ impl ExecPolicyManager { | |
| features: &Features, | ||
| config_stack: &ConfigLayerStack, | ||
| ) -> Result<Self, ExecPolicyError> { | ||
| let policy = load_exec_policy_for_features(features, config_stack).await?; | ||
| let (policy, warning) = | ||
| load_exec_policy_for_features_with_warning(features, config_stack).await?; | ||
| if let Some(err) = warning.as_ref() { | ||
| tracing::warn!("failed to parse rules: {err}"); | ||
| } | ||
| Ok(Self::new(Arc::new(policy))) | ||
| } | ||
|
|
||
|
|
@@ -195,14 +199,26 @@ impl Default for ExecPolicyManager { | |
| } | ||
| } | ||
|
|
||
| async fn load_exec_policy_for_features( | ||
| pub async fn check_execpolicy_for_warnings( | ||
| features: &Features, | ||
| config_stack: &ConfigLayerStack, | ||
| ) -> Result<Policy, ExecPolicyError> { | ||
| ) -> Result<Option<ExecPolicyError>, ExecPolicyError> { | ||
| let (_, warning) = load_exec_policy_for_features_with_warning(features, config_stack).await?; | ||
| Ok(warning) | ||
| } | ||
|
|
||
| async fn load_exec_policy_for_features_with_warning( | ||
| features: &Features, | ||
| config_stack: &ConfigLayerStack, | ||
| ) -> Result<(Policy, Option<ExecPolicyError>), ExecPolicyError> { | ||
| if !features.enabled(Feature::ExecPolicy) { | ||
| Ok(Policy::empty()) | ||
| } else { | ||
| load_exec_policy(config_stack).await | ||
| return Ok((Policy::empty(), None)); | ||
| } | ||
|
|
||
| match load_exec_policy(config_stack).await { | ||
| Ok(policy) => Ok((policy, None)), | ||
| Err(err @ ExecPolicyError::ParsePolicy { .. }) => Ok((Policy::empty(), Some(err))), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to falling back to a default config, falling back to an empty could be particularly harmful/confusing for a user.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree — that's why it's important to surface the warning to them.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #9187 should help a bit so that at least all Maybe we should have a
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you think of a case where we need multiple paths? Shouldn't we always be able to map an error to a specific file? Also, do you think we should go so far as to provide a line number and character offset? Or even a character range? This is a slippery slope which is why I kind of didn't want to go there right now since my main objective is to just keep the VSCE from hanging on startup. |
||
| Err(err) => Err(err), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -239,7 +255,7 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result<Policy, | |
| } | ||
|
|
||
| let policy = parser.build(); | ||
| tracing::debug!("loaded execpolicy from {} files", policy_paths.len()); | ||
| tracing::debug!("loaded rules from {} files", policy_paths.len()); | ||
|
|
||
| Ok(policy) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize it is not easy to do, but it would probably be most helpful if we could include the file (or files) that caused the issue and then helped the user open them in VS Code so they can make the necessary fixes and then reload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about that. I didn't want to make this PR more complicated. Could be a follow-on PR.