mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
Fixed symlink support for config.toml (#9445)
We already support reading from `config.toml` through a symlink, but the code was not properly handling updates to a symlinked config file. This PR generalizes safe symlink-chain resolution and atomic writes into path_utils, updating all config write paths to use the shared logic (including set_default_oss_provider, which previously didn't use the common path), and adds tests for symlink chains and cycles. This resolves #6646. Notes: * Symlink cycles or resolution failures replace the top-level symlink with a real file. * Shared config write path now handles symlinks consistently across edits, defaults, and empty-user-layer creation. This PR was inspired by https://github.com/openai/codex/pull/9437, which was contributed by @ryoppippi
This commit is contained in:
@@ -1,13 +1,14 @@
|
||||
use crate::config::CONFIG_TOML_FILE;
|
||||
use crate::config::types::McpServerConfig;
|
||||
use crate::config::types::Notice;
|
||||
use crate::path_utils::resolve_symlink_write_paths;
|
||||
use crate::path_utils::write_atomically;
|
||||
use anyhow::Context;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use std::collections::BTreeMap;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::NamedTempFile;
|
||||
use tokio::task;
|
||||
use toml_edit::ArrayOfTables;
|
||||
use toml_edit::DocumentMut;
|
||||
@@ -625,10 +626,14 @@ pub fn apply_blocking(
|
||||
}
|
||||
|
||||
let config_path = codex_home.join(CONFIG_TOML_FILE);
|
||||
let serialized = match std::fs::read_to_string(&config_path) {
|
||||
Ok(contents) => contents,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => String::new(),
|
||||
Err(err) => return Err(err.into()),
|
||||
let write_paths = resolve_symlink_write_paths(&config_path)?;
|
||||
let serialized = match write_paths.read_path {
|
||||
Some(path) => match std::fs::read_to_string(&path) {
|
||||
Ok(contents) => contents,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => String::new(),
|
||||
Err(err) => return Err(err.into()),
|
||||
},
|
||||
None => String::new(),
|
||||
};
|
||||
|
||||
let doc = if serialized.is_empty() {
|
||||
@@ -654,22 +659,13 @@ pub fn apply_blocking(
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
std::fs::create_dir_all(codex_home).with_context(|| {
|
||||
write_atomically(&write_paths.write_path, &document.doc.to_string()).with_context(|| {
|
||||
format!(
|
||||
"failed to create Codex home directory at {}",
|
||||
codex_home.display()
|
||||
"failed to persist config.toml at {}",
|
||||
write_paths.write_path.display()
|
||||
)
|
||||
})?;
|
||||
|
||||
let tmp = NamedTempFile::new_in(codex_home)?;
|
||||
std::fs::write(tmp.path(), document.doc.to_string()).with_context(|| {
|
||||
format!(
|
||||
"failed to write temporary config file at {}",
|
||||
tmp.path().display()
|
||||
)
|
||||
})?;
|
||||
tmp.persist(config_path)?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -813,6 +809,8 @@ mod tests {
|
||||
use crate::config::types::McpServerTransportConfig;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use pretty_assertions::assert_eq;
|
||||
#[cfg(unix)]
|
||||
use std::os::unix::fs::symlink;
|
||||
use tempfile::tempdir;
|
||||
use toml::Value as TomlValue;
|
||||
|
||||
@@ -952,6 +950,71 @@ profiles = { fast = { model = "gpt-4o", sandbox_mode = "strict" } }
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn blocking_set_model_writes_through_symlink_chain() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
let codex_home = tmp.path();
|
||||
let target_dir = tempdir().expect("target dir");
|
||||
let target_path = target_dir.path().join(CONFIG_TOML_FILE);
|
||||
let link_path = codex_home.join("config-link.toml");
|
||||
let config_path = codex_home.join(CONFIG_TOML_FILE);
|
||||
|
||||
symlink(&target_path, &link_path).expect("symlink link");
|
||||
symlink("config-link.toml", &config_path).expect("symlink config");
|
||||
|
||||
apply_blocking(
|
||||
codex_home,
|
||||
None,
|
||||
&[ConfigEdit::SetModel {
|
||||
model: Some("gpt-5.1-codex".to_string()),
|
||||
effort: Some(ReasoningEffort::High),
|
||||
}],
|
||||
)
|
||||
.expect("persist");
|
||||
|
||||
let meta = std::fs::symlink_metadata(&config_path).expect("config metadata");
|
||||
assert!(meta.file_type().is_symlink());
|
||||
|
||||
let contents = std::fs::read_to_string(&target_path).expect("read target");
|
||||
let expected = r#"model = "gpt-5.1-codex"
|
||||
model_reasoning_effort = "high"
|
||||
"#;
|
||||
assert_eq!(contents, expected);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn blocking_set_model_replaces_symlink_on_cycle() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
let codex_home = tmp.path();
|
||||
let link_a = codex_home.join("a.toml");
|
||||
let link_b = codex_home.join("b.toml");
|
||||
let config_path = codex_home.join(CONFIG_TOML_FILE);
|
||||
|
||||
symlink("b.toml", &link_a).expect("symlink a");
|
||||
symlink("a.toml", &link_b).expect("symlink b");
|
||||
symlink("a.toml", &config_path).expect("symlink config");
|
||||
|
||||
apply_blocking(
|
||||
codex_home,
|
||||
None,
|
||||
&[ConfigEdit::SetModel {
|
||||
model: Some("gpt-5.1-codex".to_string()),
|
||||
effort: None,
|
||||
}],
|
||||
)
|
||||
.expect("persist");
|
||||
|
||||
let meta = std::fs::symlink_metadata(&config_path).expect("config metadata");
|
||||
assert!(!meta.file_type().is_symlink());
|
||||
|
||||
let contents = std::fs::read_to_string(&config_path).expect("read config");
|
||||
let expected = r#"model = "gpt-5.1-codex"
|
||||
"#;
|
||||
assert_eq!(contents, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn batch_write_table_upsert_preserves_inline_comments() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
use crate::auth::AuthCredentialsStoreMode;
|
||||
use crate::config::edit::ConfigEdit;
|
||||
use crate::config::edit::ConfigEditsBuilder;
|
||||
use crate::config::types::DEFAULT_OTEL_ENVIRONMENT;
|
||||
use crate::config::types::History;
|
||||
use crate::config::types::McpServerConfig;
|
||||
@@ -751,30 +753,17 @@ pub fn set_default_oss_provider(codex_home: &Path, provider: &str) -> std::io::R
|
||||
));
|
||||
}
|
||||
}
|
||||
let config_path = codex_home.join(CONFIG_TOML_FILE);
|
||||
|
||||
// Read existing config or create empty string if file doesn't exist
|
||||
let content = match std::fs::read_to_string(&config_path) {
|
||||
Ok(content) => content,
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => String::new(),
|
||||
Err(e) => return Err(e),
|
||||
};
|
||||
|
||||
// Parse as DocumentMut for editing while preserving structure
|
||||
let mut doc = content.parse::<DocumentMut>().map_err(|e| {
|
||||
std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidData,
|
||||
format!("failed to parse config.toml: {e}"),
|
||||
)
|
||||
})?;
|
||||
|
||||
// Set the default_oss_provider at root level
|
||||
use toml_edit::value;
|
||||
doc["oss_provider"] = value(provider);
|
||||
|
||||
// Write the modified document back
|
||||
std::fs::write(&config_path, doc.to_string())?;
|
||||
Ok(())
|
||||
let edits = [ConfigEdit::SetPath {
|
||||
segments: vec!["oss_provider".to_string()],
|
||||
value: value(provider),
|
||||
}];
|
||||
|
||||
ConfigEditsBuilder::new(codex_home)
|
||||
.with_edits(edits)
|
||||
.apply_blocking()
|
||||
.map_err(|err| std::io::Error::other(format!("failed to persist config.toml: {err}")))
|
||||
}
|
||||
|
||||
/// Base config deserialized from ~/.codex/config.toml.
|
||||
|
||||
@@ -9,6 +9,9 @@ use crate::config_loader::LoaderOverrides;
|
||||
use crate::config_loader::load_config_layers_state;
|
||||
use crate::config_loader::merge_toml_values;
|
||||
use crate::path_utils;
|
||||
use crate::path_utils::SymlinkWritePaths;
|
||||
use crate::path_utils::resolve_symlink_write_paths;
|
||||
use crate::path_utils::write_atomically;
|
||||
use codex_app_server_protocol::Config as ApiConfig;
|
||||
use codex_app_server_protocol::ConfigBatchWriteParams;
|
||||
use codex_app_server_protocol::ConfigLayerMetadata;
|
||||
@@ -27,6 +30,7 @@ use std::borrow::Cow;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use thiserror::Error;
|
||||
use tokio::task;
|
||||
use toml::Value as TomlValue;
|
||||
use toml_edit::Item as TomlItem;
|
||||
|
||||
@@ -362,19 +366,30 @@ impl ConfigService {
|
||||
async fn create_empty_user_layer(
|
||||
config_toml: &AbsolutePathBuf,
|
||||
) -> Result<ConfigLayerEntry, ConfigServiceError> {
|
||||
let toml_value = match tokio::fs::read_to_string(config_toml).await {
|
||||
Ok(contents) => toml::from_str(&contents).map_err(|e| {
|
||||
ConfigServiceError::toml("failed to parse existing user config.toml", e)
|
||||
})?,
|
||||
Err(e) => {
|
||||
if e.kind() == std::io::ErrorKind::NotFound {
|
||||
tokio::fs::write(config_toml, "").await.map_err(|e| {
|
||||
ConfigServiceError::io("failed to create empty user config.toml", e)
|
||||
})?;
|
||||
let SymlinkWritePaths {
|
||||
read_path,
|
||||
write_path,
|
||||
} = resolve_symlink_write_paths(config_toml.as_path())
|
||||
.map_err(|err| ConfigServiceError::io("failed to resolve user config path", err))?;
|
||||
let toml_value = match read_path {
|
||||
Some(path) => match tokio::fs::read_to_string(&path).await {
|
||||
Ok(contents) => toml::from_str(&contents).map_err(|e| {
|
||||
ConfigServiceError::toml("failed to parse existing user config.toml", e)
|
||||
})?,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
|
||||
write_empty_user_config(write_path.clone()).await?;
|
||||
TomlValue::Table(toml::map::Map::new())
|
||||
} else {
|
||||
return Err(ConfigServiceError::io("failed to read user config.toml", e));
|
||||
}
|
||||
Err(err) => {
|
||||
return Err(ConfigServiceError::io(
|
||||
"failed to read user config.toml",
|
||||
err,
|
||||
));
|
||||
}
|
||||
},
|
||||
None => {
|
||||
write_empty_user_config(write_path).await?;
|
||||
TomlValue::Table(toml::map::Map::new())
|
||||
}
|
||||
};
|
||||
Ok(ConfigLayerEntry::new(
|
||||
@@ -385,6 +400,13 @@ async fn create_empty_user_layer(
|
||||
))
|
||||
}
|
||||
|
||||
async fn write_empty_user_config(write_path: PathBuf) -> Result<(), ConfigServiceError> {
|
||||
task::spawn_blocking(move || write_atomically(&write_path, ""))
|
||||
.await
|
||||
.map_err(|err| ConfigServiceError::anyhow("config persistence task panicked", err.into()))?
|
||||
.map_err(|err| ConfigServiceError::io("failed to create empty user config.toml", err))
|
||||
}
|
||||
|
||||
fn parse_value(value: JsonValue) -> Result<Option<TomlValue>, String> {
|
||||
if value.is_null() {
|
||||
return Ok(None);
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::collections::HashSet;
|
||||
use std::io;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
use crate::env;
|
||||
|
||||
@@ -8,6 +12,106 @@ pub fn normalize_for_path_comparison(path: impl AsRef<Path>) -> std::io::Result<
|
||||
Ok(normalize_for_wsl(canonical))
|
||||
}
|
||||
|
||||
pub struct SymlinkWritePaths {
|
||||
pub read_path: Option<PathBuf>,
|
||||
pub write_path: PathBuf,
|
||||
}
|
||||
|
||||
/// Resolve the final filesystem target for `path` while retaining a safe write path.
|
||||
///
|
||||
/// This follows symlink chains (including relative symlink targets) until it reaches a
|
||||
/// non-symlink path. If the chain cycles or any metadata/link resolution fails, it
|
||||
/// returns `read_path: None` and uses the original absolute path as `write_path`.
|
||||
/// There is no fixed max-resolution count; cycles are detected via a visited set.
|
||||
pub fn resolve_symlink_write_paths(path: &Path) -> io::Result<SymlinkWritePaths> {
|
||||
let root = AbsolutePathBuf::from_absolute_path(path)
|
||||
.map(AbsolutePathBuf::into_path_buf)
|
||||
.unwrap_or_else(|_| path.to_path_buf());
|
||||
let mut current = root.clone();
|
||||
let mut visited = HashSet::new();
|
||||
|
||||
// Follow symlink chains while guarding against cycles.
|
||||
loop {
|
||||
let meta = match std::fs::symlink_metadata(¤t) {
|
||||
Ok(meta) => meta,
|
||||
Err(err) if err.kind() == io::ErrorKind::NotFound => {
|
||||
return Ok(SymlinkWritePaths {
|
||||
read_path: Some(current.clone()),
|
||||
write_path: current,
|
||||
});
|
||||
}
|
||||
Err(_) => {
|
||||
return Ok(SymlinkWritePaths {
|
||||
read_path: None,
|
||||
write_path: root,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
if !meta.file_type().is_symlink() {
|
||||
return Ok(SymlinkWritePaths {
|
||||
read_path: Some(current.clone()),
|
||||
write_path: current,
|
||||
});
|
||||
}
|
||||
|
||||
// If we've already seen this path, the chain cycles.
|
||||
if !visited.insert(current.clone()) {
|
||||
return Ok(SymlinkWritePaths {
|
||||
read_path: None,
|
||||
write_path: root,
|
||||
});
|
||||
}
|
||||
|
||||
let target = match std::fs::read_link(¤t) {
|
||||
Ok(target) => target,
|
||||
Err(_) => {
|
||||
return Ok(SymlinkWritePaths {
|
||||
read_path: None,
|
||||
write_path: root,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
let next = if target.is_absolute() {
|
||||
AbsolutePathBuf::from_absolute_path(&target)
|
||||
} else if let Some(parent) = current.parent() {
|
||||
AbsolutePathBuf::resolve_path_against_base(&target, parent)
|
||||
} else {
|
||||
return Ok(SymlinkWritePaths {
|
||||
read_path: None,
|
||||
write_path: root,
|
||||
});
|
||||
};
|
||||
|
||||
let next = match next {
|
||||
Ok(path) => path.into_path_buf(),
|
||||
Err(_) => {
|
||||
return Ok(SymlinkWritePaths {
|
||||
read_path: None,
|
||||
write_path: root,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
current = next;
|
||||
}
|
||||
}
|
||||
|
||||
pub fn write_atomically(write_path: &Path, contents: &str) -> io::Result<()> {
|
||||
let parent = write_path.parent().ok_or_else(|| {
|
||||
io::Error::new(
|
||||
io::ErrorKind::InvalidInput,
|
||||
format!("path {} has no parent directory", write_path.display()),
|
||||
)
|
||||
})?;
|
||||
std::fs::create_dir_all(parent)?;
|
||||
let tmp = NamedTempFile::new_in(parent)?;
|
||||
std::fs::write(tmp.path(), contents)?;
|
||||
tmp.persist(write_path)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn normalize_for_wsl(path: PathBuf) -> PathBuf {
|
||||
normalize_for_wsl_with_flag(path, env::is_wsl())
|
||||
}
|
||||
@@ -84,6 +188,29 @@ fn lower_ascii_path(path: PathBuf) -> PathBuf {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
#[cfg(unix)]
|
||||
mod symlinks {
|
||||
use super::super::resolve_symlink_write_paths;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::os::unix::fs::symlink;
|
||||
|
||||
#[test]
|
||||
fn symlink_cycles_fall_back_to_root_write_path() -> std::io::Result<()> {
|
||||
let dir = tempfile::tempdir()?;
|
||||
let a = dir.path().join("a");
|
||||
let b = dir.path().join("b");
|
||||
|
||||
symlink(&b, &a)?;
|
||||
symlink(&a, &b)?;
|
||||
|
||||
let resolved = resolve_symlink_write_paths(&a)?;
|
||||
|
||||
assert_eq!(resolved.read_path, None);
|
||||
assert_eq!(resolved.write_path, a);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
mod wsl {
|
||||
use super::super::normalize_for_wsl_with_flag;
|
||||
|
||||
Reference in New Issue
Block a user