From 9f71dcbf577c2f87048c97c9b533817313b93955 Mon Sep 17 00:00:00 2001 From: Dylan Date: Thu, 21 Aug 2025 19:58:07 -0700 Subject: [PATCH 1/8] [shell_tool] Small updates to ensure shell consistency (#2571) ## Summary Small update to hopefully improve some shell edge cases, and make the function clearer to the model what is going on. Keeping `timeout` as an alias means that calls with the previous name will still work. ## Test Plan - [x] Tested locally, model still works --- codex-rs/core/src/models.rs | 6 ++---- codex-rs/core/src/openai_tools.rs | 14 +++++++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/codex-rs/core/src/models.rs b/codex-rs/core/src/models.rs index f6323e2724..0c509ee282 100644 --- a/codex-rs/core/src/models.rs +++ b/codex-rs/core/src/models.rs @@ -197,10 +197,8 @@ pub struct ShellToolCallParams { pub command: Vec, pub workdir: Option, - /// This is the maximum time in seconds that the command is allowed to run. - #[serde(rename = "timeout")] - // The wire format uses `timeout`, which has ambiguous units, so we use - // `timeout_ms` as the field name so it is clear in code. + /// This is the maximum time in milliseconds that the command is allowed to run. + #[serde(alias = "timeout")] pub timeout_ms: Option, #[serde(skip_serializing_if = "Option::is_none")] pub with_escalated_permissions: Option, diff --git a/codex-rs/core/src/openai_tools.rs b/codex-rs/core/src/openai_tools.rs index dd5eb12516..f02c784777 100644 --- a/codex-rs/core/src/openai_tools.rs +++ b/codex-rs/core/src/openai_tools.rs @@ -115,16 +115,20 @@ fn create_shell_tool() -> OpenAiTool { "command".to_string(), JsonSchema::Array { items: Box::new(JsonSchema::String { description: None }), - description: None, + description: Some("The command to execute".to_string()), }, ); properties.insert( "workdir".to_string(), - JsonSchema::String { description: None }, + JsonSchema::String { + description: Some("The working directory to execute the command in".to_string()), + }, ); properties.insert( - "timeout".to_string(), - JsonSchema::Number { description: None }, + "timeout_ms".to_string(), + JsonSchema::Number { + description: Some("The timeout for the command in milliseconds".to_string()), + }, ); OpenAiTool::Function(ResponsesApiTool { @@ -155,7 +159,7 @@ fn create_shell_tool_for_sandbox(sandbox_policy: &SandboxPolicy) -> OpenAiTool { }, ); properties.insert( - "timeout".to_string(), + "timeout_ms".to_string(), JsonSchema::Number { description: Some("The timeout for the command in milliseconds".to_string()), }, From e4c275d615e6ba9dd0805fb2f4c73099201011a0 Mon Sep 17 00:00:00 2001 From: Dylan Date: Thu, 21 Aug 2025 20:07:41 -0700 Subject: [PATCH 2/8] [apply-patch] Clean up apply-patch tool definitions (#2539) ## Summary We've experienced a bit of drift in system prompting for `apply_patch`: - As pointed out in #2030 , our prettier formatting started altering prompt.md in a few ways - We introduced a separate markdown file for apply_patch instructions in #993, but currently duplicate them in the prompt.md file - We added a first-class apply_patch tool in #2303, which has yet another definition This PR starts to consolidate our logic in a few ways: - We now only use `apply_patch_tool_instructions.md](https://github.com/openai/codex/compare/dh--apply-patch-tool-definition?expand=1#diff-d4fffee5f85cb1975d3f66143a379e6c329de40c83ed5bf03ffd3829df985bea) for system instructions - We no longer include apply_patch system instructions if the tool is specified I'm leaving the definition in openai_tools.rs as duplicated text for now because we're going to be iterated on the first-class tool soon. ## Testing - [x] Added integration tests to verify prompt stability - [x] Tested locally with several different models (gpt-5, gpt-oss, o4-mini) --- .prettierignore | 4 + .../apply_patch_tool_instructions.md | 69 +++++-- codex-rs/core/prompt.md | 61 ------ codex-rs/core/src/client_common.rs | 12 +- codex-rs/core/src/openai_tools.rs | 70 ++++--- codex-rs/core/tests/prompt_caching.rs | 173 ++++++++++++++++++ 6 files changed, 282 insertions(+), 107 deletions(-) diff --git a/.prettierignore b/.prettierignore index 1800016bda..f5b50f6ba2 100644 --- a/.prettierignore +++ b/.prettierignore @@ -1,3 +1,7 @@ /codex-cli/dist /codex-cli/node_modules pnpm-lock.yaml + +prompt.md +*_prompt.md +*_instructions.md diff --git a/codex-rs/apply-patch/apply_patch_tool_instructions.md b/codex-rs/apply-patch/apply_patch_tool_instructions.md index 3c51d9cfbf..64114daa8f 100644 --- a/codex-rs/apply-patch/apply_patch_tool_instructions.md +++ b/codex-rs/apply-patch/apply_patch_tool_instructions.md @@ -1,17 +1,23 @@ -To edit files, ALWAYS use the `shell` tool with `apply_patch` CLI. `apply_patch` effectively allows you to execute a diff/patch against a file, but the format of the diff specification is unique to this task, so pay careful attention to these instructions. To use the `apply_patch` CLI, you should call the shell tool with the following structure: +## `apply_patch` -```bash -{"cmd": ["apply_patch", "<<'EOF'\\n*** Begin Patch\\n[YOUR_PATCH]\\n*** End Patch\\nEOF\\n"], "workdir": "..."} -``` +Use the `apply_patch` shell command to edit files. +Your patch language is a stripped‑down, file‑oriented diff format designed to be easy to parse and safe to apply. You can think of it as a high‑level envelope: -Where [YOUR_PATCH] is the actual content of your patch, specified in the following V4A diff format. +*** Begin Patch +[ one or more file sections ] +*** End Patch -*** [ACTION] File: [path/to/file] -> ACTION can be one of Add, Update, or Delete. -For each snippet of code that needs to be changed, repeat the following: -[context_before] -> See below for further instructions on context. -- [old_code] -> Precede the old code with a minus sign. -+ [new_code] -> Precede the new, replacement code with a plus sign. -[context_after] -> See below for further instructions on context. +Within that envelope, you get a sequence of file operations. +You MUST include a header to specify the action you are taking. +Each operation starts with one of three headers: + +*** Add File: - create a new file. Every following line is a + line (the initial contents). +*** Delete File: - remove an existing file. Nothing follows. +*** Update File: - patch an existing file in place (optionally with a rename). + +May be immediately followed by *** Move to: if you want to rename the file. +Then one or more “hunks”, each introduced by @@ (optionally followed by a hunk header). +Within a hunk each line starts with: For instructions on [context_before] and [context_after]: - By default, show 3 lines of code immediately above and 3 lines immediately below each change. If a change is within 3 lines of a previous change, do NOT duplicate the first change’s [context_after] lines in the second change’s [context_before] lines. @@ -25,16 +31,45 @@ For instructions on [context_before] and [context_after]: - If a code block is repeated so many times in a class or function such that even a single `@@` statement and 3 lines of context cannot uniquely identify the snippet of code, you can use multiple `@@` statements to jump to the right context. For instance: @@ class BaseClass -@@ def method(): +@@ def method(): [3 lines of pre-context] - [old_code] + [new_code] [3 lines of post-context] -Note, then, that we do not use line numbers in this diff format, as the context is enough to uniquely identify code. An example of a message that you might pass as "input" to this function, in order to apply a patch, is shown below. +The full grammar definition is below: +Patch := Begin { FileOp } End +Begin := "*** Begin Patch" NEWLINE +End := "*** End Patch" NEWLINE +FileOp := AddFile | DeleteFile | UpdateFile +AddFile := "*** Add File: " path NEWLINE { "+" line NEWLINE } +DeleteFile := "*** Delete File: " path NEWLINE +UpdateFile := "*** Update File: " path NEWLINE [ MoveTo ] { Hunk } +MoveTo := "*** Move to: " newPath NEWLINE +Hunk := "@@" [ header ] NEWLINE { HunkLine } [ "*** End of File" NEWLINE ] +HunkLine := (" " | "-" | "+") text NEWLINE + +A full patch can combine several operations: + +*** Begin Patch +*** Add File: hello.txt ++Hello world +*** Update File: src/app.py +*** Move to: src/main.py +@@ def greet(): +-print("Hi") ++print("Hello, world!") +*** Delete File: obsolete.txt +*** End Patch + +It is important to remember: + +- You must include a header with your intended action (Add/Delete/Update) +- You must prefix new lines with `+` even when creating a new file +- File references can only be relative, NEVER ABSOLUTE. + +You can invoke apply_patch like: -```bash -{"cmd": ["apply_patch", "<<'EOF'\\n*** Begin Patch\\n*** Update File: pygorithm/searching/binary_search.py\\n@@ class BaseClass\\n@@ def search():\\n- pass\\n+ raise NotImplementedError()\\n@@ class Subclass\\n@@ def search():\\n- pass\\n+ raise NotImplementedError()\\n*** End Patch\\nEOF\\n"], "workdir": "..."} ``` - -File references can only be relative, NEVER ABSOLUTE. After the apply_patch command is run, it will always say "Done!", regardless of whether the patch was successfully applied or not. However, you can determine if there are issue and errors by looking at any warnings or logging lines printed BEFORE the "Done!" is output. +shell {"command":["apply_patch","*** Begin Patch\n*** Add File: hello.txt\n+Hello, world!\n*** End Patch\n"]} +``` diff --git a/codex-rs/core/prompt.md b/codex-rs/core/prompt.md index ff5c2acde6..1860dccd99 100644 --- a/codex-rs/core/prompt.md +++ b/codex-rs/core/prompt.md @@ -270,67 +270,6 @@ When using the shell, you must adhere to the following guidelines: - When searching for text or files, prefer using `rg` or `rg --files` respectively because `rg` is much faster than alternatives like `grep`. (If the `rg` command is not found, then use alternatives.) - Read files in chunks with a max chunk size of 250 lines. Do not use python scripts to attempt to output larger chunks of a file. Command line output will be truncated after 10 kilobytes or 256 lines of output, regardless of the command used. -## `apply_patch` - -Your patch language is a stripped‑down, file‑oriented diff format designed to be easy to parse and safe to apply. You can think of it as a high‑level envelope: - -**_ Begin Patch -[ one or more file sections ] -_** End Patch - -Within that envelope, you get a sequence of file operations. -You MUST include a header to specify the action you are taking. -Each operation starts with one of three headers: - -**_ Add File: - create a new file. Every following line is a + line (the initial contents). -_** Delete File: - remove an existing file. Nothing follows. -\*\*\* Update File: - patch an existing file in place (optionally with a rename). - -May be immediately followed by \*\*\* Move to: if you want to rename the file. -Then one or more “hunks”, each introduced by @@ (optionally followed by a hunk header). -Within a hunk each line starts with: - -- for inserted text, - -* for removed text, or - space ( ) for context. - At the end of a truncated hunk you can emit \*\*\* End of File. - -Patch := Begin { FileOp } End -Begin := "**_ Begin Patch" NEWLINE -End := "_** End Patch" NEWLINE -FileOp := AddFile | DeleteFile | UpdateFile -AddFile := "**_ Add File: " path NEWLINE { "+" line NEWLINE } -DeleteFile := "_** Delete File: " path NEWLINE -UpdateFile := "**_ Update File: " path NEWLINE [ MoveTo ] { Hunk } -MoveTo := "_** Move to: " newPath NEWLINE -Hunk := "@@" [ header ] NEWLINE { HunkLine } [ "*** End of File" NEWLINE ] -HunkLine := (" " | "-" | "+") text NEWLINE - -A full patch can combine several operations: - -**_ Begin Patch -_** Add File: hello.txt -+Hello world -**_ Update File: src/app.py -_** Move to: src/main.py -@@ def greet(): --print("Hi") -+print("Hello, world!") -**_ Delete File: obsolete.txt -_** End Patch - -It is important to remember: - -- You must include a header with your intended action (Add/Delete/Update) -- You must prefix new lines with `+` even when creating a new file - -You can invoke apply_patch like: - -``` -shell {"command":["apply_patch","*** Begin Patch\n*** Add File: hello.txt\n+Hello, world!\n*** End Patch\n"]} -``` - ## `update_plan` A tool named `update_plan` is available to you. You can use it to keep an up‑to‑date, step‑by‑step plan for the task. diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index 15b8ea89c0..bf7bc133b9 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -47,7 +47,17 @@ impl Prompt { .as_deref() .unwrap_or(BASE_INSTRUCTIONS); let mut sections: Vec<&str> = vec![base]; - if model.needs_special_apply_patch_instructions { + + // When there are no custom instructions, add apply_patch if either: + // - the model needs special instructions, or + // - there is no apply_patch tool present + let is_apply_patch_tool_present = self + .tools + .iter() + .any(|t| matches!(t, OpenAiTool::Function(f) if f.name == "apply_patch")); + if self.base_instructions_override.is_none() + && (model.needs_special_apply_patch_instructions || !is_apply_patch_tool_present) + { sections.push(APPLY_PATCH_TOOL_INSTRUCTIONS); } Cow::Owned(sections.join("\n")) diff --git a/codex-rs/core/src/openai_tools.rs b/codex-rs/core/src/openai_tools.rs index f02c784777..df60d0c373 100644 --- a/codex-rs/core/src/openai_tools.rs +++ b/codex-rs/core/src/openai_tools.rs @@ -175,7 +175,7 @@ fn create_shell_tool_for_sandbox(sandbox_policy: &SandboxPolicy) -> OpenAiTool { properties.insert( "justification".to_string(), JsonSchema::String { - description: Some("Only set if ask_for_escalated_permissions is true. 1-sentence explanation of why we want to run this command.".to_string()), + description: Some("Only set if with_escalated_permissions is true. 1-sentence explanation of why we want to run this command.".to_string()), }, ); } @@ -247,8 +247,8 @@ pub(crate) struct ApplyPatchToolArgs { pub(crate) input: String, } -fn create_apply_patch_tool() -> OpenAiTool { - // Minimal schema: one required string argument containing the patch body +/// Returns a JSON tool that can be used to edit files. Public for testing, please use `get_openai_tools`. +fn create_apply_patch_json_tool() -> OpenAiTool { let mut properties = BTreeMap::new(); properties.insert( "input".to_string(), @@ -259,59 +259,73 @@ fn create_apply_patch_tool() -> OpenAiTool { OpenAiTool::Function(ResponsesApiTool { name: "apply_patch".to_string(), - description: r#"Use this tool to edit files. + description: r#"Use the `apply_patch` tool to edit files. Your patch language is a stripped‑down, file‑oriented diff format designed to be easy to parse and safe to apply. You can think of it as a high‑level envelope: -**_ Begin Patch +*** Begin Patch [ one or more file sections ] -_** End Patch +*** End Patch Within that envelope, you get a sequence of file operations. You MUST include a header to specify the action you are taking. Each operation starts with one of three headers: -**_ Add File: - create a new file. Every following line is a + line (the initial contents). -_** Delete File: - remove an existing file. Nothing follows. -\*\*\* Update File: - patch an existing file in place (optionally with a rename). +*** Add File: - create a new file. Every following line is a + line (the initial contents). +*** Delete File: - remove an existing file. Nothing follows. +*** Update File: - patch an existing file in place (optionally with a rename). -May be immediately followed by \*\*\* Move to: if you want to rename the file. +May be immediately followed by *** Move to: if you want to rename the file. Then one or more “hunks”, each introduced by @@ (optionally followed by a hunk header). Within a hunk each line starts with: -- for inserted text, +For instructions on [context_before] and [context_after]: +- By default, show 3 lines of code immediately above and 3 lines immediately below each change. If a change is within 3 lines of a previous change, do NOT duplicate the first change’s [context_after] lines in the second change’s [context_before] lines. +- If 3 lines of context is insufficient to uniquely identify the snippet of code within the file, use the @@ operator to indicate the class or function to which the snippet belongs. For instance, we might have: +@@ class BaseClass +[3 lines of pre-context] +- [old_code] ++ [new_code] +[3 lines of post-context] -* for removed text, or - space ( ) for context. - At the end of a truncated hunk you can emit \*\*\* End of File. +- If a code block is repeated so many times in a class or function such that even a single `@@` statement and 3 lines of context cannot uniquely identify the snippet of code, you can use multiple `@@` statements to jump to the right context. For instance: +@@ class BaseClass +@@ def method(): +[3 lines of pre-context] +- [old_code] ++ [new_code] +[3 lines of post-context] + +The full grammar definition is below: Patch := Begin { FileOp } End -Begin := "**_ Begin Patch" NEWLINE -End := "_** End Patch" NEWLINE +Begin := "*** Begin Patch" NEWLINE +End := "*** End Patch" NEWLINE FileOp := AddFile | DeleteFile | UpdateFile -AddFile := "**_ Add File: " path NEWLINE { "+" line NEWLINE } -DeleteFile := "_** Delete File: " path NEWLINE -UpdateFile := "**_ Update File: " path NEWLINE [ MoveTo ] { Hunk } -MoveTo := "_** Move to: " newPath NEWLINE +AddFile := "*** Add File: " path NEWLINE { "+" line NEWLINE } +DeleteFile := "*** Delete File: " path NEWLINE +UpdateFile := "*** Update File: " path NEWLINE [ MoveTo ] { Hunk } +MoveTo := "*** Move to: " newPath NEWLINE Hunk := "@@" [ header ] NEWLINE { HunkLine } [ "*** End of File" NEWLINE ] HunkLine := (" " | "-" | "+") text NEWLINE A full patch can combine several operations: -**_ Begin Patch -_** Add File: hello.txt +*** Begin Patch +*** Add File: hello.txt +Hello world -**_ Update File: src/app.py -_** Move to: src/main.py +*** Update File: src/app.py +*** Move to: src/main.py @@ def greet(): -print("Hi") +print("Hello, world!") -**_ Delete File: obsolete.txt -_** End Patch +*** Delete File: obsolete.txt +*** End Patch It is important to remember: - You must include a header with your intended action (Add/Delete/Update) - You must prefix new lines with `+` even when creating a new file +- File references can only be relative, NEVER ABSOLUTE. "# .to_string(), strict: false, @@ -326,7 +340,7 @@ It is important to remember: /// Returns JSON values that are compatible with Function Calling in the /// Responses API: /// https://platform.openai.com/docs/guides/function-calling?api-mode=responses -pub(crate) fn create_tools_json_for_responses_api( +pub fn create_tools_json_for_responses_api( tools: &Vec, ) -> crate::error::Result> { let mut tools_json = Vec::new(); @@ -544,7 +558,7 @@ pub(crate) fn get_openai_tools( } if config.apply_patch_tool { - tools.push(create_apply_patch_tool()); + tools.push(create_apply_patch_json_tool()); } if let Some(mcp_tools) = mcp_tools { diff --git a/codex-rs/core/tests/prompt_caching.rs b/codex-rs/core/tests/prompt_caching.rs index bf49262a88..ac1dbcd969 100644 --- a/codex-rs/core/tests/prompt_caching.rs +++ b/codex-rs/core/tests/prompt_caching.rs @@ -25,6 +25,179 @@ fn sse_completed(id: &str) -> String { load_sse_fixture_with_id("tests/fixtures/completed_template.json", id) } +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn default_system_instructions_contain_apply_patch() { + use pretty_assertions::assert_eq; + + let server = MockServer::start().await; + + let sse = sse_completed("resp"); + let template = ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(sse, "text/event-stream"); + + // Expect two POSTs to /v1/responses + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with(template) + .expect(2) + .mount(&server) + .await; + + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + + let cwd = TempDir::new().unwrap(); + let codex_home = TempDir::new().unwrap(); + let mut config = load_default_config_for_test(&codex_home); + config.cwd = cwd.path().to_path_buf(); + config.model_provider = model_provider; + config.user_instructions = Some("be consistent and helpful".to_string()); + + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .expect("create new conversation") + .conversation; + + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: "hello 1".into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: "hello 2".into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2, "expected two POST requests"); + + let expected_instructions = [ + include_str!("../prompt.md"), + include_str!("../../apply-patch/apply_patch_tool_instructions.md"), + ] + .join("\n"); + + let body0 = requests[0].body_json::().unwrap(); + assert_eq!( + body0["instructions"], + serde_json::json!(expected_instructions), + ); + let body1 = requests[1].body_json::().unwrap(); + assert_eq!( + body1["instructions"], + serde_json::json!(expected_instructions), + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn prompt_tools_are_consistent_across_requests() { + use pretty_assertions::assert_eq; + + let server = MockServer::start().await; + + let sse = sse_completed("resp"); + let template = ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(sse, "text/event-stream"); + + // Expect two POSTs to /v1/responses + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with(template) + .expect(2) + .mount(&server) + .await; + + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + + let cwd = TempDir::new().unwrap(); + let codex_home = TempDir::new().unwrap(); + let mut config = load_default_config_for_test(&codex_home); + config.cwd = cwd.path().to_path_buf(); + config.model_provider = model_provider; + config.user_instructions = Some("be consistent and helpful".to_string()); + config.include_apply_patch_tool = true; + config.include_plan_tool = true; + + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .expect("create new conversation") + .conversation; + + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: "hello 1".into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: "hello 2".into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2, "expected two POST requests"); + + let expected_instructions: &str = include_str!("../prompt.md"); + // our internal implementation is responsible for keeping tools in sync + // with the OpenAI schema, so we just verify the tool presence here + let expected_tools_names: &[&str] = &["shell", "update_plan", "apply_patch"]; + fn assert_tool_names(body: &serde_json::Value, expected_names: &[&str]) { + assert_eq!( + body["tools"] + .as_array() + .unwrap() + .iter() + .map(|t| t["name"].as_str().unwrap().to_string()) + .collect::>(), + expected_names + ); + } + + let body0 = requests[0].body_json::().unwrap(); + assert_eq!( + body0["instructions"], + serde_json::json!(expected_instructions), + ); + assert_tool_names(&body0, expected_tools_names); + + let body1 = requests[1].body_json::().unwrap(); + assert_eq!( + body1["instructions"], + serde_json::json!(expected_instructions), + ); + assert_tool_names(&body1, expected_tools_names); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn prefixes_context_and_instructions_once_and_consistently_across_requests() { use pretty_assertions::assert_eq; From 76dc3f60546c69b71589f6f10a05ba4d4bde3cd1 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 22 Aug 2025 08:24:13 -0700 Subject: [PATCH 3/8] show diff output in the pager (#2568) this shows `/diff` output in an overlay like the transcript, instead of dumping it into history. https://github.com/user-attachments/assets/48e79b65-7f66-45dd-97b3-d5c627ac7349 --- codex-rs/tui/src/app.rs | 28 ++++++++++++++++++++++++++-- codex-rs/tui/src/chatwidget.rs | 3 +-- codex-rs/tui/src/history_cell.rs | 14 -------------- codex-rs/tui/src/transcript_app.rs | 21 +++++++++++++++++---- 4 files changed, 44 insertions(+), 22 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index e706fa723b..8737347f1d 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -5,6 +5,7 @@ use crate::file_search::FileSearchManager; use crate::transcript_app::TranscriptApp; use crate::tui; use crate::tui::TuiEvent; +use codex_ansi_escape::ansi_escape_line; use codex_core::ConversationManager; use codex_core::config::Config; use codex_core::protocol::TokenUsage; @@ -17,6 +18,7 @@ use crossterm::terminal::EnterAlternateScreen; use crossterm::terminal::LeaveAlternateScreen; use crossterm::terminal::supports_keyboard_enhancement; use ratatui::layout::Rect; +use ratatui::style::Stylize; use ratatui::text::Line; use std::path::PathBuf; use std::sync::Arc; @@ -126,8 +128,8 @@ impl App { tui.insert_history_lines(lines); } self.transcript_overlay = None; + tui.frame_requester().schedule_frame(); } - tui.frame_requester().schedule_frame(); } else { match event { TuiEvent::Key(key_event) => { @@ -238,7 +240,29 @@ impl App { } AppEvent::CodexOp(op) => self.chat_widget.submit_op(op), AppEvent::DiffResult(text) => { - self.chat_widget.add_diff_output(text); + // Clear the in-progress state in the bottom pane + self.chat_widget.on_diff_complete(); + + // Enter alternate screen and set viewport to full size. + let _ = execute!(tui.terminal.backend_mut(), EnterAlternateScreen); + if let Ok(size) = tui.terminal.size() { + self.transcript_saved_viewport = Some(tui.terminal.viewport_area); + tui.terminal + .set_viewport_area(Rect::new(0, 0, size.width, size.height)); + let _ = tui.terminal.clear(); + } + + // Build pager lines directly without the "/diff" header + let pager_lines: Vec> = if text.trim().is_empty() { + vec!["No changes detected.".italic().into()] + } else { + text.lines().map(ansi_escape_line).collect() + }; + self.transcript_overlay = Some(TranscriptApp::with_title( + pager_lines, + "D I F F".to_string(), + )); + tui.frame_requester().schedule_frame(); } AppEvent::StartFileSearch(query) => { if !query.is_empty() { diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index fa18342b53..6e6f8de834 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -818,9 +818,8 @@ impl ChatWidget { self.request_redraw(); } - pub(crate) fn add_diff_output(&mut self, diff_output: String) { + pub(crate) fn on_diff_complete(&mut self) { self.bottom_pane.set_task_running(false); - self.add_to_history(history_cell::new_diff_output(diff_output)); self.mark_needs_redraw(); } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 9f236d78d1..0cbb4888df 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -506,20 +506,6 @@ pub(crate) fn new_completed_mcp_tool_call( Box::new(PlainHistoryCell { lines }) } -pub(crate) fn new_diff_output(message: String) -> PlainHistoryCell { - let mut lines: Vec> = Vec::new(); - lines.push(Line::from("/diff".magenta())); - - if message.trim().is_empty() { - lines.push(Line::from("No changes detected.".italic())); - } else { - lines.extend(message.lines().map(ansi_escape_line)); - } - - lines.push(Line::from("")); - PlainHistoryCell { lines } -} - pub(crate) fn new_status_output( config: &Config, usage: &TokenUsage, diff --git a/codex-rs/tui/src/transcript_app.rs b/codex-rs/tui/src/transcript_app.rs index 059e2a07c8..0c84ad552c 100644 --- a/codex-rs/tui/src/transcript_app.rs +++ b/codex-rs/tui/src/transcript_app.rs @@ -21,6 +21,7 @@ pub(crate) struct TranscriptApp { pub(crate) transcript_lines: Vec>, pub(crate) scroll_offset: usize, pub(crate) is_done: bool, + title: String, } impl TranscriptApp { @@ -29,6 +30,16 @@ impl TranscriptApp { transcript_lines, scroll_offset: 0, is_done: false, + title: "T R A N S C R I P T".to_string(), + } + } + + pub(crate) fn with_title(transcript_lines: Vec>, title: String) -> Self { + Self { + transcript_lines, + scroll_offset: 0, + is_done: false, + title, } } @@ -114,8 +125,11 @@ impl TranscriptApp { } => { self.scroll_offset = usize::MAX; } - _ => {} + _ => { + return; + } } + tui.frame_requester().schedule_frame(); } fn scroll_area(&self, area: Rect) -> Rect { @@ -130,9 +144,8 @@ impl TranscriptApp { Span::from("/ ".repeat(area.width as usize / 2)) .dim() .render_ref(area, buf); - Span::from("/ T R A N S C R I P T") - .dim() - .render_ref(area, buf); + let header = format!("/ {}", self.title); + Span::from(header).dim().render_ref(area, buf); // Main content area (excludes header and bottom status section) let content_area = self.scroll_area(area); From 80b00a193eeed29b4f92a5610a71e6868501f4b5 Mon Sep 17 00:00:00 2001 From: vjain419 Date: Fri, 22 Aug 2025 17:12:10 +0100 Subject: [PATCH 4/8] =?UTF-8?q?feat(gpt5):=20add=20model=5Fverbosity=20for?= =?UTF-8?q?=20GPT=E2=80=915=20via=20Responses=20API=20(#2108)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Summary** - Adds `model_verbosity` config (values: low, medium, high). - Sends `text.verbosity` only for GPT‑5 family models via the Responses API. - Updates docs and adds serialization tests. **Motivation** - GPT‑5 introduces a verbosity control to steer output length/detail without pro mpt surgery. - Exposing it as a config knob keeps prompts stable and makes behavior explicit and repeatable. **Changes** - Config: - Added `Verbosity` enum (low|medium|high). - Added optional `model_verbosity` to `ConfigToml`, `Config`, and `ConfigProfi le`. - Request wiring: - Extended `ResponsesApiRequest` with optional `text` object. - Populates `text.verbosity` only when model family is `gpt-5`; omitted otherw ise. - Tests: - Verifies `text.verbosity` serializes when set and is omitted when not set. - Docs: - Added “GPT‑5 Verbosity” section in `codex-rs/README.md`. - Added `model_verbosity` section to `codex-rs/config.md`. **Usage** - In `~/.codex/config.toml`: - `model = "gpt-5"` - `model_verbosity = "low"` (or `"medium"` default, `"high"`) - CLI override example: - `codex -c model="gpt-5" -c model_verbosity="high"` **API Impact** - Requests to GPT‑5 via Responses API include: `text: { verbosity: "low|medium|h igh" }` when configured. - For legacy models or Chat Completions providers, `text` is omitted. **Backward Compatibility** - Default behavior unchanged when `model_verbosity` is not set (server default “ medium”). **Testing** - Added unit tests for serialization/omission of `text.verbosity`. - Ran `cargo fmt` and `cargo test --all-features` (all green). **Docs** - `README.md`: new “GPT‑5 Verbosity” note under Config with example. - `config.md`: new `model_verbosity` section. **Out of Scope** - No changes to temperature/top_p or other GPT‑5 parameters. - No changes to Chat Completions wiring. **Risks / Notes** - If OpenAI changes the wire shape for verbosity, we may need to update `Respons esApiRequest`. - Behavior gated to `gpt-5` model family to avoid unexpected effects elsewhere. **Checklist** - [x] Code gated to GPT‑5 family only - [x] Docs updated (`README.md`, `config.md`) - [x] Tests added and passing - [x] Formatting applied Release note: Add `model_verbosity` config to control GPT‑5 output verbosity via the Responses API (low|medium|high). --- codex-rs/config.md | 19 ++++++ codex-rs/core/src/client.rs | 15 +++++ codex-rs/core/src/client_common.rs | 90 +++++++++++++++++++++++++++++ codex-rs/core/src/config.rs | 11 +++- codex-rs/core/src/config_profile.rs | 2 + codex-rs/core/src/config_types.rs | 42 ++++++++++++++ 6 files changed, 178 insertions(+), 1 deletion(-) diff --git a/codex-rs/config.md b/codex-rs/config.md index 1af963a16f..a9d01dbc26 100644 --- a/codex-rs/config.md +++ b/codex-rs/config.md @@ -243,6 +243,25 @@ To disable reasoning summaries, set `model_reasoning_summary` to `"none"` in you model_reasoning_summary = "none" # disable reasoning summaries ``` +## model_verbosity + +Controls output length/detail on GPT‑5 family models when using the Responses API. Supported values: + +- `"low"` +- `"medium"` (default when omitted) +- `"high"` + +When set, Codex includes a `text` object in the request payload with the configured verbosity, for example: `"text": { "verbosity": "low" }`. + +Example: + +```toml +model = "gpt-5" +model_verbosity = "low" +``` + +Note: This applies only to providers using the Responses API. Chat Completions providers are unaffected. + ## model_supports_reasoning_summaries By default, `reasoning` is only set on requests to OpenAI models that are known to support them. To force `reasoning` to set on requests to the current model, you can force this behavior by setting the following in `config.toml`: diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 5534e11f36..471312d35c 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -28,6 +28,7 @@ use crate::client_common::ResponseEvent; use crate::client_common::ResponseStream; use crate::client_common::ResponsesApiRequest; use crate::client_common::create_reasoning_param_for_request; +use crate::client_common::create_text_param_for_request; use crate::config::Config; use crate::error::CodexErr; use crate::error::Result; @@ -164,6 +165,19 @@ impl ModelClient { let input_with_instructions = prompt.get_formatted_input(); + // Only include `text.verbosity` for GPT-5 family models + let text = if self.config.model_family.family == "gpt-5" { + create_text_param_for_request(self.config.model_verbosity) + } else { + if self.config.model_verbosity.is_some() { + warn!( + "model_verbosity is set but ignored for non-gpt-5 model family: {}", + self.config.model_family.family + ); + } + None + }; + let payload = ResponsesApiRequest { model: &self.config.model, instructions: &full_instructions, @@ -176,6 +190,7 @@ impl ModelClient { stream: true, include, prompt_cache_key: Some(self.session_id.to_string()), + text, }; let mut attempt = 0; diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index bf7bc133b9..f99fe94518 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -1,3 +1,4 @@ +use crate::config_types::Verbosity as VerbosityConfig; use crate::error::Result; use crate::model_family::ModelFamily; use crate::models::ContentItem; @@ -99,6 +100,32 @@ pub(crate) struct Reasoning { pub(crate) summary: ReasoningSummaryConfig, } +/// Controls under the `text` field in the Responses API for GPT-5. +#[derive(Debug, Serialize, Default, Clone, Copy)] +pub(crate) struct TextControls { + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) verbosity: Option, +} + +#[derive(Debug, Serialize, Default, Clone, Copy)] +#[serde(rename_all = "lowercase")] +pub(crate) enum OpenAiVerbosity { + Low, + #[default] + Medium, + High, +} + +impl From for OpenAiVerbosity { + fn from(v: VerbosityConfig) -> Self { + match v { + VerbosityConfig::Low => OpenAiVerbosity::Low, + VerbosityConfig::Medium => OpenAiVerbosity::Medium, + VerbosityConfig::High => OpenAiVerbosity::High, + } + } +} + /// Request object that is serialized as JSON and POST'ed when using the /// Responses API. #[derive(Debug, Serialize)] @@ -119,6 +146,8 @@ pub(crate) struct ResponsesApiRequest<'a> { pub(crate) include: Vec, #[serde(skip_serializing_if = "Option::is_none")] pub(crate) prompt_cache_key: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) text: Option, } pub(crate) fn create_reasoning_param_for_request( @@ -133,6 +162,14 @@ pub(crate) fn create_reasoning_param_for_request( } } +pub(crate) fn create_text_param_for_request( + verbosity: Option, +) -> Option { + verbosity.map(|v| TextControls { + verbosity: Some(v.into()), + }) +} + pub(crate) struct ResponseStream { pub(crate) rx_event: mpsc::Receiver>, } @@ -161,4 +198,57 @@ mod tests { let full = prompt.get_full_instructions(&model_family); assert_eq!(full, expected); } + + #[test] + fn serializes_text_verbosity_when_set() { + let input: Vec = vec![]; + let tools: Vec = vec![]; + let req = ResponsesApiRequest { + model: "gpt-5", + instructions: "i", + input: &input, + tools: &tools, + tool_choice: "auto", + parallel_tool_calls: false, + reasoning: None, + store: true, + stream: true, + include: vec![], + prompt_cache_key: None, + text: Some(TextControls { + verbosity: Some(OpenAiVerbosity::Low), + }), + }; + + let v = serde_json::to_value(&req).expect("json"); + assert_eq!( + v.get("text") + .and_then(|t| t.get("verbosity")) + .and_then(|s| s.as_str()), + Some("low") + ); + } + + #[test] + fn omits_text_when_not_set() { + let input: Vec = vec![]; + let tools: Vec = vec![]; + let req = ResponsesApiRequest { + model: "gpt-5", + instructions: "i", + input: &input, + tools: &tools, + tool_choice: "auto", + parallel_tool_calls: false, + reasoning: None, + store: true, + stream: true, + include: vec![], + prompt_cache_key: None, + text: None, + }; + + let v = serde_json::to_value(&req).expect("json"); + assert!(v.get("text").is_none()); + } } diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 5b6a1ed265..31ff5f103e 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -6,6 +6,7 @@ use crate::config_types::ShellEnvironmentPolicy; use crate::config_types::ShellEnvironmentPolicyToml; use crate::config_types::Tui; use crate::config_types::UriBasedFileOpener; +use crate::config_types::Verbosity; use crate::model_family::ModelFamily; use crate::model_family::find_family_for_model; use crate::model_provider_info::ModelProviderInfo; @@ -150,6 +151,9 @@ pub struct Config { /// request using the Responses API. pub model_reasoning_summary: ReasoningSummary, + /// Optional verbosity control for GPT-5 models (Responses API `text.verbosity`). + pub model_verbosity: Option, + /// Base URL for requests to ChatGPT (as opposed to the OpenAI API). pub chatgpt_base_url: String, @@ -441,6 +445,8 @@ pub struct ConfigToml { pub model_reasoning_effort: Option, pub model_reasoning_summary: Option, + /// Optional verbosity control for GPT-5 models (Responses API `text.verbosity`). + pub model_verbosity: Option, /// Override to force-enable reasoning summaries for the configured model. pub model_supports_reasoning_summaries: Option, @@ -718,7 +724,7 @@ impl Config { .model_reasoning_summary .or(cfg.model_reasoning_summary) .unwrap_or_default(), - + model_verbosity: config_profile.model_verbosity.or(cfg.model_verbosity), chatgpt_base_url: config_profile .chatgpt_base_url .or(cfg.chatgpt_base_url) @@ -1087,6 +1093,7 @@ disable_response_storage = true show_raw_agent_reasoning: false, model_reasoning_effort: ReasoningEffort::High, model_reasoning_summary: ReasoningSummary::Detailed, + model_verbosity: None, chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(), experimental_resume: None, base_instructions: None, @@ -1140,6 +1147,7 @@ disable_response_storage = true show_raw_agent_reasoning: false, model_reasoning_effort: ReasoningEffort::default(), model_reasoning_summary: ReasoningSummary::default(), + model_verbosity: None, chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(), experimental_resume: None, base_instructions: None, @@ -1208,6 +1216,7 @@ disable_response_storage = true show_raw_agent_reasoning: false, model_reasoning_effort: ReasoningEffort::default(), model_reasoning_summary: ReasoningSummary::default(), + model_verbosity: None, chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(), experimental_resume: None, base_instructions: None, diff --git a/codex-rs/core/src/config_profile.rs b/codex-rs/core/src/config_profile.rs index 8bb0738788..54869919fb 100644 --- a/codex-rs/core/src/config_profile.rs +++ b/codex-rs/core/src/config_profile.rs @@ -1,6 +1,7 @@ use serde::Deserialize; use std::path::PathBuf; +use crate::config_types::Verbosity; use crate::protocol::AskForApproval; use codex_protocol::config_types::ReasoningEffort; use codex_protocol::config_types::ReasoningSummary; @@ -17,6 +18,7 @@ pub struct ConfigProfile { pub disable_response_storage: Option, pub model_reasoning_effort: Option, pub model_reasoning_summary: Option, + pub model_verbosity: Option, pub chatgpt_base_url: Option, pub experimental_instructions_file: Option, } diff --git a/codex-rs/core/src/config_types.rs b/codex-rs/core/src/config_types.rs index d6661699fe..88be151fff 100644 --- a/codex-rs/core/src/config_types.rs +++ b/codex-rs/core/src/config_types.rs @@ -8,6 +8,8 @@ use std::path::PathBuf; use wildmatch::WildMatchPattern; use serde::Deserialize; +use serde::Serialize; +use strum_macros::Display; #[derive(Deserialize, Debug, Clone, PartialEq)] pub struct McpServerConfig { @@ -183,3 +185,43 @@ impl From for ShellEnvironmentPolicy { } } } + +/// See https://platform.openai.com/docs/guides/reasoning?api-mode=responses#get-started-with-reasoning +#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display)] +#[serde(rename_all = "lowercase")] +#[strum(serialize_all = "lowercase")] +pub enum ReasoningEffort { + Low, + #[default] + Medium, + High, + /// Option to disable reasoning. + None, +} + +/// A summary of the reasoning performed by the model. This can be useful for +/// debugging and understanding the model's reasoning process. +/// See https://platform.openai.com/docs/guides/reasoning?api-mode=responses#reasoning-summaries +#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display)] +#[serde(rename_all = "lowercase")] +#[strum(serialize_all = "lowercase")] +pub enum ReasoningSummary { + #[default] + Auto, + Concise, + Detailed, + /// Option to disable reasoning summaries. + None, +} + +/// Controls output length/detail on GPT-5 models via the Responses API. +/// Serialized with lowercase values to match the OpenAI API. +#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display)] +#[serde(rename_all = "lowercase")] +#[strum(serialize_all = "lowercase")] +pub enum Verbosity { + Low, + #[default] + Medium, + High, +} From 59f6b1654f7e911431105956da41e11e0c07a803 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 22 Aug 2025 09:41:15 -0700 Subject: [PATCH 5/8] improve suspend behavior (#2569) This is a somewhat roundabout way to fix the issue that pressing ^Z would put the shell prompt in the wrong place (overwriting some of the status area below the composer). While I'm at it, clean up the suspend logic and fix some suspend-while-in-alt-screen behavior too. --- codex-rs/tui/src/app.rs | 43 +-------- codex-rs/tui/src/transcript_app.rs | 1 + codex-rs/tui/src/tui.rs | 146 +++++++++++++++++++++++++---- 3 files changed, 133 insertions(+), 57 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 8737347f1d..057d77ff94 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -13,11 +13,7 @@ use color_eyre::eyre::Result; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; -use crossterm::execute; -use crossterm::terminal::EnterAlternateScreen; -use crossterm::terminal::LeaveAlternateScreen; use crossterm::terminal::supports_keyboard_enhancement; -use ratatui::layout::Rect; use ratatui::style::Stylize; use ratatui::text::Line; use std::path::PathBuf; @@ -44,7 +40,6 @@ pub(crate) struct App { // Transcript overlay state transcript_overlay: Option, deferred_history_lines: Vec>, - transcript_saved_viewport: Option, enhanced_keys_supported: bool, @@ -89,7 +84,6 @@ impl App { transcript_lines: Vec::new(), transcript_overlay: None, deferred_history_lines: Vec::new(), - transcript_saved_viewport: None, commit_anim_running: Arc::new(AtomicBool::new(false)), }; @@ -119,10 +113,7 @@ impl App { overlay.handle_event(tui, event)?; if overlay.is_done { // Exit alternate screen and restore viewport. - let _ = execute!(tui.terminal.backend_mut(), LeaveAlternateScreen); - if let Some(saved) = self.transcript_saved_viewport.take() { - tui.terminal.set_viewport_area(saved); - } + let _ = tui.leave_alt_screen(); if !self.deferred_history_lines.is_empty() { let lines = std::mem::take(&mut self.deferred_history_lines); tui.insert_history_lines(lines); @@ -154,16 +145,6 @@ impl App { }, )?; } - #[cfg(unix)] - TuiEvent::ResumeFromSuspend => { - let cursor_pos = tui.terminal.get_cursor_position()?; - tui.terminal.set_viewport_area(ratatui::layout::Rect::new( - 0, - cursor_pos.y, - 0, - 0, - )); - } } } Ok(true) @@ -242,17 +223,8 @@ impl App { AppEvent::DiffResult(text) => { // Clear the in-progress state in the bottom pane self.chat_widget.on_diff_complete(); - - // Enter alternate screen and set viewport to full size. - let _ = execute!(tui.terminal.backend_mut(), EnterAlternateScreen); - if let Ok(size) = tui.terminal.size() { - self.transcript_saved_viewport = Some(tui.terminal.viewport_area); - tui.terminal - .set_viewport_area(Rect::new(0, 0, size.width, size.height)); - let _ = tui.terminal.clear(); - } - - // Build pager lines directly without the "/diff" header + // Enter alternate screen using TUI helper and build pager lines + let _ = tui.enter_alt_screen(); let pager_lines: Vec> = if text.trim().is_empty() { vec!["No changes detected.".italic().into()] } else { @@ -317,14 +289,7 @@ impl App { .. } => { // Enter alternate screen and set viewport to full size. - let _ = execute!(tui.terminal.backend_mut(), EnterAlternateScreen); - if let Ok(size) = tui.terminal.size() { - self.transcript_saved_viewport = Some(tui.terminal.viewport_area); - tui.terminal - .set_viewport_area(Rect::new(0, 0, size.width, size.height)); - let _ = tui.terminal.clear(); - } - + let _ = tui.enter_alt_screen(); self.transcript_overlay = Some(TranscriptApp::new(self.transcript_lines.clone())); tui.frame_requester().schedule_frame(); } diff --git a/codex-rs/tui/src/transcript_app.rs b/codex-rs/tui/src/transcript_app.rs index 0c84ad552c..efb0c2de8b 100644 --- a/codex-rs/tui/src/transcript_app.rs +++ b/codex-rs/tui/src/transcript_app.rs @@ -62,6 +62,7 @@ impl TranscriptApp { fn handle_key_event(&mut self, tui: &mut tui::Tui, key_event: KeyEvent) { match key_event { + // Ctrl+Z is handled at the App level when transcript overlay is active KeyEvent { code: KeyCode::Char('q'), kind: KeyEventKind::Press, diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 4ec17ea8af..3f4df39b7e 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -2,6 +2,11 @@ use std::io::Result; use std::io::Stdout; use std::io::stdout; use std::pin::Pin; +use std::sync::Arc; +use std::sync::atomic::AtomicBool; +#[cfg(unix)] +use std::sync::atomic::AtomicU8; +use std::sync::atomic::Ordering; use std::time::Duration; use std::time::Instant; @@ -10,12 +15,12 @@ use crossterm::cursor; use crossterm::cursor::MoveTo; use crossterm::event::DisableBracketedPaste; use crossterm::event::EnableBracketedPaste; -use crossterm::event::KeyCode; use crossterm::event::KeyEvent; -use crossterm::event::KeyEventKind; use crossterm::event::KeyboardEnhancementFlags; use crossterm::event::PopKeyboardEnhancementFlags; use crossterm::event::PushKeyboardEnhancementFlags; +use crossterm::terminal::EnterAlternateScreen; +use crossterm::terminal::LeaveAlternateScreen; use crossterm::terminal::ScrollUp; use ratatui::backend::Backend; use ratatui::backend::CrosstermBackend; @@ -98,8 +103,6 @@ pub enum TuiEvent { Key(KeyEvent), Paste(String), Draw, - #[cfg(unix)] - ResumeFromSuspend, } pub struct Tui { @@ -107,6 +110,29 @@ pub struct Tui { draw_tx: tokio::sync::broadcast::Sender<()>, pub(crate) terminal: Terminal, pending_history_lines: Vec>, + alt_saved_viewport: Option, + #[cfg(unix)] + resume_pending: Arc, // Stores a ResumeAction + // True when overlay alt-screen UI is active + alt_screen_active: Arc, +} + +#[cfg(unix)] +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +#[repr(u8)] +enum ResumeAction { + None = 0, + RealignInline = 1, + RestoreAlt = 2, +} + +#[cfg(unix)] +fn take_resume_action(pending: &AtomicU8) -> ResumeAction { + match pending.swap(ResumeAction::None as u8, Ordering::Relaxed) { + 1 => ResumeAction::RealignInline, + 2 => ResumeAction::RestoreAlt, + _ => ResumeAction::None, + } } #[derive(Clone, Debug)] @@ -184,6 +210,10 @@ impl Tui { draw_tx, terminal, pending_history_lines: vec![], + alt_saved_viewport: None, + #[cfg(unix)] + resume_pending: Arc::new(AtomicU8::new(0)), + alt_screen_active: Arc::new(AtomicBool::new(false)), } } @@ -197,25 +227,38 @@ impl Tui { use tokio_stream::StreamExt; let mut crossterm_events = crossterm::event::EventStream::new(); let mut draw_rx = self.draw_tx.subscribe(); + #[cfg(unix)] + let resume_pending = self.resume_pending.clone(); + #[cfg(unix)] + let alt_screen_active = self.alt_screen_active.clone(); let event_stream = async_stream::stream! { loop { select! { Some(Ok(event)) = crossterm_events.next() => { match event { - crossterm::event::Event::Key(KeyEvent { - code: KeyCode::Char('z'), - modifiers: crossterm::event::KeyModifiers::CONTROL, - kind: KeyEventKind::Press, - .. - }) => { - #[cfg(unix)] - { - let _ = Tui::suspend(); - yield TuiEvent::ResumeFromSuspend; - yield TuiEvent::Draw; - } - } crossterm::event::Event::Key(key_event) => { + #[cfg(unix)] + if matches!( + key_event, + crossterm::event::KeyEvent { + code: crossterm::event::KeyCode::Char('z'), + modifiers: crossterm::event::KeyModifiers::CONTROL, + kind: crossterm::event::KeyEventKind::Press, + .. + } + ) + { + if alt_screen_active.load(Ordering::Relaxed) { + let _ = execute!(stdout(), LeaveAlternateScreen); + resume_pending.store(ResumeAction::RestoreAlt as u8, Ordering::Relaxed); + } else { + resume_pending.store(ResumeAction::RealignInline as u8, Ordering::Relaxed); + } + let _ = execute!(stdout(), crossterm::cursor::Show); + let _ = Tui::suspend(); + yield TuiEvent::Draw; + continue; + } yield TuiEvent::Key(key_event); } crossterm::event::Event::Resize(_, _) => { @@ -246,7 +289,6 @@ impl Tui { }; Box::pin(event_stream) } - #[cfg(unix)] fn suspend() -> Result<()> { restore()?; @@ -255,6 +297,69 @@ impl Tui { Ok(()) } + #[cfg(unix)] + fn apply_resume_action(&mut self, action: ResumeAction) -> Result<()> { + match action { + ResumeAction::RealignInline => { + let cursor_pos = self.terminal.get_cursor_position()?; + self.terminal + .set_viewport_area(ratatui::layout::Rect::new(0, cursor_pos.y, 0, 0)); + } + ResumeAction::RestoreAlt => { + // When we're resuming from alt screen, we need to save what the cursor position + // _was_ when we resumed. That way, when we leave the alt screen, we can restore + // the cursor to the new position. + if let Ok((_x, y)) = crossterm::cursor::position() + && let Some(saved) = self.alt_saved_viewport.as_mut() + { + saved.y = y; + } + let _ = execute!(self.terminal.backend_mut(), EnterAlternateScreen); + if let Ok(size) = self.terminal.size() { + self.terminal.set_viewport_area(ratatui::layout::Rect::new( + 0, + 0, + size.width, + size.height, + )); + self.terminal.clear()?; + } + } + ResumeAction::None => {} + } + Ok(()) + } + + // Public suspend() removed; Ctrl+Z is handled internally via event_stream + draw. + + /// Enter alternate screen and expand the viewport to full terminal size, saving the current + /// inline viewport for restoration when leaving. + pub fn enter_alt_screen(&mut self) -> Result<()> { + let _ = execute!(self.terminal.backend_mut(), EnterAlternateScreen); + if let Ok(size) = self.terminal.size() { + self.alt_saved_viewport = Some(self.terminal.viewport_area); + self.terminal.set_viewport_area(ratatui::layout::Rect::new( + 0, + 0, + size.width, + size.height, + )); + let _ = self.terminal.clear(); + } + self.alt_screen_active.store(true, Ordering::Relaxed); + Ok(()) + } + + /// Leave alternate screen and restore the previously saved inline viewport, if any. + pub fn leave_alt_screen(&mut self) -> Result<()> { + let _ = execute!(self.terminal.backend_mut(), LeaveAlternateScreen); + if let Some(saved) = self.alt_saved_viewport.take() { + self.terminal.set_viewport_area(saved); + } + self.alt_screen_active.store(false, Ordering::Relaxed); + Ok(()) + } + pub fn insert_history_lines(&mut self, lines: Vec>) { self.pending_history_lines.extend(lines); self.frame_requester().schedule_frame(); @@ -266,6 +371,11 @@ impl Tui { draw_fn: impl FnOnce(&mut custom_terminal::Frame), ) -> Result<()> { std::io::stdout().sync_update(|_| { + #[cfg(unix)] + { + // Apply any post-resume action before layout/clear/draw. + self.apply_resume_action(take_resume_action(&self.resume_pending))?; + } let terminal = &mut self.terminal; let screen_size = terminal.size()?; let last_known_screen_size = terminal.last_known_screen_size; From c5d21a4564efc03ec47b14c3396b1cad3e6854ed Mon Sep 17 00:00:00 2001 From: pap-openai Date: Fri, 22 Aug 2025 18:05:43 +0100 Subject: [PATCH 6/8] ctrl+v image + @file accepts images (#1695) allow ctrl+v in TUI for images + @file that are images are appended as raw files (and read by the model) rather than pasted as a path that cannot be read by the model. Re-used components and same interface we're using for copying pasted content in https://github.com/openai/codex/commit/72504f1d9c6eb17086d86ef1fb0d17676812461b. @aibrahim-oai as you've implemented this, mind having a look at this one? https://github.com/user-attachments/assets/c6c1153b-6b32-4558-b9a2-f8c57d2be710 --------- Co-authored-by: easong-openai Co-authored-by: Daniel Edrisian Co-authored-by: Michael Bolin --- codex-rs/Cargo.lock | 164 ++++++++ codex-rs/tui/Cargo.toml | 7 +- codex-rs/tui/src/app.rs | 9 + codex-rs/tui/src/bottom_pane/chat_composer.rs | 385 +++++++++++++++++- codex-rs/tui/src/bottom_pane/mod.rs | 19 + codex-rs/tui/src/chatwidget.rs | 21 +- codex-rs/tui/src/clipboard_paste.rs | 97 +++++ codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/tui.rs | 39 +- 9 files changed, 728 insertions(+), 14 deletions(-) create mode 100644 codex-rs/tui/src/clipboard_paste.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 34e7932053..cb4cd68d81 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -186,6 +186,26 @@ version = "1.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dde20b3d026af13f561bdd0f15edf01fc734f0dafcedbaf42bba506a9517f223" +[[package]] +name = "arboard" +version = "3.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55f533f8e0af236ffe5eb979b99381df3258853f00ba2e44b6e1955292c75227" +dependencies = [ + "clipboard-win", + "image", + "log", + "objc2", + "objc2-app-kit", + "objc2-core-foundation", + "objc2-core-graphics", + "objc2-foundation", + "parking_lot", + "percent-encoding", + "windows-sys 0.59.0", + "x11rb", +] + [[package]] name = "arg_enum_proc_macro" version = "0.3.4" @@ -928,6 +948,7 @@ name = "codex-tui" version = "0.0.0" dependencies = [ "anyhow", + "arboard", "async-stream", "base64 0.22.1", "chrono", @@ -962,6 +983,7 @@ dependencies = [ "strum 0.27.2", "strum_macros 0.27.2", "supports-color", + "tempfile", "textwrap 0.16.2", "tokio", "tokio-stream", @@ -1410,6 +1432,16 @@ dependencies = [ "winapi", ] +[[package]] +name = "dispatch2" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "89a09f22a6c6069a18470eb92d2298acf25463f14256d24778e1230d789a2aec" +dependencies = [ + "bitflags 2.9.1", + "objc2", +] + [[package]] name = "display_container" version = "0.9.0" @@ -1863,6 +1895,16 @@ dependencies = [ "version_check", ] +[[package]] +name = "gethostname" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0176e0459c2e4a1fe232f984bca6890e681076abb9934f6cea7c326f3fc47818" +dependencies = [ + "libc", + "windows-targets 0.48.5", +] + [[package]] name = "getopts" version = "0.2.23" @@ -3059,6 +3101,42 @@ dependencies = [ "objc2-encode", ] +[[package]] +name = "objc2-app-kit" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6f29f568bec459b0ddff777cec4fe3fd8666d82d5a40ebd0ff7e66134f89bcc" +dependencies = [ + "bitflags 2.9.1", + "objc2", + "objc2-core-graphics", + "objc2-foundation", +] + +[[package]] +name = "objc2-core-foundation" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1c10c2894a6fed806ade6027bcd50662746363a9589d3ec9d9bef30a4e4bc166" +dependencies = [ + "bitflags 2.9.1", + "dispatch2", + "objc2", +] + +[[package]] +name = "objc2-core-graphics" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "989c6c68c13021b5c2d6b71456ebb0f9dc78d752e86a98da7c716f4f9470f5a4" +dependencies = [ + "bitflags 2.9.1", + "dispatch2", + "objc2", + "objc2-core-foundation", + "objc2-io-surface", +] + [[package]] name = "objc2-encode" version = "4.1.0" @@ -3073,6 +3151,18 @@ checksum = "900831247d2fe1a09a683278e5384cfb8c80c79fe6b166f9d14bfdde0ea1b03c" dependencies = [ "bitflags 2.9.1", "objc2", + "objc2-core-foundation", +] + +[[package]] +name = "objc2-io-surface" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7282e9ac92529fa3457ce90ebb15f4ecbc383e8338060960760fa2cf75420c3c" +dependencies = [ + "bitflags 2.9.1", + "objc2", + "objc2-core-foundation", ] [[package]] @@ -5846,6 +5936,21 @@ dependencies = [ "windows_x86_64_msvc 0.42.2", ] +[[package]] +name = "windows-targets" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a2fa6e2155d7247be68c096456083145c183cbbbc2764150dda45a87197940c" +dependencies = [ + "windows_aarch64_gnullvm 0.48.5", + "windows_aarch64_msvc 0.48.5", + "windows_i686_gnu 0.48.5", + "windows_i686_msvc 0.48.5", + "windows_x86_64_gnu 0.48.5", + "windows_x86_64_gnullvm 0.48.5", + "windows_x86_64_msvc 0.48.5", +] + [[package]] name = "windows-targets" version = "0.52.6" @@ -5884,6 +5989,12 @@ version = "0.42.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "597a5118570b68bc08d8d59125332c54f1ba9d9adeedeef5b99b02ba2b0698f8" +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b38e32f0abccf9987a4e3079dfb67dcd799fb61361e53e2882c3cbaf0d905d8" + [[package]] name = "windows_aarch64_gnullvm" version = "0.52.6" @@ -5902,6 +6013,12 @@ version = "0.42.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e08e8864a60f06ef0d0ff4ba04124db8b0fb3be5776a5cd47641e942e58c4d43" +[[package]] +name = "windows_aarch64_msvc" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc35310971f3b2dbbf3f0690a219f40e2d9afcf64f9ab7cc1be722937c26b4bc" + [[package]] name = "windows_aarch64_msvc" version = "0.52.6" @@ -5920,6 +6037,12 @@ version = "0.42.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c61d927d8da41da96a81f029489353e68739737d3beca43145c8afec9a31a84f" +[[package]] +name = "windows_i686_gnu" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a75915e7def60c94dcef72200b9a8e58e5091744960da64ec734a6c6e9b3743e" + [[package]] name = "windows_i686_gnu" version = "0.52.6" @@ -5950,6 +6073,12 @@ version = "0.42.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "44d840b6ec649f480a41c8d80f9c65108b92d89345dd94027bfe06ac444d1060" +[[package]] +name = "windows_i686_msvc" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f55c233f70c4b27f66c523580f78f1004e8b5a8b659e05a4eb49d4166cca406" + [[package]] name = "windows_i686_msvc" version = "0.52.6" @@ -5968,6 +6097,12 @@ version = "0.42.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8de912b8b8feb55c064867cf047dda097f92d51efad5b491dfb98f6bbb70cb36" +[[package]] +name = "windows_x86_64_gnu" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53d40abd2583d23e4718fddf1ebec84dbff8381c07cae67ff7768bbf19c6718e" + [[package]] name = "windows_x86_64_gnu" version = "0.52.6" @@ -5986,6 +6121,12 @@ version = "0.42.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "26d41b46a36d453748aedef1486d5c7a85db22e56aff34643984ea85514e94a3" +[[package]] +name = "windows_x86_64_gnullvm" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b7b52767868a23d5bab768e390dc5f5c55825b6d30b86c844ff2dc7414044cc" + [[package]] name = "windows_x86_64_gnullvm" version = "0.52.6" @@ -6004,6 +6145,12 @@ version = "0.42.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9aec5da331524158c6d1a4ac0ab1541149c0b9505fde06423b02f5ef0106b9f0" +[[package]] +name = "windows_x86_64_msvc" +version = "0.48.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed94fce61571a4006852b7389a063ab983c02eb1bb37b47f8272ce92d06d9538" + [[package]] name = "windows_x86_64_msvc" version = "0.52.6" @@ -6070,6 +6217,23 @@ version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ea2f10b9bb0928dfb1b42b65e1f9e36f7f54dbdf08457afefb38afcdec4fa2bb" +[[package]] +name = "x11rb" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d91ffca73ee7f68ce055750bf9f6eca0780b8c85eff9bc046a3b0da41755e12" +dependencies = [ + "gethostname", + "rustix 0.38.44", + "x11rb-protocol", +] + +[[package]] +name = "x11rb-protocol" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec107c4503ea0b4a98ef47356329af139c0a4f7750e621cf2973cd3385ebcb3d" + [[package]] name = "yaml-rust" version = "0.4.5" diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 20ceb0b79c..6d69e97e73 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -22,6 +22,7 @@ workspace = true [dependencies] anyhow = "1" +arboard = "3" async-stream = "0.3.6" base64 = "0.22.1" chrono = { version = "0.4", features = ["serde"] } @@ -41,7 +42,10 @@ codex-protocol = { path = "../protocol" } color-eyre = "0.6.3" crossterm = { version = "0.28.1", features = ["bracketed-paste", "event-stream"] } diffy = "0.4.2" -image = { version = "^0.25.6", default-features = false, features = ["jpeg"] } +image = { version = "^0.25.6", default-features = false, features = [ + "jpeg", + "png", +] } lazy_static = "1" mcp-types = { path = "../mcp-types" } once_cell = "1" @@ -61,6 +65,7 @@ shlex = "1.3.0" strum = "0.27.2" strum_macros = "0.27.2" supports-color = "3.0.2" +tempfile = "3" textwrap = "0.16.2" tokio = { version = "1", features = [ "io-std", diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 057d77ff94..a532ba71c8 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -145,6 +145,15 @@ impl App { }, )?; } + TuiEvent::AttachImage { + path, + width, + height, + format_label, + } => { + self.chat_widget + .attach_image(path, width, height, format_label); + } } } Ok(true) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 0cc34542c5..909ecbc0d9 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -31,6 +31,9 @@ use crate::bottom_pane::textarea::TextArea; use crate::bottom_pane::textarea::TextAreaState; use codex_file_search::FileMatch; use std::cell::RefCell; +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; /// If the pasted content exceeds this number of characters, replace it with a /// placeholder in the UI. @@ -43,6 +46,12 @@ pub enum InputResult { None, } +#[derive(Clone, Debug, PartialEq)] +struct AttachedImage { + placeholder: String, + path: PathBuf, +} + struct TokenUsageInfo { total_token_usage: TokenUsage, last_token_usage: TokenUsage, @@ -71,6 +80,7 @@ pub(crate) struct ChatComposer { pending_pastes: Vec<(String, String)>, token_usage_info: Option, has_focus: bool, + attached_images: Vec, placeholder_text: String, } @@ -103,6 +113,7 @@ impl ChatComposer { pending_pastes: Vec::new(), token_usage_info: None, has_focus: has_input_focus, + attached_images: Vec::new(), placeholder_text, } } @@ -196,6 +207,20 @@ impl ChatComposer { true } + pub fn attach_image(&mut self, path: PathBuf, width: u32, height: u32, format_label: &str) { + let placeholder = format!("[image {width}x{height} {format_label}]"); + // Insert as an element to match large paste placeholder behavior: + // styled distinctly and treated atomically for cursor/mutations. + self.textarea.insert_element(&placeholder); + self.attached_images + .push(AttachedImage { placeholder, path }); + } + + pub fn take_recent_submission_images(&mut self) -> Vec { + let images = std::mem::take(&mut self.attached_images); + images.into_iter().map(|img| img.path).collect() + } + /// Integrate results from an asynchronous file search. pub(crate) fn on_file_search_result(&mut self, query: String, matches: Vec) { // Only apply if user is still editing a token starting with `query`. @@ -346,19 +371,74 @@ impl ChatComposer { modifiers: KeyModifiers::NONE, .. } => { - if let Some(sel) = popup.selected_match() { - let sel_path = sel.to_string(); - // Drop popup borrow before using self mutably again. - self.insert_selected_path(&sel_path); + let Some(sel) = popup.selected_match() else { self.active_popup = ActivePopup::None; return (InputResult::None, true); + }; + + let sel_path = sel.to_string(); + // If selected path looks like an image (png/jpeg), attach as image instead of inserting text. + let is_image = Self::is_image_path(&sel_path); + if is_image { + // Determine dimensions; if that fails fall back to normal path insertion. + let path_buf = PathBuf::from(&sel_path); + if let Ok((w, h)) = image::image_dimensions(&path_buf) { + // Remove the current @token (mirror logic from insert_selected_path without inserting text) + // using the flat text and byte-offset cursor API. + let cursor_offset = self.textarea.cursor(); + let text = self.textarea.text(); + let before_cursor = &text[..cursor_offset]; + let after_cursor = &text[cursor_offset..]; + + // Determine token boundaries in the full text. + let start_idx = before_cursor + .char_indices() + .rfind(|(_, c)| c.is_whitespace()) + .map(|(idx, c)| idx + c.len_utf8()) + .unwrap_or(0); + let end_rel_idx = after_cursor + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(after_cursor.len()); + let end_idx = cursor_offset + end_rel_idx; + + self.textarea.replace_range(start_idx..end_idx, ""); + self.textarea.set_cursor(start_idx); + + let format_label = match Path::new(&sel_path) + .extension() + .and_then(|e| e.to_str()) + .map(|s| s.to_ascii_lowercase()) + { + Some(ext) if ext == "png" => "PNG", + Some(ext) if ext == "jpg" || ext == "jpeg" => "JPEG", + _ => "IMG", + }; + self.attach_image(path_buf.clone(), w, h, format_label); + // Add a trailing space to keep typing fluid. + self.textarea.insert_str(" "); + } else { + // Fallback to plain path insertion if metadata read fails. + self.insert_selected_path(&sel_path); + } + } else { + // Non-image: inserting file path. + self.insert_selected_path(&sel_path); } - (InputResult::None, false) + // No selection: treat Enter as closing the popup/session. + self.active_popup = ActivePopup::None; + (InputResult::None, true) } input => self.handle_input_basic(input), } } + fn is_image_path(path: &str) -> bool { + let lower = path.to_ascii_lowercase(); + lower.ends_with(".png") || lower.ends_with(".jpg") || lower.ends_with(".jpeg") + } + /// Extract the `@token` that the cursor is currently positioned on, if any. /// /// The returned string **does not** include the leading `@`. @@ -545,12 +625,19 @@ impl ChatComposer { } self.pending_pastes.clear(); - if text.is_empty() { - (InputResult::None, true) - } else { - self.history.record_local_submission(&text); - (InputResult::Submitted(text), true) + // Strip image placeholders from the submitted text; images are retrieved via take_recent_submission_images() + for img in &self.attached_images { + if text.contains(&img.placeholder) { + text = text.replace(&img.placeholder, ""); + } } + + text = text.trim().to_string(); + if !text.is_empty() { + self.history.record_local_submission(&text); + } + // Do not clear attached_images here; ChatWidget drains them via take_recent_submission_images(). + (InputResult::Submitted(text), true) } input => self.handle_input_basic(input), } @@ -558,6 +645,16 @@ impl ChatComposer { /// Handle generic Input events that modify the textarea content. fn handle_input_basic(&mut self, input: KeyEvent) -> (InputResult, bool) { + // Special handling for backspace on placeholders + if let KeyEvent { + code: KeyCode::Backspace, + .. + } = input + && self.try_remove_any_placeholder_at_cursor() + { + return (InputResult::None, true); + } + // Normal input handling self.textarea.input(input); let text_after = self.textarea.text(); @@ -566,9 +663,165 @@ impl ChatComposer { self.pending_pastes .retain(|(placeholder, _)| text_after.contains(placeholder)); + // Keep attached images in proportion to how many matching placeholders exist in the text. + // This handles duplicate placeholders that share the same visible label. + if !self.attached_images.is_empty() { + let mut needed: HashMap = HashMap::new(); + for img in &self.attached_images { + needed + .entry(img.placeholder.clone()) + .or_insert_with(|| text_after.matches(&img.placeholder).count()); + } + + let mut used: HashMap = HashMap::new(); + let mut kept: Vec = Vec::with_capacity(self.attached_images.len()); + for img in self.attached_images.drain(..) { + let total_needed = *needed.get(&img.placeholder).unwrap_or(&0); + let used_count = used.entry(img.placeholder.clone()).or_insert(0); + if *used_count < total_needed { + kept.push(img); + *used_count += 1; + } + } + self.attached_images = kept; + } + (InputResult::None, true) } + /// Attempts to remove an image or paste placeholder if the cursor is at the end of one. + /// Returns true if a placeholder was removed. + fn try_remove_any_placeholder_at_cursor(&mut self) -> bool { + let p = self.textarea.cursor(); + let text = self.textarea.text(); + + // Try image placeholders first + let mut out: Option<(usize, String)> = None; + // Detect if the cursor is at the end of any image placeholder. + // If duplicates exist, remove the specific occurrence's mapping. + for (i, img) in self.attached_images.iter().enumerate() { + let ph = &img.placeholder; + if p < ph.len() { + continue; + } + let start = p - ph.len(); + if text[start..p] != *ph { + continue; + } + + // Count the number of occurrences of `ph` before `start`. + let mut occ_before = 0usize; + let mut search_pos = 0usize; + while search_pos < start { + if let Some(found) = text[search_pos..start].find(ph) { + occ_before += 1; + search_pos += found + ph.len(); + } else { + break; + } + } + + // Remove the occ_before-th attached image that shares this placeholder label. + out = if let Some((remove_idx, _)) = self + .attached_images + .iter() + .enumerate() + .filter(|(_, img2)| img2.placeholder == *ph) + .nth(occ_before) + { + Some((remove_idx, ph.clone())) + } else { + Some((i, ph.clone())) + }; + break; + } + if let Some((idx, placeholder)) = out { + self.textarea.replace_range(p - placeholder.len()..p, ""); + self.attached_images.remove(idx); + return true; + } + + // Also handle when the cursor is at the START of an image placeholder. + // let result = 'out: { + let out: Option<(usize, String)> = 'out: { + for (i, img) in self.attached_images.iter().enumerate() { + let ph = &img.placeholder; + if p + ph.len() > text.len() { + continue; + } + if &text[p..p + ph.len()] != ph { + continue; + } + + // Count occurrences of `ph` before `p`. + let mut occ_before = 0usize; + let mut search_pos = 0usize; + while search_pos < p { + if let Some(found) = text[search_pos..p].find(ph) { + occ_before += 1; + search_pos += found + ph.len(); + } else { + break 'out None; + } + } + + if let Some((remove_idx, _)) = self + .attached_images + .iter() + .enumerate() + .filter(|(_, img2)| img2.placeholder == *ph) + .nth(occ_before) + { + break 'out Some((remove_idx, ph.clone())); + } else { + break 'out Some((i, ph.clone())); + } + } + None + }; + + if let Some((idx, placeholder)) = out { + self.textarea.replace_range(p..p + placeholder.len(), ""); + self.attached_images.remove(idx); + return true; + } + + // Then try pasted-content placeholders + if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { + if p < ph.len() { + return None; + } + let start = p - ph.len(); + if text[start..p] == *ph { + Some(ph.clone()) + } else { + None + } + }) { + self.textarea.replace_range(p - placeholder.len()..p, ""); + self.pending_pastes.retain(|(ph, _)| ph != &placeholder); + return true; + } + + // Also handle when the cursor is at the START of a pasted-content placeholder. + if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { + if p + ph.len() > text.len() { + return None; + } + if &text[p..p + ph.len()] == ph { + Some(ph.clone()) + } else { + None + } + }) { + self.textarea.replace_range(p..p + placeholder.len(), ""); + self.pending_pastes.retain(|(ph, _)| ph != &placeholder); + return true; + } + + false + } + /// Synchronize `self.command_popup` with the current text in the /// textarea. This must be called after every modification that can change /// the text so the popup is shown/updated/hidden as appropriate. @@ -746,10 +999,14 @@ impl WidgetRef for &ChatComposer { #[cfg(test)] mod tests { + use super::*; + use std::path::PathBuf; + use crate::app_event::AppEvent; use crate::bottom_pane::AppEventSender; use crate::bottom_pane::ChatComposer; use crate::bottom_pane::InputResult; + use crate::bottom_pane::chat_composer::AttachedImage; use crate::bottom_pane::chat_composer::LARGE_PASTE_CHAR_THRESHOLD; use crate::bottom_pane::textarea::TextArea; use tokio::sync::mpsc::unbounded_channel; @@ -1312,4 +1569,112 @@ mod tests { ] ); } + + // --- Image attachment tests --- + #[test] + fn attach_image_and_submit_includes_image_paths() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = + ChatComposer::new(true, sender, false, "Ask Codex to do anything".to_string()); + let path = PathBuf::from("/tmp/image1.png"); + composer.attach_image(path.clone(), 32, 16, "PNG"); + composer.handle_paste(" hi".into()); + let (result, _) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + match result { + InputResult::Submitted(text) => assert_eq!(text, "hi"), + _ => panic!("expected Submitted"), + } + let imgs = composer.take_recent_submission_images(); + assert_eq!(vec![path], imgs); + } + + #[test] + fn attach_image_without_text_submits_empty_text_and_images() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = + ChatComposer::new(true, sender, false, "Ask Codex to do anything".to_string()); + let path = PathBuf::from("/tmp/image2.png"); + composer.attach_image(path.clone(), 10, 5, "PNG"); + let (result, _) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + match result { + InputResult::Submitted(text) => assert!(text.is_empty()), + _ => panic!("expected Submitted"), + } + let imgs = composer.take_recent_submission_images(); + assert_eq!(imgs.len(), 1); + assert_eq!(imgs[0], path); + assert!(composer.attached_images.is_empty()); + } + + #[test] + fn image_placeholder_backspace_behaves_like_text_placeholder() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = + ChatComposer::new(true, sender, false, "Ask Codex to do anything".to_string()); + let path = PathBuf::from("/tmp/image3.png"); + composer.attach_image(path.clone(), 20, 10, "PNG"); + let placeholder = composer.attached_images[0].placeholder.clone(); + + // Case 1: backspace at end + composer.textarea.move_cursor_to_end_of_line(false); + composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); + assert!(!composer.textarea.text().contains(&placeholder)); + assert!(composer.attached_images.is_empty()); + + // Re-add and test backspace in middle: should break the placeholder string + // and drop the image mapping (same as text placeholder behavior). + composer.attach_image(path.clone(), 20, 10, "PNG"); + let placeholder2 = composer.attached_images[0].placeholder.clone(); + // Move cursor to roughly middle of placeholder + if let Some(start_pos) = composer.textarea.text().find(&placeholder2) { + let mid_pos = start_pos + (placeholder2.len() / 2); + composer.textarea.set_cursor(mid_pos); + composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); + assert!(!composer.textarea.text().contains(&placeholder2)); + assert!(composer.attached_images.is_empty()); + } else { + panic!("Placeholder not found in textarea"); + } + } + + #[test] + fn deleting_one_of_duplicate_image_placeholders_removes_matching_entry() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = + ChatComposer::new(true, sender, false, "Ask Codex to do anything".to_string()); + + let path1 = PathBuf::from("/tmp/image_dup1.png"); + let path2 = PathBuf::from("/tmp/image_dup2.png"); + + composer.attach_image(path1.clone(), 10, 5, "PNG"); + // separate placeholders with a space for clarity + composer.handle_paste(" ".into()); + composer.attach_image(path2.clone(), 10, 5, "PNG"); + + let ph = composer.attached_images[0].placeholder.clone(); + let text = composer.textarea.text().to_string(); + let start1 = text.find(&ph).expect("first placeholder present"); + let end1 = start1 + ph.len(); + composer.textarea.set_cursor(end1); + + // Backspace should delete the first placeholder and its mapping. + composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); + + let new_text = composer.textarea.text().to_string(); + assert_eq!(1, new_text.matches(&ph).count(), "one placeholder remains"); + assert_eq!( + vec![AttachedImage { + path: path2, + placeholder: "[image 10x5 PNG]".to_string() + }], + composer.attached_images, + "one image mapping remains" + ); + } } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 48ad3f02b5..04f5d4b9bf 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -1,4 +1,5 @@ //! Bottom pane: shows the ChatComposer or a BottomPaneView, if one is active. +use std::path::PathBuf; use crate::app_event_sender::AppEventSender; use crate::tui::FrameRequester; @@ -342,6 +343,24 @@ impl BottomPane { self.composer.on_file_search_result(query, matches); self.request_redraw(); } + + pub(crate) fn attach_image( + &mut self, + path: PathBuf, + width: u32, + height: u32, + format_label: &str, + ) { + if self.active_view.is_none() { + self.composer + .attach_image(path, width, height, format_label); + self.request_redraw(); + } + } + + pub(crate) fn take_recent_submission_images(&mut self) -> Vec { + self.composer.take_recent_submission_images() + } } impl WidgetRef for &BottomPane { diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 6e6f8de834..0e4bd85664 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -583,7 +583,11 @@ impl ChatWidget { match self.bottom_pane.handle_key_event(key_event) { InputResult::Submitted(text) => { - self.submit_user_message(text.into()); + let images = self.bottom_pane.take_recent_submission_images(); + self.submit_user_message(UserMessage { + text, + image_paths: images, + }); } InputResult::Command(cmd) => { self.dispatch_command(cmd); @@ -592,6 +596,21 @@ impl ChatWidget { } } + pub(crate) fn attach_image( + &mut self, + path: PathBuf, + width: u32, + height: u32, + format_label: &str, + ) { + tracing::info!( + "attach_image path={path:?} width={width} height={height} format={format_label}", + ); + self.bottom_pane + .attach_image(path.clone(), width, height, format_label); + self.request_redraw(); + } + fn dispatch_command(&mut self, cmd: SlashCommand) { match cmd { SlashCommand::New => { diff --git a/codex-rs/tui/src/clipboard_paste.rs b/codex-rs/tui/src/clipboard_paste.rs new file mode 100644 index 0000000000..3888ac343f --- /dev/null +++ b/codex-rs/tui/src/clipboard_paste.rs @@ -0,0 +1,97 @@ +use std::path::PathBuf; +use tempfile::Builder; + +#[derive(Debug)] +pub enum PasteImageError { + ClipboardUnavailable(String), + NoImage(String), + EncodeFailed(String), + IoError(String), +} + +impl std::fmt::Display for PasteImageError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + PasteImageError::ClipboardUnavailable(msg) => write!(f, "clipboard unavailable: {msg}"), + PasteImageError::NoImage(msg) => write!(f, "no image on clipboard: {msg}"), + PasteImageError::EncodeFailed(msg) => write!(f, "could not encode image: {msg}"), + PasteImageError::IoError(msg) => write!(f, "io error: {msg}"), + } + } +} +impl std::error::Error for PasteImageError {} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum EncodedImageFormat { + Png, +} + +impl EncodedImageFormat { + pub fn label(self) -> &'static str { + match self { + EncodedImageFormat::Png => "PNG", + } + } +} + +#[derive(Debug, Clone)] +pub struct PastedImageInfo { + pub width: u32, + pub height: u32, + pub encoded_format: EncodedImageFormat, // Always PNG for now. +} + +/// Capture image from system clipboard, encode to PNG, and return bytes + info. +pub fn paste_image_as_png() -> Result<(Vec, PastedImageInfo), PasteImageError> { + tracing::debug!("attempting clipboard image read"); + let mut cb = arboard::Clipboard::new() + .map_err(|e| PasteImageError::ClipboardUnavailable(e.to_string()))?; + let img = cb + .get_image() + .map_err(|e| PasteImageError::NoImage(e.to_string()))?; + let w = img.width as u32; + let h = img.height as u32; + + let mut png: Vec = Vec::new(); + let Some(rgba_img) = image::RgbaImage::from_raw(w, h, img.bytes.into_owned()) else { + return Err(PasteImageError::EncodeFailed("invalid RGBA buffer".into())); + }; + let dyn_img = image::DynamicImage::ImageRgba8(rgba_img); + tracing::debug!("clipboard image decoded RGBA {w}x{h}"); + { + let mut cursor = std::io::Cursor::new(&mut png); + dyn_img + .write_to(&mut cursor, image::ImageFormat::Png) + .map_err(|e| PasteImageError::EncodeFailed(e.to_string()))?; + } + + tracing::debug!( + "clipboard image encoded to PNG ({len} bytes)", + len = png.len() + ); + Ok(( + png, + PastedImageInfo { + width: w, + height: h, + encoded_format: EncodedImageFormat::Png, + }, + )) +} + +/// Convenience: write to a temp file and return its path + info. +pub fn paste_image_to_temp_png() -> Result<(PathBuf, PastedImageInfo), PasteImageError> { + let (png, info) = paste_image_as_png()?; + // Create a unique temporary file with a .png suffix to avoid collisions. + let tmp = Builder::new() + .prefix("codex-clipboard-") + .suffix(".png") + .tempfile() + .map_err(|e| PasteImageError::IoError(e.to_string()))?; + std::fs::write(tmp.path(), &png).map_err(|e| PasteImageError::IoError(e.to_string()))?; + // Persist the file (so it remains after the handle is dropped) and return its PathBuf. + let (_file, path) = tmp + .keep() + .map_err(|e| PasteImageError::IoError(e.error.to_string()))?; + Ok((path, info)) +} diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 454a7f3ead..bce0d89933 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -30,6 +30,7 @@ mod bottom_pane; mod chatwidget; mod citation_regex; mod cli; +mod clipboard_paste; mod common; pub mod custom_terminal; mod diff_render; diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 3f4df39b7e..4498f46e17 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -1,6 +1,7 @@ use std::io::Result; use std::io::Stdout; use std::io::stdout; +use std::path::PathBuf; use std::pin::Pin; use std::sync::Arc; use std::sync::atomic::AtomicBool; @@ -15,7 +16,11 @@ use crossterm::cursor; use crossterm::cursor::MoveTo; use crossterm::event::DisableBracketedPaste; use crossterm::event::EnableBracketedPaste; +use crossterm::event::Event; +use crossterm::event::KeyCode; use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; +use crossterm::event::KeyModifiers; use crossterm::event::KeyboardEnhancementFlags; use crossterm::event::PopKeyboardEnhancementFlags; use crossterm::event::PushKeyboardEnhancementFlags; @@ -30,6 +35,7 @@ use ratatui::crossterm::terminal::enable_raw_mode; use ratatui::layout::Offset; use ratatui::text::Line; +use crate::clipboard_paste::paste_image_to_temp_png; use crate::custom_terminal; use crate::custom_terminal::Terminal as CustomTerminal; use tokio::select; @@ -103,6 +109,12 @@ pub enum TuiEvent { Key(KeyEvent), Paste(String), Draw, + AttachImage { + path: PathBuf, + width: u32, + height: u32, + format_label: &'static str, + }, } pub struct Tui { @@ -236,6 +248,29 @@ impl Tui { select! { Some(Ok(event)) = crossterm_events.next() => { match event { + // Detect Ctrl+V to attach an image from the clipboard. + Event::Key(key_event @ KeyEvent { + code: KeyCode::Char('v'), + modifiers: KeyModifiers::CONTROL, + kind: KeyEventKind::Press, + .. + }) => { + match paste_image_to_temp_png() { + Ok((path, info)) => { + yield TuiEvent::AttachImage { + path, + width: info.width, + height: info.height, + format_label: info.encoded_format.label(), + }; + } + Err(_) => { + // Fall back to normal key handling if no image is available. + yield TuiEvent::Key(key_event); + } + } + } + crossterm::event::Event::Key(key_event) => { #[cfg(unix)] if matches!( @@ -261,10 +296,10 @@ impl Tui { } yield TuiEvent::Key(key_event); } - crossterm::event::Event::Resize(_, _) => { + Event::Resize(_, _) => { yield TuiEvent::Draw; } - crossterm::event::Event::Paste(pasted) => { + Event::Paste(pasted) => { yield TuiEvent::Paste(pasted); } _ => {} From cdc77c10fb337ff3626f635ccf0844c70dec96d5 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Fri, 22 Aug 2025 12:23:58 -0700 Subject: [PATCH 7/8] Fix/tui windows multiline paste (#2544) Introduce a minimal paste-burst heuristic in the chat composer so Enter is treated as a newline during paste-like bursts (plain chars arriving in very short intervals), avoiding premature submit after the first line on Windows consoles that lack bracketed paste. - Detect tight sequences of plain Char events; open a short window where Enter inserts a newline instead of submitting. - Extend the window on newline to handle blank lines in pasted content. - No behavior change for terminals that already emit Event::Paste; no OS/env toggles added. --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 196 ++++++++++++++++++ 1 file changed, 196 insertions(+) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 909ecbc0d9..986dfd6182 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -34,6 +34,13 @@ use std::cell::RefCell; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; +use std::time::Duration; +use std::time::Instant; + +// Heuristic thresholds for detecting paste-like input bursts. +const PASTE_BURST_MIN_CHARS: u16 = 3; +const PASTE_BURST_CHAR_INTERVAL: Duration = Duration::from_millis(8); +const PASTE_ENTER_SUPPRESS_WINDOW: Duration = Duration::from_millis(120); /// If the pasted content exceeds this number of characters, replace it with a /// placeholder in the UI. @@ -82,6 +89,13 @@ pub(crate) struct ChatComposer { has_focus: bool, attached_images: Vec, placeholder_text: String, + // Heuristic state to detect non-bracketed paste bursts. + last_plain_char_time: Option, + consecutive_plain_char_burst: u16, + paste_burst_until: Option, + // Buffer to accumulate characters during a detected non-bracketed paste burst. + paste_burst_buffer: String, + in_paste_burst_mode: bool, } /// Popup state – at most one can be visible at any time. @@ -115,6 +129,11 @@ impl ChatComposer { has_focus: has_input_focus, attached_images: Vec::new(), placeholder_text, + last_plain_char_time: None, + consecutive_plain_char_burst: 0, + paste_burst_until: None, + paste_burst_buffer: String::new(), + in_paste_burst_mode: false, } } @@ -202,6 +221,10 @@ impl ChatComposer { } else { self.textarea.insert_str(&pasted); } + // Explicit paste events should not trigger Enter suppression. + self.last_plain_char_time = None; + self.consecutive_plain_char_burst = 0; + self.paste_burst_until = None; self.sync_command_popup(); self.sync_file_search_popup(); true @@ -614,6 +637,60 @@ impl ChatComposer { modifiers: KeyModifiers::NONE, .. } => { + // If we're in a paste-like burst capture, treat Enter as part of the burst + // and accumulate it rather than submitting or inserting immediately. + // Do not treat Enter as paste inside a slash-command context. + let in_slash_context = matches!(self.active_popup, ActivePopup::Command(_)) + || self + .textarea + .text() + .lines() + .next() + .unwrap_or("") + .starts_with('/'); + if (self.in_paste_burst_mode || !self.paste_burst_buffer.is_empty()) + && !in_slash_context + { + self.paste_burst_buffer.push('\n'); + let now = Instant::now(); + // Keep the window alive so subsequent lines are captured too. + self.paste_burst_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW); + return (InputResult::None, true); + } + // If we have pending placeholder pastes, submit immediately to expand them. + if !self.pending_pastes.is_empty() { + let mut text = self.textarea.text().to_string(); + self.textarea.set_text(""); + for (placeholder, actual) in &self.pending_pastes { + if text.contains(placeholder) { + text = text.replace(placeholder, actual); + } + } + self.pending_pastes.clear(); + if text.is_empty() { + return (InputResult::None, true); + } + self.history.record_local_submission(&text); + return (InputResult::Submitted(text), true); + } + + // During a paste-like burst, treat Enter as a newline instead of submit. + let now = Instant::now(); + let tight_after_char = self + .last_plain_char_time + .is_some_and(|t| now.duration_since(t) <= PASTE_BURST_CHAR_INTERVAL); + let recent_after_char = self + .last_plain_char_time + .is_some_and(|t| now.duration_since(t) <= PASTE_ENTER_SUPPRESS_WINDOW); + let burst_by_count = + recent_after_char && self.consecutive_plain_char_burst >= PASTE_BURST_MIN_CHARS; + let in_burst_window = self.paste_burst_until.is_some_and(|until| now <= until); + + if tight_after_char || burst_by_count || in_burst_window { + self.textarea.insert_str("\n"); + self.paste_burst_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW); + return (InputResult::None, true); + } let mut text = self.textarea.text().to_string(); self.textarea.set_text(""); @@ -645,6 +722,94 @@ impl ChatComposer { /// Handle generic Input events that modify the textarea content. fn handle_input_basic(&mut self, input: KeyEvent) -> (InputResult, bool) { + // If we have a buffered non-bracketed paste burst and enough time has + // elapsed since the last char, flush it before handling a new input. + let now = Instant::now(); + let timed_out = self + .last_plain_char_time + .is_some_and(|t| now.duration_since(t) > PASTE_BURST_CHAR_INTERVAL); + if timed_out && (!self.paste_burst_buffer.is_empty() || self.in_paste_burst_mode) { + let pasted = std::mem::take(&mut self.paste_burst_buffer); + self.in_paste_burst_mode = false; + // Reuse normal paste path (handles large-paste placeholders). + self.handle_paste(pasted); + } + + // If we're capturing a burst and receive Enter, accumulate it instead of inserting. + if matches!(input.code, KeyCode::Enter) + && (self.in_paste_burst_mode || !self.paste_burst_buffer.is_empty()) + { + self.paste_burst_buffer.push('\n'); + self.paste_burst_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW); + return (InputResult::None, true); + } + + // Intercept plain Char inputs to optionally accumulate into a burst buffer. + if let KeyEvent { + code: KeyCode::Char(ch), + modifiers, + .. + } = input + { + let has_ctrl_or_alt = + modifiers.contains(KeyModifiers::CONTROL) || modifiers.contains(KeyModifiers::ALT); + if !has_ctrl_or_alt { + // Update burst heuristics. + match self.last_plain_char_time { + Some(prev) if now.duration_since(prev) <= PASTE_BURST_CHAR_INTERVAL => { + self.consecutive_plain_char_burst = + self.consecutive_plain_char_burst.saturating_add(1); + } + _ => { + self.consecutive_plain_char_burst = 1; + } + } + self.last_plain_char_time = Some(now); + + // If we're already buffering, capture the char into the buffer. + if self.in_paste_burst_mode { + self.paste_burst_buffer.push(ch); + // Keep the window alive while we receive the burst. + self.paste_burst_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW); + return (InputResult::None, true); + } else if self.consecutive_plain_char_burst >= PASTE_BURST_MIN_CHARS { + // Do not start burst buffering while typing a slash command (first line starts with '/'). + let first_line = self.textarea.text().lines().next().unwrap_or(""); + if first_line.starts_with('/') { + // Keep heuristics but do not buffer. + self.paste_burst_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW); + // Insert normally. + self.textarea.input(input); + let text_after = self.textarea.text(); + self.pending_pastes + .retain(|(placeholder, _)| text_after.contains(placeholder)); + return (InputResult::None, true); + } + // Begin buffering from this character onward. + self.paste_burst_buffer.push(ch); + self.in_paste_burst_mode = true; + // Keep the window alive to continue capturing. + self.paste_burst_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW); + return (InputResult::None, true); + } + + // Not buffering: insert normally and continue. + self.textarea.input(input); + let text_after = self.textarea.text(); + self.pending_pastes + .retain(|(placeholder, _)| text_after.contains(placeholder)); + return (InputResult::None, true); + } else { + // Modified char ends any burst: flush buffered content before applying. + if !self.paste_burst_buffer.is_empty() || self.in_paste_burst_mode { + let pasted = std::mem::take(&mut self.paste_burst_buffer); + self.in_paste_burst_mode = false; + self.handle_paste(pasted); + } + } + } + + // For non-char inputs (or after flushing), handle normally. // Special handling for backspace on placeholders if let KeyEvent { code: KeyCode::Backspace, @@ -659,6 +824,37 @@ impl ChatComposer { self.textarea.input(input); let text_after = self.textarea.text(); + // Update paste-burst heuristic for plain Char (no Ctrl/Alt) events. + let crossterm::event::KeyEvent { + code, modifiers, .. + } = input; + match code { + KeyCode::Char(_) => { + let has_ctrl_or_alt = modifiers.contains(KeyModifiers::CONTROL) + || modifiers.contains(KeyModifiers::ALT); + if has_ctrl_or_alt { + // Modified char: clear burst window. + self.consecutive_plain_char_burst = 0; + self.last_plain_char_time = None; + self.paste_burst_until = None; + self.in_paste_burst_mode = false; + self.paste_burst_buffer.clear(); + } + // Plain chars handled above. + } + KeyCode::Enter => { + // Keep burst window alive (supports blank lines in paste). + } + _ => { + // Other keys: clear burst window and any buffer (after flushing earlier). + self.consecutive_plain_char_burst = 0; + self.last_plain_char_time = None; + self.paste_burst_until = None; + self.in_paste_burst_mode = false; + // Do not clear paste_burst_buffer here; it should have been flushed above. + } + } + // Check if any placeholders were removed and remove their corresponding pending pastes self.pending_pastes .retain(|(placeholder, _)| text_after.contains(placeholder)); From 81adcb1482a0e0c09396ad88206110067036606f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 22 Aug 2025 13:16:42 -0700 Subject: [PATCH 8/8] fix: prefer sending MCP structuredContent as the function call response, if available --- codex-rs/core/src/codex.rs | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 7d616f96e0..e7e8264b98 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1407,14 +1407,32 @@ async fn run_task( Ok(CallToolResult { content, is_error, - structured_content: _, - }) => match serde_json::to_string(content) { - Ok(content) => (content, *is_error), - Err(e) => { - warn!("Failed to serialize MCP tool call output: {e}"); - (e.to_string(), Some(true)) - } - }, + structured_content, + }) => { + // In terms of what to send back to the + // model, we prefer structured_content, if + // available, and fal back to content, + // otherwise. + let mut is_error = *is_error; + let content = if let Some(structured_content) = + structured_content + && let Ok(serialized_structured_content) = + serde_json::to_string(&structured_content) + { + serialized_structured_content + } else if let Ok(serialized_content) = + serde_json::to_string(content) + { + serialized_content + } else { + // If we could not serialize either + // content or structured_content to + // JSON, flag this as an error. + is_error = Some(true); + "".to_owned() + }; + (content, is_error) + } Err(e) => (e.clone(), Some(true)), }; items_to_record_in_conversation_history.push(