diff --git a/SUMMARY.md b/SUMMARY.md new file mode 100644 index 0000000000..5cdecdbf4c --- /dev/null +++ b/SUMMARY.md @@ -0,0 +1,21 @@ +You are a summarization assistant. A conversation follows between a user and a coding-focused AI (Codex). Your task is to generate a clear summary capturing: + +• High-level objective or problem being solved +• Key instructions or design decisions given by the user +• Main code actions or behaviors from the AI +• Important variables, functions, modules, or outputs discussed +• Any unresolved questions or next steps + +Produce the summary in a structured format like: + +**Objective:** … + +**User instructions:** … (bulleted) + +**AI actions / code behavior:** … (bulleted) + +**Important entities:** … (e.g. function names, variables, files) + +**Open issues / next steps:** … (if any) + +**Summary (concise):** (one or two sentences) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 120050c227..9123bd6374 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -793,6 +793,7 @@ dependencies = [ "reqwest", "serde", "serde_json", + "tempfile", "tokio", ] @@ -2642,6 +2643,7 @@ version = "0.0.0" dependencies = [ "anyhow", "assert_cmd", + "codex-core", "codex-mcp-server", "mcp-types", "pretty_assertions", @@ -2649,6 +2651,7 @@ dependencies = [ "shlex", "tempfile", "tokio", + "uuid", "wiremock", ] diff --git a/codex-rs/chatgpt/src/chatgpt_token.rs b/codex-rs/chatgpt/src/chatgpt_token.rs index 55ebc22a08..55b6886c59 100644 --- a/codex-rs/chatgpt/src/chatgpt_token.rs +++ b/codex-rs/chatgpt/src/chatgpt_token.rs @@ -18,7 +18,7 @@ pub fn set_chatgpt_token_data(value: TokenData) { /// Initialize the ChatGPT token from auth.json file pub async fn init_chatgpt_token_from_auth(codex_home: &Path) -> std::io::Result<()> { - let auth = codex_login::load_auth(codex_home)?; + let auth = codex_login::load_auth(codex_home, true)?; if let Some(auth) = auth { let token_data = auth.get_token_data().await?; set_chatgpt_token_data(token_data); diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index 905b746168..7f0983cbc6 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -4,10 +4,10 @@ use codex_common::CliConfigOverrides; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config_types::SandboxMode; -use codex_core::exec::StdioPolicy; use codex_core::exec::spawn_command_under_linux_sandbox; -use codex_core::exec::spawn_command_under_seatbelt; use codex_core::exec_env::create_env; +use codex_core::seatbelt::spawn_command_under_seatbelt; +use codex_core::spawn::StdioPolicy; use crate::LandlockCommand; use crate::SeatbeltCommand; diff --git a/codex-rs/cli/src/login.rs b/codex-rs/cli/src/login.rs index 390c310030..4fa13f0cc6 100644 --- a/codex-rs/cli/src/login.rs +++ b/codex-rs/cli/src/login.rs @@ -1,8 +1,12 @@ +use std::env; + use codex_common::CliConfigOverrides; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_login::AuthMode; +use codex_login::OPENAI_API_KEY_ENV_VAR; use codex_login::load_auth; +use codex_login::login_with_api_key; use codex_login::login_with_chatgpt; pub async fn run_login_with_chatgpt(cli_config_overrides: CliConfigOverrides) -> ! { @@ -21,14 +25,40 @@ pub async fn run_login_with_chatgpt(cli_config_overrides: CliConfigOverrides) -> } } +pub async fn run_login_with_api_key( + cli_config_overrides: CliConfigOverrides, + api_key: String, +) -> ! { + let config = load_config_or_exit(cli_config_overrides); + + match login_with_api_key(&config.codex_home, &api_key) { + Ok(_) => { + eprintln!("Successfully logged in"); + std::process::exit(0); + } + Err(e) => { + eprintln!("Error logging in: {e}"); + std::process::exit(1); + } + } +} + pub async fn run_login_status(cli_config_overrides: CliConfigOverrides) -> ! { let config = load_config_or_exit(cli_config_overrides); - match load_auth(&config.codex_home) { + match load_auth(&config.codex_home, true) { Ok(Some(auth)) => match auth.mode { AuthMode::ApiKey => { if let Some(api_key) = auth.api_key.as_deref() { eprintln!("Logged in using an API key - {}", safe_format_key(api_key)); + + if let Ok(env_api_key) = env::var(OPENAI_API_KEY_ENV_VAR) { + if env_api_key == api_key { + eprintln!( + " API loaded from OPENAI_API_KEY environment variable or .env file" + ); + } + } } else { eprintln!("Logged in using an API key"); } diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index c5fd69f9cd..27f8312193 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -8,6 +8,7 @@ use codex_chatgpt::apply_command::run_apply_command; use codex_cli::LandlockCommand; use codex_cli::SeatbeltCommand; use codex_cli::login::run_login_status; +use codex_cli::login::run_login_with_api_key; use codex_cli::login::run_login_with_chatgpt; use codex_cli::proto; use codex_common::CliConfigOverrides; @@ -92,6 +93,9 @@ struct LoginCommand { #[clap(skip)] config_overrides: CliConfigOverrides, + #[arg(long = "api-key", value_name = "API_KEY")] + api_key: Option, + #[command(subcommand)] action: Option, } @@ -133,7 +137,11 @@ async fn cli_main(codex_linux_sandbox_exe: Option) -> anyhow::Result<() run_login_status(login_cli.config_overrides).await; } None => { - run_login_with_chatgpt(login_cli.config_overrides).await; + if let Some(api_key) = login_cli.api_key { + run_login_with_api_key(login_cli.config_overrides, api_key).await; + } else { + run_login_with_chatgpt(login_cli.config_overrides).await; + } } } } diff --git a/codex-rs/cli/src/proto.rs b/codex-rs/cli/src/proto.rs index 291e1680f1..9f9a94ed4d 100644 --- a/codex-rs/cli/src/proto.rs +++ b/codex-rs/cli/src/proto.rs @@ -36,7 +36,7 @@ pub async fn run_main(opts: ProtoCli) -> anyhow::Result<()> { .map_err(anyhow::Error::msg)?; let config = Config::load_with_cli_overrides(overrides_vec, ConfigOverrides::default())?; - let auth = load_auth(&config.codex_home)?; + let auth = load_auth(&config.codex_home, true)?; let ctrl_c = notify_on_sigint(); let CodexSpawnOk { codex, .. } = Codex::spawn(config, auth, ctrl_c.clone()).await?; let codex = Arc::new(codex); diff --git a/codex-rs/core/prompt.md b/codex-rs/core/prompt.md index 66cd55b628..0a4578270a 100644 --- a/codex-rs/core/prompt.md +++ b/codex-rs/core/prompt.md @@ -96,3 +96,12 @@ You can invoke apply_patch like: ``` shell {"command":["apply_patch","*** Begin Patch\n*** Add File: hello.txt\n+Hello, world!\n*** End Patch\n"]} ``` + +Plan updates + +A tool named `update_plan` is available. Use it to keep an up‑to‑date, step‑by‑step plan for the task so you can follow your progress. When making your plans, keep in mind that you are a deployed coding agent - `update_plan` calls should not involve doing anything that you aren't capable of doing. For example, `update_plan` calls should NEVER contain tasks to merge your own pull requests. Only stop to ask the user if you genuinely need their feedback on a change. + +- At the start of the task, call `update_plan` with an initial plan: a short list of 1‑sentence steps with a `status` for each step (`pending`, `in_progress`, or `completed`). There should always be exactly one `in_progress` step until everything is done. +- Whenever you finish a step, call `update_plan` again, marking the finished step as `completed` and the next step as `in_progress`. +- If your plan needs to change, call `update_plan` with the revised steps and include an `explanation` describing the change. +- When all steps are complete, make a final `update_plan` call with all steps marked `completed`. diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index f116c790ab..dc11aed023 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,19 +1,12 @@ use crate::codex::Session; use crate::models::FunctionCallOutputPayload; use crate::models::ResponseInputItem; -use crate::protocol::Event; -use crate::protocol::EventMsg; use crate::protocol::FileChange; -use crate::protocol::PatchApplyBeginEvent; -use crate::protocol::PatchApplyEndEvent; use crate::protocol::ReviewDecision; use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; -use anyhow::Context; -use codex_apply_patch::AffectedPaths; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; -use codex_apply_patch::print_summary; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; @@ -26,12 +19,18 @@ pub(crate) enum InternalApplyPatchInvocation { /// result to use with the `shell` function call that contained `apply_patch`. Output(ResponseInputItem), - /// The `apply_patch` call was auto-approved, which means that, on the - /// surface, it appears to be safe, but it should be run in a sandbox if the - /// user has configured one because a path being written could be a hard - /// link to a file outside the writable folders, so only the sandbox can - /// faithfully prevent the write in that case. - DelegateToExec(ApplyPatchAction), + /// The `apply_patch` call was approved, either automatically because it + /// appears that it should be allowed based on the user's sandbox policy + /// *or* because the user explicitly approved it. In either case, we use + /// exec with [`CODEX_APPLY_PATCH_ARG1`] to realize the `apply_patch` call, + /// but [`ApplyPatchExec::auto_approved`] is used to determine the sandbox + /// used with the `exec()`. + DelegateToExec(ApplyPatchExec), +} + +pub(crate) struct ApplyPatchExec { + pub(crate) action: ApplyPatchAction, + pub(crate) user_explicitly_approved_this_action: bool, } impl From for InternalApplyPatchInvocation { @@ -52,254 +51,57 @@ pub(crate) async fn apply_patch( guard.clone() }; - let auto_approved = match assess_patch_safety( + match assess_patch_safety( &action, sess.approval_policy, &writable_roots_snapshot, &sess.cwd, ) { SafetyCheck::AutoApprove { .. } => { - return InternalApplyPatchInvocation::DelegateToExec(action); + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + user_explicitly_approved_this_action: false, + }) } SafetyCheck::AskUser => { // Compute a readable summary of path changes to include in the // approval request so the user can make an informed decision. + // + // Note that it might be worth expanding this approval request to + // give the user the option to expand the set of writable roots so + // that similar patches can be auto-approved in the future during + // this session. let rx_approve = sess .request_patch_approval(sub_id.to_owned(), call_id.to_owned(), &action, None, None) .await; match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + user_explicitly_approved_this_action: true, + }) + } ReviewDecision::Denied | ReviewDecision::Abort => { - return ResponseInputItem::FunctionCallOutput { + ResponseInputItem::FunctionCallOutput { call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: "patch rejected by user".to_string(), success: Some(false), }, } - .into(); + .into() } } } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id: call_id.to_owned(), - output: FunctionCallOutputPayload { - content: format!("patch rejected: {reason}"), - success: Some(false), - }, - } - .into(); - } - }; - - // Verify write permissions before touching the filesystem. - let writable_snapshot = { - #[allow(clippy::unwrap_used)] - sess.writable_roots.lock().unwrap().clone() - }; - - if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - - let rx = sess - .request_patch_approval( - sub_id.to_owned(), - call_id.to_owned(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - - if !matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - return ResponseInputItem::FunctionCallOutput { - call_id: call_id.to_owned(), - output: FunctionCallOutputPayload { - content: "patch rejected by user".to_string(), - success: Some(false), - }, - } - .into(); - } - - // user approved, extend writable roots for this session - #[allow(clippy::unwrap_used)] - sess.writable_roots.lock().unwrap().push(root); - } - - let _ = sess - .tx_event - .send(Event { - id: sub_id.to_owned(), - msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id: call_id.to_owned(), - auto_approved, - changes: convert_apply_patch_to_protocol(&action), - }), - }) - .await; - - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - // Enforce writable roots. If a write is blocked, collect offending root - // and prompt the user to extend permissions. - let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr); - - if let Err(err) = &result { - if err.kind() == std::io::ErrorKind::PermissionDenied { - // Determine first offending path. - let offending_opt = action - .changes() - .iter() - .flat_map(|(path, change)| match change { - ApplyPatchFileChange::Add { .. } => vec![path.as_ref()], - ApplyPatchFileChange::Delete => vec![path.as_ref()], - ApplyPatchFileChange::Update { - move_path: Some(move_path), - .. - } => { - vec![path.as_ref(), move_path.as_ref()] - } - ApplyPatchFileChange::Update { - move_path: None, .. - } => vec![path.as_ref()], - }) - .find_map(|path: &Path| { - // ApplyPatchAction promises to guarantee absolute paths. - if !path.is_absolute() { - panic!("apply_patch invariant failed: path is not absolute: {path:?}"); - } - - let writable = { - #[allow(clippy::unwrap_used)] - let roots = sess.writable_roots.lock().unwrap(); - roots.iter().any(|root| path.starts_with(root)) - }; - if writable { - None - } else { - Some(path.to_path_buf()) - } - }); - - if let Some(offending) = offending_opt { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - let rx = sess - .request_patch_approval( - sub_id.to_owned(), - call_id.to_owned(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - if matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - // Extend writable roots. - #[allow(clippy::unwrap_used)] - sess.writable_roots.lock().unwrap().push(root); - stdout.clear(); - stderr.clear(); - result = apply_changes_from_apply_patch_and_report( - &action, - &mut stdout, - &mut stderr, - ); - } - } - } - } - - // Emit PatchApplyEnd event. - let success_flag = result.is_ok(); - let _ = sess - .tx_event - .send(Event { - id: sub_id.to_owned(), - msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { - call_id: call_id.to_owned(), - stdout: String::from_utf8_lossy(&stdout).to_string(), - stderr: String::from_utf8_lossy(&stderr).to_string(), - success: success_flag, - }), - }) - .await; - - let item = match result { - Ok(_) => ResponseInputItem::FunctionCallOutput { + SafetyCheck::Reject { reason } => ResponseInputItem::FunctionCallOutput { call_id: call_id.to_owned(), output: FunctionCallOutputPayload { - content: String::from_utf8_lossy(&stdout).to_string(), - success: None, - }, - }, - Err(e) => ResponseInputItem::FunctionCallOutput { - call_id: call_id.to_owned(), - output: FunctionCallOutputPayload { - content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), + content: format!("patch rejected: {reason}"), success: Some(false), }, - }, - }; - InternalApplyPatchInvocation::Output(item) -} - -/// Return the first path in `hunks` that is NOT under any of the -/// `writable_roots` (after normalising). If all paths are acceptable, -/// returns None. -fn first_offending_path( - action: &ApplyPatchAction, - writable_roots: &[PathBuf], - cwd: &Path, -) -> Option { - let changes = action.changes(); - for (path, change) in changes { - let candidate = match change { - ApplyPatchFileChange::Add { .. } => path, - ApplyPatchFileChange::Delete => path, - ApplyPatchFileChange::Update { move_path, .. } => move_path.as_ref().unwrap_or(path), - }; - - let abs = if candidate.is_absolute() { - candidate.clone() - } else { - cwd.join(candidate) - }; - - let mut allowed = false; - for root in writable_roots { - let root_abs = if root.is_absolute() { - root.clone() - } else { - cwd.join(root) - }; - if abs.starts_with(&root_abs) { - allowed = true; - break; - } - } - - if !allowed { - return Some(candidate.clone()); } + .into(), } - None } pub(crate) fn convert_apply_patch_to_protocol( @@ -327,85 +129,6 @@ pub(crate) fn convert_apply_patch_to_protocol( result } -fn apply_changes_from_apply_patch_and_report( - action: &ApplyPatchAction, - stdout: &mut impl std::io::Write, - stderr: &mut impl std::io::Write, -) -> std::io::Result<()> { - match apply_changes_from_apply_patch(action) { - Ok(affected_paths) => { - print_summary(&affected_paths, stdout)?; - } - Err(err) => { - writeln!(stderr, "{err:?}")?; - } - } - - Ok(()) -} - -fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { - let mut added: Vec = Vec::new(); - let mut modified: Vec = Vec::new(); - let mut deleted: Vec = Vec::new(); - - let changes = action.changes(); - for (path, change) in changes { - match change { - ApplyPatchFileChange::Add { content } => { - if let Some(parent) = path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", path.display()) - })?; - } - } - std::fs::write(path, content) - .with_context(|| format!("Failed to write file {}", path.display()))?; - added.push(path.clone()); - } - ApplyPatchFileChange::Delete => { - std::fs::remove_file(path) - .with_context(|| format!("Failed to delete file {}", path.display()))?; - deleted.push(path.clone()); - } - ApplyPatchFileChange::Update { - unified_diff: _unified_diff, - move_path, - new_content, - } => { - if let Some(move_path) = move_path { - if let Some(parent) = move_path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!( - "Failed to create parent directories for {}", - move_path.display() - ) - })?; - } - } - - std::fs::rename(path, move_path) - .with_context(|| format!("Failed to rename file {}", path.display()))?; - std::fs::write(move_path, new_content)?; - modified.push(move_path.clone()); - deleted.push(path.clone()); - } else { - std::fs::write(path, new_content)?; - modified.push(path.clone()); - } - } - } - } - - Ok(AffectedPaths { - added, - modified, - deleted, - }) -} - pub(crate) fn get_writable_roots(cwd: &Path) -> Vec { let mut writable_roots = Vec::new(); if cfg!(target_os = "macos") { diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 4e0e62c0f7..0f7f51c2fc 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -192,7 +192,7 @@ impl ModelClient { loop { attempt += 1; - let req_builder = self + let mut req_builder = self .client .post(format!("{base_url}/responses")) .header("OpenAI-Beta", "responses=experimental") @@ -201,6 +201,12 @@ impl ModelClient { .header(reqwest::header::ACCEPT, "text/event-stream") .json(&payload); + if auth.mode == AuthMode::ChatGPT { + if let Some(account_id) = auth.get_account_id().await { + req_builder = req_builder.header("chatgpt-account-id", account_id); + } + } + let req_builder = self.provider.apply_http_headers(req_builder); let res = req_builder.send().await; diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 308916341a..e43ee120b4 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4,7 +4,6 @@ use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; @@ -31,6 +30,7 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::apply_patch::ApplyPatchExec; use crate::apply_patch::CODEX_APPLY_PATCH_ARG1; use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; @@ -76,8 +76,11 @@ use crate::protocol::EventMsg; use crate::protocol::ExecApprovalRequestEvent; use crate::protocol::ExecCommandBeginEvent; use crate::protocol::ExecCommandEndEvent; +use crate::protocol::FileChange; use crate::protocol::InputItem; use crate::protocol::Op; +use crate::protocol::PatchApplyBeginEvent; +use crate::protocol::PatchApplyEndEvent; use crate::protocol::ReviewDecision; use crate::protocol::SandboxPolicy; use crate::protocol::SessionConfiguredEvent; @@ -361,20 +364,32 @@ impl Session { } } - async fn notify_exec_command_begin( - &self, - sub_id: &str, - call_id: &str, - command_for_display: Vec, - command_cwd: &Path, - ) { + async fn notify_exec_command_begin(&self, exec_command_context: ExecCommandContext) { + let ExecCommandContext { + sub_id, + call_id, + command_for_display, + cwd, + apply_patch, + } = exec_command_context; + let msg = match apply_patch { + Some(ApplyPatchCommandContext { + user_explicitly_approved_this_action, + changes, + }) => EventMsg::PatchApplyBegin(PatchApplyBeginEvent { + call_id, + auto_approved: !user_explicitly_approved_this_action, + changes, + }), + None => EventMsg::ExecCommandBegin(ExecCommandBeginEvent { + call_id, + command: command_for_display.clone(), + cwd, + }), + }; let event = Event { id: sub_id.to_string(), - msg: EventMsg::ExecCommandBegin(ExecCommandBeginEvent { - call_id: call_id.to_string(), - command: command_for_display, - cwd: command_cwd.to_path_buf(), - }), + msg, }; let _ = self.tx_event.send(event).await; } @@ -386,18 +401,33 @@ impl Session { stdout: &str, stderr: &str, exit_code: i32, + is_apply_patch: bool, ) { + // Because stdout and stderr could each be up to 100 KiB, we send + // truncated versions. const MAX_STREAM_OUTPUT: usize = 5 * 1024; // 5KiB + let stdout = stdout.chars().take(MAX_STREAM_OUTPUT).collect(); + let stderr = stderr.chars().take(MAX_STREAM_OUTPUT).collect(); + + let msg = if is_apply_patch { + EventMsg::PatchApplyEnd(PatchApplyEndEvent { + call_id: call_id.to_string(), + stdout, + stderr, + success: exit_code == 0, + }) + } else { + EventMsg::ExecCommandEnd(ExecCommandEndEvent { + call_id: call_id.to_string(), + stdout, + stderr, + exit_code, + }) + }; + let event = Event { id: sub_id.to_string(), - // Because stdout and stderr could each be up to 100 KiB, we send - // truncated versions. - msg: EventMsg::ExecCommandEnd(ExecCommandEndEvent { - call_id: call_id.to_string(), - stdout: stdout.chars().take(MAX_STREAM_OUTPUT).collect(), - stderr: stderr.chars().take(MAX_STREAM_OUTPUT).collect(), - exit_code, - }), + msg, }; let _ = self.tx_event.send(event).await; } @@ -415,6 +445,12 @@ impl Session { let _ = self.tx_event.send(event).await; } + /// Build the full turn input by concatenating the current conversation + /// history with additional items for this turn. + pub fn turn_input_with_history(&self, extra: Vec) -> Vec { + [self.state.lock().unwrap().history.contents(), extra].concat() + } + /// Returns the input if there was no task running to inject into pub fn inject_input(&self, input: Vec) -> Result<(), Vec> { let mut state = self.state.lock().unwrap(); @@ -505,6 +541,21 @@ impl State { } } +#[derive(Clone, Debug)] +pub(crate) struct ExecCommandContext { + pub(crate) sub_id: String, + pub(crate) call_id: String, + pub(crate) command_for_display: Vec, + pub(crate) cwd: PathBuf, + pub(crate) apply_patch: Option, +} + +#[derive(Clone, Debug)] +pub(crate) struct ApplyPatchCommandContext { + pub(crate) user_explicitly_approved_this_action: bool, + pub(crate) changes: HashMap, +} + /// A series of Turns in response to user input. pub(crate) struct AgentTask { sess: Arc, @@ -522,6 +573,25 @@ impl AgentTask { handle, } } + fn compact( + sess: Arc, + sub_id: String, + input: Vec, + compact_instructions: String, + ) -> Self { + let handle = tokio::spawn(run_compact_task( + Arc::clone(&sess), + sub_id.clone(), + input, + compact_instructions, + )) + .abort_handle(); + Self { + sess, + sub_id, + handle, + } + } fn abort(self) { if !self.handle.is_finished() { @@ -843,6 +913,31 @@ async fn submission_loop( } }); } + Op::Compact => { + let sess = match sess.as_ref() { + Some(sess) => sess, + None => { + send_no_session_event(sub.id).await; + continue; + } + }; + + // Create a summarization request as user input + const SUMMARIZATION_PROMPT: &str = include_str!("../../../SUMMARY.md"); + + // Attempt to inject input into current task + if let Err(items) = sess.inject_input(vec![InputItem::Text { + text: "Start Summarization".to_string(), + }]) { + let task = AgentTask::compact( + sess.clone(), + sub.id, + items, + SUMMARIZATION_PROMPT.to_string(), + ); + sess.set_task(task); + } + } Op::Shutdown => { info!("Shutting down Codex instance"); @@ -904,7 +999,7 @@ async fn run_task(sess: Arc, sub_id: String, input: Vec) { return; } - let initial_input_for_turn = ResponseInputItem::from(input); + let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); sess.record_conversation_items(&[initial_input_for_turn.clone().into()]) .await; @@ -925,8 +1020,7 @@ async fn run_task(sess: Arc, sub_id: String, input: Vec) { // conversation history on each turn. The rollout file, however, should // only record the new items that originated in this turn so that it // represents an append-only log without duplicates. - let turn_input: Vec = - [sess.state.lock().unwrap().history.contents(), pending_input].concat(); + let turn_input: Vec = sess.turn_input_with_history(pending_input); let turn_input_messages: Vec = turn_input .iter() @@ -1253,6 +1347,88 @@ async fn try_run_turn( } } +async fn run_compact_task( + sess: Arc, + sub_id: String, + input: Vec, + compact_instructions: String, +) { + let start_event = Event { + id: sub_id.clone(), + msg: EventMsg::TaskStarted, + }; + if sess.tx_event.send(start_event).await.is_err() { + return; + } + + let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); + let turn_input: Vec = + sess.turn_input_with_history(vec![initial_input_for_turn.clone().into()]); + + let prompt = Prompt { + input: turn_input, + user_instructions: None, + store: !sess.disable_response_storage, + extra_tools: HashMap::new(), + base_instructions_override: Some(compact_instructions.clone()), + }; + + let max_retries = sess.client.get_provider().stream_max_retries(); + let mut retries = 0; + + loop { + let attempt_result = drain_to_completed(&sess, &prompt).await; + + match attempt_result { + Ok(()) => break, + Err(CodexErr::Interrupted) => return, + Err(e) => { + if retries < max_retries { + retries += 1; + let delay = backoff(retries); + sess.notify_background_event( + &sub_id, + format!( + "stream error: {e}; retrying {retries}/{max_retries} in {delay:?}…" + ), + ) + .await; + tokio::time::sleep(delay).await; + continue; + } else { + let event = Event { + id: sub_id.clone(), + msg: EventMsg::Error(ErrorEvent { + message: e.to_string(), + }), + }; + sess.send_event(event).await; + return; + } + } + } + } + + sess.remove_task(&sub_id); + let event = Event { + id: sub_id.clone(), + msg: EventMsg::AgentMessage(AgentMessageEvent { + message: "Compact task completed".to_string(), + }), + }; + sess.send_event(event).await; + let event = Event { + id: sub_id.clone(), + msg: EventMsg::TaskComplete(TaskCompleteEvent { + last_agent_message: None, + }), + }; + sess.send_event(event).await; + + let mut state = sess.state.lock().unwrap(); + state.history.keep_last_messages(1); +} + async fn handle_response_item( sess: &Session, sub_id: &str, @@ -1453,35 +1629,39 @@ async fn handle_container_exec_with_params( call_id: String, ) -> ResponseInputItem { // check if this was a patch, and apply it if so - let apply_patch_action_for_exec = - match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { - MaybeApplyPatchVerified::Body(changes) => { - match apply_patch::apply_patch(sess, &sub_id, &call_id, changes).await { - InternalApplyPatchInvocation::Output(item) => return item, - InternalApplyPatchInvocation::DelegateToExec(action) => Some(action), + let apply_patch_exec = match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { + MaybeApplyPatchVerified::Body(changes) => { + match apply_patch::apply_patch(sess, &sub_id, &call_id, changes).await { + InternalApplyPatchInvocation::Output(item) => return item, + InternalApplyPatchInvocation::DelegateToExec(apply_patch_exec) => { + Some(apply_patch_exec) } } - 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:?}"); - None - } - MaybeApplyPatchVerified::NotApplyPatch => None, - }; + } + 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:?}"); + None + } + MaybeApplyPatchVerified::NotApplyPatch => None, + }; - let (params, safety, command_for_display) = match apply_patch_action_for_exec { - Some(ApplyPatchAction { patch, cwd, .. }) => { + let (params, safety, command_for_display) = match &apply_patch_exec { + Some(ApplyPatchExec { + action: ApplyPatchAction { patch, cwd, .. }, + user_explicitly_approved_this_action, + }) => { let path_to_codex = std::env::current_exe() .ok() .map(|p| p.to_string_lossy().to_string()); @@ -1501,13 +1681,22 @@ async fn handle_container_exec_with_params( CODEX_APPLY_PATCH_ARG1.to_string(), patch.clone(), ], - cwd, + cwd: cwd.clone(), timeout_ms: params.timeout_ms, env: HashMap::new(), }; - let safety = - assess_safety_for_untrusted_command(sess.approval_policy, &sess.sandbox_policy); - (params, safety, vec!["apply_patch".to_string(), patch]) + let safety = if *user_explicitly_approved_this_action { + SafetyCheck::AutoApprove { + sandbox_type: SandboxType::None, + } + } else { + assess_safety_for_untrusted_command(sess.approval_policy, &sess.sandbox_policy) + }; + ( + params, + safety, + vec!["apply_patch".to_string(), patch.clone()], + ) } None => { let safety = { @@ -1568,7 +1757,22 @@ async fn handle_container_exec_with_params( } }; - sess.notify_exec_command_begin(&sub_id, &call_id, command_for_display.clone(), ¶ms.cwd) + let exec_command_context = ExecCommandContext { + sub_id: sub_id.clone(), + call_id: call_id.clone(), + command_for_display: command_for_display.clone(), + cwd: params.cwd.clone(), + apply_patch: apply_patch_exec.map( + |ApplyPatchExec { + action, + user_explicitly_approved_this_action, + }| ApplyPatchCommandContext { + user_explicitly_approved_this_action, + changes: convert_apply_patch_to_protocol(&action), + }, + ), + }; + sess.notify_exec_command_begin(exec_command_context.clone()) .await; let params = maybe_run_with_user_profile(params, sess); @@ -1590,8 +1794,15 @@ async fn handle_container_exec_with_params( duration, } = output; - sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) - .await; + sess.notify_exec_command_end( + &sub_id, + &call_id, + &stdout, + &stderr, + exit_code, + exec_command_context.apply_patch.is_some(), + ) + .await; let is_success = exit_code == 0; let content = format_exec_output( @@ -1609,16 +1820,7 @@ async fn handle_container_exec_with_params( } } Err(CodexErr::Sandbox(error)) => { - handle_sandbox_error( - error, - sandbox_type, - params, - command_for_display, - sess, - sub_id, - call_id, - ) - .await + handle_sandbox_error(params, exec_command_context, error, sandbox_type, sess).await } Err(e) => { // Handle non-sandbox errors @@ -1634,14 +1836,17 @@ async fn handle_container_exec_with_params( } async fn handle_sandbox_error( + params: ExecParams, + exec_command_context: ExecCommandContext, error: SandboxErr, sandbox_type: SandboxType, - params: ExecParams, - command_for_display: Vec, sess: &Session, - sub_id: String, - call_id: String, ) -> ResponseInputItem { + let call_id = exec_command_context.call_id.clone(); + let sub_id = exec_command_context.sub_id.clone(); + let cwd = exec_command_context.cwd.clone(); + let is_apply_patch = exec_command_context.apply_patch.is_some(); + // 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 { @@ -1671,7 +1876,7 @@ async fn handle_sandbox_error( sub_id.clone(), call_id.clone(), params.command.clone(), - params.cwd.clone(), + cwd.clone(), Some("command failed; retry without sandbox?".to_string()), ) .await; @@ -1687,8 +1892,7 @@ async fn handle_sandbox_error( sess.notify_background_event(&sub_id, "retrying command without sandbox") .await; - sess.notify_exec_command_begin(&sub_id, &call_id, command_for_display, ¶ms.cwd) - .await; + sess.notify_exec_command_begin(exec_command_context).await; // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. @@ -1710,8 +1914,15 @@ async fn handle_sandbox_error( duration, } = retry_output; - sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) - .await; + sess.notify_exec_command_end( + &sub_id, + &call_id, + &stdout, + &stderr, + exit_code, + is_apply_patch, + ) + .await; let is_success = exit_code == 0; let content = format_exec_output( @@ -1801,3 +2012,20 @@ fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) -> Option CodexResult<()> { + let mut stream = sess.client.clone().stream(prompt).await?; + loop { + let maybe_event = stream.next().await; + let Some(event) = maybe_event else { + return Err(CodexErr::Stream( + "stream closed before response.completed".into(), + )); + }; + match event { + Ok(ResponseEvent::Completed { .. }) => return Ok(()), + Ok(_) => continue, + Err(e) => return Err(e), + } + } +} diff --git a/codex-rs/core/src/codex_wrapper.rs b/codex-rs/core/src/codex_wrapper.rs index 1e26a9ebed..eeb4a7b470 100644 --- a/codex-rs/core/src/codex_wrapper.rs +++ b/codex-rs/core/src/codex_wrapper.rs @@ -26,7 +26,7 @@ pub struct CodexConversation { /// that callers can surface the information to the UI. pub async fn init_codex(config: Config) -> anyhow::Result { let ctrl_c = notify_on_sigint(); - let auth = load_auth(&config.codex_home)?; + let auth = load_auth(&config.codex_home, true)?; let CodexSpawnOk { codex, init_id, diff --git a/codex-rs/core/src/conversation_history.rs b/codex-rs/core/src/conversation_history.rs index 4cd989cbd9..f5254f339e 100644 --- a/codex-rs/core/src/conversation_history.rs +++ b/codex-rs/core/src/conversation_history.rs @@ -30,6 +30,34 @@ impl ConversationHistory { } } } + + pub(crate) fn keep_last_messages(&mut self, n: usize) { + if n == 0 { + self.items.clear(); + return; + } + + // Collect the last N message items (assistant/user), newest to oldest. + let mut kept: Vec = Vec::with_capacity(n); + for item in self.items.iter().rev() { + if let ResponseItem::Message { role, content, .. } = item { + kept.push(ResponseItem::Message { + // we need to remove the id or the model will complain that messages are sent without + // their reasonings + id: None, + role: role.clone(), + content: content.clone(), + }); + if kept.len() == n { + break; + } + } + } + + // Preserve chronological order (oldest to newest) within the kept slice. + kept.reverse(); + self.items = kept; + } } /// Anything that is not a system message or "reasoning" message is considered diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 230c4ec134..06416e6768 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -6,7 +6,6 @@ use std::io; use std::path::Path; use std::path::PathBuf; use std::process::ExitStatus; -use std::process::Stdio; use std::sync::Arc; use std::time::Duration; use std::time::Instant; @@ -15,14 +14,15 @@ use tokio::io::AsyncRead; use tokio::io::AsyncReadExt; use tokio::io::BufReader; use tokio::process::Child; -use tokio::process::Command; use tokio::sync::Notify; -use tracing::trace; use crate::error::CodexErr; use crate::error::Result; use crate::error::SandboxErr; use crate::protocol::SandboxPolicy; +use crate::seatbelt::spawn_command_under_seatbelt; +use crate::spawn::StdioPolicy; +use crate::spawn::spawn_child_async; // Maximum we send for each stream, which is either: // - 10KiB OR @@ -37,24 +37,6 @@ const DEFAULT_TIMEOUT_MS: u64 = 10_000; const SIGKILL_CODE: i32 = 9; const TIMEOUT_CODE: i32 = 64; -const MACOS_SEATBELT_BASE_POLICY: &str = include_str!("seatbelt_base_policy.sbpl"); - -/// When working with `sandbox-exec`, only consider `sandbox-exec` in `/usr/bin` -/// to defend against an attacker trying to inject a malicious version on the -/// PATH. If /usr/bin/sandbox-exec has been tampered with, then the attacker -/// already has root access. -const MACOS_PATH_TO_SEATBELT_EXECUTABLE: &str = "/usr/bin/sandbox-exec"; - -/// Experimental environment variable that will be set to some non-empty value -/// if both of the following are true: -/// -/// 1. The process was spawned by Codex as part of a shell tool call. -/// 2. SandboxPolicy.has_full_network_access() was false for the tool call. -/// -/// We may try to have just one environment variable for all sandboxing -/// attributes, so this may change in the future. -pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED"; - #[derive(Debug, Clone)] pub struct ExecParams { pub command: Vec, @@ -168,27 +150,6 @@ pub async fn process_exec_tool_call( } } -pub async fn spawn_command_under_seatbelt( - command: Vec, - sandbox_policy: &SandboxPolicy, - cwd: PathBuf, - stdio_policy: StdioPolicy, - env: HashMap, -) -> std::io::Result { - let args = create_seatbelt_command_args(command, sandbox_policy, &cwd); - let arg0 = None; - spawn_child_async( - PathBuf::from(MACOS_PATH_TO_SEATBELT_EXECUTABLE), - args, - arg0, - cwd, - sandbox_policy, - stdio_policy, - env, - ) - .await -} - /// Spawn a shell tool command under the Linux Landlock+seccomp sandbox helper /// (codex-linux-sandbox). /// @@ -248,65 +209,6 @@ fn create_linux_sandbox_command_args( linux_cmd } -fn create_seatbelt_command_args( - command: Vec, - sandbox_policy: &SandboxPolicy, - cwd: &Path, -) -> Vec { - let (file_write_policy, extra_cli_args) = { - if sandbox_policy.has_full_disk_write_access() { - // Allegedly, this is more permissive than `(allow file-write*)`. - ( - r#"(allow file-write* (regex #"^/"))"#.to_string(), - Vec::::new(), - ) - } else { - let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); - let (writable_folder_policies, cli_args): (Vec, Vec) = writable_roots - .iter() - .enumerate() - .map(|(index, root)| { - let param_name = format!("WRITABLE_ROOT_{index}"); - let policy: String = format!("(subpath (param \"{param_name}\"))"); - let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); - (policy, cli_arg) - }) - .unzip(); - if writable_folder_policies.is_empty() { - ("".to_string(), Vec::::new()) - } else { - let file_write_policy = format!( - "(allow file-write*\n{}\n)", - writable_folder_policies.join(" ") - ); - (file_write_policy, cli_args) - } - } - }; - - let file_read_policy = if sandbox_policy.has_full_disk_read_access() { - "; allow read-only file operations\n(allow file-read*)" - } else { - "" - }; - - // TODO(mbolin): apply_patch calls must also honor the SandboxPolicy. - let network_policy = if sandbox_policy.has_full_network_access() { - "(allow network-outbound)\n(allow network-inbound)\n(allow system-socket)" - } else { - "" - }; - - let full_policy = format!( - "{MACOS_SEATBELT_BASE_POLICY}\n{file_read_policy}\n{file_write_policy}\n{network_policy}" - ); - let mut seatbelt_args: Vec = vec!["-p".to_string(), full_policy]; - seatbelt_args.extend(extra_cli_args); - seatbelt_args.push("--".to_string()); - seatbelt_args.extend(command); - seatbelt_args -} - #[derive(Debug)] pub struct RawExecToolCallOutput { pub exit_status: ExitStatus, @@ -352,90 +254,6 @@ async fn exec( consume_truncated_output(child, ctrl_c, timeout_ms).await } -#[derive(Debug, Clone, Copy)] -pub enum StdioPolicy { - RedirectForShellTool, - Inherit, -} - -/// Spawns the appropriate child process for the ExecParams and SandboxPolicy, -/// ensuring the args and environment variables used to create the `Command` -/// (and `Child`) honor the configuration. -/// -/// For now, we take `SandboxPolicy` as a parameter to spawn_child() because -/// we need to determine whether to set the -/// `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` environment variable. -async fn spawn_child_async( - program: PathBuf, - args: Vec, - #[cfg_attr(not(unix), allow(unused_variables))] arg0: Option<&str>, - cwd: PathBuf, - sandbox_policy: &SandboxPolicy, - stdio_policy: StdioPolicy, - env: HashMap, -) -> std::io::Result { - trace!( - "spawn_child_async: {program:?} {args:?} {arg0:?} {cwd:?} {sandbox_policy:?} {stdio_policy:?} {env:?}" - ); - - let mut cmd = Command::new(&program); - #[cfg(unix)] - cmd.arg0(arg0.map_or_else(|| program.to_string_lossy().to_string(), String::from)); - cmd.args(args); - cmd.current_dir(cwd); - cmd.env_clear(); - cmd.envs(env); - - if !sandbox_policy.has_full_network_access() { - cmd.env(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR, "1"); - } - - // If this Codex process dies (including being killed via SIGKILL), we want - // any child processes that were spawned as part of a `"shell"` tool call - // to also be terminated. - - // This relies on prctl(2), so it only works on Linux. - #[cfg(target_os = "linux")] - unsafe { - cmd.pre_exec(|| { - // This prctl call effectively requests, "deliver SIGTERM when my - // current parent dies." - if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 { - return Err(io::Error::last_os_error()); - } - - // Though if there was a race condition and this pre_exec() block is - // run _after_ the parent (i.e., the Codex process) has already - // exited, then the parent is the _init_ process (which will never - // die), so we should just terminate the child process now. - if libc::getppid() == 1 { - libc::raise(libc::SIGTERM); - } - Ok(()) - }); - } - - match stdio_policy { - StdioPolicy::RedirectForShellTool => { - // Do not create a file descriptor for stdin because otherwise some - // commands may hang forever waiting for input. For example, ripgrep has - // a heuristic where it may try to read from stdin as explained here: - // https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103 - cmd.stdin(Stdio::null()); - - cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); - } - StdioPolicy::Inherit => { - // Inherit stdin, stdout, and stderr from the parent process. - cmd.stdin(Stdio::inherit()) - .stdout(Stdio::inherit()) - .stderr(Stdio::inherit()); - } - } - - cmd.kill_on_drop(true).spawn() -} - /// Consumes the output of a child process, truncating it so it is suitable for /// use as the output of a `shell` tool call. Also enforces specified timeout. pub(crate) async fn consume_truncated_output( diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 054abd742a..a33e185afb 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -39,7 +39,9 @@ mod project_doc; pub mod protocol; mod rollout; mod safety; +pub mod seatbelt; pub mod shell; +pub mod spawn; mod user_notification; pub mod util; diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 36152b4cb8..120a45bc9e 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -121,6 +121,10 @@ pub enum Op { /// Request a single history entry identified by `log_id` + `offset`. GetHistoryEntryRequest { offset: usize, log_id: u64 }, + /// Request the agent to summarize the current conversation context. + /// The agent will use its existing context (either conversation history or previous response id) + /// to generate a summary which will be returned as an AgentMessage event. + Compact, /// Request to shut down codex instance. Shutdown, } diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index f9bc27e058..224705f8f3 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -41,11 +41,13 @@ pub fn assess_patch_safety( } } - if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) { - SafetyCheck::AutoApprove { - sandbox_type: SandboxType::None, - } - } else if policy == AskForApproval::OnFailure { + // Even though the patch *appears* to be constrained to writable paths, it + // is possible that paths in the patch are hard links to files outside the + // writable roots, so we should still run `apply_patch` in a sandbox in that + // case. + if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) + || policy == AskForApproval::OnFailure + { // Only auto‑approve when we can actually enforce a sandbox. Otherwise // fall back to asking the user because the patch may touch arbitrary // paths outside the project. diff --git a/codex-rs/core/src/seatbelt.rs b/codex-rs/core/src/seatbelt.rs new file mode 100644 index 0000000000..be2acb1bdc --- /dev/null +++ b/codex-rs/core/src/seatbelt.rs @@ -0,0 +1,96 @@ +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; +use tokio::process::Child; + +use crate::protocol::SandboxPolicy; +use crate::spawn::StdioPolicy; +use crate::spawn::spawn_child_async; + +const MACOS_SEATBELT_BASE_POLICY: &str = include_str!("seatbelt_base_policy.sbpl"); + +/// When working with `sandbox-exec`, only consider `sandbox-exec` in `/usr/bin` +/// to defend against an attacker trying to inject a malicious version on the +/// PATH. If /usr/bin/sandbox-exec has been tampered with, then the attacker +/// already has root access. +const MACOS_PATH_TO_SEATBELT_EXECUTABLE: &str = "/usr/bin/sandbox-exec"; + +pub async fn spawn_command_under_seatbelt( + command: Vec, + sandbox_policy: &SandboxPolicy, + cwd: PathBuf, + stdio_policy: StdioPolicy, + env: HashMap, +) -> std::io::Result { + let args = create_seatbelt_command_args(command, sandbox_policy, &cwd); + let arg0 = None; + spawn_child_async( + PathBuf::from(MACOS_PATH_TO_SEATBELT_EXECUTABLE), + args, + arg0, + cwd, + sandbox_policy, + stdio_policy, + env, + ) + .await +} + +fn create_seatbelt_command_args( + command: Vec, + sandbox_policy: &SandboxPolicy, + cwd: &Path, +) -> Vec { + let (file_write_policy, extra_cli_args) = { + if sandbox_policy.has_full_disk_write_access() { + // Allegedly, this is more permissive than `(allow file-write*)`. + ( + r#"(allow file-write* (regex #"^/"))"#.to_string(), + Vec::::new(), + ) + } else { + let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); + let (writable_folder_policies, cli_args): (Vec, Vec) = writable_roots + .iter() + .enumerate() + .map(|(index, root)| { + let param_name = format!("WRITABLE_ROOT_{index}"); + let policy: String = format!("(subpath (param \"{param_name}\"))"); + let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); + (policy, cli_arg) + }) + .unzip(); + if writable_folder_policies.is_empty() { + ("".to_string(), Vec::::new()) + } else { + let file_write_policy = format!( + "(allow file-write*\n{}\n)", + writable_folder_policies.join(" ") + ); + (file_write_policy, cli_args) + } + } + }; + + let file_read_policy = if sandbox_policy.has_full_disk_read_access() { + "; allow read-only file operations\n(allow file-read*)" + } else { + "" + }; + + // TODO(mbolin): apply_patch calls must also honor the SandboxPolicy. + let network_policy = if sandbox_policy.has_full_network_access() { + "(allow network-outbound)\n(allow network-inbound)\n(allow system-socket)" + } else { + "" + }; + + let full_policy = format!( + "{MACOS_SEATBELT_BASE_POLICY}\n{file_read_policy}\n{file_write_policy}\n{network_policy}" + ); + let mut seatbelt_args: Vec = vec!["-p".to_string(), full_policy]; + seatbelt_args.extend(extra_cli_args); + seatbelt_args.push("--".to_string()); + seatbelt_args.extend(command); + seatbelt_args +} diff --git a/codex-rs/core/src/spawn.rs b/codex-rs/core/src/spawn.rs new file mode 100644 index 0000000000..9fde26539b --- /dev/null +++ b/codex-rs/core/src/spawn.rs @@ -0,0 +1,102 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::process::Stdio; +use tokio::process::Child; +use tokio::process::Command; +use tracing::trace; + +use crate::protocol::SandboxPolicy; + +/// Experimental environment variable that will be set to some non-empty value +/// if both of the following are true: +/// +/// 1. The process was spawned by Codex as part of a shell tool call. +/// 2. SandboxPolicy.has_full_network_access() was false for the tool call. +/// +/// We may try to have just one environment variable for all sandboxing +/// attributes, so this may change in the future. +pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED"; + +#[derive(Debug, Clone, Copy)] +pub enum StdioPolicy { + RedirectForShellTool, + Inherit, +} + +/// Spawns the appropriate child process for the ExecParams and SandboxPolicy, +/// ensuring the args and environment variables used to create the `Command` +/// (and `Child`) honor the configuration. +/// +/// For now, we take `SandboxPolicy` as a parameter to spawn_child() because +/// we need to determine whether to set the +/// `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` environment variable. +pub(crate) async fn spawn_child_async( + program: PathBuf, + args: Vec, + #[cfg_attr(not(unix), allow(unused_variables))] arg0: Option<&str>, + cwd: PathBuf, + sandbox_policy: &SandboxPolicy, + stdio_policy: StdioPolicy, + env: HashMap, +) -> std::io::Result { + trace!( + "spawn_child_async: {program:?} {args:?} {arg0:?} {cwd:?} {sandbox_policy:?} {stdio_policy:?} {env:?}" + ); + + let mut cmd = Command::new(&program); + #[cfg(unix)] + cmd.arg0(arg0.map_or_else(|| program.to_string_lossy().to_string(), String::from)); + cmd.args(args); + cmd.current_dir(cwd); + cmd.env_clear(); + cmd.envs(env); + + if !sandbox_policy.has_full_network_access() { + cmd.env(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR, "1"); + } + + // If this Codex process dies (including being killed via SIGKILL), we want + // any child processes that were spawned as part of a `"shell"` tool call + // to also be terminated. + + // This relies on prctl(2), so it only works on Linux. + #[cfg(target_os = "linux")] + unsafe { + cmd.pre_exec(|| { + // This prctl call effectively requests, "deliver SIGTERM when my + // current parent dies." + if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 { + return Err(std::io::Error::last_os_error()); + } + + // Though if there was a race condition and this pre_exec() block is + // run _after_ the parent (i.e., the Codex process) has already + // exited, then the parent is the _init_ process (which will never + // die), so we should just terminate the child process now. + if libc::getppid() == 1 { + libc::raise(libc::SIGTERM); + } + Ok(()) + }); + } + + match stdio_policy { + StdioPolicy::RedirectForShellTool => { + // Do not create a file descriptor for stdin because otherwise some + // commands may hang forever waiting for input. For example, ripgrep has + // a heuristic where it may try to read from stdin as explained here: + // https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103 + cmd.stdin(Stdio::null()); + + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + } + StdioPolicy::Inherit => { + // Inherit stdin, stdout, and stderr from the parent process. + cmd.stdin(Stdio::inherit()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()); + } + } + + cmd.kill_on_drop(true).spawn() +} diff --git a/codex-rs/core/tests/cli_stream.rs b/codex-rs/core/tests/cli_stream.rs index ee0377fc10..c8570577ce 100644 --- a/codex-rs/core/tests/cli_stream.rs +++ b/codex-rs/core/tests/cli_stream.rs @@ -1,7 +1,7 @@ #![expect(clippy::unwrap_used)] use assert_cmd::Command as AssertCommand; -use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; +use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use std::time::Duration; use std::time::Instant; use tempfile::TempDir; diff --git a/codex-rs/core/tests/client.rs b/codex-rs/core/tests/client.rs index 67d95cb8f6..bfa29657d5 100644 --- a/codex-rs/core/tests/client.rs +++ b/codex-rs/core/tests/client.rs @@ -5,11 +5,11 @@ use codex_core::Codex; use codex_core::CodexSpawnOk; use codex_core::ModelProviderInfo; use codex_core::built_in_model_providers; -use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_core::protocol::EventMsg; use codex_core::protocol::InputItem; use codex_core::protocol::Op; use codex_core::protocol::SessionConfiguredEvent; +use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_login::AuthDotJson; use codex_login::AuthMode; use codex_login::CodexAuth; @@ -237,6 +237,7 @@ async fn chatgpt_auth_sends_correct_request() { let request_session_id = request.headers.get("session_id").unwrap(); let request_originator = request.headers.get("originator").unwrap(); let request_authorization = request.headers.get("authorization").unwrap(); + let request_chatgpt_account_id = request.headers.get("chatgpt-account-id").unwrap(); let request_body = request.body_json::().unwrap(); assert!(current_session_id.is_some()); @@ -249,6 +250,7 @@ async fn chatgpt_auth_sends_correct_request() { request_authorization.to_str().unwrap(), "Bearer Access Token" ); + assert_eq!(request_chatgpt_account_id.to_str().unwrap(), "account_id"); assert!(!request_body["store"].as_bool().unwrap()); assert!(request_body["stream"].as_bool().unwrap()); assert_eq!( @@ -327,14 +329,14 @@ fn auth_from_token(id_token: String) -> CodexAuth { AuthMode::ChatGPT, PathBuf::new(), Some(AuthDotJson { - tokens: TokenData { + openai_api_key: None, + tokens: Some(TokenData { id_token, access_token: "Access Token".to_string(), refresh_token: "test".to_string(), - account_id: None, - }, - last_refresh: Utc::now(), - openai_api_key: None, + account_id: Some("account_id".to_string()), + }), + last_refresh: Some(Utc::now()), }), ) } diff --git a/codex-rs/core/tests/compact.rs b/codex-rs/core/tests/compact.rs new file mode 100644 index 0000000000..8967c86a8f --- /dev/null +++ b/codex-rs/core/tests/compact.rs @@ -0,0 +1,254 @@ +#![expect(clippy::unwrap_used)] + +use codex_core::Codex; +use codex_core::CodexSpawnOk; +use codex_core::ModelProviderInfo; +use codex_core::built_in_model_providers; +use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; +use codex_core::protocol::EventMsg; +use codex_core::protocol::InputItem; +use codex_core::protocol::Op; +use codex_login::CodexAuth; +use core_test_support::load_default_config_for_test; +use core_test_support::wait_for_event; +use serde_json::Value; +use tempfile::TempDir; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; +use wiremock::matchers::path; + +use pretty_assertions::assert_eq; + +// --- Test helpers ----------------------------------------------------------- + +/// Build an SSE stream body from a list of JSON events. +fn sse(events: Vec) -> String { + use std::fmt::Write as _; + let mut out = String::new(); + for ev in events { + let kind = ev.get("type").and_then(|v| v.as_str()).unwrap(); + writeln!(&mut out, "event: {kind}").unwrap(); + if !ev.as_object().map(|o| o.len() == 1).unwrap_or(false) { + write!(&mut out, "data: {ev}\n\n").unwrap(); + } else { + out.push('\n'); + } + } + out +} + +/// Convenience: SSE event for a completed response with a specific id. +fn ev_completed(id: &str) -> Value { + serde_json::json!({ + "type": "response.completed", + "response": { + "id": id, + "usage": {"input_tokens":0,"input_tokens_details":null,"output_tokens":0,"output_tokens_details":null,"total_tokens":0} + } + }) +} + +/// Convenience: SSE event for a single assistant message output item. +fn ev_assistant_message(id: &str, text: &str) -> Value { + serde_json::json!({ + "type": "response.output_item.done", + "item": { + "type": "message", + "role": "assistant", + "id": id, + "content": [{"type": "output_text", "text": text}] + } + }) +} + +fn sse_response(body: String) -> ResponseTemplate { + ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(body, "text/event-stream") +} + +async fn mount_sse_once(server: &MockServer, matcher: M, body: String) +where + M: wiremock::Match + Send + Sync + 'static, +{ + Mock::given(method("POST")) + .and(path("/v1/responses")) + .and(matcher) + .respond_with(sse_response(body)) + .expect(1) + .mount(server) + .await; +} + +const FIRST_REPLY: &str = "FIRST_REPLY"; +const SUMMARY_TEXT: &str = "SUMMARY_ONLY_CONTEXT"; +const SUMMARIZE_TRIGGER: &str = "Start Summarization"; +const THIRD_USER_MSG: &str = "next turn"; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn summarize_context_three_requests_and_instructions() { + if std::env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { + println!( + "Skipping test because it cannot execute when network is disabled in a Codex sandbox." + ); + return; + } + + // Set up a mock server that we can inspect after the run. + let server = MockServer::start().await; + + // SSE 1: assistant replies normally so it is recorded in history. + let sse1 = sse(vec![ + ev_assistant_message("m1", FIRST_REPLY), + ev_completed("r1"), + ]); + + // SSE 2: summarizer returns a summary message. + let sse2 = sse(vec![ + ev_assistant_message("m2", SUMMARY_TEXT), + ev_completed("r2"), + ]); + + // SSE 3: minimal completed; we only need to capture the request body. + let sse3 = sse(vec![ev_completed("r3")]); + + // Mount three expectations, one per request, matched by body content. + let first_matcher = |req: &wiremock::Request| { + let body = std::str::from_utf8(&req.body).unwrap_or(""); + body.contains("\"text\":\"hello world\"") + && !body.contains(&format!("\"text\":\"{SUMMARIZE_TRIGGER}\"")) + }; + mount_sse_once(&server, first_matcher, sse1).await; + + let second_matcher = |req: &wiremock::Request| { + let body = std::str::from_utf8(&req.body).unwrap_or(""); + body.contains(&format!("\"text\":\"{SUMMARIZE_TRIGGER}\"")) + }; + mount_sse_once(&server, second_matcher, sse2).await; + + let third_matcher = |req: &wiremock::Request| { + let body = std::str::from_utf8(&req.body).unwrap_or(""); + body.contains(&format!("\"text\":\"{THIRD_USER_MSG}\"")) + }; + mount_sse_once(&server, third_matcher, sse3).await; + + // Build config pointing to the mock server and spawn Codex. + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + let home = TempDir::new().unwrap(); + let mut config = load_default_config_for_test(&home); + config.model_provider = model_provider; + let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); + let CodexSpawnOk { codex, .. } = Codex::spawn( + config, + Some(CodexAuth::from_api_key("dummy".to_string())), + ctrl_c.clone(), + ) + .await + .unwrap(); + + // 1) Normal user input – should hit server once. + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: "hello world".into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + // 2) Summarize – second hit with summarization instructions. + codex.submit(Op::Compact).await.unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + // 3) Next user input – third hit; history should include only the summary. + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: THIRD_USER_MSG.into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + // Inspect the three captured requests. + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 3, "expected exactly three requests"); + + let req1 = &requests[0]; + let req2 = &requests[1]; + let req3 = &requests[2]; + + let body1 = req1.body_json::().unwrap(); + let body2 = req2.body_json::().unwrap(); + let body3 = req3.body_json::().unwrap(); + + // System instructions should change for the summarization turn. + let instr1 = body1.get("instructions").and_then(|v| v.as_str()).unwrap(); + let instr2 = body2.get("instructions").and_then(|v| v.as_str()).unwrap(); + assert_ne!( + instr1, instr2, + "summarization should override base instructions" + ); + assert!( + instr2.contains("You are a summarization assistant"), + "summarization instructions not applied" + ); + + // The summarization request should include the injected user input marker. + let input2 = body2.get("input").and_then(|v| v.as_array()).unwrap(); + // The last item is the user message created from the injected input. + let last2 = input2.last().unwrap(); + assert_eq!(last2.get("type").unwrap().as_str().unwrap(), "message"); + assert_eq!(last2.get("role").unwrap().as_str().unwrap(), "user"); + let text2 = last2["content"][0]["text"].as_str().unwrap(); + assert!(text2.contains(SUMMARIZE_TRIGGER)); + + // Third request must contain only the summary from step 2 as prior history plus new user msg. + let input3 = body3.get("input").and_then(|v| v.as_array()).unwrap(); + println!("third request body: {body3}"); + assert!( + input3.len() >= 2, + "expected summary + new user message in third request" + ); + + // Collect all (role, text) message tuples. + let mut messages: Vec<(String, String)> = Vec::new(); + for item in input3 { + if item["type"].as_str() == Some("message") { + let role = item["role"].as_str().unwrap_or_default().to_string(); + let text = item["content"][0]["text"] + .as_str() + .unwrap_or_default() + .to_string(); + messages.push((role, text)); + } + } + + // Exactly one assistant message should remain after compaction and the new user message is present. + let assistant_count = messages.iter().filter(|(r, _)| r == "assistant").count(); + assert_eq!( + assistant_count, 1, + "exactly one assistant message should remain after compaction" + ); + assert!( + messages + .iter() + .any(|(r, t)| r == "user" && t == THIRD_USER_MSG), + "third request should include the new user message" + ); + assert!( + !messages.iter().any(|(_, t)| t.contains("hello world")), + "third request should not include the original user input" + ); + assert!( + !messages.iter().any(|(_, t)| t.contains(SUMMARIZE_TRIGGER)), + "third request should not include the summarize trigger" + ); +} diff --git a/codex-rs/core/tests/stream_no_completed.rs b/codex-rs/core/tests/stream_no_completed.rs index d2fc035569..3e30d93709 100644 --- a/codex-rs/core/tests/stream_no_completed.rs +++ b/codex-rs/core/tests/stream_no_completed.rs @@ -6,10 +6,10 @@ use std::time::Duration; use codex_core::Codex; use codex_core::CodexSpawnOk; use codex_core::ModelProviderInfo; -use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_core::protocol::EventMsg; use codex_core::protocol::InputItem; use codex_core::protocol::Op; +use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_login::CodexAuth; use core_test_support::load_default_config_for_test; use core_test_support::load_sse_fixture; diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index e10666b092..650291b3bc 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -18,3 +18,6 @@ tokio = { version = "1", features = [ "rt-multi-thread", "signal", ] } + +[dev-dependencies] +tempfile = "3" diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index 47dbbca9fb..3d55c20264 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -20,7 +20,7 @@ use tokio::process::Command; const SOURCE_FOR_PYTHON_SERVER: &str = include_str!("./login_with_chatgpt.py"); const CLIENT_ID: &str = "app_EMoamEEZ73f0CkXaXp7hrann"; -const OPENAI_API_KEY_ENV_VAR: &str = "OPENAI_API_KEY"; +pub const OPENAI_API_KEY_ENV_VAR: &str = "OPENAI_API_KEY"; #[derive(Clone, Debug, PartialEq)] pub enum AuthMode { @@ -70,13 +70,16 @@ impl CodexAuth { pub async fn get_token_data(&self) -> Result { #[expect(clippy::unwrap_used)] let auth_dot_json = self.auth_dot_json.lock().unwrap().clone(); - match auth_dot_json { - Some(auth_dot_json) => { - if auth_dot_json.last_refresh < Utc::now() - chrono::Duration::days(28) { + Some(AuthDotJson { + tokens: Some(mut tokens), + last_refresh: Some(last_refresh), + .. + }) => { + if last_refresh < Utc::now() - chrono::Duration::days(28) { let refresh_response = tokio::time::timeout( Duration::from_secs(60), - try_refresh_token(auth_dot_json.tokens.refresh_token.clone()), + try_refresh_token(tokens.refresh_token.clone()), ) .await .map_err(|_| { @@ -92,13 +95,21 @@ impl CodexAuth { ) .await?; + tokens = updated_auth_dot_json + .tokens + .clone() + .ok_or(std::io::Error::other( + "Token data is not available after refresh.", + ))?; + #[expect(clippy::unwrap_used)] - let mut auth_dot_json = self.auth_dot_json.lock().unwrap(); - *auth_dot_json = Some(updated_auth_dot_json); + let mut auth_lock = self.auth_dot_json.lock().unwrap(); + *auth_lock = Some(updated_auth_dot_json); } - Ok(auth_dot_json.tokens.clone()) + + Ok(tokens) } - None => Err(std::io::Error::other("Token data is not available.")), + _ => Err(std::io::Error::other("Token data is not available.")), } } @@ -112,11 +123,22 @@ impl CodexAuth { } } } + + pub async fn get_account_id(&self) -> Option { + match self.mode { + AuthMode::ApiKey => None, + AuthMode::ChatGPT => { + let token_data = self.get_token_data().await.ok()?; + + token_data.account_id.clone() + } + } + } } // Loads the available auth information from the auth.json or OPENAI_API_KEY environment variable. -pub fn load_auth(codex_home: &Path) -> std::io::Result> { - let auth_file = codex_home.join("auth.json"); +pub fn load_auth(codex_home: &Path, include_env_var: bool) -> std::io::Result> { + let auth_file = get_auth_file(codex_home); let auth_dot_json = try_read_auth_json(&auth_file).ok(); @@ -125,12 +147,21 @@ pub fn load_auth(codex_home: &Path) -> std::io::Result> { .and_then(|a| a.openai_api_key.clone()) .filter(|s| !s.is_empty()); - let openai_api_key = env::var(OPENAI_API_KEY_ENV_VAR) - .ok() - .filter(|s| !s.is_empty()) - .or(auth_json_api_key); + let openai_api_key = if include_env_var { + env::var(OPENAI_API_KEY_ENV_VAR) + .ok() + .filter(|s| !s.is_empty()) + .or(auth_json_api_key) + } else { + auth_json_api_key + }; - if openai_api_key.is_none() && auth_dot_json.is_none() { + let has_tokens = auth_dot_json + .as_ref() + .and_then(|a| a.tokens.as_ref()) + .is_some(); + + if openai_api_key.is_none() && !has_tokens { return Ok(None); } @@ -148,6 +179,10 @@ pub fn load_auth(codex_home: &Path) -> std::io::Result> { })) } +fn get_auth_file(codex_home: &Path) -> PathBuf { + codex_home.join("auth.json") +} + /// Run `python3 -c {{SOURCE_FOR_PYTHON_SERVER}}` with the CODEX_HOME /// environment variable set to the provided `codex_home` path. If the /// subprocess exits 0, read the OPENAI_API_KEY property out of @@ -187,6 +222,15 @@ pub async fn login_with_chatgpt(codex_home: &Path, capture_output: bool) -> std: } } +pub fn login_with_api_key(codex_home: &Path, api_key: &str) -> std::io::Result<()> { + let auth_dot_json = AuthDotJson { + openai_api_key: Some(api_key.to_string()), + tokens: None, + last_refresh: None, + }; + write_auth_json(&get_auth_file(codex_home), &auth_dot_json) +} + /// Attempt to read and refresh the `auth.json` file in the given `CODEX_HOME` directory. /// Returns the full AuthDotJson structure after refreshing if necessary. pub fn try_read_auth_json(auth_file: &Path) -> std::io::Result { @@ -198,35 +242,38 @@ pub fn try_read_auth_json(auth_file: &Path) -> std::io::Result { Ok(auth_dot_json) } -async fn update_tokens( - auth_file: &Path, - id_token: String, - access_token: Option, - refresh_token: Option, -) -> std::io::Result { +fn write_auth_json(auth_file: &Path, auth_dot_json: &AuthDotJson) -> std::io::Result<()> { + let json_data = serde_json::to_string_pretty(auth_dot_json)?; let mut options = OpenOptions::new(); options.truncate(true).write(true).create(true); #[cfg(unix)] { options.mode(0o600); } + let mut file = options.open(auth_file)?; + file.write_all(json_data.as_bytes())?; + file.flush()?; + Ok(()) +} + +async fn update_tokens( + auth_file: &Path, + id_token: String, + access_token: Option, + refresh_token: Option, +) -> std::io::Result { let mut auth_dot_json = try_read_auth_json(auth_file)?; - auth_dot_json.tokens.id_token = id_token.to_string(); + let tokens = auth_dot_json.tokens.get_or_insert_with(TokenData::default); + tokens.id_token = id_token.to_string(); if let Some(access_token) = access_token { - auth_dot_json.tokens.access_token = access_token.to_string(); + tokens.access_token = access_token.to_string(); } if let Some(refresh_token) = refresh_token { - auth_dot_json.tokens.refresh_token = refresh_token.to_string(); - } - auth_dot_json.last_refresh = Utc::now(); - - let json_data = serde_json::to_string_pretty(&auth_dot_json)?; - { - let mut file = options.open(auth_file)?; - file.write_all(json_data.as_bytes())?; - file.flush()?; + tokens.refresh_token = refresh_token.to_string(); } + auth_dot_json.last_refresh = Some(Utc::now()); + write_auth_json(auth_file, &auth_dot_json)?; Ok(auth_dot_json) } @@ -282,12 +329,14 @@ pub struct AuthDotJson { #[serde(rename = "OPENAI_API_KEY")] pub openai_api_key: Option, - pub tokens: TokenData, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub tokens: Option, - pub last_refresh: DateTime, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub last_refresh: Option>, } -#[derive(Deserialize, Serialize, Clone, Debug, PartialEq)] +#[derive(Deserialize, Serialize, Clone, Debug, PartialEq, Default)] pub struct TokenData { /// This is a JWT. pub id_token: String, @@ -299,3 +348,95 @@ pub struct TokenData { pub account_id: Option, } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[test] + #[expect(clippy::unwrap_used)] + fn writes_api_key_and_loads_auth() { + let dir = tempdir().unwrap(); + login_with_api_key(dir.path(), "sk-test-key").unwrap(); + let auth = load_auth(dir.path(), false).unwrap().unwrap(); + assert_eq!(auth.mode, AuthMode::ApiKey); + assert_eq!(auth.api_key.as_deref(), Some("sk-test-key")); + } + + #[test] + #[expect(clippy::unwrap_used)] + fn loads_from_env_var_if_env_var_exists() { + let dir = tempdir().unwrap(); + + let env_var = std::env::var(OPENAI_API_KEY_ENV_VAR); + + if let Ok(env_var) = env_var { + let auth = load_auth(dir.path(), true).unwrap().unwrap(); + assert_eq!(auth.mode, AuthMode::ApiKey); + assert_eq!(auth.api_key, Some(env_var)); + } + } + + #[tokio::test] + #[expect(clippy::unwrap_used)] + async fn loads_token_data_from_auth_json() { + let dir = tempdir().unwrap(); + let auth_file = dir.path().join("auth.json"); + std::fs::write( + auth_file, + format!( + r#" + {{ + "OPENAI_API_KEY": null, + "tokens": {{ + "id_token": "test-id-token", + "access_token": "test-access-token", + "refresh_token": "test-refresh-token" + }}, + "last_refresh": "{}" + }} + "#, + Utc::now().to_rfc3339() + ), + ) + .unwrap(); + + let auth = load_auth(dir.path(), false).unwrap().unwrap(); + assert_eq!(auth.mode, AuthMode::ChatGPT); + assert_eq!(auth.api_key, None); + assert_eq!( + auth.get_token_data().await.unwrap(), + TokenData { + id_token: "test-id-token".to_string(), + access_token: "test-access-token".to_string(), + refresh_token: "test-refresh-token".to_string(), + account_id: None, + } + ); + } + + #[tokio::test] + #[expect(clippy::unwrap_used)] + async fn loads_api_key_from_auth_json() { + let dir = tempdir().unwrap(); + let auth_file = dir.path().join("auth.json"); + std::fs::write( + auth_file, + r#" + { + "OPENAI_API_KEY": "sk-test-key", + "tokens": null, + "last_refresh": null + } + "#, + ) + .unwrap(); + + let auth = load_auth(dir.path(), false).unwrap().unwrap(); + assert_eq!(auth.mode, AuthMode::ApiKey); + assert_eq!(auth.api_key, Some("sk-test-key".to_string())); + + assert!(auth.get_token_data().await.is_err()); + } +} diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index 0912fed118..ebef8ca903 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -19,7 +19,7 @@ mod codex_tool_config; mod codex_tool_runner; mod exec_approval; mod json_to_toml; -mod mcp_protocol; +pub mod mcp_protocol; mod message_processor; mod outgoing_message; mod patch_approval; diff --git a/codex-rs/mcp-server/src/mcp_protocol.rs b/codex-rs/mcp-server/src/mcp_protocol.rs index e507376c16..23304dc43d 100644 --- a/codex-rs/mcp-server/src/mcp_protocol.rs +++ b/codex-rs/mcp-server/src/mcp_protocol.rs @@ -7,7 +7,10 @@ use serde::Serialize; use strum_macros::Display; use uuid::Uuid; +use mcp_types::CallToolResult; +use mcp_types::ContentBlock; use mcp_types::RequestId; +use mcp_types::TextContent; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(transparent)] @@ -118,10 +121,47 @@ pub struct ToolCallResponse { pub request_id: RequestId, #[serde(default, skip_serializing_if = "Option::is_none")] pub is_error: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none", flatten)] pub result: Option, } +impl From for CallToolResult { + fn from(val: ToolCallResponse) -> Self { + let ToolCallResponse { + request_id: _request_id, + is_error, + result, + } = val; + let (content, structured_content, is_error_out) = match result { + Some(res) => match serde_json::to_value(&res) { + Ok(v) => { + let content = vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_string(), + text: v.to_string(), + annotations: None, + })]; + (content, Some(v), is_error) + } + Err(e) => { + let content = vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_string(), + text: format!("Failed to serialize tool result: {e}"), + annotations: None, + })]; + (content, None, Some(true)) + } + }, + None => (vec![], None, is_error), + }; + + CallToolResult { + content, + is_error: is_error_out, + structured_content, + } + } +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(untagged)] pub enum ToolCallResponseResult { @@ -141,8 +181,10 @@ pub struct ConversationCreateResult { pub struct ConversationStreamResult {} #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationSendMessageResult { - pub success: bool, +#[serde(tag = "status", rename_all = "camelCase")] +pub enum ConversationSendMessageResult { + Ok, + Error { message: String }, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -455,10 +497,13 @@ mod tests { }, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 1, - "result": { + "content": [ + { "type": "text", "text": "{\"conversation_id\":\"d0f6ecbe-84a2-41c1-b23d-b20473b25eab\",\"model\":\"o3\"}" } + ], + "structuredContent": { "conversation_id": "d0f6ecbe-84a2-41c1-b23d-b20473b25eab", "model": "o3" } @@ -467,6 +512,7 @@ mod tests { observed, expected, "response (ConversationCreate) must match" ); + assert_eq!(req_id, RequestId::Integer(1)); } #[test] @@ -478,15 +524,17 @@ mod tests { ConversationStreamResult {}, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 2, - "result": {} + "content": [ { "type": "text", "text": "{}" } ], + "structuredContent": {} }); assert_eq!( observed, expected, "response (ConversationStream) must have empty object result" ); + assert_eq!(req_id, RequestId::Integer(2)); } #[test] @@ -495,18 +543,20 @@ mod tests { request_id: RequestId::Integer(3), is_error: None, result: Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult { success: true }, + ConversationSendMessageResult::Ok, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 3, - "result": { "success": true } + "content": [ { "type": "text", "text": "{\"status\":\"ok\"}" } ], + "structuredContent": { "status": "ok" } }); assert_eq!( observed, expected, "response (ConversationSendMessageAccepted) must match" ); + assert_eq!(req_id, RequestId::Integer(3)); } #[test] @@ -526,10 +576,13 @@ mod tests { }, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 4, - "result": { + "content": [ + { "type": "text", "text": "{\"conversations\":[{\"conversation_id\":\"67e55044-10b1-426f-9247-bb680e5fe0c8\",\"title\":\"Refactor config loader\"}],\"next_cursor\":\"next123\"}" } + ], + "structuredContent": { "conversations": [ { "conversation_id": "67e55044-10b1-426f-9247-bb680e5fe0c8", @@ -543,6 +596,7 @@ mod tests { observed, expected, "response (ConversationsList with cursor) must match" ); + assert_eq!(req_id, RequestId::Integer(4)); } #[test] @@ -552,15 +606,17 @@ mod tests { is_error: Some(true), result: None, }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 4, + "content": [], "isError": true }); assert_eq!( observed, expected, "error response must omit `result` and include `isError`" ); + assert_eq!(req_id, RequestId::Integer(4)); } // ----- Notifications ----- diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 7ba827d60b..a4013cc3a3 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -6,6 +6,7 @@ use crate::codex_tool_config::CodexToolCallParam; use crate::codex_tool_config::CodexToolCallReplyParam; use crate::codex_tool_config::create_tool_for_codex_tool_call_param; use crate::codex_tool_config::create_tool_for_codex_tool_call_reply_param; +use crate::mcp_protocol::ToolCallRequestParams; use crate::outgoing_message::OutgoingMessageSender; use codex_core::Codex; @@ -300,6 +301,14 @@ impl MessageProcessor { params: ::Params, ) { tracing::info!("tools/call -> params: {:?}", params); + // Serialize params into JSON and try to parse as new type + if let Ok(new_params) = + serde_json::to_value(¶ms).and_then(serde_json::from_value::) + { + // New tool call matched → forward + self.handle_new_tool_calls(id, new_params).await; + return; + } let CallToolRequestParams { name, arguments } = params; match name.as_str() { @@ -323,6 +332,20 @@ impl MessageProcessor { } } } + async fn handle_new_tool_calls(&self, request_id: RequestId, _params: ToolCallRequestParams) { + // TODO: implement the new tool calls + let result = CallToolResult { + content: vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_string(), + text: "Unknown tool".to_string(), + annotations: None, + })], + is_error: Some(true), + structured_content: None, + }; + self.send_response::(request_id, result) + .await; + } async fn handle_tool_call_codex(&self, id: RequestId, arguments: Option) { let (initial_prompt, config): (String, CodexConfig) = match arguments { diff --git a/codex-rs/mcp-server/tests/codex_tool.rs b/codex-rs/mcp-server/tests/codex_tool.rs index 0f06483f24..7d59ff202e 100644 --- a/codex-rs/mcp-server/tests/codex_tool.rs +++ b/codex-rs/mcp-server/tests/codex_tool.rs @@ -3,9 +3,9 @@ use std::env; use std::path::Path; use std::path::PathBuf; -use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_core::protocol::FileChange; use codex_core::protocol::ReviewDecision; +use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_mcp_server::CodexToolCallParam; use codex_mcp_server::ExecApprovalElicitRequestParams; use codex_mcp_server::ExecApprovalResponse; diff --git a/codex-rs/mcp-server/tests/common/Cargo.toml b/codex-rs/mcp-server/tests/common/Cargo.toml index 3aa246f154..420b97811d 100644 --- a/codex-rs/mcp-server/tests/common/Cargo.toml +++ b/codex-rs/mcp-server/tests/common/Cargo.toml @@ -10,6 +10,7 @@ path = "lib.rs" anyhow = "1" assert_cmd = "2" codex-mcp-server = { path = "../.." } +codex-core = { path = "../../../core" } mcp-types = { path = "../../../mcp-types" } pretty_assertions = "1.4.1" serde_json = "1" @@ -22,3 +23,4 @@ tokio = { version = "1", features = [ "rt-multi-thread", ] } wiremock = "0.6" +uuid = { version = "1", features = ["serde", "v4"] } \ No newline at end of file diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 8138749c40..d7144af480 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -11,8 +11,13 @@ use tokio::process::ChildStdout; use anyhow::Context; use assert_cmd::prelude::*; +use codex_core::protocol::InputItem; use codex_mcp_server::CodexToolCallParam; use codex_mcp_server::CodexToolCallReplyParam; +use codex_mcp_server::mcp_protocol::ConversationId; +use codex_mcp_server::mcp_protocol::ConversationSendMessageArgs; +use codex_mcp_server::mcp_protocol::ToolCallRequestParams; + use mcp_types::CallToolRequestParams; use mcp_types::ClientCapabilities; use mcp_types::Implementation; @@ -29,6 +34,7 @@ use pretty_assertions::assert_eq; use serde_json::json; use std::process::Command as StdCommand; use tokio::process::Command; +use uuid::Uuid; pub struct McpProcess { next_request_id: AtomicI64, @@ -174,6 +180,26 @@ impl McpProcess { .await } + pub async fn send_user_message_tool_call( + &mut self, + message: &str, + session_id: &str, + ) -> anyhow::Result { + let params = ToolCallRequestParams::ConversationSendMessage(ConversationSendMessageArgs { + conversation_id: ConversationId(Uuid::parse_str(session_id)?), + content: vec![InputItem::Text { + text: message.to_string(), + }], + parent_message_id: None, + conversation_overrides: None, + }); + self.send_request( + mcp_types::CallToolRequest::METHOD, + Some(serde_json::to_value(params)?), + ) + .await + } + async fn send_request( &mut self, method: &str, diff --git a/codex-rs/mcp-server/tests/interrupt.rs b/codex-rs/mcp-server/tests/interrupt.rs index 313bc7afab..b2767f5e58 100644 --- a/codex-rs/mcp-server/tests/interrupt.rs +++ b/codex-rs/mcp-server/tests/interrupt.rs @@ -3,7 +3,7 @@ use std::path::Path; -use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; +use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_mcp_server::CodexToolCallParam; use mcp_types::JSONRPCResponse; use mcp_types::RequestId; diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 63d287ca11..468f2f3be6 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -62,6 +62,8 @@ unicode-segmentation = "1.12.0" unicode-width = "0.1" uuid = "1" + + [dev-dependencies] insta = "1.43.1" pretty_assertions = "1" diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index ad4bc24fd4..d4c2f3c12d 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -11,12 +11,16 @@ use codex_core::config::Config; use codex_core::protocol::Event; use codex_core::protocol::EventMsg; use codex_core::protocol::ExecApprovalRequestEvent; +use codex_core::protocol::Op; use color_eyre::eyre::Result; use crossterm::SynchronizedUpdate; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; +use crossterm::terminal::supports_keyboard_enhancement; use ratatui::layout::Offset; use ratatui::prelude::Backend; +use ratatui::text::Line; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicBool; @@ -55,9 +59,13 @@ pub(crate) struct App<'a> { /// True when a redraw has been scheduled but not yet executed. pending_redraw: Arc, + pending_history_lines: Vec>, + /// Stored parameters needed to instantiate the ChatWidget later, e.g., /// after dismissing the Git-repo warning. chat_args: Option, + + enhanced_keys_supported: bool, } /// Aggregate parameters needed to create a `ChatWidget`, as creation may be @@ -67,6 +75,7 @@ struct ChatWidgetArgs { config: Config, initial_prompt: Option, initial_images: Vec, + enhanced_keys_supported: bool, } impl App<'_> { @@ -80,6 +89,8 @@ impl App<'_> { let app_event_tx = AppEventSender::new(app_event_tx); let pending_redraw = Arc::new(AtomicBool::new(false)); + let enhanced_keys_supported = supports_keyboard_enhancement().unwrap_or(false); + // Spawn a dedicated thread for reading the crossterm event loop and // re-publishing the events as AppEvents, as appropriate. { @@ -132,6 +143,7 @@ impl App<'_> { config: config.clone(), initial_prompt, initial_images, + enhanced_keys_supported, }), ) } else { @@ -140,6 +152,7 @@ impl App<'_> { app_event_tx.clone(), initial_prompt, initial_images, + enhanced_keys_supported, ); ( AppState::Chat { @@ -152,12 +165,14 @@ impl App<'_> { let file_search = FileSearchManager::new(config.cwd.clone(), app_event_tx.clone()); Self { app_event_tx, + pending_history_lines: Vec::new(), app_event_rx, app_state, config, file_search, pending_redraw, chat_args, + enhanced_keys_supported, } } @@ -197,7 +212,7 @@ impl App<'_> { while let Ok(event) = self.app_event_rx.recv() { match event { AppEvent::InsertHistory(lines) => { - crate::insert_history::insert_history_lines(terminal, lines); + self.pending_history_lines.extend(lines); self.app_event_tx.send(AppEvent::RequestRedraw); } AppEvent::RequestRedraw => { @@ -211,6 +226,7 @@ impl App<'_> { KeyEvent { code: KeyCode::Char('c'), modifiers: crossterm::event::KeyModifiers::CONTROL, + kind: KeyEventKind::Press, .. } => { match &mut self.app_state { @@ -225,6 +241,7 @@ impl App<'_> { KeyEvent { code: KeyCode::Char('d'), modifiers: crossterm::event::KeyModifiers::CONTROL, + kind: KeyEventKind::Press, .. } => { match &mut self.app_state { @@ -243,9 +260,15 @@ impl App<'_> { } } } - _ => { + KeyEvent { + kind: KeyEventKind::Press | KeyEventKind::Repeat, + .. + } => { self.dispatch_key_event(key_event); } + _ => { + // Ignore Release key events for now. + } }; } AppEvent::Paste(text) => { @@ -272,10 +295,17 @@ impl App<'_> { self.app_event_tx.clone(), None, Vec::new(), + self.enhanced_keys_supported, )); self.app_state = AppState::Chat { widget: new_widget }; self.app_event_tx.send(AppEvent::RequestRedraw); } + SlashCommand::Compact => { + if let AppState::Chat { widget } = &mut self.app_state { + widget.clear_token_usage(); + self.app_event_tx.send(AppEvent::CodexOp(Op::Compact)); + } + } SlashCommand::Quit => { break; } @@ -302,14 +332,41 @@ impl App<'_> { } #[cfg(debug_assertions)] SlashCommand::TestApproval => { + use std::collections::HashMap; + + use codex_core::protocol::ApplyPatchApprovalRequestEvent; + use codex_core::protocol::FileChange; + self.app_event_tx.send(AppEvent::CodexEvent(Event { id: "1".to_string(), - msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { - call_id: "1".to_string(), - command: vec!["git".into(), "apply".into()], - cwd: self.config.cwd.clone(), - reason: Some("test".to_string()), - }), + // msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + // call_id: "1".to_string(), + // command: vec!["git".into(), "apply".into()], + // cwd: self.config.cwd.clone(), + // reason: Some("test".to_string()), + // }), + msg: EventMsg::ApplyPatchApprovalRequest( + ApplyPatchApprovalRequestEvent { + call_id: "1".to_string(), + changes: HashMap::from([ + ( + PathBuf::from("/tmp/test.txt"), + FileChange::Add { + content: "test".to_string(), + }, + ), + ( + PathBuf::from("/tmp/test2.txt"), + FileChange::Update { + unified_diff: "+test\n-test2".to_string(), + move_path: None, + }, + ), + ]), + reason: None, + grant_root: Some(PathBuf::from("/tmp")), + }, + ), })); } }, @@ -361,8 +418,9 @@ impl App<'_> { AppState::Chat { widget } => widget.desired_height(size.width), AppState::GitWarning { .. } => 10, }; + let mut area = terminal.viewport_area; - area.height = desired_height; + area.height = desired_height.min(size.height); area.width = size.width; if area.bottom() > size.height { terminal @@ -374,6 +432,13 @@ impl App<'_> { terminal.clear()?; terminal.set_viewport_area(area); } + if !self.pending_history_lines.is_empty() { + crate::insert_history::insert_history_lines( + terminal, + self.pending_history_lines.clone(), + ); + self.pending_history_lines.clear(); + } match &mut self.app_state { AppState::Chat { widget } => { terminal.draw(|frame| frame.render_widget_ref(&**widget, frame.area()))?; @@ -405,6 +470,7 @@ impl App<'_> { self.app_event_tx.clone(), args.initial_prompt, args.initial_images, + args.enhanced_keys_supported, )); self.app_state = AppState::Chat { widget }; self.app_event_tx.send(AppEvent::RequestRedraw); diff --git a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs index 4cd952f9eb..8e9ff6d936 100644 --- a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs +++ b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs @@ -99,6 +99,7 @@ mod tests { let mut pane = BottomPane::new(super::super::BottomPaneParams { app_event_tx: AppEventSender::new(tx_raw2), has_input_focus: true, + enhanced_keys_supported: false, }); assert_eq!(CancellationEvent::Handled, view.on_ctrl_c(&mut pane)); assert!(view.queue.is_empty()); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 3bc573a003..7e187bec8b 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -41,6 +41,7 @@ pub(crate) struct ChatComposer<'a> { app_event_tx: AppEventSender, history: ChatComposerHistory, ctrl_c_quit_hint: bool, + use_shift_enter_hint: bool, dismissed_file_popup_token: Option, current_file_query: Option, pending_pastes: Vec<(String, String)>, @@ -54,17 +55,24 @@ enum ActivePopup { } impl ChatComposer<'_> { - pub fn new(has_input_focus: bool, app_event_tx: AppEventSender) -> Self { + pub fn new( + has_input_focus: bool, + app_event_tx: AppEventSender, + enhanced_keys_supported: bool, + ) -> Self { let mut textarea = TextArea::default(); textarea.set_placeholder_text(BASE_PLACEHOLDER_TEXT); textarea.set_cursor_line_style(ratatui::style::Style::default()); + let use_shift_enter_hint = enhanced_keys_supported; + let mut this = Self { textarea, active_popup: ActivePopup::None, app_event_tx, history: ChatComposerHistory::new(), ctrl_c_quit_hint: false, + use_shift_enter_hint, dismissed_file_popup_token: None, current_file_query: None, pending_pastes: Vec::new(), @@ -712,11 +720,16 @@ impl WidgetRef for &ChatComposer<'_> { Span::from(" to quit"), ] } else { + let newline_hint_key = if self.use_shift_enter_hint { + "Shift+⏎" + } else { + "Ctrl+J" + }; vec![ Span::from(" "), "⏎".set_style(key_hint_style), Span::from(" send "), - "Shift+⏎".set_style(key_hint_style), + newline_hint_key.set_style(key_hint_style), Span::from(" newline "), "Ctrl+C".set_style(key_hint_style), Span::from(" quit"), @@ -890,7 +903,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); let needs_redraw = composer.handle_paste("hello".to_string()); assert!(needs_redraw); @@ -913,7 +926,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); let large = "x".repeat(LARGE_PASTE_CHAR_THRESHOLD + 10); let needs_redraw = composer.handle_paste(large.clone()); @@ -942,7 +955,7 @@ mod tests { let large = "y".repeat(LARGE_PASTE_CHAR_THRESHOLD + 1); let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); composer.handle_paste(large); assert_eq!(composer.pending_pastes.len(), 1); @@ -978,7 +991,7 @@ mod tests { for (name, input) in test_cases { // Create a fresh composer for each test case - let mut composer = ChatComposer::new(true, sender.clone()); + let mut composer = ChatComposer::new(true, sender.clone(), false); if let Some(text) = input { composer.handle_paste(text); @@ -1015,7 +1028,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); // Define test cases: (paste content, is_large) let test_cases = [ @@ -1088,7 +1101,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); // Define test cases: (content, is_large) let test_cases = [ @@ -1161,7 +1174,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); // Define test cases: (cursor_position_from_end, expected_pending_count) let test_cases = [ diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 2710a3e997..281f0859a5 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -50,12 +50,18 @@ pub(crate) struct BottomPane<'a> { pub(crate) struct BottomPaneParams { pub(crate) app_event_tx: AppEventSender, pub(crate) has_input_focus: bool, + pub(crate) enhanced_keys_supported: bool, } impl BottomPane<'_> { pub fn new(params: BottomPaneParams) -> Self { + let enhanced_keys_supported = params.enhanced_keys_supported; Self { - composer: ChatComposer::new(params.has_input_focus, params.app_event_tx.clone()), + composer: ChatComposer::new( + params.has_input_focus, + params.app_event_tx.clone(), + enhanced_keys_supported, + ), active_view: None, app_event_tx: params.app_event_tx, has_input_focus: params.has_input_focus, @@ -298,6 +304,7 @@ mod tests { let mut pane = BottomPane::new(BottomPaneParams { app_event_tx: tx, has_input_focus: true, + enhanced_keys_supported: false, }); pane.push_approval_request(exec_request()); assert_eq!(CancellationEvent::Handled, pane.on_ctrl_c()); diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap index 4f155dab30..178e2cc40f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap index 4e8371f177..7a1a7f4ef3 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap index 80fea40d5f..d3f67c9219 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap index 26e8d26733..999e7b2e3f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap index 0f1b9e6426..68d7711aab 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c4c2de7761..2908d9e5b5 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -26,6 +26,7 @@ use codex_core::protocol::PatchApplyBeginEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TokenUsage; use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; use ratatui::buffer::Buffer; use ratatui::layout::Rect; use ratatui::widgets::Widget; @@ -95,6 +96,7 @@ impl ChatWidget<'_> { app_event_tx: AppEventSender, initial_prompt: Option, initial_images: Vec, + enhanced_keys_supported: bool, ) -> Self { let (codex_op_tx, mut codex_op_rx) = unbounded_channel::(); @@ -140,6 +142,7 @@ impl ChatWidget<'_> { bottom_pane: BottomPane::new(BottomPaneParams { app_event_tx, has_input_focus: true, + enhanced_keys_supported, }), config, initial_user_message: create_initial_user_message( @@ -158,7 +161,9 @@ impl ChatWidget<'_> { } pub(crate) fn handle_key_event(&mut self, key_event: KeyEvent) { - self.bottom_pane.clear_ctrl_c_quit_hint(); + if key_event.kind == KeyEventKind::Press { + self.bottom_pane.clear_ctrl_c_quit_hint(); + } match self.bottom_pane.handle_key_event(key_event) { InputResult::Submitted(text) => { @@ -298,6 +303,10 @@ impl ChatWidget<'_> { self.add_to_history(HistoryCell::new_error_event(message.clone())); self.bottom_pane.set_task_running(false); } + EventMsg::PlanUpdate(update) => { + self.add_to_history(HistoryCell::new_plan_update(update)); + self.request_redraw(); + } EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { call_id: _, command, @@ -498,6 +507,12 @@ impl ChatWidget<'_> { pub(crate) fn token_usage(&self) -> &TokenUsage { &self.token_usage } + + pub(crate) fn clear_token_usage(&mut self) { + self.token_usage = TokenUsage::default(); + self.bottom_pane + .set_token_usage(self.token_usage.clone(), self.config.model_context_window); + } } impl WidgetRef for &ChatWidget<'_> { diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 956a0cc7ef..0e9f25332d 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -9,6 +9,9 @@ use codex_common::summarize_sandbox_policy; use codex_core::WireApi; use codex_core::config::Config; use codex_core::model_supports_reasoning_summaries; +use codex_core::plan_tool::PlanItemArg; +use codex_core::plan_tool::StepStatus; +use codex_core::plan_tool::UpdatePlanArgs; use codex_core::protocol::FileChange; use codex_core::protocol::McpInvocation; use codex_core::protocol::SessionConfiguredEvent; @@ -109,6 +112,10 @@ pub(crate) enum HistoryCell { /// behaviour of `ActiveExecCommand` so the user sees *what* patch the /// model wants to apply before being prompted to approve or deny it. PendingPatch { view: TextBlock }, + + /// A human‑friendly rendering of the model's current plan and step + /// statuses provided via the `update_plan` tool. + PlanUpdate { view: TextBlock }, } const TOOL_CALL_MAX_LINES: usize = 5; @@ -130,6 +137,7 @@ impl HistoryCell { | HistoryCell::CompletedExecCommand { view } | HistoryCell::CompletedMcpToolCall { view } | HistoryCell::PendingPatch { view } + | HistoryCell::PlanUpdate { view } | HistoryCell::ActiveExecCommand { view, .. } | HistoryCell::ActiveMcpToolCall { view, .. } => { view.lines.iter().map(line_to_static).collect() @@ -477,6 +485,94 @@ impl HistoryCell { } } + /// Render a user‑friendly plan update with colourful status icons and a + /// simple progress indicator so users can follow along. + pub(crate) fn new_plan_update(update: UpdatePlanArgs) -> Self { + let UpdatePlanArgs { explanation, plan } = update; + + let mut lines: Vec> = Vec::new(); + + // Title + lines.push(Line::from("plan".magenta().bold())); + + if !plan.is_empty() { + // Progress bar – show completed/total with a visual bar + let total = plan.len(); + let completed = plan + .iter() + .filter(|p| matches!(p.status, StepStatus::Completed)) + .count(); + let width: usize = 20; + let filled = (completed * width + total / 2) / total; + let empty = width.saturating_sub(filled); + let mut bar_spans: Vec = Vec::new(); + if filled > 0 { + bar_spans.push(Span::styled( + "█".repeat(filled), + Style::default().fg(Color::Green), + )); + } + if empty > 0 { + bar_spans.push(Span::styled( + "░".repeat(empty), + Style::default().fg(Color::Gray), + )); + } + let progress_prefix = Span::raw("progress ["); + let progress_suffix = Span::raw("] "); + let fraction = Span::raw(format!("{completed}/{total}")); + let mut progress_line_spans = vec![progress_prefix]; + progress_line_spans.extend(bar_spans); + progress_line_spans.push(progress_suffix); + progress_line_spans.push(fraction); + lines.push(Line::from(progress_line_spans)); + } + + // Optional explanation/note from the model + if let Some(expl) = explanation.and_then(|s| { + let t = s.trim().to_string(); + if t.is_empty() { None } else { Some(t) } + }) { + lines.push(Line::from("note".gray().italic())); + for l in expl.lines() { + lines.push(Line::from(l.to_string()).gray()); + } + } + + // Steps (1‑based numbering) with fun, readable status icons + if plan.is_empty() { + lines.push(Line::from("(no steps provided)".gray().italic())); + } else { + for (idx, PlanItemArg { step, status }) in plan.into_iter().enumerate() { + let num = idx + 1; + let (icon, style): (&str, Style) = match status { + StepStatus::Completed => ("✓", Style::default().fg(Color::Green)), + StepStatus::InProgress => ( + "▶", + Style::default() + .fg(Color::Yellow) + .add_modifier(Modifier::BOLD), + ), + StepStatus::Pending => ("○", Style::default().fg(Color::Gray)), + }; + let prefix = vec![ + Span::raw(format!("{num:>2}. [")), + Span::styled(icon.to_string(), style), + Span::raw("] "), + ]; + let mut spans = prefix; + spans.push(Span::raw(step)); + lines.push(Line::from(spans)); + } + } + + lines.push(Line::from("")); + + HistoryCell::PlanUpdate { + view: TextBlock::new(lines), + } + } + /// Create a new `PendingPatch` cell that lists the file‑level summary of /// a proposed patch. The summary lines should already be formatted (e.g. /// "A path/to/file.rs"). diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index 54faf4beb8..efd08a71c2 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -36,12 +36,12 @@ pub(crate) fn insert_history_lines(terminal: &mut tui::Tui, lines: Vec) { .backend_mut() .scroll_region_down(area.top()..screen_size.height, scroll_amount) .ok(); - let cursor_top = area.top() - 1; + let cursor_top = area.top().saturating_sub(1); area.y += scroll_amount; terminal.set_viewport_area(area); cursor_top } else { - area.top() - 1 + area.top().saturating_sub(1) }; // Limit the scroll region to the lines from the top of the screen to the diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 6b5fe7f7ae..f0a0e9d833 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -75,7 +75,7 @@ pub async fn run_main( config_profile: cli.config_profile.clone(), codex_linux_sandbox_exe, base_instructions: None, - include_plan_tool: None, + include_plan_tool: Some(true), }; // Parse `-c` overrides from the CLI. let cli_kv_overrides = match cli.config_overrides.parse_overrides() { @@ -226,7 +226,7 @@ fn should_show_login_screen(config: &Config) -> bool { // Reading the OpenAI API key is an async operation because it may need // to refresh the token. Block on it. let codex_home = config.codex_home.clone(); - match load_auth(&codex_home) { + match load_auth(&codex_home, true) { Ok(Some(_)) => false, Ok(None) => true, Err(err) => { diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index 7df1bcbdec..d82a16608f 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -13,6 +13,7 @@ pub enum SlashCommand { // DO NOT ALPHA-SORT! Enum order is presentation order in the popup, so // more frequently used commands should be listed first. New, + Compact, Diff, Quit, #[cfg(debug_assertions)] @@ -24,6 +25,7 @@ impl SlashCommand { pub fn description(self) -> &'static str { match self { SlashCommand::New => "Start a new chat.", + SlashCommand::Compact => "Compact the chat history.", SlashCommand::Quit => "Exit the application.", SlashCommand::Diff => { "Show git diff of the working directory (including untracked files)" diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index 855a7ea3db..91febde208 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -7,23 +7,24 @@ //! driven workflow – a fully‑fledged visual match is not required. use std::path::PathBuf; +use std::sync::LazyLock; use codex_core::protocol::Op; use codex_core::protocol::ReviewDecision; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; use ratatui::buffer::Buffer; use ratatui::layout::Rect; use ratatui::prelude::*; use ratatui::text::Line; -use ratatui::text::Span; -use ratatui::widgets::List; +use ratatui::widgets::Block; +use ratatui::widgets::BorderType; +use ratatui::widgets::Borders; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use ratatui::widgets::WidgetRef; use ratatui::widgets::Wrap; -use tui_input::Input; -use tui_input::backend::crossterm::EventHandler; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; @@ -47,68 +48,62 @@ pub(crate) enum ApprovalRequest { /// Options displayed in the *select* mode. struct SelectOption { - label: &'static str, - decision: Option, - /// `true` when this option switches the widget to *input* mode. - enters_input_mode: bool, + label: Line<'static>, + description: &'static str, + key: KeyCode, + decision: ReviewDecision, } -// keep in same order as in the TS implementation -const SELECT_OPTIONS: &[SelectOption] = &[ - SelectOption { - label: "Yes (y)", - decision: Some(ReviewDecision::Approved), +static COMMAND_SELECT_OPTIONS: LazyLock> = LazyLock::new(|| { + vec![ + SelectOption { + label: Line::from(vec!["Y".underlined(), "es".into()]), + description: "Approve and run the command", + key: KeyCode::Char('y'), + decision: ReviewDecision::Approved, + }, + SelectOption { + label: Line::from(vec!["A".underlined(), "lways".into()]), + description: "Approve the command for the remainder of this session", + key: KeyCode::Char('a'), + decision: ReviewDecision::ApprovedForSession, + }, + SelectOption { + label: Line::from(vec!["N".underlined(), "o".into()]), + description: "Do not run the command", + key: KeyCode::Char('n'), + decision: ReviewDecision::Denied, + }, + ] +}); - enters_input_mode: false, - }, - SelectOption { - label: "Yes, always approve this exact command for this session (a)", - decision: Some(ReviewDecision::ApprovedForSession), - - enters_input_mode: false, - }, - SelectOption { - label: "Edit or give feedback (e)", - decision: None, - - enters_input_mode: true, - }, - SelectOption { - label: "No, and keep going (n)", - decision: Some(ReviewDecision::Denied), - - enters_input_mode: false, - }, - SelectOption { - label: "No, and stop for now (esc)", - decision: Some(ReviewDecision::Abort), - - enters_input_mode: false, - }, -]; - -/// Internal mode the widget is in – mirrors the TypeScript component. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum Mode { - Select, - Input, -} +static PATCH_SELECT_OPTIONS: LazyLock> = LazyLock::new(|| { + vec![ + SelectOption { + label: Line::from(vec!["Y".underlined(), "es".into()]), + description: "Approve and apply the changes", + key: KeyCode::Char('y'), + decision: ReviewDecision::Approved, + }, + SelectOption { + label: Line::from(vec!["N".underlined(), "o".into()]), + description: "Do not apply the changes", + key: KeyCode::Char('n'), + decision: ReviewDecision::Denied, + }, + ] +}); /// A modal prompting the user to approve or deny the pending request. pub(crate) struct UserApprovalWidget<'a> { approval_request: ApprovalRequest, app_event_tx: AppEventSender, confirmation_prompt: Paragraph<'a>, + select_options: &'a Vec, /// Currently selected index in *select* mode. selected_option: usize, - /// State for the optional input widget. - input: Input, - - /// Current mode. - mode: Mode, - /// Set to `true` once a decision has been sent – the parent view can then /// remove this widget from its queue. done: bool, @@ -116,7 +111,6 @@ pub(crate) struct UserApprovalWidget<'a> { impl UserApprovalWidget<'_> { pub(crate) fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self { - let input = Input::default(); let confirmation_prompt = match &approval_request { ApprovalRequest::Exec { command, @@ -132,25 +126,20 @@ impl UserApprovalWidget<'_> { None => cwd.display().to_string(), }; let mut contents: Vec = vec![ - Line::from(vec![ - Span::from(cwd_str).dim(), - Span::from("$"), - Span::from(format!(" {cmd}")), - ]), + Line::from(vec!["codex".bold().magenta(), " wants to run:".into()]), + Line::from(vec![cwd_str.dim(), "$".into(), format!(" {cmd}").into()]), Line::from(""), ]; if let Some(reason) = reason { contents.push(Line::from(reason.clone().italic())); contents.push(Line::from("")); } - contents.extend(vec![Line::from("Allow command?"), Line::from("")]); Paragraph::new(contents).wrap(Wrap { trim: false }) } ApprovalRequest::ApplyPatch { reason, grant_root, .. } => { - let mut contents: Vec = - vec![Line::from("Apply patch".bold()), Line::from("")]; + let mut contents: Vec = vec![]; if let Some(r) = reason { contents.push(Line::from(r.clone().italic())); @@ -165,20 +154,19 @@ impl UserApprovalWidget<'_> { contents.push(Line::from("")); } - contents.push(Line::from("Allow changes?")); - contents.push(Line::from("")); - - Paragraph::new(contents) + Paragraph::new(contents).wrap(Wrap { trim: false }) } }; Self { + select_options: match &approval_request { + ApprovalRequest::Exec { .. } => &COMMAND_SELECT_OPTIONS, + ApprovalRequest::ApplyPatch { .. } => &PATCH_SELECT_OPTIONS, + }, approval_request, app_event_tx, confirmation_prompt, selected_option: 0, - input, - mode: Mode::Select, done: false, } } @@ -194,9 +182,8 @@ impl UserApprovalWidget<'_> { /// captures input while visible, we don’t need to report whether the event /// was consumed—callers can assume it always is. pub(crate) fn handle_key_event(&mut self, key: KeyEvent) { - match self.mode { - Mode::Select => self.handle_select_key(key), - Mode::Input => self.handle_input_key(key), + if key.kind == KeyEventKind::Press { + self.handle_select_key(key); } } @@ -208,58 +195,24 @@ impl UserApprovalWidget<'_> { fn handle_select_key(&mut self, key_event: KeyEvent) { match key_event.code { - KeyCode::Up => { - if self.selected_option == 0 { - self.selected_option = SELECT_OPTIONS.len() - 1; - } else { - self.selected_option -= 1; - } + KeyCode::Left => { + self.selected_option = (self.selected_option + self.select_options.len() - 1) + % self.select_options.len(); } - KeyCode::Down => { - self.selected_option = (self.selected_option + 1) % SELECT_OPTIONS.len(); - } - KeyCode::Char('y') => { - self.send_decision(ReviewDecision::Approved); - } - KeyCode::Char('a') => { - self.send_decision(ReviewDecision::ApprovedForSession); - } - KeyCode::Char('n') => { - self.send_decision(ReviewDecision::Denied); - } - KeyCode::Char('e') => { - self.mode = Mode::Input; + KeyCode::Right => { + self.selected_option = (self.selected_option + 1) % self.select_options.len(); } KeyCode::Enter => { - let opt = &SELECT_OPTIONS[self.selected_option]; - if opt.enters_input_mode { - self.mode = Mode::Input; - } else if let Some(decision) = opt.decision { - self.send_decision(decision); - } + let opt = &self.select_options[self.selected_option]; + self.send_decision(opt.decision); } KeyCode::Esc => { self.send_decision(ReviewDecision::Abort); } - _ => {} - } - } - - fn handle_input_key(&mut self, key_event: KeyEvent) { - // Handle special keys first. - match key_event.code { - KeyCode::Enter => { - let feedback = self.input.value().to_string(); - self.send_decision_with_feedback(ReviewDecision::Denied, feedback); - } - KeyCode::Esc => { - // Cancel input – treat as deny without feedback. - self.send_decision(ReviewDecision::Denied); - } - _ => { - // Feed into input widget for normal editing. - let ct_event = crossterm::event::Event::Key(key_event); - self.input.handle_event(&ct_event); + other => { + if let Some(opt) = self.select_options.iter().find(|opt| opt.key == other) { + self.send_decision(opt.decision); + } } } } @@ -312,87 +265,68 @@ impl UserApprovalWidget<'_> { } pub(crate) fn desired_height(&self, width: u16) -> u16 { - self.get_confirmation_prompt_height(width - 2) + SELECT_OPTIONS.len() as u16 + 2 + self.get_confirmation_prompt_height(width) + self.select_options.len() as u16 } } -const PLAIN: Style = Style::new(); -const BLUE_FG: Style = Style::new().fg(Color::LightCyan); - impl WidgetRef for &UserApprovalWidget<'_> { fn render_ref(&self, area: Rect, buf: &mut Buffer) { - // Take the area, wrap it in a block with a border, and divide up the - // remaining area into two chunks: one for the confirmation prompt and - // one for the response. - let inner = area.inner(Margin::new(0, 2)); - - // Determine how many rows we can allocate for the static confirmation - // prompt while *always* keeping enough space for the interactive - // response area (select list or input field). When the full prompt - // would exceed the available height we truncate it so the response - // options never get pushed out of view. This keeps the approval modal - // usable even when the overall bottom viewport is small. - - // Full height of the prompt (may be larger than the available area). - let full_prompt_height = self.get_confirmation_prompt_height(inner.width); - - // Minimum rows that must remain for the interactive section. - let min_response_rows = match self.mode { - Mode::Select => SELECT_OPTIONS.len() as u16, - // In input mode we need exactly two rows: one for the guidance - // prompt and one for the single-line input field. - Mode::Input => 2, - }; - - // Clamp prompt height so confirmation + response never exceed the - // available space. `saturating_sub` avoids underflow when the area is - // too small even for the minimal layout – in this unlikely case we - // fall back to zero-height prompt so at least the options are - // visible. - let prompt_height = full_prompt_height.min(inner.height.saturating_sub(min_response_rows)); - - let chunks = Layout::default() + let prompt_height = self.get_confirmation_prompt_height(area.width); + let [prompt_chunk, response_chunk] = Layout::default() .direction(Direction::Vertical) .constraints([Constraint::Length(prompt_height), Constraint::Min(0)]) - .split(inner); - let prompt_chunk = chunks[0]; - let response_chunk = chunks[1]; + .areas(area); - // Build the inner lines based on the mode. Collect them into a List of - // non-wrapping lines rather than a Paragraph for predictable layout. - let lines = match self.mode { - Mode::Select => SELECT_OPTIONS - .iter() - .enumerate() - .map(|(idx, opt)| { - let (prefix, style) = if idx == self.selected_option { - ("▶", BLUE_FG) - } else { - (" ", PLAIN) - }; - Line::styled(format!(" {prefix} {}", opt.label), style) - }) - .collect(), - Mode::Input => { - vec![ - Line::from("Give the model feedback on this command:"), - Line::from(self.input.value()), - ] - } + let lines: Vec = self + .select_options + .iter() + .enumerate() + .map(|(idx, opt)| { + let style = if idx == self.selected_option { + Style::new().bg(Color::Cyan).fg(Color::Black) + } else { + Style::new().bg(Color::DarkGray) + }; + opt.label.clone().alignment(Alignment::Center).style(style) + }) + .collect(); + + let [title_area, button_area, description_area] = Layout::vertical([ + Constraint::Length(1), + Constraint::Length(1), + Constraint::Min(0), + ]) + .areas(response_chunk.inner(Margin::new(1, 0))); + let title = match &self.approval_request { + ApprovalRequest::Exec { .. } => "Allow command?", + ApprovalRequest::ApplyPatch { .. } => "Apply changes?", }; - - let border = ("◢◤") - .repeat((area.width / 2).into()) - .fg(Color::LightYellow); - - border.render_ref(area, buf); - Paragraph::new(" Execution Request ".bold().black().on_light_yellow()) - .alignment(Alignment::Center) - .render_ref(area, buf); + Line::from(title).render(title_area, buf); self.confirmation_prompt.clone().render(prompt_chunk, buf); - List::new(lines).render_ref(response_chunk, buf); + let areas = Layout::horizontal( + lines + .iter() + .map(|l| Constraint::Length(l.width() as u16 + 2)), + ) + .spacing(1) + .split(button_area); + for (idx, area) in areas.iter().enumerate() { + let line = &lines[idx]; + line.render(*area, buf); + } - border.render_ref(Rect::new(0, area.y + area.height - 1, area.width, 1), buf); + Line::from(self.select_options[self.selected_option].description) + .style(Style::new().italic().fg(Color::DarkGray)) + .render(description_area.inner(Margin::new(1, 0)), buf); + + Block::bordered() + .border_type(BorderType::QuadrantOutside) + .border_style(Style::default().fg(Color::Cyan)) + .borders(Borders::LEFT) + .render_ref( + Rect::new(0, response_chunk.y, 1, response_chunk.height), + buf, + ); } }