diff --git a/.github/workflows/rust-release.yml b/.github/workflows/rust-release.yml index 92b47873d3..c122378d7d 100644 --- a/.github/workflows/rust-release.yml +++ b/.github/workflows/rust-release.yml @@ -11,6 +11,9 @@ on: tags: - "rust-v*.*.*" +permissions: + id-token: write # Required for OIDC + concurrency: group: ${{ github.workflow }} cancel-in-progress: true @@ -187,6 +190,20 @@ jobs: version="${GITHUB_REF_NAME#rust-v}" echo "name=${version}" >> $GITHUB_OUTPUT + # Publish to npm using OIDC authentication. + # July 31, 2025: https://github.blog/changelog/2025-07-31-npm-trusted-publishing-with-oidc-is-generally-available/ + # npm docs: https://docs.npmjs.com/trusted-publishers + - name: Setup Node.js + uses: actions/setup-node@v5 + with: + node-version: 22 + registry-url: "https://registry.npmjs.org" + scope: "@openai" + + # Trusted publishing requires npm CLI version 11.5.1 or later. + - name: Update npm + run: npm install -g npm@latest + - name: Stage npm package env: GH_TOKEN: ${{ github.token }} @@ -215,6 +232,10 @@ jobs: tag: ${{ github.ref_name }} config: .github/dotslash-config.json + # No NODE_AUTH_TOKEN needed because we use OIDC. + - name: Publish to npm + run: npm publish "${GITHUB_WORKSPACE}/dist/npm/codex-npm-${{ steps.release_name.outputs.name }}.tgz" + update-branch: name: Update latest-alpha-cli branch permissions: @@ -233,3 +254,4 @@ jobs: -X PATCH \ -f sha="${GITHUB_SHA}" \ -F force=true + diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a6f32b35c5..81fc8683cb 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3926,9 +3926,9 @@ dependencies = [ [[package]] name = "serde" -version = "1.0.223" +version = "1.0.224" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a505d71960adde88e293da5cb5eda57093379f64e61cf77bf0e6a63af07a7bac" +checksum = "6aaeb1e94f53b16384af593c71e20b095e958dab1d26939c1b70645c5cfbcc0b" dependencies = [ "serde_core", "serde_derive", @@ -3936,18 +3936,18 @@ dependencies = [ [[package]] name = "serde_core" -version = "1.0.223" +version = "1.0.224" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "20f57cbd357666aa7b3ac84a90b4ea328f1d4ddb6772b430caa5d9e1309bb9e9" +checksum = "32f39390fa6346e24defbcdd3d9544ba8a19985d0af74df8501fbfe9a64341ab" dependencies = [ "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.223" +version = "1.0.224" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d428d07faf17e306e699ec1e91996e5a165ba5d6bce5b5155173e91a8a01a56" +checksum = "87ff78ab5e8561c9a675bfc1785cb07ae721f0ee53329a595cefd8c04c2ac4e0" dependencies = [ "proc-macro2", "quote", @@ -5292,9 +5292,9 @@ dependencies = [ [[package]] name = "wildmatch" -version = "2.4.0" +version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "68ce1ab1f8c62655ebe1350f589c61e505cf94d385bc6a12899442d9081e71fd" +checksum = "39b7d07a236abaef6607536ccfaf19b396dbe3f5110ddb73d39f4562902ed382" [[package]] name = "winapi" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 4fb10affb3..b4ed4a937a 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -57,7 +57,7 @@ tree-sitter = "0.25.9" tree-sitter-bash = "0.25.0" uuid = { version = "1", features = ["serde", "v4"] } which = "6" -wildmatch = "2.4.0" +wildmatch = "2.5.0" [target.'cfg(target_os = "linux")'.dependencies] diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 375183ab82..b3b75ec76a 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -11,6 +11,7 @@ use std::time::Duration; use crate::AuthManager; use crate::client_common::REVIEW_PROMPT; use crate::event_mapping::map_response_item_to_event_messages; +use crate::review_format::format_review_findings_block; use async_channel::Receiver; use async_channel::Sender; use codex_apply_patch::ApplyPatchAction; @@ -1154,20 +1155,16 @@ impl AgentTask { fn abort(self, reason: TurnAbortReason) { // TOCTOU? if !self.handle.is_finished() { - if self.kind == AgentTaskKind::Review { - let sess = self.sess.clone(); - let sub_id = self.sub_id.clone(); - tokio::spawn(async move { - exit_review_mode(sess, sub_id, None).await; - }); - } self.handle.abort(); let event = Event { - id: self.sub_id, + id: self.sub_id.clone(), msg: EventMsg::TurnAborted(TurnAbortedEvent { reason }), }; let sess = self.sess; tokio::spawn(async move { + if self.kind == AgentTaskKind::Review { + exit_review_mode(sess.clone(), self.sub_id, None).await; + } sess.send_event(event).await; }); } @@ -1348,10 +1345,21 @@ async fn submission_loop( cwd, is_review_mode: false, }; - // TODO: record the new environment context in the conversation history + + // if the environment context has changed, record it in the conversation history + let previous_env_context = EnvironmentContext::from(turn_context.as_ref()); + let new_env_context = EnvironmentContext::from(&fresh_turn_context); + if !new_env_context.equals_except_shell(&previous_env_context) { + sess.record_conversation_items(&[ResponseItem::from(new_env_context)]) + .await; + } + + // Install the new persistent context for subsequent tasks/turns. + turn_context = Arc::new(fresh_turn_context); + // no current task, spawn a new one with the per‑turn context let task = - AgentTask::spawn(sess.clone(), Arc::new(fresh_turn_context), sub.id, items); + AgentTask::spawn(sess.clone(), Arc::clone(&turn_context), sub.id, items); sess.set_task(task); } } @@ -1549,7 +1557,8 @@ async fn spawn_review_thread( experimental_unified_exec_tool: config.use_experimental_unified_exec_tool, }); - let base_instructions = Some(REVIEW_PROMPT.to_string()); + let base_instructions = REVIEW_PROMPT.to_string(); + let review_prompt = review_request.prompt.clone(); let provider = parent_turn_context.client.get_provider(); let auth_manager = parent_turn_context.client.get_auth_manager(); let model_family = review_model_family.clone(); @@ -1558,16 +1567,19 @@ async fn spawn_review_thread( let mut per_turn_config = (*config).clone(); per_turn_config.model = model.clone(); per_turn_config.model_family = model_family.clone(); + per_turn_config.model_reasoning_effort = Some(ReasoningEffortConfig::Low); + per_turn_config.model_reasoning_summary = ReasoningSummaryConfig::Detailed; if let Some(model_info) = get_model_info(&model_family) { per_turn_config.model_context_window = Some(model_info.context_window); } + let per_turn_config = Arc::new(per_turn_config); let client = ModelClient::new( - Arc::new(per_turn_config), + per_turn_config.clone(), auth_manager, provider, - parent_turn_context.client.get_reasoning_effort(), - parent_turn_context.client.get_reasoning_summary(), + per_turn_config.model_reasoning_effort, + per_turn_config.model_reasoning_summary, sess.conversation_id, ); @@ -1575,7 +1587,7 @@ async fn spawn_review_thread( client, tools_config, user_instructions: None, - base_instructions, + base_instructions: Some(base_instructions.clone()), approval_policy: parent_turn_context.approval_policy, sandbox_policy: parent_turn_context.sandbox_policy.clone(), shell_environment_policy: parent_turn_context.shell_environment_policy.clone(), @@ -1585,7 +1597,7 @@ async fn spawn_review_thread( // Seed the child task with the review prompt as the initial user message. let input: Vec = vec![InputItem::Text { - text: review_request.prompt.clone(), + text: format!("{base_instructions}\n\n---\n\nNow, here's your task: {review_prompt}"), }]; let tc = Arc::new(review_turn_context); @@ -1643,6 +1655,8 @@ async fn run_task( let is_review_mode = turn_context.is_review_mode; let mut review_thread_history: Vec = Vec::new(); if is_review_mode { + // Seed review threads with environment context so the model knows the working directory. + review_thread_history.extend(sess.build_initial_context(turn_context.as_ref())); review_thread_history.push(initial_input_for_turn.into()); } else { sess.record_input_and_rollout_usermsg(&initial_input_for_turn) @@ -3246,7 +3260,8 @@ fn convert_call_tool_result_to_function_call_output_payload( } } -/// Emits an ExitedReviewMode Event with optional ReviewOutput. +/// Emits an ExitedReviewMode Event with optional ReviewOutput, +/// and records a developer message with the review output. async fn exit_review_mode( session: Arc, task_sub_id: String, @@ -3254,9 +3269,50 @@ async fn exit_review_mode( ) { let event = Event { id: task_sub_id, - msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output }), + msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: review_output.clone(), + }), }; session.send_event(event).await; + + let mut user_message = String::new(); + if let Some(out) = review_output { + let mut findings_str = String::new(); + let text = out.overall_explanation.trim(); + if !text.is_empty() { + findings_str.push_str(text); + } + if !out.findings.is_empty() { + let block = format_review_findings_block(&out.findings, None); + findings_str.push_str(&format!("\n{block}")); + } + user_message.push_str(&format!( + r#" + User initiated a review task. Here's the full review output from reviewer model. User may select one or more comments to resolve. + review + + {findings_str} + + +"#)); + } else { + user_message.push_str(r#" + User initiated a review task, but was interrupted. If user asks about this, tell them to re-initiate a review with `/review` and wait for it to complete. + review + + None. + + +"#); + } + + session + .record_conversation_items(&[ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { text: user_message }], + }]) + .await; } #[cfg(test)] diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 0479e857c2..6a613abbba 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -38,7 +38,7 @@ use toml_edit::Item as TomlItem; use toml_edit::Table as TomlTable; const OPENAI_DEFAULT_MODEL: &str = "gpt-5"; -const OPENAI_DEFAULT_REVIEW_MODEL: &str = "gpt-5"; +const OPENAI_DEFAULT_REVIEW_MODEL: &str = "gpt-5-codex"; pub const GPT_5_CODEX_MEDIUM_MODEL: &str = "gpt-5-codex"; /// Maximum number of bytes of the documentation that will be embedded. Larger @@ -1581,7 +1581,7 @@ model_verbosity = "high" assert_eq!( Config { model: "o3".to_string(), - review_model: "gpt-5".to_string(), + review_model: OPENAI_DEFAULT_REVIEW_MODEL.to_string(), model_family: find_family_for_model("o3").expect("known model slug"), model_context_window: Some(200_000), model_max_output_tokens: Some(100_000), @@ -1639,7 +1639,7 @@ model_verbosity = "high" )?; let expected_gpt3_profile_config = Config { model: "gpt-3.5-turbo".to_string(), - review_model: "gpt-5".to_string(), + review_model: OPENAI_DEFAULT_REVIEW_MODEL.to_string(), model_family: find_family_for_model("gpt-3.5-turbo").expect("known model slug"), model_context_window: Some(16_385), model_max_output_tokens: Some(4_096), @@ -1712,7 +1712,7 @@ model_verbosity = "high" )?; let expected_zdr_profile_config = Config { model: "o3".to_string(), - review_model: "gpt-5".to_string(), + review_model: OPENAI_DEFAULT_REVIEW_MODEL.to_string(), model_family: find_family_for_model("o3").expect("known model slug"), model_context_window: Some(200_000), model_max_output_tokens: Some(100_000), @@ -1771,7 +1771,7 @@ model_verbosity = "high" )?; let expected_gpt5_profile_config = Config { model: "gpt-5".to_string(), - review_model: "gpt-5".to_string(), + review_model: OPENAI_DEFAULT_REVIEW_MODEL.to_string(), model_family: find_family_for_model("gpt-5").expect("known model slug"), model_context_window: Some(272_000), model_max_output_tokens: Some(128_000), diff --git a/codex-rs/core/src/conversation_history.rs b/codex-rs/core/src/conversation_history.rs index 891a2ea472..7c23e4dc29 100644 --- a/codex-rs/core/src/conversation_history.rs +++ b/codex-rs/core/src/conversation_history.rs @@ -47,8 +47,9 @@ fn is_api_message(message: &ResponseItem) -> bool { | ResponseItem::CustomToolCall { .. } | ResponseItem::CustomToolCallOutput { .. } | ResponseItem::LocalShellCall { .. } - | ResponseItem::Reasoning { .. } => true, - ResponseItem::WebSearchCall { .. } | ResponseItem::Other => false, + | ResponseItem::Reasoning { .. } + | ResponseItem::WebSearchCall { .. } => true, + ResponseItem::Other => false, } } diff --git a/codex-rs/core/src/environment_context.rs b/codex-rs/core/src/environment_context.rs index 89af9e1c91..8f3292a226 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/environment_context.rs @@ -2,6 +2,7 @@ use serde::Deserialize; use serde::Serialize; use strum_macros::Display as DeriveDisplay; +use crate::codex::TurnContext; use crate::protocol::AskForApproval; use crate::protocol::SandboxPolicy; use crate::shell::Shell; @@ -71,6 +72,39 @@ impl EnvironmentContext { shell, } } + + /// Compares two environment contexts, ignoring the shell. Useful when + /// comparing turn to turn, since the initial environment_context will + /// include the shell, and then it is not configurable from turn to turn. + pub fn equals_except_shell(&self, other: &EnvironmentContext) -> bool { + let EnvironmentContext { + cwd, + approval_policy, + sandbox_mode, + network_access, + writable_roots, + // should compare all fields except shell + shell: _, + } = other; + + self.cwd == *cwd + && self.approval_policy == *approval_policy + && self.sandbox_mode == *sandbox_mode + && self.network_access == *network_access + && self.writable_roots == *writable_roots + } +} + +impl From<&TurnContext> for EnvironmentContext { + fn from(turn_context: &TurnContext) -> Self { + Self::new( + Some(turn_context.cwd.clone()), + Some(turn_context.approval_policy), + Some(turn_context.sandbox_policy.clone()), + // Shell is not configurable from turn to turn + None, + ) + } } impl EnvironmentContext { @@ -140,6 +174,9 @@ impl From for ResponseItem { #[cfg(test)] mod tests { + use crate::shell::BashShell; + use crate::shell::ZshShell; + use super::*; use pretty_assertions::assert_eq; @@ -210,4 +247,82 @@ mod tests { assert_eq!(context.serialize_to_xml(), expected); } + + #[test] + fn equals_except_shell_compares_approval_policy() { + // Approval policy + let context1 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::OnRequest), + Some(workspace_write_policy(vec!["/repo"], false)), + None, + ); + let context2 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::Never), + Some(workspace_write_policy(vec!["/repo"], true)), + None, + ); + assert!(!context1.equals_except_shell(&context2)); + } + + #[test] + fn equals_except_shell_compares_sandbox_policy() { + let context1 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::OnRequest), + Some(SandboxPolicy::new_read_only_policy()), + None, + ); + let context2 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::OnRequest), + Some(SandboxPolicy::new_workspace_write_policy()), + None, + ); + + assert!(!context1.equals_except_shell(&context2)); + } + + #[test] + fn equals_except_shell_compares_workspace_write_policy() { + let context1 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::OnRequest), + Some(workspace_write_policy(vec!["/repo", "/tmp", "/var"], false)), + None, + ); + let context2 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::OnRequest), + Some(workspace_write_policy(vec!["/repo", "/tmp"], true)), + None, + ); + + assert!(!context1.equals_except_shell(&context2)); + } + + #[test] + fn equals_except_shell_ignores_shell() { + let context1 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::OnRequest), + Some(workspace_write_policy(vec!["/repo"], false)), + Some(Shell::Bash(BashShell { + shell_path: "/bin/bash".into(), + bashrc_path: "/home/user/.bashrc".into(), + })), + ); + let context2 = EnvironmentContext::new( + Some(PathBuf::from("/repo")), + Some(AskForApproval::OnRequest), + Some(workspace_write_policy(vec!["/repo"], false)), + Some(Shell::Zsh(ZshShell { + shell_path: "/bin/zsh".into(), + zshrc_path: "/home/user/.zshrc".into(), + })), + ); + + assert!(context1.equals_except_shell(&context2)); + } } diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 7b9c3dc9f0..e024effbe2 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -46,6 +46,7 @@ pub use model_provider_info::built_in_model_providers; pub use model_provider_info::create_oss_provider_with_base_url; mod conversation_manager; mod event_mapping; +pub mod review_format; pub use codex_protocol::protocol::InitialHistory; pub use conversation_manager::ConversationManager; pub use conversation_manager::NewConversation; @@ -88,6 +89,7 @@ pub use codex_protocol::config_types as protocol_config_types; pub use client::ModelClient; pub use client_common::Prompt; +pub use client_common::REVIEW_PROMPT; pub use client_common::ResponseEvent; pub use client_common::ResponseStream; pub use codex_protocol::models::ContentItem; diff --git a/codex-rs/core/src/openai_tools.rs b/codex-rs/core/src/openai_tools.rs index a511d6dd16..f4d724815e 100644 --- a/codex-rs/core/src/openai_tools.rs +++ b/codex-rs/core/src/openai_tools.rs @@ -273,7 +273,7 @@ fn create_shell_tool_for_sandbox(sandbox_policy: &SandboxPolicy) -> OpenAiTool { }, ); - if matches!(sandbox_policy, SandboxPolicy::WorkspaceWrite { .. }) { + if !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess) { properties.insert( "with_escalated_permissions".to_string(), JsonSchema::Boolean { diff --git a/codex-rs/core/src/review_format.rs b/codex-rs/core/src/review_format.rs new file mode 100644 index 0000000000..272010d564 --- /dev/null +++ b/codex-rs/core/src/review_format.rs @@ -0,0 +1,55 @@ +use crate::protocol::ReviewFinding; + +// Note: We keep this module UI-agnostic. It returns plain strings that +// higher layers (e.g., TUI) may style as needed. + +fn format_location(item: &ReviewFinding) -> String { + let path = item.code_location.absolute_file_path.display(); + let start = item.code_location.line_range.start; + let end = item.code_location.line_range.end; + format!("{path}:{start}-{end}") +} + +/// Format a full review findings block as plain text lines. +/// +/// - When `selection` is `Some`, each item line includes a checkbox marker: +/// "[x]" for selected items and "[ ]" for unselected. Missing indices +/// default to selected. +/// - When `selection` is `None`, the marker is omitted and a simple bullet is +/// rendered ("- Title — path:start-end"). +pub fn format_review_findings_block( + findings: &[ReviewFinding], + selection: Option<&[bool]>, +) -> String { + let mut lines: Vec = Vec::new(); + + // Header + let header = if findings.len() > 1 { + "Full review comments:" + } else { + "Review comment:" + }; + lines.push(header.to_string()); + + for (idx, item) in findings.iter().enumerate() { + lines.push(String::new()); + + let title = &item.title; + let location = format_location(item); + + if let Some(flags) = selection { + // Default to selected if index is out of bounds. + let checked = flags.get(idx).copied().unwrap_or(true); + let marker = if checked { "[x]" } else { "[ ]" }; + lines.push(format!("- {marker} {title} — {location}")); + } else { + lines.push(format!("- {title} — {location}")); + } + + for body_line in item.body.lines() { + lines.push(format!(" {body_line}")); + } + } + + lines.join("\n") +} diff --git a/codex-rs/core/src/rollout/policy.rs b/codex-rs/core/src/rollout/policy.rs index 0d8fee92b0..2fd0efb0dc 100644 --- a/codex-rs/core/src/rollout/policy.rs +++ b/codex-rs/core/src/rollout/policy.rs @@ -25,8 +25,9 @@ pub(crate) fn should_persist_response_item(item: &ResponseItem) -> bool { | ResponseItem::FunctionCall { .. } | ResponseItem::FunctionCallOutput { .. } | ResponseItem::CustomToolCall { .. } - | ResponseItem::CustomToolCallOutput { .. } => true, - ResponseItem::WebSearchCall { .. } | ResponseItem::Other => false, + | ResponseItem::CustomToolCallOutput { .. } + | ResponseItem::WebSearchCall { .. } => true, + ResponseItem::Other => false, } } diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 1734d5de65..cb278fdd46 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -5,20 +5,20 @@ use std::path::PathBuf; #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct ZshShell { - shell_path: String, - zshrc_path: String, + pub(crate) shell_path: String, + pub(crate) zshrc_path: String, } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct BashShell { - shell_path: String, - bashrc_path: String, + pub(crate) shell_path: String, + pub(crate) bashrc_path: String, } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct PowerShellConfig { - exe: String, // Executable name or path, e.g. "pwsh" or "powershell.exe". - bash_exe_fallback: Option, // In case the model generates a bash command. + pub(crate) exe: String, // Executable name or path, e.g. "pwsh" or "powershell.exe". + pub(crate) bash_exe_fallback: Option, // In case the model generates a bash command. } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index c6731fee34..a69f57a201 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -12,6 +12,7 @@ use codex_core::protocol::Op; use codex_core::protocol::SandboxPolicy; use codex_core::protocol_config_types::ReasoningEffort; use codex_core::protocol_config_types::ReasoningSummary; +use codex_core::shell::Shell; use codex_core::shell::default_user_shell; use core_test_support::load_default_config_for_test; use core_test_support::load_sse_fixture_with_id; @@ -23,6 +24,30 @@ use wiremock::ResponseTemplate; use wiremock::matchers::method; use wiremock::matchers::path; +fn text_user_input(text: String) -> serde_json::Value { + serde_json::json!({ + "type": "message", + "role": "user", + "content": [ { "type": "input_text", "text": text } ] + }) +} + +fn default_env_context_str(cwd: &str, shell: &Shell) -> String { + format!( + r#" + {} + on-request + read-only + restricted +{}"#, + cwd, + match shell.name() { + Some(name) => format!(" {name}\n"), + None => String::new(), + } + ) +} + /// Build minimal SSE stream with completed marker using the JSON fixture. fn sse_completed(id: &str) -> String { load_sse_fixture_with_id("tests/fixtures/completed_template.json", id) @@ -546,12 +571,262 @@ async fn per_turn_overrides_keep_cached_prefix_and_key_constant() { "role": "user", "content": [ { "type": "input_text", "text": "hello 2" } ] }); + let expected_env_text_2 = format!( + r#" + {} + never + workspace-write + enabled + + {} + +"#, + new_cwd.path().to_string_lossy(), + writable.path().to_string_lossy(), + ); + let expected_env_msg_2 = serde_json::json!({ + "type": "message", + "role": "user", + "content": [ { "type": "input_text", "text": expected_env_text_2 } ] + }); let expected_body2 = serde_json::json!( [ body1["input"].as_array().unwrap().as_slice(), - [expected_user_message_2].as_slice(), + [expected_env_msg_2, expected_user_message_2].as_slice(), ] .concat() ); assert_eq!(body2["input"], expected_body2); } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn send_user_turn_with_no_changes_does_not_send_environment_context() { + use pretty_assertions::assert_eq; + + let server = MockServer::start().await; + + let sse = sse_completed("resp"); + let template = ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(sse, "text/event-stream"); + + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with(template) + .expect(2) + .mount(&server) + .await; + + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + + let cwd = TempDir::new().unwrap(); + let codex_home = TempDir::new().unwrap(); + let mut config = load_default_config_for_test(&codex_home); + config.cwd = cwd.path().to_path_buf(); + config.model_provider = model_provider; + config.user_instructions = Some("be consistent and helpful".to_string()); + + let default_cwd = config.cwd.clone(); + let default_approval_policy = config.approval_policy; + let default_sandbox_policy = config.sandbox_policy.clone(); + let default_model = config.model.clone(); + let default_effort = config.model_reasoning_effort; + let default_summary = config.model_reasoning_summary; + + let conversation_manager = + ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key")); + let codex = conversation_manager + .new_conversation(config) + .await + .expect("create new conversation") + .conversation; + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "hello 1".into(), + }], + cwd: default_cwd.clone(), + approval_policy: default_approval_policy, + sandbox_policy: default_sandbox_policy.clone(), + model: default_model.clone(), + effort: default_effort, + summary: default_summary, + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "hello 2".into(), + }], + cwd: default_cwd.clone(), + approval_policy: default_approval_policy, + sandbox_policy: default_sandbox_policy.clone(), + model: default_model.clone(), + effort: default_effort, + summary: default_summary, + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2, "expected two POST requests"); + + let body1 = requests[0].body_json::().unwrap(); + let body2 = requests[1].body_json::().unwrap(); + + let shell = default_user_shell().await; + let expected_ui_text = + "\n\nbe consistent and helpful\n\n"; + let expected_ui_msg = text_user_input(expected_ui_text.to_string()); + + let expected_env_msg_1 = text_user_input(default_env_context_str( + &cwd.path().to_string_lossy(), + &shell, + )); + let expected_user_message_1 = text_user_input("hello 1".to_string()); + + let expected_input_1 = serde_json::Value::Array(vec![ + expected_ui_msg.clone(), + expected_env_msg_1.clone(), + expected_user_message_1.clone(), + ]); + assert_eq!(body1["input"], expected_input_1); + + let expected_user_message_2 = text_user_input("hello 2".to_string()); + let expected_input_2 = serde_json::Value::Array(vec![ + expected_ui_msg, + expected_env_msg_1, + expected_user_message_1, + expected_user_message_2, + ]); + assert_eq!(body2["input"], expected_input_2); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn send_user_turn_with_changes_sends_environment_context() { + use pretty_assertions::assert_eq; + + let server = MockServer::start().await; + + let sse = sse_completed("resp"); + let template = ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(sse, "text/event-stream"); + + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with(template) + .expect(2) + .mount(&server) + .await; + + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + + let cwd = TempDir::new().unwrap(); + let codex_home = TempDir::new().unwrap(); + let mut config = load_default_config_for_test(&codex_home); + config.cwd = cwd.path().to_path_buf(); + config.model_provider = model_provider; + config.user_instructions = Some("be consistent and helpful".to_string()); + + let default_cwd = config.cwd.clone(); + let default_approval_policy = config.approval_policy; + let default_sandbox_policy = config.sandbox_policy.clone(); + let default_model = config.model.clone(); + let default_effort = config.model_reasoning_effort; + let default_summary = config.model_reasoning_summary; + + let conversation_manager = + ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key")); + let codex = conversation_manager + .new_conversation(config.clone()) + .await + .expect("create new conversation") + .conversation; + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "hello 1".into(), + }], + cwd: default_cwd.clone(), + approval_policy: default_approval_policy, + sandbox_policy: default_sandbox_policy.clone(), + model: default_model, + effort: default_effort, + summary: default_summary, + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "hello 2".into(), + }], + cwd: default_cwd.clone(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: "o3".to_string(), + effort: Some(ReasoningEffort::High), + summary: ReasoningSummary::Detailed, + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2, "expected two POST requests"); + + let body1 = requests[0].body_json::().unwrap(); + let body2 = requests[1].body_json::().unwrap(); + + let shell = default_user_shell().await; + let expected_ui_text = + "\n\nbe consistent and helpful\n\n"; + let expected_ui_msg = serde_json::json!({ + "type": "message", + "role": "user", + "content": [ { "type": "input_text", "text": expected_ui_text } ] + }); + let expected_env_text_1 = default_env_context_str(&default_cwd.to_string_lossy(), &shell); + let expected_env_msg_1 = text_user_input(expected_env_text_1); + let expected_user_message_1 = text_user_input("hello 1".to_string()); + let expected_input_1 = serde_json::Value::Array(vec![ + expected_ui_msg.clone(), + expected_env_msg_1.clone(), + expected_user_message_1.clone(), + ]); + assert_eq!(body1["input"], expected_input_1); + + let expected_env_msg_2 = text_user_input(format!( + r#" + {} + never + danger-full-access + enabled +"#, + default_cwd.to_string_lossy() + )); + let expected_user_message_2 = text_user_input("hello 2".to_string()); + let expected_input_2 = serde_json::Value::Array(vec![ + expected_ui_msg, + expected_env_msg_1, + expected_user_message_1, + expected_env_msg_2, + expected_user_message_2, + ]); + assert_eq!(body2["input"], expected_input_2); +} diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index f65913a2ca..a20807e4ec 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -1,9 +1,14 @@ use codex_core::CodexAuth; use codex_core::CodexConversation; +use codex_core::ContentItem; use codex_core::ConversationManager; use codex_core::ModelProviderInfo; +use codex_core::REVIEW_PROMPT; +use codex_core::ResponseItem; use codex_core::built_in_model_providers; use codex_core::config::Config; +use codex_core::protocol::ConversationPathResponseEvent; +use codex_core::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG; use codex_core::protocol::EventMsg; use codex_core::protocol::ExitedReviewModeEvent; use codex_core::protocol::InputItem; @@ -13,6 +18,8 @@ use codex_core::protocol::ReviewFinding; use codex_core::protocol::ReviewLineRange; use codex_core::protocol::ReviewOutputEvent; use codex_core::protocol::ReviewRequest; +use codex_core::protocol::RolloutItem; +use codex_core::protocol::RolloutLine; use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use core_test_support::load_default_config_for_test; use core_test_support::load_sse_fixture_with_id_from_str; @@ -115,6 +122,46 @@ async fn review_op_emits_lifecycle_and_review_output() { assert_eq!(expected, review); let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + // Also verify that a user message with the header and a formatted finding + // was recorded back in the parent session's rollout. + codex.submit(Op::GetPath).await.unwrap(); + let history_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ConversationPath(_))).await; + let path = match history_event { + EventMsg::ConversationPath(ConversationPathResponseEvent { path, .. }) => path, + other => panic!("expected ConversationPath event, got {other:?}"), + }; + let text = std::fs::read_to_string(&path).expect("read rollout file"); + + let mut saw_header = false; + let mut saw_finding_line = false; + for line in text.lines() { + if line.trim().is_empty() { + continue; + } + let v: serde_json::Value = serde_json::from_str(line).expect("jsonl line"); + let rl: RolloutLine = serde_json::from_value(v).expect("rollout line"); + if let RolloutItem::ResponseItem(ResponseItem::Message { role, content, .. }) = rl.item + && role == "user" + { + for c in content { + if let ContentItem::InputText { text } = c { + if text.contains("full review output from reviewer model") { + saw_header = true; + } + if text.contains("- Prefer Stylize helpers — /tmp/file.rs:10-20") { + saw_finding_line = true; + } + } + } + } + } + assert!(saw_header, "user header missing from rollout"); + assert!( + saw_finding_line, + "formatted finding line missing from rollout" + ); + server.verify().await; } @@ -419,17 +466,73 @@ async fn review_input_isolated_from_parent_history() { .await; let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - // Assert the request `input` contains only the single review user message. + // Assert the request `input` contains the environment context followed by the review prompt. let request = &server.received_requests().await.unwrap()[0]; let body = request.body_json::().unwrap(); - let expected_input = serde_json::json!([ - { - "type": "message", - "role": "user", - "content": [{"type": "input_text", "text": review_prompt}] + let input = body["input"].as_array().expect("input array"); + assert_eq!( + input.len(), + 2, + "expected environment context and review prompt" + ); + + let env_msg = &input[0]; + assert_eq!(env_msg["type"].as_str().unwrap(), "message"); + assert_eq!(env_msg["role"].as_str().unwrap(), "user"); + let env_text = env_msg["content"][0]["text"].as_str().expect("env text"); + assert!( + env_text.starts_with(ENVIRONMENT_CONTEXT_OPEN_TAG), + "environment context must be the first item" + ); + assert!( + env_text.contains(""), + "environment context should include cwd" + ); + + let review_msg = &input[1]; + assert_eq!(review_msg["type"].as_str().unwrap(), "message"); + assert_eq!(review_msg["role"].as_str().unwrap(), "user"); + assert_eq!( + review_msg["content"][0]["text"].as_str().unwrap(), + format!("{REVIEW_PROMPT}\n\n---\n\nNow, here's your task: Please review only this",) + ); + + // Also verify that a user interruption note was recorded in the rollout. + codex.submit(Op::GetPath).await.unwrap(); + let history_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ConversationPath(_))).await; + let path = match history_event { + EventMsg::ConversationPath(ConversationPathResponseEvent { path, .. }) => path, + other => panic!("expected ConversationPath event, got {other:?}"), + }; + let text = std::fs::read_to_string(&path).expect("read rollout file"); + let mut saw_interruption_message = false; + for line in text.lines() { + if line.trim().is_empty() { + continue; } - ]); - assert_eq!(body["input"], expected_input); + let v: serde_json::Value = serde_json::from_str(line).expect("jsonl line"); + let rl: RolloutLine = serde_json::from_value(v).expect("rollout line"); + if let RolloutItem::ResponseItem(ResponseItem::Message { role, content, .. }) = rl.item + && role == "user" + { + for c in content { + if let ContentItem::InputText { text } = c + && text.contains("User initiated a review task, but was interrupted.") + { + saw_interruption_message = true; + break; + } + } + } + if saw_interruption_message { + break; + } + } + assert!( + saw_interruption_message, + "expected user interruption message in rollout" + ); server.verify().await; } diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 2138d54098..bf11ff571d 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -142,14 +142,16 @@ impl ChatComposer { .desired_height(width.saturating_sub(LIVE_PREFIX_COLS)) + match &self.active_popup { ActivePopup::None => FOOTER_HEIGHT_WITH_HINT, - ActivePopup::Command(c) => c.calculate_required_height(), + ActivePopup::Command(c) => c.calculate_required_height(width), ActivePopup::File(c) => c.calculate_required_height(), } } pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { let popup_constraint = match &self.active_popup { - ActivePopup::Command(popup) => Constraint::Max(popup.calculate_required_height()), + ActivePopup::Command(popup) => { + Constraint::Max(popup.calculate_required_height(area.width)) + } ActivePopup::File(popup) => Constraint::Max(popup.calculate_required_height()), ActivePopup::None => Constraint::Max(FOOTER_HEIGHT_WITH_HINT), }; @@ -1232,7 +1234,10 @@ impl ChatComposer { impl WidgetRef for ChatComposer { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let (popup_constraint, hint_spacing) = match &self.active_popup { - ActivePopup::Command(popup) => (Constraint::Max(popup.calculate_required_height()), 0), + ActivePopup::Command(popup) => ( + Constraint::Max(popup.calculate_required_height(area.width)), + 0, + ), ActivePopup::File(popup) => (Constraint::Max(popup.calculate_required_height()), 0), ActivePopup::None => ( Constraint::Length(FOOTER_HEIGHT_WITH_HINT), diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index a5961e3188..8de266f819 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -92,10 +92,35 @@ impl CommandPopup { .ensure_visible(matches_len, MAX_POPUP_ROWS.min(matches_len)); } - /// Determine the preferred height of the popup. This is the number of - /// rows required to show at most MAX_POPUP_ROWS commands. - pub(crate) fn calculate_required_height(&self) -> u16 { - self.filtered_items().len().clamp(1, MAX_POPUP_ROWS) as u16 + /// Determine the preferred height of the popup for a given width. + /// Accounts for wrapped descriptions so that long tooltips don't overflow. + pub(crate) fn calculate_required_height(&self, width: u16) -> u16 { + use super::selection_popup_common::GenericDisplayRow; + use super::selection_popup_common::measure_rows_height; + let matches = self.filtered(); + let rows_all: Vec = if matches.is_empty() { + Vec::new() + } else { + matches + .into_iter() + .map(|(item, indices, _)| match item { + CommandItem::Builtin(cmd) => GenericDisplayRow { + name: format!("/{}", cmd.command()), + match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()), + is_current: false, + description: Some(cmd.description().to_string()), + }, + CommandItem::UserPrompt(i) => GenericDisplayRow { + name: format!("/{}", self.prompts[i].name), + match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()), + is_current: false, + description: Some("send saved prompt".to_string()), + }, + }) + .collect() + }; + + measure_rows_height(&rows_all, &self.state, MAX_POPUP_ROWS, width) } /// Compute fuzzy-filtered matches over built-in commands and user prompts, diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 7be91db202..5d5dbf0f33 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -17,6 +17,7 @@ use super::bottom_pane_view::BottomPaneView; use super::popup_consts::MAX_POPUP_ROWS; use super::scroll_state::ScrollState; use super::selection_popup_common::GenericDisplayRow; +use super::selection_popup_common::measure_rows_height; use super::selection_popup_common::render_rows; /// One selectable item in the generic selection list. @@ -135,11 +136,36 @@ impl BottomPaneView for ListSelectionView { CancellationEvent::Handled } - fn desired_height(&self, _width: u16) -> u16 { - let rows = (self.items.len()).clamp(1, MAX_POPUP_ROWS); + fn desired_height(&self, width: u16) -> u16 { + // Measure wrapped height for up to MAX_POPUP_ROWS items at the given width. + // Build the same display rows used by the renderer so wrapping math matches. + let rows: Vec = self + .items + .iter() + .enumerate() + .map(|(i, it)| { + let is_selected = self.state.selected_idx == Some(i); + let prefix = if is_selected { '>' } else { ' ' }; + let name_with_marker = if it.is_current { + format!("{} (current)", it.name) + } else { + it.name.clone() + }; + let display_name = format!("{} {}. {}", prefix, i + 1, name_with_marker); + GenericDisplayRow { + name: display_name, + match_indices: None, + is_current: it.is_current, + description: it.description.clone(), + } + }) + .collect(); + + let rows_height = measure_rows_height(&rows, &self.state, MAX_POPUP_ROWS, width); + // +1 for the title row, +1 for a spacer line beneath the header, - // +1 for optional subtitle, +1 for optional footer - let mut height = rows as u16 + 2; + // +1 for optional subtitle, +1 for optional footer (2 lines incl. spacing) + let mut height = rows_height + 2; if self.subtitle.is_some() { // +1 for subtitle (the spacer is accounted for above) height = height.saturating_add(1); diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 3852d10bc1..61a26f9341 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -1,6 +1,7 @@ use ratatui::buffer::Buffer; use ratatui::layout::Rect; -use ratatui::prelude::Constraint; +// Note: Table-based layout previously used Constraint; the manual renderer +// below no longer requires it. use ratatui::style::Color; use ratatui::style::Modifier; use ratatui::style::Style; @@ -10,9 +11,7 @@ use ratatui::text::Span; use ratatui::widgets::Block; use ratatui::widgets::BorderType; use ratatui::widgets::Borders; -use ratatui::widgets::Cell; -use ratatui::widgets::Row; -use ratatui::widgets::Table; +use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use super::scroll_state::ScrollState; @@ -27,6 +26,61 @@ pub(crate) struct GenericDisplayRow { impl GenericDisplayRow {} +/// Compute a shared description-column start based on the widest visible name +/// plus two spaces of padding. Ensures at least one column is left for the +/// description. +fn compute_desc_col( + rows_all: &[GenericDisplayRow], + start_idx: usize, + visible_items: usize, + content_width: u16, +) -> usize { + let visible_range = start_idx..(start_idx + visible_items); + let max_name_width = rows_all + .iter() + .enumerate() + .filter(|(i, _)| visible_range.contains(i)) + .map(|(_, r)| Line::from(r.name.clone()).width()) + .max() + .unwrap_or(0); + let mut desc_col = max_name_width.saturating_add(2); + if (desc_col as u16) >= content_width { + desc_col = content_width.saturating_sub(1) as usize; + } + desc_col +} + +/// Build the full display line for a row with the description padded to start +/// at `desc_col`. Applies fuzzy-match bolding when indices are present and +/// dims the description. +fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { + let mut name_spans: Vec = Vec::with_capacity(row.name.len()); + if let Some(idxs) = row.match_indices.as_ref() { + let mut idx_iter = idxs.iter().peekable(); + for (char_idx, ch) in row.name.chars().enumerate() { + if idx_iter.peek().is_some_and(|next| **next == char_idx) { + idx_iter.next(); + name_spans.push(ch.to_string().bold()); + } else { + name_spans.push(ch.to_string().into()); + } + } + } else { + name_spans.push(row.name.clone().into()); + } + + let this_name_width = Line::from(name_spans.clone()).width(); + let mut full_spans: Vec = name_spans; + if let Some(desc) = row.description.as_ref() { + let gap = desc_col.saturating_sub(this_name_width); + if gap > 0 { + full_spans.push(" ".repeat(gap).into()); + } + full_spans.push(desc.clone().dim()); + } + Line::from(full_spans) +} + /// Render a list of rows using the provided ScrollState, with shared styling /// and behavior for selection popups. pub(crate) fn render_rows( @@ -38,84 +92,168 @@ pub(crate) fn render_rows( _dim_non_selected: bool, empty_message: &str, ) { - let mut rows: Vec = Vec::new(); + // Always draw a dim left border to match other popups. + let block = Block::default() + .borders(Borders::LEFT) + .border_type(BorderType::QuadrantOutside) + .border_style(Style::default().add_modifier(Modifier::DIM)); + block.render(area, buf); + + // Content renders to the right of the border. + let content_area = Rect { + x: area.x.saturating_add(1), + y: area.y, + width: area.width.saturating_sub(1), + height: area.height, + }; + if rows_all.is_empty() { - rows.push(Row::new(vec![Cell::from(Line::from( - empty_message.dim().italic(), - ))])); - } else { - let max_rows_from_area = area.height as usize; - let visible_rows = max_results - .min(rows_all.len()) - .min(max_rows_from_area.max(1)); - - // Compute starting index based on scroll state and selection. - let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); - if let Some(sel) = state.selected_idx { - if sel < start_idx { - start_idx = sel; - } else if visible_rows > 0 { - let bottom = start_idx + visible_rows - 1; - if sel > bottom { - start_idx = sel + 1 - visible_rows; - } - } + if content_area.height > 0 { + let para = Paragraph::new(Line::from(empty_message.dim().italic())); + para.render( + Rect { + x: content_area.x, + y: content_area.y, + width: content_area.width, + height: 1, + }, + buf, + ); } + return; + } - for (i, row) in rows_all - .iter() - .enumerate() - .skip(start_idx) - .take(visible_rows) - { - let GenericDisplayRow { - name, - match_indices, - is_current: _is_current, - description, - } = row; + // Determine which logical rows (items) are visible given the selection and + // the max_results clamp. Scrolling is still item-based for simplicity. + let max_rows_from_area = content_area.height as usize; + let visible_items = max_results + .min(rows_all.len()) + .min(max_rows_from_area.max(1)); - // Highlight fuzzy indices when present. - let mut spans: Vec = Vec::with_capacity(name.len()); - if let Some(idxs) = match_indices.as_ref() { - let mut idx_iter = idxs.iter().peekable(); - for (char_idx, ch) in name.chars().enumerate() { - if idx_iter.peek().is_some_and(|next| **next == char_idx) { - idx_iter.next(); - spans.push(ch.to_string().bold()); - } else { - spans.push(ch.to_string().into()); - } - } - } else { - spans.push(name.clone().into()); + let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); + if let Some(sel) = state.selected_idx { + if sel < start_idx { + start_idx = sel; + } else if visible_items > 0 { + let bottom = start_idx + visible_items - 1; + if sel > bottom { + start_idx = sel + 1 - visible_items; } - - if let Some(desc) = description.as_ref() { - spans.push(" ".into()); - spans.push(desc.clone().dim()); - } - - let mut cell = Cell::from(Line::from(spans)); - if Some(i) == state.selected_idx { - cell = cell.style( - Style::default() - .fg(Color::Cyan) - .add_modifier(Modifier::BOLD), - ); - } - rows.push(Row::new(vec![cell])); } } - let table = Table::new(rows, vec![Constraint::Percentage(100)]) - .block( - Block::default() - .borders(Borders::LEFT) - .border_type(BorderType::QuadrantOutside) - .border_style(Style::default().add_modifier(Modifier::DIM)), - ) - .widths([Constraint::Percentage(100)]); + let desc_col = compute_desc_col(rows_all, start_idx, visible_items, content_area.width); - table.render(area, buf); + // Render items, wrapping descriptions and aligning wrapped lines under the + // shared description column. Stop when we run out of vertical space. + let mut cur_y = content_area.y; + for (i, row) in rows_all + .iter() + .enumerate() + .skip(start_idx) + .take(visible_items) + { + if cur_y >= content_area.y + content_area.height { + break; + } + + let GenericDisplayRow { + name, + match_indices, + is_current: _is_current, + description, + } = row; + + let full_line = build_full_line( + &GenericDisplayRow { + name: name.clone(), + match_indices: match_indices.clone(), + is_current: *_is_current, + description: description.clone(), + }, + desc_col, + ); + + // Wrap with subsequent indent aligned to the description column. + use crate::wrapping::RtOptions; + use crate::wrapping::word_wrap_line; + let options = RtOptions::new(content_area.width as usize) + .initial_indent(Line::from("")) + .subsequent_indent(Line::from(" ".repeat(desc_col))); + let wrapped = word_wrap_line(&full_line, options); + + // Render the wrapped lines. + for mut line in wrapped { + if cur_y >= content_area.y + content_area.height { + break; + } + if Some(i) == state.selected_idx { + // Match previous behavior: cyan + bold for the selected row. + line.style = Style::default() + .fg(Color::Cyan) + .add_modifier(Modifier::BOLD); + } + let para = Paragraph::new(line); + para.render( + Rect { + x: content_area.x, + y: cur_y, + width: content_area.width, + height: 1, + }, + buf, + ); + cur_y = cur_y.saturating_add(1); + } + } +} + +/// Compute the number of terminal rows required to render up to `max_results` +/// items from `rows_all` given the current scroll/selection state and the +/// available `width`. Accounts for description wrapping and alignment so the +/// caller can allocate sufficient vertical space. +pub(crate) fn measure_rows_height( + rows_all: &[GenericDisplayRow], + state: &ScrollState, + max_results: usize, + width: u16, +) -> u16 { + if rows_all.is_empty() { + return 1; // placeholder "no matches" line + } + + let content_width = width.saturating_sub(1).max(1); + + let visible_items = max_results.min(rows_all.len()); + let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); + if let Some(sel) = state.selected_idx { + if sel < start_idx { + start_idx = sel; + } else if visible_items > 0 { + let bottom = start_idx + visible_items - 1; + if sel > bottom { + start_idx = sel + 1 - visible_items; + } + } + } + + let desc_col = compute_desc_col(rows_all, start_idx, visible_items, content_width); + + use crate::wrapping::RtOptions; + use crate::wrapping::word_wrap_line; + let mut total: u16 = 0; + for row in rows_all + .iter() + .enumerate() + .skip(start_idx) + .take(visible_items) + .map(|(_, r)| r) + { + let full_line = build_full_line(row, desc_col); + let opts = RtOptions::new(content_width as usize) + .initial_indent(Line::from("")) + .subsequent_indent(Line::from(" ".repeat(desc_col))); + total = total.saturating_add(word_wrap_line(&full_line, opts).len() as u16); + } + total.max(1) } diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap index b626f80ca1..f908fb6144 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap @@ -4,5 +4,5 @@ expression: terminal.backend() --- "▌ /mo " "▌ " -"▌/model choose what model and reasoning effort to use " +"▌/model choose what model and reasoning effort to use " "▌/mention mention a file " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap index aab7e7c9b6..65606ed7d0 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap @@ -6,6 +6,6 @@ expression: render_lines(&view) ▌ Switch between Codex approval presets ▌ ▌> 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit files +▌ 2. Full Access Codex can edit files Press Enter to confirm or Esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap index 77552f2b93..b42a5f8c6b 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap @@ -5,6 +5,6 @@ expression: render_lines(&view) ▌ Select Approval Mode ▌ ▌> 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit files +▌ 2. Full Access Codex can edit files Press Enter to confirm or Esc to go back diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c03e7d3bc5..fb91d75456 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -226,12 +226,11 @@ impl ChatWidget { // At the end of a reasoning block, record transcript-only content. self.full_reasoning_buffer.push_str(&self.reasoning_buffer); if !self.full_reasoning_buffer.is_empty() { - for cell in history_cell::new_reasoning_summary_block( + let cell = history_cell::new_reasoning_summary_block( self.full_reasoning_buffer.clone(), &self.config, - ) { - self.add_boxed_history(cell); - } + ); + self.add_boxed_history(cell); } self.reasoning_buffer.clear(); self.full_reasoning_buffer.clear(); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index d6add6d328..9cdd29dd02 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -121,6 +121,45 @@ impl HistoryCell for UserHistoryCell { } } +#[derive(Debug)] +pub(crate) struct ReasoningSummaryCell { + _header: Vec>, + content: Vec>, +} + +impl ReasoningSummaryCell { + pub(crate) fn new(header: Vec>, content: Vec>) -> Self { + Self { + _header: header, + content, + } + } +} + +impl HistoryCell for ReasoningSummaryCell { + fn display_lines(&self, width: u16) -> Vec> { + let summary_lines = self + .content + .iter() + .map(|l| l.clone().dim().italic()) + .collect::>(); + + word_wrap_lines( + &summary_lines, + RtOptions::new(width as usize) + .initial_indent("• ".into()) + .subsequent_indent(" ".into()), + ) + } + + fn transcript_lines(&self) -> Vec> { + let mut out: Vec> = Vec::new(); + out.push("thinking".magenta().bold().into()); + out.extend(self.content.clone()); + out + } +} + #[derive(Debug)] pub(crate) struct AgentMessageCell { lines: Vec>, @@ -1417,7 +1456,7 @@ pub(crate) fn new_reasoning_block( pub(crate) fn new_reasoning_summary_block( full_reasoning_buffer: String, config: &Config, -) -> Vec> { +) -> Box { if config.model_family.reasoning_summary_format == ReasoningSummaryFormat::Experimental { // Experimental format is following: // ** header ** @@ -1434,27 +1473,19 @@ pub(crate) fn new_reasoning_summary_block( // then we don't have a summary to inject into history if after_close_idx < full_reasoning_buffer.len() { let header_buffer = full_reasoning_buffer[..after_close_idx].to_string(); - let summary_buffer = full_reasoning_buffer[after_close_idx..].to_string(); - - let mut header_lines: Vec> = Vec::new(); - header_lines.push(Line::from("Thinking".magenta().italic())); + let mut header_lines = Vec::new(); append_markdown(&header_buffer, &mut header_lines, config); - let mut summary_lines: Vec> = Vec::new(); - summary_lines.push(Line::from("Thinking".magenta().bold())); + let summary_buffer = full_reasoning_buffer[after_close_idx..].to_string(); + let mut summary_lines = Vec::new(); append_markdown(&summary_buffer, &mut summary_lines, config); - return vec![ - Box::new(TranscriptOnlyHistoryCell { - lines: header_lines, - }), - Box::new(AgentMessageCell::new(summary_lines, true)), - ]; + return Box::new(ReasoningSummaryCell::new(header_lines, summary_lines)); } } } } - vec![Box::new(new_reasoning_block(full_reasoning_buffer, config))] + Box::new(new_reasoning_block(full_reasoning_buffer, config)) } struct OutputLinesParams { @@ -1558,6 +1589,7 @@ mod tests { use codex_core::config::ConfigOverrides; use codex_core::config::ConfigToml; use dirs::home_dir; + use pretty_assertions::assert_eq; fn test_config() -> Config { Config::load_from_base_config_with_overrides( @@ -2076,17 +2108,35 @@ mod tests { let rendered = render_lines(&lines).join("\n"); insta::assert_snapshot!(rendered); } + #[test] + fn reasoning_summary_block() { + let mut config = test_config(); + config.model_family.reasoning_summary_format = ReasoningSummaryFormat::Experimental; + + let cell = new_reasoning_summary_block( + "**High level reasoning**\n\nDetailed reasoning goes here.".to_string(), + &config, + ); + + let rendered_display = render_lines(&cell.display_lines(80)); + assert_eq!(rendered_display, vec!["• Detailed reasoning goes here."]); + + let rendered_transcript = render_transcript(cell.as_ref()); + assert_eq!( + rendered_transcript, + vec!["thinking", "Detailed reasoning goes here."] + ); + } #[test] fn reasoning_summary_block_returns_reasoning_cell_when_feature_disabled() { let mut config = test_config(); config.model_family.reasoning_summary_format = ReasoningSummaryFormat::Experimental; - let cells = + let cell = new_reasoning_summary_block("Detailed reasoning goes here.".to_string(), &config); - assert_eq!(cells.len(), 1); - let rendered = render_transcript(cells[0].as_ref()); + let rendered = render_transcript(cell.as_ref()); assert_eq!(rendered, vec!["thinking", "Detailed reasoning goes here."]); } @@ -2095,13 +2145,12 @@ mod tests { let mut config = test_config(); config.model_family.reasoning_summary_format = ReasoningSummaryFormat::Experimental; - let cells = new_reasoning_summary_block( + let cell = new_reasoning_summary_block( "**High level reasoning without closing".to_string(), &config, ); - assert_eq!(cells.len(), 1); - let rendered = render_transcript(cells[0].as_ref()); + let rendered = render_transcript(cell.as_ref()); assert_eq!( rendered, vec!["thinking", "**High level reasoning without closing"] @@ -2113,25 +2162,23 @@ mod tests { let mut config = test_config(); config.model_family.reasoning_summary_format = ReasoningSummaryFormat::Experimental; - let cells = new_reasoning_summary_block( + let cell = new_reasoning_summary_block( "**High level reasoning without closing**".to_string(), &config, ); - assert_eq!(cells.len(), 1); - let rendered = render_transcript(cells[0].as_ref()); + let rendered = render_transcript(cell.as_ref()); assert_eq!( rendered, vec!["thinking", "High level reasoning without closing"] ); - let cells = new_reasoning_summary_block( + let cell = new_reasoning_summary_block( "**High level reasoning without closing**\n\n ".to_string(), &config, ); - assert_eq!(cells.len(), 1); - let rendered = render_transcript(cells[0].as_ref()); + let rendered = render_transcript(cell.as_ref()); assert_eq!( rendered, vec!["thinking", "High level reasoning without closing"] @@ -2143,21 +2190,18 @@ mod tests { let mut config = test_config(); config.model_family.reasoning_summary_format = ReasoningSummaryFormat::Experimental; - let cells = new_reasoning_summary_block( + let cell = new_reasoning_summary_block( "**High level plan**\n\nWe should fix the bug next.".to_string(), &config, ); - assert_eq!(cells.len(), 2); - - let header_lines = render_transcript(cells[0].as_ref()); - assert_eq!(header_lines, vec!["Thinking", "High level plan"]); - - let summary_lines = render_transcript(cells[1].as_ref()); + let rendered_display = render_lines(&cell.display_lines(80)); + assert_eq!(rendered_display, vec!["• We should fix the bug next."]); + let rendered_transcript = render_transcript(cell.as_ref()); assert_eq!( - summary_lines, - vec!["codex", "Thinking", "We should fix the bug next."] - ) + rendered_transcript, + vec!["thinking", "We should fix the bug next."] + ); } } diff --git a/codex-rs/tui/src/markdown_stream.rs b/codex-rs/tui/src/markdown_stream.rs index 7245afb102..12b4308210 100644 --- a/codex-rs/tui/src/markdown_stream.rs +++ b/codex-rs/tui/src/markdown_stream.rs @@ -333,11 +333,11 @@ mod tests { ); for (i, l) in non_blank.iter().enumerate() { assert_eq!( - l.style.fg, + l.spans[0].style.fg, Some(Color::Green), "wrapped line {} should preserve green style, got {:?}", i, - l.style.fg + l.spans[0].style.fg ); } } diff --git a/codex-rs/tui/src/wrapping.rs b/codex-rs/tui/src/wrapping.rs index bb309784b4..c97f0e3c6c 100644 --- a/codex-rs/tui/src/wrapping.rs +++ b/codex-rs/tui/src/wrapping.rs @@ -187,7 +187,6 @@ where // Build first wrapped line with initial indent. let mut first_line = rt_opts.initial_indent.clone(); - first_line.style = first_line.style.patch(line.style); { let sliced = slice_line_spans(line, &span_bounds, first_line_range); let mut spans = first_line.spans; @@ -216,7 +215,6 @@ where continue; } let mut subsequent_line = rt_opts.subsequent_indent.clone(); - subsequent_line.style = subsequent_line.style.patch(line.style); let offset_range = (r.start + base)..(r.end + base); let sliced = slice_line_spans(line, &span_bounds, &offset_range); let mut spans = subsequent_line.spans; diff --git a/docs/config.md b/docs/config.md index 01b3efe0a1..4f287b824e 100644 --- a/docs/config.md +++ b/docs/config.md @@ -69,17 +69,6 @@ base_url = "https://api.mistral.ai/v1" env_key = "MISTRAL_API_KEY" ``` -Note that Azure requires `api-version` to be passed as a query parameter, so be sure to specify it as part of `query_params` when defining the Azure provider: - -```toml -[model_providers.azure] -name = "Azure" -# Make sure you set the appropriate subdomain for this URL. -base_url = "https://YOUR_PROJECT_NAME.openai.azure.com/openai" -env_key = "AZURE_OPENAI_API_KEY" # Or "OPENAI_API_KEY", whichever you use. -query_params = { api-version = "2025-04-01-preview" } -``` - It is also possible to configure a provider to include extra HTTP headers with a request. These can be hardcoded values (`http_headers`) or values read from environment variables (`env_http_headers`): ```toml @@ -96,6 +85,22 @@ http_headers = { "X-Example-Header" = "example-value" } env_http_headers = { "X-Example-Features" = "EXAMPLE_FEATURES" } ``` +### Azure model provider example + +Note that Azure requires `api-version` to be passed as a query parameter, so be sure to specify it as part of `query_params` when defining the Azure provider: + +```toml +[model_providers.azure] +name = "Azure" +# Make sure you set the appropriate subdomain for this URL. +base_url = "https://YOUR_PROJECT_NAME.openai.azure.com/openai" +env_key = "AZURE_OPENAI_API_KEY" # Or "OPENAI_API_KEY", whichever you use. +query_params = { api-version = "2025-04-01-preview" } +wire_api = "responses" +``` + +Export your key before launching Codex: `export AZURE_OPENAI_API_KEY=…` + ### Per-provider network tuning The following optional settings control retry behaviour and streaming idle timeouts **per model provider**. They must be specified inside the corresponding `[model_providers.]` block in `config.toml`. (Older releases accepted top‑level keys; those are now ignored.) diff --git a/docs/release_management.md b/docs/release_management.md index ed12de6e4a..d2b3264554 100644 --- a/docs/release_management.md +++ b/docs/release_management.md @@ -30,14 +30,7 @@ When the workflow finishes, the GitHub Release is "done," but you still have to ## Publishing to npm -After the GitHub Release is done, you can publish to npm. Note the GitHub Release includes the appropriate artifact for npm (which is the output of `npm pack`), which should be named `codex-npm-VERSION.tgz`. To publish to npm, run: - -``` -VERSION=0.21.0 -./scripts/publish_to_npm.py "$VERSION" -``` - -Note that you must have permissions to publish to https://www.npmjs.com/package/@openai/codex for this to succeed. +The GitHub Action is responsible for publishing to npm. ## Publishing to Homebrew diff --git a/scripts/publish_to_npm.py b/scripts/publish_to_npm.py deleted file mode 100755 index f79843ffc8..0000000000 --- a/scripts/publish_to_npm.py +++ /dev/null @@ -1,118 +0,0 @@ -#!/usr/bin/env python3 - -""" -Download a release artifact for the npm package and publish it. - -Given a release version like `0.20.0`, this script: - - Downloads the `codex-npm-.tgz` asset from the GitHub release - tagged `rust-v` in the `openai/codex` repository using `gh`. - - Runs `npm publish` on the downloaded tarball to publish `@openai/codex`. - -Flags: - - `--dry-run` delegates to `npm publish --dry-run`. The artifact is still - downloaded so npm can inspect the archive contents without publishing. - -Requirements: - - GitHub CLI (`gh`) must be installed and authenticated to access the repo. - - npm must be logged in with an account authorized to publish - `@openai/codex`. This may trigger a browser for 2FA. -""" - -import argparse -import os -import subprocess -import sys -import tempfile -from pathlib import Path - - -def run_checked(cmd: list[str], cwd: Path | None = None) -> None: - """Run a subprocess command and raise if it fails.""" - proc = subprocess.run(cmd, cwd=str(cwd) if cwd else None) - proc.check_returncode() - - -def main() -> int: - parser = argparse.ArgumentParser( - description=( - "Download the npm release artifact for a given version and publish it." - ) - ) - parser.add_argument( - "version", - help="Release version to publish, e.g. 0.20.0 (without the 'v' prefix)", - ) - parser.add_argument( - "--dir", - type=Path, - help=( - "Optional directory to download the artifact into. Defaults to a temporary directory." - ), - ) - parser.add_argument( - "-n", - "--dry-run", - action="store_true", - help="Delegate to `npm publish --dry-run` (still downloads the artifact).", - ) - args = parser.parse_args() - - version: str = args.version.lstrip("v") - tag = f"rust-v{version}" - asset_name = f"codex-npm-{version}.tgz" - - download_dir_context_manager = ( - tempfile.TemporaryDirectory() if args.dir is None else None - ) - # Use provided dir if set, else the temporary one created above - download_dir: Path = args.dir if args.dir else Path(download_dir_context_manager.name) # type: ignore[arg-type] - download_dir.mkdir(parents=True, exist_ok=True) - - # 1) Download the artifact using gh - repo = "openai/codex" - gh_cmd = [ - "gh", - "release", - "download", - tag, - "--repo", - repo, - "--pattern", - asset_name, - "--dir", - str(download_dir), - ] - print(f"Downloading {asset_name} from {repo}@{tag} into {download_dir}...") - # Even in --dry-run we download so npm can inspect the tarball. - run_checked(gh_cmd) - - artifact_path = download_dir / asset_name - if not args.dry_run and not artifact_path.is_file(): - print( - f"Error: expected artifact not found after download: {artifact_path}", - file=sys.stderr, - ) - return 1 - - # 2) Publish to npm - npm_cmd = ["npm", "publish"] - if args.dry_run: - npm_cmd.append("--dry-run") - npm_cmd.append(str(artifact_path)) - - # Ensure CI is unset so npm can open a browser for 2FA if needed. - env = os.environ.copy() - if env.get("CI"): - env.pop("CI") - - print("Running:", " ".join(npm_cmd)) - proc = subprocess.run(npm_cmd, env=env) - proc.check_returncode() - - print("Publish complete.") - # Keep the temporary directory alive until here; it is cleaned up on exit - return 0 - - -if __name__ == "__main__": - sys.exit(main())