mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
Report syntax errors in rules file (#11686)
Currently, if there are syntax errors detected in the starlark rules file, the entire policy is silently ignored by the CLI. The app server correctly emits a message that can be displayed in a GUI. This PR changes the CLI (both the TUI and non-interactive exec) to fail when the rules file can't be parsed. It then prints out an error message and exits with a non-zero exit code. This is consistent with the handling of errors in the config file. This addresses #11603
This commit is contained in:
@@ -265,6 +265,77 @@ pub async fn check_execpolicy_for_warnings(
|
||||
Ok(warning)
|
||||
}
|
||||
|
||||
fn exec_policy_message_for_display(source: &codex_execpolicy::Error) -> String {
|
||||
let message = source.to_string();
|
||||
if let Some(line) = message
|
||||
.lines()
|
||||
.find(|line| line.trim_start().starts_with("error: "))
|
||||
{
|
||||
return line.to_owned();
|
||||
}
|
||||
if let Some(first_line) = message.lines().next()
|
||||
&& let Some((_, detail)) = first_line.rsplit_once(": starlark error: ")
|
||||
{
|
||||
return detail.trim().to_string();
|
||||
}
|
||||
|
||||
message
|
||||
.lines()
|
||||
.next()
|
||||
.unwrap_or_default()
|
||||
.trim()
|
||||
.to_string()
|
||||
}
|
||||
|
||||
fn parse_starlark_line_from_message(message: &str) -> Option<(PathBuf, usize)> {
|
||||
let first_line = message.lines().next()?.trim();
|
||||
let (path_and_position, _) = first_line.rsplit_once(": starlark error:")?;
|
||||
|
||||
let mut parts = path_and_position.rsplitn(3, ':');
|
||||
let _column = parts.next()?.parse::<usize>().ok()?;
|
||||
let line = parts.next()?.parse::<usize>().ok()?;
|
||||
let path = PathBuf::from(parts.next()?);
|
||||
|
||||
if line == 0 {
|
||||
return None;
|
||||
}
|
||||
|
||||
Some((path, line))
|
||||
}
|
||||
|
||||
pub fn format_exec_policy_error_with_source(error: &ExecPolicyError) -> String {
|
||||
match error {
|
||||
ExecPolicyError::ParsePolicy { path, source } => {
|
||||
let rendered_source = source.to_string();
|
||||
let structured_location = source
|
||||
.location()
|
||||
.map(|location| (PathBuf::from(location.path), location.range.start.line));
|
||||
let parsed_location = parse_starlark_line_from_message(&rendered_source);
|
||||
let location = match (structured_location, parsed_location) {
|
||||
(Some((_, 1)), Some((parsed_path, parsed_line))) if parsed_line > 1 => {
|
||||
Some((parsed_path, parsed_line))
|
||||
}
|
||||
(Some(structured), _) => Some(structured),
|
||||
(None, parsed) => parsed,
|
||||
};
|
||||
let message = exec_policy_message_for_display(source);
|
||||
match location {
|
||||
Some((path, line)) => {
|
||||
format!(
|
||||
"{}:{}: {} (problem is on or around line {})",
|
||||
path.display(),
|
||||
line,
|
||||
message,
|
||||
line
|
||||
)
|
||||
}
|
||||
None => format!("{path}: {message}"),
|
||||
}
|
||||
}
|
||||
_ => error.to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
async fn load_exec_policy_with_warning(
|
||||
config_stack: &ConfigLayerStack,
|
||||
) -> Result<(Policy, Option<ExecPolicyError>), ExecPolicyError> {
|
||||
@@ -679,6 +750,51 @@ mod tests {
|
||||
assert!(files.is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn format_exec_policy_error_with_source_renders_range() {
|
||||
let temp_dir = tempdir().expect("create temp dir");
|
||||
let config_stack = config_stack_for_dot_codex_folder(temp_dir.path());
|
||||
let policy_dir = temp_dir.path().join(RULES_DIR_NAME);
|
||||
fs::create_dir_all(&policy_dir).expect("create policy dir");
|
||||
let broken_path = policy_dir.join("broken.rules");
|
||||
fs::write(
|
||||
&broken_path,
|
||||
r#"prefix_rule(
|
||||
pattern = ["tmux capture-pane"],
|
||||
decision = "allow",
|
||||
match = ["tmux capture-pane -p"],
|
||||
)"#,
|
||||
)
|
||||
.expect("write broken policy file");
|
||||
|
||||
let err = load_exec_policy(&config_stack)
|
||||
.await
|
||||
.expect_err("expected parse error");
|
||||
let rendered = format_exec_policy_error_with_source(&err);
|
||||
|
||||
assert!(rendered.contains("broken.rules:1:"));
|
||||
assert!(rendered.contains("on or around line 1"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_starlark_line_from_message_extracts_path_and_line() {
|
||||
let parsed = parse_starlark_line_from_message(
|
||||
"/tmp/default.rules:143:1: starlark error: error: Parse error: unexpected new line",
|
||||
)
|
||||
.expect("parse should succeed");
|
||||
|
||||
assert_eq!(parsed.0, PathBuf::from("/tmp/default.rules"));
|
||||
assert_eq!(parsed.1, 143);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_starlark_line_from_message_rejects_zero_line() {
|
||||
let parsed = parse_starlark_line_from_message(
|
||||
"/tmp/default.rules:0:1: starlark error: error: Parse error: unexpected new line",
|
||||
);
|
||||
assert_eq!(parsed, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn loads_policies_from_policy_subdirectory() {
|
||||
let temp_dir = tempdir().expect("create temp dir");
|
||||
|
||||
@@ -144,6 +144,7 @@ pub use apply_patch::CODEX_APPLY_PATCH_ARG1;
|
||||
pub use client::X_CODEX_TURN_METADATA_HEADER;
|
||||
pub use exec_policy::ExecPolicyError;
|
||||
pub use exec_policy::check_execpolicy_for_warnings;
|
||||
pub use exec_policy::format_exec_policy_error_with_source;
|
||||
pub use exec_policy::load_exec_policy;
|
||||
pub use file_watcher::FileWatcherEvent;
|
||||
pub use safety::get_platform_sandbox;
|
||||
|
||||
@@ -20,6 +20,7 @@ use codex_core::NewThread;
|
||||
use codex_core::OLLAMA_OSS_PROVIDER_ID;
|
||||
use codex_core::ThreadManager;
|
||||
use codex_core::auth::enforce_login_restrictions;
|
||||
use codex_core::check_execpolicy_for_warnings;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
@@ -28,6 +29,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::format_exec_policy_error_with_source;
|
||||
use codex_core::git_info::get_git_repo_root;
|
||||
use codex_core::models_manager::manager::RefreshStrategy;
|
||||
use codex_core::protocol::AskForApproval;
|
||||
@@ -267,6 +269,19 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
.cloud_requirements(cloud_requirements)
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
#[allow(clippy::print_stderr)]
|
||||
match check_execpolicy_for_warnings(&config.config_layer_stack).await {
|
||||
Ok(None) => {}
|
||||
Ok(Some(err)) | Err(err) => {
|
||||
eprintln!(
|
||||
"Error loading rules:\n{}",
|
||||
format_exec_policy_error_with_source(&err)
|
||||
);
|
||||
std::process::exit(1);
|
||||
}
|
||||
}
|
||||
|
||||
set_default_client_residency_requirement(config.enforce_residency.value());
|
||||
|
||||
if let Err(err) = enforce_login_restrictions(&config) {
|
||||
|
||||
@@ -15,6 +15,7 @@ use codex_core::RolloutRecorder;
|
||||
use codex_core::ThreadSortKey;
|
||||
use codex_core::auth::AuthMode;
|
||||
use codex_core::auth::enforce_login_restrictions;
|
||||
use codex_core::check_execpolicy_for_warnings;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
@@ -27,6 +28,7 @@ use codex_core::config_loader::format_config_error_with_source;
|
||||
use codex_core::default_client::set_default_client_residency_requirement;
|
||||
use codex_core::find_thread_path_by_id_str;
|
||||
use codex_core::find_thread_path_by_name_str;
|
||||
use codex_core::format_exec_policy_error_with_source;
|
||||
use codex_core::path_utils;
|
||||
use codex_core::protocol::AskForApproval;
|
||||
use codex_core::read_session_meta_line;
|
||||
@@ -288,6 +290,19 @@ pub async fn run_main(
|
||||
cloud_requirements.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
#[allow(clippy::print_stderr)]
|
||||
match check_execpolicy_for_warnings(&config.config_layer_stack).await {
|
||||
Ok(None) => {}
|
||||
Ok(Some(err)) | Err(err) => {
|
||||
eprintln!(
|
||||
"Error loading rules:\n{}",
|
||||
format_exec_policy_error_with_source(&err)
|
||||
);
|
||||
std::process::exit(1);
|
||||
}
|
||||
}
|
||||
|
||||
set_default_client_residency_requirement(config.enforce_residency.value());
|
||||
|
||||
if let Some(warning) =
|
||||
|
||||
Reference in New Issue
Block a user