mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Improve handling of config and rules errors for app server clients (#9182)
When an invalid config.toml key or value is detected, the CLI currently just quits. This leaves the VSCE in a dead state. This PR changes the behavior to not quit and bubble up the config error to users to make it actionable. It also surfaces errors related to "rules" parsing. This allows us to surface these errors to users in the VSCE, like this: <img width="342" height="129" alt="Screenshot 2026-01-13 at 4 29 22 PM" src="https://github.com/user-attachments/assets/a79ffbe7-7604-400c-a304-c5165b6eebc4" /> <img width="346" height="244" alt="Screenshot 2026-01-13 at 4 45 06 PM" src="https://github.com/user-attachments/assets/de874f7c-16a2-4a95-8c6d-15f10482e67b" />
This commit is contained in:
@@ -251,7 +251,7 @@ impl Codex {
|
||||
|
||||
let exec_policy = ExecPolicyManager::load(&config.features, &config.config_layer_stack)
|
||||
.await
|
||||
.map_err(|err| CodexErr::Fatal(format!("failed to load execpolicy: {err}")))?;
|
||||
.map_err(|err| CodexErr::Fatal(format!("failed to load rules: {err}")))?;
|
||||
|
||||
let config = Arc::new(config);
|
||||
let _ = models_manager
|
||||
|
||||
@@ -460,6 +460,28 @@ impl Config {
|
||||
.await
|
||||
}
|
||||
|
||||
/// Load a default configuration when user config files are invalid.
|
||||
pub fn load_default_with_cli_overrides(
|
||||
cli_overrides: Vec<(String, TomlValue)>,
|
||||
) -> std::io::Result<Self> {
|
||||
let codex_home = find_codex_home()?;
|
||||
let mut merged = toml::Value::try_from(ConfigToml::default()).map_err(|e| {
|
||||
std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidData,
|
||||
format!("failed to serialize default config: {e}"),
|
||||
)
|
||||
})?;
|
||||
let cli_layer = crate::config_loader::build_cli_overrides_layer(&cli_overrides);
|
||||
crate::config_loader::merge_toml_values(&mut merged, &cli_layer);
|
||||
let config_toml = deserialize_config_toml_with_base(merged, &codex_home)?;
|
||||
Self::load_config_with_layer_stack(
|
||||
config_toml,
|
||||
ConfigOverrides::default(),
|
||||
codex_home,
|
||||
ConfigLayerStack::default(),
|
||||
)
|
||||
}
|
||||
|
||||
/// This is a secondary way of creating [Config], which is appropriate when
|
||||
/// the harness is meant to be used with a specific configuration that
|
||||
/// ignores user settings. For example, the `codex exec` subcommand is
|
||||
|
||||
@@ -31,6 +31,7 @@ pub use config_requirements::McpServerRequirement;
|
||||
pub use config_requirements::RequirementSource;
|
||||
pub use config_requirements::SandboxModeRequirement;
|
||||
pub use merge::merge_toml_values;
|
||||
pub(crate) use overrides::build_cli_overrides_layer;
|
||||
pub use state::ConfigLayerEntry;
|
||||
pub use state::ConfigLayerStack;
|
||||
pub use state::ConfigLayerStackOrdering;
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
use toml::Value as TomlValue;
|
||||
|
||||
pub(super) fn default_empty_table() -> TomlValue {
|
||||
pub(crate) fn default_empty_table() -> TomlValue {
|
||||
TomlValue::Table(Default::default())
|
||||
}
|
||||
|
||||
pub(super) fn build_cli_overrides_layer(cli_overrides: &[(String, TomlValue)]) -> TomlValue {
|
||||
pub(crate) fn build_cli_overrides_layer(cli_overrides: &[(String, TomlValue)]) -> TomlValue {
|
||||
let mut root = default_empty_table();
|
||||
for (path, value) in cli_overrides {
|
||||
apply_toml_override(&mut root, path, value.clone());
|
||||
|
||||
@@ -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))),
|
||||
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)
|
||||
}
|
||||
|
||||
@@ -114,6 +114,7 @@ pub use apply_patch::CODEX_APPLY_PATCH_ARG1;
|
||||
pub use command_safety::is_dangerous_command;
|
||||
pub use command_safety::is_safe_command;
|
||||
pub use exec_policy::ExecPolicyError;
|
||||
pub use exec_policy::check_execpolicy_for_warnings;
|
||||
pub use exec_policy::load_exec_policy;
|
||||
pub use safety::get_platform_sandbox;
|
||||
pub use safety::is_windows_elevated_sandbox_enabled;
|
||||
|
||||
Reference in New Issue
Block a user