core/core-agent-ide
8
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: introduce AbsolutePathBuf and resolve relative paths in config.toml (#7796)

Browse source 
This PR attempts to solve two problems by introducing a
`AbsolutePathBuf` type with a special deserializer:

- `AbsolutePathBuf` attempts to be a generally useful abstraction, as it
ensures, by constructing, that it represents a value that is an
absolute, normalized path, which is a stronger guarantee than an
arbitrary `PathBuf`.
- Values in `config.toml` that can be either an absolute or relative
path should be resolved against the folder containing the `config.toml`
in the relative path case. This PR makes this easy to support: the main
cost is ensuring `AbsolutePathBufGuard` is used inside
`deserialize_config_toml_with_base()`.

While `AbsolutePathBufGuard` may seem slightly distasteful because it
relies on thread-local storage, this seems much cleaner to me than using
than my various experiments with
https://docs.rs/serde/latest/serde/de/trait.DeserializeSeed.html.
Further, since the `deserialize()` method from the `Deserialize` trait
is not async, we do not really have to worry about the deserialization
work being spread across multiple threads in a way that would interfere
with `AbsolutePathBufGuard`.

To start, this PR introduces the use of `AbsolutePathBuf` in
`OtelTlsConfig`. Note how this simplifies `otel_provider.rs` because it
no longer requires `settings.codex_home` to be threaded through.
Furthermore, this sets us up better for a world where multiple
`config.toml` files from different folders could be loaded and then
merged together, as the absolutifying of the paths must be done against
the correct parent folder.
This commit is contained in:
Michael Bolin 2025-12-09 17:37:52 -08:00 • committed by GitHub
parent 0c8828c5e2
commit fa4cac1e6b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 246 additions and 62 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
codex-rs/Cargo.lock generated
View file

@ -1157,6 +1157,7 @@ dependencies = [
"codex-otel",
"codex-protocol",
"codex-rmcp-client",
"codex-utils-absolute-path",
"codex-utils-pty",
"codex-utils-readiness",
"codex-utils-string",
@ -1464,6 +1465,7 @@ dependencies = [
"chrono",
"codex-app-server-protocol",
"codex-protocol",
"codex-utils-absolute-path",
"eventsource-stream",
"http",
"opentelemetry",
@ -1650,6 +1652,16 @@ dependencies = [
"codex-tui",
]
[[package]]
name = "codex-utils-absolute-path"
version = "0.0.0"
dependencies = [
"path-absolutize",
"serde",
"serde_json",
"tempfile",
]
[[package]]
name = "codex-utils-cache"
version = "0.0.0"

2
codex-rs/Cargo.toml
View file

@ -35,6 +35,7 @@ members = [
"otel",
"tui",
"tui2",
"utils/absolute-path",
"utils/git",
"utils/cache",
"utils/image",
@ -90,6 +91,7 @@ codex-rmcp-client = { path = "rmcp-client" }
codex-stdio-to-uds = { path = "stdio-to-uds" }
codex-tui = { path = "tui" }
codex-tui2 = { path = "tui2" }
codex-utils-absolute-path = { path = "utils/absolute-path" }
codex-utils-cache = { path = "utils/cache" }
codex-utils-image = { path = "utils/image" }
codex-utils-json-to-toml = { path = "utils/json-to-toml" }

19
codex-rs/core/Cargo.toml
View file

@ -1,8 +1,8 @@
[package]
name = "codex-core"
version.workspace = true
edition.workspace = true
license.workspace = true
name = "codex-core"
version.workspace = true
[lib]
doctest = false
@ -18,12 +18,12 @@ askama = { workspace = true }
async-channel = { workspace = true }
async-trait = { workspace = true }
base64 = { workspace = true }
chrono = { workspace = true, features = ["serde"] }
chardetng = { workspace = true }
chrono = { workspace = true, features = ["serde"] }
codex-api = { workspace = true }
codex-app-server-protocol = { workspace = true }
codex-apply-patch = { workspace = true }
codex-async-utils = { workspace = true }
codex-api = { workspace = true }
codex-execpolicy = { workspace = true }
codex-file-search = { workspace = true }
codex-git = { workspace = true }
@ -31,14 +31,15 @@ codex-keyring-store = { workspace = true }
codex-otel = { workspace = true, features = ["otel"] }
codex-protocol = { workspace = true }
codex-rmcp-client = { workspace = true }
codex-utils-absolute-path = { workspace = true }
codex-utils-pty = { workspace = true }
codex-utils-readiness = { workspace = true }
codex-utils-string = { workspace = true }
codex-windows-sandbox = { package = "codex-windows-sandbox", path = "../windows-sandbox-rs" }
dirs = { workspace = true }
dunce = { workspace = true }
env-flags = { workspace = true }
encoding_rs = { workspace = true }
env-flags = { workspace = true }
eventsource-stream = { workspace = true }
futures = { workspace = true }
http = { workspace = true }
@ -46,8 +47,10 @@ indexmap = { workspace = true }
keyring = { workspace = true, features = ["crypto-rust"] }
libc = { workspace = true }
mcp-types = { workspace = true }
once_cell = { workspace = true }
os_info = { workspace = true }
rand = { workspace = true }
regex = { workspace = true }
regex-lite = { workspace = true }
reqwest = { workspace = true, features = ["json", "stream"] }
serde = { workspace = true, features = ["derive"] }
@ -58,9 +61,6 @@ sha2 = { workspace = true }
shlex = { workspace = true }
similar = { workspace = true }
strum_macros = { workspace = true }
url = { workspace = true }
once_cell = { workspace = true }
regex = { workspace = true }
tempfile = { workspace = true }
test-case = "3.3.1"
test-log = { workspace = true }
@ -84,6 +84,7 @@ toml_edit = { workspace = true }
tracing = { workspace = true, features = ["log"] }
tree-sitter = { workspace = true }
tree-sitter-bash = { workspace = true }
url = { workspace = true }
uuid = { workspace = true, features = ["serde", "v4", "v5"] }
which = { workspace = true }
wildmatch = { workspace = true }
@ -94,9 +95,9 @@ test-support = []
[target.'cfg(target_os = "linux")'.dependencies]
keyring = { workspace = true, features = ["linux-native-async-persistent"] }
landlock = { workspace = true }
seccompiler = { workspace = true }
keyring = { workspace = true, features = ["linux-native-async-persistent"] }
[target.'cfg(target_os = "macos")'.dependencies]
core-foundation = "0.9"

39
codex-rs/core/src/config/mod.rs
View file

@ -40,6 +40,7 @@ use codex_protocol::config_types::TrustLevel;
use codex_protocol::config_types::Verbosity;
use codex_protocol::openai_models::ReasoningEffort;
use codex_rmcp_client::OAuthCredentialsStoreMode;
use codex_utils_absolute_path::AbsolutePathBufGuard;
use dirs::home_dir;
use dunce::canonicalize;
use serde::Deserialize;
@ -299,9 +300,9 @@ impl Config {
)
.await?;
let cfg: ConfigToml = root_value.try_into().map_err(|e| {
let cfg = deserialize_config_toml_with_base(root_value, &codex_home).map_err(|e| {
tracing::error!("Failed to deserialize overridden config: {e}");
std::io::Error::new(std::io::ErrorKind::InvalidData, e)
e
})?;
Self::load_from_base_config_with_overrides(cfg, overrides, codex_home)
@ -319,9 +320,9 @@ pub async fn load_config_as_toml_with_cli_overrides(
)
.await?;
let cfg: ConfigToml = root_value.try_into().map_err(|e| {
let cfg = deserialize_config_toml_with_base(root_value, codex_home).map_err(|e| {
tracing::error!("Failed to deserialize overridden config: {e}");
std::io::Error::new(std::io::ErrorKind::InvalidData, e)
e
})?;
Ok(cfg)
@ -357,6 +358,18 @@ fn apply_overlays(
base
}
fn deserialize_config_toml_with_base(
root_value: TomlValue,
config_base_dir: &Path,
) -> std::io::Result<ConfigToml> {
// This guard ensures that any relative paths that is deserialized into an
// [AbsolutePathBuf] is resolved against `config_base_dir`.
let _guard = AbsolutePathBufGuard::new(config_base_dir);
root_value
.try_into()
.map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))
}
pub async fn load_global_mcp_servers(
codex_home: &Path,
) -> std::io::Result<BTreeMap<String, McpServerConfig>> {
@ -1852,10 +1865,11 @@ trust_level = "trusted"
};
let root_value = load_resolved_config(codex_home.path(), Vec::new(), overrides).await?;
let cfg: ConfigToml = root_value.try_into().map_err(|e| {
tracing::error!("Failed to deserialize overridden config: {e}");
std::io::Error::new(std::io::ErrorKind::InvalidData, e)
})?;
let cfg =
deserialize_config_toml_with_base(root_value, codex_home.path()).map_err(|e| {
tracing::error!("Failed to deserialize overridden config: {e}");
e
})?;
assert_eq!(
cfg.mcp_oauth_credentials_store,
Some(OAuthCredentialsStoreMode::Keyring),
@ -1972,10 +1986,11 @@ trust_level = "trusted"
)
.await?;
let cfg: ConfigToml = root_value.try_into().map_err(|e| {
tracing::error!("Failed to deserialize overridden config: {e}");
std::io::Error::new(std::io::ErrorKind::InvalidData, e)
})?;
let cfg =
deserialize_config_toml_with_base(root_value, codex_home.path()).map_err(|e| {
tracing::error!("Failed to deserialize overridden config: {e}");
e
})?;
assert_eq!(cfg.model.as_deref(), Some("managed_config"));
Ok(())

9
codex-rs/core/src/config/types.rs
View file

@ -3,13 +3,14 @@
// Note this file should generally be restricted to simple struct/enum
// definitions that do not contain business logic.
use serde::Deserializer;
use codex_utils_absolute_path::AbsolutePathBuf;
use std::collections::HashMap;
use std::path::PathBuf;
use std::time::Duration;
use wildmatch::WildMatchPattern;
use serde::Deserialize;
use serde::Deserializer;
use serde::Serialize;
use serde::de::Error as SerdeError;
@ -285,9 +286,9 @@ pub enum OtelHttpProtocol {
#[derive(Deserialize, Debug, Clone, PartialEq, Default)]
#[serde(rename_all = "kebab-case")]
pub struct OtelTlsConfig {
pub ca_certificate: Option<PathBuf>,
pub client_certificate: Option<PathBuf>,
pub client_private_key: Option<PathBuf>,
pub ca_certificate: Option<AbsolutePathBuf>,
pub client_certificate: Option<AbsolutePathBuf>,
pub client_private_key: Option<AbsolutePathBuf>,
}
/// Which OTEL exporter to use.

1
codex-rs/otel/Cargo.toml
View file

@ -21,6 +21,7 @@ otel = ["opentelemetry", "opentelemetry_sdk", "opentelemetry-otlp", "tonic"]
[dependencies]
chrono = { workspace = true }
codex-app-server-protocol = { workspace = true }
codex-utils-absolute-path = { workspace = true }
codex-protocol = { workspace = true }
eventsource-stream = { workspace = true }
opentelemetry = { workspace = true, features = ["logs"], optional = true }

8
codex-rs/otel/src/config.rs
View file

@ -1,6 +1,8 @@
use std::collections::HashMap;
use std::path::PathBuf;
use codex_utils_absolute_path::AbsolutePathBuf;
#[derive(Clone, Debug)]
pub struct OtelSettings {
pub environment: String,
@ -20,9 +22,9 @@ pub enum OtelHttpProtocol {
#[derive(Clone, Debug, Default)]
pub struct OtelTlsConfig {
pub ca_certificate: Option<PathBuf>,
pub client_certificate: Option<PathBuf>,
pub client_private_key: Option<PathBuf>,
pub ca_certificate: Option<AbsolutePathBuf>,
pub client_certificate: Option<AbsolutePathBuf>,
pub client_private_key: Option<AbsolutePathBuf>,
}
#[derive(Clone, Debug)]

49
codex-rs/otel/src/otel_provider.rs
View file

@ -2,6 +2,7 @@ use crate::config::OtelExporter;
use crate::config::OtelHttpProtocol;
use crate::config::OtelSettings;
use crate::config::OtelTlsConfig;
use codex_utils_absolute_path::AbsolutePathBuf;
use http::Uri;
use opentelemetry::KeyValue;
use opentelemetry_otlp::LogExporter;
@ -25,7 +26,6 @@ use std::error::Error;
use std::fs;
use std::io::ErrorKind;
use std::io::{self};
use std::path::Path;
use std::path::PathBuf;
use std::time::Duration;
use tonic::metadata::MetadataMap;
@ -85,12 +85,7 @@ impl OtelProvider {
.assume_http2(true);
let tls_config = match tls.as_ref() {
Some(tls) => build_grpc_tls_config(
endpoint,
base_tls_config,
tls,
settings.codex_home.as_path(),
)?,
Some(tls) => build_grpc_tls_config(endpoint, base_tls_config, tls)?,
None => base_tls_config,
};
@ -123,7 +118,7 @@ impl OtelProvider {
.with_headers(headers.clone());
if let Some(tls) = tls.as_ref() {
let client = build_http_client(tls, settings.codex_home.as_path())?;
let client = build_http_client(tls)?;
exporter_builder = exporter_builder.with_http_client(client);
}
@ -149,7 +144,6 @@ fn build_grpc_tls_config(
endpoint: &str,
tls_config: ClientTlsConfig,
tls: &OtelTlsConfig,
codex_home: &Path,
) -> Result<ClientTlsConfig, Box<dyn Error>> {
let uri: Uri = endpoint.parse()?;
let host = uri.host().ok_or_else(|| {
@ -161,14 +155,14 @@ fn build_grpc_tls_config(
let mut config = tls_config.domain_name(host.to_owned());
if let Some(path) = tls.ca_certificate.as_ref() {
let (pem, _) = read_bytes(codex_home, path)?;
let (pem, _) = read_bytes(path)?;
config = config.ca_certificate(TonicCertificate::from_pem(pem));
}
match (&tls.client_certificate, &tls.client_private_key) {
(Some(cert_path), Some(key_path)) => {
let (cert_pem, _) = read_bytes(codex_home, cert_path)?;
let (key_pem, _) = read_bytes(codex_home, key_path)?;
let (cert_pem, _) = read_bytes(cert_path)?;
let (key_pem, _) = read_bytes(key_path)?;
config = config.identity(TonicIdentity::from_pem(cert_pem, key_pem));
}
(Some(_), None) | (None, Some(_)) => {
@ -188,24 +182,20 @@ fn build_grpc_tls_config(
/// `opentelemetry_sdk` `BatchLogProcessor` spawns a dedicated OS thread that uses
/// `futures_executor::block_on()` rather than tokio. When the async reqwest client's
/// timeout calls `tokio::time::sleep()`, it panics with "no reactor running".
fn build_http_client(
tls: &OtelTlsConfig,
codex_home: &Path,
) -> Result<reqwest::blocking::Client, Box<dyn Error>> {
fn build_http_client(tls: &OtelTlsConfig) -> Result<reqwest::blocking::Client, Box<dyn Error>> {
// Wrap in block_in_place because reqwest::blocking::Client creates its own
// internal tokio runtime, which would panic if built directly from an async context.
tokio::task::block_in_place(|| build_http_client_inner(tls, codex_home))
tokio::task::block_in_place(|| build_http_client_inner(tls))
}
fn build_http_client_inner(
tls: &OtelTlsConfig,
codex_home: &Path,
) -> Result<reqwest::blocking::Client, Box<dyn Error>> {
let mut builder = reqwest::blocking::Client::builder()
.timeout(resolve_otlp_timeout(OTEL_EXPORTER_OTLP_LOGS_TIMEOUT));
if let Some(path) = tls.ca_certificate.as_ref() {
let (pem, location) = read_bytes(codex_home, path)?;
let (pem, location) = read_bytes(path)?;
let certificate = ReqwestCertificate::from_pem(pem.as_slice()).map_err(|error| {
config_error(format!(
"failed to parse certificate {}: {error}",
@ -220,8 +210,8 @@ fn build_http_client_inner(
match (&tls.client_certificate, &tls.client_private_key) {
(Some(cert_path), Some(key_path)) => {
let (mut cert_pem, cert_location) = read_bytes(codex_home, cert_path)?;
let (key_pem, key_location) = read_bytes(codex_home, key_path)?;
let (mut cert_pem, cert_location) = read_bytes(cert_path)?;
let (key_pem, key_location) = read_bytes(key_path)?;
cert_pem.extend_from_slice(key_pem.as_slice());
let identity = ReqwestIdentity::from_pem(cert_pem.as_slice()).map_err(|error| {
config_error(format!(
@ -264,25 +254,16 @@ fn read_timeout_env(var: &str) -> Option<Duration> {
Some(Duration::from_millis(parsed as u64))
}
fn read_bytes(base: &Path, provided: &PathBuf) -> Result<(Vec<u8>, PathBuf), Box<dyn Error>> {
let resolved = resolve_config_path(base, provided);
match fs::read(&resolved) {
Ok(bytes) => Ok((bytes, resolved)),
fn read_bytes(path: &AbsolutePathBuf) -> Result<(Vec<u8>, PathBuf), Box<dyn Error>> {
match fs::read(path) {
Ok(bytes) => Ok((bytes, path.to_path_buf())),
Err(error) => Err(Box::new(io::Error::new(
error.kind(),
format!("failed to read {}: {error}", resolved.display()),
format!("failed to read {}: {error}", path.display()),
))),
}
}
fn resolve_config_path(base: &Path, provided: &PathBuf) -> PathBuf {
if provided.is_absolute() {
provided.clone()
} else {
base.join(provided)
}
}
fn config_error(message: impl Into<String>) -> Box<dyn Error> {
Box::new(io::Error::new(ErrorKind::InvalidData, message.into()))
}

17
codex-rs/utils/absolute-path/Cargo.toml Normal file
View file

@ -0,0 +1,17 @@
[package]
name = "codex-utils-absolute-path"
version.workspace = true
edition.workspace = true
license.workspace = true
[lints]
workspace = true
[dependencies]
path-absolutize = { workspace = true }
serde = { workspace = true, features = ["derive"] }
[dev-dependencies]
serde_json = { workspace = true }
tempfile = { workspace = true }

152
codex-rs/utils/absolute-path/src/lib.rs Normal file
View file

@ -0,0 +1,152 @@
use path_absolutize::Absolutize;
use serde::Deserialize;
use serde::Deserializer;
use serde::Serialize;
use serde::de::Error as SerdeError;
use std::cell::RefCell;
use std::path::Display;
use std::path::Path;
use std::path::PathBuf;
/// A path that is guaranteed to be absolute and normalized (though it is not
/// guaranteed to be canonicalized or exist on the filesystem).
///
/// IMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set
/// using `AbsolutePathBufGuard::new(base_path)`. If no base path is set, the
/// deserialization will fail unless the path being deserialized is already
/// absolute.
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
pub struct AbsolutePathBuf(PathBuf);
impl AbsolutePathBuf {
pub fn resolve_path_against_base<P, B>(path: P, base_path: B) -> std::io::Result<Self>
where
P: AsRef<Path>,
B: AsRef<Path>,
{
let absolute_path = path.as_ref().absolutize_from(base_path.as_ref())?;
Ok(Self(absolute_path.into_owned()))
}
pub fn from_absolute_path<P>(path: P) -> std::io::Result<Self>
where
P: AsRef<Path>,
{
let absolute_path = path.as_ref().absolutize()?;
Ok(Self(absolute_path.into_owned()))
}
pub fn as_path(&self) -> &Path {
&self.0
}
pub fn into_path_buf(self) -> PathBuf {
self.0
}
pub fn to_path_buf(&self) -> PathBuf {
self.0.clone()
}
pub fn display(&self) -> Display<'_> {
self.0.display()
}
}
thread_local! {
static ABSOLUTE_PATH_BASE: RefCell<Option<PathBuf>> = const { RefCell::new(None) };
}
pub struct AbsolutePathBufGuard;
impl AbsolutePathBufGuard {
pub fn new(base_path: &Path) -> Self {
ABSOLUTE_PATH_BASE.with(|cell| {
*cell.borrow_mut() = Some(base_path.to_path_buf());
});
Self
}
}
impl Drop for AbsolutePathBufGuard {
fn drop(&mut self) {
ABSOLUTE_PATH_BASE.with(|cell| {
*cell.borrow_mut() = None;
});
}
}
impl<'de> Deserialize<'de> for AbsolutePathBuf {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let path = PathBuf::deserialize(deserializer)?;
ABSOLUTE_PATH_BASE.with(|cell| match cell.borrow().as_deref() {
Some(base) => {
Ok(Self::resolve_path_against_base(path, base).map_err(SerdeError::custom)?)
}
None if path.is_absolute() => {
Self::from_absolute_path(path).map_err(SerdeError::custom)
}
None => Err(SerdeError::custom(
"AbsolutePathBuf deserialized without a base path",
)),
})
}
}
impl AsRef<Path> for AbsolutePathBuf {
fn as_ref(&self) -> &Path {
self.as_path()
}
}
impl From<AbsolutePathBuf> for PathBuf {
fn from(path: AbsolutePathBuf) -> Self {
path.into_path_buf()
}
}
#[cfg(test)]
mod tests {
use super::*;
use tempfile::tempdir;
#[test]
fn create_with_absolute_path_ignores_base_path() {
let base_dir = tempdir().expect("base dir");
let absolute_dir = tempdir().expect("absolute dir");
let base_path = base_dir.path();
let absolute_path = absolute_dir.path().join("file.txt");
let abs_path_buf =
AbsolutePathBuf::resolve_path_against_base(absolute_path.clone(), base_path)
.expect("failed to create");
assert_eq!(abs_path_buf.as_path(), absolute_path.as_path());
}
#[test]
fn relative_path_is_resolved_against_base_path() {
let temp_dir = tempdir().expect("base dir");
let base_dir = temp_dir.path();
let abs_path_buf = AbsolutePathBuf::resolve_path_against_base("file.txt", base_dir)
.expect("failed to create");
assert_eq!(abs_path_buf.as_path(), base_dir.join("file.txt").as_path());
}
#[test]
fn guard_used_in_deserialization() {
let temp_dir = tempdir().expect("base dir");
let base_dir = temp_dir.path();
let relative_path = "subdir/file.txt";
let abs_path_buf = {
let _guard = AbsolutePathBufGuard::new(base_dir);
serde_json::from_str::<AbsolutePathBuf>(&format!(r#""{relative_path}""#))
.expect("failed to deserialize")
};
assert_eq!(
abs_path_buf.as_path(),
base_dir.join(relative_path).as_path()
);
}
}