Merge branch 'main' into codex/persist-eventmsg-error-and-eventmsg-streamerror

This commit is contained in:
Anton Panasenko
2025-12-12 09:24:23 -08:00
committed by GitHub
13 changed files with 219 additions and 285 deletions

View File

@@ -2079,12 +2079,19 @@ impl CodexMessageProcessor {
}
async fn list_mcp_servers(&self, request_id: RequestId, params: ListMcpServersParams) {
let snapshot = collect_mcp_snapshot(self.config.as_ref()).await;
let config = match self.load_latest_config().await {
Ok(config) => config,
Err(error) => {
self.outgoing.send_error(request_id, error).await;
return;
}
};
let snapshot = collect_mcp_snapshot(&config).await;
let tools_by_server = group_tools_by_server(&snapshot.tools);
let mut server_names: Vec<String> = self
.config
let mut server_names: Vec<String> = config
.mcp_servers
.keys()
.cloned()

View File

@@ -699,7 +699,13 @@ fn derive_new_contents_from_chunks(
}
};
let original_lines: Vec<String> = build_lines_from_contents(&original_contents);
let mut original_lines: Vec<String> = original_contents.split('\n').map(String::from).collect();
// Drop the trailing empty element that results from the final newline so
// that line counts match the behaviour of standard `diff`.
if original_lines.last().is_some_and(String::is_empty) {
original_lines.pop();
}
let replacements = compute_replacements(&original_lines, path, chunks)?;
let new_lines = apply_replacements(original_lines, &replacements);
@@ -707,67 +713,13 @@ fn derive_new_contents_from_chunks(
if !new_lines.last().is_some_and(String::is_empty) {
new_lines.push(String::new());
}
let new_contents = build_contents_from_lines(&original_contents, &new_lines);
let new_contents = new_lines.join("\n");
Ok(AppliedPatch {
original_contents,
new_contents,
})
}
// TODO(dylan-hurd-oai): I think we can migrate to just use `contents.lines()`
// across all platforms.
fn build_lines_from_contents(contents: &str) -> Vec<String> {
if cfg!(windows) {
contents.lines().map(String::from).collect()
} else {
let mut lines: Vec<String> = contents.split('\n').map(String::from).collect();
// Drop the trailing empty element that results from the final newline so
// that line counts match the behaviour of standard `diff`.
if lines.last().is_some_and(String::is_empty) {
lines.pop();
}
lines
}
}
fn build_contents_from_lines(original_contents: &str, lines: &[String]) -> String {
if cfg!(windows) {
// for now, only compute this if we're on Windows.
let uses_crlf = contents_uses_crlf(original_contents);
if uses_crlf {
lines.join("\r\n")
} else {
lines.join("\n")
}
} else {
lines.join("\n")
}
}
/// Detects whether the source file uses Windows CRLF line endings consistently.
/// We only consider a file CRLF-formatted if every newline is part of a
/// CRLF sequence. This avoids rewriting an LF-formatted file that merely
/// contains embedded sequences of "\r\n".
///
/// Returns `true` if the file uses CRLF line endings, `false` otherwise.
fn contents_uses_crlf(contents: &str) -> bool {
let bytes = contents.as_bytes();
let mut n_newlines = 0usize;
let mut n_crlf = 0usize;
for i in 0..bytes.len() {
if bytes[i] == b'\n' {
n_newlines += 1;
if i > 0 && bytes[i - 1] == b'\r' {
n_crlf += 1;
}
}
}
n_newlines > 0 && n_crlf == n_newlines
}
/// Compute a list of replacements needed to transform `original_lines` into the
/// new lines, given the patch `chunks`. Each replacement is returned as
/// `(start_index, old_len, new_lines)`.
@@ -1414,72 +1366,6 @@ PATCH"#,
assert_eq!(contents, "a\nB\nc\nd\nE\nf\ng\n");
}
/// Ensure CRLF line endings are preserved for updated files on Windowsstyle inputs.
#[cfg(windows)]
#[test]
fn test_preserve_crlf_line_endings_on_update() {
let dir = tempdir().unwrap();
let path = dir.path().join("crlf.txt");
// Original file uses CRLF (\r\n) endings.
std::fs::write(&path, b"a\r\nb\r\nc\r\n").unwrap();
// Replace `b` -> `B` and append `d`.
let patch = wrap_patch(&format!(
r#"*** Update File: {}
@@
a
-b
+B
@@
c
+d
*** End of File"#,
path.display()
));
let mut stdout = Vec::new();
let mut stderr = Vec::new();
apply_patch(&patch, &mut stdout, &mut stderr).unwrap();
let out = std::fs::read(&path).unwrap();
// Expect all CRLF endings; count occurrences of CRLF and ensure there are 4 lines.
let content = String::from_utf8_lossy(&out);
assert!(content.contains("\r\n"));
// No bare LF occurrences immediately preceding a non-CR: the text should not contain "a\nb".
assert!(!content.contains("a\nb"));
// Validate exact content sequence with CRLF delimiters.
assert_eq!(content, "a\r\nB\r\nc\r\nd\r\n");
}
/// Ensure CRLF inputs with embedded carriage returns in the content are preserved.
#[cfg(windows)]
#[test]
fn test_preserve_crlf_embedded_carriage_returns_on_append() {
let dir = tempdir().unwrap();
let path = dir.path().join("crlf_cr_content.txt");
// Original file: first line has a literal '\r' in the content before the CRLF terminator.
std::fs::write(&path, b"foo\r\r\nbar\r\n").unwrap();
// Append a new line without modifying existing ones.
let patch = wrap_patch(&format!(
r#"*** Update File: {}
@@
+BAZ
*** End of File"#,
path.display()
));
let mut stdout = Vec::new();
let mut stderr = Vec::new();
apply_patch(&patch, &mut stdout, &mut stderr).unwrap();
let out = std::fs::read(&path).unwrap();
// CRLF endings must be preserved and the extra CR in "foo\r\r" must not be collapsed.
assert_eq!(out.as_slice(), b"foo\r\r\nbar\r\nBAZ\r\n");
}
#[test]
fn test_pure_addition_chunk_followed_by_removal() {
let dir = tempdir().unwrap();
@@ -1665,37 +1551,6 @@ PATCH"#,
assert_eq!(expected, diff);
}
/// For LF-only inputs with a trailing newline ensure that the helper used
/// on Windows-style builds drops the synthetic trailing empty element so
/// replacements behave like standard `diff` line numbering.
#[test]
fn test_derive_new_contents_lf_trailing_newline() {
let dir = tempdir().unwrap();
let path = dir.path().join("lf_trailing_newline.txt");
fs::write(&path, "foo\nbar\n").unwrap();
let patch = wrap_patch(&format!(
r#"*** Update File: {}
@@
foo
-bar
+BAR
"#,
path.display()
));
let patch = parse_patch(&patch).unwrap();
let chunks = match patch.hunks.as_slice() {
[Hunk::UpdateFile { chunks, .. }] => chunks,
_ => panic!("Expected a single UpdateFile hunk"),
};
let AppliedPatch { new_contents, .. } =
derive_new_contents_from_chunks(&path, chunks).unwrap();
assert_eq!(new_contents, "foo\nBAR\n");
}
#[test]
fn test_unified_diff_insert_at_eof() {
// Insert a new line at endoffile.

View File

@@ -47,24 +47,47 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
.file_name()
.and_then(|osstr| osstr.to_str())
{
Some(cmd) if cfg!(target_os = "linux") && matches!(cmd, "numfmt" | "tac") => true,
#[rustfmt::skip]
Some(
"cat" |
"cd" |
"cut" |
"echo" |
"expr" |
"false" |
"grep" |
"head" |
"id" |
"ls" |
"nl" |
"paste" |
"pwd" |
"rev" |
"seq" |
"stat" |
"tail" |
"tr" |
"true" |
"uname" |
"uniq" |
"wc" |
"which") => {
"which" |
"whoami") => {
true
},
Some("base64") => {
const UNSAFE_BASE64_OPTIONS: &[&str] = &["-o", "--output"];
!command.iter().skip(1).any(|arg| {
UNSAFE_BASE64_OPTIONS.contains(&arg.as_str())
|| arg.starts_with("--output=")
|| (arg.starts_with("-o") && arg != "-o")
})
}
Some("find") => {
// Certain options to `find` can delete files, write to files, or
// execute arbitrary commands, so we cannot auto-approve the
@@ -184,6 +207,7 @@ mod tests {
fn known_safe_examples() {
assert!(is_safe_to_call_with_exec(&vec_str(&["ls"])));
assert!(is_safe_to_call_with_exec(&vec_str(&["git", "status"])));
assert!(is_safe_to_call_with_exec(&vec_str(&["base64"])));
assert!(is_safe_to_call_with_exec(&vec_str(&[
"sed", "-n", "1,5p", "file.txt"
])));
@@ -197,6 +221,14 @@ mod tests {
assert!(is_safe_to_call_with_exec(&vec_str(&[
"find", ".", "-name", "file.txt"
])));
if cfg!(target_os = "linux") {
assert!(is_safe_to_call_with_exec(&vec_str(&["numfmt", "1000"])));
assert!(is_safe_to_call_with_exec(&vec_str(&["tac", "Cargo.toml"])));
} else {
assert!(!is_safe_to_call_with_exec(&vec_str(&["numfmt", "1000"])));
assert!(!is_safe_to_call_with_exec(&vec_str(&["tac", "Cargo.toml"])));
}
}
#[test]
@@ -233,6 +265,21 @@ mod tests {
}
}
#[test]
fn base64_output_options_are_unsafe() {
for args in [
vec_str(&["base64", "-o", "out.bin"]),
vec_str(&["base64", "--output", "out.bin"]),
vec_str(&["base64", "--output=out.bin"]),
vec_str(&["base64", "-ob64.txt"]),
] {
assert!(
!is_safe_to_call_with_exec(&args),
"expected {args:?} to be considered unsafe due to output option"
);
}
}
#[test]
fn ripgrep_rules() {
// Safe ripgrep invocations none of the unsafe flags are present.

View File

@@ -1,5 +1,8 @@
use std::path::PathBuf;
#[cfg(windows)]
use codex_utils_absolute_path::AbsolutePathBuf;
use crate::shell::ShellType;
use crate::shell::detect_shell_type;
@@ -40,6 +43,93 @@ pub fn extract_powershell_command(command: &[String]) -> Option<(&str, &str)> {
None
}
/// This function attempts to find a valid PowerShell executable on the system.
/// It first tries to find pwsh.exe, and if that fails, it tries to find
/// powershell.exe.
#[cfg(windows)]
#[allow(dead_code)]
pub(crate) fn try_find_powershellish_executable_blocking() -> Option<AbsolutePathBuf> {
if let Some(pwsh_path) = try_find_pwsh_executable_blocking() {
Some(pwsh_path)
} else {
try_find_powershell_executable_blocking()
}
}
/// This function attempts to find a powershell.exe executable on the system.
#[cfg(windows)]
pub(crate) fn try_find_powershell_executable_blocking() -> Option<AbsolutePathBuf> {
try_find_powershellish_executable_in_path(&["powershell.exe"])
}
/// This function attempts to find a pwsh.exe executable on the system.
/// Note that pwsh.exe and powershell.exe are different executables:
///
/// - pwsh.exe is the cross-platform PowerShell Core (v6+) executable
/// - powershell.exe is the Windows PowerShell (v5.1 and earlier) executable
///
/// Further, while powershell.exe is included by default on Windows systems,
/// pwsh.exe must be installed separately by the user. And even when the user
/// has installed pwsh.exe, it may not be available in the system PATH, in which
/// case we attempt to locate it via other means.
#[cfg(windows)]
pub(crate) fn try_find_pwsh_executable_blocking() -> Option<AbsolutePathBuf> {
if let Some(ps_home) = std::process::Command::new("cmd")
.args(["/C", "pwsh", "-NoProfile", "-Command", "$PSHOME"])
.output()
.ok()
.and_then(|out| {
if !out.status.success() {
return None;
}
let stdout = String::from_utf8_lossy(&out.stdout);
let trimmed = stdout.trim();
(!trimmed.is_empty()).then(|| trimmed.to_string())
})
{
let candidate = AbsolutePathBuf::resolve_path_against_base("pwsh.exe", &ps_home);
if let Ok(candidate_abs_path) = candidate
&& is_powershellish_executable_available(candidate_abs_path.as_path())
{
return Some(candidate_abs_path);
}
}
try_find_powershellish_executable_in_path(&["pwsh.exe"])
}
#[cfg(windows)]
fn try_find_powershellish_executable_in_path(candidates: &[&str]) -> Option<AbsolutePathBuf> {
for candidate in candidates {
let Ok(resolved_path) = which::which(candidate) else {
continue;
};
if !is_powershellish_executable_available(&resolved_path) {
continue;
}
let Ok(abs_path) = AbsolutePathBuf::from_absolute_path(resolved_path) else {
continue;
};
return Some(abs_path);
}
None
}
#[cfg(windows)]
fn is_powershellish_executable_available(powershell_or_pwsh_exe: &std::path::Path) -> bool {
// This test works for both powershell.exe and pwsh.exe.
std::process::Command::new(powershell_or_pwsh_exe)
.args(["-NoLogo", "-NoProfile", "-Command", "Write-Output ok"])
.output()
.map(|output| output.status.success())
.unwrap_or(false)
}
#[cfg(test)]
mod tests {
use super::extract_powershell_command;

View File

@@ -22,8 +22,8 @@ struct SkillFrontmatter {
const SKILLS_FILENAME: &str = "SKILL.md";
const SKILLS_DIR_NAME: &str = "skills";
const REPO_ROOT_CONFIG_DIR_NAME: &str = ".codex";
const MAX_NAME_LEN: usize = 100;
const MAX_DESCRIPTION_LEN: usize = 500;
const MAX_NAME_LEN: usize = 64;
const MAX_DESCRIPTION_LEN: usize = 1024;
#[derive(Debug)]
enum SkillParseError {
@@ -171,7 +171,7 @@ fn validate_field(
if value.is_empty() {
return Err(SkillParseError::MissingField(field_name));
}
if value.len() > max_len {
if value.chars().count() > max_len {
return Err(SkillParseError::InvalidField {
field: field_name,
reason: format!("exceeds maximum length of {max_len} characters"),
@@ -295,12 +295,22 @@ mod tests {
#[test]
fn enforces_length_limits() {
let codex_home = tempfile::tempdir().expect("tempdir");
let long_desc = "a".repeat(MAX_DESCRIPTION_LEN + 1);
write_skill(&codex_home, "too-long", "toolong", &long_desc);
let max_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN);
write_skill(&codex_home, "max-len", "max-len", &max_desc);
let cfg = make_config(&codex_home);
let outcome = load_skills(&cfg);
assert_eq!(outcome.skills.len(), 0);
assert!(
outcome.errors.is_empty(),
"unexpected errors: {:?}",
outcome.errors
);
assert_eq!(outcome.skills.len(), 1);
let too_long_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN + 1);
write_skill(&codex_home, "too-long", "too-long", &too_long_desc);
let outcome = load_skills(&cfg);
assert_eq!(outcome.skills.len(), 1);
assert_eq!(outcome.errors.len(), 1);
assert!(
outcome.errors[0].message.contains("invalid description"),

View File

@@ -1250,94 +1250,3 @@ async fn apply_patch_change_context_disambiguates_target(
assert_eq!(contents, "fn a\nx=10\ny=2\nfn b\nx=11\ny=20\n");
Ok(())
}
/// Ensure that applying a patch can update a CRLF file with unicode characters.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ApplyPatchModelOutput::Freeform)]
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_updates_unicode_characters(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
skip_if_no_network!(Ok(()));
let harness = apply_patch_harness().await?;
let target = harness.path("unicode.txt");
fs::write(&target, "first ⚠️\nsecond ❌\nthird 🔥\n")?;
let patch = format!(
r#"*** Begin Patch
*** Update File: {}
@@
first ⚠️
-second ❌
+SECOND ✅
@@
third 🔥
+FOURTH
*** End of File
*** End Patch"#,
target.display()
);
let call_id = "apply-unicode-update";
mount_apply_patch(&harness, call_id, patch.as_str(), "ok", model_output).await;
harness
.submit("update unicode characters via apply_patch CLI")
.await?;
let file_contents = fs::read(&target)?;
let content = String::from_utf8_lossy(&file_contents);
assert_eq!(content, "first ⚠️\nSECOND ✅\nthird 🔥\nFOURTH\n");
Ok(())
}
/// Ensure that applying a patch via the CLI preserves CRLF line endings for
/// Windows-style inputs even when updating the file contents.
#[cfg(windows)]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ApplyPatchModelOutput::Freeform)]
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_updates_crlf_file_preserves_line_endings(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
skip_if_no_network!(Ok(()));
let harness = apply_patch_harness().await?;
let target = harness.path("crlf.txt");
fs::write(&target, b"first\r\nsecond\r\nthird\r\n")?;
let patch = format!(
r#"*** Begin Patch
*** Update File: {}
@@
first
-second
+SECOND
@@
third
+FOURTH
*** End of File
*** End Patch"#,
target.display()
);
let call_id = "apply-crlf-update";
mount_apply_patch(&harness, call_id, patch.as_str(), "ok", model_output).await;
harness
.submit("update crlf file via apply_patch CLI")
.await?;
let file_contents = fs::read(&target)?;
let content = String::from_utf8_lossy(&file_contents);
assert!(content.contains("\r\n"));
assert_eq!(content, "first\r\nSECOND\r\nthird\r\nFOURTH\r\n");
Ok(())
}

View File

@@ -2395,7 +2395,8 @@ impl ChatWidget {
format!("{effort_label} reasoning effort can quickly consume Plus plan rate limits.")
});
let warn_for_model = preset.model.starts_with("gpt-5.1-codex")
|| preset.model.starts_with("gpt-5.1-codex-max");
|| preset.model.starts_with("gpt-5.1-codex-max")
|| preset.model.starts_with("gpt-5.2");
struct EffortChoice {
stored: Option<ReasoningEffortConfig>,

View File

@@ -277,33 +277,42 @@ impl Tui {
let event_stream = async_stream::stream! {
loop {
select! {
Some(Ok(event)) = crossterm_events.next() => {
match event {
Event::Key(key_event) => {
#[cfg(unix)]
if SUSPEND_KEY.is_press(key_event) {
let _ = suspend_context.suspend(&alt_screen_active);
// We continue here after resume.
yield TuiEvent::Draw;
continue;
event_result = crossterm_events.next() => {
match event_result {
Some(Ok(event)) => {
match event {
Event::Key(key_event) => {
#[cfg(unix)]
if SUSPEND_KEY.is_press(key_event) {
let _ = suspend_context.suspend(&alt_screen_active);
// We continue here after resume.
yield TuiEvent::Draw;
continue;
}
yield TuiEvent::Key(key_event);
}
Event::Resize(_, _) => {
yield TuiEvent::Draw;
}
Event::Paste(pasted) => {
yield TuiEvent::Paste(pasted);
}
Event::FocusGained => {
terminal_focused.store(true, Ordering::Relaxed);
crate::terminal_palette::requery_default_colors();
yield TuiEvent::Draw;
}
Event::FocusLost => {
terminal_focused.store(false, Ordering::Relaxed);
}
_ => {}
}
yield TuiEvent::Key(key_event);
}
Event::Resize(_, _) => {
yield TuiEvent::Draw;
Some(Err(_)) | None => {
// Exit the loop in case of broken pipe as we will never
// recover from it
break;
}
Event::Paste(pasted) => {
yield TuiEvent::Paste(pasted);
}
Event::FocusGained => {
terminal_focused.store(true, Ordering::Relaxed);
crate::terminal_palette::requery_default_colors();
yield TuiEvent::Draw;
}
Event::FocusLost => {
terminal_focused.store(false, Ordering::Relaxed);
}
_ => {}
}
}
result = draw_rx.recv() => {
@@ -316,7 +325,9 @@ impl Tui {
yield TuiEvent::Draw;
}
Err(tokio::sync::broadcast::error::RecvError::Closed) => {
// Sender dropped; stop emitting draws from this source.
// Sender dropped. This stream likely outlived its owning `Tui`;
// exit to avoid spinning on a permanently-closed receiver.
break;
}
}
}

View File

@@ -2379,7 +2379,8 @@ impl ChatWidget {
format!("{effort_label} reasoning effort can quickly consume Plus plan rate limits.")
});
let warn_for_model = preset.model.starts_with("gpt-5.1-codex")
|| preset.model.starts_with("gpt-5.1-codex-max");
|| preset.model.starts_with("gpt-5.1-codex-max")
|| preset.model.starts_with("gpt-5.2");
struct EffortChoice {
stored: Option<ReasoningEffortConfig>,

View File

@@ -54,6 +54,7 @@ impl ExecCommandSession {
pub fn new(
writer_tx: mpsc::Sender<Vec<u8>>,
output_tx: broadcast::Sender<Vec<u8>>,
initial_output_rx: broadcast::Receiver<Vec<u8>>,
killer: Box<dyn portable_pty::ChildKiller + Send + Sync>,
reader_handle: JoinHandle<()>,
writer_handle: JoinHandle<()>,
@@ -62,7 +63,6 @@ impl ExecCommandSession {
exit_code: Arc<StdMutex<Option<i32>>>,
pair: PtyPairWrapper,
) -> (Self, broadcast::Receiver<Vec<u8>>) {
let initial_output_rx = output_tx.subscribe();
(
Self {
writer_tx,
@@ -177,6 +177,8 @@ pub async fn spawn_pty_process(
let (writer_tx, mut writer_rx) = mpsc::channel::<Vec<u8>>(128);
let (output_tx, _) = broadcast::channel::<Vec<u8>>(256);
// Subscribe before starting the reader thread.
let initial_output_rx = output_tx.subscribe();
let mut reader = pair.master.try_clone_reader()?;
let output_tx_clone = output_tx.clone();
@@ -242,6 +244,7 @@ pub async fn spawn_pty_process(
let (session, output_rx) = ExecCommandSession::new(
writer_tx,
output_tx,
initial_output_rx,
killer,
reader_handle,
writer_handle,

View File

@@ -195,7 +195,7 @@ If the selected model is known to support reasoning (for example: `o3`, `o4-mini
- `"low"`
- `"medium"` (default)
- `"high"`
- `"xhigh"` (available only on `gpt-5.1-codex-max`)
- `"xhigh"` (available on `gpt-5.1-codex-max` and `gpt-5.2`)
Note: to minimize reasoning, choose `"minimal"`.

View File

@@ -37,7 +37,7 @@ model_provider = "openai"
# Reasoning & Verbosity (Responses API capable models)
################################################################################
# Reasoning effort: minimal | low | medium | high | xhigh (default: medium; xhigh only on gpt-5.1-codex-max)
# Reasoning effort: minimal | low | medium | high | xhigh (default: medium; xhigh on gpt-5.1-codex-max and gpt-5.2)
model_reasoning_effort = "medium"
# Reasoning summary: auto | concise | detailed | none (default: auto)

View File

@@ -8,7 +8,7 @@ In 2021, OpenAI released Codex, an AI system designed to generate code from natu
### Which models are supported?
We recommend using Codex with GPT-5.1 Codex Max, our best coding model. The default reasoning level is medium, and you can upgrade to high or xhigh (Codex Max only) for complex tasks with the `/model` command.
We recommend using Codex with GPT-5.1 Codex Max, our best coding model. The default reasoning level is medium, and you can upgrade to high or xhigh (where supported, e.g. `gpt-5.1-codex-max` and `gpt-5.2`) for complex tasks with the `/model` command.
You can also use older models by using API-based auth and launching codex with the `--model` flag.