mirror of
https://github.com/openai/codex.git
synced 2026-11-09 00:39:32 +00:00
fix: PathBuf -> AbsolutePathBuf in ConfigToml struct (#8205)
We should not have any `PathBuf` fields in `ConfigToml` or any of the transitive structs we include, as we should use `AbsolutePathBuf` instead so that we do not have to keep track of the file from which `ConfigToml` was loaded such that we need it to resolve relative paths later when the values of `ConfigToml` are used. I only found two instances of this: `experimental_instructions_file` and `experimental_compact_prompt_file`. Incidentally, when these were specified as relative paths, they were resolved against `cwd` rather than `config.toml`'s parent, which seems wrong to me. I changed the behavior so they are resolved against the parent folder of the `config.toml` being parsed, which we get "for free" due to the introduction of `AbsolutePathBufGuard ` in https://github.com/openai/codex/pull/7796. While it is not great to change the behavior of a released feature, these fields are prefixed with `experimental_`, which I interpret to mean we have the liberty to change the contract. For reference: - `experimental_instructions_file` was introduced in https://github.com/openai/codex/pull/1803 - `experimental_compact_prompt_file` was introduced in https://github.com/openai/codex/pull/5959
This commit is contained in:
1 parent
3d92b443b0
commit
1e9babe178
3 files changed
+21
-22
No files matched your search
@@ -26,7 +26,6 @@ use crate::project_doc::DEFAULT_PROJECT_DOC_FILENAME;
|
|||||||
use crate::project_doc::LOCAL_PROJECT_DOC_FILENAME;
|
use crate::project_doc::LOCAL_PROJECT_DOC_FILENAME;
|
||||||
use crate::protocol::AskForApproval;
|
use crate::protocol::AskForApproval;
|
||||||
use crate::protocol::SandboxPolicy;
|
use crate::protocol::SandboxPolicy;
|
||||||
use crate::util::resolve_path;
|
|
||||||
use codex_app_server_protocol::Tools;
|
use codex_app_server_protocol::Tools;
|
||||||
use codex_app_server_protocol::UserSavedConfig;
|
use codex_app_server_protocol::UserSavedConfig;
|
||||||
use codex_protocol::config_types::ForcedLoginMethod;
|
use codex_protocol::config_types::ForcedLoginMethod;
|
||||||
@@ -692,8 +691,8 @@ pub struct ConfigToml {
|
|||||||
pub notice: Option<Notice>,
|
pub notice: Option<Notice>,
|
||||||
|
|
||||||
/// Legacy, now use features
|
/// Legacy, now use features
|
||||||
pub experimental_instructions_file: Option<PathBuf>,
|
pub experimental_instructions_file: Option<AbsolutePathBuf>,
|
||||||
pub experimental_compact_prompt_file: Option<PathBuf>,
|
pub experimental_compact_prompt_file: Option<AbsolutePathBuf>,
|
||||||
pub experimental_use_unified_exec_tool: Option<bool>,
|
pub experimental_use_unified_exec_tool: Option<bool>,
|
||||||
pub experimental_use_rmcp_client: Option<bool>,
|
pub experimental_use_rmcp_client: Option<bool>,
|
||||||
pub experimental_use_freeform_apply_patch: Option<bool>,
|
pub experimental_use_freeform_apply_patch: Option<bool>,
|
||||||
@@ -1132,9 +1131,8 @@ impl Config {
|
|||||||
.experimental_instructions_file
|
.experimental_instructions_file
|
||||||
.as_ref()
|
.as_ref()
|
||||||
.or(cfg.experimental_instructions_file.as_ref());
|
.or(cfg.experimental_instructions_file.as_ref());
|
||||||
let file_base_instructions = Self::load_override_from_file(
|
let file_base_instructions = Self::try_read_non_empty_file(
|
||||||
experimental_instructions_path,
|
experimental_instructions_path,
|
||||||
&resolved_cwd,
|
|
||||||
"experimental instructions file",
|
"experimental instructions file",
|
||||||
)?;
|
)?;
|
||||||
let base_instructions = base_instructions.or(file_base_instructions);
|
let base_instructions = base_instructions.or(file_base_instructions);
|
||||||
@@ -1144,9 +1142,8 @@ impl Config {
|
|||||||
.experimental_compact_prompt_file
|
.experimental_compact_prompt_file
|
||||||
.as_ref()
|
.as_ref()
|
||||||
.or(cfg.experimental_compact_prompt_file.as_ref());
|
.or(cfg.experimental_compact_prompt_file.as_ref());
|
||||||
let file_compact_prompt = Self::load_override_from_file(
|
let file_compact_prompt = Self::try_read_non_empty_file(
|
||||||
experimental_compact_prompt_path,
|
experimental_compact_prompt_path,
|
||||||
&resolved_cwd,
|
|
||||||
"experimental compact prompt file",
|
"experimental compact prompt file",
|
||||||
)?;
|
)?;
|
||||||
let compact_prompt = compact_prompt.or(file_compact_prompt);
|
let compact_prompt = compact_prompt.or(file_compact_prompt);
|
||||||
@@ -1278,21 +1275,21 @@ impl Config {
|
|||||||
None
|
None
|
||||||
}
|
}
|
||||||
|
|
||||||
fn load_override_from_file(
|
/// If `path` is `Some`, attempts to read the file at the given path and
|
||||||
path: Option<&PathBuf>,
|
/// returns its contents as a trimmed `String`. If the file is empty, or
|
||||||
cwd: &Path,
|
/// is `Some` but cannot be read, returns an `Err`.
|
||||||
description: &str,
|
fn try_read_non_empty_file(
|
||||||
|
path: Option<&AbsolutePathBuf>,
|
||||||
|
context: &str,
|
||||||
) -> std::io::Result<Option<String>> {
|
) -> std::io::Result<Option<String>> {
|
||||||
let Some(p) = path else {
|
let Some(path) = path else {
|
||||||
return Ok(None);
|
return Ok(None);
|
||||||
};
|
};
|
||||||
|
|
||||||
let full_path = resolve_path(cwd, p);
|
let contents = std::fs::read_to_string(path).map_err(|e| {
|
||||||
|
|
||||||
let contents = std::fs::read_to_string(&full_path).map_err(|e| {
|
|
||||||
std::io::Error::new(
|
std::io::Error::new(
|
||||||
e.kind(),
|
e.kind(),
|
||||||
format!("failed to read {description} {}: {e}", full_path.display()),
|
format!("failed to read {context} {}: {e}", path.display()),
|
||||||
)
|
)
|
||||||
})?;
|
})?;
|
||||||
|
|
||||||
@@ -1300,7 +1297,7 @@ impl Config {
|
|||||||
if s.is_empty() {
|
if s.is_empty() {
|
||||||
Err(std::io::Error::new(
|
Err(std::io::Error::new(
|
||||||
std::io::ErrorKind::InvalidData,
|
std::io::ErrorKind::InvalidData,
|
||||||
format!("{description} is empty: {}", full_path.display()),
|
format!("{context} is empty: {}", path.display()),
|
||||||
))
|
))
|
||||||
} else {
|
} else {
|
||||||
Ok(Some(s))
|
Ok(Some(s))
|
||||||
@@ -2804,7 +2801,9 @@ model = "gpt-5.1-codex"
|
|||||||
std::fs::write(&prompt_path, " summarize differently ")?;
|
std::fs::write(&prompt_path, " summarize differently ")?;
|
||||||
|
|
||||||
let cfg = ConfigToml {
|
let cfg = ConfigToml {
|
||||||
experimental_compact_prompt_file: Some(PathBuf::from("compact_prompt.txt")),
|
experimental_compact_prompt_file: Some(AbsolutePathBuf::from_absolute_path(
|
||||||
|
prompt_path,
|
||||||
|
)?),
|
||||||
..Default::default()
|
..Default::default()
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
|
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||||
use serde::Deserialize;
|
use serde::Deserialize;
|
||||||
use std::path::PathBuf;
|
|
||||||
|
|
||||||
use crate::protocol::AskForApproval;
|
use crate::protocol::AskForApproval;
|
||||||
use codex_protocol::config_types::ReasoningSummary;
|
use codex_protocol::config_types::ReasoningSummary;
|
||||||
@@ -21,8 +21,8 @@ pub struct ConfigProfile {
|
|||||||
pub model_reasoning_summary: Option<ReasoningSummary>,
|
pub model_reasoning_summary: Option<ReasoningSummary>,
|
||||||
pub model_verbosity: Option<Verbosity>,
|
pub model_verbosity: Option<Verbosity>,
|
||||||
pub chatgpt_base_url: Option<String>,
|
pub chatgpt_base_url: Option<String>,
|
||||||
pub experimental_instructions_file: Option<PathBuf>,
|
pub experimental_instructions_file: Option<AbsolutePathBuf>,
|
||||||
pub experimental_compact_prompt_file: Option<PathBuf>,
|
pub experimental_compact_prompt_file: Option<AbsolutePathBuf>,
|
||||||
pub include_apply_patch_tool: Option<bool>,
|
pub include_apply_patch_tool: Option<bool>,
|
||||||
pub experimental_use_unified_exec_tool: Option<bool>,
|
pub experimental_use_unified_exec_tool: Option<bool>,
|
||||||
pub experimental_use_rmcp_client: Option<bool>,
|
pub experimental_use_rmcp_client: Option<bool>,
|
||||||
|
|||||||
@@ -311,7 +311,7 @@ experimental_use_freeform_apply_patch = false
|
|||||||
# model_reasoning_summary = "auto"
|
# model_reasoning_summary = "auto"
|
||||||
# model_verbosity = "medium"
|
# model_verbosity = "medium"
|
||||||
# chatgpt_base_url = "https://chatgpt.com/backend-api/"
|
# chatgpt_base_url = "https://chatgpt.com/backend-api/"
|
||||||
# experimental_compact_prompt_file = "compact_prompt.txt"
|
# experimental_compact_prompt_file = "./compact_prompt.txt"
|
||||||
# include_apply_patch_tool = false
|
# include_apply_patch_tool = false
|
||||||
# experimental_use_freeform_apply_patch = false
|
# experimental_use_freeform_apply_patch = false
|
||||||
# tools_web_search = false
|
# tools_web_search = false
|
||||||
|
|||||||
Reference in new issue
Block a user