valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: overhaul SandboxPolicy and config loading in Rust (#732)

Browse Source 
Previous to this PR, `SandboxPolicy` was a bit difficult to work with:


237f8a11e1/codex-rs/core/src/protocol.rs (L98-L108)

Specifically:

* It was an `enum` and therefore options were mutually exclusive as
opposed to additive.
* It defined things in terms of what the agent _could not_ do as opposed
to what they _could_ do. This made things hard to support because we
would prefer to build up a sandbox config by starting with something
extremely restrictive and only granting permissions for things the user
as explicitly allowed.

This PR changes things substantially by redefining the policy in terms
of two concepts:

* A `SandboxPermission` enum that defines permissions that can be
granted to the agent/sandbox.
* A `SandboxPolicy` that internally stores a `Vec<SandboxPermission>`,
but externally exposes a simpler API that can be used to configure
Seatbelt/Landlock.

Previous to this PR, we supported a `--sandbox` flag that effectively
mapped to an enum value in `SandboxPolicy`. Though now that
`SandboxPolicy` is a wrapper around `Vec<SandboxPermission>`, the single
`--sandbox` flag no longer makes sense. While I could have turned it
into a flag that the user can specify multiple times, I think the
current values to use with such a flag are long and potentially messy,
so for the moment, I have dropped support for `--sandbox` altogether and
we can bring it back once we have figured out the naming thing.

Since `--sandbox` is gone, users now have to specify `--full-auto` to
get a sandbox that allows writes in `cwd`. Admittedly, there is no clean
way to specify the equivalent of `--full-auto` in your `config.toml`
right now, so we will have to revisit that, as well.

Because `Config` presents a `SandboxPolicy` field and `SandboxPolicy`
changed considerably, I had to overhaul how config loading works, as
well. There are now two distinct concepts, `ConfigToml` and `Config`:

* `ConfigToml` is the deserialization of `~/.codex/config.toml`. As one
might expect, every field is `Optional` and it is `#[derive(Deserialize,
Default)]`. Consistent use of `Optional` makes it clear what the user
has specified explicitly.
* `Config` is the "normalized config" and is produced by merging
`ConfigToml` with `ConfigOverrides`. Where `ConfigToml` contains a raw
`Option<Vec<SandboxPermission>>`, `Config` presents only the final
`SandboxPolicy`.

The changes to `core/src/exec.rs` and `core/src/linux.rs` merit extra
special attention to ensure we are faithfully mapping the
`SandboxPolicy` to the Seatbelt and Landlock configs, respectively.

Also, take note that `core/src/seatbelt_readonly_policy.sbpl` has been
renamed to `codex-rs/core/src/seatbelt_base_policy.sbpl` and that
`(allow file-read*)` has been removed from the `.sbpl` file as now this
is added to the policy in `core/src/exec.rs` when
`sandbox_policy.has_full_disk_read_access()` is `true`.
This commit is contained in:
Michael Bolin
2025-04-29 15:01:16 -07:00
committed by GitHub
parent 237f8a11e1
commit 0a00b5ed29
21 changed files with 408 additions and 259 deletions
Download Patch File Download Diff File Expand all files Collapse all files

136
codex-rs/core/src/config.rs
View File

@@ -1,5 +1,6 @@
use crate::flags::OPENAI_DEFAULT_MODEL;
use crate::protocol::AskForApproval;
use crate::protocol::SandboxPermission;
use crate::protocol::SandboxPolicy;
use dirs::home_dir;
use serde::Deserialize;
@@ -11,27 +12,68 @@ use std::path::PathBuf;
const EMBEDDED_INSTRUCTIONS: &str = include_str!("../prompt.md");
/// Application configuration loaded from disk and merged with overrides.
#[derive(Deserialize, Debug, Clone)]
#[derive(Debug, Clone)]
pub struct Config {
/// Optional override of model selection.
#[serde(default = "default_model")]
pub model: String,
/// Default approval policy for executing commands.
#[serde(default)]
/// Approval policy for executing commands.
pub approval_policy: AskForApproval,
#[serde(default)]
pub sandbox_policy: SandboxPolicy,
/// Disable server-side response storage (sends the full conversation
/// context with every request). Currently necessary for OpenAI customers
/// who have opted into Zero Data Retention (ZDR).
#[serde(default)]
pub disable_response_storage: bool,
/// System instructions.
pub instructions: Option<String>,
}
/// Base config deserialized from ~/.codex/config.toml.
#[derive(Deserialize, Debug, Clone, Default)]
pub struct ConfigToml {
/// Optional override of model selection.
pub model: Option<String>,
/// Default approval policy for executing commands.
pub approval_policy: Option<AskForApproval>,
pub sandbox_permissions: Option<Vec<SandboxPermission>>,
/// Disable server-side response storage (sends the full conversation
/// context with every request). Currently necessary for OpenAI customers
/// who have opted into Zero Data Retention (ZDR).
pub disable_response_storage: Option<bool>,
/// System instructions.
pub instructions: Option<String>,
}
impl ConfigToml {
/// Attempt to parse the file at `~/.codex/config.toml`. If it does not
/// exist, return a default config. Though if it exists and cannot be
/// parsed, report that to the user and force them to fix it.
fn load_from_toml() -> std::io::Result<Self> {
let config_toml_path = codex_dir()?.join("config.toml");
match std::fs::read_to_string(&config_toml_path) {
Ok(contents) => toml::from_str::<Self>(&contents).map_err(|e| {
tracing::error!("Failed to parse config.toml: {e}");
std::io::Error::new(std::io::ErrorKind::InvalidData, e)
}),
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
tracing::info!("config.toml not found, using defaults");
Ok(Self::default())
}
Err(e) => {
tracing::error!("Failed to read config.toml: {e}");
Err(e)
}
}
}
}
/// Optional overrides for user configuration (e.g., from CLI flags).
#[derive(Default, Debug, Clone)]
pub struct ConfigOverrides {
@@ -46,11 +88,14 @@ impl Config {
/// ~/.codex/config.toml, ~/.codex/instructions.md, embedded defaults, and
/// any values provided in `overrides` (highest precedence).
pub fn load_with_overrides(overrides: ConfigOverrides) -> std::io::Result<Self> {
let mut cfg: Config = Self::load_from_toml()?;
let cfg: ConfigToml = ConfigToml::load_from_toml()?;
tracing::warn!("Config parsed from config.toml: {cfg:?}");
Ok(Self::load_from_base_config_with_overrides(cfg, overrides))
}
fn load_from_base_config_with_overrides(cfg: ConfigToml, overrides: ConfigOverrides) -> Self {
// Instructions: user-provided instructions.md > embedded default.
cfg.instructions =
let instructions =
Self::load_instructions().or_else(|| Some(EMBEDDED_INSTRUCTIONS.to_string()));
// Destructure ConfigOverrides fully to ensure all overrides are applied.
@@ -61,57 +106,48 @@ impl Config {
disable_response_storage,
} = overrides;
if let Some(model) = model {
cfg.model = model;
}
if let Some(approval_policy) = approval_policy {
cfg.approval_policy = approval_policy;
}
if let Some(sandbox_policy) = sandbox_policy {
cfg.sandbox_policy = sandbox_policy;
}
if let Some(disable_response_storage) = disable_response_storage {
cfg.disable_response_storage = disable_response_storage;
}
Ok(cfg)
}
/// Attempt to parse the file at `~/.codex/config.toml` into a Config.
fn load_from_toml() -> std::io::Result<Self> {
let config_toml_path = codex_dir()?.join("config.toml");
match std::fs::read_to_string(&config_toml_path) {
Ok(contents) => toml::from_str::<Self>(&contents).map_err(|e| {
tracing::error!("Failed to parse config.toml: {e}");
std::io::Error::new(std::io::ErrorKind::InvalidData, e)
}),
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
tracing::info!("config.toml not found, using defaults");
Ok(Self::load_default_config())
}
Err(e) => {
tracing::error!("Failed to read config.toml: {e}");
Err(e)
let sandbox_policy = match sandbox_policy {
Some(sandbox_policy) => sandbox_policy,
None => {
// Derive a SandboxPolicy from the permissions in the config.
match cfg.sandbox_permissions {
// Note this means the user can explicitly set permissions
// to the empty list in the config file, granting it no
// permissions whatsoever.
Some(permissions) => SandboxPolicy::from(permissions),
// Default to read only rather than completely locked down.
None => SandboxPolicy::new_read_only_policy(),
}
}
};
Self {
model: model.or(cfg.model).unwrap_or_else(default_model),
approval_policy: approval_policy
.or(cfg.approval_policy)
.unwrap_or_else(AskForApproval::default),
sandbox_policy,
disable_response_storage: disable_response_storage
.or(cfg.disable_response_storage)
.unwrap_or(false),
instructions,
}
}
/// Meant to be used exclusively for tests: load_with_overrides() should be
/// used in all other cases.
pub fn load_default_config_for_test() -> Self {
Self::load_default_config()
}
fn load_default_config() -> Self {
// Load from an empty string to exercise #[serde(default)] to
// get the default values for each field.
toml::from_str::<Self>("").expect("empty string should parse as TOML")
}
fn load_instructions() -> Option<String> {
let mut p = codex_dir().ok()?;
p.push("instructions.md");
std::fs::read_to_string(&p).ok()
}
/// Meant to be used exclusively for tests: `load_with_overrides()` should
/// be used in all other cases.
pub fn load_default_config_for_test() -> Self {
Self::load_from_base_config_with_overrides(
ConfigToml::default(),
ConfigOverrides::default(),
)
}
}
fn default_model() -> String {