From 1e3918939379d8a43e4487eb01521c0cee2f6537 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 16 May 2025 11:33:08 -0700 Subject: [PATCH 1/2] feat: add support for file_opener option in Rust, similiar to #911 (#957) This ports the enhancement introduced in https://github.com/openai/codex/pull/911 (and the fixes in https://github.com/openai/codex/pull/919) for the TypeScript CLI to the Rust one. --- codex-rs/Cargo.lock | 10 ++ codex-rs/README.md | 16 ++ codex-rs/core/src/config.rs | 45 +++++- codex-rs/tui/Cargo.toml | 8 +- codex-rs/tui/src/chatwidget.rs | 6 +- codex-rs/tui/src/citation_regex.rs | 22 +++ .../tui/src/conversation_history_widget.rs | 8 +- codex-rs/tui/src/history_cell.rs | 8 +- codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/markdown.rs | 137 +++++++++++++++++- 10 files changed, 247 insertions(+), 14 deletions(-) create mode 100644 codex-rs/tui/src/citation_regex.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 7b12b654ef..5358065cd5 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -629,8 +629,12 @@ dependencies = [ "codex-core", "color-eyre", "crossterm", + "lazy_static", "mcp-types", + "path-clean", + "pretty_assertions", "ratatui", + "regex", "serde_json", "shlex", "strum 0.27.1", @@ -2468,6 +2472,12 @@ dependencies = [ "path-dedot", ] +[[package]] +name = "path-clean" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "17359afc20d7ab31fdb42bb844c8b3bb1dabd7dcf7e68428492da7f16966fcef" + [[package]] name = "path-dedot" version = "3.1.1" diff --git a/codex-rs/README.md b/codex-rs/README.md index 9fe9827bff..a8b5841d4a 100644 --- a/codex-rs/README.md +++ b/codex-rs/README.md @@ -310,6 +310,22 @@ To disable this behavior, configure `[history]` as follows: persistence = "none" # "save-all" is the default value ``` +### file_opener + +Identifies the editor/URI scheme to use for hyperlinking citations in model output. If set, citations to files in the model output will be hyperlinked using the specified URI scheme so they can be ctrl/cmd-clicked from the terminal to open them. + +For example, if the model output includes a reference such as `【F:/home/user/project/main.py†L42-L50】`, then this would be rewritten to link to the URI `vscode://file/home/user/project/main.py:42`. + +Note this is **not** a general editor setting (like `$EDITOR`), as it only accepts a fixed set of values: + +- `"vscode"` (default) +- `"vscode-insiders"` +- `"windsurf"` +- `"cursor"` +- `"none"` to explicitly disable this feature + +Currently, `"vscode"` is the default, though Codex does not verify VS Code is installed. As such, `file_opener` may default to `"none"` or something else in the future. + ### project_doc_max_bytes Maximum number of bytes to read from an `AGENTS.md` file to include in the instructions sent with the first turn of a session. Defaults to 32 KiB. diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index b63b51e036..fc56e85e61 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -84,6 +84,10 @@ pub struct Config { /// Settings that govern if and what will be written to `~/.codex/history.jsonl`. pub history: History, + + /// Optional URI-based file opener. If set, citations to files in the model + /// output will be hyperlinked using the specified URI scheme. + pub file_opener: UriBasedFileOpener, } /// Settings that govern if and what will be written to `~/.codex/history.jsonl`. @@ -97,7 +101,7 @@ pub struct History { pub max_bytes: Option, } -#[derive(Deserialize, Debug, Clone, PartialEq, Default)] +#[derive(Deserialize, Debug, Copy, Clone, PartialEq, Default)] #[serde(rename_all = "kebab-case")] pub enum HistoryPersistence { /// Save all history entries to disk. @@ -107,6 +111,37 @@ pub enum HistoryPersistence { None, } +#[derive(Deserialize, Debug, Copy, Clone, PartialEq)] +pub enum UriBasedFileOpener { + #[serde(rename = "vscode")] + VsCode, + + #[serde(rename = "vscode-insiders")] + VsCodeInsiders, + + #[serde(rename = "windsurf")] + Windsurf, + + #[serde(rename = "cursor")] + Cursor, + + /// Option to disable the URI-based file opener. + #[serde(rename = "none")] + None, +} + +impl UriBasedFileOpener { + pub fn get_scheme(&self) -> Option<&str> { + match self { + UriBasedFileOpener::VsCode => Some("vscode"), + UriBasedFileOpener::VsCodeInsiders => Some("vscode-insiders"), + UriBasedFileOpener::Windsurf => Some("windsurf"), + UriBasedFileOpener::Cursor => Some("cursor"), + UriBasedFileOpener::None => None, + } + } +} + /// Base config deserialized from ~/.codex/config.toml. #[derive(Deserialize, Debug, Clone, Default)] pub struct ConfigToml { @@ -158,6 +193,10 @@ pub struct ConfigToml { /// Settings that govern if and what will be written to `~/.codex/history.jsonl`. #[serde(default)] pub history: Option, + + /// Optional URI-based file opener. If set, citations to files in the model + /// output will be hyperlinked using the specified URI scheme. + pub file_opener: Option, } impl ConfigToml { @@ -351,6 +390,7 @@ impl Config { project_doc_max_bytes: cfg.project_doc_max_bytes.unwrap_or(PROJECT_DOC_MAX_BYTES), codex_home, history, + file_opener: cfg.file_opener.unwrap_or(UriBasedFileOpener::VsCode), }; Ok(config) } @@ -686,6 +726,7 @@ disable_response_storage = true project_doc_max_bytes: PROJECT_DOC_MAX_BYTES, codex_home: fixture.codex_home(), history: History::default(), + file_opener: UriBasedFileOpener::VsCode, }, o3_profile_config ); @@ -721,6 +762,7 @@ disable_response_storage = true project_doc_max_bytes: PROJECT_DOC_MAX_BYTES, codex_home: fixture.codex_home(), history: History::default(), + file_opener: UriBasedFileOpener::VsCode, }; assert_eq!(expected_gpt3_profile_config, gpt3_profile_config); @@ -771,6 +813,7 @@ disable_response_storage = true project_doc_max_bytes: PROJECT_DOC_MAX_BYTES, codex_home: fixture.codex_home(), history: History::default(), + file_opener: UriBasedFileOpener::VsCode, }; assert_eq!(expected_zdr_profile_config, zdr_profile_config); diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index fa075ada4a..c09baf28fa 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -22,11 +22,14 @@ codex-core = { path = "../core" } codex-common = { path = "../common", features = ["cli", "elapsed"] } color-eyre = "0.6.3" crossterm = { version = "0.28.1", features = ["bracketed-paste"] } +lazy_static = "1" mcp-types = { path = "../mcp-types" } +path-clean = "1.0.1" ratatui = { version = "0.29.0", features = [ "unstable-widget-ref", "unstable-rendered-line-info", ] } +regex = "1" serde_json = "1" shlex = "1.3.0" strum = "0.27.1" @@ -44,4 +47,7 @@ tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } tui-input = "0.11.1" tui-markdown = "0.3.3" tui-textarea = "0.7.0" -uuid = { version = "1" } +uuid = "1" + +[dev-dependencies] +pretty_assertions = "1" diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 6771adb1fa..24d37c4c0a 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -209,11 +209,13 @@ impl ChatWidget<'_> { self.request_redraw(); } EventMsg::AgentMessage(AgentMessageEvent { message }) => { - self.conversation_history.add_agent_message(message); + self.conversation_history + .add_agent_message(&self.config, message); self.request_redraw(); } EventMsg::AgentReasoning(AgentReasoningEvent { text }) => { - self.conversation_history.add_agent_reasoning(text); + self.conversation_history + .add_agent_reasoning(&self.config, text); self.request_redraw(); } EventMsg::TaskStarted => { diff --git a/codex-rs/tui/src/citation_regex.rs b/codex-rs/tui/src/citation_regex.rs new file mode 100644 index 0000000000..7cda1ef11f --- /dev/null +++ b/codex-rs/tui/src/citation_regex.rs @@ -0,0 +1,22 @@ +#![allow(clippy::expect_used)] + +use regex::Regex; + +// This is defined in its own file so we can limit the scope of +// `allow(clippy::expect_used)` because we cannot scope it to the `lazy_static!` +// macro. +lazy_static::lazy_static! { + /// Regular expression that matches Codex-style source file citations such as: + /// + /// ```text + /// 【F:src/main.rs†L10-L20】 + /// ``` + /// + /// Capture groups: + /// 1. file path (anything except the dagger `†` symbol) + /// 2. start line number (digits) + /// 3. optional end line (digits or `?`) + pub(crate) static ref CITATION_REGEX: Regex = Regex::new( + r"【F:([^†]+)†L(\d+)(?:-L(\d+|\?))?】" + ).expect("failed to compile citation regex"); +} diff --git a/codex-rs/tui/src/conversation_history_widget.rs b/codex-rs/tui/src/conversation_history_widget.rs index 3d2d1cd59b..16fc3f4874 100644 --- a/codex-rs/tui/src/conversation_history_widget.rs +++ b/codex-rs/tui/src/conversation_history_widget.rs @@ -183,12 +183,12 @@ impl ConversationHistoryWidget { self.add_to_history(HistoryCell::new_user_prompt(message)); } - pub fn add_agent_message(&mut self, message: String) { - self.add_to_history(HistoryCell::new_agent_message(message)); + pub fn add_agent_message(&mut self, config: &Config, message: String) { + self.add_to_history(HistoryCell::new_agent_message(config, message)); } - pub fn add_agent_reasoning(&mut self, text: String) { - self.add_to_history(HistoryCell::new_agent_reasoning(text)); + pub fn add_agent_reasoning(&mut self, config: &Config, text: String) { + self.add_to_history(HistoryCell::new_agent_reasoning(config, text)); } pub fn add_background_event(&mut self, message: String) { diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 066ed335df..fab9432724 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -155,19 +155,19 @@ impl HistoryCell { HistoryCell::UserPrompt { lines } } - pub(crate) fn new_agent_message(message: String) -> Self { + pub(crate) fn new_agent_message(config: &Config, message: String) -> Self { let mut lines: Vec> = Vec::new(); lines.push(Line::from("codex".magenta().bold())); - append_markdown(&message, &mut lines); + append_markdown(&message, &mut lines, config); lines.push(Line::from("")); HistoryCell::AgentMessage { lines } } - pub(crate) fn new_agent_reasoning(text: String) -> Self { + pub(crate) fn new_agent_reasoning(config: &Config, text: String) -> Self { let mut lines: Vec> = Vec::new(); lines.push(Line::from("thinking".magenta().italic())); - append_markdown(&text, &mut lines); + append_markdown(&text, &mut lines, config); lines.push(Line::from("")); HistoryCell::AgentReasoning { lines } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5e3ed9b6a0..a6849f62fb 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -19,6 +19,7 @@ mod app_event; mod app_event_sender; mod bottom_pane; mod chatwidget; +mod citation_regex; mod cli; mod conversation_history_widget; mod exec_command; diff --git a/codex-rs/tui/src/markdown.rs b/codex-rs/tui/src/markdown.rs index 9837f3e20c..afe668c072 100644 --- a/codex-rs/tui/src/markdown.rs +++ b/codex-rs/tui/src/markdown.rs @@ -1,8 +1,32 @@ +use codex_core::config::Config; +use codex_core::config::UriBasedFileOpener; use ratatui::text::Line; use ratatui::text::Span; +use std::borrow::Cow; +use std::path::Path; -pub(crate) fn append_markdown(markdown_source: &str, lines: &mut Vec>) { - let markdown = tui_markdown::from_str(markdown_source); +use crate::citation_regex::CITATION_REGEX; + +pub(crate) fn append_markdown( + markdown_source: &str, + lines: &mut Vec>, + config: &Config, +) { + append_markdown_with_opener_and_cwd(markdown_source, lines, config.file_opener, &config.cwd); +} + +fn append_markdown_with_opener_and_cwd( + markdown_source: &str, + lines: &mut Vec>, + file_opener: UriBasedFileOpener, + cwd: &Path, +) { + // Perform citation rewrite *before* feeding the string to the markdown + // renderer. When `file_opener` is absent we bypass the transformation to + // avoid unnecessary allocations. + let processed_markdown = rewrite_file_citations(markdown_source, file_opener, cwd); + + let markdown = tui_markdown::from_str(&processed_markdown); // `tui_markdown` returns a `ratatui::text::Text` where every `Line` borrows // from the input `message` string. Since the `HistoryCell` stores its lines @@ -28,3 +52,112 @@ pub(crate) fn append_markdown(markdown_source: &str, lines: &mut Vec://file: +/// ``` +fn rewrite_file_citations<'a>( + src: &'a str, + file_opener: UriBasedFileOpener, + cwd: &Path, +) -> Cow<'a, str> { + // Map enum values to the corresponding URI scheme strings. + let scheme: &str = match file_opener.get_scheme() { + Some(scheme) => scheme, + None => return Cow::Borrowed(src), + }; + + CITATION_REGEX.replace_all(src, |caps: ®ex::Captures<'_>| { + let file = &caps[1]; + let start_line = &caps[2]; + + // Resolve the path against `cwd` when it is relative. + let absolute_path = { + let p = Path::new(file); + let absolute_path = if p.is_absolute() { + path_clean::clean(p) + } else { + path_clean::clean(cwd.join(p)) + }; + // VS Code expects forward slashes even on Windows because URIs use + // `/` as the path separator. + absolute_path.to_string_lossy().replace('\\', "/") + }; + + // Render as a normal markdown link so the downstream renderer emits + // the hyperlink escape sequence (when supported by the terminal). + // + // In practice, sometimes multiple citations for the same file, but with a + // different line number, are shown sequentially, so we: + // - include the line number in the label to disambiguate them + // - add a space after the link to make it easier to read + format!("[{file}:{start_line}]({scheme}://file{absolute_path}:{start_line}) ") + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn citation_is_rewritten_with_absolute_path() { + let markdown = "See 【F:/src/main.rs†L42-L50】 for details."; + let cwd = Path::new("/workspace"); + let result = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); + + assert_eq!( + "See [/src/main.rs:42](vscode://file/src/main.rs:42) for details.", + result + ); + } + + #[test] + fn citation_is_rewritten_with_relative_path() { + let markdown = "Refer to 【F:lib/mod.rs†L5】 here."; + let cwd = Path::new("/home/user/project"); + let result = rewrite_file_citations(markdown, UriBasedFileOpener::Windsurf, cwd); + + assert_eq!( + "Refer to [lib/mod.rs:5](windsurf://file/home/user/project/lib/mod.rs:5) here.", + result + ); + } + + #[test] + fn citation_followed_by_space_so_they_do_not_run_together() { + let markdown = "TODOs on lines 【F:src/foo.rs†L24】【F:src/foo.rs†L42】"; + let cwd = Path::new("/home/user/project"); + let result = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); + + assert_eq!( + "TODOs on lines [src/foo.rs:24](vscode://file/home/user/project/src/foo.rs:24) [src/foo.rs:42](vscode://file/home/user/project/src/foo.rs:42) ", + result + ); + } + + #[test] + fn citation_unchanged_without_file_opener() { + let markdown = "Look at 【F:file.rs†L1】."; + let cwd = Path::new("/"); + let unchanged = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); + // The helper itself always rewrites – this test validates behaviour of + // append_markdown when `file_opener` is None. + let mut out = Vec::new(); + append_markdown_with_opener_and_cwd(markdown, &mut out, UriBasedFileOpener::None, cwd); + // Convert lines back to string for comparison. + let rendered: String = out + .iter() + .flat_map(|l| l.spans.iter()) + .map(|s| s.content.clone()) + .collect::>() + .join(""); + assert_eq!(markdown, rendered); + // Ensure helper rewrites. + assert_ne!(markdown, unchanged); + } +} From 89ed7e54ac08c828131cba4b02a789c03961993d Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 16 May 2025 12:00:56 -0700 Subject: [PATCH 2/2] chore: refactor handle_function_call() into smaller functions --- codex-rs/core/src/codex.rs | 558 ++++++++++++++++++++----------------- 1 file changed, 299 insertions(+), 259 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index e3cd1a7ad7..4d164adc7a 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -40,6 +40,7 @@ use crate::config::Config; use crate::conversation_history::ConversationHistory; use crate::error::CodexErr; use crate::error::Result as CodexResult; +use crate::error::SandboxErr; use crate::exec::ExecParams; use crate::exec::ExecToolCallOutput; use crate::exec::SandboxType; @@ -1042,265 +1043,7 @@ async fn handle_function_call( ) -> ResponseInputItem { match name.as_str() { "container.exec" | "shell" => { - // parse command - let params: ExecParams = match serde_json::from_str::(&arguments) { - Ok(shell_tool_call_params) => ExecParams { - command: shell_tool_call_params.command, - cwd: sess.resolve_path(shell_tool_call_params.workdir.clone()), - timeout_ms: shell_tool_call_params.timeout_ms, - }, - Err(e) => { - // allow model to re-sample - let output = ResponseInputItem::FunctionCallOutput { - call_id, - output: crate::models::FunctionCallOutputPayload { - content: format!("failed to parse function arguments: {e}"), - success: None, - }, - }; - return output; - } - }; - - // check if this was a patch, and apply it if so - match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { - MaybeApplyPatchVerified::Body(changes) => { - return apply_patch(sess, sub_id, call_id, changes).await; - } - MaybeApplyPatchVerified::CorrectnessError(parse_error) => { - // It looks like an invocation of `apply_patch`, but we - // could not resolve it into a patch that would apply - // cleanly. Return to model for resample. - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("error: {parse_error:#}"), - success: None, - }, - }; - } - MaybeApplyPatchVerified::ShellParseError(error) => { - trace!("Failed to parse shell command, {error:?}"); - } - MaybeApplyPatchVerified::NotApplyPatch => (), - } - - // safety checks - let safety = { - let state = sess.state.lock().unwrap(); - assess_command_safety( - ¶ms.command, - sess.approval_policy, - &sess.sandbox_policy, - &state.approved_commands, - ) - }; - let sandbox_type = match safety { - SafetyCheck::AutoApprove { sandbox_type } => sandbox_type, - SafetyCheck::AskUser => { - let rx_approve = sess - .request_command_approval( - sub_id.clone(), - params.command.clone(), - params.cwd.clone(), - None, - ) - .await; - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved => (), - ReviewDecision::ApprovedForSession => { - sess.add_approved_command(params.command.clone()); - } - ReviewDecision::Denied | ReviewDecision::Abort => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: crate::models::FunctionCallOutputPayload { - content: "exec command rejected by user".to_string(), - success: None, - }, - }; - } - } - // No sandboxing is applied because the user has given - // explicit approval. Often, we end up in this case because - // the command cannot be run in a sandbox, such as - // installing a new dependency that requires network access. - SandboxType::None - } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: crate::models::FunctionCallOutputPayload { - content: format!("exec command rejected: {reason}"), - success: None, - }, - }; - } - }; - - sess.notify_exec_command_begin(&sub_id, &call_id, ¶ms) - .await; - - let output_result = process_exec_tool_call( - params.clone(), - sandbox_type, - sess.ctrl_c.clone(), - &sess.sandbox_policy, - ) - .await; - - match output_result { - Ok(output) => { - let ExecToolCallOutput { - exit_code, - stdout, - stderr, - duration, - } = output; - - sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) - .await; - - let is_success = exit_code == 0; - let content = format_exec_output( - if is_success { &stdout } else { &stderr }, - exit_code, - duration, - ); - - ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content, - success: Some(is_success), - }, - } - } - Err(CodexErr::Sandbox(e)) => { - // Early out if the user never wants to be asked for approval; just return to the model immediately - if sess.approval_policy == AskForApproval::Never { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!( - "failed in sandbox {:?} with execution error: {e}", - sandbox_type - ), - success: Some(false), - }, - }; - } - - // Ask the user to retry without sandbox - sess.notify_background_event(&sub_id, format!("Execution failed: {e}")) - .await; - - let rx_approve = sess - .request_command_approval( - sub_id.clone(), - params.command.clone(), - params.cwd.clone(), - Some("command failed; retry without sandbox?".to_string()), - ) - .await; - - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { - // Persist this command as pre‑approved for the - // remainder of the session so future - // executions skip the sandbox directly. - // TODO(ragona): Isn't this a bug? It always saves the command in an | fork? - sess.add_approved_command(params.command.clone()); - // Inform UI we are retrying without sandbox. - sess.notify_background_event( - &sub_id, - "retrying command without sandbox", - ) - .await; - - // Emit a fresh Begin event so progress bars reset. - let retry_call_id = format!("{call_id}-retry"); - sess.notify_exec_command_begin(&sub_id, &retry_call_id, ¶ms) - .await; - - // This is an escalated retry; the policy will not be - // examined and the sandbox has been set to `None`. - let retry_output_result = process_exec_tool_call( - params, - SandboxType::None, - sess.ctrl_c.clone(), - &sess.sandbox_policy, - ) - .await; - - match retry_output_result { - Ok(retry_output) => { - let ExecToolCallOutput { - exit_code, - stdout, - stderr, - duration, - } = retry_output; - - sess.notify_exec_command_end( - &sub_id, - &retry_call_id, - &stdout, - &stderr, - exit_code, - ) - .await; - - let is_success = exit_code == 0; - let content = format_exec_output( - if is_success { &stdout } else { &stderr }, - exit_code, - duration, - ); - - ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content, - success: Some(is_success), - }, - } - } - Err(e) => { - // Handle retry failure - ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("retry failed: {e}"), - success: None, - }, - } - } - } - } - ReviewDecision::Denied | ReviewDecision::Abort => { - // Fall through to original failure handling. - ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: "exec command rejected by user".to_string(), - success: None, - }, - } - } - } - } - Err(e) => { - // Handle non-sandbox errors - ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("execution error: {e}"), - success: None, - }, - } - } - } + handle_container_exec_function_call(sess, sub_id, arguments, call_id).await } _ => { match try_parse_fully_qualified_tool_name(&name) { @@ -1327,6 +1070,303 @@ async fn handle_function_call( } } +fn parse_container_exec_arguments( + arguments: String, + sess: &Session, + call_id: &str, +) -> Result { + // parse command + match serde_json::from_str::(&arguments) { + Ok(shell_tool_call_params) => Ok(ExecParams { + command: shell_tool_call_params.command, + cwd: sess.resolve_path(shell_tool_call_params.workdir.clone()), + timeout_ms: shell_tool_call_params.timeout_ms, + }), + Err(e) => { + // allow model to re-sample + let output = ResponseInputItem::FunctionCallOutput { + call_id: call_id.to_string(), + output: crate::models::FunctionCallOutputPayload { + content: format!("failed to parse function arguments: {e}"), + success: None, + }, + }; + Err(output) + } + } +} + +async fn handle_container_exec_function_call( + sess: &Session, + sub_id: String, + arguments: String, + call_id: String, +) -> ResponseInputItem { + let params = match parse_container_exec_arguments(arguments, sess, &call_id) { + Ok(params) => params, + Err(output) => { + return output; + } + }; + + handle_container_exec_with_params(params, sess, sub_id, call_id).await +} + +async fn handle_container_exec_with_params( + params: ExecParams, + sess: &Session, + sub_id: String, + call_id: String, +) -> ResponseInputItem { + // check if this was a patch, and apply it if so + match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { + MaybeApplyPatchVerified::Body(changes) => { + return apply_patch(sess, sub_id, call_id, changes).await; + } + MaybeApplyPatchVerified::CorrectnessError(parse_error) => { + // It looks like an invocation of `apply_patch`, but we + // could not resolve it into a patch that would apply + // cleanly. Return to model for resample. + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("error: {parse_error:#}"), + success: None, + }, + }; + } + MaybeApplyPatchVerified::ShellParseError(error) => { + trace!("Failed to parse shell command, {error:?}"); + } + MaybeApplyPatchVerified::NotApplyPatch => (), + } + + // safety checks + let safety = { + let state = sess.state.lock().unwrap(); + assess_command_safety( + ¶ms.command, + sess.approval_policy, + &sess.sandbox_policy, + &state.approved_commands, + ) + }; + let sandbox_type = match safety { + SafetyCheck::AutoApprove { sandbox_type } => sandbox_type, + SafetyCheck::AskUser => { + let rx_approve = sess + .request_command_approval( + sub_id.clone(), + params.command.clone(), + params.cwd.clone(), + None, + ) + .await; + match rx_approve.await.unwrap_or_default() { + ReviewDecision::Approved => (), + ReviewDecision::ApprovedForSession => { + sess.add_approved_command(params.command.clone()); + } + ReviewDecision::Denied | ReviewDecision::Abort => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: crate::models::FunctionCallOutputPayload { + content: "exec command rejected by user".to_string(), + success: None, + }, + }; + } + } + // No sandboxing is applied because the user has given + // explicit approval. Often, we end up in this case because + // the command cannot be run in a sandbox, such as + // installing a new dependency that requires network access. + SandboxType::None + } + SafetyCheck::Reject { reason } => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: crate::models::FunctionCallOutputPayload { + content: format!("exec command rejected: {reason}"), + success: None, + }, + }; + } + }; + + sess.notify_exec_command_begin(&sub_id, &call_id, ¶ms) + .await; + + let output_result = process_exec_tool_call( + params.clone(), + sandbox_type, + sess.ctrl_c.clone(), + &sess.sandbox_policy, + ) + .await; + + match output_result { + Ok(output) => { + let ExecToolCallOutput { + exit_code, + stdout, + stderr, + duration, + } = output; + + sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) + .await; + + let is_success = exit_code == 0; + let content = format_exec_output( + if is_success { &stdout } else { &stderr }, + exit_code, + duration, + ); + + ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content, + success: Some(is_success), + }, + } + } + Err(CodexErr::Sandbox(error)) => { + handle_sanbox_error(error, sandbox_type, params, sess, sub_id, call_id).await + } + Err(e) => { + // Handle non-sandbox errors + ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("execution error: {e}"), + success: None, + }, + } + } + } +} + +async fn handle_sanbox_error( + error: SandboxErr, + sandbox_type: SandboxType, + params: ExecParams, + sess: &Session, + sub_id: String, + call_id: String, +) -> ResponseInputItem { + // Early out if the user never wants to be asked for approval; just return to the model immediately + if sess.approval_policy == AskForApproval::Never { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!( + "failed in sandbox {:?} with execution error: {error}", + sandbox_type + ), + success: Some(false), + }, + }; + } + + // Ask the user to retry without sandbox + sess.notify_background_event(&sub_id, format!("Execution failed: {error}")) + .await; + + let rx_approve = sess + .request_command_approval( + sub_id.clone(), + params.command.clone(), + params.cwd.clone(), + Some("command failed; retry without sandbox?".to_string()), + ) + .await; + + match rx_approve.await.unwrap_or_default() { + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { + // Persist this command as pre‑approved for the + // remainder of the session so future + // executions skip the sandbox directly. + // TODO(ragona): Isn't this a bug? It always saves the command in an | fork? + sess.add_approved_command(params.command.clone()); + // Inform UI we are retrying without sandbox. + sess.notify_background_event(&sub_id, "retrying command without sandbox") + .await; + + // Emit a fresh Begin event so progress bars reset. + let retry_call_id = format!("{call_id}-retry"); + sess.notify_exec_command_begin(&sub_id, &retry_call_id, ¶ms) + .await; + + // This is an escalated retry; the policy will not be + // examined and the sandbox has been set to `None`. + let retry_output_result = process_exec_tool_call( + params, + SandboxType::None, + sess.ctrl_c.clone(), + &sess.sandbox_policy, + ) + .await; + + match retry_output_result { + Ok(retry_output) => { + let ExecToolCallOutput { + exit_code, + stdout, + stderr, + duration, + } = retry_output; + + sess.notify_exec_command_end( + &sub_id, + &retry_call_id, + &stdout, + &stderr, + exit_code, + ) + .await; + + let is_success = exit_code == 0; + let content = format_exec_output( + if is_success { &stdout } else { &stderr }, + exit_code, + duration, + ); + + ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content, + success: Some(is_success), + }, + } + } + Err(e) => { + // Handle retry failure + ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("retry failed: {e}"), + success: None, + }, + } + } + } + } + ReviewDecision::Denied | ReviewDecision::Abort => { + // Fall through to original failure handling. + ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: "exec command rejected by user".to_string(), + success: None, + }, + } + } + } +} + async fn apply_patch( sess: &Session, sub_id: String,